diff mbox

[v3,1/4] hwmon: iio_hwmon: delay probing with late_initcall

Message ID 5277864.qlyRKbC1Zy@ws-stein (mailing list archive)
State New, archived
Headers show

Commit Message

Alexander Stein July 26, 2016, 9:05 a.m. UTC
On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> On 26/07/2016 10:21, Alexander Stein wrote:
> > On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >> and
> >> properties in the Device Tree or channels whose consumer_dev_name matches
> >> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >> which might be probed after iio_hwmon.
> > 
> > Would it work if iio_channel_get_all returning ENODEV is used for
> > returning
> > EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> > driver/device
> > dependencies seems not right for me at this place.
> 
> Then what if the iio_channel_get_all is called outside of the probe of a
> driver? We'll have to change the error code, things we are apparently
> trying to avoid (see v2 patches' discussions).

Maybe I didn't express my idea enough. I don't want to change the behavior of  
iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
in iio_hwmon_probe. I have something link the patch below in mind.

Best regards,
Alexander
---

Comments

Quentin Schulz July 26, 2016, 9:33 a.m. UTC | #1
On 26/07/2016 11:05, Alexander Stein wrote:
> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>> On 26/07/2016 10:21, Alexander Stein wrote:
>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
>>>> and
>>>> properties in the Device Tree or channels whose consumer_dev_name matches
>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>>> which might be probed after iio_hwmon.
>>>
>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>> returning
>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>> driver/device
>>> dependencies seems not right for me at this place.
>>
>> Then what if the iio_channel_get_all is called outside of the probe of a
>> driver? We'll have to change the error code, things we are apparently
>> trying to avoid (see v2 patches' discussions).
> 
> Maybe I didn't express my idea enough. I don't want to change the behavior of  
> iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
> in iio_hwmon_probe. I have something link the patch below in mind.
> 
> Best regards,
> Alexander
> ---
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index b550ba5..e32d150 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>                 name = dev->of_node->name;
>  
>         channels = iio_channel_get_all(dev);
> -       if (IS_ERR(channels))
> -               return PTR_ERR(channels);
> +       if (IS_ERR(channels)) {
> +               if (PTR_ERR(channels) == -ENODEV)
> +                       return -EPROBE_DEFER;
> +               else
> +                       return PTR_ERR(channels);
> +       }
>  
>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>         if (st == NULL) {

Indeed, I misunderstood what you told me.

Actually, the patch you proposed is part of my v1
(https://lkml.org/lkml/2016/6/28/203) and v2
(https://lkml.org/lkml/2016/7/15/215).
Jonathan and Guenter didn't really like the idea of changing the -ENODEV
in -EPROBE_DEFER.

What I thought you were proposing was to change the -ENODEV return code
inside iio_channel_get_all. This cannot be an option since the function
might be called outside of a probe (it is not yet, but might be in the
future?).

Of what I understood, two possibilities are then possible (proposed
either by Guenter or Jonathan): either rework the iio framework to
register iio map array earlier or to use late_initcall instead of init
for the driver consuming the iio channels.

Thanks,
Quentin
Alexander Stein July 26, 2016, 10 a.m. UTC | #2
On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
> > On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> >> On 26/07/2016 10:21, Alexander Stein wrote:
> >>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >>>> and
> >>>> properties in the Device Tree or channels whose consumer_dev_name
> >>>> matches
> >>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >>>> which might be probed after iio_hwmon.
> >>> 
> >>> Would it work if iio_channel_get_all returning ENODEV is used for
> >>> returning
> >>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> >>> driver/device
> >>> dependencies seems not right for me at this place.
> >> 
> >> Then what if the iio_channel_get_all is called outside of the probe of a
> >> driver? We'll have to change the error code, things we are apparently
> >> trying to avoid (see v2 patches' discussions).
> > 
> > Maybe I didn't express my idea enough. I don't want to change the behavior
> > of iio_channel_get_all at all. Just the result evaluation of
> > iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> > below in mind.
> > 
> > Best regards,
> > Alexander
> > ---
> > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > index b550ba5..e32d150 100644
> > --- a/drivers/hwmon/iio_hwmon.c
> > +++ b/drivers/hwmon/iio_hwmon.c
> > @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> > *pdev)
> > 
> >                 name = dev->of_node->name;
> >         
> >         channels = iio_channel_get_all(dev);
> > 
> > -       if (IS_ERR(channels))
> > -               return PTR_ERR(channels);
> > +       if (IS_ERR(channels)) {
> > +               if (PTR_ERR(channels) == -ENODEV)
> > +                       return -EPROBE_DEFER;
> > +               else
> > +                       return PTR_ERR(channels);
> > +       }
> > 
> >         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> >         if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.

Thanks for the links.

> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).

AFAICS this is a helper function not knowing about device probing itself. And 
it should stay at that.

> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.

Interestingly using this problem would not arise due to module dependencies. 
But using late_initcall would mean this needs to be done on any driver using 
iio channels? I would rather keep those consumers simple.

Best regards,
Alexander
Quentin Schulz July 26, 2016, 10:07 a.m. UTC | #3
On 26/07/2016 12:00, Alexander Stein wrote:
> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>> On 26/07/2016 11:05, Alexander Stein wrote:
>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
>>>>>> and
>>>>>> properties in the Device Tree or channels whose consumer_dev_name
>>>>>> matches
>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>>>>> which might be probed after iio_hwmon.
>>>>>
>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>>> returning
>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>> driver/device
>>>>> dependencies seems not right for me at this place.
>>>>
>>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>>> driver? We'll have to change the error code, things we are apparently
>>>> trying to avoid (see v2 patches' discussions).
>>>
>>> Maybe I didn't express my idea enough. I don't want to change the behavior
>>> of iio_channel_get_all at all. Just the result evaluation of
>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
>>> below in mind.
>>>
>>> Best regards,
>>> Alexander
>>> ---
>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>> index b550ba5..e32d150 100644
>>> --- a/drivers/hwmon/iio_hwmon.c
>>> +++ b/drivers/hwmon/iio_hwmon.c
>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
>>> *pdev)
>>>
>>>                 name = dev->of_node->name;
>>>         
>>>         channels = iio_channel_get_all(dev);
>>>
>>> -       if (IS_ERR(channels))
>>> -               return PTR_ERR(channels);
>>> +       if (IS_ERR(channels)) {
>>> +               if (PTR_ERR(channels) == -ENODEV)
>>> +                       return -EPROBE_DEFER;
>>> +               else
>>> +                       return PTR_ERR(channels);
>>> +       }
>>>
>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>         if (st == NULL) {
>>
>> Indeed, I misunderstood what you told me.
>>
>> Actually, the patch you proposed is part of my v1
>> (https://lkml.org/lkml/2016/6/28/203) and v2
>> (https://lkml.org/lkml/2016/7/15/215).
>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
>> in -EPROBE_DEFER.
> 
> Thanks for the links.
> 
>> What I thought you were proposing was to change the -ENODEV return code
>> inside iio_channel_get_all. This cannot be an option since the function
>> might be called outside of a probe (it is not yet, but might be in the
>> future?).
> 
> AFAICS this is a helper function not knowing about device probing itself. And 
> it should stay at that.
> 
>> Of what I understood, two possibilities are then possible (proposed
>> either by Guenter or Jonathan): either rework the iio framework to
>> register iio map array earlier or to use late_initcall instead of init
>> for the driver consuming the iio channels.
> 
> Interestingly using this problem would not arise due to module dependencies. 
> But using late_initcall would mean this needs to be done on any driver using 
> iio channels? I would rather keep those consumers simple.

This would mean this needs to be done in any driver *using iio_map
array* to get iio channels. The other way of getting iio channels is
using properties in the Device Tree so no need for late_initcall in that
case.

Quentin
Guenter Roeck July 26, 2016, 4:04 p.m. UTC | #4
On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> > On 26/07/2016 11:05, Alexander Stein wrote:
> > > On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> > >> On 26/07/2016 10:21, Alexander Stein wrote:
> > >>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> > >>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> > >>>> and
> > >>>> properties in the Device Tree or channels whose consumer_dev_name
> > >>>> matches
> > >>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> > >>>> which might be probed after iio_hwmon.
> > >>> 
> > >>> Would it work if iio_channel_get_all returning ENODEV is used for
> > >>> returning
> > >>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> > >>> driver/device
> > >>> dependencies seems not right for me at this place.
> > >> 
> > >> Then what if the iio_channel_get_all is called outside of the probe of a
> > >> driver? We'll have to change the error code, things we are apparently
> > >> trying to avoid (see v2 patches' discussions).
> > > 
> > > Maybe I didn't express my idea enough. I don't want to change the behavior
> > > of iio_channel_get_all at all. Just the result evaluation of
> > > iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> > > below in mind.
> > > 
> > > Best regards,
> > > Alexander
> > > ---
> > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> > > index b550ba5..e32d150 100644
> > > --- a/drivers/hwmon/iio_hwmon.c
> > > +++ b/drivers/hwmon/iio_hwmon.c
> > > @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> > > *pdev)
> > > 
> > >                 name = dev->of_node->name;
> > >         
> > >         channels = iio_channel_get_all(dev);
> > > 
> > > -       if (IS_ERR(channels))
> > > -               return PTR_ERR(channels);
> > > +       if (IS_ERR(channels)) {
> > > +               if (PTR_ERR(channels) == -ENODEV)
> > > +                       return -EPROBE_DEFER;
> > > +               else
> > > +                       return PTR_ERR(channels);
> > > +       }
> > > 
> > >         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> > >         if (st == NULL) {
> > 
> > Indeed, I misunderstood what you told me.
> > 
> > Actually, the patch you proposed is part of my v1
> > (https://lkml.org/lkml/2016/6/28/203) and v2
> > (https://lkml.org/lkml/2016/7/15/215).
> > Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> > in -EPROBE_DEFER.
> 
> Thanks for the links.
> 
> > What I thought you were proposing was to change the -ENODEV return code
> > inside iio_channel_get_all. This cannot be an option since the function
> > might be called outside of a probe (it is not yet, but might be in the
> > future?).
> 
> AFAICS this is a helper function not knowing about device probing itself. And 
> it should stay at that.
> 
> > Of what I understood, two possibilities are then possible (proposed
> > either by Guenter or Jonathan): either rework the iio framework to
> > register iio map array earlier or to use late_initcall instead of init
> > for the driver consuming the iio channels.
> 
> Interestingly using this problem would not arise due to module dependencies. 
> But using late_initcall would mean this needs to be done on any driver using 
> iio channels? I would rather keep those consumers simple.
> 
Me too, but that would imply a solution in iio. The change you propose above
isn't exactly simple either, and would also be needed in each consumer driver.

Just for the record, I dislike the late_initcall solution as well, but I prefer
it over blindly converting ENODEV to EPROBE_DEFER.

Guenter
Jonathan Cameron Aug. 15, 2016, 3:36 p.m. UTC | #5
On 26/07/16 10:33, Quentin Schulz wrote:
> On 26/07/2016 11:05, Alexander Stein wrote:
>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
>>>>> and
>>>>> properties in the Device Tree or channels whose consumer_dev_name matches
>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>>>> which might be probed after iio_hwmon.
>>>>
>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>> returning
>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>> driver/device
>>>> dependencies seems not right for me at this place.
>>>
>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>> driver? We'll have to change the error code, things we are apparently
>>> trying to avoid (see v2 patches' discussions).
>>
>> Maybe I didn't express my idea enough. I don't want to change the behavior of  
>> iio_channel_get_all at all. Just the result evaluation of iio_channel_get_all 
>> in iio_hwmon_probe. I have something link the patch below in mind.
>>
>> Best regards,
>> Alexander
>> ---
>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>> index b550ba5..e32d150 100644
>> --- a/drivers/hwmon/iio_hwmon.c
>> +++ b/drivers/hwmon/iio_hwmon.c
>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>>                 name = dev->of_node->name;
>>  
>>         channels = iio_channel_get_all(dev);
>> -       if (IS_ERR(channels))
>> -               return PTR_ERR(channels);
>> +       if (IS_ERR(channels)) {
>> +               if (PTR_ERR(channels) == -ENODEV)
>> +                       return -EPROBE_DEFER;
>> +               else
>> +                       return PTR_ERR(channels);
>> +       }
>>  
>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>         if (st == NULL) {
> 
> Indeed, I misunderstood what you told me.
> 
> Actually, the patch you proposed is part of my v1
> (https://lkml.org/lkml/2016/6/28/203) and v2
> (https://lkml.org/lkml/2016/7/15/215).
> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> in -EPROBE_DEFER.
> 
> What I thought you were proposing was to change the -ENODEV return code
> inside iio_channel_get_all. This cannot be an option since the function
> might be called outside of a probe (it is not yet, but might be in the
> future?).
If that did happen I'd be tempted to introduce a new function to be
used outside of probe.  I definitely don't like rewriting in individual
drivers.

The defer question is still rather open in my mind. Will address it in the
other thread.

> 
> Of what I understood, two possibilities are then possible (proposed
> either by Guenter or Jonathan): either rework the iio framework to
> register iio map array earlier or to use late_initcall instead of init
> for the driver consuming the iio channels.
Ultimately we probably need to rethink how the registration works
(it was written before deferred probing came along and that has rather
changed things for us!)

For now I'm not convinced that deferring is that painful even if we
do have to try reprobing every time a new module gets probed..

Jonathan
> 
> Thanks,
> Quentin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Jonathan Cameron Aug. 15, 2016, 3:40 p.m. UTC | #6
On 26/07/16 17:04, Guenter Roeck wrote:
> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
>>>>>>> and
>>>>>>> properties in the Device Tree or channels whose consumer_dev_name
>>>>>>> matches
>>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>>>>>> which might be probed after iio_hwmon.
>>>>>>
>>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
>>>>>> returning
>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>> driver/device
>>>>>> dependencies seems not right for me at this place.
>>>>>
>>>>> Then what if the iio_channel_get_all is called outside of the probe of a
>>>>> driver? We'll have to change the error code, things we are apparently
>>>>> trying to avoid (see v2 patches' discussions).
>>>>
>>>> Maybe I didn't express my idea enough. I don't want to change the behavior
>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
>>>> below in mind.
>>>>
>>>> Best regards,
>>>> Alexander
>>>> ---
>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
>>>> index b550ba5..e32d150 100644
>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
>>>> *pdev)
>>>>
>>>>                 name = dev->of_node->name;
>>>>         
>>>>         channels = iio_channel_get_all(dev);
>>>>
>>>> -       if (IS_ERR(channels))
>>>> -               return PTR_ERR(channels);
>>>> +       if (IS_ERR(channels)) {
>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>> +                       return -EPROBE_DEFER;
>>>> +               else
>>>> +                       return PTR_ERR(channels);
>>>> +       }
>>>>
>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>         if (st == NULL) {
>>>
>>> Indeed, I misunderstood what you told me.
>>>
>>> Actually, the patch you proposed is part of my v1
>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>> (https://lkml.org/lkml/2016/7/15/215).
>>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
>>> in -EPROBE_DEFER.
>>
>> Thanks for the links.
>>
>>> What I thought you were proposing was to change the -ENODEV return code
>>> inside iio_channel_get_all. This cannot be an option since the function
>>> might be called outside of a probe (it is not yet, but might be in the
>>> future?).
>>
>> AFAICS this is a helper function not knowing about device probing itself. And 
>> it should stay at that.
>>
>>> Of what I understood, two possibilities are then possible (proposed
>>> either by Guenter or Jonathan): either rework the iio framework to
>>> register iio map array earlier or to use late_initcall instead of init
>>> for the driver consuming the iio channels.
>>
>> Interestingly using this problem would not arise due to module dependencies. 
>> But using late_initcall would mean this needs to be done on any driver using 
>> iio channels? I would rather keep those consumers simple.
>>
> Me too, but that would imply a solution in iio. The change you propose above
> isn't exactly simple either, and would also be needed in each consumer driver.
> 
> Just for the record, I dislike the late_initcall solution as well, but I prefer
> it over blindly converting ENODEV to EPROBE_DEFER.
I'm falling on the other side on this one right now. Though I'd be tempted
to renaming the function to something like iio_channel_get_all_or_defer
to make it explicit that it can result in deferred probing.

To my mind late_initcall tricks are rather hideous as well.  At least 
deferred probing should always work (with a fair cost associated with it of
course)

Lots of discussions ongoing on how to do dependency resolution that will
hopefully mean deferred probing only occurs in the pathological cases in
the future anyway. Interesting to see where that goes.

Jonathan
> 
> Guenter
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Guenter Roeck Aug. 15, 2016, 5:07 p.m. UTC | #7
On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
> On 26/07/16 17:04, Guenter Roeck wrote:
> > On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
> >> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
> >>> On 26/07/2016 11:05, Alexander Stein wrote:
> >>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
> >>>>> On 26/07/2016 10:21, Alexander Stein wrote:
> >>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
> >>>>>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles
> >>>>>>> and
> >>>>>>> properties in the Device Tree or channels whose consumer_dev_name
> >>>>>>> matches
> >>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> >>>>>>> which might be probed after iio_hwmon.
> >>>>>>
> >>>>>> Would it work if iio_channel_get_all returning ENODEV is used for
> >>>>>> returning
> >>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
> >>>>>> driver/device
> >>>>>> dependencies seems not right for me at this place.
> >>>>>
> >>>>> Then what if the iio_channel_get_all is called outside of the probe of a
> >>>>> driver? We'll have to change the error code, things we are apparently
> >>>>> trying to avoid (see v2 patches' discussions).
> >>>>
> >>>> Maybe I didn't express my idea enough. I don't want to change the behavior
> >>>> of iio_channel_get_all at all. Just the result evaluation of
> >>>> iio_channel_get_all in iio_hwmon_probe. I have something link the patch
> >>>> below in mind.
> >>>>
> >>>> Best regards,
> >>>> Alexander
> >>>> ---
> >>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> >>>> index b550ba5..e32d150 100644
> >>>> --- a/drivers/hwmon/iio_hwmon.c
> >>>> +++ b/drivers/hwmon/iio_hwmon.c
> >>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct platform_device
> >>>> *pdev)
> >>>>
> >>>>                 name = dev->of_node->name;
> >>>>         
> >>>>         channels = iio_channel_get_all(dev);
> >>>>
> >>>> -       if (IS_ERR(channels))
> >>>> -               return PTR_ERR(channels);
> >>>> +       if (IS_ERR(channels)) {
> >>>> +               if (PTR_ERR(channels) == -ENODEV)
> >>>> +                       return -EPROBE_DEFER;
> >>>> +               else
> >>>> +                       return PTR_ERR(channels);
> >>>> +       }
> >>>>
> >>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> >>>>         if (st == NULL) {
> >>>
> >>> Indeed, I misunderstood what you told me.
> >>>
> >>> Actually, the patch you proposed is part of my v1
> >>> (https://lkml.org/lkml/2016/6/28/203) and v2
> >>> (https://lkml.org/lkml/2016/7/15/215).
> >>> Jonathan and Guenter didn't really like the idea of changing the -ENODEV
> >>> in -EPROBE_DEFER.
> >>
> >> Thanks for the links.
> >>
> >>> What I thought you were proposing was to change the -ENODEV return code
> >>> inside iio_channel_get_all. This cannot be an option since the function
> >>> might be called outside of a probe (it is not yet, but might be in the
> >>> future?).
> >>
> >> AFAICS this is a helper function not knowing about device probing itself. And 
> >> it should stay at that.
> >>
> >>> Of what I understood, two possibilities are then possible (proposed
> >>> either by Guenter or Jonathan): either rework the iio framework to
> >>> register iio map array earlier or to use late_initcall instead of init
> >>> for the driver consuming the iio channels.
> >>
> >> Interestingly using this problem would not arise due to module dependencies. 
> >> But using late_initcall would mean this needs to be done on any driver using 
> >> iio channels? I would rather keep those consumers simple.
> >>
> > Me too, but that would imply a solution in iio. The change you propose above
> > isn't exactly simple either, and would also be needed in each consumer driver.
> > 
> > Just for the record, I dislike the late_initcall solution as well, but I prefer
> > it over blindly converting ENODEV to EPROBE_DEFER.
> I'm falling on the other side on this one right now. Though I'd be tempted
> to renaming the function to something like iio_channel_get_all_or_defer
> to make it explicit that it can result in deferred probing.
> 
Would this new function return -EPROBE_DEFER instead of -ENODEV ?

Thanks,
Guenter
Jonathan Cameron Aug. 15, 2016, 9:35 p.m. UTC | #8
On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>> On 26/07/16 17:04, Guenter Roeck wrote:
>> > On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>> >> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>> >>> On 26/07/2016 11:05, Alexander Stein wrote:
>> >>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>> >>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>> >>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>> >>>>>>> iio_channel_get_all returns -ENODEV when it cannot find
>either phandles
>> >>>>>>> and
>> >>>>>>> properties in the Device Tree or channels whose
>consumer_dev_name
>> >>>>>>> matches
>> >>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by
>iio drivers
>> >>>>>>> which might be probed after iio_hwmon.
>> >>>>>>
>> >>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>for
>> >>>>>> returning
>> >>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>> >>>>>> driver/device
>> >>>>>> dependencies seems not right for me at this place.
>> >>>>>
>> >>>>> Then what if the iio_channel_get_all is called outside of the
>probe of a
>> >>>>> driver? We'll have to change the error code, things we are
>apparently
>> >>>>> trying to avoid (see v2 patches' discussions).
>> >>>>
>> >>>> Maybe I didn't express my idea enough. I don't want to change
>the behavior
>> >>>> of iio_channel_get_all at all. Just the result evaluation of
>> >>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>the patch
>> >>>> below in mind.
>> >>>>
>> >>>> Best regards,
>> >>>> Alexander
>> >>>> ---
>> >>>> diff --git a/drivers/hwmon/iio_hwmon.c
>b/drivers/hwmon/iio_hwmon.c
>> >>>> index b550ba5..e32d150 100644
>> >>>> --- a/drivers/hwmon/iio_hwmon.c
>> >>>> +++ b/drivers/hwmon/iio_hwmon.c
>> >>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>platform_device
>> >>>> *pdev)
>> >>>>
>> >>>>                 name = dev->of_node->name;
>> >>>>         
>> >>>>         channels = iio_channel_get_all(dev);
>> >>>>
>> >>>> -       if (IS_ERR(channels))
>> >>>> -               return PTR_ERR(channels);
>> >>>> +       if (IS_ERR(channels)) {
>> >>>> +               if (PTR_ERR(channels) == -ENODEV)
>> >>>> +                       return -EPROBE_DEFER;
>> >>>> +               else
>> >>>> +                       return PTR_ERR(channels);
>> >>>> +       }
>> >>>>
>> >>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>> >>>>         if (st == NULL) {
>> >>>
>> >>> Indeed, I misunderstood what you told me.
>> >>>
>> >>> Actually, the patch you proposed is part of my v1
>> >>> (https://lkml.org/lkml/2016/6/28/203) and v2
>> >>> (https://lkml.org/lkml/2016/7/15/215).
>> >>> Jonathan and Guenter didn't really like the idea of changing the
>-ENODEV
>> >>> in -EPROBE_DEFER.
>> >>
>> >> Thanks for the links.
>> >>
>> >>> What I thought you were proposing was to change the -ENODEV
>return code
>> >>> inside iio_channel_get_all. This cannot be an option since the
>function
>> >>> might be called outside of a probe (it is not yet, but might be
>in the
>> >>> future?).
>> >>
>> >> AFAICS this is a helper function not knowing about device probing
>itself. And 
>> >> it should stay at that.
>> >>
>> >>> Of what I understood, two possibilities are then possible
>(proposed
>> >>> either by Guenter or Jonathan): either rework the iio framework
>to
>> >>> register iio map array earlier or to use late_initcall instead of
>init
>> >>> for the driver consuming the iio channels.
>> >>
>> >> Interestingly using this problem would not arise due to module
>dependencies. 
>> >> But using late_initcall would mean this needs to be done on any
>driver using 
>> >> iio channels? I would rather keep those consumers simple.
>> >>
>> > Me too, but that would imply a solution in iio. The change you
>propose above
>> > isn't exactly simple either, and would also be needed in each
>consumer driver.
>> > 
>> > Just for the record, I dislike the late_initcall solution as well,
>but I prefer
>> > it over blindly converting ENODEV to EPROBE_DEFER.
>> I'm falling on the other side on this one right now. Though I'd be
>tempted
>> to renaming the function to something like
>iio_channel_get_all_or_defer
>> to make it explicit that it can result in deferred probing.
>> 
>Would this new function return -EPROBE_DEFER instead of -ENODEV ?
Yes. Though whether it really adds much over doing that in drivers isn't clear.

Hmm. Needs more thought...
>
>Thanks,
>Guenter
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
Quentin Schulz Sept. 1, 2016, 7:15 a.m. UTC | #9
On 15/08/2016 23:35, Jonathan Cameron wrote:
> 
> 
> On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>>> On 26/07/16 17:04, Guenter Roeck wrote:
>>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>>>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>>>>>>> iio_channel_get_all returns -ENODEV when it cannot find
>> either phandles
>>>>>>>>>> and
>>>>>>>>>> properties in the Device Tree or channels whose
>> consumer_dev_name
>>>>>>>>>> matches
>>>>>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by
>> iio drivers
>>>>>>>>>> which might be probed after iio_hwmon.
>>>>>>>>>
>>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>> for
>>>>>>>>> returning
>>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>>>>> driver/device
>>>>>>>>> dependencies seems not right for me at this place.
>>>>>>>>
>>>>>>>> Then what if the iio_channel_get_all is called outside of the
>> probe of a
>>>>>>>> driver? We'll have to change the error code, things we are
>> apparently
>>>>>>>> trying to avoid (see v2 patches' discussions).
>>>>>>>
>>>>>>> Maybe I didn't express my idea enough. I don't want to change
>> the behavior
>>>>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>> the patch
>>>>>>> below in mind.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Alexander
>>>>>>> ---
>>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c
>> b/drivers/hwmon/iio_hwmon.c
>>>>>>> index b550ba5..e32d150 100644
>>>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>> platform_device
>>>>>>> *pdev)
>>>>>>>
>>>>>>>                 name = dev->of_node->name;
>>>>>>>         
>>>>>>>         channels = iio_channel_get_all(dev);
>>>>>>>
>>>>>>> -       if (IS_ERR(channels))
>>>>>>> -               return PTR_ERR(channels);
>>>>>>> +       if (IS_ERR(channels)) {
>>>>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>>>>> +                       return -EPROBE_DEFER;
>>>>>>> +               else
>>>>>>> +                       return PTR_ERR(channels);
>>>>>>> +       }
>>>>>>>
>>>>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>>>>         if (st == NULL) {
>>>>>>
>>>>>> Indeed, I misunderstood what you told me.
>>>>>>
>>>>>> Actually, the patch you proposed is part of my v1
>>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>>>>> (https://lkml.org/lkml/2016/7/15/215).
>>>>>> Jonathan and Guenter didn't really like the idea of changing the
>> -ENODEV
>>>>>> in -EPROBE_DEFER.
>>>>>
>>>>> Thanks for the links.
>>>>>
>>>>>> What I thought you were proposing was to change the -ENODEV
>> return code
>>>>>> inside iio_channel_get_all. This cannot be an option since the
>> function
>>>>>> might be called outside of a probe (it is not yet, but might be
>> in the
>>>>>> future?).
>>>>>
>>>>> AFAICS this is a helper function not knowing about device probing
>> itself. And 
>>>>> it should stay at that.
>>>>>
>>>>>> Of what I understood, two possibilities are then possible
>> (proposed
>>>>>> either by Guenter or Jonathan): either rework the iio framework
>> to
>>>>>> register iio map array earlier or to use late_initcall instead of
>> init
>>>>>> for the driver consuming the iio channels.
>>>>>
>>>>> Interestingly using this problem would not arise due to module
>> dependencies. 
>>>>> But using late_initcall would mean this needs to be done on any
>> driver using 
>>>>> iio channels? I would rather keep those consumers simple.
>>>>>
>>>> Me too, but that would imply a solution in iio. The change you
>> propose above
>>>> isn't exactly simple either, and would also be needed in each
>> consumer driver.
>>>>
>>>> Just for the record, I dislike the late_initcall solution as well,
>> but I prefer
>>>> it over blindly converting ENODEV to EPROBE_DEFER.
>>> I'm falling on the other side on this one right now. Though I'd be
>> tempted
>>> to renaming the function to something like
>> iio_channel_get_all_or_defer
>>> to make it explicit that it can result in deferred probing.
>>>
>> Would this new function return -EPROBE_DEFER instead of -ENODEV ?
> Yes. Though whether it really adds much over doing that in drivers isn't clear.
> 
> Hmm. Needs more thought...

Either we do the exact same "hack" as in the v2[1] in what you call
iio_channel_get_all_or_defer or we duplicate the code from
iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem
right to me but I really dislike the late_initcall method. With this
method we can only have one level of "channel dependency".

This means if we ever create a new driver which depends on channels from
the driver using late_initcall, we will also have to use late_initcall
and we can't be sure the new driver will always be probed after the
driver he depends on.

[1] https://lkml.org/lkml/2016/7/15/215

Quentin
>>
>> Thanks,
>> Guenter
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Quentin Schulz Sept. 1, 2016, 9:03 a.m. UTC | #10
On 01/09/2016 09:15, Quentin Schulz wrote:
> On 15/08/2016 23:35, Jonathan Cameron wrote:
>>
>>
>> On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>>> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>>>> On 26/07/16 17:04, Guenter Roeck wrote:
>>>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>>>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>>>>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>>>>>>>> iio_channel_get_all returns -ENODEV when it cannot find
>>> either phandles
>>>>>>>>>>> and
>>>>>>>>>>> properties in the Device Tree or channels whose
>>> consumer_dev_name
>>>>>>>>>>> matches
>>>>>>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by
>>> iio drivers
>>>>>>>>>>> which might be probed after iio_hwmon.
>>>>>>>>>>
>>>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>>> for
>>>>>>>>>> returning
>>>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>>>>>> driver/device
>>>>>>>>>> dependencies seems not right for me at this place.
>>>>>>>>>
>>>>>>>>> Then what if the iio_channel_get_all is called outside of the
>>> probe of a
>>>>>>>>> driver? We'll have to change the error code, things we are
>>> apparently
>>>>>>>>> trying to avoid (see v2 patches' discussions).
>>>>>>>>
>>>>>>>> Maybe I didn't express my idea enough. I don't want to change
>>> the behavior
>>>>>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>>> the patch
>>>>>>>> below in mind.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>> Alexander
>>>>>>>> ---
>>>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c
>>> b/drivers/hwmon/iio_hwmon.c
>>>>>>>> index b550ba5..e32d150 100644
>>>>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>>> platform_device
>>>>>>>> *pdev)
>>>>>>>>
>>>>>>>>                 name = dev->of_node->name;
>>>>>>>>         
>>>>>>>>         channels = iio_channel_get_all(dev);
>>>>>>>>
>>>>>>>> -       if (IS_ERR(channels))
>>>>>>>> -               return PTR_ERR(channels);
>>>>>>>> +       if (IS_ERR(channels)) {
>>>>>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>>>>>> +                       return -EPROBE_DEFER;
>>>>>>>> +               else
>>>>>>>> +                       return PTR_ERR(channels);
>>>>>>>> +       }
>>>>>>>>
>>>>>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>>>>>         if (st == NULL) {
>>>>>>>
>>>>>>> Indeed, I misunderstood what you told me.
>>>>>>>
>>>>>>> Actually, the patch you proposed is part of my v1
>>>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>>>>>> (https://lkml.org/lkml/2016/7/15/215).
>>>>>>> Jonathan and Guenter didn't really like the idea of changing the
>>> -ENODEV
>>>>>>> in -EPROBE_DEFER.
>>>>>>
>>>>>> Thanks for the links.
>>>>>>
>>>>>>> What I thought you were proposing was to change the -ENODEV
>>> return code
>>>>>>> inside iio_channel_get_all. This cannot be an option since the
>>> function
>>>>>>> might be called outside of a probe (it is not yet, but might be
>>> in the
>>>>>>> future?).
>>>>>>
>>>>>> AFAICS this is a helper function not knowing about device probing
>>> itself. And 
>>>>>> it should stay at that.
>>>>>>
>>>>>>> Of what I understood, two possibilities are then possible
>>> (proposed
>>>>>>> either by Guenter or Jonathan): either rework the iio framework
>>> to
>>>>>>> register iio map array earlier or to use late_initcall instead of
>>> init
>>>>>>> for the driver consuming the iio channels.
>>>>>>
>>>>>> Interestingly using this problem would not arise due to module
>>> dependencies. 
>>>>>> But using late_initcall would mean this needs to be done on any
>>> driver using 
>>>>>> iio channels? I would rather keep those consumers simple.
>>>>>>
>>>>> Me too, but that would imply a solution in iio. The change you
>>> propose above
>>>>> isn't exactly simple either, and would also be needed in each
>>> consumer driver.
>>>>>
>>>>> Just for the record, I dislike the late_initcall solution as well,
>>> but I prefer
>>>>> it over blindly converting ENODEV to EPROBE_DEFER.
>>>> I'm falling on the other side on this one right now. Though I'd be
>>> tempted
>>>> to renaming the function to something like
>>> iio_channel_get_all_or_defer
>>>> to make it explicit that it can result in deferred probing.
>>>>
>>> Would this new function return -EPROBE_DEFER instead of -ENODEV ?
>> Yes. Though whether it really adds much over doing that in drivers isn't clear.
>>
>> Hmm. Needs more thought...
> 
> Either we do the exact same "hack" as in the v2[1] in what you call
> iio_channel_get_all_or_defer or we duplicate the code from
> iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem
> right to me but I really dislike the late_initcall method. With this
> method we can only have one level of "channel dependency".
> 
> This means if we ever create a new driver which depends on channels from
> the driver using late_initcall, we will also have to use late_initcall
> and we can't be sure the new driver will always be probed after the
> driver he depends on.
> 
> [1] https://lkml.org/lkml/2016/7/15/215
> 
> Quentin

Should I revert back to the hack introduced in v2 then?

>>>
>>> Thanks,
>>> Guenter
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
Jonathan Cameron Sept. 3, 2016, 7:32 p.m. UTC | #11
On 01/09/16 10:03, Quentin Schulz wrote:
> On 01/09/2016 09:15, Quentin Schulz wrote:
>> On 15/08/2016 23:35, Jonathan Cameron wrote:
>>>
>>>
>>> On 15 August 2016 18:07:30 BST, Guenter Roeck <linux@roeck-us.net> wrote:
>>>> On Mon, Aug 15, 2016 at 04:40:21PM +0100, Jonathan Cameron wrote:
>>>>> On 26/07/16 17:04, Guenter Roeck wrote:
>>>>>> On Tue, Jul 26, 2016 at 12:00:33PM +0200, Alexander Stein wrote:
>>>>>>> On Tuesday 26 July 2016 11:33:59, Quentin Schulz wrote:
>>>>>>>> On 26/07/2016 11:05, Alexander Stein wrote:
>>>>>>>>> On Tuesday 26 July 2016 10:24:48, Quentin Schulz wrote:
>>>>>>>>>> On 26/07/2016 10:21, Alexander Stein wrote:
>>>>>>>>>>> On Tuesday 26 July 2016 09:43:44, Quentin Schulz wrote:
>>>>>>>>>>>> iio_channel_get_all returns -ENODEV when it cannot find
>>>> either phandles
>>>>>>>>>>>> and
>>>>>>>>>>>> properties in the Device Tree or channels whose
>>>> consumer_dev_name
>>>>>>>>>>>> matches
>>>>>>>>>>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by
>>>> iio drivers
>>>>>>>>>>>> which might be probed after iio_hwmon.
>>>>>>>>>>>
>>>>>>>>>>> Would it work if iio_channel_get_all returning ENODEV is used
>>>> for
>>>>>>>>>>> returning
>>>>>>>>>>> EPROBE_DEFER in iio_channel_get_all? Using late initcalls for
>>>>>>>>>>> driver/device
>>>>>>>>>>> dependencies seems not right for me at this place.
>>>>>>>>>>
>>>>>>>>>> Then what if the iio_channel_get_all is called outside of the
>>>> probe of a
>>>>>>>>>> driver? We'll have to change the error code, things we are
>>>> apparently
>>>>>>>>>> trying to avoid (see v2 patches' discussions).
>>>>>>>>>
>>>>>>>>> Maybe I didn't express my idea enough. I don't want to change
>>>> the behavior
>>>>>>>>> of iio_channel_get_all at all. Just the result evaluation of
>>>>>>>>> iio_channel_get_all in iio_hwmon_probe. I have something link
>>>> the patch
>>>>>>>>> below in mind.
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Alexander
>>>>>>>>> ---
>>>>>>>>> diff --git a/drivers/hwmon/iio_hwmon.c
>>>> b/drivers/hwmon/iio_hwmon.c
>>>>>>>>> index b550ba5..e32d150 100644
>>>>>>>>> --- a/drivers/hwmon/iio_hwmon.c
>>>>>>>>> +++ b/drivers/hwmon/iio_hwmon.c
>>>>>>>>> @@ -73,8 +73,12 @@ static int iio_hwmon_probe(struct
>>>> platform_device
>>>>>>>>> *pdev)
>>>>>>>>>
>>>>>>>>>                 name = dev->of_node->name;
>>>>>>>>>         
>>>>>>>>>         channels = iio_channel_get_all(dev);
>>>>>>>>>
>>>>>>>>> -       if (IS_ERR(channels))
>>>>>>>>> -               return PTR_ERR(channels);
>>>>>>>>> +       if (IS_ERR(channels)) {
>>>>>>>>> +               if (PTR_ERR(channels) == -ENODEV)
>>>>>>>>> +                       return -EPROBE_DEFER;
>>>>>>>>> +               else
>>>>>>>>> +                       return PTR_ERR(channels);
>>>>>>>>> +       }
>>>>>>>>>
>>>>>>>>>         st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>>>>>>>>>         if (st == NULL) {
>>>>>>>>
>>>>>>>> Indeed, I misunderstood what you told me.
>>>>>>>>
>>>>>>>> Actually, the patch you proposed is part of my v1
>>>>>>>> (https://lkml.org/lkml/2016/6/28/203) and v2
>>>>>>>> (https://lkml.org/lkml/2016/7/15/215).
>>>>>>>> Jonathan and Guenter didn't really like the idea of changing the
>>>> -ENODEV
>>>>>>>> in -EPROBE_DEFER.
>>>>>>>
>>>>>>> Thanks for the links.
>>>>>>>
>>>>>>>> What I thought you were proposing was to change the -ENODEV
>>>> return code
>>>>>>>> inside iio_channel_get_all. This cannot be an option since the
>>>> function
>>>>>>>> might be called outside of a probe (it is not yet, but might be
>>>> in the
>>>>>>>> future?).
>>>>>>>
>>>>>>> AFAICS this is a helper function not knowing about device probing
>>>> itself. And 
>>>>>>> it should stay at that.
>>>>>>>
>>>>>>>> Of what I understood, two possibilities are then possible
>>>> (proposed
>>>>>>>> either by Guenter or Jonathan): either rework the iio framework
>>>> to
>>>>>>>> register iio map array earlier or to use late_initcall instead of
>>>> init
>>>>>>>> for the driver consuming the iio channels.
>>>>>>>
>>>>>>> Interestingly using this problem would not arise due to module
>>>> dependencies. 
>>>>>>> But using late_initcall would mean this needs to be done on any
>>>> driver using 
>>>>>>> iio channels? I would rather keep those consumers simple.
>>>>>>>
>>>>>> Me too, but that would imply a solution in iio. The change you
>>>> propose above
>>>>>> isn't exactly simple either, and would also be needed in each
>>>> consumer driver.
>>>>>>
>>>>>> Just for the record, I dislike the late_initcall solution as well,
>>>> but I prefer
>>>>>> it over blindly converting ENODEV to EPROBE_DEFER.
>>>>> I'm falling on the other side on this one right now. Though I'd be
>>>> tempted
>>>>> to renaming the function to something like
>>>> iio_channel_get_all_or_defer
>>>>> to make it explicit that it can result in deferred probing.
>>>>>
>>>> Would this new function return -EPROBE_DEFER instead of -ENODEV ?
>>> Yes. Though whether it really adds much over doing that in drivers isn't clear.
>>>
>>> Hmm. Needs more thought...
>>
>> Either we do the exact same "hack" as in the v2[1] in what you call
>> iio_channel_get_all_or_defer or we duplicate the code from
>> iio_channel_get_all in iio_channel_get_all_or_defer. Both do not seem
>> right to me but I really dislike the late_initcall method. With this
>> method we can only have one level of "channel dependency".
>>
>> This means if we ever create a new driver which depends on channels from
>> the driver using late_initcall, we will also have to use late_initcall
>> and we can't be sure the new driver will always be probed after the
>> driver he depends on.
>>
>> [1] https://lkml.org/lkml/2016/7/15/215
>>
>> Quentin
> 
> Should I revert back to the hack introduced in v2 then?
I think so.  Sorry I didn't see this until after you'd sent v4.

That hack had it's disadvantages but in many ways it was a least clean.

Jonathan
> 
>>>>
>>>> Thanks,
>>>> Guenter
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
diff mbox

Patch

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..e32d150 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -73,8 +73,12 @@  static int iio_hwmon_probe(struct platform_device *pdev)
                name = dev->of_node->name;
 
        channels = iio_channel_get_all(dev);
-       if (IS_ERR(channels))
-               return PTR_ERR(channels);
+       if (IS_ERR(channels)) {
+               if (PTR_ERR(channels) == -ENODEV)
+                       return -EPROBE_DEFER;
+               else
+                       return PTR_ERR(channels);
+       }
 
        st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
        if (st == NULL) {