Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1462938 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Jonathan Cameron <jic23@kernel.org> - 2016-08-15 17:50 +0200
    Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Guenter Roeck <linux@roeck-us.net> - 2016-08-15 19:10 +0200
      Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Jonathan Cameron <jic23@kernel.org> - 2016-08-15 23:40 +0200

#1462938 — Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall

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
> 

[toc] | [next] | [standalone]

#1463005

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

[toc] | [prev] | [next] | [standalone]

#1463191

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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web