[PATCH v7 1/6] mfd: sy7636a: Initial commit

[PATCH v7 1/6] mfd: sy7636a: Initial commit

[Alistair Francis]

Initial support for the Silergy SY7636A Power Management chip.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/mfd/Kconfig         |  9 +++++
 drivers/mfd/Makefile        |  1 +
 drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
 4 files changed, 138 insertions(+)
 create mode 100644 drivers/mfd/sy7636a.c
 create mode 100644 include/linux/mfd/sy7636a.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6a3fd2d75f96..7b59aa0fd3f2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1352,6 +1352,15 @@ config MFD_SYSCON
 	  Select this option to enable accessing system control registers
 	  via regmap.
 
+config MFD_SY7636A
+	tristate "Silergy SY7636A Power Management chip"
+	select MFD_CORE
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Select this option to enable support for the Silergy SY7636A
+	  Power Management chip.
+
 config MFD_DAVINCI_VOICECODEC
 	tristate
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 8116c19d5fd4..cbe581e87fa9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o
 obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o
 
+obj-$(CONFIG_MFD_SY7636A)	+= sy7636a.o
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
 obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
 obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
new file mode 100644
index 000000000000..345892e11221
--- /dev/null
+++ b/drivers/mfd/sy7636a.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// MFD parent driver for SY7636A chip
+//
+// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
+//
+// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+//          Alistair Francis <alistair@alistair23.me>
+//
+// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
+
+#include <linux/interrupt.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+
+#include <linux/mfd/sy7636a.h>
+
+static const struct regmap_config sy7636a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct mfd_cell sy7636a_cells[] = {
+	{ .name = "sy7636a-regulator", },
+	{ .name = "sy7636a-temperature", },
+	{ .name = "sy7636a-thermal", },
+};
+
+static const struct of_device_id of_sy7636a_match_table[] = {
+	{ .compatible = "silergy,sy7636a", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);
+
+static int sy7636a_probe(struct i2c_client *client,
+			 const struct i2c_device_id *ids)
+{
+	struct sy7636a *sy7636a;
+	int ret;
+
+	sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);
+	if (!sy7636a)
+		return -ENOMEM;
+
+	sy7636a->dev = &client->dev;
+
+	sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
+	if (IS_ERR(sy7636a->regmap)) {
+		ret = PTR_ERR(sy7636a->regmap);
+		dev_err(sy7636a->dev,
+			"Failed to initialize register map: %d\n", ret);
+		return ret;
+	}
+
+	i2c_set_clientdata(client, sy7636a);
+
+	return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
+					sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
+					NULL, 0, NULL);
+}
+
+static const struct i2c_device_id sy7636a_id_table[] = {
+	{ "sy7636a", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);
+
+static struct i2c_driver sy7636a_driver = {
+	.driver	= {
+		.name	= "sy7636a",
+		.of_match_table = of_sy7636a_match_table,
+	},
+	.probe = sy7636a_probe,
+	.id_table = sy7636a_id_table,
+};
+module_i2c_driver(sy7636a_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
+MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
new file mode 100644
index 000000000000..b6845a3572b8
--- /dev/null
+++ b/include/linux/mfd/sy7636a.h
@@ -0,0 +1,47 @@
+/* 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
+
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+#define SY7636A_REG_OPERATION_MODE_CRL		0x00
+#define SY7636A_OPERATION_MODE_CRL_VCOMCTL	BIT(6)
+#define SY7636A_OPERATION_MODE_CRL_ONOFF	BIT(7)
+#define SY7636A_REG_VCOM_ADJUST_CTRL_L		0x01
+#define SY7636A_REG_VCOM_ADJUST_CTRL_H		0x02
+#define SY7636A_REG_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_MIN		0
+#define VCOM_MAX		5000
+
+#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
+
+struct sy7636a {
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *pgood_gpio;
+};
+
+#endif /* __LINUX_MFD_SY7636A_H */
-- 
2.31.1

[PATCH v7 2/6] thermal: sy7636a: Add thermal driver for sy7636a

[Alistair Francis]

From: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.no>

Add thermal driver to enable kernel based polling
and shutdown of device for temperatures out of spec

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 drivers/thermal/Kconfig           |   7 ++
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/sy7636a_thermal.c | 107 ++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/thermal/sy7636a_thermal.c

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index d7f44deab5b1..7112c63d9021 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -450,6 +450,13 @@ depends on (ARCH_STI || ARCH_STM32) && OF
 source "drivers/thermal/st/Kconfig"
 endmenu
 
+config SY7636A_THERMAL
+	tristate "SY7636A thermal management"
+	depends on MFD_SY7636A
+	help
+	  Enable the sy7636a thermal driver, which supports the
+	  temperature sensor embedded in Silabs SY7636A chip.
+
 source "drivers/thermal/tegra/Kconfig"
 
 config GENERIC_ADC_THERMAL
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 82fc3e616e54..2e1aca8a0a09 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
 obj-y				+= intel/
 obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
 obj-y				+= st/
+obj-$(CONFIG_SY7636A_THERMAL)	+= sy7636a_thermal.o
 obj-$(CONFIG_QCOM_TSENS)	+= qcom/
 obj-y				+= tegra/
 obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
diff --git a/drivers/thermal/sy7636a_thermal.c b/drivers/thermal/sy7636a_thermal.c
new file mode 100644
index 000000000000..705a16fb1045
--- /dev/null
+++ b/drivers/thermal/sy7636a_thermal.c
@@ -0,0 +1,107 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Functions to access SY3686A power management chip temperature
+//
+// Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+//
+// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+//          Alistair Francis <alistair@alistair23.me>
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#include <linux/mfd/sy7636a.h>
+
+struct sy7636a_data {
+	struct sy7636a *sy7636a;
+	struct thermal_zone_device *thermal_zone_dev;
+};
+
+static int sy7636a_get_temp(void *arg, int *res)
+{
+	unsigned int reg_val, mode_ctr;
+	int ret;
+	struct sy7636a_data *data = arg;
+	bool isVoltageActive;
+
+	ret = regmap_read(data->sy7636a->regmap,
+			SY7636A_REG_OPERATION_MODE_CRL, &mode_ctr);
+	if (ret)
+		return ret;
+
+	isVoltageActive = mode_ctr & SY7636A_OPERATION_MODE_CRL_ONOFF;
+
+	if (!isVoltageActive) {
+		ret = regmap_write(data->sy7636a->regmap,
+				SY7636A_REG_OPERATION_MODE_CRL,
+				mode_ctr | SY7636A_OPERATION_MODE_CRL_ONOFF);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(data->sy7636a->regmap,
+			SY7636A_REG_TERMISTOR_READOUT, &reg_val);
+	if (ret)
+		return ret;
+
+	if (!isVoltageActive) {
+		ret = regmap_write(data->sy7636a->regmap,
+				SY7636A_REG_OPERATION_MODE_CRL,
+				mode_ctr);
+		if (ret)
+			return ret;
+	}
+
+	*res = *((signed char*)&reg_val);
+	*res *= 1000;
+
+	return ret;
+}
+
+static const struct thermal_zone_of_device_ops ops = {
+	.get_temp	= sy7636a_get_temp,
+};
+
+static int sy7636a_thermal_probe(struct platform_device *pdev)
+{
+	struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
+	struct sy7636a_data *data;
+
+	if (!sy7636a)
+		return -EPROBE_DEFER;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, data);
+
+	data->sy7636a = sy7636a;
+	data->thermal_zone_dev = devm_thermal_zone_of_sensor_register(
+			pdev->dev.parent,
+			0,
+			data,
+			&ops);
+
+	return PTR_ERR_OR_ZERO(data->thermal_zone_dev);
+}
+
+static const struct platform_device_id sy7636a_thermal_id_table[] = {
+	{ "sy7636a-thermal", },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, sy7636a_thermal_id_table);
+
+static struct platform_driver sy7636a_thermal_driver = {
+	.driver = {
+		.name = "sy7636a-thermal",
+	},
+	.probe = sy7636a_thermal_probe,
+	.id_table = sy7636a_thermal_id_table,
+};
+module_platform_driver(sy7636a_thermal_driver);
+
+MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
+MODULE_DESCRIPTION("SY7636A thermal driver");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1

[PATCH v7 3/6] hwmon: sy7636a: Add temperature driver for sy7636a

[Alistair Francis]

This is a multi-function device to interface with the sy7636a
EPD PMIC chip from Silergy.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/configs/imx_v6_v7_defconfig |   1 +
 drivers/hwmon/Kconfig                |  10 +++
 drivers/hwmon/Makefile               |   1 +
 drivers/hwmon/sy7636a-hwmon.c        | 106 +++++++++++++++++++++++++++
 4 files changed, 118 insertions(+)
 create mode 100644 drivers/hwmon/sy7636a-hwmon.c

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index cd80e85d37cf..e9c0be5629c6 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -227,6 +227,7 @@ CONFIG_RN5T618_POWER=m
 CONFIG_SENSORS_MC13783_ADC=y
 CONFIG_SENSORS_GPIO_FAN=y
 CONFIG_SENSORS_IIO_HWMON=y
+CONFIG_SENSORS_SY7636A=y
 CONFIG_THERMAL_STATISTICS=y
 CONFIG_THERMAL_WRITABLE_TRIPS=y
 CONFIG_CPU_THERMAL=y
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e3675377bc5d..6cae12de59cd 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1631,6 +1631,16 @@ config SENSORS_SIS5595
 	  This driver can also be built as a module. If so, the module
 	  will be called sis5595.
 
+config SENSORS_SY7636A
+	tristate "Silergy SY7636A"
+	depends on I2C
+	help
+	  If you say yes here you get support for the thermistor readout of
+	  the Silergy SY7636A PMIC.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sy7636a-hwmon.
+
 config SENSORS_DME1737
 	tristate "SMSC DME1737, SCH311x and compatibles"
 	depends on I2C && !PPC
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index d712c61c1f5e..8b2e09e25b24 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -180,6 +180,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
 obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
 obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
 obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
+obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
 obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
 obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
 obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
new file mode 100644
index 000000000000..4edbee99b693
--- /dev/null
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -0,0 +1,106 @@
+/*
+ * Functions to access SY3686A power management chip temperature
+ *
+ * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
+ *
+ * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/sy7636a.h>
+
+struct sy7636a_data {
+	struct sy7636a *sy7636a;
+	struct device *hwmon_dev;
+};
+
+static ssize_t show_temp(struct device *dev,
+	struct device_attribute *attr, char *buf)
+{
+	unsigned int reg_val;
+	signed char temp;
+	int ret;
+	struct sy7636a_data *data = dev_get_drvdata(dev);
+
+	ret = regmap_read(data->sy7636a->regmap,
+			SY7636A_REG_TERMISTOR_READOUT, &reg_val);
+	if (ret)
+		return ret;
+
+	temp = *((signed char*)&reg_val);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
+}
+
+static SENSOR_DEVICE_ATTR(temp0, S_IRUGO, show_temp, NULL, 0);
+
+static struct attribute *sy7636a_attrs[] = {
+	&sensor_dev_attr_temp0.dev_attr.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(sy7636a);
+
+static int sy7636a_sensor_probe(struct platform_device *pdev)
+{
+	struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
+	struct sy7636a_data *data;
+	int err;
+
+	if (!sy7636a)
+		return -EPROBE_DEFER;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
+	if (!data) {
+		return -ENOMEM;
+	}
+
+	data->sy7636a = sy7636a;
+	data->hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+			"sy7636a_temperature", data, sy7636a_groups);
+	if (IS_ERR(data->hwmon_dev)) {
+		err = PTR_ERR(data->hwmon_dev);
+		dev_err(&pdev->dev, "Unable to register hwmon device, returned %d", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id sy7636a_sensor_id[] = {
+	{ "sy7636a-temperature", 0},
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, sy7636a_sensor_id);
+
+static struct platform_driver sy7636a_sensor_driver = {
+	.probe = sy7636a_sensor_probe,
+	.id_table = sy7636a_sensor_id,
+	.driver = {
+		.name = "sy7636a-temperature",
+	},
+};
+module_platform_driver(sy7636a_sensor_driver);
+
+MODULE_DESCRIPTION("SY7636A sensor driver");
+MODULE_LICENSE("GPL");
+
-- 
2.31.1

[PATCH v7 4/6] ARM: imx_v6_v7_defconfig: Enable silergy,sy7636a

[Alistair Francis]

Enable the silergy,sy7636a and silergy,sy7636a-regulator for the
reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/configs/imx_v6_v7_defconfig | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index e9c0be5629c6..51b04d71777f 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -232,6 +232,7 @@ CONFIG_THERMAL_STATISTICS=y
 CONFIG_THERMAL_WRITABLE_TRIPS=y
 CONFIG_CPU_THERMAL=y
 CONFIG_IMX_THERMAL=y
+CONFIG_SY7636A_THERMAL=y
 CONFIG_WATCHDOG=y
 CONFIG_DA9062_WATCHDOG=y
 CONFIG_DA9063_WATCHDOG=m
@@ -245,7 +246,7 @@ CONFIG_MFD_MC13XXX_SPI=y
 CONFIG_MFD_MC13XXX_I2C=y
 CONFIG_MFD_RN5T618=y
 CONFIG_MFD_STMPE=y
-CONFIG_REGULATOR=y
+CONFIG_MFD_SY7636A=y
 CONFIG_REGULATOR_FIXED_VOLTAGE=y
 CONFIG_REGULATOR_ANATOP=y
 CONFIG_REGULATOR_DA9052=y
@@ -256,6 +257,7 @@ CONFIG_REGULATOR_MC13783=y
 CONFIG_REGULATOR_MC13892=y
 CONFIG_REGULATOR_PFUZE100=y
 CONFIG_REGULATOR_RN5T618=y
+CONFIG_REGULATOR_SY7636A=y
 CONFIG_RC_CORE=y
 CONFIG_RC_DEVICES=y
 CONFIG_IR_GPIO_CIR=y
-- 
2.31.1

[PATCH v7 5/6] ARM: dts: imx7d: remarkable2: Enable silergy,sy7636a

[Alistair Francis]

Enable the silergy,sy7636a and silergy,sy7636a-regulator on the
reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/boot/dts/imx7d-remarkable2.dts | 42 +++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index cc33b53ae6ba..9bdae1c1236e 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -86,6 +86,34 @@ wacom_digitizer: digitizer@9 {
 	};
 };
 
+&i2c4 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default", "sleep";
+	pinctrl-0 = <&pinctrl_i2c4>;
+	pinctrl-1 = <&pinctrl_i2c4>;
+	status = "okay";
+
+	epd_pmic: sy7636a@62 {
+		compatible = "silergy,sy7636a";
+		reg = <0x62>;
+		status = "okay";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_epdpmic>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		#thermal-sensor-cells = <0>;
+
+		epd-pwr-good-gpios = <&gpio6 21 GPIO_ACTIVE_HIGH>;
+		regulators {
+			compatible = "silergy,sy7636a-regulator";
+			reg_epdpmic: vcom {
+				regulator-name = "vcom";
+				regulator-boot-on;
+			};
+		};
+	};
+};
+
 &snvs_pwrkey {
 	status = "okay";
 };
@@ -179,6 +207,13 @@ MX7D_PAD_SAI1_TX_BCLK__GPIO6_IO13	0x14
 		>;
 	};
 
+	pinctrl_epdpmic: epdpmicgrp {
+		fsl,pins = <
+			MX7D_PAD_SAI2_RX_DATA__GPIO6_IO21 0x00000074
+			MX7D_PAD_ENET1_RGMII_TXC__GPIO7_IO11 0x00000014
+		>;
+	};
+
 	pinctrl_i2c1: i2c1grp {
 		fsl,pins = <
 			MX7D_PAD_I2C1_SDA__I2C1_SDA		0x4000007f
@@ -186,6 +221,13 @@ MX7D_PAD_I2C1_SCL__I2C1_SCL		0x4000007f
 		>;
 	};
 
+	pinctrl_i2c4: i2c4grp {
+		fsl,pins = <
+			MX7D_PAD_I2C4_SDA__I2C4_SDA		0x4000007f
+			MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
+		>;
+	};
+
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
 			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
-- 
2.31.1

[PATCH v7 6/6] ARM: dts: imx7d: remarkable2: Enable lcdif

[Alistair Francis]

Connect the dispaly on the reMarkable2.

Signed-off-by: Alistair Francis <alistair@alistair23.me>
---
 arch/arm/boot/dts/imx7d-remarkable2.dts | 75 +++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/arch/arm/boot/dts/imx7d-remarkable2.dts b/arch/arm/boot/dts/imx7d-remarkable2.dts
index 9bdae1c1236e..12af892f4b15 100644
--- a/arch/arm/boot/dts/imx7d-remarkable2.dts
+++ b/arch/arm/boot/dts/imx7d-remarkable2.dts
@@ -47,6 +47,16 @@ reg_digitizer: regulator-digitizer {
 		startup-delay-us = <100000>; /* 100 ms */
 	};
 
+	reg_sdoe: regulator-sdoe {
+		compatible = "regulator-fixed";
+		regulator-name = "SDOE";
+		pinctrl-names = "default", "sleep";
+		pinctrl-0 = <&pinctrl_sdoe_reg>;
+		pinctrl-1 = <&pinctrl_sdoe_reg>;
+		gpio = <&gpio3 27 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	wifi_pwrseq: wifi_pwrseq {
 		compatible = "mmc-pwrseq-simple";
 		pinctrl-names = "default";
@@ -55,6 +65,16 @@ wifi_pwrseq: wifi_pwrseq {
 		clocks = <&clks IMX7D_CLKO2_ROOT_DIV>;
 		clock-names = "ext_clock";
 	};
+
+	panel {
+		compatible = "eink,vb3300-kca";
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&display_out>;
+			};
+		};
+	};
 };
 
 &clks {
@@ -114,6 +134,21 @@ reg_epdpmic: vcom {
 	};
 };
 
+&lcdif {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_lcdif>;
+	lcd-supply = <&reg_epdpmic>;
+	lcd2-supply = <&reg_sdoe>;
+	prevent-frying-pan;
+	status = "okay";
+
+	port {
+		display_out: endpoint {
+			remote-endpoint = <&panel_in>;
+		};
+	};
+};
+
 &snvs_pwrkey {
 	status = "okay";
 };
@@ -228,6 +263,46 @@ MX7D_PAD_I2C4_SCL__I2C4_SCL		0x4000007f
 		>;
 	};
 
+	pinctrl_lcdif: lcdifgrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_DATA00__LCD_DATA0		0x79
+			MX7D_PAD_LCD_DATA01__LCD_DATA1		0x79
+			MX7D_PAD_LCD_DATA02__LCD_DATA2		0x79
+			MX7D_PAD_LCD_DATA03__LCD_DATA3		0x79
+			MX7D_PAD_LCD_DATA04__LCD_DATA4		0x79
+			MX7D_PAD_LCD_DATA05__LCD_DATA5		0x79
+			MX7D_PAD_LCD_DATA06__LCD_DATA6		0x79
+			MX7D_PAD_LCD_DATA07__LCD_DATA7		0x79
+			MX7D_PAD_LCD_DATA08__LCD_DATA8		0x79
+			MX7D_PAD_LCD_DATA09__LCD_DATA9		0x79
+			MX7D_PAD_LCD_DATA10__LCD_DATA10		0x79
+			MX7D_PAD_LCD_DATA11__LCD_DATA11		0x79
+			MX7D_PAD_LCD_DATA12__LCD_DATA12		0x79
+			MX7D_PAD_LCD_DATA13__LCD_DATA13		0x79
+			MX7D_PAD_LCD_DATA14__LCD_DATA14		0x79
+			MX7D_PAD_LCD_DATA15__LCD_DATA15		0x79
+
+			MX7D_PAD_LCD_DATA17__LCD_DATA17		0x79
+			MX7D_PAD_LCD_DATA18__LCD_DATA18		0x79
+			MX7D_PAD_LCD_DATA19__LCD_DATA19		0x79
+			MX7D_PAD_LCD_DATA20__LCD_DATA20		0x79
+			MX7D_PAD_LCD_DATA21__LCD_DATA21		0x79
+
+			MX7D_PAD_LCD_DATA23__LCD_DATA23		0x79
+			MX7D_PAD_LCD_CLK__LCD_CLK		0x79
+			MX7D_PAD_LCD_ENABLE__LCD_ENABLE		0x79
+			MX7D_PAD_LCD_VSYNC__LCD_VSYNC		0x79
+			MX7D_PAD_LCD_HSYNC__LCD_HSYNC		0x79
+			MX7D_PAD_LCD_RESET__LCD_RESET		0x79
+		>;
+	};
+
+	pinctrl_sdoe_reg: sdoereggrp {
+		fsl,pins = <
+			MX7D_PAD_LCD_DATA22__GPIO3_IO27		0x74
+		>;
+	};
+
 	pinctrl_uart1: uart1grp {
 		fsl,pins = <
 			MX7D_PAD_UART1_TX_DATA__UART1_DCE_TX	0x79
-- 
2.31.1

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Lee Jones]

