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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75Ve=xJ6vTUydaTw9GuYr22ZXp3HFA5N0tP7NET=CvZJB8Q@mail.gmail.com>
Date: Sat, 30 Aug 2025 07:37:27 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Duje Mihanović <duje@...emihanovic.xyz>, 
	Jonathan Cameron <jic23@...nel.org>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Karel Balej <balejk@...fyz.cz>, Lee Jones <lee@...nel.org>, 
	David Wronek <david@...nlining.org>, phone-devel@...r.kernel.org, 
	~postmarketos/upstreaming@...ts.sr.ht, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: Add driver for Marvell 88PM886 PMIC ADC

On Fri, Aug 29, 2025 at 2:41 AM David Lechner <dlechner@...libre.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