2024-02-19 17:04:09

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

Add a new initial driver for Maxim MAX6958/6959 chips.
While developing that driver I realised that there is a lot
of duplication between ht16k33 and a new one. Hence set of
cleanups and refactorings.

Note, the new driver has minimum support of the hardware and
I have plans to cover more features in the future.

In v3:
- dropped applied patches
- fixed compilation error (Geert)
- dragging back and force added memory allocation for the character buffer
- dropped 'with keyscan' in new driver (Geert)
- rephrased a bit description in DT bindings for new driver (Geert)
- added tags to the (almost) unchanged patches (Geert, Krzysztof, Robin)

In v2:
- updated DT bindings to follow specifications and requirements (Krzysztof)
- unified return code variable (err everywhere)
- left patches 10 and 13 untouched, we may amend later on (Robin)

Andy Shevchenko (9):
auxdisplay: linedisp: Group display drivers together
auxdisplay: linedisp: Allocate buffer for the string
auxdisplay: ht16k33: Add default to switch-cases
auxdisplay: ht16k33: Move ht16k33_linedisp_ops down
auxdisplay: ht16k33: Define a few helper macros
auxdisplay: ht16k33: Switch to use line display character mapping
auxdisplay: ht16k33: Drop struct ht16k33_seg
dt-bindings: auxdisplay: Add Maxim MAX6958/6959
auxdisplay: Add driver for MAX695x 7-segment LED controllers

.../bindings/auxdisplay/maxim,max6959.yaml | 44 +++
drivers/auxdisplay/Kconfig | 306 ++++++++++--------
drivers/auxdisplay/Makefile | 16 +-
drivers/auxdisplay/ht16k33.c | 177 ++++------
drivers/auxdisplay/img-ascii-lcd.c | 17 +-
drivers/auxdisplay/line-display.c | 11 +-
drivers/auxdisplay/line-display.h | 3 +-
drivers/auxdisplay/max6959.c | 194 +++++++++++
8 files changed, 496 insertions(+), 272 deletions(-)
create mode 100644 Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
create mode 100644 drivers/auxdisplay/max6959.c

--
2.43.0.rc1.1.gbec44491f096



2024-02-19 17:04:21

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 2/9] auxdisplay: linedisp: Allocate buffer for the string

Always allocate a buffer for the currently displayed characters.
It makes the line display API simpler.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/ht16k33.c | 8 +++-----
drivers/auxdisplay/img-ascii-lcd.c | 17 +++++++----------
drivers/auxdisplay/line-display.c | 11 +++++++----
drivers/auxdisplay/line-display.h | 3 +--
4 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 32d3afd29177..19805f39a257 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -92,7 +92,6 @@ struct ht16k33_seg {
struct seg14_conversion_map seg14;
} map;
unsigned int map_size;
- char curr[4];
};

struct ht16k33_priv {
@@ -457,7 +456,7 @@ static void ht16k33_seg7_update(struct work_struct *work)
struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
work.work);
struct ht16k33_seg *seg = &priv->seg;
- char *s = seg->curr;
+ char *s = seg->linedisp.buf;
uint8_t buf[9];

buf[0] = map_to_seg7(&seg->map.seg7, *s++);
@@ -478,7 +477,7 @@ static void ht16k33_seg14_update(struct work_struct *work)
struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
work.work);
struct ht16k33_seg *seg = &priv->seg;
- char *s = seg->curr;
+ char *s = seg->linedisp.buf;
uint8_t buf[8];

put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf);
@@ -700,8 +699,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
if (err)
return err;

- err = linedisp_register(&seg->linedisp, dev, 4, seg->curr,
- &ht16k33_linedisp_ops);
+ err = linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
if (err)
goto err_remove_map_file;

diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index ecfb1c05bf55..925c4cd101e9 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -37,7 +37,6 @@ struct img_ascii_lcd_config {
* @regmap: the regmap through which LCD registers are accessed
* @offset: the offset within regmap to the start of the LCD registers
* @cfg: pointer to the LCD model configuration
- * @curr: the string currently displayed on the LCD
*/
struct img_ascii_lcd_ctx {
struct linedisp linedisp;
@@ -47,7 +46,6 @@ struct img_ascii_lcd_ctx {
};
u32 offset;
const struct img_ascii_lcd_config *cfg;
- char curr[] __aligned(8);
};

/*
@@ -61,12 +59,12 @@ static void boston_update(struct linedisp *linedisp)
ulong val;

#if BITS_PER_LONG == 64
- val = *((u64 *)&ctx->curr[0]);
+ val = *((u64 *)&linedisp->buf[0]);
__raw_writeq(val, ctx->base);
#elif BITS_PER_LONG == 32
- val = *((u32 *)&ctx->curr[0]);
+ val = *((u32 *)&linedisp->buf[0]);
__raw_writel(val, ctx->base);
- val = *((u32 *)&ctx->curr[4]);
+ val = *((u32 *)&linedisp->buf[4]);
__raw_writel(val, ctx->base + 4);
#else
# error Not 32 or 64 bit
@@ -93,7 +91,7 @@ static void malta_update(struct linedisp *linedisp)

for (i = 0; i < linedisp->num_chars; i++) {
err = regmap_write(ctx->regmap,
- ctx->offset + (i * 8), ctx->curr[i]);
+ ctx->offset + (i * 8), linedisp->buf[i]);
if (err)
break;
}
@@ -195,7 +193,7 @@ static void sead3_update(struct linedisp *linedisp)

err = regmap_write(ctx->regmap,
ctx->offset + SEAD3_REG_LCD_DATA,
- ctx->curr[i]);
+ linedisp->buf[i]);
if (err)
break;
}
@@ -236,7 +234,7 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
struct img_ascii_lcd_ctx *ctx;
int err;

- ctx = devm_kzalloc(dev, sizeof(*ctx) + cfg->num_chars, GFP_KERNEL);
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
if (!ctx)
return -ENOMEM;

@@ -253,8 +251,7 @@ static int img_ascii_lcd_probe(struct platform_device *pdev)
return PTR_ERR(ctx->base);
}

- err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, ctx->curr,
- &cfg->ops);
+ err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, &cfg->ops);
if (err)
return err;

diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 13be7c2f6bc3..e2b546210f8d 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -265,6 +265,7 @@ static void linedisp_release(struct device *dev)

kfree(linedisp->map);
kfree(linedisp->message);
+ kfree(linedisp->buf);
ida_free(&linedisp_id, linedisp->id);
}

@@ -316,14 +317,12 @@ static int linedisp_init_map(struct linedisp *linedisp)
* @linedisp: pointer to character line display structure
* @parent: parent device
* @num_chars: the number of characters that can be displayed
- * @buf: pointer to a buffer that can hold @num_chars characters
* @ops: character line display operations
*
* Return: zero on success, else a negative error code.
*/
int linedisp_register(struct linedisp *linedisp, struct device *parent,
- unsigned int num_chars, char *buf,
- const struct linedisp_ops *ops)
+ unsigned int num_chars, const struct linedisp_ops *ops)
{
int err;

@@ -331,7 +330,6 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
linedisp->dev.parent = parent;
linedisp->dev.type = &linedisp_type;
linedisp->ops = ops;
- linedisp->buf = buf;
linedisp->num_chars = num_chars;
linedisp->scroll_rate = DEFAULT_SCROLL_RATE;

@@ -343,6 +341,11 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
device_initialize(&linedisp->dev);
dev_set_name(&linedisp->dev, "linedisp.%u", linedisp->id);

+ err = -ENOMEM;
+ linedisp->buf = kzalloc(linedisp->num_chars, GFP_KERNEL);
+ if (!linedisp->buf)
+ goto out_put_device;
+
/* initialise a character mapping, if required */
err = linedisp_init_map(linedisp);
if (err)
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 4e310b0e611e..4348d7a2f69a 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -82,8 +82,7 @@ struct linedisp {
};

int linedisp_register(struct linedisp *linedisp, struct device *parent,
- unsigned int num_chars, char *buf,
- const struct linedisp_ops *ops);
+ unsigned int num_chars, const struct linedisp_ops *ops);
void linedisp_unregister(struct linedisp *linedisp);

#endif /* LINEDISP_H */
--
2.43.0.rc1.1.gbec44491f096


2024-02-19 17:04:30

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 3/9] auxdisplay: ht16k33: Add default to switch-cases

Currently the compiler (GCC) is able to figure out that there is no
other choices possible than those that are already listed in the
switch-cases. However, if we want to move some code to the callback,
compiler will start complaining that no default is defined. Make
sure we have all switch-cases equiped with default.

Reported-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/ht16k33.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 19805f39a257..c0067a3b2b61 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -677,11 +677,6 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
return err;

switch (priv->type) {
- case DISP_MATRIX:
- /* not handled here */
- err = -EINVAL;
- break;
-
case DISP_QUAD_7SEG:
INIT_DELAYED_WORK(&priv->work, ht16k33_seg7_update);
seg->map.seg7 = initial_map_seg7;
@@ -695,6 +690,9 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
seg->map_size = sizeof(seg->map.seg14);
err = device_create_file(dev, &dev_attr_map_seg14);
break;
+
+ default:
+ return -EINVAL;
}
if (err)
return err;
@@ -772,6 +770,9 @@ static int ht16k33_probe(struct i2c_client *client)
/* Segment Display */
err = ht16k33_seg_probe(dev, priv, dft_brightness);
break;
+
+ default:
+ return -EINVAL;
}
return err;
}
@@ -796,6 +797,9 @@ static void ht16k33_remove(struct i2c_client *client)
device_remove_file(&client->dev, &dev_attr_map_seg7);
device_remove_file(&client->dev, &dev_attr_map_seg14);
break;
+
+ default:
+ break;
}
}

--
2.43.0.rc1.1.gbec44491f096


2024-02-19 17:04:43

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

Add initial driver for the MAX6958 and MAX6959 7-segment LED
controllers.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/Kconfig | 14 +++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/max6959.c | 194 +++++++++++++++++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 drivers/auxdisplay/max6959.c

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index 109ac253d160..bc295ede3c2c 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -59,6 +59,20 @@ config LCD2S
is a simple single color character display. You have to connect it
to an I2C bus.

+config MAX6959
+ tristate "Maxim MAX6958/6959 7-segment LED controller"
+ depends on I2C
+ select REGMAP_I2C
+ select LINEDISP
+ help
+ If you say yes here you get support for the following Maxim chips
+ (I2C 7-segment LED display controller):
+ - MAX6958
+ - MAX6959 (input support)
+
+ This driver can also be built as a module. If so, the module
+ will be called max6959.
+
menuconfig PARPORT_PANEL
tristate "Parallel port LCD/Keypad Panel support"
depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 9197ea34e2d6..9718aedf6ee2 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -15,5 +15,6 @@ obj-$(CONFIG_CFAG12864B) += cfag12864b.o cfag12864bfb.o
obj-$(CONFIG_LINEDISP) += line-display.o
obj-$(CONFIG_IMG_ASCII_LCD) += img-ascii-lcd.o
obj-$(CONFIG_HT16K33) += ht16k33.o
+obj-$(CONFIG_MAX6959) += max6959.o

obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o
diff --git a/drivers/auxdisplay/max6959.c b/drivers/auxdisplay/max6959.c
new file mode 100644
index 000000000000..5519c014bd29
--- /dev/null
+++ b/drivers/auxdisplay/max6959.c
@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MAX6958/6959 7-segment LED display controller
+ * Datasheet:
+ * https://www.analog.com/media/en/technical-documentation/data-sheets/MAX6958-MAX6959.pdf
+ *
+ * Copyright (c) 2024, Intel Corporation.
+ * Author: Andy Shevchenko <[email protected]>
+ */
+#include <linux/array_size.h>
+#include <linux/bitrev.h>
+#include <linux/bits.h>
+#include <linux/container_of.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/workqueue.h>
+
+#include <linux/map_to_7segment.h>
+
+#include "line-display.h"
+
+/* Registers */
+#define REG_DECODE_MODE 0x01
+#define REG_INTENSITY 0x02
+#define REG_SCAN_LIMIT 0x03
+#define REG_CONFIGURATION 0x04
+#define REG_CONFIGURATION_S_BIT BIT(0)
+
+#define REG_DIGIT(x) (0x20 + (x))
+#define REG_DIGIT0 0x20
+#define REG_DIGIT1 0x21
+#define REG_DIGIT2 0x22
+#define REG_DIGIT3 0x23
+
+#define REG_SEGMENTS 0x24
+#define REG_MAX REG_SEGMENTS
+
+struct max6959_priv {
+ struct linedisp linedisp;
+ struct delayed_work work;
+ struct regmap *regmap;
+};
+
+static void max6959_disp_update(struct work_struct *work)
+{
+ struct max6959_priv *priv = container_of(work, struct max6959_priv, work.work);
+ struct linedisp *linedisp = &priv->linedisp;
+ struct linedisp_map *map = linedisp->map;
+ char *s = linedisp->buf;
+ u8 buf[4];
+
+ /* Map segments according to datasheet */
+ buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+ buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+ buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+ buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
+
+ regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));
+}
+
+static int max6959_linedisp_get_map_type(struct linedisp *linedisp)
+{
+ struct max6959_priv *priv = container_of(linedisp, struct max6959_priv, linedisp);
+
+ INIT_DELAYED_WORK(&priv->work, max6959_disp_update);
+ return LINEDISP_MAP_SEG7;
+}
+
+static void max6959_linedisp_update(struct linedisp *linedisp)
+{
+ struct max6959_priv *priv = container_of(linedisp, struct max6959_priv, linedisp);
+
+ schedule_delayed_work(&priv->work, 0);
+}
+
+static const struct linedisp_ops max6959_linedisp_ops = {
+ .get_map_type = max6959_linedisp_get_map_type,
+ .update = max6959_linedisp_update,
+};
+
+static int max6959_enable(struct max6959_priv *priv, bool enable)
+{
+ u8 mask = REG_CONFIGURATION_S_BIT;
+ u8 value = enable ? mask : 0;
+
+ return regmap_update_bits(priv->regmap, REG_CONFIGURATION, mask, value);
+}
+
+static void max6959_power_off(void *priv)
+{
+ max6959_enable(priv, false);
+}
+
+static int max6959_power_on(struct max6959_priv *priv)
+{
+ struct device *dev = regmap_get_device(priv->regmap);
+ int ret;
+
+ ret = max6959_enable(priv, true);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(dev, max6959_power_off, priv);
+}
+
+static const struct regmap_config max6959_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = REG_MAX,
+ .cache_type = REGCACHE_MAPLE,
+};
+
+static int max6959_i2c_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct max6959_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->regmap = devm_regmap_init_i2c(client, &max6959_regmap_config);
+ if (IS_ERR(priv->regmap))
+ return PTR_ERR(priv->regmap);
+
+ ret = max6959_power_on(priv);
+ if (ret)
+ return ret;
+
+ ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);
+ if (ret)
+ return ret;
+
+ i2c_set_clientdata(client, priv);
+
+ return 0;
+}
+
+static void max6959_i2c_remove(struct i2c_client *client)
+{
+ struct max6959_priv *priv = i2c_get_clientdata(client);
+
+ cancel_delayed_work_sync(&priv->work);
+ linedisp_unregister(&priv->linedisp);
+}
+
+static int max6959_suspend(struct device *dev)
+{
+ return max6959_enable(dev_get_drvdata(dev), false);
+}
+
+static int max6959_resume(struct device *dev)
+{
+ return max6959_enable(dev_get_drvdata(dev), true);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(max6959_pm_ops, max6959_suspend, max6959_resume);
+
+static const struct i2c_device_id max6959_i2c_id[] = {
+ { "max6959" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, max6959_i2c_id);
+
+static const struct of_device_id max6959_of_table[] = {
+ { .compatible = "maxim,max6959" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, max6959_of_table);
+
+static struct i2c_driver max6959_i2c_driver = {
+ .driver = {
+ .name = "max6959",
+ .pm = pm_sleep_ptr(&max6959_pm_ops),
+ .of_match_table = max6959_of_table,
+ },
+ .probe = max6959_i2c_probe,
+ .remove = max6959_i2c_remove,
+ .id_table = max6959_i2c_id,
+};
+module_i2c_driver(max6959_i2c_driver);
+
+MODULE_DESCRIPTION("MAX6958/6959 7-segment LED controller");
+MODULE_AUTHOR("Andy Shevchenko <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
--
2.43.0.rc1.1.gbec44491f096


2024-02-19 17:05:16

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

For better usability group the display drivers together in Kconfig
and Makefile. With this we will have the following sections:
- Character LCD
- Samsung KS0108 LCD controller
- Single character line display
- Character LCD with non-conforming interface

While at it, drop redundant 'default n' entries.

Tested-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/Kconfig | 296 +++++++++++++++++++-----------------
drivers/auxdisplay/Makefile | 15 +-
2 files changed, 163 insertions(+), 148 deletions(-)

diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d944d5298eca..109ac253d160 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -16,6 +16,9 @@ menuconfig AUXDISPLAY

if AUXDISPLAY

+#
+# Character LCD section
+#
config CHARLCD
tristate "Character LCD core support" if COMPILE_TEST
help
@@ -25,12 +28,6 @@ config CHARLCD
This is some character LCD core interface that multiple drivers can
use.

-config LINEDISP
- tristate "Character line display core support" if COMPILE_TEST
- help
- This is the core support for single-line character displays, to be
- selected by drivers that use it.
-
config HD44780_COMMON
tristate "Common functions for HD44780 (and compatibles) LCD displays" if COMPILE_TEST
select CHARLCD
@@ -52,131 +49,6 @@ config HD44780
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
- default n
- help
- If you have a LCD controlled by one or more KS0108
- controllers, say Y. You will need also another more specific
- driver for your LCD.
-
- Depends on Parallel Port support. If you say Y at
- parport, you will be able to compile this as a module (M)
- and built-in as well (Y).
-
- To compile this as a module, choose M here:
- the module will be called ks0108.
-
- If unsure, say N.
-
-config KS0108_PORT
- hex "Parallel port where the LCD is connected"
- depends on KS0108
- default 0x378
- help
- The address of the parallel port where the LCD is connected.
-
- The first standard parallel port address is 0x378.
- The second standard parallel port address is 0x278.
- The third standard parallel port address is 0x3BC.
-
- You can specify a different address if you need.
-
- If you don't know what I'm talking about, load the parport module,
- and execute "dmesg" or "cat /proc/ioports". You can see there how
- many parallel ports are present and which address each one has.
-
- Usually you only need to use 0x378.
-
- If you compile this as a module, you can still override this
- using the module parameters.
-
-config KS0108_DELAY
- int "Delay between each control writing (microseconds)"
- depends on KS0108
- default "2"
- help
- Amount of time the ks0108 should wait between each control write
- to the parallel port.
-
- If your LCD seems to miss random writings, increment this.
-
- If you don't know what I'm talking about, ignore it.
-
- If you compile this as a module, you can still override this
- value using the module parameters.
-
-config CFAG12864B
- tristate "CFAG12864B LCD"
- depends on X86
- depends on FB
- depends on KS0108
- select FB_SYSMEM_HELPERS
- default n
- help
- If you have a Crystalfontz 128x64 2-color LCD, cfag12864b Series,
- say Y. You also need the ks0108 LCD Controller driver.
-
- For help about how to wire your LCD to the parallel port,
- check Documentation/admin-guide/auxdisplay/cfag12864b.rst
-
- Depends on the x86 arch and the framebuffer support.
-
- The LCD framebuffer driver can be attached to a console.
- It will work fine. However, you can't attach it to the fbdev driver
- of the xorg server.
-
- To compile this as a module, choose M here:
- the modules will be called cfag12864b and cfag12864bfb.
-
- If unsure, say N.
-
-config CFAG12864B_RATE
- int "Refresh rate (hertz)"
- depends on CFAG12864B
- default "20"
- help
- Refresh rate of the LCD.
-
- As the LCD is not memory mapped, the driver has to make the work by
- software. This means you should be careful setting this value higher.
- If your CPUs are really slow or you feel the system is slowed down,
- decrease the value.
-
- Be careful modifying this value to a very high value:
- You can freeze the computer, or the LCD maybe can't draw as fast as you
- are requesting.
-
- If you don't know what I'm talking about, ignore it.
-
- If you compile this as a module, you can still override this
- value using the module parameters.
-
-config IMG_ASCII_LCD
- tristate "Imagination Technologies ASCII LCD Display"
- depends on HAS_IOMEM
- default y if MIPS_MALTA
- select MFD_SYSCON
- select LINEDISP
- help
- Enable this to support the simple ASCII LCD displays found on
- development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
- from Imagination Technologies.
-
-config HT16K33
- tristate "Holtek Ht16K33 LED controller with keyscan"
- depends on FB && I2C && INPUT
- select FB_SYSMEM_HELPERS
- select INPUT_MATRIXKMAP
- select FB_BACKLIGHT
- select NEW_LEDS
- select LEDS_CLASS
- select LINEDISP
- help
- Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
- LED controller driver with keyscan.
-
config LCD2S
tristate "lcd2s 20x4 character display over I2C console"
depends on I2C
@@ -187,16 +59,6 @@ config LCD2S
is a simple single color character display. You have to connect it
to an I2C bus.

-config ARM_CHARLCD
- bool "ARM Ltd. Character LCD Driver"
- depends on PLAT_VERSATILE
- help
- This is a driver for the character LCD found on the ARM Ltd.
- Versatile and RealView Platform Baseboards. It doesn't do
- very much more than display the text "ARM Linux" on the first
- line and the Linux version on the second line, but that's
- still useful.
-
menuconfig PARPORT_PANEL
tristate "Parallel port LCD/Keypad Panel support"
depends on PARPORT
@@ -455,7 +317,6 @@ endif # PARPORT_PANEL
config PANEL_CHANGE_MESSAGE
bool "Change LCD initialization message ?"
depends on CHARLCD
- default "n"
help
This allows you to replace the boot message indicating the kernel version
and the driver version with a custom message. This is useful on appliances
@@ -504,8 +365,159 @@ choice

endchoice

+#
+# Samsung KS0108 LCD controller section
+#
+config KS0108
+ tristate "KS0108 LCD Controller"
+ depends on PARPORT_PC
+ help
+ If you have a LCD controlled by one or more KS0108
+ controllers, say Y. You will need also another more specific
+ driver for your LCD.
+
+ Depends on Parallel Port support. If you say Y at
+ parport, you will be able to compile this as a module (M)
+ and built-in as well (Y).
+
+ To compile this as a module, choose M here:
+ the module will be called ks0108.
+
+ If unsure, say N.
+
+config KS0108_PORT
+ hex "Parallel port where the LCD is connected"
+ depends on KS0108
+ default 0x378
+ help
+ The address of the parallel port where the LCD is connected.
+
+ The first standard parallel port address is 0x378.
+ The second standard parallel port address is 0x278.
+ The third standard parallel port address is 0x3BC.
+
+ You can specify a different address if you need.
+
+ If you don't know what I'm talking about, load the parport module,
+ and execute "dmesg" or "cat /proc/ioports". You can see there how
+ many parallel ports are present and which address each one has.
+
+ Usually you only need to use 0x378.
+
+ If you compile this as a module, you can still override this
+ using the module parameters.
+
+config KS0108_DELAY
+ int "Delay between each control writing (microseconds)"
+ depends on KS0108
+ default "2"
+ help
+ Amount of time the ks0108 should wait between each control write
+ to the parallel port.
+
+ If your LCD seems to miss random writings, increment this.
+
+ If you don't know what I'm talking about, ignore it.
+
+ If you compile this as a module, you can still override this
+ value using the module parameters.
+
+config CFAG12864B
+ tristate "CFAG12864B LCD"
+ depends on X86
+ depends on FB
+ depends on KS0108
+ select FB_SYSMEM_HELPERS
+ help
+ If you have a Crystalfontz 128x64 2-color LCD, cfag12864b Series,
+ say Y. You also need the ks0108 LCD Controller driver.
+
+ For help about how to wire your LCD to the parallel port,
+ check Documentation/admin-guide/auxdisplay/cfag12864b.rst
+
+ Depends on the x86 arch and the framebuffer support.
+
+ The LCD framebuffer driver can be attached to a console.
+ It will work fine. However, you can't attach it to the fbdev driver
+ of the xorg server.
+
+ To compile this as a module, choose M here:
+ the modules will be called cfag12864b and cfag12864bfb.
+
+ If unsure, say N.
+
+config CFAG12864B_RATE
+ int "Refresh rate (hertz)"
+ depends on CFAG12864B
+ default "20"
+ help
+ Refresh rate of the LCD.
+
+ As the LCD is not memory mapped, the driver has to make the work by
+ software. This means you should be careful setting this value higher.
+ If your CPUs are really slow or you feel the system is slowed down,
+ decrease the value.
+
+ Be careful modifying this value to a very high value:
+ You can freeze the computer, or the LCD maybe can't draw as fast as you
+ are requesting.
+
+ If you don't know what I'm talking about, ignore it.
+
+ If you compile this as a module, you can still override this
+ value using the module parameters.
+
+#
+# Single character line display section
+#
+config LINEDISP
+ tristate "Character line display core support" if COMPILE_TEST
+ help
+ This is the core support for single-line character displays, to be
+ selected by drivers that use it.
+
+config IMG_ASCII_LCD
+ tristate "Imagination Technologies ASCII LCD Display"
+ depends on HAS_IOMEM
+ default y if MIPS_MALTA
+ select MFD_SYSCON
+ select LINEDISP
+ help
+ Enable this to support the simple ASCII LCD displays found on
+ development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
+ from Imagination Technologies.
+
+config HT16K33
+ tristate "Holtek Ht16K33 LED controller with keyscan"
+ depends on FB && I2C && INPUT
+ select FB_SYSMEM_HELPERS
+ select INPUT_MATRIXKMAP
+ select FB_BACKLIGHT
+ select NEW_LEDS
+ select LEDS_CLASS
+ select LINEDISP
+ help
+ Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
+ LED controller driver with keyscan.
+
+#
+# Character LCD with non-conforming interface section
+#
+config ARM_CHARLCD
+ bool "ARM Ltd. Character LCD Driver"
+ depends on PLAT_VERSATILE
+ help
+ This is a driver for the character LCD found on the ARM Ltd.
+ Versatile and RealView Platform Baseboards. It doesn't do
+ very much more than display the text "ARM Linux" on the first
+ line and the Linux version on the second line, but that's
+ still useful.
+
endif # AUXDISPLAY

+#
+# Deprecated options
+#
config PANEL
tristate "Parallel port LCD/Keypad Panel support (OLD OPTION)"
depends on PARPORT
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..9197ea34e2d6 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -5,12 +5,15 @@

obj-$(CONFIG_CHARLCD) += charlcd.o
obj-$(CONFIG_HD44780_COMMON) += hd44780_common.o
-obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o
+obj-$(CONFIG_HD44780) += hd44780.o
+obj-$(CONFIG_LCD2S) += lcd2s.o
+obj-$(CONFIG_PARPORT_PANEL) += panel.o
+
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_HT16K33) += ht16k33.o
-obj-$(CONFIG_PARPORT_PANEL) += panel.o
-obj-$(CONFIG_LCD2S) += lcd2s.o
+
obj-$(CONFIG_LINEDISP) += line-display.o
+obj-$(CONFIG_IMG_ASCII_LCD) += img-ascii-lcd.o
+obj-$(CONFIG_HT16K33) += ht16k33.o
+
+obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o
--
2.43.0.rc1.1.gbec44491f096


2024-02-19 18:24:53

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 7/9] auxdisplay: ht16k33: Drop struct ht16k33_seg

The struct ht16k33_seg is repeating struct linedisp. Use the latter
directly.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/ht16k33.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 41a961342dc3..96acfb2b58cd 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -86,10 +86,6 @@ struct ht16k33_fbdev {
uint8_t *cache;
};

-struct ht16k33_seg {
- struct linedisp linedisp;
-};
-
struct ht16k33_priv {
struct i2c_client *client;
struct delayed_work work;
@@ -97,7 +93,7 @@ struct ht16k33_priv {
struct ht16k33_keypad keypad;
union {
struct ht16k33_fbdev fbdev;
- struct ht16k33_seg seg;
+ struct linedisp linedisp;
};
enum display_type type;
uint8_t blink;
@@ -110,7 +106,7 @@ struct ht16k33_priv {
container_of(p, struct ht16k33_priv, led)

#define ht16k33_linedisp_to_priv(p) \
- container_of(p, struct ht16k33_priv, seg.linedisp)
+ container_of(p, struct ht16k33_priv, linedisp)

static const struct fb_fix_screeninfo ht16k33_fb_fix = {
.id = DRIVER_NAME,
@@ -417,9 +413,8 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
static void ht16k33_seg7_update(struct work_struct *work)
{
struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
- struct ht16k33_seg *seg = &priv->seg;
- struct linedisp_map *map = seg->linedisp.map;
- char *s = seg->linedisp.buf;
+ struct linedisp_map *map = priv->linedisp.map;
+ char *s = priv->linedisp.buf;
uint8_t buf[9];

buf[0] = map_to_seg7(&map->map.seg7, *s++);
@@ -438,9 +433,8 @@ static void ht16k33_seg7_update(struct work_struct *work)
static void ht16k33_seg14_update(struct work_struct *work)
{
struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
- struct ht16k33_seg *seg = &priv->seg;
- struct linedisp_map *map = seg->linedisp.map;
- char *s = seg->linedisp.buf;
+ struct linedisp_map *map = priv->linedisp.map;
+ char *s = priv->linedisp.buf;
uint8_t buf[8];

put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 0);
@@ -662,14 +656,14 @@ static int ht16k33_fbdev_probe(struct device *dev, struct ht16k33_priv *priv,
static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
uint32_t brightness)
{
- struct ht16k33_seg *seg = &priv->seg;
+ struct linedisp *linedisp = &priv->linedisp;
int err;

err = ht16k33_brightness_set(priv, brightness);
if (err)
return err;

- return linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
+ return linedisp_register(linedisp, dev, 4, &ht16k33_linedisp_ops);
}

static int ht16k33_probe(struct i2c_client *client)
@@ -756,7 +750,7 @@ static void ht16k33_remove(struct i2c_client *client)

case DISP_QUAD_7SEG:
case DISP_QUAD_14SEG:
- linedisp_unregister(&priv->seg.linedisp);
+ linedisp_unregister(&priv->linedisp);
break;

default:
--
2.43.0.rc1.1.gbec44491f096


2024-02-19 20:05:09

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 6/9] auxdisplay: ht16k33: Switch to use line display character mapping

Since line display library supports necessary bits to map the characters
(if required), switch this driver to use that.

Reviewed-by: Geert Uytterhoeven <[email protected]>
Acked-by: Robin van der Gracht <[email protected]>
Tested-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/ht16k33.c | 103 ++++++++++-------------------------
1 file changed, 30 insertions(+), 73 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index b76c4d83528f..41a961342dc3 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -88,11 +88,6 @@ struct ht16k33_fbdev {

struct ht16k33_seg {
struct linedisp linedisp;
- union {
- struct seg7_conversion_map seg7;
- struct seg14_conversion_map seg14;
- } map;
- unsigned int map_size;
};

struct ht16k33_priv {
@@ -144,33 +139,6 @@ static const struct fb_var_screeninfo ht16k33_fb_var = {
.vmode = FB_VMODE_NONINTERLACED,
};

-static const SEG7_DEFAULT_MAP(initial_map_seg7);
-static const SEG14_DEFAULT_MAP(initial_map_seg14);
-
-static ssize_t map_seg_show(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- struct ht16k33_priv *priv = dev_get_drvdata(dev);
-
- memcpy(buf, &priv->seg.map, priv->seg.map_size);
- return priv->seg.map_size;
-}
-
-static ssize_t map_seg_store(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t cnt)
-{
- struct ht16k33_priv *priv = dev_get_drvdata(dev);
-
- if (cnt != priv->seg.map_size)
- return -EINVAL;
-
- memcpy(&priv->seg.map, buf, cnt);
- return cnt;
-}
-
-static DEVICE_ATTR(map_seg7, 0644, map_seg_show, map_seg_store);
-static DEVICE_ATTR(map_seg14, 0644, map_seg_show, map_seg_store);
-
static int ht16k33_display_on(struct ht16k33_priv *priv)
{
uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | priv->blink;
@@ -450,18 +418,19 @@ static void ht16k33_seg7_update(struct work_struct *work)
{
struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
struct ht16k33_seg *seg = &priv->seg;
+ struct linedisp_map *map = seg->linedisp.map;
char *s = seg->linedisp.buf;
uint8_t buf[9];

- buf[0] = map_to_seg7(&seg->map.seg7, *s++);
+ buf[0] = map_to_seg7(&map->map.seg7, *s++);
buf[1] = 0;
- buf[2] = map_to_seg7(&seg->map.seg7, *s++);
+ buf[2] = map_to_seg7(&map->map.seg7, *s++);
buf[3] = 0;
buf[4] = 0;
buf[5] = 0;
- buf[6] = map_to_seg7(&seg->map.seg7, *s++);
+ buf[6] = map_to_seg7(&map->map.seg7, *s++);
buf[7] = 0;
- buf[8] = map_to_seg7(&seg->map.seg7, *s++);
+ buf[8] = map_to_seg7(&map->map.seg7, *s++);

i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
}
@@ -470,17 +439,36 @@ static void ht16k33_seg14_update(struct work_struct *work)
{
struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
struct ht16k33_seg *seg = &priv->seg;
+ struct linedisp_map *map = seg->linedisp.map;
char *s = seg->linedisp.buf;
uint8_t buf[8];

- put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf);
- put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf + 2);
- put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf + 4);
- put_unaligned_le16(map_to_seg14(&seg->map.seg14, *s++), buf + 6);
+ put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 0);
+ put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 2);
+ put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 4);
+ put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 6);

i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
}

+static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
+{
+ struct ht16k33_priv *priv = ht16k33_linedisp_to_priv(linedisp);
+
+ switch (priv->type) {
+ case DISP_QUAD_7SEG:
+ INIT_DELAYED_WORK(&priv->work, ht16k33_seg7_update);
+ return LINEDISP_MAP_SEG7;
+
+ case DISP_QUAD_14SEG:
+ INIT_DELAYED_WORK(&priv->work, ht16k33_seg14_update);
+ return LINEDISP_MAP_SEG14;
+
+ default:
+ return -EINVAL;
+ }
+}
+
static void ht16k33_linedisp_update(struct linedisp *linedisp)
{
struct ht16k33_priv *priv = ht16k33_linedisp_to_priv(linedisp);
@@ -489,6 +477,7 @@ static void ht16k33_linedisp_update(struct linedisp *linedisp)
}

static const struct linedisp_ops ht16k33_linedisp_ops = {
+ .get_map_type = ht16k33_linedisp_get_map_type,
.update = ht16k33_linedisp_update,
};

@@ -680,37 +669,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
if (err)
return err;

- switch (priv->type) {
- case DISP_QUAD_7SEG:
- INIT_DELAYED_WORK(&priv->work, ht16k33_seg7_update);
- seg->map.seg7 = initial_map_seg7;
- seg->map_size = sizeof(seg->map.seg7);
- err = device_create_file(dev, &dev_attr_map_seg7);
- break;
-
- case DISP_QUAD_14SEG:
- INIT_DELAYED_WORK(&priv->work, ht16k33_seg14_update);
- seg->map.seg14 = initial_map_seg14;
- seg->map_size = sizeof(seg->map.seg14);
- err = device_create_file(dev, &dev_attr_map_seg14);
- break;
-
- default:
- return -EINVAL;
- }
- if (err)
- return err;
-
- err = linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
- if (err)
- goto err_remove_map_file;
-
- return 0;
-
-err_remove_map_file:
- device_remove_file(dev, &dev_attr_map_seg7);
- device_remove_file(dev, &dev_attr_map_seg14);
- return err;
+ return linedisp_register(&seg->linedisp, dev, 4, &ht16k33_linedisp_ops);
}

static int ht16k33_probe(struct i2c_client *client)
@@ -798,8 +757,6 @@ static void ht16k33_remove(struct i2c_client *client)
case DISP_QUAD_7SEG:
case DISP_QUAD_14SEG:
linedisp_unregister(&priv->seg.linedisp);
- device_remove_file(&client->dev, &dev_attr_map_seg7);
- device_remove_file(&client->dev, &dev_attr_map_seg14);
break;

default:
--
2.43.0.rc1.1.gbec44491f096


2024-02-19 20:10:34

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 5/9] auxdisplay: ht16k33: Define a few helper macros

