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

Groups > linux.kernel > #1450449 > unrolled thread

[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

  [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-26 09:50 +0200
    Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Alexander Stein <alexander.stein@systec-electronic.com> - 2016-07-26 10:30 +0200
      Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-26 10:30 +0200
        Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Alexander Stein <alexander.stein@systec-electronic.com> - 2016-07-26 11:10 +0200
          Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-26 11:40 +0200
            Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Alexander Stein <alexander.stein@systec-electronic.com> - 2016-07-26 12:10 +0200
              Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-26 12:10 +0200
              Re: [PATCH v3 1/4] hwmon: iio_hwmon: delay probing with late_initcall Guenter Roeck <linux@roeck-us.net> - 2016-07-26 18:10 +0200

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

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.

This makes sure iio_hwmon is probed after all iio drivers which provides
channels to iio_hwmon are probed, be they present in the DT or using
iio_map_list.

This replaces module_platform_driver() by an explicit code variant which
calls late_initcall() install of module_init(), meaning it probes after
all the drivers using module_init() as their init.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

v3:
 - use late_initcall instead of deferring probe,

 drivers/hwmon/iio_hwmon.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..0a00bfb 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -192,7 +192,21 @@ static struct platform_driver __refdata iio_hwmon_driver = {
 	.remove = iio_hwmon_remove,
 };
 
-module_platform_driver(iio_hwmon_driver);
+static struct platform_driver * const drivers[] = {
+	&iio_hwmon_driver,
+};
+
+static int __init iio_hwmon_late_init(void)
+{
+	return platform_register_drivers(drivers, ARRAY_SIZE(drivers));
+}
+late_initcall(iio_hwmon_late_init);
+
+static void __exit iio_hwmon_exit(void)
+{
+	platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
+}
+module_exit(iio_hwmon_exit);
 
 MODULE_AUTHOR("Jonathan Cameron <jic23@kernel.org>");
 MODULE_DESCRIPTION("IIO to hwmon driver");
-- 
2.5.0

[toc] | [next] | [standalone]

#1450467

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.

Best regards,
Alexander

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

#1450469

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).

Thanks,
Quentin

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

#1450485

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) {

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

#1450500

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

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

#1450517

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

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

#1450521

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

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

#1450666

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web