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

[Krzysztof Kozlowski]

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