Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3358f88c-5c58-ae0d-2c26-7ba9a954b491@linaro.org>
Date:   Tue, 5 Jul 2022 14:55:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH 1/2] dt-bindings: leds: Add cznic,turris1x-leds.yaml
 binding
Content-Language: en-US
To:     =?UTF-8?Q?Pali_Roh=c3=a1r?= <pali@kernel.org>
Cc:     Pavel Machek <pavel@ucw.cz>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        =?UTF-8?Q?Marek_Beh=c3=ban?= <kabel@kernel.org>,
        linux-leds@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220705000448.14337-1-pali@kernel.org>
 <42d837dd-fbd1-6294-2fa0-8a07ae0f8d44@linaro.org>
 <20220705114238.xwgexavgozqskwbw@pali>
 <90fd55cb-13f4-eac2-2b1a-85ae628ecc89@linaro.org>
 <20220705121541.t7jjcjp4hkqprsdo@pali>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20220705121541.t7jjcjp4hkqprsdo@pali>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.

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.

> 
>>>
>>>>> +
>>>>> +  "#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


> 
>> [1]
>> https://lore.kernel.org/all/20220624112106.111351-1-krzysztof.kozlowski@linaro.org/
>>
>>>
>>>>> +
>>>>> +    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.

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

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

Best regards,
Krzysztof