hd44780-i2c-lcd: new driver and overlay

[agordon]

This small driver creates an LCD device (/dev/lcd)
for HD44780 LCD over I2C. These modules typically use the PCF8574 chip
to convert I2C input to HD44780's SPI protocol.

signed-off-by: Assaf Gordon [email protected]

[agordon]

Dear RPI/Linux devs,

I would like to offer a small new driver+overlay to work with the very common HD44780 LCD module over I2C.
E.g. this module: https://tutorials-raspberrypi.com/control-a-raspberry-pi-hd44780-lcd-display-via-i2c/ .
This patch is against rpi-5.12.y branch (not the current rpi-5.10.y because it doesn't have the refactored hd44780_common.c file).
The driver is a thin layer connecting the hd47800_common driver with the i2c bus using i2c_smbus_write_byte .

As I'm not a kernel developer, I'm certain there will be some needed changes - feedback and suggestions very welcomed.
Thanks!

[pelwell]

The driver looks plausible, and apart from a few minor niggles with the README etc. the overlay looks good. The real problem is that this driver should be submitted upstream - it is not Pi-specific in any way, and we can't take on maintenance of random bits of code like this.

[pelwell]

This section should come before hd44780-lcd.

[pelwell]

This line starts with a hard TAB character - it should be 8 spaces.

[pelwell]

This overlay needs an entry in the Makefile, otherwise it won't be compiled.

[6by9]

checkpatch has a fair few complaints too. Best to fix those before considering submitting upstream.

WARNING: 'Signed-off-by:' is the preferred signature form
#10: 
signed-off-by: Assaf Gordon <[email protected]>

WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#69: 
new file mode 100644

WARNING: Block comments use * on subsequent lines
#96: FILE: drivers/auxdisplay/hd44780-i2c.c:23:
+/*
+  I2C/hd88470 bits in 4-bit GPIO mode:

WARNING: Block comments use * on subsequent lines
#164: FILE: drivers/auxdisplay/hd44780-i2c.c:91:
+/* Write 4bits of data to the HD44780 through i2c bus.
+   Three i2c 'write' commands are issues, toggling the "EN/ENALBLE" bit.

WARNING: Avoid unnecessary line continuations
#184: FILE: drivers/auxdisplay/hd44780-i2c.c:111:
+		pr_err("write_gpio4_nibble, part 1: " \

WARNING: Avoid unnecessary line continuations
#192: FILE: drivers/auxdisplay/hd44780-i2c.c:119:
+		pr_err("write_gpio4_nibble, part 2: " \

WARNING: Avoid unnecessary line continuations
#200: FILE: drivers/auxdisplay/hd44780-i2c.c:127:
+		pr_err("write_gpio4_nibble, part 3: " \

WARNING: Block comments use * on subsequent lines
#210: FILE: drivers/auxdisplay/hd44780-i2c.c:137:
+/* writes an octet (8bit) to the hd44780 through the I2C bus,
+ by splitting the octet to two nibbles and sending them. */

WARNING: Block comments use a trailing */ on a separate line
#210: FILE: drivers/auxdisplay/hd44780-i2c.c:137:
+ by splitting the octet to two nibbles and sending them. */

CHECK: spaces preferred around that '|' (ctx:VxV)
#219: FILE: drivers/auxdisplay/hd44780-i2c.c:146:
+	ret = hd44780_i2c_write_gpio4_nibble(hdc, (data & 0xF0)|rs|bl);
 	                                                       ^

CHECK: spaces preferred around that '|' (ctx:VxV)
#219: FILE: drivers/auxdisplay/hd44780-i2c.c:146:
+	ret = hd44780_i2c_write_gpio4_nibble(hdc, (data & 0xF0)|rs|bl);
 	                                                          ^

CHECK: spaces preferred around that '<<' (ctx:VxV)
#223: FILE: drivers/auxdisplay/hd44780-i2c.c:150:
+	ret = hd44780_i2c_write_gpio4_nibble(hdc, (data & 0x0F)<<4|rs|bl);
 	                                                       ^

CHECK: spaces preferred around that '|' (ctx:VxV)
#223: FILE: drivers/auxdisplay/hd44780-i2c.c:150:
+	ret = hd44780_i2c_write_gpio4_nibble(hdc, (data & 0x0F)<<4|rs|bl);
 	                                                          ^

CHECK: spaces preferred around that '|' (ctx:VxV)
#223: FILE: drivers/auxdisplay/hd44780-i2c.c:150:
+	ret = hd44780_i2c_write_gpio4_nibble(hdc, (data & 0x0F)<<4|rs|bl);
 	                                                             ^

CHECK: Alignment should match open parenthesis
#236: FILE: drivers/auxdisplay/hd44780-i2c.c:163:
+		pr_err("write_cmd_gpio4: failed to send 0x%02x, ret = %d",
+			cmd, ret);

CHECK: Alignment should match open parenthesis
#241: FILE: drivers/auxdisplay/hd44780-i2c.c:168:
+static void hd44780_i2c_write_cmd_raw_gpio4(struct hd44780_common *hdc,
+						int cmd)

CHECK: Alignment should match open parenthesis
#249: FILE: drivers/auxdisplay/hd44780-i2c.c:176:
+		pr_err("write_cmd_raw_gpio4: failed to send 0x%02x, ret = %d",
+			cmd, ret);

WARNING: Missing a blank line after declarations
#258: FILE: drivers/auxdisplay/hd44780-i2c.c:185:
+	int ret = hd44780_i2c_write_gpio4_byte(hdc, data, DATA, BACKLIGHT_ON);
+	if (ret)

CHECK: Alignment should match open parenthesis
#260: FILE: drivers/auxdisplay/hd44780-i2c.c:187:
+		pr_err("write_data_gpio4: failed to send 0x%02x, ret = %d",
+			data, ret);

CHECK: Alignment should match open parenthesis
#264: FILE: drivers/auxdisplay/hd44780-i2c.c:191:
+static int hd44780_i2c_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)

