[PATCH v1 0/3] pinctrl: intel: Get rid of ifdeffery leftovers

[PATCH v1 0/3] pinctrl: intel: Get rid of ifdeffery leftovers

[Andy Shevchenko]

There are two benefits of this series:
1) no more ugly ifdeffery in the code;
2) the PM callbacks are all being synchronised via using the same macro,
i.e. pm_sleep_ptr() everywhere.

Andy Shevchenko (3):
  pinctrl: intel: Replace ifdeffery by pm_sleep_ptr() macro
  pinctrl: baytrail: Replace ifdeffery by pm_sleep_ptr() macro
  pinctrl: cherryview: Replace ifdeffery by pm_sleep_ptr() macro

 drivers/pinctrl/intel/pinctrl-baytrail.c   | 21 ++++++++++++++-------
 drivers/pinctrl/intel/pinctrl-cherryview.c | 20 +++++++++++++-------
 drivers/pinctrl/intel/pinctrl-intel.c      |  5 +----
 3 files changed, 28 insertions(+), 18 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

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

[Andy Shevchenko]

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
 
 	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;
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/3] pinctrl: baytrail: Replace ifdeffery by pm_sleep_ptr() macro

[Andy Shevchenko]

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-baytrail.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 4533c4d0a9e7..7aa0ddca7a59 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1514,13 +1514,6 @@ static int byt_gpio_probe(struct intel_pinctrl *vg)
 	gc->parent	= vg->dev;
 	gc->ngpio	= vg->soc->npins;
 
-#ifdef CONFIG_PM_SLEEP
-	vg->context.pads = devm_kcalloc(vg->dev, gc->ngpio, sizeof(*vg->context.pads),
-					GFP_KERNEL);
-	if (!vg->context.pads)
-		return -ENOMEM;
-#endif
-
 	/* set up interrupts  */
 	irq = platform_get_irq_optional(pdev, 0);
 	if (irq > 0) {
@@ -1581,6 +1574,16 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = {
 	{ }
 };
 
+static int byt_pinctrl_pm_init(struct intel_pinctrl *vg)
+{
+	vg->context.pads = devm_kcalloc(vg->dev, vg->soc->npins,
+					sizeof(*vg->context.pads), GFP_KERNEL);
+	if (!vg->context.pads)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int byt_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct intel_pinctrl_soc_data *soc_data;
@@ -1603,6 +1606,10 @@ static int byt_pinctrl_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = pm_sleep_ptr(byt_pinctrl_pm_init) ? byt_pinctrl_pm_init(vg) : 0;
+	if (ret)
+		return ret;
+
 	vg->pctldesc		= byt_pinctrl_desc;
 	vg->pctldesc.name	= dev_name(dev);
 	vg->pctldesc.pins	= vg->soc->pins;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/3] pinctrl: cherryview: Replace ifdeffery by pm_sleep_ptr() macro

[Andy Shevchenko]

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-cherryview.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 2f0e29c78dfb..9cdffd73e345 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1608,6 +1608,16 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function,
 	return AE_OK;
 }
 
