From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967070Ab2ERThR (ORCPT ); Fri, 18 May 2012 15:37:17 -0400 Received: from g1t0026.austin.hp.com ([15.216.28.33]:26916 "EHLO g1t0026.austin.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965529Ab2ERThO (ORCPT ); Fri, 18 May 2012 15:37:14 -0400 Message-ID: <1337369831.2426.15.camel@lorien2> Subject: Re: [PATCH] ARM: leds: Add MAX6956 driver From: Shuah Khan Reply-To: shuahkhan@gmail.com To: Uwe =?ISO-8859-1?Q?Kleine-K=F6nig?= Cc: shuahkhan@gmail.com, Bryan Wu , Richard Purdie , kernel@pengutronix.de, linux-kernel@vger.kernel.org Date: Fri, 18 May 2012 13:37:11 -0600 In-Reply-To: <1337355945-16421-1-git-send-email-u.kleine-koenig@pengutronix.de> References: <1337355945-16421-1-git-send-email-u.kleine-koenig@pengutronix.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 8bit Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#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, ®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); 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 "); > +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