Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210117042539.1609-1-alistair@alistair23.me> <20210117042539.1609-2-alistair@alistair23.me>
 <20210204103126.GA2789116@dell> <CAKmqyKOJ251sRUaxUKJ_U=puYRkypPEHAVZOnAKbQtP2skMJBA@mail.gmail.com>
 <20210323093541.GQ2916463@dell>
In-Reply-To: <20210323093541.GQ2916463@dell>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Thu, 25 Mar 2021 09:49:39 -0400
Message-ID: <CAKmqyKNaVmj2thEbuxZShjnX_UCsP0YW5gjBrjdAr3YtAn5Yhg@mail.gmail.com>
Subject: Re: [PATCH 2/6] mfd: Initial commit of sy7636a
To:     Lee Jones <lee.jones@linaro.org>
Cc:     Alistair Francis <alistair@alistair23.me>,
        Rob Herring <robh+dt@kernel.org>, lgirdwood@gmail.com,
        Mark Brown <broonie@kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>,
        Sascha Hauer <kernel@pengutronix.de>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 23, 2021 at 5:35 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Sat, 20 Mar 2021, Alistair Francis wrote:
>
> > On Thu, Feb 4, 2021 at 5:31 AM Lee Jones <lee.jones@linaro.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@remarkable.com=
>
>
> Or ping this guy.

I reached out to him and have permission to bump the year.

>
> [...]
>
> > > > +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 =3D (unsigned int)(vcom / 10) & 0x1ff;
> > >
> > > As above.
> >
> > I have used defines for all of these.
> >
> > >
> > > > +     ret =3D regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, =
val);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     ret =3D 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.

Ok, I have moved these to the regulator.

>
> [...]
>
> > > > +     if (val >=3D ARRAY_SIZE(states)) {
> > > > +             dev_err(sy7636a->dev, "Unexpected value read from dev=
ice: %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

Thanks!

>
> [...]
>
> > > > +static struct attribute *sy7636a_sysfs_attrs[] =3D {
> > > > +     &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.

Done.

Alistair

>
> [...]
>
> --
> Lee Jones [=E6=9D=8E=E7=90=BC=E6=96=AF]
> Senior Technical Lead - Developer Services
> Linaro.org =E2=94=82 Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog