| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d3d8f72a-e4fe-4f85-8ead-6c104aa32893@kernel.org>
Date: Mon, 30 Jun 2025 08:19:11 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jean-François Lessard <jefflessard3@...il.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>, devicetree@...r.kernel.org,
linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
Andreas Färber <afaerber@...e.de>,
Boris Gjenero <boris.gjenero@...il.com>,
Christian Hewitt <christianshewitt@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Paolo Sabatino <paolo.sabatino@...il.com>
Subject: Re: [PATCH v2 6/8] dt-bindings: auxdisplay: add Titan Micro
Electronics TM16XX
On 29/06/2025 14:59, Jean-François Lessard wrote:
> Add documentation for Titanmec TM16XX and compatible LED display controllers.
>
> This patch is inspired by previous work from Andreas Färber and Heiner Kallweit.
Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> Co-developed-by: Andreas Färber <afaerber@...e.de>
> Co-developed-by: Heiner Kallweit <hkallweit1@...il.com>
> Signed-off-by: Jean-François Lessard <jefflessard3@...il.com>
> ---
> .../bindings/auxdisplay/titanmec,tm16xx.yaml | 210 ++++++++++++++++++
> 1 file changed, 210 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
>
> diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> new file mode 100644
> index 0000000000..65c43e3ba4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm16xx.yaml
> @@ -0,0 +1,210 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm16xx.yaml#
Why isn't this in leds directory? Everything below describes it as LED
controller.
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Auxiliary displays based on TM16xx and compatible LED controllers
> +
> +maintainers:
> + - Jean-François Lessard <jefflessard3@...il.com>
> +
> +description: |
> + TM16xx controllers manage a matrix of LEDs organized in grids (rows) and segments (columns).
> + Each grid or segment can be wired to drive either a digit or individual icons, depending on the
Wrap according to Linux coding style, so at 80.
> + board design.
> +
> + Typical display example:
> +
> + --- --- --- ---
> + WIFI | | | | - | | | | USB PLAY
> + --- --- --- ---
> + LAN | | | | - | | | | BT PAUSE
> + --- --- --- ---
> +
> + The controller itself is agnostic of the display layout. The specific arrangement
> + (which grids and segments drive which digits or icons) is determined by the board-level
> + wiring. Therefore, these bindings describe hardware configuration at the PCB level
> + to enable support of multiple display implementations using these LED controllers.
> +
> +properties:
> + compatible:
> + enum:
> + - titanmec,tm1618
> + - titanmec,tm1620
> + - titanmec,tm1628
> + - titanmec,tm1650
> + - fdhisi,fd620
> + - fdhisi,fd628
> + - fdhisi,fd650
> + - fdhisi,fd6551
> + - fdhisi,fd655
> + - icore,aip650
> + - icore,aip1618
> + - icore,aip1628
> + - princeton,pt6964
> + - winrise,hbs658
Several devices are compatible, so express it here and drop redundant
entries in the driver.
> +
> + reg:
> + maxItems: 1
> +
> + titanmec,digits:
> + description: |
> + Array of grid (row) indexes corresponding to specific wiring of digits in the display matrix.
What is wiring of digits? This and other descriptions don't tell me much.
Wrap according to Linux coding style, so at 80.
> + Defines which grid lines are connected to digit elements.
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> + items:
> + minimum: 0
> + maximum: 7
> + minItems: 1
> + maxItems: 8
> +
> + titanmec,segment-mapping:
> + description: |
Do not need '|' unless you need to preserve formatting.
> + Array of segment (column) indexes specifying the hardware layout mapping used for digit display.
> + Each entry gives the segment index corresponding to a standard 7-segment element (a-g).
Wrap according to Linux coding style, so at 80.
This looks like duplicating the reg property.
> + $ref: /schemas/types.yaml#/definitions/uint8-array
> + items:
> + minimum: 0
> + maximum: 7
> + minItems: 7
> + maxItems: 7
> +
> + titanmec,transposed:
> + description: |
> + Optional flag indicating if grids and segments are swapped compared to standard matrix orientation.
> + This accommodates devices where segments are wired to rows and grids to columns.
> + $ref: /schemas/types.yaml#/definitions/flag
> +
> + "#address-cells":
> + const: 2
> +
> + "#size-cells":
> + const: 0
> +
> +patternProperties:
> + "^led@[0-7],[0-7]$":
Why do you have two addresses? It's not used in your example.
> + $ref: /schemas/leds/common.yaml#
> + properties:
> + reg:
> + description: Grid (row) and segment (column) index in the matrix of this individual LED icon
Missing constraints.
> + required:
> + - reg
> +
> +required:
> + - compatible
> + - reg
> + - titanmec,digits
> + - titanmec,segment-mapping
> +
> +additionalProperties: true
No, this cannot be true. Look at any other binding, look at example-schema.
> +
> +examples:
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + display-controller@24 {
> + reg = <0x24>;
> + compatible = "fdhisi,fd655";
> + titanmec,digits = [01 02 03 04];
> + titanmec,segment-mapping = [03 04 05 00 01 02 06];
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + led@0,0 {
> + reg = <0 0>;
> + function = LED_FUNCTION_ALARM;
> + };
> +
> + led@0,1 {
> + reg = <0 1>;
> + function = LED_FUNCTION_USB;
> + };
> +
> + led@0,2 {
> + reg = <0 2>;
> + function = "play";
> + };
> +
> + led@0,3 {
> + reg = <0 3>;
> + function = "pause";
> + };
> +
> + led@0,4 {
> + reg = <0 4>;
> + function = "colon";
> + };
> +
> + led@0,5 {
> + reg = <0 5>;
> + function = LED_FUNCTION_LAN;
> + };
> +
> + led@0,6 {
> + reg = <0 6>;
> + function = LED_FUNCTION_WLAN;
> + };
> + };
> + };
> +
> + - |
> + #include <dt-bindings/leds/common.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + display-controller@0 {
> + reg = <0x0>;
> + compatible = "fdhisi,fd628";
> + titanmec,transposed;
> + titanmec,digits = [00 01 02 03];
> + titanmec,segment-mapping = [00 01 02 03 04 05 06];
> + spi-3wire;
> + spi-lsb-first;
> + spi-rx-delay-us = <1>;
> + spi-max-frequency = <500000>;
> + #address-cells = <2>;
> + #size-cells = <0>;
> +
> + led@4,0 {
> + reg = <4 0>;
> + function = "apps";
> + };
> +
> + led@4,1 {
> + reg = <4 1>;
> + function = "setup";
> + };
> +
> + led@4,2 {
> + reg = <4 2>;
> + function = LED_FUNCTION_USB;
> + };
> +
> + led@4,3 {
> + reg = <4 3>;
> + function = LED_FUNCTION_SD;
> + };
> +
> + led@4,4 {
> + reg = <4 4>;
> + function = "colon";
> + };
> +
> + led@4,5 {
> + reg = <4 5>;
> + function = "hdmi";
> + };
> +
> + led@4,6 {
> + reg = <4 6>;
> + function = "video";
> + };
> + };
> + };
Best regards,
Krzysztof
Powered by blists - more mailing lists