[PATCH v1 0/4] platform/x86: int3472: A few cleanups

[PATCH v1 0/4] platform/x86: int3472: A few cleanups

[Andy Shevchenko]

A few cleanups to the driver. It includes an amendment to
dev_err_probe() to ignore 0 error code. The patches 1 and 2 are
dependent, while patches 2 and 3 may be applied in any order.

Assumed to go via PDx86 tree if no objections to the code.

Andy Shevchenko (4):
  driver core: Ignore 0 in dev_err_probe()
  platform/x86: int3472: Simplify dev_err_probe() usage
  platform/x86: int3472: Use GPIO_LOOKUP() macro
  platform/x86: int3472: Use str_high_low()

 drivers/base/core.c                           |  4 ++++
 drivers/platform/x86/intel/int3472/discrete.c | 14 ++++----------
 2 files changed, 8 insertions(+), 10 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

In the similar way, ignore 0 error code (AKA "success") in
dev_err_probe(). This helps to simplify a code such as

  if (ret < 0)
    return dev_err_probe(int3472->dev, ret, err_msg);

  return ret;

to

  return dev_err_probe(int3472->dev, ret, err_msg);

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 6d3897492285..144cb8c201fb 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5046,6 +5046,10 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
 		 */
 		break;
 
+	case 0:
+		/* Success, no need to issue an error message */
+		break;
+
 	default:
 		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
 		break;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Andy Shevchenko]

Since dev_err_probe() ignores success, i.e. 0. we may call
it unconditionally.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 07b302e09340..cd0743300d7f 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -289,10 +289,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 	int3472->ngpios++;
 	ACPI_FREE(obj);
 
-	if (ret < 0)
-		return dev_err_probe(int3472->dev, ret, err_msg);
-
-	return ret;
+	return dev_err_probe(int3472->dev, ret, err_msg);
 }
 
 static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/4] platform/x86: int3472: Use GPIO_LOOKUP() macro

[Andy Shevchenko]

Use GPIO_LOOKUP() macro which provides a compound literal
and can be used with dynamic data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index cd0743300d7f..96a9673a0165 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -69,11 +69,7 @@ static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
 	if (!adev)
 		return -ENODEV;
 
-	table_entry->key = acpi_dev_name(adev);
-	table_entry->chip_hwnum = agpio->pin_table[0];
-	table_entry->con_id = func;
-	table_entry->idx = 0;
-	table_entry->flags = polarity;
+	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], func, polarity);
 
 	return 0;
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 4/4] platform/x86: int3472: Use str_high_low()

[Andy Shevchenko]

Use str_high_low() rather than open coding.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
index 96a9673a0165..b5f6f71bb1dd 100644
--- a/drivers/platform/x86/intel/int3472/discrete.c
+++ b/drivers/platform/x86/intel/int3472/discrete.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/overflow.h>
 #include <linux/platform_device.h>
+#include <linux/string_choices.h>
 #include <linux/uuid.h>
 
 #include "common.h"
@@ -230,7 +231,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
 
 	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
 		agpio->resource_source.string_ptr, agpio->pin_table[0],
-		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
+		str_high_low(polarity == GPIO_ACTIVE_HIGH));
 
 	switch (type) {
 	case INT3472_GPIO_TYPE_RESET:
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 4/4] platform/x86: int3472: Use str_high_low()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Wed, 21 Aug 2024, Andy Shevchenko wrote:

> Use str_high_low() rather than open coding.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 96a9673a0165..b5f6f71bb1dd 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -11,6 +11,7 @@
>  #include <linux/module.h>
>  #include <linux/overflow.h>
>  #include <linux/platform_device.h>
> +#include <linux/string_choices.h>
>  #include <linux/uuid.h>
>  
>  #include "common.h"
> @@ -230,7 +231,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  
>  	dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func,
>  		agpio->resource_source.string_ptr, agpio->pin_table[0],
> -		(polarity == GPIO_ACTIVE_HIGH) ? "high" : "low");
> +		str_high_low(polarity == GPIO_ACTIVE_HIGH));
>  
>  	switch (type) {
>  	case INT3472_GPIO_TYPE_RESET:
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v1 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Christophe JAILLET]

Le 21/08/2024 à 14:20, Andy Shevchenko a écrit :
> Since dev_err_probe() ignores success,

Hi,

Does it really?
It does not seem to be the case (at least on linux-next). Or I miss 
something obvious?

CJ

> i.e. 0. we may call
> it unconditionally.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/platform/x86/intel/int3472/discrete.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 07b302e09340..cd0743300d7f 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -289,10 +289,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>   	int3472->ngpios++;
>   	ACPI_FREE(obj);
>   
> -	if (ret < 0)
> -		return dev_err_probe(int3472->dev, ret, err_msg);
> -
> -	return ret;
> +	return dev_err_probe(int3472->dev, ret, err_msg);
>   }
>   
>   static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)

Re: [PATCH v1 1/4] driver core: Ignore 0 in dev_err_probe()

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

On Wed, 21 Aug 2024, Andy Shevchenko wrote:

> In the similar way, ignore 0 error code (AKA "success") in
> dev_err_probe(). This helps to simplify a code such as
> 
>   if (ret < 0)
>     return dev_err_probe(int3472->dev, ret, err_msg);
> 
>   return ret;
> 
> to
> 
>   return dev_err_probe(int3472->dev, ret, err_msg);
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/base/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 6d3897492285..144cb8c201fb 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -5046,6 +5046,10 @@ int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
>  		 */
>  		break;
>  
> +	case 0:
> +		/* Success, no need to issue an error message */
> +		break;
> +
>  	default:
>  		dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
>  		break;
> 

This seems generally useful,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

A nit, the code sequence that dev_err_probe() is documented to replace is 
no longer matches with the behavior (the else would need if (err < 0) 
added). 

-- 
 i.

Re: [PATCH v1 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1307 bytes --]

On Wed, 21 Aug 2024, Christophe JAILLET wrote:

> Le 21/08/2024 à 14:20, Andy Shevchenko a écrit :
> > Since dev_err_probe() ignores success,
> 
> Hi,
> 
> Does it really?
> It does not seem to be the case (at least on linux-next). Or I miss something
> obvious?

Yes, you're missing the first patch of the series. :-)

-- 
 i.


> > i.e. 0. we may call
> > it unconditionally.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >   drivers/platform/x86/intel/int3472/discrete.c | 5 +----
> >   1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel/int3472/discrete.c
> > b/drivers/platform/x86/intel/int3472/discrete.c
> > index 07b302e09340..cd0743300d7f 100644
> > --- a/drivers/platform/x86/intel/int3472/discrete.c
> > +++ b/drivers/platform/x86/intel/int3472/discrete.c
> > @@ -289,10 +289,7 @@ static int skl_int3472_handle_gpio_resources(struct
> > acpi_resource *ares,
> >   	int3472->ngpios++;
> >   	ACPI_FREE(obj);
> >   -	if (ret < 0)
> > -		return dev_err_probe(int3472->dev, ret, err_msg);
> > -
> > -	return ret;
> > +	return dev_err_probe(int3472->dev, ret, err_msg);
> >   }
> >     static int skl_int3472_parse_crs(struct int3472_discrete_device
> > *int3472)
> 
> 

Re: [PATCH v1 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

On Wed, 21 Aug 2024, Andy Shevchenko wrote:

> Since dev_err_probe() ignores success, i.e. 0. we may call
> it unconditionally.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index 07b302e09340..cd0743300d7f 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -289,10 +289,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
>  	int3472->ngpios++;
>  	ACPI_FREE(obj);
>  
> -	if (ret < 0)
> -		return dev_err_probe(int3472->dev, ret, err_msg);
> -
> -	return ret;
> +	return dev_err_probe(int3472->dev, ret, err_msg);
>  }
>  
>  static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v1 3/4] platform/x86: int3472: Use GPIO_LOOKUP() macro

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On Wed, 21 Aug 2024, Andy Shevchenko wrote:

> Use GPIO_LOOKUP() macro which provides a compound literal
> and can be used with dynamic data.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/platform/x86/intel/int3472/discrete.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c
> index cd0743300d7f..96a9673a0165 100644
> --- a/drivers/platform/x86/intel/int3472/discrete.c
> +++ b/drivers/platform/x86/intel/int3472/discrete.c
> @@ -69,11 +69,7 @@ static int skl_int3472_fill_gpiod_lookup(struct gpiod_lookup *table_entry,
>  	if (!adev)
>  		return -ENODEV;
>  
> -	table_entry->key = acpi_dev_name(adev);
> -	table_entry->chip_hwnum = agpio->pin_table[0];
> -	table_entry->con_id = func;
> -	table_entry->idx = 0;
> -	table_entry->flags = polarity;
> +	*table_entry = GPIO_LOOKUP(acpi_dev_name(adev), agpio->pin_table[0], func, polarity);
>  
>  	return 0;
>  }
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

Re: [PATCH v1 2/4] platform/x86: int3472: Simplify dev_err_probe() usage

[Christophe JAILLET]

Le 21/08/2024 à 14:39, Ilpo Järvinen a écrit :
> On Wed, 21 Aug 2024, Christophe JAILLET wrote:
> 
>> Le 21/08/2024 à 14:20, Andy Shevchenko a écrit :
>>> Since dev_err_probe() ignores success,
>>
>> Hi,
>>
>> Does it really?
>> It does not seem to be the case (at least on linux-next). Or I miss something
>> obvious?
> 
> Yes, you're missing the first patch of the series. :-)
> 

Lol.

Sorry for the noise. (* blushing *)

CJ

Re: [PATCH v1 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

On Wed, Aug 21, 2024 at 03:38:15PM +0300, Ilpo Järvinen wrote:
> On Wed, 21 Aug 2024, Andy Shevchenko wrote:
> 
> > In the similar way, ignore 0 error code (AKA "success") in
> > dev_err_probe(). This helps to simplify a code such as
> > 
> >   if (ret < 0)
> >     return dev_err_probe(int3472->dev, ret, err_msg);
> > 
> >   return ret;
> > 
> > to
> > 
> >   return dev_err_probe(int3472->dev, ret, err_msg);

...

> This seems generally useful,
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks!

> A nit, the code sequence that dev_err_probe() is documented to replace is 
> no longer matches with the behavior (the else would need if (err < 0) 
> added).

Okay, let's wait what Greg says about it, if he is okay with the proposal,
I'll amend documentation as well in v2.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/4] driver core: Ignore 0 in dev_err_probe()

[Greg Kroah-Hartman]

On Wed, Aug 21, 2024 at 04:12:22PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 21, 2024 at 03:38:15PM +0300, Ilpo Järvinen wrote:
> > On Wed, 21 Aug 2024, Andy Shevchenko wrote:
> > 
> > > In the similar way, ignore 0 error code (AKA "success") in
> > > dev_err_probe(). This helps to simplify a code such as
> > > 
> > >   if (ret < 0)
> > >     return dev_err_probe(int3472->dev, ret, err_msg);
> > > 
> > >   return ret;
> > > 
> > > to
> > > 
> > >   return dev_err_probe(int3472->dev, ret, err_msg);
> 
> ...
> 
> > This seems generally useful,
> > 
> > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Thanks!
> 
> > A nit, the code sequence that dev_err_probe() is documented to replace is 
> > no longer matches with the behavior (the else would need if (err < 0) 
> > added).
> 
> Okay, let's wait what Greg says about it, if he is okay with the proposal,
> I'll amend documentation as well in v2.

I like it, and will be glad to take it if you update the documentation :)

thanks,

greg k-h

Re: [PATCH v1 1/4] driver core: Ignore 0 in dev_err_probe()

[Andy Shevchenko]

On Thu, Aug 22, 2024 at 07:32:33AM +0800, Greg Kroah-Hartman wrote:
> On Wed, Aug 21, 2024 at 04:12:22PM +0300, Andy Shevchenko wrote:

...

> I like it, and will be glad to take it if you update the documentation :)

Consider it done.

Thanks!

-- 
With Best Regards,
Andy Shevchenko