On Fri, 11 Jul 2008 17:54:31 +0300 Yauhen Kharuzhy [off-list ref] wrote:

Add a new driver for eInk Apollo controller. This
controller is used in various bookreaders with eInk
displays.

The eink_apollofb driver supports many features of
Apollo chip, in difference with the hecubafb driver:
2-bit color depth, partial picture loading, flash
read/write.

The driver uses platform_device framework for
platform-specific functions (set values of control
lines, read/write data from/to bus etc.) and can be
used on any platform.

This driver has been developed for the OpenInkpot
project (http://openinkpot.org/).

Please copy linux-fbdev-devel on fbdev patches.

Please consider adding a MAINTAINERS record when adding new drivers.

checkpatch correctly warns:

WARNING: consider using strict_strtoul in preference to simple_strtoul
#703: FILE: drivers/video/eink_apollofb.c:573:
+	unsigned long state = simple_strtoul(buf, &after, 10);

WARNING: consider using strict_strtoul in preference to simple_strtoul
#734: FILE: drivers/video/eink_apollofb.c:604:
+	unsigned long state = simple_strtoul(buf, &after, 10);

WARNING: consider using strict_strtoul in preference to simple_strtoul
#773: FILE: drivers/video/eink_apollofb.c:643:
+	unsigned long state = simple_strtoul(buf, &after, 10);

Whitespace inconsistency there, which I will fix.

...

+/* Display specific information */
+#define DPY_W 600
+#define DPY_H 800
+
+#define is_portrait(var) (!(var.rotate % 180))

Could be implemented in a C function.

...

+static inline int apollo_wait_for_ack_value(struct apollofb_par *par,
+		unsigned int value)
+{
+	unsigned long timeout = jiffies + 2 * HZ;
+
+	while ((par->ops->get_ctl_pin(H_ACK) != value) &&
+			time_before(jiffies, timeout))
+		schedule();
+
+	if (par->ops->get_ctl_pin(H_ACK) != value) {
+		printk(KERN_ERR "%s: Wait for H_ACK == %u, timeout\n",
+				__func__, value);
+		return 1;
+	}
+
+	return 0;
+}

Is a busy-wait unavoidable here?

Are you sure this function is never called from atomic context?

This function is far too large to inline.  I'll fix that.

+#define apollo_wait_for_ack(par)	apollo_wait_for_ack_value(par, 0)
+#define apollo_wait_for_ack_clear(par)	apollo_wait_for_ack_value(par, 1)

Could be implemented as C functions.

+static int apollo_send_data(struct apollofb_par *par, unsigned char data)
+{
+	int res = 0;;
+
+	par->ops->write_value(data);
+	par->ops->set_ctl_pin(H_DS, 0);
+	res = apollo_wait_for_ack(par);
+	par->ops->set_ctl_pin(H_DS, 1);
+	if (!res)
+		apollo_wait_for_ack_clear(par);
+	return res;
+}
+
+

extra newline

...

+static void apollofb_apollo_update_part(struct apollofb_par *par,
+		unsigned int x1, unsigned int y1,
+		unsigned int x2, unsigned int y2)
+{
+	int i, j, k;
+	struct fb_info *info = par->info;
+	unsigned int width = is_portrait(info->var) ? info->var.xres :
+							info->var.yres;
+	unsigned int bpp = info->var.green.length;
+	unsigned int pixels_in_byte = 8 / bpp;
+	unsigned char *buf = (unsigned char __force *)info->screen_base;
+	unsigned char tmp, mask;
+
+	y1 -= y1 % 4;
+
+	if ((y2 + 1) % 4)
+		y2 += 4 - ((y2 + 1) % 4);
+
+	x1 -= x1 % 4;
+
+	if ((x2 + 1) % 4)
+		x2 += 4 - ((x2 + 1) % 4);
+
+	mask = 0;
+	for (i = 0; i < bpp; i++)
+		mask = (mask << 1) | 1;

I think this is this equivalent to

	(1 << bpp) - 1

+	mutex_lock(&par->lock);
+
+	if (par->current_mode == APOLLO_STATUS_MODE_SLEEP)
+		apollo_set_normal_mode(par);
+
+	if (par->options.manual_refresh)
+		apollo_send_command(par, APOLLO_MANUAL_REFRESH);
+
+	apollo_send_command(par, APOLLO_LOAD_PARTIAL_PICTURE);
+	apollo_send_data(par, (x1 >> 8) & 0xff);
+	apollo_send_data(par, x1 & 0xff);
+	apollo_send_data(par, (y1 >> 8) & 0xff);
+	apollo_send_data(par, y1 & 0xff);
+	apollo_send_data(par, (x2 >> 8) & 0xff);
+	apollo_send_data(par, x2 & 0xff);
+	apollo_send_data(par, (y2 >> 8) & 0xff);
+	apollo_send_data(par, y2 & 0xff);
+
+	k = 0;
+	tmp = 0;
+	for (i = y1; i <= y2; i++)
+		for (j = x1; j <= x2; j++) {
+			tmp = (tmp << bpp) | (buf[i * width + j] & mask);
+			k++;
+			if (k % pixels_in_byte == 0)
+				apollo_send_data(par, tmp);
+		}
+
+	apollo_send_command(par, APOLLO_STOP_LOADING);
+	apollo_send_command(par, APOLLO_DISPLAY_PARTIAL_PICTURE);
+
+	if (par->options.use_sleep_mode)
+		apollo_set_sleep_mode(par);
+
+	mutex_unlock(&par->lock);
+}
+
+/* this is called back from the deferred io workqueue */
+static void apollofb_dpy_deferred_io(struct fb_info *info,
+				struct list_head *pagelist)
+{
+
+	struct apollofb_par *par = info->par;
+	unsigned int width = is_portrait(info->var) ? info->var.xres :
+							info->var.yres;
+	unsigned int height = is_portrait(info->var) ? info->var.yres :
+							info->var.xres;
+	unsigned long int start_page = -1, end_page = -1;
+	unsigned int y1 = 0, y2 = 0;
+	struct page *cur;
+
+

extra newline

+	list_for_each_entry(cur, pagelist, lru) {
+		if (start_page == -1) {
+			start_page = cur->index;
+			end_page = cur->index;
+			continue;
+		}
+
+		if (cur->index == end_page + 1) {
+			end_page++;
+		} else {
+			y1 = start_page * PAGE_SIZE / width;
+			y2 = ((end_page + 1) * PAGE_SIZE - 1) / width;
+			if (y2 >= height)
+				y2 = height - 1;
+
+			apollofb_apollo_update_part(par, 0, y1, width - 1, y2);
+
+			start_page = cur->index;
+			end_page = cur->index;
+		}
+	}
+
+	y1 = start_page * PAGE_SIZE / width;
+	y2 = ((end_page + 1) * PAGE_SIZE - 1) / width;
+	if (y2 >= height)
+		y2 = height - 1;
+
+	apollofb_apollo_update_part(par, 0, y1,	width - 1, y2);
+}
+

...


+static void apollofb_imageblit(struct fb_info *info,
+				const struct fb_image *image)
+{
+	struct apollofb_par *par = info->par;
+
+	sys_imageblit(info, image);
+
+	schedule_delayed_work(&par->deferred_work, info->fbdefio->delay);
+}

hrm.  The sys_foo namespace is normally for system calls.  Looks like
the fbdev layer has been involved in a bit of namespace piracy.

+int apollofb_cursor(struct fb_info *info, struct fb_cursor *cursor)
+{
+	return 0;
+}

I made this static.

+/*
+ * this is the slow path from userspace. they can seek and write to
+ * the fb. it's inefficient to do anything less than a full screen draw
+ */
+static ssize_t apollofb_write(struct fb_info *info, const char __user *buf,
+				size_t count, loff_t *ppos)
+{
+	unsigned long p;
+	int err = -EINVAL;
+	struct apollofb_par *par;
+	unsigned int xres;
+	unsigned int fbmemlength;
+
+	p = *ppos;
+	par = info->par;
+	xres = info->var.xres;
+	fbmemlength = (xres * info->var.yres)/8 * info->var.bits_per_pixel;
+
+	if (p > fbmemlength)
+		return -ENOSPC;
+
+	err = 0;
+	if ((count + p) > fbmemlength) {
+		count = fbmemlength - p;
+		err = -ENOSPC;
+	}

ENOSPC is "ran out of disk space".  It's a bit weird to use it here. 
EINVAL would make more sense?

+	if (count) {
+		char *base_addr;
+
+		base_addr = (char __force *)info->screen_base;
+		count -= copy_from_user(base_addr + p, buf, count);
+		*ppos += count;
+		err = -EFAULT;
+	}
+
+	schedule_delayed_work(&par->deferred_work, info->fbdefio->delay);
+
+	if (count)
+		return count;
+
+	return err;
+}
+

...

+static ssize_t apollofb_temperature_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+	struct apollofb_par *par = info->par;
+	char temp;
+
+	mutex_lock(&par->lock);
+
+	apollo_send_command(par, APOLLO_READ_TEMPERATURE);
+
+	temp = apollo_read_data(par);
+
+	mutex_unlock(&par->lock);
+
+	sprintf(buf, "%d\n", temp);
+	return strlen(buf) + 1;

I think

	return sprintf(buf, "%d\n", temp);

would work here.  Not sure about the accounting of the trailing \0.

+}
+
+static ssize_t apollofb_manual_refresh_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+	struct apollofb_par *par = info->par;
+
+	sprintf(buf, "%d\n", par->options.manual_refresh);
+	return strlen(buf) + 1;

