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-4-alistair@alistair23.me>
 <20210118123158.GE4455@sirena.org.uk> <CAKmqyKMyXk_OjTKD24Qxn4QXJ4FwP4yDQqtmTsxAJMJzy34PcA@mail.gmail.com>
 <20210122133718.GB6391@sirena.org.uk>
In-Reply-To: <20210122133718.GB6391@sirena.org.uk>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Sat, 23 Jan 2021 00:34:44 -0800
Message-ID: <CAKmqyKPsVkUDts=X1sLMyc7hWs_E=1hW6ydzPDXMDMNVWa1u0A@mail.gmail.com>
Subject: Re: [PATCH 4/6] regulator: Initial commit of sy7636a
To:     Mark Brown <broonie@kernel.org>
Cc:     Alistair Francis <alistair@alistair23.me>,
        Lee Jones <lee.jones@linaro.org>,
        Rob Herring <robh+dt@kernel.org>, lgirdwood@gmail.com,
        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"
Precedence: bulk

On Fri, Jan 22, 2021 at 5:37 AM Mark Brown <broonie@kernel.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@kernel.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