[PATCH 0/2] auxdisplay: HD44780 connected to I2C via PCF8574

[PATCH 0/2] auxdisplay: HD44780 connected to I2C via PCF8574

[Ralf Schlatterbeck]

Character LCD driver for the Hitachi HD44780 with I2C connection:
There already is a driver for displays with parallel gpio connections.
Re-uses the high-level API.

Concerning the compatible-string: I'm currently using

"hit,hd44780+nxp,pcf8575"

- "hit,hd44780" is used for the parallel-connected version of the display
- nxp,pcf8575 already exists in drivers/gpio/gpio-pcf857x.c which uses
  the same chip for gpio expansion.

Let me know if this choice of names is fine or should be changed.
For the other device-tree names I'm reusing (some of) the names from the
parallel connected display.

 .../auxdisplay/hit,hd44780+nxp,pcf8575.yaml (new)  |  65 +++++++
 drivers/auxdisplay/Kconfig                         |  14 +-
 drivers/auxdisplay/Makefile                        |   1 +
 drivers/auxdisplay/hd44780-pcf8574.c (new)         | 207 +++++++++++++++++++++
 4 files changed, 286 insertions(+), 1 deletion(-)

-- 
Ralf Schlatterbeck                      Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com

[PATCH 1/2] auxdisplay: HD44780 connected to I2C via PCF8574

[Ralf Schlatterbeck]

Add HD44780 character display connected via I2C I/O expander.
Re-uses the high-level interface of the existing HD44780 driver.

Signed-off-by: Ralf Schlatterbeck <rsc@runtux.com>
---
 drivers/auxdisplay/Kconfig           |  14 +-
 drivers/auxdisplay/Makefile          |   1 +
 drivers/auxdisplay/hd44780-pcf8574.c | 207 +++++++++++++++++++++++++++
 3 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 drivers/auxdisplay/hd44780-pcf8574.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 81757eeded68..1e373783c90e 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -17,7 +17,7 @@ menuconfig AUXDISPLAY
 if AUXDISPLAY
 
 config HD44780
-	tristate "HD44780 Character LCD support"
+	tristate "HD44780 Character LCD support, parallel connection"
 	depends on GPIOLIB || COMPILE_TEST
 	select CHARLCD
 	help
@@ -27,6 +27,18 @@ config HD44780
 	  kernel and started at boot.
 	  If you don't understand what all this is about, say N.
 
+config HD44780_PCF8574
+	tristate "HD44780 Character LCD support, I2C-connection"
+	depends on I2C
+	select CHARLCD
+	help
+	  Enable support for Character LCDs using a HD44780 controller.
+	  The HD44780 is connected to an I2C bus via PCF8574 I/O expancer.
+	  The LCD is accessible through the /dev/lcd char device (10, 156).
+	  This code can either be compiled as a module, or linked into the
+	  kernel and started at boot.
+	  If you don't understand what all this is about, say N.
+
 config KS0108
 	tristate "KS0108 LCD Controller"
 	depends on PARPORT_PC
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index cf54b5efb07e..cf008ce28545 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_KS0108)		+= ks0108.o
 obj-$(CONFIG_CFAG12864B)	+= cfag12864b.o cfag12864bfb.o
 obj-$(CONFIG_IMG_ASCII_LCD)	+= img-ascii-lcd.o
 obj-$(CONFIG_HD44780)		+= hd44780.o
