[PATCH v1 1/3] mux: gpio: Use bitmap API instead of direct assignment

[PATCH v1 1/3] mux: gpio: Use bitmap API instead of direct assignment

[Andy Shevchenko]

Assigning bitmaps like it's done in the driver might be error prone.
Fix this by using bitmap API.

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

diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
index 02c1f2c014e8..891ee0274733 100644
--- a/drivers/mux/gpio.c
+++ b/drivers/mux/gpio.c
@@ -6,7 +6,7 @@
  *
  * Author: Peter Rosin <peda@axentia.se>
  */
-
+#include <linux/bitmap.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -23,8 +23,9 @@ static int mux_gpio_set(struct mux_control *mux, int state)
 {
 	struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
 	DECLARE_BITMAP(values, BITS_PER_TYPE(state));
+	u32 value = state;
 
-	values[0] = state;
+	bitmap_from_arr32(values, &value, BITS_PER_TYPE(value));
 
 	gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
 				       mux_gpio->gpios->desc,
@@ -71,7 +72,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
 		return ret;
 	}
 	WARN_ON(pins != mux_gpio->gpios->ndescs);
-	mux_chip->mux->states = 1 << pins;
+	mux_chip->mux->states = BIT(pins);
 
 	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
 	if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
-- 
2.30.2

[PATCH v1 2/3] mux: gpio: Make it OF independent

[Andy Shevchenko]

Module doesn't use OF APIs anyhow, make it OF independent by replacing
headers and dropping useless of_match_ptr() call.

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

diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
index 891ee0274733..e5ef9284e2b4 100644
--- a/drivers/mux/gpio.c
+++ b/drivers/mux/gpio.c
@@ -10,8 +10,8 @@
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/mux/driver.h>
-#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 
@@ -97,7 +97,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
 static struct platform_driver mux_gpio_driver = {
 	.driver = {
 		.name = "gpio-mux",
-		.of_match_table	= of_match_ptr(mux_gpio_dt_ids),
+		.of_match_table	= mux_gpio_dt_ids,
 	},
 	.probe = mux_gpio_probe,
 };
-- 
2.30.2

Re: [PATCH v1 2/3] mux: gpio: Make it OF independent

[Peter Rosin]

Hi!

On 2021-03-26 18:24, Andy Shevchenko wrote:
> Module doesn't use OF APIs anyhow, make it OF independent by replacing
> headers and dropping useless of_match_ptr() call.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mux/gpio.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 891ee0274733..e5ef9284e2b4 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -10,8 +10,8 @@
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> +#include <linux/mod_devicetable.h>

mod_devicetable.h sorts before module.h. With that changed,

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

>  #include <linux/mux/driver.h>
> -#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  
> @@ -97,7 +97,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
>  static struct platform_driver mux_gpio_driver = {
>  	.driver = {
>  		.name = "gpio-mux",
> -		.of_match_table	= of_match_ptr(mux_gpio_dt_ids),
> +		.of_match_table	= mux_gpio_dt_ids,
>  	},
>  	.probe = mux_gpio_probe,
>  };
> 

[PATCH v1 3/3] mux: gpio: Simplify code by using dev_err_probe()

[Andy Shevchenko]

Use already prepared dev_err_probe() introduced by the commit
a787e5400a1c ("driver core: add device probe log helper").
It simplifies EPROBE_DEFER handling.

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

diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
index e5ef9284e2b4..c3036bfffd50 100644
--- a/drivers/mux/gpio.c
+++ b/drivers/mux/gpio.c
@@ -65,12 +65,8 @@ static int mux_gpio_probe(struct platform_device *pdev)
 	mux_chip->ops = &mux_gpio_ops;
 
 	mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
-	if (IS_ERR(mux_gpio->gpios)) {
-		ret = PTR_ERR(mux_gpio->gpios);
-		if (ret != -EPROBE_DEFER)
-			dev_err(dev, "failed to get gpios\n");
-		return ret;
-	}
+	if (IS_ERR(mux_gpio->gpios))
+		return dev_err_probe(dev, PTR_ERR(mux_gpio->gpios), "failed to get gpios\n");
 	WARN_ON(pins != mux_gpio->gpios->ndescs);
 	mux_chip->mux->states = BIT(pins);
 
-- 
2.30.2

