On 03/29/2018 08:56 PM, Oleh Kravchenko wrote:
>
>
> On 28.03.18 23:23, Jacek Anaszewski wrote:
>> On 03/28/2018 09:48 PM, Oleh Kravchenko wrote:
>>>
>>>
>>> On 28.03.18 22:21, Jacek Anaszewski wrote:
>>>> On 03/28/2018 08:36 AM, Oleh Kravchenko wrote:
>>>>> Hello Jacek,
>>>>>
>>>>> On 27.03.18 23:58, Jacek Anaszewski wrote:
>>>>>>> +Example
>>>>>>> +-------
>>>>>>> +
>>>>>>> +led-controller@0 {
>>>>>>> + compatible = "crane,cr0014114";
>>>>>>> + reg = <0>;
>>>>>>> + spi-max-frequency = <50000>;
>>>>>>> + #address-cells = <1>;
>>>>>>> + #size-cells = <0>;
>>>>>>> +
>>>>>>> + led@0 {
>>>>>>> + reg = <0>;
>>>>>>> + label = "cr0:red:coin";
>>>>>>> + };
>>>>>>> + led@1 {
>>>>>>> + reg = <1>;
>>>>>>> + label = "cr0:green:coin";
>>>>>>> + };
>>>>>>> + led@2 {
>>>>>>> + reg = <2>;
>>>>>>> + label = "cr0:blue:coin";
>>>>>>> + };
>>>>>>> + led@3 {
>>>>>>> + reg = <3>;
>>>>>>> + label = "cr1:red:bill";
>>>>>>> + };
>>>>>>> + led@4 {
>>>>>>> + reg = <4>;
>>>>>>> + label = "cr1:green:bill";
>>>>>>> + };
>>>>>>> + led@5 {
>>>>>>> + reg = <5>;
>>>>>>> + label = "cr1:blue:bill";
>>>>>>> + };
>>>>>>
>>>>>> Why cr0 and cr1? It should be cr0014114 to stick to the
>>>>>> current LED naming pattern <devicename:colour:function>.
>>>>>>
>>>>>> Nonetheless, we lately came to the conclusion that devicename
>>>>>> segment is redundant in LED class device name, so until we change
>>>>>> LED naming convention officially, let's remove devicename segment
>>>>>> at least from DT and prepend the label with "cr0014114" in the driver.
>>>>>>
>>>>>> Please compare how it is approached in [0] (not merged yet).
>>>>>>
>>>>>
>>>>> It's just example.
>>>>> But anyway our applications works with LEDs by numbers.
>>>>>
>>>>> Using function names instead numbers will increase code complexity,
>>>>> so we use numbers :)
>>>>
>>>> This is LED class device naming convention in mainline and examples
>>>> also must stick to it to keep the things consistent.
>>>>
>>>> Please switch labels so that they matched the pattern
>>>>
>>>> label = "color:function";
>>>
>>>> Why cr0 and cr1? It should be cr0014114 to stick to the
>>>> current LED naming pattern <devicename:colour:function>.
>>>
>>> So how it should be? I'm confused :(
>>
>> You seem to have overlooked second part of my previous message:
>>
>> "
>> Nonetheless, we lately came to the conclusion that devicename
>> segment is redundant in LED class device name, so until we change
>> LED naming convention officially, let's remove devicename segment
>> at least from DT and prepend the label with "cr0014114" in the driver.
>>
>> Please compare how it is approached in [0] (not merged yet).
>> "
>>
>> Please note that DT label is not intended to be used as-is
>> for LED class device name, but it's been frequently abused so.
>>
>> Here is the example of how you could modify your code:
>>
>> DT:
>>
>> led@0 {
>> reg = <0>;
>> label = "red:coin";
>> };
>>
>> LED class driver:
>>
>> #include <uapi/linux/uleds.h>
>>
>>
>> struct cr0014114_led {
>> char name[LED_MAX_NAME_SIZE];
>> struct cr0014114 *priv;
>> struct led_classdev ldev;
>> u8 brightness;
>> };
>>
>>
>> static int cr0014114_probe_dt(struct cr0014114 *priv)
>> {
>> ...
>> const char *str;
>> ...
>> device_for_each_child_node(priv->dev, child)
>> {
>> ...
>> if (fwnode_property_read_string(child, "label", &str))
>> snprintf(led->name, sizeof(led->name), "cr0014114::"),
>> else
>> snprintf(led->name, sizeof(led->name), "cr0014114:%s", str),
>>
>>
>> I hope it makes all clear now.
>
> I would like to keep current implementation.
> This is acceptable?
It heavily depends on DT label property value, since in your
current implementation you're adopting label as-is for the
LED class device name. I plan on removing devicename from all
LED labels in all existing dts files and modify drivers to add
devicename by themselves. Effectively the driver code will anyway
end up in this form. Let's do it right at once.
--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
