linux-kernel - Re: [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 12 Feb 2024 09:24:13 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 Miguel Ojeda <ojeda@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Robin van der Gracht
 <robin@...tonic.nl>, Paul Burton <paulburton@...nel.org>,
 Geert Uytterhoeven <geert+renesas@...der.be>
Subject: Re: [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

On 09/02/2024 14:59, Andy Shevchenko wrote:
> On Fri, Feb 09, 2024 at 09:02:44AM +0100, Krzysztof Kozlowski wrote:
>> On 08/02/2024 19:48, Andy Shevchenko wrote:
>>> Add initial device tree documentation for Maxim MAX6958/6959.
> 
> ...
> 
>> Please describe the device, e.g. bus/interface.
> 
> OK.
> 
> ...
> 
>>> +properties:
>>> +  compatible:
>>> +    const: maxim,max6959
>>
>> Your title said also max6958, so I would expect it to be here as well.
>> Cam be followed by 6959 fallback compatible, if they are compatible.
> 
> Same question as I asked before, why should we have them separated?
> The hardware features can be autodetected. What's the reason for (unneeded
> in my opinion and duplicative) compatible?

And which part of device description in the binding, or at least commit
msg but better description, explained it?

For every unexplained deviation from common rules - and documenting
compatibles is explicitly asked in writing bindings document - you will
get questions from reviewers...

Please add this information to description.

> 
> ...
> 
>>> +  reg:
>>> +    maxItems: 1
>>
>> No power supplies? No reset pins?
> 
> No power supplies, no reset pins. At least there is no as such in
> the datasheet. Do you see them there?

How do I know? I don't have datasheets and I don't have really time to
investigate each datasheet of every device people send bindings for.
Several people make mistakes of not putting such stuff because "driver
does not need it", so how can I know that here it was not the case?

> 
> ...
> 
>>> +examples:
>>> +  - |
>>> +    i2c {
>>> +            #address-cells = <1>;
>>
>> Use 4 spaces for example indentation. 2 is also fine.
> 
> Sure. Btw, this is copy&pasted from the existing YAML. Are you going to
> fix them?

I fixed several of them. At some point I might fix all of them, but
that's lower priority. I wished I have all the time for this :)

> 
>>> +            #size-cells = <0>;
>>> +
>>> +            max6959: max6959@38 {
>>
>> Node names should be generic. See also an explanation and list of
> 
> (Same remark: it's a pattern from the existing code. Are you going to fix
>  that?)
> 

Same answer.

Best regards,
Krzysztof