Re: [PATCH v4 3/5] regulator: sy7636a: Initial commit

[Mark Brown]

On Thu, Mar 25, 2021 at 09:55:09PM -0400, Alistair Francis wrote:

> Initial support for the Silergy SY7636A-regulator Power Management chip.

In general this driver seems like it's trying to implement a bunch of
policy and extra features beyond standard regulator support - please
strip it back more, if you feel that there's things that really do need
to be added in the driver itself post them as separate patches.  It's
also open coding some features the core provides.  This should all make
the driver much smaller and simpler.

> +++ b/Documentation/ABI/testing/sysfs-driver-sy7636a-regulator
> @@ -0,0 +1,21 @@
> +What:/sys/bus/regulator/drivers/sy7636a-regulator/state
> +Date:April 2021
> +KernelVersion:   5.12

None of these sysfs files are appropriate for a driver, if they are
useful they should be added to the core (but some of them seem like
they duplicate files that already exist, this one being an example).
There's absolutely nothing device specific about any of them.

> +static int sy7636a_disable_regulator(struct regulator_dev *rdev)
> +{
> + int ret = 0;
> +
> + ret = regulator_disable_regmap(rdev);
> + // Delay for ~35ms after disabling the regulator, to allow power ramp
> + // down to go undisturbed
> + usleep_range(3, 35000);

If this is needed add it to the core, but really this sort of stuff is
going to be very board specific - it'll depend on what the load on the
regulator is - and it's pretty rare for anything to care, you don't have
the same issues you have on enable.

> +static int sy7636a_regulator_is_enabled(struct regulator_dev *rdev)
> +{
> + return regulator_is_enabled_regmap(rdev);
> +}

Just use the generic operation, this wrapper is not adding anything
except code size.

> +static int sy7636a_get_status(struct regulator_dev *rdev)
> +{
> + struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> + int pwr_good = 0;
> + const unsigned int wait_time = 500;
> + unsigned int wait_cnt;
> + int ret = 0;
> +
> + 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;
> + return ret;
> + } else if (pwr_good)
> + break;

This should just read the status, if the caller wants to retry for a
while then the caller can make that decision.

> + 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);

Use poll_enabled_time to check the status, no need for a custom enable
operation.

> + ret = pwr_good;
> + goto finish;
> + } else if (pwr_good)
> + break;

As per coding-style.rst both sides of the if statement should use braces
if one does.

> + ret = sysfs_create_group(>dev.kobj, _sysfs_attr_group);
> + if (ret) {
> + dev_err(sy7636a->dev, "Failed to create sysfs attributes\n");
> + return ret;
> + }

*If* the driver is creating sysfs devices it *definitely* shouldn't be
creating and destroying them dynamically at runtime.

> + ret = sy7636a_regulator_init(sy7636a);
> + if (ret) {
> + dev_err(sy7636a->dev, "Failed to initialize regulator: %d\n", 
> ret);
> + return ret;
> + }

This function has one caller and one statement in it, just inline it.


signature.asc
Description: PGP signature

[PATCH v4 3/5] regulator: sy7636a: Initial commit

[Alistair Francis]

Initial support for the Silergy SY7636A-regulator Power Management chip.

Signed-off-by: Alistair Francis 
---
v3:
 - Move sysfs power from mfd to regulaator
 - Add ABI documentation
v2:
 - N/A
 .../testing/sysfs-driver-sy7636a-regulator|  21 ++
 drivers/regulator/Kconfig |   6 +
 drivers/regulator/Makefile|   1 +
 drivers/regulator/sy7636a-regulator.c | 354 ++
 include/linux/mfd/sy7636a.h   |   1 +
 5 files changed, 383 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-sy7636a-regulator
 create mode 100644 drivers/regulator/sy7636a-regulator.c

diff --git a/Documentation/ABI/testing/sysfs-driver-sy7636a-regulator 
b/Documentation/ABI/testing/sysfs-driver-sy7636a-regulator
new file mode 100644
index ..ab534a8ea21a
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-sy7636a-regulator
@@ -0,0 +1,21 @@
+What:  /sys/bus/regulator/drivers/sy7636a-regulator/state
+Date:  April 2021
+KernelVersion: 5.12
+Contact:   alist...@alistair23.me
+Description:
+   This file allows you to see the current power rail state.
+
+What:  /sys/bus/regulator/drivers/sy7636a-regulator/power_good
+Date:  April 2021
+KernelVersion: 5.12
+Contact:   alist...@alistair23.me
+Description:
+   This file allows you to see the current state of the regulator
+   as either ON or OFF.
+
+What:  /sys/bus/regulator/drivers/sy7636a-regulator/vcom
+Date:  April 2021
+KernelVersion: 5.12
+Contact:   alist...@alistair23.me
+Description:
+   This file allows you to see and set the current voltage in mV.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 77c43134bc9e..6d501ce921a8 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1130,6 +1130,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 44d2f8bf4b74..5a981036a9f0 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -134,6 +134,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 ..0ec6f852cb3d
--- /dev/null
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -0,0 +1,354 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Functions to access SY3686A power management chip voltages
+//
+// Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+//
+// Authors: Lars Ivar Miljeteig 
+//  Alistair Francis 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static const char * const states[] = {
+   "no fault event",
+   "UVP at VP rail",
+   "UVP at VN rail",
+   "UVP at VPOS rail",
+   "UVP at VNEG rail",
+   "UVP at VDDH rail",
+   "UVP at VEE rail",
+   "SCP at VP rail",
+   "SCP at VN rail",
+   "SCP at VPOS rail",
+   "SCP at VNEG rail",
+   "SCP at VDDH rail",
+   "SCP at VEE rail",
+   "SCP at V COM rail",
+   "UVLO",
+   "Thermal shutdown",
+};
+
+static int sy7636a_get_vcom_voltage_mv(struct regmap *regmap)
+{
+   int ret;
+   unsigned int val, val_h;
+
+   ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, );
+   if (ret)
+   return ret;
+
+   ret = regmap_read(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, _h);
+   if (ret)
+   return ret;
+
+   val |= (val_h << VCOM_ADJUST_CTRL_SHIFT);
+
+   return (val & VCOM_ADJUST_CTRL_MASK) * VCOM_ADJUST_CTRL_SCAL;
+}
+
+static int sy7636a_set_vcom_voltage_mv(struct regmap *regmap, unsigned int 
vcom)
+{
+   int ret;
+   unsigned int val;
+
+   if (vcom < VCOM_MIN || vcom > VCOM_MAX)
+   return -EINVAL;
+
+   val = (unsigned int)(vcom / VCOM_ADJUST_CTRL_SCAL) & 
VCOM_ADJUST_CTRL_MASK;
+
+   ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, val);
+   if (ret)
+   return ret;
+
+   ret = regmap_write(regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, val >> 
VCOM_ADJUST_CTRL_SHIFT);
+   if (ret)
+