[v2,1/1] pinctlr: mcp23s08: Fix add_data and irqchip_add_nested call order
diff mbox series

Message ID 1560306258-54654-1-git-send-email-preid@electromag.com.au
State New
Headers show
Series
  • [v2,1/1] pinctlr: mcp23s08: Fix add_data and irqchip_add_nested call order
Related show

Commit Message

Phil Reid June 12, 2019, 2:24 a.m. UTC
Currently probing of the mcp23s08 results in an error message
"detected irqchip that is shared with multiple gpiochips:
please fix the driver"

This is due to the following:

Call to mcp23s08_irqchip_setup() with call hierarchy:
mcp23s08_irqchip_setup()
  gpiochip_irqchip_add_nested()
    gpiochip_irqchip_add_key()
      gpiochip_set_irq_hooks()

Call to devm_gpiochip_add_data() with call hierarchy:
devm_gpiochip_add_data()
  gpiochip_add_data_with_key()
    gpiochip_add_irqchip()
      gpiochip_set_irq_hooks()

The gpiochip_add_irqchip() returns immediately if there isn't a irqchip
but we added a irqchip due to the previous mcp23s08_irqchip_setup()
call. So it calls gpiochip_set_irq_hooks() a second time.

Fix this by moving the call to devm_gpiochip_add_data before
the call to mcp23s08_irqchip_setup

Suggested-by: Marco Felsch <m.felsch@pengutronix.de>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---

Notes:
    v2:
    - remove unrelated whitespace changes

 drivers/pinctrl/pinctrl-mcp23s08.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Marco Felsch June 12, 2019, 8:32 a.m. UTC | #1
Hi Phil,

thanks for the patch. Can you check that the error which should be fixed
by commit 02e389e6 ("pinctrl: mcp23s08: fix irq setup order") do not
appear. If so we should also add a Fixes line.

Regards,
  Marco

