| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 7 Aug 2023 21:24:21 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>,
Jean Delvare <jdelvare@...e.com>
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
Mathew McBride <matt@...verse.com.au>
Subject: Re: [PATCH 1/2] hwmon: Add driver for EMC181x temperature sensors
On 8/7/23 18:31, Mark Tomlinson wrote:
> This patch adds a HWMON driver for the EMC1812, EMC1813, EMC1814,
> EMC1815 and EMC1833 temperature sensor chips from microchip. Does not
Microchip
> currently support the alert outputs.
>
It doesn't support limit registers either. Odd, but I guess it is better to have
some driver support than nothing.
checkpatch --strict reports a couple of problems. Please fix.
> Signed-off-by: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>
> Co-developed-by: Mathew McBride <matt@...verse.com.au>
> Signed-off-by: Mathew McBride <matt@...verse.com.au>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/emc181x.c | 296 ++++++++++++++++++++++++++++++++++++++++
Documentation missing.
Also, please no wildcards in file names. EMC1833 doesn't even fit the pattern,
and the driver doesn't support ENC18[01] or EMC181[6-9]. Use one of the chips
as name. The datasheet says "EMC1812 family", which just as well works here.
> 3 files changed, 307 insertions(+)
> create mode 100644 drivers/hwmon/emc181x.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 307477b8a371..196d91494536 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1820,6 +1820,16 @@ config SENSORS_EMC1403
> Threshold values can be configured using sysfs.
> Data from the different diodes are accessible via sysfs.
>
> +config SENSORS_EMC181X
> + tristate "SMSC EMC181X thermal sensor"
Again, no wildcards here. Also, s/SMSC/Microchip/
> + depends on I2C
> + select REGMAP_I2C
> + help
> + If you say yes here you get support for the SMSC EMC181X
> + temperature monitoring chip.
EMC181X does not exist. List the supported chips.
> +
> + Data from the different diodes are accessible via sysfs.
That has no value.
> +
> config SENSORS_EMC2103
> tristate "SMSC EMC2103"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3f4b0fda0998..bcea887dfe17 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -69,6 +69,7 @@ obj-$(CONFIG_SENSORS_DRIVETEMP) += drivetemp.o
> obj-$(CONFIG_SENSORS_DS620) += ds620.o
> obj-$(CONFIG_SENSORS_DS1621) += ds1621.o
> obj-$(CONFIG_SENSORS_EMC1403) += emc1403.o
> +obj-$(CONFIG_SENSORS_EMC181X) += emc181x.o
> obj-$(CONFIG_SENSORS_EMC2103) += emc2103.o
> obj-$(CONFIG_SENSORS_EMC2305) += emc2305.o
> obj-$(CONFIG_SENSORS_EMC6W201) += emc6w201.o
> diff --git a/drivers/hwmon/emc181x.c b/drivers/hwmon/emc181x.c
> new file mode 100644
> index 000000000000..364d2bfb15ca
> --- /dev/null
> +++ b/drivers/hwmon/emc181x.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Microchip/SMSC EMC181x Temperature monitor
> + *
> + * Copyright (C) 2018-2019 Traverse Technologies
> + * Author: Mathew McBride <matt@...verse.com.au>
> + *
> + * Copyright (C) 2023 Allied Telesis Labs
> + * Author: Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>
> + *
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Not needed
> +#include <linux/of_device.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
Alphabetic order
> +
> +#define EMC1812_ID 0x81
> +#define EMC1813_ID 0x87
> +#define EMC1814_ID 0x84
> +#define EMC1815_ID 0x85
> +#define EMC1833_ID 0x83
> +
> +#define MICROCHIP_ID 0x54
> +
> +#define EMC181X_STATUS_REG 0x02
> +#define EMC181X_CONFIG_REG 0x03
> +#define EMC181X_DIODE_DATA_BASE 0x60
> +#define EMC181X_PID_REG 0xfd
> +#define EMC181X_SMSC_ID_REG 0xfe
> +#define EMC181X_REVISION_REG 0xff
Some of the above are not used. Drop unused definitions.
> +
> +/* Adjustable address type is 0x7c, 0x5c, 0x4c, 0x6c, 0x1c, 0x3c */
> +/* Fixed address is either 0x4c or 0x45 */
Irrelevant here.
> +static const unsigned short emc181x_address_list[] = {
> + 0x7c, 0x5c, 0x4c, 0x6c, 0x1c, 0x3c, 0x45, I2C_CLIENT_END
Numeric order, please.
> +};
> +
> +enum chips {
> + emc1812, emc1813,
> + emc1814, emc1815,
> + emc1833,
One line is enough.
> +};
> +
> +struct emc181x_data {
> + struct i2c_client *i2c;
> + enum chips type;
> + bool is_extended_range;
> +};
> +
> +static int emc181x_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct emc181x_data *data = dev_get_drvdata(dev);
> + struct i2c_client *i2c = data->i2c;
> +
Unnecessary empty line.
> + u8 channel_reg = 0;
> + s32 channel_deg = 0;
> + s32 channel_frac = 0;
No unnecessary initializations, please.
> +
> + if (type != hwmon_temp)
> + return -EOPNOTSUPP;
> + if (channel > 4)
> + return -EINVAL;
> +
> + channel_reg = EMC181X_DIODE_DATA_BASE + (channel * 0x02);
> + channel_deg = i2c_smbus_read_byte_data(i2c, channel_reg);
> + if (channel_deg < 0)
> + return channel_deg;
> + if (data->is_extended_range)
> + channel_deg -= 64;
> +
> + channel_frac = i2c_smbus_read_byte_data(i2c, channel_reg + 0x01);
> + if (channel_frac < 0)
> + return channel_frac;
> +
The above either needs to be mutex protected, or i2c_smbus_read_word_data()
needs to be used.
> + *val = ((channel_deg << 3) | (channel_frac >> 5)) * 125;
> + return 0;
> +}
> +
> +static umode_t emc181x_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type == hwmon_temp && attr == hwmon_temp_input)
> + return 0444;
> + else
else after return is pointless.
> + return 0;
> +}
> +
> +static const struct hwmon_channel_info *emc1812_info[] = {
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT, HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_channel_info *emc1813_info[] = {
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_channel_info *emc1814_info[] = {
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT,
> + HWMON_T_INPUT),
> + NULL
> +};
> +
> +static const struct hwmon_channel_info *emc1815_info[] = {
> + HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
> + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT, HWMON_T_INPUT, HWMON_T_INPUT,
> + HWMON_T_INPUT, HWMON_T_INPUT),
> + NULL
> +};
Only one of those is needed. The others should be handled with
the is_visible() function.
> +
> +static const struct hwmon_ops emc181x_ops = {
> + .is_visible = emc181x_is_visible,
> + .read = emc181x_read,
> +};
> +
> +static const struct hwmon_chip_info emc181x_chip_info[] = {
> + [emc1812] = {
> + .ops = &emc181x_ops,
> + .info = emc1812_info,
> + },
> + [emc1813] = {
> + .ops = &emc181x_ops,
> + .info = emc1813_info,
> + },
> + [emc1814] = {
> + .ops = &emc181x_ops,
> + .info = emc1814_info,
> + },
> + [emc1815] = {
> + .ops = &emc181x_ops,
> + .info = emc1815_info,
> + },
> + [emc1833] = {
> + .ops = &emc181x_ops,
> + .info = emc1813_info,
> + },
> +};
> +
> +static const struct i2c_device_id emc181x_i2c_id[] = {
> + { "emc1812", emc1812 },
> + { "emc1813", emc1813 },
> + { "emc1814", emc1814 },
> + { "emc1815", emc1815 },
> + { "emc1833", emc1833 },
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, emc181x_i2c_id);
> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
... which is violated below.
> +static int emc181x_i2c_detect(struct i2c_client *client,
> + struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = client->adapter;
> + int pid, id, rev;
> + const char *name;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA))
While the driver should use I2C_FUNC_SMBUS_WORD_DATA, it currently doesn't.
> + return -ENODEV;
> +
> + /* Determine the chip type */
Unnecessary comment
> + id = i2c_smbus_read_byte_data(client, EMC181X_SMSC_ID_REG);
> + if (id < 0)
> + return id;
-ENODEV should be returned here; if this is not an expected chip there
may be no register at this address.
> + if (id != MICROCHIP_ID)
> + return -ENODEV;
> + pid = i2c_smbus_read_byte_data(client, EMC181X_PID_REG);
> + if (pid < 0)
> + return pid;
> + rev = i2c_smbus_read_byte_data(client, EMC181X_REVISION_REG);
> + if (rev < 0)
> + return rev;
> +
> + switch (pid) {
> + case EMC1812_ID:
> + name = emc181x_i2c_id[emc1812].name;
> + break;
> + case EMC1813_ID:
> + name = emc181x_i2c_id[emc1813].name;
> + break;
> + case EMC1814_ID:
> + name = emc181x_i2c_id[emc1814].name;
> + break;
> + case EMC1815_ID:
> + name = emc181x_i2c_id[emc1815].name;
> + break;
> + case EMC1833_ID:
> + name = emc181x_i2c_id[emc1833].name;
> + break;
> + default:
> + return -ENODEV;
> + }
> + strscpy(info->type, name, I2C_NAME_SIZE);
> +
> + dev_dbg(&client->dev,
> + "Detected device %s at 0x%02x with COMPANY: 0x%02x and PID: 0x%02x REV: 0x%02x\n",
> + name, client->addr, id, pid, rev);
COMPANY is pretty pointless here.
> +
> + return 0;
> +}
> +
> +static int emc181x_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct device *hwmon_dev;
> + struct emc181x_data *data;
> + s32 regval;
> + u8 config;
> +
> + data = devm_kzalloc(dev, sizeof(struct emc181x_data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->i2c = client;
> + if (dev_fwnode(dev)) {
> + data->type = (enum chips)device_get_match_data(dev);
> + data->is_extended_range = device_property_present(dev, "emc181x,extended-range");
> + } else
> + data->type = i2c_match_id(emc181x_i2c_id, client)->driver_data;
> +
> + regval = i2c_smbus_read_byte_data(client, EMC181X_CONFIG_REG);
> + if (regval < 0) {
> + dev_dbg(dev, "Failed to read config reg %d", regval);
> + return regval;
> + }
> +
> + /* By default, extended range is disabled in the EMC181X,
> + * if enabled we need to set this in the CONFIG register.
> + */
This is not the networking subsystem. Please use standard multi-line comments.
> + config = regval & ~0x04;
> + if (data->is_extended_range)
> + config |= 0x04;
> +
> + dev_dbg(dev, "EMC181X setting CONFIG to %d\n", config);
> + if (config != regval)
> + i2c_smbus_write_byte_data(client, EMC181X_CONFIG_REG, config);
Error handling missing.
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev,
> + client->name,
> + data,
> + &emc181x_chip_info[data->type],
> + NULL);
Limit the number of continuation lines to what is necessary.
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id __maybe_unused emc181x_of_match[] = {
Drop __maybe_unused
> + {
> + .compatible = "microchip,emc1812",
> + .data = (void *)emc1812
> + },
> + {
> + .compatible = "microchip,emc1813",
> + .data = (void *)emc1813
> + },
> + {
> + .compatible = "microchip,emc1814",
> + .data = (void *)emc1814
> + },
> + {
> + .compatible = "microchip,emc1815",
> + .data = (void *)emc1815
> + },
> + {
> + .compatible = "microchip,emc1833",
> + .data = (void *)emc1833
> + },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, emc181x_of_match);
> +
> +static struct i2c_driver emc181x_i2c_driver = {
> + .driver = {
> + .name = "emc181x",
> + .of_match_table = of_match_ptr(emc181x_of_match),
Drop of_match_ptr
> + },
> + .probe = emc181x_i2c_probe,
> + .id_table = emc181x_i2c_id,
> + .detect = emc181x_i2c_detect,
> + .address_list = emc181x_address_list,
> +};
> +
> +module_i2c_driver(emc181x_i2c_driver);
> +
> +MODULE_DESCRIPTION("EMC181X Sensor Driver");
> +MODULE_AUTHOR("Mark Tomlinson <mark.tomlinson@...iedtelesis.co.nz>");
> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists