[PATCH v1 0/5] w1: gpio: A set of cleanups

[PATCH v1 0/5] w1: gpio: A set of cleanups

[Andy Shevchenko]

A set of ad-hoc cleanups with making driver to be used in e.g.,
ACPI envionment.

Andy Shevchenko (5):
  w1: gpio: Make use of device properties
  w1: gpio: Switch to use dev_err_probe()
  w1: gpio: Use sizeof(*pointer) instead of sizeof(type)
  w1: gpio: Remove duplicate NULL checks
  w1: gpio: Don't use "proxy" headers

 drivers/w1/masters/w1-gpio.c | 62 +++++++++++++++---------------------
 1 file changed, 25 insertions(+), 37 deletions(-)

-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 1/5] w1: gpio: Make use of device properties

[Andy Shevchenko]

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/w1/masters/w1-gpio.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index 34128e6bbbfa..8ea53c757c99 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -6,13 +6,13 @@
  */
 
 #include <linux/init.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/gpio/consumer.h>
-#include <linux/of_platform.h>
 #include <linux/err.h>
-#include <linux/of.h>
 #include <linux/delay.h>
 
 #include <linux/w1.h>
@@ -63,20 +63,11 @@ static u8 w1_gpio_read_bit(void *data)
 	return gpiod_get_value(ddata->gpiod) ? 1 : 0;
 }
 
-#if defined(CONFIG_OF)
-static const struct of_device_id w1_gpio_dt_ids[] = {
-	{ .compatible = "w1-gpio" },
-	{}
-};
-MODULE_DEVICE_TABLE(of, w1_gpio_dt_ids);
-#endif
-
 static int w1_gpio_probe(struct platform_device *pdev)
 {
 	struct w1_bus_master *master;
 	struct w1_gpio_ddata *ddata;
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 	/* Enforce open drain mode by default */
 	enum gpiod_flags gflags = GPIOD_OUT_LOW_OPEN_DRAIN;
 	int err;
@@ -91,7 +82,7 @@ static int w1_gpio_probe(struct platform_device *pdev)
 	 * driver it high/low like we are in full control of the line and
 	 * open drain will happen transparently.
 	 */
-	if (of_property_present(np, "linux,open-drain"))
+	if (device_property_present(dev, "linux,open-drain"))
 		gflags = GPIOD_OUT_LOW;
 
 	master = devm_kzalloc(dev, sizeof(struct w1_bus_master),
@@ -152,10 +143,16 @@ static void w1_gpio_remove(struct platform_device *pdev)
 	w1_remove_master_device(master);
 }
 
+static const struct of_device_id w1_gpio_dt_ids[] = {
+	{ .compatible = "w1-gpio" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, w1_gpio_dt_ids);
+
 static struct platform_driver w1_gpio_driver = {
 	.driver = {
 		.name	= "w1-gpio",
-		.of_match_table = of_match_ptr(w1_gpio_dt_ids),
+		.of_match_table = w1_gpio_dt_ids,
 	},
 	.probe = w1_gpio_probe,
 	.remove_new = w1_gpio_remove,
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 2/5] w1: gpio: Switch to use dev_err_probe()

[Andy Shevchenko]

Switch to use dev_err_probe() to simplify the error path and
unify a message template.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/w1/masters/w1-gpio.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index 8ea53c757c99..a5ac34b32f54 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -91,18 +91,14 @@ static int w1_gpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ddata->gpiod = devm_gpiod_get_index(dev, NULL, 0, gflags);
-	if (IS_ERR(ddata->gpiod)) {
-		dev_err(dev, "gpio_request (pin) failed\n");
-		return PTR_ERR(ddata->gpiod);
-	}
+	if (IS_ERR(ddata->gpiod))
+		return dev_err_probe(dev, PTR_ERR(ddata->gpiod), "gpio_request (pin) failed\n");
 
 	ddata->pullup_gpiod =
 		devm_gpiod_get_index_optional(dev, NULL, 1, GPIOD_OUT_LOW);
-	if (IS_ERR(ddata->pullup_gpiod)) {
-		dev_err(dev, "gpio_request_one "
-			"(ext_pullup_enable_pin) failed\n");
-		return PTR_ERR(ddata->pullup_gpiod);
-	}
+	if (IS_ERR(ddata->pullup_gpiod))
+		return dev_err_probe(dev, PTR_ERR(ddata->pullup_gpiod),
+				     "gpio_request (ext_pullup_enable_pin) failed\n");
 
 	master->data = ddata;
 	master->read_bit = w1_gpio_read_bit;
@@ -119,10 +115,8 @@ static int w1_gpio_probe(struct platform_device *pdev)
 		master->set_pullup = w1_gpio_set_pullup;
 
 	err = w1_add_master_device(master);
-	if (err) {
-		dev_err(dev, "w1_add_master device failed\n");
-		return err;
-	}
+	if (err)
+		return dev_err_probe(dev, err, "w1_add_master device failed\n");
 
 	if (ddata->pullup_gpiod)
 		gpiod_set_value(ddata->pullup_gpiod, 1);
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 3/5] w1: gpio: Use sizeof(*pointer) instead of sizeof(type)

[Andy Shevchenko]

It is preferred to use sizeof(*pointer) instead of sizeof(type).
The type of the variable can change and one needs not change
the former (unlike the latter). No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/w1/masters/w1-gpio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index a5ac34b32f54..3881f2eaed2f 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -85,8 +85,7 @@ static int w1_gpio_probe(struct platform_device *pdev)
 	if (device_property_present(dev, "linux,open-drain"))
 		gflags = GPIOD_OUT_LOW;
 
-	master = devm_kzalloc(dev, sizeof(struct w1_bus_master),
-			GFP_KERNEL);
+	master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
 
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 4/5] w1: gpio: Remove duplicate NULL checks

[Andy Shevchenko]

gpiod_set_value() is NULL-aware, no need to check that in the caller.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/w1/masters/w1-gpio.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index 3881f2eaed2f..8fd9fedd8c56 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -117,8 +117,7 @@ static int w1_gpio_probe(struct platform_device *pdev)
 	if (err)
 		return dev_err_probe(dev, err, "w1_add_master device failed\n");
 
-	if (ddata->pullup_gpiod)
-		gpiod_set_value(ddata->pullup_gpiod, 1);
+	gpiod_set_value(ddata->pullup_gpiod, 1);
 
 	platform_set_drvdata(pdev, master);
 
@@ -130,8 +129,7 @@ static void w1_gpio_remove(struct platform_device *pdev)
 	struct w1_bus_master *master = platform_get_drvdata(pdev);
 	struct w1_gpio_ddata *ddata = master->data;
 
-	if (ddata->pullup_gpiod)
-		gpiod_set_value(ddata->pullup_gpiod, 0);
+	gpiod_set_value(ddata->pullup_gpiod, 0);
 
 	w1_remove_master_device(master);
 }
-- 
2.43.0.rc1.1.gbec44491f096

[PATCH v1 5/5] w1: gpio: Don't use "proxy" headers

[Andy Shevchenko]

Update header inclusions to follow IWYU (Include What You Use)
principle.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/w1/masters/w1-gpio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index 8fd9fedd8c56..a39fa8bf866a 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -5,15 +5,15 @@
  * Copyright (C) 2007 Ville Syrjala <syrjala@sci.fi>
  */
 
-#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
-#include <linux/slab.h>
-#include <linux/gpio/consumer.h>
-#include <linux/err.h>
-#include <linux/delay.h>
+#include <linux/types.h>
 
 #include <linux/w1.h>
 
-- 
2.43.0.rc1.1.gbec44491f096

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Uwe Kleine-König]

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

Hello Andy,

I wonder about your choice of recipients. I would have added Krzysztof
to To and me at most to Cc:.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 2/5] w1: gpio: Switch to use dev_err_probe()

[Uwe Kleine-König]

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

On Thu, Mar 07, 2024 at 04:35:48PM +0200, Andy Shevchenko wrote:
> Switch to use dev_err_probe() to simplify the error path and
> unify a message template.

This also has the upside that the error code is included in the message
and EPROBE_DEFER is correctly handled.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 3/5] w1: gpio: Use sizeof(*pointer) instead of sizeof(type)

[Uwe Kleine-König]

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

