Re: [PATCH v3 2/2] leds: spi-byte: add single byte SPI LED driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>>>>



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux