Introduce MEN 14F021P BMC driver series

Quoted 28 lines · Andreas Werner

The MEN 14F021P00 Board Management Controller provides an
I2C interface to the host to access the feature implemented in the BMC. The BMC is a PIC Microntroller assembled on CPCI Card from MEN Mikroelektronik and on a few Box/Display Computer.

Added MFD Core driver, supporting the I2C communication to the device.

The MFD driver currently supports the following features:

(quoted diff) +0 -0

 	- Watchdog
 	- LEDs

- Hwmon (voltage monitoring)

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/mfd/Kconfig     |  12 +++++
 drivers/mfd/Makefile    |   1 +
 drivers/mfd/menf21bmc.c | 132 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+)
 create mode 100644 drivers/mfd/menf21bmc.c

drivers/mfd/Kconfig +0 -0

index b8d9ca0..bf155b9 100644
@@ -453,6 +453,18 @@ config MFD_MAX8998
 	  additional drivers must be enabled in order to use the functionality
 	  of the device.
 

drivers/mfd/Kconfig config MFD_MAX8998 · line 458

Quoted 3 lines

drivers/mfd/Kconfig +3 -0

+config MFD_MENF21BMC
+	tristate "MEN 14F021P00 Board Management Controller Support"
+	depends on I2C=y

Why must I2C be built-in?

drivers/mfd/Kconfig config MFD_MAX8998 · line 467

Quoted 10 lines

drivers/mfd/Kconfig +9 -0

+	select MFD_CORE
+	help
+	  Say yes here to add support for the MEN 14F021P00 BMC
+	  which is a Board Management Controller connected to the I2C bus.
+	  The device supports multiple sub-devices like LED, HWMON  and WDT.
+	  This driver provides common support for accessing the devices;
+	  additional drivers must be enabled in order to use the
+	  functionality of the BMC device.
+

Apart from that the patch looks good to me.

I'm assuming this can go in via the MFD tree by itself?

Acked-by Lee Jones <[email protected]>

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Added driver to support the 14F021P00 BMC Watchdog.
The BMC is a Board Management Controller including watchdog functionality.

Signed-off-by Andreas Werner <[email protected]>

Added driver to support the 14F021P00 BMC LEDs.
The BMC is a Board Management Controller including four LEDs which can be switched on and off.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/leds/Kconfig          |   6 ++
 drivers/leds/Makefile         |   1 +
 drivers/leds/leds-menf21bmc.c | 131 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/leds/leds-menf21bmc.c

drivers/leds/Kconfig +6 -0

index 27cf0cd..d38ff3f 100644
@@ -458,6 +458,12 @@ config LEDS_OT200
          This option enables support for the LEDs on the Bachmann OT200.
          Say Y to enable LEDs on the Bachmann OT200.
 
+config LEDS_MENF21BMC
+       tristate "LED support for the MEN 14F021P00 BMC"
+       depends on LEDS_CLASS && MFD_MENF21BMC
+       help
+         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
 
 config LEDS_BLINKM

drivers/leds/Makefile +1 -0

index 3c03666..cadc433 100644
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
 obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
 obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
 obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
+obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o

drivers/leds/leds-menf21bmc.c new file +115 -0

new file mode 100644
index 0000000..4621f72
@@ -0,0 +1,131 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) LEDs Driver.
+ *
+ *  This is the core LED driver of the MEN 14F021P00 BMC.
+ *  There are four LEDs available which can be switched on and off.
+ *  STATUS LED, HOT SWAP LED, USER LED 1, USER LED 2
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/i2c.h>
+
+#define BMC_CMD_LED_GET_SET    0xA0
+#define BMC_BIT_LED_STATUS     BIT(0)
+#define BMC_BIT_LED_HOTSWAP    BIT(1)
+#define BMC_BIT_LED_USER1      BIT(2)
+#define BMC_BIT_LED_USER2      BIT(3)
+
+struct menf21bmc_led {
+       struct led_classdev cdev;
+       u8 led_bit;
+       const char *name;
+       struct i2c_client *i2c_client;
+};
+
+static struct menf21bmc_led leds[] = {
+       {
+               .name = "menf21bmc:led_status",
+               .led_bit = BMC_BIT_LED_STATUS,
+       },
+       {
+               .name = "menf21bmc:led_hotswap",
+               .led_bit = BMC_BIT_LED_HOTSWAP,
+       },
+       {
+               .name = "menf21bmc:led_user1",
+               .led_bit = BMC_BIT_LED_USER1,
+       },
+       {
+               .name = "menf21bmc:led_user2",
+               .led_bit = BMC_BIT_LED_USER2,
+       }
+};
+
+static DEFINE_MUTEX(led_lock);
+
+static void
+menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+       int led_val;
+       struct menf21bmc_led *led = container_of(led_cdev,
+                                       struct menf21bmc_led, cdev);
+
+       mutex_lock(&led_lock);
+       led_val = i2c_smbus_read_byte_data(led->i2c_client,
+                                               BMC_CMD_LED_GET_SET);
+       if (led_val < 0)
+               goto err_out;
+
+       if (value = LED_OFF)
+               led_val &= ~led->led_bit;
+       else
+               led_val |= led->led_bit;
+
+       i2c_smbus_write_byte_data(led->i2c_client,
+                                       BMC_CMD_LED_GET_SET, led_val);
+err_out:
+       mutex_unlock(&led_lock);
+}
+
+static int menf21bmc_led_probe(struct platform_device *pdev)
+{
+       int i;
+       int ret;
+       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
+
+       for (i = 0; i < ARRAY_SIZE(leds); i++) {
+               leds[i].cdev.name = leds[i].name;
+               leds[i].cdev.brightness_set = menf21bmc_led_set;
+               leds[i].i2c_client = i2c_client;
+               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
+               if (ret < 0)
+                       goto err_free_leds;
+       }
+       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
+
+       return 0;
+
+err_free_leds:
+       dev_err(&pdev->dev, "failed to register LED device\n");
+
+       for (i = i - 1; i >= 0; i--)
+               led_classdev_unregister(&leds[i].cdev);
+
+       return ret;
+}
+
+static int menf21bmc_led_remove(struct platform_device *pdev)
+{
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(leds); i++)
+               led_classdev_unregister(&leds[i].cdev);
+
+       return 0;
+}

