Re: [PATCH v5 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver

Re: [PATCH v5 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver

[Guenter Roeck]

On Tue, Aug 26, 2014 at 07:46:53PM +0200, Andreas Werner wrote:
> 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 <andreas.werner@men.de>

Hi Andreas,

Couple of additional comments below. Sorry I didn't notice earlier.

Guenter

> ---
>  Documentation/hwmon/menf21bmc   |  49 +++++++++
>  drivers/hwmon/Kconfig           |   7 ++
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/menf21bmc_hwmon.c | 230 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 287 insertions(+)
>  create mode 100644 Documentation/hwmon/menf21bmc
>  create mode 100644 drivers/hwmon/menf21bmc_hwmon.c
> 
> diff --git a/Documentation/hwmon/menf21bmc b/Documentation/hwmon/menf21bmc
> new file mode 100644
> index 0000000..22b6840
> --- /dev/null
> +++ b/Documentation/hwmon/menf21bmc
> @@ -0,0 +1,49 @@
> +Kernel driver menf21bmc_hwmon
> +=============================
> +
> +Supported chips:
> +	* MEN 14F021P00
> +	  Prefix: 'menf21bmc_hwmon'
> +	  Adresses scanned: -
> +
> +Author: Andreas Werner <andreas.werner@men.de>
> +
> +Description
> +-----------
> +
> +The menf21bmc is a Board Management Controller (BMC) which provides an I2C
> +interface to the host to access the features implemented in the BMC.
> +
> +This driver gives access to the voltage monitoring feature of the main
> +voltages of the board.
> +The voltage sensors are connected to the ADC inputs of the BMC which is
> +a PIC16F917 Mikrocontroller.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not auto-detect devices. You will have to instantiate the
> +devices explicitly. Please see Documentation/i2c/instantiating-devices for
> +details.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. All attributes are read only
> +The Limits are read once by the driver.
> +
> +in0_input	+3.3V input voltage
> +in1_input	+5.0V input voltage
> +in2_input	+12.0V input voltage
> +in3_input	+5V Standby input voltage
> +in4_input	VBAT (on board battery)
> +
> +in[0-4]_min	Minimum voltage limit
> +in[0-4]_max	Maximum voltage limit
> +
> +in0_label	"MON_3_3V"
> +in1_label	"MON_5V"
> +in2_label	"MON_12V"
> +in3_label	"5V_STANDBY"
> +in4_label	"VBAT"
> +

The empty line adds a whitespace error when applying the patch.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 37908ff..db3a6eb 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -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
> +	help
> +	  Say Y here to include support for the MEN 14F021P00 BMC
> +	  hardware monitoring.
> +
It is customary to add a note describing how the module is called
if the driver is built as module.

>  config SENSORS_ADCXX
>  	tristate "National Semiconductor ADCxxxSxxx"
>  	depends on SPI_MASTER
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 1362382..56ab872 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -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
> diff --git a/drivers/hwmon/menf21bmc_hwmon.c b/drivers/hwmon/menf21bmc_hwmon.c
> new file mode 100644
> index 0000000..2eaec6a
> --- /dev/null
> +++ b/drivers/hwmon/menf21bmc_hwmon.c
> @@ -0,0 +1,230 @@
> +/*
> + *  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
> + *
> + *  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/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/jiffies.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +
> +#define DRV_NAME  "menf21bmc_hwmon"
> +
> +#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 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;

Please use bool (and true / false to set it). char doesn't
save anything and may actually increase code size depending
on the architecture.

> +	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;

That doesn't work well with the checks against < 0.
Please compile with W=1 to find all instances of this problem
(there is another one below).

> +	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) {

Line over 80 characters.

> +		for (i = 0; i < BMC_VOLT_COUNT; i++) {
> +			val = i2c_smbus_read_word_data(drv_data->i2c_client,
> +						       IDX_TO_VOLT_INP_CMD(i));
> +			if (val < 0) {
> +				data_ret = ERR_PTR(val);
> +				goto abort;
> +			}
> +			drv_data->in_val[i] = val;
> +		}
> +		drv_data->last_update = jiffies;
> +		drv_data->valid = 1;
> +	}
> +abort:
> +	return data_ret;
> +}
> +
> +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)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
> +
> +	if (IS_ERR(drv_data))
> +		return PTR_ERR(drv_data);
> +
> +	return sprintf(buf, "%d\n", drv_data->in_val[attr->index]);
> +}
> +
> +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 = dev_get_drvdata(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 = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%d\n", drv_data->in_max[attr->index]);
> +}
> +
> +#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_attrs[] = {
> +	&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
> +};
> +
> +ATTRIBUTE_GROUPS(menf21bmc_hwmon);
> +
> +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),
> +				GFP_KERNEL);
> +	if (!drv_data)
> +		return -ENOMEM;
> +
> +	drv_data->i2c_client = i2c_client;
> +
> +	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,
> +							   "menf21bmc", drv_data,
> +							   menf21bmc_hwmon_groups);

Line over 80 characters. Just use less indentation for the continuation lines.

> +	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		= DRV_NAME,
> +		.owner		= THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(menf21bmc_hwmon);
> +
> +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> +MODULE_DESCRIPTION("MEN 14F021P00 BMC hwmon");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:menf21bmc_hwmon");
> -- 
> 2.0.4
> 
> 

[PATCH v5 0/4] Introduce MEN 14F021P00 BMC driver series

[Andreas Werner]

This patch set add support for the MEN 14F021P00 Board Management called BMC.

The BMC is a PIC Mikrocontroller which assembled on almost all of our
3U Compact PCI CPU board and a few Box PCs.
main part of the BMC is to start and monitor the board, but there are a
lot more features which can be accessed using an I2C Host interface

Features supported in this Patchset:
        - Watchdog
        - LEDs
        - HWMON

The Patchset includes a MFD Core driver, Watchdog, hwmon  and LEDs driver.

Changes in v5:
	- fixed I2C dependency in MFD Kconfig
	- added correct error handling in update function of the hwmon slave
	  (Use of ERR_PTR)
	- add ATTRIBUTE_GROUP Macro in hwmon slave
	  and deleted platform_set_drvdata

Changes in v4:
        - fixed some style issues
        - fixed GPL license version
        - added hwmon voltage monitoring slave driver

Changes in v3:
        - deleted i2c_smbus wrapper functions and use native one
        - some cosmentics and variable renaming to be more clear
        - renamed "leave production" mode to "exit production mode"

Changes in v2:
        - changed i2c_smbus_read wrapper function to return both, value
          and error.
        - moved "leave production mode" from Watchdog driver to mfd core.
        - fixed some return values in the watchdog driver to return the original
          error value instead of -EIO.


Andreas Werner (4):
  drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver
  drivers/watchdog/menf21bmc_wdt: introduce MEN 14F021P00 BMC Watchdog
    driver
  drivers/leds/leds-menf21bmc: introduce MEN 14F021P00 BMC LED driver
  drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver

 Documentation/hwmon/menf21bmc    |  49 +++++++++
 drivers/hwmon/Kconfig            |   7 ++
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/menf21bmc_hwmon.c  | 230 +++++++++++++++++++++++++++++++++++++++
 drivers/leds/Kconfig             |   6 +
 drivers/leds/Makefile            |   1 +
 drivers/leds/leds-menf21bmc.c    | 131 ++++++++++++++++++++++
 drivers/mfd/Kconfig              |  12 ++
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/menf21bmc.c          | 132 ++++++++++++++++++++++
 drivers/watchdog/Kconfig         |   7 ++
 drivers/watchdog/Makefile        |   1 +
 drivers/watchdog/menf21bmc_wdt.c | 203 ++++++++++++++++++++++++++++++++++
 13 files changed, 781 insertions(+)
 create mode 100644 Documentation/hwmon/menf21bmc
 create mode 100644 drivers/hwmon/menf21bmc_hwmon.c
 create mode 100644 drivers/leds/leds-menf21bmc.c
 create mode 100644 drivers/mfd/menf21bmc.c
 create mode 100644 drivers/watchdog/menf21bmc_wdt.c

-- 
2.0.4

[PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[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:
 	- Watchdog
 	- LEDs
	- Hwmon (voltage monitoring)

Signed-off-by: Andreas Werner <andreas.werner@men.de>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b8d9ca0..6a9f101 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -453,6 +453,18 @@ config MFD_MAX8998
 	  additional drivers must be enabled in order to use the functionality
 	  of the device.
 
+config MFD_MENF21BMC
+	tristate "MEN 14F021P00 Board Management Controller Support"
+	depends on I2C
+	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.
+
 config EZX_PCAP
 	bool "Motorola EZXPCAP Support"
 	depends on SPI_MASTER
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 4e2bc25..37bf336 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
 obj-$(CONFIG_MFD_AS3722)	+= as3722.o
 obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
 obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
+obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
 
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
new file mode 100644
index 0000000..a6eb03f
--- /dev/null
+++ b/drivers/mfd/menf21bmc.c
@@ -0,0 +1,132 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
+ *
+ *  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/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+
+#define BMC_CMD_WDT_EXIT_PROD	0x18
+#define BMC_CMD_WDT_PROD_STAT	0x19
+#define BMC_CMD_REV_MAJOR	0x80
+#define BMC_CMD_REV_MINOR	0x81
+#define BMC_CMD_REV_MAIN	0x82
+
+static struct mfd_cell menf21bmc_cell[] = {
+	{ .name = "menf21bmc_wdt", },
+	{ .name = "menf21bmc_led", },
+	{ .name = "menf21bmc_hwmon", }
+};
+
+static int menf21bmc_wdt_exit_prod_mode(struct i2c_client *client)
+{
+	int val, ret;
+
+	val = i2c_smbus_read_byte_data(client, BMC_CMD_WDT_PROD_STAT);
+	if (val < 0)
+		return val;
+
+	/*
+	 * Production mode should be not active after delivery of the Board.
+	 * To be sure we check it, inform the user and exit the mode
+	 * if active.
+	 */
+	if (val == 0x00) {
+		dev_info(&client->dev,
+			"BMC in production mode. Exit production mode\n");
+
+		ret = i2c_smbus_write_byte(client, BMC_CMD_WDT_EXIT_PROD);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
+{
+	int ret;
+	int rev_major, rev_minor, rev_main;
+
+	ret = i2c_check_functionality(client->adapter,
+				      I2C_FUNC_SMBUS_BYTE_DATA |
+				      I2C_FUNC_SMBUS_WORD_DATA |
+				      I2C_FUNC_SMBUS_BYTE);
+	if (!ret)
+		return -ENODEV;
+
+	rev_major = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAJOR);
+	if (rev_major < 0) {
+		dev_err(&client->dev, "failed to get BMC major revision\n");
+		return rev_major;
+	}
+
+	rev_minor = i2c_smbus_read_word_data(client, BMC_CMD_REV_MINOR);
+	if (rev_minor < 0) {
+		dev_err(&client->dev, "failed to get BMC minor revision\n");
+		return rev_minor;
+	}
+
+	rev_main = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAIN);
+	if (rev_main < 0) {
+		dev_err(&client->dev, "failed to get BMC main revision\n");
+		return rev_main;
+	}
+
+	dev_info(&client->dev, "FW Revision: %02d.%02d.%02d\n",
+					rev_major, rev_minor, rev_main);
+
+	/*
+	 * We have to exit the Production Mode of the BMC to activate the
+	 * Watchdog functionality and the BIOS life sign monitoring.
+	 */
+	ret = menf21bmc_wdt_exit_prod_mode(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to leave production mode\n");
+		return ret;
+	}
+
+	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
+				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to add BMC sub-devices\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int menf21bmc_remove(struct i2c_client *client)
+{
+	mfd_remove_devices(&client->dev);
+	return 0;
+}
+
+static const struct i2c_device_id menf21bmc_id_table[] = {
+	{ "menf21bmc", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
+
+static struct i2c_driver menf21bmc_driver = {
+	.driver.name	= "menf21bmc",
+	.id_table	= menf21bmc_id_table,
+	.probe		= menf21bmc_probe,
+	.remove		= menf21bmc_remove,
+};
+
+module_i2c_driver(menf21bmc_driver);
+
+MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_LICENSE("GPL v2");
-- 
2.0.4

[PATCH v5 2/4] drivers/watchdog/menf21bmc_wdt: introduce MEN 14F021P00 BMC Watchdog driver

[Andreas Werner]

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

Signed-off-by: Andreas Werner <andreas.werner@men.de>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/Kconfig         |   7 ++
 drivers/watchdog/Makefile        |   1 +
 drivers/watchdog/menf21bmc_wdt.c | 203 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 211 insertions(+)
 create mode 100644 drivers/watchdog/menf21bmc_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 76dd541..f96431d 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -95,6 +95,13 @@ config GPIO_WATCHDOG
 	  If you say yes here you get support for watchdog device
 	  controlled through GPIO-line.
 
+config MENF21BMC_WATCHDOG
+	tristate "MEN 14F021P00 BMC Watchdog"
+	depends on MFD_MENF21BMC
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for the MEN 14F021P00 BMC Watchdog.
+
 config WM831X_WATCHDOG
 	tristate "WM831x watchdog"
 	depends on MFD_WM831X
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 468c320..de17014 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -178,3 +178,4 @@ obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o
 obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o
 obj-$(CONFIG_MAX63XX_WATCHDOG) += max63xx_wdt.o
 obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o
+obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
diff --git a/drivers/watchdog/menf21bmc_wdt.c b/drivers/watchdog/menf21bmc_wdt.c
new file mode 100644
index 0000000..604824d
--- /dev/null
+++ b/drivers/watchdog/menf21bmc_wdt.c
@@ -0,0 +1,203 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) Watchdog Driver.
+ *
+ *  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/kernel.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/watchdog.h>
+#include <linux/platform_device.h>
+#include <linux/i2c.h>
+
+#define DEVNAME "menf21bmc_wdt"
+
+#define BMC_CMD_WD_ON		0x11
+#define BMC_CMD_WD_OFF		0x12
+#define BMC_CMD_WD_TRIG		0x13
+#define BMC_CMD_WD_TIME		0x14
+#define BMC_CMD_WD_STATE	0x17
+#define BMC_WD_OFF_VAL		0x69
+#define BMC_CMD_RST_RSN		0x92
+
+#define BMC_WD_TIMEOUT_MIN	1	/* in sec */
+#define BMC_WD_TIMEOUT_MAX	6553	/* in sec */
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+struct menf21bmc_wdt {
+	struct watchdog_device wdt;
+	struct i2c_client *i2c_client;
+};
+
+static int menf21bmc_wdt_set_bootstatus(struct menf21bmc_wdt *data)
+{
+	int rst_rsn;
+
+	rst_rsn = i2c_smbus_read_byte_data(data->i2c_client, BMC_CMD_RST_RSN);
+	if (rst_rsn < 0)
+		return rst_rsn;
+
+	if (rst_rsn == 0x02)
+		data->wdt.bootstatus |= WDIOF_CARDRESET;
+	else if (rst_rsn == 0x05)
+		data->wdt.bootstatus |= WDIOF_EXTERN1;
+	else if (rst_rsn == 0x06)
+		data->wdt.bootstatus |= WDIOF_EXTERN2;
+	else if (rst_rsn == 0x0A)
+		data->wdt.bootstatus |= WDIOF_POWERUNDER;
+
+	return 0;
+}
+
+static int menf21bmc_wdt_start(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wdt *drv_data = watchdog_get_drvdata(wdt);
+
+	return i2c_smbus_write_byte(drv_data->i2c_client, BMC_CMD_WD_ON);
+}
+
+static int menf21bmc_wdt_stop(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wdt *drv_data = watchdog_get_drvdata(wdt);
+
+	return i2c_smbus_write_byte_data(drv_data->i2c_client,
+					BMC_CMD_WD_OFF, BMC_WD_OFF_VAL);
+}
+
+static int
+menf21bmc_wdt_settimeout(struct watchdog_device *wdt, unsigned int timeout)
+{
+	int ret;
+	struct menf21bmc_wdt *drv_data = watchdog_get_drvdata(wdt);
+
+	/*
+	 *  BMC Watchdog does have a resolution of 100ms.
+	 *  Watchdog API defines the timeout in seconds, so we have to
+	 *  multiply the value.
+	 */
+	ret = i2c_smbus_write_word_data(drv_data->i2c_client,
+						BMC_CMD_WD_TIME, timeout * 10);
+	if (ret < 0)
+		return ret;
+
+	wdt->timeout = timeout;
+
+	return 0;
+}
+
+static int menf21bmc_wdt_ping(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wdt *drv_data = watchdog_get_drvdata(wdt);
+
+	return i2c_smbus_write_byte(drv_data->i2c_client, BMC_CMD_WD_TRIG);
+}
+
+static const struct watchdog_info menf21bmc_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = DEVNAME,
+};
+
+static const struct watchdog_ops menf21bmc_wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= menf21bmc_wdt_start,
+	.stop		= menf21bmc_wdt_stop,
+	.ping		= menf21bmc_wdt_ping,
+	.set_timeout	= menf21bmc_wdt_settimeout,
+};
+
+static int menf21bmc_wdt_probe(struct platform_device *pdev)
+{
+	int ret, bmc_timeout;
+	struct menf21bmc_wdt *drv_data;
+	struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
+
+	drv_data = devm_kzalloc(&pdev->dev,
+				sizeof(struct menf21bmc_wdt), GFP_KERNEL);
+	if (!drv_data)
+		return -ENOMEM;
+
+	drv_data->wdt.ops = &menf21bmc_wdt_ops;
+	drv_data->wdt.info = &menf21bmc_wdt_info;
+	drv_data->wdt.min_timeout = BMC_WD_TIMEOUT_MIN;
+	drv_data->wdt.max_timeout = BMC_WD_TIMEOUT_MAX;
+	drv_data->i2c_client = i2c_client;
+
+	/*
+	 * Get the current wdt timeout value from the BMC because
+	 * the BMC will save the value set before if the system restarts.
+	 */
+	bmc_timeout = i2c_smbus_read_word_data(drv_data->i2c_client,
+							BMC_CMD_WD_TIME);
+	if (bmc_timeout < 0) {
+		dev_err(&pdev->dev, "failed to get current WDT timeout\n");
+		return bmc_timeout;
+	}
+
+	watchdog_init_timeout(&drv_data->wdt, bmc_timeout / 10, &pdev->dev);
+	watchdog_set_nowayout(&drv_data->wdt, nowayout);
+	watchdog_set_drvdata(&drv_data->wdt, drv_data);
+	platform_set_drvdata(pdev, drv_data);
+
+	ret = menf21bmc_wdt_set_bootstatus(drv_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to set Watchdog bootstatus\n");
+		return ret;
+	}
+
+	ret = watchdog_register_device(&drv_data->wdt);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register Watchdog device\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "MEN 14F021P00 BMC Watchdog device enabled\n");
+
+	return 0;
+}
+
+static int menf21bmc_wdt_remove(struct platform_device *pdev)
+{
+	struct menf21bmc_wdt *drv_data = platform_get_drvdata(pdev);
+
+	dev_warn(&pdev->dev,
+		"Unregister MEN 14F021P00 BMC Watchdog device, board may reset\n");
+
+	watchdog_unregister_device(&drv_data->wdt);
+
+	return 0;
+}
+
+static void menf21bmc_wdt_shutdown(struct platform_device *pdev)
+{
+	struct menf21bmc_wdt *drv_data = platform_get_drvdata(pdev);
+
+	i2c_smbus_write_word_data(drv_data->i2c_client,
+					BMC_CMD_WD_OFF, BMC_WD_OFF_VAL);
+}
+
+static struct  platform_driver menf21bmc_wdt = {
+	.driver		= {
+		.owner = THIS_MODULE,
+		.name	= DEVNAME,
+	},
+	.probe		= menf21bmc_wdt_probe,
+	.remove		= menf21bmc_wdt_remove,
+	.shutdown	= menf21bmc_wdt_shutdown,
+};
+
+module_platform_driver(menf21bmc_wdt);
+
+MODULE_DESCRIPTION("MEN 14F021P00 BMC Watchdog driver");
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_wdt");
-- 
2.0.4

