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

[PATCH v2 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 only while the rails are on instead, and bind the
regulator to the I2C device directly, with the hwmon driver as its
auxiliary driver. The regulator also gains VCOM voltage control and
waits for power good on enable.

Patch 6 depends on patches 1-5 and touches the regulator, hwmon and
MFD drivers.

Tested on a PocketBook Verse (B288).

v2:
- drop the DT patch keeping VCOM always-on (Andreas)
- patch 3: only wait for power good, VCOM_EN stays as it was (Andreas)
- new patch 4: fix negative temperatures
- keep the temperature in drivers/hwmon as an auxiliary driver of the
  regulator, instead of moving it into the regulator driver (Guenter,
  Andreas)
- patch 5: no VCOM enable at probe, which also removes the probe race
- patch 5: read power good and the temperature in one transfer (Andreas)

Pablo Mazzini (7):
  regulator: sy7636a: fix the chip name
  regulator: sy7636a: add VCOM voltage control
  regulator: sy7636a: wait for power good on enable
  hwmon: sy7636a: fix negative temperatures
  hwmon: sy7636a: read the temperature only while the rails are on
  regulator: sy7636a: bind to the I2C device directly
  ARM: imx_v6_v7_defconfig: drop the SY7636A MFD option

 arch/arm/configs/imx_v6_v7_defconfig  |   1 -
 drivers/hwmon/Kconfig                 |   2 +-
 drivers/hwmon/sy7636a-hwmon.c         |  58 +++++----
 drivers/mfd/Kconfig                   |  10 --
 drivers/mfd/simple-mfd-i2c.c          |  11 --
 drivers/regulator/Kconfig             |   6 +-
 drivers/regulator/sy7636a-regulator.c | 163 ++++++++++++++++++++------
 include/linux/mfd/sy7636a.h           |  34 ------
 8 files changed, 162 insertions(+), 123 deletions(-)
 delete mode 100644 include/linux/mfd/sy7636a.h


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.52.0

[PATCH v2 1/7] regulator: sy7636a: fix the chip name

[Pablo Mazzini]

The driver header and the Kconfig help call the chip SY3686A.

Reviewed-by: Andreas Kemnade <andreas@kemnade.info>
Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/regulator/Kconfig             | 2 +-
 drivers/regulator/sy7636a-regulator.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index be6b72daa76f..88ba2770e378 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1623,7 +1623,7 @@ config REGULATOR_SY7636A
 	tristate "Silergy SY7636A voltage regulator"
 	depends on MFD_SY7636A
 	help
-	  This driver supports Silergy SY3686A voltage regulator.
+	  This driver supports Silergy SY7636A voltage regulator.
 
 config REGULATOR_SY8106A
 	tristate "Silergy SY8106A regulator"
diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
index c44c445ea139..015227cb113e 100644
--- a/drivers/regulator/sy7636a-regulator.c
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0+
 //
-// Functions to access SY3686A power management chip voltages
+// Functions to access SY7636A power management chip voltages
 //
 // Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
 //
-- 
2.52.0

[PATCH v2 2/7] regulator: sy7636a: add VCOM voltage control

[Pablo Mazzini]

VCOM is panel specific, so let boards set it in the devicetree. Switch
to the selector ops and add the write side.

Per the datasheet, VCOM bit 8 is bit 7 of register 0x02, not bit 0.

Reviewed-by: Andreas Kemnade <andreas@kemnade.info>
Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/regulator/sy7636a-regulator.c | 31 +++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
index 015227cb113e..382bab017784 100644
--- a/drivers/regulator/sy7636a-regulator.c
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -24,7 +24,7 @@ struct sy7636a_data {
 	struct gpio_desc *vcom_en_gpio;
 };
 
-static int sy7636a_get_vcom_voltage_op(struct regulator_dev *rdev)
+static int sy7636a_get_vcom_voltage_sel(struct regulator_dev *rdev)
 {
 	int ret;
 	unsigned int val, val_h;
@@ -37,9 +37,20 @@ static int sy7636a_get_vcom_voltage_op(struct regulator_dev *rdev)
 	if (ret)
 		return ret;
 
-	val |= (val_h << VCOM_ADJUST_CTRL_SHIFT);
+	/* VCOM[8] is bit 7 of register 0x02 */
+	return val | (val_h & BIT(7)) << 1;
+}
 
-	return (val & VCOM_ADJUST_CTRL_MASK) * VCOM_ADJUST_CTRL_SCAL;
+static int sy7636a_set_vcom_voltage_sel(struct regulator_dev *rdev, unsigned int sel)
+{
+	int ret;
+
+	ret = regmap_write(rdev->regmap, SY7636A_REG_VCOM_ADJUST_CTRL_L, sel & 0xff);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(rdev->regmap, SY7636A_REG_VCOM_ADJUST_CTRL_H, BIT(7),
+				  (sel & BIT(8)) >> 1);
 }
 
 static int sy7636a_get_status(struct regulator_dev *rdev)
@@ -55,19 +66,31 @@ static int sy7636a_get_status(struct regulator_dev *rdev)
 }
 
 static const struct regulator_ops sy7636a_vcom_volt_ops = {
-	.get_voltage = sy7636a_get_vcom_voltage_op,
+	.list_voltage = regulator_list_voltage_linear_range,
+	.map_voltage = regulator_map_voltage_linear_range,
+	.get_voltage_sel = sy7636a_get_vcom_voltage_sel,
+	.set_voltage_sel = sy7636a_set_vcom_voltage_sel,
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
 	.get_status = sy7636a_get_status,
 };
 
+/* 10 mV steps up to 5 V, higher codes also give 5 V */
+static const struct linear_range sy7636a_vcom_ranges[] = {
+	REGULATOR_LINEAR_RANGE(0, 0x000, 0x1f4, VCOM_ADJUST_CTRL_SCAL),
+	REGULATOR_LINEAR_RANGE(5000000, 0x1f5, 0x1ff, 0),
+};
+
 static const struct regulator_desc desc = {
 	.name = "vcom",
 	.id = 0,
 	.ops = &sy7636a_vcom_volt_ops,
 	.type = REGULATOR_VOLTAGE,
 	.owner = THIS_MODULE,
+	.n_voltages = VCOM_ADJUST_CTRL_MASK + 1,
+	.linear_ranges = sy7636a_vcom_ranges,
+	.n_linear_ranges = ARRAY_SIZE(sy7636a_vcom_ranges),
 	.enable_reg = SY7636A_REG_OPERATION_MODE_CRL,
 	.enable_mask = SY7636A_OPERATION_MODE_CRL_ONOFF,
 	.regulators_node = of_match_ptr("regulators"),
-- 
2.52.0

[PATCH v2 3/7] regulator: sy7636a: wait for power good on enable

[Pablo Mazzini]

Wait for power good on enable, as the TPS65185 and FP9931 drivers do,
so that consumers can use the rails once the enable returns.

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/regulator/sy7636a-regulator.c | 52 +++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
index 382bab017784..f87144e921da 100644
--- a/drivers/regulator/sy7636a-regulator.c
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -7,8 +7,10 @@
 // Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
 //          Alistair Francis <alistair@alistair23.me>
 
+#include <linux/completion.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/mfd/sy7636a.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -17,11 +19,14 @@
 #include <linux/regulator/machine.h>
 #include <linux/regmap.h>
 
+#define PGOOD_TIMEOUT_MSECS 200
+
 struct sy7636a_data {
 	struct regmap *regmap;
 	struct gpio_desc *pgood_gpio;
 	struct gpio_desc *en_gpio;
 	struct gpio_desc *vcom_en_gpio;
+	struct completion pgood_completion;
 };
 
 static int sy7636a_get_vcom_voltage_sel(struct regulator_dev *rdev)
@@ -65,12 +70,43 @@ static int sy7636a_get_status(struct regulator_dev *rdev)
 	return ret;
 }
 
+static irqreturn_t sy7636a_pgood_handler(int irq, void *dev_id)
+{
+	struct sy7636a_data *data = dev_id;
+
+	complete(&data->pgood_completion);
+
+	return IRQ_HANDLED;
+}
+
+/* return once the rails are good, so that consumers can use them */
+static int sy7636a_enable(struct regulator_dev *rdev)
+{
+	struct sy7636a_data *data = dev_get_drvdata(rdev->dev.parent);
+	int ret;
+
+	reinit_completion(&data->pgood_completion);
+	ret = regulator_enable_regmap(rdev);
+	if (ret)
+		return ret;
+
+	wait_for_completion_timeout(&data->pgood_completion,
+				    msecs_to_jiffies(PGOOD_TIMEOUT_MSECS));
+	ret = gpiod_get_value_cansleep(data->pgood_gpio);
+	if (ret <= 0) {
+		regulator_disable_regmap(rdev);
+		return ret ? ret : -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static const struct regulator_ops sy7636a_vcom_volt_ops = {
 	.list_voltage = regulator_list_voltage_linear_range,
 	.map_voltage = regulator_map_voltage_linear_range,
 	.get_voltage_sel = sy7636a_get_vcom_voltage_sel,
 	.set_voltage_sel = sy7636a_set_vcom_voltage_sel,
-	.enable = regulator_enable_regmap,
+	.enable = sy7636a_enable,
 	.disable = regulator_disable_regmap,
 	.is_enabled = regulator_is_enabled_regmap,
 	.get_status = sy7636a_get_status,
@@ -104,7 +140,7 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
 	struct regulator_dev *rdev;
 	struct gpio_desc *gdp;
 	struct sy7636a_data *data;
-	int ret;
+	int irq, ret;
 
 	if (!regmap)
 		return -EPROBE_DEFER;
@@ -156,6 +192,18 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	irq = gpiod_to_irq(data->pgood_gpio);
+	if (irq < 0)
+		return dev_err_probe(&pdev->dev, irq, "no power good interrupt\n");
+
+	init_completion(&data->pgood_completion);
+	ret = devm_request_threaded_irq(&pdev->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");
+
 	config.dev = &pdev->dev;
 	config.regmap = regmap;
 
-- 
2.52.0

[PATCH v2 4/7] hwmon: sy7636a: fix negative temperatures

[Pablo Mazzini]

The thermistor readout is a signed 8-bit value in degrees Celsius.

Fixes: de34a4053250 ("hwmon: sy7636a: Add temperature driver for sy7636a")
Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/hwmon/sy7636a-hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
index d51daaf63d63..f01f368d68e5 100644
--- a/drivers/hwmon/sy7636a-hwmon.c
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -29,7 +29,7 @@ static int sy7636a_read(struct device *dev, enum hwmon_sensor_types type,
 	if (ret)
 		return ret;
 
-	*temp = reg_val * 1000;
+	*temp = (s8)reg_val * 1000;
 
 	return 0;
 }
-- 
2.52.0

[PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Pablo Mazzini]

The driver enables VCOM at probe, which keeps the panel's high voltage
rails on for good, and gets a dummy regulator if it probes before the
regulator exists. Read the temperature only while the rails are on, when
the chip samples it, and return -ENODATA otherwise. Read power good with
it in one transfer, so the value can not be from before the first sample
or after the rails went off. Register it under the chip name, as the
TPS65185 and FP9931 drivers do.

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
index f01f368d68e5..475aff997c2d 100644
--- a/drivers/hwmon/sy7636a-hwmon.c
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -14,7 +14,6 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
-#include <linux/regulator/machine.h>
 
 #include <linux/mfd/sy7636a.h>
 
@@ -22,14 +21,22 @@ 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);
+	u8 buf[2];
+	int ret;
+
+	/*
+	 * Read power good and the thermistor in one transfer: the thermistor is
+	 * only sampled while the rails are on, and power good comes after the
+	 * first sample
+	 */
+	ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
 	if (ret)
 		return ret;
 
-	*temp = (s8)reg_val * 1000;
+	if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
+		return -ENODATA;
+
+	*temp = (s8)buf[1] * 1000;
 
 	return 0;
 }
@@ -72,12 +79,8 @@ static int sy7636a_sensor_probe(struct platform_device *pdev)
 	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", regmap,
 							 &sy7636a_chip_info, NULL);
 
 	if (IS_ERR(hwmon_dev)) {
-- 
2.52.0

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

[Pablo Mazzini]

Bind the regulator to the chip directly, as the TPS65185 and FP9931
drivers do, and have it create an auxiliary device for the hwmon driver,
instead of simple-mfd-i2c creating a cell for each. 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(). The hwmon driver then only
accesses the chip after vin-supply is enabled.

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 drivers/hwmon/Kconfig                 |  2 +-
 drivers/hwmon/sy7636a-hwmon.c         | 33 +++++-----
 drivers/mfd/Kconfig                   | 10 ----
 drivers/mfd/simple-mfd-i2c.c          | 11 ----
 drivers/regulator/Kconfig             |  4 +-
 drivers/regulator/sy7636a-regulator.c | 86 ++++++++++++++++-----------
 include/linux/mfd/sy7636a.h           | 34 -----------
 7 files changed, 73 insertions(+), 107 deletions(-)
 delete mode 100644 include/linux/mfd/sy7636a.h

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 18c682e1a475..247507a4ddde 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -2106,7 +2106,7 @@ config SENSORS_SIS5595
 
 config SENSORS_SY7636A
 	tristate "Silergy SY7636A"
-	depends on MFD_SY7636A
+	depends on REGULATOR_SY7636A
 	help
 	  If you say yes here you get support for the thermistor readout of
 	  the Silergy SY7636A PMIC.
diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
index 475aff997c2d..14d9516f15e7 100644
--- a/drivers/hwmon/sy7636a-hwmon.c
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -8,14 +8,15 @@
  *          Alistair Francis <alistair@alistair23.me>
  */
 
+#include <linux/auxiliary_bus.h>
+#include <linux/bits.h>
 #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/mfd/sy7636a.h>
+#define SY7636A_REG_FAULT_FLAG		0x07
+#define SY7636A_FAULT_FLAG_PG		BIT(0)
 
 static int sy7636a_read(struct device *dev, enum hwmon_sensor_types type,
 			u32 attr, int channel, long *temp)
@@ -70,36 +71,40 @@ static const struct hwmon_chip_info sy7636a_chip_info = {
 	.info = sy7636a_info,
 };
 
-static int sy7636a_sensor_probe(struct platform_device *pdev)
+static int sy7636a_sensor_probe(struct auxiliary_device *adev,
+				const struct auxiliary_device_id *id)
 {
-	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	struct regmap *regmap = dev_get_regmap(adev->dev.parent, NULL);
 	struct device *hwmon_dev;
 	int err;
 
 	if (!regmap)
-		return -EPROBE_DEFER;
+		return -ENODEV;
 
-	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+	hwmon_dev = devm_hwmon_device_register_with_info(&adev->dev,
 							 "sy7636a", 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);
+		dev_err(&adev->dev, "Unable to register hwmon device, returned %d\n", err);
 		return err;
 	}
 
 	return 0;
 }
 
-static struct platform_driver sy7636a_sensor_driver = {
+static const struct auxiliary_device_id sy7636a_sensor_ids[] = {
+	{ .name = "sy7636a_regulator.hwmon" },
+	{ }
+};
+MODULE_DEVICE_TABLE(auxiliary, sy7636a_sensor_ids);
+
+static struct auxiliary_driver sy7636a_sensor_driver = {
 	.probe = sy7636a_sensor_probe,
-	.driver = {
-		.name = "sy7636a-temperature",
-	},
+	.id_table = sy7636a_sensor_ids,
 };
-module_platform_driver(sy7636a_sensor_driver);
+module_auxiliary_driver(sy7636a_sensor_driver);
 
 MODULE_DESCRIPTION("SY7636A sensor driver");
 MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:sy7636a-temperature");
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 87ed55c155f0..c7ff8997beb1 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1319,16 +1319,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 ef3ce4bdf98b..0c9162034899 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -67,16 +67,6 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static const struct mfd_cell sy7636a_cells[] = {
-	{ .name = "sy7636a-regulator", },
-	{ .name = "sy7636a-temperature", },
-};
-
-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", },
@@ -124,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 88ba2770e378..6039abec0c37 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1621,7 +1621,9 @@ config REGULATOR_SUN20I
 
 config REGULATOR_SY7636A
 	tristate "Silergy SY7636A voltage regulator"
-	depends on MFD_SY7636A
+	depends on I2C
+	select AUXILIARY_BUS
+	select REGMAP_I2C
 	help
 	  This driver supports Silergy SY7636A voltage regulator.
 
diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
index f87144e921da..709e90517821 100644
--- a/drivers/regulator/sy7636a-regulator.c
+++ b/drivers/regulator/sy7636a-regulator.c
@@ -7,18 +7,26 @@
 // Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
 //          Alistair Francis <alistair@alistair23.me>
 
+#include <linux/auxiliary_bus.h>
 #include <linux/completion.h>
 #include <linux/gpio/consumer.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_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 VCOM_ADJUST_CTRL_MASK	0x1ff
+#define VCOM_ADJUST_CTRL_SCAL	10000
+
 #define PGOOD_TIMEOUT_MSECS 200
 
 struct sy7636a_data {
@@ -133,50 +141,54 @@ static const struct regulator_desc desc = {
 	.of_match = of_match_ptr("vcom"),
 };
 
-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 auxiliary_device *adev;
 	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");
 
 	/* Let VCOM just follow the default power on sequence */
-	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");
 
@@ -184,54 +196,56 @@ 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");
 
-	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);
 	}
 
+	adev = devm_auxiliary_device_create(dev, "hwmon", NULL);
+	if (!adev)
+		return dev_err_probe(dev, -ENODEV, "failed to create hwmon device\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

[PATCH v2 7/7] ARM: imx_v6_v7_defconfig: drop the SY7636A MFD option

[Pablo Mazzini]

The SY7636A is no longer bound through an MFD.

Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index ed588add8d31..f02b5605e533 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -239,7 +239,6 @@ CONFIG_MFD_DA9062=y
 CONFIG_MFD_DA9063=y
 CONFIG_MFD_MC13XXX_SPI=y
 CONFIG_MFD_MC13XXX_I2C=y
-CONFIG_MFD_SY7636A=y
 CONFIG_MFD_RN5T618=y
 CONFIG_MFD_STMPE=y
 CONFIG_MFD_ROHM_BD71828=y
-- 
2.52.0

Re: [PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Guenter Roeck]

On 11/1/26 14:31, Pablo Mazzini wrote:
> The driver enables VCOM at probe, which keeps the panel's high voltage
> rails on for good, and gets a dummy regulator if it probes before the
> regulator exists. Read the temperature only while the rails are on, when
> the chip samples it, and return -ENODATA otherwise. Read power good with
> it in one transfer, so the value can not be from before the first sample
> or after the rails went off. Register it under the chip name, as the
> TPS65185 and FP9931 drivers do.
> 
> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
> ---
>   drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> index f01f368d68e5..475aff997c2d 100644
> --- a/drivers/hwmon/sy7636a-hwmon.c
> +++ b/drivers/hwmon/sy7636a-hwmon.c
> @@ -14,7 +14,6 @@
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/regmap.h>
> -#include <linux/regulator/machine.h>
>   
>   #include <linux/mfd/sy7636a.h>
>   
> @@ -22,14 +21,22 @@ 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);
> +	u8 buf[2];
> +	int ret;
> +
> +	/*
> +	 * Read power good and the thermistor in one transfer: the thermistor is
> +	 * only sampled while the rails are on, and power good comes after the
> +	 * first sample
> +	 */
> +	ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
>   	if (ret)
>   		return ret;
>   
> -	*temp = (s8)reg_val * 1000;
> +	if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
> +		return -ENODATA;
> +

I had a look at the datasheet. It does not say anything about the
temperature not being measured when PG is not set.

The datasheet does suggest that temperatures are only measured
if bit 7 of the operation mode register is set, but that is
different from the "power good" bit in the fault register.

So why the association with the PG bit and not with the On/Off bit
of the operation mode register ? That warrants some more explanation.

Thanks,
Guenter
  > +	*temp = (s8)buf[1] * 1000;
>   
>   	return 0;
>   }
> @@ -72,12 +79,8 @@ static int sy7636a_sensor_probe(struct platform_device *pdev)
>   	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", regmap,
>   							 &sy7636a_chip_info, NULL);
>   
>   	if (IS_ERR(hwmon_dev)) {

Re: [PATCH v2 3/7] regulator: sy7636a: wait for power good on enable

[Andreas Kemnade]

On Sun,  1 Nov 2026 22:31:21 +0000
Pablo Mazzini <pmazzini@gmail.com> wrote:

> Wait for power good on enable, as the TPS65185 and FP9931 drivers do,
> so that consumers can use the rails once the enable returns.
> 
> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>

Reviewed-by: Andreas Kemnade <andreas@kemnade.info>

Regards,
Andreas

> ---
>  drivers/regulator/sy7636a-regulator.c | 52 +++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/sy7636a-regulator.c b/drivers/regulator/sy7636a-regulator.c
> index 382bab017784..f87144e921da 100644
> --- a/drivers/regulator/sy7636a-regulator.c
> +++ b/drivers/regulator/sy7636a-regulator.c
> @@ -7,8 +7,10 @@
>  // Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
>  //          Alistair Francis <alistair@alistair23.me>
>  
> +#include <linux/completion.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> +#include <linux/interrupt.h>
>  #include <linux/mfd/sy7636a.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -17,11 +19,14 @@
>  #include <linux/regulator/machine.h>
>  #include <linux/regmap.h>
>  
> +#define PGOOD_TIMEOUT_MSECS 200
> +
>  struct sy7636a_data {
>  	struct regmap *regmap;
>  	struct gpio_desc *pgood_gpio;
>  	struct gpio_desc *en_gpio;
>  	struct gpio_desc *vcom_en_gpio;
> +	struct completion pgood_completion;
>  };
>  
>  static int sy7636a_get_vcom_voltage_sel(struct regulator_dev *rdev)
> @@ -65,12 +70,43 @@ static int sy7636a_get_status(struct regulator_dev *rdev)
>  	return ret;
>  }
>  
> +static irqreturn_t sy7636a_pgood_handler(int irq, void *dev_id)
> +{
> +	struct sy7636a_data *data = dev_id;
> +
> +	complete(&data->pgood_completion);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +/* return once the rails are good, so that consumers can use them */
> +static int sy7636a_enable(struct regulator_dev *rdev)
> +{
> +	struct sy7636a_data *data = dev_get_drvdata(rdev->dev.parent);
> +	int ret;
> +
> +	reinit_completion(&data->pgood_completion);
> +	ret = regulator_enable_regmap(rdev);
> +	if (ret)
> +		return ret;
> +
> +	wait_for_completion_timeout(&data->pgood_completion,
> +				    msecs_to_jiffies(PGOOD_TIMEOUT_MSECS));
> +	ret = gpiod_get_value_cansleep(data->pgood_gpio);
> +	if (ret <= 0) {
> +		regulator_disable_regmap(rdev);
> +		return ret ? ret : -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct regulator_ops sy7636a_vcom_volt_ops = {
>  	.list_voltage = regulator_list_voltage_linear_range,
>  	.map_voltage = regulator_map_voltage_linear_range,
>  	.get_voltage_sel = sy7636a_get_vcom_voltage_sel,
>  	.set_voltage_sel = sy7636a_set_vcom_voltage_sel,
> -	.enable = regulator_enable_regmap,
> +	.enable = sy7636a_enable,
>  	.disable = regulator_disable_regmap,
>  	.is_enabled = regulator_is_enabled_regmap,
>  	.get_status = sy7636a_get_status,
> @@ -104,7 +140,7 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
>  	struct regulator_dev *rdev;
>  	struct gpio_desc *gdp;
>  	struct sy7636a_data *data;
> -	int ret;
> +	int irq, ret;
>  
>  	if (!regmap)
>  		return -EPROBE_DEFER;
> @@ -156,6 +192,18 @@ static int sy7636a_regulator_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	irq = gpiod_to_irq(data->pgood_gpio);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq, "no power good interrupt\n");
> +
> +	init_completion(&data->pgood_completion);
> +	ret = devm_request_threaded_irq(&pdev->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");
> +
>  	config.dev = &pdev->dev;
>  	config.regmap = regmap;
>  

Re: [PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Pablo Mazzini]

Thanks for looking at it.

Reading the temperature races with the rails being switched on and
off, as Andreas pointed out on v1.

PG only goes high once all rails are up, after the first measurement.
It is also in register 0x07, next to the thermistor readout, so one
short transfer reads both.

Pablo

On Mon, Nov 2, 2026 at 3:35 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 11/1/26 14:31, Pablo Mazzini wrote:
> > The driver enables VCOM at probe, which keeps the panel's high voltage
> > rails on for good, and gets a dummy regulator if it probes before the
> > regulator exists. Read the temperature only while the rails are on, when
> > the chip samples it, and return -ENODATA otherwise. Read power good with
> > it in one transfer, so the value can not be from before the first sample
> > or after the rails went off. Register it under the chip name, as the
> > TPS65185 and FP9931 drivers do.
> >
> > Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
> > ---
> >   drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> > index f01f368d68e5..475aff997c2d 100644
> > --- a/drivers/hwmon/sy7636a-hwmon.c
> > +++ b/drivers/hwmon/sy7636a-hwmon.c
> > @@ -14,7 +14,6 @@
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/regmap.h>
> > -#include <linux/regulator/machine.h>
> >
> >   #include <linux/mfd/sy7636a.h>
> >
> > @@ -22,14 +21,22 @@ 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);
> > +     u8 buf[2];
> > +     int ret;
> > +
> > +     /*
> > +      * Read power good and the thermistor in one transfer: the thermistor is
> > +      * only sampled while the rails are on, and power good comes after the
> > +      * first sample
> > +      */
> > +     ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
> >       if (ret)
> >               return ret;
> >
> > -     *temp = (s8)reg_val * 1000;
> > +     if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
> > +             return -ENODATA;
> > +
>
> I had a look at the datasheet. It does not say anything about the
> temperature not being measured when PG is not set.
>
> The datasheet does suggest that temperatures are only measured
> if bit 7 of the operation mode register is set, but that is
> different from the "power good" bit in the fault register.
>
> So why the association with the PG bit and not with the On/Off bit
> of the operation mode register ? That warrants some more explanation.
>
> Thanks,
> Guenter
>   > +   *temp = (s8)buf[1] * 1000;
> >
> >       return 0;
> >   }
> > @@ -72,12 +79,8 @@ static int sy7636a_sensor_probe(struct platform_device *pdev)
> >       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", regmap,
> >                                                        &sy7636a_chip_info, NULL);
> >
> >       if (IS_ERR(hwmon_dev)) {
>

Re: [PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Andreas Kemnade]

On Sun, 1 Nov 2026 19:35:16 -0800
Guenter Roeck <linux@roeck-us.net> wrote:

> On 11/1/26 14:31, Pablo Mazzini wrote:
> > The driver enables VCOM at probe, which keeps the panel's high voltage
> > rails on for good, and gets a dummy regulator if it probes before the
> > regulator exists. Read the temperature only while the rails are on, when
> > the chip samples it, and return -ENODATA otherwise. Read power good with
> > it in one transfer, so the value can not be from before the first sample
> > or after the rails went off. Register it under the chip name, as the
> > TPS65185 and FP9931 drivers do.
> > 
> > Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
> > ---
> >   drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
> >   1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> > index f01f368d68e5..475aff997c2d 100644
> > --- a/drivers/hwmon/sy7636a-hwmon.c
> > +++ b/drivers/hwmon/sy7636a-hwmon.c
> > @@ -14,7 +14,6 @@
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/regmap.h>
> > -#include <linux/regulator/machine.h
> >   
> >   #include <linux/mfd/sy7636a.h>
> >   
> > @@ -22,14 +21,22 @@ 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);
> > +	u8 buf[2];
> > +	int ret;
> > +
> > +	/*
> > +	 * Read power good and the thermistor in one transfer: the thermistor is
> > +	 * only sampled while the rails are on, and power good comes after the
> > +	 * first sample
> > +	 */
> > +	ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
> >   	if (ret)
> >   		return ret;
> >   
> > -	*temp = (s8)reg_val * 1000;
> > +	if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
> > +		return -ENODATA;
> > +  
> 
> I had a look at the datasheet. It does not say anything about the
> temperature not being measured when PG is not set.
> 
That is what I read from there, too. But the PG bit will only
be set if the On/Off bit is also set.

> The datasheet does suggest that temperatures are only measured
> if bit 7 of the operation mode register is set, but that is
> different from the "power good" bit in the fault register.
> 
I can confirm from tests that temperature reads zero if regulator
was not yet powered on.

I read the datasheet in that way so things happen it that order:

1. on bit is set
2. ramping up voltages start / temperature sampling starts.
3. after some time (sampling time + conversion time (ADC_SPL+ADC_DEL)?),
   the first sample is ready and the temperature can be read.
4. after some more time voltage lines stabilize
5. after 1.5 ms PG is asserted

> So why the association with the PG bit and not with the On/Off bit
> of the operation mode register ? That warrants some more explanation.

That should definitively be commented in the code. I have pointed out
races earlier. This has of course the drawback that it limits the time
frame in which reads are possible a bit. I doubt it is worth dealing with
synchronisation code to use that on/off bit (although posssible).

Regards,
Andreas

Re: [PATCH v2 3/7] regulator: sy7636a: wait for power good on enable

[Mark Brown]

> Wait for power good on enable, as the TPS65185 and FP9931 drivers do,
> so that consumers can use the rails once the enable returns.

> +static int sy7636a_enable(struct regulator_dev *rdev)
> +{
> +	struct sy7636a_data *data = dev_get_drvdata(rdev->dev.parent);
> +	int ret;
> +
> +	reinit_completion(&data->pgood_completion);
> +	ret = regulator_enable_regmap(rdev);
> +	if (ret)
> +		return ret;
> +
> +	wait_for_completion_timeout(&data->pgood_completion,
> +				    msecs_to_jiffies(PGOOD_TIMEOUT_MSECS));
> +	ret = gpiod_get_value_cansleep(data->pgood_gpio);
> +	if (ret <= 0) {
> +		regulator_disable_regmap(rdev);
> +		return ret ? ret : -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

One thing here is that if the regulator was already enabled on power up
we'll get the full delay during startup if the regulator was configured
as boot on but already enabled.

-- 
Mark Brown <broonie@kernel.org>

Re: [PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Guenter Roeck]

On 11/2/26 00:09, Pablo Mazzini wrote:
> Thanks for looking at it.
> 
> Reading the temperature races with the rails being switched on and
> off, as Andreas pointed out on v1.
> 

This should be explained as comment, with source (I don't see that
in the datasheet).

> PG only goes high once all rails are up, after the first measurement.
> It is also in register 0x07, next to the thermistor readout, so one
> short transfer reads both.
> 

That is not really an argument. With the current implementation,
static state with EN=1 and PG=0 will report no temperatures even though
temperatures may be available.

Guenter

> Pablo
> 
> On Mon, Nov 2, 2026 at 3:35 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 11/1/26 14:31, Pablo Mazzini wrote:
>>> The driver enables VCOM at probe, which keeps the panel's high voltage
>>> rails on for good, and gets a dummy regulator if it probes before the
>>> regulator exists. Read the temperature only while the rails are on, when
>>> the chip samples it, and return -ENODATA otherwise. Read power good with
>>> it in one transfer, so the value can not be from before the first sample
>>> or after the rails went off. Register it under the chip name, as the
>>> TPS65185 and FP9931 drivers do.
>>>
>>> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
>>> ---
>>>    drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
>>>    1 file changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
>>> index f01f368d68e5..475aff997c2d 100644
>>> --- a/drivers/hwmon/sy7636a-hwmon.c
>>> +++ b/drivers/hwmon/sy7636a-hwmon.c
>>> @@ -14,7 +14,6 @@
>>>    #include <linux/module.h>
>>>    #include <linux/platform_device.h>
>>>    #include <linux/regmap.h>
>>> -#include <linux/regulator/machine.h>
>>>
>>>    #include <linux/mfd/sy7636a.h>
>>>
>>> @@ -22,14 +21,22 @@ 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);
>>> +     u8 buf[2];
>>> +     int ret;
>>> +
>>> +     /*
>>> +      * Read power good and the thermistor in one transfer: the thermistor is
>>> +      * only sampled while the rails are on, and power good comes after the
>>> +      * first sample
>>> +      */
>>> +     ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
>>>        if (ret)
>>>                return ret;
>>>
>>> -     *temp = (s8)reg_val * 1000;
>>> +     if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
>>> +             return -ENODATA;
>>> +
>>
>> I had a look at the datasheet. It does not say anything about the
>> temperature not being measured when PG is not set.
>>
>> The datasheet does suggest that temperatures are only measured
>> if bit 7 of the operation mode register is set, but that is
>> different from the "power good" bit in the fault register.
>>
>> So why the association with the PG bit and not with the On/Off bit
>> of the operation mode register ? That warrants some more explanation.
>>
>> Thanks,
>> Guenter
>>    > +   *temp = (s8)buf[1] * 1000;
>>>
>>>        return 0;
>>>    }
>>> @@ -72,12 +79,8 @@ static int sy7636a_sensor_probe(struct platform_device *pdev)
>>>        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", regmap,
>>>                                                         &sy7636a_chip_info, NULL);
>>>
>>>        if (IS_ERR(hwmon_dev)) {
>>

Re: [PATCH v2 5/7] hwmon: sy7636a: read the temperature only while the rails are on

[Guenter Roeck]

On 11/2/26 02:25, Andreas Kemnade wrote:
> On Sun, 1 Nov 2026 19:35:16 -0800
> Guenter Roeck <linux@roeck-us.net> wrote:
> 
>> On 11/1/26 14:31, Pablo Mazzini wrote:
>>> The driver enables VCOM at probe, which keeps the panel's high voltage
>>> rails on for good, and gets a dummy regulator if it probes before the
>>> regulator exists. Read the temperature only while the rails are on, when
>>> the chip samples it, and return -ENODATA otherwise. Read power good with
>>> it in one transfer, so the value can not be from before the first sample
>>> or after the rails went off. Register it under the chip name, as the
>>> TPS65185 and FP9931 drivers do.
>>>
>>> Signed-off-by: Pablo Mazzini <pmazzini@gmail.com>
>>> ---
>>>    drivers/hwmon/sy7636a-hwmon.c | 25 ++++++++++++++-----------
>>>    1 file changed, 14 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
>>> index f01f368d68e5..475aff997c2d 100644
>>> --- a/drivers/hwmon/sy7636a-hwmon.c
>>> +++ b/drivers/hwmon/sy7636a-hwmon.c
>>> @@ -14,7 +14,6 @@
>>>    #include <linux/module.h>
>>>    #include <linux/platform_device.h>
>>>    #include <linux/regmap.h>
>>> -#include <linux/regulator/machine.h
>>>    
>>>    #include <linux/mfd/sy7636a.h>
>>>    
>>> @@ -22,14 +21,22 @@ 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);
>>> +	u8 buf[2];
>>> +	int ret;
>>> +
>>> +	/*
>>> +	 * Read power good and the thermistor in one transfer: the thermistor is
>>> +	 * only sampled while the rails are on, and power good comes after the
>>> +	 * first sample
>>> +	 */
>>> +	ret = regmap_bulk_read(regmap, SY7636A_REG_FAULT_FLAG, buf, sizeof(buf));
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> -	*temp = (s8)reg_val * 1000;
>>> +	if (!(buf[0] & SY7636A_FAULT_FLAG_PG))
>>> +		return -ENODATA;
>>> +
>>
>> I had a look at the datasheet. It does not say anything about the
>> temperature not being measured when PG is not set.
>>
> That is what I read from there, too. But the PG bit will only
> be set if the On/Off bit is also set.
> 
>> The datasheet does suggest that temperatures are only measured
>> if bit 7 of the operation mode register is set, but that is
>> different from the "power good" bit in the fault register.
>>
> I can confirm from tests that temperature reads zero if regulator
> was not yet powered on.
> 
> I read the datasheet in that way so things happen it that order:
> 
> 1. on bit is set
> 2. ramping up voltages start / temperature sampling starts.
> 3. after some time (sampling time + conversion time (ADC_SPL+ADC_DEL)?),
>     the first sample is ready and the temperature can be read.
> 4. after some more time voltage lines stabilize
> 5. after 1.5 ms PG is asserted
> 
>> So why the association with the PG bit and not with the On/Off bit
>> of the operation mode register ? That warrants some more explanation.
> 
> That should definitively be commented in the code. I have pointed out
> races earlier. This has of course the drawback that it limits the time
> frame in which reads are possible a bit. I doubt it is worth dealing with
> synchronisation code to use that on/off bit (although posssible).
> 
As mentioned in the other mail, downside is that the temperature will not
be reported if EN=1 and PG=0 (for whatever reason), even though
the temperature may be available. At the very least the reason for
accepting this limitation will need to be explained in the driver.

Guenter