CHECK: Alignment should match open parenthesis
#307: FILE: drivers/auxdisplay/hd44780-i2c.c:234:
+	pr_debug("hd44780-i2c: width: %d, height: %d",
+		lcd->width, lcd->height);

WARNING: DT compatible string "hit,hd44780_i2c" appears un-documented -- check ./Documentation/devicetree/bindings/
#367: FILE: drivers/auxdisplay/hd44780-i2c.c:294:
+		.compatible = "hit,hd44780_i2c",

total: 0 errors, 11 warnings, 11 checks, 354 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH] hd44780-i2c-lcd: new driver and overlay" has style problems, please review.

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

(Note that that was ./scripts/checkpatch.pl --strict --codespell which most of the upstream reviewers will be using).

But also isn't the HD44780 over I2C is actually just an HD44780 with a PCF8574 I2C GPIO chip strapped to the front? Rereading your overlay readme confirms that.
The same result can therefore be achieved using the existing HD44780 driver and the PCF857x driver (CONFIG_GPIO_PCF857X) configured through device tree. It's more I2C traffic as each bit will be changed individually, but I suspect it would be the preferred solution upstream.

[oniongarlic]

Just noticed this and I've been planing on submitting this overlay someday, but just haven't had the time to dig into how to do that properly. As noted above, a separate driver is not needed as you can just plug into into the existing one.

pcf8574-hd44780.dts.txt

[pelwell]

Thanks - that looks like a better solution since the PCF8574 driver exists and is more fully featured.

[pelwell]

@agordon Are you happy to use the alternative overlay suggested above? Rename it to match your overlay and README entry, then drop your driver and the associated Kconfig and Makefile changes.

[agordon]

Hi,

@agordon Are you happy to use the alternative overlay suggested above?

I could not get this overlay to work, but it could be just me.

The hd44780 driver does software big-banging of one-sided SPI protocol using 4 GPIOs, not sure how you overlay this over the pcf8574 driver - but again, I'm far from an expert on this.

I would say, that kernel developers split the old hd44780 driver into two parts - hd44780_common.c (the protocol commands) and hd44780.c (the SPI physical layer) - and they did that exactly so that other physical layers could be added (e.g. hd44780-over-I2C instead SPI, just like me patch above adds).

However, if you are able to make this alternative overlay work - then by all means, I'm happy to use it (perhaps adding some documentation could be useful?).