[PATCH 0/7] sy7636a: power the panel only during updates

[PATCH 0/7] sy7636a: power the panel only during updates

[Pablo Mazzini]

The SY7636A hwmon driver keeps VCOM on from probe, so a display driver
can not power the panel only during updates.

Read the temperature in the regulator driver instead, as the TPS65185
and FP9931 drivers do, and remove the hwmon driver. Boards that relied
on VCOM staying on mark it always-on. The regulator also switches VCOM
with VCOM_EN, gains VCOM voltage control and binds to the I2C device
directly.

Tested on a PocketBook Verse (B288).

Pablo Mazzini (7):
  ARM: dts: imx: keep the SY7636A VCOM on
  regulator: sy7636a: fix the chip name
  regulator: sy7636a: add VCOM voltage control
  regulator: sy7636a: wait for power good, then switch VCOM_EN
  regulator: sy7636a: report the temperature
  regulator: sy7636a: bind to the I2C device directly
  ARM: imx_v6_v7_defconfig: drop the SY7636A MFD and hwmon options

 Documentation/hwmon/index.rst                 |   1 -
 Documentation/hwmon/sy7636a-hwmon.rst         |  26 --
 arch/arm/boot/dts/nxp/imx/e70k02.dtsi         |   1 +
 .../boot/dts/nxp/imx/imx7d-remarkable2.dts    |   1 +
 arch/arm/configs/imx_v6_v7_defconfig          |   2 -
 drivers/hwmon/Kconfig                         |  10 -
 drivers/hwmon/Makefile                        |   1 -
 drivers/hwmon/sy7636a-hwmon.c                 | 102 --------
 drivers/mfd/Kconfig                           |  10 -
 drivers/mfd/simple-mfd-i2c.c                  |  11 -
 drivers/regulator/Kconfig                     |   6 +-
 drivers/regulator/sy7636a-regulator.c         | 240 +++++++++++++++---
 include/linux/mfd/sy7636a.h                   |  34 ---
 13 files changed, 205 insertions(+), 240 deletions(-)
 delete mode 100644 Documentation/hwmon/sy7636a-hwmon.rst
 delete mode 100644 drivers/hwmon/sy7636a-hwmon.c
 delete mode 100644 include/linux/mfd/sy7636a.h


base-commit: aac26bee2287c88af5be5a5ff96d783b19a28790
-- 
2.52.0

[PATCH 5/7] regulator: sy7636a: report the temperature

[Pablo Mazzini]

The separate hwmon cell enables VCOM at probe. Report the temperature
from here, as the TPS65185 and FP9931 drivers do, and under the chip
name as they do: read it while the rails are on and return -ENODATA
otherwise. Boards that need the rails on mark VCOM always-on. Drop the
hwmon driver.

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 Documentation/hwmon/index.rst         |   1 -
 Documentation/hwmon/sy7636a-hwmon.rst |  26 -------
 drivers/hwmon/Kconfig                 |  10 ---
 drivers/hwmon/Makefile                |   1 -
 drivers/hwmon/sy7636a-hwmon.c         | 102 --------------------------
 drivers/mfd/simple-mfd-i2c.c          |   1 -
 drivers/regulator/Kconfig             |   1 +
 drivers/regulator/sy7636a-regulator.c |  56 ++++++++++++++
 8 files changed, 57 insertions(+), 141 deletions(-)
 delete mode 100644 Documentation/hwmon/sy7636a-hwmon.rst
 delete mode 100644 drivers/hwmon/sy7636a-hwmon.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 199ca79621e9..43aecc4e49cd 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -265,7 +265,6 @@ Hardware Monitoring Kernel Drivers
    sq24860
    stpddc60
    surface_fan
-   sy7636a-hwmon
    tc654
    tc74
    tda38740
diff --git a/Documentation/hwmon/sy7636a-hwmon.rst b/Documentation/hwmon/sy7636a-hwmon.rst
deleted file mode 100644
index 03d866aba6e8..000000000000
--- a/Documentation/hwmon/sy7636a-hwmon.rst
+++ /dev/null
@@ -1,26 +0,0 @@
-.. SPDX-License-Identifier: GPL-2.0-or-later
-
-Kernel driver sy7636a-hwmon
-===========================
-
-Supported chips:
-
- * Silergy SY7636A PMIC
-
-
-Description
------------
-
-This driver adds hardware temperature reading support for
-the Silergy SY7636A PMIC.
-
-The following sensors are supported
-
-  * Temperature
-      - Temperature of external NTC in milli-degree C
-
-sysfs-Interface
----------------
-
-temp1_input
-	- Temperature of external NTC (milli-degree C)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 18c682e1a475..36e08c333e01 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2104,16 +2104,6 @@ config SENSORS_SIS5595
 	  This driver can also be built as a module. If so, the module
 	  will be called sis5595.
 
-config SENSORS_SY7636A
-	tristate "Silergy SY7636A"
-	depends on MFD_SY7636A
-	help
-	  If you say yes here you get support for the thermistor readout of
-	  the Silergy SY7636A PMIC.
-
-	  This driver can also be built as a module.  If so, the module
-	  will be called sy7636a-hwmon.
-
 config SENSORS_DME1737
 	tristate "SMSC DME1737, SCH311x and compatibles"
 	depends on HAS_IOPORT
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d4c345ed7261..537fb0bc1778 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -234,7 +234,6 @@ obj-$(CONFIG_SENSORS_SPD5118)	+= spd5118.o
 obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
 obj-$(CONFIG_SENSORS_SURFACE_FAN)+= surface_fan.o
 obj-$(CONFIG_SENSORS_SURFACE_TEMP)+= surface_temp.o
-obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
 obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
 obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
deleted file mode 100644
index d51daaf63d63..000000000000
--- a/drivers/hwmon/sy7636a-hwmon.c
+++ /dev/null
@@ -1,102 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * Functions to access SY3686A power management chip temperature
- *
- * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
- *
- * Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
- *          Alistair Francis <alistair@alistair23.me>
- */
-
-#include <linux/err.h>
-#include <linux/hwmon.h>
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/regmap.h>
-#include <linux/regulator/machine.h>
-
-#include <linux/mfd/sy7636a.h>
-
-static int sy7636a_read(struct device *dev, enum hwmon_sensor_types type,
-			u32 attr, int channel, long *temp)
-{
-	struct regmap *regmap = dev_get_drvdata(dev);
-	int ret, reg_val;
-
-	ret = regmap_read(regmap,
-			  SY7636A_REG_TERMISTOR_READOUT, &reg_val);
-	if (ret)
-		return ret;
-
-	*temp = reg_val * 1000;
-
-	return 0;
-}
-
-static umode_t sy7636a_is_visible(const void *data,
-				  enum hwmon_sensor_types type,
-				  u32 attr, int channel)
-{
-	if (type != hwmon_temp)
-		return 0;
-
-	if (attr != hwmon_temp_input)
-		return 0;
-
-	return 0444;
-}
-
-static const struct hwmon_ops sy7636a_hwmon_ops = {
-	.is_visible = sy7636a_is_visible,
-	.read = sy7636a_read,
-};
-
-static const struct hwmon_channel_info * const sy7636a_info[] = {
-	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
-	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
-	NULL
-};
-
-static const struct hwmon_chip_info sy7636a_chip_info = {
-	.ops = &sy7636a_hwmon_ops,
-	.info = sy7636a_info,
-};
-
-static int sy7636a_sensor_probe(struct platform_device *pdev)
-{
-	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
-	struct device *hwmon_dev;
-	int err;
-
-	if (!regmap)
-		return -EPROBE_DEFER;
-
-	err = devm_regulator_get_enable(&pdev->dev, "vcom");
-	if (err)
-		return err;
-
-	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
-							 "sy7636a_temperature", regmap,
-							 &sy7636a_chip_info, NULL);
-
-	if (IS_ERR(hwmon_dev)) {
-		err = PTR_ERR(hwmon_dev);
-		dev_err(&pdev->dev, "Unable to register hwmon device, returned %d\n", err);
-		return err;
-	}
-
-	return 0;
-}
-
-static struct platform_driver sy7636a_sensor_driver = {
-	.probe = sy7636a_sensor_probe,
-	.driver = {
-		.name = "sy7636a-temperature",
-	},
-};
-module_platform_driver(sy7636a_sensor_driver);
-
-MODULE_DESCRIPTION("SY7636A sensor driver");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:sy7636a-temperature");
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index ef3ce4bdf98b..168b4cf49b8c 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -69,7 +69,6 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 
 static const struct mfd_cell sy7636a_cells[] = {
 	{ .name = "sy7636a-regulator", },
-	{ .name = "sy7636a-temperature", },
 };
 
 static const struct simple_mfd_data silergy_sy7636a = {
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 88ba2770e378..f0d8d1319a1b 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1624,6 +1624,7 @@ config REGULATOR_SY7636A
 	depends on MFD_SY7636A
 	help
 	  This driver supports Silergy SY7636A voltage regulator.
+	  If HWMON is enabled, it also provides temperature measurement.
 
 config REGULATOR_SY8106A
 	tristate "Silergy SY8106A regulator"
diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
index 5705599d1b55..816df7716c0e 100644
--- a/drivers/regulator/sy7636a-regulator.c
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -9,6 +9,7 @@
 
 #include <linux/completion.h>
 #include <linux/gpio/consumer.h>
+#include <linux/hwmon.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/sy7636a.h>
@@ -147,6 +148,52 @@ static const struct regulator_desc desc = {
 	.of_match = of_match_ptr("vcom"),
 };
 
+static int sy7636a_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *temp)
+{
+	struct sy7636a_data *data = dev_get_drvdata(dev);
+	unsigned int mode, val;
+	int ret;
+
+	ret = regmap_read(data->regmap, SY7636A_REG_OPERATION_MODE_CRL, &mode);
+	if (ret)
+		return ret;
+
+	/* the thermistor is only sampled while the rails are on */
+	if (!(mode & SY7636A_OPERATION_MODE_CRL_ONOFF))
+		return -ENODATA;
+
+	ret = regmap_read(data->regmap, SY7636A_REG_TERMISTOR_READOUT, &val);
+	if (ret)
+		return ret;
+
+	*temp = (s8)val * 1000;
+
+	return 0;
+}
+
+static umode_t sy7636a_hwmon_is_visible(const void *data, enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	return 0444;
+}
+
+static const struct hwmon_channel_info * const sy7636a_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+	NULL
+};
+
+static const struct hwmon_ops sy7636a_hwmon_ops = {
+	.is_visible = sy7636a_hwmon_is_visible,
+	.read = sy7636a_hwmon_read,
+};
+
+static const struct hwmon_chip_info sy7636a_hwmon_chip_info = {
+	.ops = &sy7636a_hwmon_ops,
+	.info = sy7636a_hwmon_info,
+};
+
 static int sy7636a_regulator_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
