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>
In-Reply-To: <20210118123158.GE4455@sirena.org.uk>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Thu, 21 Jan 2021 22:24:10 -0800
Message-ID: <CAKmqyKMyXk_OjTKD24Qxn4QXJ4FwP4yDQqtmTsxAJMJzy34PcA@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@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 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:
>
> > --- /dev/null
> > +++ b/drivers/regulator/sy7636a-regulator.c
> > @@ -0,0 +1,233 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Functions to access SY3686A power management chip voltages
> > + *
>
> Please make the entire comment a C++ one so things look more
> intentional.

Fixed.

>
> > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > + *
> > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
>
> This probably needs an update.
>
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
>
> This boilerplate is redundant and should be removed.

Fixed.

>
> > +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?

>
> > +static int disable_regulator(struct regulator_dev *rdev)
> > +{
> > +     struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> > +     int ret = 0;
> > +
> > +     mutex_lock(&sy7636a->reglock);
> > +     ret = regulator_disable_regmap(rdev);
> > +     usleep_range(30000, 35000);
> > +     mutex_unlock(&sy7636a->reglock);
>
> 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.

> to serve?  I can't understand what it's intended to protect.

Apparently the mutex is to protect enable/disable, I don't think it's
required and I will remove it.

>
> > +     mutex_lock(&sy7636a->reglock);
> > +     ret = regulator_is_enabled_regmap(rdev);
> > +     mutex_unlock(&sy7636a->reglock);
>
> This lock usage in particular looks confused.
>
> > +     ret = regulator_enable_regmap(rdev);
> > +     if (ret)
> > +             goto finish;
>
> > +     if (!pwr_good) {
> > +             dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
> > +                             jiffies_to_msecs(t1 - t0));
> > +             ret = -ETIME;
> > +             goto finish;
> > +     }
>
> This doesn't undo the underlying enable, leaving the regulator in a
> partially enabled state.

Thanks, fixed.

>
> > +static const struct regulator_ops sy7636a_vcom_volt_ops = {
> > +     .get_voltage = get_vcom_voltage_op,
> > +     .enable = enable_regulator_pgood,
> > +     .disable = disable_regulator,
> > +     .is_enabled = sy7636a_regulator_is_enabled,
> > +};
>
> The namespacing for functions is very random and prone to clashes.

Fixed.

> Given the power good signal I'd also expect a get_status() operation.

Added.

>
> > +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 have fixed the variable type.

> it's surprising to need a suspend operation for a regulator.
>
> > +     sy7636a->pgood_gpio = gdp;
> > +     dev_info(sy7636a->dev,
> > +             "Power good GPIO registered (gpio# %d)\n",
> > +             desc_to_gpio(sy7636a->pgood_gpio));
>
> This print is just adding noise to the boot process.

Removed.


Alistair