| Message ID | 1539969334-24577-3-git-send-email-dan@emutex.com |
|---|---|
| State | New |
| Headers | show |
| Series | UP Squared board drivers | expand |
On Fri, Oct 19, 2018 at 8:27 PM Dan O'Donovan <dan@emutex.com> wrote: > > From: Javier Arteaga <javier@emutex.com> > > Allow userspace to use the on-board LEDs as "upboard:<color>:". > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/mfd/upboard.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/acpi.h> For better maintenance keep this ordered. > + adev = ACPI_COMPANION(dev); > + if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01")) > + return -ENODEV; Why do you need this part? > + if (led_index >= ARRAY_SIZE(upboard_led_names)) > + return -EINVAL; > + if (!dev->parent) > + return -EINVAL; ...I think this check will cover cases when driver is being probed standalone. Otherwise it's caller's responsibility not to instantiate it if platform doesn't have this LED feature. > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + return -EINVAL;
On Sat 2018-10-20 14:17:58, Andy Shevchenko wrote: 1;2802;0c> On Fri, Oct 19, 2018 at 8:27 PM Dan O'Donovan <dan@emutex.com> wrote: > > > > From: Javier Arteaga <javier@emutex.com> > > > > Allow userspace to use the on-board LEDs as "upboard:<color>:". > > > > > +#include <linux/kernel.h> > > +#include <linux/leds.h> > > +#include <linux/mfd/upboard.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/acpi.h> > > For better maintenance keep this ordered. This is good as-is. Pavel
Hi Javier, Thank you for the patch. I have few trivial issues below, please take a look. On 10/19/2018 07:15 PM, Dan O'Donovan wrote: > From: Javier Arteaga <javier@emutex.com> > > Allow userspace to use the on-board LEDs as "upboard:<color>:". > > Acked-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Javier Arteaga <javier@emutex.com> > Signed-off-by: Dan O'Donovan <dan@emutex.com> > --- > drivers/leds/Kconfig | 10 +++++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-upboard.c | 104 ++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 115 insertions(+) > create mode 100644 drivers/leds/leds-upboard.c > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 44097a3..0ed8857 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -756,6 +756,16 @@ config LEDS_NIC78BX > To compile this driver as a module, choose M here: the module > will be called leds-nic78bx. > > +config LEDS_UPBOARD > + tristate "LED support for the UP Squared" > + depends on LEDS_CLASS > + depends on MFD_UPBOARD > + help > + This option enables support for the LEDs on the UP Squared board. > + > + This driver can also be built as a module. If so, the module > + will be called leds-upboard. > + > comment "LED Triggers" > source "drivers/leds/trigger/Kconfig" > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index 420b5d2..c85f18f 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o > obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o > obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o > obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o > +obj-$(CONFIG_LEDS_UPBOARD) += leds-upboard.o > > # LED SPI Drivers > obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o > diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c > new file mode 100644 > index 0000000..34a6973 > --- /dev/null > +++ b/drivers/leds/leds-upboard.c > @@ -0,0 +1,104 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// UP Board LED driver > +// > +// Copyright (c) 2018, Emutex Ltd. > +// > +// Author: Javier Arteaga <javier@emutex.com> > +// > + > +#include <linux/kernel.h> > +#include <linux/leds.h> > +#include <linux/mfd/upboard.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > +#include <linux/acpi.h> The last include should go first to keep alphabetical order. > + > +#define to_upboard_led(cdev) container_of(cdev, struct upboard_led, cdev) > + > +static const char * const upboard_led_names[] = { > + "upboard:blue:", > + "upboard:yellow:", > + "upboard:green:", > + "upboard:red:", > +}; > + > +struct upboard_led { > + struct regmap_field *field; > + struct led_classdev cdev; > +}; > + > +static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev) > +{ > + struct upboard_led *led = to_upboard_led(cdev); > + int brightness = 0; > + > + regmap_field_read(led->field, &brightness); Please check the return value. > + return brightness; > +} > + > +static void upboard_led_brightness_set(struct led_classdev *cdev, > + enum led_brightness brightness) > +{ > + struct upboard_led *led = to_upboard_led(cdev); > + > + regmap_field_write(led->field, brightness); Ditto. > +} > + > +static int upboard_led_probe(struct platform_device *pdev) > +{ > + unsigned int led_index = pdev->id; > + struct device *dev = &pdev->dev; > + struct acpi_device *adev; > + struct upboard_led *led; > + struct regmap *regmap; > + struct reg_field conf = { > + .reg = UPBOARD_REG_FUNC_EN0, > + .lsb = led_index, > + .msb = led_index, > + }; > + > + adev = ACPI_COMPANION(dev); > + if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01")) > + return -ENODEV; > + > + if (led_index >= ARRAY_SIZE(upboard_led_names)) > + return -EINVAL; > + > + if (!dev->parent) > + return -EINVAL; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + return -EINVAL; > + > + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + led->field = devm_regmap_field_alloc(dev, regmap, conf); > + if (IS_ERR(led->field)) > + return PTR_ERR(led->field); > + > + led->cdev.max_brightness = 1; s/1/LED_ON/ > + led->cdev.brightness_get = upboard_led_brightness_get; > + led->cdev.brightness_set = upboard_led_brightness_set; > + led->cdev.name = upboard_led_names[led_index]; > + > + return devm_led_classdev_register(dev, &led->cdev); > +} > + > +static struct platform_driver upboard_led_driver = { > + .driver = { > + .name = "upboard-led", > + }, > +}; > + > +module_platform_driver_probe(upboard_led_driver, upboard_led_probe); > + > +MODULE_ALIAS("platform:upboard-led"); > +MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>"); > +MODULE_DESCRIPTION("UP Board LED driver"); > +MODULE_LICENSE("GPL v2"); >
Hi! > > + led->field = devm_regmap_field_alloc(dev, regmap, conf); > > + if (IS_ERR(led->field)) > > + return PTR_ERR(led->field); > > + > > + led->cdev.max_brightness = 1; > > s/1/LED_ON/ Actually, I prefer constant 1 here, as it makes it immediately obvious this supports just 0/1. Yes, LED_ON is also 1, but I had to grep the header files for that... (I thought it was 255). Best regards, Pavel
On 10/23/2018 08:54 PM, Pavel Machek wrote: > Hi! > >>> + led->field = devm_regmap_field_alloc(dev, regmap, conf); >>> + if (IS_ERR(led->field)) >>> + return PTR_ERR(led->field); >>> + >>> + led->cdev.max_brightness = 1; >> >> s/1/LED_ON/ > > Actually, I prefer constant 1 here, as it makes it immediately obvious > this supports just 0/1. > > Yes, LED_ON is also 1, but I had to grep the header files for > that... (I thought it was 255). If we have the enum for that, let's use it. Here's the commit message of the patch adding LED_ON - it should be somehow familiar to you - see the ack. commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384 Author: Andi Shyti <andi.shyti@samsung.com> Date: Thu Jan 5 11:34:12 2017 +0900 leds: add LED_ON brightness as boolean value Some devices do not handle the led brightness or simply don't care about it. Conceptually said devices want to just switch on or off the led. It is useless in this case to have a 255 range of brightness, while just having an LED_ON and LED_OFF improves the boolean meaning of the led status. Signed-off-by: Andi Shyti <andi.shyti@samsung.com> Acked-by: Pavel Machek <pavel@ucw.cz> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
On Tue, 2018-10-23 at 20:50 +0200, Jacek Anaszewski wrote: > > diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c > > new file mode 100644 > > index 0000000..34a6973 > > --- /dev/null > > +++ b/drivers/leds/leds-upboard.c > > @@ -0,0 +1,104 @@ [] > > +#include <linux/kernel.h> > > +#include <linux/leds.h> > > +#include <linux/mfd/upboard.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > +#include <linux/acpi.h> > > The last include should go first to keep alphabetical order. There is no accepted single kernel style for #include file ordering. drivers/leds does not use a single style nor is this particular variant documented anywhere to my knowledge. Until such a time when either a local preferred style document or a treewide preferred style exists, please stop asking people to modify #include ordering for various styles like reverse christmas tree by length, alphabetic ordering, or other individual styles. My preferred style would always have kernel.h first as that may help with precompiled headers and overall kernel compilation time one day.
On Tue 2018-10-23 21:09:54, Jacek Anaszewski wrote: > On 10/23/2018 08:54 PM, Pavel Machek wrote: > > Hi! > > > >>> + led->field = devm_regmap_field_alloc(dev, regmap, conf); > >>> + if (IS_ERR(led->field)) > >>> + return PTR_ERR(led->field); > >>> + > >>> + led->cdev.max_brightness = 1; > >> > >> s/1/LED_ON/ > > > > Actually, I prefer constant 1 here, as it makes it immediately obvious > > this supports just 0/1. > > > > Yes, LED_ON is also 1, but I had to grep the header files for > > that... (I thought it was 255). > > If we have the enum for that, let's use it. > Here's the commit message of the patch adding LED_ON - it should > be somehow familiar to you - see the ack. Well .. brightness = LED_ON; is good usage. max_brightness = LED_ON is IMO less readable than max_brightness = 1. Looking at situation again... Having LED_ON and LED_FULL, with some leds having max brightness of 1023, so LED_FULL is not really full brightness any more... Maybe it is time to get rid of the enum, and make it plain int. It does not really enumarate anything, and it does not help readability, either. Pavel > commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384 > Author: Andi Shyti <andi.shyti@samsung.com> > Date: Thu Jan 5 11:34:12 2017 +0900 > > leds: add LED_ON brightness as boolean value > > Some devices do not handle the led brightness or simply don't > care about it. Conceptually said devices want to just switch on > or off the led. It is useless in this case to have a 255 range > of brightness, while just having an LED_ON and LED_OFF improves > the boolean meaning of the led status. > > Signed-off-by: Andi Shyti <andi.shyti@samsung.com> > Acked-by: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> > >
On 10/23/2018 09:23 PM, Joe Perches wrote: > On Tue, 2018-10-23 at 20:50 +0200, Jacek Anaszewski wrote: >>> diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c >>> new file mode 100644 >>> index 0000000..34a6973 >>> --- /dev/null >>> +++ b/drivers/leds/leds-upboard.c >>> @@ -0,0 +1,104 @@ > [] >>> +#include <linux/kernel.h> >>> +#include <linux/leds.h> >>> +#include <linux/mfd/upboard.h> >>> +#include <linux/module.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> +#include <linux/acpi.h> >> >> The last include should go first to keep alphabetical order. > > There is no accepted single kernel style for #include > file ordering. Well, I myself have been asked several times for sorting includes, that's why I keep requiring it for the LED subsystem too. When it's by the occasion of a new driver submission, it costs virtually nothing. And it allows for avoiding any prospective noise on the list due to the submissions like "Order includes". As a first shot the following returns 77: git log | grep -i "include.*alphabetical" | wc -l Aside of that, in case of this particular patch the intention seemingly was to have includes ordered lexicographically, since only the last item didn't fit for this pattern. > drivers/leds does not use a single style nor is this > particular variant documented anywhere to my knowledge. Unspecified kind of includes sorting is mentioned in the Documentation/process/coding-style.rst, line 637. > Until such a time when either a local preferred style > document or a treewide preferred style exists, please > stop asking people to modify #include ordering for > various styles like reverse christmas tree by length, > alphabetic ordering, or other individual style > My preferred style would always have kernel.h first > as that may help with precompiled headers and overall > kernel compilation time one day. If that will happen we'll see massive rearrangement of includes anyway. But OK, I've skimmed through other subsystems and core kernel files and realized that this is indeed often not preserved. So, provided there are no other strong arguments in favor of sorting, I will give up this nitpicking from now on.
On Tue, Oct 23, 2018 at 12:23:13PM -0700, Joe Perches wrote: > On Tue, 2018-10-23 at 20:50 +0200, Jacek Anaszewski wrote: > > > diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c > > > new file mode 100644 > > > index 0000000..34a6973 > > > --- /dev/null > > > +++ b/drivers/leds/leds-upboard.c > > > @@ -0,0 +1,104 @@ > [] > > > +#include <linux/kernel.h> > > > +#include <linux/leds.h> > > > +#include <linux/mfd/upboard.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/regmap.h> > > > +#include <linux/acpi.h> > > > > The last include should go first to keep alphabetical order. > > There is no accepted single kernel style for #include > file ordering. There is a rule of (subtly) better maintenance. If you need to add / remove some header later in a (long) list of unordered list, it would be error prone. Just run `make includecheck` and see the result. I personally fixed some header duplications and removal of init.h in unsorted lists, which have been missed by some reasons. > drivers/leds does not use a single style nor is this > particular variant documented anywhere to my knowledge. Neither does kernel in general. But kernel is evolving and styles also. When you do such statement consider to divide by a time period when certain code was pushed to upstream. > Until such a time when either a local preferred style > document or a treewide preferred style exists, please > stop asking people to modify #include ordering for > various styles like reverse christmas tree by length, > alphabetic ordering, or other individual styles. Why? It makes a sense to ask for new code (and even for patches against old one in some cases). > My preferred style would always have kernel.h first > as that may help with precompiled headers and overall > kernel compilation time one day. How ordering would screw this up?
On Wed, 2018-10-24 at 13:13 +0300, Andy Shevchenko wrote: > On Tue, Oct 23, 2018 at 12:23:13PM -0700, Joe Perches wrote: > > Until such a time when either a local preferred style > > document or a treewide preferred style exists, please > > stop asking people to modify #include ordering for > > various styles like reverse christmas tree by length, > > alphabetic ordering, or other individual styles. > > Why? It makes a sense to ask for new code (and even for patches against old one in some cases). It's just a nit and frequently impossible to require as ordering dependencies between include files do exist. > > My preferred style would always have kernel.h first > > as that may help with precompiled headers and overall > > kernel compilation time one day. > > How ordering would screw this up? gcc has many limits on the use of precompiled headers. https://gcc.gnu.org/onlinedocs/gcc/Precompiled-Headers.html Precompiled headers are often shared by multiple compilation units and precompilation can be stopped after a specific header.
On 10/23/2018 09:30 PM, Pavel Machek wrote: > On Tue 2018-10-23 21:09:54, Jacek Anaszewski wrote: >> On 10/23/2018 08:54 PM, Pavel Machek wrote: >>> Hi! >>> >>>>> + led->field = devm_regmap_field_alloc(dev, regmap, conf); >>>>> + if (IS_ERR(led->field)) >>>>> + return PTR_ERR(led->field); >>>>> + >>>>> + led->cdev.max_brightness = 1; >>>> >>>> s/1/LED_ON/ >>> >>> Actually, I prefer constant 1 here, as it makes it immediately obvious >>> this supports just 0/1. >>> >>> Yes, LED_ON is also 1, but I had to grep the header files for >>> that... (I thought it was 255). >> >> If we have the enum for that, let's use it. >> Here's the commit message of the patch adding LED_ON - it should >> be somehow familiar to you - see the ack. > > Well .. brightness = LED_ON; is good usage. max_brightness = LED_ON is > IMO less readable than max_brightness = 1. > > Looking at situation again... Having LED_ON and LED_FULL, with some > leds having max brightness of 1023, so LED_FULL is not really full > brightness any more... Maybe it is time to get rid of the enum, and > make it plain int. It does not really enumarate anything, and it does > not help readability, either. I agree that it introduces confusion. Yet, there are many out of LED subsystem files to update: find -name "*.c" -o -name "*.h" | xargs grep "enum led_brightness" | awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l returns 87. >> commit 4e552c8cb5bc9137e67e035bab8df6dddbca7384 >> Author: Andi Shyti <andi.shyti@samsung.com> >> Date: Thu Jan 5 11:34:12 2017 +0900 >> >> leds: add LED_ON brightness as boolean value >> >> Some devices do not handle the led brightness or simply don't >> care about it. Conceptually said devices want to just switch on >> or off the led. It is useless in this case to have a 255 range >> of brightness, while just having an LED_ON and LED_OFF improves >> the boolean meaning of the led status. >> >> Signed-off-by: Andi Shyti <andi.shyti@samsung.com> >> Acked-by: Pavel Machek <pavel@ucw.cz> >> Signed-off-by: Jacek Anaszewski <jacek.anaszewski@gmail.com> >> >> >
On Wed, Oct 24, 2018 at 10:07:30PM +0200, Jacek Anaszewski wrote: > Yet, there are many out of LED subsystem files to update: > > find -name "*.c" -o -name "*.h" | xargs grep "enum led_brightness" | > awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l > > returns 87. Side note: git grep -n 'enum led_brightness' | ... a bit more effitient.
On 10/25/2018 11:22 AM, Andy Shevchenko wrote: > On Wed, Oct 24, 2018 at 10:07:30PM +0200, Jacek Anaszewski wrote: > >> Yet, there are many out of LED subsystem files to update: >> >> find -name "*.c" -o -name "*.h" | xargs grep "enum led_brightness" | >> awk -F: '{print $1}' | sort -u | grep -v "leds" | wc -l >> >> returns 87. > > Side note: > > git grep -n 'enum led_brightness' | ... > > a bit more effitient. Right, thanks for the hint.
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 44097a3..0ed8857 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -756,6 +756,16 @@ config LEDS_NIC78BX To compile this driver as a module, choose M here: the module will be called leds-nic78bx. +config LEDS_UPBOARD + tristate "LED support for the UP Squared" + depends on LEDS_CLASS + depends on MFD_UPBOARD + help + This option enables support for the LEDs on the UP Squared board. + + This driver can also be built as a module. If so, the module + will be called leds-upboard. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig" diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 420b5d2..c85f18f 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o obj-$(CONFIG_LEDS_LM3601X) += leds-lm3601x.o +obj-$(CONFIG_LEDS_UPBOARD) += leds-upboard.o # LED SPI Drivers obj-$(CONFIG_LEDS_CR0014114) += leds-cr0014114.o diff --git a/drivers/leds/leds-upboard.c b/drivers/leds/leds-upboard.c new file mode 100644 index 0000000..34a6973 --- /dev/null +++ b/drivers/leds/leds-upboard.c @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// UP Board LED driver +// +// Copyright (c) 2018, Emutex Ltd. +// +// Author: Javier Arteaga <javier@emutex.com> +// + +#include <linux/kernel.h> +#include <linux/leds.h> +#include <linux/mfd/upboard.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/acpi.h> + +#define to_upboard_led(cdev) container_of(cdev, struct upboard_led, cdev) + +static const char * const upboard_led_names[] = { + "upboard:blue:", + "upboard:yellow:", + "upboard:green:", + "upboard:red:", +}; + +struct upboard_led { + struct regmap_field *field; + struct led_classdev cdev; +}; + +static enum led_brightness upboard_led_brightness_get(struct led_classdev *cdev) +{ + struct upboard_led *led = to_upboard_led(cdev); + int brightness = 0; + + regmap_field_read(led->field, &brightness); + + return brightness; +} + +static void upboard_led_brightness_set(struct led_classdev *cdev, + enum led_brightness brightness) +{ + struct upboard_led *led = to_upboard_led(cdev); + + regmap_field_write(led->field, brightness); +} + +static int upboard_led_probe(struct platform_device *pdev) +{ + unsigned int led_index = pdev->id; + struct device *dev = &pdev->dev; + struct acpi_device *adev; + struct upboard_led *led; + struct regmap *regmap; + struct reg_field conf = { + .reg = UPBOARD_REG_FUNC_EN0, + .lsb = led_index, + .msb = led_index, + }; + + adev = ACPI_COMPANION(dev); + if (!adev || strcmp(acpi_device_hid(adev), "AANT0F01")) + return -ENODEV; + + if (led_index >= ARRAY_SIZE(upboard_led_names)) + return -EINVAL; + + if (!dev->parent) + return -EINVAL; + + regmap = dev_get_regmap(dev->parent, NULL); + if (!regmap) + return -EINVAL; + + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + led->field = devm_regmap_field_alloc(dev, regmap, conf); + if (IS_ERR(led->field)) + return PTR_ERR(led->field); + + led->cdev.max_brightness = 1; + led->cdev.brightness_get = upboard_led_brightness_get; + led->cdev.brightness_set = upboard_led_brightness_set; + led->cdev.name = upboard_led_names[led_index]; + + return devm_led_classdev_register(dev, &led->cdev); +} + +static struct platform_driver upboard_led_driver = { + .driver = { + .name = "upboard-led", + }, +}; + +module_platform_driver_probe(upboard_led_driver, upboard_led_probe); + +MODULE_ALIAS("platform:upboard-led"); +MODULE_AUTHOR("Javier Arteaga <javier@emutex.com>"); +MODULE_DESCRIPTION("UP Board LED driver"); +MODULE_LICENSE("GPL v2");