drivers/leds/leds-menf21bmc.c new file +6 -1

+module_platform_driver(menf21bmc_led);
+
+MODULE_AUTHOR("Andreas Werner <[email protected]>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC led driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_led");
--

2.0.4

Quoted 168 lines · Andreas Werner

Added driver to support the 14F021P00 BMC LEDs.
The BMC is a Board Management Controller including four LEDs which can be switched on and off.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/leds/Kconfig          |   6 ++
 drivers/leds/Makefile         |   1 +
 drivers/leds/leds-menf21bmc.c | 131 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/leds/leds-menf21bmc.c

drivers/leds/Kconfig +6 -0

index 27cf0cd..d38ff3f 100644
@@ -458,6 +458,12 @@ config LEDS_OT200
          This option enables support for the LEDs on the Bachmann OT200.
          Say Y to enable LEDs on the Bachmann OT200.
 
+config LEDS_MENF21BMC
+       tristate "LED support for the MEN 14F021P00 BMC"
+       depends on LEDS_CLASS && MFD_MENF21BMC
+       help
+         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
 
 config LEDS_BLINKM

drivers/leds/Makefile +1 -0

index 3c03666..cadc433 100644
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
 obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
 obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
 obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
+obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o

drivers/leds/leds-menf21bmc.c new file +121 -0

new file mode 100644
index 0000000..4621f72
@@ -0,0 +1,131 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) LEDs Driver.
+ *
+ *  This is the core LED driver of the MEN 14F021P00 BMC.
+ *  There are four LEDs available which can be switched on and off.
+ *  STATUS LED, HOT SWAP LED, USER LED 1, USER LED 2
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/leds.h>
+#include <linux/i2c.h>
+
+#define BMC_CMD_LED_GET_SET    0xA0
+#define BMC_BIT_LED_STATUS     BIT(0)
+#define BMC_BIT_LED_HOTSWAP    BIT(1)
+#define BMC_BIT_LED_USER1      BIT(2)
+#define BMC_BIT_LED_USER2      BIT(3)
+
+struct menf21bmc_led {
+       struct led_classdev cdev;
+       u8 led_bit;
+       const char *name;
+       struct i2c_client *i2c_client;
+};
+
+static struct menf21bmc_led leds[] = {
+       {
+               .name = "menf21bmc:led_status",
+               .led_bit = BMC_BIT_LED_STATUS,
+       },
+       {
+               .name = "menf21bmc:led_hotswap",
+               .led_bit = BMC_BIT_LED_HOTSWAP,
+       },
+       {
+               .name = "menf21bmc:led_user1",
+               .led_bit = BMC_BIT_LED_USER1,
+       },
+       {
+               .name = "menf21bmc:led_user2",
+               .led_bit = BMC_BIT_LED_USER2,
+       }
+};
+
+static DEFINE_MUTEX(led_lock);
+
+static void
+menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+       int led_val;
+       struct menf21bmc_led *led = container_of(led_cdev,
+                                       struct menf21bmc_led, cdev);
+
+       mutex_lock(&led_lock);
+       led_val = i2c_smbus_read_byte_data(led->i2c_client,
+                                               BMC_CMD_LED_GET_SET);
+       if (led_val < 0)
+               goto err_out;
+
+       if (value = LED_OFF)
+               led_val &= ~led->led_bit;
+       else
+               led_val |= led->led_bit;
+
+       i2c_smbus_write_byte_data(led->i2c_client,
+                                       BMC_CMD_LED_GET_SET, led_val);
+err_out:
+       mutex_unlock(&led_lock);
+}
+
+static int menf21bmc_led_probe(struct platform_device *pdev)
+{
+       int i;
+       int ret;
+       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
+
+       for (i = 0; i < ARRAY_SIZE(leds); i++) {
+               leds[i].cdev.name = leds[i].name;
+               leds[i].cdev.brightness_set = menf21bmc_led_set;
+               leds[i].i2c_client = i2c_client;
+               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
+               if (ret < 0)
+                       goto err_free_leds;
+       }
+       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
+
+       return 0;
+
+err_free_leds:
+       dev_err(&pdev->dev, "failed to register LED device\n");
+
+       for (i = i - 1; i >= 0; i--)
+               led_classdev_unregister(&leds[i].cdev);
+
+       return ret;
+}
+
+static int menf21bmc_led_remove(struct platform_device *pdev)
+{
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(leds); i++)
+               led_classdev_unregister(&leds[i].cdev);
+
+       return 0;
+}
+
+static struct platform_driver menf21bmc_led = {
+       .probe          = menf21bmc_led_probe,
+       .remove         = menf21bmc_led_remove,
+       .driver         = {
+               .name           = "menf21bmc_led",

drivers/leds/leds-menf21bmc.c line 125

Quoted 4 lines

drivers/leds/leds-menf21bmc.c new file +4 -0

+               .owner          = THIS_MODULE,
+       },
+};
+

