* Re: [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x
2023-07-11 14:06 [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x Andre Werner
2023-07-11 14:06 ` [PATCH 2/2] dt-bindings: hwmon: Add description for new hwmon driver hs300x Andre Werner
@ 2023-07-11 16:34 ` Krzysztof Kozlowski
2023-07-11 17:38 ` Guenter Roeck
2 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-07-11 16:34 UTC (permalink / raw)
To: Andre Werner, jdelvare; +Cc: linux-hwmon, Andre Werner
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x
2023-07-11 14:06 [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x Andre Werner
2023-07-11 14:06 ` [PATCH 2/2] dt-bindings: hwmon: Add description for new hwmon driver hs300x Andre Werner
2023-07-11 16:34 ` [PATCH 1/2] hwmon: (hs300x) Add driver for Renesas HS300x Krzysztof Kozlowski
@ 2023-07-11 17:38 ` Guenter Roeck
2 siblings, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2023-07-11 17:38 UTC (permalink / raw)
To: Andre Werner; +Cc: jdelvare, linux-hwmon, Andre Werner
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread