| Message ID | 90668779-b53d-b3e7-5327-af11ff4a1d18@gmail.com |
|---|---|
| Headers | show |
| Series | auxdisplay: Add support for the Titanmec TM1628 7 segment display controller | expand |
On 25.02.2022 22:09, Heiner Kallweit wrote: > This series adds support for the Titanmec TM1628 7 segment display > controller. It's based on previous RFC work from Andreas Färber. > The RFC version placed the driver in the LED subsystem, but this was > NAK'ed by the LED maintainer. Therefore I moved the driver to > /drivers/auxdisplay what seems most reasonable to me. > > Further changes to the RFC version: > - Driver can be built also w/o LED class support, for displays that > don't have any symbols to be exposed as LED's. > - Simplified the code and rewrote a lot of it. > - Driver is now kind of a MVP, but functionality should be sufficient > for most use cases. > - Use the existing 7 segment support in uapi/linux/map_to_7segment.h > as suggested by Geert Uytterhoeven. > > Note: There's a number of chips from other manufacturers that are > almost identical, e.g. FD628, SM1628. Only difference I saw so > far is that they partially support other display modes. > TM1628: 6x12, 7x11 > SM1628C: 4x13, 5x12, 6x11, 7x10 > For typical displays on devices using these chips this > difference shouldn't matter. > > Successfully tested on a TX3 Mini TV box that has an SM1628C and a > display with 4 digits and 7 symbols. > > v2: > - (re-)add Andreas' SoB to two patches > - fix YAML issues > - include ctype.h explicitly > - add info message in probe() > > v3: > - remove patch 1 because it has been applied via the SPI tree already > - fix remaining YAML issues in patch 2 > - follow Miguel's suggestion on usage of Co-Developed-by > > v4: > - add patch for MAINTAINERS entry > - incorporate Miguel's review comments > - Replace Co-Developed-by with Co-developed-by (checkpatch) > v5: > - add vendor prefix to driver-specific dt properties > > Andreas Färber (1): > dt-bindings: vendor-prefixes: Add Titan Micro Electronics > > Heiner Kallweit (5): > dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 > docs: ABI: document tm1628 attribute display-text > auxdisplay: add support for Titanmec TM1628 7 segment display > controller > arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment > display > MAINTAINERS: Add entry for tm1628 auxdisplay driver > > .../testing/sysfs-devices-auxdisplay-tm1628 | 7 + > .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > MAINTAINERS | 7 + > .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++ > drivers/auxdisplay/Kconfig | 11 + > drivers/auxdisplay/Makefile | 1 + > drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++ > 8 files changed, 555 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628 > create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml > create mode 100644 drivers/auxdisplay/tm1628.c > Hi Miguel, the DT extensions have been acked/reviewed. Any other feedback from your side? Or can it be applied as-is? Heiner
On 2022-02-25 21:09, Heiner Kallweit wrote: > This series adds support for the Titanmec TM1628 7 segment display > controller. It's based on previous RFC work from Andreas Färber. > The RFC version placed the driver in the LED subsystem, but this was > NAK'ed by the LED maintainer. Therefore I moved the driver to > /drivers/auxdisplay what seems most reasonable to me. > > Further changes to the RFC version: > - Driver can be built also w/o LED class support, for displays that > don't have any symbols to be exposed as LED's. > - Simplified the code and rewrote a lot of it. > - Driver is now kind of a MVP, but functionality should be sufficient > for most use cases. > - Use the existing 7 segment support in uapi/linux/map_to_7segment.h > as suggested by Geert Uytterhoeven. > > Note: There's a number of chips from other manufacturers that are > almost identical, e.g. FD628, SM1628. Only difference I saw so > far is that they partially support other display modes. > TM1628: 6x12, 7x11 > SM1628C: 4x13, 5x12, 6x11, 7x10 > For typical displays on devices using these chips this > difference shouldn't matter. > > Successfully tested on a TX3 Mini TV box that has an SM1628C and a > display with 4 digits and 7 symbols. FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so: titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>; titanmec,grid = /bits/ 8 <5 4 2 1>; (grid 3 segment 2 is used for a colon in the middle) If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used? I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...) Cheers, Robin. > v2: > - (re-)add Andreas' SoB to two patches > - fix YAML issues > - include ctype.h explicitly > - add info message in probe() > > v3: > - remove patch 1 because it has been applied via the SPI tree already > - fix remaining YAML issues in patch 2 > - follow Miguel's suggestion on usage of Co-Developed-by > > v4: > - add patch for MAINTAINERS entry > - incorporate Miguel's review comments > - Replace Co-Developed-by with Co-developed-by (checkpatch) > v5: > - add vendor prefix to driver-specific dt properties > > Andreas Färber (1): > dt-bindings: vendor-prefixes: Add Titan Micro Electronics > > Heiner Kallweit (5): > dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 > docs: ABI: document tm1628 attribute display-text > auxdisplay: add support for Titanmec TM1628 7 segment display > controller > arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment > display > MAINTAINERS: Add entry for tm1628 auxdisplay driver > > .../testing/sysfs-devices-auxdisplay-tm1628 | 7 + > .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > MAINTAINERS | 7 + > .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++ > drivers/auxdisplay/Kconfig | 11 + > drivers/auxdisplay/Makefile | 1 + > drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++ > 8 files changed, 555 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628 > create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml > create mode 100644 drivers/auxdisplay/tm1628.c >
On 16.03.2022 01:38, Robin Murphy wrote: > On 2022-02-25 21:09, Heiner Kallweit wrote: >> This series adds support for the Titanmec TM1628 7 segment display >> controller. It's based on previous RFC work from Andreas Färber. >> The RFC version placed the driver in the LED subsystem, but this was >> NAK'ed by the LED maintainer. Therefore I moved the driver to >> /drivers/auxdisplay what seems most reasonable to me. >> >> Further changes to the RFC version: >> - Driver can be built also w/o LED class support, for displays that >> don't have any symbols to be exposed as LED's. >> - Simplified the code and rewrote a lot of it. >> - Driver is now kind of a MVP, but functionality should be sufficient >> for most use cases. >> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h >> as suggested by Geert Uytterhoeven. >> >> Note: There's a number of chips from other manufacturers that are >> almost identical, e.g. FD628, SM1628. Only difference I saw so >> far is that they partially support other display modes. >> TM1628: 6x12, 7x11 >> SM1628C: 4x13, 5x12, 6x11, 7x10 >> For typical displays on devices using these chips this >> difference shouldn't matter. >> >> Successfully tested on a TX3 Mini TV box that has an SM1628C and a >> display with 4 digits and 7 symbols. > > FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so: > > titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>; > titanmec,grid = /bits/ 8 <5 4 2 1>; > > (grid 3 segment 2 is used for a colon in the middle) > > If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used? > Definitely this could be one future extension. It could also consider that there's a number of more or less identical chips from other vendors that differ primarily in the supported display modes. > I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...) > With regards to inputs at least I have no plans because I have no hw supporting input. Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result. Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version may result in longer discussions until maintainer or I give up. Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break existing users. And I think that's the case. > Cheers, > Robin. > Heiner >> v2: >> - (re-)add Andreas' SoB to two patches >> - fix YAML issues >> - include ctype.h explicitly >> - add info message in probe() >> >> v3: >> - remove patch 1 because it has been applied via the SPI tree already >> - fix remaining YAML issues in patch 2 >> - follow Miguel's suggestion on usage of Co-Developed-by >> >> v4: >> - add patch for MAINTAINERS entry >> - incorporate Miguel's review comments >> - Replace Co-Developed-by with Co-developed-by (checkpatch) >> v5: >> - add vendor prefix to driver-specific dt properties >> >> Andreas Färber (1): >> dt-bindings: vendor-prefixes: Add Titan Micro Electronics >> >> Heiner Kallweit (5): >> dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 >> docs: ABI: document tm1628 attribute display-text >> auxdisplay: add support for Titanmec TM1628 7 segment display >> controller >> arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment >> display >> MAINTAINERS: Add entry for tm1628 auxdisplay driver >> >> .../testing/sysfs-devices-auxdisplay-tm1628 | 7 + >> .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++ >> .../devicetree/bindings/vendor-prefixes.yaml | 2 + >> MAINTAINERS | 7 + >> .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++ >> drivers/auxdisplay/Kconfig | 11 + >> drivers/auxdisplay/Makefile | 1 + >> drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++ >> 8 files changed, 555 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628 >> create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml >> create mode 100644 drivers/auxdisplay/tm1628.c >>
On 2022-03-16 21:19, Heiner Kallweit wrote: > On 16.03.2022 01:38, Robin Murphy wrote: >> On 2022-02-25 21:09, Heiner Kallweit wrote: >>> This series adds support for the Titanmec TM1628 7 segment display >>> controller. It's based on previous RFC work from Andreas Färber. >>> The RFC version placed the driver in the LED subsystem, but this was >>> NAK'ed by the LED maintainer. Therefore I moved the driver to >>> /drivers/auxdisplay what seems most reasonable to me. >>> >>> Further changes to the RFC version: >>> - Driver can be built also w/o LED class support, for displays that >>> don't have any symbols to be exposed as LED's. >>> - Simplified the code and rewrote a lot of it. >>> - Driver is now kind of a MVP, but functionality should be sufficient >>> for most use cases. >>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h >>> as suggested by Geert Uytterhoeven. >>> >>> Note: There's a number of chips from other manufacturers that are >>> almost identical, e.g. FD628, SM1628. Only difference I saw so >>> far is that they partially support other display modes. >>> TM1628: 6x12, 7x11 >>> SM1628C: 4x13, 5x12, 6x11, 7x10 >>> For typical displays on devices using these chips this >>> difference shouldn't matter. >>> >>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a >>> display with 4 digits and 7 symbols. >> >> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so: >> >> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>; >> titanmec,grid = /bits/ 8 <5 4 2 1>; >> >> (grid 3 segment 2 is used for a colon in the middle) >> >> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used? >> > Definitely this could be one future extension. It could also consider that there's a number of more or less > identical chips from other vendors that differ primarily in the supported display modes. > >> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...) >> > With regards to inputs at least I have no plans because I have no hw supporting input. FWIW, if you've got a board with exposed GPIO/SPI headers, searching "TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards. I believe they're quite popular with the Arduino crowd, so I expect that may well carry over to the Raspberry Pi crowd once they get wind of a kernel driver that can be driven by DT overlays. > Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result. > Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version > may result in longer discussions until maintainer or I give up. Unfortunately the principle is that DT bindings describe the device, not whatever the current level of Linux driver support for it might be. Perhaps I'm a little sensitised since I'm currently feeling the pain of extending a decade-old binding with functionality that was overlooked at the time, and not breaking compatibility is now rather awkward. I'm not suggesting that there needs to be any support implemented in the driver, just to be certain that we're not painting ourselves into a corner with the binding. > Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break > existing users. And I think that's the case. May I ask what you have in mind? I figure that inputs would most likely want to be described individually, similarly to the gpio-keys binding, which would lend itself to having them as child nodes, except that doesn't fit with the current scheme of child nodes having to be LEDs addressed by (grid,segment). I suppose there is a possible escape hatch of abusing unused addresses, e.g. saying a node at address (0,n) is input n rather than an LED segment, but that seems pretty horrid (and I'm not sure how well schema could validate it). Or possibly pretending to also be a GPIO controller to reference from a separate gpio-keys node, but again that seems ugly and more like something to only do if there's no other option. IMO it would be cleanest just to have an extra level of hierarchy, e.g.: led-controller@0 { compatible = "titanmec,tm1628"; ... leds { #address-cells = <2>; #size-cells = <0>; alarm@5,4 { ... }; }; }; That way there's clearly almost no risk of breakage if an additional "inputs" node with its own children turns up later. Plus it should also be a trivial change to the current driver, compared to having to implement trick special cases or whole other APIs down the line - of course bindings should not be designed expressly for ease of driver implementation, but if they do work out that way it's usually a good sign :) Thanks, Robin.
On 17.03.2022 21:08, Robin Murphy wrote: > On 2022-03-16 21:19, Heiner Kallweit wrote: >> On 16.03.2022 01:38, Robin Murphy wrote: >>> On 2022-02-25 21:09, Heiner Kallweit wrote: >>>> This series adds support for the Titanmec TM1628 7 segment display >>>> controller. It's based on previous RFC work from Andreas Färber. >>>> The RFC version placed the driver in the LED subsystem, but this was >>>> NAK'ed by the LED maintainer. Therefore I moved the driver to >>>> /drivers/auxdisplay what seems most reasonable to me. >>>> >>>> Further changes to the RFC version: >>>> - Driver can be built also w/o LED class support, for displays that >>>> don't have any symbols to be exposed as LED's. >>>> - Simplified the code and rewrote a lot of it. >>>> - Driver is now kind of a MVP, but functionality should be sufficient >>>> for most use cases. >>>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h >>>> as suggested by Geert Uytterhoeven. >>>> >>>> Note: There's a number of chips from other manufacturers that are >>>> almost identical, e.g. FD628, SM1628. Only difference I saw so >>>> far is that they partially support other display modes. >>>> TM1628: 6x12, 7x11 >>>> SM1628C: 4x13, 5x12, 6x11, 7x10 >>>> For typical displays on devices using these chips this >>>> difference shouldn't matter. >>>> >>>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a >>>> display with 4 digits and 7 symbols. >>> >>> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so: >>> >>> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>; >>> titanmec,grid = /bits/ 8 <5 4 2 1>; >>> >>> (grid 3 segment 2 is used for a colon in the middle) >>> >>> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used? >>> >> Definitely this could be one future extension. It could also consider that there's a number of more or less >> identical chips from other vendors that differ primarily in the supported display modes. >> >>> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...) >>> >> With regards to inputs at least I have no plans because I have no hw supporting input. > > FWIW, if you've got a board with exposed GPIO/SPI headers, searching "TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards. I believe they're quite popular with the Arduino crowd, so I expect that may well carry over to the Raspberry Pi crowd once they get wind of a kernel driver that can be driven by DT overlays. > >> Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result. >> Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version >> may result in longer discussions until maintainer or I give up. > > Unfortunately the principle is that DT bindings describe the device, not whatever the current level of Linux driver support for it might be. Perhaps I'm a little sensitised since I'm currently feeling the pain of extending a decade-old binding with functionality that was overlooked at the time, and not breaking compatibility is now rather awkward. > > I'm not suggesting that there needs to be any support implemented in the driver, just to be certain that we're not painting ourselves into a corner with the binding. > >> Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break >> existing users. And I think that's the case. > > May I ask what you have in mind? I figure that inputs would most likely want to be described individually, similarly to the gpio-keys binding, which would lend itself to having them as child nodes, except that doesn't fit with the current scheme of child nodes having to be LEDs addressed by (grid,segment). I suppose there is a possible escape hatch of abusing unused addresses, e.g. saying a node at address (0,n) is input n rather than an LED segment, but that seems pretty horrid (and I'm not sure how well schema could validate it). Or possibly pretending to also be a GPIO controller to reference from a separate gpio-keys node, but again that seems ugly and more like something to only do if there's no other option. > Not being an expert in OF stuff I'm just focused on getting support for the hw I own. I tried to do this in the most simple and generic way so that others can follow-up and add additional functionality. > IMO it would be cleanest just to have an extra level of hierarchy, e.g.: > > > led-controller@0 { > compatible = "titanmec,tm1628"; > ... > > leds { > #address-cells = <2>; > #size-cells = <0>; > > alarm@5,4 { > ... > }; > }; > }; > > That way there's clearly almost no risk of breakage if an additional "inputs" node with its own children turns up later. Plus it should also be a trivial change to the current driver, compared to having to implement trick special cases or whole other APIs down the line - of course bindings should not be designed expressly for ease of driver implementation, but if they do work out that way it's usually a good sign :) > > Thanks, > Robin. Heiner
On 2022-03-17 21:49, Heiner Kallweit wrote: > On 17.03.2022 21:08, Robin Murphy wrote: >> On 2022-03-16 21:19, Heiner Kallweit wrote: >>> On 16.03.2022 01:38, Robin Murphy wrote: >>>> On 2022-02-25 21:09, Heiner Kallweit wrote: >>>>> This series adds support for the Titanmec TM1628 7 segment display >>>>> controller. It's based on previous RFC work from Andreas Färber. >>>>> The RFC version placed the driver in the LED subsystem, but this was >>>>> NAK'ed by the LED maintainer. Therefore I moved the driver to >>>>> /drivers/auxdisplay what seems most reasonable to me. >>>>> >>>>> Further changes to the RFC version: >>>>> - Driver can be built also w/o LED class support, for displays that >>>>> don't have any symbols to be exposed as LED's. >>>>> - Simplified the code and rewrote a lot of it. >>>>> - Driver is now kind of a MVP, but functionality should be sufficient >>>>> for most use cases. >>>>> - Use the existing 7 segment support in uapi/linux/map_to_7segment.h >>>>> as suggested by Geert Uytterhoeven. >>>>> >>>>> Note: There's a number of chips from other manufacturers that are >>>>> almost identical, e.g. FD628, SM1628. Only difference I saw so >>>>> far is that they partially support other display modes. >>>>> TM1628: 6x12, 7x11 >>>>> SM1628C: 4x13, 5x12, 6x11, 7x10 >>>>> For typical displays on devices using these chips this >>>>> difference shouldn't matter. >>>>> >>>>> Successfully tested on a TX3 Mini TV box that has an SM1628C and a >>>>> display with 4 digits and 7 symbols. >>>> >>>> FWIW I gave this a go on my Beelink A1, which has an AiP1618 and a clock display which would mapped like so: >>>> >>>> titanmec,segment-mapping = /bits/ 8 <1 2 3 13 12 5 4>; >>>> titanmec,grid = /bits/ 8 <5 4 2 1>; >>>> >>>> (grid 3 segment 2 is used for a colon in the middle) >>>> >>>> If I bodge around the lack of support for non-contiguous grids, it does otherwise work fairly well, other than being 6-segment displays because it needs to be in display mode 1 to drive SEG13 rather than GRID6. I wonder if we could be a bit cleverer about picking a display mode based on the grid/segment numbers used? >>>> >>> Definitely this could be one future extension. It could also consider that there's a number of more or less >>> identical chips from other vendors that differ primarily in the supported display modes. >>> >>>> I also have a couple of those TM1638 breakout boards with 8 digits, 8 single LEDs and 8 buttons that I might have a go with too. Have you given any thought to how the DT binding might support inputs as well? (The best time to be future-proof is before it's merged...) >>>> >>> With regards to inputs at least I have no plans because I have no hw supporting input. >> >> FWIW, if you've got a board with exposed GPIO/SPI headers, searching "TM1638" on ebay/aliexpress/etc. should find the cheapo breakout boards. I believe they're quite popular with the Arduino crowd, so I expect that may well carry over to the Raspberry Pi crowd once they get wind of a kernel driver that can be driven by DT overlays. >> >>> Since the first attempts to support this LED driver hw two years have been passed w/o any tangible (mainline) result. >>> Therefore I want to keep the initial version a MVP. Wanting to have too many features in an initial version >>> may result in longer discussions until maintainer or I give up. >> >> Unfortunately the principle is that DT bindings describe the device, not whatever the current level of Linux driver support for it might be. Perhaps I'm a little sensitised since I'm currently feeling the pain of extending a decade-old binding with functionality that was overlooked at the time, and not breaking compatibility is now rather awkward. >> >> I'm not suggesting that there needs to be any support implemented in the driver, just to be certain that we're not painting ourselves into a corner with the binding. >> >>> Important is that user space interface / DT bindings are flexible enough so that future extensions don't have to break >>> existing users. And I think that's the case. >> >> May I ask what you have in mind? I figure that inputs would most likely want to be described individually, similarly to the gpio-keys binding, which would lend itself to having them as child nodes, except that doesn't fit with the current scheme of child nodes having to be LEDs addressed by (grid,segment). I suppose there is a possible escape hatch of abusing unused addresses, e.g. saying a node at address (0,n) is input n rather than an LED segment, but that seems pretty horrid (and I'm not sure how well schema could validate it). Or possibly pretending to also be a GPIO controller to reference from a separate gpio-keys node, but again that seems ugly and more like something to only do if there's no other option. >> > > Not being an expert in OF stuff I'm just focused on getting support for the hw I own. > I tried to do this in the most simple and generic way so that others can follow-up > and add additional functionality. Sure, I appreciate that, and what I'm saying is that while what we currently have is pleasantly simple, I think it's actually a little *too* simple and not generic enough to extend easily. I'm more than happy to send patches adding the functionality I'm interested in to the driver once it's merged, but I can't make significant changes to the binding at that point and break it for early adopters. But let me go make proper review comments on the patch rather than confusing meta-review here... Thanks, Robin. > > >> IMO it would be cleanest just to have an extra level of hierarchy, e.g.: >> >> >> led-controller@0 { >> compatible = "titanmec,tm1628"; >> ... >> >> leds { >> #address-cells = <2>; >> #size-cells = <0>; >> >> alarm@5,4 { >> ... >> }; >> }; >> }; >> >> That way there's clearly almost no risk of breakage if an additional "inputs" node with its own children turns up later. Plus it should also be a trivial change to the current driver, compared to having to implement trick special cases or whole other APIs down the line - of course bindings should not be designed expressly for ease of driver implementation, but if they do work out that way it's usually a good sign :) >> >> Thanks, >> Robin. > > Heiner
On Fri, Feb 25, 2022 at 10:09 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > > This series adds support for the Titanmec TM1628 7 segment display > controller. It's based on previous RFC work from Andreas Färber. AFAIU the discussion has converged at this point, correct? Is there any feedback left to address? Cheers, Miguel
This series adds support for the Titanmec TM1628 7 segment display controller. It's based on previous RFC work from Andreas Färber. The RFC version placed the driver in the LED subsystem, but this was NAK'ed by the LED maintainer. Therefore I moved the driver to /drivers/auxdisplay what seems most reasonable to me. Further changes to the RFC version: - Driver can be built also w/o LED class support, for displays that don't have any symbols to be exposed as LED's. - Simplified the code and rewrote a lot of it. - Driver is now kind of a MVP, but functionality should be sufficient for most use cases. - Use the existing 7 segment support in uapi/linux/map_to_7segment.h as suggested by Geert Uytterhoeven. Note: There's a number of chips from other manufacturers that are almost identical, e.g. FD628, SM1628. Only difference I saw so far is that they partially support other display modes. TM1628: 6x12, 7x11 SM1628C: 4x13, 5x12, 6x11, 7x10 For typical displays on devices using these chips this difference shouldn't matter. Successfully tested on a TX3 Mini TV box that has an SM1628C and a display with 4 digits and 7 symbols. v2: - (re-)add Andreas' SoB to two patches - fix YAML issues - include ctype.h explicitly - add info message in probe() v3: - remove patch 1 because it has been applied via the SPI tree already - fix remaining YAML issues in patch 2 - follow Miguel's suggestion on usage of Co-Developed-by v4: - add patch for MAINTAINERS entry - incorporate Miguel's review comments - Replace Co-Developed-by with Co-developed-by (checkpatch) v5: - add vendor prefix to driver-specific dt properties Andreas Färber (1): dt-bindings: vendor-prefixes: Add Titan Micro Electronics Heiner Kallweit (5): dt-bindings: auxdisplay: Add Titan Micro Electronics TM1628 docs: ABI: document tm1628 attribute display-text auxdisplay: add support for Titanmec TM1628 7 segment display controller arm64: dts: meson-gxl-s905w-tx3-mini: add support for the 7 segment display MAINTAINERS: Add entry for tm1628 auxdisplay driver .../testing/sysfs-devices-auxdisplay-tm1628 | 7 + .../bindings/auxdisplay/titanmec,tm1628.yaml | 92 +++++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 7 + .../dts/amlogic/meson-gxl-s905w-tx3-mini.dts | 59 +++ drivers/auxdisplay/Kconfig | 11 + drivers/auxdisplay/Makefile | 1 + drivers/auxdisplay/tm1628.c | 376 ++++++++++++++++++ 8 files changed, 555 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-devices-auxdisplay-tm1628 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1628.yaml create mode 100644 drivers/auxdisplay/tm1628.c