I still think this driver should be an I2C driver which is simply than a platform driver.
Did I miss some discussion about this in our email?

Added driver to support the 14F021P00 BMC Hardware Monitoring. The BMC is a Board Management Controller including monitoring of the board voltages.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 238 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/hwmon/menf21bmc_hwmon.c

drivers/hwmon/Kconfig +7 -0

index 37908ff..437c00f 100644
@@ -828,6 +828,13 @@ config SENSORS_MCP3021
 	  This driver can also be built as a module.  If so, the module
 	  will be called mcp3021.
 
+config SENSORS_MENF21BMC_HWMON
+	tristate "MEN 14F021P00 BMC Hardware Monitoring"
+	depends on MFD_MENF21BMC && I2C
+	help
+	  Say Y here to include support for the MEN 14F021P00 BMC
+	  hardware monitoring.
+
 config SENSORS_ADCXX
 	tristate "National Semiconductor ADCxxxSxxx"
 	depends on SPI_MASTER

drivers/hwmon/Makefile +1 -0

index 1362382..56ab872 100644
@@ -114,6 +114,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
+obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
 obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

new file mode 100644
index 0000000..ecb0ef1
@@ -0,0 +1,238 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) hwmon driver.
+ *

drivers/hwmon/menf21bmc_hwmon.c new file +32 -0