On 19-06-12 10:24, Phil Reid wrote:
> Currently probing of the mcp23s08 results in an error message
> "detected irqchip that is shared with multiple gpiochips:
> please fix the driver"
> 
> This is due to the following:
> 
> Call to mcp23s08_irqchip_setup() with call hierarchy:
> mcp23s08_irqchip_setup()
>   gpiochip_irqchip_add_nested()
>     gpiochip_irqchip_add_key()
>       gpiochip_set_irq_hooks()
> 
> Call to devm_gpiochip_add_data() with call hierarchy:
> devm_gpiochip_add_data()
>   gpiochip_add_data_with_key()
>     gpiochip_add_irqchip()
>       gpiochip_set_irq_hooks()
> 
> The gpiochip_add_irqchip() returns immediately if there isn't a irqchip
> but we added a irqchip due to the previous mcp23s08_irqchip_setup()
> call. So it calls gpiochip_set_irq_hooks() a second time.
> 
> Fix this by moving the call to devm_gpiochip_add_data before
> the call to mcp23s08_irqchip_setup
> 
> Suggested-by: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
> 
> Notes:
>     v2:
>     - remove unrelated whitespace changes
> 
>  drivers/pinctrl/pinctrl-mcp23s08.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 5d7a851..b727de56 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -881,6 +881,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  	if (ret < 0)
>  		goto fail;
>  
> +	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
> +	if (ret < 0)
> +		goto fail;
> +
>  	mcp->irq_controller =
>  		device_property_read_bool(dev, "interrupt-controller");
>  	if (mcp->irq && mcp->irq_controller) {
> @@ -922,10 +926,6 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>  			goto fail;
>  	}
>  
> -	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
> -	if (ret < 0)
> -		goto fail;
> -
>  	if (one_regmap_config) {
>  		mcp->pinctrl_desc.name = devm_kasprintf(dev, GFP_KERNEL,
>  				"mcp23xxx-pinctrl.%d", raw_chip_address);
> -- 
> 1.8.3.1
> 
>
Phil Reid June 12, 2019, 9:16 a.m. UTC | #2
On 12/06/2019 16:32, Marco Felsch wrote:
> Hi Phil,
> 
> thanks for the patch. Can you check that the error which should be fixed
> by commit 02e389e6 ("pinctrl: mcp23s08: fix irq setup order") do not
> appear. If so we should also add a Fixes line.
> 
G'day Marco,

I remember that one know.
I'm also using the mcp with gpio-keys driver.
I don't think I saw the same behaviour with my setup then.

I'm using the (spi) mcp23s16 (with gpio-keys), and Dmitry was using mcp23008 (i2c).

I noted at the time the difference in when
i2c_set_clientdata & spi_set_drvdata are called in the spi / i2c probe paths.

It seems wrong to call i2c_set_clientdata after devm_pinctrl_register is called.
But I'm by no means an expert.

I do have a system with an i2c variant now, but it doesn't use the gpio-keys driver.

Anyways I'm still not seeing any adverse behaviour with the patch so far.



> Regards,
>    Marco
> 
> On 19-06-12 10:24, Phil Reid wrote:
>> Currently probing of the mcp23s08 results in an error message
>> "detected irqchip that is shared with multiple gpiochips:
>> please fix the driver"
>>
>> This is due to the following:
>>
>> Call to mcp23s08_irqchip_setup() with call hierarchy:
>> mcp23s08_irqchip_setup()
>>    gpiochip_irqchip_add_nested()
>>      gpiochip_irqchip_add_key()
>>        gpiochip_set_irq_hooks()
>>
>> Call to devm_gpiochip_add_data() with call hierarchy:
>> devm_gpiochip_add_data()
>>    gpiochip_add_data_with_key()
>>      gpiochip_add_irqchip()
>>        gpiochip_set_irq_hooks()
>>
>> The gpiochip_add_irqchip() returns immediately if there isn't a irqchip
>> but we added a irqchip due to the previous mcp23s08_irqchip_setup()
>> call. So it calls gpiochip_set_irq_hooks() a second time.
>>
>> Fix this by moving the call to devm_gpiochip_add_data before
>> the call to mcp23s08_irqchip_setup
>>
>> Suggested-by: Marco Felsch <m.felsch@pengutronix.de>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>
>> Notes:
>>      v2:
>>      - remove unrelated whitespace changes
>>
>>   drivers/pinctrl/pinctrl-mcp23s08.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
>> index 5d7a851..b727de56 100644
>> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
>> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
>> @@ -881,6 +881,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>   	if (ret < 0)
>>   		goto fail;
>>   
>> +	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
>> +	if (ret < 0)
>> +		goto fail;
>> +
>>   	mcp->irq_controller =
>>   		device_property_read_bool(dev, "interrupt-controller");
>>   	if (mcp->irq && mcp->irq_controller) {
>> @@ -922,10 +926,6 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>   			goto fail;
>>   	}
>>   
>> -	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
>> -	if (ret < 0)
>> -		goto fail;
>> -
>>   	if (one_regmap_config) {
>>   		mcp->pinctrl_desc.name = devm_kasprintf(dev, GFP_KERNEL,
>>   				"mcp23xxx-pinctrl.%d", raw_chip_address);
>> -- 
>> 1.8.3.1
>>
>>
>
Marco Felsch June 12, 2019, 9:29 a.m. UTC | #3
On 19-06-12 17:16, Phil Reid wrote:
> On 12/06/2019 16:32, Marco Felsch wrote:
> > Hi Phil,
> > 
> > thanks for the patch. Can you check that the error which should be fixed
> > by commit 02e389e6 ("pinctrl: mcp23s08: fix irq setup order") do not
> > appear. If so we should also add a Fixes line.
> > 
> G'day Marco,
> 
> I remember that one know.
> I'm also using the mcp with gpio-keys driver.
> I don't think I saw the same behaviour with my setup then.
> 
> I'm using the (spi) mcp23s16 (with gpio-keys), and Dmitry was using mcp23008 (i2c).
> 
> I noted at the time the difference in when
> i2c_set_clientdata & spi_set_drvdata are called in the spi / i2c probe paths.
> 
> It seems wrong to call i2c_set_clientdata after devm_pinctrl_register is called.
> But I'm by no means an expert.
> 
> I do have a system with an i2c variant now, but it doesn't use the gpio-keys driver.
> 
> Anyways I'm still not seeing any adverse behaviour with the patch so far.

Thanks for testing that, can you add a fixes line?

Regards,
  Marco

> 
> > Regards,
> >    Marco
> > 
> > On 19-06-12 10:24, Phil Reid wrote:
> > > Currently probing of the mcp23s08 results in an error message
> > > "detected irqchip that is shared with multiple gpiochips:
> > > please fix the driver"
> > > 
> > > This is due to the following:
> > > 
> > > Call to mcp23s08_irqchip_setup() with call hierarchy:
> > > mcp23s08_irqchip_setup()
> > >    gpiochip_irqchip_add_nested()
> > >      gpiochip_irqchip_add_key()
> > >        gpiochip_set_irq_hooks()
> > > 
> > > Call to devm_gpiochip_add_data() with call hierarchy:
> > > devm_gpiochip_add_data()
> > >    gpiochip_add_data_with_key()
> > >      gpiochip_add_irqchip()
> > >        gpiochip_set_irq_hooks()
> > > 
> > > The gpiochip_add_irqchip() returns immediately if there isn't a irqchip
> > > but we added a irqchip due to the previous mcp23s08_irqchip_setup()
> > > call. So it calls gpiochip_set_irq_hooks() a second time.
> > > 
> > > Fix this by moving the call to devm_gpiochip_add_data before
> > > the call to mcp23s08_irqchip_setup
> > > 
> > > Suggested-by: Marco Felsch <m.felsch@pengutronix.de>
> > > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > > ---
> > > 
> > > Notes:
> > >      v2:
> > >      - remove unrelated whitespace changes
> > > 
> > >   drivers/pinctrl/pinctrl-mcp23s08.c | 8 ++++----
> > >   1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> > > index 5d7a851..b727de56 100644
> > > --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> > > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> > > @@ -881,6 +881,10 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> > >   	if (ret < 0)
> > >   		goto fail;
> > > +	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
> > > +	if (ret < 0)
> > > +		goto fail;
> > > +
> > >   	mcp->irq_controller =
> > >   		device_property_read_bool(dev, "interrupt-controller");
> > >   	if (mcp->irq && mcp->irq_controller) {
> > > @@ -922,10 +926,6 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> > >   			goto fail;
> > >   	}
> > > -	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
> > > -	if (ret < 0)
> > > -		goto fail;
> > > -
> > >   	if (one_regmap_config) {
> > >   		mcp->pinctrl_desc.name = devm_kasprintf(dev, GFP_KERNEL,
> > >   				"mcp23xxx-pinctrl.%d", raw_chip_address);
> > > -- 
> > > 1.8.3.1
> > > 
> > > 
> > 
> 
> 
> -- 
> Regards
> Phil Reid
> 
> ElectroMagnetic Imaging Technology Pty Ltd
> Development of Geophysical Instrumentation & Software
> www.electromag.com.au
> 
> 3 The Avenue, Midland WA 6056, AUSTRALIA
> Ph: +61 8 9250 8100
> Fax: +61 8 9250 7100
> Email: preid@electromag.com.au
>
Linus Walleij June 18, 2019, 11:33 a.m. UTC | #4
On Wed, Jun 12, 2019 at 4:24 AM Phil Reid <preid@electromag.com.au> wrote:

> Currently probing of the mcp23s08 results in an error message
> "detected irqchip that is shared with multiple gpiochips:
> please fix the driver"
>
> This is due to the following:
>
> Call to mcp23s08_irqchip_setup() with call hierarchy:
> mcp23s08_irqchip_setup()
>   gpiochip_irqchip_add_nested()
>     gpiochip_irqchip_add_key()
>       gpiochip_set_irq_hooks()
>
> Call to devm_gpiochip_add_data() with call hierarchy:
> devm_gpiochip_add_data()
>   gpiochip_add_data_with_key()
>     gpiochip_add_irqchip()
>       gpiochip_set_irq_hooks()
>
> The gpiochip_add_irqchip() returns immediately if there isn't a irqchip
> but we added a irqchip due to the previous mcp23s08_irqchip_setup()
> call. So it calls gpiochip_set_irq_hooks() a second time.
>
> Fix this by moving the call to devm_gpiochip_add_data before
> the call to mcp23s08_irqchip_setup
>
> Suggested-by: Marco Felsch <m.felsch@pengutronix.de>
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>
> Notes:
>     v2:
>     - remove unrelated whitespace changes

I applied this for fixes some days back, seems my mail about
it didn't make it out.

Yours,
Linus Walleij

Patch
diff mbox series

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 5d7a851..b727de56 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -881,6 +881,10 @@  static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 	if (ret < 0)
 		goto fail;
 
+	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
+	if (ret < 0)
+		goto fail;
+
 	mcp->irq_controller =
 		device_property_read_bool(dev, "interrupt-controller");
 	if (mcp->irq && mcp->irq_controller) {
@@ -922,10 +926,6 @@  static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
 			goto fail;
 	}
 
-	ret = devm_gpiochip_add_data(dev, &mcp->chip, mcp);
-	if (ret < 0)
-		goto fail;
-
 	if (one_regmap_config) {
 		mcp->pinctrl_desc.name = devm_kasprintf(dev, GFP_KERNEL,
 				"mcp23xxx-pinctrl.%d", raw_chip_address);