| Message ID | 1467101897-15946-4-git-send-email-quentin.schulz@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 28, 2016 at 10:18:17AM +0200, 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. > > It is better to defer the probe of iio_hwmon if such error is returned by > iio_channel_get_all in order to let a chance to iio drivers to expose > channels in iio_map_list. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > drivers/hwmon/iio_hwmon.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c > index b550ba5..c0da4d9 100644 > --- a/drivers/hwmon/iio_hwmon.c > +++ b/drivers/hwmon/iio_hwmon.c > @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev) > name = dev->of_node->name; > > channels = iio_channel_get_all(dev); > - if (IS_ERR(channels)) > + if (IS_ERR(channels)) { > + if (PTR_ERR(channels) == -ENODEV) > + return -EPROBE_DEFER; The problem, as I see it, is with iio, which should return -EPROBE_DEFER in this situation. We can not convert -ENODEV to -EPROBE_DEFER without risking that the channels are _really_ not there, which would result in endless "deferred" messages. Guenter > return PTR_ERR(channels); > + } > > st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); > if (st == NULL) {
On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote: >On Tue, Jun 28, 2016 at 10:18:17AM +0200, 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. >> >> It is better to defer the probe of iio_hwmon if such error is >returned by >> iio_channel_get_all in order to let a chance to iio drivers to expose >> channels in iio_map_list. >> >> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >> --- >> drivers/hwmon/iio_hwmon.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >> index b550ba5..c0da4d9 100644 >> --- a/drivers/hwmon/iio_hwmon.c >> +++ b/drivers/hwmon/iio_hwmon.c >> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device >*pdev) >> name = dev->of_node->name; >> >> channels = iio_channel_get_all(dev); >> - if (IS_ERR(channels)) >> + if (IS_ERR(channels)) { >> + if (PTR_ERR(channels) == -ENODEV) >> + return -EPROBE_DEFER; > >The problem, as I see it, is with iio, which should return >-EPROBE_DEFER >in this situation. Agreed. New fangled stuff this deferred probing :) > >We can not convert -ENODEV to -EPROBE_DEFER without risking that the >channels are _really_ not there, which would result in endless >"deferred" >messages. Hmm not entirely sure how we prevent that happening wherever it is done.. > >Guenter > >> return PTR_ERR(channels); >> + } >> >> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); >> if (st == NULL) { >-- >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 06/30/2016 03:59 PM, Jonathan Cameron wrote: > > > On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote: >> On Tue, Jun 28, 2016 at 10:18:17AM +0200, 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. >>> >>> It is better to defer the probe of iio_hwmon if such error is >> returned by >>> iio_channel_get_all in order to let a chance to iio drivers to expose >>> channels in iio_map_list. >>> >>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >>> --- >>> drivers/hwmon/iio_hwmon.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >>> index b550ba5..c0da4d9 100644 >>> --- a/drivers/hwmon/iio_hwmon.c >>> +++ b/drivers/hwmon/iio_hwmon.c >>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device >> *pdev) >>> name = dev->of_node->name; >>> >>> channels = iio_channel_get_all(dev); >>> - if (IS_ERR(channels)) >>> + if (IS_ERR(channels)) { >>> + if (PTR_ERR(channels) == -ENODEV) >>> + return -EPROBE_DEFER; >> >> The problem, as I see it, is with iio, which should return >> -EPROBE_DEFER >> in this situation. > Agreed. New fangled stuff this deferred probing :) >> >> We can not convert -ENODEV to -EPROBE_DEFER without risking that the >> channels are _really_ not there, which would result in endless >> "deferred" >> messages. > Hmm not entirely sure how we prevent that happening wherever it is done.. EPROBE_DEFER is supposed to be returned if you know that a provider was specified, but the provider is not yet available. Whereas ENODEV/ENOENT/ENXIO (choose your favorite) is supposed to be returned when no provider is available. In the case of the bridge the later case does not really makes sense though since it wouldn't work without any channels. This of course requires that all provider-consumer maps are available before the first consumer is registered. Which is not the case with IIO where the provider registers the consumer map. We should really do what GPIO does which registers the map in the board file.
On 06/30/2016 06:59 AM, Jonathan Cameron wrote: > > > On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote: >> On Tue, Jun 28, 2016 at 10:18:17AM +0200, 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. >>> >>> It is better to defer the probe of iio_hwmon if such error is >> returned by >>> iio_channel_get_all in order to let a chance to iio drivers to expose >>> channels in iio_map_list. >>> >>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >>> --- >>> drivers/hwmon/iio_hwmon.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >>> index b550ba5..c0da4d9 100644 >>> --- a/drivers/hwmon/iio_hwmon.c >>> +++ b/drivers/hwmon/iio_hwmon.c >>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device >> *pdev) >>> name = dev->of_node->name; >>> >>> channels = iio_channel_get_all(dev); >>> - if (IS_ERR(channels)) >>> + if (IS_ERR(channels)) { >>> + if (PTR_ERR(channels) == -ENODEV) >>> + return -EPROBE_DEFER; >> >> The problem, as I see it, is with iio, which should return >> -EPROBE_DEFER >> in this situation. > Agreed. New fangled stuff this deferred probing :) >> >> We can not convert -ENODEV to -EPROBE_DEFER without risking that the >> channels are _really_ not there, which would result in endless >> "deferred" >> messages. > Hmm not entirely sure how we prevent that happening wherever it is done.. > Outch. Better at the source, though. I didn't look at the iio code recently, but can you detect the defer situation at least with devicetree ? For non-devicetree situations, the only option I can think of would be to replace the module initcall with a later initcall. That should solve the problem if both iio_hwmon and and underlying drivers are built into the kernel. If iio_hwmon is modular, the only real option I can see is to make sure that all drivers it needs are loaded first. Does this make sense ? Guenter >> >> Guenter >> >>> return PTR_ERR(channels); >>> + } >>> >>> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); >>> if (st == NULL) { >> -- >> 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 30/06/16 15:51, Guenter Roeck wrote: > On 06/30/2016 06:59 AM, Jonathan Cameron wrote: >> >> >> On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote: >>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, 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. >>>> >>>> It is better to defer the probe of iio_hwmon if such error is >>> returned by >>>> iio_channel_get_all in order to let a chance to iio drivers to expose >>>> channels in iio_map_list. >>>> >>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >>>> --- >>>> drivers/hwmon/iio_hwmon.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >>>> index b550ba5..c0da4d9 100644 >>>> --- a/drivers/hwmon/iio_hwmon.c >>>> +++ b/drivers/hwmon/iio_hwmon.c >>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device >>> *pdev) >>>> name = dev->of_node->name; >>>> >>>> channels = iio_channel_get_all(dev); >>>> - if (IS_ERR(channels)) >>>> + if (IS_ERR(channels)) { >>>> + if (PTR_ERR(channels) == -ENODEV) >>>> + return -EPROBE_DEFER; >>> >>> The problem, as I see it, is with iio, which should return >>> -EPROBE_DEFER >>> in this situation. >> Agreed. New fangled stuff this deferred probing :) >>> >>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the >>> channels are _really_ not there, which would result in endless >>> "deferred" >>> messages. >> Hmm not entirely sure how we prevent that happening wherever it is done.. >> > > Outch. Better at the source, though. I didn't look at the iio code recently, > but can you detect the defer situation at least with devicetree ? > > For non-devicetree situations, the only option I can think of would be > to replace the module initcall with a later initcall. That should solve > the problem if both iio_hwmon and and underlying drivers are built > into the kernel. If iio_hwmon is modular, the only real option I can > see is to make sure that all drivers it needs are loaded first. > > Does this make sense ? I think we need to look in a couple of directions. Firstly, investigate doing something similar to gpio and basically move the setup of maps much earlier. This will replace drivers presenting their own maps The other direction is to get userspace (i.e. configfs) setup of these maps working so for cases where it's a bit less hardware defined (i.e. using an accelerometer as an input device) we can do once we know all devices relevant are present and instantiate new instances on the fly. Anyhow, neither is trivial unfortunately. J > > Guenter > >>> >>> Guenter >>> >>>> return PTR_ERR(channels); >>>> + } >>>> >>>> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); >>>> if (st == NULL) { >>> -- >>> 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
On 07/03/2016 03:47 AM, Jonathan Cameron wrote: > On 30/06/16 15:51, Guenter Roeck wrote: >> On 06/30/2016 06:59 AM, Jonathan Cameron wrote: >>> >>> >>> On 30 June 2016 04:47:25 BST, Guenter Roeck <linux@roeck-us.net> wrote: >>>> On Tue, Jun 28, 2016 at 10:18:17AM +0200, 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. >>>>> >>>>> It is better to defer the probe of iio_hwmon if such error is >>>> returned by >>>>> iio_channel_get_all in order to let a chance to iio drivers to expose >>>>> channels in iio_map_list. >>>>> >>>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> >>>>> --- >>>>> drivers/hwmon/iio_hwmon.c | 5 ++++- >>>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c >>>>> index b550ba5..c0da4d9 100644 >>>>> --- a/drivers/hwmon/iio_hwmon.c >>>>> +++ b/drivers/hwmon/iio_hwmon.c >>>>> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device >>>> *pdev) >>>>> name = dev->of_node->name; >>>>> >>>>> channels = iio_channel_get_all(dev); >>>>> - if (IS_ERR(channels)) >>>>> + if (IS_ERR(channels)) { >>>>> + if (PTR_ERR(channels) == -ENODEV) >>>>> + return -EPROBE_DEFER; >>>> >>>> The problem, as I see it, is with iio, which should return >>>> -EPROBE_DEFER >>>> in this situation. >>> Agreed. New fangled stuff this deferred probing :) >>>> >>>> We can not convert -ENODEV to -EPROBE_DEFER without risking that the >>>> channels are _really_ not there, which would result in endless >>>> "deferred" >>>> messages. >>> Hmm not entirely sure how we prevent that happening wherever it is done.. >>> >> >> Outch. Better at the source, though. I didn't look at the iio code recently, >> but can you detect the defer situation at least with devicetree ? >> >> For non-devicetree situations, the only option I can think of would be >> to replace the module initcall with a later initcall. That should solve >> the problem if both iio_hwmon and and underlying drivers are built >> into the kernel. If iio_hwmon is modular, the only real option I can >> see is to make sure that all drivers it needs are loaded first. >> >> Does this make sense ? > I think we need to look in a couple of directions. Firstly, investigate doing > something similar to gpio and basically move the setup of maps much earlier. > This will replace drivers presenting their own maps > > The other direction is to get userspace (i.e. configfs) setup of these maps > working so for cases where it's a bit less hardware defined (i.e. using an > accelerometer as an input device) we can do once we know all devices relevant > are present and instantiate new instances on the fly. > > Anyhow, neither is trivial unfortunately. > On a higher level, I think it would be better if the iio-hwmon bridge was tied to chips (and thus to the chip driver), and not be an independent binding. No idea if/how we can do that, though. Guenter
diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c index b550ba5..c0da4d9 100644 --- a/drivers/hwmon/iio_hwmon.c +++ b/drivers/hwmon/iio_hwmon.c @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev) name = dev->of_node->name; channels = iio_channel_get_all(dev); - if (IS_ERR(channels)) + if (IS_ERR(channels)) { + if (PTR_ERR(channels) == -ENODEV) + return -EPROBE_DEFER; return PTR_ERR(channels); + } st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL); if (st == NULL) {
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. It is better to defer the probe of iio_hwmon if such error is returned by iio_channel_get_all in order to let a chance to iio drivers to expose channels in iio_map_list. Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> --- drivers/hwmon/iio_hwmon.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)