Am 21.08.2017 um 23:28 schrieb Peter Rosin:
> On 2017-08-21 21:56, Heiner Kallweit wrote:
>> Am 21.08.2017 um 20:42 schrieb Peter Rosin:
>>> Hi!
>>>
>>> On 2017-08-21 20:24, Heiner Kallweit wrote:
>>>> i2c_new_dummy is typically called from the probe function of the
>>>> driver for the primary i2c client. It requires calls to
>>>> i2c_unregister_device in the error path of the probe function and
>>>> in the remove function.
>>>> This can be simplified by introducing a device-managed version.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>>>> ---
>>>> drivers/i2c/i2c-core-base.c | 29 +++++++++++++++++++++++++++++
>>>> include/linux/i2c.h | 3 +++
>>>> 2 files changed, 32 insertions(+)
>>>
>>> You need to update Documentation/driver-model/devres.txt
>>>
>> Right, thanks for the hint.
>>
>>>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>>>> index 12822a4b..7eb48328 100644
>>>> --- a/drivers/i2c/i2c-core-base.c
>>>> +++ b/drivers/i2c/i2c-core-base.c
>>>> @@ -868,6 +868,35 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
>>>> }
>>>> EXPORT_SYMBOL_GPL(i2c_new_dummy);
>>>>
>>>> +static void devm_i2c_release_dummy(struct device *dev, void *res)
>>>> +{
>>>> + i2c_unregister_device(*(struct i2c_client **)res);
>>>> +}
>>>> +
>>>> +/* managed version of i2c_new_dummy */
>>>
>>> A kernel-doc comment is preferred for public functions. Especially new
>>> functions...
>>>
>> OK
>>
>>>> +struct i2c_client *devm_i2c_new_dummy(struct device *dev,
>>>> + struct i2c_adapter *adapter,
>>>> + u16 address)
>>>> +{
>>>> + struct i2c_client *ret, **dr;
>>>> +
>>>> + dr = devres_alloc(devm_i2c_release_dummy, sizeof(*dr), GFP_KERNEL);
>>>> + if (!dr)
>>>> + return NULL;
>>>
>>> Would it not be splendid if you returned an error pointer
>>> instead, preferably you should also change the i2c_new_dummy
>>> interface (don't forget to update callers) to return such a
>>> thing instead of NULL on error so that -EBUSY etc is
>>> propagated? But there are a few dozen callers, I don't know
>>> if it's worth it at this point?
>>>
>> i2c_new_dummy does little more than calling i2c_new_device
>> which returns NULL in case of an error. So we would have to
>> change i2c_new_device too. This may be a little too ambitious.
>
> Yes and no.
>
> Yes, it is ambitious, but the right thing to do in the end. And
> it is mechanical, so it is not really hard. But I already said that
> it might not be worth it at this point...
>
> No, we do not have to follow the return value convention from
> i2c_new_dummy in devm_i2c_new_dummy. What I meant below was that
> for the time being devm_i2c_new_dummy can return ERR_PTR(-Ewhatever)
> when i2c_new_dummy returns NULL. Then, when most/all i2c_new_dummy
> users have been converted over to devm_i2c_new_dummy (which might
> take a while) it will be less daunting to convert i2c_new_dummy to
> not return a plain old boring NULL on failure. And then it will be
> really easy to change devm_i2c_new_dummy to propagate that value w/o
> causing massive churn in all callers at once.
>
I see, yes this would make sense IMHO.
>> When talking about changing the interface:
>> devm_i2c_new_dummy usually will be called with parameters
>>
>> dev = &client->dev and
>> adapter = client->adapter
>>
>> So we could change and simplify it to
>> struct i2c_client *devm_i2c_new_dummy(struct i2c_client *client, u16 address)
>>
>> However maintainers usually insist on first parameter of a devm_
>> function has to be a struct device *, although there are existing
>> devm_ functions like devm_regmap_init_i2c not following this rule.
>>
>> I'd appreciate feedback whether it would be acceptable in our case.
>
> It would probably be good to target that question with an audience
> a bit wider than the i2c list. Not sure where exactly it fits best
> though?
>
I think for now I will skip this as there are few users of
i2c_new_dummy using it with the i2c client being provided by a
parent device. Example: drivers/rtc/rtc-s5m.c
Rgds, Heiner
> Cheers,
> peda
>
>>> Anyway, if we do it right for the managed function from the
>>> start, that count of i2c_new_dummy callers might get more
>>> manageable in the future?
>>>
>>> Cheers,
>>> peda
>>>
>>>> +
>>>> + ret = i2c_new_dummy(adapter, address);
>>>> + if (!ret) {
>>>> + devres_free(dr);
>>>> + return NULL;
>>>> + }
>>>> +
>>>> + *dr = ret;
>>>> + devres_add(dev, dr);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(devm_i2c_new_dummy);
>>>> +
>>>> /**
>>>> * i2c_new_secondary_device - Helper to get the instantiated secondary address
>>>> * and create the associated device
>>>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>>>> index d501d395..d0c091e6 100644
>>>> --- a/include/linux/i2c.h
>>>> +++ b/include/linux/i2c.h
>>>> @@ -381,6 +381,9 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
>>>> extern struct i2c_client *
>>>> i2c_new_dummy(struct i2c_adapter *adap, u16 address);
>>>>
>>>> +extern struct i2c_client *
>>>> +devm_i2c_new_dummy(struct device *dev, struct i2c_adapter *adap, u16 address);
>>>> +
>>>> extern struct i2c_client *
>>>> i2c_new_secondary_device(struct i2c_client *client,
>>>> const char *name,
>>>>
>>>
>>>
>>
>
>