+obj-$(CONFIG_HD44780_PCF8574)	+= hd44780-pcf8574.o
 obj-$(CONFIG_HT16K33)		+= ht16k33.o
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
diff --git a/drivers/auxdisplay/hd44780-pcf8574.c b/drivers/auxdisplay/hd44780-pcf8574.c
new file mode 100644
index 000000000000..ffd9d604928b
--- /dev/null
+++ b/drivers/auxdisplay/hd44780-pcf8574.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * HD44780 Character LCD driver for Linux
+ * With PCF8574 I2C I/O Expander
+ *
+ * Copyright (C) 2020, Ralf Schlatterbeck <rsc@runtux.com>
+ */
+
+#define DEBUG
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/i2c.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+
+/*
+ * The display uses 4-bit mode (the I/O expander has only 8 bits)
+ * The control signals RS, R/W, E are on three bits and on many displays
+ * the backlight is on bit 3. The upper 4 bits are data.
+ */
+#define HD44780_RS_SHIFT	0
+#define HD44780_RW_SHIFT	1
+#define HD44780_E_SHIFT		2
+#define HD44780_BACKLIGHT_SHIFT	3
+
+#include "charlcd.h"
+
+struct hd44780 {
+	const struct i2c_client *i2c_client;
+	int backlight;
+};
+
+static void hd44780_backlight(struct charlcd *lcd, int on)
+{
+	struct hd44780 *hd = lcd->drvdata;
+	u8 b = BIT(HD44780_RW_SHIFT); /* Read-bit */
+
+	hd->backlight = on;
+	/* Note: We do not toggle the E-bit, display will ignore this */
+	b |= hd->backlight ? BIT(HD44780_BACKLIGHT_SHIFT) : 0;
+	(void)i2c_master_send(hd->i2c_client, &b, 1);
+}
+
+static int hd44780_send_nibble(struct hd44780 *hd, u8 outbyte)
+{
+	const struct i2c_client *i2c_client = hd->i2c_client;
+	u8 backlight = hd->backlight ? BIT(HD44780_BACKLIGHT_SHIFT) : 0;
+	u8 b = outbyte;
+	int err;
+	/*
+	 * Transfers first send the output byte with the E-bit 0
+	 * Then the spec says to wait for 20us, then we set the E-bit to 1
+	 * and wait for 40us, then reset the E-bit again.
+	 * A max-speed (400kbit/s) I2C transfer for a single byte will
+	 * already take 25us. So the first delay of 20us can be skipped.
+	 * The second delay becomes 40us - 25us = 15us.
+	 */
+	b = (outbyte & ~BIT(HD44780_E_SHIFT)) | backlight;
+	dev_dbg(&i2c_client->dev, "I2C send: 0x%x", b);
+	err = i2c_master_send(i2c_client, &b, 1);
+	if (err < 0)
+		goto errout;
+	b = (outbyte |  BIT(HD44780_E_SHIFT)) | backlight;
+	dev_dbg(&i2c_client->dev, "I2C send: 0x%x", b);
+	err = i2c_master_send(i2c_client, &b, 1);
+	if (err < 0)
+		goto errout;
+	udelay(15);
+	b = (outbyte & ~BIT(HD44780_E_SHIFT)) | backlight;
+	dev_dbg(&i2c_client->dev, "I2C send: 0x%x", b);
+	err = i2c_master_send(i2c_client, &b, 1);
+	if (err < 0)
+		goto errout;
+	return 0;
+errout:
+	dev_err(&i2c_client->dev, "Error sending to display: %d", err);
+	return err;
+}
+
+static void hd44780_write_cmd_raw_nibble(struct charlcd *lcd, int cmd)
+{
+	struct hd44780 *hd = lcd->drvdata;
+
+	(void)hd44780_send_nibble(hd, (cmd & 0x0F) << 4);
+	udelay(20);
+}
+
+/* Send a command to the LCD panel */
+static void hd44780_write_cmd(struct charlcd *lcd, int cmd)
+{
+	struct hd44780 *hd = lcd->drvdata;
+	int ret;
+
+	ret = hd44780_send_nibble(hd, (cmd & 0xF0));
+	if (ret)
+		return;
+	ret = hd44780_send_nibble(hd, (cmd << 4));
+	if (ret)
+		return;
+	/* The shortest command takes at least 120 us */
+	udelay(120);
+}
+
+/* Send data to the LCD panel */
+static void hd44780_write_data(struct charlcd *lcd, int data)
+{
+	struct hd44780 *hd = lcd->drvdata;
+	int ret;
+
+	ret = hd44780_send_nibble(hd, (data & 0xF0) | BIT(HD44780_RS_SHIFT));
+	if (ret)
+		return;
+	ret = hd44780_send_nibble(hd, (data << 4)   | BIT(HD44780_RS_SHIFT));
+	if (ret)
+		return;
+	/* The shortest data takes at least 45 us */
+	udelay(45);
+}
+
+static const struct charlcd_ops hd44780_ops = {
+	.write_cmd	= hd44780_write_cmd,
+	.write_cmd_raw4 = hd44780_write_cmd_raw_nibble,
+	.write_data	= hd44780_write_data,
+	.backlight      = hd44780_backlight,
+};
+
+static int hd44780_i2c_probe(struct i2c_client *client,
+			     const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct charlcd *lcd;
+	struct hd44780 *hd;
+
+	int ret;
+
+	lcd = charlcd_alloc(sizeof(*hd));
+	if (!lcd)
+		return -ENOMEM;
+
+	hd = lcd->drvdata;
+	hd->i2c_client = client;
+	hd->backlight = 1;
+
+	/* Required properties */
+	ret = device_property_read_u32(dev, "display-height-chars",
+				       &lcd->height);
+	if (ret)
+		goto fail;
+	ret = device_property_read_u32(dev, "display-width-chars", &lcd->width);
+	if (ret)
+		goto fail;
+
+	/*
+	 * On displays with more than two rows, the internal buffer width is
+	 * usually equal to the display width
+	 */
+	if (lcd->height > 2)
+		lcd->bwidth = lcd->width;
+
+	/* Optional properties */
+	device_property_read_u32(dev, "internal-buffer-width", &lcd->bwidth);
+
+	lcd->ifwidth = 4;
+	lcd->ops = &hd44780_ops;
+
+	ret = charlcd_register(lcd);
+	if (ret)
+		goto fail;
+	dev_info(&client->dev, "Found HD44780 display");
+
+	return 0;
+
+fail:
+	charlcd_free(lcd);
+	return ret;
+}
+
+static int hd44780_i2c_remove(struct i2c_client *client)
+{
+	struct charlcd *lcd = i2c_get_clientdata(client);
+
+	charlcd_unregister(lcd);
+
+	charlcd_free(lcd);
+	return 0;
+}
+
+static const struct of_device_id hd44780_of_match[] = {
+	{ .compatible = "hit,hd44780+nxp,pcf8575" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, hd44780_of_match);
+
+static struct i2c_driver hd44780_i2c_driver = {
+	.driver = {
+		.name = "hd44780+pcf8574",
+		.of_match_table = hd44780_of_match,
+	},
+	.probe = hd44780_i2c_probe,
+	.remove = hd44780_i2c_remove,
+};
+module_i2c_driver(hd44780_i2c_driver);
+
+MODULE_DESCRIPTION("HD44780 + PCF8574 I2C Character LCD driver");
+MODULE_AUTHOR("Ralf Schlatterbeck <rsc@runtux.com>");
+MODULE_LICENSE("GPL");
-- 
2.20.1

[PATCH 2/2] auxdisplay: devicetree doc for HD44780/PCF8574

[Ralf Schlatterbeck]

Add devicetree documentation with example.

Signed-off-by: Ralf Schlatterbeck <rsc@runtux.com>
---
 .../auxdisplay/hit,hd44780+nxp,pcf8575.yaml   | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/hit,hd44780+nxp,pcf8575.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/hit,hd44780+nxp,pcf8575.yaml b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780+nxp,pcf8575.yaml
new file mode 100644
index 000000000000..c8e822c43b10
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/hit,hd44780+nxp,pcf8575.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/hit,hd44780+nxp,pcf8575.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hitachi HD44780 Character LCD Controller attached to I2C via PCF8575
+
+maintainers:
+  - Ralf Schlatterbeck <rsc@runtux.com>
+
+description:
+  The Hitachi HD44780 Character LCD Controller is commonly used on character
+  LCDs that can display one or more lines of text. The display itself
+  exposes a parallel connection but they're often used with a PCF8575
+  I/O expander to connect them to an I2C bus.
+
+properties:
+  compatible:
+    const: hit,hd44780+nxp,pcf8575
+
+  reg:
+    description: I2C-bus address
+    maxItems: 1
+
+  display-height-chars:
+    description: Height of the display, in character cells,
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 4
+
+  display-width-chars:
+    description: Width of the display, in character cells.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 64
+
+  internal-buffer-width:
+    description:
+      Internal buffer width (default is 40 for displays with 1 or 2 lines, and
+      display-width-chars for displays with more than 2 lines).
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 64
+
+required:
+  - compatible
+  - display-height-chars
+  - display-width-chars
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c@2000 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      auxdisplay: hd44780@27 {
+        compatible = "hit,hd44780+nxp,pcf8575";
+        reg = <0x27>;
+        display-height-chars = <2>;
+        display-width-chars = <16>;
+      };
+    };
-- 
2.20.1

