This patch adds a HWMON driver for the EMC1812, EMC1813, EMC1814,
EMC1815 and EMC1833 temperature sensor chips from microchip. Does not
currently support the alert outputs.
Signed-off-by: Mark Tomlinson <[email protected]>
Co-developed-by: Mathew McBride <[email protected]>
Signed-off-by: Mathew McBride <[email protected]>
---
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/emc181x.c | 296 ++++++++++++++++++++++++++++++++++++++++
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"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ If you say yes here you get support for the SMSC EMC181X
+ temperature monitoring chip.
+
+ Data from the different diodes are accessible via sysfs.
+
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 <[email protected]>
+ *
+ * Copyright (C) 2023 Allied Telesis Labs
+ * Author: Mark Tomlinson <[email protected]>
+ *
+ */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/of_device.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+
+#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
+
+/* Adjustable address type is 0x7c, 0x5c, 0x4c, 0x6c, 0x1c, 0x3c */
+/* Fixed address is either 0x4c or 0x45 */
+static const unsigned short emc181x_address_list[] = {
+ 0x7c, 0x5c, 0x4c, 0x6c, 0x1c, 0x3c, 0x45, I2C_CLIENT_END
+};
+
+enum chips {
+ emc1812, emc1813,
+ emc1814, emc1815,
+ emc1833,
+};
+
+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;
+
+ u8 channel_reg = 0;
+ s32 channel_deg = 0;
+ s32 channel_frac = 0;
+
+ 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;
+
+ *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
+ 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
+};
+
+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 */
+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))
+ return -ENODEV;
+
+ /* Determine the chip type */
+ id = i2c_smbus_read_byte_data(client, EMC181X_SMSC_ID_REG);
+ if (id < 0)
+ return id;
+ 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);
+
+ 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.
+ */
+ 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);
+
+ hwmon_dev = devm_hwmon_device_register_with_info(dev,
+ client->name,
+ data,
+ &emc181x_chip_info[data->type],
+ NULL);
+
+ return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id __maybe_unused emc181x_of_match[] = {
+ {
+ .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),
+ },
+ .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 <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.41.0
The EMC181x range are I2C temperature sensors with a varying number of
sensors in each device. Each has one internal sensor, and one to four
external sensor diodes.
The default range is from 0°C to +127°C, but can be extended to -64°C to
+191°C.
Signed-off-by: Mark Tomlinson <[email protected]>
---
.../bindings/hwmon/microchip,emc181x.yaml | 45 +++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
new file mode 100644
index 000000000000..5967f98ad7ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/microchip,emc181x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip EMC1812/EMC1813/EMC1814/EMC1815/EMC1833 temperature sensors
+
+maintainers:
+ - Mark Tomlinson <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - microchip,emc1812
+ - microchip,emc1813
+ - microchip,emc1814
+ - microchip,emc1815
+ - microchip,emc1833
+
+ reg:
+ maxItems: 1
+
+ emc181x,extended-range:
+ description: Allow for reading of extended temperature range (-64-192C)
+
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ temperature-sensor@7c {
+ compatible = "microchip,emc1812";
+ reg = <0x7c>;
+ };
+ };
--
2.41.0
On 08/08/2023 03:31, Mark Tomlinson wrote:
> The EMC181x range are I2C temperature sensors with a varying number of
> sensors in each device. Each has one internal sensor, and one to four
> external sensor diodes.
>
> The default range is from 0°C to +127°C, but can be extended to -64°C to
> +191°C.
>
> Signed-off-by: Mark Tomlinson <[email protected]>
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.
Limited review follows:
> ---
> .../bindings/hwmon/microchip,emc181x.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
> new file mode 100644
> index 000000000000..5967f98ad7ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
Drop blank line.
> +$id: http://devicetree.org/schemas/hwmon/microchip,emc181x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip EMC1812/EMC1813/EMC1814/EMC1815/EMC1833 temperature sensors
> +
> +maintainers:
> + - Mark Tomlinson <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,emc1812
> + - microchip,emc1813
> + - microchip,emc1814
> + - microchip,emc1815
> + - microchip,emc1833
> +
> + reg:
> + maxItems: 1
> +
> + emc181x,extended-range:
Incorrect vendor prefix.
> + description: Allow for reading of extended temperature range (-64-192C)
And why would we disallow it otherwise?
Missing type... so this wasn't even tested.
> +
> +
Just one blank line.
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + temperature-sensor@7c {
> + compatible = "microchip,emc1812";
> + reg = <0x7c>;
Include optional properties in the example as well.
Best regards,
Krzysztof
On 08/08/2023 03: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
> currently support the alert outputs.
>
> Signed-off-by: Mark Tomlinson <[email protected]>
> Co-developed-by: Mathew McBride <[email protected]>
> Signed-off-by: Mathew McBride <[email protected]>
The order of these tags is clearly not correct. It says Mathew is
sending, but you are not Mathew?
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/emc181x.c | 296 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 307 insertions(+)
> create mode 100644 drivers/hwmon/emc181x.c
...
> +
> +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);
Keep the entire table next to OF device ID table.
> +
> +/* Return 0 if detection is successful, -ENODEV otherwise */
> +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))
> + return -ENODEV;
> +
> + /* Determine the chip type */
> + id = i2c_smbus_read_byte_data(client, EMC181X_SMSC_ID_REG);
> + if (id < 0)
> + return id;
> + 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);
If you have detection, why not using it for device matching? All devices
should be OF-compatible with one fallback.
> +
> + 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);
sizeof(*)
> + 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;
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.
> +
> + 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.
> + */
> + 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);
> +
> + hwmon_dev = devm_hwmon_device_register_with_info(dev,
> + client->name,
> + data,
> + &emc181x_chip_info[data->type],
> + NULL);
> +
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id __maybe_unused emc181x_of_match[] = {
> + {
> + .compatible = "microchip,emc1812",
Bindings go before the user.
> + .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(). This will cause warnings.
> + .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 <[email protected]>");
> +MODULE_LICENSE("GPL");
Best regards,
Krzysztof
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 <[email protected]>
> Co-developed-by: Mathew McBride <[email protected]>
> Signed-off-by: Mathew McBride <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * Copyright (C) 2023 Allied Telesis Labs
> + * Author: Mark Tomlinson <[email protected]>
> + *
> + */
> +
> +#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 <[email protected]>");
> +MODULE_LICENSE("GPL");
On 8/7/23 18:31, Mark Tomlinson wrote:
> The EMC181x range are I2C temperature sensors with a varying number of
> sensors in each device. Each has one internal sensor, and one to four
> external sensor diodes.
>
> The default range is from 0°C to +127°C, but can be extended to -64°C to
> +191°C.
>
> Signed-off-by: Mark Tomlinson <[email protected]>
> ---
> .../bindings/hwmon/microchip,emc181x.yaml | 45 +++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml b/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
> new file mode 100644
> index 000000000000..5967f98ad7ba
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/microchip,emc181x.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/microchip,emc181x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip EMC1812/EMC1813/EMC1814/EMC1815/EMC1833 temperature sensors
> +
> +maintainers:
> + - Mark Tomlinson <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - microchip,emc1812
> + - microchip,emc1813
> + - microchip,emc1814
> + - microchip,emc1815
> + - microchip,emc1833
> +
> + reg:
> + maxItems: 1
> +
> + emc181x,extended-range:
> + description: Allow for reading of extended temperature range (-64-192C)
> +
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
The chip has various other configuration options. I would have expected to see
at least beta compensation, ideality factor, resistance error correction,
and anti-parallel diode operation.
Yes, I understand you probably don't plan to implement those in the driver,
but the devicetree property description should at least try to be complete.
Guenter
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + temperature-sensor@7c {
> + compatible = "microchip,emc1812";
> + reg = <0x7c>;
> + };
> + };
Hi Krzysztof,
On Tue, Aug 8, 2023, at 5:38 PM, Krzysztof Kozlowski wrote:
> On 08/08/2023 03: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
> > currently support the alert outputs.
> >
> > Signed-off-by: Mark Tomlinson <[email protected]>
> > Co-developed-by: Mathew McBride <[email protected]>
> > Signed-off-by: Mathew McBride <[email protected]>
>
> The order of these tags is clearly not correct. It says Mathew is
> sending, but you are not Mathew?
Just to clarify, Mark has developed this version based off something I wrote a while ago:
https://gitlab.com/traversetech/ls1088firmware/traverse-sensors/-/blob/master/emc181x/emc181x.c?ref_type=heads
Hence my copyright is listed in the header.
I advised him to list the authors in that order following the example in the kernel's "submitting patches" guide [2] which provides an example of such a situation:
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
If I interpreted the guidelines incorrectly, happy to be corrected.
Best Regards,
Matt
On 08/08/2023 09:54, Mathew McBride wrote:
> Hi Krzysztof,
>
> On Tue, Aug 8, 2023, at 5:38 PM, Krzysztof Kozlowski wrote:
>> On 08/08/2023 03: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
>>> currently support the alert outputs.
>>>
>>> Signed-off-by: Mark Tomlinson <[email protected]>
>>> Co-developed-by: Mathew McBride <[email protected]>
>>> Signed-off-by: Mathew McBride <[email protected]>
>>
>> The order of these tags is clearly not correct. It says Mathew is
>> sending, but you are not Mathew?
>
> Just to clarify, Mark has developed this version based off something I wrote a while ago:
>
> https://gitlab.com/traversetech/ls1088firmware/traverse-sensors/-/blob/master/emc181x/emc181x.c?ref_type=heads
>
> Hence my copyright is listed in the header.
>
> I advised him to list the authors in that order following the example in the kernel's "submitting patches" guide [2] which provides an example of such a situation:
>
> https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
The last entry is:
Signed-off-by: From Author <[email protected]>
So it is expected to be Mark.
Best regards,
Krzysztof