[PATCH v2 0/3] Introduce MEN 14F02100 BMC driver series

[PATCH v2 0/3] Introduce MEN 14F02100 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.
The 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

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


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 (3):
  drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver
  drivers/watchdog/menf21bmc_wd: introduce MEN 14F021P00 BMC Watchdog
    driver
  drivers/leds/leds-menf21bmc: introduce MEN 14F021P00 BMC LED driver

 drivers/leds/Kconfig            |   6 ++
 drivers/leds/Makefile           |   2 +-
 drivers/leds/leds-menf21bmc.c   | 136 +++++++++++++++++++++++++
 drivers/mfd/Kconfig             |  12 +++
 drivers/mfd/Makefile            |   1 +
 drivers/mfd/menf21bmc.c         | 220 ++++++++++++++++++++++++++++++++++++++++
 drivers/watchdog/Kconfig        |   7 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/menf21bmc_wd.c | 217 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/menf21bmc.h   |  31 ++++++
 10 files changed, 632 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-menf21bmc.c
 create mode 100644 drivers/mfd/menf21bmc.c
 create mode 100644 drivers/watchdog/menf21bmc_wd.c
 create mode 100644 include/linux/mfd/menf21bmc.h

-- 
1.9.3

[PATCH v2 0/3] Introduce MEN 14F02100 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.
The 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

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


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 (3):
  drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver
  drivers/watchdog/menf21bmc_wd: introduce MEN 14F021P00 BMC Watchdog
    driver
  drivers/leds/leds-menf21bmc: introduce MEN 14F021P00 BMC LED driver

 drivers/leds/Kconfig            |   6 ++
 drivers/leds/Makefile           |   2 +-
 drivers/leds/leds-menf21bmc.c   | 136 +++++++++++++++++++++++++
 drivers/mfd/Kconfig             |  12 +++
 drivers/mfd/Makefile            |   1 +
 drivers/mfd/menf21bmc.c         | 220 ++++++++++++++++++++++++++++++++++++++++
 drivers/watchdog/Kconfig        |   7 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/menf21bmc_wd.c | 217 +++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/menf21bmc.h   |  31 ++++++
 10 files changed, 632 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-menf21bmc.c
 create mode 100644 drivers/mfd/menf21bmc.c
 create mode 100644 drivers/watchdog/menf21bmc_wd.c
 create mode 100644 include/linux/mfd/menf21bmc.h

-- 
1.9.3

[PATCH v2 1/3] 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

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 drivers/mfd/Kconfig           |  12 +++
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/menf21bmc.h |  31 ++++++
 4 files changed, 264 insertions(+)
 create mode 100644 drivers/mfd/menf21bmc.c
 create mode 100644 include/linux/mfd/menf21bmc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ab5a43c..7c2e8d2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -427,6 +427,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=y
+	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.
+	  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 5913208..8f2be38 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -167,3 +167,4 @@ 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
diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
new file mode 100644
index 0000000..8de92b5
--- /dev/null
+++ b/drivers/mfd/menf21bmc.c
@@ -0,0 +1,220 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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>
+#include <linux/mfd/menf21bmc.h>
+
+#define BMC_CMD_REV_MAJOR	0x80
+#define BMC_CMD_REV_MINOR	0x81
+#define BMC_CMD_REV_MAIN	0x82
+#define BMC_CMD_REV_BUILD	0x83
+#define BMC_CMD_REV_VERI	0x84
+#define BMC_CMD_WD_ARM_SET	0x18
+#define BMC_CMD_WD_ARM_GET	0x19
+
+static struct mfd_cell menf21bmc_cell[] = {
+	{
+		.name = "menf21bmc_wd",
+	},
+	{
+		.name = "menf21bmc_led",
+	},
+};
+
+static int
+menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_byte_data(client, reg);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int
+menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_word_data(client, reg);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_write_byte_data(client, reg, val);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_write_word_data(client, reg, val);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_write_byte(client, val);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_wd_leave_prod_mode(struct i2c_client *client)
+{
+	int val, ret;
+
+	val = menf21bmc_read_byte_data(client, BMC_CMD_WD_ARM_GET);
+	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 leave the mode
+	 * if active.
+	 */
+	if (val == 0x00) {
+		dev_info(&client->dev,
+			"BMC in production mode. Leave production mode\n");
+
+		ret = menf21bmc_write_byte(client, BMC_CMD_WD_ARM_SET);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
+{
+	int ret;
+	struct menf21bmc *data;
+	int rev_major, rev_minor, rev_main;
+
+	if (!i2c_check_functionality(client->adapter,
+						I2C_FUNC_SMBUS_BYTE_DATA |
+						I2C_FUNC_SMBUS_WORD_DATA |
+						I2C_FUNC_SMBUS_BYTE))
+		return -EIO;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct menf21bmc), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	mutex_init(&data->lock);
+
+	i2c_set_clientdata(client, data);
+	data->client = client;
+	data->read_word_data = menf21bmc_read_word_data;
+	data->read_byte_data = menf21bmc_read_byte_data;
+	data->write_word_data = menf21bmc_write_word_data;
+	data->write_byte_data = menf21bmc_write_byte_data;
+	data->write_byte = menf21bmc_write_byte;
+
+	rev_major = menf21bmc_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 = menf21bmc_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 = menf21bmc_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 leave the Production Mode of the BMC to activate the
+	 * Watchdog functionality and the BIOS life sign monitoring.
+	 */
+	ret = menf21bmc_wd_leave_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, "mfd_add_devices failed\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",
+		.owner	= THIS_MODULE,
+	},
+	.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");
diff --git a/include/linux/mfd/menf21bmc.h b/include/linux/mfd/menf21bmc.h
new file mode 100644
index 0000000..1fc9b5c
--- /dev/null
+++ b/include/linux/mfd/menf21bmc.h
@@ -0,0 +1,31 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC)
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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.
+ *
+ */
+
+#ifndef MENF21BMC_H
+#define MENF21BMC_H
+
+struct menf21bmc {
+	struct i2c_client *client;
+	struct mutex lock;
+
+	int (*read_word_data)(struct i2c_client *client, u8 reg);
+	int (*read_byte_data)(struct i2c_client *client, u8 reg);
+	int (*write_word_data)(struct i2c_client *client, u8 reg, u16 val);
+	int (*write_byte_data)(struct i2c_client *client, u8 reg, u8 val);
+	int (*write_byte)(struct i2c_client *client, u8 val);
+};
+
+#endif /* MENF21BMC_H */
+
+
-- 
1.9.3

[PATCH v2 1/3] 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

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 drivers/mfd/Kconfig           |  12 +++
 drivers/mfd/Makefile          |   1 +
 drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/menf21bmc.h |  31 ++++++
 4 files changed, 264 insertions(+)
 create mode 100644 drivers/mfd/menf21bmc.c
 create mode 100644 include/linux/mfd/menf21bmc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index ab5a43c..7c2e8d2 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -427,6 +427,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=y
+	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.
+	  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 5913208..8f2be38 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -167,3 +167,4 @@ 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
diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
new file mode 100644
index 0000000..8de92b5
--- /dev/null
+++ b/drivers/mfd/menf21bmc.c
@@ -0,0 +1,220 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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>
+#include <linux/mfd/menf21bmc.h>
+
+#define BMC_CMD_REV_MAJOR	0x80
+#define BMC_CMD_REV_MINOR	0x81
+#define BMC_CMD_REV_MAIN	0x82
+#define BMC_CMD_REV_BUILD	0x83
+#define BMC_CMD_REV_VERI	0x84
+#define BMC_CMD_WD_ARM_SET	0x18
+#define BMC_CMD_WD_ARM_GET	0x19
+
+static struct mfd_cell menf21bmc_cell[] = {
+	{
+		.name = "menf21bmc_wd",
+	},
+	{
+		.name = "menf21bmc_led",
+	},
+};
+
+static int
+menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_byte_data(client, reg);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int
+menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_read_word_data(client, reg);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_write_byte_data(client, reg, val);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_write_word_data(client, reg, val);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
+{
+	int ret;
+	struct menf21bmc *data = i2c_get_clientdata(client);
+
+	mutex_lock(&data->lock);
+	ret = i2c_smbus_write_byte(client, val);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int menf21bmc_wd_leave_prod_mode(struct i2c_client *client)
+{
+	int val, ret;
+
+	val = menf21bmc_read_byte_data(client, BMC_CMD_WD_ARM_GET);
+	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 leave the mode
+	 * if active.
+	 */
+	if (val == 0x00) {
+		dev_info(&client->dev,
+			"BMC in production mode. Leave production mode\n");
+
+		ret = menf21bmc_write_byte(client, BMC_CMD_WD_ARM_SET);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int
+menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
+{
+	int ret;
+	struct menf21bmc *data;
+	int rev_major, rev_minor, rev_main;
+
+	if (!i2c_check_functionality(client->adapter,
+						I2C_FUNC_SMBUS_BYTE_DATA |
+						I2C_FUNC_SMBUS_WORD_DATA |
+						I2C_FUNC_SMBUS_BYTE))
+		return -EIO;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct menf21bmc), GFP_KERNEL);
+	if (data == NULL)
+		return -ENOMEM;
+
+	mutex_init(&data->lock);
+
+	i2c_set_clientdata(client, data);
+	data->client = client;
+	data->read_word_data = menf21bmc_read_word_data;
+	data->read_byte_data = menf21bmc_read_byte_data;
+	data->write_word_data = menf21bmc_write_word_data;
+	data->write_byte_data = menf21bmc_write_byte_data;
+	data->write_byte = menf21bmc_write_byte;
+
+	rev_major = menf21bmc_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 = menf21bmc_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 = menf21bmc_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 leave the Production Mode of the BMC to activate the
+	 * Watchdog functionality and the BIOS life sign monitoring.
+	 */
+	ret = menf21bmc_wd_leave_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, "mfd_add_devices failed\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",
+		.owner	= THIS_MODULE,
+	},
+	.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");
diff --git a/include/linux/mfd/menf21bmc.h b/include/linux/mfd/menf21bmc.h
new file mode 100644
index 0000000..1fc9b5c
--- /dev/null
+++ b/include/linux/mfd/menf21bmc.h
@@ -0,0 +1,31 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC)
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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.
+ *
+ */
+
+#ifndef MENF21BMC_H
+#define MENF21BMC_H
+
+struct menf21bmc {
+	struct i2c_client *client;
+	struct mutex lock;
+
+	int (*read_word_data)(struct i2c_client *client, u8 reg);
+	int (*read_byte_data)(struct i2c_client *client, u8 reg);
+	int (*write_word_data)(struct i2c_client *client, u8 reg, u16 val);
+	int (*write_byte_data)(struct i2c_client *client, u8 reg, u8 val);
+	int (*write_byte)(struct i2c_client *client, u8 val);
+};
+
+#endif /* MENF21BMC_H */
+
+
-- 
1.9.3

[PATCH v2 2/3] drivers/watchdog/menf21bmc_wd: 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.

This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 drivers/watchdog/Kconfig        |   7 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/menf21bmc_wd.c | 217 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/watchdog/menf21bmc_wd.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 2b4c1fc..ede3201 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 1b5f3d5..0a5465d 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_wd.o
diff --git a/drivers/watchdog/menf21bmc_wd.c b/drivers/watchdog/menf21bmc_wd.c
new file mode 100644
index 0000000..abd2869
--- /dev/null
+++ b/drivers/watchdog/menf21bmc_wd.c
@@ -0,0 +1,217 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) Watchdog Driver.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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/mfd/menf21bmc.h>
+
+#define DEVNAME "menf21bmc_wd"
+
+#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_wd_data {
+	struct watchdog_device wdt;
+	struct menf21bmc *menf21bmc;
+};
+
+static int menf21bmc_wd_set_bootstatus(struct menf21bmc_wd_data *data)
+{
+	int rst_rsn;
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	rst_rsn = menf21bmc->read_byte_data(menf21bmc->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_wd_start(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	return menf21bmc->write_byte(menf21bmc->client, BMC_CMD_WD_ON);
+}
+
+static int menf21bmc_wd_stop(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	return menf21bmc->write_byte_data(menf21bmc->client,
+					BMC_CMD_WD_OFF, BMC_WD_OFF_VAL);
+}
+
+static int
+menf21bmc_wd_settimeout(struct watchdog_device *wdt, unsigned int timeout)
+{
+	int ret;
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	/*
+	 *  BMC Watchdog does have a resolution of 100ms.
+	 *  Watchdog API defines the timeout in seconds, so we have to
+	 *  multiply the value.
+	 */
+	ret = menf21bmc->write_word_data(menf21bmc->client,
+						BMC_CMD_WD_TIME, timeout * 10);
+	if (ret < 0)
+		return ret;
+
+	wdt->timeout = timeout;
+
+	return 0;
+}
+
+static int menf21bmc_wd_ping(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	return menf21bmc->write_byte(menf21bmc->client, BMC_CMD_WD_TRIG);
+}
+
+static const struct watchdog_info menf21bmc_wd_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = DEVNAME,
+};
+
+static const struct watchdog_ops menf21bmc_wd_ops = {
+	.owner		= THIS_MODULE,
+	.start		= menf21bmc_wd_start,
+	.stop		= menf21bmc_wd_stop,
+	.ping		= menf21bmc_wd_ping,
+	.set_timeout	= menf21bmc_wd_settimeout,
+};
+
+static int menf21bmc_wd_probe(struct platform_device *pdev)
+{
+	int ret, bmc_timeout;
+	struct menf21bmc_wd_data *driver_data;
+	struct watchdog_device *menf21bmc_wd;
+	struct menf21bmc *menf21bmc = dev_get_drvdata(pdev->dev.parent);
+
+	driver_data = devm_kzalloc(&pdev->dev,
+				sizeof(struct menf21bmc_wd_data), GFP_KERNEL);
+	if (!driver_data)
+		return -ENOMEM;
+
+	driver_data->menf21bmc = menf21bmc;
+
+	menf21bmc_wd = &driver_data->wdt;
+	menf21bmc_wd->ops = &menf21bmc_wd_ops;
+	menf21bmc_wd->info = &menf21bmc_wd_info;
+	menf21bmc_wd->min_timeout = BMC_WD_TIMEOUT_MIN;
+	menf21bmc_wd->max_timeout = BMC_WD_TIMEOUT_MAX;
+
+	/*
+	 * Get the current wdt timeout value from the BMC because
+	 * the BMC will save the value set before if the system restarts.
+	 */
+	bmc_timeout = menf21bmc->read_word_data(menf21bmc->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(menf21bmc_wd, bmc_timeout / 10, &pdev->dev);
+
+	watchdog_set_nowayout(menf21bmc_wd, nowayout);
+	watchdog_set_drvdata(menf21bmc_wd, driver_data);
+	platform_set_drvdata(pdev, driver_data);
+
+	ret = menf21bmc_wd_set_bootstatus(driver_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to set Watchdog bootstatus\n");
+		return ret;
+	}
+
+	ret = watchdog_register_device(&driver_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_wd_remove(struct platform_device *pdev)
+{
+	struct menf21bmc_wd_data *data = platform_get_drvdata(pdev);
+
+	dev_warn(&pdev->dev,
+		"Untregister MEN 14F021P00 BMC Watchdog device, board may reset\n");
+
+	watchdog_unregister_device(&data->wdt);
+
+	return 0;
+}
+
+static void menf21bmc_wd_shutdown(struct platform_device *pdev)
+{
+	struct menf21bmc_wd_data *data = platform_get_drvdata(pdev);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	menf21bmc->write_word_data(menf21bmc->client,
+					BMC_CMD_WD_OFF, BMC_WD_OFF_VAL);
+}
+
+static struct  platform_driver menf21bmc_wd = {
+	.driver		= {
+		.owner = THIS_MODULE,
+		.name	= DEVNAME,
+	},
+	.probe		= menf21bmc_wd_probe,
+	.remove		= menf21bmc_wd_remove,
+	.shutdown	= menf21bmc_wd_shutdown,
+};
+
+module_platform_driver(menf21bmc_wd);
+
+MODULE_DESCRIPTION("MEN 14F021P00 BMC Watchdog driver");
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:menf21bmc_wd");
-- 
1.9.3

[PATCH v2 2/3] drivers/watchdog/menf21bmc_wd: 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.

This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 drivers/watchdog/Kconfig        |   7 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/menf21bmc_wd.c | 217 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/watchdog/menf21bmc_wd.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 2b4c1fc..ede3201 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 1b5f3d5..0a5465d 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_wd.o
diff --git a/drivers/watchdog/menf21bmc_wd.c b/drivers/watchdog/menf21bmc_wd.c
new file mode 100644
index 0000000..abd2869
--- /dev/null
+++ b/drivers/watchdog/menf21bmc_wd.c
@@ -0,0 +1,217 @@
+/*
+ *  MEN 14F021P00 Board Management Controller (BMC) Watchdog Driver.
+ *
+ *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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/mfd/menf21bmc.h>
+
+#define DEVNAME "menf21bmc_wd"
+
+#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_wd_data {
+	struct watchdog_device wdt;
+	struct menf21bmc *menf21bmc;
+};
+
+static int menf21bmc_wd_set_bootstatus(struct menf21bmc_wd_data *data)
+{
+	int rst_rsn;
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	rst_rsn = menf21bmc->read_byte_data(menf21bmc->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_wd_start(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	return menf21bmc->write_byte(menf21bmc->client, BMC_CMD_WD_ON);
+}
+
+static int menf21bmc_wd_stop(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	return menf21bmc->write_byte_data(menf21bmc->client,
+					BMC_CMD_WD_OFF, BMC_WD_OFF_VAL);
+}
+
+static int
+menf21bmc_wd_settimeout(struct watchdog_device *wdt, unsigned int timeout)
+{
+	int ret;
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	/*
+	 *  BMC Watchdog does have a resolution of 100ms.
+	 *  Watchdog API defines the timeout in seconds, so we have to
+	 *  multiply the value.
+	 */
+	ret = menf21bmc->write_word_data(menf21bmc->client,
+						BMC_CMD_WD_TIME, timeout * 10);
+	if (ret < 0)
+		return ret;
+
+	wdt->timeout = timeout;
+
+	return 0;
+}
+
+static int menf21bmc_wd_ping(struct watchdog_device *wdt)
+{
+	struct menf21bmc_wd_data *data = watchdog_get_drvdata(wdt);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	return menf21bmc->write_byte(menf21bmc->client, BMC_CMD_WD_TRIG);
+}
+
+static const struct watchdog_info menf21bmc_wd_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = DEVNAME,
+};
+
+static const struct watchdog_ops menf21bmc_wd_ops = {
+	.owner		= THIS_MODULE,
+	.start		= menf21bmc_wd_start,
+	.stop		= menf21bmc_wd_stop,
+	.ping		= menf21bmc_wd_ping,
+	.set_timeout	= menf21bmc_wd_settimeout,
+};
+
+static int menf21bmc_wd_probe(struct platform_device *pdev)
+{
+	int ret, bmc_timeout;
+	struct menf21bmc_wd_data *driver_data;
+	struct watchdog_device *menf21bmc_wd;
+	struct menf21bmc *menf21bmc = dev_get_drvdata(pdev->dev.parent);
+
+	driver_data = devm_kzalloc(&pdev->dev,
+				sizeof(struct menf21bmc_wd_data), GFP_KERNEL);
+	if (!driver_data)
+		return -ENOMEM;
+
+	driver_data->menf21bmc = menf21bmc;
+
+	menf21bmc_wd = &driver_data->wdt;
+	menf21bmc_wd->ops = &menf21bmc_wd_ops;
+	menf21bmc_wd->info = &menf21bmc_wd_info;
+	menf21bmc_wd->min_timeout = BMC_WD_TIMEOUT_MIN;
+	menf21bmc_wd->max_timeout = BMC_WD_TIMEOUT_MAX;
+
+	/*
+	 * Get the current wdt timeout value from the BMC because
+	 * the BMC will save the value set before if the system restarts.
+	 */
+	bmc_timeout = menf21bmc->read_word_data(menf21bmc->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(menf21bmc_wd, bmc_timeout / 10, &pdev->dev);
+
+	watchdog_set_nowayout(menf21bmc_wd, nowayout);
+	watchdog_set_drvdata(menf21bmc_wd, driver_data);
+	platform_set_drvdata(pdev, driver_data);
+
+	ret = menf21bmc_wd_set_bootstatus(driver_data);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to set Watchdog bootstatus\n");
+		return ret;
+	}
+
+	ret = watchdog_register_device(&driver_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_wd_remove(struct platform_device *pdev)
+{
+	struct menf21bmc_wd_data *data = platform_get_drvdata(pdev);
+
+	dev_warn(&pdev->dev,
+		"Untregister MEN 14F021P00 BMC Watchdog device, board may reset\n");
+
+	watchdog_unregister_device(&data->wdt);
+
+	return 0;
+}
+
+static void menf21bmc_wd_shutdown(struct platform_device *pdev)
+{
+	struct menf21bmc_wd_data *data = platform_get_drvdata(pdev);
+	struct menf21bmc *menf21bmc = data->menf21bmc;
+
+	menf21bmc->write_word_data(menf21bmc->client,
+					BMC_CMD_WD_OFF, BMC_WD_OFF_VAL);
+}
+
+static struct  platform_driver menf21bmc_wd = {
+	.driver		= {
+		.owner = THIS_MODULE,
+		.name	= DEVNAME,
+	},
+	.probe		= menf21bmc_wd_probe,
+	.remove		= menf21bmc_wd_remove,
+	.shutdown	= menf21bmc_wd_shutdown,
+};
+
+module_platform_driver(menf21bmc_wd);
+
+MODULE_DESCRIPTION("MEN 14F021P00 BMC Watchdog driver");
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:menf21bmc_wd");
-- 
1.9.3

[PATCH v2 3/3] 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 Controll include four LEDs which
can be switched on and off.

This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 drivers/leds/Kconfig          |   6 ++
 drivers/leds/Makefile         |   2 +-
 drivers/leds/leds-menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-menf21bmc.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1e81d7a..8f98e0f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -466,6 +466,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 79c5155..1c465fe 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -54,7 +54,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..6099524
--- /dev/null
+++ b/drivers/leds/leds-menf21bmc.c
@@ -0,0 +1,136 @@
+/*
+ *  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
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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/mfd/menf21bmc.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_data {
+	struct led_classdev cdev;
+	struct menf21bmc *menf21bmc;
+	u8 led_bit;
+	const char *name;
+};
+
+static struct menf21bmc_led_data 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_data *led = container_of(led_cdev,
+					struct menf21bmc_led_data, cdev);
+	struct menf21bmc *menf21bmc = led->menf21bmc;
+
+	mutex_lock(&led_lock);
+	led_val = menf21bmc->read_byte_data(menf21bmc->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;
+
+	menf21bmc->write_byte_data(menf21bmc->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 menf21bmc *menf21bmc = dev_get_drvdata(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].menf21bmc = menf21bmc;
+
+		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 devicei\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");
+MODULE_ALIAS("platform:menf21bmc_led");
-- 
1.9.3

[PATCH v2 3/3] 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 Controll include four LEDs which
can be switched on and off.

This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.

Signed-off-by: Andreas Werner <andreas.werner@men.de>
---
 drivers/leds/Kconfig          |   6 ++
 drivers/leds/Makefile         |   2 +-
 drivers/leds/leds-menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 143 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-menf21bmc.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 1e81d7a..8f98e0f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -466,6 +466,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 79c5155..1c465fe 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -54,7 +54,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..6099524
--- /dev/null
+++ b/drivers/leds/leds-menf21bmc.c
@@ -0,0 +1,136 @@
+/*
+ *  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
+ *  Author: Andreas Werner <andreas.werner@men.de>
+ *  All rights reserved.
+ *
+ *  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/mfd/menf21bmc.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_data {
+	struct led_classdev cdev;
+	struct menf21bmc *menf21bmc;
+	u8 led_bit;
+	const char *name;
+};
+
+static struct menf21bmc_led_data 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_data *led = container_of(led_cdev,
+					struct menf21bmc_led_data, cdev);
+	struct menf21bmc *menf21bmc = led->menf21bmc;
+
+	mutex_lock(&led_lock);
+	led_val = menf21bmc->read_byte_data(menf21bmc->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;
+
+	menf21bmc->write_byte_data(menf21bmc->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 menf21bmc *menf21bmc = dev_get_drvdata(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].menf21bmc = menf21bmc;
+
+		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 devicei\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");
+MODULE_ALIAS("platform:menf21bmc_led");
-- 
1.9.3

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

[Lee Jones]

> 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
> 
> Signed-off-by: Andreas Werner <andreas.werner@men.de>
> ---
>  drivers/mfd/Kconfig           |  12 +++
>  drivers/mfd/Makefile          |   1 +
>  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/menf21bmc.h |  31 ++++++
>  4 files changed, 264 insertions(+)
>  create mode 100644 drivers/mfd/menf21bmc.c
>  create mode 100644 include/linux/mfd/menf21bmc.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index ab5a43c..7c2e8d2 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -427,6 +427,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=y
> +	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.
> +	  This driver provides common support for accessing the devices;
> +	  additional drivers must be enabled in order to use the
> +	  functionality of the BMC device.

Would be good if you mention the WD and LED devices here too.

> +
> +

To many '\n's here.

>  config EZX_PCAP
>  	bool "Motorola EZXPCAP Support"
>  	depends on SPI_MASTER
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 5913208..8f2be38 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -167,3 +167,4 @@ 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
> diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> new file mode 100644
> index 0000000..8de92b5
> --- /dev/null
> +++ b/drivers/mfd/menf21bmc.c
> @@ -0,0 +1,220 @@
> +/*
> + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> + *
> + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> + *  Author: Andreas Werner <andreas.werner@men.de>
> + *  All rights reserved.
> + *
> + *  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>
> +#include <linux/mfd/menf21bmc.h>
> +
> +#define BMC_CMD_REV_MAJOR	0x80
> +#define BMC_CMD_REV_MINOR	0x81
> +#define BMC_CMD_REV_MAIN	0x82
> +#define BMC_CMD_REV_BUILD	0x83
> +#define BMC_CMD_REV_VERI	0x84
> +#define BMC_CMD_WD_ARM_SET	0x18
> +#define BMC_CMD_WD_ARM_GET	0x19
> +
> +static struct mfd_cell menf21bmc_cell[] = {
> +	{
> +		.name = "menf21bmc_wd",

I prefer wdog to wd, as it's a little move obvious what we're dealing
with.

> +	},
> +	{
> +		.name = "menf21bmc_led",
> +	},
> +};

If these are the only struct attributes, please place them on a single
line (each).

> +static int
> +menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
> +{
> +	int ret;
> +	struct menf21bmc *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_byte_data(client, reg);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int
> +menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
> +{
> +	int ret;
> +	struct menf21bmc *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_read_word_data(client, reg);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> +{
> +	int ret;
> +	struct menf21bmc *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_write_byte_data(client, reg, val);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
> +{
> +	int ret;
> +	struct menf21bmc *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_write_word_data(client, reg, val);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> +{
> +	int ret;
> +	struct menf21bmc *data = i2c_get_clientdata(client);
> +
> +	mutex_lock(&data->lock);
> +	ret = i2c_smbus_write_byte(client, val);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}

Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
from within the driver.  The I2C subsystem conducts its own locking.
I'm really starting to frown on aggregation for the sake of
aggregation.  It's just overhead.

> +static int menf21bmc_wd_leave_prod_mode(struct i2c_client *client)
> +{
> +	int val, ret;
> +
> +	val = menf21bmc_read_byte_data(client, BMC_CMD_WD_ARM_GET);

ARM like the processor, or 'arm' as in "arm the missile", or something
else?

> +	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 leave the mode
> +	 * if active.
> +	 */

I prefer the term 'exit', as 'leave' is ambiguous.  It could read
"leave it in production mode", or "leave (exit) production mode".

> +	if (val == 0x00) {
> +		dev_info(&client->dev,
> +			"BMC in production mode. Leave production mode\n");
> +
> +		ret = menf21bmc_write_byte(client, BMC_CMD_WD_ARM_SET);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> +{
> +	int ret;
> +	struct menf21bmc *data;
> +	int rev_major, rev_minor, rev_main;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +						I2C_FUNC_SMBUS_BYTE_DATA |
> +						I2C_FUNC_SMBUS_WORD_DATA |
> +						I2C_FUNC_SMBUS_BYTE))

Tabbing is off.  Try to line up with the '('.

> +		return -EIO;

-ENODEV is more common.

> +	data = devm_kzalloc(&client->dev, sizeof(struct menf21bmc), GFP_KERNEL);

s/sizeof(struct menf21bmc)/sizeof(*data)/

data doesn't tend to be a good name for a variable.

> +	if (data == NULL)

if (!data)

> +		return -ENOMEM;
> +
> +	mutex_init(&data->lock);
> +
> +	i2c_set_clientdata(client, data);
> +	data->client = client;
> +	data->read_word_data = menf21bmc_read_word_data;
> +	data->read_byte_data = menf21bmc_read_byte_data;
> +	data->write_word_data = menf21bmc_write_word_data;
> +	data->write_byte_data = menf21bmc_write_byte_data;
> +	data->write_byte = menf21bmc_write_byte;

Yuck, please remove these.

> +	rev_major = menf21bmc_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 = menf21bmc_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 = menf21bmc_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 leave the Production Mode of the BMC to activate the
> +	 * Watchdog functionality and the BIOS life sign monitoring.
> +	 */
> +	ret = menf21bmc_wd_leave_prod_mode(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to leave production mode\n");
> +		return ret;
> +	}

Is it possible that someone might want to use production mode?

> +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "mfd_add_devices failed\n");

This error message should be more useful to the user.

E.g. "Failed to add sub-devices"

> +		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},

Please add a ' ' after '{' and before '}'.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> +
> +static struct i2c_driver menf21bmc_driver = {
> +	.driver		= {
> +		.name	= "menf21bmc",
> +		.owner	= THIS_MODULE,
> +	},
> +	.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");

Missing '>'.

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/menf21bmc.h b/include/linux/mfd/menf21bmc.h
> new file mode 100644
> index 0000000..1fc9b5c
> --- /dev/null
> +++ b/include/linux/mfd/menf21bmc.h
> @@ -0,0 +1,31 @@
> +/*
> + *  MEN 14F021P00 Board Management Controller (BMC)
> + *
> + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> + *  Author: Andreas Werner <andreas.werner@men.de>
> + *  All rights reserved.
> + *
> + *  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.
> + *
> + */
> +
> +#ifndef MENF21BMC_H
> +#define MENF21BMC_H
> +
> +struct menf21bmc {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +
> +	int (*read_word_data)(struct i2c_client *client, u8 reg);
> +	int (*read_byte_data)(struct i2c_client *client, u8 reg);
> +	int (*write_word_data)(struct i2c_client *client, u8 reg, u16 val);
> +	int (*write_byte_data)(struct i2c_client *client, u8 reg, u8 val);
> +	int (*write_byte)(struct i2c_client *client, u8 val);
> +};
> +
> +#endif /* MENF21BMC_H */
> +
> +

Too many '\n's here.

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

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

[Johannes Thumshirn]

On Tue, May 27, 2014 at 04:05:47PM +0100, Lee Jones 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
> >
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/mfd/Kconfig           |  12 +++
> >  drivers/mfd/Makefile          |   1 +
> >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/menf21bmc.h |  31 ++++++
> >  4 files changed, 264 insertions(+)
> >  create mode 100644 drivers/mfd/menf21bmc.c
> >  create mode 100644 include/linux/mfd/menf21bmc.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ab5a43c..7c2e8d2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -427,6 +427,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=y
> > +	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.
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
>
> Would be good if you mention the WD and LED devices here too.
>
> > +
> > +
>
> To many '\n's here.
>
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5913208..8f2be38 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -167,3 +167,4 @@ 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
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..8de92b5
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,220 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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>
> > +#include <linux/mfd/menf21bmc.h>
> > +
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +#define BMC_CMD_REV_BUILD	0x83
> > +#define BMC_CMD_REV_VERI	0x84
> > +#define BMC_CMD_WD_ARM_SET	0x18
> > +#define BMC_CMD_WD_ARM_GET	0x19
> > +
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{
> > +		.name = "menf21bmc_wd",
>
> I prefer wdog to wd, as it's a little move obvious what we're dealing
> with.
>
> > +	},
> > +	{
> > +		.name = "menf21bmc_led",
> > +	},
> > +};
>
> If these are the only struct attributes, please place them on a single
> line (each).
>
> > +static int
> > +menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_word_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_word_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte(client, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
>
> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> from within the driver.  The I2C subsystem conducts its own locking.
> I'm really starting to frown on aggregation for the sake of
> aggregation.  It's just overhead.
>

Correct me if I'm wrong but as far as I remember Guenther asked to retain the
original API, not the remove the "abstraction layer". Once we build a board with
one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
in order to re-use these drivers.

Just my 2 cents.


     Johannes

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

[Johannes Thumshirn]

On Tue, May 27, 2014 at 04:05:47PM +0100, Lee Jones 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
> >
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/mfd/Kconfig           |  12 +++
> >  drivers/mfd/Makefile          |   1 +
> >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/menf21bmc.h |  31 ++++++
> >  4 files changed, 264 insertions(+)
> >  create mode 100644 drivers/mfd/menf21bmc.c
> >  create mode 100644 include/linux/mfd/menf21bmc.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ab5a43c..7c2e8d2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -427,6 +427,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=y
> > +	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.
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
>
> Would be good if you mention the WD and LED devices here too.
>
> > +
> > +
>
> To many '\n's here.
>
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5913208..8f2be38 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -167,3 +167,4 @@ 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
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..8de92b5
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,220 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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>
> > +#include <linux/mfd/menf21bmc.h>
> > +
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +#define BMC_CMD_REV_BUILD	0x83
> > +#define BMC_CMD_REV_VERI	0x84
> > +#define BMC_CMD_WD_ARM_SET	0x18
> > +#define BMC_CMD_WD_ARM_GET	0x19
> > +
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{
> > +		.name = "menf21bmc_wd",
>
> I prefer wdog to wd, as it's a little move obvious what we're dealing
> with.
>
> > +	},
> > +	{
> > +		.name = "menf21bmc_led",
> > +	},
> > +};
>
> If these are the only struct attributes, please place them on a single
> line (each).
>
> > +static int
> > +menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_word_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_word_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte(client, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
>
> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> from within the driver.  The I2C subsystem conducts its own locking.
> I'm really starting to frown on aggregation for the sake of
> aggregation.  It's just overhead.
>

Correct me if I'm wrong but as far as I remember Guenther asked to retain the
original API, not the remove the "abstraction layer". Once we build a board with
one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
in order to re-use these drivers.

Just my 2 cents.


     Johannes

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

[Lee Jones]

> > > 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
> > >
> > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > ---
> > >  drivers/mfd/Kconfig           |  12 +++
> > >  drivers/mfd/Makefile          |   1 +
> > >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/menf21bmc.h |  31 ++++++
> > >  4 files changed, 264 insertions(+)
> > >  create mode 100644 drivers/mfd/menf21bmc.c
> > >  create mode 100644 include/linux/mfd/menf21bmc.h

[...]

> > > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > > +{
> > > +	int ret;
> > > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > > +
> > > +	mutex_lock(&data->lock);
> > > +	ret = i2c_smbus_write_byte(client, val);
> > > +	mutex_unlock(&data->lock);
> > > +
> > > +	return ret;
> > > +}
> >
> > Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> > from within the driver.  The I2C subsystem conducts its own locking.
> > I'm really starting to frown on aggregation for the sake of
> > aggregation.  It's just overhead.
> >
> 
> Correct me if I'm wrong but as far as I remember Guenther asked to retain the
> original API, not the remove the "abstraction layer". Once we build a board with
> one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
> in order to re-use these drivers.

If there are two or more possible interfaces then I agree, these
aggregations would be the best approach.  However, as it stands, that's
not currently the case.

Genuine question; are Men on the verge of building such a board, or
are we talking about 'ifs' and 'maybes'?

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

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

[Andreas Werner]

aOn Wed, May 28, 2014 at 09:24:05AM +0100, Lee Jones 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
> > > >
> > > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > > ---
> > > >  drivers/mfd/Kconfig           |  12 +++
> > > >  drivers/mfd/Makefile          |   1 +
> > > >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/mfd/menf21bmc.h |  31 ++++++
> > > >  4 files changed, 264 insertions(+)
> > > >  create mode 100644 drivers/mfd/menf21bmc.c
> > > >  create mode 100644 include/linux/mfd/menf21bmc.h
> 
> [...]
> 
> > > > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > > > +{
> > > > +	int ret;
> > > > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > > > +
> > > > +	mutex_lock(&data->lock);
> > > > +	ret = i2c_smbus_write_byte(client, val);
> > > > +	mutex_unlock(&data->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > >
> > > Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> > > from within the driver.  The I2C subsystem conducts its own locking.
> > > I'm really starting to frown on aggregation for the sake of
> > > aggregation.  It's just overhead.
> > >
> > 
> > Correct me if I'm wrong but as far as I remember Guenther asked to retain the
> > original API, not the remove the "abstraction layer". Once we build a board with
> > one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
> > in order to re-use these drivers.
> 
> If there are two or more possible interfaces then I agree, these
> aggregations would be the best approach.  However, as it stands, that's
> not currently the case.
> 
> Genuine question; are Men on the verge of building such a board, or
> are we talking about 'ifs' and 'maybes'?
>

I think it was a missunderstanding. I also thought that i just
have to adapt the wrapper to the original API, that is what I did in the patch,
and not to delete the functions completly.

Anayway, we currently have another project which use a STM32 MCR connected to USB.
A nice feature of this STM is to update the firmware using the USB interface.
This is easy to implement and fast.

We want to use such a functionality in all of our MCRs to easily update the firmware
at the customer if we found a bug.

We also plan to have a BMC connected to USB. Then we have i2c and USB.

Regards
Andy

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

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

[Andreas Werner]

aOn Wed, May 28, 2014 at 09:24:05AM +0100, Lee Jones 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
> > > >
> > > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > > ---
> > > >  drivers/mfd/Kconfig           |  12 +++
> > > >  drivers/mfd/Makefile          |   1 +
> > > >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/mfd/menf21bmc.h |  31 ++++++
> > > >  4 files changed, 264 insertions(+)
> > > >  create mode 100644 drivers/mfd/menf21bmc.c
> > > >  create mode 100644 include/linux/mfd/menf21bmc.h
> 
> [...]
> 
> > > > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > > > +{
> > > > +	int ret;
> > > > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > > > +
> > > > +	mutex_lock(&data->lock);
> > > > +	ret = i2c_smbus_write_byte(client, val);
> > > > +	mutex_unlock(&data->lock);
> > > > +
> > > > +	return ret;
> > > > +}
> > >
> > > Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> > > from within the driver.  The I2C subsystem conducts its own locking.
> > > I'm really starting to frown on aggregation for the sake of
> > > aggregation.  It's just overhead.
> > >
> > 
> > Correct me if I'm wrong but as far as I remember Guenther asked to retain the
> > original API, not the remove the "abstraction layer". Once we build a board with
> > one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
> > in order to re-use these drivers.
> 
> If there are two or more possible interfaces then I agree, these
> aggregations would be the best approach.  However, as it stands, that's
> not currently the case.
> 
> Genuine question; are Men on the verge of building such a board, or
> are we talking about 'ifs' and 'maybes'?
>

I think it was a missunderstanding. I also thought that i just
have to adapt the wrapper to the original API, that is what I did in the patch,
and not to delete the functions completly.

Anayway, we currently have another project which use a STM32 MCR connected to USB.
A nice feature of this STM is to update the firmware using the USB interface.
This is easy to implement and fast.

We want to use such a functionality in all of our MCRs to easily update the firmware
at the customer if we found a bug.

We also plan to have a BMC connected to USB. Then we have i2c and USB.

Regards
Andy

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

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

[Guenter Roeck]

On 05/28/2014 12:02 AM, Johannes Thumshirn wrote:
[ ... ]

>>> +
>>> +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
>>> +{
>>> +	int ret;
>>> +	struct menf21bmc *data = i2c_get_clientdata(client);
>>> +
>>> +	mutex_lock(&data->lock);
>>> +	ret = i2c_smbus_write_byte(client, val);
>>> +	mutex_unlock(&data->lock);
>>> +
>>> +	return ret;
>>> +}
>>
>> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
>> from within the driver.  The I2C subsystem conducts its own locking.
>> I'm really starting to frown on aggregation for the sake of
>> aggregation.  It's just overhead.
>>
>
> Correct me if I'm wrong but as far as I remember Guenther asked to retain the
> original API, not the remove the "abstraction layer". Once we build a board with
> one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
> in order to re-use these drivers.
>

If you introduce boards with SPI support, you'll likely have to change those APIs,
since some of the functions don't make sense in the SPI world. So I tend to agree
with Lee. If you need those APIs later on you can introduce them at that time,
and introduce them such that they truly work for all hardware variants.

Guenter

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

[Guenter Roeck]

On 05/28/2014 04:51 AM, Andreas Werner wrote:
> aOn Wed, May 28, 2014 at 09:24:05AM +0100, Lee Jones 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
>>>>>
>>>>> Signed-off-by: Andreas Werner <andreas.werner@men.de>
>>>>> ---
>>>>>   drivers/mfd/Kconfig           |  12 +++
>>>>>   drivers/mfd/Makefile          |   1 +
>>>>>   drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
>>>>>   include/linux/mfd/menf21bmc.h |  31 ++++++
>>>>>   4 files changed, 264 insertions(+)
>>>>>   create mode 100644 drivers/mfd/menf21bmc.c
>>>>>   create mode 100644 include/linux/mfd/menf21bmc.h
>>
>> [...]
>>
>>>>> +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
>>>>> +{
>>>>> +	int ret;
>>>>> +	struct menf21bmc *data = i2c_get_clientdata(client);
>>>>> +
>>>>> +	mutex_lock(&data->lock);
>>>>> +	ret = i2c_smbus_write_byte(client, val);
>>>>> +	mutex_unlock(&data->lock);
>>>>> +
>>>>> +	return ret;
>>>>> +}
>>>>
>>>> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
>>>> from within the driver.  The I2C subsystem conducts its own locking.
>>>> I'm really starting to frown on aggregation for the sake of
>>>> aggregation.  It's just overhead.
>>>>
>>>
>>> Correct me if I'm wrong but as far as I remember Guenther asked to retain the
>>> original API, not the remove the "abstraction layer". Once we build a board with
>>> one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
>>> in order to re-use these drivers.
>>
>> If there are two or more possible interfaces then I agree, these
>> aggregations would be the best approach.  However, as it stands, that's
>> not currently the case.
>>
>> Genuine question; are Men on the verge of building such a board, or
>> are we talking about 'ifs' and 'maybes'?
>>
>
> I think it was a missunderstanding. I also thought that i just
> have to adapt the wrapper to the original API, that is what I did in the patch,
> and not to delete the functions completly.
>
> Anayway, we currently have another project which use a STM32 MCR connected to USB.
> A nice feature of this STM is to update the firmware using the USB interface.
> This is easy to implement and fast.
>
> We want to use such a functionality in all of our MCRs to easily update the firmware
> at the customer if we found a bug.
>
> We also plan to have a BMC connected to USB. Then we have i2c and USB.
>

Even more likely that you would have to change the API in that case to be less
I2C centric.

Guenter

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

[Guenter Roeck]

On 05/28/2014 06:29 AM, Guenter Roeck wrote:
> On 05/28/2014 04:51 AM, Andreas Werner wrote:
>> aOn Wed, May 28, 2014 at 09:24:05AM +0100, Lee Jones 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
>>>>>>
>>>>>> Signed-off-by: Andreas Werner <andreas.werner@men.de>
>>>>>> ---
>>>>>>   drivers/mfd/Kconfig           |  12 +++
>>>>>>   drivers/mfd/Makefile          |   1 +
>>>>>>   drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>   include/linux/mfd/menf21bmc.h |  31 ++++++
>>>>>>   4 files changed, 264 insertions(+)
>>>>>>   create mode 100644 drivers/mfd/menf21bmc.c
>>>>>>   create mode 100644 include/linux/mfd/menf21bmc.h
>>>
>>> [...]
>>>
>>>>>> +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    struct menf21bmc *data = i2c_get_clientdata(client);
>>>>>> +
>>>>>> +    mutex_lock(&data->lock);
>>>>>> +    ret = i2c_smbus_write_byte(client, val);
>>>>>> +    mutex_unlock(&data->lock);
>>>>>> +
>>>>>> +    return ret;
>>>>>> +}
>>>>>
>>>>> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
>>>>> from within the driver.  The I2C subsystem conducts its own locking.
>>>>> I'm really starting to frown on aggregation for the sake of
>>>>> aggregation.  It's just overhead.
>>>>>
>>>>
>>>> Correct me if I'm wrong but as far as I remember Guenther asked to retain the
>>>> original API, not the remove the "abstraction layer". Once we build a board with
>>>> one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
>>>> in order to re-use these drivers.
>>>
>>> If there are two or more possible interfaces then I agree, these
>>> aggregations would be the best approach.  However, as it stands, that's
>>> not currently the case.
>>>
>>> Genuine question; are Men on the verge of building such a board, or
>>> are we talking about 'ifs' and 'maybes'?
>>>
>>
>> I think it was a missunderstanding. I also thought that i just
>> have to adapt the wrapper to the original API, that is what I did in the patch,
>> and not to delete the functions completly.
>>
>> Anayway, we currently have another project which use a STM32 MCR connected to USB.
>> A nice feature of this STM is to update the firmware using the USB interface.
>> This is easy to implement and fast.
>>
>> We want to use such a functionality in all of our MCRs to easily update the firmware
>> at the customer if we found a bug.
>>
>> We also plan to have a BMC connected to USB. Then we have i2c and USB.
>>
>
> Even more likely that you would have to change the API in that case to be less
> I2C centric.
>

... especially since you have "struct i2c_client *client" in your API function.

Guenter

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

[Andreas Werner]

On Wed, May 28, 2014 at 06:52:56AM -0700, Guenter Roeck wrote:
> On 05/28/2014 06:29 AM, Guenter Roeck wrote:
> >On 05/28/2014 04:51 AM, Andreas Werner wrote:
> >>aOn Wed, May 28, 2014 at 09:24:05AM +0100, Lee Jones 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
> >>>>>>
> >>>>>>Signed-off-by: Andreas Werner <andreas.werner@men.de>
> >>>>>>---
> >>>>>>  drivers/mfd/Kconfig           |  12 +++
> >>>>>>  drivers/mfd/Makefile          |   1 +
> >>>>>>  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  include/linux/mfd/menf21bmc.h |  31 ++++++
> >>>>>>  4 files changed, 264 insertions(+)
> >>>>>>  create mode 100644 drivers/mfd/menf21bmc.c
> >>>>>>  create mode 100644 include/linux/mfd/menf21bmc.h
> >>>
> >>>[...]
> >>>
> >>>>>>+static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> >>>>>>+{
> >>>>>>+    int ret;
> >>>>>>+    struct menf21bmc *data = i2c_get_clientdata(client);
> >>>>>>+
> >>>>>>+    mutex_lock(&data->lock);
> >>>>>>+    ret = i2c_smbus_write_byte(client, val);
> >>>>>>+    mutex_unlock(&data->lock);
> >>>>>>+
> >>>>>>+    return ret;
> >>>>>>+}
> >>>>>
> >>>>>Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> >>>>>from within the driver.  The I2C subsystem conducts its own locking.
> >>>>>I'm really starting to frown on aggregation for the sake of
> >>>>>aggregation.  It's just overhead.
> >>>>>
> >>>>
> >>>>Correct me if I'm wrong but as far as I remember Guenther asked to retain the
> >>>>original API, not the remove the "abstraction layer". Once we build a board with
> >>>>one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
> >>>>in order to re-use these drivers.
> >>>
> >>>If there are two or more possible interfaces then I agree, these
> >>>aggregations would be the best approach.  However, as it stands, that's
> >>>not currently the case.
> >>>
> >>>Genuine question; are Men on the verge of building such a board, or
> >>>are we talking about 'ifs' and 'maybes'?
> >>>
> >>
> >>I think it was a missunderstanding. I also thought that i just
> >>have to adapt the wrapper to the original API, that is what I did in the patch,
> >>and not to delete the functions completly.
> >>
> >>Anayway, we currently have another project which use a STM32 MCR connected to USB.
> >>A nice feature of this STM is to update the firmware using the USB interface.
> >>This is easy to implement and fast.
> >>
> >>We want to use such a functionality in all of our MCRs to easily update the firmware
> >>at the customer if we found a bug.
> >>
> >>We also plan to have a BMC connected to USB. Then we have i2c and USB.
> >>
> >
> >Even more likely that you would have to change the API in that case to be less
> >I2C centric.
> >
> 
> ... especially since you have "struct i2c_client *client" in your API function.
> 
> Guenter
> 
> 
Ok I agree. I will change that and delete the wrapper functions completley.
I will prepare and send a v3.

Thanks for the discussion.

Regards
Andy

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

[Andreas Werner]

On Wed, May 28, 2014 at 06:52:56AM -0700, Guenter Roeck wrote:
> On 05/28/2014 06:29 AM, Guenter Roeck wrote:
> >On 05/28/2014 04:51 AM, Andreas Werner wrote:
> >>aOn Wed, May 28, 2014 at 09:24:05AM +0100, Lee Jones 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
> >>>>>>
> >>>>>>Signed-off-by: Andreas Werner <andreas.werner@men.de>
> >>>>>>---
> >>>>>>  drivers/mfd/Kconfig           |  12 +++
> >>>>>>  drivers/mfd/Makefile          |   1 +
> >>>>>>  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  include/linux/mfd/menf21bmc.h |  31 ++++++
> >>>>>>  4 files changed, 264 insertions(+)
> >>>>>>  create mode 100644 drivers/mfd/menf21bmc.c
> >>>>>>  create mode 100644 include/linux/mfd/menf21bmc.h
> >>>
> >>>[...]
> >>>
> >>>>>>+static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> >>>>>>+{
> >>>>>>+    int ret;
> >>>>>>+    struct menf21bmc *data = i2c_get_clientdata(client);
> >>>>>>+
> >>>>>>+    mutex_lock(&data->lock);
> >>>>>>+    ret = i2c_smbus_write_byte(client, val);
> >>>>>>+    mutex_unlock(&data->lock);
> >>>>>>+
> >>>>>>+    return ret;
> >>>>>>+}
> >>>>>
> >>>>>Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> >>>>>from within the driver.  The I2C subsystem conducts its own locking.
> >>>>>I'm really starting to frown on aggregation for the sake of
> >>>>>aggregation.  It's just overhead.
> >>>>>
> >>>>
> >>>>Correct me if I'm wrong but as far as I remember Guenther asked to retain the
> >>>>original API, not the remove the "abstraction layer". Once we build a board with
> >>>>one of these BMCs attached via e.g. SPI we would have to reintroduce it anyways,
> >>>>in order to re-use these drivers.
> >>>
> >>>If there are two or more possible interfaces then I agree, these
> >>>aggregations would be the best approach.  However, as it stands, that's
> >>>not currently the case.
> >>>
> >>>Genuine question; are Men on the verge of building such a board, or
> >>>are we talking about 'ifs' and 'maybes'?
> >>>
> >>
> >>I think it was a missunderstanding. I also thought that i just
> >>have to adapt the wrapper to the original API, that is what I did in the patch,
> >>and not to delete the functions completly.
> >>
> >>Anayway, we currently have another project which use a STM32 MCR connected to USB.
> >>A nice feature of this STM is to update the firmware using the USB interface.
> >>This is easy to implement and fast.
> >>
> >>We want to use such a functionality in all of our MCRs to easily update the firmware
> >>at the customer if we found a bug.
> >>
> >>We also plan to have a BMC connected to USB. Then we have i2c and USB.
> >>
> >
> >Even more likely that you would have to change the API in that case to be less
> >I2C centric.
> >
> 
> ... especially since you have "struct i2c_client *client" in your API function.
> 
> Guenter
> 
> 
Ok I agree. I will change that and delete the wrapper functions completley.
I will prepare and send a v3.

Thanks for the discussion.

Regards
Andy

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

[Andreas Werner]

On Tue, May 27, 2014 at 04:05:47PM +0100, Lee Jones 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
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/mfd/Kconfig           |  12 +++
> >  drivers/mfd/Makefile          |   1 +
> >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/menf21bmc.h |  31 ++++++
> >  4 files changed, 264 insertions(+)
> >  create mode 100644 drivers/mfd/menf21bmc.c
> >  create mode 100644 include/linux/mfd/menf21bmc.h
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ab5a43c..7c2e8d2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -427,6 +427,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=y
> > +	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.
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
> 
> Would be good if you mention the WD and LED devices here too.
> 
> > +
> > +
> 
> To many '\n's here.

I will ad WD and LED and fix the \n.

> 
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5913208..8f2be38 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -167,3 +167,4 @@ 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
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..8de92b5
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,220 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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>
> > +#include <linux/mfd/menf21bmc.h>
> > +
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +#define BMC_CMD_REV_BUILD	0x83
> > +#define BMC_CMD_REV_VERI	0x84
> > +#define BMC_CMD_WD_ARM_SET	0x18
> > +#define BMC_CMD_WD_ARM_GET	0x19
> > +
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{
> > +		.name = "menf21bmc_wd",
> 
> I prefer wdog to wd, as it's a little move obvious what we're dealing
> with.

I agree.

> 
> > +	},
> > +	{
> > +		.name = "menf21bmc_led",
> > +	},
> > +};
> 
> If these are the only struct attributes, please place them on a single
> line (each).
> 

Ok i'll change that.

> > +static int
> > +menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_word_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_word_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte(client, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> 
> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> from within the driver.  The I2C subsystem conducts its own locking.
> I'm really starting to frown on aggregation for the sake of
> aggregation.  It's just overhead.
> 

I'll remove that.

> > +static int menf21bmc_wd_leave_prod_mode(struct i2c_client *client)
> > +{
> > +	int val, ret;
> > +
> > +	val = menf21bmc_read_byte_data(client, BMC_CMD_WD_ARM_GET);
> 
> ARM like the processor, or 'arm' as in "arm the missile", or something
> else?
> 

I will change that name.

> > +	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 leave the mode
> > +	 * if active.
> > +	 */
> 
> I prefer the term 'exit', as 'leave' is ambiguous.  It could read
> "leave it in production mode", or "leave (exit) production mode".
> 

I agree.

> > +	if (val == 0x00) {
> > +		dev_info(&client->dev,
> > +			"BMC in production mode. Leave production mode\n");
> > +
> > +		ret = menf21bmc_write_byte(client, BMC_CMD_WD_ARM_SET);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data;
> > +	int rev_major, rev_minor, rev_main;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +						I2C_FUNC_SMBUS_BYTE_DATA |
> > +						I2C_FUNC_SMBUS_WORD_DATA |
> > +						I2C_FUNC_SMBUS_BYTE))
> 
> Tabbing is off.  Try to line up with the '('.
> 

I agree.

> > +		return -EIO;
> 
> -ENODEV is more common.

I agree.

> 
> > +	data = devm_kzalloc(&client->dev, sizeof(struct menf21bmc), GFP_KERNEL);
> 
> s/sizeof(struct menf21bmc)/sizeof(*data)/
> 
> data doesn't tend to be a good name for a variable.
> 
> > +	if (data == NULL)
> 
> if (!data)
> 
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&data->lock);
> > +
> > +	i2c_set_clientdata(client, data);
> > +	data->client = client;
> > +	data->read_word_data = menf21bmc_read_word_data;
> > +	data->read_byte_data = menf21bmc_read_byte_data;
> > +	data->write_word_data = menf21bmc_write_word_data;
> > +	data->write_byte_data = menf21bmc_write_byte_data;
> > +	data->write_byte = menf21bmc_write_byte;
> 
> Yuck, please remove these.
>

Yes will be removed.
 
> > +	rev_major = menf21bmc_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 = menf21bmc_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 = menf21bmc_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 leave the Production Mode of the BMC to activate the
> > +	 * Watchdog functionality and the BIOS life sign monitoring.
> > +	 */
> > +	ret = menf21bmc_wd_leave_prod_mode(client);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to leave production mode\n");
> > +		return ret;
> > +	}
> 
> Is it possible that someone might want to use production mode?
> 

No it is just for the board bring up. Because if the BIOS does not implement
the Life sign the BMC will not start the Board. Production mode is
active after programming the PIC.

> > +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> > +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "mfd_add_devices failed\n");
> 
> This error message should be more useful to the user.
> 
> E.g. "Failed to add sub-devices"
> 

I'll change that.

> > +		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},
> 
> Please add a ' ' after '{' and before '}'.
> 

