Re: [PATCH 4/6] regulator: Initial commit of sy7636a
On Fri, Jan 22, 2021 at 5:37 AM Mark Brown 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 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
Re: [PATCH 4/6] regulator: Initial commit of sy7636a
On Thu, Jan 21, 2021 at 10:24:10PM -0800, Alistair Francis wrote:
> On Mon, Jan 18, 2021 at 4:32 AM Mark Brown 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.
> > 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.
signature.asc
Description: PGP signature
Re: [PATCH 4/6] regulator: Initial commit of sy7636a
On Mon, Jan 18, 2021 at 4:32 AM Mark Brown 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
>
> 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(>reglock);
> > + ret = regulator_disable_regmap(rdev);
> > + usleep_range(3, 35000);
> > + mutex_unlock(>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(>reglock);
> > + ret = regulator_is_enabled_regmap(rdev);
> > + mutex_unlock(>reglock);
>
> This lock usage in particular looks confused.
>
> > + ret = regulator_enable_regmap(rdev);
> > + if (ret)
> > + goto finish;
>
> > + if (!pwr_good) {
> > + dev_err(>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
Re: [PATCH 4/6] regulator: Initial commit of sy7636a
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.
> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> + *
> + * Author: Lars Ivar Miljeteig
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.
> +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.
> +static int disable_regulator(struct regulator_dev *rdev)
> +{
> + struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> + int ret = 0;
> +
> + mutex_lock(>reglock);
> + ret = regulator_disable_regmap(rdev);
> + usleep_range(3, 35000);
> + mutex_unlock(>reglock);
Why do you need this delay here, and what purpose is this lock intended
to serve? I can't understand what it's intended to protect.
> + mutex_lock(>reglock);
> + ret = regulator_is_enabled_regmap(rdev);
> + mutex_unlock(>reglock);
This lock usage in particular looks confused.
> + ret = regulator_enable_regmap(rdev);
> + if (ret)
> + goto finish;
> + if (!pwr_good) {
> + dev_err(>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.
> +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.
Given the power good signal I'd also expect a get_status() operation.
> +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'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.
signature.asc
Description: PGP signature
[PATCH 4/6] regulator: Initial commit of sy7636a
Initial support for the Silergy SY7636A-regulator Power Management chip
driver.
Signed-off-by: Alistair Francis
---
drivers/regulator/Kconfig | 6 +
drivers/regulator/Makefile| 1 +
drivers/regulator/sy7636a-regulator.c | 233 ++
3 files changed, 240 insertions(+)
create mode 100644 drivers/regulator/sy7636a-regulator.c
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 5abdd29fb9f3..76510a0db4f9 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1097,6 +1097,12 @@ config REGULATOR_STW481X_VMMC
This driver supports the internal VMMC regulator in the STw481x
PMIC chips.
+config REGULATOR_SY7636A
+ tristate "Silergy SY7636A voltage regulator"
+ depends on MFD_SY7636A
+ help
+ This driver supports Silergy SY3686A voltage regulator.
+
config REGULATOR_SY8106A
tristate "Silergy SY8106A regulator"
depends on I2C && (OF || COMPILE_TEST)
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 680e539f6579..fe05347cbf84 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -131,6 +131,7 @@ obj-$(CONFIG_REGULATOR_STM32_VREFBUF) += stm32-vrefbuf.o
obj-$(CONFIG_REGULATOR_STM32_PWR) += stm32-pwr.o
obj-$(CONFIG_REGULATOR_STPMIC1) += stpmic1_regulator.o
obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
+obj-$(CONFIG_REGULATOR_SY7636A) += sy7636a-regulator.o
obj-$(CONFIG_REGULATOR_SY8106A) += sy8106a-regulator.o
obj-$(CONFIG_REGULATOR_SY8824X) += sy8824x.o
obj-$(CONFIG_REGULATOR_SY8827N) += sy8827n.o
diff --git a/drivers/regulator/sy7636a-regulator.c
b/drivers/regulator/sy7636a-regulator.c
new file mode 100644
index ..a639830298d6
--- /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
+ *
+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * Author: Lars Ivar Miljeteig
+ *
+ * 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.
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+
+static int get_vcom_voltage_op(struct regulator_dev *rdev)
+{
+ int ret = get_vcom_voltage_mv(rdev->regmap);
+
+ if (ret < 0)
+ return ret;
+
+ return ret * 1000;
+}
+
+static int disable_regulator(struct regulator_dev *rdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+ int ret = 0;
+
+ mutex_lock(>reglock);
+ ret = regulator_disable_regmap(rdev);
+ usleep_range(3, 35000);
+ mutex_unlock(>reglock);
+
+ return ret;
+}
+
+static int sy7636a_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+ int ret;
+
+ mutex_lock(>reglock);
+ ret = regulator_is_enabled_regmap(rdev);
+ mutex_unlock(>reglock);
+
+ return ret;
+}
+
+static int enable_regulator_pgood(struct regulator_dev *rdev)
+{
+ struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
+ int pwr_good = 0;
+ int ret = 0;
+ unsigned long t0, t1;
+ const unsigned int wait_time = 500;
+ unsigned int wait_cnt;
+
+ t0 = jiffies;
+
+ mutex_lock(>reglock);
+
+ ret = regulator_enable_regmap(rdev);
+ if (ret)
+ goto finish;
+
+ for (wait_cnt = 0; wait_cnt < wait_time; wait_cnt++) {
+ pwr_good = gpiod_get_value_cansleep(sy7636a->pgood_gpio);
+ if (pwr_good < 0) {
+ dev_err(>dev, "Failed to read pgood gpio: %d\n",
pwr_good);
+ ret = pwr_good;
+ goto finish;
+ } else if (pwr_good)
+ break;
+
+ usleep_range(1000, 1500);
+ }
+
+ t1 = jiffies;
+
+ if (!pwr_good) {
+ dev_err(>dev, "Power good signal timeout after %u ms\n",
+ jiffies_to_msecs(t1 - t0));
+ ret = -ETIME;
+ goto finish;
+ }
+
+ dev_dbg(>dev, "Power good OK (took %u ms, %u waits)\n",
+ jiffies_to_msecs(t1 - t0),
+ wait_cnt);
+
+finish:
+ mutex_unlock(>reglock);
+ return ret;
+}
+
+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,
