Re: [PATCH v7 05/10] i2c: core: call of_i2c_setup_smbus_alert in i2c_register_adapter

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

On Jun 28 2017 or thereabouts, Phil Reid wrote:
> On 23/06/2017 20:19, Benjamin Tissoires wrote:
> > On Jun 19 2017 or thereabouts, Wolfram Sang wrote:
> > > On Thu, Jun 15, 2017 at 09:59:33PM +0800, Phil Reid wrote:
> > > > Add a call to of_i2c_setup_smbus_alert when a i2c adapter is registered
> > > > so the the smbalert driver can be registered.
> > > > 
> > > > Signed-off-by: Phil Reid <preid@electromag.com.au>
> > > 
> > > CCing Benjamin
> > > 
> > > > ---
> > > >   drivers/i2c/i2c-core.c | 4 ++++
> > > >   1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > > > index d2402bb..626471b 100644
> > > > --- a/drivers/i2c/i2c-core.c
> > > > +++ b/drivers/i2c/i2c-core.c
> > > > @@ -40,6 +40,7 @@
> > > >   #include <linux/gpio.h>
> > > >   #include <linux/hardirq.h>
> > > >   #include <linux/i2c.h>
> > > > +#include <linux/i2c-smbus.h>
> > > >   #include <linux/idr.h>
> > > >   #include <linux/init.h>
> > > >   #include <linux/irqflags.h>
> > > > @@ -2045,6 +2046,9 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
> > > >   		dev_warn(&adap->dev,
> > > >   			 "Failed to create compatibility class link\n");
> > > >   #endif
> > > > +	res = of_i2c_setup_smbus_alert(adap);
> > > > +	if (res)
> > > > +		goto out_list;
> > 
> > See my concerns in patch 4/10.
> > 
> > In addition, shouldn't this be placed before device_register() for the
> > least? pm_runtime_enable() would require a matching pm_runtime_disable(),
> > and device_register() some unregistering behavior too.
> > 
> 
> G'day Ben,
> 
> Thanks for the review.
> Yes this makes sense. I tried having it before the device_register and I get an error
> about a kobject not being initialised in the a call from of_i2c_setup_smbus_alert.
> 
> Having a look at what I'm doing in of_i2c_setup_smbus_alert now I'm not sure there's
> a need to bail out on an error now. Originally I was registering the irq in the setup call.
> Which need to handle probe defer. Now this should be handled in the alert probe call.
> 
> WDYR?

Well, of_i2c_setup_smbus_alert() returns an error code, so it has to be
accounted for. If the error is not a reason to fail, you should at least
shout some error messages and act accordingly.

However, looking at of_i2c_setup_smbus_alert() again, I wonder if we
should not split it in 2: one part that checks for the OF flag presence
and bail out early if an error is encountered (before device
registration), and one part once the device is registered that calls
i2c_setup_smbus_alert(). In case of a failure here, we need to
unregister the adapter because we don't have all the elements at hands
(assuming we consider that smbus-alert should be mandatory).

Cheers,
Benjamin