* [PATCH v5 1/6] dt-bindings: vendor-prefixes: Add Titan Micro Electronics
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
@ 2022-02-25 21:10 ` Heiner Kallweit
2022-02-25 21:13 ` [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 Heiner Kallweit
` (7 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-02-25 21:10 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
From: Andreas Färber <afaerber@suse.de>
Assign vendor prefix "titanmec", matching their domain name.
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index e062a8187..6ffdec91f 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1238,6 +1238,8 @@ patternProperties:
description: Texas Instruments
"^tianma,.*":
description: Tianma Micro-electronics Co., Ltd.
+ "^titanmec,.*":
+ description: Shenzhen Titan Micro Electronics Co., Ltd.
"^tlm,.*":
description: Trusted Logic Mobility
"^tmt,.*":
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
2022-02-25 21:10 ` [PATCH v5 1/6] dt-bindings: vendor-prefixes: Add Titan Micro Electronics Heiner Kallweit
@ 2022-02-25 21:13 ` Heiner Kallweit
2022-03-04 23:07 ` Rob Herring
` (2 more replies)
2022-02-25 21:16 ` [PATCH v5 3/6] docs: ABI: document tm1628 attribute display_text Heiner Kallweit
` (6 subsequent siblings)
8 siblings, 3 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-02-25 21:13 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
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@suse.de>.
Co-developed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
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@suse.de>
+ - Heiner Kallweit <hkallweit1@gmail.com>
+
+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
+
+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
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/leds/common.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led-controller@0 {
+ compatible = "titanmec,tm1628";
+ reg = <0>;
+ spi-3-wire;
+ spi-lsb-first;
+ spi-max-frequency = <500000>;
+ titanmec,grid = /bits/ 8 <4 3 2 1>;
+ titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ alarm@5,4 {
+ reg = <5 4>;
+ function = LED_FUNCTION_ALARM;
+ };
+ };
+ };
+...
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-02-25 21:13 ` [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 Heiner Kallweit
@ 2022-03-04 23:07 ` Rob Herring
2022-03-18 20:50 ` Robin Murphy
2022-03-21 8:28 ` Krzysztof Kozlowski
2 siblings, 0 replies; 26+ messages in thread
From: Rob Herring @ 2022-03-04 23:07 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Neil Armstrong, Geert Uytterhoeven, linux-arm-kernel,
Miguel Ojeda, open list:ARM/Amlogic Meson...,
Martin Blumenstingl, Krzysztof Kozlowski, linux-spi, devicetree,
Jerome Brunet, Kevin Hilman, Andreas Färber, Rob Herring
On Fri, 25 Feb 2022 22:13:16 +0100, 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@suse.de>.
>
> Co-developed-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> 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
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-02-25 21:13 ` [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 Heiner Kallweit
2022-03-04 23:07 ` Rob Herring
@ 2022-03-18 20:50 ` Robin Murphy
2022-03-21 8:12 ` Geert Uytterhoeven
2022-03-21 8:34 ` Krzysztof Kozlowski
2022-03-21 8:28 ` Krzysztof Kozlowski
2 siblings, 2 replies; 26+ messages in thread
From: Robin Murphy @ 2022-03-18 20:50 UTC (permalink / raw)
To: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
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@suse.de>.
>
> Co-developed-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> 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@suse.de>
> + - Heiner Kallweit <hkallweit1@gmail.com>
> +
> +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.
Thanks,
Robin.
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led-controller@0 {
> + compatible = "titanmec,tm1628";
> + reg = <0>;
> + spi-3-wire;
> + spi-lsb-first;
> + spi-max-frequency = <500000>;
> + titanmec,grid = /bits/ 8 <4 3 2 1>;
> + titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + alarm@5,4 {
> + reg = <5 4>;
> + function = LED_FUNCTION_ALARM;
> + };
> + };
> + };
> +...
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-03-18 20:50 ` Robin Murphy
@ 2022-03-21 8:12 ` Geert Uytterhoeven
2022-04-19 22:31 ` Robin Murphy
2022-03-21 8:34 ` Krzysztof Kozlowski
1 sibling, 1 reply; 26+ messages in thread
From: Geert Uytterhoeven @ 2022-03-21 8:12 UTC (permalink / raw)
To: Robin Murphy
Cc: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda, linux-spi, devicetree,
linux-arm-kernel, open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
Hi Robin,
On Fri, Mar 18, 2022 at 9:50 PM Robin Murphy <robin.murphy@arm.com> 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@suse.de>.
> >
> > Co-developed-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Andreas Färber <afaerber@suse.de>
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> > +
> > +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).
Sounds similar to HT16K33?
> 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.
The problem with these devices is that there are thousands of different
ways to wire them, and coming up with a generic wiring description in
DT and writing code to handle that can be very hard.
For HT16K33 non-dot-matrix wirings, I just added extra compatible
values matching the wiring of a few known devices[1]. That way the
driver can handle them efficiently.
It does have the disadvantage that adding support for new devices
means introducing more compatible values, and adding more code.
Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-03-21 8:12 ` Geert Uytterhoeven
@ 2022-04-19 22:31 ` Robin Murphy
0 siblings, 0 replies; 26+ messages in thread
From: Robin Murphy @ 2022-04-19 22:31 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda, linux-spi, devicetree,
linux-arm-kernel, open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 2022-03-21 08:12, Geert Uytterhoeven wrote:
> Hi Robin,
>
> On Fri, Mar 18, 2022 at 9:50 PM Robin Murphy <robin.murphy@arm.com> 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@suse.de>.
>>>
>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>
>>> +
>>> +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).
>
> Sounds similar to HT16K33?
/me searches up a datasheet...
Keypad-wise, it appears so, however the display side of this
1618/1628/1638 family is very much tuned for 7-segment displays rather
than arbitrary dot-matrix ones.
I do recall when I was digging a few years ago, turning up an old Holtek
VFD driver which looked suspiciously like it might be the origin of the
particular 3-wire protocol and command set (including weird non-linear
brightness scale) which all these LED driver clones seem to borrow from,
but I can't now remember the part number :(
>> 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.
>
> The problem with these devices is that there are thousands of different
> ways to wire them, and coming up with a generic wiring description in
> DT and writing code to handle that can be very hard.
>
> For HT16K33 non-dot-matrix wirings, I just added extra compatible
> values matching the wiring of a few known devices[1]. That way the
> driver can handle them efficiently.
> It does have the disadvantage that adding support for new devices
> means introducing more compatible values, and adding more code.
>
> Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml
I think the display side of Heiner's binding is fine for what these
chips can do - I've finally had a bit more time to play with this
series, and (with minor driver hacks) it works just fine to describe my
TM1638 breakout board with an 8-digit display, where segments 8 and 9 of
each grid are respectively a decimal point and a discrete indicator LED,
managed as separate LED nodes.
However, I think you've indirectly addressed my outstanding concern
there - I wasn't aware of the "linux,keymap" property, but since that
brings its own implicit (row,column) addresses distinct from the DT
address space, it looks like that might be sufficient as a neat standard
way to extend this binding in future *without* any other changes.
Thanks,
Robin.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-03-18 20:50 ` Robin Murphy
2022-03-21 8:12 ` Geert Uytterhoeven
@ 2022-03-21 8:34 ` Krzysztof Kozlowski
2022-03-23 20:33 ` Heiner Kallweit
1 sibling, 1 reply; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-03-21 8:34 UTC (permalink / raw)
To: Robin Murphy, Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
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@suse.de>.
>>
>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> 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@suse.de>
>> + - Heiner Kallweit <hkallweit1@gmail.com>
>> +
>> +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.
Best regards,
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-03-21 8:34 ` Krzysztof Kozlowski
@ 2022-03-23 20:33 ` Heiner Kallweit
2022-03-30 5:54 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-03-23 20:33 UTC (permalink / raw)
To: Krzysztof Kozlowski, Robin Murphy, Rob Herring,
Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
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@suse.de>.
>>>
>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> 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@suse.de>
>>> + - Heiner Kallweit <hkallweit1@gmail.com>
>>> +
>>> +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.
> Best regards,
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-03-23 20:33 ` Heiner Kallweit
@ 2022-03-30 5:54 ` Heiner Kallweit
2022-04-19 23:04 ` Robin Murphy
0 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-03-30 5:54 UTC (permalink / raw)
To: Krzysztof Kozlowski, Robin Murphy, Rob Herring,
Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
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@suse.de>.
>>>>
>>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> 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@suse.de>
>>>> + - Heiner Kallweit <hkallweit1@gmail.com>
>>>> +
>>>> +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.
>> Best regards,
>> Best regards,
>> Krzysztof
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-03-30 5:54 ` Heiner Kallweit
@ 2022-04-19 23:04 ` Robin Murphy
2022-04-20 16:27 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Robin Murphy @ 2022-04-19 23:04 UTC (permalink / raw)
To: Heiner Kallweit, Krzysztof Kozlowski, Rob Herring,
Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
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@suse.de>.
>>>>>
>>>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>> 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@suse.de>
>>>>> + - Heiner Kallweit <hkallweit1@gmail.com>
>>>>> +
>>>>> +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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-04-19 23:04 ` Robin Murphy
@ 2022-04-20 16:27 ` Heiner Kallweit
0 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-20 16:27 UTC (permalink / raw)
To: Robin Murphy, Krzysztof Kozlowski, Rob Herring,
Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 20.04.2022 01:04, Robin Murphy wrote:
> 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@suse.de>.
>>>>>>
>>>>>> Co-developed-by: Andreas Färber <afaerber@suse.de>
>>>>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> ---
>>>>>> 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@suse.de>
>>>>>> + - Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> +
>>>>>> +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 for following up on the recent discussion. Good to see that the parrot
is not dead but was just resting. My reaction was based on a number of discussions
I witnessed that ended in nowhere. One example that comes to my mind is
LED subsystem support for network devices with hardware-triggered LEDs.
> Thanks,
> Robin.
Heiner
> [1] https://gitlab.arm.com/linux-arm/linux-rm/-/commits/tm1628
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
2022-02-25 21:13 ` [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 Heiner Kallweit
2022-03-04 23:07 ` Rob Herring
2022-03-18 20:50 ` Robin Murphy
@ 2022-03-21 8:28 ` Krzysztof Kozlowski
2 siblings, 0 replies; 26+ messages in thread
From: Krzysztof Kozlowski @ 2022-03-21 8:28 UTC (permalink / raw)
To: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 25/02/2022 22: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@suse.de>.
>
(...)
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + led-controller@0 {
> + compatible = "titanmec,tm1628";
> + reg = <0>;
> + spi-3-wire;
> + spi-lsb-first;
> + spi-max-frequency = <500000>;
> + titanmec,grid = /bits/ 8 <4 3 2 1>;
> + titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + alarm@5,4 {
A nit: generic node name, so "led".
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 3/6] docs: ABI: document tm1628 attribute display_text
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
2022-02-25 21:10 ` [PATCH v5 1/6] dt-bindings: vendor-prefixes: Add Titan Micro Electronics Heiner Kallweit
2022-02-25 21:13 ` [PATCH v5 2/6] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 Heiner Kallweit
@ 2022-02-25 21:16 ` Heiner Kallweit
2022-02-25 21:20 ` [PATCH v5 4/6] auxdisplay: add support for Titanmec TM1628 7 segment display controller Heiner Kallweit
` (5 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-02-25 21:16 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
Document the attribute for reading / writing the text to be displayed on
the 7 segment display.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628 | 7 +++++++
1 file changed, 7 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
diff --git a/Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628 b/Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
new file mode 100644
index 000000000..382757e72
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
@@ -0,0 +1,7 @@
+What: /sys/devices/.../display_text
+Date: February 2022
+Contact: Heiner Kallweit <hkallweit1@gmail.com>
+Description:
+ The text to be displayed on the 7 segment display.
+ Any printable character is allowed as input, but some
+ can not be displayed in a readable way with 7 segments.
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 4/6] auxdisplay: add support for Titanmec TM1628 7 segment display controller
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (2 preceding siblings ...)
2022-02-25 21:16 ` [PATCH v5 3/6] docs: ABI: document tm1628 attribute display_text Heiner Kallweit
@ 2022-02-25 21:20 ` Heiner Kallweit
2022-02-25 21:22 ` [PATCH v5 5/6] arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display Heiner Kallweit
` (4 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-02-25 21:20 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
This patch adds support for the Titanmec TM1628 7 segment display
controller. It's based on previous RFC work from Andreas Färber.
The RFC version placed the driver in the LED subsystem, but this was
NAK'ed by the LED maintainer. Therefore I moved the driver to
/drivers/auxdisplay what seems most reasonable to me.
Further changes to the RFC version:
- Driver can be built also w/o LED class support, for displays that
don't have any symbols to be exposed as LED's.
- Simplified the code and rewrote a lot of it.
- Driver is now kind of a MVP, but functionality should be sufficient
for most use cases.
- Use the existing 7 segment support in uapi/linux/map_to_7segment.h
as suggested by Geert Uytterhoeven.
Note: There's a number of chips from other manufacturers that are
almost identical, e.g. FD628, SM1628. Only difference I saw so
far is that they partially support other display modes.
TM1628: 6x12, 7x11
SM1628C: 4x13, 5x12, 6x11, 7x10
For typical displays on devices using these chips this
difference shouldn't matter.
Successfully tested on a TX3 Mini TV box that has an SM1628C and a
display with 4 digits and 7 symbols.
Co-developed-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- use Co-Developed-by
v4:
- switch to Co-developed-by
- style fixes
- fix potential write beyond end in display_text_store()
v5:
- add vendor prefix to controller-specific properties
---
drivers/auxdisplay/Kconfig | 11 ++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++++++++++++++++++++
3 files changed, 388 insertions(+)
create mode 100644 drivers/auxdisplay/tm1628.c
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 64012cda4..2764afc5c 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -203,6 +203,17 @@ config ARM_CHARLCD
line and the Linux version on the second line, but that's
still useful.
+config TM1628
+ tristate "TM1628 driver for LED 7/11 segment displays"
+ depends on SPI
+ depends on OF || COMPILE_TEST
+ help
+ Say Y to enable support for Titan Micro Electronics TM1628
+ LED controller.
+
+ It's a 3-wire SPI device controlling a two-dimensional grid of
+ LEDs. Dimming is applied to all outputs through an internal PWM.
+
menuconfig PARPORT_PANEL
tristate "Parallel port LCD/Keypad Panel support"
depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3..7728e17e1 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -14,3 +14,4 @@ obj-$(CONFIG_HT16K33) += ht16k33.o
obj-$(CONFIG_PARPORT_PANEL) += panel.o
obj-$(CONFIG_LCD2S) += lcd2s.o
obj-$(CONFIG_LINEDISP) += line-display.o
+obj-$(CONFIG_TM1628) += tm1628.o
diff --git a/drivers/auxdisplay/tm1628.c b/drivers/auxdisplay/tm1628.c
new file mode 100644
index 000000000..43d6f2626
--- /dev/null
+++ b/drivers/auxdisplay/tm1628.c
@@ -0,0 +1,376 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Titan Micro Electronics TM1628 LED controller
+ *
+ * Copyright (c) 2019 Andreas Färber
+ * Copyright (c) 2022 Heiner Kallweit
+ */
+
+#include <linux/ctype.h>
+#include <linux/delay.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/spi/spi.h>
+#include <uapi/linux/map_to_7segment.h>
+
+#define TM1628_CMD_DISPLAY_MODE (0 << 6)
+#define TM1628_DISPLAY_MODE_6_12 0x02
+#define TM1628_DISPLAY_MODE_7_11 0x03
+
+#define TM1628_CMD_DATA (1 << 6)
+#define TM1628_DATA_TEST_MODE BIT(3)
+#define TM1628_DATA_FIXED_ADDR BIT(2)
+#define TM1628_DATA_WRITE_DATA 0x00
+#define TM1628_DATA_READ_DATA 0x02
+
+#define TM1628_CMD_DISPLAY_CTRL (2 << 6)
+#define TM1628_DISPLAY_CTRL_DISPLAY_ON BIT(3)
+
+#define TM1628_CMD_SET_ADDRESS (3 << 6)
+
+#define TM1628_BRIGHTNESS_MAX 7
+#define NUM_LED_SEGS 7
+
+/* Physical limits, depending on the mode the chip may support less */
+#define MAX_GRID_SIZE 7
+#define MAX_SEGMENT_NUM 16
+
+struct tm1628_led {
+ struct led_classdev leddev;
+ struct tm1628 *ctrl;
+ u32 grid;
+ u32 seg;
+};
+
+struct tm1628 {
+ struct spi_device *spi;
+ __le16 data[MAX_GRID_SIZE];
+ struct mutex disp_lock;
+ char text[MAX_GRID_SIZE + 1];
+ u8 segment_mapping[NUM_LED_SEGS];
+ u8 grid[MAX_GRID_SIZE];
+ int grid_size;
+ struct tm1628_led leds[];
+};
+
+/* Command 1: Display Mode Setting */
+static int tm1628_set_display_mode(struct spi_device *spi, u8 grid_mode)
+{
+ const u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;
+
+ return spi_write(spi, &cmd, 1);
+}
+
+/* Command 3: Address Setting */
+static int tm1628_set_address(struct spi_device *spi, u8 offset)
+{
+ const u8 cmd = TM1628_CMD_SET_ADDRESS | (offset * sizeof(__le16));
+
+ return spi_write(spi, &cmd, 1);
+}
+
+/* Command 2: Data Setting */
+static int tm1628_write_data(struct spi_device *spi, unsigned int offset,
+ unsigned int len)
+{
+ struct tm1628 *s = spi_get_drvdata(spi);
+ const u8 cmd = TM1628_CMD_DATA | TM1628_DATA_WRITE_DATA;
+ struct spi_transfer xfers[] = {
+ {
+ .tx_buf = &cmd,
+ .len = 1,
+ },
+ {
+ .tx_buf = (__force void *)(s->data + offset),
+ .len = len * sizeof(__le16),
+ },
+ };
+
+ if (offset + len > MAX_GRID_SIZE) {
+ dev_err(&spi->dev, "Invalid data address offset %u len %u\n",
+ offset, len);
+ return -EINVAL;
+ }
+
+ tm1628_set_address(spi, offset);
+
+ return spi_sync_transfer(spi, xfers, ARRAY_SIZE(xfers));
+}
+
+/* Command 4: Display Control */
+static int tm1628_set_display_ctrl(struct spi_device *spi, bool on)
+{
+ u8 cmd = TM1628_CMD_DISPLAY_CTRL | TM1628_BRIGHTNESS_MAX;
+
+ if (on)
+ cmd |= TM1628_DISPLAY_CTRL_DISPLAY_ON;
+
+ return spi_write(spi, &cmd, 1);
+}
+
+static int tm1628_show_text(struct tm1628 *s)
+{
+ static SEG7_CONVERSION_MAP(map_seg7, MAP_ASCII7SEG_ALPHANUM);
+ int msg_len, i, ret;
+
+ msg_len = strlen(s->text);
+
+ mutex_lock(&s->disp_lock);
+
+ for (i = 0; i < s->grid_size; i++) {
+ int pos = s->grid[i] - 1;
+ int j, char7_raw, char7;
+
+ if (i >= msg_len) {
+ s->data[pos] = 0;
+ continue;
+ }
+
+ char7_raw = map_to_seg7(&map_seg7, s->text[i]);
+
+ for (j = 0, char7 = 0; j < NUM_LED_SEGS; j++) {
+ if (char7_raw & BIT(j))
+ char7 |= BIT(s->segment_mapping[j] - 1);
+ }
+
+ s->data[pos] = cpu_to_le16(char7);
+ }
+
+ ret = tm1628_write_data(s->spi, 0, s->grid_size);
+
+ mutex_unlock(&s->disp_lock);
+
+ return ret;
+}
+
+static int tm1628_led_set_brightness(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
+ struct tm1628 *s = led->ctrl;
+ int ret, offset = led->grid - 1;
+ __le16 bit = cpu_to_le16(BIT(led->seg - 1));
+
+ mutex_lock(&s->disp_lock);
+
+ if (brightness == LED_OFF)
+ s->data[offset] &= ~bit;
+ else
+ s->data[offset] |= bit;
+
+ ret = tm1628_write_data(s->spi, offset, 1);
+
+ mutex_unlock(&s->disp_lock);
+
+ return ret;
+}
+
+static enum led_brightness tm1628_led_get_brightness(struct led_classdev *led_cdev)
+{
+ struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
+ struct tm1628 *s = led->ctrl;
+ int offset = led->grid - 1;
+ __le16 bit = cpu_to_le16(BIT(led->seg - 1));
+ bool on;
+
+ mutex_lock(&s->disp_lock);
+ on = s->data[offset] & bit;
+ mutex_unlock(&s->disp_lock);
+
+ return on ? LED_ON : LED_OFF;
+}
+
+static int tm1628_register_led(struct tm1628 *s, struct fwnode_handle *node,
+ u32 grid, u32 seg, struct tm1628_led *led)
+{
+ struct device *dev = &s->spi->dev;
+ struct led_init_data init_data = { .fwnode = node };
+
+ led->ctrl = s;
+ led->grid = grid;
+ led->seg = seg;
+ led->leddev.max_brightness = LED_ON;
+ led->leddev.brightness_set_blocking = tm1628_led_set_brightness;
+ led->leddev.brightness_get = tm1628_led_get_brightness;
+
+ return devm_led_classdev_register_ext(dev, &led->leddev, &init_data);
+}
+
+static ssize_t display_text_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct tm1628 *s = dev_get_drvdata(dev);
+
+ return sysfs_emit(buf, "%s\n", s->text);
+}
+
+static ssize_t display_text_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct tm1628 *s = dev_get_drvdata(dev);
+ int ret, i;
+
+ for (i = 0; i < count && i < s->grid_size && isprint(buf[i]); i++)
+ s->text[i] = buf[i];
+
+ s->text[i] = '\0';
+
+ ret = tm1628_show_text(s);
+ if (ret < 0)
+ return ret;
+
+ return count;
+}
+
+static const DEVICE_ATTR_RW(display_text);
+
+static int tm1628_spi_probe(struct spi_device *spi)
+{
+ struct fwnode_handle *child;
+ unsigned int num_leds;
+ struct tm1628 *s;
+ int ret, i;
+
+ num_leds = device_get_child_node_count(&spi->dev);
+
+ s = devm_kzalloc(&spi->dev, struct_size(s, leds, num_leds), GFP_KERNEL);
+ if (!s)
+ return -ENOMEM;
+
+ s->spi = spi;
+ spi_set_drvdata(spi, s);
+
+ mutex_init(&s->disp_lock);
+
+ /* According to TM1628 datasheet */
+ msleep(200);
+
+ /* Clear screen */
+ ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
+ if (ret)
+ return ret;
+
+ /* For now we support 6x12 mode only. This should be sufficient for most use cases */
+ ret = tm1628_set_display_mode(spi, TM1628_DISPLAY_MODE_6_12);
+ if (ret)
+ return ret;
+
+ ret = tm1628_set_display_ctrl(spi, true);
+ if (ret)
+ return ret;
+
+ num_leds = 0;
+
+ if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
+ goto no_leds;
+
+ device_for_each_child_node(&spi->dev, child) {
+ u32 reg[2];
+
+ ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
+ if (ret) {
+ dev_err(&spi->dev, "Reading %s reg property failed (%d)\n",
+ fwnode_get_name(child), ret);
+ continue;
+ }
+
+ if (reg[0] == 0 || reg[0] > MAX_GRID_SIZE) {
+ dev_err(&spi->dev, "Invalid grid %u at %s\n",
+ reg[0], fwnode_get_name(child));
+ continue;
+ }
+
+ if (reg[1] == 0 || reg[1] > MAX_SEGMENT_NUM) {
+ dev_err(&spi->dev, "Invalid segment %u at %s\n",
+ reg[1], fwnode_get_name(child));
+ continue;
+ }
+
+ ret = tm1628_register_led(s, child, reg[0], reg[1], s->leds + num_leds);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to register LED %s (%d)\n",
+ fwnode_get_name(child), ret);
+ continue;
+ }
+ num_leds++;
+ }
+
+no_leds:
+ ret = device_property_count_u8(&spi->dev, "titanmec,grid");
+ if (ret < 1 || ret > MAX_GRID_SIZE) {
+ dev_err(&spi->dev, "Invalid display length (%d)\n", ret);
+ return -EINVAL;
+ }
+
+ s->grid_size = ret;
+
+ ret = device_property_read_u8_array(&spi->dev, "titanmec,grid", s->grid, s->grid_size);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < s->grid_size; i++) {
+ if (s->grid[i] < 1 || s->grid[i] > s->grid_size)
+ return -EINVAL;
+ }
+
+ ret = device_property_read_u8_array(&spi->dev, "titanmec,segment-mapping",
+ s->segment_mapping, NUM_LED_SEGS);
+ if (ret < 0)
+ return ret;
+
+ for (i = 0; i < NUM_LED_SEGS; i++) {
+ if (s->segment_mapping[i] < 1 || s->segment_mapping[i] > MAX_SEGMENT_NUM)
+ return -EINVAL;
+ }
+
+ ret = device_create_file(&spi->dev, &dev_attr_display_text);
+ if (ret)
+ return ret;
+
+ dev_info(&spi->dev, "Configured display with %u digits and %u symbols\n",
+ s->grid_size, num_leds);
+
+ return 0;
+}
+
+static void tm1628_spi_remove(struct spi_device *spi)
+{
+ device_remove_file(&spi->dev, &dev_attr_display_text);
+ tm1628_set_display_ctrl(spi, false);
+}
+
+static void tm1628_spi_shutdown(struct spi_device *spi)
+{
+ tm1628_set_display_ctrl(spi, false);
+}
+
+static const struct of_device_id tm1628_spi_of_matches[] = {
+ { .compatible = "titanmec,tm1628" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, tm1628_spi_of_matches);
+
+static const struct spi_device_id tm1628_spi_id_table[] = {
+ { "tm1628" },
+ {},
+};
+MODULE_DEVICE_TABLE(spi, tm1628_spi_id_table);
+
+static struct spi_driver tm1628_spi_driver = {
+ .probe = tm1628_spi_probe,
+ .remove = tm1628_spi_remove,
+ .shutdown = tm1628_spi_shutdown,
+ .id_table = tm1628_spi_id_table,
+
+ .driver = {
+ .name = "tm1628",
+ .of_match_table = tm1628_spi_of_matches,
+ },
+};
+module_spi_driver(tm1628_spi_driver);
+
+MODULE_DESCRIPTION("TM1628 LED controller driver");
+MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
+MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");
+MODULE_LICENSE("GPL");
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 5/6] arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (3 preceding siblings ...)
2022-02-25 21:20 ` [PATCH v5 4/6] auxdisplay: add support for Titanmec TM1628 7 segment display controller Heiner Kallweit
@ 2022-02-25 21:22 ` Heiner Kallweit
2022-02-25 21:22 ` [PATCH v5 6/6] MAINTAINERS: Add entry for tm1628 auxdisplay driver Heiner Kallweit
` (3 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-02-25 21:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
This patch adds support for the 7 segment display of the device.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v5:
- add vendor prefix to controller-specific properties
---
.../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts b/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
index 6705c2082..20bbd931e 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl-s905w-tx3-mini.dts
@@ -10,6 +10,7 @@
#include "meson-gxl-s905x.dtsi"
#include "meson-gx-p23x-q20x.dtsi"
+#include <dt-bindings/leds/common.h>
/ {
compatible = "oranth,tx3-mini", "amlogic,s905w", "amlogic,meson-gxl";
@@ -19,6 +20,64 @@ memory@0 {
device_type = "memory";
reg = <0x0 0x0 0x0 0x40000000>; /* 1 GiB or 2 GiB */
};
+
+ spi {
+ compatible = "spi-gpio";
+ sck-gpios = <&gpio GPIODV_27 GPIO_ACTIVE_HIGH>;
+ mosi-gpios = <&gpio GPIODV_26 GPIO_ACTIVE_HIGH>;
+ cs-gpios = <&gpio_ao GPIOAO_4 GPIO_ACTIVE_LOW>;
+ num-chipselects = <1>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ tm1628: led-controller@0 {
+ compatible = "titanmec,tm1628";
+ reg = <0>;
+ spi-3wire;
+ spi-lsb-first;
+ spi-rx-delay-us = <1>;
+ spi-max-frequency = <500000>;
+ #address-cells = <2>;
+ #size-cells = <0>;
+
+ titanmec,segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+ titanmec,grid = /bits/ 8 <4 3 2 1>;
+
+ alarm@5,1 {
+ reg = <5 1>;
+ function = LED_FUNCTION_ALARM;
+ };
+
+ usb@5,2 {
+ reg = <5 2>;
+ function = LED_FUNCTION_USB;
+ };
+ play@5,3 {
+ reg = <5 3>;
+ function = "play";
+ };
+
+ pause@5,4 {
+ reg = <5 4>;
+ function = "pause";
+ };
+
+ colon@5,5 {
+ reg = <5 5>;
+ function = "colon";
+ };
+
+ lan@5,6 {
+ reg = <5 6>;
+ function = LED_FUNCTION_LAN;
+ };
+
+ wlan@5,7 {
+ reg = <5 7>;
+ function = LED_FUNCTION_WLAN;
+ };
+ };
+ };
};
&ir {
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 6/6] MAINTAINERS: Add entry for tm1628 auxdisplay driver
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (4 preceding siblings ...)
2022-02-25 21:22 ` [PATCH v5 5/6] arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display Heiner Kallweit
@ 2022-02-25 21:22 ` Heiner Kallweit
2022-03-08 18:32 ` [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (2 subsequent siblings)
8 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-02-25 21:22 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index eb321d82f..d66da447d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19672,6 +19672,13 @@ W: http://sourceforge.net/projects/tlan/
F: Documentation/networking/device_drivers/ethernet/ti/tlan.rst
F: drivers/net/ethernet/ti/tlan.*
+TM1628 LED CONTROLLER DRIVER
+M: Heiner Kallweit <hkallweit1@gmail.com>
+S: Maintained
+F: Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
+F: Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
+F: drivers/auxdisplay/tm1628.c
+
TM6000 VIDEO4LINUX DRIVER
M: Mauro Carvalho Chehab <mchehab@kernel.org>
L: linux-media@vger.kernel.org
--
2.35.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (5 preceding siblings ...)
2022-02-25 21:22 ` [PATCH v5 6/6] MAINTAINERS: Add entry for tm1628 auxdisplay driver Heiner Kallweit
@ 2022-03-08 18:32 ` Heiner Kallweit
2022-03-16 0:38 ` Robin Murphy
2022-04-23 20:57 ` Miguel Ojeda
8 siblings, 0 replies; 26+ messages in thread
From: Heiner Kallweit @ 2022-03-08 18:32 UTC (permalink / raw)
To: Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven, Rob Herring, Andreas Färber,
Krzysztof Kozlowski
On 25.02.2022 22:09, Heiner Kallweit wrote:
> This series adds support for the Titanmec TM1628 7 segment display
> controller. It's based on previous RFC work from Andreas Färber.
> The RFC version placed the driver in the LED subsystem, but this was
> NAK'ed by the LED maintainer. Therefore I moved the driver to
> /drivers/auxdisplay what seems most reasonable to me.
>
> Further changes to the RFC version:
> - Driver can be built also w/o LED class support, for displays that
> don't have any symbols to be exposed as LED's.
> - Simplified the code and rewrote a lot of it.
> - Driver is now kind of a MVP, but functionality should be sufficient
> for most use cases.
> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h
> as suggested by Geert Uytterhoeven.
>
> Note: There's a number of chips from other manufacturers that are
> almost identical, e.g. FD628, SM1628. Only difference I saw so
> far is that they partially support other display modes.
> TM1628: 6x12, 7x11
> SM1628C: 4x13, 5x12, 6x11, 7x10
> For typical displays on devices using these chips this
> difference shouldn't matter.
>
> Successfully tested on a TX3 Mini TV box that has an SM1628C and a
> display with 4 digits and 7 symbols.
>
> v2:
> - (re-)add Andreas' SoB to two patches
> - fix YAML issues
> - include ctype.h explicitly
> - add info message in probe()
>
> v3:
> - remove patch 1 because it has been applied via the SPI tree already
> - fix remaining YAML issues in patch 2
> - follow Miguel's suggestion on usage of Co-Developed-by
>
> v4:
> - add patch for MAINTAINERS entry
> - incorporate Miguel's review comments
> - Replace Co-Developed-by with Co-developed-by (checkpatch)
> v5:
> - add vendor prefix to driver-specific dt properties
>
> Andreas Färber (1):
> dt-bindings: vendor-prefixes: Add Titan Micro Electronics
>
> Heiner Kallweit (5):
> dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
> docs: ABI: document tm1628 attribute display-text
> auxdisplay: add support for Titanmec TM1628 7 segment display
> controller
> arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment
> display
> MAINTAINERS: Add entry for tm1628 auxdisplay driver
>
> .../testing/sysfs-devices-auxdisplay-tm1628 | 7 +
> .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> MAINTAINERS | 7 +
> .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++
> drivers/auxdisplay/Kconfig | 11 +
> drivers/auxdisplay/Makefile | 1 +
> drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++
> 8 files changed, 555 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> create mode 100644 drivers/auxdisplay/tm1628.c
>
Hi Miguel,
the DT extensions have been acked/reviewed.
Any other feedback from your side? Or can it be applied as-is?
Heiner
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (6 preceding siblings ...)
2022-03-08 18:32 ` [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
@ 2022-03-16 0:38 ` Robin Murphy
2022-03-16 21:19 ` Heiner Kallweit
2022-04-23 20:57 ` Miguel Ojeda
8 siblings, 1 reply; 26+ messages in thread
From: Robin Murphy @ 2022-03-16 0:38 UTC (permalink / raw)
To: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 2022-02-25 21:09, Heiner Kallweit wrote:
> This series adds support for the Titanmec TM1628 7 segment display
> controller. It's based on previous RFC work from Andreas Färber.
> The RFC version placed the driver in the LED subsystem, but this was
> NAK'ed by the LED maintainer. Therefore I moved the driver to
> /drivers/auxdisplay what seems most reasonable to me.
>
> Further changes to the RFC version:
> - Driver can be built also w/o LED class support, for displays that
> don't have any symbols to be exposed as LED's.
> - Simplified the code and rewrote a lot of it.
> - Driver is now kind of a MVP, but functionality should be sufficient
> for most use cases.
> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h
> as suggested by Geert Uytterhoeven.
>
> Note: There's a number of chips from other manufacturers that are
> almost identical, e.g. FD628, SM1628. Only difference I saw so
> far is that they partially support other display modes.
> TM1628: 6x12, 7x11
> SM1628C: 4x13, 5x12, 6x11, 7x10
> For typical displays on devices using these chips this
> difference shouldn't matter.
>
> Successfully tested on a TX3 Mini TV box that has an SM1628C and a
> display with 4 digits and 7 symbols.
FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock
display which would mapped like so:
titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>;
titanmec,grid = /bits/ 8 <5 4 2 1>;
(grid 3 segment 2 is used for a colon in the middle)
If I bodge around the lack of support for non-contiguous grids, it does
otherwise work fairly well, other than being 6-segment displays because
it needs to be in display mode 1 to drive SEG13 rather than GRID6. I
wonder if we could be a bit cleverer about picking a display mode based
on the grid/segment numbers used?
I also have a couple of those TM1638 breakout boards with 8 digits, 8
single LEDs and 8 buttons that I might have a go with too. Have you
given any thought to how the DT binding might support inputs as well?
(The best time to be future-proof is before it's merged...)
Cheers,
Robin.
> v2:
> - (re-)add Andreas' SoB to two patches
> - fix YAML issues
> - include ctype.h explicitly
> - add info message in probe()
>
> v3:
> - remove patch 1 because it has been applied via the SPI tree already
> - fix remaining YAML issues in patch 2
> - follow Miguel's suggestion on usage of Co-Developed-by
>
> v4:
> - add patch for MAINTAINERS entry
> - incorporate Miguel's review comments
> - Replace Co-Developed-by with Co-developed-by (checkpatch)
> v5:
> - add vendor prefix to driver-specific dt properties
>
> Andreas Färber (1):
> dt-bindings: vendor-prefixes: Add Titan Micro Electronics
>
> Heiner Kallweit (5):
> dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
> docs: ABI: document tm1628 attribute display-text
> auxdisplay: add support for Titanmec TM1628 7 segment display
> controller
> arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment
> display
> MAINTAINERS: Add entry for tm1628 auxdisplay driver
>
> .../testing/sysfs-devices-auxdisplay-tm1628 | 7 +
> .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> MAINTAINERS | 7 +
> .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++
> drivers/auxdisplay/Kconfig | 11 +
> drivers/auxdisplay/Makefile | 1 +
> drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++
> 8 files changed, 555 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
> create mode 100644 drivers/auxdisplay/tm1628.c
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-03-16 0:38 ` Robin Murphy
@ 2022-03-16 21:19 ` Heiner Kallweit
2022-03-17 20:08 ` Robin Murphy
0 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-03-16 21:19 UTC (permalink / raw)
To: Robin Murphy, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 16.03.2022 01:38, Robin Murphy wrote:
> On 2022-02-25 21:09, Heiner Kallweit wrote:
>> This series adds support for the Titanmec TM1628 7 segment display
>> controller. It's based on previous RFC work from Andreas Färber.
>> The RFC version placed the driver in the LED subsystem, but this was
>> NAK'ed by the LED maintainer. Therefore I moved the driver to
>> /drivers/auxdisplay what seems most reasonable to me.
>>
>> Further changes to the RFC version:
>> - Driver can be built also w/o LED class support, for displays that
>> don't have any symbols to be exposed as LED's.
>> - Simplified the code and rewrote a lot of it.
>> - Driver is now kind of a MVP, but functionality should be sufficient
>> for most use cases.
>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h
>> as suggested by Geert Uytterhoeven.
>>
>> Note: There's a number of chips from other manufacturers that are
>> almost identical, e.g. FD628, SM1628. Only difference I saw so
>> far is that they partially support other display modes.
>> TM1628: 6x12, 7x11
>> SM1628C: 4x13, 5x12, 6x11, 7x10
>> For typical displays on devices using these chips this
>> difference shouldn't matter.
>>
>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a
>> display with 4 digits and 7 symbols.
>
> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so:
>
> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>;
> titanmec,grid = /bits/ 8 <5 4 2 1>;
>
> (grid 3 segment 2 is used for a colon in the middle)
>
> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used?
>
Definitely this could be one future extension. It could also consider that there's a number of more or less
identical chips from other vendors that differ primarily in the supported display modes.
> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...)
>
With regards to inputs at least I have no plans because I have no hw supporting input.
Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result.
Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version
may result in longer discussions until maintainer or I give up.
Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break
existing users. And I think that's the case.
> Cheers,
> Robin.
>
Heiner
>> v2:
>> - (re-)add Andreas' SoB to two patches
>> - fix YAML issues
>> - include ctype.h explicitly
>> - add info message in probe()
>>
>> v3:
>> - remove patch 1 because it has been applied via the SPI tree already
>> - fix remaining YAML issues in patch 2
>> - follow Miguel's suggestion on usage of Co-Developed-by
>>
>> v4:
>> - add patch for MAINTAINERS entry
>> - incorporate Miguel's review comments
>> - Replace Co-Developed-by with Co-developed-by (checkpatch)
>> v5:
>> - add vendor prefix to driver-specific dt properties
>>
>> Andreas Färber (1):
>> dt-bindings: vendor-prefixes: Add Titan Micro Electronics
>>
>> Heiner Kallweit (5):
>> dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628
>> docs: ABI: document tm1628 attribute display-text
>> auxdisplay: add support for Titanmec TM1628 7 segment display
>> controller
>> arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment
>> display
>> MAINTAINERS: Add entry for tm1628 auxdisplay driver
>>
>> .../testing/sysfs-devices-auxdisplay-tm1628 | 7 +
>> .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++
>> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
>> MAINTAINERS | 7 +
>> .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++
>> drivers/auxdisplay/Kconfig | 11 +
>> drivers/auxdisplay/Makefile | 1 +
>> drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++
>> 8 files changed, 555 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628
>> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml
>> create mode 100644 drivers/auxdisplay/tm1628.c
>>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-03-16 21:19 ` Heiner Kallweit
@ 2022-03-17 20:08 ` Robin Murphy
2022-03-17 21:49 ` Heiner Kallweit
0 siblings, 1 reply; 26+ messages in thread
From: Robin Murphy @ 2022-03-17 20:08 UTC (permalink / raw)
To: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 2022-03-16 21:19, Heiner Kallweit wrote:
> On 16.03.2022 01:38, Robin Murphy wrote:
>> On 2022-02-25 21:09, Heiner Kallweit wrote:
>>> This series adds support for the Titanmec TM1628 7 segment display
>>> controller. It's based on previous RFC work from Andreas Färber.
>>> The RFC version placed the driver in the LED subsystem, but this was
>>> NAK'ed by the LED maintainer. Therefore I moved the driver to
>>> /drivers/auxdisplay what seems most reasonable to me.
>>>
>>> Further changes to the RFC version:
>>> - Driver can be built also w/o LED class support, for displays that
>>> don't have any symbols to be exposed as LED's.
>>> - Simplified the code and rewrote a lot of it.
>>> - Driver is now kind of a MVP, but functionality should be sufficient
>>> for most use cases.
>>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h
>>> as suggested by Geert Uytterhoeven.
>>>
>>> Note: There's a number of chips from other manufacturers that are
>>> almost identical, e.g. FD628, SM1628. Only difference I saw so
>>> far is that they partially support other display modes.
>>> TM1628: 6x12, 7x11
>>> SM1628C: 4x13, 5x12, 6x11, 7x10
>>> For typical displays on devices using these chips this
>>> difference shouldn't matter.
>>>
>>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a
>>> display with 4 digits and 7 symbols.
>>
>> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so:
>>
>> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>;
>> titanmec,grid = /bits/ 8 <5 4 2 1>;
>>
>> (grid 3 segment 2 is used for a colon in the middle)
>>
>> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used?
>>
> Definitely this could be one future extension. It could also consider that there's a number of more or less
> identical chips from other vendors that differ primarily in the supported display modes.
>
>> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...)
>>
> With regards to inputs at least I have no plans because I have no hw supporting input.
FWIW, if you've got a board with exposed GPIO/SPI headers, searching
"TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards.
I believe they're quite popular with the Arduino crowd, so I expect that
may well carry over to the Raspberry Pi crowd once they get wind of a
kernel driver that can be driven by DT overlays.
> Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result.
> Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version
> may result in longer discussions until maintainer or I give up.
Unfortunately the principle is that DT bindings describe the device, not
whatever the current level of Linux driver support for it might be.
Perhaps I'm a little sensitised since I'm currently feeling the pain of
extending a decade-old binding with functionality that was overlooked at
the time, and not breaking compatibility is now rather awkward.
I'm not suggesting that there needs to be any support implemented in the
driver, just to be certain that we're not painting ourselves into a
corner with the binding.
> Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break
> existing users. And I think that's the case.
May I ask what you have in mind? I figure that inputs would most likely
want to be described individually, similarly to the gpio-keys binding,
which would lend itself to having them as child nodes, except that
doesn't fit with the current scheme of child nodes having to be LEDs
addressed by (grid,segment). I suppose there is a possible escape hatch
of abusing unused addresses, e.g. saying a node at address (0,n) is
input n rather than an LED segment, but that seems pretty horrid (and
I'm not sure how well schema could validate it). Or possibly pretending
to also be a GPIO controller to reference from a separate gpio-keys
node, but again that seems ugly and more like something to only do if
there's no other option.
IMO it would be cleanest just to have an extra level of hierarchy, e.g.:
led-controller@0 {
compatible = "titanmec,tm1628";
...
leds {
#address-cells = <2>;
#size-cells = <0>;
alarm@5,4 {
...
};
};
};
That way there's clearly almost no risk of breakage if an additional
"inputs" node with its own children turns up later. Plus it should also
be a trivial change to the current driver, compared to having to
implement trick special cases or whole other APIs down the line - of
course bindings should not be designed expressly for ease of driver
implementation, but if they do work out that way it's usually a good sign :)
Thanks,
Robin.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-03-17 20:08 ` Robin Murphy
@ 2022-03-17 21:49 ` Heiner Kallweit
2022-03-18 20:13 ` Robin Murphy
0 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-03-17 21:49 UTC (permalink / raw)
To: Robin Murphy, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 17.03.2022 21:08, Robin Murphy wrote:
> On 2022-03-16 21:19, Heiner Kallweit wrote:
>> On 16.03.2022 01:38, Robin Murphy wrote:
>>> On 2022-02-25 21:09, Heiner Kallweit wrote:
>>>> This series adds support for the Titanmec TM1628 7 segment display
>>>> controller. It's based on previous RFC work from Andreas Färber.
>>>> The RFC version placed the driver in the LED subsystem, but this was
>>>> NAK'ed by the LED maintainer. Therefore I moved the driver to
>>>> /drivers/auxdisplay what seems most reasonable to me.
>>>>
>>>> Further changes to the RFC version:
>>>> - Driver can be built also w/o LED class support, for displays that
>>>> don't have any symbols to be exposed as LED's.
>>>> - Simplified the code and rewrote a lot of it.
>>>> - Driver is now kind of a MVP, but functionality should be sufficient
>>>> for most use cases.
>>>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h
>>>> as suggested by Geert Uytterhoeven.
>>>>
>>>> Note: There's a number of chips from other manufacturers that are
>>>> almost identical, e.g. FD628, SM1628. Only difference I saw so
>>>> far is that they partially support other display modes.
>>>> TM1628: 6x12, 7x11
>>>> SM1628C: 4x13, 5x12, 6x11, 7x10
>>>> For typical displays on devices using these chips this
>>>> difference shouldn't matter.
>>>>
>>>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a
>>>> display with 4 digits and 7 symbols.
>>>
>>> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so:
>>>
>>> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>;
>>> titanmec,grid = /bits/ 8 <5 4 2 1>;
>>>
>>> (grid 3 segment 2 is used for a colon in the middle)
>>>
>>> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used?
>>>
>> Definitely this could be one future extension. It could also consider that there's a number of more or less
>> identical chips from other vendors that differ primarily in the supported display modes.
>>
>>> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...)
>>>
>> With regards to inputs at least I have no plans because I have no hw supporting input.
>
> FWIW, if you've got a board with exposed GPIO/SPI headers, searching "TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards. I believe they're quite popular with the Arduino crowd, so I expect that may well carry over to the Raspberry Pi crowd once they get wind of a kernel driver that can be driven by DT overlays.
>
>> Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result.
>> Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version
>> may result in longer discussions until maintainer or I give up.
>
> Unfortunately the principle is that DT bindings describe the device, not whatever the current level of Linux driver support for it might be. Perhaps I'm a little sensitised since I'm currently feeling the pain of extending a decade-old binding with functionality that was overlooked at the time, and not breaking compatibility is now rather awkward.
>
> I'm not suggesting that there needs to be any support implemented in the driver, just to be certain that we're not painting ourselves into a corner with the binding.
>
>> Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break
>> existing users. And I think that's the case.
>
> May I ask what you have in mind? I figure that inputs would most likely want to be described individually, similarly to the gpio-keys binding, which would lend itself to having them as child nodes, except that doesn't fit with the current scheme of child nodes having to be LEDs addressed by (grid,segment). I suppose there is a possible escape hatch of abusing unused addresses, e.g. saying a node at address (0,n) is input n rather than an LED segment, but that seems pretty horrid (and I'm not sure how well schema could validate it). Or possibly pretending to also be a GPIO controller to reference from a separate gpio-keys node, but again that seems ugly and more like something to only do if there's no other option.
>
Not being an expert in OF stuff I'm just focused on getting support for the hw I own.
I tried to do this in the most simple and generic way so that others can follow-up
and add additional functionality.
> IMO it would be cleanest just to have an extra level of hierarchy, e.g.:
>
>
> led-controller@0 {
> compatible = "titanmec,tm1628";
> ...
>
> leds {
> #address-cells = <2>;
> #size-cells = <0>;
>
> alarm@5,4 {
> ...
> };
> };
> };
>
> That way there's clearly almost no risk of breakage if an additional "inputs" node with its own children turns up later. Plus it should also be a trivial change to the current driver, compared to having to implement trick special cases or whole other APIs down the line - of course bindings should not be designed expressly for ease of driver implementation, but if they do work out that way it's usually a good sign :)
>
> Thanks,
> Robin.
Heiner
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-03-17 21:49 ` Heiner Kallweit
@ 2022-03-18 20:13 ` Robin Murphy
0 siblings, 0 replies; 26+ messages in thread
From: Robin Murphy @ 2022-03-18 20:13 UTC (permalink / raw)
To: Heiner Kallweit, Rob Herring, Krzysztof Kozlowski,
Andreas Färber, Miguel Ojeda
Cc: linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 2022-03-17 21:49, Heiner Kallweit wrote:
> On 17.03.2022 21:08, Robin Murphy wrote:
>> On 2022-03-16 21:19, Heiner Kallweit wrote:
>>> On 16.03.2022 01:38, Robin Murphy wrote:
>>>> On 2022-02-25 21:09, Heiner Kallweit wrote:
>>>>> This series adds support for the Titanmec TM1628 7 segment display
>>>>> controller. It's based on previous RFC work from Andreas Färber.
>>>>> The RFC version placed the driver in the LED subsystem, but this was
>>>>> NAK'ed by the LED maintainer. Therefore I moved the driver to
>>>>> /drivers/auxdisplay what seems most reasonable to me.
>>>>>
>>>>> Further changes to the RFC version:
>>>>> - Driver can be built also w/o LED class support, for displays that
>>>>> don't have any symbols to be exposed as LED's.
>>>>> - Simplified the code and rewrote a lot of it.
>>>>> - Driver is now kind of a MVP, but functionality should be sufficient
>>>>> for most use cases.
>>>>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h
>>>>> as suggested by Geert Uytterhoeven.
>>>>>
>>>>> Note: There's a number of chips from other manufacturers that are
>>>>> almost identical, e.g. FD628, SM1628. Only difference I saw so
>>>>> far is that they partially support other display modes.
>>>>> TM1628: 6x12, 7x11
>>>>> SM1628C: 4x13, 5x12, 6x11, 7x10
>>>>> For typical displays on devices using these chips this
>>>>> difference shouldn't matter.
>>>>>
>>>>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a
>>>>> display with 4 digits and 7 symbols.
>>>>
>>>> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so:
>>>>
>>>> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>;
>>>> titanmec,grid = /bits/ 8 <5 4 2 1>;
>>>>
>>>> (grid 3 segment 2 is used for a colon in the middle)
>>>>
>>>> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used?
>>>>
>>> Definitely this could be one future extension. It could also consider that there's a number of more or less
>>> identical chips from other vendors that differ primarily in the supported display modes.
>>>
>>>> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...)
>>>>
>>> With regards to inputs at least I have no plans because I have no hw supporting input.
>>
>> FWIW, if you've got a board with exposed GPIO/SPI headers, searching "TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards. I believe they're quite popular with the Arduino crowd, so I expect that may well carry over to the Raspberry Pi crowd once they get wind of a kernel driver that can be driven by DT overlays.
>>
>>> Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result.
>>> Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version
>>> may result in longer discussions until maintainer or I give up.
>>
>> Unfortunately the principle is that DT bindings describe the device, not whatever the current level of Linux driver support for it might be. Perhaps I'm a little sensitised since I'm currently feeling the pain of extending a decade-old binding with functionality that was overlooked at the time, and not breaking compatibility is now rather awkward.
>>
>> I'm not suggesting that there needs to be any support implemented in the driver, just to be certain that we're not painting ourselves into a corner with the binding.
>>
>>> Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break
>>> existing users. And I think that's the case.
>>
>> May I ask what you have in mind? I figure that inputs would most likely want to be described individually, similarly to the gpio-keys binding, which would lend itself to having them as child nodes, except that doesn't fit with the current scheme of child nodes having to be LEDs addressed by (grid,segment). I suppose there is a possible escape hatch of abusing unused addresses, e.g. saying a node at address (0,n) is input n rather than an LED segment, but that seems pretty horrid (and I'm not sure how well schema could validate it). Or possibly pretending to also be a GPIO controller to reference from a separate gpio-keys node, but again that seems ugly and more like something to only do if there's no other option.
>>
>
> Not being an expert in OF stuff I'm just focused on getting support for the hw I own.
> I tried to do this in the most simple and generic way so that others can follow-up
> and add additional functionality.
Sure, I appreciate that, and what I'm saying is that while what we
currently have is pleasantly simple, I think it's actually a little
*too* simple and not generic enough to extend easily. I'm more than
happy to send patches adding the functionality I'm interested in to the
driver once it's merged, but I can't make significant changes to the
binding at that point and break it for early adopters. But let me go
make proper review comments on the patch rather than confusing
meta-review here...
Thanks,
Robin.
>
>
>> IMO it would be cleanest just to have an extra level of hierarchy, e.g.:
>>
>>
>> led-controller@0 {
>> compatible = "titanmec,tm1628";
>> ...
>>
>> leds {
>> #address-cells = <2>;
>> #size-cells = <0>;
>>
>> alarm@5,4 {
>> ...
>> };
>> };
>> };
>>
>> That way there's clearly almost no risk of breakage if an additional "inputs" node with its own children turns up later. Plus it should also be a trivial change to the current driver, compared to having to implement trick special cases or whole other APIs down the line - of course bindings should not be designed expressly for ease of driver implementation, but if they do work out that way it's usually a good sign :)
>>
>> Thanks,
>> Robin.
>
> Heiner
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-02-25 21:09 [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller Heiner Kallweit
` (7 preceding siblings ...)
2022-03-16 0:38 ` Robin Murphy
@ 2022-04-23 20:57 ` Miguel Ojeda
2022-04-24 9:06 ` Heiner Kallweit
8 siblings, 1 reply; 26+ messages in thread
From: Miguel Ojeda @ 2022-04-23 20:57 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Rob Herring, Krzysztof Kozlowski, Andreas Färber,
Miguel Ojeda, linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On Fri, Feb 25, 2022 at 10:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> This series adds support for the Titanmec TM1628 7 segment display
> controller. It's based on previous RFC work from Andreas Färber.
AFAIU the discussion has converged at this point, correct? Is there
any feedback left to address?
Cheers,
Miguel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-04-23 20:57 ` Miguel Ojeda
@ 2022-04-24 9:06 ` Heiner Kallweit
2022-05-12 12:46 ` Robin Murphy
0 siblings, 1 reply; 26+ messages in thread
From: Heiner Kallweit @ 2022-04-24 9:06 UTC (permalink / raw)
To: Miguel Ojeda, Robin Murphy
Cc: Rob Herring, Krzysztof Kozlowski, Andreas Färber,
Miguel Ojeda, linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 23.04.2022 22:57, Miguel Ojeda wrote:
> On Fri, Feb 25, 2022 at 10:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> This series adds support for the Titanmec TM1628 7 segment display
>> controller. It's based on previous RFC work from Andreas Färber.
>
> AFAIU the discussion has converged at this point, correct? Is there
> any feedback left to address?
>
Still open is to define DT bindings that can support also the key input
feature of the chip. Robin picked up this topic and has some ideas.
> Cheers,
> Miguel
Heiner
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 0/6] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller
2022-04-24 9:06 ` Heiner Kallweit
@ 2022-05-12 12:46 ` Robin Murphy
0 siblings, 0 replies; 26+ messages in thread
From: Robin Murphy @ 2022-05-12 12:46 UTC (permalink / raw)
To: Heiner Kallweit, Miguel Ojeda
Cc: Rob Herring, Krzysztof Kozlowski, Andreas Färber,
Miguel Ojeda, linux-spi, devicetree, linux-arm-kernel,
open list:ARM/Amlogic Meson...,
Jerome Brunet, Martin Blumenstingl, Kevin Hilman, Neil Armstrong,
Geert Uytterhoeven
On 2022-04-24 10:06, Heiner Kallweit wrote:
> On 23.04.2022 22:57, Miguel Ojeda wrote:
>> On Fri, Feb 25, 2022 at 10:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>
>>> This series adds support for the Titanmec TM1628 7 segment display
>>> controller. It's based on previous RFC work from Andreas Färber.
>>
>> AFAIU the discussion has converged at this point, correct? Is there
>> any feedback left to address?
>>
> Still open is to define DT bindings that can support also the key input
> feature of the chip. Robin picked up this topic and has some ideas.
Sorry this slipped through the cracks again... :(
As mentioned, I think the discovery of the "linux,keymap" alleviates the
concern I had with the binding - it seemed ugly to have to invent a
device-specific property that worked that way, but if a common one
already exists that's a different matter. I'm pretty confident now that
the ideas I have for supporting the input side of things and the other
16x8 chips that I have here can all be done as pure additions with no
compatibility concerns, so from my PoV I'm happy if you want to go ahead
and land this as-is for 5.19, and I can send patches later.
Cheers,
Robin.
^ permalink raw reply [flat|nested] 26+ messages in thread