Re: [PATCH 4/6] regulator: Initial commit of sy7636a

[Alistair Francis]

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