Re: [PATCH v1 3/3] mux: gpio: Simplify code by using dev_err_probe()

[Peter Rosin]

Hi!

On 2021-03-26 18:24, Andy Shevchenko wrote:
> Use already prepared dev_err_probe() introduced by the commit
> a787e5400a1c ("driver core: add device probe log helper").
> It simplifies EPROBE_DEFER handling.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mux/gpio.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index e5ef9284e2b4..c3036bfffd50 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -65,12 +65,8 @@ static int mux_gpio_probe(struct platform_device *pdev)
>  	mux_chip->ops = &mux_gpio_ops;
>  
>  	mux_gpio->gpios = devm_gpiod_get_array(dev, "mux", GPIOD_OUT_LOW);
> -	if (IS_ERR(mux_gpio->gpios)) {
> -		ret = PTR_ERR(mux_gpio->gpios);
> -		if (ret != -EPROBE_DEFER)
> -			dev_err(dev, "failed to get gpios\n");
> -		return ret;
> -	}
> +	if (IS_ERR(mux_gpio->gpios))
> +		return dev_err_probe(dev, PTR_ERR(mux_gpio->gpios), "failed to get gpios\n");

Please break this line, keeping it as one line does not significantly
increase readability. I know many people think long lines are super
nice, but I'm not sold and am stubbornly sticking to 80 cols. I'd rater
have room for one more window instead of wasting loads of screen on
mostly short lines and a long one here and there. Sorry to be a pest,
but coding-style.rst agrees with me:

"The preferred limit on the length of a single line is 80 columns."

So, with that changed,

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
Peter

>  	WARN_ON(pins != mux_gpio->gpios->ndescs);
>  	mux_chip->mux->states = BIT(pins);
>  
> 

Re: [PATCH v1 1/3] mux: gpio: Use bitmap API instead of direct assignment

[Andy Shevchenko]

On Fri, Mar 26, 2021 at 07:23:59PM +0200, Andy Shevchenko wrote:
> Assigning bitmaps like it's done in the driver might be error prone.
> Fix this by using bitmap API.

Peter, are you okay with the patches?
Should I reroute them to Greg?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/3] mux: gpio: Use bitmap API instead of direct assignment

[Peter Rosin]

Hi!

On 2021-03-26 18:23, Andy Shevchenko wrote:
> Assigning bitmaps like it's done in the driver might be error prone.
> Fix this by using bitmap API.

A bit strongly worded perhaps, since the size of a mux chip with
anywhere near 31 inputs and 2^31 possible selections is a bit
ridiculous. Please send a photo of that HW if someone is producing
one :-)

But there's always the someone-borrows-code-factor, so I guess it's
fine as-is.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mux/gpio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mux/gpio.c b/drivers/mux/gpio.c
> index 02c1f2c014e8..891ee0274733 100644
> --- a/drivers/mux/gpio.c
> +++ b/drivers/mux/gpio.c
> @@ -6,7 +6,7 @@
>   *
>   * Author: Peter Rosin <peda@axentia.se>
>   */
> -

Nit, please keep the empty line here. With that,

Acked-by: Peter Rosin <peda@axentia.se>

And please send directly to Greg, that would be excellent, thanks!

Cheers,
Peter

> +#include <linux/bitmap.h>
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> @@ -23,8 +23,9 @@ static int mux_gpio_set(struct mux_control *mux, int state)
>  {
>  	struct mux_gpio *mux_gpio = mux_chip_priv(mux->chip);
>  	DECLARE_BITMAP(values, BITS_PER_TYPE(state));
> +	u32 value = state;
>  
> -	values[0] = state;
> +	bitmap_from_arr32(values, &value, BITS_PER_TYPE(value));
>  
>  	gpiod_set_array_value_cansleep(mux_gpio->gpios->ndescs,
>  				       mux_gpio->gpios->desc,
> @@ -71,7 +72,7 @@ static int mux_gpio_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  	WARN_ON(pins != mux_gpio->gpios->ndescs);
> -	mux_chip->mux->states = 1 << pins;
> +	mux_chip->mux->states = BIT(pins);
>  
>  	ret = device_property_read_u32(dev, "idle-state", (u32 *)&idle_state);
>  	if (ret >= 0 && idle_state != MUX_IDLE_AS_IS) {
>