I agree.

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > +
> > +static struct i2c_driver menf21bmc_driver = {
> > +	.driver		= {
> > +		.name	= "menf21bmc",
> > +		.owner	= THIS_MODULE,
> > +	},
> > +	.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");
> 
> Missing '>'.

I'll change that.

> 
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/menf21bmc.h b/include/linux/mfd/menf21bmc.h
> > new file mode 100644
> > index 0000000..1fc9b5c
> > --- /dev/null
> > +++ b/include/linux/mfd/menf21bmc.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC)
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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.
> > + *
> > + */
> > +
> > +#ifndef MENF21BMC_H
> > +#define MENF21BMC_H
> > +
> > +struct menf21bmc {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> > +
> > +	int (*read_word_data)(struct i2c_client *client, u8 reg);
> > +	int (*read_byte_data)(struct i2c_client *client, u8 reg);
> > +	int (*write_word_data)(struct i2c_client *client, u8 reg, u16 val);
> > +	int (*write_byte_data)(struct i2c_client *client, u8 reg, u8 val);
> > +	int (*write_byte)(struct i2c_client *client, u8 val);
> > +};
> > +
> > +#endif /* MENF21BMC_H */
> > +
> > +
> 
> Too many '\n's here.
>

I'll change that.
 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog


Thanks.
Andy

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

[Andreas Werner]

On Tue, May 27, 2014 at 04:05:47PM +0100, Lee Jones 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
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/mfd/Kconfig           |  12 +++
> >  drivers/mfd/Makefile          |   1 +
> >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/menf21bmc.h |  31 ++++++
> >  4 files changed, 264 insertions(+)
> >  create mode 100644 drivers/mfd/menf21bmc.c
> >  create mode 100644 include/linux/mfd/menf21bmc.h
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ab5a43c..7c2e8d2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -427,6 +427,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=y
> > +	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.
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
> 
> Would be good if you mention the WD and LED devices here too.
> 
> > +
> > +
> 
> To many '\n's here.

I will ad WD and LED and fix the \n.

> 
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5913208..8f2be38 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -167,3 +167,4 @@ 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
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..8de92b5
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,220 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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>
> > +#include <linux/mfd/menf21bmc.h>
> > +
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +#define BMC_CMD_REV_BUILD	0x83
> > +#define BMC_CMD_REV_VERI	0x84
> > +#define BMC_CMD_WD_ARM_SET	0x18
> > +#define BMC_CMD_WD_ARM_GET	0x19
> > +
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{
> > +		.name = "menf21bmc_wd",
> 
> I prefer wdog to wd, as it's a little move obvious what we're dealing
> with.

I agree.

> 
> > +	},
> > +	{
> > +		.name = "menf21bmc_led",
> > +	},
> > +};
> 
> If these are the only struct attributes, please place them on a single
> line (each).
> 

