[PATCH v1 1/5] i2c: mux: pca954x: Refactor pca954x_irq_handler()

[PATCH v1 1/5] i2c: mux: pca954x: Refactor pca954x_irq_handler()

[Andy Shevchenko]

Refactor pca954x_irq_handler() to:
  - use for_each_set_bit() macro
  - use IRQ_RETVAL() macro

Above change makes code easy to read and understand.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index a0d926ae3f86..819ff95e64ba 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -327,21 +327,18 @@ static DEVICE_ATTR_RW(idle_state);
 static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
 {
 	struct pca954x *data = dev_id;
-	unsigned int child_irq;
-	int ret, i, handled = 0;
+	unsigned long pending;
+	int ret, i;
 
 	ret = i2c_smbus_read_byte(data->client);
 	if (ret < 0)
 		return IRQ_NONE;
 
-	for (i = 0; i < data->chip->nchans; i++) {
-		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
-			child_irq = irq_linear_revmap(data->irq, i);
-			handle_nested_irq(child_irq);
-			handled++;
-		}
-	}
-	return handled ? IRQ_HANDLED : IRQ_NONE;
+	pending = ret >> PCA954X_IRQ_OFFSET;
+	for_each_set_bit(i, &pending, data->chip->nchans)
+		handle_nested_irq(irq_linear_revmap(data->irq, i));
+
+	return IRQ_RETVAL(pending);
 }
 
 static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
-- 
2.25.1

[PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Andy Shevchenko]

Device property API allows to gather device resources from different sources,
such as ACPI. Convert the drivers to unleash the power of device property API.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 39 +++++++++++++----------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 819ff95e64ba..2e42a113ef1f 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -43,10 +43,8 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/module.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
-#include <linux/of_irq.h>
 #include <linux/pm.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <dt-bindings/mux/mux.h>
@@ -182,23 +180,22 @@ static const struct chip_desc chips[] = {
 };
 
 static const struct i2c_device_id pca954x_id[] = {
-	{ "pca9540", pca_9540 },
-	{ "pca9542", pca_9542 },
-	{ "pca9543", pca_9543 },
-	{ "pca9544", pca_9544 },
-	{ "pca9545", pca_9545 },
-	{ "pca9546", pca_9546 },
-	{ "pca9547", pca_9547 },
-	{ "pca9548", pca_9548 },
-	{ "pca9846", pca_9846 },
-	{ "pca9847", pca_9847 },
-	{ "pca9848", pca_9848 },
-	{ "pca9849", pca_9849 },
+	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
+	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
+	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
+	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
+	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
+	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
+	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
+	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
+	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
+	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
+	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
+	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, pca954x_id);
 
-#ifdef CONFIG_OF
 static const struct of_device_id pca954x_of_match[] = {
 	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
 	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
@@ -215,7 +212,6 @@ static const struct of_device_id pca954x_of_match[] = {
 	{}
 };
 MODULE_DEVICE_TABLE(of, pca954x_of_match);
-#endif
 
 /* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
    for this as they will try to lock adapter a second time */
@@ -426,7 +422,6 @@ static int pca954x_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adap = client->adapter;
 	struct device *dev = &client->dev;
-	struct device_node *np = dev->of_node;
 	struct gpio_desc *gpio;
 	struct i2c_mux_core *muxc;
 	struct pca954x *data;
@@ -456,7 +451,7 @@ static int pca954x_probe(struct i2c_client *client,
 		udelay(1);
 	}
 
-	data->chip = of_device_get_match_data(dev);
+	data->chip = device_get_match_data(dev);
 	if (!data->chip)
 		data->chip = &chips[id->driver_data];
 
@@ -478,8 +473,8 @@ static int pca954x_probe(struct i2c_client *client,
 	}
 
 	data->idle_state = MUX_IDLE_AS_IS;
-	if (of_property_read_u32(np, "idle-state", &data->idle_state)) {
-		if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))
+	if (device_property_read_u32(dev, "idle-state", &data->idle_state)) {
+		if (device_property_read_bool(dev, "i2c-mux-idle-disconnect"))
 			data->idle_state = MUX_IDLE_DISCONNECT;
 	}
 
