Re: [PATCH 24/34] iio: inkern: move to fwnode properties

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 11 Jun 2022 16:30:57 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 10 Jun 2022 22:01:09 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Fri, 2022-06-10 at 17:19 +0200, Andy Shevchenko wrote:  
> > > On Fri, Jun 10, 2022 at 10:48 AM Nuno Sá <nuno.sa@analog.com> wrote:    
> > > > 
> > > > This moves the IIO in kernel interface to use fwnode properties and
> > > > thus
> > > > be firmware agnostic.
> > > > 
> > > > Note that the interface is still not firmware agnostic. At this
> > > > point we
> > > > have both OF and fwnode interfaces so that we don't break any user.
> > > > On
> > > > top of this we also want to have a per driver conversion and that
> > > > is the
> > > > main reason we have both of_xlate() and fwnode_xlate() support.    
> > > 
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Thanks!
> > > 
> > > A few nit-picks below, though.
> > >     
> ...
> 
> >   
> > >     
> > > > -       err = of_parse_phandle_with_args(np, "io-channels",
> > > > -                                        "#io-channel-cells",
> > > > -                                        index, &iiospec);
> > > > +       err = fwnode_property_get_reference_args(fwnode, "io-
> > > > channels",
> > > > +                                                "#io-channel-
> > > > cells", 0,
> > > > +                                                index, &iiospec);
> > > >         if (err)
> > > >                 return err;
> > > > 
> > > > -       idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> > > > +       idev = bus_find_device(&iio_bus_type, NULL, iiospec.fwnode,
> > > >                                iio_dev_node_match);    
> > > 
> > > Wondering if this
> > > https://elixir.bootlin.com/linux/v5.19-rc1/C/ident/bus_find_device_by_fwnode
> > > can be utilized (yes, I noticed iio_device_type above).    
> > 
> > Hmm, at first glance I would say we can use it. AFAICT, we are already
> > grabbing a node which contains "#io-channel-cells" which is very
> > indicative that is an IIO device. I also find it very unlikely to have
> > two IIO devices with the same fwnode (I guess it would be an issue even
> > in the old code) and even more unlikely two devices of diferent types
> > with the same fwnode?  
> 
> If we are talking struct iio_dev instances, then there are quite a few cases
> where there are multiple with the same fwnode.  We had to do that pre
> multiple buffers being introduced so it's fairly common, though not in
> ADCs which is probably why we haven't seen breakage here. Not sure how
> broken things already are as a result or if any of those devices (most
> IMUs) provide #io-channel-cells etc.  I'd put that breakage down as
> one to look into if anyone every hits it or one of us is bored enough
> to poke at it.  (superficially I think we'd have to check all matches
> for an xlate success).

Having said that adding a warning comment here, so we remember this is
a potential issue would be great.

> 
> Also, possible (I'm not totally sure) that we have other subdevices using
> the same firmware node, such as triggers.  I can't immediately think
> of why they would need it, but I'd rather we were at least partly protected
> against that.
> 
> > 
> > Anyways, I guess Jonathan can help in here...
> > 
> >   
> > >     
> > > >         if (idev == NULL) {
> > > > -               of_node_put(iiospec.np);
> > > > +               fwnode_handle_put(iiospec.fwnode);
> > > >                 return -EPROBE_DEFER;
> > > >         }
> > > > 
> > > >         indio_dev = dev_to_iio_dev(idev);
> > > >         channel->indio_dev = indio_dev;
> > > >         if (indio_dev->info->of_xlate)
> > > > -               index = indio_dev->info->of_xlate(indio_dev,
> > > > &iiospec);
> > > > +               index = __fwnode_to_of_xlate(indio_dev, &iiospec);
> > > > +       else if (indio_dev->info->fwnode_xlate)
> > > > +               index = indio_dev->info->fwnode_xlate(indio_dev,
> > > > &iiospec);
> > > >         else
> > > > -               index = __of_iio_simple_xlate(indio_dev, &iiospec);
> > > > -       of_node_put(iiospec.np);
> > > > +               index = __fwnode_iio_simple_xlate(indio_dev,
> > > > &iiospec);
> > > > +       fwnode_handle_put(iiospec.fwnode);
> > > >         if (index < 0)
> > > >                 goto err_put;
> > > >         channel->channel = &indio_dev->channels[index];
> > > > @@ -188,7 +209,8 @@ static int __of_iio_channel_get(struct
> > > > iio_channel *channel,
> > > >         return index;
> > > >  }  
> 
> > >     
> > > >                 *parent_lookup = false;
> > > >         }
> > > > 
> > > >         return chan;
> > > >  }
> > > > 
> > > > -struct iio_channel *of_iio_channel_get_by_name(struct device_node
> > > > *np,
> > > > -                                              const char *name)
> > > > +struct iio_channel *fwnode_iio_channel_get_by_name(struct
> > > > fwnode_handle *fwnode,
> > > > +                                                  const char
> > > > *name)
> > > >  {
> > > >         struct iio_channel *chan;
> > > > +       struct fwnode_handle *parent;
> > > >         bool parent_lookup = true;
> > > > 
> > > >         /* Walk up the tree of devices looking for a matching iio
> > > > channel */
> > > > -       chan = __of_iio_channel_get_by_name(np, name,
> > > > &parent_lookup);
> > > > +       chan = __fwnode_iio_channel_get_by_name(fwnode, name,
> > > > &parent_lookup);
> > > >         if (!parent_lookup)
> > > >                 return chan;
> > > > 
> > > > @@ -255,33 +279,34 @@ struct iio_channel
> > > > *of_iio_channel_get_by_name(struct device_node *np,
> > > >          * If the parent node has a "io-channel-ranges" property,
> > > >          * then we can try one of its channels.
> > > >          */
> > > > -       np = np->parent;
> > > > -       while (np) {
> > > > -               if (!of_get_property(np, "io-channel-ranges",
> > > > NULL))
> > > > +       fwnode_for_each_parent_node(fwnode, parent) {
> > > > +               if (!fwnode_property_present(parent, "io-channel-
> > > > ranges")) {
> > > > +                       fwnode_handle_put(parent);
> > > >                         return chan;    
> > > 
> > > break; ?    
> > 
> > The return in place was a request from Jonathan in the RFC...  
> 
> :)  I prefer not having to scroll down when we can get out quickly.
> 
> >   
> > > 
> > > (Yes, I understand pros and cons of each variant, up to you)
> > >     
> > > > +               }
> > > > 
> > > > -               chan = __of_iio_channel_get_by_name(np, name,
> > > > &parent_lookup);
> > > > -               if (!parent_lookup)
> > > > +               chan = __fwnode_iio_channel_get_by_name(parent,
> > > > name, &parent_lookup);
> > > > +               if (!parent_lookup) {
> > > > +                       fwnode_handle_put(parent);
> > > >                         return chan;    
> > > 
> > > Ditto.
> > >     
> > > > -               np = np->parent;
> > > > +               }
> > > >         }
> > > > 
> > > >         return chan;
> > > >  }
> > > > -EXPORT_SYMBOL_GPL(of_iio_channel_get_by_name);
> > > > +EXPORT_SYMBOL_GPL(fwnode_iio_channel_get_by_name);    
> > > 
> > > Wondering if we may move this to the IIO namespace.    
> > 
> > I guess it makes sense but surely on a different patch...  
> 
> Yup - moving to a IIO namespace is a work in progress (got snarled
> up by the PM related macros needed for some of the sub namespaces
> which is now sorted)  Let's do it after this.
> 
>