Define a few helper macros — wrappers on contaoner_of)() — for easier
maintenance in the future. While at it, include missing container_of.h.

Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/auxdisplay/ht16k33.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index f016130835b1..b76c4d83528f 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -15,6 +15,7 @@
#include <linux/property.h>
#include <linux/fb.h>
#include <linux/backlight.h>
+#include <linux/container_of.h>
#include <linux/input.h>
#include <linux/input/matrix_keypad.h>
#include <linux/leds.h>
@@ -107,6 +108,15 @@ struct ht16k33_priv {
uint8_t blink;
};

+#define ht16k33_work_to_priv(p) \
+ container_of(p, struct ht16k33_priv, work.work)
+
+#define ht16k33_led_to_priv(p) \
+ container_of(p, struct ht16k33_priv, led)
+
+#define ht16k33_linedisp_to_priv(p) \
+ container_of(p, struct ht16k33_priv, seg.linedisp)
+
static const struct fb_fix_screeninfo ht16k33_fb_fix = {
.id = DRIVER_NAME,
.type = FB_TYPE_PACKED_PIXELS,
@@ -194,8 +204,7 @@ static int ht16k33_brightness_set(struct ht16k33_priv *priv,
static int ht16k33_brightness_set_blocking(struct led_classdev *led_cdev,
enum led_brightness brightness)
{
- struct ht16k33_priv *priv = container_of(led_cdev, struct ht16k33_priv,
- led);
+ struct ht16k33_priv *priv = ht16k33_led_to_priv(led_cdev);

return ht16k33_brightness_set(priv, brightness);
}
@@ -203,8 +212,7 @@ static int ht16k33_brightness_set_blocking(struct led_classdev *led_cdev,
static int ht16k33_blink_set(struct led_classdev *led_cdev,
unsigned long *delay_on, unsigned long *delay_off)
{
- struct ht16k33_priv *priv = container_of(led_cdev, struct ht16k33_priv,
- led);
+ struct ht16k33_priv *priv = ht16k33_led_to_priv(led_cdev);
unsigned int delay;
uint8_t blink;
int err;
@@ -246,8 +254,7 @@ static void ht16k33_fb_queue(struct ht16k33_priv *priv)
*/
static void ht16k33_fb_update(struct work_struct *work)
{
- struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
- work.work);
+ struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
struct ht16k33_fbdev *fbdev = &priv->fbdev;

uint8_t *p1, *p2;
@@ -441,8 +448,7 @@ static void ht16k33_keypad_stop(struct input_dev *dev)

static void ht16k33_seg7_update(struct work_struct *work)
{
- struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
- work.work);
+ struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
struct ht16k33_seg *seg = &priv->seg;
char *s = seg->linedisp.buf;
uint8_t buf[9];
@@ -462,8 +468,7 @@ static void ht16k33_seg7_update(struct work_struct *work)

static void ht16k33_seg14_update(struct work_struct *work)
{
- struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
- work.work);
+ struct ht16k33_priv *priv = ht16k33_work_to_priv(work);
struct ht16k33_seg *seg = &priv->seg;
char *s = seg->linedisp.buf;
uint8_t buf[8];
@@ -478,8 +483,7 @@ static void ht16k33_seg14_update(struct work_struct *work)

static void ht16k33_linedisp_update(struct linedisp *linedisp)
{
- struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
- seg.linedisp);
+ struct ht16k33_priv *priv = ht16k33_linedisp_to_priv(linedisp);

schedule_delayed_work(&priv->work, 0);
}
--
2.43.0.rc1.1.gbec44491f096


2024-02-19 20:11:59

by Andy Shevchenko

[permalink] [raw]
Subject: [PATCH v3 8/9] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

Add initial device tree documentation for Maxim MAX6958/6959.
As per reviewer's request mention the fact of absence reset and
power enable pins, since the hardware is quite simple.

Signed-off-by: Andy Shevchenko <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
.../bindings/auxdisplay/maxim,max6959.yaml | 44 +++++++++++++++++++
1 file changed, 44 insertions(+)
create mode 100644 Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
new file mode 100644
index 000000000000..6c78bb185348
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/maxim,max6959.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MAX6958/6959 7-segment LED display controller
+
+maintainers:
+ - Andy Shevchenko <[email protected]>
+
+description:
+ The Maxim MAX6958/6959 7-segment LED display controller provides
+ an I2C interface to up to four 7-segment LED digits. The MAX6959,
+ in comparison to MAX6958, adds input support. Type of the chip can
+ be autodetected via specific register read, and hence the features
+ may be enabled in the driver at run-time, in case they are requested
+ via Device Tree. A given hardware is simple and does not provide
+ any additional pins, such as reset or enable.
+
+properties:
+ compatible:
+ const: maxim,max6959
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ display-controller@38 {
+ compatible = "maxim,max6959";
+ reg = <0x38>;
+ };
+ };
--
2.43.0.rc1.1.gbec44491f096


2024-02-20 18:05:41

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 5/9] auxdisplay: ht16k33: Define a few helper macros

On Mon, Feb 19, 2024 at 06:58:04PM +0200, Andy Shevchenko wrote:
> Define a few helper macros — wrappers on contaoner_of)() — for easier

I have fixed 'container_of()' above locally.

> maintenance in the future. While at it, include missing container_of.h.

--
With Best Regards,
Andy Shevchenko



2024-02-22 13:51:53

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> Add a new initial driver for Maxim MAX6958/6959 chips.
> While developing that driver I realised that there is a lot
> of duplication between ht16k33 and a new one. Hence set of
> cleanups and refactorings.
>
> Note, the new driver has minimum support of the hardware and
> I have plans to cover more features in the future.

Geert, would it be possible to give one more round of reviewing/testing
this week? I want to close auxdisplay for next merge window next week.

--
With Best Regards,
Andy Shevchenko



2024-02-22 13:57:00

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

Hi Andy,

On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > Add a new initial driver for Maxim MAX6958/6959 chips.
> > While developing that driver I realised that there is a lot
> > of duplication between ht16k33 and a new one. Hence set of
> > cleanups and refactorings.
> >
> > Note, the new driver has minimum support of the hardware and
> > I have plans to cover more features in the future.
>
> Geert, would it be possible to give one more round of reviewing/testing
> this week? I want to close auxdisplay for next merge window next week.

For 1-7 (linedisp and ht16k33):
Tested-by: Geert Uytterhoeven <[email protected]>

I hope to get to the actual review later...

Gr{oetje,eeting}s,

Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 14:33:14

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > While developing that driver I realised that there is a lot
> > > of duplication between ht16k33 and a new one. Hence set of
> > > cleanups and refactorings.
> > >
> > > Note, the new driver has minimum support of the hardware and
> > > I have plans to cover more features in the future.
> >
> > Geert, would it be possible to give one more round of reviewing/testing
> > this week? I want to close auxdisplay for next merge window next week.
>
> For 1-7 (linedisp and ht16k33):
> Tested-by: Geert Uytterhoeven <[email protected]>

