Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628

Robin Murphy <robin.murphy@xxxxxxx> · Wed, 20 Apr 2022 00:04:01 +0100

On 2022-03-30 06:54, Heiner Kallweit wrote:
On 23.03.2022 21:33, Heiner Kallweit wrote:
On 21.03.2022 09:34, Krzysztof Kozlowski wrote:
On 18/03/2022 21:50, Robin Murphy wrote:
On 2022-02-25 21:13, Heiner Kallweit wrote:
Add a YAML schema binding for TM1628 auxdisplay
(7/11-segment LED) controller.

This patch is partially based on previous work from
Andreas Färber <afaerber@xxxxxxx>.

Co-developed-by: Andreas Färber <afaerber@xxxxxxx>
Signed-off-by: Andreas Färber <afaerber@xxxxxxx>
Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
---
v5:
- add vendor prefix to driver-specific properties
---
   .../bindings/auxdisplay/titanmec,tm1628.yaml  | 92 +++++++++++++++++++
   1 file changed, 92 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
new file mode 100644
index 000000000..2a1ef692c
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
@@ -0,0 +1,92 @@
+# SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1628.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Titan Micro Electronics TM1628 LED controller
+
+maintainers:
+  - Andreas Färber <afaerber@xxxxxxx>
+  - Heiner Kallweit <hkallweit1@xxxxxxxxx>
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: titanmec,tm1628
+
+  reg:
+    maxItems: 1
+
+  titanmec,grid:
+    description:
+      Mapping of display digit position to grid number.
+      This implicitly defines the display size.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    minItems: 1
+    maxItems: 7
+
+  titanmec,segment-mapping:
+    description:
+      Mapping of 7 segment display segments A-G to bit numbers 1-12.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    minItems: 7
+    maxItems: 7
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 0
+
+required:
+  - compatible
+  - reg

Would it be fair to say that "spi-lsb-first" and "spi-3wire" are also
required? The chips aren't configurable so won't exactly be usable any
other way. Furthermore I believe the transmission format actually works
out equivalent to SPI mode 3, so should warrant "spi-cpha" and
"spi-cpol" as well.

+
+patternProperties:
+  "^.*@[1-7],([1-9]|1[0-6])$":
+    type: object
+    $ref: /schemas/leds/common.yaml#
+    unevaluatedProperties: false
+    description: |
+      Properties for a single LED.
+
+    properties:
+      reg:
+        description: |
+          1-based grid number, followed by 1-based segment bit number.
+        maxItems: 1
+
+    required:
+      - reg

I'm concerned that this leaves us no room to support the additional
keypad functionality in future. Having now double-checked a datasheet,
the inputs are also a two-dimensional mux (sharing the segment lines),
so the device effectively has two distinct but numerically-overlapping
child address spaces - one addressed by (grid,segment) and the other by
(segment,key).

Rob, Krysztof, any thoughts on the best DT idiom to leave accommodation
for that? I'm thinking either require an intermediate node to contain
each notional address space, or perhaps add another leading address cell
to select between them? I don't believe any of these things have further
functionality beyond that.

I think intermediate nodes - leds, keys - are more appropriate, because
it is self-describing. Additional address space number would require
decoding this "0" or "1" into LED/key. For complex devices - like PMICs
with regulators, RTC and clocks - we already have such patterns.

Then it's just the question who can implement such an intermediate node
based on what has been done so far.

As it is now it seems we end up with empty hands again and have to wait
further two years for the next one to make an attempt.
That's a pity because for most users the relevant use cases are supported.

Or, y'know, we could just reach a productive conclusion rather than 
doom-and-gloom catastrophising. I apologise for not having much time for 
non-work-related kernel hacking at the moment, but it didn't seem 
particularly urgent to follow up on this in the middle of a merge window 
anyway. In the course of helpfully being left to address my own review 
feedback, I did eventually get round to implementing the intermediate 
"leds" node[1] last weekend, but having now stumbled across the 
matrix-keymap helpers and common "linux,keymap" property, I'm personally 
inclined to think that that's even cleaner than a "keys" node with 
children that we'd have to write more parsing code for, and thus may 
well make the whole intermediate node notion moot anyway. If only anyone 
had pointed it out sooner...

Thanks,
Robin.

[1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/tm1628






