mbox series

[v5,00/19] auxdisplay: ht16k33: Add character display support

Message ID 20210811095759.1281480-1-geert@linux-m68k.org
Headers show
Series auxdisplay: ht16k33: Add character display support | expand

Message

Geert Uytterhoeven Aug. 11, 2021, 9:57 a.m. UTC
Hi all,

The Holtek HT16K33 LED controller is not only used for driving
dot-matrix displays, but also for driving segment displays.
The current auxdisplay driver is limited to dot-matrix displays, which
are exposed as a frame buffer device.

This patch series extends the driver to 4-digit 7-segment and quad
14-segment alphanumeric displays, allowing the user to display and
scroll text messages.

List of patches:
  - Patch 1 provides font data for displaying ASCII characters on
    14-segment displays,
  - Patch 2 updates the HT16K33 DT bindings for segment displays,
  - Patches 3-5 contain a bug fix and small improvements for the
    Imagination Technologies ASCII LCD Display driver,
  - Patch 6 extracts the character line display core support from the
    Imagination Technologies ASCII LCD Display driver, for reuse,
  - Patches 7-8 contain cleanups and improvements for the character line
    display core driver,
  - Patches 9-16 contain a bug fix, cleanups and improvements for the
    HT16K33 driver, to prepare for segment display support,
  - Patch 17 adds support for 7/14-segment displays to the HT16K33
    driver,
  - Patch 18 updates the HT16K33 DT bindings to document an LED subnode,
  - Patch 19 adds segment display LED support to the HT16K33 driver,
    to make use of hardware blinking, and to expose display color.

Changes compared to v4[1]:
  - Add Reviewed-by,
  - Add missing select NEW_LEDS.

Changes compared to v3[2]:
  - Combine compatible values for 7/14 segment displays into an enum,
  - Add Reviewed-by,
  - Add missing select LEDS_CLASS.

Changes compared to v2[3]:
  - Drop color property from display node,
  - Use compat_only_sysfs_link_entry_to_kobj() instead of cooking our
    own helper on top of kernfs_create_link(),
  - Use "err" instead of "error" to be consistent with existing driver
    naming style,
  - Pass "dev" instead of "client" to ht16k33_fbdev_probe() and
    ht16k33_seg_probe(),
  - Drop local variable "node",
  - Remove unneeded inclusion of <linux/leds.h> and <linux/of_device.h>,
  - Document LED subnode,
  - Remove unneeded C++ comment,
  - Make the creation of the LED device dependent on the presence of the
    "led" subnode in DT, so it can be used in dot-matrix mode too.
  - Use led_init_data() and devm_led_classdev_register_ext() to retrieve
    all LED properties from DT, instead of manual LED name construction
    based on just the "color" property.

Changes compared to v1[4]:
  - Fix type of color to uint32,
  - "refresh-rate-hz" is still required for dot-matrix displays.
  - Move "select LINEDISP" for HT16K33 symbol to correct patch,
  - Add backwards compatibility "message" symlink to img-ascii-lcd,
  - Connect backlight to fbdev in ht16k33 dot-matrix mode,
  - Set "err = -EINVAL" in switch() case that cannot happen,
  - Use "auxdisplay" instead of DRIVER_NAME in LED name.

This series has been tested using an Adafruit 0.54" Quad Alphanumeric
Red FeatherWing Display, connected to an OrangeCrab ECP5 FPGA board
running a 64 MHz VexRiscv RISC-V softcore.
7-segment display support is based purely on schematics, and has not
been tested on actual hardware.  The changes to img-ascii-lcd.c are also
untested, due to lack of hardware.

Thanks!

[1] "[PATCH v4 00/19] auxdisplay: ht16k33: Add character display support"
    https://lore.kernel.org/r/20210727140459.3767788-1-geert@linux-m68k.org/
[2] "[PATCH v3 00/19] auxdisplay: ht16k33: Add character display support"
    https://lore.kernel.org/r/20210714151130.2531831-1-geert@linux-m68k.org/
[3] "[PATCH v2 00/18] auxdisplay: ht16k33: Add character display support"
    https://lore.kernel.org/r/20210625125902.1162428-1-geert@linux-m68k.org/
[4] "[PATCH 00/17] auxdisplay: ht16k33: Add character display support"
    https://lore.kernel.org/r/20210322144848.1065067-1-geert@linux-m68k.org/

Geert Uytterhoeven (19):
  uapi: Add <linux/map_to_14segment.h>
  dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays
  auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string
  auxdisplay: img-ascii-lcd: Add helper variable dev
  auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit()
  auxdisplay: Extract character line display core support
  auxdisplay: linedisp: Use kmemdup_nul() helper
  auxdisplay: linedisp: Add support for changing scroll rate
  auxdisplay: ht16k33: Connect backlight to fbdev
  auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize()
  auxdisplay: ht16k33: Remove unneeded error check in keypad probe()
  auxdisplay: ht16k33: Convert to simple i2c probe function
  auxdisplay: ht16k33: Add helper variable dev
  auxdisplay: ht16k33: Move delayed work
  auxdisplay: ht16k33: Extract ht16k33_brightness_set()
  auxdisplay: ht16k33: Extract frame buffer probing
  auxdisplay: ht16k33: Add support for segment displays
  dt-bindings: auxdisplay: ht16k33: Document LED subnode
  auxdisplay: ht16k33: Add LED support

 .../bindings/auxdisplay/holtek,ht16k33.yaml   |  31 +-
 drivers/auxdisplay/Kconfig                    |  10 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/ht16k33.c                  | 473 ++++++++++++++----
 drivers/auxdisplay/img-ascii-lcd.c            | 205 ++------
 drivers/auxdisplay/line-display.c             | 261 ++++++++++
 drivers/auxdisplay/line-display.h             |  43 ++
 include/uapi/linux/map_to_14segment.h         | 239 +++++++++
 8 files changed, 996 insertions(+), 267 deletions(-)
 create mode 100644 drivers/auxdisplay/line-display.c
 create mode 100644 drivers/auxdisplay/line-display.h
 create mode 100644 include/uapi/linux/map_to_14segment.h

Comments

Geert Uytterhoeven Aug. 11, 2021, 11:29 a.m. UTC | #1
Hi Marek,

On Wed, Aug 11, 2021 at 12:48 PM Marek Behún <kabel@kernel.org> wrote:
> On Wed, 11 Aug 2021 11:57:59 +0200
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > Instantiate a single LED based on the "led" subnode in DT.
> > This allows the user to control display brightness and blinking (backed
> > by hardware support) through the LED class API and triggers, and exposes
> > the display color.  The LED will be named
> > "auxdisplay:<color>:<function>".
> >
> > When running in dot-matrix mode and if no "led" subnode is found, the
> > driver falls back to the traditional backlight mode, to preserve
> > backwards compatibility.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> Reviewed-by: Marek Behún <kabel@kernel.org>

Thanks!

> BTW, this driver does not need to depend on OF, methinks.
> The few instances of properties reading can be
> easily rewritten to device_* functions (from include/linux/property.h).
> The of_get_child_by_name() can become device_get_named_child_node().
>
> Geert, what do you think?

Sure, that can be done later, when an ACPI user appears?
The dependency on OF was pre-existing, and this series is already
at v5.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Aug. 13, 2021, 12:52 p.m. UTC | #2
Hi Andy,

On Thu, Aug 12, 2021 at 2:33 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wednesday, August 11, 2021, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> On Wed, Aug 11, 2021 at 12:48 PM Marek Behún <kabel@kernel.org> wrote:
>> > On Wed, 11 Aug 2021 11:57:59 +0200
>> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> > > Instantiate a single LED based on the "led" subnode in DT.
>> > > This allows the user to control display brightness and blinking (backed
>> > > by hardware support) through the LED class API and triggers, and exposes
>> > > the display color.  The LED will be named
>> > > "auxdisplay:<color>:<function>".
>> > >
>> > > When running in dot-matrix mode and if no "led" subnode is found, the
>> > > driver falls back to the traditional backlight mode, to preserve
>> > > backwards compatibility.
>> > >
>> > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> >
>> > Reviewed-by: Marek Behún <kabel@kernel.org>
>>
>> Thanks!
>>
>> > BTW, this driver does not need to depend on OF, methinks.
>> > The few instances of properties reading can be
>> > easily rewritten to device_* functions (from include/linux/property.h).
>> > The of_get_child_by_name() can become device_get_named_child_node().
>> >
>> > Geert, what do you think?
>>
>> Sure, that can be done later, when an ACPI user appears?
>
> Actually with PRP0001 approach any of compatible driver may be used onACPI platform. So, what you are saying can be interpreted the way “we don’t care about users on ACPI based platforms”. If it is the case, then it should be told explicitly.

I think you're interpreting too much ;-)
My point is simply:

>> The dependency on OF was pre-existing, and this series is already
>> at v5.

If any OF compatible driver can now be used on ACPI platforms, perhaps
this should be handled at the API level? I.e. the distinction between
OF and device properties should be dropped completely, and all drivers
be converted mechanically in one shot, instead of a gradual ad-hoc
conversion being sneaked in through other series like this one?

Gr{oetje,eeting}s,

                        Geert
Andy Shevchenko Aug. 13, 2021, 1:59 p.m. UTC | #3
On Fri, Aug 13, 2021 at 3:53 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Thu, Aug 12, 2021 at 2:33 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wednesday, August 11, 2021, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >> On Wed, Aug 11, 2021 at 12:48 PM Marek Behún <kabel@kernel.org> wrote:
> >> > On Wed, 11 Aug 2021 11:57:59 +0200

...

> >> Sure, that can be done later, when an ACPI user appears?
> >
> > Actually with PRP0001 approach any of compatible driver may be used onACPI platform. So, what you are saying can be interpreted the way “we don’t care about users on ACPI based platforms”. If it is the case, then it should be told explicitly.
>
> I think you're interpreting too much ;-)
> My point is simply:
>
> >> The dependency on OF was pre-existing, and this series is already
> >> at v5.

Okay, but we can get rid of it. Why not make it more generic at the
same time? Does it make sense?
(I believe this is what Marek is asking initially)

> If any OF compatible driver can now be used on ACPI platforms, perhaps
> this should be handled at the API level? I.e. the distinction between
> OF and device properties should be dropped completely,

And this is done by device_*() / fwnode_*() APIs. And that's what can
be easily done here.

>  and all drivers
> be converted mechanically in one shot, instead of a gradual ad-hoc
> conversion being sneaked in through other series like this one?

Do you realize that you are asking for something impossible?

Moreover, an ad-hoc approach is what we do for plenty of things in the
kernel (WRT new APIs, that don't replace old ones immediately).