Thank you!

> I hope to get to the actual review later...

Hope you will find time soon...

--
With Best Regards,
Andy Shevchenko



2024-02-26 15:38:44

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 3/9] auxdisplay: ht16k33: Add default to switch-cases

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> Currently the compiler (GCC) is able to figure out that there is no
> other choices possible than those that are already listed in the
> switch-cases. However, if we want to move some code to the callback,
> compiler will start complaining that no default is defined. Make
> sure we have all switch-cases equiped with default.
>
> Reported-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 15:42:40

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 2/9] auxdisplay: linedisp: Allocate buffer for the string

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> Always allocate a buffer for the currently displayed characters.
> It makes the line display API simpler.
>
> Signed-off-by: Andy Shevchenko <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 15:47:52

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 7/9] auxdisplay: ht16k33: Drop struct ht16k33_seg

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> The struct ht16k33_seg is repeating struct linedisp. Use the latter
> directly.
>
> Signed-off-by: Andy Shevchenko <[email protected]>

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 15:53:56

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 8/9] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

Hi Andy,

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> Add initial device tree documentation for Maxim MAX6958/6959.
> As per reviewer's request mention the fact of absence reset and
> power enable pins, since the hardware is quite simple.
>
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> @@ -0,0 +1,44 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/auxdisplay/maxim,max6959.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MAX6958/6959 7-segment LED display controller
> +
> +maintainers:
> + - Andy Shevchenko <[email protected]>
> +
> +description:
> + The Maxim MAX6958/6959 7-segment LED display controller provides
> + an I2C interface to up to four 7-segment LED digits. The MAX6959,
> + in comparison to MAX6958, adds input support. Type of the chip can
> + be autodetected via specific register read, and hence the features
> + may be enabled in the driver at run-time, in case they are requested
> + via Device Tree. A given hardware is simple and does not provide
> + any additional pins, such as reset or enable.
> +
> +properties:
> + compatible:
> + const: maxim,max6959
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg

To handle cases where less than 4 characters are wired
(based on hit,hd44780.yaml):

display-width-chars:
description: Width of the display, in character cells.
minimum: 1
maximum: 4
default: 4

The rest LGTM, so
Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 16:01:58

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

On Mon, Feb 26, 2024 at 04:28:20PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <[email protected]> wrote:
> > For better usability group the display drivers together in Kconfig
> > and Makefile. With this we will have the following sections:
> > - Character LCD
> > - Samsung KS0108 LCD controller
> > - Single character line display
> > - Character LCD with non-conforming interface

..

> > +config HT16K33
> > + tristate "Holtek Ht16K33 LED controller with keyscan"
>
> HT16K33 also supports dot-matrix displays using fbdev...
> Yes, categorizing is difficult.

So, what to do here?

..

> I still think these should be sorted alphabetically.

Okay.

--
With Best Regards,
Andy Shevchenko



2024-02-26 16:03:36

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

Hi Andy,

On Mon, Feb 26, 2024 at 5:00 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Feb 26, 2024 at 04:28:20PM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > For better usability group the display drivers together in Kconfig
> > > and Makefile. With this we will have the following sections:
> > > - Character LCD
> > > - Samsung KS0108 LCD controller
> > > - Single character line display
> > > - Character LCD with non-conforming interface
>
> ...
>
> > > +config HT16K33
> > > + tristate "Holtek Ht16K33 LED controller with keyscan"
> >
> > HT16K33 also supports dot-matrix displays using fbdev...
> > Yes, categorizing is difficult.
>
> So, what to do here?

Create a new section for multi-function displays?

>
> ...
>
> > I still think these should be sorted alphabetically.
>
> Okay.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 16:10:34

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

Hi Andy,

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> For better usability group the display drivers together in Kconfig
> and Makefile. With this we will have the following sections:
> - Character LCD
> - Samsung KS0108 LCD controller
> - Single character line display
> - Character LCD with non-conforming interface
>
> While at it, drop redundant 'default n' entries.
>
> Tested-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Andy Shevchenko <[email protected]>

Thanks for your patch!

> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig

> +#
> +# Single character line display section
> +#
> +config LINEDISP
> + tristate "Character line display core support" if COMPILE_TEST
> + help
> + This is the core support for single-line character displays, to be
> + selected by drivers that use it.
> +
> +config IMG_ASCII_LCD
> + tristate "Imagination Technologies ASCII LCD Display"
> + depends on HAS_IOMEM
> + default y if MIPS_MALTA
> + select MFD_SYSCON
> + select LINEDISP
> + help
> + Enable this to support the simple ASCII LCD displays found on
> + development boards such as the MIPS Boston, MIPS Malta & MIPS SEAD3
> + from Imagination Technologies.
> +
> +config HT16K33
> + tristate "Holtek Ht16K33 LED controller with keyscan"

HT16K33 also supports dot-matrix displays using fbdev...
Yes, categorizing is difficult.

> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -5,12 +5,15 @@
>
> obj-$(CONFIG_CHARLCD) += charlcd.o
> obj-$(CONFIG_HD44780_COMMON) += hd44780_common.o
> -obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o
> +obj-$(CONFIG_HD44780) += hd44780.o
> +obj-$(CONFIG_LCD2S) += lcd2s.o
> +obj-$(CONFIG_PARPORT_PANEL) += panel.o
> +
> 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_HT16K33) += ht16k33.o
> -obj-$(CONFIG_PARPORT_PANEL) += panel.o
> -obj-$(CONFIG_LCD2S) += lcd2s.o
> +
> obj-$(CONFIG_LINEDISP) += line-display.o
> +obj-$(CONFIG_IMG_ASCII_LCD) += img-ascii-lcd.o
> +obj-$(CONFIG_HT16K33) += ht16k33.o
> +
> +obj-$(CONFIG_ARM_CHARLCD) += arm-charlcd.o

I still think these should be sorted alphabetically.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 16:17:44

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

On Mon, Feb 26, 2024 at 05:01:46PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <[email protected]> wrote:
> > Add initial driver for the MAX6958 and MAX6959 7-segment LED
> > controllers.

> LGTM, so
> Reviewed-by: Geert Uytterhoeven <[email protected]>

Thanks, but see below.

..

> > + u8 buf[4];
> > +
> > + /* Map segments according to datasheet */
> > + buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > + buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > + buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > + buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
>
> for (unsigned int i = 0; i < linedisp->num_chars; i++) { ... }
>
> > +
> > + regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));
>
> linedisp->num_chars

Maybe, but then we probably want to synchronize the 4 there and here as we
can't have VLA on stack.

> > +}

..

> > + ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);
>
> + device_property_read_u32(dev, "display-width-chars", ...) handling.

Not sure it should be part of this series.

--
With Best Regards,
Andy Shevchenko



2024-02-26 16:25:41

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > While developing that driver I realised that there is a lot
> > > of duplication between ht16k33 and a new one. Hence set of
> > > cleanups and refactorings.
> > >
> > > Note, the new driver has minimum support of the hardware and
> > > I have plans to cover more features in the future.
> >
> > Geert, would it be possible to give one more round of reviewing/testing
> > this week? I want to close auxdisplay for next merge window next week.
>
> For 1-7 (linedisp and ht16k33):
> Tested-by: Geert Uytterhoeven <[email protected]>

Thank you for the testing and review, I have pushed patches 2-7, postponed
patch 1 and will see what I can do with patches 8-9.

--
With Best Regards,
Andy Shevchenko



2024-02-26 16:43:07

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 5/9] auxdisplay: ht16k33: Define a few helper macros

On Tue, Feb 20, 2024 at 7:05 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Feb 19, 2024 at 06:58:04PM +0200, Andy Shevchenko wrote:
> > Define a few helper macros — wrappers on contaoner_of)() — for easier
>
> I have fixed 'container_of()' above locally.

Reviewed-by: Geert Uytterhoeven <[email protected]>

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 16:47:18

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

Hi Andy,

On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
<[email protected]> wrote:
> Add initial driver for the MAX6958 and MAX6959 7-segment LED
> controllers.
>
> Signed-off-by: Andy Shevchenko <[email protected]>

