* [PATCH v1 01/15] auxdisplay: img-ascii-lcd: Make container_of() no-op for struct linedisp
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
@ 2024-02-08 18:47 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 02/15] auxdisplay: linedisp: Free allocated resources in ->release() Andy Shevchenko
` (13 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:47 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Move embedded struct linedisp member to make container_of() no-op.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/img-ascii-lcd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 56efda0740fb..09014ada38bd 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -32,21 +32,21 @@ struct img_ascii_lcd_config {
/**
* struct img_ascii_lcd_ctx - Private data structure
+ * @linedisp: line display structure
* @base: the base address of the LCD registers
* @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
- * @linedisp: line display structure
* @curr: the string currently displayed on the LCD
*/
struct img_ascii_lcd_ctx {
+ struct linedisp linedisp;
union {
void __iomem *base;
struct regmap *regmap;
};
u32 offset;
const struct img_ascii_lcd_config *cfg;
- struct linedisp linedisp;
char curr[] __aligned(8);
};
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 02/15] auxdisplay: linedisp: Free allocated resources in ->release()
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
2024-02-08 18:47 ` [PATCH v1 01/15] auxdisplay: img-ascii-lcd: Make container_of() no-op for struct linedisp Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 03/15] auxdisplay: linedisp: Use unique number for id Andy Shevchenko
` (12 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
While there is no issue currently with the resources allocation,
the code may still be made more robust by deallocating message
in the ->release() callback.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/line-display.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 03e7f104aa1a..310e9bfb41ae 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -188,8 +188,16 @@ static struct attribute *linedisp_attrs[] = {
};
ATTRIBUTE_GROUPS(linedisp);
+static void linedisp_release(struct device *dev)
+{
+ struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+
+ kfree(linedisp->message);
+}
+
static const struct device_type linedisp_type = {
.groups = linedisp_groups,
+ .release = linedisp_release,
};
/**
@@ -253,7 +261,6 @@ void linedisp_unregister(struct linedisp *linedisp)
{
device_del(&linedisp->dev);
del_timer_sync(&linedisp->timer);
- kfree(linedisp->message);
put_device(&linedisp->dev);
}
EXPORT_SYMBOL_GPL(linedisp_unregister);
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 03/15] auxdisplay: linedisp: Use unique number for id
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
2024-02-08 18:47 ` [PATCH v1 01/15] auxdisplay: img-ascii-lcd: Make container_of() no-op for struct linedisp Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 02/15] auxdisplay: linedisp: Free allocated resources in ->release() Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 04/15] auxdisplay: linedisp: Unshadow error codes in ->store() Andy Shevchenko
` (11 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
The absence of decrementation of linedisp_id is incorrect in two ways,
i.e. it may cause:
- an ID exhaustion
- (and if the above is addressed) a duplicate id number may be allocated
next time a device is added
Replace above mentioned approach by using IDA framework.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/line-display.c | 13 ++++++++++---
drivers/auxdisplay/line-display.h | 2 ++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 310e9bfb41ae..c4dbb13293d1 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -11,6 +11,7 @@
#include <generated/utsrelease.h>
#include <linux/device.h>
+#include <linux/idr.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -188,11 +189,14 @@ static struct attribute *linedisp_attrs[] = {
};
ATTRIBUTE_GROUPS(linedisp);
+static DEFINE_IDA(linedisp_id);
+
static void linedisp_release(struct device *dev)
{
struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
kfree(linedisp->message);
+ ida_free(&linedisp_id, linedisp->id);
}
static const struct device_type linedisp_type = {
@@ -214,7 +218,6 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
unsigned int num_chars, char *buf,
void (*update)(struct linedisp *linedisp))
{
- static atomic_t linedisp_id = ATOMIC_INIT(-1);
int err;
memset(linedisp, 0, sizeof(*linedisp));
@@ -225,9 +228,13 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
linedisp->num_chars = num_chars;
linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
+ err = ida_alloc(&linedisp_id, GFP_KERNEL);
+ if (err < 0)
+ return err;
+ linedisp->id = err;
+
device_initialize(&linedisp->dev);
- dev_set_name(&linedisp->dev, "linedisp.%lu",
- (unsigned long)atomic_inc_return(&linedisp_id));
+ dev_set_name(&linedisp->dev, "linedisp.%u", linedisp->id);
/* initialise a timer for scrolling the message */
timer_setup(&linedisp->timer, linedisp_scroll, 0);
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 0f5891d34c48..1fbe57fdc4cb 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -14,6 +14,7 @@
/**
* struct linedisp - character line display private data structure
* @dev: the line display device
+ * @id: instance id of this display
* @timer: timer used to implement scrolling
* @update: function called to update the display
* @buf: pointer to the buffer for the string currently displayed
@@ -25,6 +26,7 @@
*/
struct linedisp {
struct device dev;
+ unsigned int id;
struct timer_list timer;
void (*update)(struct linedisp *linedisp);
char *buf;
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 04/15] auxdisplay: linedisp: Unshadow error codes in ->store()
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (2 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 03/15] auxdisplay: linedisp: Use unique number for id Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 05/15] auxdisplay: linedisp: Add missing header(s) Andy Shevchenko
` (10 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
kstrtox() may return different error codes.
Unshadow them in the ->store() callback to give better error report.
While at it, add missing kstrtox.h inclusion.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/line-display.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index c4dbb13293d1..4b92ae7781f1 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -12,6 +12,7 @@
#include <linux/device.h>
#include <linux/idr.h>
+#include <linux/kstrtox.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/string.h>
@@ -166,9 +167,11 @@ static ssize_t scroll_step_ms_store(struct device *dev,
{
struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
unsigned int ms;
+ int ret;
- if (kstrtouint(buf, 10, &ms) != 0)
- return -EINVAL;
+ ret = kstrtouint(buf, 10, &ms);
+ if (ret)
+ return ret;
linedisp->scroll_rate = msecs_to_jiffies(ms);
if (linedisp->message && linedisp->message_len > linedisp->num_chars) {
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 05/15] auxdisplay: linedisp: Add missing header(s)
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (3 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 04/15] auxdisplay: linedisp: Unshadow error codes in ->store() Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 06/15] auxdisplay: linedisp: Move exported symbols to a namespace Andy Shevchenko
` (9 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Do not imply that some of the generic headers may be always included.
Instead, include explicitly what we are direct user of.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/line-display.c | 3 +++
drivers/auxdisplay/line-display.h | 3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 4b92ae7781f1..6b3d25e20eeb 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -10,8 +10,11 @@
#include <generated/utsrelease.h>
+#include <linux/container_of.h>
#include <linux/device.h>
+#include <linux/export.h>
#include <linux/idr.h>
+#include <linux/jiffies.h>
#include <linux/kstrtox.h>
#include <linux/module.h>
#include <linux/slab.h>
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 1fbe57fdc4cb..d72c1899dc50 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -11,6 +11,9 @@
#ifndef _LINEDISP_H
#define _LINEDISP_H
+#include <linux/device.h>
+#include <linux/timer_types.h>
+
/**
* struct linedisp - character line display private data structure
* @dev: the line display device
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 06/15] auxdisplay: linedisp: Move exported symbols to a namespace
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (4 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 05/15] auxdisplay: linedisp: Add missing header(s) Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 07/15] auxdisplay: linedisp: Group line display drivers together Andy Shevchenko
` (8 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Avoid unnecessary pollution of the global symbol namespace by
moving library functions in to a specific namespace and import
that into the drivers that make use of the functions.
For more info: https://lwn.net/Articles/760045/
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/ht16k33.c | 1 +
drivers/auxdisplay/img-ascii-lcd.c | 1 +
drivers/auxdisplay/line-display.c | 4 ++--
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index a90430b7d07b..c6a42c5c128f 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -831,4 +831,5 @@ module_i2c_driver(ht16k33_driver);
MODULE_DESCRIPTION("Holtek HT16K33 driver");
MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
MODULE_AUTHOR("Robin van der Gracht <robin@protonic.nl>");
diff --git a/drivers/auxdisplay/img-ascii-lcd.c b/drivers/auxdisplay/img-ascii-lcd.c
index 09014ada38bd..c571e54d9eb5 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -298,3 +298,4 @@ module_platform_driver(img_ascii_lcd_driver);
MODULE_DESCRIPTION("Imagination Technologies ASCII LCD Display");
MODULE_AUTHOR("Paul Burton <paul.burton@mips.com>");
MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 6b3d25e20eeb..851b2c0f9443 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -263,7 +263,7 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
put_device(&linedisp->dev);
return err;
}
-EXPORT_SYMBOL_GPL(linedisp_register);
+EXPORT_SYMBOL_NS_GPL(linedisp_register, LINEDISP);
/**
* linedisp_unregister - unregister a character line display
@@ -276,6 +276,6 @@ void linedisp_unregister(struct linedisp *linedisp)
del_timer_sync(&linedisp->timer);
put_device(&linedisp->dev);
}
-EXPORT_SYMBOL_GPL(linedisp_unregister);
+EXPORT_SYMBOL_NS_GPL(linedisp_unregister, LINEDISP);
MODULE_LICENSE("GPL");
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 07/15] auxdisplay: linedisp: Group line display drivers together
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (5 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 06/15] auxdisplay: linedisp: Move exported symbols to a namespace Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 08/15] auxdisplay: linedisp: Provide struct linedisp_ops for future extension Andy Shevchenko
` (7 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
For better usability group the line display drivers together in Kconfig
and Makefile.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/Kconfig | 32 ++++++++++++++++----------------
drivers/auxdisplay/Makefile | 12 ++++++------
2 files changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index d944d5298eca..a34a9a52158f 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -25,12 +25,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,6 +46,16 @@ config HD44780
kernel and started at boot.
If you don't understand what all this is about, say N.
+config LCD2S
+ tristate "lcd2s 20x4 character display over I2C console"
+ depends on I2C
+ select CHARLCD
+ help
+ This is a driver that lets you use the lcd2s 20x4 character display
+ from Modtronix engineering as a console output device. The display
+ is a simple single color character display. You have to connect it
+ to an I2C bus.
+
config KS0108
tristate "KS0108 LCD Controller"
depends on PARPORT_PC
@@ -153,6 +157,12 @@ config CFAG12864B_RATE
If you compile this as a module, you can still override this
value using the module parameters.
+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
@@ -177,16 +187,6 @@ config HT16K33
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
- select CHARLCD
- help
- This is a driver that lets you use the lcd2s 20x4 character display
- from Modtronix engineering as a console output device. The display
- 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
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 6968ed4d3f0a..43bad850481c 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -5,12 +5,12 @@
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_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
+obj-$(CONFIG_PARPORT_PANEL) += panel.o
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 08/15] auxdisplay: linedisp: Provide struct linedisp_ops for future extension
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (6 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 07/15] auxdisplay: linedisp: Group line display drivers together Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 09/15] auxdisplay: linedisp: Add support for overriding character mapping Andy Shevchenko
` (6 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Currently the line display library doesn't scale in case we want to
provide more operations. Prepare the library to take a newly created
struct linedisp_ops that scales.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/ht16k33.c | 7 +++++--
drivers/auxdisplay/img-ascii-lcd.c | 19 ++++++++++++-------
drivers/auxdisplay/line-display.c | 11 +++++------
drivers/auxdisplay/line-display.h | 17 +++++++++++++----
4 files changed, 35 insertions(+), 19 deletions(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index c6a42c5c128f..0cdf3fbdf81e 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -448,6 +448,10 @@ static void ht16k33_linedisp_update(struct linedisp *linedisp)
schedule_delayed_work(&priv->work, 0);
}
+static const struct linedisp_ops ht16k33_linedisp_ops = {
+ .update = ht16k33_linedisp_update,
+};
+
static void ht16k33_seg7_update(struct work_struct *work)
{
struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
@@ -696,8 +700,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_update);
+ err = linedisp_register(&seg->linedisp, dev, 4, seg->curr, &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 c571e54d9eb5..c0ec7d9a1c62 100644
--- a/drivers/auxdisplay/img-ascii-lcd.c
+++ b/drivers/auxdisplay/img-ascii-lcd.c
@@ -22,12 +22,12 @@ struct img_ascii_lcd_ctx;
* struct img_ascii_lcd_config - Configuration information about an LCD model
* @num_chars: the number of characters the LCD can display
* @external_regmap: true if registers are in a system controller, else false
- * @update: function called to update the LCD
+ * @ops: character line display operations
*/
struct img_ascii_lcd_config {
unsigned int num_chars;
bool external_regmap;
- void (*update)(struct linedisp *linedisp);
+ const struct linedisp_ops ops;
};
/**
@@ -75,7 +75,9 @@ static void boston_update(struct linedisp *linedisp)
static struct img_ascii_lcd_config boston_config = {
.num_chars = 8,
- .update = boston_update,
+ .ops = {
+ .update = boston_update,
+ },
};
/*
@@ -103,7 +105,9 @@ static void malta_update(struct linedisp *linedisp)
static struct img_ascii_lcd_config malta_config = {
.num_chars = 8,
.external_regmap = true,
- .update = malta_update,
+ .ops = {
+ .update = malta_update,
+ },
};
/*
@@ -203,7 +207,9 @@ static void sead3_update(struct linedisp *linedisp)
static struct img_ascii_lcd_config sead3_config = {
.num_chars = 16,
.external_regmap = true,
- .update = sead3_update,
+ .ops = {
+ .update = sead3_update,
+ },
};
static const struct of_device_id img_ascii_lcd_matches[] = {
@@ -247,8 +253,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->update);
+ err = linedisp_register(&ctx->linedisp, dev, cfg->num_chars, ctx->curr, &cfg->ops);
if (err)
return err;
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 851b2c0f9443..62e8a911bb12 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -50,7 +50,7 @@ static void linedisp_scroll(struct timer_list *t)
}
/* update the display */
- linedisp->update(linedisp);
+ linedisp->ops->update(linedisp);
/* move on to the next character */
linedisp->scroll_pos++;
@@ -94,7 +94,7 @@ static int linedisp_display(struct linedisp *linedisp, const char *msg,
linedisp->message = NULL;
linedisp->message_len = 0;
memset(linedisp->buf, ' ', linedisp->num_chars);
- linedisp->update(linedisp);
+ linedisp->ops->update(linedisp);
return 0;
}
@@ -216,20 +216,19 @@ static const struct device_type linedisp_type = {
* @parent: parent device
* @num_chars: the number of characters that can be displayed
* @buf: pointer to a buffer that can hold @num_chars characters
- * @update: Function called to update the display. This must not sleep!
+ * @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,
- void (*update)(struct linedisp *linedisp))
+ unsigned int num_chars, char *buf, const struct linedisp_ops *ops)
{
int err;
memset(linedisp, 0, sizeof(*linedisp));
linedisp->dev.parent = parent;
linedisp->dev.type = &linedisp_type;
- linedisp->update = update;
+ linedisp->ops = ops;
linedisp->buf = buf;
linedisp->num_chars = num_chars;
linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index d72c1899dc50..a4f0d1bf5848 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -14,12 +14,22 @@
#include <linux/device.h>
#include <linux/timer_types.h>
+struct linedisp;
+
+/**
+ * struct linedisp_ops - character line display operations
+ * @update: Function called to update the display. This must not sleep!
+ */
+struct linedisp_ops {
+ void (*update)(struct linedisp *linedisp);
+};
+
/**
* struct linedisp - character line display private data structure
* @dev: the line display device
* @id: instance id of this display
* @timer: timer used to implement scrolling
- * @update: function called to update the display
+ * @ops: character line display operations
* @buf: pointer to the buffer for the string currently displayed
* @message: the full message to display or scroll on the display
* @num_chars: the number of characters that can be displayed
@@ -31,7 +41,7 @@ struct linedisp {
struct device dev;
unsigned int id;
struct timer_list timer;
- void (*update)(struct linedisp *linedisp);
+ const struct linedisp_ops *ops;
char *buf;
char *message;
unsigned int num_chars;
@@ -41,8 +51,7 @@ struct linedisp {
};
int linedisp_register(struct linedisp *linedisp, struct device *parent,
- unsigned int num_chars, char *buf,
- void (*update)(struct linedisp *linedisp));
+ unsigned int num_chars, char *buf, const struct linedisp_ops *ops);
void linedisp_unregister(struct linedisp *linedisp);
#endif /* LINEDISP_H */
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 09/15] auxdisplay: linedisp: Add support for overriding character mapping
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (7 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 08/15] auxdisplay: linedisp: Provide struct linedisp_ops for future extension Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp Andy Shevchenko
` (5 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
There is already the driver using character mapping table for
7 or 14 segment display. It is possible to override it. Make
the similar in the line display library to allow other drivers
to utilise the same functionality.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/line-display.c | 111 +++++++++++++++++++++++++++++-
drivers/auxdisplay/line-display.h | 31 +++++++++
2 files changed, 140 insertions(+), 2 deletions(-)
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index 62e8a911bb12..da47fc59f6cb 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -22,6 +22,9 @@
#include <linux/sysfs.h>
#include <linux/timer.h>
+#include <linux/map_to_7segment.h>
+#include <linux/map_to_14segment.h>
+
#include "line-display.h"
#define DEFAULT_SCROLL_RATE (HZ / 2)
@@ -188,12 +191,71 @@ static ssize_t scroll_step_ms_store(struct device *dev,
static DEVICE_ATTR_RW(scroll_step_ms);
+static ssize_t map_seg_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+ struct linedisp_map *map = linedisp->map;
+
+ memcpy(buf, &map->map, map->size);
+ return map->size;
+}
+
+static ssize_t map_seg_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+ struct linedisp_map *map = linedisp->map;
+
+ if (count != map->size)
+ return -EINVAL;
+
+ memcpy(&map->map, buf, count);
+ return count;
+}
+
+static const SEG7_DEFAULT_MAP(initial_map_seg7);
+static DEVICE_ATTR(map_seg7, 0644, map_seg_show, map_seg_store);
+
+static const SEG14_DEFAULT_MAP(initial_map_seg14);
+static DEVICE_ATTR(map_seg14, 0644, map_seg_show, map_seg_store);
+
static struct attribute *linedisp_attrs[] = {
&dev_attr_message.attr,
&dev_attr_scroll_step_ms.attr,
- NULL,
+ &dev_attr_map_seg7.attr,
+ &dev_attr_map_seg14.attr,
+ NULL
};
-ATTRIBUTE_GROUPS(linedisp);
+
+static umode_t linedisp_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+ struct device *dev = kobj_to_dev(kobj);
+ struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+ struct linedisp_map *map = linedisp->map;
+ umode_t mode = attr->mode;
+
+ if (attr == &dev_attr_map_seg7.attr) {
+ if (!map)
+ return 0;
+ if (map->type != LINEDISP_MAP_SEG7)
+ return 0;
+ }
+
+ if (attr == &dev_attr_map_seg14.attr) {
+ if (!map)
+ return 0;
+ if (map->type != LINEDISP_MAP_SEG14)
+ return 0;
+ }
+
+ return mode;
+};
+
+static const struct attribute_group linedisp_group = {
+ .is_visible = linedisp_attr_is_visible,
+ .attrs = linedisp_attrs,
+};
+__ATTRIBUTE_GROUPS(linedisp);
static DEFINE_IDA(linedisp_id);
@@ -201,6 +263,7 @@ static void linedisp_release(struct device *dev)
{
struct linedisp *linedisp = container_of(dev, struct linedisp, dev);
+ kfree(linedisp->map);
kfree(linedisp->message);
ida_free(&linedisp_id, linedisp->id);
}
@@ -210,6 +273,44 @@ static const struct device_type linedisp_type = {
.release = linedisp_release,
};
+static int linedisp_init_map(struct linedisp *linedisp)
+{
+ struct linedisp_map *map;
+ int err;
+
+ if (!linedisp->ops->get_map_type)
+ return 0;
+
+ err = linedisp->ops->get_map_type(linedisp);
+ if (err < 0)
+ return err;
+
+ map = kmalloc(sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return -ENOMEM;
+
+ map->type = err;
+
+ /* assign initial mapping */
+ switch (map->type) {
+ case LINEDISP_MAP_SEG7:
+ map->map.seg7 = initial_map_seg7;
+ map->size = sizeof(map->map.seg7);
+ break;
+ case LINEDISP_MAP_SEG14:
+ map->map.seg14 = initial_map_seg14;
+ map->size = sizeof(map->map.seg14);
+ break;
+ default:
+ kfree(map);
+ return -EINVAL;
+ }
+
+ linedisp->map = map;
+
+ return 0;
+}
+
/**
* linedisp_register - register a character line display
* @linedisp: pointer to character line display structure
@@ -241,6 +342,11 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
device_initialize(&linedisp->dev);
dev_set_name(&linedisp->dev, "linedisp.%u", linedisp->id);
+ /* initialise a character mapping, if required */
+ err = linedisp_init_map(linedisp);
+ if (err)
+ goto out_put_device;
+
/* initialise a timer for scrolling the message */
timer_setup(&linedisp->timer, linedisp_scroll, 0);
@@ -259,6 +365,7 @@ int linedisp_register(struct linedisp *linedisp, struct device *parent,
device_del(&linedisp->dev);
out_del_timer:
del_timer_sync(&linedisp->timer);
+out_put_device:
put_device(&linedisp->dev);
return err;
}
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index a4f0d1bf5848..65d782111f53 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -14,13 +14,43 @@
#include <linux/device.h>
#include <linux/timer_types.h>
+#include <linux/map_to_7segment.h>
+#include <linux/map_to_14segment.h>
+
struct linedisp;
+/**
+ * enum linedisp_map_type - type of the character mapping
+ * @LINEDISP_MAP_SEG7: Map characters to 7 segment display
+ * @LINEDISP_MAP_SEG14: Map characters to 14 segment display
+ */
+enum linedisp_map_type {
+ LINEDISP_MAP_SEG7,
+ LINEDISP_MAP_SEG14,
+};
+
+/**
+ * struct linedisp_map - character mapping
+ * @type: type of the character mapping
+ * @map: conversion character mapping
+ * @size: size of the @map
+ */
+struct linedisp_map {
+ enum linedisp_map_type type;
+ union {
+ struct seg7_conversion_map seg7;
+ struct seg14_conversion_map seg14;
+ } map;
+ unsigned int size;
+};
+
/**
* struct linedisp_ops - character line display operations
+ * @get_map_type: Function called to get the character mapping, if required
* @update: Function called to update the display. This must not sleep!
*/
struct linedisp_ops {
+ int (*get_map_type)(struct linedisp *linedisp);
void (*update)(struct linedisp *linedisp);
};
@@ -42,6 +72,7 @@ struct linedisp {
unsigned int id;
struct timer_list timer;
const struct linedisp_ops *ops;
+ struct linedisp_map *map;
char *buf;
char *message;
unsigned int num_chars;
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (8 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 09/15] auxdisplay: linedisp: Add support for overriding character mapping Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-12 8:25 ` Robin van der Gracht
2024-02-08 18:48 ` [PATCH v1 11/15] auxdisplay: ht16k33: Move ht16k33_linedisp_ops down Andy Shevchenko
` (4 subsequent siblings)
14 siblings, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
There is a driver that uses small buffer for the string, when we
add a new one, we may avoid duplication and use one provided by
the line display library. Allow user to skip buffer pointer when
registering a device.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/line-display.c | 4 ++--
drivers/auxdisplay/line-display.h | 4 ++++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
index da47fc59f6cb..a3c706e1739d 100644
--- a/drivers/auxdisplay/line-display.c
+++ b/drivers/auxdisplay/line-display.c
@@ -330,8 +330,8 @@ 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->buf = buf ? buf : linedisp->curr;
+ linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
err = ida_alloc(&linedisp_id, GFP_KERNEL);
diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
index 65d782111f53..4c354b8f376e 100644
--- a/drivers/auxdisplay/line-display.h
+++ b/drivers/auxdisplay/line-display.h
@@ -54,12 +54,15 @@ struct linedisp_ops {
void (*update)(struct linedisp *linedisp);
};
+#define LINEDISP_DEFAULT_BUF_SZ 8u
+
/**
* struct linedisp - character line display private data structure
* @dev: the line display device
* @id: instance id of this display
* @timer: timer used to implement scrolling
* @ops: character line display operations
+ * @curr: fallback buffer for the string
* @buf: pointer to the buffer for the string currently displayed
* @message: the full message to display or scroll on the display
* @num_chars: the number of characters that can be displayed
@@ -73,6 +76,7 @@ struct linedisp {
struct timer_list timer;
const struct linedisp_ops *ops;
struct linedisp_map *map;
+ char curr[LINEDISP_DEFAULT_BUF_SZ];
char *buf;
char *message;
unsigned int num_chars;
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp
2024-02-08 18:48 ` [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp Andy Shevchenko
@ 2024-02-12 8:25 ` Robin van der Gracht
2024-02-12 11:50 ` Andy Shevchenko
0 siblings, 1 reply; 24+ messages in thread
From: Robin van der Gracht @ 2024-02-12 8:25 UTC (permalink / raw)
To: Andy Shevchenko
Cc: devicetree, linux-kernel, Miguel Ojeda, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Burton,
Geert Uytterhoeven
Hi Andy,
Thank you for your patches.
See inline comment below.
On Thu, 8 Feb 2024 20:48:08 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> There is a driver that uses small buffer for the string, when we
> add a new one, we may avoid duplication and use one provided by
> the line display library. Allow user to skip buffer pointer when
> registering a device.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/auxdisplay/line-display.c | 4 ++--
> drivers/auxdisplay/line-display.h | 4 ++++
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/auxdisplay/line-display.c b/drivers/auxdisplay/line-display.c
> index da47fc59f6cb..a3c706e1739d 100644
> --- a/drivers/auxdisplay/line-display.c
> +++ b/drivers/auxdisplay/line-display.c
> @@ -330,8 +330,8 @@ 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->buf = buf ? buf : linedisp->curr;
> + linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
It's not a big buffer, but now it's always there even if it's not used.
And even if it's used, it might be only partially used.
Why not used a malloc instead?
> linedisp->scroll_rate = DEFAULT_SCROLL_RATE;
>
> err = ida_alloc(&linedisp_id, GFP_KERNEL);
> diff --git a/drivers/auxdisplay/line-display.h b/drivers/auxdisplay/line-display.h
> index 65d782111f53..4c354b8f376e 100644
> --- a/drivers/auxdisplay/line-display.h
> +++ b/drivers/auxdisplay/line-display.h
> @@ -54,12 +54,15 @@ struct linedisp_ops {
> void (*update)(struct linedisp *linedisp);
> };
>
> +#define LINEDISP_DEFAULT_BUF_SZ 8u
> +
> /**
> * struct linedisp - character line display private data structure
> * @dev: the line display device
> * @id: instance id of this display
> * @timer: timer used to implement scrolling
> * @ops: character line display operations
> + * @curr: fallback buffer for the string
> * @buf: pointer to the buffer for the string currently displayed
> * @message: the full message to display or scroll on the display
> * @num_chars: the number of characters that can be displayed
> @@ -73,6 +76,7 @@ struct linedisp {
> struct timer_list timer;
> const struct linedisp_ops *ops;
> struct linedisp_map *map;
> + char curr[LINEDISP_DEFAULT_BUF_SZ];
> char *buf;
> char *message;
> unsigned int num_chars;
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp
2024-02-12 8:25 ` Robin van der Gracht
@ 2024-02-12 11:50 ` Andy Shevchenko
2024-02-12 16:49 ` Andy Shevchenko
0 siblings, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-12 11:50 UTC (permalink / raw)
To: Robin van der Gracht
Cc: devicetree, linux-kernel, Miguel Ojeda, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Burton,
Geert Uytterhoeven
On Mon, Feb 12, 2024 at 09:25:00AM +0100, Robin van der Gracht wrote:
> On Thu, 8 Feb 2024 20:48:08 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > + linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
>
> It's not a big buffer, but now it's always there even if it's not used.
> And even if it's used, it might be only partially used.
> Why not used a malloc instead?
malloc() infra takes more than this IIRC (something like up to 32 bytes on
64-bit platforms) or comparable sizes. Yes, the malloc() along with the
linedisp structure might make sense, but will require more invasive change.
Do you want me to drop this one from the set?
(I have no hard feelings about it, as I see better way and just having no
time for taking care about, as it's not the main point of the series.)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp
2024-02-12 11:50 ` Andy Shevchenko
@ 2024-02-12 16:49 ` Andy Shevchenko
2024-02-13 10:53 ` Robin van der Gracht
0 siblings, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-12 16:49 UTC (permalink / raw)
To: Robin van der Gracht
Cc: devicetree, linux-kernel, Miguel Ojeda, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Burton,
Geert Uytterhoeven
On Mon, Feb 12, 2024 at 01:50:13PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 12, 2024 at 09:25:00AM +0100, Robin van der Gracht wrote:
> > On Thu, 8 Feb 2024 20:48:08 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> > > + linedisp->num_chars = buf ? num_chars : min(num_chars, LINEDISP_DEFAULT_BUF_SZ);
> >
> > It's not a big buffer, but now it's always there even if it's not used.
> > And even if it's used, it might be only partially used.
> > Why not used a malloc instead?
>
> malloc() infra takes more than this IIRC (something like up to 32 bytes on
> 64-bit platforms) or comparable sizes. Yes, the malloc() along with the
> linedisp structure might make sense, but will require more invasive change.
>
> Do you want me to drop this one from the set?
> (I have no hard feelings about it, as I see better way and just having no
> time for taking care about, as it's not the main point of the series.)
Looking again into it, the allocation separately with linedisp structure
is indeed too much invasive. I prefer (as we save lines of code and deduplicate
the buffer at least for two drivers, including a new one) to leave this patch
for now. We may rework it later on. Do you agree?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp
2024-02-12 16:49 ` Andy Shevchenko
@ 2024-02-13 10:53 ` Robin van der Gracht
0 siblings, 0 replies; 24+ messages in thread
From: Robin van der Gracht @ 2024-02-13 10:53 UTC (permalink / raw)
To: Andy Shevchenko
Cc: devicetree, linux-kernel, Miguel Ojeda, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Burton,
Geert Uytterhoeven
Hi Andy,
On Mon, 12 Feb 2024 18:49:49 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> Looking again into it, the allocation separately with linedisp structure
> is indeed too much invasive. I prefer (as we save lines of code and deduplicate
> the buffer at least for two drivers, including a new one) to leave this patch
> for now. We may rework it later on. Do you agree?
Agreed. The additional overhead is probably not worth it. If the buffer size
needs to be increased later on we'll take another look.
Robin
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v1 11/15] auxdisplay: ht16k33: Move ht16k33_linedisp_ops down
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (9 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 10/15] auxdisplay: linedisp: Provide a small buffer in the struct linedisp Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 12/15] auxdisplay: ht16k33: Switch to use line display character mapping Andy Shevchenko
` (3 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
We will need the update functions to be defined before
ht16k33_linedisp_ops. Move the latter down in the code.
No functional change intended.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/ht16k33.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 0cdf3fbdf81e..75c4a8d31642 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -440,18 +440,6 @@ static void ht16k33_keypad_stop(struct input_dev *dev)
disable_irq(keypad->client->irq);
}
-static void ht16k33_linedisp_update(struct linedisp *linedisp)
-{
- struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
- seg.linedisp);
-
- schedule_delayed_work(&priv->work, 0);
-}
-
-static const struct linedisp_ops ht16k33_linedisp_ops = {
- .update = ht16k33_linedisp_update,
-};
-
static void ht16k33_seg7_update(struct work_struct *work)
{
struct ht16k33_priv *priv = container_of(work, struct ht16k33_priv,
@@ -489,6 +477,18 @@ static void ht16k33_seg14_update(struct work_struct *work)
i2c_smbus_write_i2c_block_data(priv->client, 0, ARRAY_SIZE(buf), buf);
}
+static void ht16k33_linedisp_update(struct linedisp *linedisp)
+{
+ struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
+ seg.linedisp);
+
+ schedule_delayed_work(&priv->work, 0);
+}
+
+static const struct linedisp_ops ht16k33_linedisp_ops = {
+ .update = ht16k33_linedisp_update,
+};
+
static int ht16k33_led_probe(struct device *dev, struct led_classdev *led,
unsigned int brightness)
{
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 12/15] auxdisplay: ht16k33: Switch to use line display character mapping
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (10 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 11/15] auxdisplay: ht16k33: Move ht16k33_linedisp_ops down Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 13/15] auxdisplay: ht16k33: Use buffer from struct linedisp Andy Shevchenko
` (2 subsequent siblings)
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Since line display library supports necessary bits to map the characters
(if required), switch this driver to use that.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/ht16k33.c | 109 +++++++++++------------------------
1 file changed, 34 insertions(+), 75 deletions(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 75c4a8d31642..b104f08252dd 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -87,11 +87,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;
char curr[4];
};
@@ -135,33 +130,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;
@@ -445,18 +413,20 @@ 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;
+ struct linedisp *linedisp = &seg->linedisp;
+ struct linedisp_map *map = linedisp->map;
char *s = seg->curr;
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);
}
@@ -466,17 +436,39 @@ 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;
+ struct linedisp *linedisp = &seg->linedisp;
+ struct linedisp_map *map = linedisp->map;
char *s = seg->curr;
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 = container_of(linedisp, struct ht16k33_priv,
+ seg.linedisp);
+
+ switch (priv->type) {
+ case DISP_MATRIX:
+ /* not handled here */
+ return -EINVAL;
+
+ 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;
+ }
+}
+
static void ht16k33_linedisp_update(struct linedisp *linedisp)
{
struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
@@ -486,6 +478,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,
};
@@ -677,39 +670,7 @@ static int ht16k33_seg_probe(struct device *dev, struct ht16k33_priv *priv,
if (err)
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;
- 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;
- }
- if (err)
- return err;
-
- err = linedisp_register(&seg->linedisp, dev, 4, seg->curr, &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, seg->curr, &ht16k33_linedisp_ops);
}
static int ht16k33_probe(struct i2c_client *client)
@@ -794,8 +755,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;
}
}
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 13/15] auxdisplay: ht16k33: Use buffer from struct linedisp
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (11 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 12/15] auxdisplay: ht16k33: Switch to use line display character mapping Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959 Andy Shevchenko
2024-02-08 18:48 ` [PATCH v1 15/15] auxdisplay: Add driver for MAX695x 7-segment LED controllers Andy Shevchenko
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
struct linedips embedds a small buffer for the string that we may reuse.
Update the driver accordingly.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/ht16k33.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index b104f08252dd..08cc05b9d216 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -85,11 +85,6 @@ struct ht16k33_fbdev {
uint8_t *cache;
};
-struct ht16k33_seg {
- struct linedisp linedisp;
- char curr[4];
-};
-
struct ht16k33_priv {
struct i2c_client *client;
struct delayed_work work;
@@ -97,7 +92,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;
@@ -412,10 +407,9 @@ 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;
- struct linedisp *linedisp = &seg->linedisp;
+ struct linedisp *linedisp = &priv->linedisp;
struct linedisp_map *map = linedisp->map;
- char *s = seg->curr;
+ char *s = linedisp->curr;
uint8_t buf[9];
buf[0] = map_to_seg7(&map->map.seg7, *s++);
@@ -435,10 +429,9 @@ 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;
- struct linedisp *linedisp = &seg->linedisp;
+ struct linedisp *linedisp = &priv->linedisp;
struct linedisp_map *map = linedisp->map;
- char *s = seg->curr;
+ char *s = linedisp->curr;
uint8_t buf[8];
put_unaligned_le16(map_to_seg14(&map->map.seg14, *s++), buf + 0);
@@ -451,8 +444,7 @@ static void ht16k33_seg14_update(struct work_struct *work)
static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
{
- struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
- seg.linedisp);
+ struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv, linedisp);
switch (priv->type) {
case DISP_MATRIX:
@@ -471,8 +463,7 @@ static int ht16k33_linedisp_get_map_type(struct linedisp *linedisp)
static void ht16k33_linedisp_update(struct linedisp *linedisp)
{
- struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv,
- seg.linedisp);
+ struct ht16k33_priv *priv = container_of(linedisp, struct ht16k33_priv, linedisp);
schedule_delayed_work(&priv->work, 0);
}
@@ -663,14 +654,13 @@ 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;
int err;
err = ht16k33_brightness_set(priv, brightness);
if (err)
return err;
- return linedisp_register(&seg->linedisp, dev, 4, seg->curr, &ht16k33_linedisp_ops);
+ return linedisp_register(&priv->linedisp, dev, 4, NULL, &ht16k33_linedisp_ops);
}
static int ht16k33_probe(struct i2c_client *client)
@@ -754,7 +744,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;
}
}
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (12 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 13/15] auxdisplay: ht16k33: Use buffer from struct linedisp Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
2024-02-09 8:02 ` Krzysztof Kozlowski
2024-02-08 18:48 ` [PATCH v1 15/15] auxdisplay: Add driver for MAX695x 7-segment LED controllers Andy Shevchenko
14 siblings, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Add initial device tree documentation for Maxim MAX6958/6959.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
.../bindings/auxdisplay/maxim,max6959.yaml | 35 +++++++++++++++++++
1 file changed, 35 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..49ce26176797
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
@@ -0,0 +1,35 @@
+# 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 with keyscan
+
+maintainers:
+ - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+
+properties:
+ compatible:
+ const: maxim,max6959
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ max6959: max6959@38 {
+ compatible = "maxim,max6959";
+ reg = <0x38>;
+ };
+ };
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
2024-02-08 18:48 ` [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959 Andy Shevchenko
@ 2024-02-09 8:02 ` Krzysztof Kozlowski
2024-02-09 13:59 ` Andy Shevchenko
0 siblings, 1 reply; 24+ messages in thread
From: Krzysztof Kozlowski @ 2024-02-09 8:02 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
On 08/02/2024 19:48, Andy Shevchenko wrote:
> Add initial device tree documentation for Maxim MAX6958/6959.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> .../bindings/auxdisplay/maxim,max6959.yaml | 35 +++++++++++++++++++
> 1 file changed, 35 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..49ce26176797
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/auxdisplay/maxim,max6959.yaml
> @@ -0,0 +1,35 @@
> +# 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 with keyscan
> +
> +maintainers:
> + - Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> +
Please describe the device, e.g. bus/interface.
> +properties:
> + compatible:
> + const: maxim,max6959
Your title said also max6958, so I would expect it to be here as well.
Cam be followed by 6959 fallback compatible, if they are compatible.
> +
> + reg:
> + maxItems: 1
No power supplies? No reset pins?
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + i2c {
> + #address-cells = <1>;
Use 4 spaces for example indentation. 2 is also fine.
> + #size-cells = <0>;
> +
> + max6959: max6959@38 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
e.g. display-controller or display
> + compatible = "maxim,max6959";
> + reg = <0x38>;
> + };
> + };
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
2024-02-09 8:02 ` Krzysztof Kozlowski
@ 2024-02-09 13:59 ` Andy Shevchenko
2024-02-12 8:24 ` Krzysztof Kozlowski
0 siblings, 1 reply; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-09 13:59 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: devicetree, linux-kernel, Miguel Ojeda, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robin van der Gracht,
Paul Burton, Geert Uytterhoeven
On Fri, Feb 09, 2024 at 09:02:44AM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2024 19:48, Andy Shevchenko wrote:
> > Add initial device tree documentation for Maxim MAX6958/6959.
...
> Please describe the device, e.g. bus/interface.
OK.
...
> > +properties:
> > + compatible:
> > + const: maxim,max6959
>
> Your title said also max6958, so I would expect it to be here as well.
> Cam be followed by 6959 fallback compatible, if they are compatible.
Same question as I asked before, why should we have them separated?
The hardware features can be autodetected. What's the reason for (unneeded
in my opinion and duplicative) compatible?
...
> > + reg:
> > + maxItems: 1
>
> No power supplies? No reset pins?
No power supplies, no reset pins. At least there is no as such in
the datasheet. Do you see them there?
...
> > +examples:
> > + - |
> > + i2c {
> > + #address-cells = <1>;
>
> Use 4 spaces for example indentation. 2 is also fine.
Sure. Btw, this is copy&pasted from the existing YAML. Are you going to
fix them?
> > + #size-cells = <0>;
> > +
> > + max6959: max6959@38 {
>
> Node names should be generic. See also an explanation and list of
(Same remark: it's a pattern from the existing code. Are you going to fix
that?)
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> e.g. display-controller or display
Sure, thanks for review!
> > + compatible = "maxim,max6959";
> > + reg = <0x38>;
> > + };
> > + };
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959
2024-02-09 13:59 ` Andy Shevchenko
@ 2024-02-12 8:24 ` Krzysztof Kozlowski
0 siblings, 0 replies; 24+ messages in thread
From: Krzysztof Kozlowski @ 2024-02-12 8:24 UTC (permalink / raw)
To: Andy Shevchenko
Cc: devicetree, linux-kernel, Miguel Ojeda, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Robin van der Gracht,
Paul Burton, Geert Uytterhoeven
On 09/02/2024 14:59, Andy Shevchenko wrote:
> On Fri, Feb 09, 2024 at 09:02:44AM +0100, Krzysztof Kozlowski wrote:
>> On 08/02/2024 19:48, Andy Shevchenko wrote:
>>> Add initial device tree documentation for Maxim MAX6958/6959.
>
> ...
>
>> Please describe the device, e.g. bus/interface.
>
> OK.
>
> ...
>
>>> +properties:
>>> + compatible:
>>> + const: maxim,max6959
>>
>> Your title said also max6958, so I would expect it to be here as well.
>> Cam be followed by 6959 fallback compatible, if they are compatible.
>
> Same question as I asked before, why should we have them separated?
> The hardware features can be autodetected. What's the reason for (unneeded
> in my opinion and duplicative) compatible?
And which part of device description in the binding, or at least commit
msg but better description, explained it?
For every unexplained deviation from common rules - and documenting
compatibles is explicitly asked in writing bindings document - you will
get questions from reviewers...
Please add this information to description.
>
> ...
>
>>> + reg:
>>> + maxItems: 1
>>
>> No power supplies? No reset pins?
>
> No power supplies, no reset pins. At least there is no as such in
> the datasheet. Do you see them there?
How do I know? I don't have datasheets and I don't have really time to
investigate each datasheet of every device people send bindings for.
Several people make mistakes of not putting such stuff because "driver
does not need it", so how can I know that here it was not the case?
>
> ...
>
>>> +examples:
>>> + - |
>>> + i2c {
>>> + #address-cells = <1>;
>>
>> Use 4 spaces for example indentation. 2 is also fine.
>
> Sure. Btw, this is copy&pasted from the existing YAML. Are you going to
> fix them?
I fixed several of them. At some point I might fix all of them, but
that's lower priority. I wished I have all the time for this :)
>
>>> + #size-cells = <0>;
>>> +
>>> + max6959: max6959@38 {
>>
>> Node names should be generic. See also an explanation and list of
>
> (Same remark: it's a pattern from the existing code. Are you going to fix
> that?)
>
Same answer.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v1 15/15] auxdisplay: Add driver for MAX695x 7-segment LED controllers
2024-02-08 18:47 [resend, PATCH v1 00/15] auxdisplay: linedisp: Clean up and add new driver Andy Shevchenko
` (13 preceding siblings ...)
2024-02-08 18:48 ` [PATCH v1 14/15] dt-bindings: auxdisplay: Add Maxim MAX6958/6959 Andy Shevchenko
@ 2024-02-08 18:48 ` Andy Shevchenko
14 siblings, 0 replies; 24+ messages in thread
From: Andy Shevchenko @ 2024-02-08 18:48 UTC (permalink / raw)
To: Andy Shevchenko, devicetree, linux-kernel
Cc: Miguel Ojeda, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Robin van der Gracht, Paul Burton, Geert Uytterhoeven
Add initial driver for the MAX6958 and MAX6959 7-segment LED
controllers.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/auxdisplay/Kconfig | 14 +++
drivers/auxdisplay/Makefile | 1 +
drivers/auxdisplay/max6959.c | 200 +++++++++++++++++++++++++++++++++++
3 files changed, 215 insertions(+)
create mode 100644 drivers/auxdisplay/max6959.c
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index a34a9a52158f..079d58bb0293 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -187,6 +187,20 @@ config HT16K33
Say yes here to add support for Holtek HT16K33, RAM mapping 16*8
LED controller driver with keyscan.
+config MAX6959
+ tristate "Maxim MAX6958/6959 7-segment LED controller with keyscan"
+ 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 with keyscan):
+ - MAX6958
+ - MAX6959 (debounce support)
+
+ This driver can also be built as a module. If so, the module
+ will be called max6959.
+
config ARM_CHARLCD
bool "ARM Ltd. Character LCD Driver"
depends on PLAT_VERSATILE
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index 43bad850481c..f62a258809ef 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -12,5 +12,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
obj-$(CONFIG_PARPORT_PANEL) += panel.o
diff --git a/drivers/auxdisplay/max6959.c b/drivers/auxdisplay/max6959.c
new file mode 100644
index 000000000000..0c5cbd16c3fe
--- /dev/null
+++ b/drivers/auxdisplay/max6959.c
@@ -0,0 +1,200 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * MAX6958/6959 7-segment LED display controller with keyscan
+ * Datasheet:
+ * https://www.analog.com/media/en/technical-documentation/data-sheets/MAX6958-MAX6959.pdf
+ *
+ * Copyright (c) 2024, Intel Corporation.
+ * Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
+ */
+#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
+
+/* Defines */
+#define MIN_BRIGHTNESS 0x01
+#define MAX_BRIGHTNESS 0x40
+
+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->curr;
+ 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, NULL, &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 with keyscan");
+MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LINEDISP);
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related [flat|nested] 24+ messages in thread