On Sun, 5 Dec 2021 17:36:03 +0200
Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Sun, Dec 5, 2021 at 5:02 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> > On Sat, Dec 4, 2021 at 7:07 PM Jonathan Cameron <jic23@xxxxxxxxxx> 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;
}
