Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml binding

[Pali Rohár]

On Tuesday 05 July 2022 14:55:10 Krzysztof Kozlowski wrote:
> On 05/07/2022 14:15, Pali Rohár wrote:
> >>>> You need to describe the items, if it is really two items. However your
> >>>> example has only one item, so this was not tested and won't work.
> >>>
> >>> Ehm? Example has two items in the reg.
> >>
> >> No, you have exactly one item.
> >> <0x13 0x1d>
> >>
> >> Two items are for example:
> >> <0x13 0x1d>, <0x23 0x1d>
> > 
> > Ok. So I should change maxItems to 1 in this case?
> 
> Yes
> 
> > 
> > And how you want to describe those items?
> 
> In that case, no need to describe.
> 
> > 
> >>>
> >>>> You'll get warning from Rob's robot soon... but you should test the
> >>>> bindings instead.
> >>>
> >>> I have tested bindings on the real hardware and it is working fine
> >>> together with the driver from patch 2/2.
> >>
> >> Bindings cannot be tested on real hardware. Bindings are tested with
> >> dt_binding_check, as explained in writing-schema.rst
> > 
> > Ou... this is something which I was not able to run, it just does not
> > work, throws lot of python dependency hell errors and it spend more than
> > hour with it. So sorry, I really cannot run it. Maybe it would be a wise
> > to provide web service for these checks for those who cannot run them
> > locally?
> 
> It's one pip command to install and one make command to run... I would
> say easy to start using, unless of course you use some unusual distro
> without Python 3 (cannot believe nowadays...) or without pip.
> 
> Rob's bot will test it for you.

Ok, so lets wait for the robot. After that I will try to fix found
issues and send a new patch version.

> Anyway, in such case please mark your bindings always as RFT, so we will
> not waste time on reviewing obvious stuff which is found by automated
> tools. I think we both agree that reviewers time should not be used for
> trivial stuff already pointed out by compiler/linter/automation.

Yes!

> > 
> >>>
> >>>>> +
> >>>>> +  "#address-cells":
> >>>>> +    const: 1
> >>>>> +
> >>>>> +  "#size-cells":
> >>>>> +    const: 0
> >>>>> +
> >>>>> +patternProperties:
> >>>>> +  "^multi-led@[0-7]$":
> >>>>> +    type: object
> >>>>> +    $ref: leds-class-multicolor.yaml#
> >>>>
> >>>> This looks incorrect, unless you rebased on my patchset?
> >>>
> >>> So what is the correct? (I used inspiration from
> >>> cznic,turris-omnia-leds.yaml file)
> >>
> >> Which according to current multicolor bindings is not correct. Correct
> >> is pwm-multicolor. However if you rebase on [1], it looks fine, except
> >> missing unevaluatedProperties.
> > 
> > Ok. So does it mean that I should just add
> > "unevaluatedProperties: false"?
> 
> Yes, on that level of indentation, so:
>     $ref: leds-class-multicolor.yaml#
>     unevaluatedProperties: false

Ok.

> > 
> >> [1]
> >> https://lore.kernel.org/all/20220624112106.111351-1-krzysztof.kozlowski@xxxxxxxxxx/
> >>
> >>>
> >>>>> +
> >>>>> +    properties:
> >>>>> +      reg:
> >>>>> +        minimum: 0
> >>>>> +        maximum: 7
> >>>>> +
> >>>>> +    required:
> >>>>> +      - reg
> >>>>> +
> >>>>> +additionalProperties: false
> >>>>> +
> >>>>> +examples:
> >>>>> +  - |
> >>>>> +
> >>>>
> >>>> No blank line.
> >>>
> >>> Ok.
> >>>
> >>>>> +    #include <dt-bindings/leds/common.h>
> >>>>> +
> >>>>> +    cpld@3,0 {
> >>>>
> >>>> Generic node name.
> >>>
> >>> Is not cpld name generic enough?
> >>
> >> No, it means nothing to me. Just like "a", "ashjd" or "wrls".
> > 
> > If you never heard about it, I would suggest to read something about
> > Programmable logic devices. It is interesting category of hardware with
> > which you can play. CPLD and FPGA are very often used in lot of products
> > and FPGA is very easy for playing and programming custom logic.
> 
> The are many different acronyms in the language so without context might
> be tricky to connect the dots.

Anyway, playing with FPGA is really a fun!

> > 
> > For example on wikipedia is list of different technologies of
> > programmable logic devices:
> > https://en.wikipedia.org/wiki/Programmable_logic_device
> > 
> > So if you want more generic name, just name it "pld"? 
> 
> That one would be fine.
> 
> > But as it is CPLD
> > device I would suggest to name it really as "cpld". It does not matter
> > from which manufactor you have CPLD, just like it does not matter from
> > which manufactor you have NAND.
> 
> Then cpld is fine as well.

Ok, so stick with cpld.

> > 
> > From bus point of view, cpld is like nand or nor nodes in DTS. All of
> > them refers to specific memory map of chip selects on the local bus.
> > 
> >> "The name of a node should be somewhat generic, reflecting the function
> >> of the device and not its precise programming
> >>  model. If appropriate, the name should be one of the following choices:"
> > 
> > Hm... You forgot to send what are those "choices:"?
> 
> I didn't, I just assumed you will Google it (or use other web-search
> engine of your choice) to get the spec. As this is a quote, Google
> results should be very accurate. No need to duplicate entire pages of
> publicly available specification.

This was the first thing which I did when I read email. No usable
result. So the next thing was that I started git grep on the linux tree.
Again no result. So at the end I come to the conclusion that you forgot
to copy+paste whole quote or something like that.

Now I started searching a bit more and found it in following documentation:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Original link to the quote would be useful (but now I have it).

> Best regards,
> Krzysztof