+static int chv_pinctrl_pm_init(struct intel_pinctrl *pctrl)
+{
+	pctrl->context.pads = devm_kcalloc(pctrl->dev, pctrl->soc->npins,
+					   sizeof(*pctrl->context.pads), GFP_KERNEL);
+	if (!pctrl->context.pads)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int chv_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct intel_pinctrl_soc_data *soc_data;
@@ -1648,13 +1658,9 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 
 	community->pad_regs = community->regs + FAMILY_PAD_REGS_OFF;
 
-#ifdef CONFIG_PM_SLEEP
-	pctrl->context.pads = devm_kcalloc(dev, pctrl->soc->npins,
-					   sizeof(*pctrl->context.pads),
-					   GFP_KERNEL);
-	if (!pctrl->context.pads)
-		return -ENOMEM;
-#endif
+	ret = pm_sleep_ptr(chv_pinctrl_pm_init) ? chv_pinctrl_pm_init(pctrl) : 0;
+	if (ret)
+		return ret;
 
 	pctrl->context.communities = devm_kcalloc(dev, pctrl->soc->ncommunities,
 						  sizeof(*pctrl->context.communities),
-- 
2.43.0.rc1.1336.g36b5255a03ac

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

[Mika Westerberg]

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

Re: [PATCH v1 2/3] pinctrl: baytrail: Replace ifdeffery by pm_sleep_ptr() macro

[Mika Westerberg]

On Tue, Sep 03, 2024 at 08:04:50PM +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-baytrail.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
> index 4533c4d0a9e7..7aa0ddca7a59 100644
> --- a/drivers/pinctrl/intel/pinctrl-baytrail.c
> +++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
> @@ -1514,13 +1514,6 @@ static int byt_gpio_probe(struct intel_pinctrl *vg)
>  	gc->parent	= vg->dev;
>  	gc->ngpio	= vg->soc->npins;
>  
> -#ifdef CONFIG_PM_SLEEP
> -	vg->context.pads = devm_kcalloc(vg->dev, gc->ngpio, sizeof(*vg->context.pads),
> -					GFP_KERNEL);
> -	if (!vg->context.pads)
> -		return -ENOMEM;
> -#endif
> -
>  	/* set up interrupts  */
>  	irq = platform_get_irq_optional(pdev, 0);
>  	if (irq > 0) {
> @@ -1581,6 +1574,16 @@ static const struct acpi_device_id byt_gpio_acpi_match[] = {
>  	{ }
>  };
>  
> +static int byt_pinctrl_pm_init(struct intel_pinctrl *vg)
> +{
> +	vg->context.pads = devm_kcalloc(vg->dev, vg->soc->npins,
> +					sizeof(*vg->context.pads), GFP_KERNEL);
> +	if (!vg->context.pads)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int byt_pinctrl_probe(struct platform_device *pdev)
>  {
>  	const struct intel_pinctrl_soc_data *soc_data;
> @@ -1603,6 +1606,10 @@ static int byt_pinctrl_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = pm_sleep_ptr(byt_pinctrl_pm_init) ? byt_pinctrl_pm_init(vg) : 0;

Same here.

> +	if (ret)
> +		return ret;
> +
>  	vg->pctldesc		= byt_pinctrl_desc;
>  	vg->pctldesc.name	= dev_name(dev);
>  	vg->pctldesc.pins	= vg->soc->pins;
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 3/3] pinctrl: cherryview: Replace ifdeffery by pm_sleep_ptr() macro

[Mika Westerberg]

On Tue, Sep 03, 2024 at 08:04:51PM +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-cherryview.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 2f0e29c78dfb..9cdffd73e345 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1608,6 +1608,16 @@ static acpi_status chv_pinctrl_mmio_access_handler(u32 function,
>  	return AE_OK;
>  }
>  
> +static int chv_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> +{
> +	pctrl->context.pads = devm_kcalloc(pctrl->dev, pctrl->soc->npins,
> +					   sizeof(*pctrl->context.pads), GFP_KERNEL);
> +	if (!pctrl->context.pads)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int chv_pinctrl_probe(struct platform_device *pdev)
>  {
>  	const struct intel_pinctrl_soc_data *soc_data;
> @@ -1648,13 +1658,9 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>  
>  	community->pad_regs = community->regs + FAMILY_PAD_REGS_OFF;
>  
> -#ifdef CONFIG_PM_SLEEP
> -	pctrl->context.pads = devm_kcalloc(dev, pctrl->soc->npins,
> -					   sizeof(*pctrl->context.pads),
> -					   GFP_KERNEL);
> -	if (!pctrl->context.pads)
> -		return -ENOMEM;
> -#endif
> +	ret = pm_sleep_ptr(chv_pinctrl_pm_init) ? chv_pinctrl_pm_init(pctrl) : 0;

and here

> +	if (ret)
> +		return ret;
>  
>  	pctrl->context.communities = devm_kcalloc(dev, pctrl->soc->ncommunities,
>  						  sizeof(*pctrl->context.communities),
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Mika Westerberg]

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.

Re: [PATCH v1 3/3] pinctrl: cherryview: Replace ifdeffery by pm_sleep_ptr() macro

[kernel test robot]

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-4-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 3/3] pinctrl: cherryview: Replace ifdeffery by pm_sleep_ptr() macro
config: x86_64-randconfig-r073-20240904 (https://download.01.org/0day-ci/archive/20240904/202409041939.VrJGEW4H-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/202409041939.VrJGEW4H-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/202409041939.VrJGEW4H-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pinctrl/intel/pinctrl-cherryview.c: In function 'chv_pinctrl_probe':
>> drivers/pinctrl/intel/pinctrl-cherryview.c:1657:49: warning: the address of 'chv_pinctrl_pm_init' will always evaluate as 'true' [-Waddress]
    1657 |         ret = pm_sleep_ptr(chv_pinctrl_pm_init) ? chv_pinctrl_pm_init(pctrl) : 0;
         |                                                 ^


vim +1657 drivers/pinctrl/intel/pinctrl-cherryview.c

  1620	
  1621	static int chv_pinctrl_probe(struct platform_device *pdev)
  1622	{
  1623		const struct intel_pinctrl_soc_data *soc_data;
  1624		struct intel_community_context *cctx;
  1625		struct intel_community *community;
  1626		struct device *dev = &pdev->dev;
  1627		struct intel_pinctrl *pctrl;
  1628		acpi_status status;
  1629		unsigned int i;
  1630		int ret, irq;
  1631	
  1632		soc_data = intel_pinctrl_get_soc_data(pdev);
  1633		if (IS_ERR(soc_data))
  1634			return PTR_ERR(soc_data);
  1635	
  1636		pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
  1637		if (!pctrl)
  1638			return -ENOMEM;
  1639	
  1640		pctrl->dev = dev;
  1641		pctrl->soc = soc_data;
  1642	
  1643		pctrl->ncommunities = pctrl->soc->ncommunities;
  1644		pctrl->communities = devm_kmemdup(dev, pctrl->soc->communities,
  1645						  pctrl->ncommunities * sizeof(*pctrl->communities),
  1646						  GFP_KERNEL);
  1647		if (!pctrl->communities)
  1648			return -ENOMEM;
  1649	
  1650		community = &pctrl->communities[0];
  1651		community->regs = devm_platform_ioremap_resource(pdev, 0);
  1652		if (IS_ERR(community->regs))
  1653			return PTR_ERR(community->regs);
  1654	
  1655		community->pad_regs = community->regs + FAMILY_PAD_REGS_OFF;
  1656	
> 1657		ret = pm_sleep_ptr(chv_pinctrl_pm_init) ? chv_pinctrl_pm_init(pctrl) : 0;
  1658		if (ret)
  1659			return ret;
  1660	
  1661		pctrl->context.communities = devm_kcalloc(dev, pctrl->soc->ncommunities,
  1662							  sizeof(*pctrl->context.communities),
  1663							  GFP_KERNEL);
  1664		if (!pctrl->context.communities)
  1665			return -ENOMEM;
  1666	
  1667		cctx = &pctrl->context.communities[0];
  1668		for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++)
  1669			cctx->intr_lines[i] = CHV_INVALID_HWIRQ;
  1670	
  1671		irq = platform_get_irq(pdev, 0);
  1672		if (irq < 0)
  1673			return irq;
  1674	
  1675		pctrl->pctldesc = chv_pinctrl_desc;
  1676		pctrl->pctldesc.name = dev_name(dev);
  1677		pctrl->pctldesc.pins = pctrl->soc->pins;
  1678		pctrl->pctldesc.npins = pctrl->soc->npins;
  1679	
  1680		pctrl->pctldev = devm_pinctrl_register(dev, &pctrl->pctldesc, pctrl);
  1681		if (IS_ERR(pctrl->pctldev)) {
  1682			dev_err(dev, "failed to register pinctrl driver\n");
  1683			return PTR_ERR(pctrl->pctldev);
  1684		}
  1685	
  1686		ret = chv_gpio_probe(pctrl, irq);
  1687		if (ret)
  1688			return ret;
  1689	
  1690		status = acpi_install_address_space_handler(ACPI_HANDLE(dev),
  1691						community->acpi_space_id,
  1692						chv_pinctrl_mmio_access_handler,
  1693						NULL, pctrl);
  1694		if (ACPI_FAILURE(status))
  1695			dev_err(dev, "failed to install ACPI addr space handler\n");
  1696	
  1697		platform_set_drvdata(pdev, pctrl);
  1698	
  1699		return 0;
  1700	}
  1701	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 2/3] pinctrl: baytrail: Replace ifdeffery by pm_sleep_ptr() macro

[kernel test robot]

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-3-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 2/3] pinctrl: baytrail: Replace ifdeffery by pm_sleep_ptr() macro
config: i386-buildonly-randconfig-002-20240904 (https://download.01.org/0day-ci/archive/20240904/202409042054.YDMtnXfx-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/202409042054.YDMtnXfx-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/202409042054.YDMtnXfx-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/pinctrl/intel/pinctrl-baytrail.c: In function 'byt_pinctrl_probe':
>> drivers/pinctrl/intel/pinctrl-baytrail.c:1610:49: warning: the address of 'byt_pinctrl_pm_init' will always evaluate as 'true' [-Waddress]
    1610 |         ret = pm_sleep_ptr(byt_pinctrl_pm_init) ? byt_pinctrl_pm_init(vg) : 0;
         |                                                 ^


vim +1610 drivers/pinctrl/intel/pinctrl-baytrail.c

  1587	
  1588	static int byt_pinctrl_probe(struct platform_device *pdev)
  1589	{
  1590		const struct intel_pinctrl_soc_data *soc_data;
  1591		struct device *dev = &pdev->dev;
  1592		struct intel_pinctrl *vg;
  1593		int ret;
  1594	
  1595		soc_data = intel_pinctrl_get_soc_data(pdev);
  1596		if (IS_ERR(soc_data))
  1597			return PTR_ERR(soc_data);
  1598	
  1599		vg = devm_kzalloc(dev, sizeof(*vg), GFP_KERNEL);
  1600		if (!vg)
  1601			return -ENOMEM;
  1602	
  1603		vg->dev = dev;
  1604		ret = byt_set_soc_data(vg, soc_data);
  1605		if (ret) {
  1606			dev_err(dev, "failed to set soc data\n");
  1607			return ret;
  1608		}
  1609	
> 1610		ret = pm_sleep_ptr(byt_pinctrl_pm_init) ? byt_pinctrl_pm_init(vg) : 0;
  1611		if (ret)
  1612			return ret;
  1613	
  1614		vg->pctldesc		= byt_pinctrl_desc;
  1615		vg->pctldesc.name	= dev_name(dev);
  1616		vg->pctldesc.pins	= vg->soc->pins;
  1617		vg->pctldesc.npins	= vg->soc->npins;
  1618	
  1619		vg->pctldev = devm_pinctrl_register(dev, &vg->pctldesc, vg);
  1620		if (IS_ERR(vg->pctldev)) {
  1621			dev_err(dev, "failed to register pinctrl driver\n");
  1622			return PTR_ERR(vg->pctldev);
  1623		}
  1624	
  1625		ret = byt_gpio_probe(vg);
  1626		if (ret)
  1627			return ret;
  1628	
  1629		platform_set_drvdata(pdev, vg);
  1630	
  1631		return 0;
  1632	}
  1633	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki