Re: [RFC PATCH 0/6] iio: inkern: make interface firmware agnostic

Re: [RFC PATCH 0/6] iio: inkern: make interface firmware agnostic

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:03 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> The main goal of this patchset is to convert the iio inkern interface to
> be firmware agnostic.
>
> In the process of doing it, I discover the bug solved in patch 1/6 which
> also served as motivation for patch 2/6.
>
> Note that patch 3/6 is touching multiple files at once since the changes
> are very simple. I can separate it in several patches if that is
> preferred for better tagging though.
>
> The same can be said for patch 5/6 even though in this case it will be
> more cumbersome to separate it in several patches (we will need to
> temporarily support both options; convert each driver separately;
> remove of). I think the changes in the drivers are simple enough that
> we can have it like it is now...
>
> Lastly, this is only compile tested with allyesconfig for arm and arm64.
> While I surely can come up with some dummy devices to make sure I can still
> properly get IIO channels, having proper tested-by tags on platforms
> relying on this interface would be very appreciated (and I suspect Jonathan
> will require it).

Thanks! I will definitely have a look into it later on.

> Nuno Sá (6):
>   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: treewide: explicitly add proper header files
>   iio: inkern: split of_iio_channel_get_by_name()
>   iio: inkern: move to fwnode properties
>   iio: inkern: fix coding style warnings
>
>  drivers/iio/adc/ab8500-gpadc.c                |   7 +-
>  drivers/iio/adc/ad7606.c                      |   1 +
>  drivers/iio/adc/ad7606_par.c                  |   1 +
>  drivers/iio/adc/at91-sama5d2_adc.c            |   7 +-
>  drivers/iio/adc/berlin2-adc.c                 |   2 +
>  drivers/iio/adc/imx7d_adc.c                   |   1 +
>  drivers/iio/adc/imx8qxp-adc.c                 |   1 +
>  drivers/iio/adc/ingenic-adc.c                 |  10 +-
>  drivers/iio/adc/mp2629_adc.c                  |   1 +
>  drivers/iio/adc/mt6360-adc.c                  |   1 +
>  drivers/iio/adc/npcm_adc.c                    |   1 +
>  drivers/iio/adc/qcom-pm8xxx-xoadc.c           |  14 +-
>  drivers/iio/adc/qcom-spmi-adc5.c              |  13 +-
>  drivers/iio/adc/qcom-spmi-vadc.c              |   7 +-
>  drivers/iio/adc/rzg2l_adc.c                   |   1 +
>  drivers/iio/adc/stm32-adc.c                   |   7 +-
>  .../cros_ec_sensors/cros_ec_lid_angle.c       |   1 +
>  .../common/cros_ec_sensors/cros_ec_sensors.c  |   1 +
>  drivers/iio/dac/stm32-dac.c                   |   2 +
>  drivers/iio/dac/vf610_dac.c                   |   1 +
>  drivers/iio/humidity/hts221_buffer.c          |   1 +
>  drivers/iio/inkern.c                          | 240 ++++++++++--------
>  drivers/iio/light/cros_ec_light_prox.c        |   1 +
>  drivers/iio/pressure/cros_ec_baro.c           |   1 +
>  drivers/iio/trigger/stm32-lptimer-trigger.c   |   1 +
>  drivers/thermal/qcom/qcom-spmi-adc-tm5.c      |   3 +-
>  include/linux/iio/consumer.h                  |  28 +-
>  include/linux/iio/iio.h                       |   7 +-
>  28 files changed, 200 insertions(+), 162 deletions(-)
>
> --
> 2.36.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/6] iio: inkern: make interface firmware agnostic

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:03 PM Nuno Sá <nuno.sa@analog.com> wrote:


You have in the addresses:

Gwendal Grignou --cc=Dmitry Baryshkov
<gwendal@chromium.orgdmitry.baryshkov@linaro.org>,

I would recommend using my "smart" script [1] that makes the list automatically.

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

P.S. Patches are welcome as usual :)

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 0/6] iio: inkern: make interface firmware agnostic

[Nuno Sá]

On Thu, 2022-06-02 at 16:32 +0200, Andy Shevchenko wrote:
> On Thu, Jun 2, 2022 at 4:03 PM Nuno Sá <nuno.sa@analog.com> wrote:
> 
> 
> You have in the addresses:
> 
> Gwendal Grignou --cc=Dmitry Baryshkov
> <gwendal@chromium.orgdmitry.baryshkov@linaro.org>,
> 

Oh crap :facepalm:

> I would recommend using my "smart" script [1] that makes the list
> automatically.
> 
> [1]:
> https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh
> 

Interesting, though I'm giving a try to git-publish [1] and integrating
it in your script might not make sense. Still there's definitely some
stuff that I can use from your script to keep me doing this again.

That said, What's worst is that, somehow, the patches still did not
make it into the mailing list. I will wait a bit longer and try to
resend them...

[1]: https://github.com/stefanha/git-publish

- Nuno Sá

Re: [RFC PATCH 1/6] iio: inkern: fix return value in devm_of_iio_channel_get_by_name()

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> of_iio_channel_get_by_name() can either return NULL or an error pointer
> so that only doing IS_ERR() is not enough. Fix it by checking the NULL
> pointer case and return -ENODEV in that case. Note this is done like this
> so that users of the function (which only check for error pointers) do
> not need to be changed. This is not ideal since we are losing error codes
> and as such, in a follow up change, things will be unified so that
> of_iio_channel_get_by_name() only returns error codes.

...

>         channel = of_iio_channel_get_by_name(np, channel_name);
> -       if (IS_ERR(channel))
> +       if (IS_ERR_OR_NULL(channel)) {
> +               if (!channel)
> +                       return ERR_PTR(-ENODEV);
>                 return channel;
> +       }

Why not make it not nested, i.e. just adding two lines after the existing check?
if (!channel)
  return -ENODEV;


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 3/6] iio: treewide: explicitly add proper header files

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Apparently some drivers are relying on the fact that iio.h includes of.h
> that in turn includes all the headers these drivers were relying on. Fix
> it so that in a following patch we can make iio firmware agnostic and
> remove of.h from iio.h.

I believe it should be split on driver-basis.

...

> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -18,6 +18,8 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/of.h>

Ordering?

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 4/6] iio: inkern: split of_iio_channel_get_by_name()

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> This change splits of_iio_channel_get_by_name() so that it decouples
> looking for channels in the current node from looking in it's parents
> nodes. This will be helpful when moving to fwnode properties where we
> need to release the handles when looking for channels in parent's nodes.

...

> +       /*
> +        * For named iio channels, first look up the name in the
> +        * "io-channel-names" property.  If it cannot be found, the
> +        * index will be an error code, and of_iio_channel_get()
> +        * will fail.
> +        */
> +       if (name)
> +               index = of_property_match_string(np, "io-channel-names", name);
> +
> +       chan = of_iio_channel_get(np, index);
> +       if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) {
> +               *parent_lookup = false;
> +       } else if (name && index >= 0) {

> +               pr_err("ERROR: could not get IIO channel %pOF:%s(%i)\n",
> +                      np, name ? name : "", index);

It's one TAB less now, means you may compress more on one line,
including replacing
name ? name : "" --> name ?: ""

> +               *parent_lookup = false;
> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 5/6] iio: inkern: move to fwnode properties

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> This moves the IIO in kernel interface to use fwnode properties and thus
> be firmware agnostic.
>
> All the users had to be naturally updated to the new interface exposed by
> IIO.

I think you may split this in an easy way, i.e. convert core to
fwnode, while providing inliners for of_node cases (like it's done in
IRQ domain) and then remove them after conversion.

I think of_xlate is not needed to be touched at all. Let it die with
OF altogether. Yes, it won't be fully OF-independent, but it will down
the scope of the next change where you can convert of_xlate to
something agnostic.

...

> --- a/drivers/iio/adc/ab8500-gpadc.c
> +++ b/drivers/iio/adc/ab8500-gpadc.c
> @@ -39,6 +39,7 @@
>  #include <linux/slab.h>
>  #include <linux/mfd/abx500.h>
>  #include <linux/mfd/abx500/ab8500.h>
> +#include <linux/fwnode.h>

Ordering.

...

> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> @@ -21,6 +21,7 @@
>  #include <linux/init.h>
>  #include <linux/interrupt.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/fwnode.h>

Ordering?

...

>   * @consumer_channel:  Unique name to identify the channel on the consumer
>   *                     side. This typically describes the channels use within

used / usage ?

...

>   * @consumer_channel:  Unique name to identify the channel on the consumer
>   *                     side. This typically describes the channels use within

Ditto.

>   *                     the consumer. E.g. 'battery_voltage'

...

> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 233d2e6b7721..18ca5a7cb154 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -10,13 +10,14 @@
>  #include <linux/device.h>
>  #include <linux/cdev.h>
>  #include <linux/iio/types.h>
> -#include <linux/of.h>

You may split this change easily since there is nothing from of.h in
use. Just add forward declaration as you have done, but for the OF
case.

...

That said, I think what you need is to split this series to three logical parts:
1) shuffle header inclusions around so, iio.h will use forward
declaration (on driver basis);
2) convert inkern.c to fwnode while providing OF wrappers (to_of_node() helps);
3) convert of_xlate (on driver basis it might be tricky, up to you).

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 5/6] iio: inkern: move to fwnode properties

[Andy Shevchenko]

On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> +       parent = fwnode_get_parent(fwnode);
> +       while (parent) {

> +               parent = fwnode_get_next_parent(parent);
>         }

Forgot to mention:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87ffea09470d94c93dd6a5a22d4b2216b395d1ea

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 1/6] iio: inkern: fix return value in devm_of_iio_channel_get_by_name()

[Nuno Sá]

On Fri, 2022-06-03 at 13:16 +0200, Andy Shevchenko wrote:
> On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > of_iio_channel_get_by_name() can either return NULL or an error
> > pointer
> > so that only doing IS_ERR() is not enough. Fix it by checking the
> > NULL
> > pointer case and return -ENODEV in that case. Note this is done
> > like this
> > so that users of the function (which only check for error pointers)
> > do
> > not need to be changed. This is not ideal since we are losing error
> > codes
> > and as such, in a follow up change, things will be unified so that
> > of_iio_channel_get_by_name() only returns error codes.
> 
> ...
> 
> >         channel = of_iio_channel_get_by_name(np, channel_name);
> > -       if (IS_ERR(channel))
> > +       if (IS_ERR_OR_NULL(channel)) {
> > +               if (!channel)
> > +                       return ERR_PTR(-ENODEV);
> >                 return channel;
> > +       }
> 
> Why not make it not nested, i.e. just adding two lines after the
> existing check?
> if (!channel)
>   return -ENODEV;
> 
> 

I see, well yeah I guess I can do it so the diff is even smaller...

- Nuno Sá

Re: [RFC PATCH 4/6] iio: inkern: split of_iio_channel_get_by_name()

[Nuno Sá]