On Thu, Mar 07, 2024 at 04:35:49PM +0200, Andy Shevchenko wrote:
> It is preferred to use sizeof(*pointer) instead of sizeof(type).
> The type of the variable can change and one needs not change
> the former (unlike the latter). No functional change intended.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Andy Shevchenko]

On Thu, Mar 07, 2024 at 05:38:54PM +0100, Uwe Kleine-König wrote:
> Hello Andy,
> 
> I wonder about your choice of recipients. I would have added Krzysztof
> to To and me at most to Cc:.

It's automatically generated using get_maintainers.pl.
See details in the source of the script [1] I'm using.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] w1: gpio: Remove duplicate NULL checks

[Uwe Kleine-König]

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

On Thu, Mar 07, 2024 at 04:35:50PM +0200, Andy Shevchenko wrote:
> gpiod_set_value() is NULL-aware, no need to check that in the caller.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/w1/masters/w1-gpio.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
> index 3881f2eaed2f..8fd9fedd8c56 100644
> --- a/drivers/w1/masters/w1-gpio.c
> +++ b/drivers/w1/masters/w1-gpio.c
> @@ -117,8 +117,7 @@ static int w1_gpio_probe(struct platform_device *pdev)
>  	if (err)
>  		return dev_err_probe(dev, err, "w1_add_master device failed\n");
>  
> -	if (ddata->pullup_gpiod)
> -		gpiod_set_value(ddata->pullup_gpiod, 1);
> +	gpiod_set_value(ddata->pullup_gpiod, 1);
>  
>  	platform_set_drvdata(pdev, master);
>  
> @@ -130,8 +129,7 @@ static void w1_gpio_remove(struct platform_device *pdev)
>  	struct w1_bus_master *master = platform_get_drvdata(pdev);
>  	struct w1_gpio_ddata *ddata = master->data;
>  
> -	if (ddata->pullup_gpiod)
> -		gpiod_set_value(ddata->pullup_gpiod, 0);
> +	gpiod_set_value(ddata->pullup_gpiod, 0);

This also has the advantage to not have to know that NULL is the dummy
value returned by the gpiod_get_optional() family of functions.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Uwe Kleine-König]

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

Hello Andy,

On Thu, Mar 07, 2024 at 06:43:56PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 07, 2024 at 05:38:54PM +0100, Uwe Kleine-König wrote:
> > I wonder about your choice of recipients. I would have added Krzysztof
> > to To and me at most to Cc:.
> 
> It's automatically generated using get_maintainers.pl.
> See details in the source of the script [1] I'm using.
> 
> [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

Getting something wrong automatically isn't an excuse for getting it
wrong :-)

That scripts has:

to=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-m --no-r)
cc=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-l)

I recommend to swap the values for to and cc here to make sure you have
the maintainer in $to and the relevant lists in $cc.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Andy Shevchenko]

On Thu, Mar 07, 2024 at 06:12:46PM +0100, Uwe Kleine-König wrote:
> On Thu, Mar 07, 2024 at 06:43:56PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 07, 2024 at 05:38:54PM +0100, Uwe Kleine-König wrote:

...

> > > I wonder about your choice of recipients. I would have added Krzysztof
> > > to To and me at most to Cc:.
> > 
> > It's automatically generated using get_maintainers.pl.
> > See details in the source of the script [1] I'm using.
> > 
> > [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
> 
> Getting something wrong automatically isn't an excuse for getting it
> wrong :-)

I'm not sure why you think it's wrong. You worked on the code lately and Git
heuristics considered that over threshold of 67%.

> That scripts has:
> 
> to=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-m --no-r)
> cc=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-l)
> 
> I recommend to swap the values for to and cc here to make sure you have
> the maintainer in $to and the relevant lists in $cc.

Hmm... I don't remember why I put it this way.

Btw, you are the first one for the entire life cycle of that script (3 years?)
who complains about such details... So, patches are welcome! :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Uwe Kleine-König]

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

Hello Andy,

