[PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x

[PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x

[Andre Werner]

Add base support for Renesas HS300x temperature
and humidity sensors.

The sensors has a fix I2C address 0x44. The resolution
is fixed to 14bit (ref. Missing feature).

Missing feature:
- Accessing non-volatile memory: Custom board has no
  possibility to control voltage supply of sensor. Thus,
  we cannot send the necessary control commands within
  the first 10ms after power-on.

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
 Documentation/hwmon/hs300x.rst |  61 +++++
 drivers/hwmon/Kconfig          |  10 +
 drivers/hwmon/Makefile         |   1 +
 drivers/hwmon/hs300x.c         | 461 +++++++++++++++++++++++++++++++++
 4 files changed, 533 insertions(+)
 create mode 100644 Documentation/hwmon/hs300x.rst
 create mode 100644 drivers/hwmon/hs300x.c

diff --git a/Documentation/hwmon/hs300x.rst b/Documentation/hwmon/hs300x.rst
new file mode 100644
index 000000000000..2be05d0f00ab
--- /dev/null
+++ b/Documentation/hwmon/hs300x.rst
@@ -0,0 +1,61 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver HS300x
+===================
+
+Supported chips:
+
+  * Renesas HS3001, HS3002, HS3003, HS3004
+
+    Prefix: 'hs300x'
+
+    Addresses scanned: -
+
+    Datasheet: https://www.renesas.com/us/en/document/dst/hs300x-datasheet?r=417401
+
+Author:
+
+  - Andre Werner <andre.werner@systec-electronic.com>
+
+Description
+-----------
+
+This driver implements support for the Renesas HS300x chips, a humidity
+and temperature family. Temperature is measured in degrees celsius, relative
+humidity is expressed as a percentage. In the sysfs interface, all values are
+scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
+
+The device communicates with the I2C protocol. Sensors can have the I2C
+address 0x44 by default.
+
+The driver does not support the sensor's configuration possibilities.
+
+The driver is able to be read out using lmsensors.
+
+sysfs-Interface
+---------------
+
+===============================================================================
+temp1_input:        temperature input
+humidity1_input:    humidity input
+temp1_max:          temperature max value
+humidity1_max:      humidity max value
+temp1_min:          temperature min value
+humidity1_min:      humidity min value
+measuretime:        Measurement delay in usec
+===============================================================================
+
+Device Tree
+-----------
+
+Required node properties:
+
+ - compatible:  must be set to "hs3001", "hs3002", "hs3003", "hs3004"
+ - reg:         I2C address of the device (0x44)
+
+Example:
+
+    humidity: hs3002@44 {
+        compatible = "hs3002";
+        reg = <0x44>;
+    };
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 307477b8a371..9ce82fe0044b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -734,6 +734,16 @@ config SENSORS_HIH6130
 	  This driver can also be built as a module. If so, the module
 	  will be called hih6130.
 
+config SENSORS_HS300x
+	tristate "Renesas HS300x humidity and temperature sensors"
+	depends on I2C
+	help
+	  If you say yes here you get support for the Renesas HS300x
+	  humidity and temperature sensors.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called hs300x.
+
 config SENSORS_IBMAEM
 	tristate "IBM Active Energy Manager temperature/power sensors and control"
 	select IPMI_SI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 3f4b0fda0998..a067c0896ca1 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
 obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
 obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
 obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
+obj-$(CONFIG_SENSORS_HS300x)	+= hs300x.o
 obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
 obj-$(CONFIG_SENSORS_I5500)	+= i5500_temp.o
 obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
diff --git a/drivers/hwmon/hs300x.c b/drivers/hwmon/hs300x.c
new file mode 100644
index 000000000000..a22cc55a01ce
--- /dev/null
+++ b/drivers/hwmon/hs300x.c
@@ -0,0 +1,461 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* This is a non-complete driver implementation for the
+ * HS300x humidity and temperature sensors. It does not include
+ * the configuration possibilities, because the hardware platform the IC is
+ * used does not control the power source for the IC. Thus, it cannot being
+ * set to 'programming mode' during power-up.
+ *
+ *
+ * Copyright (C) 2022 SYS TEC electronic AG
+ * Author: Andre Werner <andre.werner@systec-electronic.com>
+ */
+
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+
+/* Measurement times */
+#define HS300X_WAKEUP_TIME 100	/* us */
+#define HS300X_UNUSED 0		/* us */
+#define HS300X_8BIT_RESOLUTION 550	/* us */
+#define HS300X_10BIT_RESOLUTION 1310	/* us */
+#define HS300X_12BIT_RESOLUTION 4500	/* us */
+#define HS300X_14BIT_RESOLUTION 16900	/* us */
+
+#define HS300X_CMD_OK 0
+#define HS300X_RESPONSE_LENGTH 4
+
+#define HS300X_FIXPOINT_ARITH 1000
+#define HS300X_MIN_TEMPERATURE (-40 * HS300X_FIXPOINT_ARITH) /* 1000 degree */
+#define HS300X_MAX_TEMPERATURE (125 * HS300X_FIXPOINT_ARITH) /* 1000 degree */
+#define HS300X_MIN_HUMIDITY (0 * HS300X_FIXPOINT_ARITH) /* 1000 % */
+#define HS300X_MAX_HUMIDITY (100 * HS300X_FIXPOINT_ARITH) /* 1000 % */
+
+#define HS300X_MASK_HUMIDITY_0X3FFF (0x3FFF)
+#define HS300X_MASK_TEMPERATURE_0XFFFC (0xFFFC)
+#define HS300X_MASK_STATUS_0XC0 (0xC0)
+#define HS300X_STATUS_SHIFT (6)
+
+/* Definitions for Status Bits of A/D Data */
+#define HS300X_DATA_VALID (0x00)	/* Valid Data */
+#define HS300X_DATA_STALE (0x01)	/* Stale Data */
+
+#define LIMIT_MAX 0
+#define LIMIT_MIN 1
+
+enum hs300x_chips {
+	hs3001,
+	hs3002,
+	hs3003,
+	hs3004,
+};
+
+struct hs300x_accurancy {
+	u32 hum_acc;		/* accurancy of humidity in *1000 % */
+	u32 temp_acc;		/* accurancy of temperature in *1000 % */
+};
+
+const struct hs300x_accurancy hs300x_accurancys[] = {
+	{ 1500, 200 },
+	{ 1800, 200 },
+	{ 2500, 250 },
+	{ 3500, 300 },
+};
+
+struct hs300x_data {
+	struct i2c_client *client;
+	struct mutex i2c_lock;	/* lock for sending i2c commands */
+	struct mutex data_lock;	/* lock for updating driver data */
+	u32 wait_time;		/* in us */
+	int temperature;	/* in *1000 degree */
+	u32 humidity;		/* in *1000 % */
+};
+
+static int hs300x_measure_command(struct i2c_client *client,
+				  struct hs300x_data *data)
+{
+	int ret = 0;
+	unsigned char buf[1] = { 0x00 };
+
+	if (!data || !client)
+		return -EINVAL;
+
+	mutex_lock(&data->i2c_lock);
+	ret = i2c_master_send(client, (const char *)buf, 0);
+	mutex_unlock(&data->i2c_lock);
+
+	if (0 > ret)
+		dev_dbg(&client->dev,
+			"Error starting measurement. Errno: %d.\n", ret);
+	else {
+		ret = HS300X_CMD_OK;
+	}
+
+	return ret;
+}
+
+static int hs300x_extract_temperature(u16 raw)
+{
+	/* fixpoint arithmetic 1 digit */
+	int temp = ((raw & HS300X_MASK_TEMPERATURE_0XFFFC) >> 2) *
+	    HS300X_FIXPOINT_ARITH;
+
+	temp /= ((1 << 14) - 1);
+
+	return (temp * 165) - (40 * HS300X_FIXPOINT_ARITH);
+}
+
+static u32 hs300x_extract_humidity(u16 raw)
+{
+	int hum = (raw & HS300X_MASK_HUMIDITY_0X3FFF) * HS300X_FIXPOINT_ARITH;
+
+	hum /= ((1 << 14) - 1);
+
+	return hum * 100;
+}
+
+static int hs300x_data_fetch_command(struct i2c_client *client,
+				     struct hs300x_data *data)
+{
+	int ret = HS300X_CMD_OK;
+	u8 buf[HS300X_RESPONSE_LENGTH];
+	u8 hs300x_status;
+
+	if (!client || !data)
+		return -EINVAL;
+
+	mutex_lock(&data->i2c_lock);
+	ret = i2c_master_recv(client, buf, HS300X_RESPONSE_LENGTH);
+	mutex_unlock(&data->i2c_lock);
+
+	if (ret != HS300X_RESPONSE_LENGTH) {
+		ret = ret < 0 ? ret : -EIO;
+		dev_dbg(&client->dev,
+			"Error in i2c communication. Error code: %d.\n", ret);
+		goto out;
+	}
+
+	hs300x_status = (buf[0] & HS300X_MASK_STATUS_0XC0) >>
+	    HS300X_STATUS_SHIFT;
+	if (hs300x_status == HS300X_DATA_STALE) {
+		dev_dbg(&client->dev, "Sensor busy.\n");
+		ret = -EBUSY;
+		goto out;
+	} else if (hs300x_status != HS300X_DATA_VALID) {
+		dev_dbg(&client->dev, "Data invalid.\n");
+		ret = -EIO;
+		goto out;
+	}
+
+	mutex_lock(&data->data_lock);
+	data->humidity =
+	    hs300x_extract_humidity(be16_to_cpup((__be16 *) &buf[0]));
+	data->temperature =
+	    hs300x_extract_temperature(be16_to_cpup((__be16 *) &buf[2]));
+	mutex_unlock(&data->data_lock);
+
+	ret = HS300X_CMD_OK;
+
+out:
+	return ret;
+}
+
+/* sysfs attributes */
+static ssize_t measuretime_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct hs300x_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	client = data->client;
+
+	if (!client)
+		return -ENODEV;
+
+	return sprintf(buf, "%d\n", data->wait_time);
+}
+
+static ssize_t temp1_input_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct hs300x_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client;
+	int ret;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	client = data->client;
+
+	if (!client) {
+		dev_dbg(dev, "No valid I2C client available.\n");
+		return -ENODEV;
+	}
+
+	ret = hs300x_measure_command(client, data);
+	if (ret != HS300X_CMD_OK)
+		return ret;
+
+	/* Sensor needs some time to process measurement depending on
+	 * resolution
+	 */
+	fsleep(data->wait_time);
+
+	ret = hs300x_data_fetch_command(client, data);
+	if (ret != HS300X_CMD_OK)
+		return ret;
+
+	return sprintf(buf, "%d\n", data->temperature);
+}
+
+static ssize_t humidity1_input_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct hs300x_data *data = dev_get_drvdata(dev);
+	struct i2c_client *client;
+	int ret;
+
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	client = data->client;
+
+	if (!client) {
+		dev_dbg(dev, "No valid I2C client available.\n");
+		return -ENODEV;
+	}
+
+	ret = hs300x_measure_command(client, data);
+	if (ret != HS300X_CMD_OK)
+		return ret;
+
+	/* Sensor needs some time to process measurement depending on
+	 * resolution
+	 */
+	fsleep(data->wait_time);
+
+	ret = hs300x_data_fetch_command(client, data);
+	if (ret != HS300X_CMD_OK)
+		return ret;
+
+	return sprintf(buf, "%u\n", data->humidity);
+}
+
+static ssize_t temp1_limit_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	int temperature_limit;
+	u8 index;
+
+	index = to_sensor_dev_attr(attr)->index;
+	if (LIMIT_MIN == index)
+		temperature_limit = HS300X_MIN_TEMPERATURE;
+	else
+		temperature_limit = HS300X_MAX_TEMPERATURE;
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", temperature_limit);
+}
+
+static ssize_t humidity1_limit_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	u32 humidity_limit;
+	u8 index;
+
+	index = to_sensor_dev_attr(attr)->index;
+	if (LIMIT_MIN == index)
+		humidity_limit = HS300X_MIN_HUMIDITY;
+	else
+		humidity_limit = HS300X_MAX_HUMIDITY;
+
+	return scnprintf(buf, PAGE_SIZE, "%u\n", humidity_limit);
+}
+
+static SENSOR_DEVICE_ATTR_RO(temp1_input, temp1_input, 0);
+static SENSOR_DEVICE_ATTR_RO(humidity1_input, humidity1_input, 0);
+static SENSOR_DEVICE_ATTR_RO(measuretime, measuretime, 0);
+static SENSOR_DEVICE_ATTR_RO(temp1_max, temp1_limit, LIMIT_MAX);
+static SENSOR_DEVICE_ATTR_RO(humidity1_max, humidity1_limit, LIMIT_MAX);
+static SENSOR_DEVICE_ATTR_RO(temp1_min, temp1_limit, LIMIT_MIN);
+static SENSOR_DEVICE_ATTR_RO(humidity1_min, humidity1_limit, LIMIT_MIN);
+
+static struct attribute *hs3001_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	&sensor_dev_attr_measuretime.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_humidity1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_humidity1_min.dev_attr.attr,
+	NULL
+};
+
+static struct attribute *hs3002_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	&sensor_dev_attr_measuretime.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_humidity1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_humidity1_min.dev_attr.attr,
+	NULL
+};
+
+static struct attribute *hs3003_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	&sensor_dev_attr_measuretime.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_humidity1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_humidity1_min.dev_attr.attr,
+	NULL
+};
+
+static struct attribute *hs3004_attrs[] = {
+	&sensor_dev_attr_temp1_input.dev_attr.attr,
+	&sensor_dev_attr_humidity1_input.dev_attr.attr,
+	&sensor_dev_attr_measuretime.dev_attr.attr,
+	&sensor_dev_attr_temp1_max.dev_attr.attr,
+	&sensor_dev_attr_humidity1_max.dev_attr.attr,
+	&sensor_dev_attr_temp1_min.dev_attr.attr,
+	&sensor_dev_attr_humidity1_min.dev_attr.attr,
+	NULL
+};
+
+ATTRIBUTE_GROUPS(hs3001);
+ATTRIBUTE_GROUPS(hs3002);
+ATTRIBUTE_GROUPS(hs3003);
+ATTRIBUTE_GROUPS(hs3004);
+
+static const struct i2c_device_id hs300x_ids[];
+#ifdef CONFIG_OF
+static const struct of_device_id hs300x_of_ids[] = {
+	{.compatible = "hs3001" },
+	{.compatible = "hs3002" },
+	{.compatible = "hs3003" },
+	{.compatible = "hs3004" },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, hs300x_of_ids);
+#endif
+
+static int hs300x_probe(struct i2c_client *client)
+{
+	struct hs300x_data *data;
+	struct device *hwmon_dev;
+	struct i2c_adapter *adap = client->adapter;
+	struct device *dev = &client->dev;
+	const struct attribute_group **attribute_groups;
+	int ret;
+#ifdef CONFIG_OF
+	const struct of_device_id *of_match;
+
+	of_match = of_match_device(hs300x_of_ids, dev);
+	if (!of_match) {
+		dev_err(dev, "No match in DT compatibles.\n");
+		return -ENODEV;
+	}
+
+#endif
+	if (!i2c_check_functionality(adap, I2C_FUNC_I2C))
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	/* Measurement time = wake-up time + measurement time temperature
+	 *      measurment time humidity This is currently static, because
+	 *      enabling programming mode is not supported, yet.
+	 */
+	data->wait_time = (HS300X_WAKEUP_TIME + HS300X_14BIT_RESOLUTION +
+			   HS300X_14BIT_RESOLUTION);
+	mutex_init(&data->i2c_lock);
+	mutex_init(&data->data_lock);
+
+	if (i2c_match_id(hs300x_ids, client)->driver_data == hs3001)
+		attribute_groups = hs3001_groups;
+	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3002)
+		attribute_groups = hs3002_groups;
+	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3003)
+		attribute_groups = hs3003_groups;
+	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3004)
+		attribute_groups = hs3004_groups;
+	else {
+		dev_err(dev, "Unknwon device for HS300x driver\n");
+		goto error;
+	}
+
+	/* Measure humidity and temperature to initialize values */
+	ret = hs300x_measure_command(client, data);
+	if (ret) {
+		goto error;
+	}
+
+	/* Sensor needs some time to process measurement depending on
+	 * resolution
+	 */
+	fsleep(data->wait_time);
+
+	ret = hs300x_data_fetch_command(client, data);
+	if (ret != HS300X_CMD_OK) {
+		goto error;
+	}
+
+	hwmon_dev =
+	    devm_hwmon_device_register_with_groups(dev, client->name, data,
+						   attribute_groups);
+
+	if (IS_ERR(hwmon_dev)) {
+		dev_err(dev, "Unable to register hwmon device %s\n",
+			client->name);
+		goto error;
+	}
+
+	dev_info(dev, "Successfully probe %s: sensor '%s'\n",
+		 dev_name(hwmon_dev), client->name);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+error:
+	devm_kfree(dev, data);
+	return -ENODEV;
+}
+
+/* device ID table */
+static const struct i2c_device_id hs300x_ids[] = {
+	{ "hs3001", hs3001 },
+	{ "hs3002", hs3002 },
+	{ "hs3003", hs3003 },
+	{ "hs3004", hs3004 },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, hs300x_ids);
+
+static struct i2c_driver hs300x_i2c_driver = {
+	.driver = {
+		   .name = "hs300x",
+		   .of_match_table = of_match_ptr(hs300x_of_ids),
+		    },
+	.probe_new = hs300x_probe,
+	.id_table = hs300x_ids,
+};
+
+module_i2c_driver(hs300x_i2c_driver);
+
+MODULE_AUTHOR("Andre Werner <andre.werner@systec-electronic.com>");
+MODULE_DESCRIPTION("HS300x humidity and temperature sensor base  driver");
+MODULE_LICENSE("GPL");
-- 
2.41.0

[PATCH 2/2] dt-bindings: hwmon: Add description for new hwmon driver hs300x

[Andre Werner]

This is the initial description.

Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
---
 .../devicetree/bindings/hwmon/hs300x.yaml     | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/hs300x.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/hs300x.yaml b/Documentation/devicetree/bindings/hwmon/hs300x.yaml
new file mode 100644
index 000000000000..7a9b3ee3738b
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/hs300x.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/hs300x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas HS300x humidity and temperature sensor
+
+maintainers:
+  - Andre Werner (andre.werner@systec-electronic.com)
+
+description: |
+  This driver implements support for the Renesas HS300x chips, a humidity
+  and temperature family. Temperature is measured in degrees celsius, relative
+  humidity is expressed as a percentage. In the sysfs interface, all values are
+  scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
+
+  The device communicates with the I2C protocol. Sensors can have the I2C
+  address 0x44 by default.
+
+  The driver does not support the sensor's configuration possibilities.
+
+  The driver is able to be read out using lmsensors.
+
+  Datasheets:
+  https://www.renesas.com/us/en/document/dst/hs300x-datasheet?r=417401
+
+
+
+properties:
+  compatible:
+    enum:
+      - hs3001
+      - hs3002
+      - hs3003
+      - hs3004
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          humidity: hs3002@44 {
+              compatible = "hs3002";
+              reg = <0x44>;
+          };
+    };
-- 
2.41.0

Re: [PATCH 2/2] dt-bindings: hwmon: Add description for new hwmon driver hs300x

[Krzysztof Kozlowski]

On 11/07/2023 16:06, Andre Werner wrote:
> This is the initial description.
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> ---
>  .../devicetree/bindings/hwmon/hs300x.yaml     | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/hs300x.yaml

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by
automated tooling. Performing review on untested code might be a waste
of time, thus I will skip this patch entirely till you follow the
process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

> 
> diff --git a/Documentation/devicetree/bindings/hwmon/hs300x.yaml b/Documentation/devicetree/bindings/hwmon/hs300x.yaml
> new file mode 100644
> index 000000000000..7a9b3ee3738b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/hs300x.yaml

Filename like compatible

> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +

Drop

> +$id: http://devicetree.org/schemas/hwmon/hs300x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas HS300x humidity and temperature sensor
> +
> +maintainers:
> +  - Andre Werner (andre.werner@systec-electronic.com)
> +
> +description: |
> +  This driver implements support for the Renesas HS300x chips, a humidity

Bindings are for hardware, not drivers.

> +  and temperature family. Temperature is measured in degrees celsius, relative
> +  humidity is expressed as a percentage. In the sysfs interface, all values are
> +  scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.

sysfs is not hardware related, drop.

> +
> +  The device communicates with the I2C protocol. Sensors can have the I2C
> +  address 0x44 by default.
> +
> +  The driver does not support the sensor's configuration possibilities.
> +
> +  The driver is able to be read out using lmsensors.
> +
> +  Datasheets:
> +  https://www.renesas.com/us/en/document/dst/hs300x-datasheet?r=417401
> +
> +

Just one blank line.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - hs3001
> +      - hs3002
> +      - hs3003
> +      - hs3004

Eh, so this was not tested... That's not correct compatible, missing
vendor prefix.

> +
> +  reg:
> +    maxItems: 1
> +

Missing several properties.

> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +
> +          humidity: hs3002@44 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +              compatible = "hs3002";
> +              reg = <0x44>;
> +          };
> +    };

Best regards,
Krzysztof

Re: [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x

[Krzysztof Kozlowski]

On 11/07/2023 16:06, Andre Werner wrote:
> Add base support for Renesas HS300x temperature
> and humidity sensors.
> 
> The sensors has a fix I2C address 0x44. The resolution
> is fixed to 14bit (ref. Missing feature).
> 
> Missing feature:
> - Accessing non-volatile memory: Custom board has no
>   possibility to control voltage supply of sensor. Thus,
>   we cannot send the necessary control commands within
>   the first 10ms after power-on.
> 

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> ---
>  Documentation/hwmon/hs300x.rst |  61 +++++
>  drivers/hwmon/Kconfig          |  10 +
>  drivers/hwmon/Makefile         |   1 +
>  drivers/hwmon/hs300x.c         | 461 +++++++++++++++++++++++++++++++++
>  4 files changed, 533 insertions(+)
>  create mode 100644 Documentation/hwmon/hs300x.rst
>  create mode 100644 drivers/hwmon/hs300x.c
> 
> diff --git a/Documentation/hwmon/hs300x.rst b/Documentation/hwmon/hs300x.rst
> new file mode 100644
> index 000000000000..2be05d0f00ab
> --- /dev/null
> +++ b/Documentation/hwmon/hs300x.rst
> @@ -0,0 +1,61 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver HS300x
> +===================
> +
> +Supported chips:
> +
> +  * Renesas HS3001, HS3002, HS3003, HS3004
> +
> +    Prefix: 'hs300x'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: https://www.renesas.com/us/en/document/dst/hs300x-datasheet?r=417401
> +
> +Author:
> +
> +  - Andre Werner <andre.werner@systec-electronic.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for the Renesas HS300x chips, a humidity
> +and temperature family. Temperature is measured in degrees celsius, relative
> +humidity is expressed as a percentage. In the sysfs interface, all values are
> +scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
> +
> +The device communicates with the I2C protocol. Sensors can have the I2C
> +address 0x44 by default.
> +
> +The driver does not support the sensor's configuration possibilities.
> +
> +The driver is able to be read out using lmsensors.
> +
> +sysfs-Interface
> +---------------
> +
> +===============================================================================
> +temp1_input:        temperature input
> +humidity1_input:    humidity input
> +temp1_max:          temperature max value
> +humidity1_max:      humidity max value
> +temp1_min:          temperature min value
> +humidity1_min:      humidity min value
> +measuretime:        Measurement delay in usec
> +===============================================================================
> +
> +Device Tree
> +-----------
> +
> +Required node properties:
> +
> + - compatible:  must be set to "hs3001", "hs3002", "hs3003", "hs3004"
> + - reg:         I2C address of the device (0x44)
> +
> +Example:
> +
> +    humidity: hs3002@44 {
> +        compatible = "hs3002";
> +        reg = <0x44>;
> +    };
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 307477b8a371..9ce82fe0044b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -734,6 +734,16 @@ config SENSORS_HIH6130
>  	  This driver can also be built as a module. If so, the module
>  	  will be called hih6130.
>  
> +config SENSORS_HS300x
> +	tristate "Renesas HS300x humidity and temperature sensors"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the Renesas HS300x
> +	  humidity and temperature sensors.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called hs300x.
> +
>  config SENSORS_IBMAEM
>  	tristate "IBM Active Energy Manager temperature/power sensors and control"
>  	select IPMI_SI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3f4b0fda0998..a067c0896ca1 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
>  obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
>  obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
>  obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
> +obj-$(CONFIG_SENSORS_HS300x)	+= hs300x.o
>  obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
>  obj-$(CONFIG_SENSORS_I5500)	+= i5500_temp.o
>  obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
> diff --git a/drivers/hwmon/hs300x.c b/drivers/hwmon/hs300x.c
> new file mode 100644
> index 000000000000..a22cc55a01ce
> --- /dev/null
> +++ b/drivers/hwmon/hs300x.c
> @@ -0,0 +1,461 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* This is a non-complete driver implementation for the
> + * HS300x humidity and temperature sensors. It does not include
> + * the configuration possibilities, because the hardware platform the IC is
> + * used does not control the power source for the IC. Thus, it cannot being
> + * set to 'programming mode' during power-up.
> + *
> + *
> + * Copyright (C) 2022 SYS TEC electronic AG
> + * Author: Andre Werner <andre.werner@systec-electronic.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>
> +
> +/* Measurement times */
> +#define HS300X_WAKEUP_TIME 100	/* us */
> +#define HS300X_UNUSED 0		/* us */
> +#define HS300X_8BIT_RESOLUTION 550	/* us */
> +#define HS300X_10BIT_RESOLUTION 1310	/* us */
> +#define HS300X_12BIT_RESOLUTION 4500	/* us */
> +#define HS300X_14BIT_RESOLUTION 16900	/* us */
> +
> +#define HS300X_CMD_OK 0
> +#define HS300X_RESPONSE_LENGTH 4
> +
> +#define HS300X_FIXPOINT_ARITH 1000
> +#define HS300X_MIN_TEMPERATURE (-40 * HS300X_FIXPOINT_ARITH) /* 1000 degree */
> +#define HS300X_MAX_TEMPERATURE (125 * HS300X_FIXPOINT_ARITH) /* 1000 degree */
> +#define HS300X_MIN_HUMIDITY (0 * HS300X_FIXPOINT_ARITH) /* 1000 % */
> +#define HS300X_MAX_HUMIDITY (100 * HS300X_FIXPOINT_ARITH) /* 1000 % */
> +
> +#define HS300X_MASK_HUMIDITY_0X3FFF (0x3FFF)
> +#define HS300X_MASK_TEMPERATURE_0XFFFC (0xFFFC)
> +#define HS300X_MASK_STATUS_0XC0 (0xC0)
> +#define HS300X_STATUS_SHIFT (6)
> +
> +/* Definitions for Status Bits of A/D Data */
> +#define HS300X_DATA_VALID (0x00)	/* Valid Data */
> +#define HS300X_DATA_STALE (0x01)	/* Stale Data */
> +
> +#define LIMIT_MAX 0
> +#define LIMIT_MIN 1
> +
> +enum hs300x_chips {
> +	hs3001,
> +	hs3002,
> +	hs3003,
> +	hs3004,
> +};
> +
> +struct hs300x_accurancy {
> +	u32 hum_acc;		/* accurancy of humidity in *1000 % */
> +	u32 temp_acc;		/* accurancy of temperature in *1000 % */
> +};
> +
> +const struct hs300x_accurancy hs300x_accurancys[] = {

Missing statuc

> +	{ 1500, 200 },
> +	{ 1800, 200 },
> +	{ 2500, 250 },
> +	{ 3500, 300 },
> +};
> +
> +struct hs300x_data {

Don't mix declarations and definitions.

> +	struct i2c_client *client;
> +	struct mutex i2c_lock;	/* lock for sending i2c commands */

Why? I think I2C device do not need to do it.


> +	struct mutex data_lock;	/* lock for updating driver data */
> +	u32 wait_time;		/* in us */
> +	int temperature;	/* in *1000 degree */
> +	u32 humidity;		/* in *1000 % */
> +};
> +


> +
> +static ssize_t humidity1_input_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct hs300x_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client;
> +	int ret;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);

How is it possible?

> +
> +	client = data->client;
> +
> +	if (!client) {

How is this possible?

Both suggest you have some troubles in probe.

> +		dev_dbg(dev, "No valid I2C client available.\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = hs300x_measure_command(client, data);
> +	if (ret != HS300X_CMD_OK)
> +		return ret;

...

> +#endif
> +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	/* Measurement time = wake-up time + measurement time temperature
> +	 *      measurment time humidity This is currently static, because

Fix typos. Remove weird indentation after *

> +	 *      enabling programming mode is not supported, yet.
> +	 */
> +	data->wait_time = (HS300X_WAKEUP_TIME + HS300X_14BIT_RESOLUTION +
> +			   HS300X_14BIT_RESOLUTION);
> +	mutex_init(&data->i2c_lock);
> +	mutex_init(&data->data_lock);
> +
> +	if (i2c_match_id(hs300x_ids, client)->driver_data == hs3001)
> +		attribute_groups = hs3001_groups;
> +	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3002)
> +		attribute_groups = hs3002_groups;
> +	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3003)
> +		attribute_groups = hs3003_groups;
> +	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3004)
> +		attribute_groups = hs3004_groups;
> +	else {
> +		dev_err(dev, "Unknwon device for HS300x driver\n");
> +		goto error;
> +	}
> +
> +	/* Measure humidity and temperature to initialize values */
> +	ret = hs300x_measure_command(client, data);
> +	if (ret) {
> +		goto error;
> +	}

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

> +
> +	/* Sensor needs some time to process measurement depending on
> +	 * resolution
> +	 */
> +	fsleep(data->wait_time);
> +
> +	ret = hs300x_data_fetch_command(client, data);
> +	if (ret != HS300X_CMD_OK) {
> +		goto error;

Please run scripts/checkpatch.pl and fix reported warnings. Some
warnings can be ignored, but the code here looks like it needs a fix.
Feel free to get in touch if the warning is not clear.

> +	}
> +
> +	hwmon_dev =

Don't wrap.

> +	    devm_hwmon_device_register_with_groups(dev, client->name, data,
> +						   attribute_groups);
> +
> +	if (IS_ERR(hwmon_dev)) {
> +		dev_err(dev, "Unable to register hwmon device %s\n",
> +			client->name);

return dev_err_probe

> +		goto error;
> +	}
> +
> +	dev_info(dev, "Successfully probe %s: sensor '%s'\n",
> +		 dev_name(hwmon_dev), client->name);

Nope, no prints like that.

> +	return PTR_ERR_OR_ZERO(hwmon_dev);

Blank line.

> +error:
> +	devm_kfree(dev, data);

You did not test it. You have double free here. If you do not believe,
please test it.

> +	return -ENODEV;
> +}
> +
> +/* device ID table */
> +static const struct i2c_device_id hs300x_ids[] = {
> +	{ "hs3001", hs3001 },
> +	{ "hs3002", hs3002 },
> +	{ "hs3003", hs3003 },
> +	{ "hs3004", hs3004 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, hs300x_ids);
> +
> +static struct i2c_driver hs300x_i2c_driver = {
> +	.driver = {
> +		   .name = "hs300x",
> +		   .of_match_table = of_match_ptr(hs300x_of_ids),

Drop of_match_ptr and all ifdefs for OF.

Best regards,
Krzysztof

Re: [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x

[Guenter Roeck]

On Tue, Jul 11, 2023 at 04:06:36PM +0200, Andre Werner wrote:
> Add base support for Renesas HS300x temperature
> and humidity sensors.
> 
> The sensors has a fix I2C address 0x44. The resolution
> is fixed to 14bit (ref. Missing feature).
> 
> Missing feature:
> - Accessing non-volatile memory: Custom board has no
>   possibility to control voltage supply of sensor. Thus,
>   we cannot send the necessary control commands within
>   the first 10ms after power-on.
> 
> Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> ---
>  Documentation/hwmon/hs300x.rst |  61 +++++
>  drivers/hwmon/Kconfig          |  10 +
>  drivers/hwmon/Makefile         |   1 +
>  drivers/hwmon/hs300x.c         | 461 +++++++++++++++++++++++++++++++++
>  4 files changed, 533 insertions(+)
>  create mode 100644 Documentation/hwmon/hs300x.rst
>  create mode 100644 drivers/hwmon/hs300x.c
> 
> diff --git a/Documentation/hwmon/hs300x.rst b/Documentation/hwmon/hs300x.rst

Drop "x" from file names and use the first supported device instead.
The driver does not support HS300{0,5...9}, and claiming that it does
can only create confusion.


> new file mode 100644
> index 000000000000..2be05d0f00ab
> --- /dev/null
> +++ b/Documentation/hwmon/hs300x.rst
> @@ -0,0 +1,61 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver HS300x
> +===================
> +
> +Supported chips:
> +
> +  * Renesas HS3001, HS3002, HS3003, HS3004
> +
> +    Prefix: 'hs300x'
> +
> +    Addresses scanned: -
> +
> +    Datasheet: https://www.renesas.com/us/en/document/dst/hs300x-datasheet?r=417401
> +
> +Author:
> +
> +  - Andre Werner <andre.werner@systec-electronic.com>
> +
> +Description
> +-----------
> +
> +This driver implements support for the Renesas HS300x chips, a humidity
> +and temperature family. Temperature is measured in degrees celsius, relative
> +humidity is expressed as a percentage. In the sysfs interface, all values are
> +scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
> +
> +The device communicates with the I2C protocol. Sensors can have the I2C
> +address 0x44 by default.

Drop "can".

> +
> +The driver does not support the sensor's configuration possibilities.
> +
> +The driver is able to be read out using lmsensors.

This sentence has no value since that is the case for all hardware monitoring
drivers.

> +
> +sysfs-Interface
> +---------------
> +
> +===============================================================================
> +temp1_input:        temperature input
> +humidity1_input:    humidity input
> +temp1_max:          temperature max value
> +humidity1_max:      humidity max value
> +temp1_min:          temperature min value
> +humidity1_min:      humidity min value
> +measuretime:        Measurement delay in usec

Use standard attribute (update_interval).
Or, rather, drop it since setting it is not supported by the driver.

> +===============================================================================
> +
> +Device Tree
> +-----------
> +
> +Required node properties:
> +
> + - compatible:  must be set to "hs3001", "hs3002", "hs3003", "hs3004"
> + - reg:         I2C address of the device (0x44)
> +
> +Example:
> +
> +    humidity: hs3002@44 {
> +        compatible = "hs3002";
> +        reg = <0x44>;
> +    };

This information does not belong into this file.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 307477b8a371..9ce82fe0044b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -734,6 +734,16 @@ config SENSORS_HIH6130
>  	  This driver can also be built as a module. If so, the module
>  	  will be called hih6130.
>  
> +config SENSORS_HS300x
> +	tristate "Renesas HS300x humidity and temperature sensors"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for the Renesas HS300x
> +	  humidity and temperature sensors.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called hs300x.
> +
>  config SENSORS_IBMAEM
>  	tristate "IBM Active Energy Manager temperature/power sensors and control"
>  	select IPMI_SI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3f4b0fda0998..a067c0896ca1 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_SENSORS_GSC)	+= gsc-hwmon.o
>  obj-$(CONFIG_SENSORS_GPIO_FAN)	+= gpio-fan.o
>  obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
>  obj-$(CONFIG_SENSORS_HIH6130)	+= hih6130.o
> +obj-$(CONFIG_SENSORS_HS300x)	+= hs300x.o
>  obj-$(CONFIG_SENSORS_ULTRA45)	+= ultra45_env.o
>  obj-$(CONFIG_SENSORS_I5500)	+= i5500_temp.o
>  obj-$(CONFIG_SENSORS_I5K_AMB)	+= i5k_amb.o
> diff --git a/drivers/hwmon/hs300x.c b/drivers/hwmon/hs300x.c
> new file mode 100644
> index 000000000000..a22cc55a01ce
> --- /dev/null
> +++ b/drivers/hwmon/hs300x.c
> @@ -0,0 +1,461 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* This is a non-complete driver implementation for the
> + * HS300x humidity and temperature sensors. It does not include
> + * the configuration possibilities, because the hardware platform the IC is
> + * used does not control the power source for the IC. Thus, it cannot being
> + * set to 'programming mode' during power-up.
> + *

Not sure what that means, but it irrelevant for the driver. Just describe
what it does.

> + *
> + * Copyright (C) 2022 SYS TEC electronic AG
> + * Author: Andre Werner <andre.werner@systec-electronic.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/of_device.h>

Alphabetic include file order, please.

> +
> +/* Measurement times */
> +#define HS300X_WAKEUP_TIME 100	/* us */
> +#define HS300X_UNUSED 0		/* us */

I can kind of understand the other unused defines below, but this one
doesn't make sense or add value. Please drop.

> +#define HS300X_8BIT_RESOLUTION 550	/* us */
> +#define HS300X_10BIT_RESOLUTION 1310	/* us */
> +#define HS300X_12BIT_RESOLUTION 4500	/* us */
> +#define HS300X_14BIT_RESOLUTION 16900	/* us */

#define<space>NAME<tab>VALUE throughout, and align VALUE

> +
> +#define HS300X_CMD_OK 0

Drop this define. Use 0 directly where needed. hs300x_measure_command()
can return a positive value if needed.

> +#define HS300X_RESPONSE_LENGTH 4
> +
> +#define HS300X_FIXPOINT_ARITH 1000
> +#define HS300X_MIN_TEMPERATURE (-40 * HS300X_FIXPOINT_ARITH) /* 1000 degree */
> +#define HS300X_MAX_TEMPERATURE (125 * HS300X_FIXPOINT_ARITH) /* 1000 degree */

1000 degree ? What is this supposed to mean ?

> +#define HS300X_MIN_HUMIDITY (0 * HS300X_FIXPOINT_ARITH) /* 1000 % */
> +#define HS300X_MAX_HUMIDITY (100 * HS300X_FIXPOINT_ARITH) /* 1000 % */

What is 1000% ?

> +
> +#define HS300X_MASK_HUMIDITY_0X3FFF (0x3FFF)
> +#define HS300X_MASK_TEMPERATURE_0XFFFC (0xFFFC)
> +#define HS300X_MASK_STATUS_0XC0 (0xC0)
> +#define HS300X_STATUS_SHIFT (6)

Unnecessary ( ) around constants

> +
> +/* Definitions for Status Bits of A/D Data */
> +#define HS300X_DATA_VALID (0x00)	/* Valid Data */
> +#define HS300X_DATA_STALE (0x01)	/* Stale Data */
> +
> +#define LIMIT_MAX 0
> +#define LIMIT_MIN 1
> +
> +enum hs300x_chips {
> +	hs3001,
> +	hs3002,
> +	hs3003,
> +	hs3004,
> +};
> +
> +struct hs300x_accurancy {

accurancy

> +	u32 hum_acc;		/* accurancy of humidity in *1000 % */
> +	u32 temp_acc;		/* accurancy of temperature in *1000 % */
> +};


> +
> +const struct hs300x_accurancy hs300x_accurancys[] = {

accuracies

> +	{ 1500, 200 },
> +	{ 1800, 200 },
> +	{ 2500, 250 },
> +	{ 3500, 300 },
> +};

The above information is not used anywhere. Please drop.

> +
> +struct hs300x_data {
> +	struct i2c_client *client;
> +	struct mutex i2c_lock;	/* lock for sending i2c commands */
> +	struct mutex data_lock;	/* lock for updating driver data */

Those locks are not really necessary.

> +	u32 wait_time;		/* in us */

That can be a constant.

> +	int temperature;	/* in *1000 degree */
> +	u32 humidity;		/* in *1000 % */
> +};
> +
> +static int hs300x_measure_command(struct i2c_client *client,
> +				  struct hs300x_data *data)
> +{
> +	int ret = 0;
> +	unsigned char buf[1] = { 0x00 };
> +
> +	if (!data || !client)
> +		return -EINVAL;
> +
Pointless check.

> +	mutex_lock(&data->i2c_lock);
> +	ret = i2c_master_send(client, (const char *)buf, 0);

Pointless typecast

> +	mutex_unlock(&data->i2c_lock);

That lock seems pointless. Please explain its value.

> +
> +	if (0 > ret)

Please refrain from Yoda programming.

> +		dev_dbg(&client->dev,
> +			"Error starting measurement. Errno: %d.\n", ret);
> +	else {
> +		ret = HS300X_CMD_OK;

> +	}
> +
> +	return ret;
> +}
> +
> +static int hs300x_extract_temperature(u16 raw)
> +{
> +	/* fixpoint arithmetic 1 digit */
> +	int temp = ((raw & HS300X_MASK_TEMPERATURE_0XFFFC) >> 2) *
> +	    HS300X_FIXPOINT_ARITH;
> +
> +	temp /= ((1 << 14) - 1);

Unnecessary outer ( ).

> + > +	return (temp * 165) - (40 * HS300X_FIXPOINT_ARITH);

Unnecessary ( )

> +}
> +
> +static u32 hs300x_extract_humidity(u16 raw)
> +{
> +	int hum = (raw & HS300X_MASK_HUMIDITY_0X3FFF) * HS300X_FIXPOINT_ARITH;
> +
> +	hum /= ((1 << 14) - 1);
> +
> +	return hum * 100;

Not that it matters much, but this calculation unnecessarily reduces
resolution to 1%.

> +}
> +
> +static int hs300x_data_fetch_command(struct i2c_client *client,
> +				     struct hs300x_data *data)
> +{
> +	int ret = HS300X_CMD_OK;
> +	u8 buf[HS300X_RESPONSE_LENGTH];
> +	u8 hs300x_status;
> +
> +	if (!client || !data)
> +		return -EINVAL;

Pointless.

> +
> +	mutex_lock(&data->i2c_lock);
> +	ret = i2c_master_recv(client, buf, HS300X_RESPONSE_LENGTH);
> +	mutex_unlock(&data->i2c_lock);
> +
> +	if (ret != HS300X_RESPONSE_LENGTH) {
> +		ret = ret < 0 ? ret : -EIO;
> +		dev_dbg(&client->dev,
> +			"Error in i2c communication. Error code: %d.\n", ret);
> +		goto out;

Please no goto to return statements.

> +	}
> +
> +	hs300x_status = (buf[0] & HS300X_MASK_STATUS_0XC0) >>
> +	    HS300X_STATUS_SHIFT;
> +	if (hs300x_status == HS300X_DATA_STALE) {
> +		dev_dbg(&client->dev, "Sensor busy.\n");
> +		ret = -EBUSY;
> +		goto out;
> +	} else if (hs300x_status != HS300X_DATA_VALID) {
> +		dev_dbg(&client->dev, "Data invalid.\n");
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	mutex_lock(&data->data_lock);
> +	data->humidity =
> +	    hs300x_extract_humidity(be16_to_cpup((__be16 *) &buf[0]));
> +	data->temperature =
> +	    hs300x_extract_temperature(be16_to_cpup((__be16 *) &buf[2]));
> +	mutex_unlock(&data->data_lock);

This lock is pointless since the caller may use the return value from a later
measurement which happened after the lock was released. Also, since the
caller does not acquire the lock prior to reading the value, data->humidity
and data->temperature could still be inconsistent.

Overall, storing humidity and temperature in data does not ad any value.
Just pass two pointers to store the returned values.

> +
> +	ret = HS300X_CMD_OK;

Just return 0; is sufficient.


> +
> +out:
> +	return ret;
> +}
> +
> +/* sysfs attributes */
> +static ssize_t measuretime_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct hs300x_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	client = data->client;
> +
> +	if (!client)
> +		return -ENODEV;

All those checks are pointless.

> +
> +	return sprintf(buf, "%d\n", data->wait_time);
> +}
> +
> +static ssize_t temp1_input_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct hs300x_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client;
> +	int ret;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	client = data->client;
> +
> +	if (!client) {
> +		dev_dbg(dev, "No valid I2C client available.\n");
> +		return -ENODEV;
> +	}

Really ? So how comes the driver is instantiated on one ?

> +
> +	ret = hs300x_measure_command(client, data);
> +	if (ret != HS300X_CMD_OK)
> +		return ret;

	if (ret < 0)

> +
> +	/* Sensor needs some time to process measurement depending on
> +	 * resolution
> +	 */
> +	fsleep(data->wait_time);
> +
> +	ret = hs300x_data_fetch_command(client, data);
> +	if (ret != HS300X_CMD_OK)
> +		return ret;

ret < 0

> +
> +	return sprintf(buf, "%d\n", data->temperature);
> +}
> +
> +static ssize_t humidity1_input_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct hs300x_data *data = dev_get_drvdata(dev);
> +	struct i2c_client *client;
> +	int ret;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	client = data->client;
> +
> +	if (!client) {
> +		dev_dbg(dev, "No valid I2C client available.\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = hs300x_measure_command(client, data);
> +	if (ret != HS300X_CMD_OK)
> +		return ret;
> +
> +	/* Sensor needs some time to process measurement depending on
> +	 * resolution
> +	 */
> +	fsleep(data->wait_time);
> +
> +	ret = hs300x_data_fetch_command(client, data);
> +	if (ret != HS300X_CMD_OK)

ret < 0 (everywhere)

> +		return ret;
> +
> +	return sprintf(buf, "%u\n", data->humidity);
> +}
> +
> +static ssize_t temp1_limit_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	int temperature_limit;
> +	u8 index;
> +
> +	index = to_sensor_dev_attr(attr)->index;
> +	if (LIMIT_MIN == index)
> +		temperature_limit = HS300X_MIN_TEMPERATURE;
> +	else
> +		temperature_limit = HS300X_MAX_TEMPERATURE;
> +

Drop those attributes. They are pointless if not supported by hardware.

> +	return scnprintf(buf, PAGE_SIZE, "%d\n", temperature_limit);
> +}
> +
> +static ssize_t humidity1_limit_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	u32 humidity_limit;
> +	u8 index;
> +
> +	index = to_sensor_dev_attr(attr)->index;
> +	if (LIMIT_MIN == index)
> +		humidity_limit = HS300X_MIN_HUMIDITY;
> +	else
> +		humidity_limit = HS300X_MAX_HUMIDITY;
> +

Same as above. If the hardware does not support setting and reading limits,
don't show them.

> +	return scnprintf(buf, PAGE_SIZE, "%u\n", humidity_limit);
> +}
> +
> +static SENSOR_DEVICE_ATTR_RO(temp1_input, temp1_input, 0);
> +static SENSOR_DEVICE_ATTR_RO(humidity1_input, humidity1_input, 0);
> +static SENSOR_DEVICE_ATTR_RO(measuretime, measuretime, 0);
> +static SENSOR_DEVICE_ATTR_RO(temp1_max, temp1_limit, LIMIT_MAX);
> +static SENSOR_DEVICE_ATTR_RO(humidity1_max, humidity1_limit, LIMIT_MAX);
> +static SENSOR_DEVICE_ATTR_RO(temp1_min, temp1_limit, LIMIT_MIN);
> +static SENSOR_DEVICE_ATTR_RO(humidity1_min, humidity1_limit, LIMIT_MIN);
> +
> +static struct attribute *hs3001_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	&sensor_dev_attr_measuretime.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_min.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *hs3002_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	&sensor_dev_attr_measuretime.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_min.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *hs3003_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	&sensor_dev_attr_measuretime.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_min.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *hs3004_attrs[] = {
> +	&sensor_dev_attr_temp1_input.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_input.dev_attr.attr,
> +	&sensor_dev_attr_measuretime.dev_attr.attr,
> +	&sensor_dev_attr_temp1_max.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_max.dev_attr.attr,
> +	&sensor_dev_attr_temp1_min.dev_attr.attr,
> +	&sensor_dev_attr_humidity1_min.dev_attr.attr,
> +	NULL
> +};
> +
> +ATTRIBUTE_GROUPS(hs3001);
> +ATTRIBUTE_GROUPS(hs3002);
> +ATTRIBUTE_GROUPS(hs3003);
> +ATTRIBUTE_GROUPS(hs3004);
> +
> +static const struct i2c_device_id hs300x_ids[];
> +#ifdef CONFIG_OF
> +static const struct of_device_id hs300x_of_ids[] = {
> +	{.compatible = "hs3001" },
> +	{.compatible = "hs3002" },
> +	{.compatible = "hs3003" },
> +	{.compatible = "hs3004" },

This should be something like "renesas,hs3001" etc.

> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, hs300x_of_ids);
> +#endif
> +
> +static int hs300x_probe(struct i2c_client *client)
> +{
> +	struct hs300x_data *data;
> +	struct device *hwmon_dev;
> +	struct i2c_adapter *adap = client->adapter;
> +	struct device *dev = &client->dev;
> +	const struct attribute_group **attribute_groups;
> +	int ret;
> +#ifdef CONFIG_OF
> +	const struct of_device_id *of_match;
> +
> +	of_match = of_match_device(hs300x_of_ids, dev);
> +	if (!of_match) {
> +		dev_err(dev, "No match in DT compatibles.\n");
> +		return -ENODEV;
> +	}

This is unnecessary since there is no difference between devices,
and the driver won't be instantiated if there is no match.

> +
> +#endif
> +	if (!i2c_check_functionality(adap, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->client = client;
> +	/* Measurement time = wake-up time + measurement time temperature

Standard multi-line comment, please.

> +	 *      measurment time humidity This is currently static, because

measurement. "." missing.


> +	 *      enabling programming mode is not supported, yet.
> +	 */

Then drop the attribute and add it if/when setting it is suppported.

> +	data->wait_time = (HS300X_WAKEUP_TIME + HS300X_14BIT_RESOLUTION +
> +			   HS300X_14BIT_RESOLUTION);
> +	mutex_init(&data->i2c_lock);
> +	mutex_init(&data->data_lock);
> +
> +	if (i2c_match_id(hs300x_ids, client)->driver_data == hs3001)
> +		attribute_groups = hs3001_groups;
> +	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3002)
> +		attribute_groups = hs3002_groups;
> +	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3003)
> +		attribute_groups = hs3003_groups;
> +	else if (i2c_match_id(hs300x_ids, client)->driver_data == hs3004)
> +		attribute_groups = hs3004_groups;

All this should be handled with the is_visible() callback.

Having said this, looking at the attributes again ... they are all the
same. Either I am missing something ot that is completely pointless.

Besides, HS3002 and HS3004 were removed from the datasheet. Do those
chips even exist ?

> +	else {
> +		dev_err(dev, "Unknwon device for HS300x driver\n");
> +		goto error;
> +	}
> +
> +	/* Measure humidity and temperature to initialize values */

What for ? That unnecessarily delays booting significantly.

> +	ret = hs300x_measure_command(client, data);
> +	if (ret) {
> +		goto error;
> +	}

Please run checkpatch --strict on your patches.

> +
> +	/* Sensor needs some time to process measurement depending on
> +	 * resolution
> +	 */
> +	fsleep(data->wait_time);
> +
> +	ret = hs300x_data_fetch_command(client, data);
> +	if (ret != HS300X_CMD_OK) {
> +		goto error;
> +	}
> +
> +	hwmon_dev =
> +	    devm_hwmon_device_register_with_groups(dev, client->name, data,
> +						   attribute_groups);

From Documentation/hwmon/hwmon-kernel-api.rst (with emphasis added):

devm_hwmon_device_register_with_info is similar to
hwmon_device_register_with_info. However, it is device managed, meaning the
hwmon device does not have to be removed explicitly by the removal function.

vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
All other hardware monitoring device registration functions are deprecated
and must not be used in new drivers.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

> +
> +	if (IS_ERR(hwmon_dev)) {
> +		dev_err(dev, "Unable to register hwmon device %s\n",
> +			client->name);
> +		goto error;

		return PTR_ERR(hwmon_dev);

> +	}
> +
> +	dev_info(dev, "Successfully probe %s: sensor '%s'\n",
> +		 dev_name(hwmon_dev), client->name);

Please no such noise.

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +error:
> +	devm_kfree(dev, data);

Pointless.

> +	return -ENODEV;
> +}
> +
> +/* device ID table */
> +static const struct i2c_device_id hs300x_ids[] = {
> +	{ "hs3001", hs3001 },
> +	{ "hs3002", hs3002 },
> +	{ "hs3003", hs3003 },
> +	{ "hs3004", hs3004 },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, hs300x_ids);
> +
> +static struct i2c_driver hs300x_i2c_driver = {
> +	.driver = {
> +		   .name = "hs300x",
> +		   .of_match_table = of_match_ptr(hs300x_of_ids),

Drop 
> +		    },
> +	.probe_new = hs300x_probe,
> +	.id_table = hs300x_ids,
> +};
> +
> +module_i2c_driver(hs300x_i2c_driver);
> +
> +MODULE_AUTHOR("Andre Werner <andre.werner@systec-electronic.com>");
> +MODULE_DESCRIPTION("HS300x humidity and temperature sensor base  driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.41.0
> 

Re: [PATCH 2/2] dt-bindings: hwmon: Add description for new hwmon driver hs300x

[Guenter Roeck]

On Tue, Jul 11, 2023 at 06:28:11PM +0200, Krzysztof Kozlowski wrote:
> On 11/07/2023 16:06, Andre Werner wrote:
> > This is the initial description.
> > 
> > Signed-off-by: Andre Werner <andre.werner@systec-electronic.com>
> > ---
> >  .../devicetree/bindings/hwmon/hs300x.yaml     | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/hs300x.yaml
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least DT list (maybe more), so this won't be tested by
> automated tooling. Performing review on untested code might be a waste
> of time, thus I will skip this patch entirely till you follow the
> process allowing the patch to be tested.
> 
> Please kindly resend and include all necessary To/Cc entries.
> 

The chips should just be added to trivial-devices.yaml.

> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/hs300x.yaml b/Documentation/devicetree/bindings/hwmon/hs300x.yaml
> > new file mode 100644
> > index 000000000000..7a9b3ee3738b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/hs300x.yaml
> 
> Filename like compatible
> 
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +
> 
> Drop
> 
> > +$id: http://devicetree.org/schemas/hwmon/hs300x.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas HS300x humidity and temperature sensor
> > +
> > +maintainers:
> > +  - Andre Werner (andre.werner@systec-electronic.com)
> > +
> > +description: |
> > +  This driver implements support for the Renesas HS300x chips, a humidity
> 
> Bindings are for hardware, not drivers.
> 
> > +  and temperature family. Temperature is measured in degrees celsius, relative
> > +  humidity is expressed as a percentage. In the sysfs interface, all values are
> > +  scaled by 1000, i.e. the value for 31.5 degrees celsius is 31500.
> 
> sysfs is not hardware related, drop.
> 
> > +
> > +  The device communicates with the I2C protocol. Sensors can have the I2C
> > +  address 0x44 by default.
> > +
> > +  The driver does not support the sensor's configuration possibilities.
> > +
> > +  The driver is able to be read out using lmsensors.
> > +
> > +  Datasheets:
> > +  https://www.renesas.com/us/en/document/dst/hs300x-datasheet?r=417401
> > +
> > +
> 
> Just one blank line.
> 
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - hs3001
> > +      - hs3002
> > +      - hs3003
> > +      - hs3004
> 
> Eh, so this was not tested... That's not correct compatible, missing
> vendor prefix.
> 

Also, from programming perspective, the chips are all the same.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> 
> Missing several properties.
> 
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +          #address-cells = <1>;
> > +          #size-cells = <0>;
> > +
> > +          humidity: hs3002@44 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> 
> > +              compatible = "hs3002";
> > +              reg = <0x44>;
> > +          };
> > +    };
> 
> Best regards,
> Krzysztof
>