[PATCH v3 0/5] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller

[PATCH v3 0/5] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

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

Andreas Färber (1):
  dt-bindings: vendor-prefixes: Add Titan Micro Electronics

Heiner Kallweit (4):
  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

 .../testing/sysfs-devices-auxdisplay-tm1628   |   7 +
 .../bindings/auxdisplay/titanmec,tm1628.yaml  |  92 +++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts  |  59 +++
 drivers/auxdisplay/Kconfig                    |  10 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/tm1628.c                   | 376 ++++++++++++++++++
 7 files changed, 547 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

-- 
2.35.1

[PATCH v3 0/5] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

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

Andreas Färber (1):
  dt-bindings: vendor-prefixes: Add Titan Micro Electronics

Heiner Kallweit (4):
  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

 .../testing/sysfs-devices-auxdisplay-tm1628   |   7 +
 .../bindings/auxdisplay/titanmec,tm1628.yaml  |  92 +++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts  |  59 +++
 drivers/auxdisplay/Kconfig                    |  10 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/tm1628.c                   | 376 ++++++++++++++++++
 7 files changed, 547 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

-- 
2.35.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 0/5] auxdisplay: Add support for the Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

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

Andreas Färber (1):
  dt-bindings: vendor-prefixes: Add Titan Micro Electronics

Heiner Kallweit (4):
  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

 .../testing/sysfs-devices-auxdisplay-tm1628   |   7 +
 .../bindings/auxdisplay/titanmec,tm1628.yaml  |  92 +++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts  |  59 +++
 drivers/auxdisplay/Kconfig                    |  10 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/tm1628.c                   | 376 ++++++++++++++++++
 7 files changed, 547 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

-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/5] dt-bindings: vendor-prefixes: Add Titan Micro Electronics

[Heiner Kallweit]

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

[PATCH v3 1/5] dt-bindings: vendor-prefixes: Add Titan Micro Electronics

[Heiner Kallweit]

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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 1/5] dt-bindings: vendor-prefixes: Add Titan Micro Electronics

[Heiner Kallweit]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/5] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628

[Heiner Kallweit]

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>
Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- fix remaining YAML issues
- use Co-Developed-by
---
 .../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
+
+  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
+
+  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>;
+            grid = /bits/ 8 <4 3 2 1>;
+            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+            #address-cells = <2>;
+            #size-cells = <0>;
+
+            alarmn@5,4 {
+                reg = <5 4>;
+                function = LED_FUNCTION_ALARM;
+            };
+        };
+    };
+...
-- 
2.35.1

[PATCH v3 2/5] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628

[Heiner Kallweit]

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>
Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- fix remaining YAML issues
- use Co-Developed-by
---
 .../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
+
+  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
+
+  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>;
+            grid = /bits/ 8 <4 3 2 1>;
+            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+            #address-cells = <2>;
+            #size-cells = <0>;
+
+            alarmn@5,4 {
+                reg = <5 4>;
+                function = LED_FUNCTION_ALARM;
+            };
+        };
+    };
+...
-- 
2.35.1



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 2/5] dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628

[Heiner Kallweit]

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>
Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- fix remaining YAML issues
- use Co-Developed-by
---
 .../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
+
+  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
+
+  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>;
+            grid = /bits/ 8 <4 3 2 1>;
+            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+            #address-cells = <2>;
+            #size-cells = <0>;
+
+            alarmn@5,4 {
+                reg = <5 4>;
+                function = LED_FUNCTION_ALARM;
+            };
+        };
+    };
+...
-- 
2.35.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/5] docs: ABI: document tm1628 attribute display-text

[Heiner Kallweit]

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

[PATCH v3 3/5] docs: ABI: document tm1628 attribute display-text

[Heiner Kallweit]

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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 3/5] docs: ABI: document tm1628 attribute display-text

[Heiner Kallweit]

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

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>
Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- use Co-Developed-by
---
 drivers/auxdisplay/Kconfig  |  10 +
 drivers/auxdisplay/Makefile |   1 +
 drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++++++++++++++++++++
 3 files changed, 387 insertions(+)
 create mode 100644 drivers/auxdisplay/tm1628.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 64012cda4..25ef2e452 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -203,6 +203,16 @@ 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..b802b5c30
--- /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
+ */
+
+#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
+
+/* 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[7];
+	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)
+{
+	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)
+{
+	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);
+	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 i, ret;
+
+	int msg_len = strlen(s->text);
+
+	mutex_lock(&s->disp_lock);
+
+	for (i = 0; i < s->grid_size; i++) {
+		int pos = s->grid[i] - 1;
+
+		if (i < msg_len) {
+			int char7_raw = map_to_seg7(&map_seg7, s->text[i]);
+			int j, char7;
+
+			for (j = 0, char7 = 0; j < 7; j++) {
+				if (char7_raw & BIT(j))
+					char7 |= BIT(s->segment_mapping[j] - 1);
+			}
+
+			s->data[pos] = cpu_to_le16(char7);
+		} else {
+			s->data[pos] = 0;
+		}
+	}
+
+	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 offset, ret;
+	__le16 bit;
+
+	offset = led->grid - 1;
+	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;
+	__le16 bit;
+	bool on;
+
+	offset = led->grid - 1;
+	bit = cpu_to_le16(BIT(led->seg - 1));
+
+	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;
+
+	if (count > s->grid_size + 1) /* consider trailing newline */
+		return -E2BIG;
+
+	for (i = 0; i < count && 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);
+
+	msleep(200); /* according to TM1628 datasheet */
+
+	/* clear screen */
+	ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
+	if (ret)
+		return ret;
+	/* Assume that subsequent SPI transfers will be ok if first was ok */
+
+	/* For now we support 6x12 mode only. This should be sufficient for most use cases */
+	tm1628_set_display_mode(spi, TM1628_DISPLAY_MODE_6_12);
+
+	tm1628_set_display_ctrl(spi, true);
+
+	if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
+		goto no_leds;
+
+	num_leds = 0;
+
+	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, "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, "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, "segment-mapping", s->segment_mapping, 7);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < 7; 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");
+MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

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>
Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- use Co-Developed-by
---
 drivers/auxdisplay/Kconfig  |  10 +
 drivers/auxdisplay/Makefile |   1 +
 drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++++++++++++++++++++
 3 files changed, 387 insertions(+)
 create mode 100644 drivers/auxdisplay/tm1628.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 64012cda4..25ef2e452 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -203,6 +203,16 @@ 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..b802b5c30
--- /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
+ */
+
+#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
+
+/* 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[7];
+	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)
+{
+	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)
+{
+	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);
+	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 i, ret;
+
+	int msg_len = strlen(s->text);
+
+	mutex_lock(&s->disp_lock);
+
+	for (i = 0; i < s->grid_size; i++) {
+		int pos = s->grid[i] - 1;
+
+		if (i < msg_len) {
+			int char7_raw = map_to_seg7(&map_seg7, s->text[i]);
+			int j, char7;
+
+			for (j = 0, char7 = 0; j < 7; j++) {
+				if (char7_raw & BIT(j))
+					char7 |= BIT(s->segment_mapping[j] - 1);
+			}
+
+			s->data[pos] = cpu_to_le16(char7);
+		} else {
+			s->data[pos] = 0;
+		}
+	}
+
+	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 offset, ret;
+	__le16 bit;
+
+	offset = led->grid - 1;
+	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;
+	__le16 bit;
+	bool on;
+
+	offset = led->grid - 1;
+	bit = cpu_to_le16(BIT(led->seg - 1));
+
+	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;
+
+	if (count > s->grid_size + 1) /* consider trailing newline */
+		return -E2BIG;
+
+	for (i = 0; i < count && 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);
+
+	msleep(200); /* according to TM1628 datasheet */
+
+	/* clear screen */
+	ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
+	if (ret)
+		return ret;
+	/* Assume that subsequent SPI transfers will be ok if first was ok */
+
+	/* For now we support 6x12 mode only. This should be sufficient for most use cases */
+	tm1628_set_display_mode(spi, TM1628_DISPLAY_MODE_6_12);
+
+	tm1628_set_display_ctrl(spi, true);
+
+	if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
+		goto no_leds;
+
+	num_leds = 0;
+
+	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, "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, "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, "segment-mapping", s->segment_mapping, 7);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < 7; 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");
+MODULE_LICENSE("GPL");
-- 
2.35.1



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

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>
Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v3:
- use Co-Developed-by
---
 drivers/auxdisplay/Kconfig  |  10 +
 drivers/auxdisplay/Makefile |   1 +
 drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++++++++++++++++++++
 3 files changed, 387 insertions(+)
 create mode 100644 drivers/auxdisplay/tm1628.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 64012cda4..25ef2e452 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -203,6 +203,16 @@ 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..b802b5c30
--- /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
+ */
+
+#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
+
+/* 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[7];
+	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)
+{
+	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)
+{
+	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);
+	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 i, ret;
+
+	int msg_len = strlen(s->text);
+
+	mutex_lock(&s->disp_lock);
+
+	for (i = 0; i < s->grid_size; i++) {
+		int pos = s->grid[i] - 1;
+
+		if (i < msg_len) {
+			int char7_raw = map_to_seg7(&map_seg7, s->text[i]);
+			int j, char7;
+
+			for (j = 0, char7 = 0; j < 7; j++) {
+				if (char7_raw & BIT(j))
+					char7 |= BIT(s->segment_mapping[j] - 1);
+			}
+
+			s->data[pos] = cpu_to_le16(char7);
+		} else {
+			s->data[pos] = 0;
+		}
+	}
+
+	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 offset, ret;
+	__le16 bit;
+
+	offset = led->grid - 1;
+	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;
+	__le16 bit;
+	bool on;
+
+	offset = led->grid - 1;
+	bit = cpu_to_le16(BIT(led->seg - 1));
+
+	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;
+
+	if (count > s->grid_size + 1) /* consider trailing newline */
+		return -E2BIG;
+
+	for (i = 0; i < count && 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);
+
+	msleep(200); /* according to TM1628 datasheet */
+
+	/* clear screen */
+	ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
+	if (ret)
+		return ret;
+	/* Assume that subsequent SPI transfers will be ok if first was ok */
+
+	/* For now we support 6x12 mode only. This should be sufficient for most use cases */
+	tm1628_set_display_mode(spi, TM1628_DISPLAY_MODE_6_12);
+
+	tm1628_set_display_ctrl(spi, true);
+
+	if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
+		goto no_leds;
+
+	num_leds = 0;
+
+	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, "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, "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, "segment-mapping", s->segment_mapping, 7);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < 7; 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");
+MODULE_LICENSE("GPL");
-- 
2.35.1



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/5] arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display

[Heiner Kallweit]

This patch adds support for the 7 segment display of the device.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 .../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>;
+
+			segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+			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

[PATCH v3 5/5] arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display

[Heiner Kallweit]

This patch adds support for the 7 segment display of the device.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 .../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>;
+
+			segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+			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



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v3 5/5] arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display

[Heiner Kallweit]

This patch adds support for the 7 segment display of the device.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 .../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>;
+
+			segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
+			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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Miguel Ojeda]

On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>

If you (Heiner) are going to be the "From" author, then this line
should not be here.

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

Maybe a newline between paragraphs?

> + * Copyright (c) 2019 Andreas Färber

...here: should there be entries for you (Heiner) too? If not, should
Andreas be the "From" author?

This also applies to the `MODULE_AUTHOR`.

Also it may be a good idea to add the emails:

    MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
    MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");

(You may also want to consider adding an entry on `MAINTAINERS`).

> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;

Consider using `const` for some of the variables.

> +       for (i = 0; i < s->grid_size; i++) {
> +               int pos = s->grid[i] - 1;
> +
> +               if (i < msg_len) {

Consider inverting the condition, doing the set to `0` + `continue;`
to avoid the indentation.

> +       struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
> +       struct tm1628 *s = led->ctrl;
> +       int offset;
> +       __le16 bit;

Style: sometimes the variables are initialized right away using a
value from above, but other times they are done below.

> +       if (count > s->grid_size + 1) /* consider trailing newline */

Style: sometimes comments are trailing the line, others are above.
Also, sometimes they start with uppercase, but in other cases they do
not.

Also, about the `+ 1`: is it possible that sysfs gives us a buffer
full of `isprint()`? i.e. is it possible that `grid_size ==
MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?

> +       ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
> +       if (ret)
> +               return ret;
> +       /* Assume that subsequent SPI transfers will be ok if first was ok */

If not, is there a consequence? i.e. why wouldn't one check and fail
similarly in the `tm1628_set_*` calls below?

> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
> +               goto no_leds;

What about putting the code in the `if` body (negating the condition)?

> +       num_leds = 0;

This is reusing the variable for a different purpose, no? i.e. if we
did not get here, we would have no leds, yet we would report the
number above.

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

Is a failure expected? i.e. this `continue;`s, but should it fail or
is it OK to proceed?

> +       for (i = 0; i < 7; i++) {

Maybe a `#define` for several of the `7`s around?

> +static void tm1628_spi_remove(struct spi_device *spi)

Doesn't `.remove` return `int`?

Thanks!

Cheers,
Miguel

Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Miguel Ojeda]

On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>

If you (Heiner) are going to be the "From" author, then this line
should not be here.

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

Maybe a newline between paragraphs?

> + * Copyright (c) 2019 Andreas Färber

...here: should there be entries for you (Heiner) too? If not, should
Andreas be the "From" author?

This also applies to the `MODULE_AUTHOR`.

Also it may be a good idea to add the emails:

    MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
    MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");

(You may also want to consider adding an entry on `MAINTAINERS`).

> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;

Consider using `const` for some of the variables.

> +       for (i = 0; i < s->grid_size; i++) {
> +               int pos = s->grid[i] - 1;
> +
> +               if (i < msg_len) {

Consider inverting the condition, doing the set to `0` + `continue;`
to avoid the indentation.

> +       struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
> +       struct tm1628 *s = led->ctrl;
> +       int offset;
> +       __le16 bit;

Style: sometimes the variables are initialized right away using a
value from above, but other times they are done below.

> +       if (count > s->grid_size + 1) /* consider trailing newline */

Style: sometimes comments are trailing the line, others are above.
Also, sometimes they start with uppercase, but in other cases they do
not.

Also, about the `+ 1`: is it possible that sysfs gives us a buffer
full of `isprint()`? i.e. is it possible that `grid_size ==
MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?

> +       ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
> +       if (ret)
> +               return ret;
> +       /* Assume that subsequent SPI transfers will be ok if first was ok */

If not, is there a consequence? i.e. why wouldn't one check and fail
similarly in the `tm1628_set_*` calls below?

> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
> +               goto no_leds;

What about putting the code in the `if` body (negating the condition)?

> +       num_leds = 0;

This is reusing the variable for a different purpose, no? i.e. if we
did not get here, we would have no leds, yet we would report the
number above.

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

Is a failure expected? i.e. this `continue;`s, but should it fail or
is it OK to proceed?

> +       for (i = 0; i < 7; i++) {

Maybe a `#define` for several of the `7`s around?

> +static void tm1628_spi_remove(struct spi_device *spi)

Doesn't `.remove` return `int`?

Thanks!

Cheers,
Miguel

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Miguel Ojeda]

On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>

If you (Heiner) are going to be the "From" author, then this line
should not be here.

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

Maybe a newline between paragraphs?

> + * Copyright (c) 2019 Andreas Färber

...here: should there be entries for you (Heiner) too? If not, should
Andreas be the "From" author?

This also applies to the `MODULE_AUTHOR`.

Also it may be a good idea to add the emails:

    MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
    MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");

(You may also want to consider adding an entry on `MAINTAINERS`).

> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;

Consider using `const` for some of the variables.

> +       for (i = 0; i < s->grid_size; i++) {
> +               int pos = s->grid[i] - 1;
> +
> +               if (i < msg_len) {

Consider inverting the condition, doing the set to `0` + `continue;`
to avoid the indentation.

> +       struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
> +       struct tm1628 *s = led->ctrl;
> +       int offset;
> +       __le16 bit;

Style: sometimes the variables are initialized right away using a
value from above, but other times they are done below.

> +       if (count > s->grid_size + 1) /* consider trailing newline */

Style: sometimes comments are trailing the line, others are above.
Also, sometimes they start with uppercase, but in other cases they do
not.

Also, about the `+ 1`: is it possible that sysfs gives us a buffer
full of `isprint()`? i.e. is it possible that `grid_size ==
MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?

> +       ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
> +       if (ret)
> +               return ret;
> +       /* Assume that subsequent SPI transfers will be ok if first was ok */

If not, is there a consequence? i.e. why wouldn't one check and fail
similarly in the `tm1628_set_*` calls below?

> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
> +               goto no_leds;

What about putting the code in the `if` body (negating the condition)?

> +       num_leds = 0;

This is reusing the variable for a different purpose, no? i.e. if we
did not get here, we would have no leds, yet we would report the
number above.

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

Is a failure expected? i.e. this `continue;`s, but should it fail or
is it OK to proceed?

> +       for (i = 0; i < 7; i++) {

Maybe a `#define` for several of the `7`s around?

> +static void tm1628_spi_remove(struct spi_device *spi)

Doesn't `.remove` return `int`?

Thanks!

Cheers,
Miguel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

On 23.02.2022 21:28, Miguel Ojeda wrote:
> On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> If you (Heiner) are going to be the "From" author, then this line
> should not be here.
> 
>> +         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.
> 
> Maybe a newline between paragraphs?
> 
OK

>> + * Copyright (c) 2019 Andreas Färber
> 
> ...here: should there be entries for you (Heiner) too? If not, should
> Andreas be the "From" author?
> 
> This also applies to the `MODULE_AUTHOR`.
> 
> Also it may be a good idea to add the emails:
> 
>     MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
>     MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");
> 
> (You may also want to consider adding an entry on `MAINTAINERS`).
> 

Maybe we should have both names in all places you mentioned.
I'd be fine with being listed as maintainer.

@Andreas: After you wrote the initial code and I rewrote bigger
parts of it: Do you want to be listed as maintainer too?
And any remark on the questions raised by Miguel?

>> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;
> 
> Consider using `const` for some of the variables.
> 
OK

>> +       for (i = 0; i < s->grid_size; i++) {
>> +               int pos = s->grid[i] - 1;
>> +
>> +               if (i < msg_len) {
> 
> Consider inverting the condition, doing the set to `0` + `continue;`
> to avoid the indentation.
> 
OK

>> +       struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
>> +       struct tm1628 *s = led->ctrl;
>> +       int offset;
>> +       __le16 bit;
> 
> Style: sometimes the variables are initialized right away using a
> value from above, but other times they are done below.
> 
OK

>> +       if (count > s->grid_size + 1) /* consider trailing newline */
> 
> Style: sometimes comments are trailing the line, others are above.
> Also, sometimes they start with uppercase, but in other cases they do
> not.
> 
OK, will beautify this.

> Also, about the `+ 1`: is it possible that sysfs gives us a buffer
> full of `isprint()`? i.e. is it possible that `grid_size ==
> MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
> out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?
> 
Typically there's no issue because command line input always ends
with the (non-printable) newline. But I checked and yes, it's possible
to pipe data w/o trailing newline to the attribute.
Hence I have to fix the code.

>> +       ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
>> +       if (ret)
>> +               return ret;
>> +       /* Assume that subsequent SPI transfers will be ok if first was ok */
> 
> If not, is there a consequence? i.e. why wouldn't one check and fail
> similarly in the `tm1628_set_*` calls below?
> 
I'll change this check the return code also for the next calls.

>> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
>> +               goto no_leds;
> 
> What about putting the code in the `if` body (negating the condition)?
> 
I did it this way to avoid having to indent the following bigger block.

>> +       num_leds = 0;
> 
> This is reusing the variable for a different purpose, no? i.e. if we
> did not get here, we would have no leds, yet we would report the
> number above.
> 
The variable is re-used for a slightly different purpose. At first it's
used for the number of LED child nodes (what may include invalid configs),
then it's used for the number of the actually configured LED's.
And right, I have to move the assignment before the check.

>> +       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);
> 
> Is a failure expected? i.e. this `continue;`s, but should it fail or
> is it OK to proceed?
> 

Symbol LED's and display digits are independent. Therefore, if there's a problem
with a DT LED definition, I chose to just skip it instead if bailing out completely.

>> +       for (i = 0; i < 7; i++) {
> 
> Maybe a `#define` for several of the `7`s around?
> 
OK

>> +static void tm1628_spi_remove(struct spi_device *spi)
> 
> Doesn't `.remove` return `int`?
> 
This has changed recently with a0386bba7093 ("spi: make remove callback a void function").

> Thanks!
> 
> Cheers,
> Miguel

Thanks for the comprehensive feedback!

Heiner

Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

On 23.02.2022 21:28, Miguel Ojeda wrote:
> On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> If you (Heiner) are going to be the "From" author, then this line
> should not be here.
> 
>> +         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.
> 
> Maybe a newline between paragraphs?
> 
OK

>> + * Copyright (c) 2019 Andreas Färber
> 
> ...here: should there be entries for you (Heiner) too? If not, should
> Andreas be the "From" author?
> 
> This also applies to the `MODULE_AUTHOR`.
> 
> Also it may be a good idea to add the emails:
> 
>     MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
>     MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");
> 
> (You may also want to consider adding an entry on `MAINTAINERS`).
> 

Maybe we should have both names in all places you mentioned.
I'd be fine with being listed as maintainer.

@Andreas: After you wrote the initial code and I rewrote bigger
parts of it: Do you want to be listed as maintainer too?
And any remark on the questions raised by Miguel?

>> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;
> 
> Consider using `const` for some of the variables.
> 
OK

>> +       for (i = 0; i < s->grid_size; i++) {
>> +               int pos = s->grid[i] - 1;
>> +
>> +               if (i < msg_len) {
> 
> Consider inverting the condition, doing the set to `0` + `continue;`
> to avoid the indentation.
> 
OK

>> +       struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
>> +       struct tm1628 *s = led->ctrl;
>> +       int offset;
>> +       __le16 bit;
> 
> Style: sometimes the variables are initialized right away using a
> value from above, but other times they are done below.
> 
OK

>> +       if (count > s->grid_size + 1) /* consider trailing newline */
> 
> Style: sometimes comments are trailing the line, others are above.
> Also, sometimes they start with uppercase, but in other cases they do
> not.
> 
OK, will beautify this.

> Also, about the `+ 1`: is it possible that sysfs gives us a buffer
> full of `isprint()`? i.e. is it possible that `grid_size ==
> MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
> out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?
> 
Typically there's no issue because command line input always ends
with the (non-printable) newline. But I checked and yes, it's possible
to pipe data w/o trailing newline to the attribute.
Hence I have to fix the code.

>> +       ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
>> +       if (ret)
>> +               return ret;
>> +       /* Assume that subsequent SPI transfers will be ok if first was ok */
> 
> If not, is there a consequence? i.e. why wouldn't one check and fail
> similarly in the `tm1628_set_*` calls below?
> 
I'll change this check the return code also for the next calls.

>> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
>> +               goto no_leds;
> 
> What about putting the code in the `if` body (negating the condition)?
> 
I did it this way to avoid having to indent the following bigger block.

>> +       num_leds = 0;
> 
> This is reusing the variable for a different purpose, no? i.e. if we
> did not get here, we would have no leds, yet we would report the
> number above.
> 
The variable is re-used for a slightly different purpose. At first it's
used for the number of LED child nodes (what may include invalid configs),
then it's used for the number of the actually configured LED's.
And right, I have to move the assignment before the check.

>> +       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);
> 
> Is a failure expected? i.e. this `continue;`s, but should it fail or
> is it OK to proceed?
> 

Symbol LED's and display digits are independent. Therefore, if there's a problem
with a DT LED definition, I chose to just skip it instead if bailing out completely.

>> +       for (i = 0; i < 7; i++) {
> 
> Maybe a `#define` for several of the `7`s around?
> 
OK

>> +static void tm1628_spi_remove(struct spi_device *spi)
> 
> Doesn't `.remove` return `int`?
> 
This has changed recently with a0386bba7093 ("spi: make remove callback a void function").

> Thanks!
> 
> Cheers,
> Miguel

Thanks for the comprehensive feedback!

Heiner

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v3 4/5] auxdisplay: add support for Titanmec TM1628 7 segment display controller

[Heiner Kallweit]

On 23.02.2022 21:28, Miguel Ojeda wrote:
> On Wed, Feb 23, 2022 at 7:02 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> 
> If you (Heiner) are going to be the "From" author, then this line
> should not be here.
> 
>> +         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.
> 
> Maybe a newline between paragraphs?
> 
OK

>> + * Copyright (c) 2019 Andreas Färber
> 
> ...here: should there be entries for you (Heiner) too? If not, should
> Andreas be the "From" author?
> 
> This also applies to the `MODULE_AUTHOR`.
> 
> Also it may be a good idea to add the emails:
> 
>     MODULE_AUTHOR("Andreas Färber <afaerber@suse.de>");
>     MODULE_AUTHOR("Heiner Kallweit <hkallweit1@gmail.com>");
> 
> (You may also want to consider adding an entry on `MAINTAINERS`).
> 

Maybe we should have both names in all places you mentioned.
I'd be fine with being listed as maintainer.

@Andreas: After you wrote the initial code and I rewrote bigger
parts of it: Do you want to be listed as maintainer too?
And any remark on the questions raised by Miguel?

>> +       u8 cmd = TM1628_CMD_DISPLAY_MODE | grid_mode;
> 
> Consider using `const` for some of the variables.
> 
OK

>> +       for (i = 0; i < s->grid_size; i++) {
>> +               int pos = s->grid[i] - 1;
>> +
>> +               if (i < msg_len) {
> 
> Consider inverting the condition, doing the set to `0` + `continue;`
> to avoid the indentation.
> 
OK

>> +       struct tm1628_led *led = container_of(led_cdev, struct tm1628_led, leddev);
>> +       struct tm1628 *s = led->ctrl;
>> +       int offset;
>> +       __le16 bit;
> 
> Style: sometimes the variables are initialized right away using a
> value from above, but other times they are done below.
> 
OK

>> +       if (count > s->grid_size + 1) /* consider trailing newline */
> 
> Style: sometimes comments are trailing the line, others are above.
> Also, sometimes they start with uppercase, but in other cases they do
> not.
> 
OK, will beautify this.

> Also, about the `+ 1`: is it possible that sysfs gives us a buffer
> full of `isprint()`? i.e. is it possible that `grid_size ==
> MAX_GRID_SIZE` and `count == MAX_GRID_SIZE + 1` and then we perform an
> out-of-bounds store to `MAX_GRID_SIZE + 2` in `text`?
> 
Typically there's no issue because command line input always ends
with the (non-printable) newline. But I checked and yes, it's possible
to pipe data w/o trailing newline to the attribute.
Hence I have to fix the code.

>> +       ret = tm1628_write_data(spi, 0, MAX_GRID_SIZE);
>> +       if (ret)
>> +               return ret;
>> +       /* Assume that subsequent SPI transfers will be ok if first was ok */
> 
> If not, is there a consequence? i.e. why wouldn't one check and fail
> similarly in the `tm1628_set_*` calls below?
> 
I'll change this check the return code also for the next calls.

>> +       if (!IS_REACHABLE(CONFIG_LEDS_CLASS))
>> +               goto no_leds;
> 
> What about putting the code in the `if` body (negating the condition)?
> 
I did it this way to avoid having to indent the following bigger block.

>> +       num_leds = 0;
> 
> This is reusing the variable for a different purpose, no? i.e. if we
> did not get here, we would have no leds, yet we would report the
> number above.
> 
The variable is re-used for a slightly different purpose. At first it's
used for the number of LED child nodes (what may include invalid configs),
then it's used for the number of the actually configured LED's.
And right, I have to move the assignment before the check.

>> +       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);
> 
> Is a failure expected? i.e. this `continue;`s, but should it fail or
> is it OK to proceed?
> 

Symbol LED's and display digits are independent. Therefore, if there's a problem
with a DT LED definition, I chose to just skip it instead if bailing out completely.

>> +       for (i = 0; i < 7; i++) {
> 
> Maybe a `#define` for several of the `7`s around?
> 
OK

>> +static void tm1628_spi_remove(struct spi_device *spi)
> 
> Doesn't `.remove` return `int`?
> 
This has changed recently with a0386bba7093 ("spi: make remove callback a void function").

> Thanks!
> 
> Cheers,
> Miguel

Thanks for the comprehensive feedback!

Heiner

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Wed, Feb 23, 2022 at 06:59:31PM +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>
> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v3:
> - fix remaining YAML issues
> - use Co-Developed-by
> ---
>  .../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
> +

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

Are these properties useful for any 7 segment display or specific to 
this controller?

The commit msg mentions 11 segment display. Does this need to be?:

oneOf:
  - minItems: 7
    maxItems: 7
  - minItems: 11
    maxItems: 11


> +
> +  "#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>;
> +            grid = /bits/ 8 <4 3 2 1>;
> +            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> +            #address-cells = <2>;
> +            #size-cells = <0>;
> +
> +            alarmn@5,4 {
> +                reg = <5 4>;
> +                function = LED_FUNCTION_ALARM;
> +            };
> +        };
> +    };
> +...
> -- 
> 2.35.1
> 
> 
> 

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

[Rob Herring]

On Wed, Feb 23, 2022 at 06:59:31PM +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>
> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v3:
> - fix remaining YAML issues
> - use Co-Developed-by
> ---
>  .../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
> +

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

Are these properties useful for any 7 segment display or specific to 
this controller?

The commit msg mentions 11 segment display. Does this need to be?:

oneOf:
  - minItems: 7
    maxItems: 7
  - minItems: 11
    maxItems: 11


> +
> +  "#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>;
> +            grid = /bits/ 8 <4 3 2 1>;
> +            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> +            #address-cells = <2>;
> +            #size-cells = <0>;
> +
> +            alarmn@5,4 {
> +                reg = <5 4>;
> +                function = LED_FUNCTION_ALARM;
> +            };
> +        };
> +    };
> +...
> -- 
> 2.35.1
> 
> 
> 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Rob Herring]

On Wed, Feb 23, 2022 at 06:59:31PM +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>
> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v3:
> - fix remaining YAML issues
> - use Co-Developed-by
> ---
>  .../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
> +

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

Are these properties useful for any 7 segment display or specific to 
this controller?

The commit msg mentions 11 segment display. Does this need to be?:

oneOf:
  - minItems: 7
    maxItems: 7
  - minItems: 11
    maxItems: 11


> +
> +  "#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>;
> +            grid = /bits/ 8 <4 3 2 1>;
> +            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
> +            #address-cells = <2>;
> +            #size-cells = <0>;
> +
> +            alarmn@5,4 {
> +                reg = <5 4>;
> +                function = LED_FUNCTION_ALARM;
> +            };
> +        };
> +    };
> +...
> -- 
> 2.35.1
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Heiner Kallweit]

On 24.02.2022 21:22, Rob Herring wrote:
> On Wed, Feb 23, 2022 at 06:59:31PM +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>
>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v3:
>> - fix remaining YAML issues
>> - use Co-Developed-by
>> ---
>>  .../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
>> +
> 
>> +  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
>> +
>> +  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
> 
> Are these properties useful for any 7 segment display or specific to 
> this controller?
> 
Both are controller-specific. E.g. the functionally similar driver
ht16k33 uses different properties.

> The commit msg mentions 11 segment display. Does this need to be?:
> 
> oneOf:
>   - minItems: 7
>     maxItems: 7
>   - minItems: 11
>     maxItems: 11
> 
The controller would be able to drive 11 segments, but the driver
supports 7 segments only (at least for now). Therefore a 11 segment
display can be used, but only the 7 segment part will be active.
All devices with this controller I've seen and heard of have
7 segment displays.

> 
>> +
>> +  "#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>;
>> +            grid = /bits/ 8 <4 3 2 1>;
>> +            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
>> +            #address-cells = <2>;
>> +            #size-cells = <0>;
>> +
>> +            alarmn@5,4 {
>> +                reg = <5 4>;
>> +                function = LED_FUNCTION_ALARM;
>> +            };
>> +        };
>> +    };
>> +...
>> -- 
>> 2.35.1
>>
>>
>>

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

[Heiner Kallweit]

On 24.02.2022 21:22, Rob Herring wrote:
> On Wed, Feb 23, 2022 at 06:59:31PM +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>
>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v3:
>> - fix remaining YAML issues
>> - use Co-Developed-by
>> ---
>>  .../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
>> +
> 
>> +  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
>> +
>> +  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
> 
> Are these properties useful for any 7 segment display or specific to 
> this controller?
> 
Both are controller-specific. E.g. the functionally similar driver
ht16k33 uses different properties.

> The commit msg mentions 11 segment display. Does this need to be?:
> 
> oneOf:
>   - minItems: 7
>     maxItems: 7
>   - minItems: 11
>     maxItems: 11
> 
The controller would be able to drive 11 segments, but the driver
supports 7 segments only (at least for now). Therefore a 11 segment
display can be used, but only the 7 segment part will be active.
All devices with this controller I've seen and heard of have
7 segment displays.

> 
>> +
>> +  "#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>;
>> +            grid = /bits/ 8 <4 3 2 1>;
>> +            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
>> +            #address-cells = <2>;
>> +            #size-cells = <0>;
>> +
>> +            alarmn@5,4 {
>> +                reg = <5 4>;
>> +                function = LED_FUNCTION_ALARM;
>> +            };
>> +        };
>> +    };
>> +...
>> -- 
>> 2.35.1
>>
>>
>>


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Heiner Kallweit]

On 24.02.2022 21:22, Rob Herring wrote:
> On Wed, Feb 23, 2022 at 06:59:31PM +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>
>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v3:
>> - fix remaining YAML issues
>> - use Co-Developed-by
>> ---
>>  .../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
>> +
> 
>> +  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
>> +
>> +  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
> 
> Are these properties useful for any 7 segment display or specific to 
> this controller?
> 
Both are controller-specific. E.g. the functionally similar driver
ht16k33 uses different properties.

> The commit msg mentions 11 segment display. Does this need to be?:
> 
> oneOf:
>   - minItems: 7
>     maxItems: 7
>   - minItems: 11
>     maxItems: 11
> 
The controller would be able to drive 11 segments, but the driver
supports 7 segments only (at least for now). Therefore a 11 segment
display can be used, but only the 7 segment part will be active.
All devices with this controller I've seen and heard of have
7 segment displays.

> 
>> +
>> +  "#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>;
>> +            grid = /bits/ 8 <4 3 2 1>;
>> +            segment-mapping = /bits/ 8 <4 5 6 1 2 3 7>;
>> +            #address-cells = <2>;
>> +            #size-cells = <0>;
>> +
>> +            alarmn@5,4 {
>> +                reg = <5 4>;
>> +                function = LED_FUNCTION_ALARM;
>> +            };
>> +        };
>> +    };
>> +...
>> -- 
>> 2.35.1
>>
>>
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Rob Herring]

On Thu, Feb 24, 2022 at 2:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 24.02.2022 21:22, Rob Herring wrote:
> > On Wed, Feb 23, 2022 at 06:59:31PM +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>
> >> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >> v3:
> >> - fix remaining YAML issues
> >> - use Co-Developed-by
> >> ---
> >>  .../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
> >> +
> >
> >> +  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
> >> +
> >> +  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
> >
> > Are these properties useful for any 7 segment display or specific to
> > this controller?
> >
> Both are controller-specific. E.g. the functionally similar driver
> ht16k33 uses different properties.

Then they both need a vendor prefix.

> > The commit msg mentions 11 segment display. Does this need to be?:
> >
> > oneOf:
> >   - minItems: 7
> >     maxItems: 7
> >   - minItems: 11
> >     maxItems: 11
> >
> The controller would be able to drive 11 segments, but the driver
> supports 7 segments only (at least for now). Therefore a 11 segment
> display can be used, but only the 7 segment part will be active.
> All devices with this controller I've seen and heard of have
> 7 segment displays.

Bindings should be complete, not just what a particular version of a
driver supports. However, if all known h/w is just 7 seg, then it is
fine.

Rob

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

[Rob Herring]

On Thu, Feb 24, 2022 at 2:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 24.02.2022 21:22, Rob Herring wrote:
> > On Wed, Feb 23, 2022 at 06:59:31PM +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>
> >> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >> v3:
> >> - fix remaining YAML issues
> >> - use Co-Developed-by
> >> ---
> >>  .../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
> >> +
> >
> >> +  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
> >> +
> >> +  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
> >
> > Are these properties useful for any 7 segment display or specific to
> > this controller?
> >
> Both are controller-specific. E.g. the functionally similar driver
> ht16k33 uses different properties.

Then they both need a vendor prefix.

> > The commit msg mentions 11 segment display. Does this need to be?:
> >
> > oneOf:
> >   - minItems: 7
> >     maxItems: 7
> >   - minItems: 11
> >     maxItems: 11
> >
> The controller would be able to drive 11 segments, but the driver
> supports 7 segments only (at least for now). Therefore a 11 segment
> display can be used, but only the 7 segment part will be active.
> All devices with this controller I've seen and heard of have
> 7 segment displays.

Bindings should be complete, not just what a particular version of a
driver supports. However, if all known h/w is just 7 seg, then it is
fine.

Rob

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Rob Herring]

On Thu, Feb 24, 2022 at 2:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 24.02.2022 21:22, Rob Herring wrote:
> > On Wed, Feb 23, 2022 at 06:59:31PM +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>
> >> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >> v3:
> >> - fix remaining YAML issues
> >> - use Co-Developed-by
> >> ---
> >>  .../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
> >> +
> >
> >> +  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
> >> +
> >> +  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
> >
> > Are these properties useful for any 7 segment display or specific to
> > this controller?
> >
> Both are controller-specific. E.g. the functionally similar driver
> ht16k33 uses different properties.

Then they both need a vendor prefix.

> > The commit msg mentions 11 segment display. Does this need to be?:
> >
> > oneOf:
> >   - minItems: 7
> >     maxItems: 7
> >   - minItems: 11
> >     maxItems: 11
> >
> The controller would be able to drive 11 segments, but the driver
> supports 7 segments only (at least for now). Therefore a 11 segment
> display can be used, but only the 7 segment part will be active.
> All devices with this controller I've seen and heard of have
> 7 segment displays.

