[PATCH 0/2] Enable sensors support for the Congatec Board Controller

[PATCH 0/2] Enable sensors support for the Congatec Board Controller

[Thomas Richard]

The Congatec Board Controller has some voltage, current, temperature
and fan sensors.
This series adds an hwmon driver to enable the support.

This series is based on linux-next (commit
1ffec08567f426a1c593e038cadc61bdc38cb467) as the MFD driver is not yet
available in the main tree.

The sensors support has been tested on the conga-SA7 board with a
conga-SEVAL board.

Note that the Board Controller returns two unknown sensors, which causes
two warnings. These unknown sensors are not defined in the driver provided
by Congatec.

cgbc-hwmon cgbc-hwmon: Board Controller returned an unknown sensor (type=2, id=17), ignore it
cgbc-hwmon cgbc-hwmon: Board Controller returned an unknown sensor (type=1, id=10), ignore it

Best Regards,

Thomas

$ sensors
cgbc_hwmon-isa-0000
Adapter: ISA adapter
DC Runtime Voltage:    4.93 V
Chipset Temperature:  +48.0 C
Board Temperature:    +44.0 C

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
Thomas Richard (2):
      hwmon: Add Congatec Board Controller monitoring driver
      mfd: cgbc: add a hwmon cell

 MAINTAINERS                |   1 +
 drivers/hwmon/Kconfig      |   9 ++
 drivers/hwmon/Makefile     |   1 +
 drivers/hwmon/cgbc-hwmon.c | 287 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/cgbc-core.c    |   1 +
 5 files changed, 299 insertions(+)
---
base-commit: 1ffec08567f426a1c593e038cadc61bdc38cb467
change-id: 20240809-congatec-board-controller-hwmon-e9e63d957d33

Best regards,
-- 
Thomas Richard <thomas.richard@bootlin.com>

[PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

[Thomas Richard]

Add support for the Congatec Board Controller. This controller exposes
temperature, voltage, current and fan sensors.

The available sensors list cannot be predicted. Some sensors can be
present or not, depending the system.
The driver has an internal list of all possible sensors, for all Congatec
boards. The Board Controller gives to the driver its sensors list, and
their status (active or not).

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 MAINTAINERS                |   1 +
 drivers/hwmon/Kconfig      |   9 ++
 drivers/hwmon/Makefile     |   1 +
 drivers/hwmon/cgbc-hwmon.c | 287 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 298 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3507df3381b1..5e96646593b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5784,6 +5784,7 @@ CONGATEC BOARD CONTROLLER MFD DRIVER
 M:	Thomas Richard <thomas.richard@bootlin.com>
 S:	Maintained
 F:	drivers/gpio/gpio-cgbc.c
+F:	drivers/hwmon/cgbc-hwmon.c
 F:	drivers/i2c/busses/i2c-cgbc.c
 F:	drivers/mfd/cgbc-core.c
 F:	drivers/watchdog/cgbc_wdt.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index cfb4e9314c62..c7b6e93aeb9b 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -463,6 +463,15 @@ config SENSORS_BT1_PVT_ALARMS
 	  the data conversion will be periodically performed and the data will be
 	  saved in the internal driver cache.
 
+config SENSORS_CGBC
+	tristate "Congatec Board Controller Sensors"
+	depends on MFD_CGBC
+	help
+	  Enables sensors support for the Congatec Board Controller.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called cgbc-hwmon.
+
 config SENSORS_CHIPCAP2
 	tristate "Amphenol ChipCap 2 relative humidity and temperature sensor"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b827b92f2a78..318da6d8f752 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_SENSORS_ASUS_ROG_RYUJIN)	+= asus_rog_ryujin.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
 obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
 obj-$(CONFIG_SENSORS_BT1_PVT)	+= bt1-pvt.o
+obj-$(CONFIG_SENSORS_CGBC)	+= cgbc-hwmon.o
 obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