On Thu, Mar 07, 2024 at 07:57:01PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 07, 2024 at 06:12:46PM +0100, Uwe Kleine-König wrote:
> > On Thu, Mar 07, 2024 at 06:43:56PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 07, 2024 at 05:38:54PM +0100, Uwe Kleine-König wrote:
> 
> ...
> 
> > > > I wonder about your choice of recipients. I would have added Krzysztof
> > > > to To and me at most to Cc:.
> > > 
> > > It's automatically generated using get_maintainers.pl.
> > > See details in the source of the script [1] I'm using.
> > > 
> > > [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
> > 
> > Getting something wrong automatically isn't an excuse for getting it
> > wrong :-)
> 
> I'm not sure why you think it's wrong. You worked on the code lately and Git
> heuristics considered that over threshold of 67%.

When I send a patch I send it "to" the maintainers, because it's them
who I want an action from. This also matches the semantic of M: in
MAINTAINERS which requests to use these contaces in "To:".

> > That scripts has:
> > 
> > to=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-m --no-r)
> > cc=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-l)
> > 
> > I recommend to swap the values for to and cc here to make sure you have
> > the maintainer in $to and the relevant lists in $cc.

Thinking again, this is wrong. I'd recommend:

	to=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-r --no-l)
	cc=$(git show -$count "$COMMIT" | scripts/get_maintainer.pl $OPTS --no-m)

> Btw, you are the first one for the entire life cycle of that script (3 years?)
> who complains about such details... So, patches are welcome! :-)

I won't do more than the above hint here.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Krzysztof Kozlowski]

On 07/03/2024 17:38, Uwe Kleine-König wrote:
> Hello Andy,
> 
> I wonder about your choice of recipients. I would have added Krzysztof
> to To and me at most to Cc:.

That's indeed odd (and reminds me Qualcomm proposing wrapper over
get_maintainers.pl which on purpose put the maintainers in "To:" and
lists in "Cc:") and I imagine people having filters depending on To: and
Cc: distinction, but for the record: I don't care. People put it usually
wrong, so my filters just choose (To Or Cc).

Anyway all the patches will wait till the end of the merge window.

Best regards,
Krzysztof

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Andy Shevchenko]

On Fri, Mar 08, 2024 at 09:34:47AM +0100, Krzysztof Kozlowski wrote:
> On 07/03/2024 17:38, Uwe Kleine-König wrote:
> > Hello Andy,
> > 
> > I wonder about your choice of recipients. I would have added Krzysztof
> > to To and me at most to Cc:.
> 
> That's indeed odd (and reminds me Qualcomm proposing wrapper over
> get_maintainers.pl which on purpose put the maintainers in "To:" and
> lists in "Cc:") and I imagine people having filters depending on To: and
> Cc: distinction, but for the record: I don't care. People put it usually
> wrong, so my filters just choose (To Or Cc).
> 
> Anyway all the patches will wait till the end of the merge window.

Fine by me, thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/5] w1: gpio: A set of cleanups

[Krzysztof Kozlowski]

On Thu, 07 Mar 2024 16:35:46 +0200, Andy Shevchenko wrote:
> A set of ad-hoc cleanups with making driver to be used in e.g.,
> ACPI envionment.
> 
> Andy Shevchenko (5):
>   w1: gpio: Make use of device properties
>   w1: gpio: Switch to use dev_err_probe()
>   w1: gpio: Use sizeof(*pointer) instead of sizeof(type)
>   w1: gpio: Remove duplicate NULL checks
>   w1: gpio: Don't use "proxy" headers
> 
> [...]

Applied, thanks!

[1/5] w1: gpio: Make use of device properties
      https://git.kernel.org/krzk/linux-w1/c/8b39a723ef1fa3737e11832ca11183bbaeda2498
[2/5] w1: gpio: Switch to use dev_err_probe()
      https://git.kernel.org/krzk/linux-w1/c/9e085c045868a6a727b3bd0fc7840ccc9e04d3a3
[3/5] w1: gpio: Use sizeof(*pointer) instead of sizeof(type)
      https://git.kernel.org/krzk/linux-w1/c/ef2b810e1152d77686032e7dc064ff89b4350b00
[4/5] w1: gpio: Remove duplicate NULL checks
      https://git.kernel.org/krzk/linux-w1/c/540d3f15c0aa2baf7e9b48a4e516391c179daab2
[5/5] w1: gpio: Don't use "proxy" headers
      https://git.kernel.org/krzk/linux-w1/c/cde37a5bdb0ed2c4c7b86ef688e5fdb697525a57

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>