Thanks for your patch!

LGTM, so
Reviewed-by: Geert Uytterhoeven <[email protected]>

> --- /dev/null
> +++ b/drivers/auxdisplay/max6959.c

> +static void max6959_disp_update(struct work_struct *work)
> +{
> + struct max6959_priv *priv = container_of(work, struct max6959_priv, work.work);
> + struct linedisp *linedisp = &priv->linedisp;
> + struct linedisp_map *map = linedisp->map;
> + char *s = linedisp->buf;
> + u8 buf[4];
> +
> + /* Map segments according to datasheet */
> + buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> + buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> + buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> + buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;

for (unsigned int i = 0; i < linedisp->num_chars; i++) { ... }

> +
> + regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));

linedisp->num_chars

> +}

> +static int max6959_i2c_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct max6959_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->regmap = devm_regmap_init_i2c(client, &max6959_regmap_config);
> + if (IS_ERR(priv->regmap))
> + return PTR_ERR(priv->regmap);
> +
> + ret = max6959_power_on(priv);
> + if (ret)
> + return ret;
> +
> + ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);

+ device_property_read_u32(dev, "display-width-chars", ...) handling.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 16:53:34

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 1/9] auxdisplay: linedisp: Group display drivers together

On Mon, Feb 26, 2024 at 05:03:10PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 26, 2024 at 5:00 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Mon, Feb 26, 2024 at 04:28:20PM +0100, Geert Uytterhoeven wrote:
> > > On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> > > <[email protected]> wrote:

..

> > > > +config HT16K33
> > > > + tristate "Holtek Ht16K33 LED controller with keyscan"
> > >
> > > HT16K33 also supports dot-matrix displays using fbdev...
> > > Yes, categorizing is difficult.
> >
> > So, what to do here?
>
> Create a new section for multi-function displays?

Not sure we need that. Too many sections.

Okay, I'll defer this patch for now. Seems too much bikeshedding around it.

--
With Best Regards,
Andy Shevchenko



2024-02-26 17:12:00

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

Hi Andy,

On Mon, Feb 26, 2024 at 5:17 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Feb 26, 2024 at 05:01:46PM +0100, Geert Uytterhoeven wrote:
> > On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > Add initial driver for the MAX6958 and MAX6959 7-segment LED
> > > controllers.
>
> > LGTM, so
> > Reviewed-by: Geert Uytterhoeven <[email protected]>
>
> Thanks, but see below.
>
> ...
>
> > > + u8 buf[4];
> > > +
> > > + /* Map segments according to datasheet */
> > > + buf[0] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > > + buf[1] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > > + buf[2] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> > > + buf[3] = bitrev8(map_to_seg7(&map->map.seg7, *s++)) >> 1;
> >
> > for (unsigned int i = 0; i < linedisp->num_chars; i++) { ... }
> >
> > > +
> > > + regmap_bulk_write(priv->regmap, REG_DIGIT(0), buf, ARRAY_SIZE(buf));
> >
> > linedisp->num_chars
>
> Maybe, but then we probably want to synchronize the 4 there and here as we
> can't have VLA on stack.

You can still keep the maximum buf[4], so no VLA needed?

>
> > > +}
>
> ...
>
> > > + ret = linedisp_register(&priv->linedisp, dev, 4, &max6959_linedisp_ops);
> >
> > + device_property_read_u32(dev, "display-width-chars", ...) handling.
>
> Not sure it should be part of this series.

Fair enough, so please go ahead without.

Even "display-width-chars" might not cover all cases, as a board
may use any subset of the 4 DIGn signals, even in any order...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 17:14:56

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 9/9] auxdisplay: Add driver for MAX695x 7-segment LED controllers

On Mon, Feb 26, 2024 at 05:01:46PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <[email protected]> wrote:
> > Add initial driver for the MAX6958 and MAX6959 7-segment LED
> > controllers.

..

> LGTM, so
> Reviewed-by: Geert Uytterhoeven <[email protected]>

As per discussion, let consider maximum digits as a next feature coming later
on. I'll take this tag, tell me if I shouldn't.
Thank you!

--
With Best Regards,
Andy Shevchenko



2024-02-26 17:15:50

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 8/9] dt-bindings: auxdisplay: Add Maxim MAX6958/6959

On Mon, Feb 26, 2024 at 04:52:34PM +0100, Geert Uytterhoeven wrote:
> On Mon, Feb 19, 2024 at 6:03 PM Andy Shevchenko
> <[email protected]> wrote:

..

> To handle cases where less than 4 characters are wired
> (based on hit,hd44780.yaml):
>
> display-width-chars:
> description: Width of the display, in character cells.
> minimum: 1
> maximum: 4
> default: 4

As discussed in the patch 9 this seems to be a material for other update.

> The rest LGTM, so
> Reviewed-by: Geert Uytterhoeven <[email protected]>

I'll take this tag when applying the patch. Tell me if it's not the case.
Thank you!

--
With Best Regards,
Andy Shevchenko



2024-02-26 18:13:35

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

Hi Andy,

On Mon, Feb 26, 2024 at 7:00 PM Andy Shevchenko
<[email protected]> wrote:
> On Mon, Feb 26, 2024 at 06:24:29PM +0200, Andy Shevchenko wrote:
> > On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> > > On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> > > <[email protected]> wrote:
> > > > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > > > While developing that driver I realised that there is a lot
> > > > > of duplication between ht16k33 and a new one. Hence set of
> > > > > cleanups and refactorings.
> > > > >
> > > > > Note, the new driver has minimum support of the hardware and
> > > > > I have plans to cover more features in the future.
> > > >
> > > > Geert, would it be possible to give one more round of reviewing/testing
> > > > this week? I want to close auxdisplay for next merge window next week.
> > >
> > > For 1-7 (linedisp and ht16k33):
> > > Tested-by: Geert Uytterhoeven <[email protected]>
> >
> > Thank you for the testing and review, I have pushed patches 2-7, postponed
> > patch 1 and will see what I can do with patches 8-9.
>
> After discussion I have applied the patches 8-9 as is in this series with
> Geert's tags. The other things are considered as new features that can be
> implemented later on.

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg

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

2024-02-26 22:10:18

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v3 0/9] auxdisplay: linedisp: Clean up and add new driver

On Mon, Feb 26, 2024 at 06:24:29PM +0200, Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 02:56:35PM +0100, Geert Uytterhoeven wrote:
> > On Thu, Feb 22, 2024 at 2:51 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Mon, Feb 19, 2024 at 06:57:59PM +0200, Andy Shevchenko wrote:
> > > > Add a new initial driver for Maxim MAX6958/6959 chips.
> > > > While developing that driver I realised that there is a lot
> > > > of duplication between ht16k33 and a new one. Hence set of
> > > > cleanups and refactorings.
> > > >
> > > > Note, the new driver has minimum support of the hardware and
> > > > I have plans to cover more features in the future.
> > >
> > > Geert, would it be possible to give one more round of reviewing/testing
> > > this week? I want to close auxdisplay for next merge window next week.
> >
> > For 1-7 (linedisp and ht16k33):
> > Tested-by: Geert Uytterhoeven <[email protected]>
>
> Thank you for the testing and review, I have pushed patches 2-7, postponed
> patch 1 and will see what I can do with patches 8-9.

After discussion I have applied the patches 8-9 as is in this series with
Geert's tags. The other things are considered as new features that can be
implemented later on.

--
With Best Regards,
Andy Shevchenko