ditto.

+}
+
+static ssize_t apollofb_manual_refresh_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+	struct apollofb_par *par = info->par;
+	char *after;
+	unsigned long state = simple_strtoul(buf, &after, 10);
+	size_t count = after - buf;
+	ssize_t ret = -EINVAL;
+
+	if (*after && isspace(*after))
+		count++;
+
+	if ((count == size) && (state <= 1)) {
+		ret = count;
+		par->options.manual_refresh = state;
+	}
+
+	return ret;
+}
+
+static ssize_t apollofb_use_sleep_mode_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+	struct apollofb_par *par = info->par;
+
+	sprintf(buf, "%d\n", par->options.use_sleep_mode);
+	return strlen(buf) + 1;

tritto

+}
+
+static ssize_t apollofb_use_sleep_mode_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+	struct apollofb_par *par = info->par;
+	char *after;
+	unsigned long state = simple_strtoul(buf, &after, 10);
+	size_t count = after - buf;
+	ssize_t ret = -EINVAL;
+
+	if (*after && isspace(*after))
+		count++;
+
+	if ((count == size) && (state <= 1)) {
+		ret = count;
+		par->options.use_sleep_mode = state;
+
+		mutex_lock(&par->lock);
+
+		if (state)
+			apollo_set_sleep_mode(par);
+		else
+			apollo_set_normal_mode(par);
+
+		mutex_unlock(&par->lock);
+
+	}
+
+	return ret;
+}