@@ -235,6 +282,15 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
 		return PTR_ERR(rdev);
 	}
 
+	if (IS_REACHABLE(CONFIG_HWMON)) {
+		struct device *hwmon_dev;
+
+		hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "sy7636a", data,
+								 &sy7636a_hwmon_chip_info, NULL);
+		if (IS_ERR(hwmon_dev))
+			dev_notice(&pdev->dev, "failed to register hwmon\n");
+	}
+
 	return 0;
 }
 
-- 
2.52.0

[PATCH 6/7] regulator: sy7636a: bind to the I2C device directly

[Pablo Mazzini]

With the temperature reported here, the regulator is all that is left
behind simple-mfd-i2c. Bind to the chip directly, as the TPS65185 and
FP9931 drivers do, and drop the MFD glue and header. It now probes before
the GPIOs may be ready, so report the power good GPIO with
dev_err_probe().

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/mfd/Kconfig                   | 10 ----
 drivers/mfd/simple-mfd-i2c.c          | 10 ----
 drivers/regulator/Kconfig             |  3 +-
 drivers/regulator/sy7636a-regulator.c | 86 +++++++++++++++------------
 include/linux/mfd/sy7636a.h           | 34 -----------
 5 files changed, 50 insertions(+), 93 deletions(-)
 delete mode 100644 include/linux/mfd/sy7636a.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 0a2778052519..9130dac7a7ff 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1305,16 +1305,6 @@ config MFD_SPMI_PMIC
 	  Say M here if you want to include support for the SPMI PMIC
 	  series as a module.  The module will be called "qcom-spmi-pmic".
 
