| Message ID | 20240903170752.3564538-2-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: intel: Get rid of ifdeffery leftovers | expand |
On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote: > Explicit ifdeffery is ugly and theoretically might be not synchronised > with the rest of functions that are assigned via pm_sleep_ptr() macro. > Replace ifdeffery by pm_sleep_ptr() macro to improve this. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/pinctrl/intel/pinctrl-intel.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c > index 7a790c437f68..bfe891522044 100644 > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -1482,7 +1482,6 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, > > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > { > -#ifdef CONFIG_PM_SLEEP > const struct intel_pinctrl_soc_data *soc = pctrl->soc; > struct intel_community_context *communities; > struct intel_pad_context *pads; > @@ -1497,7 +1496,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > if (!communities) > return -ENOMEM; > > - > for (i = 0; i < pctrl->ncommunities; i++) { > struct intel_community *community = &pctrl->communities[i]; > u32 *intmask, *hostown; > @@ -1519,7 +1517,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > > pctrl->context.pads = pads; > pctrl->context.communities = communities; > -#endif Can't we make this a stub when !PM_SLEEP? #ifdef CONFIG_PM_SLEEP static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) { ... } #else static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) { return 0; } #endif > > return 0; > } > @@ -1649,7 +1646,7 @@ int intel_pinctrl_probe(struct platform_device *pdev, > if (irq < 0) > return irq; > > - ret = intel_pinctrl_pm_init(pctrl); > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; Then this still looks like a function call and not like some weird conditional. > if (ret) > return ret; > > -- > 2.43.0.rc1.1336.g36b5255a03ac
On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote: > > Explicit ifdeffery is ugly and theoretically might be not synchronised > > with the rest of functions that are assigned via pm_sleep_ptr() macro. > > Replace ifdeffery by pm_sleep_ptr() macro to improve this. ... > Can't we make this a stub when !PM_SLEEP? > > #ifdef CONFIG_PM_SLEEP > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > { > ... > } > #else > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > { > return 0; > } > #endif There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery. ... > > - ret = intel_pinctrl_pm_init(pctrl); > > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; > > Then this still looks like a function call and not like some weird > conditional. I understand that, but the point is to make all PM callbacks use the same approach against kernel configuration. Current state of affairs is simple inconsistency, but it might, however quite unlikely, lead to desynchronization between two pm_sleep_ptr() and ifdeffery approaches. Approach that I have before this one (and I kinda agree that ternary here looks a bit weird) is to typedef the function and do something like pinctrl-intel.h: typedef alloc_fn; static inline int ctx_alloc(pctrl, alloc_fn) { if (alloc_fn) return alloc_fn(pctrl); return 0; } pinctrl-intel.c: ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init)) if (ret) return ret; Altogether it will be ~20+ LoCs in addition to the current codebase.
On Wed, Sep 4, 2024 at 10:47 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg > <mika.westerberg@linux.intel.com> wrote: > > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote: > > > Explicit ifdeffery is ugly and theoretically might be not synchronised > > > with the rest of functions that are assigned via pm_sleep_ptr() macro. > > > Replace ifdeffery by pm_sleep_ptr() macro to improve this. ... > > Can't we make this a stub when !PM_SLEEP? > > > > #ifdef CONFIG_PM_SLEEP > > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > > { > > ... > > } > > #else > > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > > { > > return 0; > > } > > #endif > > There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery. > > ... > > > > - ret = intel_pinctrl_pm_init(pctrl); > > > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; > > > > Then this still looks like a function call and not like some weird > > conditional. > > I understand that, but the point is to make all PM callbacks use the > same approach against kernel configuration. Current state of affairs > is simple inconsistency, but it might, however quite unlikely, lead to > desynchronization between two pm_sleep_ptr() and ifdeffery approaches. > > Approach that I have before this one (and I kinda agree that ternary > here looks a bit weird) is to typedef the function and do something > like > > pinctrl-intel.h: > typedef alloc_fn; Actually typedef is not needed as it may be embedded in the below inline as it's used only once. > static inline int ctx_alloc(pctrl, alloc_fn) > { > if (alloc_fn) > return alloc_fn(pctrl); > > return 0; > } > > pinctrl-intel.c: > > ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init)) > if (ret) > return ret; > > Altogether it will be ~20+ LoCs in addition to the current codebase.
Hi Andy, kernel test robot noticed the following build warnings: [auto build test WARNING on linusw-pinctrl/devel] [also build test WARNING on linusw-pinctrl/for-next linus/master v6.11-rc6 next-20240904] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/pinctrl-intel-Replace-ifdeffery-by-pm_sleep_ptr-macro/20240904-011041 base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel patch link: https://lore.kernel.org/r/20240903170752.3564538-2-andriy.shevchenko%40linux.intel.com patch subject: [PATCH v1 1/3] pinctrl: intel: Replace ifdeffery by pm_sleep_ptr() macro config: i386-buildonly-randconfig-002-20240904 (https://download.01.org/0day-ci/archive/20240904/202409041756.jHFGLs72-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409041756.jHFGLs72-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409041756.jHFGLs72-lkp@intel.com/ All warnings (new ones prefixed by >>): drivers/pinctrl/intel/pinctrl-intel.c: In function 'intel_pinctrl_probe': >> drivers/pinctrl/intel/pinctrl-intel.c:1600:51: warning: the address of 'intel_pinctrl_pm_init' will always evaluate as 'true' [-Waddress] 1600 | ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; | ^ vim +1600 drivers/pinctrl/intel/pinctrl-intel.c 1503 1504 int intel_pinctrl_probe(struct platform_device *pdev, 1505 const struct intel_pinctrl_soc_data *soc_data) 1506 { 1507 struct device *dev = &pdev->dev; 1508 struct intel_pinctrl *pctrl; 1509 int i, ret, irq; 1510 1511 pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL); 1512 if (!pctrl) 1513 return -ENOMEM; 1514 1515 pctrl->dev = dev; 1516 pctrl->soc = soc_data; 1517 raw_spin_lock_init(&pctrl->lock); 1518 1519 /* 1520 * Make a copy of the communities which we can use to hold pointers 1521 * to the registers. 1522 */ 1523 pctrl->ncommunities = pctrl->soc->ncommunities; 1524 pctrl->communities = devm_kcalloc(dev, pctrl->ncommunities, 1525 sizeof(*pctrl->communities), GFP_KERNEL); 1526 if (!pctrl->communities) 1527 return -ENOMEM; 1528 1529 for (i = 0; i < pctrl->ncommunities; i++) { 1530 struct intel_community *community = &pctrl->communities[i]; 1531 void __iomem *regs; 1532 u32 offset; 1533 u32 value; 1534 1535 *community = pctrl->soc->communities[i]; 1536 1537 regs = devm_platform_ioremap_resource(pdev, community->barno); 1538 if (IS_ERR(regs)) 1539 return PTR_ERR(regs); 1540 1541 /* 1542 * Determine community features based on the revision. 1543 * A value of all ones means the device is not present. 1544 */ 1545 value = readl(regs + REVID); 1546 if (value == ~0u) 1547 return -ENODEV; 1548 if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) { 1549 community->features |= PINCTRL_FEATURE_DEBOUNCE; 1550 community->features |= PINCTRL_FEATURE_1K_PD; 1551 } 1552 1553 /* Determine community features based on the capabilities */ 1554 offset = CAPLIST; 1555 do { 1556 value = readl(regs + offset); 1557 switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) { 1558 case CAPLIST_ID_GPIO_HW_INFO: 1559 community->features |= PINCTRL_FEATURE_GPIO_HW_INFO; 1560 break; 1561 case CAPLIST_ID_PWM: 1562 community->features |= PINCTRL_FEATURE_PWM; 1563 break; 1564 case CAPLIST_ID_BLINK: 1565 community->features |= PINCTRL_FEATURE_BLINK; 1566 break; 1567 case CAPLIST_ID_EXP: 1568 community->features |= PINCTRL_FEATURE_EXP; 1569 break; 1570 default: 1571 break; 1572 } 1573 offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT; 1574 } while (offset); 1575 1576 dev_dbg(dev, "Community%d features: %#08x\n", i, community->features); 1577 1578 /* Read offset of the pad configuration registers */ 1579 offset = readl(regs + PADBAR); 1580 1581 community->regs = regs; 1582 community->pad_regs = regs + offset; 1583 1584 if (community->gpps) 1585 ret = intel_pinctrl_add_padgroups_by_gpps(pctrl, community); 1586 else 1587 ret = intel_pinctrl_add_padgroups_by_size(pctrl, community); 1588 if (ret) 1589 return ret; 1590 1591 ret = intel_pinctrl_probe_pwm(pctrl, community); 1592 if (ret) 1593 return ret; 1594 } 1595 1596 irq = platform_get_irq(pdev, 0); 1597 if (irq < 0) 1598 return irq; 1599 > 1600 ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; 1601 if (ret) 1602 return ret; 1603 1604 pctrl->pctldesc = intel_pinctrl_desc; 1605 pctrl->pctldesc.name = dev_name(dev); 1606 pctrl->pctldesc.pins = pctrl->soc->pins; 1607 pctrl->pctldesc.npins = pctrl->soc->npins; 1608 1609 pctrl->pctldev = devm_pinctrl_register(dev, &pctrl->pctldesc, pctrl); 1610 if (IS_ERR(pctrl->pctldev)) { 1611 dev_err(dev, "failed to register pinctrl driver\n"); 1612 return PTR_ERR(pctrl->pctldev); 1613 } 1614 1615 ret = intel_gpio_probe(pctrl, irq); 1616 if (ret) 1617 return ret; 1618 1619 platform_set_drvdata(pdev, pctrl); 1620 1621 return 0; 1622 } 1623 EXPORT_SYMBOL_NS_GPL(intel_pinctrl_probe, PINCTRL_INTEL); 1624
On Wed, Sep 04, 2024 at 10:48:42AM +0300, Andy Shevchenko wrote: > On Wed, Sep 4, 2024 at 10:47 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg > > <mika.westerberg@linux.intel.com> wrote: > > > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote: > > > > Explicit ifdeffery is ugly and theoretically might be not synchronised > > > > with the rest of functions that are assigned via pm_sleep_ptr() macro. > > > > Replace ifdeffery by pm_sleep_ptr() macro to improve this. > > ... > > > > Can't we make this a stub when !PM_SLEEP? > > > > > > #ifdef CONFIG_PM_SLEEP > > > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > > > { > > > ... > > > } > > > #else > > > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) > > > { > > > return 0; > > > } > > > #endif > > > > There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery. > > > > ... > > > > > > - ret = intel_pinctrl_pm_init(pctrl); > > > > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; > > > > > > Then this still looks like a function call and not like some weird > > > conditional. > > > > I understand that, but the point is to make all PM callbacks use the > > same approach against kernel configuration. Current state of affairs > > is simple inconsistency, but it might, however quite unlikely, lead to > > desynchronization between two pm_sleep_ptr() and ifdeffery approaches. > > > > Approach that I have before this one (and I kinda agree that ternary > > here looks a bit weird) is to typedef the function and do something > > like > > > > pinctrl-intel.h: > > > typedef alloc_fn; > > Actually typedef is not needed as it may be embedded in the below > inline as it's used only once. > > > static inline int ctx_alloc(pctrl, alloc_fn) > > { > > if (alloc_fn) > > return alloc_fn(pctrl); > > > > return 0; > > } > > > > pinctrl-intel.c: > > > > ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init)) > > if (ret) > > return ret; I don't think this makes it any better :( We want the driver to be readable for anyone, not just for you. I prefer the stub and ifdeffery.
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 7a790c437f68..bfe891522044 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1482,7 +1482,6 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) { -#ifdef CONFIG_PM_SLEEP const struct intel_pinctrl_soc_data *soc = pctrl->soc; struct intel_community_context *communities; struct intel_pad_context *pads; @@ -1497,7 +1496,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) if (!communities) return -ENOMEM; - for (i = 0; i < pctrl->ncommunities; i++) { struct intel_community *community = &pctrl->communities[i]; u32 *intmask, *hostown; @@ -1519,7 +1517,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl) pctrl->context.pads = pads; pctrl->context.communities = communities; -#endif return 0; } @@ -1649,7 +1646,7 @@ int intel_pinctrl_probe(struct platform_device *pdev, if (irq < 0) return irq; - ret = intel_pinctrl_pm_init(pctrl); + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0; if (ret) return ret;
Explicit ifdeffery is ugly and theoretically might be not synchronised with the rest of functions that are assigned via pm_sleep_ptr() macro. Replace ifdeffery by pm_sleep_ptr() macro to improve this. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/pinctrl/intel/pinctrl-intel.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)