diff mbox series

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

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

Commit Message

Andy Shevchenko Feb. 12, 2024, 5:01 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. 15, 2024, 8:21 a.m. UTC | #1 
On 12/02/2024 18:16, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
>> On Mon, Feb 12, 2024 at 07:01:47PM +0200, Andy Shevchenko wrote:
>>> Add initial device tree documentation for Maxim MAX6958/6959.
>>
>> Oh, this is an old version :-(
> 
> Here is a new one:
> 
> From d8c826e06cf9237cd5fc6b2bb0b1cac5aff4fd8a Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Thu, 8 Feb 2024 17:23:38 +0200
> Subject: [PATCH 1/1] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
> 
> Add initial device tree documentation for Maxim MAX6958/6959.
> 

Anyway you need to send the new version to the list, so the bot will
test it.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Geert Uytterhoeven Feb. 15, 2024, 10:57 a.m. UTC | #2 
Hi Andy,

On Mon, Feb 12, 2024 at 6:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 07:01:47PM +0200, Andy Shevchenko wrote:
> > > Add initial device tree documentation for Maxim MAX6958/6959.
> >
> > Oh, this is an old version :-(
>
> Here is a new one:
>
> From d8c826e06cf9237cd5fc6b2bb0b1cac5aff4fd8a Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Thu, 8 Feb 2024 17:23:38 +0200
> Subject: [PATCH 1/1] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
>
> Add initial device tree documentation for Maxim MAX6958/6959.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> @@ -0,0 +1,44 @@
> +# 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>
> +
> +description:
> +  The Maxim MAX6958/6959 7-segment LED display controller provides
> +  an I2C interface to up to four 7-segment LED digits. The MAX6959
> +  in comparison to MAX6958 has the debounce and interrupt support.

IUIC, the primary differentiating factor is that the MAX6959 adds input
and GPIO support?  Debounce and interrupt support are merely features
of input support.

> +  Type of the chip can be autodetected via specific register read,
> +  and hence the features may be enabled in the driver at run-time.

I don't think you need to read that register, as the users of the
features (keypad mapping, interrupts property, ...) also need to be
described in DTS (once supported).

> +  Given hardware is simple and does not provide any additional pins,
> +  such as reset or enable.

Does this matter? I.e. is it important to say this in the bindings?

Gr{oetje,eeting}s,

                        Geert
Andy Shevchenko Feb. 15, 2024, 11:17 a.m. UTC | #3 
On Thu, Feb 15, 2024 at 11:57:53AM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 12, 2024 at 6:16 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:

..

> > +description:
> > +  The Maxim MAX6958/6959 7-segment LED display controller provides
> > +  an I2C interface to up to four 7-segment LED digits. The MAX6959
> > +  in comparison to MAX6958 has the debounce and interrupt support.
> 
> IUIC, the primary differentiating factor is that the MAX6959 adds input
> and GPIO support?  Debounce and interrupt support are merely features
> of input support.

What should I do here? Rephrase?

> > +  Type of the chip can be autodetected via specific register read,
> > +  and hence the features may be enabled in the driver at run-time.
> 
> I don't think you need to read that register, as the users of the
> features (keypad mapping, interrupts property, ...) also need to be
> described in DTS (once supported).

So, the idea that if DT describes those we will check the chip ID and
instantiate what is asked?

> > +  Given hardware is simple and does not provide any additional pins,
> > +  such as reset or enable.
> 
> Does this matter? I.e. is it important to say this in the bindings?

From Krzysztof's review of v1 I understood that it is important to say so
people wouldn't wonder if HW has support of that which is not implemented
(yet) or simply has no such pins.

Personally I would lean towards leaving this in the description.

..

Can you propose the full description text how you see it?
Andy Shevchenko Feb. 15, 2024, 11:18 a.m. UTC | #4 
On Thu, Feb 15, 2024 at 09:21:10AM +0100, Krzysztof Kozlowski wrote:
> On 12/02/2024 18:16, Andy Shevchenko wrote:
> > On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:

..

> > Add initial device tree documentation for Maxim MAX6958/6959.
> 
> Anyway you need to send the new version to the list, so the bot will
> test it.

Sure, that's the plan after applying first ones, so the only two can be left
in v4. I will wait for Geert's comments as well.

> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Thank you!
Geert Uytterhoeven Feb. 15, 2024, 12:26 p.m. UTC | #5 
Hi Andy,

On Thu, Feb 15, 2024 at 12:17 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, Feb 15, 2024 at 11:57:53AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 12, 2024 at 6:16 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Mon, Feb 12, 2024 at 07:14:53PM +0200, Andy Shevchenko wrote:
> > > +description:
> > > +  The Maxim MAX6958/6959 7-segment LED display controller provides
> > > +  an I2C interface to up to four 7-segment LED digits. The MAX6959
> > > +  in comparison to MAX6958 has the debounce and interrupt support.
> >
> > IUIC, the primary differentiating factor is that the MAX6959 adds input
> > and GPIO support?  Debounce and interrupt support are merely features
> > of input support.
>
> What should I do here? Rephrase?

    The Maxim MAX6958/6959 7-segment LED display controller provides
    an I2C interface to up to four 7-segment LED digits. The MAX6959
    adds input support.

> > > +  Type of the chip can be autodetected via specific register read,
> > > +  and hence the features may be enabled in the driver at run-time.
> >
> > I don't think you need to read that register, as the users of the
> > features (keypad mapping, interrupts property, ...) also need to be
> > described in DTS (once supported).
>
> So, the idea that if DT describes those we will check the chip ID and
> instantiate what is asked?

Even the check for the chip ID is probably not needed. E.g.
  - If the DTS has an interrupt property, (the future version of)
    the driver sets up the interrupt.
  - If the DTS has a linux,keymap, (the future version of) enables the
    keypad.

> > > +  Given hardware is simple and does not provide any additional pins,
> > > +  such as reset or enable.
> >
> > Does this matter? I.e. is it important to say this in the bindings?
>
> From Krzysztof's review of v1 I understood that it is important to say so
> people wouldn't wonder if HW has support of that which is not implemented
> (yet) or simply has no such pins.

It might be good to mention that in the commit description.
IMHO a list of all possible things that do not exist does not belong in the
bindings.

Gr{oetje,eeting}s,

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