-config MFD_SY7636A
-	tristate "Silergy SY7636A voltage regulator"
-	depends on I2C
-	select MFD_SIMPLE_MFD_I2C
-	help
-	  Enable support for Silergy SY7636A voltage regulator.
-
-	  To enable support for building sub-devices as modules,
-	  choose M here.
-
 config MFD_RDC321X
 	tristate "RDC R-321x southbridge"
 	select MFD_CORE
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 168b4cf49b8c..0c9162034899 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -67,15 +67,6 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static const struct mfd_cell sy7636a_cells[] = {
-	{ .name = "sy7636a-regulator", },
-};
-
-static const struct simple_mfd_data silergy_sy7636a = {
-	.mfd_cell = sy7636a_cells,
-	.mfd_cell_size = ARRAY_SIZE(sy7636a_cells),
-};
-
 static const struct mfd_cell max5970_cells[] = {
 	{ .name = "max5970-regulator", },
 	{ .name = "max5970-iio", },
@@ -123,7 +114,6 @@ static const struct of_device_id simple_mfd_i2c_of_match[] = {
 	{ .compatible = "maxim,max5970", .data = &maxim_max5970 },
 	{ .compatible = "maxim,max5978", .data = &maxim_max5970 },
 	{ .compatible = "maxim,max77705-battery", .data = &maxim_mon_max77705 },
-	{ .compatible = "silergy,sy7636a", .data = &silergy_sy7636a },
 	{ .compatible = "spacemit,p1", .data = &spacemit_p1 },
 	{}
 };
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f0d8d1319a1b..49b7c6dc541f 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1621,7 +1621,8 @@ config REGULATOR_SUN20I
 
 config REGULATOR_SY7636A
 	tristate "Silergy SY7636A voltage regulator"
-	depends on MFD_SY7636A
+	depends on I2C
+	select REGMAP_I2C
 	help
 	  This driver supports Silergy SY7636A voltage regulator.
 	  If HWMON is enabled, it also provides temperature measurement.
diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
index 816df7716c0e..e170461a15ad 100644
--- a/drivers/regulator/sy7636a-regulator.c
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -12,14 +12,23 @@
 #include <linux/hwmon.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
-#include <linux/mfd/sy7636a.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/regmap.h>
 
+#define SY7636A_REG_OPERATION_MODE_CRL		0x00
+#define SY7636A_OPERATION_MODE_CRL_VCOMCTL	BIT(6)
+#define SY7636A_OPERATION_MODE_CRL_ONOFF	BIT(7)
+#define SY7636A_REG_VCOM_ADJUST_CTRL_L		0x01
+#define SY7636A_REG_VCOM_ADJUST_CTRL_H		0x02
+#define SY7636A_REG_POWER_ON_DELAY_TIME		0x06
+#define SY7636A_REG_TERMISTOR_READOUT		0x08
+
+#define VCOM_ADJUST_CTRL_MASK	0x1ff
+#define VCOM_ADJUST_CTRL_SCAL	10000
+
 #define PGOOD_TIMEOUT_MSECS 200
 
 struct sy7636a_data {
@@ -194,49 +203,52 @@ static const struct hwmon_chip_info sy7636a_hwmon_chip_info = {
 	.info = sy7636a_hwmon_info,
 };
 
-static int sy7636a_regulator_probe(struct platform_device *pdev)
+static const struct regmap_config sy7636a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int sy7636a_regulator_probe(struct i2c_client *client)
 {
-	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	struct device *dev = &client->dev;
 	struct regulator_config config = { };
 	struct regulator_dev *rdev;
 	struct gpio_desc *gdp;
 	struct sy7636a_data *data;
+	struct regmap *regmap;
 	int irq, ret;
 
-	if (!regmap)
-		return -EPROBE_DEFER;
-
-	device_set_of_node_from_dev(&pdev->dev, pdev->dev.parent);
+	regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap), "failed to allocate regmap\n");
 
-	gdp = devm_gpiod_get(&pdev->dev, "epd-pwr-good", GPIOD_IN);
-	if (IS_ERR(gdp)) {
-		dev_err(&pdev->dev, "Power good GPIO fault %ld\n", PTR_ERR(gdp));
-		return PTR_ERR(gdp);
-	}
+	gdp = devm_gpiod_get(dev, "epd-pwr-good", GPIOD_IN);
+	if (IS_ERR(gdp))
+		return dev_err_probe(dev, PTR_ERR(gdp), "Power good GPIO fault\n");
 
-	data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
+	data = devm_kzalloc(dev, sizeof(struct sy7636a_data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	data->regmap = regmap;
 	data->pgood_gpio = gdp;
 
-	ret = devm_regulator_get_enable_optional(&pdev->dev, "vin");
+	ret = devm_regulator_get_enable_optional(dev, "vin");
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
+		return dev_err_probe(dev, ret,
 				     "failed to get vin regulator\n");
 
-	data->en_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+	data->en_gpio = devm_gpiod_get_optional(dev, "enable",
 						GPIOD_OUT_HIGH);
 	if (IS_ERR(data->en_gpio))
-		return dev_err_probe(&pdev->dev,
+		return dev_err_probe(dev,
 				     PTR_ERR(data->en_gpio),
 				     "failed to get en gpio\n");
 
-	data->vcom_en_gpio = devm_gpiod_get_optional(&pdev->dev,
+	data->vcom_en_gpio = devm_gpiod_get_optional(dev,
 						     "vcom-en", GPIOD_OUT_LOW);
 	if (IS_ERR(data->vcom_en_gpio))
-		return dev_err_probe(&pdev->dev,
+		return dev_err_probe(dev,
 				     PTR_ERR(data->vcom_en_gpio),
 				     "failed to get vcom-en gpio\n");
 
@@ -244,25 +256,24 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
 	if (data->en_gpio)
 		usleep_range(2500, 4000);
 
-	platform_set_drvdata(pdev, data);
+	i2c_set_clientdata(client, data);
 
 	ret = regmap_write(regmap, SY7636A_REG_POWER_ON_DELAY_TIME, 0x0);
 	if (ret) {
-		dev_err(pdev->dev.parent, "Failed to initialize regulator: %d\n", ret);
+		dev_err(dev, "Failed to initialize regulator: %d\n", ret);
 		return ret;
 	}
 
 	irq = gpiod_to_irq(data->pgood_gpio);
 	if (irq < 0)
-		return dev_err_probe(&pdev->dev, irq, "no power good interrupt\n");
+		return dev_err_probe(dev, irq, "no power good interrupt\n");
 
 	init_completion(&data->pgood_completion);
-	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, sy7636a_pgood_handler,
+	ret = devm_request_threaded_irq(dev, irq, NULL, sy7636a_pgood_handler,
 					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
 					"sy7636a-pgood", data);
 	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "failed to request power good interrupt\n");
+		return dev_err_probe(dev, ret, "failed to request power good interrupt\n");
 
 	/* VCOM switched by the VCOM_EN line; otherwise it follows the power on sequence */
 	if (data->vcom_en_gpio) {
@@ -272,43 +283,42 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
 			return ret;
 	}
 
-	config.dev = &pdev->dev;
+	config.dev = dev;
 	config.regmap = regmap;
 
-	rdev = devm_regulator_register(&pdev->dev, &desc, &config);
+	rdev = devm_regulator_register(dev, &desc, &config);
 	if (IS_ERR(rdev)) {
-		dev_err(pdev->dev.parent, "Failed to register %s regulator\n",
-			pdev->name);
+		dev_err(dev, "Failed to register %s regulator\n", desc.name);
 		return PTR_ERR(rdev);
 	}
 
 	if (IS_REACHABLE(CONFIG_HWMON)) {
 		struct device *hwmon_dev;
 
-		hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "sy7636a", data,
+		hwmon_dev = devm_hwmon_device_register_with_info(dev, "sy7636a", data,
 								 &sy7636a_hwmon_chip_info, NULL);
 		if (IS_ERR(hwmon_dev))
-			dev_notice(&pdev->dev, "failed to register hwmon\n");
+			dev_notice(dev, "failed to register hwmon\n");
 	}
 
 	return 0;
 }
 
-static const struct platform_device_id sy7636a_regulator_id_table[] = {
-	{ .name = "sy7636a-regulator" },
+static const struct of_device_id sy7636a_of_match[] = {
+	{ .compatible = "silergy,sy7636a" },
 	{ }
 };
-MODULE_DEVICE_TABLE(platform, sy7636a_regulator_id_table);
+MODULE_DEVICE_TABLE(of, sy7636a_of_match);
 
-static struct platform_driver sy7636a_regulator_driver = {
+static struct i2c_driver sy7636a_regulator_driver = {
 	.driver = {
 		.name = "sy7636a-regulator",
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+		.of_match_table = sy7636a_of_match,
 	},
 	.probe = sy7636a_regulator_probe,
-	.id_table = sy7636a_regulator_id_table,
 };
-module_platform_driver(sy7636a_regulator_driver);
+module_i2c_driver(sy7636a_regulator_driver);
 
 MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
 MODULE_DESCRIPTION("SY7636A voltage regulator driver");
diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
deleted file mode 100644
index 22f03b2f851e..000000000000
--- a/include/linux/mfd/sy7636a.h
+++ /dev/null
@@ -1,34 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Functions to access SY3686A power management chip.
- *
- * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
- */
-
-#ifndef __MFD_SY7636A_H
-#define __MFD_SY7636A_H
-
-#define SY7636A_REG_OPERATION_MODE_CRL		0x00
-/* It is set if a gpio is used to control the regulator */
-#define SY7636A_OPERATION_MODE_CRL_VCOMCTL	BIT(6)
-#define SY7636A_OPERATION_MODE_CRL_ONOFF	BIT(7)
-#define SY7636A_REG_VCOM_ADJUST_CTRL_L		0x01
-#define SY7636A_REG_VCOM_ADJUST_CTRL_H		0x02
-#define SY7636A_REG_VCOM_ADJUST_CTRL_MASK	0x01ff
-#define SY7636A_REG_VLDO_VOLTAGE_ADJULST_CTRL	0x03
-#define SY7636A_REG_POWER_ON_DELAY_TIME		0x06
-#define SY7636A_REG_FAULT_FLAG			0x07
-#define SY7636A_FAULT_FLAG_PG			BIT(0)
-#define SY7636A_REG_TERMISTOR_READOUT		0x08
-
-#define SY7636A_REG_MAX				0x08
-
-#define VCOM_ADJUST_CTRL_MASK	0x1ff
-// Used to shift the high byte
-#define VCOM_ADJUST_CTRL_SHIFT	8
-// Used to scale from VCOM_ADJUST_CTRL to mv
-#define VCOM_ADJUST_CTRL_SCAL	10000
-
-#define FAULT_FLAG_SHIFT	1
-
-#endif /* __LINUX_MFD_SY7636A_H */
-- 
2.52.0

Re: [PATCH 6/7] regulator: sy7636a: bind to the I2C device directly

[sashiko-bot]

> With the temperature reported here, the regulator is all that is left
> behind simple-mfd-i2c. Bind to the chip directly, as the TPS65185 and
> FP9931 drivers do, and drop the MFD glue and header. It now probes before
> the GPIOs may be ready, so report the power good GPIO with
> dev_err_probe().
> 
> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026003918.155321-1-pmazzini@gmail.com?part=6

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] regulator: sy7636a: registering hwmon device outside drivers/hwmon/
--

commit 3011502577fb90233215473b5182522faa4b0058
Author: Pablo Mazzini <pmazzini@gmail.com>

regulator: sy7636a: report the temperature

This commit removes the dedicated sy7636a hardware monitoring driver and
moves temperature reporting into the sy7636a regulator driver, reading the
thermistor register only when the rails are enabled.

> diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
> index 5705599d1b55e..816df7716c0e0 100644
> --- a/drivers/regulator/sy7636a-regulator.c
> +++ b/drivers/regulator/sy7636a-regulator.c

[ ... ]

> @@ -147,6 +148,52 @@ static const struct regulator_desc desc = {
>  	.of_match = of_match_ptr("vcom"),
>  };
>  
> +static int sy7636a_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *temp)
> +{
> +	struct sy7636a_data *data = dev_get_drvdata(dev);
> +	unsigned int mode, val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, SY7636A_REG_OPERATION_MODE_CRL, &mode);
> +	if (ret)
> +		return ret;
> +
> +	/* the thermistor is only sampled while the rails are on */
> +	if (!(mode & SY7636A_OPERATION_MODE_CRL_ONOFF))
> +		return -ENODATA;
> +
> +	ret = regmap_read(data->regmap, SY7636A_REG_TERMISTOR_READOUT, &val);
> +	if (ret)
> +		return ret;
> +
> +	*temp = (s8)val * 1000;
> +
> +	return 0;
> +}

[ ... ]

> @@ -235,6 +282,15 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
>  		return PTR_ERR(rdev);
>  	}
>  
> +	if (IS_REACHABLE(CONFIG_HWMON)) {
> +		struct device *hwmon_dev;
> +
> +		hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "sy7636a", data,
> +								 &sy7636a_hwmon_chip_info, NULL);
> +		if (IS_ERR(hwmon_dev))
> +			dev_notice(&pdev->dev, "failed to register hwmon\n");
> +	}

[Severity: Medium]
Should the hardware monitoring driver remain in drivers/hwmon/ rather than
calling devm_hwmon_device_register_with_info() directly from
sy7636a_regulator_probe() in drivers/regulator/sy7636a-regulator.c?

Hardware monitoring drivers are expected to reside in drivers/hwmon/, and
when a chip's primary functionality is in another subsystem, its hardware
monitoring support should be implemented as an auxiliary or subdevice driver
in drivers/hwmon/ rather than registering a hwmon device directly from
outside drivers/hwmon/.

While the follow-up commit ("regulator: sy7636a: bind to the i2c device
directly") removes the mfd cell table in drivers/mfd/simple-mfd-i2c.c,
sy7636a_hwmon_read() only accesses the shared regmap via regmap_read() and
does not depend on internal regulator driver state. Could the temperature
reading check be kept in drivers/hwmon/sy7636a-hwmon.c (for instance via an
auxiliary device registered by the regulator driver)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026003918.155321-1-pmazzini@gmail.com?part=5

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Pablo Mazzini]

This follows the TPS65185 and FP9931 drivers, which register their
temperature sensor in the same way.

On Mon, Oct 26, 2026 at 1:35 AM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] regulator: sy7636a: registering hwmon device outside drivers/hwmon/
> --
>
> commit 3011502577fb90233215473b5182522faa4b0058
> Author: Pablo Mazzini <pmazzini@gmail.com>
>
> regulator: sy7636a: report the temperature
>
> This commit removes the dedicated sy7636a hardware monitoring driver and
> moves temperature reporting into the sy7636a regulator driver, reading the
> thermistor register only when the rails are enabled.
>
> > diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
> > index 5705599d1b55e..816df7716c0e0 100644
> > --- a/drivers/regulator/sy7636a-regulator.c
> > +++ b/drivers/regulator/sy7636a-regulator.c
>
> [ ... ]
>
> > @@ -147,6 +148,52 @@ static const struct regulator_desc desc = {
> >       .of_match = of_match_ptr("vcom"),
> >  };
> >
> > +static int sy7636a_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> > +                           u32 attr, int channel, long *temp)
> > +{
> > +     struct sy7636a_data *data = dev_get_drvdata(dev);
> > +     unsigned int mode, val;
> > +     int ret;
> > +
> > +     ret = regmap_read(data->regmap, SY7636A_REG_OPERATION_MODE_CRL, &mode);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* the thermistor is only sampled while the rails are on */
> > +     if (!(mode & SY7636A_OPERATION_MODE_CRL_ONOFF))
> > +             return -ENODATA;
> > +
> > +     ret = regmap_read(data->regmap, SY7636A_REG_TERMISTOR_READOUT, &val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *temp = (s8)val * 1000;
> > +
> > +     return 0;
> > +}
>
> [ ... ]
>
> > @@ -235,6 +282,15 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
> >               return PTR_ERR(rdev);
> >       }
> >
> > +     if (IS_REACHABLE(CONFIG_HWMON)) {
> > +             struct device *hwmon_dev;
> > +
> > +             hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev, "sy7636a", data,
> > +                                                              &sy7636a_hwmon_chip_info, NULL);
> > +             if (IS_ERR(hwmon_dev))
> > +                     dev_notice(&pdev->dev, "failed to register hwmon\n");
> > +     }
>
> [Severity: Medium]
> Should the hardware monitoring driver remain in drivers/hwmon/ rather than
> calling devm_hwmon_device_register_with_info() directly from
> sy7636a_regulator_probe() in drivers/regulator/sy7636a-regulator.c?
>
> Hardware monitoring drivers are expected to reside in drivers/hwmon/, and
> when a chip's primary functionality is in another subsystem, its hardware
> monitoring support should be implemented as an auxiliary or subdevice driver
> in drivers/hwmon/ rather than registering a hwmon device directly from
> outside drivers/hwmon/.
>
> While the follow-up commit ("regulator: sy7636a: bind to the i2c device
> directly") removes the mfd cell table in drivers/mfd/simple-mfd-i2c.c,
> sy7636a_hwmon_read() only accesses the shared regmap via regmap_read() and
> does not depend on internal regulator driver state. Could the temperature
> reading check be kept in drivers/hwmon/sy7636a-hwmon.c (for instance via an
> auxiliary device registered by the regulator driver)?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261026003918.155321-1-pmazzini@gmail.com?part=5

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Andreas Kemnade]

