Re: [PATCH v14 5/8] hwmon: sy7636a: Add temperature driver for sy7636a
From: Guenter Roeck
Date: Mon Oct 25 2021 - 10:26:13 EST
On 10/25/21 3:53 AM, 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@xxxxxxxxxxxxx>
---
Documentation/hwmon/sy7636a-hwmon.rst | 24 ++++++
Needs to be added to Documentation/hwmon/index.rst
drivers/hwmon/Kconfig | 9 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/sy7636a-hwmon.c | 114 ++++++++++++++++++++++++++
4 files changed, 148 insertions(+)
create mode 100644 Documentation/hwmon/sy7636a-hwmon.rst
create mode 100644 drivers/hwmon/sy7636a-hwmon.c
diff --git a/Documentation/hwmon/sy7636a-hwmon.rst b/Documentation/hwmon/sy7636a-hwmon.rst
new file mode 100644
index 000000000000..6b3e36d028dd
--- /dev/null
+++ b/Documentation/hwmon/sy7636a-hwmon.rst
@@ -0,0 +1,24 @@
+Kernel driver sy7636a-hwmon
+=========================
+
+Supported chips:
+
+ * Silergy SY7636A PMIC
+
+
+Description
+-----------
+
+This driver adds hardware temperature reading support for
+the Silergy SY7636A PMIC.
+
+The following sensors are supported
+
+ * Temperature
+ - SoC on-die temperature in milli-degree C
+
+sysfs-Interface
+---------------
+
+temp0_input
+ - SoC on-die temperature (milli-degree C)
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index c4578e8f34bb..d768b833b721 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1651,6 +1651,15 @@ 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"
+ 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 162940270661..1355ffdb1481 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -181,6 +181,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..1727a5f2a2ea
--- /dev/null
+++ b/drivers/hwmon/sy7636a-hwmon.c
@@ -0,0 +1,114 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Functions to access SY3686A power management chip temperature
+ *
+ * Copyright (C) 2021 reMarkable AS - http://www.remarkable.com/
+ *
+ * Authors: Lars Ivar Miljeteig <lars.ivar.miljeteig@xxxxxxxxxxxxxx>
+ * Alistair Francis <alistair@xxxxxxxxxxxxx>
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/machine.h>
+
+#include <linux/mfd/sy7636a.h>
+
+struct sy7636a_data {
+ struct regmap *regmap;
+ struct regulator *regulator;
+};
+
+static int sy7636a_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *temp)
+{
+ struct sy7636a_data *data = dev_get_drvdata(dev);
+ int ret, reg_val;
+
+ ret = regulator_enable(data->regulator);
+ if (ret)
+ return ret;
+
This doesn't look correct. There is no matching call to regulator_disable,
meaning the use count will go up with each read. Also, it seems to be too
late to enable the regulator just before reading the temperature value.
If the regulator is indeed useful, it would enable power to the circuitry
performing the temperature read operation just before actually reading
the temperature, leaving the circuitry no time to actually measure the
temperature.
+ ret = regmap_read(data->regmap,
+ SY7636A_REG_TERMISTOR_READOUT, ®_val);
+ if (ret)
+ return ret;
+
+ *temp = reg_val * 1000;
+
+ return 0;
+}
+
+static umode_t sy7636a_is_visible(const void *data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_temp)
+ return 0;
+
+ if (attr != hwmon_temp_input)
+ return 0;
+
+ return 0444;
+}
+
+static const struct hwmon_ops sy7636a_hwmon_ops = {
+ .is_visible = sy7636a_is_visible,
+ .read = sy7636a_read,
+};
+
+static const struct hwmon_channel_info *sy7636a_info[] = {
+ HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
+ HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
+ NULL
+};
+
+static const struct hwmon_chip_info sy7636a_chip_info = {
+ .ops = &sy7636a_hwmon_ops,
+ .info = sy7636a_info,
+};
+
+static int sy7636a_sensor_probe(struct platform_device *pdev)
+{
+ struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ struct sy7636a_data *data;
+ struct device *hwmon_dev;
+ int err;
+
+ if (!regmap)
+ return -EPROBE_DEFER;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(struct sy7636a_data), GFP_KERNEL);
+ if (!data) {
+ return -ENOMEM;
+ }
+
+ data->regmap = regmap;
+ data->regulator = devm_regulator_get(&pdev->dev, "vcom");
+
This needs an error check.
+ hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+ "sy7636a_temperature", regmap, &sy7636a_chip_info, NULL);
+
+ if (IS_ERR(hwmon_dev)) {
+ err = PTR_ERR(hwmon_dev);
+ dev_err(&pdev->dev, "Unable to register hwmon device, returned %d\n", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static struct platform_driver sy7636a_sensor_driver = {
+ .probe = sy7636a_sensor_probe,
+ .driver = {
+ .name = "sy7636a-temperature",
+ },
+};
+module_platform_driver(sy7636a_sensor_driver);
+
+MODULE_DESCRIPTION("SY7636A sensor driver");
+MODULE_LICENSE("GPL");