Is this usersapce interface documented anywhere?

+static ssize_t apollofb_defio_delay_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+
+	sprintf(buf, "%lu\n", info->fbdefio->delay * 1000 / HZ);
+	return strlen(buf) + 1;

whatever comes after tritto ;)

+}
+
+static ssize_t apollofb_defio_delay_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct fb_info *info = dev_get_drvdata(dev);
+	char *after;
+	unsigned long state = simple_strtoul(buf, &after, 10);
+	size_t count = after - buf;
+	ssize_t ret = -EINVAL;
+
+	if (*after && isspace(*after))
+		count++;
+
+	state = state * HZ / 1000;
+
+	if (!state)
+		state = 1;
+
+	if (count == size) {
+		ret = count;
+		info->fbdefio->delay = state;
+	}
+
+	return ret;
+}

See, there might be simpler ways of doing all this string parsing.  But
there's no description of what it's doing and I can't be bothered
reverse-engineering it.

+static void apollofb_remove_chrdev(struct apollofb_par *par)
+{
+	cdev_del(&par->cdev);
+	unregister_chrdev_region(par->cdev.dev, 1);
+}

It creates a chardev as well?  Please, these things must be documented
_at least_ in the changelog.  Fully.

+static u16 red4[] __read_mostly = {
+    0x0000, 0x5555, 0xaaaa, 0xffff
+};
+static u16 green4[] __read_mostly = {
+    0x0000, 0x5555, 0xaaaa, 0xffff
+};
+static u16 blue4[] __read_mostly = {
+    0x0000, 0x5555, 0xaaaa, 0xffff
+};
+

...

+static int __devexit apollofb_remove(struct platform_device *dev)
+{
+	struct fb_info *info = platform_get_drvdata(dev);
+	struct apollofb_par *par = info->par;
+
+	if (info) {
+		fb_deferred_io_cleanup(info);
+		cancel_delayed_work(&par->deferred_work);
+		flush_scheduled_work();

I suspect cancel_delayed_work_sync() would suffice here.

+		device_remove_file(info->dev, &dev_attr_use_sleep_mode);
+		device_remove_file(info->dev, &dev_attr_manual_refresh);
+		device_remove_file(info->dev, &dev_attr_defio_delay);
+		device_remove_file(info->dev, &dev_attr_temperature);
+		unregister_framebuffer(info);
+		vfree((void __force *)info->screen_base);
+		apollofb_remove_chrdev(info->par);
+		framebuffer_release(info);
+	}
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int apollofb_suspend(struct platform_device *pdev, pm_message_t message)
+{
+	struct fb_info *info = platform_get_drvdata(pdev);
+	struct apollofb_par *par = info->par;
+
+	mutex_lock(&par->lock);
+	apollo_send_command(par, APOLLO_STANDBY_MODE);
+	par->current_mode = APOLLO_STATUS_MODE_SLEEP;
+	mutex_unlock(&par->lock);
+
+	return 0;
+}
+
+static int apollofb_resume(struct platform_device *pdev)
+{
+	struct fb_info *info = platform_get_drvdata(pdev);
+	struct apollofb_par *par = info->par;
+
+	mutex_lock(&par->lock);
+	apollo_wakeup(par);
+	if (!par->options.use_sleep_mode)
+		apollo_set_normal_mode(par);
+	mutex_unlock(&par->lock);
+
+	return 0;
+}

Please put

#else
#define apollofb_suspend NULL
#define apollofb_resume NULL

here

+#endif
+
+
+static struct platform_driver apollofb_driver = {
+	.probe	= apollofb_probe,
+	.remove = apollofb_remove,
+	.driver	= {
+		.owner	= THIS_MODULE,
+		.name	= "eink-apollo",
+	},
+#ifdef CONFIG_PM
+	.suspend = apollofb_suspend,
+	.resume = apollofb_resume,
+#endif

and remove these ifdefs.  For consistency, and it saves an ifdef.

+};
+
+static int __init apollofb_init(void)
+{
+	int ret = 0;

Unneeded initialization

+
+	ret = platform_driver_register(&apollofb_driver);
+
+	return ret;
+}

Plain old

	return platform_driver_register(&apollofb_driver);

would suffice?

+static void __exit apollofb_exit(void)
+{
+	platform_driver_unregister(&apollofb_driver);
+}
+
+

-------------------------------------------------------------------------
Sponsored by: SourceForge.net Community Choice Awards: VOTE NOW!
Studies have shown that voting for your favorite open source project,
along with a healthy diet, reduces your potential for chronic lameness
and boredom. Vote Now at http://www.sourceforge.net/community/cca08