[PATCH v3 0/3] Introduce MEN 14F021P BMC driver series

[PATCH v3 0/3] Introduce MEN 14F021P 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 v3:
	- deleted i2c_smbus wrapper functions and use native one
	- some cosmentics and variable renaming to be more clear
	- renamed "leave production" mode to "exit production mode"

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

Andreas Werner (3):
  drivers/mfd/menf21bmc: introduce MEN 14F021P00 BMC MFD Core driver
  drivers/watchdog/menf21bmc_wdt: introduce MEN 14F021P00 BMC Watchdog
    driver
  drivers/leds/leds-menf21bmc: introduce MEN 14F021P00 BMC LED driver

 drivers/leds/Kconfig             |   6 ++
 drivers/leds/Makefile            |   1 +
 drivers/leds/leds-menf21bmc.c    | 134 +++++++++++++++++++++++++
 drivers/mfd/Kconfig              |  12 +++
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/menf21bmc.c          | 136 ++++++++++++++++++++++++++
 drivers/watchdog/Kconfig         |   7 ++
 drivers/watchdog/Makefile        |   1 +
 drivers/watchdog/menf21bmc_wdt.c | 206 +++++++++++++++++++++++++++++++++++++++
 9 files changed, 504 insertions(+)
 create mode 100644 drivers/leds/leds-menf21bmc.c
 create mode 100644 drivers/mfd/menf21bmc.c
 create mode 100644 drivers/watchdog/menf21bmc_wdt.c

-- 
2.0.1

[PATCH v3 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 | 136 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 149 insertions(+)
 create mode 100644 drivers/mfd/menf21bmc.c

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

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

[Lee Jones]

On Thu, 17 Jul 2014, Andreas Werner wrote:
> The MEN 14F021P00 Board Management Controller provides an
> I2C interface to the host to access the feature implemented in the BMC.
> The BMC is a PIC Microntroller assembled on CPCI Card from MEN Mikroelektronik
> and on a few Box/Display Computer.
> 
> Added MFD Core driver, supporting the I2C communication to the device.
> 
> The MFD driver currently supports the following features:
>  	- Watchdog
>  	- LEDs
> 
> Signed-off-by: Andreas Werner <andreas.werner@men.de>
> ---
>  drivers/mfd/Kconfig     |  12 +++++
>  drivers/mfd/Makefile    |   1 +
>  drivers/mfd/menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 149 insertions(+)
>  create mode 100644 drivers/mfd/menf21bmc.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b8d9ca0..6e2cd14 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -453,6 +453,18 @@ config MFD_MAX8998
>  	  additional drivers must be enabled in order to use the functionality
>  	  of the device.
>  
> +config MFD_MENF21BMC
> +	tristate "MEN 14F021P00 Board Management Controller Support"
> +	depends on I2C=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.
> +	  The device supports multiple sub-devices like LED and WDT.
> +	  This driver provides common support for accessing the devices;
> +	  additional drivers must be enabled in order to use the
> +	  functionality of the BMC device.

Can you mention what those devices are here in the help text?

>  config EZX_PCAP
>  	bool "Motorola EZXPCAP Support"
>  	depends on SPI_MASTER
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 4e2bc25..37bf336 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
>  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
>  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
>  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> +obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> new file mode 100644
> index 0000000..76f94cd
> --- /dev/null
> +++ b/drivers/mfd/menf21bmc.c
> @@ -0,0 +1,136 @@
> +/*
> + *  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 normally resides wit the company, rather than the author.

> + *  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.
> + *

Remove this line.

> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +
> +#define BMC_CMD_REV_MAJOR	0x80
> +#define BMC_CMD_REV_MINOR	0x81
> +#define BMC_CMD_REV_MAIN	0x82
> +#define BMC_CMD_WDT_EXIT_PROD	0x18
> +#define BMC_CMD_WDT_PROD_STAT	0x19

If these are in the same address space, can you order them by number.

> +static struct mfd_cell menf21bmc_cell[] = {
> +	{ .name = "menf21bmc_wdt", },
> +	{ .name = "menf21bmc_led", },
> +};
> +
> +static int menf21bmc_wdt_exit_prod_mode(struct i2c_client *client)
> +{
> +	int val, ret;
> +
> +	val = i2c_smbus_read_byte_data(client, BMC_CMD_WDT_PROD_STAT);
> +	if (val < 0)
> +		return val;
> +
> +	/*
> +	 * Production mode should be not active after delivery of the Board.
> +	 * To be sure we check it, inform the user and exit the mode
> +	 * if active.
> +	 */
> +	if (val == 0x00) {
> +		dev_info(&client->dev,
> +			"BMC in production mode. Exit production mode\n");
> +
> +		ret = i2c_smbus_write_byte(client, BMC_CMD_WDT_EXIT_PROD);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> +{
> +	int ret;
> +	int rev_major, rev_minor, rev_main;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA |
> +				     I2C_FUNC_SMBUS_BYTE))

Can you take the function call out of the if() please?

> +		return -ENODEV;
> +
> +	rev_major = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAJOR);
> +	if (rev_major < 0) {
> +		dev_err(&client->dev, "failed to get BMC major revision\n");
> +		return rev_major;
> +	}
> +
> +	rev_minor = i2c_smbus_read_word_data(client, BMC_CMD_REV_MINOR);
> +	if (rev_minor < 0) {
> +		dev_err(&client->dev, "failed to get BMC minor revision\n");
> +		return rev_minor;
> +	}
> +
> +	rev_main = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAIN);
> +	if (rev_main < 0) {
> +		dev_err(&client->dev, "failed to get BMC main revision\n");
> +		return rev_main;
> +	}
> +
> +	dev_info(&client->dev, "FW Revision: %02d.%02d.%02d\n",
> +					rev_major, rev_minor, rev_main);
> +
> +	/*
> +	 * We have to exit the Production Mode of the BMC to activate the
> +	 * Watchdog functionality and the BIOS life sign monitoring.
> +	 */
> +	ret = menf21bmc_wdt_exit_prod_mode(client);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to leave production mode\n");
> +		return ret;
> +	}
> +
> +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to add BMC sub-devices\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int menf21bmc_remove(struct i2c_client *client)
> +{
> +	mfd_remove_devices(&client->dev);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id menf21bmc_id_table[] = {
> +	{ "menf21bmc", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> +
> +static struct i2c_driver menf21bmc_driver = {
> +	.driver		= {
> +		.name	= "menf21bmc",
> +		.owner	= THIS_MODULE,

Remove this line, it's handled elsewhere.

> +	},
> +	.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");

The header says that this is GPL v2.

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

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

[Andreas Werner]

On Thu, Jul 17, 2014 at 01:41:56PM +0100, Lee Jones wrote:
> On Thu, 17 Jul 2014, Andreas Werner wrote:
> > The MEN 14F021P00 Board Management Controller provides an
> > I2C interface to the host to access the feature implemented in the BMC.
> > The BMC is a PIC Microntroller assembled on CPCI Card from MEN Mikroelektronik
> > and on a few Box/Display Computer.
> > 
> > Added MFD Core driver, supporting the I2C communication to the device.
> > 
> > The MFD driver currently supports the following features:
> >  	- Watchdog
> >  	- LEDs
> > 
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/mfd/Kconfig     |  12 +++++
> >  drivers/mfd/Makefile    |   1 +
> >  drivers/mfd/menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 149 insertions(+)
> >  create mode 100644 drivers/mfd/menf21bmc.c
> > 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index b8d9ca0..6e2cd14 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -453,6 +453,18 @@ config MFD_MAX8998
> >  	  additional drivers must be enabled in order to use the functionality
> >  	  of the device.
> >  
> > +config MFD_MENF21BMC
> > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > +	depends on I2C=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.
> > +	  The device supports multiple sub-devices like LED and WDT.
> > +	  This driver provides common support for accessing the devices;
> > +	  additional drivers must be enabled in order to use the
> > +	  functionality of the BMC device.
> 
> Can you mention what those devices are here in the help text?
>

I've mentioned the sub-devices "LED" and "WDT" but i can also write
the name to the help text like "menf21bmc_wdt" and "leds-menf21bmc".
 
> >  config EZX_PCAP
> >  	bool "Motorola EZXPCAP Support"
> >  	depends on SPI_MASTER
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 4e2bc25..37bf336 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -169,6 +169,7 @@ obj-$(CONFIG_MFD_AS3711)	+= as3711.o
> >  obj-$(CONFIG_MFD_AS3722)	+= as3722.o
> >  obj-$(CONFIG_MFD_STW481X)	+= stw481x.o
> >  obj-$(CONFIG_MFD_IPAQ_MICRO)	+= ipaq-micro.o
> > +obj-$(CONFIG_MFD_MENF21BMC)	+= menf21bmc.o
> >  
> >  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> > diff --git a/drivers/mfd/menf21bmc.c b/drivers/mfd/menf21bmc.c
> > new file mode 100644
> > index 0000000..76f94cd
> > --- /dev/null
> > +++ b/drivers/mfd/menf21bmc.c
> > @@ -0,0 +1,136 @@
> > +/*
> > + *  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 normally resides wit the company, rather than the author.

Ok, i'wíll the author suff.

> 
> > + *  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.
> > + *
> 
> Remove this line.
> 
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mfd/core.h>
> > +
> > +#define BMC_CMD_REV_MAJOR	0x80
> > +#define BMC_CMD_REV_MINOR	0x81
> > +#define BMC_CMD_REV_MAIN	0x82
> > +#define BMC_CMD_WDT_EXIT_PROD	0x18
> > +#define BMC_CMD_WDT_PROD_STAT	0x19
> 
> If these are in the same address space, can you order them by number.

No problem, i will order them, they are in the same space.

> 
> > +static struct mfd_cell menf21bmc_cell[] = {
> > +	{ .name = "menf21bmc_wdt", },
> > +	{ .name = "menf21bmc_led", },
> > +};
> > +
> > +static int menf21bmc_wdt_exit_prod_mode(struct i2c_client *client)
> > +{
> > +	int val, ret;
> > +
> > +	val = i2c_smbus_read_byte_data(client, BMC_CMD_WDT_PROD_STAT);
> > +	if (val < 0)
> > +		return val;
> > +
> > +	/*
> > +	 * Production mode should be not active after delivery of the Board.
> > +	 * To be sure we check it, inform the user and exit the mode
> > +	 * if active.
> > +	 */
> > +	if (val == 0x00) {
> > +		dev_info(&client->dev,
> > +			"BMC in production mode. Exit production mode\n");
> > +
> > +		ret = i2c_smbus_write_byte(client, BMC_CMD_WDT_EXIT_PROD);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +menf21bmc_probe(struct i2c_client *client, const struct i2c_device_id *ids)
> > +{
> > +	int ret;
> > +	int rev_major, rev_minor, rev_main;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_BYTE_DATA |
> > +				     I2C_FUNC_SMBUS_WORD_DATA |
> > +				     I2C_FUNC_SMBUS_BYTE))
> 
> Can you take the function call out of the if() please?

Yes I can do it.

> 
> > +		return -ENODEV;
> > +
> > +	rev_major = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAJOR);
> > +	if (rev_major < 0) {
> > +		dev_err(&client->dev, "failed to get BMC major revision\n");
> > +		return rev_major;
> > +	}
> > +
> > +	rev_minor = i2c_smbus_read_word_data(client, BMC_CMD_REV_MINOR);
> > +	if (rev_minor < 0) {
> > +		dev_err(&client->dev, "failed to get BMC minor revision\n");
> > +		return rev_minor;
> > +	}
> > +
> > +	rev_main = i2c_smbus_read_word_data(client, BMC_CMD_REV_MAIN);
> > +	if (rev_main < 0) {
> > +		dev_err(&client->dev, "failed to get BMC main revision\n");
> > +		return rev_main;
> > +	}
> > +
> > +	dev_info(&client->dev, "FW Revision: %02d.%02d.%02d\n",
> > +					rev_major, rev_minor, rev_main);
> > +
> > +	/*
> > +	 * We have to exit the Production Mode of the BMC to activate the
> > +	 * Watchdog functionality and the BIOS life sign monitoring.
> > +	 */
> > +	ret = menf21bmc_wdt_exit_prod_mode(client);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to leave production mode\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = mfd_add_devices(&client->dev, 0, menf21bmc_cell,
> > +				ARRAY_SIZE(menf21bmc_cell), NULL, 0, NULL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed to add BMC sub-devices\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int menf21bmc_remove(struct i2c_client *client)
> > +{
> > +	mfd_remove_devices(&client->dev);
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id menf21bmc_id_table[] = {
> > +	{ "menf21bmc", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > +
> > +static struct i2c_driver menf21bmc_driver = {
> > +	.driver		= {
> > +		.name	= "menf21bmc",
> > +		.owner	= THIS_MODULE,
> 
> Remove this line, it's handled elsewhere.

Which line? the ".owner" line?

> 
> > +	},
> > +	.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");
> 
> The header says that this is GPL v2.

Will change that.

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

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

[Lee Jones]

On Thu, 17 Jul 2014, Andreas Werner wrote:
> On Thu, Jul 17, 2014 at 01:41:56PM +0100, Lee Jones wrote:
> > On Thu, 17 Jul 2014, Andreas Werner wrote:
> > > The MEN 14F021P00 Board Management Controller provides an
> > > I2C interface to the host to access the feature implemented in the BMC.
> > > The BMC is a PIC Microntroller assembled on CPCI Card from MEN Mikroelektronik
> > > and on a few Box/Display Computer.
> > > 
> > > Added MFD Core driver, supporting the I2C communication to the device.
> > > 
> > > The MFD driver currently supports the following features:
> > >  	- Watchdog
> > >  	- LEDs
> > > 
> > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > ---
> > >  drivers/mfd/Kconfig     |  12 +++++
> > >  drivers/mfd/Makefile    |   1 +
> > >  drivers/mfd/menf21bmc.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 149 insertions(+)
> > >  create mode 100644 drivers/mfd/menf21bmc.c
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index b8d9ca0..6e2cd14 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -453,6 +453,18 @@ config MFD_MAX8998
> > >  	  additional drivers must be enabled in order to use the functionality
> > >  	  of the device.
> > >  
> > > +config MFD_MENF21BMC
> > > +	tristate "MEN 14F021P00 Board Management Controller Support"
> > > +	depends on I2C=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.
> > > +	  The device supports multiple sub-devices like LED and WDT.
> > > +	  This driver provides common support for accessing the devices;
> > > +	  additional drivers must be enabled in order to use the
> > > +	  functionality of the BMC device.
> > 
> > Can you mention what those devices are here in the help text?
> >
> 
> I've mentioned the sub-devices "LED" and "WDT" but i can also write
> the name to the help text like "menf21bmc_wdt" and "leds-menf21bmc".

Scrap this comment, I was being silly and didn't see "LED and WDT".

[...]

> > > +MODULE_DEVICE_TABLE(i2c, menf21bmc_id_table);
> > > +
> > > +static struct i2c_driver menf21bmc_driver = {
> > > +	.driver		= {
> > > +		.name	= "menf21bmc",
> > > +		.owner	= THIS_MODULE,
> > 
> > Remove this line, it's handled elsewhere.
> 
> Which line? the ".owner" line?

Yeah.

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

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

[Andreas Werner]

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

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_wdt.c | 206 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)
 create mode 100644 drivers/watchdog/menf21bmc_wdt.c

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

Re: [PATCH v3 2/3] drivers/watchdog/menf21bmc_wdt: introduce MEN 14F021P00 BMC Watchdog driver

[Guenter Roeck]

On Thu, Jul 17, 2014 at 03:18:31PM +0200, Andreas Werner wrote:
> 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>

Only thing I dislike is continuation line alignment, where I really prefer
the 'official' style. That doesn't raise to the level of objection, though.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

Guenter

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

[PATCH v3 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         |   1 +
 drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 141 insertions(+)
 create mode 100644 drivers/leds/leds-menf21bmc.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 27cf0cd..d38ff3f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -458,6 +458,12 @@ config LEDS_OT200
 	  This option enables support for the LEDs on the Bachmann OT200.
 	  Say Y to enable LEDs on the Bachmann OT200.
 
+config LEDS_MENF21BMC
+	tristate "LED support for the MEN 14F021P00 BMC"
+	depends on LEDS_CLASS && MFD_MENF21BMC
+	help
+	  Say Y here to include support for the MEN 14F021P00 BMC LEDs.
+
 comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
 
 config LEDS_BLINKM
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3c03666..cadc433 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)		+= leds-max8997.o
 obj-$(CONFIG_LEDS_LM355x)		+= leds-lm355x.o
 obj-$(CONFIG_LEDS_BLINKM)		+= leds-blinkm.o
 obj-$(CONFIG_LEDS_VERSATILE)		+= leds-versatile.o
+obj-$(CONFIG_LEDS_MENF21BMC)		+= leds-menf21bmc.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
new file mode 100644
index 0000000..5eaa119
--- /dev/null
+++ b/drivers/leds/leds-menf21bmc.c
@@ -0,0 +1,134 @@
+/*
+ *  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/i2c.h>
+
+#define BMC_CMD_LED_GET_SET	0xA0
+#define BMC_BIT_LED_STATUS	BIT(0)
+#define BMC_BIT_LED_HOTSWAP	BIT(1)
+#define BMC_BIT_LED_USER1	BIT(2)
+#define BMC_BIT_LED_USER2	BIT(3)
+
+struct menf21bmc_led {
+	struct led_classdev cdev;
+	u8 led_bit;
+	const char *name;
+	struct i2c_client *i2c_client;
+};
+
+static struct menf21bmc_led leds[] = {
+	{
+		.name = "menf21bmc:led_status",
+		.led_bit = BMC_BIT_LED_STATUS,
+	},
+	{
+		.name = "menf21bmc:led_hotswap",
+		.led_bit = BMC_BIT_LED_HOTSWAP,
+	},
+	{
+		.name = "menf21bmc:led_user1",
+		.led_bit = BMC_BIT_LED_USER1,
+	},
+	{
+		.name = "menf21bmc:led_user2",
+		.led_bit = BMC_BIT_LED_USER2,
+	}
+};
+
+static DEFINE_MUTEX(led_lock);
+
+static void
+menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
+{
+	int led_val;
+	struct menf21bmc_led *led = container_of(led_cdev,
+					struct menf21bmc_led, cdev);
+
+	mutex_lock(&led_lock);
+	led_val = i2c_smbus_read_byte_data(led->i2c_client,
+						BMC_CMD_LED_GET_SET);
+	if (led_val < 0)
+		goto err_out;
+
+	if (value == LED_OFF)
+		led_val &= ~led->led_bit;
+	else
+		led_val |= led->led_bit;
+
+	i2c_smbus_write_byte_data(led->i2c_client,
+					BMC_CMD_LED_GET_SET, led_val);
+err_out:
+	mutex_unlock(&led_lock);
+}
+
+static int menf21bmc_led_probe(struct platform_device *pdev)
+{
+	int i;
+	int ret;
+	struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
+
+	for (i = 0; i < ARRAY_SIZE(leds); i++) {
+		leds[i].cdev.name = leds[i].name;
+		leds[i].cdev.brightness_set = menf21bmc_led_set;
+		leds[i].i2c_client = i2c_client;
+		ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
+		if (ret < 0)
+			goto err_free_leds;
+	}
+	dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
+
+	return 0;
+
+err_free_leds:
+	dev_err(&pdev->dev, "failed to register LED device\n");
+
+	for (i = i - 1; i >= 0; i--)
+		led_classdev_unregister(&leds[i].cdev);
+
+	return ret;
+}
+
+static int menf21bmc_led_remove(struct platform_device *pdev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(leds); i++)
+		led_classdev_unregister(&leds[i].cdev);
+
+	return 0;
+}
+
+static struct platform_driver menf21bmc_led = {
+	.probe		= menf21bmc_led_probe,
+	.remove		= menf21bmc_led_remove,
+	.driver		= {
+		.name		= "menf21bmc_led",
+		.owner		= THIS_MODULE,
+	},
+};
+
+module_platform_driver(menf21bmc_led);
+
+MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
+MODULE_DESCRIPTION("MEN 14F021P00 BMC led driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:menf21bmc_led");
-- 
2.0.1

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

[Bryan Wu]

On Thu, Jul 17, 2014 at 6:18 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.
>
> Signed-off-by: Andreas Werner <andreas.werner@men.de>
> ---
>  drivers/leds/Kconfig          |   6 ++
>  drivers/leds/Makefile         |   1 +
>  drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 141 insertions(+)
>  create mode 100644 drivers/leds/leds-menf21bmc.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 27cf0cd..d38ff3f 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -458,6 +458,12 @@ config LEDS_OT200
>           This option enables support for the LEDs on the Bachmann OT200.
>           Say Y to enable LEDs on the Bachmann OT200.
>
> +config LEDS_MENF21BMC
> +       tristate "LED support for the MEN 14F021P00 BMC"
> +       depends on LEDS_CLASS && MFD_MENF21BMC

I think it also depends on I2C.

> +       help
> +         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
> +
>  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>
>  config LEDS_BLINKM
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 3c03666..cadc433 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
>  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
>  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
>  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> +obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
>
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> new file mode 100644
> index 0000000..5eaa119
> --- /dev/null
> +++ b/drivers/leds/leds-menf21bmc.c
> @@ -0,0 +1,134 @@
> +/*
> + *  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/i2c.h>
> +
> +#define BMC_CMD_LED_GET_SET    0xA0
> +#define BMC_BIT_LED_STATUS     BIT(0)
> +#define BMC_BIT_LED_HOTSWAP    BIT(1)
> +#define BMC_BIT_LED_USER1      BIT(2)
> +#define BMC_BIT_LED_USER2      BIT(3)
> +
> +struct menf21bmc_led {
> +       struct led_classdev cdev;
> +       u8 led_bit;
> +       const char *name;
> +       struct i2c_client *i2c_client;
> +};
> +
> +static struct menf21bmc_led leds[] = {
> +       {
> +               .name = "menf21bmc:led_status",
> +               .led_bit = BMC_BIT_LED_STATUS,
> +       },
> +       {
> +               .name = "menf21bmc:led_hotswap",
> +               .led_bit = BMC_BIT_LED_HOTSWAP,
> +       },
> +       {
> +               .name = "menf21bmc:led_user1",
> +               .led_bit = BMC_BIT_LED_USER1,
> +       },
> +       {
> +               .name = "menf21bmc:led_user2",
> +               .led_bit = BMC_BIT_LED_USER2,
> +       }
> +};
> +
> +static DEFINE_MUTEX(led_lock);
> +
> +static void
> +menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> +{
> +       int led_val;
> +       struct menf21bmc_led *led = container_of(led_cdev,
> +                                       struct menf21bmc_led, cdev);
> +
> +       mutex_lock(&led_lock);
> +       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> +                                               BMC_CMD_LED_GET_SET);
> +       if (led_val < 0)
> +               goto err_out;
> +
> +       if (value == LED_OFF)
> +               led_val &= ~led->led_bit;
> +       else
> +               led_val |= led->led_bit;
> +
> +       i2c_smbus_write_byte_data(led->i2c_client,
> +                                       BMC_CMD_LED_GET_SET, led_val);
> +err_out:
> +       mutex_unlock(&led_lock);
> +}
> +
> +static int menf21bmc_led_probe(struct platform_device *pdev)
> +{
> +       int i;
> +       int ret;
> +       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
> +
> +       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> +               leds[i].cdev.name = leds[i].name;
> +               leds[i].cdev.brightness_set = menf21bmc_led_set;
> +               leds[i].i2c_client = i2c_client;
> +               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
> +               if (ret < 0)
> +                       goto err_free_leds;
> +       }
> +       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
> +
> +       return 0;
> +
> +err_free_leds:
> +       dev_err(&pdev->dev, "failed to register LED device\n");
> +
> +       for (i = i - 1; i >= 0; i--)
> +               led_classdev_unregister(&leds[i].cdev);
> +
> +       return ret;
> +}
> +
> +static int menf21bmc_led_remove(struct platform_device *pdev)
> +{
> +       int i;
> +
> +       for (i = 0; i < ARRAY_SIZE(leds); i++)
> +               led_classdev_unregister(&leds[i].cdev);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver menf21bmc_led = {
> +       .probe          = menf21bmc_led_probe,
> +       .remove         = menf21bmc_led_remove,
> +       .driver         = {
> +               .name           = "menf21bmc_led",
> +               .owner          = THIS_MODULE,
> +       },
> +};
> +

This is not a normal platform driver, it should be a I2C driver.

> +module_platform_driver(menf21bmc_led);

So please move to use module_i2c_driver.

Thanks,
-Bryan

> +
> +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> +MODULE_DESCRIPTION("MEN 14F021P00 BMC led driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:menf21bmc_led");
> --
> 2.0.1
>

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

[Andreas Werner]

aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
> On Thu, Jul 17, 2014 at 6:18 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.
> >
> > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > ---
> >  drivers/leds/Kconfig          |   6 ++
> >  drivers/leds/Makefile         |   1 +
> >  drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 141 insertions(+)
> >  create mode 100644 drivers/leds/leds-menf21bmc.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index 27cf0cd..d38ff3f 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -458,6 +458,12 @@ config LEDS_OT200
> >           This option enables support for the LEDs on the Bachmann OT200.
> >           Say Y to enable LEDs on the Bachmann OT200.
> >
> > +config LEDS_MENF21BMC
> > +       tristate "LED support for the MEN 14F021P00 BMC"
> > +       depends on LEDS_CLASS && MFD_MENF21BMC
> 
> I think it also depends on I2C.

Yes you are right.

> 
> > +       help
> > +         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
> > +
> >  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
> >
> >  config LEDS_BLINKM
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 3c03666..cadc433 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
> >  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
> >  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> >  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> > +obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
> >
> >  # LED SPI Drivers
> >  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> > diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> > new file mode 100644
> > index 0000000..5eaa119
> > --- /dev/null
> > +++ b/drivers/leds/leds-menf21bmc.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + *  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/i2c.h>
> > +
> > +#define BMC_CMD_LED_GET_SET    0xA0
> > +#define BMC_BIT_LED_STATUS     BIT(0)
> > +#define BMC_BIT_LED_HOTSWAP    BIT(1)
> > +#define BMC_BIT_LED_USER1      BIT(2)
> > +#define BMC_BIT_LED_USER2      BIT(3)
> > +
> > +struct menf21bmc_led {
> > +       struct led_classdev cdev;
> > +       u8 led_bit;
> > +       const char *name;
> > +       struct i2c_client *i2c_client;
> > +};
> > +
> > +static struct menf21bmc_led leds[] = {
> > +       {
> > +               .name = "menf21bmc:led_status",
> > +               .led_bit = BMC_BIT_LED_STATUS,
> > +       },
> > +       {
> > +               .name = "menf21bmc:led_hotswap",
> > +               .led_bit = BMC_BIT_LED_HOTSWAP,
> > +       },
> > +       {
> > +               .name = "menf21bmc:led_user1",
> > +               .led_bit = BMC_BIT_LED_USER1,
> > +       },
> > +       {
> > +               .name = "menf21bmc:led_user2",
> > +               .led_bit = BMC_BIT_LED_USER2,
> > +       }
> > +};
> > +
> > +static DEFINE_MUTEX(led_lock);
> > +
> > +static void
> > +menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> > +{
> > +       int led_val;
> > +       struct menf21bmc_led *led = container_of(led_cdev,
> > +                                       struct menf21bmc_led, cdev);
> > +
> > +       mutex_lock(&led_lock);
> > +       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> > +                                               BMC_CMD_LED_GET_SET);
> > +       if (led_val < 0)
> > +               goto err_out;
> > +
> > +       if (value == LED_OFF)
> > +               led_val &= ~led->led_bit;
> > +       else
> > +               led_val |= led->led_bit;
> > +
> > +       i2c_smbus_write_byte_data(led->i2c_client,
> > +                                       BMC_CMD_LED_GET_SET, led_val);
> > +err_out:
> > +       mutex_unlock(&led_lock);
> > +}
> > +
> > +static int menf21bmc_led_probe(struct platform_device *pdev)
> > +{
> > +       int i;
> > +       int ret;
> > +       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
> > +
> > +       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> > +               leds[i].cdev.name = leds[i].name;
> > +               leds[i].cdev.brightness_set = menf21bmc_led_set;
> > +               leds[i].i2c_client = i2c_client;
> > +               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
> > +               if (ret < 0)
> > +                       goto err_free_leds;
> > +       }
> > +       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
> > +
> > +       return 0;
> > +
> > +err_free_leds:
> > +       dev_err(&pdev->dev, "failed to register LED device\n");
> > +
> > +       for (i = i - 1; i >= 0; i--)
> > +               led_classdev_unregister(&leds[i].cdev);
> > +
> > +       return ret;
> > +}
> > +
> > +static int menf21bmc_led_remove(struct platform_device *pdev)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(leds); i++)
> > +               led_classdev_unregister(&leds[i].cdev);
> > +
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver menf21bmc_led = {
> > +       .probe          = menf21bmc_led_probe,
> > +       .remove         = menf21bmc_led_remove,
> > +       .driver         = {
> > +               .name           = "menf21bmc_led",
> > +               .owner          = THIS_MODULE,
> > +       },
> > +};
> > +
> 
> This is not a normal platform driver, it should be a I2C driver.
> 
> > +module_platform_driver(menf21bmc_led);
> 
> So please move to use module_i2c_driver.

Ok, but then I have to change that in the watchdog driver too, which
is quite the same as here.

Lee, Guenther what do you think regarding the watchdog driver?

> 
> Thanks,
> -Bryan
> 
> > +
> > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > +MODULE_DESCRIPTION("MEN 14F021P00 BMC led driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:menf21bmc_led");
> > --
> > 2.0.1
> >

Thanks for review.

Andy

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

[Wim Van Sebroeck]

Hi Andreas,

> aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
> > On Thu, Jul 17, 2014 at 6:18 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.
> > >
> > > Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > > ---
> > >  drivers/leds/Kconfig          |   6 ++
> > >  drivers/leds/Makefile         |   1 +
> > >  drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 141 insertions(+)
> > >  create mode 100644 drivers/leds/leds-menf21bmc.c
> > >
> > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > > index 27cf0cd..d38ff3f 100644
> > > --- a/drivers/leds/Kconfig
> > > +++ b/drivers/leds/Kconfig
> > > @@ -458,6 +458,12 @@ config LEDS_OT200
> > >           This option enables support for the LEDs on the Bachmann OT200.
> > >           Say Y to enable LEDs on the Bachmann OT200.
> > >
> > > +config LEDS_MENF21BMC
> > > +       tristate "LED support for the MEN 14F021P00 BMC"
> > > +       depends on LEDS_CLASS && MFD_MENF21BMC
> > 
> > I think it also depends on I2C.
> 
> Yes you are right.
> 
> > 
> > > +       help
> > > +         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
> > > +
> > >  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
> > >
> > >  config LEDS_BLINKM
> > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > index 3c03666..cadc433 100644
> > > --- a/drivers/leds/Makefile
> > > +++ b/drivers/leds/Makefile
> > > @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
> > >  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
> > >  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> > >  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> > > +obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
> > >
> > >  # LED SPI Drivers
> > >  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> > > diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> > > new file mode 100644
> > > index 0000000..5eaa119
> > > --- /dev/null
> > > +++ b/drivers/leds/leds-menf21bmc.c
> > > @@ -0,0 +1,134 @@
> > > +/*
> > > + *  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/i2c.h>
> > > +
> > > +#define BMC_CMD_LED_GET_SET    0xA0
> > > +#define BMC_BIT_LED_STATUS     BIT(0)
> > > +#define BMC_BIT_LED_HOTSWAP    BIT(1)
> > > +#define BMC_BIT_LED_USER1      BIT(2)
> > > +#define BMC_BIT_LED_USER2      BIT(3)
> > > +
> > > +struct menf21bmc_led {
> > > +       struct led_classdev cdev;
> > > +       u8 led_bit;
> > > +       const char *name;
> > > +       struct i2c_client *i2c_client;
> > > +};
> > > +
> > > +static struct menf21bmc_led leds[] = {
> > > +       {
> > > +               .name = "menf21bmc:led_status",
> > > +               .led_bit = BMC_BIT_LED_STATUS,
> > > +       },
> > > +       {
> > > +               .name = "menf21bmc:led_hotswap",
> > > +               .led_bit = BMC_BIT_LED_HOTSWAP,
> > > +       },
> > > +       {
> > > +               .name = "menf21bmc:led_user1",
> > > +               .led_bit = BMC_BIT_LED_USER1,
> > > +       },
> > > +       {
> > > +               .name = "menf21bmc:led_user2",
> > > +               .led_bit = BMC_BIT_LED_USER2,
> > > +       }
> > > +};
> > > +
> > > +static DEFINE_MUTEX(led_lock);
> > > +
> > > +static void
> > > +menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> > > +{
> > > +       int led_val;
> > > +       struct menf21bmc_led *led = container_of(led_cdev,
> > > +                                       struct menf21bmc_led, cdev);
> > > +
> > > +       mutex_lock(&led_lock);
> > > +       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> > > +                                               BMC_CMD_LED_GET_SET);
> > > +       if (led_val < 0)
> > > +               goto err_out;
> > > +
> > > +       if (value == LED_OFF)
> > > +               led_val &= ~led->led_bit;
> > > +       else
> > > +               led_val |= led->led_bit;
> > > +
> > > +       i2c_smbus_write_byte_data(led->i2c_client,
> > > +                                       BMC_CMD_LED_GET_SET, led_val);
> > > +err_out:
> > > +       mutex_unlock(&led_lock);
> > > +}
> > > +
> > > +static int menf21bmc_led_probe(struct platform_device *pdev)
> > > +{
> > > +       int i;
> > > +       int ret;
> > > +       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> > > +               leds[i].cdev.name = leds[i].name;
> > > +               leds[i].cdev.brightness_set = menf21bmc_led_set;
> > > +               leds[i].i2c_client = i2c_client;
> > > +               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
> > > +               if (ret < 0)
> > > +                       goto err_free_leds;
> > > +       }
> > > +       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
> > > +
> > > +       return 0;
> > > +
> > > +err_free_leds:
> > > +       dev_err(&pdev->dev, "failed to register LED device\n");
> > > +
> > > +       for (i = i - 1; i >= 0; i--)
> > > +               led_classdev_unregister(&leds[i].cdev);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > > +static int menf21bmc_led_remove(struct platform_device *pdev)
> > > +{
> > > +       int i;
> > > +
> > > +       for (i = 0; i < ARRAY_SIZE(leds); i++)
> > > +               led_classdev_unregister(&leds[i].cdev);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static struct platform_driver menf21bmc_led = {
> > > +       .probe          = menf21bmc_led_probe,
> > > +       .remove         = menf21bmc_led_remove,
> > > +       .driver         = {
> > > +               .name           = "menf21bmc_led",
> > > +               .owner          = THIS_MODULE,
> > > +       },
> > > +};
> > > +
> > 
> > This is not a normal platform driver, it should be a I2C driver.
> > 
> > > +module_platform_driver(menf21bmc_led);
> > 
> > So please move to use module_i2c_driver.
> 
> Ok, but then I have to change that in the watchdog driver too, which
> is quite the same as here.
> 
> Lee, Guenther what do you think regarding the watchdog driver?

If the watchdog part is indeed also i2c alike device then the watchdog part should also be an i2c driver...

> > 
> > Thanks,
> > -Bryan
> > 
> > > +
> > > +MODULE_AUTHOR("Andreas Werner <andreas.werner@men.de>");
> > > +MODULE_DESCRIPTION("MEN 14F021P00 BMC led driver");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:menf21bmc_led");
> > > --
> > > 2.0.1
> > >
> 
> Thanks for review.
> 
> Andy

Kind regards,
Wim.

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

[Guenter Roeck]

On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
> Hi Andreas,
>
>> aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
>>> On Thu, Jul 17, 2014 at 6:18 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.
>>>>
>>>> Signed-off-by: Andreas Werner <andreas.werner@men.de>
>>>> ---
>>>>   drivers/leds/Kconfig          |   6 ++
>>>>   drivers/leds/Makefile         |   1 +
>>>>   drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 141 insertions(+)
>>>>   create mode 100644 drivers/leds/leds-menf21bmc.c
>>>>
>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>> index 27cf0cd..d38ff3f 100644
>>>> --- a/drivers/leds/Kconfig
>>>> +++ b/drivers/leds/Kconfig
>>>> @@ -458,6 +458,12 @@ config LEDS_OT200
>>>>            This option enables support for the LEDs on the Bachmann OT200.
>>>>            Say Y to enable LEDs on the Bachmann OT200.
>>>>
>>>> +config LEDS_MENF21BMC
>>>> +       tristate "LED support for the MEN 14F021P00 BMC"
>>>> +       depends on LEDS_CLASS && MFD_MENF21BMC
>>>
>>> I think it also depends on I2C.
>>
>> Yes you are right.
>>
>>>
>>>> +       help
>>>> +         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
>>>> +
>>>>   comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>>>>
>>>>   config LEDS_BLINKM
>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>> index 3c03666..cadc433 100644
>>>> --- a/drivers/leds/Makefile
>>>> +++ b/drivers/leds/Makefile
>>>> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
>>>>   obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
>>>>   obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
>>>>   obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
>>>> +obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
>>>>
>>>>   # LED SPI Drivers
>>>>   obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
>>>> diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
>>>> new file mode 100644
>>>> index 0000000..5eaa119
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-menf21bmc.c
>>>> @@ -0,0 +1,134 @@
>>>> +/*
>>>> + *  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/i2c.h>
>>>> +
>>>> +#define BMC_CMD_LED_GET_SET    0xA0
>>>> +#define BMC_BIT_LED_STATUS     BIT(0)
>>>> +#define BMC_BIT_LED_HOTSWAP    BIT(1)
>>>> +#define BMC_BIT_LED_USER1      BIT(2)
>>>> +#define BMC_BIT_LED_USER2      BIT(3)
>>>> +
>>>> +struct menf21bmc_led {
>>>> +       struct led_classdev cdev;
>>>> +       u8 led_bit;
>>>> +       const char *name;
>>>> +       struct i2c_client *i2c_client;
>>>> +};
>>>> +
>>>> +static struct menf21bmc_led leds[] = {
>>>> +       {
>>>> +               .name = "menf21bmc:led_status",
>>>> +               .led_bit = BMC_BIT_LED_STATUS,
>>>> +       },
>>>> +       {
>>>> +               .name = "menf21bmc:led_hotswap",
>>>> +               .led_bit = BMC_BIT_LED_HOTSWAP,
>>>> +       },
>>>> +       {
>>>> +               .name = "menf21bmc:led_user1",
>>>> +               .led_bit = BMC_BIT_LED_USER1,
>>>> +       },
>>>> +       {
>>>> +               .name = "menf21bmc:led_user2",
>>>> +               .led_bit = BMC_BIT_LED_USER2,
>>>> +       }
>>>> +};
>>>> +
>>>> +static DEFINE_MUTEX(led_lock);
>>>> +
>>>> +static void
>>>> +menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>>> +{
>>>> +       int led_val;
>>>> +       struct menf21bmc_led *led = container_of(led_cdev,
>>>> +                                       struct menf21bmc_led, cdev);
>>>> +
>>>> +       mutex_lock(&led_lock);
>>>> +       led_val = i2c_smbus_read_byte_data(led->i2c_client,
>>>> +                                               BMC_CMD_LED_GET_SET);
>>>> +       if (led_val < 0)
>>>> +               goto err_out;
>>>> +
>>>> +       if (value == LED_OFF)
>>>> +               led_val &= ~led->led_bit;
>>>> +       else
>>>> +               led_val |= led->led_bit;
>>>> +
>>>> +       i2c_smbus_write_byte_data(led->i2c_client,
>>>> +                                       BMC_CMD_LED_GET_SET, led_val);
>>>> +err_out:
>>>> +       mutex_unlock(&led_lock);
>>>> +}
>>>> +
>>>> +static int menf21bmc_led_probe(struct platform_device *pdev)
>>>> +{
>>>> +       int i;
>>>> +       int ret;
>>>> +       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
>>>> +
>>>> +       for (i = 0; i < ARRAY_SIZE(leds); i++) {
>>>> +               leds[i].cdev.name = leds[i].name;
>>>> +               leds[i].cdev.brightness_set = menf21bmc_led_set;
>>>> +               leds[i].i2c_client = i2c_client;
>>>> +               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
>>>> +               if (ret < 0)
>>>> +                       goto err_free_leds;
>>>> +       }
>>>> +       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
>>>> +
>>>> +       return 0;
>>>> +
>>>> +err_free_leds:
>>>> +       dev_err(&pdev->dev, "failed to register LED device\n");
>>>> +
>>>> +       for (i = i - 1; i >= 0; i--)
>>>> +               led_classdev_unregister(&leds[i].cdev);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>> +static int menf21bmc_led_remove(struct platform_device *pdev)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; i < ARRAY_SIZE(leds); i++)
>>>> +               led_classdev_unregister(&leds[i].cdev);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver menf21bmc_led = {
>>>> +       .probe          = menf21bmc_led_probe,
>>>> +       .remove         = menf21bmc_led_remove,
>>>> +       .driver         = {
>>>> +               .name           = "menf21bmc_led",
>>>> +               .owner          = THIS_MODULE,
>>>> +       },
>>>> +};
>>>> +
>>>
>>> This is not a normal platform driver, it should be a I2C driver.
>>>
>>>> +module_platform_driver(menf21bmc_led);
>>>
>>> So please move to use module_i2c_driver.
>>
>> Ok, but then I have to change that in the watchdog driver too, which
>> is quite the same as here.
>>
>> Lee, Guenther what do you think regarding the watchdog driver?
>
> If the watchdog part is indeed also i2c alike device then the watchdog part should also be an i2c driver...
>

Isn't this all the same chip ? If so, there should be only one i2c driver.
Unless the i2c access is too complicated, it might make sense to use regmap
to access the chip, and pass the regmap pointer to the mfd slave drivers
to handle the actual communication with the chip.

Guenter

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

[Andreas Werner]

On Tue, Jul 29, 2014 at 02:47:08PM -0700, Guenter Roeck wrote:
> On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
> >Hi Andreas,
> >
> >>aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
> >>>On Thu, Jul 17, 2014 at 6:18 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.
> >>>>
> >>>>Signed-off-by: Andreas Werner <andreas.werner@men.de>
> >>>>---
> >>>>  drivers/leds/Kconfig          |   6 ++
> >>>>  drivers/leds/Makefile         |   1 +
> >>>>  drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 141 insertions(+)
> >>>>  create mode 100644 drivers/leds/leds-menf21bmc.c
> >>>>
> >>>>diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >>>>index 27cf0cd..d38ff3f 100644
> >>>>--- a/drivers/leds/Kconfig
> >>>>+++ b/drivers/leds/Kconfig
> >>>>@@ -458,6 +458,12 @@ config LEDS_OT200
> >>>>           This option enables support for the LEDs on the Bachmann OT200.
> >>>>           Say Y to enable LEDs on the Bachmann OT200.
> >>>>
> >>>>+config LEDS_MENF21BMC
> >>>>+       tristate "LED support for the MEN 14F021P00 BMC"
> >>>>+       depends on LEDS_CLASS && MFD_MENF21BMC
> >>>
> >>>I think it also depends on I2C.
> >>
> >>Yes you are right.
> >>
> >>>
> >>>>+       help
> >>>>+         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
> >>>>+
> >>>>  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
> >>>>
> >>>>  config LEDS_BLINKM
> >>>>diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> >>>>index 3c03666..cadc433 100644
> >>>>--- a/drivers/leds/Makefile
> >>>>+++ b/drivers/leds/Makefile
> >>>>@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
> >>>>  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
> >>>>  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> >>>>  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> >>>>+obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
> >>>>
> >>>>  # LED SPI Drivers
> >>>>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> >>>>diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> >>>>new file mode 100644
> >>>>index 0000000..5eaa119
> >>>>--- /dev/null
> >>>>+++ b/drivers/leds/leds-menf21bmc.c
> >>>>@@ -0,0 +1,134 @@
> >>>>+/*
> >>>>+ *  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/i2c.h>
> >>>>+
> >>>>+#define BMC_CMD_LED_GET_SET    0xA0
> >>>>+#define BMC_BIT_LED_STATUS     BIT(0)
> >>>>+#define BMC_BIT_LED_HOTSWAP    BIT(1)
> >>>>+#define BMC_BIT_LED_USER1      BIT(2)
> >>>>+#define BMC_BIT_LED_USER2      BIT(3)
> >>>>+
> >>>>+struct menf21bmc_led {
> >>>>+       struct led_classdev cdev;
> >>>>+       u8 led_bit;
> >>>>+       const char *name;
> >>>>+       struct i2c_client *i2c_client;
> >>>>+};
> >>>>+
> >>>>+static struct menf21bmc_led leds[] = {
> >>>>+       {
> >>>>+               .name = "menf21bmc:led_status",
> >>>>+               .led_bit = BMC_BIT_LED_STATUS,
> >>>>+       },
> >>>>+       {
> >>>>+               .name = "menf21bmc:led_hotswap",
> >>>>+               .led_bit = BMC_BIT_LED_HOTSWAP,
> >>>>+       },
> >>>>+       {
> >>>>+               .name = "menf21bmc:led_user1",
> >>>>+               .led_bit = BMC_BIT_LED_USER1,
> >>>>+       },
> >>>>+       {
> >>>>+               .name = "menf21bmc:led_user2",
> >>>>+               .led_bit = BMC_BIT_LED_USER2,
> >>>>+       }
> >>>>+};
> >>>>+
> >>>>+static DEFINE_MUTEX(led_lock);
> >>>>+
> >>>>+static void
> >>>>+menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> >>>>+{
> >>>>+       int led_val;
> >>>>+       struct menf21bmc_led *led = container_of(led_cdev,
> >>>>+                                       struct menf21bmc_led, cdev);
> >>>>+
> >>>>+       mutex_lock(&led_lock);
> >>>>+       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> >>>>+                                               BMC_CMD_LED_GET_SET);
> >>>>+       if (led_val < 0)
> >>>>+               goto err_out;
> >>>>+
> >>>>+       if (value == LED_OFF)
> >>>>+               led_val &= ~led->led_bit;
> >>>>+       else
> >>>>+               led_val |= led->led_bit;
> >>>>+
> >>>>+       i2c_smbus_write_byte_data(led->i2c_client,
> >>>>+                                       BMC_CMD_LED_GET_SET, led_val);
> >>>>+err_out:
> >>>>+       mutex_unlock(&led_lock);
> >>>>+}
> >>>>+
> >>>>+static int menf21bmc_led_probe(struct platform_device *pdev)
> >>>>+{
> >>>>+       int i;
> >>>>+       int ret;
> >>>>+       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
> >>>>+
> >>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> >>>>+               leds[i].cdev.name = leds[i].name;
> >>>>+               leds[i].cdev.brightness_set = menf21bmc_led_set;
> >>>>+               leds[i].i2c_client = i2c_client;
> >>>>+               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
> >>>>+               if (ret < 0)
> >>>>+                       goto err_free_leds;
> >>>>+       }
> >>>>+       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
> >>>>+
> >>>>+       return 0;
> >>>>+
> >>>>+err_free_leds:
> >>>>+       dev_err(&pdev->dev, "failed to register LED device\n");
> >>>>+
> >>>>+       for (i = i - 1; i >= 0; i--)
> >>>>+               led_classdev_unregister(&leds[i].cdev);
> >>>>+
> >>>>+       return ret;
> >>>>+}
> >>>>+
> >>>>+static int menf21bmc_led_remove(struct platform_device *pdev)
> >>>>+{
> >>>>+       int i;
> >>>>+
> >>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++)
> >>>>+               led_classdev_unregister(&leds[i].cdev);
> >>>>+
> >>>>+       return 0;
> >>>>+}
> >>>>+
> >>>>+static struct platform_driver menf21bmc_led = {
> >>>>+       .probe          = menf21bmc_led_probe,
> >>>>+       .remove         = menf21bmc_led_remove,
> >>>>+       .driver         = {
> >>>>+               .name           = "menf21bmc_led",
> >>>>+               .owner          = THIS_MODULE,
> >>>>+       },
> >>>>+};
> >>>>+
> >>>
> >>>This is not a normal platform driver, it should be a I2C driver.
> >>>
> >>>>+module_platform_driver(menf21bmc_led);
> >>>
> >>>So please move to use module_i2c_driver.
> >>
> >>Ok, but then I have to change that in the watchdog driver too, which
> >>is quite the same as here.
> >>
> >>Lee, Guenther what do you think regarding the watchdog driver?
> >
> >If the watchdog part is indeed also i2c alike device then the watchdog part should also be an i2c driver...
> >
> 
> Isn't this all the same chip ? If so, there should be only one i2c driver.
> Unless the i2c access is too complicated, it might make sense to use regmap
> to access the chip, and pass the regmap pointer to the mfd slave drivers
> to handle the actual communication with the chip.
> 
> Guenter
> 

Yes this is all the same chip which has different features like LEDs and Watchdog.
This is why the mfd is the i2c_driver which instantiates the slave devices which 
are platform drivers.

Is that not the normal way to handle those device? 
Is it really necessary to handle this to regmap?

Regards
Andy

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

[Guenter Roeck]

On 07/30/2014 01:08 AM, Andreas Werner wrote:
> On Tue, Jul 29, 2014 at 02:47:08PM -0700, Guenter Roeck wrote:
>> On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
>>> Hi Andreas,
>>>
>>>> aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
>>>>> On Thu, Jul 17, 2014 at 6:18 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.
>>>>>>
>>>>>> Signed-off-by: Andreas Werner <andreas.werner@men.de>
>>>>>> ---
>>>>>>   drivers/leds/Kconfig          |   6 ++
>>>>>>   drivers/leds/Makefile         |   1 +
>>>>>>   drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>   3 files changed, 141 insertions(+)
>>>>>>   create mode 100644 drivers/leds/leds-menf21bmc.c
>>>>>>
>>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>>> index 27cf0cd..d38ff3f 100644
>>>>>> --- a/drivers/leds/Kconfig
>>>>>> +++ b/drivers/leds/Kconfig
>>>>>> @@ -458,6 +458,12 @@ config LEDS_OT200
>>>>>>            This option enables support for the LEDs on the Bachmann OT200.
>>>>>>            Say Y to enable LEDs on the Bachmann OT200.
>>>>>>
>>>>>> +config LEDS_MENF21BMC
>>>>>> +       tristate "LED support for the MEN 14F021P00 BMC"
>>>>>> +       depends on LEDS_CLASS && MFD_MENF21BMC
>>>>>
>>>>> I think it also depends on I2C.
>>>>
>>>> Yes you are right.
>>>>
>>>>>
>>>>>> +       help
>>>>>> +         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
>>>>>> +
>>>>>>   comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>>>>>>
>>>>>>   config LEDS_BLINKM
>>>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>>>> index 3c03666..cadc433 100644
>>>>>> --- a/drivers/leds/Makefile
>>>>>> +++ b/drivers/leds/Makefile
>>>>>> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
>>>>>>   obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
>>>>>>   obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
>>>>>>   obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
>>>>>> +obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
>>>>>>
>>>>>>   # LED SPI Drivers
>>>>>>   obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
>>>>>> diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
>>>>>> new file mode 100644
>>>>>> index 0000000..5eaa119
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/leds/leds-menf21bmc.c
>>>>>> @@ -0,0 +1,134 @@
>>>>>> +/*
>>>>>> + *  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/i2c.h>
>>>>>> +
>>>>>> +#define BMC_CMD_LED_GET_SET    0xA0
>>>>>> +#define BMC_BIT_LED_STATUS     BIT(0)
>>>>>> +#define BMC_BIT_LED_HOTSWAP    BIT(1)
>>>>>> +#define BMC_BIT_LED_USER1      BIT(2)
>>>>>> +#define BMC_BIT_LED_USER2      BIT(3)
>>>>>> +
>>>>>> +struct menf21bmc_led {
>>>>>> +       struct led_classdev cdev;
>>>>>> +       u8 led_bit;
>>>>>> +       const char *name;
>>>>>> +       struct i2c_client *i2c_client;
>>>>>> +};
>>>>>> +
>>>>>> +static struct menf21bmc_led leds[] = {
>>>>>> +       {
>>>>>> +               .name = "menf21bmc:led_status",
>>>>>> +               .led_bit = BMC_BIT_LED_STATUS,
>>>>>> +       },
>>>>>> +       {
>>>>>> +               .name = "menf21bmc:led_hotswap",
>>>>>> +               .led_bit = BMC_BIT_LED_HOTSWAP,
>>>>>> +       },
>>>>>> +       {
>>>>>> +               .name = "menf21bmc:led_user1",
>>>>>> +               .led_bit = BMC_BIT_LED_USER1,
>>>>>> +       },
>>>>>> +       {
>>>>>> +               .name = "menf21bmc:led_user2",
>>>>>> +               .led_bit = BMC_BIT_LED_USER2,
>>>>>> +       }
>>>>>> +};
>>>>>> +
>>>>>> +static DEFINE_MUTEX(led_lock);
>>>>>> +
>>>>>> +static void
>>>>>> +menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>>>>> +{
>>>>>> +       int led_val;
>>>>>> +       struct menf21bmc_led *led = container_of(led_cdev,
>>>>>> +                                       struct menf21bmc_led, cdev);
>>>>>> +
>>>>>> +       mutex_lock(&led_lock);
>>>>>> +       led_val = i2c_smbus_read_byte_data(led->i2c_client,
>>>>>> +                                               BMC_CMD_LED_GET_SET);
>>>>>> +       if (led_val < 0)
>>>>>> +               goto err_out;
>>>>>> +
>>>>>> +       if (value == LED_OFF)
>>>>>> +               led_val &= ~led->led_bit;
>>>>>> +       else
>>>>>> +               led_val |= led->led_bit;
>>>>>> +
>>>>>> +       i2c_smbus_write_byte_data(led->i2c_client,
>>>>>> +                                       BMC_CMD_LED_GET_SET, led_val);
>>>>>> +err_out:
>>>>>> +       mutex_unlock(&led_lock);
>>>>>> +}
>>>>>> +
>>>>>> +static int menf21bmc_led_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +       int i;
>>>>>> +       int ret;
>>>>>> +       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
>>>>>> +
>>>>>> +       for (i = 0; i < ARRAY_SIZE(leds); i++) {
>>>>>> +               leds[i].cdev.name = leds[i].name;
>>>>>> +               leds[i].cdev.brightness_set = menf21bmc_led_set;
>>>>>> +               leds[i].i2c_client = i2c_client;
>>>>>> +               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
>>>>>> +               if (ret < 0)
>>>>>> +                       goto err_free_leds;
>>>>>> +       }
>>>>>> +       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
>>>>>> +
>>>>>> +       return 0;
>>>>>> +
>>>>>> +err_free_leds:
>>>>>> +       dev_err(&pdev->dev, "failed to register LED device\n");
>>>>>> +
>>>>>> +       for (i = i - 1; i >= 0; i--)
>>>>>> +               led_classdev_unregister(&leds[i].cdev);
>>>>>> +
>>>>>> +       return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int menf21bmc_led_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> +       int i;
>>>>>> +
>>>>>> +       for (i = 0; i < ARRAY_SIZE(leds); i++)
>>>>>> +               led_classdev_unregister(&leds[i].cdev);
>>>>>> +
>>>>>> +       return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static struct platform_driver menf21bmc_led = {
>>>>>> +       .probe          = menf21bmc_led_probe,
>>>>>> +       .remove         = menf21bmc_led_remove,
>>>>>> +       .driver         = {
>>>>>> +               .name           = "menf21bmc_led",
>>>>>> +               .owner          = THIS_MODULE,
>>>>>> +       },
>>>>>> +};
>>>>>> +
>>>>>
>>>>> This is not a normal platform driver, it should be a I2C driver.
>>>>>
>>>>>> +module_platform_driver(menf21bmc_led);
>>>>>
>>>>> So please move to use module_i2c_driver.
>>>>
>>>> Ok, but then I have to change that in the watchdog driver too, which
>>>> is quite the same as here.
>>>>
>>>> Lee, Guenther what do you think regarding the watchdog driver?
>>>
>>> If the watchdog part is indeed also i2c alike device then the watchdog part should also be an i2c driver...
>>>
>>
>> Isn't this all the same chip ? If so, there should be only one i2c driver.
>> Unless the i2c access is too complicated, it might make sense to use regmap
>> to access the chip, and pass the regmap pointer to the mfd slave drivers
>> to handle the actual communication with the chip.
>>
>> Guenter
>>
>
> Yes this is all the same chip which has different features like LEDs and Watchdog.
> This is why the mfd is the i2c_driver which instantiates the slave devices which
> are platform drivers.
>
> Is that not the normal way to handle those device?
> Is it really necessary to handle this to regmap?
>

Not really sure if 'necessary' is the right term, it is just convenient.
Regmap takes care of register caching, for example, so you would not have to
re-read register values again and again from the chip just to modify a bit
of a register.
You can pass the regmap pointer in the platform data, or you can pass the
pointer to the i2c client. Using regmap makes the client driver independent
of the i2c subsystem. The one thing you can _not_ do is to declare the client
drivers to be i2c drivers.

Guenter

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

[Andreas Werner]

On Wed, Jul 30, 2014 at 06:57:46AM -0700, Guenter Roeck wrote:
> On 07/30/2014 01:08 AM, Andreas Werner wrote:
> >On Tue, Jul 29, 2014 at 02:47:08PM -0700, Guenter Roeck wrote:
> >>On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
> >>>Hi Andreas,
> >>>
> >>>>aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
> >>>>>On Thu, Jul 17, 2014 at 6:18 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.
> >>>>>>
> >>>>>>Signed-off-by: Andreas Werner <andreas.werner@men.de>
> >>>>>>---
> >>>>>>  drivers/leds/Kconfig          |   6 ++
> >>>>>>  drivers/leds/Makefile         |   1 +
> >>>>>>  drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>  3 files changed, 141 insertions(+)
> >>>>>>  create mode 100644 drivers/leds/leds-menf21bmc.c
> >>>>>>
> >>>>>>diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >>>>>>index 27cf0cd..d38ff3f 100644
> >>>>>>--- a/drivers/leds/Kconfig
> >>>>>>+++ b/drivers/leds/Kconfig
> >>>>>>@@ -458,6 +458,12 @@ config LEDS_OT200
> >>>>>>           This option enables support for the LEDs on the Bachmann OT200.
> >>>>>>           Say Y to enable LEDs on the Bachmann OT200.
> >>>>>>
> >>>>>>+config LEDS_MENF21BMC
> >>>>>>+       tristate "LED support for the MEN 14F021P00 BMC"
> >>>>>>+       depends on LEDS_CLASS && MFD_MENF21BMC
> >>>>>
> >>>>>I think it also depends on I2C.
> >>>>
> >>>>Yes you are right.
> >>>>
> >>>>>
> >>>>>>+       help
> >>>>>>+         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
> >>>>>>+
> >>>>>>  comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
> >>>>>>
> >>>>>>  config LEDS_BLINKM
> >>>>>>diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> >>>>>>index 3c03666..cadc433 100644
> >>>>>>--- a/drivers/leds/Makefile
> >>>>>>+++ b/drivers/leds/Makefile
> >>>>>>@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
> >>>>>>  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
> >>>>>>  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> >>>>>>  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> >>>>>>+obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
> >>>>>>
> >>>>>>  # LED SPI Drivers
> >>>>>>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> >>>>>>diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
> >>>>>>new file mode 100644
> >>>>>>index 0000000..5eaa119
> >>>>>>--- /dev/null
> >>>>>>+++ b/drivers/leds/leds-menf21bmc.c
> >>>>>>@@ -0,0 +1,134 @@
> >>>>>>+/*
> >>>>>>+ *  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/i2c.h>
> >>>>>>+
> >>>>>>+#define BMC_CMD_LED_GET_SET    0xA0
> >>>>>>+#define BMC_BIT_LED_STATUS     BIT(0)
> >>>>>>+#define BMC_BIT_LED_HOTSWAP    BIT(1)
> >>>>>>+#define BMC_BIT_LED_USER1      BIT(2)
> >>>>>>+#define BMC_BIT_LED_USER2      BIT(3)
> >>>>>>+
> >>>>>>+struct menf21bmc_led {
> >>>>>>+       struct led_classdev cdev;
> >>>>>>+       u8 led_bit;
> >>>>>>+       const char *name;
> >>>>>>+       struct i2c_client *i2c_client;
> >>>>>>+};
> >>>>>>+
> >>>>>>+static struct menf21bmc_led leds[] = {
> >>>>>>+       {
> >>>>>>+               .name = "menf21bmc:led_status",
> >>>>>>+               .led_bit = BMC_BIT_LED_STATUS,
> >>>>>>+       },
> >>>>>>+       {
> >>>>>>+               .name = "menf21bmc:led_hotswap",
> >>>>>>+               .led_bit = BMC_BIT_LED_HOTSWAP,
> >>>>>>+       },
> >>>>>>+       {
> >>>>>>+               .name = "menf21bmc:led_user1",
> >>>>>>+               .led_bit = BMC_BIT_LED_USER1,
> >>>>>>+       },
> >>>>>>+       {
> >>>>>>+               .name = "menf21bmc:led_user2",
> >>>>>>+               .led_bit = BMC_BIT_LED_USER2,
> >>>>>>+       }
> >>>>>>+};
> >>>>>>+
> >>>>>>+static DEFINE_MUTEX(led_lock);
> >>>>>>+
> >>>>>>+static void
> >>>>>>+menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
> >>>>>>+{
> >>>>>>+       int led_val;
> >>>>>>+       struct menf21bmc_led *led = container_of(led_cdev,
> >>>>>>+                                       struct menf21bmc_led, cdev);
> >>>>>>+
> >>>>>>+       mutex_lock(&led_lock);
> >>>>>>+       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> >>>>>>+                                               BMC_CMD_LED_GET_SET);
> >>>>>>+       if (led_val < 0)
> >>>>>>+               goto err_out;
> >>>>>>+
> >>>>>>+       if (value == LED_OFF)
> >>>>>>+               led_val &= ~led->led_bit;
> >>>>>>+       else
> >>>>>>+               led_val |= led->led_bit;
> >>>>>>+
> >>>>>>+       i2c_smbus_write_byte_data(led->i2c_client,
> >>>>>>+                                       BMC_CMD_LED_GET_SET, led_val);
> >>>>>>+err_out:
> >>>>>>+       mutex_unlock(&led_lock);
> >>>>>>+}
> >>>>>>+
> >>>>>>+static int menf21bmc_led_probe(struct platform_device *pdev)
> >>>>>>+{
> >>>>>>+       int i;
> >>>>>>+       int ret;
> >>>>>>+       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
> >>>>>>+
> >>>>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> >>>>>>+               leds[i].cdev.name = leds[i].name;
> >>>>>>+               leds[i].cdev.brightness_set = menf21bmc_led_set;
> >>>>>>+               leds[i].i2c_client = i2c_client;
> >>>>>>+               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
> >>>>>>+               if (ret < 0)
> >>>>>>+                       goto err_free_leds;
> >>>>>>+       }
> >>>>>>+       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
> >>>>>>+
> >>>>>>+       return 0;
> >>>>>>+
> >>>>>>+err_free_leds:
> >>>>>>+       dev_err(&pdev->dev, "failed to register LED device\n");
> >>>>>>+
> >>>>>>+       for (i = i - 1; i >= 0; i--)
> >>>>>>+               led_classdev_unregister(&leds[i].cdev);
> >>>>>>+
> >>>>>>+       return ret;
> >>>>>>+}
> >>>>>>+
> >>>>>>+static int menf21bmc_led_remove(struct platform_device *pdev)
> >>>>>>+{
> >>>>>>+       int i;
> >>>>>>+
> >>>>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++)
> >>>>>>+               led_classdev_unregister(&leds[i].cdev);
> >>>>>>+
> >>>>>>+       return 0;
> >>>>>>+}
> >>>>>>+
> >>>>>>+static struct platform_driver menf21bmc_led = {
> >>>>>>+       .probe          = menf21bmc_led_probe,
> >>>>>>+       .remove         = menf21bmc_led_remove,
> >>>>>>+       .driver         = {
> >>>>>>+               .name           = "menf21bmc_led",
> >>>>>>+               .owner          = THIS_MODULE,
> >>>>>>+       },
> >>>>>>+};
> >>>>>>+
> >>>>>
> >>>>>This is not a normal platform driver, it should be a I2C driver.
> >>>>>
> >>>>>>+module_platform_driver(menf21bmc_led);
> >>>>>
> >>>>>So please move to use module_i2c_driver.
> >>>>
> >>>>Ok, but then I have to change that in the watchdog driver too, which
> >>>>is quite the same as here.
> >>>>
> >>>>Lee, Guenther what do you think regarding the watchdog driver?
> >>>
> >>>If the watchdog part is indeed also i2c alike device then the watchdog part should also be an i2c driver...
> >>>
> >>
> >>Isn't this all the same chip ? If so, there should be only one i2c driver.
> >>Unless the i2c access is too complicated, it might make sense to use regmap
> >>to access the chip, and pass the regmap pointer to the mfd slave drivers
> >>to handle the actual communication with the chip.
> >>
> >>Guenter
> >>
> >
> >Yes this is all the same chip which has different features like LEDs and Watchdog.
> >This is why the mfd is the i2c_driver which instantiates the slave devices which
> >are platform drivers.
> >
> >Is that not the normal way to handle those device?
> >Is it really necessary to handle this to regmap?
> >
> 
> Not really sure if 'necessary' is the right term, it is just convenient.
> Regmap takes care of register caching, for example, so you would not have to
> re-read register values again and again from the chip just to modify a bit
> of a register.
> You can pass the regmap pointer in the platform data, or you can pass the
> pointer to the i2c client. Using regmap makes the client driver independent
> of the i2c subsystem. The one thing you can _not_ do is to declare the client
> drivers to be i2c drivers.
> 
> Guenter
> 
I've checked out the regmap and it really sounds good.
Before I will made those changes which is almost a complete replacement
of the interface, i would like to know exactly why the slaves are no platform 
devices.

For now I think the implementation of the menf21bmc using an mfd as the i2c_driver
and the slaves as platform devices is correct.
Regmap would be just another type of interfaces which I could use.

Please correct me if I'm'wrong.

Regards
Andy

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

[Guenter Roeck]

On 08/08/2014 12:49 AM, Andreas Werner wrote:
> On Wed, Jul 30, 2014 at 06:57:46AM -0700, Guenter Roeck wrote:
>> On 07/30/2014 01:08 AM, Andreas Werner wrote:
>>> On Tue, Jul 29, 2014 at 02:47:08PM -0700, Guenter Roeck wrote:
>>>> On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
>>>>> Hi Andreas,
>>>>>
>>>>>> aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
>>>>>>> On Thu, Jul 17, 2014 at 6:18 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.
>>>>>>>>
>>>>>>>> Signed-off-by: Andreas Werner <andreas.werner@men.de>
>>>>>>>> ---
>>>>>>>>   drivers/leds/Kconfig          |   6 ++
>>>>>>>>   drivers/leds/Makefile         |   1 +
>>>>>>>>   drivers/leds/leds-menf21bmc.c | 134 ++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>   3 files changed, 141 insertions(+)
>>>>>>>>   create mode 100644 drivers/leds/leds-menf21bmc.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>>>>>>> index 27cf0cd..d38ff3f 100644
>>>>>>>> --- a/drivers/leds/Kconfig
>>>>>>>> +++ b/drivers/leds/Kconfig
>>>>>>>> @@ -458,6 +458,12 @@ config LEDS_OT200
>>>>>>>>            This option enables support for the LEDs on the Bachmann OT200.
>>>>>>>>            Say Y to enable LEDs on the Bachmann OT200.
>>>>>>>>
>>>>>>>> +config LEDS_MENF21BMC
>>>>>>>> +       tristate "LED support for the MEN 14F021P00 BMC"
>>>>>>>> +       depends on LEDS_CLASS && MFD_MENF21BMC
>>>>>>>
>>>>>>> I think it also depends on I2C.
>>>>>>
>>>>>> Yes you are right.
>>>>>>
>>>>>>>
>>>>>>>> +       help
>>>>>>>> +         Say Y here to include support for the MEN 14F021P00 BMC LEDs.
>>>>>>>> +
>>>>>>>>   comment "LED driver for blink(1) USB RGB LED is under Special HID drivers (HID_THINGM)"
>>>>>>>>
>>>>>>>>   config LEDS_BLINKM
>>>>>>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>>>>>>> index 3c03666..cadc433 100644
>>>>>>>> --- a/drivers/leds/Makefile
>>>>>>>> +++ b/drivers/leds/Makefile
>>>>>>>> @@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += leds-max8997.o
>>>>>>>>   obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
>>>>>>>>   obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
>>>>>>>>   obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
>>>>>>>> +obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
>>>>>>>>
>>>>>>>>   # LED SPI Drivers
>>>>>>>>   obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
>>>>>>>> diff --git a/drivers/leds/leds-menf21bmc.c b/drivers/leds/leds-menf21bmc.c
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000..5eaa119
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/leds/leds-menf21bmc.c
>>>>>>>> @@ -0,0 +1,134 @@
>>>>>>>> +/*
>>>>>>>> + *  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/i2c.h>
>>>>>>>> +
>>>>>>>> +#define BMC_CMD_LED_GET_SET    0xA0
>>>>>>>> +#define BMC_BIT_LED_STATUS     BIT(0)
>>>>>>>> +#define BMC_BIT_LED_HOTSWAP    BIT(1)
>>>>>>>> +#define BMC_BIT_LED_USER1      BIT(2)
>>>>>>>> +#define BMC_BIT_LED_USER2      BIT(3)
>>>>>>>> +
>>>>>>>> +struct menf21bmc_led {
>>>>>>>> +       struct led_classdev cdev;
>>>>>>>> +       u8 led_bit;
>>>>>>>> +       const char *name;
>>>>>>>> +       struct i2c_client *i2c_client;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct menf21bmc_led leds[] = {
>>>>>>>> +       {
>>>>>>>> +               .name = "menf21bmc:led_status",
>>>>>>>> +               .led_bit = BMC_BIT_LED_STATUS,
>>>>>>>> +       },
>>>>>>>> +       {
>>>>>>>> +               .name = "menf21bmc:led_hotswap",
>>>>>>>> +               .led_bit = BMC_BIT_LED_HOTSWAP,
>>>>>>>> +       },
>>>>>>>> +       {
>>>>>>>> +               .name = "menf21bmc:led_user1",
>>>>>>>> +               .led_bit = BMC_BIT_LED_USER1,
>>>>>>>> +       },
>>>>>>>> +       {
>>>>>>>> +               .name = "menf21bmc:led_user2",
>>>>>>>> +               .led_bit = BMC_BIT_LED_USER2,
>>>>>>>> +       }
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static DEFINE_MUTEX(led_lock);
>>>>>>>> +
>>>>>>>> +static void
>>>>>>>> +menf21bmc_led_set(struct led_classdev *led_cdev, enum led_brightness value)
>>>>>>>> +{
>>>>>>>> +       int led_val;
>>>>>>>> +       struct menf21bmc_led *led = container_of(led_cdev,
>>>>>>>> +                                       struct menf21bmc_led, cdev);
>>>>>>>> +
>>>>>>>> +       mutex_lock(&led_lock);
>>>>>>>> +       led_val = i2c_smbus_read_byte_data(led->i2c_client,
>>>>>>>> +                                               BMC_CMD_LED_GET_SET);
>>>>>>>> +       if (led_val < 0)
>>>>>>>> +               goto err_out;
>>>>>>>> +
>>>>>>>> +       if (value == LED_OFF)
>>>>>>>> +               led_val &= ~led->led_bit;
>>>>>>>> +       else
>>>>>>>> +               led_val |= led->led_bit;
>>>>>>>> +
>>>>>>>> +       i2c_smbus_write_byte_data(led->i2c_client,
>>>>>>>> +                                       BMC_CMD_LED_GET_SET, led_val);
>>>>>>>> +err_out:
>>>>>>>> +       mutex_unlock(&led_lock);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int menf21bmc_led_probe(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> +       int i;
>>>>>>>> +       int ret;
>>>>>>>> +       struct i2c_client *i2c_client = to_i2c_client(pdev->dev.parent);
>>>>>>>> +
>>>>>>>> +       for (i = 0; i < ARRAY_SIZE(leds); i++) {
>>>>>>>> +               leds[i].cdev.name = leds[i].name;
>>>>>>>> +               leds[i].cdev.brightness_set = menf21bmc_led_set;
>>>>>>>> +               leds[i].i2c_client = i2c_client;
>>>>>>>> +               ret = led_classdev_register(&pdev->dev, &leds[i].cdev);
>>>>>>>> +               if (ret < 0)
>>>>>>>> +                       goto err_free_leds;
>>>>>>>> +       }
>>>>>>>> +       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device enabled\n");
>>>>>>>> +
>>>>>>>> +       return 0;
>>>>>>>> +
>>>>>>>> +err_free_leds:
>>>>>>>> +       dev_err(&pdev->dev, "failed to register LED device\n");
>>>>>>>> +
>>>>>>>> +       for (i = i - 1; i >= 0; i--)
>>>>>>>> +               led_classdev_unregister(&leds[i].cdev);
>>>>>>>> +
>>>>>>>> +       return ret;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int menf21bmc_led_remove(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> +       int i;
>>>>>>>> +
>>>>>>>> +       for (i = 0; i < ARRAY_SIZE(leds); i++)
>>>>>>>> +               led_classdev_unregister(&leds[i].cdev);
>>>>>>>> +
>>>>>>>> +       return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static struct platform_driver menf21bmc_led = {
>>>>>>>> +       .probe          = menf21bmc_led_probe,
>>>>>>>> +       .remove         = menf21bmc_led_remove,
>>>>>>>> +       .driver         = {
>>>>>>>> +               .name           = "menf21bmc_led",
>>>>>>>> +               .owner          = THIS_MODULE,
>>>>>>>> +       },
>>>>>>>> +};
>>>>>>>> +
>>>>>>>
>>>>>>> This is not a normal platform driver, it should be a I2C driver.
>>>>>>>
>>>>>>>> +module_platform_driver(menf21bmc_led);
>>>>>>>
>>>>>>> So please move to use module_i2c_driver.
>>>>>>
>>>>>> Ok, but then I have to change that in the watchdog driver too, which
>>>>>> is quite the same as here.
>>>>>>
>>>>>> Lee, Guenther what do you think regarding the watchdog driver?
>>>>>
>>>>> If the watchdog part is indeed also i2c alike device then the watchdog part should also be an i2c driver...
>>>>>
>>>>
>>>> Isn't this all the same chip ? If so, there should be only one i2c driver.
>>>> Unless the i2c access is too complicated, it might make sense to use regmap
>>>> to access the chip, and pass the regmap pointer to the mfd slave drivers
>>>> to handle the actual communication with the chip.
>>>>
>>>> Guenter
>>>>
>>>
>>> Yes this is all the same chip which has different features like LEDs and Watchdog.
>>> This is why the mfd is the i2c_driver which instantiates the slave devices which
>>> are platform drivers.
>>>
>>> Is that not the normal way to handle those device?
>>> Is it really necessary to handle this to regmap?
>>>
>>
>> Not really sure if 'necessary' is the right term, it is just convenient.
>> Regmap takes care of register caching, for example, so you would not have to
>> re-read register values again and again from the chip just to modify a bit
>> of a register.
>> You can pass the regmap pointer in the platform data, or you can pass the
>> pointer to the i2c client. Using regmap makes the client driver independent
>> of the i2c subsystem. The one thing you can _not_ do is to declare the client
>> drivers to be i2c drivers.
>>
>> Guenter
>>
> I've checked out the regmap and it really sounds good.
> Before I will made those changes which is almost a complete replacement
> of the interface, i would like to know exactly why the slaves are no platform
> devices.
>
Did I say that ?

Guenter

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

[Wim Van Sebroeck]

> On 08/08/2014 12:49 AM, Andreas Werner wrote:
> >On Wed, Jul 30, 2014 at 06:57:46AM -0700, Guenter Roeck wrote:
> >>On 07/30/2014 01:08 AM, Andreas Werner wrote:
> >>>On Tue, Jul 29, 2014 at 02:47:08PM -0700, Guenter Roeck wrote:
> >>>>On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
> >>>>>Hi Andreas,
> >>>>>
> >>>>>>aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
> >>>>>>>On Thu, Jul 17, 2014 at 6:18 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.
> >>>>>>>>
> >>>>>>>>Signed-off-by: Andreas Werner <andreas.werner@men.de>
> >>>>>>>>---
> >>>>>>>>  drivers/leds/Kconfig          |   6 ++
> >>>>>>>>  drivers/leds/Makefile         |   1 +
> >>>>>>>>  drivers/leds/leds-menf21bmc.c | 134 
> >>>>>>>>  ++++++++++++++++++++++++++++++++++++++++++
> >>>>>>>>  3 files changed, 141 insertions(+)
> >>>>>>>>  create mode 100644 drivers/leds/leds-menf21bmc.c
> >>>>>>>>
> >>>>>>>>diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> >>>>>>>>index 27cf0cd..d38ff3f 100644
> >>>>>>>>--- a/drivers/leds/Kconfig
> >>>>>>>>+++ b/drivers/leds/Kconfig
> >>>>>>>>@@ -458,6 +458,12 @@ config LEDS_OT200
> >>>>>>>>           This option enables support for the LEDs on the Bachmann 
> >>>>>>>>           OT200.
> >>>>>>>>           Say Y to enable LEDs on the Bachmann OT200.
> >>>>>>>>
> >>>>>>>>+config LEDS_MENF21BMC
> >>>>>>>>+       tristate "LED support for the MEN 14F021P00 BMC"
> >>>>>>>>+       depends on LEDS_CLASS && MFD_MENF21BMC
> >>>>>>>
> >>>>>>>I think it also depends on I2C.
> >>>>>>
> >>>>>>Yes you are right.
> >>>>>>
> >>>>>>>
> >>>>>>>>+       help
> >>>>>>>>+         Say Y here to include support for the MEN 14F021P00 BMC 
> >>>>>>>>LEDs.
> >>>>>>>>+
> >>>>>>>>  comment "LED driver for blink(1) USB RGB LED is under Special HID 
> >>>>>>>>  drivers (HID_THINGM)"
> >>>>>>>>
> >>>>>>>>  config LEDS_BLINKM
> >>>>>>>>diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> >>>>>>>>index 3c03666..cadc433 100644
> >>>>>>>>--- a/drivers/leds/Makefile
> >>>>>>>>+++ b/drivers/leds/Makefile
> >>>>>>>>@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += 
> >>>>>>>>leds-max8997.o
> >>>>>>>>  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
> >>>>>>>>  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> >>>>>>>>  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> >>>>>>>>+obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
> >>>>>>>>
> >>>>>>>>  # LED SPI Drivers
> >>>>>>>>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> >>>>>>>>diff --git a/drivers/leds/leds-menf21bmc.c 
> >>>>>>>>b/drivers/leds/leds-menf21bmc.c
> >>>>>>>>new file mode 100644
> >>>>>>>>index 0000000..5eaa119
> >>>>>>>>--- /dev/null
> >>>>>>>>+++ b/drivers/leds/leds-menf21bmc.c
> >>>>>>>>@@ -0,0 +1,134 @@
> >>>>>>>>+/*
> >>>>>>>>+ *  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/i2c.h>
> >>>>>>>>+
> >>>>>>>>+#define BMC_CMD_LED_GET_SET    0xA0
> >>>>>>>>+#define BMC_BIT_LED_STATUS     BIT(0)
> >>>>>>>>+#define BMC_BIT_LED_HOTSWAP    BIT(1)
> >>>>>>>>+#define BMC_BIT_LED_USER1      BIT(2)
> >>>>>>>>+#define BMC_BIT_LED_USER2      BIT(3)
> >>>>>>>>+
> >>>>>>>>+struct menf21bmc_led {
> >>>>>>>>+       struct led_classdev cdev;
> >>>>>>>>+       u8 led_bit;
> >>>>>>>>+       const char *name;
> >>>>>>>>+       struct i2c_client *i2c_client;
> >>>>>>>>+};
> >>>>>>>>+
> >>>>>>>>+static struct menf21bmc_led leds[] = {
> >>>>>>>>+       {
> >>>>>>>>+               .name = "menf21bmc:led_status",
> >>>>>>>>+               .led_bit = BMC_BIT_LED_STATUS,
> >>>>>>>>+       },
> >>>>>>>>+       {
> >>>>>>>>+               .name = "menf21bmc:led_hotswap",
> >>>>>>>>+               .led_bit = BMC_BIT_LED_HOTSWAP,
> >>>>>>>>+       },
> >>>>>>>>+       {
> >>>>>>>>+               .name = "menf21bmc:led_user1",
> >>>>>>>>+               .led_bit = BMC_BIT_LED_USER1,
> >>>>>>>>+       },
> >>>>>>>>+       {
> >>>>>>>>+               .name = "menf21bmc:led_user2",
> >>>>>>>>+               .led_bit = BMC_BIT_LED_USER2,
> >>>>>>>>+       }
> >>>>>>>>+};
> >>>>>>>>+
> >>>>>>>>+static DEFINE_MUTEX(led_lock);
> >>>>>>>>+
> >>>>>>>>+static void
> >>>>>>>>+menf21bmc_led_set(struct led_classdev *led_cdev, enum 
> >>>>>>>>led_brightness value)
> >>>>>>>>+{
> >>>>>>>>+       int led_val;
> >>>>>>>>+       struct menf21bmc_led *led = container_of(led_cdev,
> >>>>>>>>+                                       struct menf21bmc_led, cdev);
> >>>>>>>>+
> >>>>>>>>+       mutex_lock(&led_lock);
> >>>>>>>>+       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> >>>>>>>>+                                               
> >>>>>>>>BMC_CMD_LED_GET_SET);
> >>>>>>>>+       if (led_val < 0)
> >>>>>>>>+               goto err_out;
> >>>>>>>>+
> >>>>>>>>+       if (value == LED_OFF)
> >>>>>>>>+               led_val &= ~led->led_bit;
> >>>>>>>>+       else
> >>>>>>>>+               led_val |= led->led_bit;
> >>>>>>>>+
> >>>>>>>>+       i2c_smbus_write_byte_data(led->i2c_client,
> >>>>>>>>+                                       BMC_CMD_LED_GET_SET, 
> >>>>>>>>led_val);
> >>>>>>>>+err_out:
> >>>>>>>>+       mutex_unlock(&led_lock);
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>+static int menf21bmc_led_probe(struct platform_device *pdev)
> >>>>>>>>+{
> >>>>>>>>+       int i;
> >>>>>>>>+       int ret;
> >>>>>>>>+       struct i2c_client *i2c_client = 
> >>>>>>>>to_i2c_client(pdev->dev.parent);
> >>>>>>>>+
> >>>>>>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> >>>>>>>>+               leds[i].cdev.name = leds[i].name;
> >>>>>>>>+               leds[i].cdev.brightness_set = menf21bmc_led_set;
> >>>>>>>>+               leds[i].i2c_client = i2c_client;
> >>>>>>>>+               ret = led_classdev_register(&pdev->dev, 
> >>>>>>>>&leds[i].cdev);
> >>>>>>>>+               if (ret < 0)
> >>>>>>>>+                       goto err_free_leds;
> >>>>>>>>+       }
> >>>>>>>>+       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device 
> >>>>>>>>enabled\n");
> >>>>>>>>+
> >>>>>>>>+       return 0;
> >>>>>>>>+
> >>>>>>>>+err_free_leds:
> >>>>>>>>+       dev_err(&pdev->dev, "failed to register LED device\n");
> >>>>>>>>+
> >>>>>>>>+       for (i = i - 1; i >= 0; i--)
> >>>>>>>>+               led_classdev_unregister(&leds[i].cdev);
> >>>>>>>>+
> >>>>>>>>+       return ret;
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>+static int menf21bmc_led_remove(struct platform_device *pdev)
> >>>>>>>>+{
> >>>>>>>>+       int i;
> >>>>>>>>+
> >>>>>>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++)
> >>>>>>>>+               led_classdev_unregister(&leds[i].cdev);
> >>>>>>>>+
> >>>>>>>>+       return 0;
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>>+static struct platform_driver menf21bmc_led = {
> >>>>>>>>+       .probe          = menf21bmc_led_probe,
> >>>>>>>>+       .remove         = menf21bmc_led_remove,
> >>>>>>>>+       .driver         = {
> >>>>>>>>+               .name           = "menf21bmc_led",
> >>>>>>>>+               .owner          = THIS_MODULE,
> >>>>>>>>+       },
> >>>>>>>>+};
> >>>>>>>>+
> >>>>>>>
> >>>>>>>This is not a normal platform driver, it should be a I2C driver.
> >>>>>>>
> >>>>>>>>+module_platform_driver(menf21bmc_led);
> >>>>>>>
> >>>>>>>So please move to use module_i2c_driver.
> >>>>>>
> >>>>>>Ok, but then I have to change that in the watchdog driver too, which
> >>>>>>is quite the same as here.
> >>>>>>
> >>>>>>Lee, Guenther what do you think regarding the watchdog driver?
> >>>>>
> >>>>>If the watchdog part is indeed also i2c alike device then the watchdog 
> >>>>>part should also be an i2c driver...
> >>>>>
> >>>>
> >>>>Isn't this all the same chip ? If so, there should be only one i2c 
> >>>>driver.
> >>>>Unless the i2c access is too complicated, it might make sense to use 
> >>>>regmap
> >>>>to access the chip, and pass the regmap pointer to the mfd slave drivers
> >>>>to handle the actual communication with the chip.
> >>>>
> >>>>Guenter
> >>>>
> >>>
> >>>Yes this is all the same chip which has different features like LEDs and 
> >>>Watchdog.
> >>>This is why the mfd is the i2c_driver which instantiates the slave 
> >>>devices which
> >>>are platform drivers.
> >>>
> >>>Is that not the normal way to handle those device?
> >>>Is it really necessary to handle this to regmap?
> >>>
> >>
> >>Not really sure if 'necessary' is the right term, it is just convenient.
> >>Regmap takes care of register caching, for example, so you would not have 
> >>to
> >>re-read register values again and again from the chip just to modify a bit
> >>of a register.
> >>You can pass the regmap pointer in the platform data, or you can pass the
> >>pointer to the i2c client. Using regmap makes the client driver 
> >>independent
> >>of the i2c subsystem. The one thing you can _not_ do is to declare the 
> >>client
> >>drivers to be i2c drivers.
> >>
> >>Guenter
> >>
> >I've checked out the regmap and it really sounds good.
> >Before I will made those changes which is almost a complete replacement
> >of the interface, i would like to know exactly why the slaves are no 
> >platform
> >devices.
> >
> Did I say that ?

What we wanted to say was:
1) if your multifunctional device is working as entity's of its own and these "standalone" devices are an i2c device then they should be configured as a i2c device driver.
2) is the multifunctional device has different functionality (like watchdog, gpio, ...) and it works as a block (thus there are overlaps) and you have no clean i2c or pci or usb boundaries then you normally use platform device drivers.

Apart from that it would in this case also be nice to use the regmap api.

Sorry that it got "complicated".

Kind regards,
Wim.

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

[Andreas Werner]

On Fri, Aug 08, 2014 at 04:36:55PM +0200, Wim Van Sebroeck wrote:
> > On 08/08/2014 12:49 AM, Andreas Werner wrote:
> > >On Wed, Jul 30, 2014 at 06:57:46AM -0700, Guenter Roeck wrote:
> > >>On 07/30/2014 01:08 AM, Andreas Werner wrote:
> > >>>On Tue, Jul 29, 2014 at 02:47:08PM -0700, Guenter Roeck wrote:
> > >>>>On 07/29/2014 02:12 PM, Wim Van Sebroeck wrote:
> > >>>>>Hi Andreas,
> > >>>>>
> > >>>>>>aOn Thu, Jul 24, 2014 at 03:00:09PM -0700, Bryan Wu wrote:
> > >>>>>>>On Thu, Jul 17, 2014 at 6:18 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.
> > >>>>>>>>
> > >>>>>>>>Signed-off-by: Andreas Werner <andreas.werner@men.de>
> > >>>>>>>>---
> > >>>>>>>>  drivers/leds/Kconfig          |   6 ++
> > >>>>>>>>  drivers/leds/Makefile         |   1 +
> > >>>>>>>>  drivers/leds/leds-menf21bmc.c | 134 
> > >>>>>>>>  ++++++++++++++++++++++++++++++++++++++++++
> > >>>>>>>>  3 files changed, 141 insertions(+)
> > >>>>>>>>  create mode 100644 drivers/leds/leds-menf21bmc.c
> > >>>>>>>>
> > >>>>>>>>diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > >>>>>>>>index 27cf0cd..d38ff3f 100644
> > >>>>>>>>--- a/drivers/leds/Kconfig
> > >>>>>>>>+++ b/drivers/leds/Kconfig
> > >>>>>>>>@@ -458,6 +458,12 @@ config LEDS_OT200
> > >>>>>>>>           This option enables support for the LEDs on the Bachmann 
> > >>>>>>>>           OT200.
> > >>>>>>>>           Say Y to enable LEDs on the Bachmann OT200.
> > >>>>>>>>
> > >>>>>>>>+config LEDS_MENF21BMC
> > >>>>>>>>+       tristate "LED support for the MEN 14F021P00 BMC"
> > >>>>>>>>+       depends on LEDS_CLASS && MFD_MENF21BMC
> > >>>>>>>
> > >>>>>>>I think it also depends on I2C.
> > >>>>>>
> > >>>>>>Yes you are right.
> > >>>>>>
> > >>>>>>>
> > >>>>>>>>+       help
> > >>>>>>>>+         Say Y here to include support for the MEN 14F021P00 BMC 
> > >>>>>>>>LEDs.
> > >>>>>>>>+
> > >>>>>>>>  comment "LED driver for blink(1) USB RGB LED is under Special HID 
> > >>>>>>>>  drivers (HID_THINGM)"
> > >>>>>>>>
> > >>>>>>>>  config LEDS_BLINKM
> > >>>>>>>>diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > >>>>>>>>index 3c03666..cadc433 100644
> > >>>>>>>>--- a/drivers/leds/Makefile
> > >>>>>>>>+++ b/drivers/leds/Makefile
> > >>>>>>>>@@ -53,6 +53,7 @@ obj-$(CONFIG_LEDS_MAX8997)            += 
> > >>>>>>>>leds-max8997.o
> > >>>>>>>>  obj-$(CONFIG_LEDS_LM355x)              += leds-lm355x.o
> > >>>>>>>>  obj-$(CONFIG_LEDS_BLINKM)              += leds-blinkm.o
> > >>>>>>>>  obj-$(CONFIG_LEDS_VERSATILE)           += leds-versatile.o
> > >>>>>>>>+obj-$(CONFIG_LEDS_MENF21BMC)           += leds-menf21bmc.o
> > >>>>>>>>
> > >>>>>>>>  # LED SPI Drivers
> > >>>>>>>>  obj-$(CONFIG_LEDS_DAC124S085)          += leds-dac124s085.o
> > >>>>>>>>diff --git a/drivers/leds/leds-menf21bmc.c 
> > >>>>>>>>b/drivers/leds/leds-menf21bmc.c
> > >>>>>>>>new file mode 100644
> > >>>>>>>>index 0000000..5eaa119
> > >>>>>>>>--- /dev/null
> > >>>>>>>>+++ b/drivers/leds/leds-menf21bmc.c
> > >>>>>>>>@@ -0,0 +1,134 @@
> > >>>>>>>>+/*
> > >>>>>>>>+ *  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/i2c.h>
> > >>>>>>>>+
> > >>>>>>>>+#define BMC_CMD_LED_GET_SET    0xA0
> > >>>>>>>>+#define BMC_BIT_LED_STATUS     BIT(0)
> > >>>>>>>>+#define BMC_BIT_LED_HOTSWAP    BIT(1)
> > >>>>>>>>+#define BMC_BIT_LED_USER1      BIT(2)
> > >>>>>>>>+#define BMC_BIT_LED_USER2      BIT(3)
> > >>>>>>>>+
> > >>>>>>>>+struct menf21bmc_led {
> > >>>>>>>>+       struct led_classdev cdev;
> > >>>>>>>>+       u8 led_bit;
> > >>>>>>>>+       const char *name;
> > >>>>>>>>+       struct i2c_client *i2c_client;
> > >>>>>>>>+};
> > >>>>>>>>+
> > >>>>>>>>+static struct menf21bmc_led leds[] = {
> > >>>>>>>>+       {
> > >>>>>>>>+               .name = "menf21bmc:led_status",
> > >>>>>>>>+               .led_bit = BMC_BIT_LED_STATUS,
> > >>>>>>>>+       },
> > >>>>>>>>+       {
> > >>>>>>>>+               .name = "menf21bmc:led_hotswap",
> > >>>>>>>>+               .led_bit = BMC_BIT_LED_HOTSWAP,
> > >>>>>>>>+       },
> > >>>>>>>>+       {
> > >>>>>>>>+               .name = "menf21bmc:led_user1",
> > >>>>>>>>+               .led_bit = BMC_BIT_LED_USER1,
> > >>>>>>>>+       },
> > >>>>>>>>+       {
> > >>>>>>>>+               .name = "menf21bmc:led_user2",
> > >>>>>>>>+               .led_bit = BMC_BIT_LED_USER2,
> > >>>>>>>>+       }
> > >>>>>>>>+};
> > >>>>>>>>+
> > >>>>>>>>+static DEFINE_MUTEX(led_lock);
> > >>>>>>>>+
> > >>>>>>>>+static void
> > >>>>>>>>+menf21bmc_led_set(struct led_classdev *led_cdev, enum 
> > >>>>>>>>led_brightness value)
> > >>>>>>>>+{
> > >>>>>>>>+       int led_val;
> > >>>>>>>>+       struct menf21bmc_led *led = container_of(led_cdev,
> > >>>>>>>>+                                       struct menf21bmc_led, cdev);
> > >>>>>>>>+
> > >>>>>>>>+       mutex_lock(&led_lock);
> > >>>>>>>>+       led_val = i2c_smbus_read_byte_data(led->i2c_client,
> > >>>>>>>>+                                               
> > >>>>>>>>BMC_CMD_LED_GET_SET);
> > >>>>>>>>+       if (led_val < 0)
> > >>>>>>>>+               goto err_out;
> > >>>>>>>>+
> > >>>>>>>>+       if (value == LED_OFF)
> > >>>>>>>>+               led_val &= ~led->led_bit;
> > >>>>>>>>+       else
> > >>>>>>>>+               led_val |= led->led_bit;
> > >>>>>>>>+
> > >>>>>>>>+       i2c_smbus_write_byte_data(led->i2c_client,
> > >>>>>>>>+                                       BMC_CMD_LED_GET_SET, 
> > >>>>>>>>led_val);
> > >>>>>>>>+err_out:
> > >>>>>>>>+       mutex_unlock(&led_lock);
> > >>>>>>>>+}
> > >>>>>>>>+
> > >>>>>>>>+static int menf21bmc_led_probe(struct platform_device *pdev)
> > >>>>>>>>+{
> > >>>>>>>>+       int i;
> > >>>>>>>>+       int ret;
> > >>>>>>>>+       struct i2c_client *i2c_client = 
> > >>>>>>>>to_i2c_client(pdev->dev.parent);
> > >>>>>>>>+
> > >>>>>>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++) {
> > >>>>>>>>+               leds[i].cdev.name = leds[i].name;
> > >>>>>>>>+               leds[i].cdev.brightness_set = menf21bmc_led_set;
> > >>>>>>>>+               leds[i].i2c_client = i2c_client;
> > >>>>>>>>+               ret = led_classdev_register(&pdev->dev, 
> > >>>>>>>>&leds[i].cdev);
> > >>>>>>>>+               if (ret < 0)
> > >>>>>>>>+                       goto err_free_leds;
> > >>>>>>>>+       }
> > >>>>>>>>+       dev_info(&pdev->dev, "MEN 140F21P00 BMC LED device 
> > >>>>>>>>enabled\n");
> > >>>>>>>>+
> > >>>>>>>>+       return 0;
> > >>>>>>>>+
> > >>>>>>>>+err_free_leds:
> > >>>>>>>>+       dev_err(&pdev->dev, "failed to register LED device\n");
> > >>>>>>>>+
> > >>>>>>>>+       for (i = i - 1; i >= 0; i--)
> > >>>>>>>>+               led_classdev_unregister(&leds[i].cdev);
> > >>>>>>>>+
> > >>>>>>>>+       return ret;
> > >>>>>>>>+}
> > >>>>>>>>+
> > >>>>>>>>+static int menf21bmc_led_remove(struct platform_device *pdev)
> > >>>>>>>>+{
> > >>>>>>>>+       int i;
> > >>>>>>>>+
> > >>>>>>>>+       for (i = 0; i < ARRAY_SIZE(leds); i++)
> > >>>>>>>>+               led_classdev_unregister(&leds[i].cdev);
> > >>>>>>>>+
> > >>>>>>>>+       return 0;
> > >>>>>>>>+}
> > >>>>>>>>+
> > >>>>>>>>+static struct platform_driver menf21bmc_led = {
> > >>>>>>>>+       .probe          = menf21bmc_led_probe,
> > >>>>>>>>+       .remove         = menf21bmc_led_remove,
> > >>>>>>>>+       .driver         = {
> > >>>>>>>>+               .name           = "menf21bmc_led",
> > >>>>>>>>+               .owner          = THIS_MODULE,
> > >>>>>>>>+       },
> > >>>>>>>>+};
> > >>>>>>>>+
> > >>>>>>>
> > >>>>>>>This is not a normal platform driver, it should be a I2C driver.
> > >>>>>>>
> > >>>>>>>>+module_platform_driver(menf21bmc_led);
> > >>>>>>>
> > >>>>>>>So please move to use module_i2c_driver.
> > >>>>>>
> > >>>>>>Ok, but then I have to change that in the watchdog driver too, which
> > >>>>>>is quite the same as here.
> > >>>>>>
> > >>>>>>Lee, Guenther what do you think regarding the watchdog driver?
> > >>>>>
> > >>>>>If the watchdog part is indeed also i2c alike device then the watchdog 
> > >>>>>part should also be an i2c driver...
> > >>>>>
> > >>>>
> > >>>>Isn't this all the same chip ? If so, there should be only one i2c 
> > >>>>driver.
> > >>>>Unless the i2c access is too complicated, it might make sense to use 
> > >>>>regmap
> > >>>>to access the chip, and pass the regmap pointer to the mfd slave drivers
> > >>>>to handle the actual communication with the chip.
> > >>>>
> > >>>>Guenter
> > >>>>
> > >>>
> > >>>Yes this is all the same chip which has different features like LEDs and 
> > >>>Watchdog.
> > >>>This is why the mfd is the i2c_driver which instantiates the slave 
> > >>>devices which
> > >>>are platform drivers.
> > >>>
> > >>>Is that not the normal way to handle those device?
> > >>>Is it really necessary to handle this to regmap?
> > >>>
> > >>
> > >>Not really sure if 'necessary' is the right term, it is just convenient.
> > >>Regmap takes care of register caching, for example, so you would not have 
> > >>to
> > >>re-read register values again and again from the chip just to modify a bit
> > >>of a register.
> > >>You can pass the regmap pointer in the platform data, or you can pass the
> > >>pointer to the i2c client. Using regmap makes the client driver 
> > >>independent
> > >>of the i2c subsystem. The one thing you can _not_ do is to declare the 
> > >>client
> > >>drivers to be i2c drivers.
> > >>
> > >>Guenter
> > >>
> > >I've checked out the regmap and it really sounds good.
> > >Before I will made those changes which is almost a complete replacement
> > >of the interface, i would like to know exactly why the slaves are no 
> > >platform
> > >devices.
> > >
> > Did I say that ?
> 
> What we wanted to say was:
> 1) if your multifunctional device is working as entity's of its own and these "standalone" devices are an i2c device then they should be configured as a i2c device driver.
> 2) is the multifunctional device has different functionality (like watchdog, gpio, ...) and it works as a block (thus there are overlaps) and you have no clean i2c or pci or usb boundaries then you normally use platform device drivers.
> 
> Apart from that it would in this case also be nice to use the regmap api.
> 
> Sorry that it got "complicated".
> 
> Kind regards,
> Wim.
> 

The chip is like you described in 2). This is why the slaves are platform device.
It is not "complicated" it was just a little confuse.
Thanks for the nice explanation.

Next time if i develope a driver like this, i will use regmap but for the bmc i
will let it as it is.

I will send a v4 with the little remarks from your guys, in the next days.

Thanks.

Regards
Andy