diff --git a/drivers/hwmon/cgbc-hwmon.c b/drivers/hwmon/cgbc-hwmon.c
new file mode 100644
index 000000000000..3234c7590acb
--- /dev/null
+++ b/drivers/hwmon/cgbc-hwmon.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * cgbc-hwmon - Congatec Board Controller hardware monitoring driver
+ *
+ * Copyright (C) 2024 Thomas Richard <thomas.richard@bootlin.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/hwmon.h>
+#include <linux/mfd/cgbc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define CGBC_HWMON_TYPE_TEMP	1
+#define CGBC_HWMON_TYPE_IN	2
+#define CGBC_HWMON_TYPE_FAN	3
+
+#define CGBC_HWMON_CMD_SENSOR		0x77
+#define CGBC_HWMON_CMD_SENSOR_DATA_SIZE	0x05
+
+#define CGBC_HWMON_TYPE_MASK	GENMASK(6, 5)
+#define CGBC_HWMON_ID_MASK	GENMASK(4, 0)
+#define CGBC_HWMON_ACTIVE_BIT	BIT(7)
+#define CGBC_HWMON_DATA_HIGH	GENMASK(15, 8)
+#define CGBC_HWMON_DATA_LOW	GENMASK(7, 0)
+
+struct cgbc_hwmon_sensor {
+	enum hwmon_sensor_types type;
+	bool active;
+	u8 index;
+	const char *label;
+};
+
+struct cgbc_hwmon_data {
+	struct cgbc_device_data *cgbc;
+	u8 nb_sensors;
+	struct cgbc_hwmon_sensor *sensors;
+};
+
+static const char * const cgbc_hwmon_labels_temp[] = {
+	"CPU Temperature",
+	"Box Temperature",
+	"Ambient Temperature",
+	"Board Temperature",
+	"Carrier Temperature",
+	"Chipset Temperature",
+	"Video Temperature",
+	"Other Temperature",
+	"TOPDIM Temperature",
+	"BOTTOMDIM Temperature",
+};
+
+static const struct {
+	enum hwmon_sensor_types type;
+	const char *label;
+} cgbc_hwmon_labels_in[] = {
+	{ hwmon_in, "CPU Voltage" },
+	{ hwmon_in, "DC Runtime Voltage" },
+	{ hwmon_in, "DC Standby Voltage" },
+	{ hwmon_in, "CMOS Battery Voltage" },
+	{ hwmon_in, "Battery Voltage" },
+	{ hwmon_in, "AC Voltage" },
+	{ hwmon_in, "Other Voltage" },
+	{ hwmon_in, "5V Voltage" },
+	{ hwmon_in, "5V Standby Voltage" },
+	{ hwmon_in, "3V3 Voltage" },
+	{ hwmon_in, "3V3 Standby Voltage" },
+	{ hwmon_in, "VCore A Voltage" },
+	{ hwmon_in, "VCore B Voltage" },
+	{ hwmon_in, "12V Voltage" },
+	{ hwmon_curr, "DC Current" },
+	{ hwmon_curr, "5V Current" },
+	{ hwmon_curr, "12V Current" },
+};
+
+static const char * const cgbc_hwmon_labels_fan[] = {
+	"CPU Fan",
+	"Box Fan",
+	"Ambient Fan",
+	"Chipset Fan",
+	"Video Fan",
+	"Other Fan",
+};
+
+static int cgbc_hwmon_cmd(struct cgbc_device_data *cgbc, u8 id, u8 *data)
+{
+	u8 cmd[2] = {CGBC_HWMON_CMD_SENSOR, id};
+
+	return cgbc_command(cgbc, cmd, sizeof(cmd), data, CGBC_HWMON_CMD_SENSOR_DATA_SIZE, NULL);
+}
+
+static int cgbc_hwmon_probe_sensors(struct device *dev, struct cgbc_hwmon_data *hwmon)
+{
+	struct cgbc_device_data *cgbc = hwmon->cgbc;
+	struct cgbc_hwmon_sensor *sensor = hwmon->sensors;
+	u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE], nb_sensors, i;
+	int ret;
+
+	ret = cgbc_hwmon_cmd(cgbc, 0, &data[0]);
+	if (ret)
+		return ret;
+
+	nb_sensors = data[0];
+
+	hwmon->sensors = devm_kzalloc(dev, sizeof(*hwmon->sensors) * nb_sensors, GFP_KERNEL);
+	sensor = hwmon->sensors;
+
+	for (i = 0; i < nb_sensors; i++) {
+		u8 type, id;
+
+		ret = cgbc_hwmon_cmd(cgbc, i, &data[0]);
+		if (ret)
+			return ret;
+
+		type = FIELD_GET(CGBC_HWMON_TYPE_MASK, data[1]);
+		id = FIELD_GET(CGBC_HWMON_ID_MASK, data[1]) - 1;
+
+		if (type == CGBC_HWMON_TYPE_TEMP && id < ARRAY_SIZE(cgbc_hwmon_labels_temp)) {
+			sensor->type = hwmon_temp;
+			sensor->label = cgbc_hwmon_labels_temp[id];
+		} else if (type == CGBC_HWMON_TYPE_IN && id < ARRAY_SIZE(cgbc_hwmon_labels_in)) {
+			/*
+			 * The Board Controller doesn't do differences between current and voltage
+			 * sensors
+			 */
+			sensor->type = cgbc_hwmon_labels_in[id].type;
+			sensor->label = cgbc_hwmon_labels_in[id].label;
+		} else if (type == CGBC_HWMON_TYPE_FAN && id < ARRAY_SIZE(cgbc_hwmon_labels_fan)) {
+			sensor->type = hwmon_fan;
+			sensor->label = cgbc_hwmon_labels_fan[id];
+		} else {
+			dev_warn(dev, "Board Controller returned an unknown sensor (type=%d, id=%d), ignore it",
+				 type, id);
+			continue;
+		}
+
+		sensor->active = FIELD_GET(CGBC_HWMON_ACTIVE_BIT, data[1]);
+		sensor->index = i;
+		sensor++;
+		hwmon->nb_sensors++;
+	}
+
+	return 0;
+}
+
+static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct cgbc_hwmon_data *hwmon,
+							enum hwmon_sensor_types type, int channel)
+{
+	struct cgbc_hwmon_sensor *sensor = NULL;
+	int i, cnt = 0;
+
+	for (i = 0; i < hwmon->nb_sensors; i++) {
+		if (hwmon->sensors[i].type == type && cnt++ == channel) {
+			sensor = &hwmon->sensors[i];
+			break;
+		}
+	}
+
+	return sensor;
+}
+
+static int cgbc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
+			   long *val)
+{
+	struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
+	struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon, type, channel);
+	struct cgbc_device_data *cgbc = hwmon->cgbc;
+	u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE];
+	int ret;
+
+	if (!sensor)
+		return -ENODEV;
+
+	ret = cgbc_hwmon_cmd(cgbc, sensor->index, &data[0]);
+	if (ret)
+		return ret;
+
+	*val = FIELD_PREP(CGBC_HWMON_DATA_HIGH, data[3]) | FIELD_PREP(CGBC_HWMON_DATA_LOW, data[2]);
+
+	/* For the Board Controller 1lsb = 0.1 degree centigrade */
+	if (sensor->type == hwmon_temp)
+		*val *= 100;
+
+	return 0;
+}
+
+static umode_t cgbc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
+				     int channel)
+{
+	struct cgbc_hwmon_data *data = (struct cgbc_hwmon_data *)_data;
+	struct cgbc_hwmon_sensor *sensor;
+
+	sensor = cgbc_hwmon_find_sensor(data, type, channel);
+	if (!sensor)
+		return 0;
+
+	return sensor->active ? 0444 : 0;
+}
+
+static int cgbc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+				  int channel, const char **str)
+{
+	struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
+	struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon, type, channel);
+
+	if (!sensor)
+		return -ENODEV;
+
+	*str = sensor->label;
+
+	return 0;
+}
+
+static const struct hwmon_channel_info * const cgbc_hwmon_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
+			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL),
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
+			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
+	HWMON_CHANNEL_INFO(curr,
+			   HWMON_C_INPUT | HWMON_C_LABEL, HWMON_C_INPUT | HWMON_C_LABEL,
+			   HWMON_C_INPUT | HWMON_C_LABEL),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL,
+			   HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL,
+			   HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops cgbc_hwmon_ops = {
+	.is_visible = cgbc_hwmon_is_visible,
+	.read = cgbc_hwmon_read,
+	.read_string = cgbc_hwmon_read_string,
+};
+
+static const struct hwmon_chip_info cgbc_chip_info = {
+	.ops = &cgbc_hwmon_ops,
+	.info = cgbc_hwmon_info,
+};
+
+static int cgbc_hwmon_probe(struct platform_device *pdev)
+{
+	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct cgbc_hwmon_data *data;
+	struct device *hwmon_dev;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->cgbc = cgbc;
+
+	platform_set_drvdata(pdev, data);
+
+	ret = cgbc_hwmon_probe_sensors(dev, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to probe sensors");
+
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cgbc_hwmon", data, &cgbc_chip_info,
+							 NULL);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct platform_driver cgbc_hwmon_driver = {
+	.driver = {
+		.name = "cgbc-hwmon",
+	},
+	.probe = cgbc_hwmon_probe,
+};
+
+module_platform_driver(cgbc_hwmon_driver);
+
+MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
+MODULE_DESCRIPTION("Congatec Board Controller Hardware Monitoring Driver");
+MODULE_LICENSE("GPL");

-- 
2.39.5

Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

[Guenter Roeck]

On 11/4/24 07:48, Thomas Richard wrote:
> Add support for the Congatec Board Controller. This controller exposes
> temperature, voltage, current and fan sensors.
> 
> The available sensors list cannot be predicted. Some sensors can be
> present or not, depending the system.
> The driver has an internal list of all possible sensors, for all Congatec
> boards. The Board Controller gives to the driver its sensors list, and
> their status (active or not).
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>   MAINTAINERS                |   1 +
>   drivers/hwmon/Kconfig      |   9 ++
>   drivers/hwmon/Makefile     |   1 +
>   drivers/hwmon/cgbc-hwmon.c | 287 +++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 298 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3507df3381b1..5e96646593b1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5784,6 +5784,7 @@ CONGATEC BOARD CONTROLLER MFD DRIVER
>   M:	Thomas Richard <thomas.richard@bootlin.com>
>   S:	Maintained
>   F:	drivers/gpio/gpio-cgbc.c
> +F:	drivers/hwmon/cgbc-hwmon.c
>   F:	drivers/i2c/busses/i2c-cgbc.c
>   F:	drivers/mfd/cgbc-core.c
>   F:	drivers/watchdog/cgbc_wdt.c
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index cfb4e9314c62..c7b6e93aeb9b 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -463,6 +463,15 @@ config SENSORS_BT1_PVT_ALARMS
>   	  the data conversion will be periodically performed and the data will be
>   	  saved in the internal driver cache.
>   
> +config SENSORS_CGBC
> +	tristate "Congatec Board Controller Sensors"
> +	depends on MFD_CGBC
> +	help
> +	  Enables sensors support for the Congatec Board Controller.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called cgbc-hwmon.
> +
>   config SENSORS_CHIPCAP2
>   	tristate "Amphenol ChipCap 2 relative humidity and temperature sensor"
>   	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b827b92f2a78..318da6d8f752 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -59,6 +59,7 @@ obj-$(CONFIG_SENSORS_ASUS_ROG_RYUJIN)	+= asus_rog_ryujin.o
>   obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>   obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
>   obj-$(CONFIG_SENSORS_BT1_PVT)	+= bt1-pvt.o
> +obj-$(CONFIG_SENSORS_CGBC)	+= cgbc-hwmon.o
>   obj-$(CONFIG_SENSORS_CHIPCAP2) += chipcap2.o
>   obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>   obj-$(CONFIG_SENSORS_CORSAIR_CPRO) += corsair-cpro.o
> diff --git a/drivers/hwmon/cgbc-hwmon.c b/drivers/hwmon/cgbc-hwmon.c
> new file mode 100644
> index 000000000000..3234c7590acb
> --- /dev/null
> +++ b/drivers/hwmon/cgbc-hwmon.c
> @@ -0,0 +1,287 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * cgbc-hwmon - Congatec Board Controller hardware monitoring driver
> + *
> + * Copyright (C) 2024 Thomas Richard <thomas.richard@bootlin.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/mfd/cgbc.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define CGBC_HWMON_TYPE_TEMP	1
> +#define CGBC_HWMON_TYPE_IN	2
> +#define CGBC_HWMON_TYPE_FAN	3
> +
> +#define CGBC_HWMON_CMD_SENSOR		0x77
> +#define CGBC_HWMON_CMD_SENSOR_DATA_SIZE	0x05
> +
> +#define CGBC_HWMON_TYPE_MASK	GENMASK(6, 5)
> +#define CGBC_HWMON_ID_MASK	GENMASK(4, 0)
> +#define CGBC_HWMON_ACTIVE_BIT	BIT(7)
> +#define CGBC_HWMON_DATA_HIGH	GENMASK(15, 8)
> +#define CGBC_HWMON_DATA_LOW	GENMASK(7, 0)
> +
> +struct cgbc_hwmon_sensor {
> +	enum hwmon_sensor_types type;
> +	bool active;
> +	u8 index;
> +	const char *label;
> +};
> +
> +struct cgbc_hwmon_data {
> +	struct cgbc_device_data *cgbc;
> +	u8 nb_sensors;

FWIW, using u8 here and in struct cgbc_hwmon_sensor doesn't save any memory,
it only makes the generated code more complex, at least on non-Intel
architectures. The same is true for using u8 for any index variables.
It would make much more sense to use natural data types.

> +	struct cgbc_hwmon_sensor *sensors;
> +};
> +
> +static const char * const cgbc_hwmon_labels_temp[] = {
> +	"CPU Temperature",
> +	"Box Temperature",
> +	"Ambient Temperature",
> +	"Board Temperature",
> +	"Carrier Temperature",
> +	"Chipset Temperature",
> +	"Video Temperature",
> +	"Other Temperature",
> +	"TOPDIM Temperature",
> +	"BOTTOMDIM Temperature",
> +};
> +
> +static const struct {
> +	enum hwmon_sensor_types type;
> +	const char *label;
> +} cgbc_hwmon_labels_in[] = {
> +	{ hwmon_in, "CPU Voltage" },
> +	{ hwmon_in, "DC Runtime Voltage" },
> +	{ hwmon_in, "DC Standby Voltage" },
> +	{ hwmon_in, "CMOS Battery Voltage" },
> +	{ hwmon_in, "Battery Voltage" },
> +	{ hwmon_in, "AC Voltage" },
> +	{ hwmon_in, "Other Voltage" },
> +	{ hwmon_in, "5V Voltage" },
> +	{ hwmon_in, "5V Standby Voltage" },
> +	{ hwmon_in, "3V3 Voltage" },
> +	{ hwmon_in, "3V3 Standby Voltage" },
> +	{ hwmon_in, "VCore A Voltage" },
> +	{ hwmon_in, "VCore B Voltage" },
> +	{ hwmon_in, "12V Voltage" },
> +	{ hwmon_curr, "DC Current" },
> +	{ hwmon_curr, "5V Current" },
> +	{ hwmon_curr, "12V Current" },
> +};
> +
> +static const char * const cgbc_hwmon_labels_fan[] = {
> +	"CPU Fan",
> +	"Box Fan",
> +	"Ambient Fan",
> +	"Chipset Fan",
> +	"Video Fan",
> +	"Other Fan",
> +};
> +
> +static int cgbc_hwmon_cmd(struct cgbc_device_data *cgbc, u8 id, u8 *data)
> +{
> +	u8 cmd[2] = {CGBC_HWMON_CMD_SENSOR, id};
> +
> +	return cgbc_command(cgbc, cmd, sizeof(cmd), data, CGBC_HWMON_CMD_SENSOR_DATA_SIZE, NULL);
> +}
> +
> +static int cgbc_hwmon_probe_sensors(struct device *dev, struct cgbc_hwmon_data *hwmon)
> +{
> +	struct cgbc_device_data *cgbc = hwmon->cgbc;
> +	struct cgbc_hwmon_sensor *sensor = hwmon->sensors;
> +	u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE], nb_sensors, i;
> +	int ret;
> +
> +	ret = cgbc_hwmon_cmd(cgbc, 0, &data[0]);
> +	if (ret)
> +		return ret;
> +
> +	nb_sensors = data[0];
> +
> +	hwmon->sensors = devm_kzalloc(dev, sizeof(*hwmon->sensors) * nb_sensors, GFP_KERNEL);
> +	sensor = hwmon->sensors;
> +
> +	for (i = 0; i < nb_sensors; i++) {
> +		u8 type, id;
> +
> +		ret = cgbc_hwmon_cmd(cgbc, i, &data[0]);

For the first loop, this essentially repeats the cgbc_hwmon_cmd() from above.
Is that how it works, i.e., that index == 0 returns both the number of sensors
in the first byte of return data plus the data for the first sensor ?

> +		if (ret)
> +			return ret;
> +
> +		type = FIELD_GET(CGBC_HWMON_TYPE_MASK, data[1]);
> +		id = FIELD_GET(CGBC_HWMON_ID_MASK, data[1]) - 1;
> +
> +		if (type == CGBC_HWMON_TYPE_TEMP && id < ARRAY_SIZE(cgbc_hwmon_labels_temp)) {
> +			sensor->type = hwmon_temp;
> +			sensor->label = cgbc_hwmon_labels_temp[id];
> +		} else if (type == CGBC_HWMON_TYPE_IN && id < ARRAY_SIZE(cgbc_hwmon_labels_in)) {
> +			/*
> +			 * The Board Controller doesn't do differences between current and voltage

doesn't differentiate

> +			 * sensors
> +			 */

This doesn't really explain what is happening. Please add something like
"Get the sensor type from cgbc_hwmon_labels_in[id].label instead".

> +			sensor->type = cgbc_hwmon_labels_in[id].type;
> +			sensor->label = cgbc_hwmon_labels_in[id].label;
> +		} else if (type == CGBC_HWMON_TYPE_FAN && id < ARRAY_SIZE(cgbc_hwmon_labels_fan)) {
> +			sensor->type = hwmon_fan;
> +			sensor->label = cgbc_hwmon_labels_fan[id];
> +		} else {
> +			dev_warn(dev, "Board Controller returned an unknown sensor (type=%d, id=%d), ignore it",
> +				 type, id);
> +			continue;
> +		}
> +
> +		sensor->active = FIELD_GET(CGBC_HWMON_ACTIVE_BIT, data[1]);
> +		sensor->index = i;
> +		sensor++;
> +		hwmon->nb_sensors++;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct cgbc_hwmon_data *hwmon,
> +							enum hwmon_sensor_types type, int channel)
> +{
> +	struct cgbc_hwmon_sensor *sensor = NULL;
> +	int i, cnt = 0;
> +
> +	for (i = 0; i < hwmon->nb_sensors; i++) {
> +		if (hwmon->sensors[i].type == type && cnt++ == channel) {

Isn't that a bit fragile ? It assumes that the nth reported sensor of a given type
reflects a specific named sensor. If that is indeed the case, why bother with
all the code in cgbc_hwmon_probe_sensors() ? The index to sensor association
should be well defined, and the sensor type plus the channel index should always
be a constant.

> +			sensor = &hwmon->sensors[i];
> +			break;
> +		}
> +	}
> +
> +	return sensor;
> +}
> +
> +static int cgbc_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> +			   long *val)
> +{
> +	struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
> +	struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon, type, channel);
> +	struct cgbc_device_data *cgbc = hwmon->cgbc;
> +	u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE];
> +	int ret;
> +
> +	if (!sensor)
> +		return -ENODEV;

