* [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020
@ 2020-03-22 7:41 Lubomir Rintel
2020-04-03 19:37 ` Dan Murphy
0 siblings, 1 reply; 5+ messages in thread
From: Lubomir Rintel @ 2020-03-22 7:41 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Pavel Machek, Dan Murphy, linux-kernel, linux-leds, Lubomir Rintel
This adds support for controlling the LEDs attached to the Embedded
Controller on a Dell Wyse 3020 "Ariel" board.
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
Changes since v2:
- Hopefully sending out the correct patch this time...
Changes since v1:
- Reduce code duplication with a loop
- Drop "ariel:" prefix from led names
- Do not print a message after a successful probe
---
drivers/leds/Kconfig | 11 ++++
drivers/leds/Makefile | 1 +
drivers/leds/leds-ariel.c | 133 ++++++++++++++++++++++++++++++++++++++
3 files changed, 145 insertions(+)
create mode 100644 drivers/leds/leds-ariel.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index d82f1dea37111..66424ee54cc01 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -83,6 +83,17 @@ config LEDS_APU
To compile this driver as a module, choose M here: the
module will be called leds-apu.
+config LEDS_ARIEL
+ tristate "Dell Wyse 3020 status LED support"
+ depends on LEDS_CLASS
+ depends on (MACH_MMP3_DT && MFD_ENE_KB3930) || COMPILE_TEST
+ help
+ This driver adds support for controlling the front panel status
+ LEDs on Dell Wyse 3020 (Ariel) board via the KB3930 Embedded
+ Controller.
+
+ Say Y to if your machine is a Dell Wyse 3020 thin client.
+
config LEDS_AS3645A
tristate "AS3645A and LM3555 LED flash controllers support"
depends on I2C && LEDS_CLASS_FLASH
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index d7e1107753fb1..bf3b22038d113 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o
obj-$(CONFIG_LEDS_AAT1290) += leds-aat1290.o
obj-$(CONFIG_LEDS_APU) += leds-apu.o
+obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
diff --git a/drivers/leds/leds-ariel.c b/drivers/leds/leds-ariel.c
new file mode 100644
index 0000000000000..8fc56722e12f4
--- /dev/null
+++ b/drivers/leds/leds-ariel.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
+/*
+ * Dell Wyse 3020 a.k.a. "Ariel" Embedded Controller LED Driver
+ *
+ * Copyright (C) 2020 Lubomir Rintel
+ */
+
+#include <linux/module.h>
+#include <linux/leds.h>
+#include <linux/regmap.h>
+#include <linux/of_platform.h>
+
+enum ec_index {
+ EC_BLUE_LED = 0x01,
+ EC_AMBER_LED = 0x02,
+ EC_GREEN_LED = 0x03,
+};
+
+enum {
+ EC_LED_OFF = 0x00,
+ EC_LED_STILL = 0x01,
+ EC_LED_FADE = 0x02,
+ EC_LED_BLINK = 0x03,
+};
+
+struct ariel_led {
+ struct regmap *ec_ram;
+ enum ec_index ec_index;
+ struct led_classdev led_cdev;
+};
+
+#define led_cdev_to_ariel_led(c) container_of(c, struct ariel_led, led_cdev)
+
+static enum led_brightness ariel_led_get(struct led_classdev *led_cdev)
+{
+ struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
+ unsigned int led_status = 0;
+
+ if (regmap_read(led->ec_ram, led->ec_index, &led_status))
+ return LED_OFF;
+
+ if (led_status == EC_LED_STILL)
+ return LED_FULL;
+ else
+ return LED_OFF;
+}
+
+static void ariel_led_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
+
+ if (brightness == LED_OFF)
+ regmap_write(led->ec_ram, led->ec_index, EC_LED_OFF);
+ else
+ regmap_write(led->ec_ram, led->ec_index, EC_LED_STILL);
+}
+
+static int ariel_blink_set(struct led_classdev *led_cdev,
+ unsigned long *delay_on, unsigned long *delay_off)
+{
+ struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
+
+ if (*delay_on == 0 && *delay_off == 0)
+ return -EINVAL;
+
+ if (*delay_on == 0) {
+ regmap_write(led->ec_ram, led->ec_index, EC_LED_OFF);
+ } else if (*delay_off == 0) {
+ regmap_write(led->ec_ram, led->ec_index, EC_LED_STILL);
+ } else {
+ *delay_on = 500;
+ *delay_off = 500;
+ regmap_write(led->ec_ram, led->ec_index, EC_LED_BLINK);
+ }
+
+ return 0;
+}
+
+#define NLEDS 3
+
+static int ariel_led_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct ariel_led *leds;
+ struct regmap *ec_ram;
+ int ret;
+ int i;
+
+ leds = devm_kcalloc(dev, NLEDS, sizeof(*leds), GFP_KERNEL);
+ if (!leds)
+ return -ENOMEM;
+
+ ec_ram = dev_get_regmap(dev->parent, "ec_ram");
+ if (!ec_ram)
+ return -ENODEV;
+
+ leds[0].ec_index = EC_BLUE_LED;
+ leds[0].led_cdev.name = "blue:power",
+ leds[0].led_cdev.default_trigger = "default-on";
+
+ leds[1].ec_index = EC_AMBER_LED;
+ leds[1].led_cdev.name = "amber:status",
+
+ leds[2].ec_index = EC_GREEN_LED;
+ leds[2].led_cdev.name = "green:status",
+ leds[2].led_cdev.default_trigger = "default-on";
+
+ for (i = 0; i < NLEDS; i++) {
+ leds[0].ec_ram = ec_ram;
+ leds[0].led_cdev.brightness_get = ariel_led_get;
+ leds[0].led_cdev.brightness_set = ariel_led_set;
+ leds[0].led_cdev.blink_set = ariel_blink_set;
+
+ ret = devm_led_classdev_register(dev, &leds[0].led_cdev);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct platform_driver ariel_led_driver = {
+ .probe = ariel_led_probe,
+ .driver = {
+ .name = "dell-wyse-ariel-led",
+ },
+};
+module_platform_driver(ariel_led_driver);
+
+MODULE_AUTHOR("Lubomir Rintel <lkundrak@v3.sk>");
+MODULE_DESCRIPTION("Dell Wyse 3020 Status LEDs Driver");
+MODULE_LICENSE("Dual BSD/GPL");
--
2.26.0.rc2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020
2020-03-22 7:41 [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020 Lubomir Rintel
@ 2020-04-03 19:37 ` Dan Murphy
2020-04-06 20:12 ` Pavel Machek
2020-04-17 9:58 ` Lubomir Rintel
0 siblings, 2 replies; 5+ messages in thread
From: Dan Murphy @ 2020-04-03 19:37 UTC (permalink / raw)
To: Lubomir Rintel, Jacek Anaszewski; +Cc: Pavel Machek, linux-kernel, linux-leds
Lubomir
On 3/22/20 2:41 AM, Lubomir Rintel wrote:
> This adds support for controlling the LEDs attached to the Embedded
> Controller on a Dell Wyse 3020 "Ariel" board.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v2:
> - Hopefully sending out the correct patch this time...
>
> Changes since v1:
> - Reduce code duplication with a loop
> - Drop "ariel:" prefix from led names
> - Do not print a message after a successful probe
> ---
> drivers/leds/Kconfig | 11 ++++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-ariel.c | 133 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 145 insertions(+)
> create mode 100644 drivers/leds/leds-ariel.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index d82f1dea37111..66424ee54cc01 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -83,6 +83,17 @@ config LEDS_APU
> To compile this driver as a module, choose M here: the
> module will be called leds-apu.
>
> +config LEDS_ARIEL
> + tristate "Dell Wyse 3020 status LED support"
> + depends on LEDS_CLASS
> + depends on (MACH_MMP3_DT && MFD_ENE_KB3930) || COMPILE_TEST
> + help
> + This driver adds support for controlling the front panel status
> + LEDs on Dell Wyse 3020 (Ariel) board via the KB3930 Embedded
> + Controller.
> +
> + Say Y to if your machine is a Dell Wyse 3020 thin client.
> +
> config LEDS_AS3645A
> tristate "AS3645A and LM3555 LED flash controllers support"
> depends on I2C && LEDS_CLASS_FLASH
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index d7e1107753fb1..bf3b22038d113 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
> obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o
> obj-$(CONFIG_LEDS_AAT1290) += leds-aat1290.o
> obj-$(CONFIG_LEDS_APU) += leds-apu.o
> +obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
> obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> diff --git a/drivers/leds/leds-ariel.c b/drivers/leds/leds-ariel.c
> new file mode 100644
> index 0000000000000..8fc56722e12f4
> --- /dev/null
> +++ b/drivers/leds/leds-ariel.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
> +/*
> + * Dell Wyse 3020 a.k.a. "Ariel" Embedded Controller LED Driver
> + *
> + * Copyright (C) 2020 Lubomir Rintel
> + */
> +
> +#include <linux/module.h>
> +#include <linux/leds.h>
> +#include <linux/regmap.h>
> +#include <linux/of_platform.h>
> +
> +enum ec_index {
> + EC_BLUE_LED = 0x01,
> + EC_AMBER_LED = 0x02,
Defining the value after the 0x0 is unnecessary as enums are incremental
only the first value needs to be defined if the following values are in
numerical order
Can these also be #defined instead of an enum? Not requesting them to
be just wondering about the design decision here.
> + EC_GREEN_LED = 0x03,
> +};
> +
> +enum {
> + EC_LED_OFF = 0x00,
> + EC_LED_STILL = 0x01,
Same comment as above
> + EC_LED_FADE = 0x02,
> + EC_LED_BLINK = 0x03,
> +};
> +
> +struct ariel_led {
> + struct regmap *ec_ram;
> + enum ec_index ec_index;
> + struct led_classdev led_cdev;
> +};
> +
> +#define led_cdev_to_ariel_led(c) container_of(c, struct ariel_led, led_cdev)
> +
> +static enum led_brightness ariel_led_get(struct led_classdev *led_cdev)
> +{
> + struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
> + unsigned int led_status = 0;
> +
> + if (regmap_read(led->ec_ram, led->ec_index, &led_status))
> + return LED_OFF;
> +
> + if (led_status == EC_LED_STILL)
> + return LED_FULL;
> + else
else is not needed here
> + return LED_OFF;
> +}
> +
> +static void ariel_led_set(struct led_classdev *led_cdev,
> + enum led_brightness brightness)
> +{
> + struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
> +
> + if (brightness == LED_OFF)
> + regmap_write(led->ec_ram, led->ec_index, EC_LED_OFF);
> + else
> + regmap_write(led->ec_ram, led->ec_index, EC_LED_STILL);
> +}
> +
> +static int ariel_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on, unsigned long *delay_off)
> +{
> + struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
> +
> + if (*delay_on == 0 && *delay_off == 0)
> + return -EINVAL;
> +
> + if (*delay_on == 0) {
> + regmap_write(led->ec_ram, led->ec_index, EC_LED_OFF);
> + } else if (*delay_off == 0) {
> + regmap_write(led->ec_ram, led->ec_index, EC_LED_STILL);
> + } else {
> + *delay_on = 500;
> + *delay_off = 500;
> + regmap_write(led->ec_ram, led->ec_index, EC_LED_BLINK);
> + }
> +
> + return 0;
> +}
> +
> +#define NLEDS 3
This define needs to be more unique.
Something like EC_NLEDS or EC_NUM_LEDS and should be moved to the top of
the file under the #includes
> +
> +static int ariel_led_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct ariel_led *leds;
> + struct regmap *ec_ram;
> + int ret;
> + int i;
> +
> + leds = devm_kcalloc(dev, NLEDS, sizeof(*leds), GFP_KERNEL);
> + if (!leds)
> + return -ENOMEM;
> +
> + ec_ram = dev_get_regmap(dev->parent, "ec_ram");
Maybe this should be checked before memory is allocated.
> + if (!ec_ram)
> + return -ENODEV;
> +
> + leds[0].ec_index = EC_BLUE_LED;
> + leds[0].led_cdev.name = "blue:power",
> + leds[0].led_cdev.default_trigger = "default-on";
> +
> + leds[1].ec_index = EC_AMBER_LED;
> + leds[1].led_cdev.name = "amber:status",
> +
> + leds[2].ec_index = EC_GREEN_LED;
> + leds[2].led_cdev.name = "green:status",
> + leds[2].led_cdev.default_trigger = "default-on";
> +
> + for (i = 0; i < NLEDS; i++) {
I don't understand this loop. i is incremented but never used.
should the below be leds[i]?
> + leds[0].ec_ram = ec_ram;
> + leds[0].led_cdev.brightness_get = ariel_led_get;
> + leds[0].led_cdev.brightness_set = ariel_led_set;
> + leds[0].led_cdev.blink_set = ariel_blink_set;
> +
> + ret = devm_led_classdev_register(dev, &leds[0].led_cdev);
> + if (ret)
> + return ret;
> + }
> +
Dan
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020
2020-04-03 19:37 ` Dan Murphy
@ 2020-04-06 20:12 ` Pavel Machek
2020-04-17 9:58 ` Lubomir Rintel
1 sibling, 0 replies; 5+ messages in thread
From: Pavel Machek @ 2020-04-06 20:12 UTC (permalink / raw)
To: Dan Murphy; +Cc: Lubomir Rintel, Jacek Anaszewski, linux-kernel, linux-leds
Hi!
> >+enum ec_index {
> >+ EC_BLUE_LED = 0x01,
> >+ EC_AMBER_LED = 0x02,
>
> Defining the value after the 0x0 is unnecessary as enums are incremental
> only the first value needs to be defined if the following values are in
> numerical order
>
> Can these also be #defined instead of an enum??? Not requesting them to be
> just wondering about the design decision here.
enum is okay here.
> >+ EC_GREEN_LED = 0x03,
> >+};
> >+
> >+enum {
> >+ EC_LED_OFF = 0x00,
> >+ EC_LED_STILL = 0x01,
> Same comment as above
> >+ EC_LED_FADE = 0x02,
> >+ EC_LED_BLINK = 0x03,
> >+};
If the values are shared with hardware (and they are), making them explicit is right thing
to do.
> >+#define NLEDS 3
>
> This define needs to be more unique.
Why?
> Something like EC_NLEDS or EC_NUM_LEDS and should be moved to the top of the file under
> the #includes
I'd do it that way, but I would not request new patch version just for that.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020
2020-04-03 19:37 ` Dan Murphy
2020-04-06 20:12 ` Pavel Machek
@ 2020-04-17 9:58 ` Lubomir Rintel
2020-04-24 9:18 ` Pavel Machek
1 sibling, 1 reply; 5+ messages in thread
From: Lubomir Rintel @ 2020-04-17 9:58 UTC (permalink / raw)
To: Dan Murphy; +Cc: Jacek Anaszewski, Pavel Machek, linux-kernel, linux-leds
On Fri, Apr 03, 2020 at 02:37:49PM -0500, Dan Murphy wrote:
> Lubomir
>
> On 3/22/20 2:41 AM, Lubomir Rintel wrote:
> > This adds support for controlling the LEDs attached to the Embedded
> > Controller on a Dell Wyse 3020 "Ariel" board.
> >
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> >
> > ---
> > Changes since v2:
> > - Hopefully sending out the correct patch this time...
> >
> > Changes since v1:
> > - Reduce code duplication with a loop
> > - Drop "ariel:" prefix from led names
> > - Do not print a message after a successful probe
> > ---
> > drivers/leds/Kconfig | 11 ++++
> > drivers/leds/Makefile | 1 +
> > drivers/leds/leds-ariel.c | 133 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 145 insertions(+)
> > create mode 100644 drivers/leds/leds-ariel.c
> >
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index d82f1dea37111..66424ee54cc01 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -83,6 +83,17 @@ config LEDS_APU
> > To compile this driver as a module, choose M here: the
> > module will be called leds-apu.
> > +config LEDS_ARIEL
> > + tristate "Dell Wyse 3020 status LED support"
> > + depends on LEDS_CLASS
> > + depends on (MACH_MMP3_DT && MFD_ENE_KB3930) || COMPILE_TEST
> > + help
> > + This driver adds support for controlling the front panel status
> > + LEDs on Dell Wyse 3020 (Ariel) board via the KB3930 Embedded
> > + Controller.
> > +
> > + Say Y to if your machine is a Dell Wyse 3020 thin client.
> > +
> > config LEDS_AS3645A
> > tristate "AS3645A and LM3555 LED flash controllers support"
> > depends on I2C && LEDS_CLASS_FLASH
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index d7e1107753fb1..bf3b22038d113 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -10,6 +10,7 @@ obj-$(CONFIG_LEDS_TRIGGERS) += led-triggers.o
> > obj-$(CONFIG_LEDS_88PM860X) += leds-88pm860x.o
> > obj-$(CONFIG_LEDS_AAT1290) += leds-aat1290.o
> > obj-$(CONFIG_LEDS_APU) += leds-apu.o
> > +obj-$(CONFIG_LEDS_ARIEL) += leds-ariel.o
> > obj-$(CONFIG_LEDS_AS3645A) += leds-as3645a.o
> > obj-$(CONFIG_LEDS_AN30259A) += leds-an30259a.o
> > obj-$(CONFIG_LEDS_BCM6328) += leds-bcm6328.o
> > diff --git a/drivers/leds/leds-ariel.c b/drivers/leds/leds-ariel.c
> > new file mode 100644
> > index 0000000000000..8fc56722e12f4
> > --- /dev/null
> > +++ b/drivers/leds/leds-ariel.c
> > @@ -0,0 +1,133 @@
> > +// SPDX-License-Identifier: BSD-2-Clause OR GPL-2.0-or-later
> > +/*
> > + * Dell Wyse 3020 a.k.a. "Ariel" Embedded Controller LED Driver
> > + *
> > + * Copyright (C) 2020 Lubomir Rintel
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/leds.h>
> > +#include <linux/regmap.h>
> > +#include <linux/of_platform.h>
> > +
> > +enum ec_index {
> > + EC_BLUE_LED = 0x01,
> > + EC_AMBER_LED = 0x02,
>
> Defining the value after the 0x0 is unnecessary as enums are incremental
> only the first value needs to be defined if the following values are in
> numerical order
I believe this improves readability, especially in case such as this
where the actual numeric values matter.
> Can these also be #defined instead of an enum? Not requesting them to be
> just wondering about the design decision here.
It seems to be that this is what enums are for and theres is no need to
get the preprocessor involved?
I guess this might be a personal preference, but it seems to me that
both enums and preprocessor defines are used across the code base.
> > + EC_GREEN_LED = 0x03,
> > +};
> > +
> > +enum {
> > + EC_LED_OFF = 0x00,
> > + EC_LED_STILL = 0x01,
> Same comment as above
> > + EC_LED_FADE = 0x02,
> > + EC_LED_BLINK = 0x03,
> > +};
> > +
> > +struct ariel_led {
> > + struct regmap *ec_ram;
> > + enum ec_index ec_index;
> > + struct led_classdev led_cdev;
> > +};
> > +
> > +#define led_cdev_to_ariel_led(c) container_of(c, struct ariel_led, led_cdev)
> > +
> > +static enum led_brightness ariel_led_get(struct led_classdev *led_cdev)
> > +{
> > + struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
> > + unsigned int led_status = 0;
> > +
> > + if (regmap_read(led->ec_ram, led->ec_index, &led_status))
> > + return LED_OFF;
> > +
> > + if (led_status == EC_LED_STILL)
> > + return LED_FULL;
> > + else
> else is not needed here
> > + return LED_OFF;
> > +}
Yes, but should it be dropped? To me it seems like explicit else is
better than implicit fallthrough. It is better when it's obvious that
the LED_OFF is returned precisely only when the status is not
EC_LED_STILL and that nothing ever happens afterwards -- and the
compiler/linter will warn when anything unreachable is added afterwards.
Not that it matters too much here. It's just that I've done this
deliberately because it seems more readable to be and would prefer to
leave it that way unless you really really care about that.
> > +
> > +static void ariel_led_set(struct led_classdev *led_cdev,
> > + enum led_brightness brightness)
> > +{
> > + struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
> > +
> > + if (brightness == LED_OFF)
> > + regmap_write(led->ec_ram, led->ec_index, EC_LED_OFF);
> > + else
> > + regmap_write(led->ec_ram, led->ec_index, EC_LED_STILL);
> > +}
> > +
> > +static int ariel_blink_set(struct led_classdev *led_cdev,
> > + unsigned long *delay_on, unsigned long *delay_off)
> > +{
> > + struct ariel_led *led = led_cdev_to_ariel_led(led_cdev);
> > +
> > + if (*delay_on == 0 && *delay_off == 0)
> > + return -EINVAL;
> > +
> > + if (*delay_on == 0) {
> > + regmap_write(led->ec_ram, led->ec_index, EC_LED_OFF);
> > + } else if (*delay_off == 0) {
> > + regmap_write(led->ec_ram, led->ec_index, EC_LED_STILL);
> > + } else {
> > + *delay_on = 500;
> > + *delay_off = 500;
> > + regmap_write(led->ec_ram, led->ec_index, EC_LED_BLINK);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#define NLEDS 3
>
> This define needs to be more unique.
>
> Something like EC_NLEDS or EC_NUM_LEDS and should be moved to the top of the
> file under the #includes
>
> > +
> > +static int ariel_led_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct ariel_led *leds;
> > + struct regmap *ec_ram;
> > + int ret;
> > + int i;
> > +
> > + leds = devm_kcalloc(dev, NLEDS, sizeof(*leds), GFP_KERNEL);
> > + if (!leds)
> > + return -ENOMEM;
> > +
> > + ec_ram = dev_get_regmap(dev->parent, "ec_ram");
> Maybe this should be checked before memory is allocated.
Will fix in next version.
> > + if (!ec_ram)
> > + return -ENODEV;
> > +
> > + leds[0].ec_index = EC_BLUE_LED;
> > + leds[0].led_cdev.name = "blue:power",
> > + leds[0].led_cdev.default_trigger = "default-on";
> > +
> > + leds[1].ec_index = EC_AMBER_LED;
> > + leds[1].led_cdev.name = "amber:status",
> > +
> > + leds[2].ec_index = EC_GREEN_LED;
> > + leds[2].led_cdev.name = "green:status",
> > + leds[2].led_cdev.default_trigger = "default-on";
> > +
> > + for (i = 0; i < NLEDS; i++) {
>
> I don't understand this loop. i is incremented but never used.
>
> should the below be leds[i]?
Sorry for this; I ended up botching this in an attempt to fix things up.
Will fix in the next version.
>
> > + leds[0].ec_ram = ec_ram;
> > + leds[0].led_cdev.brightness_get = ariel_led_get;
> > + leds[0].led_cdev.brightness_set = ariel_led_set;
> > + leds[0].led_cdev.blink_set = ariel_blink_set;
> > +
> > + ret = devm_led_classdev_register(dev, &leds[0].led_cdev);
> > + if (ret)
> > + return ret;
> > + }
> > +
>
> Dan
Thank you!
Lubo
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020
2020-04-17 9:58 ` Lubomir Rintel
@ 2020-04-24 9:18 ` Pavel Machek
0 siblings, 0 replies; 5+ messages in thread
From: Pavel Machek @ 2020-04-24 9:18 UTC (permalink / raw)
To: Lubomir Rintel; +Cc: Dan Murphy, Jacek Anaszewski, linux-kernel, linux-leds
[-- Attachment #1: Type: text/plain, Size: 1881 bytes --]
Hi!
> > > +enum ec_index {
> > > + EC_BLUE_LED = 0x01,
> > > + EC_AMBER_LED = 0x02,
> >
> > Defining the value after the 0x0 is unnecessary as enums are incremental
> > only the first value needs to be defined if the following values are in
> > numerical order
>
> I believe this improves readability, especially in case such as this
> where the actual numeric values matter.
>
> > Can these also be #defined instead of an enum? Not requesting them to be
> > just wondering about the design decision here.
>
> It seems to be that this is what enums are for and theres is no need to
> get the preprocessor involved?
>
> I guess this might be a personal preference, but it seems to me that
> both enums and preprocessor defines are used across the code base.
enums are okay.
> > > + if (regmap_read(led->ec_ram, led->ec_index, &led_status))
> > > + return LED_OFF;
> > > +
> > > + if (led_status == EC_LED_STILL)
> > > + return LED_FULL;
> > > + else
> > else is not needed here
> > > + return LED_OFF;
> > > +}
>
> Yes, but should it be dropped? To me it seems like explicit else is
> better than implicit fallthrough. It is better when it's obvious that
> the LED_OFF is returned precisely only when the status is not
> EC_LED_STILL and that nothing ever happens afterwards -- and the
> compiler/linter will warn when anything unreachable is added afterwards.
>
> Not that it matters too much here. It's just that I've done this
> deliberately because it seems more readable to be and would prefer to
> leave it that way unless you really really care about that.
Both versions are okay. I may have tiny bit of preference for deleting
the else, but...
Thank you,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-04-24 9:18 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-22 7:41 [PATCH v3] leds: ariel: Add driver for status LEDs on Dell Wyse 3020 Lubomir Rintel
2020-04-03 19:37 ` Dan Murphy
2020-04-06 20:12 ` Pavel Machek
2020-04-17 9:58 ` Lubomir Rintel
2020-04-24 9:18 ` Pavel Machek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).