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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 13 Jun 2022 09:13:00 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2022-06-11 at 16:32 +0100, Jonathan Cameron wrote:
> > 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.
> >   
> 
> can add the warning... And AFAIU, we can go with Andy suggestion right?

Yes, I think so...   Might chance my mind when I see the code though :)

Jonathan



> 
> - Nuno Sá
>