Re: [PATCH] ARM: leds: Add MAX6956 driver
From: Bryan Wu
Date: Mon May 21 2012 - 00:50:31 EST
Hi Uwe,
This patch looks quite nice to me, just some comments as below.
On Fri, May 18, 2012 at 11:45 PM, Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> This adds a driver for Maxim's MAX6956 28-Port LED Display Driver and
> I/O Expander.
>
MAX6956 is a MFD for LED + GPIO. and most of this driver are adding an
new gpiochip driver. Is that possible we split these 2 functions into
2 drivers?
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
> drivers/leds/Kconfig | 10 +
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-max6956.c | 359 ++++++++++++++++++++++++++++
> include/linux/platform_data/leds-max6956.h | 17 ++
> 4 files changed, 387 insertions(+)
> create mode 100644 drivers/leds/leds-max6956.c
> create mode 100644 include/linux/platform_data/leds-max6956.h
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ff4b8cf..79ef2a1 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -387,6 +387,16 @@ config LEDS_TCA6507
> LED driver chips accessed via the I2C bus.
> Driver support brightness control and hardware-assisted blinking.
>
> +config LEDS_MAX6956
> + tristate "LED support for MAX6956 LED Display Driver and I/O Expander"
> + depends on LEDS_CLASS
> + depends on GPIOLIB
> + depends on I2C
> + select REGMAP_I2C
> + help
> + This option enables support the LEDs and GPIOs connected to Maxim's
> + MAX6956 28-Port LED Display Driver and I/O Expander.
> +
> config LEDS_MAX8997
> tristate "LED support for MAX8997 PMIC"
> depends on LEDS_CLASS && MFD_MAX8997
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 890481c..87ec494 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -44,6 +44,7 @@ obj-$(CONFIG_LEDS_NS2) += leds-ns2.o
> obj-$(CONFIG_LEDS_NETXBIG) += leds-netxbig.o
> obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
> obj-$(CONFIG_LEDS_RENESAS_TPU) += leds-renesas-tpu.o
> +obj-$(CONFIG_LEDS_MAX6956) += leds-max6956.o
> obj-$(CONFIG_LEDS_MAX8997) += leds-max8997.o
>
> # LED SPI Drivers
> diff --git a/drivers/leds/leds-max6956.c b/drivers/leds/leds-max6956.c
> new file mode 100644
> index 0000000..976ed91
> --- /dev/null
> +++ b/drivers/leds/leds-max6956.c
> @@ -0,0 +1,359 @@
> +/*
> + * Maxim 28-Port LED Display Driver and I/O Expander
> + *
> + * Copyright (C) 2012 Pengutronix
> + * Uwe Kleine-Koenig <u.kleine-koenig@xxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation.
> + *
> + * Datasheet: http://datasheets.maxim-ic.com/en/ds/MAX6956.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/leds.h>
> +#include <linux/input.h>
> +#include <linux/mutex.h>
> +#include <linux/leds-pca9532.h>
I think we need move all these platform data headers into
include/linux/platform_data/ as you did below. Anyway, I will take
care of this.
Wait, why do you need this header file in your driver? I failed to see
any usage of it here.
> +#include <linux/gpio.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/platform_data/leds-max6956.h>
> +
> +#define MAX6956_REG_GLOBAL_CURRENT 0x02
> +#define MAX6956_REG_CONFIGURATION 0x04
> +#define MAX6956_REG_CONFIGURATION_S 0x01
> +#define MAX6956_REG_CONFIGURATION_I 0x40
> +#define MAX6956_REG_CONFIGURATION_M 0x80
> +#define MAX6956_REG_TRANSITION_DETECT_MASK 0x06
> +#define MAX6956_REG_DISPLAY_TEST 0x07
> +/*
> + * MAX6956_REG_PORT_CONFIGURATION(i) holds the configuration for ports
> + * 4 * i, 4 * i + 1, ..., 4 * i + 3 for i in [1, ... 7].
> + */
> +#define MAX6956_REG_PORT_CONFIGURATION(i) (0x08 + (i))
> +#define MAX6956_REG_PORT_CONFIGURATION_LED 0x0
> +#define MAX6956_REG_PORT_CONFIGURATION_GPIOOUT 0x1
> +#define MAX6956_REG_PORT_CONFIGURATION_GPIOINPULLUP 0x2
> +#define MAX6956_REG_PORT_CONFIGURATION_GPIOIN 0x3
> +/*
> + * MAX6956_REG_CURRENT(i) holds the current for segments 2 * i, 2 * i + 1 for i
> + * in [2, .. 15].
Looks like it should be [2, ... 15].
> + */
> +#define MAX6956_REG_CURRENT(i) (0x10 + (i))
> +/*
> + * MAX6956_REG_PORT(i) is valid for i in [4, ... 31]. Data bit 0 holds the value
> + * for port i.
> + */
> +#define MAX6956_REG_PORT(i) (0x20 + (i))
> +/*
> + * MAX6956_REG_MULTIPORT(i) contains MAX6956_REG_PORT(j) for j in [i, ... i + 7]
> + * for i in [0, ... 31]. Note that data for invalid ports (i.e. 0-3 and 31-38)
> + * read as 0 and writes have no effect.
> + * Note that there is a bug in the documentation (as of revision 2) specifying
> + * that at the high end the data is contained in the lower bits.
> + */
> +#define MAX6956_REG_MULTIPORT(i) (0x40 + (i))
> +
> +#define MAX6956_NUM_REGISTERS 0x60
> +
> +struct max6956_led_ddata {
> + unsigned offset;
> + struct led_classdev cdev;
> + struct work_struct work;
> + enum led_brightness brightness;
> +};
> +
> +struct max6956_ddata {
> + struct device *dev;
> +
> + struct regmap *regmap;
> +
> + struct gpio_chip gpio_chip;
> +
> + struct max6956_pdata pdata;
> +
> + struct max6956_led_ddata leds[32];
> +
> + const char *gpio_names[32];
> +};
> +
> +#define ddata_from_gpio_chip(chip) \
> + container_of(chip, struct max6956_ddata, gpio_chip)
> +#define ddata_from_led_cdev(cdev) \
> + dev_get_drvdata(cdev->dev->parent)
> +#define ddata_from_work(_work) \
> + ddata_from_led_cdev(&lddata_from_work(_work)->cdev)
> +
> +#define lddata_from_led_cdev(_cdev) \
> + container_of(_cdev, struct max6956_led_ddata, cdev)
> +#define lddata_from_work(_work) \
> + container_of(_work, struct max6956_led_ddata, work)
> +
> +static const struct regmap_config max6956_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .cache_type = REGCACHE_NONE,
> +
> + .max_register = 0x5f,
> +};
> +
> +static int max6956_portconfig_set(struct max6956_ddata *ddata, unsigned offset,
> + unsigned mode)
> +{
> + unsigned int reg = MAX6956_REG_PORT_CONFIGURATION(offset / 4);
> + unsigned int shift = 2 * (offset % 4);
> +
> + return regmap_update_bits(ddata->regmap, reg,
> + 0x3 << shift, mode << shift);
> +}
> +
> +static int max6956_set_sink_current(struct max6956_ddata *ddata,
> + unsigned offset, unsigned regcurrent)
> +{
> + unsigned reg = MAX6956_REG_CURRENT(offset / 2);
> + unsigned shift = offset & 1 ? 4 : 0;
> +
> + return regmap_update_bits(ddata->regmap, reg,
> + 0xf << shift, (regcurrent - 1) << shift);
> +}
> +
> +static void max6956_led_work(struct work_struct *work)
> +{
> + struct max6956_led_ddata *lddata = lddata_from_work(work);
> + struct led_classdev *led_cdev = &lddata->cdev;
> +
> + struct max6956_ddata *ddata = ddata_from_led_cdev(led_cdev);
> +
> + BUG_ON(&ddata->leds[lddata->offset] != lddata);
> +
> + if (!lddata->brightness) {
> + regmap_write(ddata->regmap,
> + MAX6956_REG_PORT(lddata->offset), 0);
> + } else {
> + max6956_set_sink_current(ddata, lddata->offset,
> + lddata->brightness);
> + regmap_write(ddata->regmap,
> + MAX6956_REG_PORT(lddata->offset), 1);
> + }
> + max6956_portconfig_set(ddata, lddata->offset, 0);
> +}
> +
I believe we might need some locking here to protect this critical
region, like mutex_lock().
> +static unsigned max6956_get_sink_current(struct max6956_ddata *ddata,
> + unsigned offset)
> +{
> + unsigned reg = MAX6956_REG_CURRENT(offset / 2);
> + unsigned shift = offset & 1 ? 4 : 0;
> + unsigned regcurrent;
> +
> + regmap_read(ddata->regmap, reg, ®current);
> +
> + return ((regcurrent >> shift) & 0xf) + 1;
> +}
> +
> +static void max6956_led_brightness_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct max6956_led_ddata *lddata = lddata_from_led_cdev(led_cdev);
> + lddata->brightness = brightness;
> + schedule_work(&lddata->work);
> +}
> +
> +static enum led_brightness max6956_led_brightness_get(
> + struct led_classdev *led_cdev)
> +{
> + struct max6956_led_ddata *lddata = lddata_from_led_cdev(led_cdev);
> + struct max6956_ddata *ddata = ddata_from_led_cdev(led_cdev);
> + unsigned val;
> +
> + BUG_ON(&ddata->leds[lddata->offset] != lddata);
> +
> + regmap_read(ddata->regmap, MAX6956_REG_PORT(lddata->offset), &val);
> +
> + if (!(val & 1))
> + return 0;
> +
> + return max6956_get_sink_current(ddata, lddata->offset);
> +}
> +
> +static int max6956_gpio_request(struct gpio_chip *chip, unsigned offset)
> +{
> + struct max6956_ddata *ddata = ddata_from_gpio_chip(chip);
> + unsigned char type = ddata->pdata.led_pdata[offset].type;
> +
> + if (type != MAX6956_TYPE_GPIO && type != MAX6956_TYPE_GPIOPULLUP)
> + return -EBUSY;
> +
> + return 0;
> +}
> +
> +static int max6956_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
> +{
> + struct max6956_ddata *ddata = ddata_from_gpio_chip(chip);
> + unsigned mode;
> +
> + switch (ddata->pdata.led_pdata[offset].type) {
> + case MAX6956_TYPE_GPIO:
> + mode = MAX6956_REG_PORT_CONFIGURATION_GPIOIN;
> + break;
> + case MAX6956_TYPE_GPIOPULLUP:
> + mode = MAX6956_REG_PORT_CONFIGURATION_GPIOINPULLUP;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return max6956_portconfig_set(ddata, offset, mode);
> +}
> +
> +static int max6956_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + struct max6956_ddata *ddata = ddata_from_gpio_chip(chip);
> + unsigned int val;
> +
> + regmap_read(ddata->regmap, MAX6956_REG_PORT(offset), &val);
> +
> + return val;
> +}
> +
> +static int max6956_gpio_direction_output(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + struct max6956_ddata *ddata = ddata_from_gpio_chip(chip);
> +
> + regmap_write(ddata->regmap, MAX6956_REG_PORT(offset), !!value);
> +
> + return max6956_portconfig_set(ddata, offset,
> + MAX6956_REG_PORT_CONFIGURATION_GPIOOUT);
> +}
> +
> +static void max6956_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct max6956_ddata *ddata = ddata_from_gpio_chip(chip);
> +
> + regmap_write(ddata->regmap, MAX6956_REG_PORT(offset), !!value);
> +}
> +
> +static const struct gpio_chip max6956_gpio_chip_init __devinitconst = {
> + .label = "max6956",
> + .owner = THIS_MODULE,
> + .request = max6956_gpio_request,
> + .direction_input = max6956_gpio_direction_input,
> + .get = max6956_gpio_get,
> + .direction_output = max6956_gpio_direction_output,
> + .set = max6956_gpio_set,
> + .base = -1,
> + .ngpio = 32,
> + .can_sleep = 1,
> +};
> +
> +static int __devinit max6956_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct max6956_ddata *ddata;
> + struct max6956_pdata *pdata = client->dev.platform_data;
> + int ret, i;
> +
> + if (!pdata)
> + return -EINVAL;
> +
> + ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> + if (!ddata) {
> + dev_err(&client->dev, "Failed to allocate driver private data\n");
> + return -ENOMEM;
> + }
> +
> + ddata->dev = &client->dev;
> + ddata->regmap = devm_regmap_init_i2c(client, &max6956_regmap_config);
> + if (IS_ERR(ddata->regmap)) {
> + ret = PTR_ERR(ddata->regmap);
> + dev_err(ddata->dev, "Failed to allocate register map: %d\n",
> + ret);
> + return ret;
As Shuah said, no kfree(ddata) here
> + }
> + ddata->pdata = *pdata;
> + i2c_set_clientdata(client, ddata);
> +
> + ddata->gpio_chip = max6956_gpio_chip_init;
> + ddata->gpio_chip.names = ddata->gpio_names;
> + ddata->gpio_chip.dev = ddata->dev;
> +
> + regmap_write(ddata->regmap, MAX6956_REG_CONFIGURATION,
> + MAX6956_REG_CONFIGURATION_S |
> + MAX6956_REG_CONFIGURATION_I);
> +
> + for (i = 4; i < 32; ++i)
> + switch (pdata->led_pdata[i].type) {
> + case MAX6956_TYPE_GPIO:
> + case MAX6956_TYPE_GPIOPULLUP:
> + ddata->gpio_names[i] = pdata->led_pdata[i].name;
> + break;
> + case MAX6956_TYPE_LED:
> + ddata->leds[i] = (struct max6956_led_ddata){
> + .offset = i,
> + .cdev = {
> + .name = pdata->led_pdata[i].name,
> + .max_brightness = 16,
> + .brightness_set =
> + max6956_led_brightness_set,
> + .brightness_get =
> + max6956_led_brightness_get,
> + },
> + };
> + INIT_WORK(&ddata->leds[i].work, max6956_led_work);
> +
> + ret = led_classdev_register(ddata->dev,
> + &ddata->leds[i].cdev);
> + if (ret)
> + dev_warn(ddata->dev,
> + "Failed to register led %s\n",
> + pdata->led_pdata[i].name);
> + break;
> + }
> +
> + ret = gpiochip_add(&ddata->gpio_chip);
> +
> + return ret;
> +}
> +
> +static int __devexit max6956_remove(struct i2c_client *client)
> +{
> + struct max6956_ddata *ddata = i2c_get_clientdata(client);
> + int ret, i;
> +
> + ret = gpiochip_remove(&ddata->gpio_chip);
> + if (ret)
> + dev_warn(ddata->dev, "Failed to remove gpiochip: %d\n", ret);
> +
> + for (i = 4; i < 32; ++i)
> + if (ddata->pdata.led_pdata[i].type == MAX6956_TYPE_LED)
> + led_classdev_unregister(&ddata->leds[i].cdev);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id max6956_device_id[] = {
> + { "max6956", },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max6956_device_id);
> +
> +static struct i2c_driver max6956_driver = {
> + .driver = {
> + .name = "leds-max6956",
> + .owner = THIS_MODULE,
> + },
> + .probe = max6956_probe,
> + .remove = max6956_remove,
> + .id_table = max6956_device_id,
> +};
> +
> +module_i2c_driver(max6956_driver);
> +
> +MODULE_AUTHOR("Uwe Kleine-Koenig <u.kleine-koenig@xxxxxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MAX6956 LED Display Driver and I/O Expander");
> diff --git a/include/linux/platform_data/leds-max6956.h b/include/linux/platform_data/leds-max6956.h
> new file mode 100644
> index 0000000..c7290a4
> --- /dev/null
> +++ b/include/linux/platform_data/leds-max6956.h
> @@ -0,0 +1,17 @@
> +#ifndef __LINUX_PLATFORM_DATA_LEDS_MAX6956_H__
> +#define __LINUX_PLATFORM_DATA_LEDS_MAX6956_H__
> +
> +#define MAX6956_TYPE_GPIO 0
> +#define MAX6956_TYPE_GPIOPULLUP 1
> +#define MAX6956_TYPE_LED 2
> +
> +struct max6956_led_pdata {
> + unsigned char type;
> + const char *name;
> +};
> +
> +struct max6956_pdata {
> + struct max6956_led_pdata led_pdata[32];
> +};
> +
> +#endif
> --
> 1.7.10
>
--
Bryan Wu <bryan.wu@xxxxxxxxxxxxx>
Kernel Developer +86.186-168-78255 Mobile
Canonical Ltd. www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
--
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/