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

From: Andreas Werner
Date: Wed Jul 30 2014 - 03:16:49 EST

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@xxxxxx> 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@xxxxxx>
> >>>>---
> >>>> 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@xxxxxx>
> >>>>+ * 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/