On Thu, 08 Jul 2021, Alistair Francis wrote:

> Initial support for the Silergy SY7636A Power Management chip.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  drivers/mfd/Kconfig         |  9 +++++
>  drivers/mfd/Makefile        |  1 +
>  drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
>  4 files changed, 138 insertions(+)
>  create mode 100644 drivers/mfd/sy7636a.c
>  create mode 100644 include/linux/mfd/sy7636a.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6a3fd2d75f96..7b59aa0fd3f2 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1352,6 +1352,15 @@ config MFD_SYSCON
>  	  Select this option to enable accessing system control registers
>  	  via regmap.
>  
> +config MFD_SY7636A
> +	tristate "Silergy SY7636A Power Management chip"

s/chip/IC/

> +	select MFD_CORE
> +	select REGMAP_I2C
> +	depends on I2C
> +	help
> +	  Select this option to enable support for the Silergy SY7636A
> +	  Power Management chip.

As above.

>  config MFD_DAVINCI_VOICECODEC
>  	tristate
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8116c19d5fd4..cbe581e87fa9 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  obj-$(CONFIG_MFD_ACER_A500_EC)	+= acer-ec-a500.o
>  obj-$(CONFIG_MFD_QCOM_PM8008)	+= qcom-pm8008.o
>  
> +obj-$(CONFIG_MFD_SY7636A)	+= sy7636a.o
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> new file mode 100644
> index 000000000000..345892e11221
> --- /dev/null
> +++ b/drivers/mfd/sy7636a.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// MFD parent driver for SY7636A chip
> +//
> +// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
> +//
> +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> +//          Alistair Francis <alistair@alistair23.me>

Only C++ comments for the SPDX please.

> +// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
> +
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#include <linux/mfd/sy7636a.h>
> +
> +static const struct regmap_config sy7636a_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static const struct mfd_cell sy7636a_cells[] = {
> +	{ .name = "sy7636a-regulator", },
> +	{ .name = "sy7636a-temperature", },
> +	{ .name = "sy7636a-thermal", },
> +};

If you put these in the Device Tree, you can use "simple-mfd-i2c"

> +static const struct of_device_id of_sy7636a_match_table[] = {
> +	{ .compatible = "silergy,sy7636a", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);

Place this near the i2c_device_id table please.

> +static int sy7636a_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *ids)
> +{
> +	struct sy7636a *sy7636a;

Please call this ddata.

> +	int ret;
> +
> +	sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);
> +	if (!sy7636a)
> +		return -ENOMEM;
> +
> +	sy7636a->dev = &client->dev;

What are you using 'dev' for?

You can normally just use 'dev->parent' from the child device.

> +	sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
> +	if (IS_ERR(sy7636a->regmap)) {
> +		ret = PTR_ERR(sy7636a->regmap);
> +		dev_err(sy7636a->dev,
> +			"Failed to initialize register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	i2c_set_clientdata(client, sy7636a);
> +
> +	return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
> +					sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
> +					NULL, 0, NULL);
> +}
> +
> +static const struct i2c_device_id sy7636a_id_table[] = {
> +	{ "sy7636a", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);

Use probe_new below, then you can omit this table.

> +static struct i2c_driver sy7636a_driver = {
> +	.driver	= {
> +		.name	= "sy7636a",
> +		.of_match_table = of_sy7636a_match_table,
> +	},
> +	.probe = sy7636a_probe,
> +	.id_table = sy7636a_id_table,
> +};
> +module_i2c_driver(sy7636a_driver);
> +
> +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
> +MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
> new file mode 100644
> index 000000000000..b6845a3572b8
> --- /dev/null
> +++ b/include/linux/mfd/sy7636a.h
> @@ -0,0 +1,47 @@
> +/* 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
> +
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +
> +#define SY7636A_REG_OPERATION_MODE_CRL		0x00
> +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL	BIT(6)
> +#define SY7636A_OPERATION_MODE_CRL_ONOFF	BIT(7)
> +#define SY7636A_REG_VCOM_ADJUST_CTRL_L		0x01
> +#define SY7636A_REG_VCOM_ADJUST_CTRL_H		0x02
> +#define SY7636A_REG_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_MIN		0
> +#define VCOM_MAX		5000
> +
> +#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
> +
> +struct sy7636a {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct gpio_desc *pgood_gpio;

This looks unused?

> +};
> +
> +#endif /* __LINUX_MFD_SY7636A_H */

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 3/6] hwmon: sy7636a: Add temperature driver for sy7636a

[Lee Jones]

On Thu, 08 Jul 2021, Alistair Francis wrote:

