On 18/07/2023 09:01, Andre Werner wrote:
> Add base support for Renesas HS3001 temperature
> and humidity sensors and its compatibles HS3002,
> HS3003 and HS3004.
>
> The sensor 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@xxxxxxxxxxxxxxxxxxxxx>
>
> Changelog:
> v1: Initial version
> v2: Extensive refactoring following recommendations of reviewers:
> - Delete unused defines and device properties. These are added in
> the initial version because the device supports a programming mode,
> but I was not able to implement it, because the custom board was
> not able to control the power supply of the device and so I cannot
> enter the programming mode of the device.
> - Correct missunderstanding comments for defines.
> - Delete mutexes for data and I2C bus accesses.
> - Replace attributes with recommented chip-info structure. In the
> initial version I followed the sth3x.c implementation that uses
> files and attributes in sysfs. The show functions are replaced by
> is_visible and read callbacks from the HWMON ABI. I also delete pointless
> function argument checks.
> - Correct Yoda programming.
> - Refactor probe function and delete sleep and measurement of humidity
> and temperature in probe function. I kept an initial I2C
> communication to ensure that the device is accessible during probe.
> - Reduce the number of atteributes to humidity and temperature input.
Also wrong placement of SoB and changelog.
> ---
> Documentation/hwmon/hs3001.rst | 37 +++++
> MAINTAINERS | 6 +
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/hs3001.c | 261 +++++++++++++++++++++++++++++++++
> 5 files changed, 315 insertions(+)
> create mode 100644 Documentation/hwmon/hs3001.rst
> create mode 100644 drivers/hwmon/hs3001.c
...
> +/* Definitions for Status Bits of A/D Data */
> +#define HS3001_DATA_VALID 0x00 /* Valid Data */
> +#define HS3001_DATA_STALE 0x01 /* Stale Data */
> +
> +#define LIMIT_MAX 0
> +#define LIMIT_MIN 1
> +
> +enum hs3001_chips {
> + hs3001,
Drop, not effectively used.
> +};
...
> +
> +/* device ID table */
> +static const struct i2c_device_id hs3001_ids[] = {
> + { "hs3001", hs3001 },
Drop match data
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, hs3001_ids);
> +
> +static const struct of_device_id hs3001_of_match[] = {
> + {.compatible = "renesas,hs3001",
> + .data = (void *)hs3001
Drop
> + },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(of, hs3001_of_match);
> +
> +static int hs3001_probe(struct i2c_client *client)
> +{
> + struct hs3001_data *data;
> + struct device *hwmon_dev;
> + struct device *dev = &client->dev;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EOPNOTSUPP;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> +
> + if (client->dev.of_node)
> + data->type = (enum hs3001_chips)of_device_get_match_data(&client->dev);
> + else
> + data->type = i2c_match_id(hs3001_ids, client)->driver_data;
This is useless and dead code. You have only one type of device. Don't
over-complicate simple things.
Best regards,
Krzysztof