@@ -562,7 +557,7 @@ static struct i2c_driver pca954x_driver = {
 	.driver		= {
 		.name	= "pca954x",
 		.pm	= &pca954x_pm,
-		.of_match_table = of_match_ptr(pca954x_of_match),
+		.of_match_table = pca954x_of_match,
 	},
 	.probe		= pca954x_probe,
 	.remove		= pca954x_remove,
-- 
2.25.1

Re: [PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Peter Rosin]

Hi!

On 2020-03-05 16:53, Andy Shevchenko wrote:
> Device property API allows to gather device resources from different sources,
> such as ACPI. Convert the drivers to unleash the power of device property API.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 39 +++++++++++++----------------
>  1 file changed, 17 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 819ff95e64ba..2e42a113ef1f 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -43,10 +43,8 @@
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> -#include <linux/of_device.h>
> -#include <linux/of_irq.h>
>  #include <linux/pm.h>
> +#include <linux/property.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <dt-bindings/mux/mux.h>
> @@ -182,23 +180,22 @@ static const struct chip_desc chips[] = {
>  };
>  
>  static const struct i2c_device_id pca954x_id[] = {
> -	{ "pca9540", pca_9540 },
> -	{ "pca9542", pca_9542 },
> -	{ "pca9543", pca_9543 },
> -	{ "pca9544", pca_9544 },
> -	{ "pca9545", pca_9545 },
> -	{ "pca9546", pca_9546 },
> -	{ "pca9547", pca_9547 },
> -	{ "pca9548", pca_9548 },
> -	{ "pca9846", pca_9846 },
> -	{ "pca9847", pca_9847 },
> -	{ "pca9848", pca_9848 },
> -	{ "pca9849", pca_9849 },
> +	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
> +	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
> +	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
> +	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
> +	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
> +	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
> +	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
> +	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
> +	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
> +	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
> +	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
> +	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },

It feels odd/wrong to specifically name .driver_data when .name is not there.
None or both...

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, pca954x_id);
>  
> -#ifdef CONFIG_OF
>  static const struct of_device_id pca954x_of_match[] = {
>  	{ .compatible = "nxp,pca9540", .data = &chips[pca_9540] },
>  	{ .compatible = "nxp,pca9542", .data = &chips[pca_9542] },
> @@ -215,7 +212,6 @@ static const struct of_device_id pca954x_of_match[] = {
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, pca954x_of_match);
> -#endif
>  
>  /* Write to mux register. Don't use i2c_transfer()/i2c_smbus_xfer()
>     for this as they will try to lock adapter a second time */
> @@ -426,7 +422,6 @@ static int pca954x_probe(struct i2c_client *client,
>  {
>  	struct i2c_adapter *adap = client->adapter;
>  	struct device *dev = &client->dev;
> -	struct device_node *np = dev->of_node;
>  	struct gpio_desc *gpio;
>  	struct i2c_mux_core *muxc;
>  	struct pca954x *data;
> @@ -456,7 +451,7 @@ static int pca954x_probe(struct i2c_client *client,
>  		udelay(1);
>  	}
>  
> -	data->chip = of_device_get_match_data(dev);
> +	data->chip = device_get_match_data(dev);
>  	if (!data->chip)
>  		data->chip = &chips[id->driver_data];

These two lines no longer make any sence.

Cheers,
Peter

>  
> @@ -478,8 +473,8 @@ static int pca954x_probe(struct i2c_client *client,
>  	}
>  
>  	data->idle_state = MUX_IDLE_AS_IS;
> -	if (of_property_read_u32(np, "idle-state", &data->idle_state)) {
> -		if (np && of_property_read_bool(np, "i2c-mux-idle-disconnect"))
> +	if (device_property_read_u32(dev, "idle-state", &data->idle_state)) {
> +		if (device_property_read_bool(dev, "i2c-mux-idle-disconnect"))
>  			data->idle_state = MUX_IDLE_DISCONNECT;
>  	}
>  
> @@ -562,7 +557,7 @@ static struct i2c_driver pca954x_driver = {
>  	.driver		= {
>  		.name	= "pca954x",
>  		.pm	= &pca954x_pm,
> -		.of_match_table = of_match_ptr(pca954x_of_match),
> +		.of_match_table = pca954x_of_match,
>  	},
>  	.probe		= pca954x_probe,
>  	.remove		= pca954x_remove,
> 

Re: [PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Andy Shevchenko]

On Thu, Mar 05, 2020 at 09:05:56PM +0000, Peter Rosin wrote:
> On 2020-03-05 16:53, Andy Shevchenko wrote:
> > Device property API allows to gather device resources from different sources,
> > such as ACPI. Convert the drivers to unleash the power of device property API.

...

> >  static const struct i2c_device_id pca954x_id[] = {
> > -	{ "pca9540", pca_9540 },
> > -	{ "pca9542", pca_9542 },
> > -	{ "pca9543", pca_9543 },
> > -	{ "pca9544", pca_9544 },
> > -	{ "pca9545", pca_9545 },
> > -	{ "pca9546", pca_9546 },
> > -	{ "pca9547", pca_9547 },
> > -	{ "pca9548", pca_9548 },
> > -	{ "pca9846", pca_9846 },
> > -	{ "pca9847", pca_9847 },
> > -	{ "pca9848", pca_9848 },
> > -	{ "pca9849", pca_9849 },
> > +	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
> > +	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
> > +	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
> > +	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
> > +	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
> > +	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
> > +	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
> > +	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
> > +	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
> > +	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
> > +	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
> > +	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },
> 
> It feels odd/wrong to specifically name .driver_data when .name is not there.
> None or both...

I will add .name as well.

> > +	data->chip = device_get_match_data(dev);
> >  	if (!data->chip)
> >  		data->chip = &chips[id->driver_data];
> 
> These two lines no longer make any sence.

Please elaborate.

IIRC Javier explained once that I²C ID table is still good to have to allow
enumeration from user space.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Peter Rosin]

On 2020-03-06 10:54, Andy Shevchenko wrote:
> On Thu, Mar 05, 2020 at 09:05:56PM +0000, Peter Rosin wrote:
>> On 2020-03-05 16:53, Andy Shevchenko wrote:
>>> Device property API allows to gather device resources from different sources,
>>> such as ACPI. Convert the drivers to unleash the power of device property API.
> 
> ...
> 
>>>  static const struct i2c_device_id pca954x_id[] = {
>>> -	{ "pca9540", pca_9540 },
>>> -	{ "pca9542", pca_9542 },
>>> -	{ "pca9543", pca_9543 },
>>> -	{ "pca9544", pca_9544 },
>>> -	{ "pca9545", pca_9545 },
>>> -	{ "pca9546", pca_9546 },
>>> -	{ "pca9547", pca_9547 },
>>> -	{ "pca9548", pca_9548 },
>>> -	{ "pca9846", pca_9846 },
>>> -	{ "pca9847", pca_9847 },
>>> -	{ "pca9848", pca_9848 },
>>> -	{ "pca9849", pca_9849 },
>>> +	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
>>> +	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
>>> +	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
>>> +	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
>>> +	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
>>> +	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
>>> +	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
>>> +	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
>>> +	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
>>> +	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
>>> +	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
>>> +	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },
>>
>> It feels odd/wrong to specifically name .driver_data when .name is not there.
>> None or both...
> 
> I will add .name as well.
> 
>>> +	data->chip = device_get_match_data(dev);
>>>  	if (!data->chip)
>>>  		data->chip = &chips[id->driver_data];
>>
>> These two lines no longer make any sence.
> 
> Please elaborate.
> 
> IIRC Javier explained once that I²C ID table is still good to have to allow
> enumeration from user space.

id->driver_data is no longer an integer index into chips[]. So, for the I2C
ID table case, either device_get_match_data returns the .driver_data as-is
from the pca954x_id array, or it returns NULL (I don't know which it is).
In the first case, if (!data->chip) ...; is useless dead code. In the latter
case, it should be

	if (!data->chip)
		data->chip = (whatever-type-chip-is *)id->driver_data;

(If it's this latter case, I get the feeling that changing the .driver_data
of pca954x_id is an orthogonal change that has little to do with using
device properties instead of of-specifics.)

Cheers,
Peter

Re: [PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Andy Shevchenko]

On Fri, Mar 06, 2020 at 12:48:14PM +0100, Peter Rosin wrote:
> On 2020-03-06 10:54, Andy Shevchenko wrote:
> > On Thu, Mar 05, 2020 at 09:05:56PM +0000, Peter Rosin wrote:
> >> On 2020-03-05 16:53, Andy Shevchenko wrote:
> >>> Device property API allows to gather device resources from different sources,
> >>> such as ACPI. Convert the drivers to unleash the power of device property API.
> > 
> > ...
> > 
> >>>  static const struct i2c_device_id pca954x_id[] = {
> >>> -	{ "pca9540", pca_9540 },
> >>> -	{ "pca9542", pca_9542 },
> >>> -	{ "pca9543", pca_9543 },
> >>> -	{ "pca9544", pca_9544 },
> >>> -	{ "pca9545", pca_9545 },
> >>> -	{ "pca9546", pca_9546 },
> >>> -	{ "pca9547", pca_9547 },
> >>> -	{ "pca9548", pca_9548 },
> >>> -	{ "pca9846", pca_9846 },
> >>> -	{ "pca9847", pca_9847 },
> >>> -	{ "pca9848", pca_9848 },
> >>> -	{ "pca9849", pca_9849 },
> >>> +	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
> >>> +	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
> >>> +	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
> >>> +	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
> >>> +	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
> >>> +	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
> >>> +	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
> >>> +	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
> >>> +	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
> >>> +	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
> >>> +	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
> >>> +	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },
> >>
> >> It feels odd/wrong to specifically name .driver_data when .name is not there.
> >> None or both...
> > 
> > I will add .name as well.
> > 
> >>> +	data->chip = device_get_match_data(dev);
> >>>  	if (!data->chip)
> >>>  		data->chip = &chips[id->driver_data];
> >>
> >> These two lines no longer make any sence.
> > 
> > Please elaborate.
> > 
> > IIRC Javier explained once that I²C ID table is still good to have to allow
> > enumeration from user space.
> 
> id->driver_data is no longer an integer index into chips[]. So, for the I2C
> ID table case, either device_get_match_data returns the .driver_data as-is
> from the pca954x_id array, or it returns NULL (I don't know which it is).

No, you took it wrong. device_get_match_data() operates with ACPI/DT tables.

> In the first case, if (!data->chip) ...; is useless dead code. In the latter
> case, it should be
> 
> 	if (!data->chip)
> 		data->chip = (whatever-type-chip-is *)id->driver_data;
> 
> (If it's this latter case, I get the feeling that changing the .driver_data
> of pca954x_id is an orthogonal change that has little to do with using
> device properties instead of of-specifics.)

But this comment is good, seems I missed that and actually change is not needed
indeed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Peter Rosin]

On 2020-03-06 14:58, Andy Shevchenko wrote:
> On Fri, Mar 06, 2020 at 12:48:14PM +0100, Peter Rosin wrote:
>> On 2020-03-06 10:54, Andy Shevchenko wrote:
>>> On Thu, Mar 05, 2020 at 09:05:56PM +0000, Peter Rosin wrote:
>>>> On 2020-03-05 16:53, Andy Shevchenko wrote:
>>>>> Device property API allows to gather device resources from different sources,
>>>>> such as ACPI. Convert the drivers to unleash the power of device property API.
>>>
>>> ...
>>>
>>>>>  static const struct i2c_device_id pca954x_id[] = {
>>>>> -	{ "pca9540", pca_9540 },
>>>>> -	{ "pca9542", pca_9542 },
>>>>> -	{ "pca9543", pca_9543 },
>>>>> -	{ "pca9544", pca_9544 },
>>>>> -	{ "pca9545", pca_9545 },
>>>>> -	{ "pca9546", pca_9546 },
>>>>> -	{ "pca9547", pca_9547 },
>>>>> -	{ "pca9548", pca_9548 },
>>>>> -	{ "pca9846", pca_9846 },
>>>>> -	{ "pca9847", pca_9847 },
>>>>> -	{ "pca9848", pca_9848 },
>>>>> -	{ "pca9849", pca_9849 },
>>>>> +	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
>>>>> +	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
>>>>> +	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
>>>>> +	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
>>>>> +	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
>>>>> +	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
>>>>> +	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
>>>>> +	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
>>>>> +	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
>>>>> +	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
>>>>> +	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
>>>>> +	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },
>>>>
>>>> It feels odd/wrong to specifically name .driver_data when .name is not there.
>>>> None or both...
>>>
>>> I will add .name as well.
>>>
>>>>> +	data->chip = device_get_match_data(dev);
>>>>>  	if (!data->chip)
>>>>>  		data->chip = &chips[id->driver_data];
>>>>
>>>> These two lines no longer make any sence.
>>>
>>> Please elaborate.
>>>
>>> IIRC Javier explained once that I²C ID table is still good to have to allow
>>> enumeration from user space.
>>
>> id->driver_data is no longer an integer index into chips[]. So, for the I2C
>> ID table case, either device_get_match_data returns the .driver_data as-is
>> from the pca954x_id array, or it returns NULL (I don't know which it is).
> 
> No, you took it wrong. device_get_match_data() operates with ACPI/DT tables.

<rant-mode>

What do you mean wrong? I said that either A or B holds but did not know which
(with these definitions):

A. device_get_match_data() digs in the i2c_device_id table and returns the
   .driver_data of the matching entry.
B. device_get_match_data() behaves as of_device_get_match_data() and does not
   dig in the i2c_device_id table, and therefore returns NULL when the driver
   is probed that way.

And that in either of these cases your patch made no sense.

At least that was what I tried to say, using less words...

And then, according to you, B holds. So, I was right: either A or B holds. BTW,
I obviously meant the either/or construct to be in the exclusive sense where
both cannot hold (but my statement is also correct in the inclusive-or sense).

I would only have been wrong if the correct description had been some third
option, which I had not mentioned. But that was apparently not the case.

Cheers,
Peter

> 
>> In the first case, if (!data->chip) ...; is useless dead code. In the latter
>> case, it should be
>>
>> 	if (!data->chip)
>> 		data->chip = (whatever-type-chip-is *)id->driver_data;
>>
>> (If it's this latter case, I get the feeling that changing the .driver_data
>> of pca954x_id is an orthogonal change that has little to do with using
>> device properties instead of of-specifics.)
> 
> But this comment is good, seems I missed that and actually change is not needed
> indeed.
> 

Re: [PATCH v1 2/5] i2c: mux: pca954x: Make use of device properties

[Andy Shevchenko]

On Fri, Mar 06, 2020 at 03:57:50PM +0100, Peter Rosin wrote:
> On 2020-03-06 14:58, Andy Shevchenko wrote:
> > On Fri, Mar 06, 2020 at 12:48:14PM +0100, Peter Rosin wrote:
> >> On 2020-03-06 10:54, Andy Shevchenko wrote:
> >>> On Thu, Mar 05, 2020 at 09:05:56PM +0000, Peter Rosin wrote:
> >>>> On 2020-03-05 16:53, Andy Shevchenko wrote:
> >>>>> Device property API allows to gather device resources from different sources,
> >>>>> such as ACPI. Convert the drivers to unleash the power of device property API.
> >>>
> >>> ...
> >>>
> >>>>>  static const struct i2c_device_id pca954x_id[] = {
> >>>>> -	{ "pca9540", pca_9540 },
> >>>>> -	{ "pca9542", pca_9542 },
> >>>>> -	{ "pca9543", pca_9543 },
> >>>>> -	{ "pca9544", pca_9544 },
> >>>>> -	{ "pca9545", pca_9545 },
> >>>>> -	{ "pca9546", pca_9546 },
> >>>>> -	{ "pca9547", pca_9547 },
> >>>>> -	{ "pca9548", pca_9548 },
> >>>>> -	{ "pca9846", pca_9846 },
> >>>>> -	{ "pca9847", pca_9847 },
> >>>>> -	{ "pca9848", pca_9848 },
> >>>>> -	{ "pca9849", pca_9849 },
> >>>>> +	{ "pca9540", .driver_data = (kernel_ulong_t)&chips[pca_9540] },
> >>>>> +	{ "pca9542", .driver_data = (kernel_ulong_t)&chips[pca_9542] },
> >>>>> +	{ "pca9543", .driver_data = (kernel_ulong_t)&chips[pca_9543] },
> >>>>> +	{ "pca9544", .driver_data = (kernel_ulong_t)&chips[pca_9544] },
> >>>>> +	{ "pca9545", .driver_data = (kernel_ulong_t)&chips[pca_9545] },
> >>>>> +	{ "pca9546", .driver_data = (kernel_ulong_t)&chips[pca_9546] },
> >>>>> +	{ "pca9547", .driver_data = (kernel_ulong_t)&chips[pca_9547] },
> >>>>> +	{ "pca9548", .driver_data = (kernel_ulong_t)&chips[pca_9548] },
> >>>>> +	{ "pca9846", .driver_data = (kernel_ulong_t)&chips[pca_9846] },
> >>>>> +	{ "pca9847", .driver_data = (kernel_ulong_t)&chips[pca_9847] },
> >>>>> +	{ "pca9848", .driver_data = (kernel_ulong_t)&chips[pca_9848] },
> >>>>> +	{ "pca9849", .driver_data = (kernel_ulong_t)&chips[pca_9849] },
> >>>>
> >>>> It feels odd/wrong to specifically name .driver_data when .name is not there.
> >>>> None or both...
> >>>
> >>> I will add .name as well.
> >>>
> >>>>> +	data->chip = device_get_match_data(dev);
> >>>>>  	if (!data->chip)
> >>>>>  		data->chip = &chips[id->driver_data];
> >>>>
> >>>> These two lines no longer make any sence.
> >>>
> >>> Please elaborate.
> >>>
> >>> IIRC Javier explained once that I²C ID table is still good to have to allow
> >>> enumeration from user space.
> >>
> >> id->driver_data is no longer an integer index into chips[].
>>> So, for the I2C
> >> ID table case, either device_get_match_data returns the .driver_data as-is
> >> from the pca954x_id array,
>>> or it returns NULL (I don't know which it is).
> > No, you took it wrong. device_get_match_data() operates with ACPI/DT tables.
> 
> <rant-mode>
> 
> What do you mean wrong? 

I meant that A is wrong.

> I said that either A or B holds but did not know which
> (with these definitions):
> 
> A. device_get_match_data() digs in the i2c_device_id table and returns the
>    .driver_data of the matching entry.
> B. device_get_match_data() behaves as of_device_get_match_data() and does not
>    dig in the i2c_device_id table, and therefore returns NULL when the driver
>    is probed that way.
> 
> And that in either of these cases your patch made no sense.
> 
> At least that was what I tried to say, using less words...
> 
> And then, according to you, B holds. So, I was right: either A or B holds. BTW,
> I obviously meant the either/or construct to be in the exclusive sense where
> both cannot hold (but my statement is also correct in the inclusive-or sense).
> 
> I would only have been wrong if the correct description had been some third
> option, which I had not mentioned. But that was apparently not the case.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 3/5] i2c: mux: pca954x: Drop useless validation for optional GPIO descriptor

[Andy Shevchenko]

There is no need to perform a check for optional GPIO descriptor.
If it's NULL, the API can handle it correctly.

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

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2e42a113ef1f..fe4320fc0b91 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -444,12 +444,12 @@ static int pca954x_probe(struct i2c_client *client,
 	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(gpio))
 		return PTR_ERR(gpio);
-	if (gpio) {
-		udelay(1);
-		gpiod_set_value_cansleep(gpio, 0);
-		/* Give the chip some time to recover. */
-		udelay(1);
-	}
+
+	/* Reset the multiplexer */
+	udelay(1);
+	gpiod_set_value_cansleep(gpio, 0);
+	/* Give the chip some time to recover. */
+	udelay(1);
 
 	data->chip = device_get_match_data(dev);
 	if (!data->chip)
-- 
2.25.1

Re: [PATCH v1 3/5] i2c: mux: pca954x: Drop useless validation for optional GPIO descriptor

[Peter Rosin]

Hi!

On 2020-03-05 16:53, Andy Shevchenko wrote:
> There is no need to perform a check for optional GPIO descriptor.
> If it's NULL, the API can handle it correctly.

But, the removed test is not only (needlessly) protecting the optional
descriptor, it also shortcuts the udelays. I think it is valid to
skip those pointless udelays when no reset is happening anyway. Not
that it matters all that much when the delays are as short as this, but
I simply think it looks sensible to skip the delays when that are not
needed.

So, I do not think this change is an improvement.

Cheers,
Peter

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 2e42a113ef1f..fe4320fc0b91 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -444,12 +444,12 @@ static int pca954x_probe(struct i2c_client *client,
>  	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(gpio))
>  		return PTR_ERR(gpio);
> -	if (gpio) {
> -		udelay(1);
> -		gpiod_set_value_cansleep(gpio, 0);
> -		/* Give the chip some time to recover. */
> -		udelay(1);
> -	}
> +
> +	/* Reset the multiplexer */
> +	udelay(1);
> +	gpiod_set_value_cansleep(gpio, 0);
> +	/* Give the chip some time to recover. */
> +	udelay(1);
>  
>  	data->chip = device_get_match_data(dev);
>  	if (!data->chip)
> 

Re: [PATCH v1 3/5] i2c: mux: pca954x: Drop useless validation for optional GPIO descriptor

[Andy Shevchenko]

On Thu, Mar 05, 2020 at 09:19:05PM +0000, Peter Rosin wrote:
> On 2020-03-05 16:53, Andy Shevchenko wrote:
> > There is no need to perform a check for optional GPIO descriptor.
> > If it's NULL, the API can handle it correctly.
> 
> But, the removed test is not only (needlessly) protecting the optional
> descriptor, it also shortcuts the udelays. I think it is valid to
> skip those pointless udelays when no reset is happening anyway. Not
> that it matters all that much when the delays are as short as this, but
> I simply think it looks sensible to skip the delays when that are not
> needed.

Perhaps moving it to a helper where we may leave the check.

> So, I do not think this change is an improvement.

I have no strong opinion, I simple will drop it then.
Thank you for review!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/5] i2c: mux: pca954x: Move device_remove_file() out of pca954x_cleanup()

[Andy Shevchenko]

device_create_file() is called the last in ->probe() but pca954x_cleanup(),
which is called earlier in error path, tries to remove never created file.
Move device_remove_file() call outside of pca954x_cleanup() to make it
slightly closer to what pca954x_init() is doing.

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

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index fe4320fc0b91..2f8f034040cb 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -383,11 +383,8 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
 static void pca954x_cleanup(struct i2c_mux_core *muxc)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
-	struct i2c_client *client = data->client;
 	int c, irq;
 
-	device_remove_file(&client->dev, &dev_attr_idle_state);
-
 	if (data->irq) {
 		for (c = 0; c < data->chip->nchans; c++) {
 			irq = irq_find_mapping(data->irq, c);
@@ -531,6 +528,8 @@ static int pca954x_remove(struct i2c_client *client)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
 
+	device_remove_file(&client->dev, &dev_attr_idle_state);
+
 	pca954x_cleanup(muxc);
 	return 0;
 }
-- 
2.25.1

[PATCH v1 5/5] i2c: mux: pca954x: Convert license to SPDX identifier

[Andy Shevchenko]

Use a shorter SPDX identifier instead of pasting the whole license.

While here, replace duplicating PCA954x with PCA984x.

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

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 2f8f034040cb..26e340931ab8 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -1,10 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * I2C multiplexer
  *
  * Copyright (c) 2008-2009 Rodolfo Giometti <giometti@linux.it>
  * Copyright (c) 2008-2009 Eurotech S.p.A. <info@eurotech.it>
  *
- * This module supports the PCA954x and PCA954x series of I2C multiplexer/switch
+ * This module supports the PCA954x and PCA984x series of I2C multiplexer/switch
  * chips made by NXP Semiconductors.
  * This includes the:
  *	 PCA9540, PCA9542, PCA9543, PCA9544, PCA9545, PCA9546, PCA9547,
@@ -29,10 +30,6 @@
  *	i2c-virtual_cb.c from Brian Kuschak <bkuschak@yahoo.com>
  * and
  *	pca9540.c from Jean Delvare <jdelvare@suse.de>.
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
  */
 
 #include <linux/device.h>
-- 
2.25.1

Re: [PATCH v1 1/5] i2c: mux: pca954x: Refactor pca954x_irq_handler()

[Peter Rosin]

Hi!

On 2020-03-05 16:53, Andy Shevchenko wrote:
> Refactor pca954x_irq_handler() to:
>   - use for_each_set_bit() macro
>   - use IRQ_RETVAL() macro
> 
> Above change makes code easy to read and understand.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index a0d926ae3f86..819ff95e64ba 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -327,21 +327,18 @@ static DEVICE_ATTR_RW(idle_state);
>  static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
>  {
>  	struct pca954x *data = dev_id;
> -	unsigned int child_irq;
> -	int ret, i, handled = 0;
> +	unsigned long pending;
> +	int ret, i;
>  
>  	ret = i2c_smbus_read_byte(data->client);
>  	if (ret < 0)
>  		return IRQ_NONE;
>  
> -	for (i = 0; i < data->chip->nchans; i++) {
> -		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
> -			child_irq = irq_linear_revmap(data->irq, i);
> -			handle_nested_irq(child_irq);
> -			handled++;
> -		}
> -	}
> -	return handled ? IRQ_HANDLED : IRQ_NONE;
> +	pending = ret >> PCA954X_IRQ_OFFSET;
> +	for_each_set_bit(i, &pending, data->chip->nchans)
> +		handle_nested_irq(irq_linear_revmap(data->irq, i));
> +
> +	return IRQ_RETVAL(pending);

What if ret has some bit set above the bit corresponding to the last channel?

Maybe that's somehow not possible, but if that's the case it's not apparent.

Cheers,
Peter

>  }
>  
>  static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> 

Re: [PATCH v1 1/5] i2c: mux: pca954x: Refactor pca954x_irq_handler()

[Andy Shevchenko]

On Thu, Mar 05, 2020 at 09:34:46PM +0000, Peter Rosin wrote:
> On 2020-03-05 16:53, Andy Shevchenko wrote:
> > Refactor pca954x_irq_handler() to:
> >   - use for_each_set_bit() macro
> >   - use IRQ_RETVAL() macro
> > 
> > Above change makes code easy to read and understand.

> > +	pending = ret >> PCA954X_IRQ_OFFSET;
> > +	for_each_set_bit(i, &pending, data->chip->nchans)
> > +		handle_nested_irq(irq_linear_revmap(data->irq, i));
> > +
> > +	return IRQ_RETVAL(pending);
> 
> What if ret has some bit set above the bit corresponding to the last channel?

We will "handle" spurious interrupt.

> Maybe that's somehow not possible, but if that's the case it's not apparent.

So, does

	pending = (ret >> PCA954X_IRQ_OFFSET) & (BIT(data->chip->nchans) - 1);

satisfy you?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] i2c: mux: pca954x: Refactor pca954x_irq_handler()

[Peter Rosin]

On 2020-03-06 11:02, Andy Shevchenko wrote:
> On Thu, Mar 05, 2020 at 09:34:46PM +0000, Peter Rosin wrote:
>> On 2020-03-05 16:53, Andy Shevchenko wrote:
>>> Refactor pca954x_irq_handler() to:
>>>   - use for_each_set_bit() macro
>>>   - use IRQ_RETVAL() macro
>>>
>>> Above change makes code easy to read and understand.
> 
>>> +	pending = ret >> PCA954X_IRQ_OFFSET;
>>> +	for_each_set_bit(i, &pending, data->chip->nchans)
>>> +		handle_nested_irq(irq_linear_revmap(data->irq, i));
>>> +
>>> +	return IRQ_RETVAL(pending);
>>
>> What if ret has some bit set above the bit corresponding to the last channel?
> 
> We will "handle" spurious interrupt.
> 
>> Maybe that's somehow not possible, but if that's the case it's not apparent.
> 
> So, does
> 
> 	pending = (ret >> PCA954X_IRQ_OFFSET) & (BIT(data->chip->nchans) - 1);
> 
> satisfy you?
> 

Yep, that's safer. Thanks!

Cheers,
Peter

Re: [PATCH v1 1/5] i2c: mux: pca954x: Refactor pca954x_irq_handler()

[Andy Shevchenko]

On Fri, Mar 06, 2020 at 11:58:47AM +0100, Peter Rosin wrote:
> On 2020-03-06 11:02, Andy Shevchenko wrote:

> Yep, that's safer. Thanks!

Thank you for review, I just sent v2.

-- 
With Best Regards,
Andy Shevchenko