Message-ID: <20160203141225.GF5464@jhogan-linux.le.imgtec.org>
Date:	Wed, 3 Feb 2016 14:12:25 +0000
From:	James Hogan <james.hogan@...tec.com>
To:	Paul Burton <paul.burton@...tec.com>
CC:	<linux-mips@...ux-mips.org>, Ralf Baechle <ralf@...ux-mips.org>,
	"Miguel Ojeda Sandonis" <miguel.ojeda.sandonis@...il.com>,
	Kalle Valo <kvalo@...eaurora.org>,
	Jiri Slaby <jslaby@...e.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	<linux-kernel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	Joe Perches <joe@...ches.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v2 02/15] auxdisplay: driver for simple memory mapped
 ASCII LCD displays

On Wed, Feb 03, 2016 at 11:30:32AM +0000, Paul Burton wrote:
> Add a driver for simple memory mapped ASCII LCD displays such as those
> found on the MIPS Malta & Boston development boards. The driver displays
> the Linux kernel version as the default message, but allows the message
> to be changed via a character device. Messages longer then the number of
> characters that the display can show will scroll.
> 
> This provides different behaviour to the existing LCD display code for
> the MIPS Malta platform in the following ways:
> 
>   - The default string to display is not "LINUX ON MALTA" but "Linux"
>     followed by the version number of the kernel (UTS_RELEASE).
> 
>   - Since that string tends to significantly longer it scrolls twice
>     as fast, moving every 500ms rather than every 1s.
> 
>   - The LCD won't be updated until the driver is probed, so it doesn't
>     provide the early "LINUX" string.
> 
> Signed-off-by: Paul Burton <paul.burton@...tec.com>
> ---
> 
> Changes in v2: None
> 
>  MAINTAINERS                    |   1 +
>  drivers/auxdisplay/Kconfig     |   7 ++
>  drivers/auxdisplay/Makefile    |   1 +
>  drivers/auxdisplay/ascii-lcd.c | 230 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 239 insertions(+)
>  create mode 100644 drivers/auxdisplay/ascii-lcd.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cd19e5f..301bc429 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1823,6 +1823,7 @@ ASCII LCD DRIVER
>  M:	Paul Burton <paul.burton@...tec.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/ascii-lcd.txt
> +F:	drivers/auxdisplay/ascii-lcd.c
>  
>  ASUS NOTEBOOKS AND EEEPC ACPI/WMI EXTRAS DRIVERS
>  M:	Corentin Chary <corentin.chary@...il.com>
> diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
> index c07e725..a465e0d 100644
> --- a/drivers/auxdisplay/Kconfig
> +++ b/drivers/auxdisplay/Kconfig
> @@ -119,4 +119,11 @@ config CFAG12864B_RATE
>  	  If you compile this as a module, you can still override this
>  	  value using the module parameters.
>  
> +config ASCII_LCD
> +	tristate "ASCII LCD Display"
> +	default y if MIPS_MALTA
> +	help
> +	  Enable this to support simple memory mapped ASCII LCD displays such
> +	  as those found on the MIPS Malta & Boston development boards.
> +
>  endif # AUXDISPLAY
> diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
> index 8a8936a..8a5aa81 100644
> --- a/drivers/auxdisplay/Makefile
> +++ b/drivers/auxdisplay/Makefile
> @@ -4,3 +4,4 @@
>  
>  obj-$(CONFIG_KS0108)		+= ks0108.o
>  obj-$(CONFIG_CFAG12864B)	+= cfag12864b.o cfag12864bfb.o
> +obj-$(CONFIG_ASCII_LCD)		+= ascii-lcd.o
> diff --git a/drivers/auxdisplay/ascii-lcd.c b/drivers/auxdisplay/ascii-lcd.c
> new file mode 100644
> index 0000000..5c9ec32
> --- /dev/null
> +++ b/drivers/auxdisplay/ascii-lcd.c
> @@ -0,0 +1,230 @@
> +/*
> + * Copyright (C) 2015 Imagination Technologies
> + * Author: Paul Burton <paul.burton@...tec.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <generated/utsrelease.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +
> +struct ascii_lcd_ctx;
> +
> +struct ascii_lcd_config {
> +	unsigned num_chars;
> +	void (*update)(struct ascii_lcd_ctx *ctx);
> +};
> +
> +struct ascii_lcd_ctx {
> +	struct platform_device *pdev;
> +	void __iomem *base;
> +	const struct ascii_lcd_config *cfg;
> +	char *message;
> +	unsigned message_len;
> +	unsigned scroll_pos;
> +	unsigned scroll_rate;
> +	struct timer_list timer;
> +	char curr[] __aligned(8);
> +};
> +
> +static void update_boston(struct ascii_lcd_ctx *ctx)
> +{
> +	u32 val32;
> +	u64 val64;
> +
> +	if (config_enabled(CONFIG_64BIT)) {
> +		val64 = *((u64 *)&ctx->curr[0]);
> +		__raw_writeq(val64, ctx->base);
> +	} else {
> +		val32 = *((u32 *)&ctx->curr[0]);
> +		__raw_writel(val32, ctx->base);
> +		val32 = *((u32 *)&ctx->curr[4]);
> +		__raw_writel(val32, ctx->base + 4);
> +	}
> +}
> +
> +static void update_malta(struct ascii_lcd_ctx *ctx)
> +{
> +	unsigned i;
> +
> +	for (i = 0; i < ctx->cfg->num_chars; i++)
> +		__raw_writel(ctx->curr[i], ctx->base + (i * 8));
> +}
> +
> +static struct ascii_lcd_config boston_config = {
> +	.num_chars = 8,
> +	.update = update_boston,
> +};
> +
> +static struct ascii_lcd_config malta_config = {
> +	.num_chars = 8,
> +	.update = update_malta,
> +};
> +
> +static const struct of_device_id ascii_lcd_matches[] = {
> +	{ .compatible = "img,boston-lcd", .data = &boston_config },
> +	{ .compatible = "mti,malta-lcd", .data = &malta_config },
> +};

MODULE_DEVICE_TABLE?

> +
> +static void ascii_lcd_scroll(unsigned long arg)
> +{
> +	struct ascii_lcd_ctx *ctx = (struct ascii_lcd_ctx *)arg;
> +	unsigned i, ch = ctx->scroll_pos;
> +	unsigned num_chars = ctx->cfg->num_chars;
> +
> +	/* update the current message string */
> +	for (i = 0; i < num_chars;) {
> +		/* copy as many characters from the string as possible */
> +		for (; i < num_chars && ch < ctx->message_len; i++, ch++)
> +			ctx->curr[i] = ctx->message[ch];
> +
> +		/* wrap around to the start of the string */
> +		ch = 0;
> +	}

should this pad ctx->curr wth spaces for the case where
ctx->message_len <= ctx->cfg->num_chars?

> +
> +	/* update the LCD */
> +	ctx->cfg->update(ctx);
> +
> +	/* move on to the next character */
> +	ctx->scroll_pos++;
> +	ctx->scroll_pos %= ctx->message_len;
> +
> +	/* rearm the timer */
> +	if (ctx->message_len > ctx->cfg->num_chars)
> +		mod_timer(&ctx->timer, jiffies + ctx->scroll_rate);
> +}
> +
> +static int ascii_lcd_display(struct ascii_lcd_ctx *ctx,
> +			     const char *msg, ssize_t count)
> +{
> +	char *new_msg;
> +
> +	/* stop the scroll timer */
> +	del_timer_sync(&ctx->timer);
> +
> +	if (count == -1)
> +		count = strlen(msg);
> +
> +	/* if the string ends with a newline, trim it */
> +	if (msg[count - 1] == '\n')
> +		count--;
> +
> +	new_msg = devm_kmalloc(&ctx->pdev->dev, count + 1, GFP_KERNEL);
> +	if (!new_msg)
> +		return -ENOMEM;
> +
> +	memcpy(new_msg, msg, count);
> +	new_msg[count] = 0;

'\0' probably shows the intent more clearly.

> +
> +	if (ctx->message)
> +		devm_kfree(&ctx->pdev->dev, ctx->message);
> +
> +	ctx->message = new_msg;
> +	ctx->message_len = count;
> +	ctx->scroll_pos = 0;

Does this need locking to prevent concurrent writes from racing and
leaking memory?

> +
> +	/* update the LCD */
> +	ascii_lcd_scroll((unsigned long)ctx);
> +
> +	return 0;
> +}
> +
> +static ssize_t message_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct ascii_lcd_ctx *ctx = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", ctx->message);
> +}
> +
> +static ssize_t message_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct ascii_lcd_ctx *ctx = dev_get_drvdata(dev);
> +	int err;
> +
> +	err = ascii_lcd_display(ctx, buf, count);
> +	return err ?: count;
> +}
> +
> +static DEVICE_ATTR_RW(message);
> +
> +static int ascii_lcd_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	const struct ascii_lcd_config *cfg;
> +	struct ascii_lcd_ctx *ctx;
> +	struct resource *res;
> +	int err;
> +
> +	match = of_match_device(ascii_lcd_matches, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	cfg = match->data;
> +	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx) + cfg->num_chars,
> +			   GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ctx->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(ctx->base))
> +		return PTR_ERR(ctx->base);
> +
> +	ctx->pdev = pdev;
> +	ctx->cfg = cfg;
> +	ctx->message = NULL;
> +	ctx->scroll_pos = 0;
> +	ctx->scroll_rate = HZ / 2;
> +
> +	/* initialise a timer for scrolling the message */
> +	init_timer(&ctx->timer);
> +	ctx->timer.function = ascii_lcd_scroll;
> +	ctx->timer.data = (unsigned long)ctx;
> +
> +	platform_set_drvdata(pdev, ctx);
> +
> +	/* display a default message */
> +	err = ascii_lcd_display(ctx, "Linux " UTS_RELEASE "       ", -1);

should the number of spaces depend on the length of the display?

> +	if (err)
> +		goto out_del_timer;

the only error case in ascii_led_display seems to be failure to
allocate, at which point timer won't have been started yet, so you could
just return err here. Any future error case added after starting timer
should probably be handled by ascii_lcd_display() anyway.

> +
> +	err = device_create_file(&pdev->dev, &dev_attr_message);
> +	if (err)
> +		goto out_del_timer;
> +
> +	return 0;
> +out_del_timer:
> +	del_timer_sync(&ctx->timer);
> +	return err;
> +}
> +
> +static int ascii_lcd_remove(struct platform_device *pdev)
> +{
> +	struct ascii_lcd_ctx *ctx = platform_get_drvdata(pdev);
> +
> +	device_remove_file(&pdev->dev, &dev_attr_message);
> +	del_timer_sync(&ctx->timer);
> +	return 0;
> +}
> +
> +static struct platform_driver ascii_lcd_driver = {
> +	.driver = {
> +		.name		= "ascii-lcd",
> +		.of_match_table	= ascii_lcd_matches,
> +	},
> +	.probe	= ascii_lcd_probe,
> +	.remove	= ascii_lcd_remove,
> +};
> +module_platform_driver(ascii_lcd_driver);

Should you have MODULE_AUTHOR/DESCRIPTION/LICENSE and friends in here
too for when built as a module?

Cheers
James

> -- 
> 2.7.0
> 
> 

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists