| Message ID | 20210322144848.1065067-1-geert@linux-m68k.org |
|---|---|
| Headers | show |
| Series | auxdisplay: ht16k33: Add character display support | expand |
On Mon, Mar 22, 2021 at 3:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > You can find an image of the full character set at > https://drive.google.com/file/d/1h3EYFBWHIjh8B_cwPA5ocAD-lFYipRie/view Should we put it in e.g. the Docs or just alongside this file? Cheers, Miguel
On 2021-03-22 15:48, Geert Uytterhoeven wrote: > This driver has many users of "client->dev". Add shorthands to > simplify > the code. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > drivers/auxdisplay/ht16k33.c | 43 ++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/drivers/auxdisplay/ht16k33.c > b/drivers/auxdisplay/ht16k33.c > index 1b67f38109bddba8..37fca1d44c3e73e1 100644 > --- a/drivers/auxdisplay/ht16k33.c > +++ b/drivers/auxdisplay/ht16k33.c > @@ -316,7 +316,8 @@ static void ht16k33_keypad_stop(struct input_dev > *dev) > static int ht16k33_keypad_probe(struct i2c_client *client, > struct ht16k33_keypad *keypad) > { > - struct device_node *node = client->dev.of_node; > + struct device *dev = &client->dev; > + struct device_node *node = dev->of_node; > u32 rows = HT16K33_MATRIX_KEYPAD_MAX_ROWS; > u32 cols = HT16K33_MATRIX_KEYPAD_MAX_COLS; > int err; > @@ -324,7 +325,7 @@ static int ht16k33_keypad_probe(struct i2c_client > *client, > keypad->client = client; > init_waitqueue_head(&keypad->wait); > > - keypad->dev = devm_input_allocate_device(&client->dev); > + keypad->dev = devm_input_allocate_device(dev); > if (!keypad->dev) > return -ENOMEM; > > @@ -341,17 +342,17 @@ static int ht16k33_keypad_probe(struct i2c_client > *client, > err = of_property_read_u32(node, "debounce-delay-ms", > &keypad->debounce_ms); > if (err) { > - dev_err(&client->dev, "key debounce delay not specified\n"); > + dev_err(dev, "key debounce delay not specified\n"); > return err; > } > > - err = matrix_keypad_parse_of_params(&client->dev, &rows, &cols); > + err = matrix_keypad_parse_of_params(dev, &rows, &cols); > if (err) > return err; > if (rows > HT16K33_MATRIX_KEYPAD_MAX_ROWS || > cols > HT16K33_MATRIX_KEYPAD_MAX_COLS) { > - dev_err(&client->dev, "%u rows or %u cols out of range in DT\n", > - rows, cols); > + dev_err(dev, "%u rows or %u cols out of range in DT\n", rows, > + cols); > return -ERANGE; > } > > @@ -362,17 +363,17 @@ static int ht16k33_keypad_probe(struct i2c_client > *client, > err = matrix_keypad_build_keymap(NULL, NULL, rows, cols, NULL, > keypad->dev); > if (err) { > - dev_err(&client->dev, "failed to build keymap\n"); > + dev_err(dev, "failed to build keymap\n"); > return err; > } > > - err = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, ht16k33_keypad_irq_thread, > + err = devm_request_threaded_irq(dev, client->irq, NULL, > + ht16k33_keypad_irq_thread, > IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > DRIVER_NAME, keypad); > if (err) { > - dev_err(&client->dev, "irq request failed %d, error %d\n", > - client->irq, err); > + dev_err(dev, "irq request failed %d, error %d\n", client->irq, > + err); > return err; > } > > @@ -389,14 +390,15 @@ static int ht16k33_probe(struct i2c_client > *client) > struct backlight_properties bl_props; > struct ht16k33_priv *priv; > struct ht16k33_fbdev *fbdev; > - struct device_node *node = client->dev.of_node; > + struct device *dev = &client->dev; > + struct device_node *node = dev->of_node; > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > - dev_err(&client->dev, "i2c_check_functionality error\n"); > + dev_err(dev, "i2c_check_functionality error\n"); > return -EIO; > } > > - priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > @@ -414,13 +416,13 @@ static int ht16k33_probe(struct i2c_client > *client) > if (!fbdev->buffer) > return -ENOMEM; > > - fbdev->cache = devm_kmalloc(&client->dev, HT16K33_FB_SIZE, > GFP_KERNEL); > + fbdev->cache = devm_kmalloc(dev, HT16K33_FB_SIZE, GFP_KERNEL); > if (!fbdev->cache) { > err = -ENOMEM; > goto err_fbdev_buffer; > } > > - fbdev->info = framebuffer_alloc(0, &client->dev); > + fbdev->info = framebuffer_alloc(0, dev); > if (!fbdev->info) { > err = -ENOMEM; > goto err_fbdev_buffer; > @@ -429,7 +431,7 @@ static int ht16k33_probe(struct i2c_client *client) > err = of_property_read_u32(node, "refresh-rate-hz", > &fbdev->refresh_rate); > if (err) { > - dev_err(&client->dev, "refresh rate not specified\n"); > + dev_err(dev, "refresh rate not specified\n"); > goto err_fbdev_info; > } > fb_bl_default_curve(fbdev->info, 0, MIN_BRIGHTNESS, MAX_BRIGHTNESS); > @@ -460,11 +462,10 @@ static int ht16k33_probe(struct i2c_client > *client) > bl_props.type = BACKLIGHT_RAW; > bl_props.max_brightness = MAX_BRIGHTNESS; > > - bl = devm_backlight_device_register(&client->dev, DRIVER_NAME"-bl", > - &client->dev, priv, > + bl = devm_backlight_device_register(dev, DRIVER_NAME"-bl", dev, priv, > &ht16k33_bl_ops, &bl_props); > if (IS_ERR(bl)) { > - dev_err(&client->dev, "failed to register backlight\n"); > + dev_err(dev, "failed to register backlight\n"); > err = PTR_ERR(bl); > goto err_fbdev_unregister; > } > @@ -474,7 +475,7 @@ static int ht16k33_probe(struct i2c_client *client) > if (err) { > dft_brightness = MAX_BRIGHTNESS; > } else if (dft_brightness > MAX_BRIGHTNESS) { > - dev_warn(&client->dev, > + dev_warn(dev, > "invalid default brightness level: %u, using %u\n", > dft_brightness, MAX_BRIGHTNESS); > dft_brightness = MAX_BRIGHTNESS; Acked-by: Robin van der Gracht <robin@protonic.nl>
On 2021-03-22 15:48, Geert Uytterhoeven wrote: > The Holtek HT16K33 LED controller is not only used for driving > dot-matrix displays, but also for driving segment displays. > > Document compatible values for the Adafruit 7-segment[1] and > 14-segment[2] FeatherWing expansion boards with red displays. > According > to the schematics, all other Adafruit 7-segment and 14-segment display > backpack and FeatherWing expansion boards (including bare boards and > boards fitted with displays) are compatible with these two boards. > Add a "color" property to support the different color variants. > > [1] https://www.adafruit.com/product/3108 > [2] https://www.adafruit.com/product/3130 > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > Alternatives I considered: > 1. Document the attached display type in a child node. > I.e. specify segment type, number of characters, and wiring. > Especially the latter would become really complex, due to the > sheer > amount of possible wiring combinations. > Using this method, you also loose the ability to just connect a > display to an i2c bus, and instantiate the device from sysfs, > without using DT: > > echo adafruit,3130 0x70 > /sys/class/i2c/i2c-adapter/.../new_device > > 2. Do not use the "color" property, but document all Adafruit > 7-segment and 14-segment display backpack and FeatherWing > expansion > boards. > This would lead to a myriad of compatible values: > > - items: > - enum: > - adafruit,878 # 0.56" 4-Digit 7-Segment Display Backpack > (Red) > - adafruit,879 # 0.56" 4-Digit 7-Segment Display Backpack > (Yellow) > - adafruit,880 # 0.56" 4-Digit 7-Segment Display Backpack > (Green) > - adafruit,881 # 0.56" 4-Digit 7-Segment Display Backpack > (Blue) > - adafruit,1002 # 0.56" 4-Digit 7-Segment Display Backpack > (White) > - const: adafruit,877 # 0.56" 4-Digit 7-Segment Backpack > - const: holtek,ht16k33 > > - items: > - enum: > - adafruit,1268 # 1.2" 4-Digit 7-Segment Display Backpack > (Green) > - adafruit,1269 # 1.2" 4-Digit 7-Segment Display Backpack > (Yellow) > - adafruit,1270 # 1.2" 4-Digit 7-Segment Display Backpack > (Red) > - const: adafruit,1271 # 1.2" 4-Digit 7-Segment Backpack > - const: holtek,ht16k33 > > - items: > - enum: > - adafruit,1911 # 0.54" Quad Alphanumeric Display Backpack > (Red) > - adafruit,1912 # 0.54" Quad Alphanumeric Display Backpack > (Blue) > - adafruit,2157 # 0.54" Quad Alphanumeric Display Backpack > (White) > - adafruit,2158 # 0.54" Quad Alphanumeric Display Backpack > (Yellow) > - adafruit,2159 # 0.54" Quad Alphanumeric Display Backpack > (Yellow-Green) > - adafruit,2160 # 0.54" Quad Alphanumeric Display Backpack > (Green) > - const: adafruit,1910 # 0.54" Quad 14-segment Alphanumeric > Backpack > - const: holtek,ht16k33 > > - items: > - enum: > - adafruit,3106 # 0.56" 4-Digit 7-Segment FeatherWing > Display (Blue) > - adafruit,3107 # 0.56" 4-Digit 7-Segment FeatherWing > Display (Green) > - adafruit,3108 # 0.56" 4-Digit 7-Segment FeatherWing > Display (Red) > - adafruit,3109 # 0.56" 4-Digit 7-Segment FeatherWing > Display (White) > - adafruit,3110 # 0.56" 4-Digit 7-Segment FeatherWing > Display (Yellow) > - const: adafruit,3088 # 0.56" 4-Digit 7-Segment FeatherWing > - const: holtek,ht16k33 > > - items: > - enum: > - adafruit,3127 # 0.54" Quad Alphanumeric FeatherWing > Display (White) > - adafruit,3128 # 0.54" Quad Alphanumeric FeatherWing > Display (Blue) > - adafruit,3129 # 0.54" Quad Alphanumeric FeatherWing > Display (Green) > - adafruit,3130 # 0.54" Quad Alphanumeric FeatherWing > Display (Red) > - adafruit,3131 # 0.54" Quad Alphanumeric FeatherWing > Display (Yellow) > - adafruit,3132 # 0.54" Quad Alphanumeric FeatherWing > Display (Yellow-Green) > - const: adafruit,3089 # 0.54" Quad 14-segment Alphanumeric > FeatherWing > - const: holtek,ht16k33 > --- > .../bindings/auxdisplay/holtek,ht16k33.yaml | 22 ++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > index 64ffff460026040f..4380a5177a67d2e2 100644 > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > @@ -14,14 +14,23 @@ allOf: > > properties: > compatible: > - const: holtek,ht16k33 > + oneOf: > + - items: > + - const: adafruit,3108 # 0.56" 4-Digit 7-Segment > FeatherWing Display (Red) > + - const: holtek,ht16k33 > + > + - items: > + - const: adafruit,3130 # 0.54" Quad Alphanumeric > FeatherWing Display (Red) > + - const: holtek,ht16k33 > + > + - const: holtek,ht16k33 # Generic 16*8 LED controller with > dot-matrix display > > reg: > maxItems: 1 > > refresh-rate-hz: > maxItems: 1 > - description: Display update interval in Hertz > + description: Display update interval in Hertz for dot-matrix > displays The above should be included in patch 16 > > interrupts: > maxItems: 1 > @@ -41,10 +50,17 @@ properties: > default: 16 > description: Initial brightness level > > + color: true > + description: > + Color of the display. Use one of the LED_COLOR_ID_* prefixed > definitions > + from the header include/dt-bindings/leds/common.h. The default > is red. > + minimum: 0 > + maximum: 9 > + default: 1 > + The above should be included in patch 17 > required: > - compatible > - reg > - - refresh-rate-hz 'refresh-rate-hz' is still a required property for the dot-matrix / fbdev setup. If it can no longer be listed here than maybe its nice to mention that it's required somewhere else (in it's description?). Rob? Groetjes / Kind regards, Robin van der Gracht
On 2021-03-22 15:48, Geert Uytterhoeven wrote: > Instantiate a single LED for a segment display. This allows the user > to > control display brightness and blinking through the LED class API and > triggers, and exposes the display color. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > For setting display brightness, this could use the existing backlight > support for frame buffer devices instantiated for dot-matrix displays. > However, using the leds subsystem instead has the advantage that the > driver can make use of the HT16K33's hardware blinking support, and can > expose the display color. > --- > drivers/auxdisplay/ht16k33.c | 81 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 77 insertions(+), 4 deletions(-) > > diff --git a/drivers/auxdisplay/ht16k33.c > b/drivers/auxdisplay/ht16k33.c > index 0b502a6039f89c6b..5821addd9aec5633 100644 > --- a/drivers/auxdisplay/ht16k33.c > +++ b/drivers/auxdisplay/ht16k33.c > @@ -29,6 +29,7 @@ > #include <asm/unaligned.h> > > #include "line-display.h" > +#include "../leds/leds.h" /* for led_colors[] */ > > /* Registers */ > #define REG_SYSTEM_SETUP 0x20 > @@ -36,6 +37,10 @@ > > #define REG_DISPLAY_SETUP 0x80 > #define REG_DISPLAY_SETUP_ON BIT(0) > +#define REG_DISPLAY_SETUP_BLINK_OFF (0 << 1) > +#define REG_DISPLAY_SETUP_BLINK_2HZ (1 << 1) > +#define REG_DISPLAY_SETUP_BLINK_1HZ (2 << 1) > +#define REG_DISPLAY_SETUP_BLINK_0HZ5 (3 << 1) > > #define REG_ROWINT_SET 0xA0 > #define REG_ROWINT_SET_INT_EN BIT(0) > @@ -57,6 +62,8 @@ > #define BYTES_PER_ROW (HT16K33_MATRIX_LED_MAX_ROWS / 8) > #define HT16K33_FB_SIZE (HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW) > > +#define COLOR_DEFAULT LED_COLOR_ID_RED > + > enum display_type { > DISP_MATRIX = 0, > DISP_QUAD_7SEG, > @@ -85,6 +92,7 @@ struct ht16k33_fbdev { > > struct ht16k33_seg { > struct linedisp linedisp; > + struct led_classdev led; > union { > struct seg7_conversion_map seg7; > struct seg14_conversion_map seg14; > @@ -102,6 +110,7 @@ struct ht16k33_priv { > struct ht16k33_seg seg; > }; > enum display_type type; > + uint8_t blink; > }; > > static const struct fb_fix_screeninfo ht16k33_fb_fix = { > @@ -160,7 +169,7 @@ 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; > + uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | > priv->blink; > > return i2c_smbus_write_byte(priv->client, data); > } > @@ -175,8 +184,11 @@ static int ht16k33_brightness_set(struct > ht16k33_priv *priv, > { > int error; > > - if (brightness == 0) > + if (brightness == 0) { > + // Disable blink mode > + priv->blink = REG_DISPLAY_SETUP_BLINK_OFF; > return ht16k33_display_off(priv); > + } > > error = ht16k33_display_on(priv); > if (error) > @@ -186,6 +198,49 @@ static int ht16k33_brightness_set(struct > ht16k33_priv *priv, > REG_BRIGHTNESS | (brightness - 1)); > } > > +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, > + seg.led); > + > + return ht16k33_brightness_set(priv, brightness); > +} > + > +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, > + seg.led); > + unsigned int delay; > + uint8_t blink; > + int error; > + > + if (!*delay_on && !*delay_off) { > + blink = REG_DISPLAY_SETUP_BLINK_1HZ; > + delay = 1000; > + } else if (*delay_on <= 750) { > + blink = REG_DISPLAY_SETUP_BLINK_2HZ; > + delay = 500; > + } else if (*delay_on <= 1500) { > + blink = REG_DISPLAY_SETUP_BLINK_1HZ; > + delay = 1000; > + } else { > + blink = REG_DISPLAY_SETUP_BLINK_0HZ5; > + delay = 2000; > + } > + > + error = i2c_smbus_write_byte(priv->client, > + REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | > + blink); > + if (error) > + return error; > + > + priv->blink = blink; > + *delay_on = *delay_off = delay; > + return 0; > +} > + > static void ht16k33_fb_queue(struct ht16k33_priv *priv) > { > struct ht16k33_fbdev *fbdev = &priv->fbdev; > @@ -578,11 +633,29 @@ static int ht16k33_fbdev_probe(struct i2c_client > *client, > static int ht16k33_seg_probe(struct i2c_client *client, > struct ht16k33_priv *priv, uint32_t brightness) > { > - struct ht16k33_seg *seg = &priv->seg; > struct device *dev = &client->dev; > + struct device_node *node = dev->of_node; > + struct ht16k33_seg *seg = &priv->seg; > + u32 color = COLOR_DEFAULT; > int err; > > - err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > + of_property_read_u32(node, "color", &color); > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > + DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT, > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > + seg->led.brightness_set_blocking = ht16k33_brightness_set_blocking; > + seg->led.blink_set = ht16k33_blink_set; > + seg->led.flags = LED_CORE_SUSPENDRESUME; > + seg->led.brightness = brightness; > + seg->led.max_brightness = MAX_BRIGHTNESS; > + > + err = devm_led_classdev_register(dev, &seg->led); > + if (err) { > + dev_err(dev, "Failed to register LED\n"); > + return err; > + } > + > + err = ht16k33_brightness_set(priv, seg->led.brightness); > if (err) > return err; The LED class can pretty much do what the backlight class can and more. Maybe we can stop registering a backlight device in the fbdev case and register a led device for both. This makes the code cleaner and drops a dependency but will break backwards compatibility. I'd prefer a single solution that covers both use cases, but I'm not sure about the 'breaking backwards compatibility' consequence... Groetjes / Kind regards, Robin van der Gracht
CC linux-leds (which I intended, but forgot to add) cover letter at https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/ On Tue, Mar 23, 2021 at 11:08 AM Robin van der Gracht <robin@protonic.nl> wrote: > > On 2021-03-22 15:48, Geert Uytterhoeven wrote: > > Instantiate a single LED for a segment display. This allows the user > > to > > control display brightness and blinking through the LED class API and > > triggers, and exposes the display color. > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > For setting display brightness, this could use the existing backlight > > support for frame buffer devices instantiated for dot-matrix displays. > > However, using the leds subsystem instead has the advantage that the > > driver can make use of the HT16K33's hardware blinking support, and can > > expose the display color. > > --- > > drivers/auxdisplay/ht16k33.c | 81 ++++++++++++++++++++++++++++++++++-- > > 1 file changed, 77 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/auxdisplay/ht16k33.c > > b/drivers/auxdisplay/ht16k33.c > > index 0b502a6039f89c6b..5821addd9aec5633 100644 > > --- a/drivers/auxdisplay/ht16k33.c > > +++ b/drivers/auxdisplay/ht16k33.c > > @@ -29,6 +29,7 @@ > > #include <asm/unaligned.h> > > > > #include "line-display.h" > > +#include "../leds/leds.h" /* for led_colors[] */ > > > > /* Registers */ > > #define REG_SYSTEM_SETUP 0x20 > > @@ -36,6 +37,10 @@ > > > > #define REG_DISPLAY_SETUP 0x80 > > #define REG_DISPLAY_SETUP_ON BIT(0) > > +#define REG_DISPLAY_SETUP_BLINK_OFF (0 << 1) > > +#define REG_DISPLAY_SETUP_BLINK_2HZ (1 << 1) > > +#define REG_DISPLAY_SETUP_BLINK_1HZ (2 << 1) > > +#define REG_DISPLAY_SETUP_BLINK_0HZ5 (3 << 1) > > > > #define REG_ROWINT_SET 0xA0 > > #define REG_ROWINT_SET_INT_EN BIT(0) > > @@ -57,6 +62,8 @@ > > #define BYTES_PER_ROW (HT16K33_MATRIX_LED_MAX_ROWS / 8) > > #define HT16K33_FB_SIZE (HT16K33_MATRIX_LED_MAX_COLS * BYTES_PER_ROW) > > > > +#define COLOR_DEFAULT LED_COLOR_ID_RED > > + > > enum display_type { > > DISP_MATRIX = 0, > > DISP_QUAD_7SEG, > > @@ -85,6 +92,7 @@ struct ht16k33_fbdev { > > > > struct ht16k33_seg { > > struct linedisp linedisp; > > + struct led_classdev led; > > union { > > struct seg7_conversion_map seg7; > > struct seg14_conversion_map seg14; > > @@ -102,6 +110,7 @@ struct ht16k33_priv { > > struct ht16k33_seg seg; > > }; > > enum display_type type; > > + uint8_t blink; > > }; > > > > static const struct fb_fix_screeninfo ht16k33_fb_fix = { > > @@ -160,7 +169,7 @@ 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; > > + uint8_t data = REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | > > priv->blink; > > > > return i2c_smbus_write_byte(priv->client, data); > > } > > @@ -175,8 +184,11 @@ static int ht16k33_brightness_set(struct > > ht16k33_priv *priv, > > { > > int error; > > > > - if (brightness == 0) > > + if (brightness == 0) { > > + // Disable blink mode > > + priv->blink = REG_DISPLAY_SETUP_BLINK_OFF; > > return ht16k33_display_off(priv); > > + } > > > > error = ht16k33_display_on(priv); > > if (error) > > @@ -186,6 +198,49 @@ static int ht16k33_brightness_set(struct > > ht16k33_priv *priv, > > REG_BRIGHTNESS | (brightness - 1)); > > } > > > > +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, > > + seg.led); > > + > > + return ht16k33_brightness_set(priv, brightness); > > +} > > + > > +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, > > + seg.led); > > + unsigned int delay; > > + uint8_t blink; > > + int error; > > + > > + if (!*delay_on && !*delay_off) { > > + blink = REG_DISPLAY_SETUP_BLINK_1HZ; > > + delay = 1000; > > + } else if (*delay_on <= 750) { > > + blink = REG_DISPLAY_SETUP_BLINK_2HZ; > > + delay = 500; > > + } else if (*delay_on <= 1500) { > > + blink = REG_DISPLAY_SETUP_BLINK_1HZ; > > + delay = 1000; > > + } else { > > + blink = REG_DISPLAY_SETUP_BLINK_0HZ5; > > + delay = 2000; > > + } > > + > > + error = i2c_smbus_write_byte(priv->client, > > + REG_DISPLAY_SETUP | REG_DISPLAY_SETUP_ON | > > + blink); > > + if (error) > > + return error; > > + > > + priv->blink = blink; > > + *delay_on = *delay_off = delay; > > + return 0; > > +} > > + > > static void ht16k33_fb_queue(struct ht16k33_priv *priv) > > { > > struct ht16k33_fbdev *fbdev = &priv->fbdev; > > @@ -578,11 +633,29 @@ static int ht16k33_fbdev_probe(struct i2c_client > > *client, > > static int ht16k33_seg_probe(struct i2c_client *client, > > struct ht16k33_priv *priv, uint32_t brightness) > > { > > - struct ht16k33_seg *seg = &priv->seg; > > struct device *dev = &client->dev; > > + struct device_node *node = dev->of_node; > > + struct ht16k33_seg *seg = &priv->seg; > > + u32 color = COLOR_DEFAULT; > > int err; > > > > - err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > > + of_property_read_u32(node, "color", &color); > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > + DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT, > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > > + seg->led.brightness_set_blocking = ht16k33_brightness_set_blocking; > > + seg->led.blink_set = ht16k33_blink_set; > > + seg->led.flags = LED_CORE_SUSPENDRESUME; > > + seg->led.brightness = brightness; > > + seg->led.max_brightness = MAX_BRIGHTNESS; > > + > > + err = devm_led_classdev_register(dev, &seg->led); > > + if (err) { > > + dev_err(dev, "Failed to register LED\n"); > > + return err; > > + } > > + > > + err = ht16k33_brightness_set(priv, seg->led.brightness); > > if (err) > > return err; > > The LED class can pretty much do what the backlight class can and more. > > Maybe we can stop registering a backlight device in the fbdev case and > register a led device for both. This makes the code cleaner and drops > a dependency but will break backwards compatibility. > > I'd prefer a single solution that covers both use cases, but I'm not > sure about the 'breaking backwards compatibility' consequence... > > Groetjes / Kind regards, > Robin van der Gracht
Hi! > CC linux-leds (which I intended, but forgot to add) > > cover letter at > https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/ Still does not tell me... riscv on fpga with 4 character display. What is this? :-). > On Tue, Mar 23, 2021 at 11:08 AM Robin van der Gracht <robin@protonic.nl> wrote: > > > > On 2021-03-22 15:48, Geert Uytterhoeven wrote: > > > Instantiate a single LED for a segment display. This allows the user > > > to > > > control display brightness and blinking through the LED class API and > > > triggers, and exposes the display color. > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > --- > > > For setting display brightness, this could use the existing backlight > > > support for frame buffer devices instantiated for dot-matrix displays. > > > However, using the leds subsystem instead has the advantage that the > > > driver can make use of the HT16K33's hardware blinking support, and can > > > expose the display color. We have multicolor support now... > > > - err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > > > + of_property_read_u32(node, "color", &color); > > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > > + DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT, > > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); And would prefer not to see driver_name as part of LED name. > > > + err = ht16k33_brightness_set(priv, seg->led.brightness); > > > if (err) > > > return err; > > > > The LED class can pretty much do what the backlight class can and more. > > > > Maybe we can stop registering a backlight device in the fbdev case and > > register a led device for both. This makes the code cleaner and drops > > a dependency but will break backwards compatibility. > > > > I'd prefer a single solution that covers both use cases, but I'm not > > sure about the 'breaking backwards compatibility' consequence... For new drivers, breaking compatibility should not be a problem. Best regards, Pavel
Hi Pavel, On Tue, Mar 23, 2021 at 9:40 PM Pavel Machek <pavel@ucw.cz> wrote: > > CC linux-leds (which I intended, but forgot to add) > > > > cover letter at > > https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/ > > Still does not tell me... riscv on fpga with 4 character display. What > is this? :-). https://gregdavill.github.io/OrangeCrab/r0.2/ https://github.com/litex-hub/linux-on-litex-vexriscv https://www.adafruit.com/product/3130 > > On Tue, Mar 23, 2021 at 11:08 AM Robin van der Gracht <robin@protonic.nl> wrote: > > > On 2021-03-22 15:48, Geert Uytterhoeven wrote: > > > > Instantiate a single LED for a segment display. This allows the user > > > > to > > > > control display brightness and blinking through the LED class API and > > > > triggers, and exposes the display color. > > > > > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > > --- > > > > For setting display brightness, this could use the existing backlight > > > > support for frame buffer devices instantiated for dot-matrix displays. > > > > However, using the leds subsystem instead has the advantage that the > > > > driver can make use of the HT16K33's hardware blinking support, and can > > > > expose the display color. > > We have multicolor support now... The color is fixed by the LED color, so there is no need for multicolor (yet, one day someone might connect RGB 7-segment LED displays (https://www.rgbdigit.com/rgbdigit/ ;-) > > > > - err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > > > > + of_property_read_u32(node, "color", &color); > > > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > > > + DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT, > > > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > > And would prefer not to see driver_name as part of LED name. OK. So what should I use instead? Or just leave the part before the first colon empty? > > > > + err = ht16k33_brightness_set(priv, seg->led.brightness); > > > > if (err) > > > > return err; > > > > > > The LED class can pretty much do what the backlight class can and more. > > > > > > Maybe we can stop registering a backlight device in the fbdev case and > > > register a led device for both. This makes the code cleaner and drops > > > a dependency but will break backwards compatibility. > > > > > > I'd prefer a single solution that covers both use cases, but I'm not > > > sure about the 'breaking backwards compatibility' consequence... > > For new drivers, breaking compatibility should not be a problem. The dot-matrix support is part of the existing driver, thus subject to backwards compatibility. Perhaps we can register the LED device for both, and build the backlight device on top of the LED device, like "led-backlight" does. Would that work? Or can't the LED no longer be controlled from sysfs (e.g. triggers) if it is in use by a backlight driver? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi! > > > > > - err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > > > > > + of_property_read_u32(node, "color", &color); > > > > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > > > > + DRIVER_NAME ":%s:" LED_FUNCTION_BACKLIGHT, > > > > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > > > > And would prefer not to see driver_name as part of LED name. > > OK. So what should I use instead? Or just leave the part before the > first colon empty? I'd suggest auxdisplay:...:backlight. And we should start documenting this somewhere. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek
Hello Geert, On 2021-03-22 15:48, Geert Uytterhoeven wrote: > The Holtek HT16K33 LED controller is not only used for driving > dot-matrix displays, but also for driving segment displays. > > Add support for 4-digit 7-segment and quad 14-segment alphanumeric > displays, like the Adafruit 7-segment and 14-segment display backpack > and FeatherWing expansion boards. Use the character line display core > support to display a message, which will be scrolled if it doesn't fit. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > The 7-segment support is based on schematics, and untested on actual > hardware. > --- > drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++-- > 1 file changed, 191 insertions(+), 7 deletions(-) > ... > > +static int ht16k33_seg_probe(struct i2c_client *client, > + struct ht16k33_priv *priv, uint32_t brightness) > +{ > + struct ht16k33_seg *seg = &priv->seg; > + struct device *dev = &client->dev; > + int err; > + > + err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > + if (err) > + return err; > + > + switch (priv->type) { > + case DISP_MATRIX: > + /* not handled here */ > + break; This 'case' shouldn't happen. Having said that, the break here will still cause the linedisp_register() function to be called for the DISP_MATRIX type. If you'd like to handle this case, a return (or setting 'err') should prevent this. > + > + 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_update); > + if (err) > + goto err_remove_map_file; > + > + return 0; Groetjes/Kind regards, Robin van der Gracht
Hoi Robin, On Mon, Mar 29, 2021 at 9:09 AM Robin van der Gracht <robin@protonic.nl> wrote: > On 2021-03-22 15:48, Geert Uytterhoeven wrote: > > The Holtek HT16K33 LED controller is not only used for driving > > dot-matrix displays, but also for driving segment displays. > > > > Add support for 4-digit 7-segment and quad 14-segment alphanumeric > > displays, like the Adafruit 7-segment and 14-segment display backpack > > and FeatherWing expansion boards. Use the character line display core > > support to display a message, which will be scrolled if it doesn't fit. > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > The 7-segment support is based on schematics, and untested on actual > > hardware. > > --- > > drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++-- > > 1 file changed, 191 insertions(+), 7 deletions(-) > > > ... > > > > +static int ht16k33_seg_probe(struct i2c_client *client, > > + struct ht16k33_priv *priv, uint32_t brightness) > > +{ > > + struct ht16k33_seg *seg = &priv->seg; > > + struct device *dev = &client->dev; > > + int err; > > + > > + err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); > > + if (err) > > + return err; > > + > > + switch (priv->type) { > > + case DISP_MATRIX: > > + /* not handled here */ > > + break; > > This 'case' shouldn't happen. Having said that, the break here will > still > cause the linedisp_register() function to be called for the DISP_MATRIX > type. > If you'd like to handle this case, a return (or setting 'err') should > prevent this. This function is never called if priv->type == DISP_MATRIX, so this cannot happen. However, gcc complains if not all enum values are handled in a switch() statement, hence the dummy case. Is there a better way to handle this? > > + 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_update); > > + if (err) > > + goto err_remove_map_file; > > + > > + return 0; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 2021-03-29 09:15, Geert Uytterhoeven wrote: > Hoi Robin, > > On Mon, Mar 29, 2021 at 9:09 AM Robin van der Gracht > <robin@protonic.nl> wrote: >> On 2021-03-22 15:48, Geert Uytterhoeven wrote: >> > The Holtek HT16K33 LED controller is not only used for driving >> > dot-matrix displays, but also for driving segment displays. >> > >> > Add support for 4-digit 7-segment and quad 14-segment alphanumeric >> > displays, like the Adafruit 7-segment and 14-segment display backpack >> > and FeatherWing expansion boards. Use the character line display core >> > support to display a message, which will be scrolled if it doesn't fit. >> > >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >> > --- >> > The 7-segment support is based on schematics, and untested on actual >> > hardware. >> > --- >> > drivers/auxdisplay/ht16k33.c | 198 +++++++++++++++++++++++++++++++++-- >> > 1 file changed, 191 insertions(+), 7 deletions(-) >> > >> ... >> > >> > +static int ht16k33_seg_probe(struct i2c_client *client, >> > + struct ht16k33_priv *priv, uint32_t brightness) >> > +{ >> > + struct ht16k33_seg *seg = &priv->seg; >> > + struct device *dev = &client->dev; >> > + int err; >> > + >> > + err = ht16k33_brightness_set(priv, MAX_BRIGHTNESS); >> > + if (err) >> > + return err; >> > + >> > + switch (priv->type) { >> > + case DISP_MATRIX: >> > + /* not handled here */ >> > + break; >> >> This 'case' shouldn't happen. Having said that, the break here will >> still >> cause the linedisp_register() function to be called for the >> DISP_MATRIX >> type. >> If you'd like to handle this case, a return (or setting 'err') should >> prevent this. > > This function is never called if priv->type == DISP_MATRIX, so this > cannot happen. However, gcc complains if not all enum values are > handled in a switch() statement, hence the dummy case. I see. But if it's there it should work right? :P > Is there a better way to handle this? How about adding the default case: default: /* not handled */ err = -EINVAL; Groetjes/Kind regards, Robin van der Gracht -- Protonic Holland Factorij 36 1689AL Zwaag +31 (0)229 212928 https://www.protonic.nl
Hoi Robin, On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote: > On 2021-03-22 15:48, Geert Uytterhoeven wrote: > > The Holtek HT16K33 LED controller is not only used for driving > > dot-matrix displays, but also for driving segment displays. > > > > Document compatible values for the Adafruit 7-segment[1] and > > 14-segment[2] FeatherWing expansion boards with red displays. > > According > > to the schematics, all other Adafruit 7-segment and 14-segment display > > backpack and FeatherWing expansion boards (including bare boards and > > boards fitted with displays) are compatible with these two boards. > > Add a "color" property to support the different color variants. > > > > [1] https://www.adafruit.com/product/3108 > > [2] https://www.adafruit.com/product/3130 > > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > > @@ -14,14 +14,23 @@ allOf: > > > > properties: > > compatible: > > - const: holtek,ht16k33 > > + oneOf: > > + - items: > > + - const: adafruit,3108 # 0.56" 4-Digit 7-Segment > > FeatherWing Display (Red) > > + - const: holtek,ht16k33 > > + > > + - items: > > + - const: adafruit,3130 # 0.54" Quad Alphanumeric > > FeatherWing Display (Red) > > + - const: holtek,ht16k33 > > + > > + - const: holtek,ht16k33 # Generic 16*8 LED controller with > > dot-matrix display > > > > reg: > > maxItems: 1 > > > > refresh-rate-hz: > > maxItems: 1 > > - description: Display update interval in Hertz > > + description: Display update interval in Hertz for dot-matrix > > displays > > The above should be included in patch 16 I disagree: bindings are independent from the driver implementation. > > interrupts: > > maxItems: 1 > > @@ -41,10 +50,17 @@ properties: > > default: 16 > > description: Initial brightness level > > > > + color: true > > + description: > > + Color of the display. Use one of the LED_COLOR_ID_* prefixed > > definitions > > + from the header include/dt-bindings/leds/common.h. The default > > is red. > > + minimum: 0 > > + maximum: 9 > > + default: 1 > > + > > The above should be included in patch 17 Same here. > > required: > > - compatible > > - reg > > - - refresh-rate-hz > > 'refresh-rate-hz' is still a required property for the dot-matrix / > fbdev setup. True. > If it can no longer be listed here than maybe its nice to mention that > it's required > somewhere else (in it's description?). if: properties: compatible: const: holtek,ht16k33 then: required: - refresh-rate-hz Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Rob, On 2021-05-18 16:35, Geert Uytterhoeven wrote: > Hoi Robin, > > On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote: >> On 2021-03-22 15:48, Geert Uytterhoeven wrote: >> > The Holtek HT16K33 LED controller is not only used for driving >> > dot-matrix displays, but also for driving segment displays. >> > >> > Document compatible values for the Adafruit 7-segment[1] and >> > 14-segment[2] FeatherWing expansion boards with red displays. >> > According >> > to the schematics, all other Adafruit 7-segment and 14-segment display >> > backpack and FeatherWing expansion boards (including bare boards and >> > boards fitted with displays) are compatible with these two boards. >> > Add a "color" property to support the different color variants. >> > >> > [1] https://www.adafruit.com/product/3108 >> > [2] https://www.adafruit.com/product/3130 >> > >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > >> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml >> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml >> > @@ -14,14 +14,23 @@ allOf: >> > >> > properties: >> > compatible: >> > - const: holtek,ht16k33 >> > + oneOf: >> > + - items: >> > + - const: adafruit,3108 # 0.56" 4-Digit 7-Segment >> > FeatherWing Display (Red) >> > + - const: holtek,ht16k33 >> > + >> > + - items: >> > + - const: adafruit,3130 # 0.54" Quad Alphanumeric >> > FeatherWing Display (Red) >> > + - const: holtek,ht16k33 >> > + >> > + - const: holtek,ht16k33 # Generic 16*8 LED controller with >> > dot-matrix display >> > >> > reg: >> > maxItems: 1 >> > >> > refresh-rate-hz: >> > maxItems: 1 >> > - description: Display update interval in Hertz >> > + description: Display update interval in Hertz for dot-matrix >> > displays >> >> The above should be included in patch 16 > > I disagree: bindings are independent from the driver implementation. > >> > interrupts: >> > maxItems: 1 >> > @@ -41,10 +50,17 @@ properties: >> > default: 16 >> > description: Initial brightness level >> > >> > + color: true >> > + description: >> > + Color of the display. Use one of the LED_COLOR_ID_* prefixed >> > definitions >> > + from the header include/dt-bindings/leds/common.h. The default >> > is red. >> > + minimum: 0 >> > + maximum: 9 >> > + default: 1 >> > + >> >> The above should be included in patch 17 > > Same here. My thought was that one without the other makes no sense. But if it's common practice to create a separate patch for device tree bindings (it's a patch-set after all) than that's fine with me. @Rob what do you think? Best regards, Met vriendelijke groet, Robin van der Gracht
Hoi Robin, On Wed, May 19, 2021 at 12:37 PM Robin van der Gracht <robin@protonic.nl> wrote: > On 2021-05-18 16:35, Geert Uytterhoeven wrote: > > On Tue, Mar 23, 2021 at 10:12 AM robin <robin@protonic.nl> wrote: > >> On 2021-03-22 15:48, Geert Uytterhoeven wrote: > >> > The Holtek HT16K33 LED controller is not only used for driving > >> > dot-matrix displays, but also for driving segment displays. > >> > > >> > Document compatible values for the Adafruit 7-segment[1] and > >> > 14-segment[2] FeatherWing expansion boards with red displays. > >> > According > >> > to the schematics, all other Adafruit 7-segment and 14-segment display > >> > backpack and FeatherWing expansion boards (including bare boards and > >> > boards fitted with displays) are compatible with these two boards. > >> > Add a "color" property to support the different color variants. > >> > > >> > [1] https://www.adafruit.com/product/3108 > >> > [2] https://www.adafruit.com/product/3130 > >> > > >> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > >> > --- a/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > >> > +++ b/Documentation/devicetree/bindings/auxdisplay/holtek,ht16k33.yaml > >> > @@ -14,14 +14,23 @@ allOf: > >> > > >> > properties: > >> > compatible: > >> > - const: holtek,ht16k33 > >> > + oneOf: > >> > + - items: > >> > + - const: adafruit,3108 # 0.56" 4-Digit 7-Segment > >> > FeatherWing Display (Red) > >> > + - const: holtek,ht16k33 > >> > + > >> > + - items: > >> > + - const: adafruit,3130 # 0.54" Quad Alphanumeric > >> > FeatherWing Display (Red) > >> > + - const: holtek,ht16k33 > >> > + > >> > + - const: holtek,ht16k33 # Generic 16*8 LED controller with > >> > dot-matrix display > >> > > >> > reg: > >> > maxItems: 1 > >> > > >> > refresh-rate-hz: > >> > maxItems: 1 > >> > - description: Display update interval in Hertz > >> > + description: Display update interval in Hertz for dot-matrix > >> > displays > >> > >> The above should be included in patch 16 > > > > I disagree: bindings are independent from the driver implementation. > > > >> > interrupts: > >> > maxItems: 1 > >> > @@ -41,10 +50,17 @@ properties: > >> > default: 16 > >> > description: Initial brightness level > >> > > >> > + color: true > >> > + description: > >> > + Color of the display. Use one of the LED_COLOR_ID_* prefixed > >> > definitions > >> > + from the header include/dt-bindings/leds/common.h. The default > >> > is red. > >> > + minimum: 0 > >> > + maximum: 9 > >> > + default: 1 > >> > + > >> > >> The above should be included in patch 17 > > > > Same here. > > My thought was that one without the other makes no sense. But if it's common > practice to create a separate patch for device tree bindings (it's a > patch-set > after all) than that's fine with me. scripts/checkpatch.pl even has a check for that: WARN("DT_SPLIT_BINDING_PATCH", "DT binding docs and includes should be a separate patch. See: Documentation/devicetree/bindings/submitting-patches.rst\n"); See also rule #5 in the doc referred above: 5) The Documentation/ portion of the patch should come in the series before the code implementing the binding. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Wed, Mar 24, 2021 at 9:31 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Mar 23, 2021 at 9:40 PM Pavel Machek <pavel@ucw.cz> wrote: > > > CC linux-leds (which I intended, but forgot to add) > > > > > > cover letter at > > > https://lore.kernel.org/linux-devicetree/20210322144848.1065067-1-geert@linux-m68k.org/ > > > > > + err = ht16k33_brightness_set(priv, seg->led.brightness); > > > > > if (err) > > > > > return err; > > > > > > > > The LED class can pretty much do what the backlight class can and more. > > > > > > > > Maybe we can stop registering a backlight device in the fbdev case and > > > > register a led device for both. This makes the code cleaner and drops > > > > a dependency but will break backwards compatibility. > > > > > > > > I'd prefer a single solution that covers both use cases, but I'm not > > > > sure about the 'breaking backwards compatibility' consequence... > > > > For new drivers, breaking compatibility should not be a problem. > > The dot-matrix support is part of the existing driver, thus subject to > backwards compatibility. > Perhaps we can register the LED device for both, and build the backlight > device on top of the LED device, like "led-backlight" does. Would that > work? Or can't the LED no longer be controlled from sysfs (e.g. > triggers) if it is in use by a backlight driver? Using "led-backlight", the backlight can no longer be controlled from sysfs, precluding the use of other triggers incl. hardware blinking. But a normal LED can be used as a backlight, with ledtrig-backlight, so that is the most flexible option, but only if no backwards compatibility is to be considered. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi all, The Holtek HT16K33 LED controller is not only used for driving dot-matrix displays, but also for driving segment displays. The current auxdisplay driver is limited to dot-matrix displays, which are exposed as a frame buffer device. This patch series extends the driver to 4-digit 7-segment and quad 14-segment alphanumeric displays, allowing the user to display and scroll text messages. List of patches: - Patch 1 provides font data for displaying ASCII characters on 14-segment displays, - Patch 2 updates the HT16K33 DT bindings for segment displays, - Patches 3-5 contain a bug fix and small improvements for the Imagination Technologies ASCII LCD Display driver, - Patch 6 extracts the character line display core support from the Imagination Technologies ASCII LCD Display driver, for reuse, - Patches 7-8 contain cleanups and improvements for the character line display core driver, - Patches 9-15 contain cleanups and improvements for the HT16K33 driver, to prepare for segment display support, - Patch 16 adds support for 7/14-segment displays to the HT16K33 driver, - Patch 17 adds segment display LED support to the HT16K33 driver, to make use of hardware blinking, and to expose display color. This series has been tested using an Adafruit 0.54" Quad Alphanumeric Red FeatherWing Display, plugged into an OrangeCrab ECP5-based FPGA board running linux-on-litex-vexriscv. 7-segment display support is based purely on schematics, and has not been tested on actual hardware. The changes to img-ascii-lcd.c are also untested, due to lack of hardware. Thanks for your comments! Geert Uytterhoeven (17): uapi: Add <linux/map_to_14segment.h> dt-bindings: auxdisplay: ht16k33: Document Adafruit segment displays auxdisplay: img-ascii-lcd: Fix lock-up when displaying empty string auxdisplay: img-ascii-lcd: Add helper variable dev auxdisplay: img-ascii-lcd: Convert device attribute to sysfs_emit() auxdisplay: Extract character line display core support auxdisplay: linedisp: Use kmemdup_nul() helper auxdisplay: linedisp: Add support for changing scroll rate auxdisplay: ht16k33: Use HT16K33_FB_SIZE in ht16k33_initialize() auxdisplay: ht16k33: Remove unneeded error check in keypad probe() auxdisplay: ht16k33: Convert to simple i2c probe function auxdisplay: ht16k33: Add helper variable dev auxdisplay: ht16k33: Move delayed work auxdisplay: ht16k33: Extract ht16k33_brightness_set() auxdisplay: ht16k33: Extract frame buffer probing auxdisplay: ht16k33: Add support for segment displays auxdisplay: ht16k33: Add segment display LED support .../bindings/auxdisplay/holtek,ht16k33.yaml | 22 +- drivers/auxdisplay/Kconfig | 8 + drivers/auxdisplay/Makefile | 1 + drivers/auxdisplay/ht16k33.c | 451 ++++++++++++++---- drivers/auxdisplay/img-ascii-lcd.c | 199 +------- drivers/auxdisplay/line-display.c | 261 ++++++++++ drivers/auxdisplay/line-display.h | 43 ++ include/uapi/linux/map_to_14segment.h | 240 ++++++++++ 8 files changed, 961 insertions(+), 264 deletions(-) create mode 100644 drivers/auxdisplay/line-display.c create mode 100644 drivers/auxdisplay/line-display.h create mode 100644 include/uapi/linux/map_to_14segment.h