linux-kernel - Re: [PATCH 2/6] mfd: Initial commit of sy7636a

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: <20210323093541.GQ2916463@dell>
Date:   Tue, 23 Mar 2021 09:35:41 +0000
From:   Lee Jones <lee.jones@...aro.org>
To:     Alistair Francis <alistair23@...il.com>
Cc:     Alistair Francis <alistair@...stair23.me>,
        Rob Herring <robh+dt@...nel.org>, lgirdwood@...il.com,
        Mark Brown <broonie@...nel.org>,
        dl-linux-imx <linux-imx@....com>,
        Sascha Hauer <kernel@...gutronix.de>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/6] mfd: Initial commit of sy7636a

On Sat, 20 Mar 2021, Alistair Francis wrote:

> On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@...aro.org> wrote:
> >
> > On Sat, 16 Jan 2021, Alistair Francis wrote:
> >
> > > Initial support for the Silergy SY7636A Power Management chip
> > > driver.
> >
> > Please remove "driver", as this is not support for the driver, it *is*
> > the driver which supports the chip.
> 
> Sorry for the long delay here.
> 
> I have addressed your comments.

[...]

> > > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> > > new file mode 100644
> > > index 000000000000..39aac965d854
> > > --- /dev/null
> > > +++ b/drivers/mfd/sy7636a.c
> > > @@ -0,0 +1,252 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * MFD driver for SY7636A chip
> >
> > "Parent driver".
> >
> > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> >
> > This is quite out of date.  Please update.
> 
> I don't own this copyright, so I would rather not change it.

I'm not comfortable taking a new driver with an old Copyright.

Maybe ask reMarkable if it's okay to bump it.

> > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@...arkable.com>

Or ping this guy.

[...]

> > > +int set_vcom_voltage_mv(struct regmap *regmap, unsigned int vcom)
> > > +{
> > > +     int ret;
> > > +     unsigned int val;
> > > +
> > > +     if (vcom < 0 || vcom > 5000)
> >
> > Please define min/max values.
> >
> > > +             return -EINVAL;
> > > +
> > > +     val = (unsigned int)(vcom / 10) & 0x1ff;
> >
> > As above.
> 
> I have used defines for all of these.
> 
> >
> > > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 8);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     return 0;
> > > +}
> >
> > Who calls these?
> 
> They sysfs store and show functions.

They should be in a power/regulator driver really.

[...]

> > > +     if (val >= ARRAY_SIZE(states)) {
> > > +             dev_err(sy7636a->dev, "Unexpected value read from device: %u\n", val);
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     return snprintf(buf, PAGE_SIZE, "%s\n", states[val]);
> > > +}
> > > +static DEVICE_ATTR(state, 0444, state_show, NULL);
> >
> > You need to document new sysfs entries.
> 
> I'm not sure how to document this. Do you mind pointing out an example
> I can use?

See the final paragraph in:

  Documentation/filesystems/sysfs.rst

[...]

> > > +static struct attribute *sy7636a_sysfs_attrs[] = {
> > > +     &dev_attr_state.attr,
> > > +     &dev_attr_power_good.attr,
> > > +     &dev_attr_vcom.attr,
> > > +     NULL,
> > > +};
> >
> > These all look like power options?  Do they really belong here?
> 
> From what I can tell I think they do. Let me know if you don't think so.

As above, I think they should be in power or regulator.

[...]

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog