linux-kernel - Re: [PATCH 4/6] regulator: 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]

Date:   Sat, 23 Jan 2021 00:34:44 -0800
From:   Alistair Francis <alistair23@...il.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Alistair Francis <alistair@...stair23.me>,
        Lee Jones <lee.jones@...aro.org>,
        Rob Herring <robh+dt@...nel.org>, lgirdwood@...il.com,
        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 4/6] regulator: Initial commit of sy7636a

On Fri, Jan 22, 2021 at 5:37 AM Mark Brown <broonie@...nel.org> wrote:
>
> On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote:
> > On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@...nel.org> wrote:
> > > On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:
>
> > > > +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> > > > +{
> > > > +     int ret = get_vcom_voltage_mv(rdev->regmap);
> > > > +
>
> > > Why is this get_vcom_voltage_mv() function not in the regulator driver,
> > > and why is it not just inline here?  It also needs namespacing.
>
> > I'm not sure what you mean, can you please explain?
>
> This is a wrapper for a function that has exactly one caller but is not
> only a separate function but also in the MFD header, part of a separate
> driver.  This seems at best pointless.

Ah I see. I think I have fixed this.

>
> > > Why do you need this delay here, and what purpose is this lock intended
>
> > The delay is to allow a power ramp up, I have added a comment.
>
> Use the standard ramp_delay, don't open code it.
>
> > > > +static int sy7636a_regulator_suspend(struct device *dev)
> > > > +{
> > > > +     int ret;
> > > > +     struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> > > > +
> > > > +     ret = get_vcom_voltage_mv(sy7636a->regmap);
> > > > +
> > > > +     if (ret > 0)
> > > > +             sy7636a->vcom = (unsigned int)ret;
> > > > +
> > > > +     return 0;
> > > > +}
>
> > > What's going on here, and if you are going to store this value over
> > > suspend why not store it in a variable of the correct type?  In general
>
> > It is part of the vendor's kernel, they specifically added it to
> > ensure vcom is set on resume.
>
> "I copied this from the vendor" isn't really a great explanation...  If
> the device is likely to get completely powered off and loosing settings
> then presumably the entire register map, not just this one value, needs
> to be saved and restored instead of just this one value.  If that is the
> case it's probably best to use a register cache and just resync it on
> resume.

Good point.

I don't have a good answer so I have removed the suspend/resume part.
I'll have to investigate in the future if/why this is required.

Alistair