Add Support for the PCA9634 chip. Simimart to the 9633, but with 8 outputs instead of 4.
Fix bug when 2 chips where present on the system, the ledclass will fail and the chip wont probe.
Protect ledout register with a mutex to support updates of more than leds at the same time
v2: Contains feedback from Peter Meerwald
Peter: Fix typo on commit message. Add bus number to name
Ricardo Ribalda Delgado (3):
leds-pca9633: Add support for PCA9634
leds-pca9633: Unique naming of the LEDs
leds-pca9633: Add mutex to the ledout register
drivers/leds/Kconfig | 7 +--
drivers/leds/leds-pca9633.c | 113 ++++++++++++++++++++++++++++++++-----------
2 files changed, 88 insertions(+), 32 deletions(-)
--
1.7.10.4
If there is more than one pca963x chips on the system and there are
some LEDs without platform_data names, the driver wont be able to
provide unique naming to them.
This will cause led_class_dev_register to fail, unregistering all the
LEDs of the chip.
This patch adds the i2c address to the name of the unnamed LEDs, making
them unique.
[ 555.346827] ------------[ cut here ]------------
[ 555.346844] WARNING: at /build/linux-voe0Su/linux-3.9.8/fs/sysfs/dir.c:536 sysfs_add_one+0x8b/0x9d()
[ 555.346847] Hardware name: QT5022
[ 555.346850] sysfs: cannot create duplicate filename '/class/leds/pca9633:6'
[ 555.346853] Modules linked in: qt5038_platform(O+) leds_pca9633(O) hid_generic ledtrig_default_on rfcomm bnep bluetooth binfmt_misc nfsd auth_rpcgss nfs_acl nfs lockd dns_resolver fscache sunrpc nls_utf8 nls_cp437 vfat fat loop fuse joydev hid_multitouch usbhid hid acpi_cpufreq mperf kvm_amd kvm evdev pn533 nfc arc4 microcode pcspkr efivars k10temp ath9k ath9k_common ath9k_hw ath fglrx(PO) mac80211 cfg80211 video rfkill processor thermal_sys sp5100_tco button i2c_piix4 ext4 crc16 jbd2 mbcache sg sd_mod crc_t10dif ahci libahci igb i2c_algo_bit i2c_core dca ptp pps_core ehci_pci ohci_hcd ehci_hcd libata usbcore usb_common scsi_mod [last unloaded: leds_pca963x]
[ 555.346940] Pid: 4766, comm: insmod Tainted: P W O 3.9-1-amd64 #1 Debian 3.9.8-1
[ 555.346943] Call Trace:
[ 555.346956] [<ffffffff8103d153>] ? warn_slowpath_common+0x76/0x8c
[ 555.346962] [<ffffffff8103d202>] ? warn_slowpath_fmt+0x47/0x49
[ 555.346968] [<ffffffff8116005d>] ? sysfs_pathname+0x3b/0x41
[ 555.346973] [<ffffffff81160767>] ? sysfs_add_one+0x8b/0x9d
[ 555.346978] [<ffffffff811610a4>] ? sysfs_do_create_link_sd+0xe8/0x174
[ 555.346985] [<ffffffff81279250>] ? device_add+0x243/0x5ab
[ 555.346991] [<ffffffff81060a16>] ? complete_all+0x31/0x40
[ 555.346998] [<ffffffff8104991a>] ? init_timer_key+0xc/0x56
[ 555.347004] [<ffffffff8127964c>] ? device_create_vargs+0x82/0xb6
[ 555.347009] [<ffffffff812796af>] ? device_create+0x2f/0x31
[ 555.347014] [<ffffffff81060add>] ? should_resched+0x5/0x23
[ 555.347021] [<ffffffff812a3a92>] ? led_classdev_register+0x24/0x103
[ 555.347028] [<ffffffffa09d01c0>] ? pca9633_probe+0x173/0x239 [leds_pca9633]
[ 555.347035] [<ffffffff8127b504>] ? __driver_attach+0x73/0x73
[ 555.347049] [<ffffffffa009dfc9>] ? i2c_device_probe+0x63/0x88 [i2c_core]
[ 555.347057] [<ffffffff8127b373>] ? driver_probe_device+0x92/0x1b0
[ 555.347064] [<ffffffff81279c5c>] ? bus_for_each_drv+0x43/0x7d
[ 555.347070] [<ffffffff8127b2af>] ? device_attach+0x68/0x83
[ 555.347078] [<ffffffff8127a990>] ? bus_probe_device+0x25/0x8d
[ 555.347083] [<ffffffff812793f7>] ? device_add+0x3ea/0x5ab
[ 555.347088] [<ffffffff81060a16>] ? complete_all+0x31/0x40
[ 555.347094] [<ffffffff8104991a>] ? init_timer_key+0xc/0x56
[ 555.347104] [<ffffffffa009d3a1>] ? i2c_new_device+0x10d/0x179 [i2c_core]
[ 555.347112] [<ffffffffa008f036>] ? qt5038_init+0x36/0x1000 [qt5038_platform]
[ 555.347119] [<ffffffffa008f000>] ? 0xffffffffa008efff
[ 555.347125] [<ffffffff8100209e>] ? do_one_initcall+0x74/0x128
[ 555.347131] [<ffffffffa008f000>] ? 0xffffffffa008efff
[ 555.347137] [<ffffffff810836f5>] ? load_module+0x1af7/0x1dfc
[ 555.347144] [<ffffffff810801c5>] ? free_notes_attrs+0x3c/0x3c
[ 555.347150] [<ffffffff81083a98>] ? sys_init_module+0x9e/0xab
[ 555.347157] [<ffffffff8138be29>] ? system_call_fastpath+0x16/0x1b
[ 555.347161] ---[ end trace ad00b85794e0de4d ]---
[ 555.347448] leds-pca9633: probe of 0-006b failed with error -17
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/leds/leds-pca9633.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/leds-pca9633.c b/drivers/leds/leds-pca9633.c
index 070358a..02766fb 100644
--- a/drivers/leds/leds-pca9633.c
+++ b/drivers/leds/leds-pca9633.c
@@ -159,10 +159,12 @@ static int pca9633_probe(struct i2c_client *client,
if (pdata->leds.leds[i].default_trigger)
pca9633[i].led_cdev.default_trigger =
pdata->leds.leds[i].default_trigger;
- } else {
- snprintf(pca9633[i].name, sizeof(pca9633[i].name),
- "pca9633:%d", i);
}
+ if (!pdata || i >= pdata->leds.num_leds ||
+ !pdata->leds.leds[i].name)
+ snprintf(pca9633[i].name, sizeof(pca9633[i].name),
+ "pca9633:%d:%.2x:%d", client->adapter->nr,
+ client->addr, i);
pca9633[i].led_cdev.name = pca9633[i].name;
pca9633[i].led_cdev.brightness_set = pca9633_led_set;
--
1.7.10.4
To update an LED a register has to be read, updated and writen. If
another LED whas been updated at the same time, this could lead into
wrong updates.
This patch addes a common mutex to all the leds of the same chip to
protect the ledout register.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/leds/leds-pca9633.c | 63 ++++++++++++++++++++++++++++---------------
1 file changed, 41 insertions(+), 22 deletions(-)
diff --git a/drivers/leds/leds-pca9633.c b/drivers/leds/leds-pca9633.c
index 02766fb..9bf33d8 100644
--- a/drivers/leds/leds-pca9633.c
+++ b/drivers/leds/leds-pca9633.c
@@ -65,9 +65,17 @@ static const struct i2c_device_id pca9633_id[] = {
};
MODULE_DEVICE_TABLE(i2c, pca9633_id);
-struct pca9633_led {
- struct i2c_client *client;
+struct pca9633_led;
+
+struct pca9633 {
struct pca9633_chipdef *chipdef;
+ struct mutex mutex;
+ struct i2c_client *client;
+ struct pca9633_led *leds;
+};
+
+struct pca9633_led {
+ struct pca9633 *chip;
struct work_struct work;
enum led_brightness brightness;
struct led_classdev led_cdev;
@@ -79,29 +87,32 @@ static void pca9633_led_work(struct work_struct *work)
{
struct pca9633_led *pca9633 = container_of(work,
struct pca9633_led, work);
- u8 ledout_addr = pca9633->chipdef->ledout_base + (pca9633->led_num / 4);
+ u8 ledout_addr = pca9633->chip->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);
+ mutex_lock(&pca9633->chip->mutex);
+ ledout = i2c_smbus_read_byte_data(pca9633->chip->client, ledout_addr);
switch (pca9633->brightness) {
case LED_FULL:
- i2c_smbus_write_byte_data(pca9633->client, ledout_addr,
+ i2c_smbus_write_byte_data(pca9633->chip->client, ledout_addr,
(ledout & ~mask) | (PCA9633_LED_ON << shift));
break;
case LED_OFF:
- i2c_smbus_write_byte_data(pca9633->client, ledout_addr,
+ i2c_smbus_write_byte_data(pca9633->chip->client, ledout_addr,
ledout & ~mask);
break;
default:
- i2c_smbus_write_byte_data(pca9633->client,
+ i2c_smbus_write_byte_data(pca9633->chip->client,
PCA9633_PWM_BASE + pca9633->led_num,
pca9633->brightness);
- i2c_smbus_write_byte_data(pca9633->client, ledout_addr,
+ i2c_smbus_write_byte_data(pca9633->chip->client, ledout_addr,
(ledout & ~mask) | (PCA9633_LED_PWM << shift));
break;
}
+ mutex_unlock(&pca9633->chip->mutex);
}
static void pca9633_led_set(struct led_classdev *led_cdev,
@@ -123,6 +134,7 @@ static void pca9633_led_set(struct led_classdev *led_cdev,
static int pca9633_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
+ struct pca9633 *pca9633_chip;
struct pca9633_led *pca9633;
struct pca9633_platform_data *pdata;
struct pca9633_chipdef *chip;
@@ -138,17 +150,30 @@ static int pca9633_probe(struct i2c_client *client,
return -EINVAL;
}
+ pca9633_chip = devm_kzalloc(&client->dev, sizeof(*pca9633_chip),
+ GFP_KERNEL);
+ if (!pca9633_chip)
+ return -ENOMEM;
pca9633 = devm_kzalloc(&client->dev, chip->n_leds * sizeof(*pca9633),
GFP_KERNEL);
if (!pca9633)
return -ENOMEM;
- i2c_set_clientdata(client, pca9633);
+ i2c_set_clientdata(client, pca9633_chip);
+
+ mutex_init(&pca9633_chip->mutex);
+ pca9633_chip->chipdef = chip;
+ pca9633_chip->client = client;
+ pca9633_chip->leds = pca9633;
+
+ /* Turn off LEDs by default*/
+ 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);
for (i = 0; i < chip->n_leds; i++) {
- pca9633[i].client = client;
pca9633[i].led_num = i;
- pca9633[i].chipdef = chip;
+ pca9633[i].chip = pca9633_chip;
/* Platform data can specify LED names and default triggers */
if (pdata && i < pdata->leds.num_leds) {
@@ -183,11 +208,6 @@ static int pca9633_probe(struct i2c_client *client,
if (pdata && pdata->outdrv == PCA9633_OPEN_DRAIN)
i2c_smbus_write_byte_data(client, PCA9633_MODE2, 0x01);
- /* Turn off LEDs */
- 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;
exit:
@@ -201,14 +221,13 @@ exit:
static int pca9633_remove(struct i2c_client *client)
{
- struct pca9633_led *pca9633 = i2c_get_clientdata(client);
+ struct pca9633 *pca9633 = i2c_get_clientdata(client);
int i;
- 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);
- }
+ for (i = 0; i < pca9633->chipdef->n_leds; i++) {
+ led_classdev_unregister(&pca9633->leds[i].led_cdev);
+ cancel_work_sync(&pca9633->leds[i].work);
+ }
return 0;
}
--
1.7.10.4
Add support for PCA9634 chip, which belongs to the same family as the
9633 but with support for 8 outputs instead of 4.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/leds/Kconfig | 7 ++--
drivers/leds/leds-pca9633.c | 74 +++++++++++++++++++++++++++++++------------
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
- 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 <[email protected]>
+ * Author: Ricardo Ribalda <[email protected]>
*
* 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 {
+ pca9633,
+ pca9634,
+};
+
+struct pca9633_chipdef {
+ u8 ledout_base;
+ int n_leds;
+};
+
+static struct pca9633_chipdef pca9633_chipdefs[] = {
+ [pca9633] = {
+ .ledout_base = 0x8,
+ .n_leds = 4,
+ },
+ [pca9634] = {
+ .ledout_base = 0xc,
+ .n_leds = 8,
+ },
+};
static const struct i2c_device_id pca9633_id[] = {
- { "pca9633", 0 },
+ { "pca9633", pca9633 },
+ { "pca9634", pca9634 },
{ }
};
MODULE_DEVICE_TABLE(i2c, pca9633_id);
struct pca9633_led {
struct i2c_client *client;
+ struct pca9633_chipdef *chipdef;
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
On Thu, Aug 8, 2013 at 9:45 AM, Ricardo Ribalda Delgado
<[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + * Author: Ricardo Ribalda <[email protected]>
> *
> * 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
>
On Thu, Aug 8, 2013 at 9:45 AM, Ricardo Ribalda Delgado
<[email protected]> wrote:
> If there is more than one pca963x chips on the system and there are
> some LEDs without platform_data names, the driver wont be able to
> provide unique naming to them.
>
> This will cause led_class_dev_register to fail, unregistering all the
> LEDs of the chip.
>
> This patch adds the i2c address to the name of the unnamed LEDs, making
> them unique.
>
> [ 555.346827] ------------[ cut here ]------------
> [ 555.346844] WARNING: at /build/linux-voe0Su/linux-3.9.8/fs/sysfs/dir.c:536 sysfs_add_one+0x8b/0x9d()
> [ 555.346847] Hardware name: QT5022
> [ 555.346850] sysfs: cannot create duplicate filename '/class/leds/pca9633:6'
> [ 555.346853] Modules linked in: qt5038_platform(O+) leds_pca9633(O) hid_generic ledtrig_default_on rfcomm bnep bluetooth binfmt_misc nfsd auth_rpcgss nfs_acl nfs lockd dns_resolver fscache sunrpc nls_utf8 nls_cp437 vfat fat loop fuse joydev hid_multitouch usbhid hid acpi_cpufreq mperf kvm_amd kvm evdev pn533 nfc arc4 microcode pcspkr efivars k10temp ath9k ath9k_common ath9k_hw ath fglrx(PO) mac80211 cfg80211 video rfkill processor thermal_sys sp5100_tco button i2c_piix4 ext4 crc16 jbd2 mbcache sg sd_mod crc_t10dif ahci libahci igb i2c_algo_bit i2c_core dca ptp pps_core ehci_pci ohci_hcd ehci_hcd libata usbcore usb_common scsi_mod [last unloaded: leds_pca963x]
> [ 555.346940] Pid: 4766, comm: insmod Tainted: P W O 3.9-1-amd64 #1 Debian 3.9.8-1
> [ 555.346943] Call Trace:
> [ 555.346956] [<ffffffff8103d153>] ? warn_slowpath_common+0x76/0x8c
> [ 555.346962] [<ffffffff8103d202>] ? warn_slowpath_fmt+0x47/0x49
> [ 555.346968] [<ffffffff8116005d>] ? sysfs_pathname+0x3b/0x41
> [ 555.346973] [<ffffffff81160767>] ? sysfs_add_one+0x8b/0x9d
> [ 555.346978] [<ffffffff811610a4>] ? sysfs_do_create_link_sd+0xe8/0x174
> [ 555.346985] [<ffffffff81279250>] ? device_add+0x243/0x5ab
> [ 555.346991] [<ffffffff81060a16>] ? complete_all+0x31/0x40
> [ 555.346998] [<ffffffff8104991a>] ? init_timer_key+0xc/0x56
> [ 555.347004] [<ffffffff8127964c>] ? device_create_vargs+0x82/0xb6
> [ 555.347009] [<ffffffff812796af>] ? device_create+0x2f/0x31
> [ 555.347014] [<ffffffff81060add>] ? should_resched+0x5/0x23
> [ 555.347021] [<ffffffff812a3a92>] ? led_classdev_register+0x24/0x103
> [ 555.347028] [<ffffffffa09d01c0>] ? pca9633_probe+0x173/0x239 [leds_pca9633]
> [ 555.347035] [<ffffffff8127b504>] ? __driver_attach+0x73/0x73
> [ 555.347049] [<ffffffffa009dfc9>] ? i2c_device_probe+0x63/0x88 [i2c_core]
> [ 555.347057] [<ffffffff8127b373>] ? driver_probe_device+0x92/0x1b0
> [ 555.347064] [<ffffffff81279c5c>] ? bus_for_each_drv+0x43/0x7d
> [ 555.347070] [<ffffffff8127b2af>] ? device_attach+0x68/0x83
> [ 555.347078] [<ffffffff8127a990>] ? bus_probe_device+0x25/0x8d
> [ 555.347083] [<ffffffff812793f7>] ? device_add+0x3ea/0x5ab
> [ 555.347088] [<ffffffff81060a16>] ? complete_all+0x31/0x40
> [ 555.347094] [<ffffffff8104991a>] ? init_timer_key+0xc/0x56
> [ 555.347104] [<ffffffffa009d3a1>] ? i2c_new_device+0x10d/0x179 [i2c_core]
> [ 555.347112] [<ffffffffa008f036>] ? qt5038_init+0x36/0x1000 [qt5038_platform]
> [ 555.347119] [<ffffffffa008f000>] ? 0xffffffffa008efff
> [ 555.347125] [<ffffffff8100209e>] ? do_one_initcall+0x74/0x128
> [ 555.347131] [<ffffffffa008f000>] ? 0xffffffffa008efff
> [ 555.347137] [<ffffffff810836f5>] ? load_module+0x1af7/0x1dfc
> [ 555.347144] [<ffffffff810801c5>] ? free_notes_attrs+0x3c/0x3c
> [ 555.347150] [<ffffffff81083a98>] ? sys_init_module+0x9e/0xab
> [ 555.347157] [<ffffffff8138be29>] ? system_call_fastpath+0x16/0x1b
> [ 555.347161] ---[ end trace ad00b85794e0de4d ]---
> [ 555.347448] leds-pca9633: probe of 0-006b failed with error -17
>
This one looks good to me. Please rebase this after rework PATCH 1.
Thanks,
-Bryan
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/leds/leds-pca9633.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-pca9633.c b/drivers/leds/leds-pca9633.c
> index 070358a..02766fb 100644
> --- a/drivers/leds/leds-pca9633.c
> +++ b/drivers/leds/leds-pca9633.c
> @@ -159,10 +159,12 @@ static int pca9633_probe(struct i2c_client *client,
> if (pdata->leds.leds[i].default_trigger)
> pca9633[i].led_cdev.default_trigger =
> pdata->leds.leds[i].default_trigger;
> - } else {
> - snprintf(pca9633[i].name, sizeof(pca9633[i].name),
> - "pca9633:%d", i);
> }
> + if (!pdata || i >= pdata->leds.num_leds ||
> + !pdata->leds.leds[i].name)
> + snprintf(pca9633[i].name, sizeof(pca9633[i].name),
> + "pca9633:%d:%.2x:%d", client->adapter->nr,
> + client->addr, i);
>
> pca9633[i].led_cdev.name = pca9633[i].name;
> pca9633[i].led_cdev.brightness_set = pca9633_led_set;
> --
> 1.7.10.4
>
Hello,
> > 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.
me too :)
> What about just rename the whole file to leds-pca963x.c. And rename
> some pca9633 to pca963x in the driver.
there are other, similar I2C LED driver chips which might be
handled with the current pca9633 driver, e.g. the pca9685 (which is
supported under pwm/ by the way)
people have argued that the numbering scheme of chips is hard to
predict; hence, the driver name should be determined by the first
device supported to avoid subsequent renaming -- but I have no strong
feelings about this
regards, p.
--
Peter Meerwald
+43-664-2444418 (mobile)
On Thu, Aug 8, 2013 at 3:36 PM, Peter Meerwald
<[email protected]> wrote:
> Hello,
>
>> > 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.
>
> me too :)
>
>> What about just rename the whole file to leds-pca963x.c. And rename
>> some pca9633 to pca963x in the driver.
>
> there are other, similar I2C LED driver chips which might be
> handled with the current pca9633 driver, e.g. the pca9685 (which is
> supported under pwm/ by the way)
>
> people have argued that the numbering scheme of chips is hard to
> predict; hence, the driver name should be determined by the first
> device supported to avoid subsequent renaming -- but I have no strong
> feelings about this
>
Giving a more generic and meaningful name of this driver should be a
easier for people to understand. So if pca9685 is coming, we can use
leds-pca96xx.c and pca96xx for this pca96xx chip family, as long as
they can share this driver code.
Please take a look at Milo did in leds-lp55xx drivers.
Thanks a lot,
-Bryan
Hello Bryan
I understand your concerns, but I think that it is more practical to
keep the old names.
First of all there will be some defconfig that we will break with the
new names, the same will happen with the platform data. I know that we
don't have to support out of tree systems, but it wont hurt to make
their live easier.
The patch produced when we rename a file is as big as the file itself,
so it is more difficult to get good reviews, compared to a 10 lines
patch.
Unless the manufacturer makes a statement, there is no way to know if
there will be a pca96339 that will be a completely different chip, and
that will led to many errors.
Finally, there are many examples of files named as the first device
supported, even on the led infrastructure (ie. LEDS_DA9052)
All that said, I have added a new patch to the series that does the
renaming :), therefore we can review the addition 96334 separately
than the other improvements and if we decide to have a generic name we
will have it.
Thanks for your review!
On Fri, Aug 9, 2013 at 12:49 AM, Bryan Wu <[email protected]> wrote:
> On Thu, Aug 8, 2013 at 3:36 PM, Peter Meerwald
> <[email protected]> wrote:
>> Hello,
>>
>>> > 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.
>>
>> me too :)
>>
>>> What about just rename the whole file to leds-pca963x.c. And rename
>>> some pca9633 to pca963x in the driver.
>>
>> there are other, similar I2C LED driver chips which might be
>> handled with the current pca9633 driver, e.g. the pca9685 (which is
>> supported under pwm/ by the way)
>>
>> people have argued that the numbering scheme of chips is hard to
>> predict; hence, the driver name should be determined by the first
>> device supported to avoid subsequent renaming -- but I have no strong
>> feelings about this
>>
>
> Giving a more generic and meaningful name of this driver should be a
> easier for people to understand. So if pca9685 is coming, we can use
> leds-pca96xx.c and pca96xx for this pca96xx chip family, as long as
> they can share this driver code.
>
> Please take a look at Milo did in leds-lp55xx drivers.
>
> Thanks a lot,
> -Bryan
--
Ricardo Ribalda