+#define BMC_VOLT_COUNT	5
+#define MENF21BMC_V33	0
+#define MENF21BMC_V5	1
+#define MENF21BMC_V12	2
+#define MENF21BMC_V5_SB	3
+#define MENF21BMC_VBAT	4
+
+#define BMC_CMD_GET_NUM_VOLT 0x8E
+#define IDX_TO_VOLT_MIN_CMD(idx) (0x40 + idx)
+#define IDX_TO_VOLT_MAX_CMD(idx) (0x50 + idx)
+#define IDX_TO_VOLT_INP_CMD(idx) (0x60 + idx)
+
+struct menf21bmc_hwmon {
+	char valid;
+	struct i2c_client *i2c_client;
+	unsigned long last_update;
+	u16 in_val[BMC_VOLT_COUNT];
+	u16 in_min[BMC_VOLT_COUNT];
+	u16 in_max[BMC_VOLT_COUNT];
+};
+
+static const char *const input_names[] = {
+	[MENF21BMC_V33]		= "MON_3,3V",
+	[MENF21BMC_V5]		= "MON_5V",
+	[MENF21BMC_V12]		= "MON_12V",
+	[MENF21BMC_V5_SB]	= "5V_STANDBY",
+	[MENF21BMC_VBAT]	= "VBAT"
+};
+
+static struct menf21bmc_hwmon *menf21bmc_hwmon_update(struct device *dev)
+{
+	int i;

drivers/hwmon/menf21bmc_hwmon.c new file +52 -0

+}
+
+static int menf21bmc_hwmon_get_volt_limits(struct menf21bmc_hwmon *drv_data)
+{
+	int i;
+	uint16_t val;
+
+	for (i = 0; i < BMC_VOLT_COUNT; i++) {
+		val = i2c_smbus_read_word_data(drv_data->i2c_client,
+					       IDX_TO_VOLT_MIN_CMD(i));
+		if (val < 0)
+			return val;
+
+		drv_data->in_min[i] = val;
+
+		val = i2c_smbus_read_word_data(drv_data->i2c_client,
+					       IDX_TO_VOLT_MAX_CMD(i));
+		if (val < 0)
+			return val;
+
+		drv_data->in_max[i] = val;
+	}
+	return 0;
+}
+
+static ssize_t
+show_label(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	return sprintf(buf, "%s\n", input_names[attr->index]);
+}
+
+static ssize_t
+show_in(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	uint16_t val;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	val = drv_data->in_val[attr->index];
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t
+show_min(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	return sprintf(buf, "%d\n", drv_data->in_min[attr->index]);
+}

drivers/hwmon/menf21bmc_hwmon.c new file +42 -0

+#define create_voltage_sysfs(idx)			\
+static SENSOR_DEVICE_ATTR(in##idx##_input, S_IRUGO,	\
+			show_in, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_min, S_IRUGO,	\
+			show_min, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_max, S_IRUGO,	\
+			show_max, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_label, S_IRUGO,	\
+			show_label, NULL, idx);
+
+create_voltage_sysfs(0);
+create_voltage_sysfs(1);
+create_voltage_sysfs(2);
+create_voltage_sysfs(3);
+create_voltage_sysfs(4);
+
+static struct attribute *menf21bmc_hwmon_attr_volts[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in0_min.dev_attr.attr,
+	&sensor_dev_attr_in0_max.dev_attr.attr,
+	&sensor_dev_attr_in0_label.dev_attr.attr,
+
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_min.dev_attr.attr,
+	&sensor_dev_attr_in1_max.dev_attr.attr,
+	&sensor_dev_attr_in1_label.dev_attr.attr,
+
+	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in2_min.dev_attr.attr,
+	&sensor_dev_attr_in2_max.dev_attr.attr,
+	&sensor_dev_attr_in2_label.dev_attr.attr,
+
+	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in3_min.dev_attr.attr,
+	&sensor_dev_attr_in3_max.dev_attr.attr,
+	&sensor_dev_attr_in3_label.dev_attr.attr,
+
+	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in4_min.dev_attr.attr,
+	&sensor_dev_attr_in4_max.dev_attr.attr,
+	&sensor_dev_attr_in4_label.dev_attr.attr,
+	NULL

drivers/hwmon/menf21bmc_hwmon.c new file +5 -0

+
+static int menf21bmc_hwmon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct menf21bmc_hwmon *drv_data;

drivers/hwmon/menf21bmc_hwmon.c new file +33 -0

+
+	ret = menf21bmc_hwmon_get_volt_limits(drv_data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to read sensor limits");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+							   DEVNAME, drv_data,
+							   menf21bmc_hwmon_groups);
+
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(&pdev->dev, "MEN 14F021P00 BMC hwmon device enabled");
+
+	return 0;
+}
+
+static struct platform_driver menf21bmc_hwmon = {
+	.probe		= menf21bmc_hwmon_probe,
+	.driver		= {
+		.name		= DEVNAME,
+		.owner		= THIS_MODULE,
+	},
+};
+
+module_platform_driver(menf21bmc_hwmon);
+
+MODULE_AUTHOR("Andreas Werner <[email protected]>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC hwmon");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_hwmon");

Signature · 6 lines

2.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 56 lines · Andreas Werner

Added driver to support the 14F021P00 BMC Hardware Monitoring. The BMC is a Board Management Controller including monitoring of the board voltages.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 238 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/hwmon/menf21bmc_hwmon.c

drivers/hwmon/Kconfig +7 -0

index 37908ff..437c00f 100644
@@ -828,6 +828,13 @@ config SENSORS_MCP3021
 	  This driver can also be built as a module.  If so, the module
 	  will be called mcp3021.
 
+config SENSORS_MENF21BMC_HWMON
+	tristate "MEN 14F021P00 BMC Hardware Monitoring"
+	depends on MFD_MENF21BMC && I2C
+	help
+	  Say Y here to include support for the MEN 14F021P00 BMC
+	  hardware monitoring.
+
 config SENSORS_ADCXX
 	tristate "National Semiconductor ADCxxxSxxx"
 	depends on SPI_MASTER

drivers/hwmon/Makefile +1 -0

index 1362382..56ab872 100644
@@ -114,6 +114,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
+obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
 obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o

drivers/hwmon/menf21bmc_hwmon.c new file +8 -0

new file mode 100644
index 0000000..ecb0ef1
@@ -0,0 +1,238 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) hwmon driver.
+ *
+ *  This is the core hwmon driver of the MEN 14F021P00 BMC.
+ *  The BMC monitors the board voltages which can be access with this
+ *  driver through sysfs.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH

drivers/hwmon/menf21bmc_hwmon.c line 13

Quoted 5 lines

drivers/hwmon/menf21bmc_hwmon.c new file +5 -0

+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.

The license statement below states GPL v2, which contradicts the statement here. Maybe it should just be GPL below ?

Quoted 4 lines

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

+ */
+
+#include <linux/module.h>

drivers/hwmon/menf21bmc_hwmon.c line 26

Quoted 10 lines

drivers/hwmon/menf21bmc_hwmon.c new file +10 -0

+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+
+#define DEVNAME "menf21bmc_hwmon"
+

While that makes sense for the driver name, it doesn't really for the hwmon device. You might want to consider just using "menf21bmc" when you register the hwmon device.

Quoted 39 lines

drivers/hwmon/menf21bmc_hwmon.c new file +39 -0

+#define BMC_VOLT_COUNT	5
+#define MENF21BMC_V33	0
+#define MENF21BMC_V5	1
+#define MENF21BMC_V12	2
+#define MENF21BMC_V5_SB	3
+#define MENF21BMC_VBAT	4
+
+#define BMC_CMD_GET_NUM_VOLT 0x8E
+#define IDX_TO_VOLT_MIN_CMD(idx) (0x40 + idx)
+#define IDX_TO_VOLT_MAX_CMD(idx) (0x50 + idx)
+#define IDX_TO_VOLT_INP_CMD(idx) (0x60 + idx)
+
+struct menf21bmc_hwmon {
+	char valid;
+	struct i2c_client *i2c_client;
+	unsigned long last_update;
+	u16 in_val[BMC_VOLT_COUNT];
+	u16 in_min[BMC_VOLT_COUNT];
+	u16 in_max[BMC_VOLT_COUNT];
+};
+
+static const char *const input_names[] = {
+	[MENF21BMC_V33]		= "MON_3,3V",
+	[MENF21BMC_V5]		= "MON_5V",
+	[MENF21BMC_V12]		= "MON_12V",
+	[MENF21BMC_V5_SB]	= "5V_STANDBY",
+	[MENF21BMC_VBAT]	= "VBAT"
+};
+
+static struct menf21bmc_hwmon *menf21bmc_hwmon_update(struct device *dev)
+{
+	int i;
+	uint16_t val;
+	struct menf21bmc_hwmon *drv_data = dev_get_drvdata(dev);
+	struct menf21bmc_hwmon *data_ret = drv_data;
+
+	if (time_after(jiffies, drv_data->last_update + HZ) || !drv_data->valid) {
+		for (i = 0; i < BMC_VOLT_COUNT; i++) {
+			val = i2c_smbus_read_word_data(drv_data->i2c_client,

drivers/hwmon/menf21bmc_hwmon.c line 68

Quoted 3 lines

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

+						       IDX_TO_VOLT_INP_CMD(i));
+			if (val < 0)
+				goto abort;

I don't really see the point here. You ignore the return value, yet you abort reading further values. The caller will assume that there was no error even though the values in in_val were either never initialized or are out of date.

Quoted 5 lines

drivers/hwmon/menf21bmc_hwmon.c new file +5 -0

+
+			drv_data->in_val[i] = val;
+		}
+		drv_data->last_update = jiffies;
+		drv_data->valid = 1;

drivers/hwmon/menf21bmc_hwmon.c line 76

Quoted 3 lines

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

+	}
+abort:
+	return data_ret;

At the same time you do have a separate variable to return.
One of those doesn't make sense.
If the idea was to return an error in data_ret, you would have to assign it accordingly, and check in the calling code if there was an error or not.

Quoted 59 lines

drivers/hwmon/menf21bmc_hwmon.c new file +58 -0

+}
+
+static int menf21bmc_hwmon_get_volt_limits(struct menf21bmc_hwmon *drv_data)
+{
+	int i;
+	uint16_t val;
+
+	for (i = 0; i < BMC_VOLT_COUNT; i++) {
+		val = i2c_smbus_read_word_data(drv_data->i2c_client,
+					       IDX_TO_VOLT_MIN_CMD(i));
+		if (val < 0)
+			return val;
+
+		drv_data->in_min[i] = val;
+
+		val = i2c_smbus_read_word_data(drv_data->i2c_client,
+					       IDX_TO_VOLT_MAX_CMD(i));
+		if (val < 0)
+			return val;
+
+		drv_data->in_max[i] = val;
+	}
+	return 0;
+}
+
+static ssize_t
+show_label(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	return sprintf(buf, "%s\n", input_names[attr->index]);
+}
+
+static ssize_t
+show_in(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	uint16_t val;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	val = drv_data->in_val[attr->index];
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t
+show_min(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	return sprintf(buf, "%d\n", drv_data->in_min[attr->index]);
+}
+
+static ssize_t
+show_max(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);

drivers/hwmon/menf21bmc_hwmon.c line 138

Quoted 4 lines

drivers/hwmon/menf21bmc_hwmon.c new file +4 -0

+
+	return sprintf(buf, "%d\n", drv_data->in_max[attr->index]);
+}
+

You could merge all the above functions into one by using a dual-indexed array and sensor_device_attribute_2. If you don't, it does not really make sense to call _hwmon_update to update the input voltages just to display the limits (which you don't update).

Quoted 46 lines

drivers/hwmon/menf21bmc_hwmon.c new file +46 -0