> This is a multi-function device to interface with the sy7636a
> EPD PMIC chip from Silergy.
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  arch/arm/configs/imx_v6_v7_defconfig |   1 +
>  drivers/hwmon/Kconfig                |  10 +++
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/sy7636a-hwmon.c        | 106 +++++++++++++++++++++++++++
>  4 files changed, 118 insertions(+)
>  create mode 100644 drivers/hwmon/sy7636a-hwmon.c
> 
> diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> index cd80e85d37cf..e9c0be5629c6 100644
> --- a/arch/arm/configs/imx_v6_v7_defconfig
> +++ b/arch/arm/configs/imx_v6_v7_defconfig
> @@ -227,6 +227,7 @@ CONFIG_RN5T618_POWER=m
>  CONFIG_SENSORS_MC13783_ADC=y
>  CONFIG_SENSORS_GPIO_FAN=y
>  CONFIG_SENSORS_IIO_HWMON=y
> +CONFIG_SENSORS_SY7636A=y
>  CONFIG_THERMAL_STATISTICS=y
>  CONFIG_THERMAL_WRITABLE_TRIPS=y
>  CONFIG_CPU_THERMAL=y
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e3675377bc5d..6cae12de59cd 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1631,6 +1631,16 @@ config SENSORS_SIS5595
>  	  This driver can also be built as a module. If so, the module
>  	  will be called sis5595.
>  
> +config SENSORS_SY7636A
> +	tristate "Silergy SY7636A"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the thermistor readout of
> +	  the Silergy SY7636A PMIC.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called sy7636a-hwmon.
> +
>  config SENSORS_DME1737
>  	tristate "SMSC DME1737, SCH311x and compatibles"
>  	depends on I2C && !PPC
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index d712c61c1f5e..8b2e09e25b24 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -180,6 +180,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)	+= smsc47m1.o
>  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
>  obj-$(CONFIG_SENSORS_SPARX5)	+= sparx5-temp.o
>  obj-$(CONFIG_SENSORS_STTS751)	+= stts751.o
> +obj-$(CONFIG_SENSORS_SY7636A)	+= sy7636a-hwmon.o
>  obj-$(CONFIG_SENSORS_AMC6821)	+= amc6821.o
>  obj-$(CONFIG_SENSORS_TC74)	+= tc74.o
>  obj-$(CONFIG_SENSORS_THMC50)	+= thmc50.o
> diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> new file mode 100644
> index 000000000000..4edbee99b693
> --- /dev/null
> +++ b/drivers/hwmon/sy7636a-hwmon.c
> @@ -0,0 +1,106 @@
> +/*
> + * Functions to access SY3686A power management chip temperature
> + *
> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> + *
> + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

The long form isn't usually accepted anymore.

Please replace with SPDX.

> + */
> +
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/mfd/sy7636a.h>
> +
> +struct sy7636a_data {
> +	struct sy7636a *sy7636a;
> +	struct device *hwmon_dev;
> +};
> +
> +static ssize_t show_temp(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	unsigned int reg_val;
> +	signed char temp;
> +	int ret;
> +	struct sy7636a_data *data = dev_get_drvdata(dev);
> +
> +	ret = regmap_read(data->sy7636a->regmap,
> +			SY7636A_REG_TERMISTOR_READOUT, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	temp = *((signed char*)&reg_val);

Whoa!  What's going on here?

You also need to run checkpatch.pl.

> +	return snprintf(buf, PAGE_SIZE, "%d\n", temp);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp0, S_IRUGO, show_temp, NULL, 0);
> +
> +static struct attribute *sy7636a_attrs[] = {
> +	&sensor_dev_attr_temp0.dev_attr.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(sy7636a);
> +
> +static int sy7636a_sensor_probe(struct platform_device *pdev)
> +{
> +	struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
> +	struct sy7636a_data *data;
> +	int err;
> +
> +	if (!sy7636a)
> +		return -EPROBE_DEFER;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);

Where is this used, outside of this function?

Not sure I see a good reason for having it around?

> +	if (!data) {
> +		return -ENOMEM;
> +	}
> +
> +	data->sy7636a = sy7636a;
> +	data->hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,

Why is this being stored into a struct?

> +			"sy7636a_temperature", data, sy7636a_groups);
> +	if (IS_ERR(data->hwmon_dev)) {
> +		err = PTR_ERR(data->hwmon_dev);
> +		dev_err(&pdev->dev, "Unable to register hwmon device, returned %d", err);
> +		return err;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id sy7636a_sensor_id[] = {
> +	{ "sy7636a-temperature", 0},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(platform, sy7636a_sensor_id);
> +
> +static struct platform_driver sy7636a_sensor_driver = {
> +	.probe = sy7636a_sensor_probe,
> +	.id_table = sy7636a_sensor_id,

What does this do?

Where is the 'device' being registered?

> +	.driver = {
> +		.name = "sy7636a-temperature",
> +	},
> +};
> +module_platform_driver(sy7636a_sensor_driver);
> +
> +MODULE_DESCRIPTION("SY7636A sensor driver");
> +MODULE_LICENSE("GPL");
> +

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 2/6] thermal: sy7636a: Add thermal driver for sy7636a

[Lee Jones]

On Thu, 08 Jul 2021, Alistair Francis wrote:

> From: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.no>
> 
> Add thermal driver to enable kernel based polling
> and shutdown of device for temperatures out of spec
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  drivers/thermal/Kconfig           |   7 ++
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/sy7636a_thermal.c | 107 ++++++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+)
>  create mode 100644 drivers/thermal/sy7636a_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index d7f44deab5b1..7112c63d9021 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -450,6 +450,13 @@ depends on (ARCH_STI || ARCH_STM32) && OF
>  source "drivers/thermal/st/Kconfig"
>  endmenu
>  
> +config SY7636A_THERMAL
> +	tristate "SY7636A thermal management"
> +	depends on MFD_SY7636A
> +	help
> +	  Enable the sy7636a thermal driver, which supports the
> +	  temperature sensor embedded in Silabs SY7636A chip.
> +
>  source "drivers/thermal/tegra/Kconfig"
>  
>  config GENERIC_ADC_THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 82fc3e616e54..2e1aca8a0a09 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
>  obj-y				+= intel/
>  obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>  obj-y				+= st/
> +obj-$(CONFIG_SY7636A_THERMAL)	+= sy7636a_thermal.o
>  obj-$(CONFIG_QCOM_TSENS)	+= qcom/
>  obj-y				+= tegra/
>  obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
> diff --git a/drivers/thermal/sy7636a_thermal.c b/drivers/thermal/sy7636a_thermal.c
> new file mode 100644
> index 000000000000..705a16fb1045
> --- /dev/null
> +++ b/drivers/thermal/sy7636a_thermal.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Functions to access SY3686A power management chip temperature
> +//
> +// Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> +//
> +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> +//          Alistair Francis <alistair@alistair23.me>
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#include <linux/mfd/sy7636a.h>
> +
> +struct sy7636a_data {
> +	struct sy7636a *sy7636a;
> +	struct thermal_zone_device *thermal_zone_dev;
> +};
> +
> +static int sy7636a_get_temp(void *arg, int *res)
> +{
> +	unsigned int reg_val, mode_ctr;
> +	int ret;
> +	struct sy7636a_data *data = arg;
> +	bool isVoltageActive;
> +
> +	ret = regmap_read(data->sy7636a->regmap,
> +			SY7636A_REG_OPERATION_MODE_CRL, &mode_ctr);
> +	if (ret)
> +		return ret;
> +
> +	isVoltageActive = mode_ctr & SY7636A_OPERATION_MODE_CRL_ONOFF;
> +
> +	if (!isVoltageActive) {
> +		ret = regmap_write(data->sy7636a->regmap,
> +				SY7636A_REG_OPERATION_MODE_CRL,
> +				mode_ctr | SY7636A_OPERATION_MODE_CRL_ONOFF);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(data->sy7636a->regmap,
> +			SY7636A_REG_TERMISTOR_READOUT, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (!isVoltageActive) {
> +		ret = regmap_write(data->sy7636a->regmap,
> +				SY7636A_REG_OPERATION_MODE_CRL,
> +				mode_ctr);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*res = *((signed char*)&reg_val);
> +	*res *= 1000;
> +
> +	return ret;
> +}
> +
> +static const struct thermal_zone_of_device_ops ops = {
> +	.get_temp	= sy7636a_get_temp,
> +};
> +
> +static int sy7636a_thermal_probe(struct platform_device *pdev)
> +{
> +	struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
> +	struct sy7636a_data *data;
> +
> +	if (!sy7636a)
> +		return -EPROBE_DEFER;

How is this possible?

> +	data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);

sizeof(*data)

> +	if (!data)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	data->sy7636a = sy7636a;
> +	data->thermal_zone_dev = devm_thermal_zone_of_sensor_register(
> +			pdev->dev.parent,
> +			0,
> +			data,

Why don't you just pass in your initial ddata?

> +			&ops);
> +
> +	return PTR_ERR_OR_ZERO(data->thermal_zone_dev);
> +}
> +
> +static const struct platform_device_id sy7636a_thermal_id_table[] = {
> +	{ "sy7636a-thermal", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, sy7636a_thermal_id_table);
> +
> +static struct platform_driver sy7636a_thermal_driver = {
> +	.driver = {
> +		.name = "sy7636a-thermal",
> +	},
> +	.probe = sy7636a_thermal_probe,
> +	.id_table = sy7636a_thermal_id_table,
> +};
> +module_platform_driver(sy7636a_thermal_driver);
> +
> +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
> +MODULE_DESCRIPTION("SY7636A thermal driver");
> +MODULE_LICENSE("GPL v2");

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 544 bytes --]

On Tue, Jul 20, 2021 at 03:53:25PM +0100, Lee Jones wrote:
> On Thu, 08 Jul 2021, Alistair Francis wrote:

> > +static const struct mfd_cell sy7636a_cells[] = {
> > +	{ .name = "sy7636a-regulator", },
> > +	{ .name = "sy7636a-temperature", },
> > +	{ .name = "sy7636a-thermal", },
> > +};

> If you put these in the Device Tree, you can use "simple-mfd-i2c"

At least the regulator probably shouldn't be - this is just a Linux
specific grouping of devices, it's not really directly a block in the
hardware in a way that's platform independent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Lee Jones]

On Tue, 20 Jul 2021, Mark Brown wrote:

> On Tue, Jul 20, 2021 at 03:53:25PM +0100, Lee Jones wrote:
> > On Thu, 08 Jul 2021, Alistair Francis wrote:
> 
> > > +static const struct mfd_cell sy7636a_cells[] = {
> > > +	{ .name = "sy7636a-regulator", },
> > > +	{ .name = "sy7636a-temperature", },
> > > +	{ .name = "sy7636a-thermal", },
> > > +};
> 
> > If you put these in the Device Tree, you can use "simple-mfd-i2c"
> 
> At least the regulator probably shouldn't be - this is just a Linux
> specific grouping of devices, it's not really directly a block in the
> hardware in a way that's platform independent.

I've seen (and authored) regulator support in DT before.

What's changed?  They're controlled by registers, right?

Is the problem that the registers are usually split?

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]

On Tue, Jul 20, 2021 at 05:09:02PM +0100, Lee Jones wrote:
> On Tue, 20 Jul 2021, Mark Brown wrote:

> > At least the regulator probably shouldn't be - this is just a Linux
> > specific grouping of devices, it's not really directly a block in the
> > hardware in a way that's platform independent.

> I've seen (and authored) regulator support in DT before.

> What's changed?  They're controlled by registers, right?

Nothing's changed, I routinely push back on regulator drivers that have
a compatible string for a MFD subfunction like this.  I do miss them
sometimes but try not to.

> Is the problem that the registers are usually split?

It's just not really describing the hardware, it's encoding the way
Linux splits things up into the DT that adds no descriptive information.
We're not getting any information about where the IPs are in the device
or anything from the compatible, and typically it's describing a set of
disjoint IPs with minimal overlap in their configuration.  If it's a
binding for something like an individual LDO or DCDC and we've got
multiple instances of that within a single chip then it starts to get
more useful but that's not what something like this is doing.  We're not
gaining anything by putting a compatible string in there, all it does is
make the DT bigger and add some ABI.

Similar issues exist with CODEC subfunctions - those are usually
describing huge piles of different IPs but we happen to want to pull
them together for Linux, typically including some clocking which if we
were going down to the level of describing components of the MFD in the
DT should be being described using their own bindings.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 2/6] thermal: sy7636a: Add thermal driver for sy7636a

[Alistair Francis]

On Wed, Jul 21, 2021 at 1:02 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 08 Jul 2021, Alistair Francis wrote:
>
> > From: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.no>
> >
> > Add thermal driver to enable kernel based polling
> > and shutdown of device for temperatures out of spec
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > ---
> >  drivers/thermal/Kconfig           |   7 ++
> >  drivers/thermal/Makefile          |   1 +
> >  drivers/thermal/sy7636a_thermal.c | 107 ++++++++++++++++++++++++++++++
> >  3 files changed, 115 insertions(+)
> >  create mode 100644 drivers/thermal/sy7636a_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index d7f44deab5b1..7112c63d9021 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -450,6 +450,13 @@ depends on (ARCH_STI || ARCH_STM32) && OF
> >  source "drivers/thermal/st/Kconfig"
> >  endmenu
> >
> > +config SY7636A_THERMAL
> > +     tristate "SY7636A thermal management"
> > +     depends on MFD_SY7636A
> > +     help
> > +       Enable the sy7636a thermal driver, which supports the
> > +       temperature sensor embedded in Silabs SY7636A chip.
> > +
> >  source "drivers/thermal/tegra/Kconfig"
> >
> >  config GENERIC_ADC_THERMAL
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 82fc3e616e54..2e1aca8a0a09 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_DA9062_THERMAL)        += da9062-thermal.o
> >  obj-y                                += intel/
> >  obj-$(CONFIG_TI_SOC_THERMAL) += ti-soc-thermal/
> >  obj-y                                += st/
> > +obj-$(CONFIG_SY7636A_THERMAL)        += sy7636a_thermal.o
> >  obj-$(CONFIG_QCOM_TSENS)     += qcom/
> >  obj-y                                += tegra/
> >  obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
> > diff --git a/drivers/thermal/sy7636a_thermal.c b/drivers/thermal/sy7636a_thermal.c
> > new file mode 100644
> > index 000000000000..705a16fb1045
> > --- /dev/null
> > +++ b/drivers/thermal/sy7636a_thermal.c
> > @@ -0,0 +1,107 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// Functions to access SY3686A power management chip temperature
> > +//
> > +// Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > +//
> > +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> > +//          Alistair Francis <alistair@alistair23.me>
> > +
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/thermal.h>
> > +
> > +#include <linux/mfd/sy7636a.h>
> > +
> > +struct sy7636a_data {
> > +     struct sy7636a *sy7636a;
> > +     struct thermal_zone_device *thermal_zone_dev;
> > +};
> > +
> > +static int sy7636a_get_temp(void *arg, int *res)
> > +{
> > +     unsigned int reg_val, mode_ctr;
> > +     int ret;
> > +     struct sy7636a_data *data = arg;
> > +     bool isVoltageActive;
> > +
> > +     ret = regmap_read(data->sy7636a->regmap,
> > +                     SY7636A_REG_OPERATION_MODE_CRL, &mode_ctr);
> > +     if (ret)
> > +             return ret;
> > +
> > +     isVoltageActive = mode_ctr & SY7636A_OPERATION_MODE_CRL_ONOFF;
> > +
> > +     if (!isVoltageActive) {
> > +             ret = regmap_write(data->sy7636a->regmap,
> > +                             SY7636A_REG_OPERATION_MODE_CRL,
> > +                             mode_ctr | SY7636A_OPERATION_MODE_CRL_ONOFF);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     ret = regmap_read(data->sy7636a->regmap,
> > +                     SY7636A_REG_TERMISTOR_READOUT, &reg_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (!isVoltageActive) {
> > +             ret = regmap_write(data->sy7636a->regmap,
> > +                             SY7636A_REG_OPERATION_MODE_CRL,
> > +                             mode_ctr);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     *res = *((signed char*)&reg_val);
> > +     *res *= 1000;
> > +
> > +     return ret;
> > +}
> > +
> > +static const struct thermal_zone_of_device_ops ops = {
> > +     .get_temp       = sy7636a_get_temp,
> > +};
> > +
> > +static int sy7636a_thermal_probe(struct platform_device *pdev)
> > +{
> > +     struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
> > +     struct sy7636a_data *data;
> > +
> > +     if (!sy7636a)
> > +             return -EPROBE_DEFER;
>
> How is this possible?

I'm not sure, I have removed this.

>
> > +     data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
>
> sizeof(*data)
>
> > +     if (!data)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, data);
> > +
> > +     data->sy7636a = sy7636a;
> > +     data->thermal_zone_dev = devm_thermal_zone_of_sensor_register(
> > +                     pdev->dev.parent,
> > +                     0,
> > +                     data,
>
> Why don't you just pass in your initial ddata?

I'm not sure what you mean, which data?

Alistair

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Alistair Francis]

On Wed, Jul 21, 2021 at 6:26 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Jul 20, 2021 at 05:09:02PM +0100, Lee Jones wrote:
> > On Tue, 20 Jul 2021, Mark Brown wrote:
>
> > > At least the regulator probably shouldn't be - this is just a Linux
> > > specific grouping of devices, it's not really directly a block in the
> > > hardware in a way that's platform independent.
>
> > I've seen (and authored) regulator support in DT before.
>
> > What's changed?  They're controlled by registers, right?
>
> Nothing's changed, I routinely push back on regulator drivers that have
> a compatible string for a MFD subfunction like this.  I do miss them
> sometimes but try not to.

Sorry, I just want to clarify what I should do.

Are you saying that I shouldn't add the regulator to the device tree?
Should I leave it as part of `sy7636a_cells` then?

Alistair

>
> > Is the problem that the registers are usually split?
>
> It's just not really describing the hardware, it's encoding the way
> Linux splits things up into the DT that adds no descriptive information.
> We're not getting any information about where the IPs are in the device
> or anything from the compatible, and typically it's describing a set of
> disjoint IPs with minimal overlap in their configuration.  If it's a
> binding for something like an individual LDO or DCDC and we've got
> multiple instances of that within a single chip then it starts to get
> more useful but that's not what something like this is doing.  We're not
> gaining anything by putting a compatible string in there, all it does is
> make the DT bigger and add some ABI.
>
> Similar issues exist with CODEC subfunctions - those are usually
> describing huge piles of different IPs but we happen to want to pull
> them together for Linux, typically including some clocking which if we
> were going down to the level of describing components of the MFD in the
> DT should be being described using their own bindings.

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

On Fri, Jul 30, 2021 at 04:21:18PM +1000, Alistair Francis wrote:

> Are you saying that I shouldn't add the regulator to the device tree?
> Should I leave it as part of `sy7636a_cells` then?

Yes, that'd be better.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 2/6] thermal: sy7636a: Add thermal driver for sy7636a

[Lee Jones]

On Wed, 28 Jul 2021, Alistair Francis wrote:

> On Wed, Jul 21, 2021 at 1:02 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 08 Jul 2021, Alistair Francis wrote:
> >
> > > From: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.no>
> > >
> > > Add thermal driver to enable kernel based polling
> > > and shutdown of device for temperatures out of spec
> > >
> > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > ---
> > >  drivers/thermal/Kconfig           |   7 ++
> > >  drivers/thermal/Makefile          |   1 +
> > >  drivers/thermal/sy7636a_thermal.c | 107 ++++++++++++++++++++++++++++++
> > >  3 files changed, 115 insertions(+)
> > >  create mode 100644 drivers/thermal/sy7636a_thermal.c
> > >
> > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > > index d7f44deab5b1..7112c63d9021 100644
> > > --- a/drivers/thermal/Kconfig
> > > +++ b/drivers/thermal/Kconfig
> > > @@ -450,6 +450,13 @@ depends on (ARCH_STI || ARCH_STM32) && OF
> > >  source "drivers/thermal/st/Kconfig"
> > >  endmenu
> > >
> > > +config SY7636A_THERMAL
> > > +     tristate "SY7636A thermal management"
> > > +     depends on MFD_SY7636A
> > > +     help
> > > +       Enable the sy7636a thermal driver, which supports the
> > > +       temperature sensor embedded in Silabs SY7636A chip.
> > > +
> > >  source "drivers/thermal/tegra/Kconfig"
> > >
> > >  config GENERIC_ADC_THERMAL
> > > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > > index 82fc3e616e54..2e1aca8a0a09 100644
> > > --- a/drivers/thermal/Makefile
> > > +++ b/drivers/thermal/Makefile
> > > @@ -51,6 +51,7 @@ obj-$(CONFIG_DA9062_THERMAL)        += da9062-thermal.o
> > >  obj-y                                += intel/
> > >  obj-$(CONFIG_TI_SOC_THERMAL) += ti-soc-thermal/
> > >  obj-y                                += st/
> > > +obj-$(CONFIG_SY7636A_THERMAL)        += sy7636a_thermal.o
> > >  obj-$(CONFIG_QCOM_TSENS)     += qcom/
> > >  obj-y                                += tegra/
> > >  obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
> > > diff --git a/drivers/thermal/sy7636a_thermal.c b/drivers/thermal/sy7636a_thermal.c
> > > new file mode 100644
> > > index 000000000000..705a16fb1045
> > > --- /dev/null
> > > +++ b/drivers/thermal/sy7636a_thermal.c
> > > @@ -0,0 +1,107 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +//
> > > +// Functions to access SY3686A power management chip temperature
> > > +//
> > > +// Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > > +//
> > > +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> > > +//          Alistair Francis <alistair@alistair23.me>
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/thermal.h>
> > > +
> > > +#include <linux/mfd/sy7636a.h>
> > > +
> > > +struct sy7636a_data {
> > > +     struct sy7636a *sy7636a;
> > > +     struct thermal_zone_device *thermal_zone_dev;
> > > +};
> > > +
> > > +static int sy7636a_get_temp(void *arg, int *res)
> > > +{
> > > +     unsigned int reg_val, mode_ctr;
> > > +     int ret;
> > > +     struct sy7636a_data *data = arg;
> > > +     bool isVoltageActive;
> > > +
> > > +     ret = regmap_read(data->sy7636a->regmap,
> > > +                     SY7636A_REG_OPERATION_MODE_CRL, &mode_ctr);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     isVoltageActive = mode_ctr & SY7636A_OPERATION_MODE_CRL_ONOFF;
> > > +
> > > +     if (!isVoltageActive) {
> > > +             ret = regmap_write(data->sy7636a->regmap,
> > > +                             SY7636A_REG_OPERATION_MODE_CRL,
> > > +                             mode_ctr | SY7636A_OPERATION_MODE_CRL_ONOFF);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     ret = regmap_read(data->sy7636a->regmap,
> > > +                     SY7636A_REG_TERMISTOR_READOUT, &reg_val);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     if (!isVoltageActive) {
> > > +             ret = regmap_write(data->sy7636a->regmap,
> > > +                             SY7636A_REG_OPERATION_MODE_CRL,
> > > +                             mode_ctr);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     *res = *((signed char*)&reg_val);
> > > +     *res *= 1000;
> > > +
> > > +     return ret;
> > > +}
> > > +
> > > +static const struct thermal_zone_of_device_ops ops = {
> > > +     .get_temp       = sy7636a_get_temp,
> > > +};
> > > +
> > > +static int sy7636a_thermal_probe(struct platform_device *pdev)
> > > +{
> > > +     struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
> > > +     struct sy7636a_data *data;
> > > +
> > > +     if (!sy7636a)
> > > +             return -EPROBE_DEFER;
> >
> > How is this possible?
> 
> I'm not sure, I have removed this.
> 
> >
> > > +     data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
> >
> > sizeof(*data)
> >
> > > +     if (!data)
> > > +             return -ENOMEM;
> > > +
> > > +     platform_set_drvdata(pdev, data);
> > > +
> > > +     data->sy7636a = sy7636a;
> > > +     data->thermal_zone_dev = devm_thermal_zone_of_sensor_register(
> > > +                     pdev->dev.parent,
> > > +                     0,
> > > +                     data,
> >
> > Why don't you just pass in your initial ddata?
> 
> I'm not sure what you mean, which data?

sy7636a

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Alistair Francis]

On Wed, Jul 21, 2021 at 12:53 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 08 Jul 2021, Alistair Francis wrote:
>
> > Initial support for the Silergy SY7636A Power Management chip.
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > ---
> >  drivers/mfd/Kconfig         |  9 +++++
> >  drivers/mfd/Makefile        |  1 +
> >  drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
> >  4 files changed, 138 insertions(+)
> >  create mode 100644 drivers/mfd/sy7636a.c
> >  create mode 100644 include/linux/mfd/sy7636a.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 6a3fd2d75f96..7b59aa0fd3f2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1352,6 +1352,15 @@ config MFD_SYSCON
> >         Select this option to enable accessing system control registers
> >         via regmap.
> >
> > +config MFD_SY7636A
> > +     tristate "Silergy SY7636A Power Management chip"
>
> s/chip/IC/
>
> > +     select MFD_CORE
> > +     select REGMAP_I2C
> > +     depends on I2C
> > +     help
> > +       Select this option to enable support for the Silergy SY7636A
> > +       Power Management chip.
>
> As above.
>
> >  config MFD_DAVINCI_VOICECODEC
> >       tristate
> >       select MFD_CORE
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 8116c19d5fd4..cbe581e87fa9 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -266,6 +266,7 @@ obj-$(CONFIG_MFD_KHADAS_MCU)      += khadas-mcu.o
> >  obj-$(CONFIG_MFD_ACER_A500_EC)       += acer-ec-a500.o
> >  obj-$(CONFIG_MFD_QCOM_PM8008)        += qcom-pm8008.o
> >
> > +obj-$(CONFIG_MFD_SY7636A)    += sy7636a.o
> >  obj-$(CONFIG_SGI_MFD_IOC3)   += ioc3.o
> >  obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)     += simple-mfd-i2c.o
> >  obj-$(CONFIG_MFD_INTEL_M10_BMC)   += intel-m10-bmc.o
> > diff --git a/drivers/mfd/sy7636a.c b/drivers/mfd/sy7636a.c
> > new file mode 100644
> > index 000000000000..345892e11221
> > --- /dev/null
> > +++ b/drivers/mfd/sy7636a.c
> > @@ -0,0 +1,81 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +//
> > +// MFD parent driver for SY7636A chip
> > +//
> > +// Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
> > +//
> > +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> > +//          Alistair Francis <alistair@alistair23.me>
>
> Only C++ comments for the SPDX please.
>
> > +// Based on the lp87565 driver by Keerthy <j-keerthy@ti.com>
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +
> > +#include <linux/mfd/sy7636a.h>
> > +
> > +static const struct regmap_config sy7636a_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +};
> > +
> > +static const struct mfd_cell sy7636a_cells[] = {
> > +     { .name = "sy7636a-regulator", },
> > +     { .name = "sy7636a-temperature", },
> > +     { .name = "sy7636a-thermal", },
> > +};
>
> If you put these in the Device Tree, you can use "simple-mfd-i2c"
>
> > +static const struct of_device_id of_sy7636a_match_table[] = {
> > +     { .compatible = "silergy,sy7636a", },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, of_sy7636a_match_table);
>
> Place this near the i2c_device_id table please.
>
> > +static int sy7636a_probe(struct i2c_client *client,
> > +                      const struct i2c_device_id *ids)
> > +{
> > +     struct sy7636a *sy7636a;
>
> Please call this ddata.
>
> > +     int ret;
> > +
> > +     sy7636a = devm_kzalloc(&client->dev, sizeof(*sy7636a), GFP_KERNEL);
> > +     if (!sy7636a)
> > +             return -ENOMEM;
> > +
> > +     sy7636a->dev = &client->dev;
>
> What are you using 'dev' for?
>
> You can normally just use 'dev->parent' from the child device.

I didn't realise that, I have removed `dev`.

>
> > +     sy7636a->regmap = devm_regmap_init_i2c(client, &sy7636a_regmap_config);
> > +     if (IS_ERR(sy7636a->regmap)) {
> > +             ret = PTR_ERR(sy7636a->regmap);
> > +             dev_err(sy7636a->dev,
> > +                     "Failed to initialize register map: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     i2c_set_clientdata(client, sy7636a);
> > +
> > +     return devm_mfd_add_devices(sy7636a->dev, PLATFORM_DEVID_AUTO,
> > +                                     sy7636a_cells, ARRAY_SIZE(sy7636a_cells),
> > +                                     NULL, 0, NULL);
> > +}
> > +
> > +static const struct i2c_device_id sy7636a_id_table[] = {
> > +     { "sy7636a", 0 },
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sy7636a_id_table);
>
> Use probe_new below, then you can omit this table.
>
> > +static struct i2c_driver sy7636a_driver = {
> > +     .driver = {
> > +             .name   = "sy7636a",
> > +             .of_match_table = of_sy7636a_match_table,
> > +     },
> > +     .probe = sy7636a_probe,
> > +     .id_table = sy7636a_id_table,
> > +};
> > +module_i2c_driver(sy7636a_driver);
> > +
> > +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
> > +MODULE_DESCRIPTION("Silergy SY7636A Multi-Function Device Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/sy7636a.h b/include/linux/mfd/sy7636a.h
> > new file mode 100644
> > index 000000000000..b6845a3572b8
> > --- /dev/null
> > +++ b/include/linux/mfd/sy7636a.h
> > @@ -0,0 +1,47 @@
> > +/* 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
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/driver.h>
> > +#include <linux/regulator/machine.h>
> > +
> > +#define SY7636A_REG_OPERATION_MODE_CRL               0x00
> > +#define SY7636A_OPERATION_MODE_CRL_VCOMCTL   BIT(6)
> > +#define SY7636A_OPERATION_MODE_CRL_ONOFF     BIT(7)
> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_L               0x01
> > +#define SY7636A_REG_VCOM_ADJUST_CTRL_H               0x02
> > +#define SY7636A_REG_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_MIN             0
> > +#define VCOM_MAX             5000
> > +
> > +#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
> > +
> > +struct sy7636a {
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     struct gpio_desc *pgood_gpio;
>
> This looks unused?

It is used in the syy636a-regulator

Alistair

>
> > +};
> > +
> > +#endif /* __LINUX_MFD_SY7636A_H */
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 3/6] hwmon: sy7636a: Add temperature driver for sy7636a

[Alistair Francis]

On Wed, Jul 21, 2021 at 12:59 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Thu, 08 Jul 2021, Alistair Francis wrote:
>
> > This is a multi-function device to interface with the sy7636a
> > EPD PMIC chip from Silergy.
> >
> > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > ---
> >  arch/arm/configs/imx_v6_v7_defconfig |   1 +
> >  drivers/hwmon/Kconfig                |  10 +++
> >  drivers/hwmon/Makefile               |   1 +
> >  drivers/hwmon/sy7636a-hwmon.c        | 106 +++++++++++++++++++++++++++
> >  4 files changed, 118 insertions(+)
> >  create mode 100644 drivers/hwmon/sy7636a-hwmon.c
> >
> > diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
> > index cd80e85d37cf..e9c0be5629c6 100644
> > --- a/arch/arm/configs/imx_v6_v7_defconfig
> > +++ b/arch/arm/configs/imx_v6_v7_defconfig
> > @@ -227,6 +227,7 @@ CONFIG_RN5T618_POWER=m
> >  CONFIG_SENSORS_MC13783_ADC=y
> >  CONFIG_SENSORS_GPIO_FAN=y
> >  CONFIG_SENSORS_IIO_HWMON=y
> > +CONFIG_SENSORS_SY7636A=y
> >  CONFIG_THERMAL_STATISTICS=y
> >  CONFIG_THERMAL_WRITABLE_TRIPS=y
> >  CONFIG_CPU_THERMAL=y
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index e3675377bc5d..6cae12de59cd 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1631,6 +1631,16 @@ config SENSORS_SIS5595
> >         This driver can also be built as a module. If so, the module
> >         will be called sis5595.
> >
> > +config SENSORS_SY7636A
> > +     tristate "Silergy SY7636A"
> > +     depends on I2C
> > +     help
> > +       If you say yes here you get support for the thermistor readout of
> > +       the Silergy SY7636A PMIC.
> > +
> > +       This driver can also be built as a module.  If so, the module
> > +       will be called sy7636a-hwmon.
> > +
> >  config SENSORS_DME1737
> >       tristate "SMSC DME1737, SCH311x and compatibles"
> >       depends on I2C && !PPC
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index d712c61c1f5e..8b2e09e25b24 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -180,6 +180,7 @@ obj-$(CONFIG_SENSORS_SMSC47M1)    += smsc47m1.o
> >  obj-$(CONFIG_SENSORS_SMSC47M192)+= smsc47m192.o
> >  obj-$(CONFIG_SENSORS_SPARX5) += sparx5-temp.o
> >  obj-$(CONFIG_SENSORS_STTS751)        += stts751.o
> > +obj-$(CONFIG_SENSORS_SY7636A)        += sy7636a-hwmon.o
> >  obj-$(CONFIG_SENSORS_AMC6821)        += amc6821.o
> >  obj-$(CONFIG_SENSORS_TC74)   += tc74.o
> >  obj-$(CONFIG_SENSORS_THMC50) += thmc50.o
> > diff --git a/drivers/hwmon/sy7636a-hwmon.c b/drivers/hwmon/sy7636a-hwmon.c
> > new file mode 100644
> > index 000000000000..4edbee99b693
> > --- /dev/null
> > +++ b/drivers/hwmon/sy7636a-hwmon.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Functions to access SY3686A power management chip temperature
> > + *
> > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> > + *
> > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
>
> The long form isn't usually accepted anymore.
>
> Please replace with SPDX.
>
> > + */
> > +
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/err.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/mfd/sy7636a.h>
> > +
> > +struct sy7636a_data {
> > +     struct sy7636a *sy7636a;
> > +     struct device *hwmon_dev;
> > +};
> > +
> > +static ssize_t show_temp(struct device *dev,
> > +     struct device_attribute *attr, char *buf)
> > +{
> > +     unsigned int reg_val;
> > +     signed char temp;
> > +     int ret;
> > +     struct sy7636a_data *data = dev_get_drvdata(dev);
> > +
> > +     ret = regmap_read(data->sy7636a->regmap,
> > +                     SY7636A_REG_TERMISTOR_READOUT, &reg_val);
> > +     if (ret)
> > +             return ret;
> > +
> > +     temp = *((signed char*)&reg_val);
>
> Whoa!  What's going on here?
>
> You also need to run checkpatch.pl.
>
> > +     return snprintf(buf, PAGE_SIZE, "%d\n", temp);
> > +}
> > +
> > +static SENSOR_DEVICE_ATTR(temp0, S_IRUGO, show_temp, NULL, 0);
> > +
> > +static struct attribute *sy7636a_attrs[] = {
> > +     &sensor_dev_attr_temp0.dev_attr.attr,
> > +     NULL
> > +};
> > +
> > +ATTRIBUTE_GROUPS(sy7636a);
> > +
> > +static int sy7636a_sensor_probe(struct platform_device *pdev)
> > +{
> > +     struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
> > +     struct sy7636a_data *data;
> > +     int err;
> > +
> > +     if (!sy7636a)
> > +             return -EPROBE_DEFER;
> > +
> > +     data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
>
> Where is this used, outside of this function?

Removed.

>
> Not sure I see a good reason for having it around?
>
> > +     if (!data) {
> > +             return -ENOMEM;
> > +     }
> > +
> > +     data->sy7636a = sy7636a;
> > +     data->hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
>
> Why is this being stored into a struct?

Good point, removed.

>
> > +                     "sy7636a_temperature", data, sy7636a_groups);
> > +     if (IS_ERR(data->hwmon_dev)) {
> > +             err = PTR_ERR(data->hwmon_dev);
> > +             dev_err(&pdev->dev, "Unable to register hwmon device, returned %d", err);
> > +             return err;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct platform_device_id sy7636a_sensor_id[] = {
> > +     { "sy7636a-temperature", 0},
> > +     { },
> > +};
> > +MODULE_DEVICE_TABLE(platform, sy7636a_sensor_id);
> > +
> > +static struct platform_driver sy7636a_sensor_driver = {
> > +     .probe = sy7636a_sensor_probe,
> > +     .id_table = sy7636a_sensor_id,
>
> What does this do?

Removed

Alistair

>
> Where is the 'device' being registered?
>
> > +     .driver = {
> > +             .name = "sy7636a-temperature",
> > +     },
> > +};
> > +module_platform_driver(sy7636a_sensor_driver);
> > +
> > +MODULE_DESCRIPTION("SY7636A sensor driver");
> > +MODULE_LICENSE("GPL");
> > +
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/6] mfd: sy7636a: Initial commit

[Lee Jones]

On Mon, 02 Aug 2021, Alistair Francis wrote:

> On Wed, Jul 21, 2021 at 12:53 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Thu, 08 Jul 2021, Alistair Francis wrote:
> >
> > > Initial support for the Silergy SY7636A Power Management chip.
> > >
> > > Signed-off-by: Alistair Francis <alistair@alistair23.me>
> > > ---
> > >  drivers/mfd/Kconfig         |  9 +++++
> > >  drivers/mfd/Makefile        |  1 +
> > >  drivers/mfd/sy7636a.c       | 81 +++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/sy7636a.h | 47 +++++++++++++++++++++
> > >  4 files changed, 138 insertions(+)
> > >  create mode 100644 drivers/mfd/sy7636a.c
> > >  create mode 100644 include/linux/mfd/sy7636a.h

[...]

> > > +struct sy7636a {
> > > +     struct device *dev;
> > > +     struct regmap *regmap;
> > > +     struct gpio_desc *pgood_gpio;
> >
> > This looks unused?
> 
> It is used in the syy636a-regulator

If it's only used in one driver, please declare it there.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 2/6] thermal: sy7636a: Add thermal driver for sy7636a

[Daniel Lezcano]

On 08/07/2021 13:58, Alistair Francis wrote:
> From: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.no>
> 
> Add thermal driver to enable kernel based polling
> and shutdown of device for temperatures out of spec
> 
> Signed-off-by: Alistair Francis <alistair@alistair23.me>
> ---
>  drivers/thermal/Kconfig           |   7 ++
>  drivers/thermal/Makefile          |   1 +
>  drivers/thermal/sy7636a_thermal.c | 107 ++++++++++++++++++++++++++++++
>  3 files changed, 115 insertions(+)
>  create mode 100644 drivers/thermal/sy7636a_thermal.c
> 
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index d7f44deab5b1..7112c63d9021 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -450,6 +450,13 @@ depends on (ARCH_STI || ARCH_STM32) && OF
>  source "drivers/thermal/st/Kconfig"
>  endmenu
>  
> +config SY7636A_THERMAL
> +	tristate "SY7636A thermal management"
> +	depends on MFD_SY7636A
> +	help
> +	  Enable the sy7636a thermal driver, which supports the
> +	  temperature sensor embedded in Silabs SY7636A chip.
> +
>  source "drivers/thermal/tegra/Kconfig"
>  
>  config GENERIC_ADC_THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 82fc3e616e54..2e1aca8a0a09 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_DA9062_THERMAL)	+= da9062-thermal.o
>  obj-y				+= intel/
>  obj-$(CONFIG_TI_SOC_THERMAL)	+= ti-soc-thermal/
>  obj-y				+= st/
> +obj-$(CONFIG_SY7636A_THERMAL)	+= sy7636a_thermal.o
>  obj-$(CONFIG_QCOM_TSENS)	+= qcom/
>  obj-y				+= tegra/
>  obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
> diff --git a/drivers/thermal/sy7636a_thermal.c b/drivers/thermal/sy7636a_thermal.c
> new file mode 100644
> index 000000000000..705a16fb1045
> --- /dev/null
> +++ b/drivers/thermal/sy7636a_thermal.c
> @@ -0,0 +1,107 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +//
> +// Functions to access SY3686A power management chip temperature
> +//
> +// Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> +//
> +// Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>
> +//          Alistair Francis <alistair@alistair23.me>

// SPDX-License-Identifier: GPL-2.0+
/*
 * Functions to access SY3686A power management chip temperature
 * ...
 *
 */

> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/thermal.h>
> +
> +#include <linux/mfd/sy7636a.h>
> +
> +struct sy7636a_data {
> +	struct sy7636a *sy7636a;

Only regmap is used AFAICT, so storing the regmap pointer is enough and
the structure is not needed neither the code associated with it.

> +	struct thermal_zone_device *thermal_zone_dev;

This field is only used to check the return code of the
thermal_zone_device_register function. It can be local to the function
below.

> +};
> +
> +static int sy7636a_get_temp(void *arg, int *res)
> +{
> +	unsigned int reg_val, mode_ctr;

Is it possible the sensor returns negative values ?

> +	int ret;
> +	struct sy7636a_data *data = arg;
> +	bool isVoltageActive;
> +	ret = regmap_read(data->sy7636a->regmap,
> +			SY7636A_REG_OPERATION_MODE_CRL, &mode_ctr);
> +	if (ret)
> +		return ret;
> +
> +	isVoltageActive = mode_ctr & SY7636A_OPERATION_MODE_CRL_ONOFF;
> +
> +	if (!isVoltageActive) {
> +		ret = regmap_write(data->sy7636a->regmap,
> +				SY7636A_REG_OPERATION_MODE_CRL,
> +				mode_ctr | SY7636A_OPERATION_MODE_CRL_ONOFF);

What is this conditional block ? Please add a comment to explain why
these 'isVoltageActive' test are needed.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(data->sy7636a->regmap,
> +			SY7636A_REG_TERMISTOR_READOUT, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (!isVoltageActive) {
> +		ret = regmap_write(data->sy7636a->regmap,
> +				SY7636A_REG_OPERATION_MODE_CRL,
> +				mode_ctr);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	*res = *((signed char*)&reg_val);

Please revisit this.

> +	*res *= 1000;
> +
> +	return ret;
> +}
> +
> +static const struct thermal_zone_of_device_ops ops = {
> +	.get_temp	= sy7636a_get_temp,
> +};
> +
> +static int sy7636a_thermal_probe(struct platform_device *pdev)
> +{
> +	struct sy7636a *sy7636a = dev_get_drvdata(pdev->dev.parent);
> +	struct sy7636a_data *data;
> +
> +	if (!sy7636a)
> +		return -EPROBE_DEFER;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, data);
> +
> +	data->sy7636a = sy7636a;
> +	data->thermal_zone_dev = devm_thermal_zone_of_sensor_register(
> +			pdev->dev.parent,
> +			0,
> +			data,
> +			&ops);
> +
> +	return PTR_ERR_OR_ZERO(data->thermal_zone_dev);
> +}
> +
> +static const struct platform_device_id sy7636a_thermal_id_table[] = {
> +	{ "sy7636a-thermal", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(platform, sy7636a_thermal_id_table);
> +
> +static struct platform_driver sy7636a_thermal_driver = {
> +	.driver = {
> +		.name = "sy7636a-thermal",
> +	},
> +	.probe = sy7636a_thermal_probe,
> +	.id_table = sy7636a_thermal_id_table,
> +};
> +module_platform_driver(sy7636a_thermal_driver);
> +
> +MODULE_AUTHOR("Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>");
> +MODULE_DESCRIPTION("SY7636A thermal driver");
> +MODULE_LICENSE("GPL v2");
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog