| Message ID | 20240225213423.690561-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | auxdisplay: 7 segment LED display | expand |
On Sun, Feb 25, 2024 at 11:34 PM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > > This series adds a driver for a 7 segment LED display. > > I'd like to get some feedback on how this could be extended to support >1 > character. The driver as presented is sufficient for my hardware which only has > a single character display but I can see that for this to be generically useful > supporting more characters would be desireable. > > Earlier I posted an idea that the characters could be represended by > sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > convenience of using devm_gpiod_get_array() (unless I've missed something). It seems you didn't know that the tree for auxdisplay has been changed. Can you rebase your stuff on top of https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-auxdisplay.git/log/?h=for-next? It will reduce your code base by ~50%. WRT subnodes, you can go with device_for_each_child_node() and retrieve gpio array per digit. It means you will have an array of arrays of GPIOs. > [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/
On Sun, Feb 25, 2024 at 11:34 PM Chris Packham <chris.packham@alliedtelesis.co.nz> wrote: > > Add a driver for a 7 segment LED display. At the moment only one > character is supported but it should be possible to expand this to > support more characters and/or 14 segment displays in the future. (I try to comment only on the things that will remain after rebasing on top of auxdisplay:for-next) ... > +config SEG_LED > + bool "Generic 7 segment LED display" Why can't it be a module? > + select LINEDISP > + help > + This driver supports a generic 7 segment LED display made up > + of GPIO pins connected to the individual segments. Checkpatch wants 3+ lines of help, I would add the module name (after changing it to be tristate, etc). ... > + * The GPIOs are wired to the 7 segments in a clock wise fashion starting from clockwise > + * the top. ... > + * The decimal point LED presnet on some devices is currently not present > + * supported. ... > +#include <linux/gpio/consumer.h> > +#include <linux/kernel.h> > +#include <linux/map_to_7segment.h> > +#include <linux/module.h> > +#include <linux/of.h> This is not used. And actually shouldn't be in a new code like this with rare exceptions. > +#include <linux/platform_device.h> This is rather semirandom, please use IWYU (Include What You Use) principle. We have, among others, container_of.h, types.h, err.h, bitmap.h, mod_devicetable.h. ... With sturct device *dev = &pdev->dev; the below code will be neater. > + priv = devm_kzalloc(&pdev->dev, struct_size(priv, curr, 1), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + priv->num_char = 1; > + > + platform_set_drvdata(pdev, priv); > + > + priv->segment_gpios = devm_gpiod_get_array(&pdev->dev, "segment", GPIOD_OUT_LOW); > + if (IS_ERR(priv->segment_gpios)) > + return PTR_ERR(priv->segment_gpios); ... > +static struct platform_driver seg_led_driver = { > + .probe = seg_led_probe, > + .remove = seg_led_remove, > + .driver = { > + .name = "seg-led", > + .of_match_table = seg_led_of_match, > + }, > +}; > + Redundant blank line. > +module_platform_driver(seg_led_driver); > + > +MODULE_AUTHOR("Chris Packham <chris.packham@alliedtelesis.co.nz>"); > +MODULE_DESCRIPTION("7 segment LED driver"); > +MODULE_LICENSE("GPL"); > + Seems like a redundant blank line at the end of the file.
Hello Chris, Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham: > This series adds a driver for a 7 segment LED display. > > I'd like to get some feedback on how this could be extended to support >1 > character. The driver as presented is sufficient for my hardware which only has > a single character display but I can see that for this to be generically useful > supporting more characters would be desireable. > > Earlier I posted an idea that the characters could be represended by > sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > convenience of using devm_gpiod_get_array() (unless I've missed something). > > [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/ Read that thread out of curiosity and I'm sorry if I'm late to the party, but I wondered why this is limited to LEDs connected to GPIOs? Would it be possible to somehow stack this on top of some existing LEDs? I mean you could wire a 7 segment device to almost any LED driver IC with enough channels, couldn't you? Greets Alex > > Chris Packham (3): > auxdisplay: Add 7 segment LED display driver > dt-bindings: auxdisplay: Add bindings for generic 7 segment LED > ARM: dts: marvell: Add 7 segment LED display on x530 > > .../auxdisplay/generic,gpio-7seg.yaml | 40 +++++ > .../boot/dts/marvell/armada-385-atl-x530.dts | 13 +- > drivers/auxdisplay/Kconfig | 7 + > drivers/auxdisplay/Makefile | 1 + > drivers/auxdisplay/seg-led.c | 152 ++++++++++++++++++ > 5 files changed, 212 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml > create mode 100644 drivers/auxdisplay/seg-led.c > > -- > 2.43.2 > >
On Mon, Feb 26, 2024 at 04:23:15AM +0200, Andy Shevchenko wrote: > On Sun, Feb 25, 2024 at 11:34 PM Chris Packham > <chris.packham@alliedtelesis.co.nz> wrote: > > > > This series adds a driver for a 7 segment LED display. > > > > I'd like to get some feedback on how this could be extended to support >1 > > character. The driver as presented is sufficient for my hardware which only has > > a single character display but I can see that for this to be generically useful > > supporting more characters would be desireable. > > > > Earlier I posted an idea that the characters could be represended by > > sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > > convenience of using devm_gpiod_get_array() (unless I've missed something). > > It seems you didn't know that the tree for auxdisplay has been changed. > Can you rebase your stuff on top of > https://git.kernel.org/pub/scm/linux/kernel/git/andy/linux-auxdisplay.git/log/?h=for-next? > It will reduce your code base by ~50%. I have just updated the branch so it adds one patch that changes the prototype of linedisp_register(). > WRT subnodes, you can go with device_for_each_child_node() and > retrieve gpio array per digit. It means you will have an array of > arrays of GPIOs. Btw, as Geert proposed for another 7-segment driver, we might gain from the display-width-chars property. But I think this property has to be parsed on top of line display library, no need to have it in each affected driver. > > [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/
On Mon, 26 Feb 2024 10:34:20 +1300, Chris Packham wrote: > This series adds a driver for a 7 segment LED display. > > I'd like to get some feedback on how this could be extended to support >1 > character. The driver as presented is sufficient for my hardware which only has > a single character display but I can see that for this to be generically useful > supporting more characters would be desireable. > > Earlier I posted an idea that the characters could be represended by > sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > convenience of using devm_gpiod_get_array() (unless I've missed something). > > [1] - https://lore.kernel.org/lkml/2a8d19ee-b18b-4b7c-869f-7d601cea30b6@alliedtelesis.co.nz/ > > Chris Packham (3): > auxdisplay: Add 7 segment LED display driver > dt-bindings: auxdisplay: Add bindings for generic 7 segment LED > ARM: dts: marvell: Add 7 segment LED display on x530 > > .../auxdisplay/generic,gpio-7seg.yaml | 40 +++++ > .../boot/dts/marvell/armada-385-atl-x530.dts | 13 +- > drivers/auxdisplay/Kconfig | 7 + > drivers/auxdisplay/Makefile | 1 + > drivers/auxdisplay/seg-led.c | 152 ++++++++++++++++++ > 5 files changed, 212 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml > create mode 100644 drivers/auxdisplay/seg-led.c > > -- > 2.43.2 > > > My bot found new DT warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y marvell/armada-385-atl-x530.dtb' for 20240225213423.690561-1-chris.packham@alliedtelesis.co.nz: arch/arm/boot/dts/marvell/armada-385-atl-x530.dtb: soc: internal-regs: {'compatible': ['simple-bus'], '#address-cells': [[1]], '#size-cells': [[1]], 'ranges': [[0, 4026597376, 0, 1048576]], 'sdramc@1400': {'compatible': ['marvell,armada-xp-sdram-controller'], 'reg': [[5120, 1280]]}, 'cache-controller@8000': {'compatible': ['arm,pl310-cache'], 'reg': [[32768, 4096]], 'cache-unified': True, 'cache-level': [[2]], 'arm,double-linefill-incr': [[0]], 'arm,double-linefill-wrap': [[0]], 'arm,double-linefill': [[0]], 'prefetch-data': [[1]]}, 'scu@c000': {'compatible': ['arm,cortex-a9-scu'], 'reg': [[49152, 88]]}, 'timer@c200': {'compatible': ['arm,cortex-a9-global-timer'], 'reg': [[49664, 32]], 'interrupts': [[1, 11, 769]], 'clocks': [[4, 2]]}, 'timer@c600': {'compatible': ['arm,cortex-a9-twd-timer'], 'reg': [[50688, 32]], 'interrupts': [[1, 13, 769]], 'clocks': [[4, 2]]}, 'interrupt-controller@d000': {'compatible': ['arm,cortex-a9-gic'], '#interrupt-cells': [[3]], '#size-cells': [[0]], 'interrupt-controller': True, 'reg': [[53248, 4096], [49408, 256]], 'phandle': [[3]]}, 'i2c@11000': {'compatible': ['marvell,mv78230-a0-i2c', 'marvell,mv64xxx-i2c'], 'reg': [[69632, 32]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 2, 4]], 'clocks': [[4, 0]], 'status': ['okay'], 'pinctrl-names': ['default', 'gpio'], 'pinctrl-0': [[5]], 'clock-frequency': [[100000]], 'pinctrl-1': [[6]], 'scl-gpio': [[7, 2, 6]], 'sda-gpio': [[7, 3, 6]], 'mux@71': {'#address-cells': [[1]], '#size-cells': [[0]], 'compatible': ['nxp,pca9544'], 'reg': [[113]], 'i2c-mux-idle-disconnect': True, 'i2c@0': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[0]]}, 'i2c@1': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[1]], 'hwmon@2e': {'compatible': ['adi,adt7476'], 'reg': [[46]]}, 'hwmon@2d': {'compatible': ['adi,adt7476'], 'reg': [[45]]}}, 'i2c@2': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[2]], 'rtc@68': {'compatible': ['dallas,ds1340'], 'reg': [[104]]}}, 'i2c@3': {'#address-cells': [[1]], '#size-cells': [[0]], 'reg': [[3]], 'gpio@20': {'compatible': ['nxp,pca9554'], 'gpio-controller': True, '#gpio-cells': [[2]], 'reg': [[32]], 'phandle': [[23]]}}}}, 'i2c@11100': {'compatible': ['marvell,mv78230-a0-i2c', 'marvell,mv64xxx-i2c'], 'reg': [[69888, 32]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 3, 4]], 'clocks': [[4, 0]], 'status': ['disabled']}, 'serial@12000': {'compatible': ['marvell,armada-38x-uart', 'ns16550a'], 'reg': [[73728, 256]], 'reg-shift': [[2]], 'interrupts': [[0, 12, 4]], 'reg-io-width': [[1]], 'clocks': [[4, 0]], 'status': ['okay'], 'pinctrl-names': ['default'], 'pinctrl-0': [[8]]}, 'serial@12100': {'compatible': ['marvell,armada-38x-uart', 'ns16550a'], 'reg': [[73984, 256]], 'reg-shift': [[2]], 'interrupts': [[0, 13, 4]], 'reg-io-width': [[1]], 'clocks': [[4, 0]], 'status': ['disabled']}, 'pinctrl@18000': {'reg': [[98304, 32]], 'compatible': ['marvell,mv88f6820-pinctrl'], 'phandle': [[9]], 'ge-rgmii-pins-0': {'marvell,pins': ['mpp6', 'mpp7', 'mpp8', 'mpp9', 'mpp10', 'mpp11', 'mpp12', 'mpp13', 'mpp14', 'mpp15', 'mpp16', 'mpp17'], 'marvell,function': ['ge0']}, 'ge-rgmii-pins-1': {'marvell,pins': ['mpp21', 'mpp27', 'mpp28', 'mpp29', 'mpp30', 'mpp31', 'mpp32', 'mpp37', 'mpp38', 'mpp39', 'mpp40', 'mpp41'], 'marvell,function': ['ge1']}, 'i2c-pins-0': {'marvell,pins': ['mpp2', 'mpp3'], 'marvell,function': ['i2c0'], 'phandle': [[5]]}, 'mdio-pins': {'marvell,pins': ['mpp4', 'mpp5'], 'marvell,function': ['ge']}, 'ref-clk-pins-0': {'marvell,pins': ['mpp45'], 'marvell,function': ['ref']}, 'ref-clk-pins-1': {'marvell,pins': ['mpp46'], 'marvell,function': ['ref']}, 'spi-pins-0': {'marvell,pins': ['mpp22', 'mpp23', 'mpp24', 'mpp25'], 'marvell,function': ['spi0']}, 'spi-pins-1': {'marvell,pins': ['mpp56', 'mpp57', 'mpp58', 'mpp59'], 'marvell,function': ['spi1'], 'phandle': [[17]]}, 'nand-pins': {'marvell,pins': ['mpp22', 'mpp34', 'mpp23', 'mpp33', 'mpp38', 'mpp28', 'mpp40', 'mpp42', 'mpp35', 'mpp36', 'mpp25', 'mpp30', 'mpp32'], 'marvell,function': ['dev']}, 'nand-rb': {'marvell,pins': ['mpp41'], 'marvell,function': ['nand']}, 'uart-pins-0': {'marvell,pins': ['mpp0', 'mpp1'], 'marvell,function': ['ua0'], 'phandle': [[8]]}, 'uart-pins-1': {'marvell,pins': ['mpp19', 'mpp20'], 'marvell,function': ['ua1']}, 'sdhci-pins': {'marvell,pins': ['mpp48', 'mpp49', 'mpp50', 'mpp52', 'mpp53', 'mpp54', 'mpp55', 'mpp57', 'mpp58', 'mpp59'], 'marvell,function': ['sd0']}, 'sata-pins-0': {'marvell,pins': ['mpp20'], 'marvell,function': ['sata0']}, 'sata-pins-1': {'marvell,pins': ['mpp19'], 'marvell,function': ['sata1']}, 'sata-pins-2': {'marvell,pins': ['mpp47'], 'marvell,function': ['sata2']}, 'sata-pins-3': {'marvell,pins': ['mpp44'], 'marvell,function': ['sata3']}, 'i2s-pins': {'marvell,pins': ['mpp48', 'mpp49', 'mpp50', 'mpp51', 'mpp52', 'mpp53'], 'marvell,function': ['audio']}, 'spdif-pins': {'marvell,pins': ['mpp51'], 'marvell,function': ['audio']}, 'i2c-gpio-pins-0': {'marvell,pins': ['mpp2', 'mpp3'], 'marvell,function': ['gpio'], 'phandle': [[6]]}}, 'gpio@18100': {'compatible': ['marvell,armada-370-gpio', 'marvell,orion-gpio'], 'reg': [[98560, 64], [98752, 8]], 'reg-names': ['gpio', 'pwm'], 'ngpios': [[32]], 'gpio-controller': True, 'gpio-ranges': [[9, 0, 0, 32]], '#gpio-cells': [[2]], '#pwm-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], 'interrupts': [[0, 53, 4], [0, 54, 4], [0, 55, 4], [0, 56, 4]], 'clocks': [[4, 0]], 'phandle': [[7]]}, 'gpio@18140': {'compatible': ['marvell,armada-370-gpio', 'marvell,orion-gpio'], 'reg': [[98624, 64], [98760, 8]], 'reg-names': ['gpio', 'pwm'], 'ngpios': [[28]], 'gpio-controller': True, 'gpio-ranges': [[9, 0, 32, 28]], '#gpio-cells': [[2]], '#pwm-cells': [[2]], 'interrupt-controller': True, '#interrupt-cells': [[2]], 'interrupts': [[0, 58, 4], [0, 59, 4], [0, 60, 4], [0, 61, 4]], 'clocks': [[4, 0]], 'phandle': [[19]]}, 'system-controller@18200': {'compatible': ['marvell,armada-380-system-controller', 'marvell,armada-370-xp-system-controller'], 'reg': [[98816, 256]]}, 'clock-gating-control@18220': {'compatible': ['marvell,armada-380-gating-clock'], 'reg': [[98848, 4]], 'clocks': [[4, 0]], '#clock-cells': [[1]], 'phandle': [[11]]}, 'phy@18300': {'compatible': ['marvell,armada-380-comphy'], 'reg-names': ['comphy', 'conf'], 'reg': [[99072, 256], [99424, 4]], '#address-cells': [[1]], '#size-cells': [[0]], 'phy@0': {'reg': [[0]], '#phy-cells': [[1]]}, 'phy@1': {'reg': [[1]], '#phy-cells': [[1]]}, 'phy@2': {'reg': [[2]], '#phy-cells': [[1]]}, 'phy@3': {'reg': [[3]], '#phy-cells': [[1]]}, 'phy@4': {'reg': [[4]], '#phy-cells': [[1]]}, 'phy@5': {'reg': [[5]], '#phy-cells': [[1]]}}, 'mvebu-sar@18600': {'compatible': ['marvell,armada-380-core-clock'], 'reg': [[99840, 4]], '#clock-cells': [[1]], 'phandle': [[4]]}, 'mbus-controller@20000': {'compatible': ['marvell,mbus-controller'], 'reg': [[131072, 256], [131456, 32], [131664, 8]], 'phandle': [[2]]}, 'interrupt-controller@20a00': {'compatible': ['marvell,mpic'], 'reg': [[133632, 720], [135280, 88]], '#interrupt-cells': [[1]], '#size-cells': [[1]], 'interrupt-controller': True, 'msi-controller': True, 'interrupts': [[1, 15, 4]], 'phandle': [[1]]}, 'timer@20300': {'compatible': ['marvell,armada-380-timer', 'marvell,armada-xp-timer'], 'reg': [[131840, 48], [135232, 48]], 'interrupts-extended': [[3, 0, 8, 4], [3, 0, 9, 4], [3, 0, 10, 4], [3, 0, 11, 4], [1, 5], [1, 6]], 'clocks': [[4, 2], [10]], 'clock-names': ['nbclk', 'fixed']}, 'watchdog@20300': {'compatible': ['marvell,armada-380-wdt'], 'reg': [[131840, 52], [132868, 4], [98912, 4]], 'clocks': [[4, 2], [10]], 'clock-names': ['nbclk', 'fixed'], 'interrupts-extended': [[3, 0, 64, 4], [3, 0, 9, 4]]}, 'cpurst@20800': {'compatible': ['marvell,armada-370-cpu-reset'], 'reg': [[133120, 16]]}, 'mpcore-soc-ctrl@20d20': {'compatible': ['marvell,armada-380-mpcore-soc-ctrl'], 'reg': [[134432, 108]]}, 'coherency-fabric@21010': {'compatible': ['marvell,armada-380-coherency-fabric'], 'reg': [[135184, 28]]}, 'pmsu@22000': {'compatible': ['marvell,armada-380-pmsu'], 'reg': [[139264, 4096]]}, 'ethernet@70000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[458752, 16384]], 'interrupts-extended': [[1, 8]], 'clocks': [[11, 4]], 'tx-csum-limit': [[9800]], 'status': ['disabled']}, 'ethernet@30000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[196608, 16384]], 'interrupts-extended': [[1, 10]], 'clocks': [[11, 3]], 'status': ['disabled']}, 'ethernet@34000': {'compatible': ['marvell,armada-370-neta'], 'reg': [[212992, 16384]], 'interrupts-extended': [[1, 12]], 'clocks': [[11, 2]], 'status': ['disabled']}, 'usb@58000': {'compatible': ['marvell,orion-ehci'], 'reg': [[360448, 1280]], 'interrupts': [[0, 18, 4]], 'clocks': [[11, 18]], 'status': ['okay']}, 'xor@60800': {'compatible': ['marvell,armada-380-xor', 'marvell,orion-xor'], 'reg': [[395264, 256], [395776, 256]], 'clocks': [[11, 22]], 'status': ['okay'], 'xor00': {'interrupts': [[0, 22, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True}, 'xor01': {'interrupts': [[0, 23, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True, 'dmacap,memset': True}}, 'xor@60900': {'compatible': ['marvell,armada-380-xor', 'marvell,orion-xor'], 'reg': [[395520, 256], [396032, 256]], 'clocks': [[11, 28]], 'status': ['okay'], 'xor10': {'interrupts': [[0, 65, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True}, 'xor11': {'interrupts': [[0, 66, 4]], 'dmacap,memcpy': True, 'dmacap,xor': True, 'dmacap,memset': True}}, 'mdio@72004': {'#address-cells': [[1]], '#size-cells': [[0]], 'compatible': ['marvell,orion-mdio'], 'reg': [[466948, 4]], 'clocks': [[11, 4]]}, 'crypto@90000': {'compatible': ['marvell,armada-38x-crypto'], 'reg': [[589824, 65536]], 'reg-names': ['regs'], 'interrupts': [[0, 19, 4], [0, 20, 4]], 'clocks': [[11, 23], [11, 21], [11, 14], [11, 16]], 'clock-names': ['cesa0', 'cesa1', 'cesaz0', 'cesaz1'], 'marvell,crypto-srams': [[12, 13]], 'marvell,crypto-sram-size': [[2048]]}, 'rtc@a3800': {'compatible': ['marvell,armada-380-rtc'], 'reg': [[669696, 32], [99488, 12]], 'reg-names': ['rtc', 'rtc-soc'], 'interrupts': [[0, 21, 4]]}, 'sata@a8000': {'compatible': ['marvell,armada-380-ahci'], 'reg': [[688128, 8192]], 'interrupts': [[0, 26, 4]], 'clocks': [[11, 15]], 'status': ['disabled']}, 'bm@c8000': {'compatible': ['marvell,armada-380-neta-bm'], 'reg': [[819200, 172]], 'clocks': [[11, 13]], 'internal-mem': [[14]], 'status': ['disabled']}, 'sata@e0000': {'compatible': ['marvell,armada-380-ahci'], 'reg': [[917504, 8192]], 'interrupts': [[0, 28, 4]], 'clocks': [[11, 30]], 'status': ['disabled']}, 'clock@e4250': {'compatible': ['marvell,armada-380-corediv-clock'], 'reg': [[934480, 12]], '#clock-cells': [[1]], 'clocks': [[15]], 'clock-output-names': ['nand'], 'phandle': [[16]]}, 'thermal@e8078': {'compatible': ['marvell,armada380-thermal'], 'reg': [[934008, 4], [934000, 8]], 'status': ['okay']}, 'nand-controller@d0000': {'compatible': ['marvell,armada370-nand-controller'], 'reg': [[851968, 84]], '#address-cells': [[1]], '#size-cells': [[0]], 'interrupts': [[0, 84, 4]], 'clocks': [[16, 0]], 'status': ['okay'], 'nand@0': {'reg': [[0]], 'label': ['pxa3xx_nand-0'], 'nand-rb': [[0]], 'nand-on-flash-bbt': True, 'nand-ecc-strength': [[4]], 'nand-ecc-step-size': [[512]], 'marvell,nand-enable-arbiter': True, 'partitions': {'compatible': ['fixed-partitions'], '#address-cells': [[1]], '#size-cells': [[1]], 'partition@0': {'reg': [[0, 251658240]], 'label': ['user']}, 'partition@f000000': {'reg': [[251658240, 8388608]], 'label': ['errlog']}, 'partition@f800000': {'reg': [[260046848, 8388608]], 'label': ['nand-bbt']}}}}, 'sdhci@d8000': {'compatible': ['marvell,armada-380-sdhci'], 'reg-names': ['sdhci', 'mbus', 'conf-sdio3'], 'reg': [[884736, 4096], [901120, 256], [99412, 4]], 'interrupts': [[0, 25, 4]], 'clocks': [[11, 17]], 'mrvl,clk-delay-cycles': [[31]], 'status': ['disabled']}, 'audio-controller@e8000': {'#sound-dai-cells': [[1]], 'compatible': ['marvell,armada-380-audio'], 'reg': [[950272, 16384], [99344, 12], [98820, 4]], 'reg-names': ['i2s_regs', 'pll_regs', 'soc_ctrl'], 'interrupts': [[0, 75, 4]], 'clocks': [[11, 0]], 'clock-names': ['internal'], 'status': ['disabled']}, 'usb3@f0000': {'compatible': ['marvell,armada-380-xhci'], 'reg': [[983040, 16384], [999424, 16384]], 'interrupts': [[0, 16, 4]], 'clocks': [[11, 9]], 'status': ['disabled']}, 'usb3@f8000': {'compatible': ['marvell,armada-380-xhci'], 'reg': [[1015808, 16384], [1032192, 16384]], 'interrupts': [[0, 17, 4]], 'clocks': [[11, 10]], 'status': ['disabled']}} should not be valid under {'type': 'object'} from schema $id: http://devicetree.org/schemas/simple-bus.yaml#
Hi Alex, On 27/02/24 05:04, Alexander Dahl wrote: > Hello Chris, > > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham: >> This series adds a driver for a 7 segment LED display. >> >> I'd like to get some feedback on how this could be extended to support >1 >> character. The driver as presented is sufficient for my hardware which only has >> a single character display but I can see that for this to be generically useful >> supporting more characters would be desireable. >> >> Earlier I posted an idea that the characters could be represended by >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the >> convenience of using devm_gpiod_get_array() (unless I've missed something). >> >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f > Read that thread out of curiosity and I'm sorry if I'm late to the > party, but I wondered why this is limited to LEDs connected to GPIOs? > > Would it be possible to somehow stack this on top of some existing > LEDs? I mean you could wire a 7 segment device to almost any LED > driver IC with enough channels, couldn't you? Mainly because the GPIO version is the hardware I have. I do wonder how this might work with something like the pca9551 which really is just a fancy version of the pca9554 on my board. A naive implementation would be to configure all the pca9551 pins as GPIOs and use what I have as-is. Making a line display out of LED triggers might be another way of doing it but not something I really want to pursue. > > Greets > Alex > >> Chris Packham (3): >> auxdisplay: Add 7 segment LED display driver >> dt-bindings: auxdisplay: Add bindings for generic 7 segment LED >> ARM: dts: marvell: Add 7 segment LED display on x530 >> >> .../auxdisplay/generic,gpio-7seg.yaml | 40 +++++ >> .../boot/dts/marvell/armada-385-atl-x530.dts | 13 +- >> drivers/auxdisplay/Kconfig | 7 + >> drivers/auxdisplay/Makefile | 1 + >> drivers/auxdisplay/seg-led.c | 152 ++++++++++++++++++ >> 5 files changed, 212 insertions(+), 1 deletion(-) >> create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml >> create mode 100644 drivers/auxdisplay/seg-led.c >> >> -- >> 2.43.2 >> >>
On 26/02/24 15:23, Andy Shevchenko wrote: > On Sun, Feb 25, 2024 at 11:34 PM Chris Packham > <chris.packham@alliedtelesis.co.nz> wrote: >> This series adds a driver for a 7 segment LED display. >> >> I'd like to get some feedback on how this could be extended to support >1 >> character. The driver as presented is sufficient for my hardware which only has >> a single character display but I can see that for this to be generically useful >> supporting more characters would be desireable. >> >> Earlier I posted an idea that the characters could be represended by >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the >> convenience of using devm_gpiod_get_array() (unless I've missed something). > It seems you didn't know that the tree for auxdisplay has been changed. > Can you rebase your stuff on top of > https://scanmail.trustwave.com/?c=20988&d=vfbb5fnU59kvIREfdD-21Pab30bpMpuTM2Ipv28now&u=https%3a%2f%2fgit%2ekernel%2eorg%2fpub%2fscm%2flinux%2fkernel%2fgit%2fandy%2flinux-auxdisplay%2egit%2flog%2f%3fh%3dfor-next%3f > It will reduce your code base by ~50%. > > WRT subnodes, you can go with device_for_each_child_node() and > retrieve gpio array per digit. It means you will have an array of > arrays of GPIOs. So would the following work? count = device_get_child_node_count(dev); struct gpio_descs **chars = devm_kzalloc(dev, sizeof(*chars) * count, GFP_KERNEL); i = 0; device_for_each_child_node(dev, child) { chars[i] = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW); } I haven't used the child. The devm_gpiod_get_array() will be looking at the fwnode of the parent.
On Tue, Feb 27, 2024 at 2:52 AM Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote: > On 26/02/24 15:23, Andy Shevchenko wrote: > > On Sun, Feb 25, 2024 at 11:34 PM Chris Packham > > <chris.packham@alliedtelesis.co.nz> wrote: > >> This series adds a driver for a 7 segment LED display. > >> > >> I'd like to get some feedback on how this could be extended to support >1 > >> character. The driver as presented is sufficient for my hardware which only has > >> a single character display but I can see that for this to be generically useful > >> supporting more characters would be desireable. > >> > >> Earlier I posted an idea that the characters could be represended by > >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > >> convenience of using devm_gpiod_get_array() (unless I've missed something). > > It seems you didn't know that the tree for auxdisplay has been changed. > > Can you rebase your stuff on top of > > https://scanmail.trustwave.com/?c=20988&d=vfbb5fnU59kvIREfdD-21Pab30bpMpuTM2Ipv28now&u=https%3a%2f%2fgit%2ekernel%2eorg%2fpub%2fscm%2flinux%2fkernel%2fgit%2fandy%2flinux-auxdisplay%2egit%2flog%2f%3fh%3dfor-next%3f > > It will reduce your code base by ~50%. > > > > WRT subnodes, you can go with device_for_each_child_node() and > > retrieve gpio array per digit. It means you will have an array of > > arrays of GPIOs. > > So would the following work? > > count = device_get_child_node_count(dev); > struct gpio_descs **chars = devm_kzalloc(dev, sizeof(*chars) * > count, GFP_KERNEL); > > i = 0; > device_for_each_child_node(dev, child) { > chars[i] = devm_gpiod_get_array(dev, "segment", GPIOD_OUT_LOW); I see what you meant earlier. This should be devm_fwnode_gpiod_get_index(), but we lack the _array variant of it... Dunno what to do, maybe adding the _array variant is the good move, maybe something else. > } > > I haven't used the child. The devm_gpiod_get_array() will be looking at > the fwnode of the parent.
Hello Chris, Am Mon, Feb 26, 2024 at 08:10:07PM +0000 schrieb Chris Packham: > Hi Alex, > > On 27/02/24 05:04, Alexander Dahl wrote: > > Hello Chris, > > > > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham: > >> This series adds a driver for a 7 segment LED display. > >> > >> I'd like to get some feedback on how this could be extended to support >1 > >> character. The driver as presented is sufficient for my hardware which only has > >> a single character display but I can see that for this to be generically useful > >> supporting more characters would be desireable. > >> > >> Earlier I posted an idea that the characters could be represended by > >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > >> convenience of using devm_gpiod_get_array() (unless I've missed something). > >> > >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f > > Read that thread out of curiosity and I'm sorry if I'm late to the > > party, but I wondered why this is limited to LEDs connected to GPIOs? > > > > Would it be possible to somehow stack this on top of some existing > > LEDs? I mean you could wire a 7 segment device to almost any LED > > driver IC with enough channels, couldn't you? > > Mainly because the GPIO version is the hardware I have. I do wonder how > this might work with something like the pca9551 which really is just a > fancy version of the pca9554 on my board. A naive implementation would > be to configure all the pca9551 pins as GPIOs and use what I have as-is. My idea was to do it on top of the LED abstraction, not on top of the GPIO abstraction. Currently you are using the GPIO consumer interface and group 7 gpios which are supplied by that pca9554 in your case, but could also be coming from a SoC or a 74hc595 etc. Why not put it a level of abstraction higher, and let it consume LEDs instead of GPIOs? Your usecase would still be possible then. As far as I could see the concept of a LED consumer can be done, the leds-group-multicolor driver in drivers/leds/rgb/leds-group-multicolor.c does that, introduced with kernel v6.6 not so long ago. It sets the sysfs entries of the LEDs part of the group to readonly so they are not usable on their own anymore and one would not disturb the grouped multicolor LED. This would allow to use LEDs as a 7 segment group driven by any LED driver including leds-gpio. > Making a line display out of LED triggers might be another way of doing > it but not something I really want to pursue. Fair enough. I just wanted to share my idea. Didn't want to pressure you in any direction. :-) Greets Alex > > > > > Greets > > Alex > > > >> Chris Packham (3): > >> auxdisplay: Add 7 segment LED display driver > >> dt-bindings: auxdisplay: Add bindings for generic 7 segment LED > >> ARM: dts: marvell: Add 7 segment LED display on x530 > >> > >> .../auxdisplay/generic,gpio-7seg.yaml | 40 +++++ > >> .../boot/dts/marvell/armada-385-atl-x530.dts | 13 +- > >> drivers/auxdisplay/Kconfig | 7 + > >> drivers/auxdisplay/Makefile | 1 + > >> drivers/auxdisplay/seg-led.c | 152 ++++++++++++++++++ > >> 5 files changed, 212 insertions(+), 1 deletion(-) > >> create mode 100644 Documentation/devicetree/bindings/auxdisplay/generic,gpio-7seg.yaml > >> create mode 100644 drivers/auxdisplay/seg-led.c > >> > >> -- > >> 2.43.2 > >> > >>
On Tue, Feb 27, 2024 at 10:43 AM Alexander Dahl <ada@thorsis.com> wrote: > Am Mon, Feb 26, 2024 at 08:10:07PM +0000 schrieb Chris Packham: > > On 27/02/24 05:04, Alexander Dahl wrote: > > > Am Mon, Feb 26, 2024 at 10:34:20AM +1300 schrieb Chris Packham: > > >> This series adds a driver for a 7 segment LED display. > > >> > > >> I'd like to get some feedback on how this could be extended to support >1 > > >> character. The driver as presented is sufficient for my hardware which only has > > >> a single character display but I can see that for this to be generically useful > > >> supporting more characters would be desireable. > > >> > > >> Earlier I posted an idea that the characters could be represended by > > >> sub-nodes[1] but there doesn't seem to be a way of having that and keeping the > > >> convenience of using devm_gpiod_get_array() (unless I've missed something). > > >> > > >> [1] - https://scanmail.trustwave.com/?c=20988&d=trbc5eARVo-5gepRnwbAKbQmiGk1bOSpqZDQx9bx7w&u=https%3a%2f%2flore%2ekernel%2eorg%2flkml%2f2a8d19ee-b18b-4b7c-869f-7d601cea30b6%40alliedtelesis%2eco%2enz%2f > > > Read that thread out of curiosity and I'm sorry if I'm late to the > > > party, but I wondered why this is limited to LEDs connected to GPIOs? > > > > > > Would it be possible to somehow stack this on top of some existing > > > LEDs? I mean you could wire a 7 segment device to almost any LED > > > driver IC with enough channels, couldn't you? > > > > Mainly because the GPIO version is the hardware I have. I do wonder how > > this might work with something like the pca9551 which really is just a > > fancy version of the pca9554 on my board. A naive implementation would > > be to configure all the pca9551 pins as GPIOs and use what I have as-is. > > My idea was to do it on top of the LED abstraction, not on top of the > GPIO abstraction. Currently you are using the GPIO consumer interface > and group 7 gpios which are supplied by that pca9554 in your case, but > could also be coming from a SoC or a 74hc595 etc. > > Why not put it a level of abstraction higher, and let it consume LEDs > instead of GPIOs? Your usecase would still be possible then. > > As far as I could see the concept of a LED consumer can be done, the > leds-group-multicolor driver in > drivers/leds/rgb/leds-group-multicolor.c does that, introduced with > kernel v6.6 not so long ago. It sets the sysfs entries of the LEDs > part of the group to readonly so they are not usable on their own > anymore and one would not disturb the grouped multicolor LED. > > This would allow to use LEDs as a 7 segment group driven by any LED > driver including leds-gpio. 7-segment LEDs are not just a bunch of leds, they have explicit meaning and using LED infrastructure is an obscuration of what's going on (too many unrelated details are exposed, too hard to achieve what user needs). In the auxdisplay we have already the concept of "line display" with a 7-segment, or 14 (that are two main standards) with respective character mapping in case somebody wants to print more than hexadecimal digits on them. If I am mistaken, I would like to see the concept in the example here on how user space will take care of displaying (an arbitrary) data. Can you provide a draft of (user-side) documentation before we even start going this direction? > > Making a line display out of LED triggers might be another way of doing > > it but not something I really want to pursue. > > Fair enough. I just wanted to share my idea. Didn't want to pressure > you in any direction. :-)
Hi! > My idea was to do it on top of the LED abstraction, not on top of the > GPIO abstraction. Currently you are using the GPIO consumer > interface Let's not do that. Pavel