| Message ID | 20210625125902.1162428-19-geert@linux-m68k.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Fri, 25 Jun 2021 22:39:16 +0200 Marek Behun <marek.behun@nic.cz> wrote: > On Fri, 25 Jun 2021 14:59:02 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> 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. > > The LED will be named "auxdisplay:<color>:backlight". > > What if there are multiple "auxdisplay"s ? > Doesn't this subsystem have IDs? So that you can use auxdisplayN for > device name, for example? Or if this driver creates a fbdev, maybe "fb<N>" for devicename? Marek
Hi Marek, On Fri, Jun 25, 2021 at 10:39 PM Marek Behun <marek.behun@nic.cz> wrote: > On Fri, 25 Jun 2021 14:59:02 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> 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. > > The LED will be named "auxdisplay:<color>:backlight". > > What if there are multiple "auxdisplay"s ? I understand the LED core will just add a suffix on a name collision. > Doesn't this subsystem have IDs? So that you can use auxdisplayN for > device name, for example? Auxdisplay does not have IDs, as there is no subsystem to register with. It's just a collection of drivers for auxiliary displays with no common API. Some drivers use fbdev, others use a chardev, or an attribute file in sysfs. BTW, I just followed Pavel's advice in naming. > > + of_property_read_u32(node, "color", &color); > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > + "auxdisplay:%s:" LED_FUNCTION_BACKLIGHT, > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > > If you use devm_led_classdev_register_ext and pass struct > led_init_data, LED core will generate name of the LED itself. Will that make any difference, except for adding more code? Looking at the implementation, I still have to use devm_kasprintf() to combine color and function for led_init_data.default_label? Gr{oetje,eeting}s, Geert
Hi Marek, On Fri, Jun 25, 2021 at 10:40 PM Marek Behun <marek.behun@nic.cz> wrote: > On Fri, 25 Jun 2021 22:39:16 +0200 > Marek Behun <marek.behun@nic.cz> wrote: > > On Fri, 25 Jun 2021 14:59:02 +0200 > > Geert Uytterhoeven <geert@linux-m68k.org> 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. > > > The LED will be named "auxdisplay:<color>:backlight". > > > > What if there are multiple "auxdisplay"s ? > > Doesn't this subsystem have IDs? So that you can use auxdisplayN for > > device name, for example? > > Or if this driver creates a fbdev, maybe "fb<N>" for devicename? This LED device is only registered when using the HT16K33 to drive segment displays. When driving a dot matrix display, the driver still use fbdev and devm_backlight_device_register(), for backwards compatibility. Gr{oetje,eeting}s, Geert
On Mon, 28 Jun 2021 11:21:04 +0200 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Marek, > > On Fri, Jun 25, 2021 at 10:39 PM Marek Behun <marek.behun@nic.cz> wrote: > > On Fri, 25 Jun 2021 14:59:02 +0200 > > Geert Uytterhoeven <geert@linux-m68k.org> 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. > > > The LED will be named "auxdisplay:<color>:backlight". > > > > What if there are multiple "auxdisplay"s ? > > I understand the LED core will just add a suffix on a name collision. > > > Doesn't this subsystem have IDs? So that you can use auxdisplayN for > > device name, for example? > > Auxdisplay does not have IDs, as there is no subsystem to register > with. It's just a collection of drivers for auxiliary displays with > no common API. Some drivers use fbdev, others use a chardev, or an > attribute file in sysfs. > > BTW, I just followed Pavel's advice in naming. Very well. > > > + of_property_read_u32(node, "color", &color); > > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > > + "auxdisplay:%s:" LED_FUNCTION_BACKLIGHT, > > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > > > > If you use devm_led_classdev_register_ext and pass struct > > led_init_data, LED core will generate name of the LED itself. > > Will that make any difference, except for adding more code? You are hardcoding the backlight function. Using the _ext() registering function will make it so that the function and color are parsed from fwnode by LED core. I understand that the function will always be "backlight" in this case, but this should be specified in the device-tree anyway, so why not use it? > Looking at the implementation, I still have to use devm_kasprintf() > to combine color and function for led_init_data.default_label? AFAIK you don't have to fill in default_label. (If the needed OF properties are not present so that default_label is tried, it means the device-tree does not correctly specify the device. In that case I don't think it is a problem if the default_label is not present and LED core will use the OF node name as the LED name.) The code could look like this struct led_init_data init_data = {}; init_data.fwnode = of_fwnode_handle(node); init_data.devicename = "auxdisplay"; init_data.devname_mandatory = true; ...register_ext(); But if you still don't want to do this then ignore me :) Marek
Hi Marek, On Mon, Jun 28, 2021 at 12:15 PM Marek Behun <marek.behun@nic.cz> wrote: > On Mon, 28 Jun 2021 11:21:04 +0200 > Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Jun 25, 2021 at 10:39 PM Marek Behun <marek.behun@nic.cz> wrote: > > > On Fri, 25 Jun 2021 14:59:02 +0200 > > > Geert Uytterhoeven <geert@linux-m68k.org> 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. > > > > The LED will be named "auxdisplay:<color>:backlight". > > > > > > What if there are multiple "auxdisplay"s ? > > > > I understand the LED core will just add a suffix on a name collision. > > > > > Doesn't this subsystem have IDs? So that you can use auxdisplayN for > > > device name, for example? > > > > Auxdisplay does not have IDs, as there is no subsystem to register > > with. It's just a collection of drivers for auxiliary displays with > > no common API. Some drivers use fbdev, others use a chardev, or an > > attribute file in sysfs. > > > > BTW, I just followed Pavel's advice in naming. > > Very well. > > > > > + of_property_read_u32(node, "color", &color); > > > > + seg->led.name = devm_kasprintf(dev, GFP_KERNEL, > > > > + "auxdisplay:%s:" LED_FUNCTION_BACKLIGHT, > > > > + color < LED_COLOR_ID_MAX ? led_colors[color] : ""); > > > > > > If you use devm_led_classdev_register_ext and pass struct > > > led_init_data, LED core will generate name of the LED itself. > > > > Will that make any difference, except for adding more code? > > You are hardcoding the backlight function. Using the _ext() registering > function will make it so that the function and color are parsed from > fwnode by LED core. I understand that the function will always be > "backlight" in this case, but this should be specified in the > device-tree anyway, so why not use it? > > > Looking at the implementation, I still have to use devm_kasprintf() > > to combine color and function for led_init_data.default_label? > > AFAIK you don't have to fill in default_label. (If the needed OF > properties are not present so that default_label is tried, it means the > device-tree does not correctly specify the device. In that case I don't > think it is a problem if the default_label is not present and LED > core will use the OF node name as the LED name.) > > The code could look like this > > struct led_init_data init_data = {}; > > init_data.fwnode = of_fwnode_handle(node); > init_data.devicename = "auxdisplay"; > init_data.devname_mandatory = true; > > ...register_ext(); > > But if you still don't want to do this then ignore me :) No, thanks a lot! Your comments made me realize I should put the LED properties in an "led" subnode, and defer all parsing to the LED core. This also allows the user to use the more powerful LED mode even in dot-matrix mode, while falling back to the existing backlight mode if no "led" subnode is found, and thus preserving backwards compatibility. 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
diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c index 28207517a4725250..cd88c7bcb1d713bf 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; @@ -576,11 +631,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, + "auxdisplay:%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;
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. The LED will be named "auxdisplay:<color>:backlight". 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. It can still be used with ledtrig-backlight. Using "led-backlight", the backlight can no longer be controlled from sysfs, precluding the use of other triggers incl. hardware blinking. v2: - Use "auxdisplay" instead of DRIVER_NAME in LED name. --- drivers/auxdisplay/ht16k33.c | 81 ++++++++++++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 4 deletions(-)