diff mbox series

[v1,1/3] pinctrl: intel: Replace ifdeffery by pm_sleep_ptr() macro

Message ID 20240903170752.3564538-2-andriy.shevchenko@linux.intel.com
State New
Headers show
Series pinctrl: intel: Get rid of ifdeffery leftovers | expand

Commit Message

Andy Shevchenko Sept. 3, 2024, 5:04 p.m. UTC
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(-)

Comments

Mika Westerberg Sept. 4, 2024, 5:05 a.m. UTC | #1
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
Andy Shevchenko Sept. 4, 2024, 7:47 a.m. UTC | #2
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.
Andy Shevchenko Sept. 4, 2024, 7:48 a.m. UTC | #3
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.
kernel test robot Sept. 4, 2024, 9:46 a.m. UTC | #4
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
Mika Westerberg Sept. 4, 2024, 11:18 a.m. UTC | #5
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 mbox series

Patch

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;