Re: [PATCH 1/3] leds: add aw2013 driver
> Hi!
>
>> +#define AW2013_NAME "aw2013"
> That's not really useful define. Make it NAME? Drop it?
Will drop it as well as (unnecessary) lines it is used in.
>> +#define AW2013_TIME_STEP 130
> I'd add comment with /* units */.
Will add.
>> +#define STATE_OFF 0
>> +#define STATE_KEEP 1
>> +#define STATE_ON 2
> We should add enum into core for this...
>
>> +static int aw2013_chip_init(struct aw2013 *chip)
>> +{
>> +int i, ret;
>> +
>> +ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
>> +if (ret) {
>> +dev_err(>client->dev, "Failed to enable the chip: %d\n",
>> +ret);
>> +goto error;
>> +}
>> +
>> +for (i = 0; i < chip->num_leds; i++) {
>> +ret = regmap_update_bits(chip->regmap,
>> + AW2013_LCFG(chip->leds[i].num),
>> + AW2013_LCFG_IMAX_MASK,
>> + chip->leds[i].imax);
>> +if (ret) {
>> +dev_err(>client->dev,
>> +"Failed to set maximum current for led %d:
>> %d\n",
>> +chip->leds[i].num, ret);
>> +goto error;
>> +}
>> +}
>> +
>> +error:
>> +return ret;
>> +}
> No need for goto if you are just returning.
Will change it.
>> +static bool aw2013_chip_in_use(struct aw2013 *chip)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < chip->num_leds; i++)
>> +if (chip->leds[i].cdev.brightness)
>> +return true;
>> +
>> +return false;
>> +}
> How is this going to interact with ledstate == KEEP?
>
>> +static int aw2013_brightness_set(struct led_classdev *cdev,
>> + enum led_brightness brightness)
>> +{
>> +struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
>> +int ret, num;
>> +
>> +mutex_lock(>chip->mutex);
>> +
>> +if (aw2013_chip_in_use(led->chip)) {
>> +ret = aw2013_chip_enable(led->chip);
>> +if (ret)
>> +return ret;
>> +}
> You are returning with mutex held.
Will fix.
>> +/* Never on - just set to off */
>> +if (!*delay_on)
>> +return aw2013_brightness_set(>cdev, LED_OFF);
>> +
>> +/* Never off - just set to brightness */
>> +if (!*delay_off)
>> +return aw2013_brightness_set(>cdev, led->cdev.brightness);
> Is this dance neccessary? Should we do it in the core somewhere?
Right now blink_set() can be called with either delay_on or delay_off
being zero.
Passing zero into calculations I do later will result in garbage so
I'm trying to avoid it.
Core could probably handle situation where both are zero (This way
default values will be shared across all drivers) and if only
delay_on is zero it could disable led and the blink mode. (As if
brightness was set to 0)
In case where only delay_off is zero it's a bit more complicated
since driver should disable blinking but leave led on if it was
blinking already.
That also means that my current solution is a bit broken since changing
delay_off to zero while led is already blinking will call brightness_set
without clearing the mode bit so the led will still blink.
For now I will fix that and leave all those checks in place.
>> +} else {
>> +led->imax = 1; // 5mA
>> +dev_info(>dev,
>> + "DT property led-max-microamp is missing!\n");
>> +}
> Lets remove the exclamation mark.
Will do.
>> +led->num = source;
>> +led->chip = chip;
>> +led->fwnode = of_fwnode_handle(child);
>> +
>> +if (!of_property_read_string(child, "default-state", )) {
>> +if (!strcmp(str, "on"))
>> +led->default_state = STATE_ON;
>> +else if (!strcmp(str, "keep"))
>> +led->default_state = STATE_KEEP;
>> +else
>> +led->default_state = STATE_OFF;
>> +}
> We should really have something in core for this. Should we support
> arbitrary brightness there?
Not sure if there is good dt property for that.
>> +static void aw2013_read_current_state(struct aw2013 *chip)
>> +{
>> +int i, led_on;
>> +
>> +regmap_read(chip->regmap, AW2013_LCTR, _on);
>> +
>> +for (i = 0; i < chip->num_leds; i++) {
>> +if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
>> +chip->leds[i].cdev.brightness = LED_OFF;
>> +continue;
>> +}
>> +regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
>> +>leds[i].cdev.brightness);
>> +}
>> +}
>> +
>> +static void aw2013_init_default_state(struct aw2013_led *led)
>> +{
>> +switch (led->default_state) {
>> +case STATE_ON:
>> +led->cdev.brightness = LED_FULL;
>> +
Re: [PATCH 1/3] leds: add aw2013 driver
Hi Nikita,
On 5/4/20 6:29 PM, nikitos...@gmail.com wrote:
From: Nikita Travkin
This commit adds support for AWINIC AW2013 3-channel LED driver.
The chip supports 3 PWM channels and is controlled with I2C.
Signed-off-by: Nikita Travkin
---
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-aw2013.c | 481 +
3 files changed, 492 insertions(+)
create mode 100644 drivers/leds/leds-aw2013.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9cdc4cfc5d11..ed943140e1fd 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -103,6 +103,16 @@ config LEDS_AS3645A
controller. V4L2 flash API is provided as well if
CONFIG_V4L2_FLASH_API is enabled.
+config LEDS_AW2013
+ tristate "LED support for Awinic AW2013"
+ depends on LEDS_CLASS && I2C && OF
+ help
+ This option enables support for the AW2013 3-channel
+ LED driver.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-aw2013.
+
config LEDS_BCM6328
tristate "LED Support for Broadcom BCM6328"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d0dff504f108..d6b8a792c936 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_APU)+= leds-apu.o
obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
obj-$(CONFIG_LEDS_AS3645A)+= leds-as3645a.o
obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
+obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
obj-$(CONFIG_LEDS_BCM6328)+= leds-bcm6328.o
obj-$(CONFIG_LEDS_BCM6358)+= leds-bcm6358.o
obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
new file mode 100644
index ..371f128ab562
--- /dev/null
+++ b/drivers/leds/leds-aw2013.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Driver for Awinic AW2013 3-channel LED driver
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define AW2013_MAX_LEDS 3
+
+/* Reset and ID register */
+#define AW2013_RSTR 0x00
+#define AW2013_RSTR_RESET 0x55
+#define AW2013_RSTR_CHIP_ID 0x33
+
+/* Global control register */
+#define AW2013_GCR 0x01
+#define AW2013_GCR_ENABLE BIT(0)
+
+/* LED channel enable register */
+#define AW2013_LCTR 0x30
+#define AW2013_LCTR_LE(x) BIT((x))
+
+/* LED channel control registers */
+#define AW2013_LCFG(x) (0x31 + (x))
+#define AW2013_LCFG_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
+#define AW2013_LCFG_MD BIT(4)
+#define AW2013_LCFG_FI BIT(5)
+#define AW2013_LCFG_FO BIT(6)
+
+/* LED channel PWM registers */
+#define AW2013_REG_PWM(x) (0x34 + (x))
+
+/* LED channel timing registers */
+#define AW2013_LEDT0(x) (0x37 + (x) * 3)
+#define AW2013_LEDT0_T1(x) ((x) << 4) // Should be 0-7
+#define AW2013_LEDT0_T2(x) (x) // Should be 0-5
+
+#define AW2013_LEDT1(x) (0x38 + (x) * 3)
+#define AW2013_LEDT1_T3(x) ((x) << 4) // Should be 0-7
+#define AW2013_LEDT1_T4(x) (x) // Should be 0-7
+
+#define AW2013_LEDT2(x) (0x39 + (x) * 3)
+#define AW2013_LEDT2_T0(x) ((x) << 4) // Should be 0-8
+#define AW2013_LEDT2_REPEAT(x) (x) // Should be 0-15
+
+#define AW2013_REG_MAX 0x77
+
+#define AW2013_NAME "aw2013"
+
+#define AW2013_TIME_STEP 130
+
+#define STATE_OFF 0
+#define STATE_KEEP 1
+#define STATE_ON 2
+
+struct aw2013;
+
+struct aw2013_led {
+ struct aw2013 *chip;
+ struct fwnode_handle *fwnode;
+ struct led_classdev cdev;
+ u32 num;
+ unsigned int imax;
+ u32 default_state;
fwnode and default_state are only needed in probe() so it is suboptimal
to keep them afterwards. You could avoid that if you did DT parsing
and LED registration in one function in the same iteration step.
You can compare other LED drivers following this convention, they
usually name related functions "*probe_dt()".
+};
+
+struct aw2013 {
+ struct mutex mutex; /* held when writing to registers */
+ struct regulator *vcc_regulator;
+ struct i2c_client *client;
+ struct aw2013_led leds[AW2013_MAX_LEDS];
+ struct regmap *regmap;
+ int num_leds;
+ bool enabled;
+};
+
+static int aw2013_chip_init(struct aw2013 *chip)
+{
+ int i, ret;
+
+ ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
+ if (ret) {
+ dev_err(>client->dev, "Failed to enable the chip: %d\n",
+ ret);
+ goto error;
+ }
+
+ for (i = 0; i < chip->num_leds; i++) {
+ ret = regmap_update_bits(chip->regmap,
+AW2013_LCFG(chip->leds[i].num),
+AW2013_LCFG_IMAX_MASK,
+chip->leds[i].imax);
+ if (ret) {
+
Re: [PATCH 1/3] leds: add aw2013 driver
Hi!
> +#define AW2013_NAME "aw2013"
That's not really useful define. Make it NAME? Drop it?
> +#define AW2013_TIME_STEP 130
I'd add comment with /* units */.
> +#define STATE_OFF 0
> +#define STATE_KEEP 1
> +#define STATE_ON 2
We should add enum into core for this...
> +static int aw2013_chip_init(struct aw2013 *chip)
> +{
> + int i, ret;
> +
> + ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
> + if (ret) {
> + dev_err(>client->dev, "Failed to enable the chip: %d\n",
> + ret);
> + goto error;
> + }
> +
> + for (i = 0; i < chip->num_leds; i++) {
> + ret = regmap_update_bits(chip->regmap,
> + AW2013_LCFG(chip->leds[i].num),
> + AW2013_LCFG_IMAX_MASK,
> + chip->leds[i].imax);
> + if (ret) {
> + dev_err(>client->dev,
> + "Failed to set maximum current for led %d:
> %d\n",
> + chip->leds[i].num, ret);
> + goto error;
> + }
> + }
> +
> +error:
> + return ret;
> +}
No need for goto if you are just returning.
> +static bool aw2013_chip_in_use(struct aw2013 *chip)
> +{
> + int i;
> +
> + for (i = 0; i < chip->num_leds; i++)
> + if (chip->leds[i].cdev.brightness)
> + return true;
> +
> + return false;
> +}
How is this going to interact with ledstate == KEEP?
> +static int aw2013_brightness_set(struct led_classdev *cdev,
> + enum led_brightness brightness)
> +{
> + struct aw2013_led *led = container_of(cdev, struct aw2013_led, cdev);
> + int ret, num;
> +
> + mutex_lock(>chip->mutex);
> +
> + if (aw2013_chip_in_use(led->chip)) {
> + ret = aw2013_chip_enable(led->chip);
> + if (ret)
> + return ret;
> + }
You are returning with mutex held.
> + /* Never on - just set to off */
> + if (!*delay_on)
> + return aw2013_brightness_set(>cdev, LED_OFF);
> +
> + /* Never off - just set to brightness */
> + if (!*delay_off)
> + return aw2013_brightness_set(>cdev, led->cdev.brightness);
Is this dance neccessary? Should we do it in the core somewhere?
> + } else {
> + led->imax = 1; // 5mA
> + dev_info(>dev,
> + "DT property led-max-microamp is missing!\n");
> + }
Lets remove the exclamation mark.
> + led->num = source;
> + led->chip = chip;
> + led->fwnode = of_fwnode_handle(child);
> +
> + if (!of_property_read_string(child, "default-state", )) {
> + if (!strcmp(str, "on"))
> + led->default_state = STATE_ON;
> + else if (!strcmp(str, "keep"))
> + led->default_state = STATE_KEEP;
> + else
> + led->default_state = STATE_OFF;
> + }
We should really have something in core for this. Should we support
arbitrary brightness there?
> +static void aw2013_read_current_state(struct aw2013 *chip)
> +{
> + int i, led_on;
> +
> + regmap_read(chip->regmap, AW2013_LCTR, _on);
> +
> + for (i = 0; i < chip->num_leds; i++) {
> + if (!(led_on & AW2013_LCTR_LE(chip->leds[i].num))) {
> + chip->leds[i].cdev.brightness = LED_OFF;
> + continue;
> + }
> + regmap_read(chip->regmap, AW2013_REG_PWM(chip->leds[i].num),
> + >leds[i].cdev.brightness);
> + }
> +}
> +
> +static void aw2013_init_default_state(struct aw2013_led *led)
> +{
> + switch (led->default_state) {
> + case STATE_ON:
> + led->cdev.brightness = LED_FULL;
> + break;
> + case STATE_OFF:
> + led->cdev.brightness = LED_OFF;
> + } /* On keep - just set brightness that was retrieved previously */
> +
> + aw2013_brightness_set(>cdev, led->cdev.brightness);
> +}
Aha; I guess this makes "keeping" the state to work. Do you really
need that functionality?
Pretty nice driver, thanks.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: PGP signature
[PATCH 1/3] leds: add aw2013 driver
From: Nikita Travkin
This commit adds support for AWINIC AW2013 3-channel LED driver.
The chip supports 3 PWM channels and is controlled with I2C.
Signed-off-by: Nikita Travkin
---
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-aw2013.c | 481 +
3 files changed, 492 insertions(+)
create mode 100644 drivers/leds/leds-aw2013.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9cdc4cfc5d11..ed943140e1fd 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -103,6 +103,16 @@ config LEDS_AS3645A
controller. V4L2 flash API is provided as well if
CONFIG_V4L2_FLASH_API is enabled.
+config LEDS_AW2013
+ tristate "LED support for Awinic AW2013"
+ depends on LEDS_CLASS && I2C && OF
+ help
+ This option enables support for the AW2013 3-channel
+ LED driver.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-aw2013.
+
config LEDS_BCM6328
tristate "LED Support for Broadcom BCM6328"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d0dff504f108..d6b8a792c936 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_LEDS_APU)+= leds-apu.o
obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
obj-$(CONFIG_LEDS_ASIC3) += leds-asic3.o
+obj-$(CONFIG_LEDS_AW2013) += leds-aw2013.o
obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
obj-$(CONFIG_LEDS_BCM6358) += leds-bcm6358.o
obj-$(CONFIG_LEDS_BD2802) += leds-bd2802.o
diff --git a/drivers/leds/leds-aw2013.c b/drivers/leds/leds-aw2013.c
new file mode 100644
index ..371f128ab562
--- /dev/null
+++ b/drivers/leds/leds-aw2013.c
@@ -0,0 +1,481 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Driver for Awinic AW2013 3-channel LED driver
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define AW2013_MAX_LEDS 3
+
+/* Reset and ID register */
+#define AW2013_RSTR 0x00
+#define AW2013_RSTR_RESET 0x55
+#define AW2013_RSTR_CHIP_ID 0x33
+
+/* Global control register */
+#define AW2013_GCR 0x01
+#define AW2013_GCR_ENABLE BIT(0)
+
+/* LED channel enable register */
+#define AW2013_LCTR 0x30
+#define AW2013_LCTR_LE(x) BIT((x))
+
+/* LED channel control registers */
+#define AW2013_LCFG(x) (0x31 + (x))
+#define AW2013_LCFG_IMAX_MASK (BIT(0) | BIT(1)) // Should be 0-3
+#define AW2013_LCFG_MD BIT(4)
+#define AW2013_LCFG_FI BIT(5)
+#define AW2013_LCFG_FO BIT(6)
+
+/* LED channel PWM registers */
+#define AW2013_REG_PWM(x) (0x34 + (x))
+
+/* LED channel timing registers */
+#define AW2013_LEDT0(x) (0x37 + (x) * 3)
+#define AW2013_LEDT0_T1(x) ((x) << 4) // Should be 0-7
+#define AW2013_LEDT0_T2(x) (x) // Should be 0-5
+
+#define AW2013_LEDT1(x) (0x38 + (x) * 3)
+#define AW2013_LEDT1_T3(x) ((x) << 4) // Should be 0-7
+#define AW2013_LEDT1_T4(x) (x) // Should be 0-7
+
+#define AW2013_LEDT2(x) (0x39 + (x) * 3)
+#define AW2013_LEDT2_T0(x) ((x) << 4) // Should be 0-8
+#define AW2013_LEDT2_REPEAT(x) (x) // Should be 0-15
+
+#define AW2013_REG_MAX 0x77
+
+#define AW2013_NAME "aw2013"
+
+#define AW2013_TIME_STEP 130
+
+#define STATE_OFF 0
+#define STATE_KEEP 1
+#define STATE_ON 2
+
+struct aw2013;
+
+struct aw2013_led {
+ struct aw2013 *chip;
+ struct fwnode_handle *fwnode;
+ struct led_classdev cdev;
+ u32 num;
+ unsigned int imax;
+ u32 default_state;
+};
+
+struct aw2013 {
+ struct mutex mutex; /* held when writing to registers */
+ struct regulator *vcc_regulator;
+ struct i2c_client *client;
+ struct aw2013_led leds[AW2013_MAX_LEDS];
+ struct regmap *regmap;
+ int num_leds;
+ bool enabled;
+};
+
+static int aw2013_chip_init(struct aw2013 *chip)
+{
+ int i, ret;
+
+ ret = regmap_write(chip->regmap, AW2013_GCR, AW2013_GCR_ENABLE);
+ if (ret) {
+ dev_err(>client->dev, "Failed to enable the chip: %d\n",
+ ret);
+ goto error;
+ }
+
+ for (i = 0; i < chip->num_leds; i++) {
+ ret = regmap_update_bits(chip->regmap,
+AW2013_LCFG(chip->leds[i].num),
+AW2013_LCFG_IMAX_MASK,
+chip->leds[i].imax);
+ if (ret) {
+ dev_err(>client->dev,
+ "Failed to set maximum current for led %d:
%d\n",
+ chip->leds[i].num, ret);
+ goto error;
+ }
+ }
+
+error:
+ return ret;
+}
+
+static void aw2013_chip_disable(struct aw2013 *chip)
+{
+ int ret;
+
+ if (!chip->enabled)
+ return;
+
+