+#define create_voltage_sysfs(idx)			\
+static SENSOR_DEVICE_ATTR(in##idx##_input, S_IRUGO,	\
+			show_in, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_min, S_IRUGO,	\
+			show_min, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_max, S_IRUGO,	\
+			show_max, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_label, S_IRUGO,	\
+			show_label, NULL, idx);
+
+create_voltage_sysfs(0);
+create_voltage_sysfs(1);
+create_voltage_sysfs(2);
+create_voltage_sysfs(3);
+create_voltage_sysfs(4);
+
+static struct attribute *menf21bmc_hwmon_attr_volts[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in0_min.dev_attr.attr,
+	&sensor_dev_attr_in0_max.dev_attr.attr,
+	&sensor_dev_attr_in0_label.dev_attr.attr,
+
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_min.dev_attr.attr,
+	&sensor_dev_attr_in1_max.dev_attr.attr,
+	&sensor_dev_attr_in1_label.dev_attr.attr,
+
+	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in2_min.dev_attr.attr,
+	&sensor_dev_attr_in2_max.dev_attr.attr,
+	&sensor_dev_attr_in2_label.dev_attr.attr,
+
+	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in3_min.dev_attr.attr,
+	&sensor_dev_attr_in3_max.dev_attr.attr,
+	&sensor_dev_attr_in3_label.dev_attr.attr,
+
+	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in4_min.dev_attr.attr,
+	&sensor_dev_attr_in4_max.dev_attr.attr,
+	&sensor_dev_attr_in4_label.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group menf21bmc_hwmon_attr_group_volts = {
+	.attrs = menf21bmc_hwmon_attr_volts,

drivers/hwmon/menf21bmc_hwmon.c line 190

Quoted 6 lines

drivers/hwmon/menf21bmc_hwmon.c new file +6 -0

+};
+
+static const struct attribute_group *menf21bmc_hwmon_groups[] = {
+	&menf21bmc_hwmon_attr_group_volts,
+	NULL
+};

Please use the ATTRIBUTE_GROUPS() macro.

Quoted 10 lines

drivers/hwmon/menf21bmc_hwmon.c new file +9 -0

+
+static int menf21bmc_hwmon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct menf21bmc_hwmon *drv_data;
+	struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
+	struct device *hwmon_dev;
+
+	drv_data = devm_kzalloc(&pdev->dev, sizeof(struct menf21bmc_hwmon),

drivers/hwmon/menf21bmc_hwmon.c line 205

Quoted 6 lines

drivers/hwmon/menf21bmc_hwmon.c new file +6 -0

+				GFP_KERNEL);
+	if (!drv_data)
+		return -ENOMEM;
+
+	drv_data->i2c_client = i2c_client;
+	platform_set_drvdata(pdev, drv_data);

What is this used for ?

Quoted 42 lines

drivers/hwmon/menf21bmc_hwmon.c new file +33 -0

+
+	ret = menf21bmc_hwmon_get_volt_limits(drv_data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to read sensor limits");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+							   DEVNAME, drv_data,
+							   menf21bmc_hwmon_groups);
+
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(&pdev->dev, "MEN 14F021P00 BMC hwmon device enabled");
+
+	return 0;
+}
+
+static struct platform_driver menf21bmc_hwmon = {
+	.probe		= menf21bmc_hwmon_probe,
+	.driver		= {
+		.name		= DEVNAME,
+		.owner		= THIS_MODULE,
+	},
+};
+
+module_platform_driver(menf21bmc_hwmon);
+
+MODULE_AUTHOR("Andreas Werner <[email protected]>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC hwmon");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_hwmon");

Signature · 6 lines

2.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 57 lines · Guenter Roeck

Quoted 56 lines · Andreas Werner

Added driver to support the 14F021P00 BMC Hardware Monitoring. The BMC is a Board Management Controller including monitoring of the board voltages.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 238 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/hwmon/menf21bmc_hwmon.c

drivers/hwmon/Kconfig +7 -0

index 37908ff..437c00f 100644
@@ -828,6 +828,13 @@ config SENSORS_MCP3021
 	  This driver can also be built as a module.  If so, the module
 	  will be called mcp3021.
 
+config SENSORS_MENF21BMC_HWMON
+	tristate "MEN 14F021P00 BMC Hardware Monitoring"
+	depends on MFD_MENF21BMC && I2C
+	help
+	  Say Y here to include support for the MEN 14F021P00 BMC
+	  hardware monitoring.
+
 config SENSORS_ADCXX
 	tristate "National Semiconductor ADCxxxSxxx"
 	depends on SPI_MASTER

drivers/hwmon/Makefile +1 -0

index 1362382..56ab872 100644
@@ -114,6 +114,7 @@ obj-$(CONFIG_SENSORS_MAX6650)	+= max6650.o
 obj-$(CONFIG_SENSORS_MAX6697)	+= max6697.o
 obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
 obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
+obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
 obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o

drivers/hwmon/menf21bmc_hwmon.c new file +8 -0

new file mode 100644
index 0000000..ecb0ef1
@@ -0,0 +1,238 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) hwmon driver.
+ *
+ *  This is the core hwmon driver of the MEN 14F021P00 BMC.
+ *  The BMC monitors the board voltages which can be access with this
+ *  driver through sysfs.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH

Quoted 8 lines

drivers/hwmon/menf21bmc_hwmon.c line 13

Quoted 5 lines

drivers/hwmon/menf21bmc_hwmon.c new file +5 -0

+ *
+ *  This program is free software; you can redistribute  it and/or modify it
+ *  under  the terms of  the GNU General  Public License as published by the
+ *  Free Software Foundation;  either version 2 of the  License, or (at your
+ *  option) any later version.

The license statement below states GPL v2, which contradicts the statement here. Maybe it should just be GPL below ?

Why dos it contradict? This is both GPLv2, in the header and in the MODULE_LICENSE like i have changed it in the other slave devices and the mfd.

Quoted 18 lines

Quoted 4 lines

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

+ */
+
+#include <linux/module.h>

drivers/hwmon/menf21bmc_hwmon.c line 26

Quoted 10 lines

drivers/hwmon/menf21bmc_hwmon.c new file +10 -0

+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+
+#define DEVNAME "menf21bmc_hwmon"
+

While that makes sense for the driver name, it doesn't really for the hwmon device. You might want to consider just using "menf21bmc" when you register the hwmon device.

menf21bmc is the name of the MFD driver. This name is to distinguish each of the slaves. For that the name represent that this device is a slave of "menf21bmc" and a "hwmon" of its own.

Quoted 39 lines

Quoted 39 lines

drivers/hwmon/menf21bmc_hwmon.c new file +39 -0

