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).
--
With Best Regards,
Andy Shevchenko