On Fri, 2022-06-03 at 13:37 +0200, Andy Shevchenko wrote:
> On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > This change splits of_iio_channel_get_by_name() so that it
> > decouples
> > looking for channels in the current node from looking in it's
> > parents
> > nodes. This will be helpful when moving to fwnode properties where
> > we
> > need to release the handles when looking for channels in parent's
> > nodes.
> 
> ...
> 
> > +       /*
> > +        * For named iio channels, first look up the name in the
> > +        * "io-channel-names" property.  If it cannot be found, the
> > +        * index will be an error code, and of_iio_channel_get()
> > +        * will fail.
> > +        */
> > +       if (name)
> > +               index = of_property_match_string(np, "io-channel-
> > names", name);
> > +
> > +       chan = of_iio_channel_get(np, index);
> > +       if (!IS_ERR(chan) || PTR_ERR(chan) == -EPROBE_DEFER) {
> > +               *parent_lookup = false;
> > +       } else if (name && index >= 0) {
> 
> > +               pr_err("ERROR: could not get IIO channel
> > %pOF:%s(%i)\n",
> > +                      np, name ? name : "", index);
> 
> It's one TAB less now, means you may compress more on one line,
> including replacing
> name ? name : "" --> name ?: ""
> 

I can taking the 100col limit... AFAIR, Jonathan wants to stick to the
old limit unless readability is severely impacted by it which I don't
think it' the case here.

That said, I'm more than fine in using the new limit as long as
Jonathan agrees with it.
> 

- Nuno Sá

Re: [RFC PATCH 5/6] iio: inkern: move to fwnode properties

[Nuno Sá]

On Fri, 2022-06-03 at 13:52 +0200, Andy Shevchenko wrote:
> On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > 
> > This moves the IIO in kernel interface to use fwnode properties and
> > thus
> > be firmware agnostic.
> > 
> > All the users had to be naturally updated to the new interface
> > exposed by
> > IIO.
> 
> I think you may split this in an easy way, i.e. convert core to
> fwnode, while providing inliners for of_node cases (like it's done in
> IRQ domain) and then remove them after conversion.
> 

I see, in our case that might be really simple as we only have one user
of devm_of_iio_channel_get_by_name() which is the only api directly
using OF. of_iio_channel_get_by_name() has no users and all the other
public APIs use 'struct device' so we can do the conversion
internally...

> I think of_xlate is not needed to be touched at all. Let it die with
> OF altogether. Yes, it won't be fully OF-independent, but it will
> down
> the scope of the next change where you can convert of_xlate to
> something agnostic.
> 
> ...
> 
> > --- a/drivers/iio/adc/ab8500-gpadc.c
> > +++ b/drivers/iio/adc/ab8500-gpadc.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/mfd/abx500.h>
> >  #include <linux/mfd/abx500/ab8500.h>
> > +#include <linux/fwnode.h>
> 
> Ordering.
> 

The ordering is completely wrong anyways. So, I did not cared about
ordering in drivers where it was already bad. Don't mind to fix it
while adding the missing headers (if acceptable).

> ...
> 
> > --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> > +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/init.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/regulator/consumer.h>
> > +#include <linux/fwnode.h>
> 
> Ordering?
> 
> ...
> 
> >   * @consumer_channel:  Unique name to identify the channel on the
> > consumer
> >   *                     side. This typically describes the channels
> > use within
> 
> used / usage ?
> 
> ...
> 
> >   * @consumer_channel:  Unique name to identify the channel on the
> > consumer
> >   *                     side. This typically describes the channels
> > use within
> 
> Ditto.
> 
> >   *                     the consumer. E.g. 'battery_voltage'
> 
> ...
> 
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 233d2e6b7721..18ca5a7cb154 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -10,13 +10,14 @@
> >  #include <linux/device.h>
> >  #include <linux/cdev.h>
> >  #include <linux/iio/types.h>
> > -#include <linux/of.h>
> 
> You may split this change easily since there is nothing from of.h in
> use. Just add forward declaration as you have done, but for the OF
> case.
> 
> ...
> 
> That said, I think what you need is to split this series to three
> logical parts:
> 1) shuffle header inclusions around so, iio.h will use forward
> declaration (on driver basis);
> 2) convert inkern.c to fwnode while providing OF wrappers
> (to_of_node() helps);

Just to be clear, we should still add an fwnode_xlate() callback? So we
have both temporarily and if some new driver needs this interface it
can already use it instead of of_xlate...

> 3) convert of_xlate (on driver basis it might be tricky, up to you).
> 

Yeah, I might see how easy it is to fully convert the drivers using 
of_xlate. If easy enough, I'll probably do it...

- Nuno Sá

Re: [RFC PATCH 5/6] iio: inkern: move to fwnode properties

[Nuno Sá]

On Fri, 2022-06-03 at 13:57 +0200, Andy Shevchenko wrote:
> On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:
> 
> ...
> 
> > +       parent = fwnode_get_parent(fwnode);
> > +       while (parent) {
> 
> > +               parent = fwnode_get_next_parent(parent);
> >         }
> 
> Forgot to mention:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87ffea09470d94c93dd6a5a22d4b2216b395d1ea
> 

I did looked for something like that but it's still not in the IIO
testing tree.

(still I actually followed that patchset but completely forgot about
the helper)

- Nuno Sá

Re: [RFC PATCH 5/6] iio: inkern: move to fwnode properties

[Andy Shevchenko]

On Fri, Jun 3, 2022 at 2:53 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Fri, 2022-06-03 at 13:57 +0200, Andy Shevchenko wrote:
> > On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > +       parent = fwnode_get_parent(fwnode);
> > > +       while (parent) {
> >
> > > +               parent = fwnode_get_next_parent(parent);
> > >         }
> >
> > Forgot to mention:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=87ffea09470d94c93dd6a5a22d4b2216b395d1ea
>
> I did looked for something like that but it's still not in the IIO
> testing tree.

Now it's in the upstream, which means it is just a matter of time to
appear in IIO testing.

> (still I actually followed that patchset but completely forgot about
> the helper)

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 5/6] iio: inkern: move to fwnode properties

[Andy Shevchenko]

On Fri, Jun 3, 2022 at 2:51 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Fri, 2022-06-03 at 13:52 +0200, Andy Shevchenko wrote:
> > On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > I think you may split this in an easy way, i.e. convert core to
> > fwnode, while providing inliners for of_node cases (like it's done in
> > IRQ domain) and then remove them after conversion.
>
> I see, in our case that might be really simple as we only have one user
> of devm_of_iio_channel_get_by_name() which is the only api directly
> using OF. of_iio_channel_get_by_name() has no users and all the other
> public APIs use 'struct device' so we can do the conversion
> internally...

Even better than I expected!

...

> > That said, I think what you need is to split this series to three
> > logical parts:
> > 1) shuffle header inclusions around so, iio.h will use forward
> > declaration (on driver basis);
> > 2) convert inkern.c to fwnode while providing OF wrappers
> > (to_of_node() helps);
>
> Just to be clear, we should still add an fwnode_xlate() callback? So we
> have both temporarily and if some new driver needs this interface it
> can already use it instead of of_xlate...

No, I mean to leave a callback to be OF-specific and pass fwnode with
to_of_node(). Then convert it separately either on per driver basis
(if possible and makes sense) or altogether.

> > 3) convert of_xlate (on driver basis it might be tricky, up to you).
>
> Yeah, I might see how easy it is to fully convert the drivers using
> of_xlate. If easy enough, I'll probably do it...

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 1/6] iio: inkern: fix return value in devm_of_iio_channel_get_by_name()

[Jonathan Cameron]

On Fri, 03 Jun 2022 14:36:15 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2022-06-03 at 13:16 +0200, Andy Shevchenko wrote:
> > On Thu, Jun 2, 2022 at 4:04 PM Nuno Sá <nuno.sa@analog.com> wrote:  
> > > 
> > > of_iio_channel_get_by_name() can either return NULL or an error
> > > pointer
> > > so that only doing IS_ERR() is not enough. Fix it by checking the
> > > NULL
> > > pointer case and return -ENODEV in that case. Note this is done
> > > like this
> > > so that users of the function (which only check for error pointers)
> > > do
> > > not need to be changed. This is not ideal since we are losing error
> > > codes
> > > and as such, in a follow up change, things will be unified so that
> > > of_iio_channel_get_by_name() only returns error codes.  
> > 
> > ...
> >   
> > >         channel = of_iio_channel_get_by_name(np, channel_name);
> > > -       if (IS_ERR(channel))
> > > +       if (IS_ERR_OR_NULL(channel)) {
> > > +               if (!channel)
> > > +                       return ERR_PTR(-ENODEV);
> > >                 return channel;
> > > +       }  
> > 
> > Why not make it not nested, i.e. just adding two lines after the
> > existing check?
> > if (!channel)
> >   return -ENODEV;
> > 
> >   
> 
> I see, well yeah I guess I can do it so the diff is even smaller...

I'd prefer the way Andy suggested as well due to the slightly simpler control
flow.

Thanks,

Jonathan

> 
> - Nuno Sá
>