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