| Message ID | 20220711123835.811358-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | make iio inkern interface firmware agnostic | expand |
On Mon, 2022-07-11 at 15:22 +0200, Andy Shevchenko wrote: > On Mon, Jul 11, 2022 at 2:38 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > First version of the series can be found here: > > > > https://lore.kernel.org/linux-iio/20220610084545.547700-1-nuno.sa@analog.com/ > > I'm under the impression that I gave tags for some of these patches > when they were the part of the bigger series. Am I wrong? > In any case for patch 6-14, > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Yes, you're right. Should I have dropped them? - Nuno Sá
On Mon, 2022-07-11 at 15:29 +0200, Andy Shevchenko wrote: > On Mon, Jul 11, 2022 at 2:38 PM Nuno Sá <nuno.sa@analog.com> wrote: > > > > APIs like of_iio_channel_get_by_name() and of_iio_channel_get_all() > > were > > returning a mix of NULL and pointers with NULL being the way to > > "notify" that we should do a "system" lookup for channels. This > > make > > it very confusing and prone to errors as commit 9f63cc0921ec > > ("iio: inkern: fix return value in > > devm_of_iio_channel_get_by_name()") > > proves. On top of this, patterns like 'if (channel != NULL) return > > channel' were being used where channel could actually be an error > > code > > which makes the code hard to read. > > > > This change also makes some functional changes on how errors were > > being > > handled. In the original behavior, even if we get an error like '- > > ENOMEM', > > we still continue with the search. We should only continue to > > lookup for > > the channel when it makes sense to do so. Hence, the main error > > handling > > in 'of_iio_channel_get_by_name()' is changed to the following > > logic: > > > > * If a channel 'name' is provided and we do find it via > > 'io-channel-names', we should be able to get it. If we get any > > error, > > we should not proceed with the lookup. Moreover, we should return > > an error > > so that callers won't proceed with a system lookup. > > * If a channel 'name' is provided and we cannot find it ('index < > > 0'), > > 'of_parse_phandle_with_args()' is expected to fail with '-EINVAL'. > > Hence, > > we should only continue if we get that error. > > * If a channel 'name' is not provided we should only carry on with > > the > > search if 'of_parse_phandle_with_args()' returns '-ENOENT'. > > > > Also note that a system channel lookup is only done if the returned > > error code (from 'of_iio_channel_get_by_name()' or > > 'of_iio_channel_get_all()' is -ENODEV. > > LGTM (but I might miss something, it's a bit too many conditionals), > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Agreed. It ended up being more complicated than I thought... - Nuno Sá
First version of the series can be found here: https://lore.kernel.org/linux-iio/20220610084545.547700-1-nuno.sa@analog.com/ v2 changes: [1/15] * Fix typo and added more description in the commit message. [3/15] * Remove superfluous code; * Commit message spell fixes and added more details; * Improved error handling (this is the most significant change in this version. More details on the commit message). [4/15] * Drop the 'ugly' parent_lookup flag. With the new error handling, we can use -ENODEV to infer if we should proceed or not with the lookup. [5/15]: * Moved some local declarations up so long lines first; * Use 'bus_find_device_by_fwnode()'; * Proper ordering in includes. * Adapted error handling in '__fwnode_iio_channel_get_by_name()' taking ACPI into account and when 'name' is given but index < 0. It seems that ACPI code can actually return -ENOENT with index < 0 for which case we should continue the search. Not sure if a check in ACPI ('if (index < 0) return -EINVAL;) like is done in OF would make sense... [12/15]: * Use 'device_property_count_u64()' to get the number of diff channels. So no need for 'magic' divisions by 2 (no idea why I haven't done like this in the first place). [15/15] * Fix wrong conversion of 'if (ptr != NULL)' to 'if (!ptr)'. Special note for patch 3/15 where -ENODEV is still used despite some talks about using -ENOENT and hence, be more in line with firmware code. The reason I kept it was to be consistent with the rest of the file. I'd say that if we want to move to -ENOENT we should do it in a separate patch and for the complete file. Nuno Sá (15): iio: inkern: only release the device node when done with it iio: inkern: fix return value in devm_of_iio_channel_get_by_name() iio: inkern: only return error codes in iio_channel_get_*() APIs iio: inkern: split of_iio_channel_get_by_name() iio: inkern: move to fwnode properties thermal: qcom: qcom-spmi-adc-tm5: convert to IIO fwnode API iio: adc: ingenic-adc: convert to IIO fwnode interface iio: adc: ab8500-gpadc: convert to device properties iio: adc: at91-sama5d2_adc: convert to device properties iio: adc: qcom-pm8xxx-xoadc: convert to device properties iio: adc: qcom-spmi-vadc: convert to device properties iio: adc: qcom-spmi-adc5: convert to device properties iio: adc: stm32-adc: convert to device properties iio: inkern: remove OF dependencies iio: inkern: fix coding style warnings drivers/iio/adc/ab8500-gpadc.c | 27 +-- drivers/iio/adc/at91-sama5d2_adc.c | 30 +-- drivers/iio/adc/ingenic-adc.c | 8 +- drivers/iio/adc/qcom-pm8xxx-xoadc.c | 58 +++-- drivers/iio/adc/qcom-spmi-adc5.c | 63 +++--- drivers/iio/adc/qcom-spmi-vadc.c | 44 ++-- drivers/iio/adc/stm32-adc.c | 121 +++++----- drivers/iio/inkern.c | 271 +++++++++++++---------- drivers/thermal/qcom/qcom-spmi-adc-tm5.c | 3 +- include/linux/iio/consumer.h | 28 +-- include/linux/iio/iio.h | 8 +- 11 files changed, 347 insertions(+), 314 deletions(-)