Re: [PATCH 1/2] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, Aug 29, 2025 at 2:41 AM David Lechner <dlechner@baylibre.com> wrote:
> On 8/28/25 5:17 PM, Duje Mihanović wrote:

...

> > +#include <linux/device.h>

See below about kernel.h, TL;DR: with device.h check carefully that
you are really using it and not something from linux/device/* and/or
linux/dev_printk.h.

> > +#include <linux/i2c.h>
> > +#include <linux/iio/driver.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/types.h>
> > +#include <linux/kernel.h>
>
> We usually try to avoid including kernel.h because it includes too much.
> There are some recent-ish messages on the iio mailing list discussing
> include-what-you-use with some tips on how to pick the headers that are
> actually being used for inclusion.

+100

In new code no driver should use kernel.h. Use proper headers from day 1.

> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/mfd/88pm886.h>
>
> Odd to have this one not grouped with the rest.

Actually it's fine, as it is almost private to this driver, but for
the sake of consistency I would also like to see the linux/iio/* be
grouped.

...

> > +     u8 buf[2];

Can't we use proper type, i.e. __be16 here?

...

> > +     ret = regmap_bulk_read(*map, regs[chan], buf, 2);

sizeof()

> > +

Redundant blank line.

> > +     if (ret)
> > +             return ret;
> > +
> > +     val = ((buf[0] & 0xff) << 4) | (buf[1] & 0xf);
> > +     val &= 0xfff;
>
> This line seems reduandant as mask was already applied in previous line.

With the previous suggestions this will be as simple as

be16_to_cpu() >> 4 no masks needed at all!

...

> > +     if (adcnum < 0 || adcnum > 3)
> > +             return -EINVAL;

in_range()

...

> > +     for (int i = 0; i < 16; i++) {

Why signed? What is the magic value here?

> > +             ret = regmap_update_bits(*map, reg, 0xf, i);

GENMASK() or even better to have a definitive constant.

> > +             if (ret)
> > +                     return ret;

...

> > +     raw = gpadc_get_raw(iio, chan->channel);
> > +     if (raw < 0) {
> > +             ret = raw;
> > +             goto out;
> > +     }

Instead just assign to ret and if okay, reassign to raw.

...

> > +     const u8 config[] = {0xff, 0xfd, 0x1};
>
> IIRC, IIO subsystem prefers spaces around the braces.
>
>                         { 0xff, 0xfd, 0x1 };

Also make them fixed width, i.e. 0x01

> Also, could use some macros to explain what these values are.

...

> > +     /* Enable/disable the ADC block */
> > +     ret = regmap_assign_bits(map, PM886_REG_GPADC_CONFIG6, BIT(0), enable);
> > +     if (ret || !enable)
> > +             return ret;

It's implicit that when "!enable" we return 0, this should be written
explicitly because it's a special case.

...

> > +     iio->dev.parent = dev;

> > +     iio->dev.of_node = parent->of_node;

It's incomplete and IIO already does it for you. IIRC the parent is
set also, but please double check that.

...

> > +     devm_pm_runtime_enable(dev);

Why use devm if not checking for the returned code?

...

> > +config 88PM886_GPADC
> > +     tristate "Marvell 88PM886 GPADC driver"
> > +     depends on MFD_88PM886_PMIC
> > +     default y

Really? Why tristate then?
I would expect default MFD_88PM886_PMIC instead,

> > +     help
> > +       Say Y here to enable support for the GPADC (General Purpose ADC)
> > +       found on the Marvell 88PM886 PMIC. The GPADC measures various
> > +       internal voltages and temperatures, including (but not limited to)
> > +       system, battery and USB.

Please, add a line about the module name if one chooses 'm'. Or see
above — drop the "tristate" and explain why this driver may not be a
module in the commit message.

-- 
With Best Regards,
Andy Shevchenko