Ok i'll change that.

> > +static int
> > +menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_word_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_word_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte(client, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> 
> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> from within the driver.  The I2C subsystem conducts its own locking.
> I'm really starting to frown on aggregation for the sake of
> aggregation.  It's just overhead.
> 

I'll remove that.

> > +static int menf21bmc_wd_leave_prod_mode(struct i2c_client *client)
> > +{
> > +	int val, ret;
> > +
> > +	val = menf21bmc_read_byte_data(client, BMC_CMD_WD_ARM_GET);
> 
> ARM like the processor, or 'arm' as in "arm the missile", or something
> else?
> 

I will change that name.

> > +	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 leave the mode
> > +	 * if active.
> > +	 */
> 
> I prefer the term 'exit', as 'leave' is ambiguous.  It could read
> "leave it in production mode", or "leave (exit) production mode".
> 

I agree.

> > +	if (val == 0x00) {
> > +		dev_info(&client->dev,
> > +			"BMC in production mode. Leave production mode\n");
> > +
> > +		ret = menf21bmc_write_byte(client, BMC_CMD_WD_ARM_SET);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data;
> > +	int rev_major, rev_minor, rev_main;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +						I2C_FUNC_SMBUS_BYTE_DATA |
> > +						I2C_FUNC_SMBUS_WORD_DATA |
> > +						I2C_FUNC_SMBUS_BYTE))
> 
> Tabbing is off.  Try to line up with the '('.
> 

I agree.

> > +		return -EIO;
> 
> -ENODEV is more common.

I agree.

> 
> > +	data = devm_kzalloc(&client->dev, sizeof(struct menf21bmc), GFP_KERNEL);
> 
> s/sizeof(struct menf21bmc)/sizeof(*data)/
> 
> data doesn't tend to be a good name for a variable.
> 
> > +	if (data == NULL)
> 
> if (!data)
> 
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&data->lock);
> > +
> > +	i2c_set_clientdata(client, data);
> > +	data->client = client;
> > +	data->read_word_data = menf21bmc_read_word_data;
> > +	data->read_byte_data = menf21bmc_read_byte_data;
> > +	data->write_word_data = menf21bmc_write_word_data;
> > +	data->write_byte_data = menf21bmc_write_byte_data;
> > +	data->write_byte = menf21bmc_write_byte;
> 
> Yuck, please remove these.
>