+#define BMC_VOLT_COUNT	5
+#define MENF21BMC_V33	0
+#define MENF21BMC_V5	1
+#define MENF21BMC_V12	2
+#define MENF21BMC_V5_SB	3
+#define MENF21BMC_VBAT	4
+
+#define BMC_CMD_GET_NUM_VOLT 0x8E
+#define IDX_TO_VOLT_MIN_CMD(idx) (0x40 + idx)
+#define IDX_TO_VOLT_MAX_CMD(idx) (0x50 + idx)
+#define IDX_TO_VOLT_INP_CMD(idx) (0x60 + idx)
+
+struct menf21bmc_hwmon {
+	char valid;
+	struct i2c_client *i2c_client;
+	unsigned long last_update;
+	u16 in_val[BMC_VOLT_COUNT];
+	u16 in_min[BMC_VOLT_COUNT];
+	u16 in_max[BMC_VOLT_COUNT];
+};
+
+static const char *const input_names[] = {
+	[MENF21BMC_V33]		= "MON_3,3V",
+	[MENF21BMC_V5]		= "MON_5V",
+	[MENF21BMC_V12]		= "MON_12V",
+	[MENF21BMC_V5_SB]	= "5V_STANDBY",
+	[MENF21BMC_VBAT]	= "VBAT"
+};
+
+static struct menf21bmc_hwmon *menf21bmc_hwmon_update(struct device *dev)
+{
+	int i;
+	uint16_t val;
+	struct menf21bmc_hwmon *drv_data = dev_get_drvdata(dev);
+	struct menf21bmc_hwmon *data_ret = drv_data;
+
+	if (time_after(jiffies, drv_data->last_update + HZ) || !drv_data->valid) {
+		for (i = 0; i < BMC_VOLT_COUNT; i++) {
+			val = i2c_smbus_read_word_data(drv_data->i2c_client,

Quoted 9 lines

drivers/hwmon/menf21bmc_hwmon.c line 68

Quoted 3 lines

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

+						       IDX_TO_VOLT_INP_CMD(i));
+			if (val < 0)
+				goto abort;

I don't really see the point here. You ignore the return value, yet you abort reading further values. The caller will assume that there was no error even though the values in in_val were either never initialized or are out of date.

Argh thats true.
I have checked out some other drivers and they just read the value from the i2c bus and save the value into the struct without error checking. Is that the normal way?
If yes then the caller hast to check if the value is a correct voltage value.

The other way what i see is to read the value, check if there is no error and then save the new value to the struct. If there is an error during read then we just do not update the value and let it as it is.

If I go this way then i do not need the second struct as the return value.

Quoted 5 lines

Quoted 5 lines

drivers/hwmon/menf21bmc_hwmon.c new file +5 -0

+
+			drv_data->in_val[i] = val;
+		}
+		drv_data->last_update = jiffies;
+		drv_data->valid = 1;

Quoted 8 lines

drivers/hwmon/menf21bmc_hwmon.c line 76

Quoted 3 lines

drivers/hwmon/menf21bmc_hwmon.c new file +3 -0

+	}
+abort:
+	return data_ret;

At the same time you do have a separate variable to return.
One of those doesn't make sense.
If the idea was to return an error in data_ret, you would have to assign it accordingly, and check in the calling code if there was an error or not.

Will be changed, see above.

Quoted 59 lines

Quoted 59 lines

drivers/hwmon/menf21bmc_hwmon.c new file +58 -0

+}
+
+static int menf21bmc_hwmon_get_volt_limits(struct menf21bmc_hwmon *drv_data)
+{
+	int i;
+	uint16_t val;
+
+	for (i = 0; i < BMC_VOLT_COUNT; i++) {
+		val = i2c_smbus_read_word_data(drv_data->i2c_client,
+					       IDX_TO_VOLT_MIN_CMD(i));
+		if (val < 0)
+			return val;
+
+		drv_data->in_min[i] = val;
+
+		val = i2c_smbus_read_word_data(drv_data->i2c_client,
+					       IDX_TO_VOLT_MAX_CMD(i));
+		if (val < 0)
+			return val;
+
+		drv_data->in_max[i] = val;
+	}
+	return 0;
+}
+
+static ssize_t
+show_label(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+
+	return sprintf(buf, "%s\n", input_names[attr->index]);
+}
+
+static ssize_t
+show_in(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	uint16_t val;
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	val = drv_data->in_val[attr->index];
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t
+show_min(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	return sprintf(buf, "%d\n", drv_data->in_min[attr->index]);
+}
+
+static ssize_t
+show_max(struct device *dev, struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);

Quoted 9 lines

drivers/hwmon/menf21bmc_hwmon.c line 138

Quoted 4 lines

drivers/hwmon/menf21bmc_hwmon.c new file +4 -0

+
+	return sprintf(buf, "%d\n", drv_data->in_max[attr->index]);
+}
+

You could merge all the above functions into one by using a dual-indexed array and sensor_device_attribute_2. If you don't, it does not really make sense to call _hwmon_update to update the input voltages just to display the limits (which you don't update).

I do not want to merge it to one function. I will delete the upate call for min and max values.

Quoted 46 lines

Quoted 46 lines

drivers/hwmon/menf21bmc_hwmon.c new file +46 -0

+#define create_voltage_sysfs(idx)			\
+static SENSOR_DEVICE_ATTR(in##idx##_input, S_IRUGO,	\
+			show_in, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_min, S_IRUGO,	\
+			show_min, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_max, S_IRUGO,	\
+			show_max, NULL, idx);		\
+static SENSOR_DEVICE_ATTR(in##idx##_label, S_IRUGO,	\
+			show_label, NULL, idx);
+
+create_voltage_sysfs(0);
+create_voltage_sysfs(1);
+create_voltage_sysfs(2);
+create_voltage_sysfs(3);
+create_voltage_sysfs(4);
+
+static struct attribute *menf21bmc_hwmon_attr_volts[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in0_min.dev_attr.attr,
+	&sensor_dev_attr_in0_max.dev_attr.attr,
+	&sensor_dev_attr_in0_label.dev_attr.attr,
+
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in1_min.dev_attr.attr,
+	&sensor_dev_attr_in1_max.dev_attr.attr,
+	&sensor_dev_attr_in1_label.dev_attr.attr,
+
+	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in2_min.dev_attr.attr,
+	&sensor_dev_attr_in2_max.dev_attr.attr,
+	&sensor_dev_attr_in2_label.dev_attr.attr,
+
+	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in3_min.dev_attr.attr,
+	&sensor_dev_attr_in3_max.dev_attr.attr,
+	&sensor_dev_attr_in3_label.dev_attr.attr,
+
+	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in4_min.dev_attr.attr,
+	&sensor_dev_attr_in4_max.dev_attr.attr,
+	&sensor_dev_attr_in4_label.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group menf21bmc_hwmon_attr_group_volts = {
+	.attrs = menf21bmc_hwmon_attr_volts,

Quoted 8 lines

drivers/hwmon/menf21bmc_hwmon.c line 190

Quoted 6 lines

drivers/hwmon/menf21bmc_hwmon.c new file +6 -0

+};
+
+static const struct attribute_group *menf21bmc_hwmon_groups[] = {
+	&menf21bmc_hwmon_attr_group_volts,
+	NULL
+};

Please use the ATTRIBUTE_GROUPS() macro.

Ok i will use the macro.

Quoted 10 lines

Quoted 10 lines

drivers/hwmon/menf21bmc_hwmon.c new file +9 -0

+
+static int menf21bmc_hwmon_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct menf21bmc_hwmon *drv_data;
+	struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
+	struct device *hwmon_dev;
+
+	drv_data = devm_kzalloc(&pdev->dev, sizeof(struct menf21bmc_hwmon),

Quoted 8 lines

drivers/hwmon/menf21bmc_hwmon.c line 205

Quoted 6 lines

drivers/hwmon/menf21bmc_hwmon.c new file +6 -0

+				GFP_KERNEL);
+	if (!drv_data)
+		return -ENOMEM;
+
+	drv_data->i2c_client = i2c_client;
+	platform_set_drvdata(pdev, drv_data);

What is this used for ?

For the update function. I need to access the drv_data there. May this could change, depends on the changes of the update function.

Quoted 37 lines

Quoted 37 lines

drivers/hwmon/menf21bmc_hwmon.c new file +33 -0

+
+	ret = menf21bmc_hwmon_get_volt_limits(drv_data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to read sensor limits");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&pdev->dev,
+							   DEVNAME, drv_data,
+							   menf21bmc_hwmon_groups);
+
+	if (IS_ERR(hwmon_dev))
+		return PTR_ERR(hwmon_dev);
+
+	dev_info(&pdev->dev, "MEN 14F021P00 BMC hwmon device enabled");
+
+	return 0;
+}
+
+static struct platform_driver menf21bmc_hwmon = {
+	.probe		= menf21bmc_hwmon_probe,
+	.driver		= {
+		.name		= DEVNAME,
+		.owner		= THIS_MODULE,
+	},
+};
+
+module_platform_driver(menf21bmc_hwmon);
+
+MODULE_AUTHOR("Andreas Werner <[email protected]>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC hwmon");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_hwmon");

Signature · 1 line

2.0.4

Quoted 5 lines

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Added driver to support the 14F021P00 BMC Hardware Monitoring. The BMC is a Board Management Controller including monitoring of the board voltages.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 238 ++++++++++++++++++++++++++++++++++++++++

Quoted 4 lines · Andreas Werner

Added driver to support the 14F021P00 BMC Hardware Monitoring. The BMC is a Board Management Controller including monitoring of the board voltages.

Quoted 5 lines

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 238 ++++++++++++++++++++++++++++++++++++++++

Forgot: Please also provide Documentation/hwmon/menf21bmc.

Thanks,
Guenter

Quoted 11 lines · Guenter Roeck

Quoted 9 lines · Andreas Werner

Added driver to support the 14F021P00 BMC Hardware Monitoring. The BMC is a Board Management Controller including monitoring of the board voltages.

Signed-off-by Andreas Werner <[email protected]>

---
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 238 ++++++++++++++++++++++++++++++++++++++++

Quoted 2 lines

Forgot: Please also provide Documentation/hwmon/menf21bmc.

Did not know that i have to do it.
But you are right it is good to have one.