Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966796Ab3HHWKV (ORCPT ); Thu, 8 Aug 2013 18:10:21 -0400 Received: from mail-la0-f49.google.com ([209.85.215.49]:41391 "EHLO mail-la0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966754Ab3HHWKU (ORCPT ); Thu, 8 Aug 2013 18:10:20 -0400 MIME-Version: 1.0 In-Reply-To: <1375980335-19943-2-git-send-email-ricardo.ribalda@gmail.com> References: <1375980335-19943-1-git-send-email-ricardo.ribalda@gmail.com> <1375980335-19943-2-git-send-email-ricardo.ribalda@gmail.com> From: Bryan Wu Date: Thu, 8 Aug 2013 15:09:57 -0700 Message-ID: Subject: Re: [PATCH v2 1/3] leds-pca9633: Add support for PCA9634 To: Ricardo Ribalda Delgado Cc: Peter Meerwald , Richard Purdie , Linux LED Subsystem , LKML Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8174 Lines: 225 On Thu, Aug 8, 2013 at 9:45 AM, Ricardo Ribalda Delgado wrote: > Add support for PCA9634 chip, which belongs to the same family as the > 9633 but with support for 8 outputs instead of 4. > Basically I like this method to add a new chip supporting. Please find my comments below. > Signed-off-by: Ricardo Ribalda Delgado > --- > drivers/leds/Kconfig | 7 ++-- > drivers/leds/leds-pca9633.c | 74 +++++++++++++++++++++++++++++++------------ What about just rename the whole file to leds-pca963x.c. And rename some pca9633 to pca963x in the driver. > 2 files changed, 58 insertions(+), 23 deletions(-) > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index e43402d..023af58 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -280,12 +280,13 @@ config LEDS_PCA955X > devices include PCA9550, PCA9551, PCA9552, and PCA9553. > > config LEDS_PCA9633 Also change this config option to LEDS_PCA963X > - tristate "LED support for PCA9633 I2C chip" > + tristate "LED support for PCA963x I2C chip" > depends on LEDS_CLASS > depends on I2C > help > - This option enables support for LEDs connected to the PCA9633 > - LED driver chip accessed via the I2C bus. > + This option enables support for LEDs connected to the PCA963x > + LED driver chip accessed via the I2C bus. Supported > + devices include PCA9633 and PCA9634 > > config LEDS_WM831X_STATUS > tristate "LED support for status LEDs on WM831x PMICs" > diff --git a/drivers/leds/leds-pca9633.c b/drivers/leds/leds-pca9633.c > index 9aae567..070358a 100644 > --- a/drivers/leds/leds-pca9633.c > +++ b/drivers/leds/leds-pca9633.c > @@ -1,7 +1,9 @@ > /* > * Copyright 2011 bct electronic GmbH > + * Copyright 2013 Qtechnology/AS > * > * Author: Peter Meerwald > + * Author: Ricardo Ribalda > * > * Based on leds-pca955x.c > * > @@ -10,6 +12,7 @@ > * directory of this archive for more details. > * > * LED driver for the PCA9633 I2C LED driver (7-bit slave address 0x62) > + * LED driver for the PCA9634 I2C LED driver (7-bit slave address set by hw.) > * > */ > > @@ -33,20 +36,42 @@ > #define PCA9633_MODE1 0x00 > #define PCA9633_MODE2 0x01 > #define PCA9633_PWM_BASE 0x02 > -#define PCA9633_LEDOUT 0x08 > + > +enum pca9633_type { pca963x_type > + pca9633, > + pca9634, > +}; > + > +struct pca9633_chipdef { pca963x_chipdef > + u8 ledout_base; > + int n_leds; > +}; > + > +static struct pca9633_chipdef pca9633_chipdefs[] = { 963x > + [pca9633] = { > + .ledout_base = 0x8, > + .n_leds = 4, > + }, > + [pca9634] = { > + .ledout_base = 0xc, > + .n_leds = 8, > + }, > +}; > > static const struct i2c_device_id pca9633_id[] = { 963x > - { "pca9633", 0 }, > + { "pca9633", pca9633 }, { "pca963x", pca9633 }, > + { "pca9634", pca9634 }, { "pca963x", pca9634 }, > { } > }; > MODULE_DEVICE_TABLE(i2c, pca9633_id); > > struct pca9633_led { > struct i2c_client *client; > + struct pca9633_chipdef *chipdef; 963x > struct work_struct work; > enum led_brightness brightness; > struct led_classdev led_cdev; > - int led_num; /* 0 .. 3 potentially */ > + int led_num; /* 0 .. 7 potentially */ > char name[32]; > }; > > @@ -54,24 +79,26 @@ static void pca9633_led_work(struct work_struct *work) > { > struct pca9633_led *pca9633 = container_of(work, > struct pca9633_led, work); > - u8 ledout = i2c_smbus_read_byte_data(pca9633->client, PCA9633_LEDOUT); > - int shift = 2 * pca9633->led_num; > + u8 ledout_addr = pca9633->chipdef->ledout_base + (pca9633->led_num / 4); > + u8 ledout; > + int shift = 2 * (pca9633->led_num % 4); > u8 mask = 0x3 << shift; > > + ledout = i2c_smbus_read_byte_data(pca9633->client, ledout_addr); > switch (pca9633->brightness) { > case LED_FULL: > - i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT, > + i2c_smbus_write_byte_data(pca9633->client, ledout_addr, > (ledout & ~mask) | (PCA9633_LED_ON << shift)); > break; > case LED_OFF: > - i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT, > + i2c_smbus_write_byte_data(pca9633->client, ledout_addr, > ledout & ~mask); > break; > default: > i2c_smbus_write_byte_data(pca9633->client, > PCA9633_PWM_BASE + pca9633->led_num, > pca9633->brightness); > - i2c_smbus_write_byte_data(pca9633->client, PCA9633_LEDOUT, > + i2c_smbus_write_byte_data(pca9633->client, ledout_addr, > (ledout & ~mask) | (PCA9633_LED_PWM << shift)); > break; > } > @@ -98,26 +125,30 @@ static int pca9633_probe(struct i2c_client *client, > { > struct pca9633_led *pca9633; > struct pca9633_platform_data *pdata; > + struct pca9633_chipdef *chip; > int i, err; > > + chip = &pca9633_chipdefs[id->driver_data]; > pdata = client->dev.platform_data; > > - if (pdata) { > - if (pdata->leds.num_leds <= 0 || pdata->leds.num_leds > 4) { > - dev_err(&client->dev, "board info must claim at most 4 LEDs"); > - return -EINVAL; > - } > + if (pdata && (pdata->leds.num_leds < 1 || > + pdata->leds.num_leds > chip->n_leds)) { > + dev_err(&client->dev, "board info must claim 1-%d LEDs", > + chip->n_leds); > + return -EINVAL; > } > > - pca9633 = devm_kzalloc(&client->dev, 4 * sizeof(*pca9633), GFP_KERNEL); > + pca9633 = devm_kzalloc(&client->dev, chip->n_leds * sizeof(*pca9633), > + GFP_KERNEL); > if (!pca9633) > return -ENOMEM; > > i2c_set_clientdata(client, pca9633); > > - for (i = 0; i < 4; i++) { > + for (i = 0; i < chip->n_leds; i++) { > pca9633[i].client = client; > pca9633[i].led_num = i; > + pca9633[i].chipdef = chip; > > /* Platform data can specify LED names and default triggers */ > if (pdata && i < pdata->leds.num_leds) { > @@ -151,7 +182,9 @@ static int pca9633_probe(struct i2c_client *client, > i2c_smbus_write_byte_data(client, PCA9633_MODE2, 0x01); > > /* Turn off LEDs */ > - i2c_smbus_write_byte_data(client, PCA9633_LEDOUT, 0x00); > + i2c_smbus_write_byte_data(client, chip->ledout_base, 0x00); > + if (chip->n_leds > 4) > + i2c_smbus_write_byte_data(client, chip->ledout_base + 1, 0x00); > > return 0; > > @@ -169,10 +202,11 @@ static int pca9633_remove(struct i2c_client *client) > struct pca9633_led *pca9633 = i2c_get_clientdata(client); > int i; > > - for (i = 0; i < 4; i++) { > - led_classdev_unregister(&pca9633[i].led_cdev); > - cancel_work_sync(&pca9633[i].work); > - } > + for (i = 0; i < pca9633->chipdef->n_leds; i++) > + if (pca9633[i].client != NULL) { > + led_classdev_unregister(&pca9633[i].led_cdev); > + cancel_work_sync(&pca9633[i].work); > + } > > return 0; > } > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/