Re: [PATCH] ARM: leds: Add MAX6956 driver

[Shuah Khan <shuahkhan@gmail.com>]

On Fri, 2012-05-18 at 17:45 +0200, Uwe Kleine-König wrote:
> This adds a driver for Maxim's MAX6956 28-Port LED Display Driver and
> I/O Expander.

Comments in line.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  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@pengutronix.de>
> + *
> + * 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>
> +#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].
> + */
> +#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);
> +}
> +
> +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, &regcurrent);
> +
> +	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);

I don't see this memory getting free'ed in error legs and also from
max6956_remove().

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

Missing kfree for ddata here?

> +		return ret;
> +	}
> +	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);

Need to check error here and do cleanup linke free'ing ddata - example
leds-pca9532.c

> +
> +	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@pengutronix.de>");
> +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