diff mbox series

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

Message ID 20240208184919.2224986-15-andriy.shevchenko@linux.intel.com
State New
Headers
Series auxdisplay: linedisp: Clean up and add new driver |

Commit Message

Andy Shevchenko Feb. 8, 2024, 6:48 p.m. UTC 
  Add initial device tree documentation for Maxim MAX6958/6959.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../bindings/auxdisplay/maxim,max6959.yaml    | 35 +++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml

Comments

Krzysztof Kozlowski Feb. 9, 2024, 8:02 a.m. UTC | #1 
On 08/02/2024 19:48, Andy Shevchenko wrote:
> Add initial device tree documentation for Maxim MAX6958/6959.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../bindings/auxdisplay/maxim,max6959.yaml    | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> 
> diff --git a/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> new file mode 100644
> index 000000000000..49ce26176797
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/maxim,max6959.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MAX6958/6959 7-segment LED display controller with keyscan
> +
> +maintainers:
> +  - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> +

Please describe the device, e.g. bus/interface.

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

> +
> +  reg:
> +    maxItems: 1

No power supplies? No reset pins?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +            #address-cells = <1>;

Use 4 spaces for example indentation. 2 is also fine.

> +            #size-cells = <0>;
> +
> +            max6959: max6959@38 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
e.g. display-controller or display

> +                    compatible = "maxim,max6959";
> +                    reg = <0x38>;
> +            };
> +      };

Best regards,
Krzysztof
Andy Shevchenko Feb. 9, 2024, 1:59 p.m. UTC | #2 
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?

..

> > +  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?

..

> > +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?

> > +            #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?)

> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> e.g. display-controller or display

Sure, thanks for review!

> > +                    compatible = "maxim,max6959";
> > +                    reg = <0x38>;
> > +            };
> > +      };
Krzysztof Kozlowski Feb. 12, 2024, 8:24 a.m. UTC | #3 
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
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
new file mode 100644
index 000000000000..49ce26176797
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
@@ -0,0 +1,35 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/maxim,max6959.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MAX6958/6959 7-segment LED display controller with keyscan
+
+maintainers:
+  - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+
+properties:
+  compatible:
+    const: maxim,max6959
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            max6959: max6959@38 {
+                    compatible = "maxim,max6959";
+                    reg = <0x38>;
+            };
+      };