diff mbox series

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

Message ID 20240208165937.2221193-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, 4:58 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

Conor Dooley Feb. 8, 2024, 8:44 p.m. UTC | #1 
On Thu, Feb 08, 2024 at 08:16:56PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 08, 2024 at 05:50:51PM +0000, Conor Dooley wrote:
> > On Thu, Feb 08, 2024 at 06:58:57PM +0200, Andy Shevchenko wrote:
> > > Add initial device tree documentation for Maxim MAX6958/6959.
> > 
> > Why "initial"? Are there elements this display that you've not
> > documented yet? 
> 
> s/documented/implemented/

Oh no, I meant documented. Whether or not you implement support in the
driver doesn't really matter, but you should endeavour to document as
much of the hardware as possible. Certainly simple things like
interrupts.

> There are features of the hardware that may need additional properties.
> 
> > > +title: MAX6958/6959 7-segment LED display controller with keyscan
> > 
> > > +properties:
> > > +  compatible:
> > > +    const: maxim,max6959
> > 
> > Where's the max6958's compatible? I don't see it in your driver either.
> 
> For now, see above, there is no need. 

I don't know what I am supposed to see above.

> Moreover, there is no need at all
> as we have autodetection mechanism. I don't see why we should have two
> compatible strings just for the sake of having them.
> 
> > It seems that the max6959 has some interrupt capabilities that are not
> > available on the max6958, so a dedicated compatible seems suitable to
> > me.
> 
> So, please clarify the DT's p.o.v. on the hardware that can be autodetected.
> Do we need to still have different compatible strings? What for? I don't
> see the need, sorry for my (silly) questions.

If there's an auto-detection mechanism, which is a valid justification,
you need to put it in the commit message. If you don't it just looks
like you're missing a compatible.
The advantage of a dedicated compatible is restricting the properties
that would only apply to either the 6958 or 6959 to just that device.

You've only partially documented these devices though, so it is not
clear how much of a divergence there'd actually be.

cheers,
Conor
Krzysztof Kozlowski Feb. 9, 2024, 9:18 a.m. UTC | #2 
On 08/02/2024 18:50, Conor Dooley wrote:
> On Thu, Feb 08, 2024 at 06:58:57PM +0200, Andy Shevchenko wrote:
>> Add initial device tree documentation for Maxim MAX6958/6959.
> 
> Why "initial"? Are there elements this display that you've not
> documented yet? 
> 

Why there are multiple versions of this patchset? This leads to
different reviewers commenting in different patchsets?

Andy,
please implement feedback given to all your patchsets...

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>;
+            };
+      };