Re: [PATCH 1/2] auxdisplay: HD44780 connected to I2C via PCF8574

[Miguel Ojeda]

Hi Ralf,

On Tue, Jan 5, 2021 at 4:04 PM Ralf Schlatterbeck <rsc@runtux.com> wrote:
>
> Add HD44780 character display connected via I2C I/O expander.
> Re-uses the high-level interface of the existing HD44780 driver.
>
> Signed-off-by: Ralf Schlatterbeck <rsc@runtux.com>

Thanks for the driver! See a first quick review below.

Also, Cc'ing others related to hd44780/charlcd that may be interested
(there is another patch in the series, too).

> +config HD44780_PCF8574
> +       tristate "HD44780 Character LCD support, I2C-connection"

There is no hyphen in the "parallel connection" one, perhaps remove it?

> +#define DEBUG

Spurious line from development?

> +/*
> + * The display uses 4-bit mode (the I/O expander has only 8 bits)
> + * The control signals RS, R/W, E are on three bits and on many displays
> + * the backlight is on bit 3. The upper 4 bits are data.
> + */
> +#define HD44780_RS_SHIFT       0
> +#define HD44780_RW_SHIFT       1
> +#define HD44780_E_SHIFT                2
> +#define HD44780_BACKLIGHT_SHIFT        3

These are always used in BIT() from a quick look, so perhaps you can
directly define the bits themselves instead.

> +struct hd44780 {
> +       const struct i2c_client *i2c_client;
> +       int backlight;
> +};

Even though the struct is internal to the driver, I'd avoid calling it
the same as the one in hd44780.c. I guess hd44780_pcf8574 would be
best, following the name of the file and the CONFIG_ option.

Same for the #define names, the functions' names, etc.

> +static void hd44780_backlight(struct charlcd *lcd, int on)
> +{
> +       struct hd44780 *hd = lcd->drvdata;
> +       u8 b = BIT(HD44780_RW_SHIFT); /* Read-bit */
> +
> +       hd->backlight = on;

Newline here. In general, please add newlines to try to separate
blocks of related code, similar to paragraphs in text. (I will give
some examples below).

> +       (void)i2c_master_send(hd->i2c_client, &b, 1);

I wonder if we should make charlcd_ops' backlight return an int rather
than void, so that we can properly return the error here (similarly in
lcd2s etc.), even if we ignore it so far in charlcd.c... Thoughts Lars
et. al.?

> +static int hd44780_send_nibble(struct hd44780 *hd, u8 outbyte)
> +{
> +       const struct i2c_client *i2c_client = hd->i2c_client;
> +       u8 backlight = hd->backlight ? BIT(HD44780_BACKLIGHT_SHIFT) : 0;

const?

> +       u8 b = outbyte;
> +       int err;

Newline.

> +       /*
> +        * Transfers first send the output byte with the E-bit 0
> +        * Then the spec says to wait for 20us, then we set the E-bit to 1
> +        * and wait for 40us, then reset the E-bit again.
> +        * A max-speed (400kbit/s) I2C transfer for a single byte will
> +        * already take 25us. So the first delay of 20us can be skipped.
> +        * The second delay becomes 40us - 25us = 15us.
> +        */
> +       b = (outbyte & ~BIT(HD44780_E_SHIFT)) | backlight;
> +       dev_dbg(&i2c_client->dev, "I2C send: 0x%x", b);
> +       err = i2c_master_send(i2c_client, &b, 1);
> +       if (err < 0)
> +               goto errout;

Newline (same for the others).

> +       b = (outbyte |  BIT(HD44780_E_SHIFT)) | backlight;
> +       dev_dbg(&i2c_client->dev, "I2C send: 0x%x", b);
> +       err = i2c_master_send(i2c_client, &b, 1);
> +       if (err < 0)
> +               goto errout;
> +       udelay(15);
> +       b = (outbyte & ~BIT(HD44780_E_SHIFT)) | backlight;
> +       dev_dbg(&i2c_client->dev, "I2C send: 0x%x", b);
> +       err = i2c_master_send(i2c_client, &b, 1);
> +       if (err < 0)
> +               goto errout;
> +       return 0;

Newline.

> +errout:
> +       dev_err(&i2c_client->dev, "Error sending to display: %d", err);
> +       return err;
> +}
> +
> +static void hd44780_write_cmd_raw_nibble(struct charlcd *lcd, int cmd)
> +{
> +       struct hd44780 *hd = lcd->drvdata;
> +
> +       (void)hd44780_send_nibble(hd, (cmd & 0x0F) << 4);

Similarly, should we make hd44780_write_cmd_raw_nibble(),
hd44780_write_data() etc. return the error, even if ignored later on?

> +       ret = hd44780_send_nibble(hd, (cmd & 0xF0));

No parenthesis needed in the argument (I guess you wrote them for
consistency with the other send*()s, but they are far apart).

> +       if (ret)
> +               return;
> +       ret = hd44780_send_nibble(hd, (cmd << 4));

Ditto.

> +       if (ret)
> +               return;

Newline.

> +static int hd44780_i2c_remove(struct i2c_client *client)
> +{
> +       struct charlcd *lcd = i2c_get_clientdata(client);
> +
> +       charlcd_unregister(lcd);

charlcd_unregister() may fail (it doesn't right now in any path, but
it returns an int, so it should be checked and returned if so).

Cheers,
Miguel

Re: [PATCH 2/2] auxdisplay: devicetree doc for HD44780/PCF8574

[Miguel Ojeda]

Hi Ralf,

Something quick I noticed...

On Tue, Jan 5, 2021 at 4:05 PM Ralf Schlatterbeck <rsc@runtux.com> wrote:
>
> +  display-height-chars:
> +    description: Height of the display, in character cells,

Spurious comma instead of period.

Cheers,
Miguel

Re: [PATCH 1/2] auxdisplay: HD44780 connected to I2C via PCF8574

[Geert Uytterhoeven]

Hi Ralf,

On Wed, Jan 6, 2021 at 3:42 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Tue, Jan 5, 2021 at 4:04 PM Ralf Schlatterbeck <rsc@runtux.com> wrote:
> > Add HD44780 character display connected via I2C I/O expander.
> > Re-uses the high-level interface of the existing HD44780 driver.
> >
> > Signed-off-by: Ralf Schlatterbeck <rsc@runtux.com>

Thanks for your patch!

> Also, Cc'ing others related to hd44780/charlcd that may be interested
> (there is another patch in the series, too).

Thanks for CCing me, Miguel!

> > +config HD44780_PCF8574
> > +       tristate "HD44780 Character LCD support, I2C-connection"
>
> There is no hyphen in the "parallel connection" one, perhaps remove it?
>
> > +#define DEBUG
>
> Spurious line from development?
>
> > +/*
> > + * The display uses 4-bit mode (the I/O expander has only 8 bits)
> > + * The control signals RS, R/W, E are on three bits and on many displays
> > + * the backlight is on bit 3. The upper 4 bits are data.
> > + */
> > +#define HD44780_RS_SHIFT       0
> > +#define HD44780_RW_SHIFT       1
> > +#define HD44780_E_SHIFT                2
> > +#define HD44780_BACKLIGHT_SHIFT        3

Unless I'm missing some little detail, there should be no need for this
driver: this wiring (and alternative wirings) can just be expressed in DT.
Examples (using 74HC595 SPI GPIO expanders) can be found in my
renesas-overlays branch:

  - ARM: dts: koelsch: exio-a: Add overlay for MSIOF1 and 2xHC595
    https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays&id=98f65d8ac2ae79d63f13d3f9cc562512670297b7

  - ARM: dts: hc595s: Add overlay for 20x4 character LCD (4 bit wiring)
    https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays&id=181f27534a7437e73e0067062dfa5a18dccd9a74

  - ARM: dts: hc595s: Add overlay for 20x4 character LCD (8 bit wiring)
    https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/renesas-overlays

A separate driver is needed only when using a microcontroller with a
custom protocol to interface with the HD44780.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] auxdisplay: HD44780 connected to I2C via PCF8574

[Ralf Schlatterbeck]

Miguel, Geert, thanks for the quick feedback.

On Wed, Jan 06, 2021 at 09:03:13AM +0100, Geert Uytterhoeven wrote:
> 
> Unless I'm missing some little detail, there should be no need for this
> driver: this wiring (and alternative wirings) can just be expressed in DT.
> Examples (using 74HC595 SPI GPIO expanders) can be found in my
> renesas-overlays branch:

Thanks for the examples.
I've tried this and it really works!
It feels somewhat slower (I'm using user-defined characters) but that
probably doesn't warrant its own driver.

So I'll contribute a documentation change that adds an example of how
to use the I2C-enabled version of the display, this may be non-obvious
to others. I'll start a new thread for this, stay tuned...

@Miguel: Thanks for your detailed feedback on coding style, I'll use
this next time ;-)

Ralf
-- 
Ralf Schlatterbeck                      Tel:   +43/2243/26465-16
Open Source Consulting                  www:   www.runtux.com
Reichergasse 131, A-3411 Weidling       email: office@runtux.com