[PATCH v5 3/4] drivers/leds/leds-menf21bmc: introduce MEN 14F021P00 BMC LED driver

[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 <andreas.werner@men.de>
---
 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

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 27cf0cd..d38ff3f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -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
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3c03666..cadc433 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -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
diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
new file mode 100644
index 0000000..4621f72
--- /dev/null
+++ b/drivers/leds/leds-menf21bmc.c
@@ -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",
+		.owner		= THIS_MODULE,
+	},
+};
+
+module_platform_driver(menf21bmc_led);
+
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC led driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_led");
-- 
2.0.4

[PATCH v5 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver

[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 <andreas.werner@men.de>
---
 Documentation/hwmon/menf21bmc   |  49 +++++++++
 drivers/hwmon/Kconfig           |   7 ++
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/menf21bmc_hwmon.c | 230 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 287 insertions(+)
 create mode 100644 Documentation/hwmon/menf21bmc
 create mode 100644 drivers/hwmon/menf21bmc_hwmon.c

diff --git a/Documentation/hwmon/menf21bmc b/Documentation/hwmon/menf21bmc
new file mode 100644
index 0000000..22b6840
--- /dev/null
+++ b/Documentation/hwmon/menf21bmc
@@ -0,0 +1,49 @@
+Kernel driver menf21bmc_hwmon
+=============================
+
+Supported chips:
+	* MEN 14F021P00
+	  Prefix: 'menf21bmc_hwmon'
+	  Adresses scanned: -
+
+Author: Andreas Werner <andreas.werner@men.de>
+
+Description
+-----------
+
+The menf21bmc is a Board Management Controller (BMC) which provides an I2C
+interface to the host to access the features implemented in the BMC.
+
+This driver gives access to the voltage monitoring feature of the main
+voltages of the board.
+The voltage sensors are connected to the ADC inputs of the BMC which is
+a PIC16F917 Mikrocontroller.
+
+Usage Notes
+-----------
+
+This driver does not auto-detect devices. You will have to instantiate the
+devices explicitly. Please see Documentation/i2c/instantiating-devices for
+details.
+
+Sysfs entries
+-------------
+
+The following attributes are supported. All attributes are read only
+The Limits are read once by the driver.
+
+in0_input	+3.3V input voltage
+in1_input	+5.0V input voltage
+in2_input	+12.0V input voltage
+in3_input	+5V Standby input voltage
+in4_input	VBAT (on board battery)
+
+in[0-4]_min	Minimum voltage limit
+in[0-4]_max	Maximum voltage limit
+
+in0_label	"MON_3_3V"
+in1_label	"MON_5V"
+in2_label	"MON_12V"
+in3_label	"5V_STANDBY"
+in4_label	"VBAT"
+
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 37908ff..db3a6eb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -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
+	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
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 1362382..56ab872 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -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
diff --git a/drivers/hwmon/menf21bmc_hwmon.c b/drivers/hwmon/menf21bmc_hwmon.c
new file mode 100644
index 0000000..2eaec6a
--- /dev/null
+++ b/drivers/hwmon/menf21bmc_hwmon.c
@@ -0,0 +1,230 @@
+/*
+ *  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
+ *
+ *  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/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/jiffies.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+
+#define DRV_NAME  "menf21bmc_hwmon"
+
+#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 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,
+						       IDX_TO_VOLT_INP_CMD(i));
+			if (val < 0) {
+				data_ret = ERR_PTR(val);
+				goto abort;
+			}
+			drv_data->in_val[i] = val;
+		}
+		drv_data->last_update = jiffies;
+		drv_data->valid = 1;
+	}
+abort:
+	return data_ret;
+}
+
+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)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
+
+	if (IS_ERR(drv_data))
+		return PTR_ERR(drv_data);
+
+	return sprintf(buf, "%d\n", drv_data->in_val[attr->index]);
+}
+
+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 = dev_get_drvdata(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 = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%d\n", drv_data->in_max[attr->index]);
+}
+
+#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_attrs[] = {
+	&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
+};
+
+ATTRIBUTE_GROUPS(menf21bmc_hwmon);
+
+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),
+				GFP_KERNEL);
+	if (!drv_data)
+		return -ENOMEM;
+
+	drv_data->i2c_client = i2c_client;
+
+	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,
+							   "menf21bmc", 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		= DRV_NAME,
+		.owner		= THIS_MODULE,
+	},
+};
+
+module_platform_driver(menf21bmc_hwmon);
+
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC hwmon");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:menf21bmc_hwmon");
-- 
2.0.4

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Lee Jones]

On Tue, 26 Aug 2014, Andreas Werner wrote:
> 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:
>  	- Watchdog
>  	- LEDs
> 	- Hwmon (voltage monitoring)
> 
> Signed-off-by: Andreas Werner <andreas.werner@men.de>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
>  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
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b8d9ca0..6a9f101 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -453,6 +453,18 @@ config MFD_MAX8998
>  	  additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_MENF21BMC
> +	tristate "MEN 14F021P00 Board Management Controller Support"
> +	depends on I2C
> +	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.

Nit: Whitespace error.

> +	  This driver provides common support for accessing the devices;
> +	  additional drivers must be enabled in order to use the
> +	  functionality of the BMC device.
> +
>  config EZX_PCAP
>  	bool "Motorola EZXPCAP Support"
>  	depends on SPI_MASTER
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 4e2bc25..37bf336 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> +obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> new file mode 100644
> index 0000000..a6eb03f
> --- /dev/null
> +++ b/drivers/mfd/menf21bmc.c
> @@ -0,0 +1,132 @@
> +/*
> + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> + *
> + *  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/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +
> +#define BMC_CMD_WDT_EXIT_PROD	0x18
> +#define BMC_CMD_WDT_PROD_STAT	0x19
> +#define BMC_CMD_REV_MAJOR	0x80
> +#define BMC_CMD_REV_MINOR	0x81
> +#define BMC_CMD_REV_MAIN	0x82
> +
> +static struct mfd_cell menf21bmc_cell[] = {
> +	{ .name = "menf21bmc_wdt", },
> +	{ .name = "menf21bmc_led", },
> +	{ .name = "menf21bmc_hwmon", }
> +};
> +
> +static int menf21bmc_wdt_exit_prod_mode(struct i2c_client *client)
> +{
> +	int val, ret;
> +
> +	val = i2c_smbus_read_byte_data(client, BMC_CMD_WDT_PROD_STAT);
> +	if (val < 0)
> +		return val;
> +
> +	/*
> +	 * Production mode should be not active after delivery of the Board.
> +	 * To be sure we check it, inform the user and exit the mode
> +	 * if active.
> +	 */
> +	if (val == 0x00) {
> +		dev_info(&client->dev,
> +			"BMC in production mode. Exit production mode\n");
> +
> +		ret = i2c_smbus_write_byte(client, BMC_CMD_WDT_EXIT_PROD);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> +{
> +	int ret;
> +	int rev_major, rev_minor, rev_main;

Really small nit (as you have to fix the whitespace err anyway):
  Can you change the order of the two lines above please?

> +	ret = i2c_check_functionality(client->adapter,
> +				      I2C_FUNC_SMBUS_BYTE_DATA |
> +				      I2C_FUNC_SMBUS_WORD_DATA |
> +				      I2C_FUNC_SMBUS_BYTE);
> +	if (!ret)
> +		return -ENODEV;
> +
> +	rev_major = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAJOR);
> +	if (rev_major < 0) {
> +		dev_err(&client->dev, "failed to get BMC major revision\n");
> +		return rev_major;
> +	}
> +
> +	rev_minor = i2c_smbus_read_word_data(client, BMC_CMD_REV_MINOR);
> +	if (rev_minor < 0) {
> +		dev_err(&client->dev, "failed to get BMC minor revision\n");
> +		return rev_minor;
> +	}
> +
> +	rev_main = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAIN);
> +	if (rev_main < 0) {
> +		dev_err(&client->dev, "failed to get BMC main revision\n");
> +		return rev_main;
> +	}
> +
> +	dev_info(&client->dev, "FW Revision: %02d.%02d.%02d\n",
> +					rev_major, rev_minor, rev_main);

You enforced a 'line > 80 chars' warning in your other driver just so
you can 'do the right thing' and line up against the '('. :)

Can you do that here please?

> +	/*
> +	 * We have to exit the Production Mode of the BMC to activate the
> +	 * Watchdog functionality and the BIOS life sign monitoring.
> +	 */
> +	ret = menf21bmc_wdt_exit_prod_mode(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to leave production mode\n");
> +		return ret;
> +	}
> +
> +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);

Line up to to '('.

> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to add BMC sub-devices\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int menf21bmc_remove(struct i2c_client *client)
> +{
> +	mfd_remove_devices(&client->dev);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id menf21bmc_id_table[] = {
> +	{ "menf21bmc", 0 },

The second attribute is unused in this driver, please remove it.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> +
> +static struct i2c_driver menf21bmc_driver = {
> +	.driver.name	= "menf21bmc",
> +	.id_table	= menf21bmc_id_table,
> +	.probe		= menf21bmc_probe,
> +	.remove		= menf21bmc_remove,
> +};

No DT support?

> +module_i2c_driver(menf21bmc_driver);
> +
> +MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");

s/mfd/MFD

> +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> +MODULE_LICENSE("GPL v2");

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

Re: [PATCH v5 4/4] drivers/hwmon/menf21bmc_hwmon: introduce MEN14F021P00 BMC HWMON driver

[Andreas Werner]

On Tue, Aug 26, 2014 at 10:15:41AM -0700, Guenter Roeck wrote:
> On Tue, Aug 26, 2014 at 07:46:53PM +0200, Andreas Werner wrote:
> > 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 <andreas.werner@men.de>
> 
> Hi Andreas,
> 
> Couple of additional comments below. Sorry I didn't notice earlier.
> 
> Guenter
> 
> > ---
> >  Documentation/hwmon/menf21bmc   |  49 +++++++++
> >  drivers/hwmon/Kconfig           |   7 ++
> >  drivers/hwmon/Makefile          |   1 +
> >  drivers/hwmon/menf21bmc_hwmon.c | 230 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 287 insertions(+)
> >  create mode 100644 Documentation/hwmon/menf21bmc
> >  create mode 100644 drivers/hwmon/menf21bmc_hwmon.c
> > 
> > diff --git a/Documentation/hwmon/menf21bmc b/Documentation/hwmon/menf21bmc
> > new file mode 100644
> > index 0000000..22b6840
> > --- /dev/null
> > +++ b/Documentation/hwmon/menf21bmc
> > @@ -0,0 +1,49 @@
> > +Kernel driver menf21bmc_hwmon
> > +=============================
> > +
> > +Supported chips:
> > +	* MEN 14F021P00
> > +	  Prefix: 'menf21bmc_hwmon'
> > +	  Adresses scanned: -
> > +
> > +Author: Andreas Werner <andreas.werner@men.de>
> > +
> > +Description
> > +-----------
> > +
> > +The menf21bmc is a Board Management Controller (BMC) which provides an I2C
> > +interface to the host to access the features implemented in the BMC.
> > +
> > +This driver gives access to the voltage monitoring feature of the main
> > +voltages of the board.
> > +The voltage sensors are connected to the ADC inputs of the BMC which is
> > +a PIC16F917 Mikrocontroller.
> > +
> > +Usage Notes
> > +-----------
> > +
> > +This driver does not auto-detect devices. You will have to instantiate the
> > +devices explicitly. Please see Documentation/i2c/instantiating-devices for
> > +details.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +The following attributes are supported. All attributes are read only
> > +The Limits are read once by the driver.
> > +
> > +in0_input	+3.3V input voltage
> > +in1_input	+5.0V input voltage
> > +in2_input	+12.0V input voltage
> > +in3_input	+5V Standby input voltage
> > +in4_input	VBAT (on board battery)
> > +
> > +in[0-4]_min	Minimum voltage limit
> > +in[0-4]_max	Maximum voltage limit
> > +
> > +in0_label	"MON_3_3V"
> > +in1_label	"MON_5V"
> > +in2_label	"MON_12V"
> > +in3_label	"5V_STANDBY"
> > +in4_label	"VBAT"
> > +
> 
> The empty line adds a whitespace error when applying the patch.
>

OK, will delete the line.
 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 37908ff..db3a6eb 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -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
> > +	help
> > +	  Say Y here to include support for the MEN 14F021P00 BMC
> > +	  hardware monitoring.
> > +
> It is customary to add a note describing how the module is called
> if the driver is built as module.
> 

OK i just write a line which describes the module name.

> >  config SENSORS_ADCXX
> >  	tristate "National Semiconductor ADCxxxSxxx"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 1362382..56ab872 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -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
> > diff --git a/drivers/hwmon/menf21bmc_hwmon.c b/drivers/hwmon/menf21bmc_hwmon.c
> > new file mode 100644
> > index 0000000..2eaec6a
> > --- /dev/null
> > +++ b/drivers/hwmon/menf21bmc_hwmon.c
> > @@ -0,0 +1,230 @@
> > +/*
> > + *  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
> > + *
> > + *  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/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +
> > +#define DRV_NAME  "menf21bmc_hwmon"
> > +
> > +#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 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;
> 
> Please use bool (and true / false to set it). char doesn't
> save anything and may actually increase code size depending
> on the architecture.
> 

Ok.

> > +	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;
> 
> That doesn't work well with the checks against < 0.
> Please compile with W=1 to find all instances of this problem
> (there is another one below).
> 

Argh, hate myself.

> > +	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) {
> 
> Line over 80 characters.
> 

Yes, checkpatch gave me a warning, but a thought it is better so let
it as it is instead of split the "if" line.
But it is no problem to change that.

> > +		for (i = 0; i < BMC_VOLT_COUNT; i++) {
> > +			val = i2c_smbus_read_word_data(drv_data->i2c_client,
> > +						       IDX_TO_VOLT_INP_CMD(i));
> > +			if (val < 0) {
> > +				data_ret = ERR_PTR(val);
> > +				goto abort;
> > +			}
> > +			drv_data->in_val[i] = val;
> > +		}
> > +		drv_data->last_update = jiffies;
> > +		drv_data->valid = 1;
> > +	}
> > +abort:
> > +	return data_ret;
> > +}
> > +
> > +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)
> > +{
> > +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> > +	struct menf21bmc_hwmon *drv_data = menf21bmc_hwmon_update(dev);
> > +
> > +	if (IS_ERR(drv_data))
> > +		return PTR_ERR(drv_data);
> > +
> > +	return sprintf(buf, "%d\n", drv_data->in_val[attr->index]);
> > +}
> > +
> > +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 = dev_get_drvdata(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 = dev_get_drvdata(dev);
> > +
> > +	return sprintf(buf, "%d\n", drv_data->in_max[attr->index]);
> > +}
> > +
> > +#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_attrs[] = {
> > +	&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
> > +};
> > +
> > +ATTRIBUTE_GROUPS(menf21bmc_hwmon);
> > +
> > +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),
> > +				GFP_KERNEL);
> > +	if (!drv_data)
> > +		return -ENOMEM;
> > +
> > +	drv_data->i2c_client = i2c_client;
> > +
> > +	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,
> > +							   "menf21bmc", drv_data,
> > +							   menf21bmc_hwmon_groups);
> 
> Line over 80 characters. Just use less indentation for the continuation lines.
> 

Same as above, checkpatch gave me a warning. I just wanted to align it to the "("
but no problem to change that.

> > +	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		= DRV_NAME,
> > +		.owner		= THIS_MODULE,
> > +	},
> > +};
> > +
> > +module_platform_driver(menf21bmc_hwmon);
> > +
> > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > +MODULE_DESCRIPTION("MEN 14F021P00 BMC hwmon");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:menf21bmc_hwmon");
> > -- 
> > 2.0.4
> > 
> > 

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Andreas Werner]

On Wed, Aug 27, 2014 at 08:26:33AM +0100, Lee Jones wrote:
> On Tue, 26 Aug 2014, Andreas Werner wrote:
> > 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:
> >  	- Watchdog
> >  	- LEDs
> > 	- Hwmon (voltage monitoring)
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  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
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b8d9ca0..6a9f101 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -453,6 +453,18 @@ config MFD_MAX8998
> >  	  additional drivers must be enabled in order to use the functionality
> >  	  of the device.
> >  
> > +config MFD_MENF21BMC
> > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > +	depends on I2C
> > +	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.
> 
> Nit: Whitespace error.
>

Forgot to run checkpatch on Kconfig since the last change. Will fix it.
 
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
> > +
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 4e2bc25..37bf336 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
> >  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
> >  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
> >  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> > +obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
> >  
> >  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..a6eb03f
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,132 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> > + *
> > + *  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/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +
> > +#define BMC_CMD_WDT_EXIT_PROD	0x18
> > +#define BMC_CMD_WDT_PROD_STAT	0x19
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{ .name = "menf21bmc_wdt", },
> > +	{ .name = "menf21bmc_led", },
> > +	{ .name = "menf21bmc_hwmon", }
> > +};
> > +
> > +static int menf21bmc_wdt_exit_prod_mode(struct i2c_client *client)
> > +{
> > +	int val, ret;
> > +
> > +	val = i2c_smbus_read_byte_data(client, BMC_CMD_WDT_PROD_STAT);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	/*
> > +	 * Production mode should be not active after delivery of the Board.
> > +	 * To be sure we check it, inform the user and exit the mode
> > +	 * if active.
> > +	 */
> > +	if (val == 0x00) {
> > +		dev_info(&client->dev,
> > +			"BMC in production mode. Exit production mode\n");
> > +
> > +		ret = i2c_smbus_write_byte(client, BMC_CMD_WDT_EXIT_PROD);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> > +{
> > +	int ret;
> > +	int rev_major, rev_minor, rev_main;
> 
> Really small nit (as you have to fix the whitespace err anyway):
>   Can you change the order of the two lines above please?

No problem.

> 
> > +	ret = i2c_check_functionality(client->adapter,
> > +				      I2C_FUNC_SMBUS_BYTE_DATA |
> > +				      I2C_FUNC_SMBUS_WORD_DATA |
> > +				      I2C_FUNC_SMBUS_BYTE);
> > +	if (!ret)
> > +		return -ENODEV;
> > +
> > +	rev_major = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAJOR);
> > +	if (rev_major < 0) {
> > +		dev_err(&client->dev, "failed to get BMC major revision\n");
> > +		return rev_major;
> > +	}
> > +
> > +	rev_minor = i2c_smbus_read_word_data(client, BMC_CMD_REV_MINOR);
> > +	if (rev_minor < 0) {
> > +		dev_err(&client->dev, "failed to get BMC minor revision\n");
> > +		return rev_minor;
> > +	}
> > +
> > +	rev_main = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAIN);
> > +	if (rev_main < 0) {
> > +		dev_err(&client->dev, "failed to get BMC main revision\n");
> > +		return rev_main;
> > +	}
> > +
> > +	dev_info(&client->dev, "FW Revision: %02d.%02d.%02d\n",
> > +					rev_major, rev_minor, rev_main);
> 
> You enforced a 'line > 80 chars' warning in your other driver just so
> you can 'do the right thing' and line up against the '('. :)
> 
> Can you do that here please?
> 

yes

> > +	/*
> > +	 * We have to exit the Production Mode of the BMC to activate the
> > +	 * Watchdog functionality and the BIOS life sign monitoring.
> > +	 */
> > +	ret = menf21bmc_wdt_exit_prod_mode(client);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to leave production mode\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> > +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> 
> Line up to to '('.
> 

ok

> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to add BMC sub-devices\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int menf21bmc_remove(struct i2c_client *client)
> > +{
> > +	mfd_remove_devices(&client->dev);
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id menf21bmc_id_table[] = {
> > +	{ "menf21bmc", 0 },
> 
> The second attribute is unused in this driver, please remove it.
> 

ok

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > +
> > +static struct i2c_driver menf21bmc_driver = {
> > +	.driver.name	= "menf21bmc",
> > +	.id_table	= menf21bmc_id_table,
> > +	.probe		= menf21bmc_probe,
> > +	.remove		= menf21bmc_remove,
> > +};
> 
> No DT support?
> 
> > +module_i2c_driver(menf21bmc_driver);
> > +
> > +MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");
> 
> s/mfd/MFD
> 
> > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > +MODULE_LICENSE("GPL v2");
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Andreas Werner]

On Wed, Aug 27, 2014 at 08:26:33AM +0100, Lee Jones wrote:
> On Tue, 26 Aug 2014, Andreas Werner wrote:
> > 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:
> >  	- Watchdog
> >  	- LEDs
> > 	- Hwmon (voltage monitoring)
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  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
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b8d9ca0..6a9f101 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -453,6 +453,18 @@ config MFD_MAX8998
> >  	  additional drivers must be enabled in order to use the functionality
> >  	  of the device.
> >  
> > +config MFD_MENF21BMC
> > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > +	depends on I2C
> > +	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.
> 
> Nit: Whitespace error.
> 

I run checkpatch but did not find any whitespace error.
Where is it?

...

> > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > +
> > +static struct i2c_driver menf21bmc_driver = {
> > +	.driver.name	= "menf21bmc",
> > +	.id_table	= menf21bmc_id_table,
> > +	.probe		= menf21bmc_probe,
> > +	.remove		= menf21bmc_remove,
> > +};
> 
> No DT support?
> 

No not at the moment because it is used only on x86 system.

> > +module_i2c_driver(menf21bmc_driver);
> > +
> > +MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");
> 
> s/mfd/MFD
> 
> > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > +MODULE_LICENSE("GPL v2");
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Lee Jones]

On Wed, 27 Aug 2014, Andreas Werner wrote:
> On Wed, Aug 27, 2014 at 08:26:33AM +0100, Lee Jones wrote:
> > On Tue, 26 Aug 2014, Andreas Werner wrote:
> > > 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:
> > >  	- Watchdog
> > >  	- LEDs
> > > 	- Hwmon (voltage monitoring)
> > > 
> > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > Acked-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  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
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index b8d9ca0..6a9f101 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -453,6 +453,18 @@ config MFD_MAX8998
> > >  	  additional drivers must be enabled in order to use the functionality
> > >  	  of the device.
> > >  
> > > +config MFD_MENF21BMC
> > > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > > +	depends on I2C
> > > +	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.
> > 
> > Nit: Whitespace error.
> > 
> 
> I run checkpatch but did not find any whitespace error.
> Where is it?

After 'HWMON'.

> > > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > > +
> > > +static struct i2c_driver menf21bmc_driver = {
> > > +	.driver.name	= "menf21bmc",
> > > +	.id_table	= menf21bmc_id_table,
> > > +	.probe		= menf21bmc_probe,
> > > +	.remove		= menf21bmc_remove,
> > > +};
> > 
> > No DT support?
> > 
> 
> No not at the moment because it is used only on x86 system.

Ah okay.  Shouldn't the Kconfig depend on X86 then?  What about ACPI?

> > > +module_i2c_driver(menf21bmc_driver);
> > > +
> > > +MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");
> > 
> > s/mfd/MFD
> > 
> > > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > > +MODULE_LICENSE("GPL v2");
> > 

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

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Lee Jones]

On Wed, 27 Aug 2014, Andreas Werner wrote:

> On Wed, Aug 27, 2014 at 12:01:42PM +0100, Lee Jones wrote:
> > On Wed, 27 Aug 2014, Andreas Werner wrote:
> > > On Wed, Aug 27, 2014 at 08:26:33AM +0100, Lee Jones wrote:
> > > > On Tue, 26 Aug 2014, Andreas Werner wrote:
> > > > > 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:
> > > > >  	- Watchdog
> > > > >  	- LEDs
> > > > > 	- Hwmon (voltage monitoring)
> > > > > 
> > > > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > > > Acked-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  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
> > > > > 
> > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > index b8d9ca0..6a9f101 100644
> > > > > --- a/drivers/mfd/Kconfig
> > > > > +++ b/drivers/mfd/Kconfig
> > > > > @@ -453,6 +453,18 @@ config MFD_MAX8998
> > > > >  	  additional drivers must be enabled in order to use the functionality
> > > > >  	  of the device.
> > > > >  
> > > > > +config MFD_MENF21BMC
> > > > > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > > > > +	depends on I2C
> > > > > +	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.
> > > > 
> > > > Nit: Whitespace error.
> > > > 
> > > 
> > > I run checkpatch but did not find any whitespace error.
> > > Where is it?
> > 
> > After 'HWMON'.
> >
> 
> Ah ok.
>  
> > > > > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > > > > +
> > > > > +static struct i2c_driver menf21bmc_driver = {
> > > > > +	.driver.name	= "menf21bmc",
> > > > > +	.id_table	= menf21bmc_id_table,
> > > > > +	.probe		= menf21bmc_probe,
> > > > > +	.remove		= menf21bmc_remove,
> > > > > +};
> > > > 
> > > > No DT support?
> > > > 
> > > 
> > > No not at the moment because it is used only on x86 system.
> > 
> > Ah okay.  Shouldn't the Kconfig depend on X86 then?  What about ACPI?
> 
> Yes and no. Could be also instantiated manually withouth DT. (e.g. i2c sysfs)
> So for me it is better to not depend on x86 to have the possibility to use
> the driver without DT on another arch.
> ACPI is no option because there are a lot boards out without
> the BMC in ACPI Table.
> 
> I think i will implement DT support if we have on board which is not x86.

Very well.

> > > > > +module_i2c_driver(menf21bmc_driver);
> > > > > +
> > > > > +MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");
> > > > 
> > > > s/mfd/MFD
> > > > 
> > > > > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > > > > +MODULE_LICENSE("GPL v2");
> > > > 
> > 

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

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Andreas Werner]

On Wed, Aug 27, 2014 at 12:01:42PM +0100, Lee Jones wrote:
> On Wed, 27 Aug 2014, Andreas Werner wrote:
> > On Wed, Aug 27, 2014 at 08:26:33AM +0100, Lee Jones wrote:
> > > On Tue, 26 Aug 2014, Andreas Werner wrote:
> > > > 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:
> > > >  	- Watchdog
> > > >  	- LEDs
> > > > 	- Hwmon (voltage monitoring)
> > > > 
> > > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > > Acked-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  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
> > > > 
> > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > index b8d9ca0..6a9f101 100644
> > > > --- a/drivers/mfd/Kconfig
> > > > +++ b/drivers/mfd/Kconfig
> > > > @@ -453,6 +453,18 @@ config MFD_MAX8998
> > > >  	  additional drivers must be enabled in order to use the functionality
> > > >  	  of the device.
> > > >  
> > > > +config MFD_MENF21BMC
> > > > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > > > +	depends on I2C
> > > > +	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.
> > > 
> > > Nit: Whitespace error.
> > > 
> > 
> > I run checkpatch but did not find any whitespace error.
> > Where is it?
> 
> After 'HWMON'.
>

Ah ok.
 
> > > > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > > > +
> > > > +static struct i2c_driver menf21bmc_driver = {
> > > > +	.driver.name	= "menf21bmc",
> > > > +	.id_table	= menf21bmc_id_table,
> > > > +	.probe		= menf21bmc_probe,
> > > > +	.remove		= menf21bmc_remove,
> > > > +};
> > > 
> > > No DT support?
> > > 
> > 
> > No not at the moment because it is used only on x86 system.
> 
> Ah okay.  Shouldn't the Kconfig depend on X86 then?  What about ACPI?

Yes and no. Could be also instantiated manually withouth DT. (e.g. i2c sysfs)
So for me it is better to not depend on x86 to have the possibility to use
the driver without DT on another arch.
ACPI is no option because there are a lot boards out without
the BMC in ACPI Table.

I think i will implement DT support if we have on board which is not x86.

> 
> > > > +module_i2c_driver(menf21bmc_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("MEN 14F021P00 BMC mfd core driver");
> > > 
> > > s/mfd/MFD
> > > 
> > > > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > > > +MODULE_LICENSE("GPL v2");
> > > 
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Guenter Roeck]

On 08/27/2014 12:26 AM, Lee Jones wrote:
> On Tue, 26 Aug 2014, Andreas Werner wrote:
>> 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:
>>   	- Watchdog
>>   	- LEDs
>> 	- Hwmon (voltage monitoring)
>>

>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
>> +
>> +static struct i2c_driver menf21bmc_driver = {
>> +	.driver.name	= "menf21bmc",
>> +	.id_table	= menf21bmc_id_table,
>> +	.probe		= menf21bmc_probe,
>> +	.remove		= menf21bmc_remove,
>> +};
>
> No DT support?
>
i2c devices auto-instantiate from dt entries even if there is no
explicit devicetree configuration here. The devicetree entry
would have to be something like "men,menf21bmc", ie it needs
to match the driver name.

Guenter

Re: [PATCH v5 1/4] drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver

[Lee Jones]

On Wed, 27 Aug 2014, Guenter Roeck wrote:
> On 08/27/2014 12:26 AM, Lee Jones wrote:
> >On Tue, 26 Aug 2014, Andreas Werner wrote:
> >>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:
> >>  	- Watchdog
> >>  	- LEDs
> >>	- Hwmon (voltage monitoring)
> >>
> 
> >>+	{ }
> >>+};
> >>+MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> >>+
> >>+static struct i2c_driver menf21bmc_driver = {
> >>+	.driver.name	= "menf21bmc",
> >>+	.id_table	= menf21bmc_id_table,
> >>+	.probe		= menf21bmc_probe,
> >>+	.remove		= menf21bmc_remove,
> >>+};
> >
> >No DT support?
> >
> i2c devices auto-instantiate from dt entries even if there is no
> explicit devicetree configuration here. The devicetree entry
> would have to be something like "men,menf21bmc", ie it needs
> to match the driver name.

https://patchwork.ozlabs.org/patch/362180/

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