On Mon, 26 Oct 2026 00:39:16 +0000
Pablo Mazzini <pmazzini@gmail.com> wrote:

> The separate hwmon cell enables VCOM at probe. Report the temperature
> from here, as the TPS65185 and FP9931 drivers do, and under the chip
> name as they do: read it while the rails are on and return -ENODATA
> otherwise. Boards that need the rails on mark VCOM always-on. Drop the
> hwmon driver.

This might deserve a Fixes: tag as it fixes a race condition in probe.
But then it depends on the 4/7 patch, so it would be quite massive.

[...]
> -
> -static int sy7636a_sensor_probe(struct platform_device *pdev)
> -{
> -	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> -	struct device *hwmon_dev;
> -	int err;
> -
> -	if (!regmap)
> -		return -EPROBE_DEFER;
> -
> -	err = devm_regulator_get_enable(&pdev->dev, "vcom");
> -	if (err)
> -		return err;
> -
and has depended on the regulator being registered first, otherwise
a dummy regulator would be provided.

Regards,
Andreas

Re: [PATCH 0/7] sy7636a: power the panel only during updates

[Andreas Kemnade]

On Mon, 26 Oct 2026 00:39:11 +0000
Pablo Mazzini <pmazzini@gmail.com> wrote:

> The SY7636A hwmon driver keeps VCOM on from probe, so a display driver
> can not power the panel only during updates.
> 
Yes, that is the common use case, power things on, tune update process
to the temperature, update, power off. So it makes completely sense.

> Read the temperature in the regulator driver instead, as the TPS65185
> and FP9931 drivers do, and remove the hwmon driver. Boards that relied
> on VCOM staying on mark it always-on. The regulator also switches VCOM
> with VCOM_EN, gains VCOM voltage control and binds to the I2C device
> directly.
> 
> Tested on a PocketBook Verse (B288).
> 
Which kernel do you use? Just curious, maybe some further upstreaming
actions might be coordinated or cross-reviewed.
There was discussion about in-kernel hwmon readout interface like it
exists for iio.

Regards,
Andreas

Re: [PATCH 0/7] sy7636a: power the panel only during updates

[Pablo Mazzini]

Mainline, on top of linux-next. I started looking at the hwmon IIO
bridge. Coordinating and cross-reviewing further work sounds good.

On Mon, Oct 26, 2026 at 9:37 AM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> On Mon, 26 Oct 2026 00:39:11 +0000
> Pablo Mazzini <pmazzini@gmail.com> wrote:
>
> > The SY7636A hwmon driver keeps VCOM on from probe, so a display driver
> > can not power the panel only during updates.
> >
> Yes, that is the common use case, power things on, tune update process
> to the temperature, update, power off. So it makes completely sense.
>
> > Read the temperature in the regulator driver instead, as the TPS65185
> > and FP9931 drivers do, and remove the hwmon driver. Boards that relied
> > on VCOM staying on mark it always-on. The regulator also switches VCOM
> > with VCOM_EN, gains VCOM voltage control and binds to the I2C device
> > directly.
> >
> > Tested on a PocketBook Verse (B288).
> >
> Which kernel do you use? Just curious, maybe some further upstreaming
> actions might be coordinated or cross-reviewed.
> There was discussion about in-kernel hwmon readout interface like it
> exists for iio.
>
> Regards,
> Andreas

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Guenter Roeck]

On Mon, Oct 26, 2026 at 12:39:16AM +0000, Pablo Mazzini wrote:
> The separate hwmon cell enables VCOM at probe. Report the temperature
> from here, as the TPS65185 and FP9931 drivers do, and under the chip
> name as they do: read it while the rails are on and return -ENODATA
> otherwise. Boards that need the rails on mark VCOM always-on. Drop the
> hwmon driver.

This is going the wrong way. The correct solution would be to convert
the hwmon driver to an auxiliary driver. I am not going to approve
this patch. I am not giving an official NACK (yet).

"Other drivers do the same wrong thing" is not an argument.

Guenter

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Andreas Kemnade]

On Mon, 26 Oct 2026 07:35:24 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> On Mon, Oct 26, 2026 at 12:39:16AM +0000, Pablo Mazzini wrote:
> > The separate hwmon cell enables VCOM at probe. Report the temperature
> > from here, as the TPS65185 and FP9931 drivers do, and under the chip
> > name as they do: read it while the rails are on and return -ENODATA
> > otherwise. Boards that need the rails on mark VCOM always-on. Drop the
> > hwmon driver.  
> 
> This is going the wrong way. The correct solution would be to convert
> the hwmon driver to an auxiliary driver. I am not going to approve
> this patch. I am not giving an official NACK (yet).
> 

First we need the problem defined, then the solution, and then we can
talk about what is the correct solution. The commit message does not
name a problem.

So what we have:
one bit to turn on a bunch of things,
after one time to sample (< 1ms), fresh values are provided on the ADC
register periodically.
after some more time (max. 50ms), several voltage outputs are ready.
Powering on those things is expensive, so you would not use this as an
ordinary temperature sensor.

The problems I see in the sensor driver this patch might want to solve:
- problems if the regulator driver is registered earlier.
- not having powered on the regulator all the time (that is described
  in the cover letter)

The way the patch handles it does not require any structural change,
availabitily of the regmap is given. So it would be a separate cleanup
patch. 

But it introduces another race condition.
The temperature might be read too early.
1ms delay should suffice.

The  SY7636A_OPERATION_MODE_CRL_ONOFF flag might be reset before 
reading the temperature. If that register does not reset to zero
but just keeps the temperature last read, then we have no problem.

If not, there needs to be some synchronisation between the regulator
part and the sensor part, probably a mutex saved in a common structure.
And in that case, structural change of the drivers is needed which
I see the following possibilities:

a) non-simple mfd driver
b) aux device as a child of the regulator driver.
c) regulator driver containing everything.

No strong personal opinion here.

Regards,
Andreas

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Pablo Mazzini]

Hi Guenter, Andreas,

On Mon, Oct 26, 2026 at 07:35:24AM -0700, Guenter Roeck wrote:
> This is going the wrong way. The correct solution would be to convert
> the hwmon driver to an auxiliary driver.

Understood. In v2 the hwmon driver stays in drivers/hwmon and becomes an
auxiliary driver of the regulator, which binds the I2C device directly.

On Tue, 27 Oct 2026 00:26:35 +0100, Andreas Kemnade wrote:
> But it introduces another race condition.
> The temperature might be read too early.
> 1ms delay should suffice.
>
> The  SY7636A_OPERATION_MODE_CRL_ONOFF flag might be reset before
> reading the temperature. If that register does not reset to zero
> but just keeps the temperature last read, then we have no problem.

Good points. For v2 I read power good (0x07) and the temperature (0x08)
in one I2C transfer, and return -ENODATA unless power good is set, so no
locking is needed.

Thanks,
Pablo

On Mon, Oct 26, 2026 at 11:26 PM Andreas Kemnade <andreas@kemnade.info> wrote:
>
> On Mon, 26 Oct 2026 07:35:24 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
> > On Mon, Oct 26, 2026 at 12:39:16AM +0000, Pablo Mazzini wrote:
> > > The separate hwmon cell enables VCOM at probe. Report the temperature
> > > from here, as the TPS65185 and FP9931 drivers do, and under the chip
> > > name as they do: read it while the rails are on and return -ENODATA
> > > otherwise. Boards that need the rails on mark VCOM always-on. Drop the
> > > hwmon driver.
> >
> > This is going the wrong way. The correct solution would be to convert
> > the hwmon driver to an auxiliary driver. I am not going to approve
> > this patch. I am not giving an official NACK (yet).
> >
>
> First we need the problem defined, then the solution, and then we can
> talk about what is the correct solution. The commit message does not
> name a problem.
>
> So what we have:
> one bit to turn on a bunch of things,
> after one time to sample (< 1ms), fresh values are provided on the ADC
> register periodically.
> after some more time (max. 50ms), several voltage outputs are ready.
> Powering on those things is expensive, so you would not use this as an
> ordinary temperature sensor.
>
> The problems I see in the sensor driver this patch might want to solve:
> - problems if the regulator driver is registered earlier.
> - not having powered on the regulator all the time (that is described
>   in the cover letter)
>
> The way the patch handles it does not require any structural change,
> availabitily of the regmap is given. So it would be a separate cleanup
> patch.
>
> But it introduces another race condition.
> The temperature might be read too early.
> 1ms delay should suffice.
>
> The  SY7636A_OPERATION_MODE_CRL_ONOFF flag might be reset before
> reading the temperature. If that register does not reset to zero
> but just keeps the temperature last read, then we have no problem.
>
> If not, there needs to be some synchronisation between the regulator
> part and the sensor part, probably a mutex saved in a common structure.
> And in that case, structural change of the drivers is needed which
> I see the following possibilities:
>
> a) non-simple mfd driver
> b) aux device as a child of the regulator driver.
> c) regulator driver containing everything.
>
> No strong personal opinion here.
>
> Regards,
> Andreas
>

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Andreas Kemnade]

On Tue, 27 Oct 2026 07:32:14 +0000
Pablo Mazzini <pmazzini@gmail.com> wrote:

> Hi Guenter, Andreas,
> 
> On Mon, Oct 26, 2026 at 07:35:24AM -0700, Guenter Roeck wrote:
> > This is going the wrong way. The correct solution would be to convert
> > the hwmon driver to an auxiliary driver.  
> 
> Understood. In v2 the hwmon driver stays in drivers/hwmon and becomes an
> auxiliary driver of the regulator, which binds the I2C device directly.
> 
But that is a cleanup action, which is at least a separate patch (maybe
replacing and exending your patch 6). So the other patches can go into
their separate trees.

> On Tue, 27 Oct 2026 00:26:35 +0100, Andreas Kemnade wrote:
> > But it introduces another race condition.
> > The temperature might be read too early.
> > 1ms delay should suffice.
> >
> > The  SY7636A_OPERATION_MODE_CRL_ONOFF flag might be reset before
> > reading the temperature. If that register does not reset to zero
> > but just keeps the temperature last read, then we have no problem.  
> 
> Good points. For v2 I read power good (0x07) and the temperature (0x08)
> in one I2C transfer, and return -ENODATA unless power good is set, so no
> locking is needed.
> 
From the data sheet we can ensure that power good occurs guaranteed after
the first sampling, so this approach is fine for the most common
use case. It reduces the time window with readable temperature
a bit more than needed, but that should not matter.

Regards,
Andreas

Re: [PATCH 5/7] regulator: sy7636a: report the temperature

[Andreas Kemnade]

On Tue, 27 Oct 2026 10:18:52 +0100
Andreas Kemnade <andreas@kemnade.info> wrote:

> On Tue, 27 Oct 2026 07:32:14 +0000
> Pablo Mazzini <pmazzini@gmail.com> wrote:
> 
> > Hi Guenter, Andreas,
> > 
> > On Mon, Oct 26, 2026 at 07:35:24AM -0700, Guenter Roeck wrote:  
> > > This is going the wrong way. The correct solution would be to convert
> > > the hwmon driver to an auxiliary driver.    
> > 
> > Understood. In v2 the hwmon driver stays in drivers/hwmon and becomes an
> > auxiliary driver of the regulator, which binds the I2C device directly.
> >   
> But that is a cleanup action, which is at least a separate patch (maybe
> replacing and exending your patch 6). So the other patches can go into
> their separate trees.
> 
looked again, I was wrong here,  vin-supply should be enabled before
i2c transfers, so we need the aux device approach.

Regards,
Andreas