On 05/05/2019 16:48, Jacek Anaszewski wrote:
> Hi Christian,
>
> Thank you for the update. I have some trivial nits,
> please refer below.
Hello Jacek,
thanks for having a look. I don't have problems with the changes and
will integrate them into a v3. Sorry for the style nitpicks for example
with the (ret != 0). It's different for each project and I haven't
written much code with Linux style yet.
I'll wait for another few hours in case someone else wants additional
changes before posting v3.
Best regards
Christian
>
> On 5/5/19 2:52 PM, oss@xxxxxxxxxxxxx wrote:
>> From: Christian Mauderer <oss@xxxxxxxxxxxxx>
>>
>> This driver adds support for simple SPI based LED controller which use
>> only one byte for setting the brightness.
>>
>> Signed-off-by: Christian Mauderer <oss@xxxxxxxxxxxxx>
>> ---
>>
>> Changes compared to v1:
>> - rename ubnt-spi to leds-spi-byte
>> - rework probe to get all parameters before allocating anything ->
>> error checks
>> all collected together and initializing all fields of the device
>> structure is
>> more obvious
>> - fix some unsteady indentations during variable declaration
>> - rework comment with protocol explanation
>> - handle case of off_bright > max_bright
>> - fix spelling in commit message
>> - mutex_destroy in remove
>> - change label to use either use the given one without a prefix or a
>> default one
>> NOTE: Should the label be a mandatory parameter instead of the
>> default label?
>>
>>
>> drivers/leds/Kconfig | 12 ++++
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds-spi-byte.c | 133 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 146 insertions(+)
>> create mode 100644 drivers/leds/leds-spi-byte.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index a72f97fca57b..0866c55e8004 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -766,6 +766,18 @@ config LEDS_NIC78BX
>> To compile this driver as a module, choose M here: the module
>> will be called leds-nic78bx.
>> +config LEDS_SPI_BYTE
>> + tristate "LED support for SPI LED controller with a single byte"
>> + depends on LEDS_CLASS
>> + depends on SPI
>> + depends on OF
>> + help
>> + This option enables support for LED controller which use a
>> single byte
>> + for controlling the brightness. The minimum and maximum value
>> of the
>> + byte can be configured via a device tree. The driver can be
>> used for
>> + example for the microcontroller based LED controller in the
>> Ubiquiti
>> + airCube ISP devices.
>> +
>> comment "LED Triggers"
>> source "drivers/leds/trigger/Kconfig"
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 4c1b0054f379..1786d7e2c236 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -75,6 +75,7 @@ obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o
>> obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o
>> obj-$(CONFIG_LEDS_MLXREG) += leds-mlxreg.o
>> obj-$(CONFIG_LEDS_NIC78BX) += leds-nic78bx.o
>> +obj-$(CONFIG_LEDS_SPI_BYTE) += leds-spi-byte.o
>> obj-$(CONFIG_LEDS_MT6323) += leds-mt6323.o
>> obj-$(CONFIG_LEDS_LM3692X) += leds-lm3692x.o
>> obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
>> diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
>> new file mode 100644
>> index 000000000000..19a68bce1a7c
>> --- /dev/null
>> +++ b/drivers/leds/leds-spi-byte.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2019 Christian Mauderer <oss@xxxxxxxxxxxxx>
>> +
>> +/*
>> + * The driver can be used for controllers with a very simple SPI
>> protocol: Only
>> + * one byte between an off and a max value (defined by devicetree)
>> will be sent.
>> + */
>> +
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/of_device.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/mutex.h>
>> +#include <uapi/linux/uleds.h>
>> +
>> +struct spi_byte_led {
>> + struct led_classdev ldev;
>> + struct spi_device *spi;
>> + char name[LED_MAX_NAME_SIZE];
>> + struct mutex mutex;
>> + u8 off_value;
>> + u8 max_value;
>> +};
>> +
>> +static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
>> + enum led_brightness brightness)
>> +{
>> + struct spi_byte_led *led = container_of(dev, struct spi_byte_led,
>> ldev);
>> + u8 value;
>> + int ret;
>> +
>> + value = (u8) brightness + led->off_value;
>> +
>> + mutex_lock(&led->mutex);
>> + ret = spi_write(led->spi, &value, sizeof(value));
>> + mutex_unlock(&led->mutex);
>> +
>> + return ret;
>> +}
>> +
>> +static int spi_byte_probe(struct spi_device *spi)
>> +{
>> + struct device *dev = &spi->dev;
>> + struct device_node *child;
>> + struct spi_byte_led *led;
>> + int ret;
>> + const char *default_name = "leds-spi-byte::";
>> + const char *name;
>> + u8 off_value;
>> + u8 max_value;
>> +
>> + if (!dev->of_node)
>> + return -ENODEV;
>> +
>> + if (of_get_child_count(dev->of_node) != 1) {
>> + dev_err(dev, "Device must have exactly one LED sub-node.");
>> + return -EINVAL;
>> + }
>> + child = of_get_next_child(dev->of_node, NULL);
>> +
>> + ret = of_property_read_string(child, "label", &name);
>> + if (ret != 0)
>
> It is more customary to have "if (ret)" in such cases.
> Applies to all occurrences below.
>
>> + name = default_name;
>> +
>> + ret = of_property_read_u8(child, "leds-spi-byte,off-value",
>> &off_value);
>> + if (ret != 0) {
>> + dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>> + return -EINVAL;
>> + }
>> +
>> + ret = of_property_read_u8(child, "leds-spi-byte,max-value",
>> &max_value);
>> + if (ret != 0) {
>> + dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>> + return -EINVAL;
>> + }
>> +
>> + if (off_value >= max_value) {
>> + dev_err(dev, "off-value has to be smaller than max-value.");
>> + return -EINVAL;
>> + }
>> +
>> + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>> + if (!led)
>> + return -ENOMEM;
>> +
>> + led->spi = spi;
>> + strlcpy(led->name, name, sizeof(led->name));
>> + mutex_init(&led->mutex);
>> + led->off_value = off_value;
>> + led->max_value = max_value;
>> + led->ldev.name = led->name;
>> + led->ldev.brightness = LED_OFF;
>
> This line is redundant - already zeroed by kzalloc.
>
>> + led->ldev.max_brightness = led->max_value - led->off_value;
>> + led->ldev.brightness_set_blocking =
>> spi_byte_brightness_set_blocking;
>> + ret = led_classdev_register(&spi->dev, &led->ldev);
>
> Please use devm_led_classdev_register().
>
>> + if (ret >= 0)
>> + spi_set_drvdata(spi, led);
>
> This looks quite odd. Please shape it as below:
>
> if (ret)
> return ret
>
> spi_set_drvdata(spi, led);
>
>> +
>> + return ret;
>
> s/ret/0/
>
>> +}
>> +
>> +static int spi_byte_remove(struct spi_device *spi)
>> +{
>> + struct spi_byte_led *led = spi_get_drvdata(spi);
>> +
>> + led_classdev_unregister(&led->ldev);
>> + mutex_destroy(&led->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id spi_byte_dt_ids[] = {
>> + { .compatible = "leds-spi-byte", },
>> + {},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, spi_byte_dt_ids);
>> +
>> +static struct spi_driver spi_byte_driver = {
>> + .probe = spi_byte_probe,
>> + .remove = spi_byte_remove,
>> + .driver = {
>> + .name = KBUILD_MODNAME,
>> + .of_match_table = spi_byte_dt_ids,
>> + },
>> +};
>> +
>> +module_spi_driver(spi_byte_driver);
>> +
>> +MODULE_AUTHOR("Christian Mauderer <oss@xxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("single byte SPI LED driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("spi:leds-spi-byte");
>>
>
