From mboxrd@z Thu Jan 1 00:00:00 1970 From: Phil Reid Subject: Re: [PATCH 1/1] leds: pca9532: Add device tree binding Date: Thu, 7 Apr 2016 14:12:51 +0800 Message-ID: <5705FA63.4090109@electromag.com.au> References: <1459912250-50878-1-git-send-email-preid@electromag.com.au> <1459912250-50878-2-git-send-email-preid@electromag.com.au> <5704C130.80001@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from anchovy2.45ru.net.au ([203.30.46.146]:36512 "EHLO anchovy.45ru.net.au" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750784AbcDGGTt (ORCPT ); Thu, 7 Apr 2016 02:19:49 -0400 In-Reply-To: <5704C130.80001@gmail.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Jacek Anaszewski , robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, riku.voipio@iki.fi, rpurdie@rpsys.net, j.anaszewski@samsung.com, devicetree@vger.kernel.org, linux-leds@vger.kernel.org G'day Jacek, Thanks for the feedback. On 6/04/2016 3:56 PM, Jacek Anaszewski wrote: > Hi Phil, > > Thanks for the patch. Please find my comments below. > > On 04/06/2016 05:10 AM, Phil Reid wrote: >> This patch adds basic device tree support for the pca9532 LEDs. >> >> Signed-off-by: Phil Reid >> --- >> .../devicetree/bindings/leds/leds-pca9532.txt | 32 +++++++++++ >> drivers/leds/leds-pca9532.c | 63 ++++++++++++++++++++-- >> include/dt-bindings/leds/leds-pca9532.h | 18 +++++++ >> include/linux/leds-pca9532.h | 8 ++- >> 4 files changed, 112 insertions(+), 9 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/leds/leds-pca9532.txt >> create mode 100644 include/dt-bindings/leds/leds-pca9532.h >> >> diff --git a/Documentation/devicetree/bindings/leds/leds-pca9532.txt b/Documentation/devicetree/bindings/leds/leds-pca9532.txt >> new file mode 100644 >> index 0000000..b48c223 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/leds/leds-pca9532.txt >> @@ -0,0 +1,32 @@ >> +*NXP - pca9532 PWM LED Driver >> + >> +The PCA9532 family is SMBus I/O expander optimized for dimming LEDs. >> +The PWM support 256 steps. >> + >> +Required properties: >> + - compatible: >> + "nxp,pca9530" >> + "nxp,pca9531" >> + "nxp,pca9532" >> + "nxp,pca9533" >> + - reg - I2C slave address >> + >> +Each led is represented as a sub-node of the nxp,pca9530. >> + >> +LED sub-node properties: >> +- type: Output configuration >> + 0 = NONE, 1 = LED, 2 = N2100_BEEP, 3 = GPIO >> + >> +Example: >> + >> + ledBL: pca9530@60 { > > I think that ledBL label is here by mistake. It looks like this > label's purpose was to describe LED color (BL -> blue), but > this info should be conveyed by child node name, per which > LED class devices are created. I'll clean this up. > >> + compatible = "nxp,pca9530"; >> + reg = <0x60>; >> + >> + led0 { > > i.e. here. Moreover there is established LED device naming > convention (see Documentation/leds/leds-class.txt): > > devicename:colour:function Ok. > > >> + type = ; > > Please add "#include " after > "Example:" above. Ok. > >> + }; >> + }; >> + >> +For more product information please see the link below: >> +http://nxp.com/documents/data_sheet/PCA9532.pdf >> diff --git a/drivers/leds/leds-pca9532.c b/drivers/leds/leds-pca9532.c >> index e3d3b1a..8694c83 100644 >> --- a/drivers/leds/leds-pca9532.c >> +++ b/drivers/leds/leds-pca9532.c >> @@ -21,6 +21,8 @@ >> #include >> #include >> #include >> +#include >> +#include >> >> /* m = num_leds*/ >> #define PCA9532_REG_INPUT(i) ((i) >> 3) >> @@ -86,9 +88,22 @@ static const struct pca9532_chip_info pca9532_chip_info_tbl[] = { >> }, >> }; >> >> +#ifdef CONFIG_OF >> +static const struct of_device_id of_pca9532_leds_match[] = { >> + { .compatible = "nxp,pca9530", .data = (void *)pca9530 }, >> + { .compatible = "nxp,pca9531", .data = (void *)pca9531 }, >> + { .compatible = "nxp,pca9532", .data = (void *)pca9532 }, >> + { .compatible = "nxp,pca9533", .data = (void *)pca9533 }, >> + {}, >> +}; >> + >> +MODULE_DEVICE_TABLE(of, of_pca9532_leds_match); >> +#endif >> + >> static struct i2c_driver pca9532_driver = { >> .driver = { >> .name = "leds-pca953x", >> + .of_match_table = of_match_ptr(of_pca9532_leds_match), >> }, >> .probe = pca9532_probe, >> .remove = pca9532_remove, >> @@ -432,15 +447,55 @@ exit: >> return err; >> } >> >> +struct pca9532_platform_data *pca9532_of_populate_pdata(struct device *dev, >> + struct device_node *np) >> +{ >> + struct pca9532_platform_data *pdata; >> + struct device_node *child; >> + int devid = (int)of_match_device(of_pca9532_leds_match, dev)->data; >> + int maxleds = pca9532_chip_info_tbl[devid].num_leds; >> + int i = 0; >> + >> + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); >> + if (!pdata) >> + return ERR_PTR(-ENOMEM); >> + >> + for_each_child_of_node(np, child) { >> + of_property_read_string(child, "name", &pdata->leds[i].name); > > Please use "label" property (see Documentation/devicetree/binding > /leds/common.txt). If not present use child node name. You can refer > to other LED class devices. Ok. > >> + of_property_read_u32(child, "type", &pdata->leds[i].type); >> + if (!pdata->leds[i].name) >> + pdata->leds[i].name = "?"; >> + if (i >= maxleds) >> + break; > > You have to call of_node_put if breaking this loop. Ok. Also I forgot to increment i, my system only has one LED on the PCA9530. > >> + } >> + >> + return pdata; >> +} >> + >> static int pca9532_probe(struct i2c_client *client, >> const struct i2c_device_id *id) >> { >> + int devid; >> struct pca9532_data *data = i2c_get_clientdata(client); >> struct pca9532_platform_data *pca9532_pdata = >> dev_get_platdata(&client->dev); >> - >> - if (!pca9532_pdata) >> - return -EIO; >> + struct device_node *np = client->dev.of_node; >> + >> + if (!pca9532_pdata) { >> + if (np) { >> + pca9532_pdata = >> + pca9532_of_populate_pdata(&client->dev, np); >> + if (IS_ERR(pca9532_pdata)) >> + return PTR_ERR(pca9532_pdata); >> + } else { >> + dev_err(&client->dev, "no platform data\n"); >> + return -EINVAL; >> + } >> + devid = (int)of_match_device( >> + of_pca9532_leds_match, &client->dev)->data; >> + } else { >> + devid = id->driver_data; >> + } >> >> if (!i2c_check_functionality(client->adapter, >> I2C_FUNC_SMBUS_BYTE_DATA)) >> @@ -450,7 +505,7 @@ static int pca9532_probe(struct i2c_client *client, >> if (!data) >> return -ENOMEM; >> >> - data->chip_info = &pca9532_chip_info_tbl[id->driver_data]; >> + data->chip_info = &pca9532_chip_info_tbl[devid]; >> >> dev_info(&client->dev, "setting platform data\n"); >> i2c_set_clientdata(client, data); >> diff --git a/include/dt-bindings/leds/leds-pca9532.h b/include/dt-bindings/leds/leds-pca9532.h >> new file mode 100644 >> index 0000000..4d917aa >> --- /dev/null >> +++ b/include/dt-bindings/leds/leds-pca9532.h >> @@ -0,0 +1,18 @@ >> +/* >> + * This header provides constants for pca9532 LED bindings. >> + * >> + * This file is licensed under the terms of the GNU General Public >> + * License version 2. This program is licensed "as is" without any >> + * warranty of any kind, whether express or implied. >> + */ >> + >> +#ifndef _DT_BINDINGS_LEDS_PCA9532_H >> +#define _DT_BINDINGS_LEDS_PCA9532_H >> + >> +#define PCA9532_TYPE_NONE 0 >> +#define PCA9532_TYPE_LED 1 >> +#define PCA9532_TYPE_N2100_BEEP 2 >> +#define PCA9532_TYPE_GPIO 3 >> +#define PCA9532_LED_TIMER2 4 >> + >> +#endif /* _DT_BINDINGS_LEDS_PCA9532_H */ >> diff --git a/include/linux/leds-pca9532.h b/include/linux/leds-pca9532.h >> index b8d6fff..4970f51 100644 >> --- a/include/linux/leds-pca9532.h >> +++ b/include/linux/leds-pca9532.h >> @@ -16,6 +16,7 @@ >> >> #include >> #include >> +#include >> >> enum pca9532_state { >> PCA9532_OFF = 0x0, >> @@ -24,16 +25,13 @@ enum pca9532_state { >> PCA9532_PWM1 = 0x3 >> }; >> >> -enum pca9532_type { PCA9532_TYPE_NONE, PCA9532_TYPE_LED, >> - PCA9532_TYPE_N2100_BEEP, PCA9532_TYPE_GPIO }; >> - >> struct pca9532_led { >> u8 id; >> struct i2c_client *client; >> - char *name; >> + const char *name; >> struct led_classdev ldev; >> struct work_struct work; >> - enum pca9532_type type; >> + u32 type; >> enum pca9532_state state; >> }; >> >> > -- Regards Phil Reid