| Message ID | 1469519027-11387-2-git-send-email-quentin.schulz@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
Hi, On 26/07/2016 09:48, Thomas Petazzoni wrote: > Hello, > > On Tue, 26 Jul 2016 09:43:44 +0200, Quentin Schulz wrote: > >> -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)); > > Why not platform_register_driver() ? > > This would avoid the need to declare an array with just one entry. Actually, it is named platform_driver_register() as you just showed me and that's the reason I didn't find it under platform_register_driver(). Thanks! Quentin
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
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
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
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 >
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 >>
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");
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(-) -- 2.5.0