Re: [PATCH v2 01/12] iio:dac:ad5755: Switch to generic firmware properties and drop pdata

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 5 Dec 2021 17:36:03 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Dec 5, 2021 at 5:02 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Dec 4, 2021 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> 
> ...
> 
> > > +       const struct ad5755_platform_data *pdata = NULL;
> > >         struct iio_dev *indio_dev;
> > >         struct ad5755_state *st;
> > >         int ret;  
> >  
> > > -       if (spi->dev.of_node)
> > > -               pdata = ad5755_parse_dt(&spi->dev);
> > > -       else
> > > -               pdata = spi->dev.platform_data;
> > > +       if (dev_fwnode(&spi->dev))
> > > +               pdata = ad5755_parse_fw(&spi->dev);
> > >
> > >         if (!pdata) {
> > > -               dev_warn(&spi->dev, "no platform data? using default\n");
> > > +               dev_warn(&spi->dev,
> > > +                        "no firmware provided parameters? using default\n");  
> >
> > It's fine to have it on one line (and not related to the 80 vs 100
> > case, it's about string literal as the last argument).
> >  
> > >                 pdata = &ad5755_default_pdata;
> > >         }  
> >
> >
> > Perhaps
> >
> >     const struct ad5755_platform_data *pdata;
> >     ...
> >     if (dev_fwnode(...))
> >       pdata = ...
> >     else
> >       pdata = &_default;
> >     if (pdata == &_default)
> >       dev_warn(...);
> >
> > ?  
> 
> After reading it again, I realized that pdata may be NULL in fwnode
> case. So, original code is fine, but I would rather move NULL
> assignment closer to the conditional (up to you, I'm fine with either,
> i.o.w. you may ignore this comment).
> 
I think a nicer way to tidy this up given I agree it's no immediately obvious
what is going on is to push

if (!dev_fwnode())
	return NULL;

into ad5755_parse_fw() then the flow here will be more obvious as just

	pdata = ad5755_parse_fw();
	if (!pdata) {
		dev_warn();
		pdata = &_default;
	}