On 06/05/2019 16:58, Dan Murphy wrote:
>
>
> On 5/6/19 7:59 AM, Christian Mauderer wrote:
>> Hello Dan,
>>
>> thanks for reviewing the patch.
>>
>> On 06/05/2019 14:05, Dan Murphy wrote:
>>> Christian
>>>
>>> On 5/5/19 3:00 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 v2:
>>>> - use "if (ret)" instead of "if (ret != 0)"
>>>> - don't initialize ldev-fields with zero
>>>> - use devm_led_classdev_register instead of led_classdev_register
>>>> - check for error instead of good case with the last if in spi_byte_probe
>>>>
>>>> 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
>>>>
>>>>
>>>> drivers/leds/Kconfig | 12 ++++
>>>> drivers/leds/Makefile | 1 +
>>>> drivers/leds/leds-spi-byte.c | 134 +++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 147 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..8170b2da497a
>>>> --- /dev/null
>>>> +++ b/drivers/leds/leds-spi-byte.c
>>>> @@ -0,0 +1,134 @@
>>>> +// 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;
>>>> +
>>>
>>> Sorry if this has been addressed but the versions moved fast.
>>>
>>> What is the purpose of adding the off_value?
>>>
>>> If max is 63 and say off value is 1 then this will set brightness to 64 but that is not what the LED framework will send.
>>> if you read the brightness.
>>>
>>> Is it safe to assume that off_value would always be 0?
>>
>> No it's not always 0.
>>
>> In my target application (a microcontroller based LED controller from
>> Ubiquiti) I have the values from 0 to 63. But after some discussion I
>> wrote the driver to be more generic so it can cover some similar
>> controllers too.
>>
>> So if there is a hypothetical SPI-based controller that uses a single
>> byte with values from 0x80 (off) to 0x8f (maximum brightness) to control
>> a LED, you could set the off and max values to these two and the driver
>> sends 0x80 if you set brightness to 0 and 0x8f if you set brightness to 15.
>>
>
> In this case would the max brightness just be 0x7f.
> Maybe an optional DT property for brightness mask would help here.
>
> Not sure what the msb is in the example but if this bit defines the LED on/off control then
> maybe an optional dt property for this would be more clear.
>
>> A more concrete application could be to let the LED slightly on to show
>> power by setting the off value to for example 10 but letting the max
>> value at 63. In that case there would be only 53 brightness levels left.
>>
>
> But then the LED is not off by user space request. When the LED is asked to be off
> it should be off. The brightness values requested by the user space should be honored and
> not modified by the driver.
That's correct. It was an example what would be possible. You are
correct that this would be an ugly hack.
>
>> Of course it would have been possible to make it a lot more universal by
>> for example adding a prefix, a bit mask or other word lengths. But that
>> would have added a lot of complexity without any actual application.
>>
>
> I have to disagree here. If this is supposed to be a universal SPI byte driver that
> needs special handling then it is either needs to be created in a universal way or needs to be made
> target specific.
If it should be a truly universal driver for SPI based controllers I
would at least need the following additional features:
- Variable direction (led brighter with lower or higher values).
- Counter at any location of the byte. Maybe even some odd combinations
like bit 7, 3 and 5 in this order.
- Sending some bytes before the LED brightness value.
- Sending multiple bytes for multiple LEDs.
- Configurable other bits.
And that would only include controllers without a SPI MISO channel. So
where does "universal" stop? I stopped when my application and maybe
some others (like using a digital potentiometer with an similar
interface) could be covered.
So it's not a universal but a multi-purpose driver that can be used for
every controller with the following interface:
- Only has a MOSI line. MISO can be ignored.
- Expect one byte via SPI.
- Expect a range of values from x to y to set brightness from off (x) to
bright (y).
It can be extended if an application appears that needs more than that.
Maybe it's a good idea to add that list of requirements to the device
tree description?
>
>>>
>>>
>>>> + 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)
>>>> + name = default_name;
>>>> +
>>>> + ret = of_property_read_u8(child, "leds-spi-byte,off-value", &off_value);
>>>> + if (ret) {
>>>> + 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) {
>>>> + dev_err(dev, "LED node needs a leds-spi-byte,max-value.");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>
>>> You could probably allocate the led struct memory first and then pass in the address of those
>>> variables as opposed to creating the stack variables.
>>>
>>> led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
>>> if (!led)
>>> return -ENOMEM;
>>>
>>> ret = of_property_read_string(child, "label", &led->ldev.name);
>>> if (ret)
>>> led->ldev.name = default_name;
>>>
>>> ret = of_property_read_u8(child, "leds-spi-byte,off-value", &led->off_value);
>>> if (ret) {
>>> dev_err(dev, "LED node needs a leds-spi-byte,off-value.");
>>> return -EINVAL;
>>> }
>>> .
>>> .
>>> .
>>
>> I had that in the first revision. Also no one objected, I noted that I
>> had to search whether I have initialized all fields when I added another
>> one. Therefore I thought it would be more readable if I initialize the
>> complete structure at one location. I put readability over efficiency in
>> that case because it's only called once during initialization. Of course
>> I can change that back.
>>
>
> Well for readability you could also create a function that parses the node after allocating
> the memory. That way all the DT parsing and value checking is done in a single function.
>
OK for me too. I'm quite indifferent here. It's a matter of preference
how to style something like that.
>>>
>>>
>>>> + 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.max_brightness = led->max_value - led->off_value;
>>>
>>> Again not sure why this is needed. This is changing the behavior of what max brightness value is defined in the DT.
>>> And this is not documented in the DT doc.
>>>
>>> max_value = 255 off_value = 254 meets all the criteria but then LED framework has max brightness is 1
>>> But that is not what is described in the DT.
>>
>> Then my description in the DT wasn't clear enough. I wanted to express
>> that with the sentence: "The value of the byte can be any value between
>> the off-value and max-value defined in the properties."
>>
>> Should I add another example (beneath the Ubiquiti controller) like the
>> following in the description to make it more clear?
>>
>> "Another example could be a controller with the following control byte
>> structure:
>> - Bit 8 to 5: always 0x8
>> - Bit 4 to 0: brightness value
>> In that case the off-value would be 0x80 and the max-value would be 0x8f."
>>
>
> In this case max-brightness would be 0xf. No math needed. With the aid of a brightness mask
> then the code would need to do a read before write.
> This makes this driver more extensible if it truly needs to be universal and generic.
That would mean that the controller allows a read of the register. Not
every controller does that. For example the one I want to support just
returns the previously sent byte.
>
> read_value = 0
>
> if (led->brightness_mask)
> spi_read()
>
> value = (u8) brightness & led->brightness_mask | read_value;
> // or it can also skip brightness_mask and use max_brightness
> // value = (u8) brightness & led->max_brightness | read_value;
>
> spi_write(value)
>
> This aligns what is declared in the DT to what is expected from the user space.
> >>>
>>>> + led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
>>>> + ret = devm_led_classdev_register(&spi->dev, &led->ldev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + spi_set_drvdata(spi, led);
>>>
>>> If you move this above the registration this can just become
>>>
>>> return = devm_led_classdev_register(&spi->dev, &led->ldev);
>>
>> Good point. I'll change that.
>>
>>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int spi_byte_remove(struct spi_device *spi)
>>>> +{
>>>> + struct spi_byte_led *led = spi_get_drvdata(spi);
>>>> +
>>>> + led_classdev_unregister(&led->ldev);
>>>
>>> Don't need this with devm call
>>
>> Thanks for the hint. Jacek told me that already. I wanted to wait for
>> some further feedback before spamming the list with another version.
>>
>
> One other thing if the LED driver is removed and the LED is on and unmanaged that is ok?
>
Is that any different for any of the other drivers? As soon as I remove
a driver, the device is unmanaged, isn't it?
Best regards
Christian
> Dan
>
>>>
>>> Dan
>>>
>>> <snip>
>>>
>>
>> Best regards
>>
>> Christian
>>
