Re: [PATCH 22/34] iio: inkern: only return error codes in iio_channel_get_*() APIs

[Jonathan Cameron <jic23@kernel.org>]

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

> On Sat, 2022-06-11 at 16:17 +0100, Jonathan Cameron wrote:
> > On Fri, 10 Jun 2022 10:45:33 +0200
> > 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 error pointers being NULL the way to  
> > 
> > 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 dbbccf7c20bf
> > > ("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.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  drivers/iio/inkern.c | 24 +++++++++++-------------
> > >  1 file changed, 11 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > > index 87fd2a0d44f2..31d9c122199a 100644
> > > --- a/drivers/iio/inkern.c
> > > +++ b/drivers/iio/inkern.c
> > > @@ -214,7 +214,7 @@ static struct iio_channel
> > > *of_iio_channel_get(struct device_node *np, int index)
> > >  struct iio_channel *of_iio_channel_get_by_name(struct device_node
> > > *np,
> > >                                                const char *name)
> > >  {
> > > -       struct iio_channel *chan = NULL;
> > > +       struct iio_channel *chan;
> > >  
> > >         /* Walk up the tree of devices looking for a matching iio
> > > channel */
> > >         while (np) {
> > > @@ -231,11 +231,11 @@ struct iio_channel
> > > *of_iio_channel_get_by_name(struct device_node *np,
> > >                                                          name);
> > >                 chan = of_iio_channel_get(np, index);
> > >                 if (!IS_ERR(chan) || PTR_ERR(chan) == -
> > > EPROBE_DEFER)
> > > -                       break;  
> > 
> > This original behaviour is 'interesting'. If we get a error like -
> > ENOMEM
> > we should return it rather than carry on.  Do we have enough
> > knowledge
> > of possible errors here to be more explicit on when we keep looking
> > up
> > the tree?  I think we can get -ENOENT from
> > of_parse_phandle_with_args()
> > 
> > That raises an interesting question on whether -ENODEV is the right
> > response
> > for the previously NULL case or is -ENOENT more consistent with other
> > of_ functions?  No device could be thought of as being the case that
> > needs
> > to defer (in hope it turns up later) whereas no entry means it will
> > never
> > succeed.  
> 
> From what I could see, of_parse_phandle_with_args() either returns 
> -EINVAL or -ENOENT. We also have the internal of_iio_channel_get()
> which can return -ENOMEM. So I guess we should only continue looking if
> we get -ENOENT?
> 
> To be clear, do you still prefer to explicitly return -ENODEV in the
> previous NULL cases or should we honor the return code from 
> of_parse_phandle_with_args() and just return chans (and thus ENOENT)?
You've looked at this more than me, so whilst I think -ENOENT is probably
slightly more consistent I'll go with whatever you conclude is the
best option.  Maybe add a small amount of description on what you chose
and why to the relevant patch descriptions.

Thanks,

Jonathan


> 
> - Nuno Sá