Bindings should be complete, not just what a particular version of a
driver supports. However, if all known h/w is just 7 seg, then it is
fine.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Heiner Kallweit]

On 24.02.2022 22:30, Rob Herring wrote:
> On Thu, Feb 24, 2022 at 2:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 24.02.2022 21:22, Rob Herring wrote:
>>> On Wed, Feb 23, 2022 at 06:59:31PM +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>
>>>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> v3:
>>>> - fix remaining YAML issues
>>>> - use Co-Developed-by
>>>> ---
>>>>  .../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
>>>> +
>>>
>>>> +  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
>>>> +
>>>> +  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
>>>
>>> Are these properties useful for any 7 segment display or specific to
>>> this controller?
>>>
>> Both are controller-specific. E.g. the functionally similar driver
>> ht16k33 uses different properties.
> 
> Then they both need a vendor prefix.
> 
OK, will add the prefix. Thanks for the hint.

>>> The commit msg mentions 11 segment display. Does this need to be?:
>>>
>>> oneOf:
>>>   - minItems: 7
>>>     maxItems: 7
>>>   - minItems: 11
>>>     maxItems: 11
>>>
>> The controller would be able to drive 11 segments, but the driver
>> supports 7 segments only (at least for now). Therefore a 11 segment
>> display can be used, but only the 7 segment part will be active.
>> All devices with this controller I've seen and heard of have
>> 7 segment displays.
> 
> Bindings should be complete, not just what a particular version of a
> driver supports. However, if all known h/w is just 7 seg, then it is
> fine.
> 
> Rob

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

[Heiner Kallweit]

On 24.02.2022 22:30, Rob Herring wrote:
> On Thu, Feb 24, 2022 at 2:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 24.02.2022 21:22, Rob Herring wrote:
>>> On Wed, Feb 23, 2022 at 06:59:31PM +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>
>>>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> v3:
>>>> - fix remaining YAML issues
>>>> - use Co-Developed-by
>>>> ---
>>>>  .../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
>>>> +
>>>
>>>> +  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
>>>> +
>>>> +  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
>>>
>>> Are these properties useful for any 7 segment display or specific to
>>> this controller?
>>>
>> Both are controller-specific. E.g. the functionally similar driver
>> ht16k33 uses different properties.
> 
> Then they both need a vendor prefix.
> 
OK, will add the prefix. Thanks for the hint.

>>> The commit msg mentions 11 segment display. Does this need to be?:
>>>
>>> oneOf:
>>>   - minItems: 7
>>>     maxItems: 7
>>>   - minItems: 11
>>>     maxItems: 11
>>>
>> The controller would be able to drive 11 segments, but the driver
>> supports 7 segments only (at least for now). Therefore a 11 segment
>> display can be used, but only the 7 segment part will be active.
>> All devices with this controller I've seen and heard of have
>> 7 segment displays.
> 
> Bindings should be complete, not just what a particular version of a
> driver supports. However, if all known h/w is just 7 seg, then it is
> fine.
> 
> Rob


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Heiner Kallweit]

On 24.02.2022 22:30, Rob Herring wrote:
> On Thu, Feb 24, 2022 at 2:56 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>> On 24.02.2022 21:22, Rob Herring wrote:
>>> On Wed, Feb 23, 2022 at 06:59:31PM +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>
>>>> Co-Developed-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> v3:
>>>> - fix remaining YAML issues
>>>> - use Co-Developed-by
>>>> ---
>>>>  .../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
>>>> +
>>>
>>>> +  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
>>>> +
>>>> +  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
>>>
>>> Are these properties useful for any 7 segment display or specific to
>>> this controller?
>>>
>> Both are controller-specific. E.g. the functionally similar driver
>> ht16k33 uses different properties.
> 
> Then they both need a vendor prefix.
> 
OK, will add the prefix. Thanks for the hint.

>>> The commit msg mentions 11 segment display. Does this need to be?:
>>>
>>> oneOf:
>>>   - minItems: 7
>>>     maxItems: 7
>>>   - minItems: 11
>>>     maxItems: 11
>>>
>> The controller would be able to drive 11 segments, but the driver
>> supports 7 segments only (at least for now). Therefore a 11 segment
>> display can be used, but only the 7 segment part will be active.
>> All devices with this controller I've seen and heard of have
>> 7 segment displays.
> 
> Bindings should be complete, not just what a particular version of a
> driver supports. However, if all known h/w is just 7 seg, then it is
> fine.
> 
> Rob


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel