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

[Phil Reid <preid@electromag.com.au>]

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?



-- 
Regards
Phil Reid