How would this ever happen ? Unless I am missing something, that means
there is a bug somewhere in the code. "No such device" is definitely
wrong here (and elsewhere). If you don't trust your code and think
this can happen, at least return -ENODATA.

> +
> +	ret = cgbc_hwmon_cmd(cgbc, sensor->index, &data[0]);
> +	if (ret)
> +		return ret;
> +
> +	*val = FIELD_PREP(CGBC_HWMON_DATA_HIGH, data[3]) | FIELD_PREP(CGBC_HWMON_DATA_LOW, data[2]);
> +

That is a pretty complex way of writing
	*val = (data[3] << 8) | data[2];
I'd say that is close to obfuscation, but that is your call.

> +	/* For the Board Controller 1lsb = 0.1 degree centigrade */

All other units are as expected (mV, mA, rpm) ?

> +	if (sensor->type == hwmon_temp)
> +		*val *= 100;
> +
> +	return 0;
> +}
> +
> +static umode_t cgbc_hwmon_is_visible(const void *_data, enum hwmon_sensor_types type, u32 attr,
> +				     int channel)
> +{
> +	struct cgbc_hwmon_data *data = (struct cgbc_hwmon_data *)_data;
> +	struct cgbc_hwmon_sensor *sensor;
> +
> +	sensor = cgbc_hwmon_find_sensor(data, type, channel);
> +	if (!sensor)
> +		return 0;
> +
> +	return sensor->active ? 0444 : 0;
> +}
> +
> +static int cgbc_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +				  int channel, const char **str)
> +{
> +	struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
> +	struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon, type, channel);
> +
> +	if (!sensor)
> +		return -ENODEV;
> +
> +	*str = sensor->label;
> +
> +	return 0;
> +}
> +
> +static const struct hwmon_channel_info * const cgbc_hwmon_info[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL,
> +			   HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT | HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(in,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL,
> +			   HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT | HWMON_I_LABEL),
> +	HWMON_CHANNEL_INFO(curr,
> +			   HWMON_C_INPUT | HWMON_C_LABEL, HWMON_C_INPUT | HWMON_C_LABEL,
> +			   HWMON_C_INPUT | HWMON_C_LABEL),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT | HWMON_F_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops cgbc_hwmon_ops = {
> +	.is_visible = cgbc_hwmon_is_visible,
> +	.read = cgbc_hwmon_read,
> +	.read_string = cgbc_hwmon_read_string,
> +};
> +
> +static const struct hwmon_chip_info cgbc_chip_info = {
> +	.ops = &cgbc_hwmon_ops,
> +	.info = cgbc_hwmon_info,
> +};
> +
> +static int cgbc_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct cgbc_hwmon_data *data;
> +	struct device *hwmon_dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->cgbc = cgbc;
> +
> +	platform_set_drvdata(pdev, data);

What is this used for ? There are no suspend/resume functions,
so I don't see the use case.

> +
> +	ret = cgbc_hwmon_probe_sensors(dev, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to probe sensors");
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "cgbc_hwmon", data, &cgbc_chip_info,
> +							 NULL);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver cgbc_hwmon_driver = {
> +	.driver = {
> +		.name = "cgbc-hwmon",
> +	},
> +	.probe = cgbc_hwmon_probe,
> +};
> +
> +module_platform_driver(cgbc_hwmon_driver);
> +
> +MODULE_AUTHOR("Thomas Richard <thomas.richard@bootlin.com>");
> +MODULE_DESCRIPTION("Congatec Board Controller Hardware Monitoring Driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

[Thomas Richard]

Hello Guenter,

Thanks for the review !!

> 
> For the first loop, this essentially repeats the cgbc_hwmon_cmd() from
> above.
> Is that how it works, i.e., that index == 0 returns both the number of
> sensors
> in the first byte of return data plus the data for the first sensor ?

Yes it is.
This is based on the Congatec implementation.
First time I read the their code, I had the same reaction than you.
But it works.

Please note that the driver provided by Congatec is the only source of
information I have (there is no technical documentation which describes
the internal behavior of this Board Controller).

I'll skip the cgbc_hwmon_cmd() for the first loop and add a comment to
not forget that it is not an error.

> 
>> +        if (ret)
>> +            return ret;
>> +
>> +        type = FIELD_GET(CGBC_HWMON_TYPE_MASK, data[1]);
>> +        id = FIELD_GET(CGBC_HWMON_ID_MASK, data[1]) - 1;
>> +
>> +        if (type == CGBC_HWMON_TYPE_TEMP && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_temp)) {
>> +            sensor->type = hwmon_temp;
>> +            sensor->label = cgbc_hwmon_labels_temp[id];
>> +        } else if (type == CGBC_HWMON_TYPE_IN && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_in)) {
>> +            /*
>> +             * The Board Controller doesn't do differences between
>> current and voltage
> 
> doesn't differentiate
> 
>> +             * sensors
>> +             */
> 
> This doesn't really explain what is happening. Please add something like
> "Get the sensor type from cgbc_hwmon_labels_in[id].label instead".
> 
>> +            sensor->type = cgbc_hwmon_labels_in[id].type;
>> +            sensor->label = cgbc_hwmon_labels_in[id].label;
>> +        } else if (type == CGBC_HWMON_TYPE_FAN && id <
>> ARRAY_SIZE(cgbc_hwmon_labels_fan)) {
>> +            sensor->type = hwmon_fan;
>> +            sensor->label = cgbc_hwmon_labels_fan[id];
>> +        } else {
>> +            dev_warn(dev, "Board Controller returned an unknown
>> sensor (type=%d, id=%d), ignore it",
>> +                 type, id);
>> +            continue;
>> +        }
>> +
>> +        sensor->active = FIELD_GET(CGBC_HWMON_ACTIVE_BIT, data[1]);
>> +        sensor->index = i;
>> +        sensor++;
>> +        hwmon->nb_sensors++;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct
>> cgbc_hwmon_data *hwmon,
>> +                            enum hwmon_sensor_types type, int channel)
>> +{
>> +    struct cgbc_hwmon_sensor *sensor = NULL;
>> +    int i, cnt = 0;
>> +
>> +    for (i = 0; i < hwmon->nb_sensors; i++) {
>> +        if (hwmon->sensors[i].type == type && cnt++ == channel) {
> 
> Isn't that a bit fragile ? It assumes that the nth reported sensor of a
> given type
> reflects a specific named sensor. If that is indeed the case, why bother
> with
> all the code in cgbc_hwmon_probe_sensors() ? The index to sensor
> association
> should be well defined, and the sensor type plus the channel index
> should always
> be a constant.

I'm not sure to get your comment.

I cannot assume that the sensor list returned by the Board Controller
will be the same for all boards.
I know the MFD driver only supports one board, but I think it's better
if we can have a generic hwmon driver.

If I add some debug in cgbc_hwmon_probe_sensors() I can dump the sensor
list returned by the Board Controller:

cgbc_hwmon_probe_sensors: index=0 type=1 id=5 label=Chipset Temperature
cgbc_hwmon_probe_sensors: index=1 type=7 id=0 label=CPU Fan
cgbc_hwmon_probe_sensors: index=4 type=1 id=3 label=Board Temperature
cgbc_hwmon_probe_sensors: index=5 type=2 id=1 label=DC Runtime Voltage

It is the type and the id which give the name of the sensor.

I don't see how to do it in a different way if I cannot assume that the
list above is not the same for all boards.
If I assume that the list returned by the Board Controller is always the
same for a board (which I not even sure, if for example a fan is
plugged), I could have a static list for each different boards. But the
driver will not be generic.

If I miss something, please let me know.

> 
>> +            sensor = &hwmon->sensors[i];
>> +            break;
>> +        }
>> +    }
>> +
>> +    return sensor;
>> +}
>> +
>> +static int cgbc_hwmon_read(struct device *dev, enum
>> hwmon_sensor_types type, u32 attr, int channel,
>> +               long *val)
>> +{
>> +    struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +    struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon,
>> type, channel);
>> +    struct cgbc_device_data *cgbc = hwmon->cgbc;
>> +    u8 data[CGBC_HWMON_CMD_SENSOR_DATA_SIZE];
>> +    int ret;
>> +
>> +    if (!sensor)
>> +        return -ENODEV;
> 
> How would this ever happen ? Unless I am missing something, that means
> there is a bug somewhere in the code. "No such device" is definitely
> wrong here (and elsewhere). If you don't trust your code and think
> this can happen, at least return -ENODATA.

I can remove this return -ENODEV (and also the one in read_string()).
The read() and read_string() callbacks can only be called if the sensor
is created in sysfs.
And the hwmon core creates the sysfs entry only if is_visible() does not
return an error.
The function cgbc_hwmon_find_sensor() is called in is_visible() and the
returned pointer is checked.
So if read() or read_string() is called, we know that is_visible()
didn't return an error, so cgbc_hwmon_find_sensor() didn't return an error.
So this "return -ENODEV" (in read() and read_string()) is dead code.

> 
>> +
>> +    ret = cgbc_hwmon_cmd(cgbc, sensor->index, &data[0]);
>> +    if (ret)
>> +        return ret;
>> +
>> +    *val = FIELD_PREP(CGBC_HWMON_DATA_HIGH, data[3]) |
>> FIELD_PREP(CGBC_HWMON_DATA_LOW, data[2]);
>> +
> 
> That is a pretty complex way of writing
>     *val = (data[3] << 8) | data[2];
> I'd say that is close to obfuscation, but that is your call.

I agree, it's easier to read.

> 
>> +    /* For the Board Controller 1lsb = 0.1 degree centigrade */
> 
> All other units are as expected (mV, mA, rpm) ?

Yes they are, I will mention it.

> 
>> +    if (sensor->type == hwmon_temp)
>> +        *val *= 100;
>> +
>> +    return 0;
>> +}
>> +
>> +static umode_t cgbc_hwmon_is_visible(const void *_data, enum
>> hwmon_sensor_types type, u32 attr,
>> +                     int channel)
>> +{
>> +    struct cgbc_hwmon_data *data = (struct cgbc_hwmon_data *)_data;
>> +    struct cgbc_hwmon_sensor *sensor;
>> +
>> +    sensor = cgbc_hwmon_find_sensor(data, type, channel);
>> +    if (!sensor)
>> +        return 0;
>> +
>> +    return sensor->active ? 0444 : 0;
>> +}
>> +
>> +static int cgbc_hwmon_read_string(struct device *dev, enum
>> hwmon_sensor_types type, u32 attr,
>> +                  int channel, const char **str)
>> +{
>> +    struct cgbc_hwmon_data *hwmon = dev_get_drvdata(dev);
>> +    struct cgbc_hwmon_sensor *sensor = cgbc_hwmon_find_sensor(hwmon,
>> type, channel);
>> +
>> +    if (!sensor)
>> +        return -ENODEV;
>> +
>> +    *str = sensor->label;
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info * const cgbc_hwmon_info[] = {
>> +    HWMON_CHANNEL_INFO(temp,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL,
>> +               HWMON_T_INPUT | HWMON_T_LABEL, HWMON_T_INPUT |
>> HWMON_T_LABEL),
>> +    HWMON_CHANNEL_INFO(in,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL,
>> +               HWMON_I_INPUT | HWMON_I_LABEL, HWMON_I_INPUT |
>> HWMON_I_LABEL),
>> +    HWMON_CHANNEL_INFO(curr,
>> +               HWMON_C_INPUT | HWMON_C_LABEL, HWMON_C_INPUT |
>> HWMON_C_LABEL,
>> +               HWMON_C_INPUT | HWMON_C_LABEL),
>> +    HWMON_CHANNEL_INFO(fan,
>> +               HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL,
>> +               HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL,
>> +               HWMON_F_INPUT | HWMON_F_LABEL, HWMON_F_INPUT |
>> HWMON_F_LABEL),
>> +    NULL
>> +};
>> +
>> +static const struct hwmon_ops cgbc_hwmon_ops = {
>> +    .is_visible = cgbc_hwmon_is_visible,
>> +    .read = cgbc_hwmon_read,
>> +    .read_string = cgbc_hwmon_read_string,
>> +};
>> +
>> +static const struct hwmon_chip_info cgbc_chip_info = {
>> +    .ops = &cgbc_hwmon_ops,
>> +    .info = cgbc_hwmon_info,
>> +};
>> +
>> +static int cgbc_hwmon_probe(struct platform_device *pdev)
>> +{
>> +    struct cgbc_device_data *cgbc = dev_get_drvdata(pdev->dev.parent);
>> +    struct device *dev = &pdev->dev;
>> +    struct cgbc_hwmon_data *data;
>> +    struct device *hwmon_dev;
>> +    int ret;
>> +
>> +    data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +    if (!data)
>> +        return -ENOMEM;
>> +
>> +    data->cgbc = cgbc;
>> +
>> +    platform_set_drvdata(pdev, data);
> 
> What is this used for ? There are no suspend/resume functions,
> so I don't see the use case.

It's useless indeed.

Regards,

Thomas

Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

[Guenter Roeck]

On 11/6/24 05:46, Thomas Richard wrote:

>>> +
>>> +static struct cgbc_hwmon_sensor *cgbc_hwmon_find_sensor(struct
>>> cgbc_hwmon_data *hwmon,
>>> +                            enum hwmon_sensor_types type, int channel)
>>> +{
>>> +    struct cgbc_hwmon_sensor *sensor = NULL;
>>> +    int i, cnt = 0;
>>> +
>>> +    for (i = 0; i < hwmon->nb_sensors; i++) {
>>> +        if (hwmon->sensors[i].type == type && cnt++ == channel) {
>>
>> Isn't that a bit fragile ? It assumes that the nth reported sensor of a
>> given type
>> reflects a specific named sensor. If that is indeed the case, why bother
>> with
>> all the code in cgbc_hwmon_probe_sensors() ? The index to sensor
>> association
>> should be well defined, and the sensor type plus the channel index
>> should always
>> be a constant.
> 
> I'm not sure to get your comment.
> 
> I cannot assume that the sensor list returned by the Board Controller
> will be the same for all boards.
> I know the MFD driver only supports one board, but I think it's better
> if we can have a generic hwmon driver.
> 
> If I add some debug in cgbc_hwmon_probe_sensors() I can dump the sensor
> list returned by the Board Controller:
> 
> cgbc_hwmon_probe_sensors: index=0 type=1 id=5 label=Chipset Temperature
> cgbc_hwmon_probe_sensors: index=1 type=7 id=0 label=CPU Fan
> cgbc_hwmon_probe_sensors: index=4 type=1 id=3 label=Board Temperature
> cgbc_hwmon_probe_sensors: index=5 type=2 id=1 label=DC Runtime Voltage
> 
> It is the type and the id which give the name of the sensor.
> 
> I don't see how to do it in a different way if I cannot assume that the
> list above is not the same for all boards.
> If I assume that the list returned by the Board Controller is always the
> same for a board (which I not even sure, if for example a fan is
> plugged), I could have a static list for each different boards. But the
> driver will not be generic.
> 
> If I miss something, please let me know.
> 

My thought was to use the sensor ID as channel index. In general it
would be preferable to know that, say, in0 is always the CPU voltage
and that temp1 is always the CPU Temperature.

Guenter

Re: [PATCH 1/2] hwmon: Add Congatec Board Controller monitoring driver

[Thomas Richard]

On 11/6/24 16:14, Guenter Roeck wrote:
> 
> My thought was to use the sensor ID as channel index. In general it
> would be preferable to know that, say, in0 is always the CPU voltage
> and that temp1 is always the CPU Temperature.
> 

Ok I understand now.
I'll try to do it for the v2.

Regards,

Thomas

[PATCH 2/2] mfd: cgbc: add a hwmon cell

[Thomas Richard]

The Board Controller has some internal sensors.
Add a hwmon cell for the cgbc-hwmon driver which adds support for
temperature, voltage, current and fan sensors.

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---
 drivers/mfd/cgbc-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
index 93004a6b29c1..7cc2872235ac 100644
--- a/drivers/mfd/cgbc-core.c
+++ b/drivers/mfd/cgbc-core.c
@@ -236,6 +236,7 @@ static struct mfd_cell cgbc_devs[] = {
 	{ .name = "cgbc-gpio"	},
 	{ .name = "cgbc-i2c", .id = 1 },
 	{ .name = "cgbc-i2c", .id = 2 },
+	{ .name = "cgbc-hwmon"	},
 };
 
 static int cgbc_map(struct cgbc_device_data *cgbc)

-- 
2.39.5