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

Thomas Richard posted 2 patches 2 weeks, 1 day ago
There is a newer version of this series
[PATCH v2 1/2] hwmon: Add Congatec Board Controller monitoring driver
Posted by Thomas Richard 2 weeks, 1 day ago
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 | 314 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 325 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..6352d3f38516
--- /dev/null
+++ b/drivers/hwmon/cgbc-hwmon.c
@@ -0,0 +1,314 @@
+// 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_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)
+
+struct cgbc_hwmon_sensor {
+	enum hwmon_sensor_types type;
+	bool active;
+	unsigned int index;
+	unsigned int channel;
+	const char *label;
+};
+
+struct cgbc_hwmon_data {
+	struct cgbc_device_data *cgbc;
+	unsigned int nb_sensors;
+	struct cgbc_hwmon_sensor *sensors;
+};
+
+enum cgbc_sensor_types {
+	CGBC_HWMON_TYPE_TEMP = 1,
+	CGBC_HWMON_TYPE_IN,
+	CGBC_HWMON_TYPE_FAN
+};
+
+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" },
+};
+
+#define cgbc_hwmon_compute_curr_channel(channel)				\
+	({									\
+		int i, cnt = 0;							\
+		for (i = 0; i < ARRAY_SIZE(cgbc_hwmon_labels_in); i++) {	\
+			if (cgbc_hwmon_labels_in[i].type == hwmon_in)		\
+				cnt++;						\
+		}								\
+		(channel) + cnt;						\
+	 })
+
+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 index, u8 *data)
+{
+	u8 cmd[2] = {CGBC_HWMON_CMD_SENSOR, index};
+
+	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++) {
+		enum cgbc_sensor_types type;
+		unsigned int channel;
+
+		/*
+		 * No need to request data for the first sensor.
+		 * We got data for the first sensor when we ask the number of sensors to the Board
+		 * Controller.
+		 */
+		if (i) {
+			ret = cgbc_hwmon_cmd(cgbc, i, &data[0]);
+			if (ret)
+				return ret;
+		}
+
+		type = FIELD_GET(CGBC_HWMON_TYPE_MASK, data[1]);
+		channel = FIELD_GET(CGBC_HWMON_ID_MASK, data[1]) - 1;
+
+		if (type == CGBC_HWMON_TYPE_TEMP && channel < ARRAY_SIZE(cgbc_hwmon_labels_temp)) {
+			sensor->type = hwmon_temp;
+			sensor->label = cgbc_hwmon_labels_temp[channel];
+		} else if (type == CGBC_HWMON_TYPE_IN &&
+			   channel < ARRAY_SIZE(cgbc_hwmon_labels_in)) {
+			/*
+			 * The Board Controller doesn't differentiate current and voltage sensors.
+			 * Get the sensor type from cgbc_hwmon_labels_in[channel].type instead.
+			 */
+			sensor->type = cgbc_hwmon_labels_in[channel].type;
+			sensor->label = cgbc_hwmon_labels_in[channel].label;
+		} else if (type == CGBC_HWMON_TYPE_FAN &&
+			   channel < ARRAY_SIZE(cgbc_hwmon_labels_fan)) {
+			sensor->type = hwmon_fan;
+			sensor->label = cgbc_hwmon_labels_fan[channel];
+		} else {
+			dev_warn(dev, "Board Controller returned an unknown sensor (type=%d, channel=%d), ignore it",
+				 type, channel);
+			continue;
+		}
+
+		sensor->active = FIELD_GET(CGBC_HWMON_ACTIVE_BIT, data[1]);
+		sensor->channel = channel;
+		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, chan;
+
+	/*
+	 * The Board Controller doesn't differentiate current and voltage sensors.
+	 * The channel value (from the Board Controller point of view) shall be computed for current
+	 * sensors.
+	 */
+	if (type == hwmon_curr)
+		chan = cgbc_hwmon_compute_curr_channel(channel);
+	else
+		chan = channel;
+
+	for (i = 0; i < hwmon->nb_sensors; i++) {
+		if (hwmon->sensors[i].type == type && hwmon->sensors[i].channel == chan) {
+			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;
+
+	ret = cgbc_hwmon_cmd(cgbc, sensor->index, &data[0]);
+	if (ret)
+		return ret;
+
+	*val = (data[3] << 8) | data[2];
+
+	/*
+	 * For the Board Controller 1lsb = 0.1 degree centigrade.
+	 * Other units are as expected.
+	 */
+	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);
+
+	*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;
+
+	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 v2 1/2] hwmon: Add Congatec Board Controller monitoring driver
Posted by Guenter Roeck 1 week, 5 days ago
On 11/8/24 07:08, 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 | 314 +++++++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 325 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..6352d3f38516
> --- /dev/null
> +++ b/drivers/hwmon/cgbc-hwmon.c
> @@ -0,0 +1,314 @@
> +// 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_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)
> +
> +struct cgbc_hwmon_sensor {
> +	enum hwmon_sensor_types type;
> +	bool active;
> +	unsigned int index;
> +	unsigned int channel;
> +	const char *label;
> +};
> +
> +struct cgbc_hwmon_data {
> +	struct cgbc_device_data *cgbc;
> +	unsigned int nb_sensors;
> +	struct cgbc_hwmon_sensor *sensors;
> +};
> +
> +enum cgbc_sensor_types {
> +	CGBC_HWMON_TYPE_TEMP = 1,
> +	CGBC_HWMON_TYPE_IN,
> +	CGBC_HWMON_TYPE_FAN
> +};
> +
> +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" },
> +};
> +
> +#define cgbc_hwmon_compute_curr_channel(channel)				\
> +	({									\
> +		int i, cnt = 0;							\
> +		for (i = 0; i < ARRAY_SIZE(cgbc_hwmon_labels_in); i++) {	\
> +			if (cgbc_hwmon_labels_in[i].type == hwmon_in)		\
> +				cnt++;						\
> +		}								\
> +		(channel) + cnt;						\
> +	 })
> +

 From Documentation/hwmon/submitting-patches.rst:

* Avoid calculations in macros and macro-generated functions. While such macros
   may save a line or so in the source, it obfuscates the code and makes code
   review more difficult. It may also result in code which is more complicated
   than necessary. Use inline functions or just regular functions instead.

I don't understand why this would be needed anyway. The current channel
index is well known, so adding or subtracting a constant should do it.

Guenter
Re: [PATCH v2 1/2] hwmon: Add Congatec Board Controller monitoring driver
Posted by Thomas Richard 1 week, 1 day ago
On 11/11/24 20:31, Guenter Roeck wrote:
> 
> I don't understand why this would be needed anyway. The current channel
> index is well known, so adding or subtracting a constant should do it.

My first idea to use this macro was to avoid issue if a new voltage
sensor is added in the future.

But it's useless. If in a new generation of Board Controller a voltage
sensor is added, it will be added at the end of the list.

But it doesn't exist for now, so no need to take care.

I'll just add a constant for curr sensors.

Regards,

Thomas
Re: [PATCH v2 1/2] hwmon: Add Congatec Board Controller monitoring driver
Posted by Guenter Roeck 1 week, 1 day ago
On 11/15/24 06:00, Thomas Richard wrote:
> On 11/11/24 20:31, Guenter Roeck wrote:
>>
>> I don't understand why this would be needed anyway. The current channel
>> index is well known, so adding or subtracting a constant should do it.
> 
> My first idea to use this macro was to avoid issue if a new voltage
> sensor is added in the future.
> 
> But it's useless. If in a new generation of Board Controller a voltage
> sensor is added, it will be added at the end of the list.
> 

Exactly. And if it isn't added to the end of the list, the array with
the sensor names and types would have to change, meaning a new array
would be needed anyway. If that happens, it would be easier to just
add another constant for that second controller variation.

Thanks,
Guenter