Hello Dan,
On 06/05/2019 17:37, Dan Murphy wrote:
> Christian
>
> On 5/6/19 10:19 AM, Christian Mauderer wrote:
>> On 06/05/2019 16:58, Dan Murphy wrote:
> <snip>
>
>>
>> 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?
>>
>
> Yes that would be a good idea since it is a multi-purpose driver with very specific requirements.
OK. I'll improve the description with this list (maybe with a slightly
better formulation).
>
>>>
>>>>>
>>>>>
>>>>>> + 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.
>>
>
> True not saying to do create the function it was just a suggestion.
>
I'll try it and have a look at what is better. If everything open
firmware related is in a extra function, that can simplify a potential
future change to get the information from somewhere else.
>>>>>
>>>>>
>>>>>> + 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.
>>
>
> True a spi_read would be ineffective here.
>
>>>
>>> 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?
>>
>
> True.
>
> Dan
>
>> Best regards
>>
>> Christian
>>
>>> Dan
>>>
>>>>>
>>>>> Dan
>>>>>
>>>>> <snip>
>>>>>
>>>>
>>>> Best regards
>>>>
>>>> Christian
>>>>