Yes will be removed.
 
> > +	rev_major = menf21bmc_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 = menf21bmc_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 = menf21bmc_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 leave the Production Mode of the BMC to activate the
> > +	 * Watchdog functionality and the BIOS life sign monitoring.
> > +	 */
> > +	ret = menf21bmc_wd_leave_prod_mode(client);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to leave production mode\n");
> > +		return ret;
> > +	}
> 
> Is it possible that someone might want to use production mode?
> 

No it is just for the board bring up. Because if the BIOS does not implement
the Life sign the BMC will not start the Board. Production mode is
active after programming the PIC.

> > +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> > +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "mfd_add_devices failed\n");
> 
> This error message should be more useful to the user.
> 
> E.g. "Failed to add sub-devices"
> 

I'll change that.

> > +		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},
> 
> Please add a ' ' after '{' and before '}'.
> 

I agree.

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > +
> > +static struct i2c_driver menf21bmc_driver = {
> > +	.driver		= {
> > +		.name	= "menf21bmc",
> > +		.owner	= THIS_MODULE,
> > +	},
> > +	.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");
> 
> Missing '>'.

I'll change that.

> 
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/menf21bmc.h b/include/linux/mfd/menf21bmc.h
> > new file mode 100644
> > index 0000000..1fc9b5c
> > --- /dev/null
> > +++ b/include/linux/mfd/menf21bmc.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC)
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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.
> > + *
> > + */
> > +
> > +#ifndef MENF21BMC_H
> > +#define MENF21BMC_H
> > +
> > +struct menf21bmc {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> > +
> > +	int (*read_word_data)(struct i2c_client *client, u8 reg);
> > +	int (*read_byte_data)(struct i2c_client *client, u8 reg);
> > +	int (*write_word_data)(struct i2c_client *client, u8 reg, u16 val);
> > +	int (*write_byte_data)(struct i2c_client *client, u8 reg, u8 val);
> > +	int (*write_byte)(struct i2c_client *client, u8 val);
> > +};
> > +
> > +#endif /* MENF21BMC_H */
> > +
> > +
> 
> Too many '\n's here.
>

I'll change that.
 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog


Thanks.
Andy

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

[Andreas Werner]

On Tue, May 27, 2014 at 04:05:47PM +0100, Lee Jones 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
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/mfd/Kconfig           |  12 +++
> >  drivers/mfd/Makefile          |   1 +
> >  drivers/mfd/menf21bmc.c       | 220 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/menf21bmc.h |  31 ++++++
> >  4 files changed, 264 insertions(+)
> >  create mode 100644 drivers/mfd/menf21bmc.c
> >  create mode 100644 include/linux/mfd/menf21bmc.h
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ab5a43c..7c2e8d2 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -427,6 +427,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=y
> > +	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.
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
> 
> Would be good if you mention the WD and LED devices here too.
> 
> > +
> > +
> 
> To many '\n's here.

I will ad WD and LED and fix the \n.

> 
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 5913208..8f2be38 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -167,3 +167,4 @@ 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
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..8de92b5
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,220 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC) MFD Core Driver.
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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>
> > +#include <linux/mfd/menf21bmc.h>
> > +
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +#define BMC_CMD_REV_BUILD	0x83
> > +#define BMC_CMD_REV_VERI	0x84
> > +#define BMC_CMD_WD_ARM_SET	0x18
> > +#define BMC_CMD_WD_ARM_GET	0x19
> > +
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{
> > +		.name = "menf21bmc_wd",
> 
> I prefer wdog to wd, as it's a little move obvious what we're dealing
> with.

I agree.

> 
> > +	},
> > +	{
> > +		.name = "menf21bmc_led",
> > +	},
> > +};
> 
> If these are the only struct attributes, please place them on a single
> line (each).
> 

Ok i'll change that.

> > +static int
> > +menf21bmc_read_byte_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_byte_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int
> > +menf21bmc_read_word_data(struct i2c_client *client, u8 reg)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_read_word_data(client, reg);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte_data(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_word_data(struct i2c_client *client, u8 reg, u16 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_word_data(client, reg, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int menf21bmc_write_byte(struct i2c_client *client, u8 val)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data = i2c_get_clientdata(client);
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = i2c_smbus_write_byte(client, val);
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> 
> Didn't we ask you to remove these?  Just make the i2c_smbus_* calls
> from within the driver.  The I2C subsystem conducts its own locking.
> I'm really starting to frown on aggregation for the sake of
> aggregation.  It's just overhead.
> 

I'll remove that.

> > +static int menf21bmc_wd_leave_prod_mode(struct i2c_client *client)
> > +{
> > +	int val, ret;
> > +
> > +	val = menf21bmc_read_byte_data(client, BMC_CMD_WD_ARM_GET);
> 
> ARM like the processor, or 'arm' as in "arm the missile", or something
> else?
> 

I will change that name.

> > +	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 leave the mode
> > +	 * if active.
> > +	 */
> 
> I prefer the term 'exit', as 'leave' is ambiguous.  It could read
> "leave it in production mode", or "leave (exit) production mode".
> 

I agree.

> > +	if (val == 0x00) {
> > +		dev_info(&client->dev,
> > +			"BMC in production mode. Leave production mode\n");
> > +
> > +		ret = menf21bmc_write_byte(client, BMC_CMD_WD_ARM_SET);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> > +{
> > +	int ret;
> > +	struct menf21bmc *data;
> > +	int rev_major, rev_minor, rev_main;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +						I2C_FUNC_SMBUS_BYTE_DATA |
> > +						I2C_FUNC_SMBUS_WORD_DATA |
> > +						I2C_FUNC_SMBUS_BYTE))
> 
> Tabbing is off.  Try to line up with the '('.
> 

I agree.

> > +		return -EIO;
> 
> -ENODEV is more common.

I agree.

> 
> > +	data = devm_kzalloc(&client->dev, sizeof(struct menf21bmc), GFP_KERNEL);
> 
> s/sizeof(struct menf21bmc)/sizeof(*data)/
> 
> data doesn't tend to be a good name for a variable.
> 
> > +	if (data == NULL)
> 
> if (!data)
> 
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&data->lock);
> > +
> > +	i2c_set_clientdata(client, data);
> > +	data->client = client;
> > +	data->read_word_data = menf21bmc_read_word_data;
> > +	data->read_byte_data = menf21bmc_read_byte_data;
> > +	data->write_word_data = menf21bmc_write_word_data;
> > +	data->write_byte_data = menf21bmc_write_byte_data;
> > +	data->write_byte = menf21bmc_write_byte;
> 
> Yuck, please remove these.
>

Yes will be removed.
 
> > +	rev_major = menf21bmc_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 = menf21bmc_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 = menf21bmc_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 leave the Production Mode of the BMC to activate the
> > +	 * Watchdog functionality and the BIOS life sign monitoring.
> > +	 */
> > +	ret = menf21bmc_wd_leave_prod_mode(client);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to leave production mode\n");
> > +		return ret;
> > +	}
> 
> Is it possible that someone might want to use production mode?
> 

No it is just for the board bring up. Because if the BIOS does not implement
the Life sign the BMC will not start the Board. Production mode is
active after programming the PIC.

> > +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> > +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "mfd_add_devices failed\n");
> 
> This error message should be more useful to the user.
> 
> E.g. "Failed to add sub-devices"
> 

I'll change that.

> > +		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},
> 
> Please add a ' ' after '{' and before '}'.
> 

I agree.

> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > +
> > +static struct i2c_driver menf21bmc_driver = {
> > +	.driver		= {
> > +		.name	= "menf21bmc",
> > +		.owner	= THIS_MODULE,
> > +	},
> > +	.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");
> 
> Missing '>'.

I'll change that.

> 
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/menf21bmc.h b/include/linux/mfd/menf21bmc.h
> > new file mode 100644
> > index 0000000..1fc9b5c
> > --- /dev/null
> > +++ b/include/linux/mfd/menf21bmc.h
> > @@ -0,0 +1,31 @@
> > +/*
> > + *  MEN 14F021P00 Board Management Controller (BMC)
> > + *
> > + *  Copyright (C) 2014 MEN Mikro Elektronik Nuernberg GmbH
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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.
> > + *
> > + */
> > +
> > +#ifndef MENF21BMC_H
> > +#define MENF21BMC_H
> > +
> > +struct menf21bmc {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> > +
> > +	int (*read_word_data)(struct i2c_client *client, u8 reg);
> > +	int (*read_byte_data)(struct i2c_client *client, u8 reg);
> > +	int (*write_word_data)(struct i2c_client *client, u8 reg, u16 val);
> > +	int (*write_byte_data)(struct i2c_client *client, u8 reg, u8 val);
> > +	int (*write_byte)(struct i2c_client *client, u8 val);
> > +};
> > +
> > +#endif /* MENF21BMC_H */
> > +
> > +
> 
> Too many '\n's here.
>

I'll change that.
 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog


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

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

[Bryan Wu]

On Tue, May 27, 2014 at 2:08 AM, Andreas Werner <andreas.werner@men.de> wrote:
> Added driver to support the 14F021P00 BMC LEDs.
> The BMC is a Board Management Controll include four LEDs which
> can be switched on and off.
>
> This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.
>

Will you submit v2 patch based on Lee and others' review? I prefer to
review the new one.

Thanks,
-Bryan

> Signed-off-by: Andreas Werner <andreas.werner@men.de>
> ---
>  drivers/leds/Kconfig          |   6 ++
>  drivers/leds/Makefile         |   2 +-
>  drivers/leds/leds-menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 143 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/leds/leds-menf21bmc.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 1e81d7a..8f98e0f 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -466,6 +466,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 79c5155..1c465fe 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -54,7 +54,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..6099524
> --- /dev/null
> +++ b/drivers/leds/leds-menf21bmc.c
> @@ -0,0 +1,136 @@
> +/*
> + *  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
> + *  Author: Andreas Werner <andreas.werner@men.de>
> + *  All rights reserved.
> + *
> + *  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/mfd/menf21bmc.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_data {
> +       struct led_classdev cdev;
> +       struct menf21bmc *menf21bmc;
> +       u8 led_bit;
> +       const char *name;
> +};
> +
> +static struct menf21bmc_led_data 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_data *led = container_of(led_cdev,
> +                                       struct menf21bmc_led_data, cdev);
> +       struct menf21bmc *menf21bmc = led->menf21bmc;
> +
> +       mutex_lock(&led_lock);
> +       led_val = menf21bmc->read_byte_data(menf21bmc->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;
> +
> +       menf21bmc->write_byte_data(menf21bmc->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 menf21bmc *menf21bmc = dev_get_drvdata(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].menf21bmc = menf21bmc;
> +
> +               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 devicei\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");
> +MODULE_ALIAS("platform:menf21bmc_led");
> --
> 1.9.3
>

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

[Thumshirn, Johannes Tobias]

> Von: Bryan Wu [mailto:cooloney@gmail.com]
> Gesendet: Donnerstag, 26. Juni 2014 01:11
> An: Werner, Andreas
> Cc: lkml; Samuel Ortiz; Lee Jones; wim@iguana.be; linux-
> watchdog@vger.kernel.org; rpurdie@rpsys.net; Linux LED Subsystem;
> Thumshirn, Johannes Tobias
> Betreff: Re: [PATCH v2 3/3] drivers/leds/leds-menf21bmc: introduce MEN
> 14F021P00 BMC LED driver
> 
> On Tue, May 27, 2014 at 2:08 AM, Andreas Werner
> <andreas.werner@men.de> wrote:
> > Added driver to support the 14F021P00 BMC LEDs.
> > The BMC is a Board Management Controll include four LEDs which
> > can be switched on and off.
> >
> > This driver use the I2C interface to the BMC using the menf21bmc MFD Core
> driver.
> >
> 
> Will you submit v2 patch based on Lee and others' review? I prefer to
> review the new one.
> 
> Thanks,
> -Bryan

Hi,
Andreas is currently on vacation, but he'll send a v3 as soon as he's back.

Johannes

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

[Thumshirn, Johannes Tobias]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1069 bytes --]

> Von: Bryan Wu [mailto:cooloney@gmail.com]
> Gesendet: Donnerstag, 26. Juni 2014 01:11
> An: Werner, Andreas
> Cc: lkml; Samuel Ortiz; Lee Jones; wim@iguana.be; linux-
> watchdog@vger.kernel.org; rpurdie@rpsys.net; Linux LED Subsystem;
> Thumshirn, Johannes Tobias
> Betreff: Re: [PATCH v2 3/3] drivers/leds/leds-menf21bmc: introduce MEN
> 14F021P00 BMC LED driver
> 
> On Tue, May 27, 2014 at 2:08 AM, Andreas Werner
> <andreas.werner@men.de> wrote:
> > Added driver to support the 14F021P00 BMC LEDs.
> > The BMC is a Board Management Controll include four LEDs which
> > can be switched on and off.
> >
> > This driver use the I2C interface to the BMC using the menf21bmc MFD Core
> driver.
> >
> 
> Will you submit v2 patch based on Lee and others' review? I prefer to
> review the new one.
> 
> Thanks,
> -Bryan

Hi,
Andreas is currently on vacation, but he'll send a v3 as soon as he's back.

Johannes
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Thumshirn, Johannes Tobias]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1071 bytes --]

> Von: Bryan Wu [mailto:cooloney@gmail.com]
> Gesendet: Donnerstag, 26. Juni 2014 01:11
> An: Werner, Andreas
> Cc: lkml; Samuel Ortiz; Lee Jones; wim@iguana.be; linux-
> watchdog@vger.kernel.org; rpurdie@rpsys.net; Linux LED Subsystem;
> Thumshirn, Johannes Tobias
> Betreff: Re: [PATCH v2 3/3] drivers/leds/leds-menf21bmc: introduce MEN
> 14F021P00 BMC LED driver
> 
> On Tue, May 27, 2014 at 2:08 AM, Andreas Werner
> <andreas.werner@men.de> wrote:
> > Added driver to support the 14F021P00 BMC LEDs.
> > The BMC is a Board Management Controll include four LEDs which
> > can be switched on and off.
> >
> > This driver use the I2C interface to the BMC using the menf21bmc MFD Core
> driver.
> >
> 
> Will you submit v2 patch based on Lee and others' review? I prefer to
> review the new one.
> 
> Thanks,
> -Bryan

Hi,
Andreas is currently on vacation, but he'll send a v3 as soon as he's back.

Johannes
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1d
ʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\f
ÿ¶ìÿ¢¸?–I¥

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

[Andreas Werner]

On Wed, Jun 25, 2014 at 04:10:55PM -0700, Bryan Wu wrote:
> On Tue, May 27, 2014 at 2:08 AM, Andreas Werner <andreas.werner@men.de> wrote:
> > Added driver to support the 14F021P00 BMC LEDs.
> > The BMC is a Board Management Controll include four LEDs which
> > can be switched on and off.
> >
> > This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.
> >
> 
> Will you submit v2 patch based on Lee and others' review? I prefer to
> review the new one.
> 

Patch v3 is submitted so you can start review it :-)

Thanks
Andy

> Thanks,
> -Bryan
> 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/leds/Kconfig          |   6 ++
> >  drivers/leds/Makefile         |   2 +-
> >  drivers/leds/leds-menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 143 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/leds/leds-menf21bmc.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 1e81d7a..8f98e0f 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -466,6 +466,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 79c5155..1c465fe 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -54,7 +54,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..6099524
> > --- /dev/null
> > +++ b/drivers/leds/leds-menf21bmc.c
> > @@ -0,0 +1,136 @@
> > +/*
> > + *  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
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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/mfd/menf21bmc.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_data {
> > +       struct led_classdev cdev;
> > +       struct menf21bmc *menf21bmc;
> > +       u8 led_bit;
> > +       const char *name;
> > +};
> > +
> > +static struct menf21bmc_led_data 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_data *led = container_of(led_cdev,
> > +                                       struct menf21bmc_led_data, cdev);
> > +       struct menf21bmc *menf21bmc = led->menf21bmc;
> > +
> > +       mutex_lock(&led_lock);
> > +       led_val = menf21bmc->read_byte_data(menf21bmc->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;
> > +
> > +       menf21bmc->write_byte_data(menf21bmc->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 menf21bmc *menf21bmc = dev_get_drvdata(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].menf21bmc = menf21bmc;
> > +
> > +               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 devicei\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");
> > +MODULE_ALIAS("platform:menf21bmc_led");
> > --
> > 1.9.3
> >

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

[Andreas Werner]

On Wed, Jun 25, 2014 at 04:10:55PM -0700, Bryan Wu wrote:
> On Tue, May 27, 2014 at 2:08 AM, Andreas Werner <andreas.werner@men.de> wrote:
> > Added driver to support the 14F021P00 BMC LEDs.
> > The BMC is a Board Management Controll include four LEDs which
> > can be switched on and off.
> >
> > This driver use the I2C interface to the BMC using the menf21bmc MFD Core driver.
> >
> 
> Will you submit v2 patch based on Lee and others' review? I prefer to
> review the new one.
> 

Patch v3 is submitted so you can start review it :-)

Thanks
Andy

> Thanks,
> -Bryan
> 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/leds/Kconfig          |   6 ++
> >  drivers/leds/Makefile         |   2 +-
> >  drivers/leds/leds-menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 143 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/leds/leds-menf21bmc.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 1e81d7a..8f98e0f 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -466,6 +466,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 79c5155..1c465fe 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -54,7 +54,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..6099524
> > --- /dev/null
> > +++ b/drivers/leds/leds-menf21bmc.c
> > @@ -0,0 +1,136 @@
> > +/*
> > + *  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
> > + *  Author: Andreas Werner <andreas.werner@men.de>
> > + *  All rights reserved.
> > + *
> > + *  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/mfd/menf21bmc.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_data {
> > +       struct led_classdev cdev;
> > +       struct menf21bmc *menf21bmc;
> > +       u8 led_bit;
> > +       const char *name;
> > +};
> > +
> > +static struct menf21bmc_led_data 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_data *led = container_of(led_cdev,
> > +                                       struct menf21bmc_led_data, cdev);
> > +       struct menf21bmc *menf21bmc = led->menf21bmc;
> > +
> > +       mutex_lock(&led_lock);
> > +       led_val = menf21bmc->read_byte_data(menf21bmc->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;
> > +
> > +       menf21bmc->write_byte_data(menf21bmc->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 menf21bmc *menf21bmc = dev_get_drvdata(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].menf21bmc = menf21bmc;
> > +
> > +               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 devicei\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");
> > +MODULE_ALIAS("platform:menf21bmc_led");
> > --
> > 1.9.3
> >