Re: [PATCH v2 1/1] pinctlr: mcp23s08: Fix add_data and irqchip_add_nested call order

[Marco Felsch <m.felsch@xxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxx>
> > > Signed-off-by: Phil Reid <preid@xxxxxxxxxxxxxxxxx>
> > > ---
> > > 
> > > 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@xxxxxxxxxxxxxxxxx
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |