[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 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

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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hwmon: sy7636a: returning -ENODATA when rails are off disables thermal zone
--

commit 72fc8d76dc4f0fe2ee1ec4e02805a6f20803ef65
Author: Pablo Mazzini <pmazzini@gmail.com>

hwmon: sy7636a: read the temperature only while the rails are on

This commit stops enabling the VCOM regulator at probe, registers the hwmon
device under the chip name "sy7636a", and reads the fault flag alongside the
thermistor in a single bulk transfer, returning -ENODATA when power good is
not set.

> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> index f01f368d68e5f..475aff997c2d2 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;

[Severity: High]
Will returning -ENODATA in sy7636a_read() when SY7636A_FAULT_FLAG_PG is
cleared cause the thermal core to permanently disable the attached thermal
zone when the rails remain off?

In drivers/hwmon/sy7636a-hwmon.c, sy7636a_info sets HWMON_C_REGISTER_TZ:

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
};

In arch/arm/boot/dts/nxp/imx/imx7d-remarkable2.dts, the epd-thermal zone
binds to &sy7636a with a 30-second polling delay while the vcom regulator only
specifies regulator-boot-on:

	thermal-zones {
		epd-thermal {
			thermal-sensors = <&sy7636a>;
			polling-delay-passive = <30000>;
			polling-delay = <30000>;

When the thermal zone is polled while the EPD rails are off, the call chain
__thermal_zone_device_update() -> __thermal_zone_get_temp() ->
hwmon_thermal_get_temp() -> sy7636a_read() passes -ENODATA to
thermal_zone_recheck() in drivers/thermal/thermal_core.c:

static void thermal_zone_recheck(struct thermal_zone_device *tz, int error)
{
	if (error == -EAGAIN) {
		thermal_zone_device_set_polling(tz, THERMAL_RECHECK_DELAY);
		return;
	}

	if (tz->recheck_delay_jiffies == THERMAL_RECHECK_DELAY)
		dev_info(&tz->device, "Temperature check failed (%d)\n", error);

	thermal_zone_device_set_polling(tz, tz->recheck_delay_jiffies);

	tz->recheck_delay_jiffies += max(tz->recheck_delay_jiffies >> 1, 1ULL);
	if (tz->recheck_delay_jiffies > THERMAL_MAX_RECHECK_DELAY) {
		thermal_zone_broken_disable(tz);

Because thermal_zone_recheck() only treats -EAGAIN as a non-fatal retry and
treats -ENODATA as a sensor failure, it logs an error, switches to
exponential backoff polling starting at 250 ms, and after roughly 5.5 minutes
with the rails off calls thermal_zone_broken_disable(), which sets the thermal
zone mode to THERMAL_DEVICE_DISABLED so thermal monitoring remains disabled
even after the rails are turned back on.

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

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

[Pablo Mazzini]

Right. On the reMarkable 2 nothing in mainline turns the rails on.
Keeping the rails on is out, so I would drop the epd-thermal zone from
imx7d-remarkable2.dts in v3.

Andreas, is that OK with you, or would you prefer something else?

On Sun, Nov 1, 2026 at 10:51 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] hwmon: sy7636a: returning -ENODATA when rails are off disables thermal zone
> --
>
> commit 72fc8d76dc4f0fe2ee1ec4e02805a6f20803ef65
> Author: Pablo Mazzini <pmazzini@gmail.com>
>
> hwmon: sy7636a: read the temperature only while the rails are on
>
> This commit stops enabling the VCOM regulator at probe, registers the hwmon
> device under the chip name "sy7636a", and reads the fault flag alongside the
> thermistor in a single bulk transfer, returning -ENODATA when power good is
> not set.
>
> > diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> > index f01f368d68e5f..475aff997c2d2 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;
>
> [Severity: High]
> Will returning -ENODATA in sy7636a_read() when SY7636A_FAULT_FLAG_PG is
> cleared cause the thermal core to permanently disable the attached thermal
> zone when the rails remain off?
>
> In drivers/hwmon/sy7636a-hwmon.c, sy7636a_info sets HWMON_C_REGISTER_TZ:
>
> 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
> };
>
> In arch/arm/boot/dts/nxp/imx/imx7d-remarkable2.dts, the epd-thermal zone
> binds to &sy7636a with a 30-second polling delay while the vcom regulator only
> specifies regulator-boot-on:
>
>         thermal-zones {
>                 epd-thermal {
>                         thermal-sensors = <&sy7636a>;
>                         polling-delay-passive = <30000>;
>                         polling-delay = <30000>;
>
> When the thermal zone is polled while the EPD rails are off, the call chain
> __thermal_zone_device_update() -> __thermal_zone_get_temp() ->
> hwmon_thermal_get_temp() -> sy7636a_read() passes -ENODATA to
> thermal_zone_recheck() in drivers/thermal/thermal_core.c:
>
> static void thermal_zone_recheck(struct thermal_zone_device *tz, int error)
> {
>         if (error == -EAGAIN) {
>                 thermal_zone_device_set_polling(tz, THERMAL_RECHECK_DELAY);
>                 return;
>         }
>
>         if (tz->recheck_delay_jiffies == THERMAL_RECHECK_DELAY)
>                 dev_info(&tz->device, "Temperature check failed (%d)\n", error);
>
>         thermal_zone_device_set_polling(tz, tz->recheck_delay_jiffies);
>
>         tz->recheck_delay_jiffies += max(tz->recheck_delay_jiffies >> 1, 1ULL);
>         if (tz->recheck_delay_jiffies > THERMAL_MAX_RECHECK_DELAY) {
>                 thermal_zone_broken_disable(tz);
>
> Because thermal_zone_recheck() only treats -EAGAIN as a non-fatal retry and
> treats -ENODATA as a sensor failure, it logs an error, switches to
> exponential backoff polling starting at 250 ms, and after roughly 5.5 minutes
> with the rails off calls thermal_zone_broken_disable(), which sets the thermal
> zone mode to THERMAL_DEVICE_DISABLED so thermal monitoring remains disabled
> even after the rails are turned back on.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20261101223125.302001-1-pmazzini@gmail.com?part=5

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

[Guenter Roeck]

On 11/1/26 14:51, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] hwmon: sy7636a: returning -ENODATA when rails are off disables thermal zone
> --
> 
> commit 72fc8d76dc4f0fe2ee1ec4e02805a6f20803ef65
> Author: Pablo Mazzini <pmazzini@gmail.com>
> 
> hwmon: sy7636a: read the temperature only while the rails are on
> 
> This commit stops enabling the VCOM regulator at probe, registers the hwmon
> device under the chip name "sy7636a", and reads the fault flag alongside the
> thermistor in a single bulk transfer, returning -ENODATA when power good is
> not set.
> 
>> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
>> index f01f368d68e5f..475aff997c2d2 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;
> 
> [Severity: High]
> Will returning -ENODATA in sy7636a_read() when SY7636A_FAULT_FLAG_PG is
> cleared cause the thermal core to permanently disable the attached thermal
> zone when the rails remain off?
> 
> In drivers/hwmon/sy7636a-hwmon.c, sy7636a_info sets HWMON_C_REGISTER_TZ:
> 
> 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
> };
> 
> In arch/arm/boot/dts/nxp/imx/imx7d-remarkable2.dts, the epd-thermal zone
> binds to &sy7636a with a 30-second polling delay while the vcom regulator only
> specifies regulator-boot-on:
> 
> 	thermal-zones {
> 		epd-thermal {
> 			thermal-sensors = <&sy7636a>;
> 			polling-delay-passive = <30000>;
> 			polling-delay = <30000>;
> 
> When the thermal zone is polled while the EPD rails are off, the call chain
> __thermal_zone_device_update() -> __thermal_zone_get_temp() ->
> hwmon_thermal_get_temp() -> sy7636a_read() passes -ENODATA to
> thermal_zone_recheck() in drivers/thermal/thermal_core.c:
> 
> static void thermal_zone_recheck(struct thermal_zone_device *tz, int error)
> {
> 	if (error == -EAGAIN) {
> 		thermal_zone_device_set_polling(tz, THERMAL_RECHECK_DELAY);
> 		return;
> 	}
> 
> 	if (tz->recheck_delay_jiffies == THERMAL_RECHECK_DELAY)
> 		dev_info(&tz->device, "Temperature check failed (%d)\n", error);
> 
> 	thermal_zone_device_set_polling(tz, tz->recheck_delay_jiffies);
> 
> 	tz->recheck_delay_jiffies += max(tz->recheck_delay_jiffies >> 1, 1ULL);
> 	if (tz->recheck_delay_jiffies > THERMAL_MAX_RECHECK_DELAY) {
> 		thermal_zone_broken_disable(tz);
> 
> Because thermal_zone_recheck() only treats -EAGAIN as a non-fatal retry and
> treats -ENODATA as a sensor failure, it logs an error, switches to
> exponential backoff polling starting at 250 ms, and after roughly 5.5 minutes
> with the rails off calls thermal_zone_broken_disable(), which sets the thermal
> zone mode to THERMAL_DEVICE_DISABLED so thermal monitoring remains disabled
> even after the rails are turned back on.
> 

-ENODATA is really meant for temporary conditions, not for permanent conditions.
It may also be returned if a sensor is explicitly disabled, for example with the
'enable' attribute. If the rail is never enabled, it might make sense to filter
out affected temperature sensors in the is_visible function.

Other than that, there is not much we can do. Returning -EAGAIN from the
driver would be inappropriate and might trigger a hard (immediate) retry
loop if reported to userspace. Converting -ENODATA to -EAGAIN in
hwmon_thermal_get_temp() might be an option but, again, -ENODATA
should be a temporary condition. The retry mechanism in the thermal
subsystem essentially works as expected - it disables the thermal zone
if the recheck delay is above 120 seconds, and if a sensor was still
not readable after that time it is unlikely that it recovers on its own.

Thanks,
Guenter

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

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

[Pablo Mazzini]

OK, I will explain it in a comment in v3, including the limitation.

On Mon, Nov 2, 2026 at 2:43 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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
>