[PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Alexander Shiyan]

This adds support for the framebuffer available in the Cirrus
Logic CLPS711X CPUs.
FB features:
- 1-2-4 bits per pixel.
- Programmable panel size to a maximum of 1024x256 at 4 bps.
- Relocatible Frame Buffer (SRAM or SDRAM).
- Programmable refresh rates.
- 16 gray scale values.
This new driver supports usage with devicetree and as a general
change it removes last user of <mach/hardware.h> for CLPS711X targets,
so this subarch will fully prepared to switch to multiplatform.
The driver have been tested with custom board equipped Cirrus Logic
EP7312 in DT and non-DT mode.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/video/fbdev/clps711x-fb.c | 456 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 456 insertions(+)
 create mode 100644 drivers/video/fbdev/clps711x-fb.c

diff --git a/drivers/video/fbdev/clps711x-fb.c b/drivers/video/fbdev/clps711x-fb.c
new file mode 100644
index 0000000..87fd42d
--- /dev/null
+++ b/drivers/video/fbdev/clps711x-fb.c
@@ -0,0 +1,456 @@
+/*
+ * Cirrus Logic CLPS711X FB driver
+ *
+ * Copyright (C) 2014 Alexander Shiyan <shc_work@mail.ru>
+ * Based on driver by Russell King <rmk@arm.linux.org.uk>
+ *
+ * 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 <linux/clk.h>
+#include <linux/fb.h>
+#include <linux/io.h>
+#include <linux/lcd.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mfd/syscon/clps711x.h>
+#include <linux/regulator/consumer.h>
+#include <video/of_display_timing.h>
+
+#define CLPS711X_FB_NAME	"clps711x-fb"
+#define CLPS711X_FB_BPP_MAX	(4)
+
+/* Registers relative to LCDCON */
+#define CLPS711X_LCDCON		(0x0000)
+# define LCDCON_GSEN		BIT(30)
+# define LCDCON_GSMD		BIT(31)
+#define CLPS711X_PALLSW		(0x0280)
+#define CLPS711X_PALMSW		(0x02c0)
+#define CLPS711X_FBADDR		(0x0d40)
+
+struct clps711x_fb_info {
+	struct clk		*clk;
+	void __iomem		*base;
+	struct regmap		*syscon;
+	resource_size_t		buffsize;
+	struct fb_videomode	mode;
+	struct regulator	*lcd_pwr;
+	u32			ac_prescale;
+	bool			cmap_invert;
+};
+
+static int clps711x_fb_setcolreg(u_int regno, u_int red, u_int green,
+				 u_int blue, u_int transp, struct fb_info *info)
+{
+	struct clps711x_fb_info *cfb = info->par;
+	u32 level, mask, shift;
+
+	if (regno >= BIT(info->var.bits_per_pixel))
+		return -EINVAL;
+
+	shift = 4 * (regno & 7);
+	mask  = 0xf << shift;
+	/* gray = 0.30*R + 0.58*G + 0.11*B */
+	level = (((red * 77 + green * 151 + blue * 28) >> 20) << shift) & mask;
+	if (cfb->cmap_invert)
+		level = 0xf - level;
+
+	regno = (regno < 8) ? CLPS711X_PALLSW : CLPS711X_PALMSW;
+
+	writel((readl(cfb->base + regno) & ~mask) | level, cfb->base + regno);
+
+	return 0;
+}
+
+static int clps711x_fb_check_var(struct fb_var_screeninfo *var,
+				 struct fb_info *info)
+{
+	u32 val;
+
+	if (var->bits_per_pixel < 1 ||
+	    var->bits_per_pixel > CLPS711X_FB_BPP_MAX)
+		return -EINVAL;
+
+	if (!var->pixclock)
+		return -EINVAL;
+
+	val = DIV_ROUND_UP(var->xres, 16) - 1;
+	if (val < 0x01 || val > 0x3f)
+		return -EINVAL;
+
+	val = DIV_ROUND_UP(var->yres * var->xres * var->bits_per_pixel, 128);
+	val--;
+	if (val < 0x001 || val > 0x1fff)
+		return -EINVAL;
+
+	var->transp.msb_right	= 0;
+	var->transp.offset	= 0;
+	var->transp.length	= 0;
+	var->red.msb_right	= 0;
+	var->red.offset		= 0;
+	var->red.length		= var->bits_per_pixel;
+	var->green		= var->red;
+	var->blue		= var->red;
+	var->grayscale		= var->bits_per_pixel > 1;
+
+	return 0;
+}
+
+static int clps711x_fb_set_par(struct fb_info *info)
+{
+	struct clps711x_fb_info *cfb = info->par;
+	resource_size_t size;
+	u32 lcdcon, pps;
+
+	size = (info->var.xres * info->var.yres * info->var.bits_per_pixel) / 8;
+	if (size > cfb->buffsize)
+		return -EINVAL;
+
+	switch (info->var.bits_per_pixel) {
+	case 1:
+		info->fix.visual = FB_VISUAL_MONO01;
+		break;
+	case 2:
+	case 4:
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	info->fix.line_length = info->var.xres * info->var.bits_per_pixel / 8;
+	info->fix.smem_len = size;
+
+	lcdcon = (info->var.xres * info->var.yres *
+		  info->var.bits_per_pixel) / 128 - 1;
+	lcdcon |= ((info->var.xres / 16) - 1) << 13;
+	lcdcon |= (cfb->ac_prescale & 0x1f) << 25;
+
+	pps = clk_get_rate(cfb->clk) / (PICOS2KHZ(info->var.pixclock) * 1000);
+	if (pps)
+		pps--;
+	lcdcon |= (pps & 0x3f) << 19;
+
+	if (info->var.bits_per_pixel = 4)
+		lcdcon |= LCDCON_GSMD;
+	if (info->var.bits_per_pixel >= 2)
+		lcdcon |= LCDCON_GSEN;
+
+	/* LCDCON must only be changed while the LCD is disabled */
+	regmap_update_bits(cfb->syscon, SYSCON_OFFSET, SYSCON1_LCDEN, 0);
+	writel(lcdcon, cfb->base + CLPS711X_LCDCON);
+	regmap_update_bits(cfb->syscon, SYSCON_OFFSET,
+			   SYSCON1_LCDEN, SYSCON1_LCDEN);
+
+	return 0;
+}
+
+static int clps711x_fb_blank(int blank, struct fb_info *info)
+{
+	/* Return happy */
+	return 0;
+}
+
+static struct fb_ops clps711x_fb_ops = {
+	.owner		= THIS_MODULE,
+	.fb_setcolreg	= clps711x_fb_setcolreg,
+	.fb_check_var	= clps711x_fb_check_var,
+	.fb_set_par	= clps711x_fb_set_par,
+	.fb_blank	= clps711x_fb_blank,
+	.fb_fillrect	= sys_fillrect,
+	.fb_copyarea	= sys_copyarea,
+	.fb_imageblit	= sys_imageblit,
+};
+
+static int clps711x_lcd_check_fb(struct lcd_device *lcddev, struct fb_info *fi)
+{
+	struct clps711x_fb_info *cfb = dev_get_drvdata(&lcddev->dev);
+
+	return (!fi || fi->par = cfb) ? 1 : 0;
+}
+
+static int clps711x_lcd_get_power(struct lcd_device *lcddev)
+{
+	struct clps711x_fb_info *cfb = dev_get_drvdata(&lcddev->dev);
+
+	if (!IS_ERR_OR_NULL(cfb->lcd_pwr))
+		return regulator_is_enabled(cfb->lcd_pwr);
+
+	return 1;
+}
+
+static int clps711x_lcd_set_power(struct lcd_device *lcddev, int power)
+{
+	struct clps711x_fb_info *cfb = dev_get_drvdata(&lcddev->dev);
+
+	if (!IS_ERR_OR_NULL(cfb->lcd_pwr)) {
+		if (power)
+			return regulator_enable(cfb->lcd_pwr);
+		else
+			return regulator_disable(cfb->lcd_pwr);
+	}
+
+	return 0;
+}
+
+static struct lcd_ops clps711x_lcd_ops = {
+	.check_fb	= clps711x_lcd_check_fb,
+	.get_power	= clps711x_lcd_get_power,
+	.set_power	= clps711x_lcd_set_power,
+};
+
+static int clps711x_fb_get_mode(struct platform_device *pdev)
+{
+	struct fb_info *info = platform_get_drvdata(pdev);
+	bool *pinvert = dev_get_platdata(&pdev->dev);
+	struct clps711x_fb_info *cfb = info->par;
+	unsigned long pixclk;
+	unsigned int val;
+	int ret;
+
+	cfb->syscon = syscon_regmap_lookup_by_pdevname("syscon.1");
+	if (IS_ERR(cfb->syscon))
+		return PTR_ERR(cfb->syscon);
+
+	ret = regmap_read(cfb->syscon, SYSCON_OFFSET, &val);
+	if (ret)
+		return ret;
+
+	if (pinvert)
+		cfb->cmap_invert = *pinvert;
+
+	/* Get mode from LCD if active */
+	if (val & SYSCON1_LCDEN) {
+		val = readl(cfb->base + CLPS711X_LCDCON);
+
+		switch (val & (LCDCON_GSMD | LCDCON_GSEN)) {
+		case LCDCON_GSMD | LCDCON_GSEN:
+			info->var.bits_per_pixel = 4;
+			break;
+		case LCDCON_GSEN:
+			info->var.bits_per_pixel = 2;
+			break;
+		default:
+			info->var.bits_per_pixel = 1;
+			break;
+		}
+
+		cfb->mode.xres = (((val >> 13) & 0x3f) + 1) * 16;
+		cfb->mode.yres = (((val & 0x1fff) + 1) * 128) /
+				 (cfb->mode.xres * info->var.bits_per_pixel);
+		cfb->ac_prescale = (val >> 25) & 0x1f;
+		pixclk = clk_get_rate(cfb->clk) / (((val >> 19) & 0x3f) + 1);
+	} else {
+		/* Fixed setup for existing users */
+		info->var.bits_per_pixel = 4;
+		cfb->mode.xres = 640;
+		cfb->mode.yres = 240;
+		cfb->ac_prescale = 13;
+		pixclk = 10000000;
+	}
+
+	cfb->mode.refresh = pixclk / (cfb->mode.xres * cfb->mode.yres);
+	cfb->mode.pixclock = KHZ2PICOS(pixclk / 1000);
+
+	return 0;
+}
+
+static int clps711x_fb_get_mode_dt(struct platform_device *pdev)
+{
+	struct device_node *disp, *np = pdev->dev.of_node;
+	struct fb_info *info = platform_get_drvdata(pdev);
+	struct clps711x_fb_info *cfb = info->par;
+	int ret;
+
+	cfb->syscon +		syscon_regmap_lookup_by_compatible("cirrus,clps711x-syscon1");
+	if (IS_ERR(cfb->syscon))
+		return PTR_ERR(cfb->syscon);
+
+	disp = of_parse_phandle(np, "display", 0);
+	if (!disp) {
+		dev_err(&pdev->dev, "No display defined\n");
+		return -ENODATA;
+	}
+
+	ret = of_get_fb_videomode(disp, &cfb->mode, OF_USE_NATIVE_MODE);
+	if (ret)
+		return ret;
+
+	of_property_read_u32(disp, "ac-prescale", &cfb->ac_prescale);
+	cfb->cmap_invert = of_property_read_bool(disp, "cmap-invert");
+
+	return of_property_read_u32(disp, "bits-per-pixel",
+				    &info->var.bits_per_pixel);
+}
+
+static int clps711x_fb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct clps711x_fb_info *cfb;
+	struct lcd_device *lcd;
+	struct fb_info *info;
+	struct resource *res;
+	int ret = -ENOENT;
+	u32 val;
+
+	if (fb_get_options(CLPS711X_FB_NAME, NULL))
+		return -ENODEV;
+
+	info = framebuffer_alloc(sizeof(*cfb), dev);
+	if (!info)
+		return -ENOMEM;
+
+	cfb = info->par;
+	platform_set_drvdata(pdev, info);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		goto out_fb_release;
+	cfb->base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!cfb->base) {
+		ret = -ENOMEM;
+		goto out_fb_release;
+	}
+
+	info->fix.mmio_start = res->start;
+	info->fix.mmio_len = resource_size(res);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	info->screen_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(info->screen_base)) {
+		ret = PTR_ERR(info->screen_base);
+		goto out_fb_release;
+	}
+
+	/* Physical address should be aligned to 256 MiB */
+	if (res->start & 0x0fffffff) {
+		ret = -EINVAL;
+		goto out_fb_release;
+	}
+
+	info->apertures = alloc_apertures(1);
+	if (!info->apertures) {
+		ret = -ENOMEM;
+		goto out_fb_release;
+	}
+
+	cfb->buffsize = resource_size(res);
+	info->fix.smem_start = res->start;
+	info->apertures->ranges[0].base = info->fix.smem_start;
+	info->apertures->ranges[0].size = cfb->buffsize;
+
+	cfb->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(cfb->clk)) {
+		ret = PTR_ERR(cfb->clk);
+		goto out_fb_release;
+	}
+
+	ret = np ? clps711x_fb_get_mode_dt(pdev) : clps711x_fb_get_mode(pdev);
+	if (ret)
+		goto out_fb_release;
+
+	/* Force disable LCD on any mismatch */
+	if (info->fix.smem_start != (readb(cfb->base + CLPS711X_FBADDR) << 28))
+		regmap_update_bits(cfb->syscon, SYSCON_OFFSET,
+				   SYSCON1_LCDEN, 0);
+
+	ret = regmap_read(cfb->syscon, SYSCON_OFFSET, &val);
+	if (ret)
+		goto out_fb_release;
+
+	if (!(val & SYSCON1_LCDEN)) {
+		/* Setup start FB address */
+		writeb(info->fix.smem_start >> 28, cfb->base + CLPS711X_FBADDR);
+		/* Clean FB memory */
+		memset(info->screen_base, 0, cfb->buffsize);
+	}
+
+	cfb->lcd_pwr = devm_regulator_get(dev, "lcd");
+	if (PTR_ERR(cfb->lcd_pwr) = -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto out_fb_release;
+	}
+
+	info->fbops = &clps711x_fb_ops;
+	info->flags = FBINFO_DEFAULT;
+	info->var.activate = FB_ACTIVATE_FORCE | FB_ACTIVATE_NOW;
+	info->var.height = -1;
+	info->var.width = -1;
+	info->var.vmode = FB_VMODE_NONINTERLACED;
+	info->fix.type = FB_TYPE_PACKED_PIXELS;
+	info->fix.accel = FB_ACCEL_NONE;
+	strlcpy(info->fix.id, CLPS711X_FB_NAME, sizeof(info->fix.id));
+	fb_videomode_to_var(&info->var, &cfb->mode);
+
+	ret = fb_alloc_cmap(&info->cmap, BIT(CLPS711X_FB_BPP_MAX), 0);
+	if (ret)
+		goto out_fb_release;
+
+	ret = fb_set_var(info, &info->var);
+	if (ret)
+		goto out_fb_dealloc_cmap;
+
+	ret = register_framebuffer(info);
+	if (ret)
+		goto out_fb_dealloc_cmap;
+
+	lcd = devm_lcd_device_register(dev, "clps711x-lcd", dev, cfb,
+				       &clps711x_lcd_ops);
+	if (!IS_ERR(lcd))
+		return 0;
+	
+	ret = PTR_ERR(lcd);
+	unregister_framebuffer(info);
+
+out_fb_dealloc_cmap:
+	regmap_update_bits(cfb->syscon, SYSCON_OFFSET, SYSCON1_LCDEN, 0);
+	fb_dealloc_cmap(&info->cmap);
+
+out_fb_release:
+	framebuffer_release(info);
+
+	return ret;
+}
+
+static int clps711x_fb_remove(struct platform_device *pdev)
+{
+	struct fb_info *info = platform_get_drvdata(pdev);
+	struct clps711x_fb_info *cfb = info->par;
+
+	regmap_update_bits(cfb->syscon, SYSCON_OFFSET, SYSCON1_LCDEN, 0);
+
+	unregister_framebuffer(info);
+	fb_dealloc_cmap(&info->cmap);
+	framebuffer_release(info);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused clps711x_fb_dt_ids[] = {
+	{ .compatible = "cirrus,clps711x-fb", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, clps711x_fb_dt_ids);
+
+static struct platform_driver clps711x_fb_driver = {
+	.driver	= {
+		.name		= CLPS711X_FB_NAME,
+		.owner		= THIS_MODULE,
+		.of_match_table	= of_match_ptr(clps711x_fb_dt_ids),
+	},
+	.probe	= clps711x_fb_probe,
+	.remove	= clps711x_fb_remove,
+};
+module_platform_driver(clps711x_fb_driver);
+
+MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>");
+MODULE_DESCRIPTION("Cirrus Logic CLPS711X FB driver");
+MODULE_LICENSE("GPL");
-- 
1.8.3.2

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Alexander Shiyan]

On Sat, 12 Apr 2014 10:53:02 +0400
Alexander Shiyan <shc_work@mail.ru> wrote:

Ping.

> This adds support for the framebuffer available in the Cirrus
> Logic CLPS711X CPUs.
> FB features:
> - 1-2-4 bits per pixel.
> - Programmable panel size to a maximum of 1024x256 at 4 bps.
> - Relocatible Frame Buffer (SRAM or SDRAM).
> - Programmable refresh rates.
> - 16 gray scale values.
> This new driver supports usage with devicetree and as a general
> change it removes last user of <mach/hardware.h> for CLPS711X targets,
> so this subarch will fully prepared to switch to multiplatform.
> The driver have been tested with custom board equipped Cirrus Logic
> EP7312 in DT and non-DT mode.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

...

-- 
Alexander Shiyan <shc_work@mail.ru>

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 918 bytes --]

Hi,

On 12/04/14 09:53, Alexander Shiyan wrote:
> This adds support for the framebuffer available in the Cirrus
> Logic CLPS711X CPUs.
> FB features:
> - 1-2-4 bits per pixel.
> - Programmable panel size to a maximum of 1024x256 at 4 bps.
> - Relocatible Frame Buffer (SRAM or SDRAM).
> - Programmable refresh rates.
> - 16 gray scale values.
> This new driver supports usage with devicetree and as a general
> change it removes last user of <mach/hardware.h> for CLPS711X targets,
> so this subarch will fully prepared to switch to multiplatform.
> The driver have been tested with custom board equipped Cirrus Logic
> EP7312 in DT and non-DT mode.

My original comment about this is still unanswered: why a totally new
driver? The proper way would be to gradually change the old driver with
a patch series. Then it's possible to review the patches and see what is
actually changed.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]

On 24/04/14 11:10, Alexander Shiyan wrote:
> Hello.
> 
> Thu, 24 Apr 2014 10:57:59 +0300 от Tomi Valkeinen <tomi.valkeinen@ti.com>:
>> Hi,
>>
>> On 12/04/14 09:53, Alexander Shiyan wrote:
>>> This adds support for the framebuffer available in the Cirrus
>>> Logic CLPS711X CPUs.
>>> FB features:
>>> - 1-2-4 bits per pixel.
>>> - Programmable panel size to a maximum of 1024x256 at 4 bps.
>>> - Relocatible Frame Buffer (SRAM or SDRAM).
>>> - Programmable refresh rates.
>>> - 16 gray scale values.
>>> This new driver supports usage with devicetree and as a general
>>> change it removes last user of <mach/hardware.h> for CLPS711X targets,
>>> so this subarch will fully prepared to switch to multiplatform.
>>> The driver have been tested with custom board equipped Cirrus Logic
>>> EP7312 in DT and non-DT mode.
>>
>> My original comment about this is still unanswered: why a totally new
>> driver? The proper way would be to gradually change the old driver with
>> a patch series. Then it's possible to review the patches and see what is
>> actually changed.
> 
> I have tried to answer here:
> http://www.spinics.net/lists/linux-fbdev/msg14218.html

If your answer means "it will be very difficult to see the changes if
all the changes are in one patch, which change the old driver in one
go", then yes, very true.

But that's wrong way to do it.

The right way to do it is, as I wrote above, by gradually changing the
old driver with a patch series. And my question is, why not do it that
way? Then it would be possible to review the patches one by one, seeing
what has changed.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

On 24/04/14 19:35, Alexander Shiyan wrote:

>> The right way to do it is, as I wrote above, by gradually changing the
>> old driver with a patch series. And my question is, why not do it that
>> way? Then it would be possible to review the patches one by one, seeing
>> what has changed.
> 
> "gradually changing"...
> I repeat that this is not an old modified driver, but written new.

Yes, I understand that. Again, my question is, why didn't you modify the
old driver? That's how things should normally be done. Instead, you made
a totally new one, making proper review against the old driver impossible.

> if you imagine a new file as a diff to the old, this can be clearly seen.
> 
> There is no reason to waste time on a series of changes since I
> can not even check these changes on real hardware, but only in the
> last stage when the driver will be the current version.

Hmm what? So is the old driver totally broken, and cannot be used at the
moment? Or why you can't test on real hardware?

Note that I don't know anything about the fb hardware in question, nor
the driver. Maybe there's a valid reason to write a new driver from
scratch. But there very rarely is.

And "because I already wrote a new driver, and it's a waste of time for
me to throw away my work and patch the old one", is not a very good reason.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Olof Johansson]

On Wed, Apr 30, 2014 at 04:36:29PM +0400, Alexander Shiyan wrote:
> Wed, 30 Apr 2014 14:14:36 +0300 от Tomi Valkeinen <tomi.valkeinen@ti.com>:
> > On 24/04/14 19:35, Alexander Shiyan wrote:
> 
> Hello.
> 
> I'm a reorder quotes a bit for convenience.
> 
> > >> The right way to do it is, as I wrote above, by gradually changing the
> > >> old driver with a patch series. And my question is, why not do it that
> > >> way? Then it would be possible to review the patches one by one, seeing
> > >> what has changed.
> > > 
> > > "gradually changing"...
> > > I repeat that this is not an old modified driver, but written new.
> > 
> > Yes, I understand that. Again, my question is, why didn't you modify the
> > old driver? That's how things should normally be done. Instead, you made
> > a totally new one, making proper review against the old driver impossible.
> ...
> > Hmm what? So is the old driver totally broken, and cannot be used at the
> > moment? Or why you can't test on real hardware?
> 
> Firstly, the driver uses a fixed values for xres, yres, pixclock and specific
> variable ac_precale.
> Secondly, the driver uses a fixed value for the physical address of the buffer.
> Totally, it does not give me the ability to use the driver in the current state.
> Unlikely that this will look good if I make these two significant changes in
> a single patch...
> 
> At this time the driver has three user.
> Only one of them should theoretically work.
> clps711x-autcpu12 should not work in the absence of memblock_reserve().
> clps711x-p720t should not work due to physical address limitation as i
> noticed before. Board means to use SRAM instead of SDRAM.
> Only clps711x-edb7211 should work fine (in theory).
> Is this a good reason to replace the driver? I think yes.
> 
> > Note that I don't know anything about the fb hardware in question, nor
> > the driver. Maybe there's a valid reason to write a new driver from
> > scratch. But there very rarely is.
> > 
> > And "because I already wrote a new driver, and it's a waste of time for
> > me to throw away my work and patch the old one", is not a very good reason.
> 
> > > if you imagine a new file as a diff to the old, this can be clearly seen.
> > > 
> > > There is no reason to waste time on a series of changes since I
> > > can not even check these changes on real hardware, but only in the
> > > last stage when the driver will be the current version.
> 
> Summing up, I want to ask why the driver can not be reviewed as a
> new and not compared to the old?
> And yes, the time can be spent on more productive things to do than
> to create a series, leading eventually to the same result.
> As far as I know, guide to creating kernel patches allows such cases.

We've definitely had cases like that in the past. Sometimes it's easier
to first post a patch that removes the old driver, then one that submits
the new one as a new piece of work. That, of course, assumes that the
driver indeed was a rewrite and not just a bunch of incremental changes
all squashed into one.


-Olof

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3208 bytes --]

On 30/04/14 15:36, Alexander Shiyan wrote:

>> Hmm what? So is the old driver totally broken, and cannot be used at the
>> moment? Or why you can't test on real hardware?
> 
> Firstly, the driver uses a fixed values for xres, yres, pixclock and specific
> variable ac_precale.
> Secondly, the driver uses a fixed value for the physical address of the buffer.
> Totally, it does not give me the ability to use the driver in the current state.
> Unlikely that this will look good if I make these two significant changes in
> a single patch...
> 
> At this time the driver has three user.
> Only one of them should theoretically work.
> clps711x-autcpu12 should not work in the absence of memblock_reserve().
> clps711x-p720t should not work due to physical address limitation as i
> noticed before. Board means to use SRAM instead of SDRAM.
> Only clps711x-edb7211 should work fine (in theory).
> Is this a good reason to replace the driver? I think yes.

Ok, if the situation is that bad, maybe we can just switch to the new
driver. Have you verified that those boards do not work from anyone? Or
asked someone to test the new driver with those boards?

I'm still not really happy about it, and I'd much rather see the current
driver fixed. But if no one having those boards is up to the task
(probably not if they have not been working at all), maybe just ditching
the old driver and adding a new is the only way forward.

One change that I think would be good is to change the series to first
remove the old driver, and then add the new one, with the same file name
as the old one. That way git log will show the history for both the new
and the old driver.

>> Note that I don't know anything about the fb hardware in question, nor
>> the driver. Maybe there's a valid reason to write a new driver from
>> scratch. But there very rarely is.
>>
>> And "because I already wrote a new driver, and it's a waste of time for
>> me to throw away my work and patch the old one", is not a very good reason.
> 
>>> if you imagine a new file as a diff to the old, this can be clearly seen.
>>>
>>> There is no reason to waste time on a series of changes since I
>>> can not even check these changes on real hardware, but only in the
>>> last stage when the driver will be the current version.
> 
> Summing up, I want to ask why the driver can not be reviewed as a
> new and not compared to the old?

It can, of course. But we normally should not.

Fixing the old driver will keep all the fixes and tweaks that have been
debugged and solved with the old driver. Creating a new driver easily
makes those old fixes disappear.

Fixing the old driver also keeps the history, making it possible to see
where an issue was introduced etc.

> And yes, the time can be spent on more productive things to do than
> to create a series, leading eventually to the same result.

Yes, your time. But if, say, the new driver introduces bugs that already
were fixed in the old driver, causing problems to other people, and to
me as the maintainer, I'm sure the other people and me are not going to
say "well this is ok, as this saved Alexander's time" =).

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2011 bytes --]

On 08/05/14 11:27, Alexander Shiyan wrote:

>>> At this time the driver has three user.
>>> Only one of them should theoretically work.
>>> clps711x-autcpu12 should not work in the absence of memblock_reserve().
>>> clps711x-p720t should not work due to physical address limitation as i
>>> noticed before. Board means to use SRAM instead of SDRAM.
>>> Only clps711x-edb7211 should work fine (in theory).
>>> Is this a good reason to replace the driver? I think yes.
>>
>> Ok, if the situation is that bad, maybe we can just switch to the new
>> driver. Have you verified that those boards do not work from anyone? Or
>> asked someone to test the new driver with those boards?
> 
> I'm not familiar with other users of this platform .
> I am do not have these boards, all that I have written before that it's just a theory.
> Firm in which I work, uses its own board with CLPS711X CPU , this board is the
> only way to check for changes on real hardware .

Ok. That makes me a bit nervous... You're removing a driver, which may
(or may not) have been working for other users. And adding a new one,
which may not (or may) work for the other users.

>> I'm still not really happy about it, and I'd much rather see the current
>> driver fixed. But if no one having those boards is up to the task
>> (probably not if they have not been working at all), maybe just ditching
>> the old driver and adding a new is the only way forward.
>>
>> One change that I think would be good is to change the series to first
>> remove the old driver, and then add the new one, with the same file name
>> as the old one. That way git log will show the history for both the new
>> and the old driver.
> 
> In this case git-bisect will be broken. Is this OK?

I think this is such a big change for the users of the driver that it's
not an issue. Of course, kernel should still build at all steps, but I
think it's fine if the fb driver in question will be out for a commit or
two.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Olof Johansson]

On Thu, May 08, 2014 at 01:14:40PM +0300, Tomi Valkeinen wrote:
> On 08/05/14 11:27, Alexander Shiyan wrote:
> 
> >>> At this time the driver has three user.
> >>> Only one of them should theoretically work.
> >>> clps711x-autcpu12 should not work in the absence of memblock_reserve().
> >>> clps711x-p720t should not work due to physical address limitation as i
> >>> noticed before. Board means to use SRAM instead of SDRAM.
> >>> Only clps711x-edb7211 should work fine (in theory).
> >>> Is this a good reason to replace the driver? I think yes.
> >>
> >> Ok, if the situation is that bad, maybe we can just switch to the new
> >> driver. Have you verified that those boards do not work from anyone? Or
> >> asked someone to test the new driver with those boards?
> > 
> > I'm not familiar with other users of this platform .
> > I am do not have these boards, all that I have written before that it's just a theory.
> > Firm in which I work, uses its own board with CLPS711X CPU , this board is the
> > only way to check for changes on real hardware .
> 
> Ok. That makes me a bit nervous... You're removing a driver, which may
> (or may not) have been working for other users. And adding a new one,
> which may not (or may) work for the other users.

Keep the old one around under another Kconfig name, mark it BROKEN,
and if nobody speaks up in a couple of releases, remove it?

> >> I'm still not really happy about it, and I'd much rather see the current
> >> driver fixed. But if no one having those boards is up to the task
> >> (probably not if they have not been working at all), maybe just ditching
> >> the old driver and adding a new is the only way forward.
> >>
> >> One change that I think would be good is to change the series to first
> >> remove the old driver, and then add the new one, with the same file name
> >> as the old one. That way git log will show the history for both the new
> >> and the old driver.
> > 
> > In this case git-bisect will be broken. Is this OK?
> 
> I think this is such a big change for the users of the driver that it's
> not an issue. Of course, kernel should still build at all steps, but I
> think it's fine if the fb driver in question will be out for a commit or
> two.

Agreed.


-Olof

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On 12/04/14 09:53, Alexander Shiyan wrote:
> This adds support for the framebuffer available in the Cirrus
> Logic CLPS711X CPUs.
> FB features:
> - 1-2-4 bits per pixel.
> - Programmable panel size to a maximum of 1024x256 at 4 bps.
> - Relocatible Frame Buffer (SRAM or SDRAM).
> - Programmable refresh rates.
> - 16 gray scale values.
> This new driver supports usage with devicetree and as a general
> change it removes last user of <mach/hardware.h> for CLPS711X targets,
> so this subarch will fully prepared to switch to multiplatform.
> The driver have been tested with custom board equipped Cirrus Logic
> EP7312 in DT and non-DT mode.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/video/fbdev/clps711x-fb.c | 456 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 456 insertions(+)
>  create mode 100644 drivers/video/fbdev/clps711x-fb.c

<snip>

> +
> +static int clps711x_fb_get_mode_dt(struct platform_device *pdev)
> +{
> +	struct device_node *disp, *np = pdev->dev.of_node;
> +	struct fb_info *info = platform_get_drvdata(pdev);
> +	struct clps711x_fb_info *cfb = info->par;
> +	int ret;
> +
> +	cfb->syscon =
> +		syscon_regmap_lookup_by_compatible("cirrus,clps711x-syscon1");
> +	if (IS_ERR(cfb->syscon))
> +		return PTR_ERR(cfb->syscon);

Hmm, what's the syscon stuff about? Looks like it's required, but the DT
documentation patch doesn't mention it at all.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2884 bytes --]

On 18/05/14 15:01, Alexander Shiyan wrote:
> Fri, 16 May 2014 15:56:26 -0700 от Olof Johansson <olof@lixom.net>:
>> On Thu, May 08, 2014 at 01:14:40PM +0300, Tomi Valkeinen wrote:
>>> On 08/05/14 11:27, Alexander Shiyan wrote:
>>>
>>>>>> At this time the driver has three user.
>>>>>> Only one of them should theoretically work.
>>>>>> clps711x-autcpu12 should not work in the absence of memblock_reserve().
>>>>>> clps711x-p720t should not work due to physical address limitation as i
>>>>>> noticed before. Board means to use SRAM instead of SDRAM.
>>>>>> Only clps711x-edb7211 should work fine (in theory).
>>>>>> Is this a good reason to replace the driver? I think yes.
>>>>>
>>>>> Ok, if the situation is that bad, maybe we can just switch to the new
>>>>> driver. Have you verified that those boards do not work from anyone? Or
>>>>> asked someone to test the new driver with those boards?
>>>>
>>>> I'm not familiar with other users of this platform .
>>>> I am do not have these boards, all that I have written before that it's just a theory.
>>>> Firm in which I work, uses its own board with CLPS711X CPU , this board is the
>>>> only way to check for changes on real hardware .
>>>
>>> Ok. That makes me a bit nervous... You're removing a driver, which may
>>> (or may not) have been working for other users. And adding a new one,
>>> which may not (or may) work for the other users.
>>
>> Keep the old one around under another Kconfig name, mark it BROKEN,
>> and if nobody speaks up in a couple of releases, remove it?
> 
> I like this variant, Tomi are you agree with this?

I don't know. All options sound somewhat bad to me, except if it's clear
nobody uses the old driver.

Anyway, it's rather late for 3.16. I'd like the removal of the old
driver to sit in the linux-next for a while.

Would it be possible to add the new driver along the old driver, and use
the new driver only for the boards you have, and for boards for which
it's clear the the old driver is not working? This could be merged for 3.16.

It's not so nice to have two drivers for the same hardware, but in this
case maybe that's not an issue. The old driver doesn't support DT, so no
conflicts there, and for non-DT the driver names seem to be different.

For 3.17, we could remove the old driver, and push that change to
linux-next right after the 3.16 merge window has closed.

Or, if you're fine with it, we can also delay adding the new driver to
3.17, and do both right after the 3.16 merge window has closed. I
personally like this most, so that we have both removal of the old and
adding the new sitting in the linux-next longer, but you might possibly
want the new driver in sooner.

Maybe I'm being overly cautious here, but as I don't have any idea about
the driver and its users, I rather be too cautious than not.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Arnd Bergmann]

On Friday 23 May 2014 17:13:58 Alexander Shiyan wrote:
> If there will be two drivers, I will do the following: remove the non-DT
> support (for new driver) and will create a patch for 3.16 (this patch will
> no affect to arm-soc).
> After that, I will do optional multiplatform support for this CPU and move
> the boards, which do not use FB.
> After this architecture will be ready to add DT support, and after all boards
> will be converted, I'll remove the old version of the driver.
> 
> OK?
> 

Sounds good to me.

	Arnd

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1745 bytes --]

On 23/05/14 16:13, Alexander Shiyan wrote:

>> Would it be possible to add the new driver along the old driver, and use
>> the new driver only for the boards you have, and for boards for which
>> it's clear the the old driver is not working? This could be merged for 3.16.
> 
> At this time yes, we can. But since I plan to add multiplatform support
> for this SOC, this seems not possible.
> I can try to make multiplatform support optional, then it could be done...

Hmm, why is that not possible with multiplatform support? What do you
mean with multiplatform support here?

While the drivers would handle the same device, if they have different
names then they are different device drivers from Linux's perspective.
Why can't one board use the old driver, and an other board use the new
driver?

> If there will be two drivers, I will do the following: remove the non-DT
> support (for new driver) and will create a patch for 3.16 (this patch will
> no affect to arm-soc).

There would be no one using the driver in 3.16, then, right?

> After that, I will do optional multiplatform support for this CPU and move
> the boards, which do not use FB.
> After this architecture will be ready to add DT support, and after all boards
> will be converted, I'll remove the old version of the driver.
> 
> OK?

I'm a bit unclear what the multiplatform stuff means here, but yes,
generally sounds ok.

But I don't want to make this more difficult for you than it needs to.
As I said, I'm fine with the current patches, if we skip 3.16 and get
them to linux-next right after the merge window. If nobody would use the
new driver in 3.16 anyway (in your proposal above), would this be the
easiest way?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Arnd Bergmann]

On Friday 23 May 2014 17:10:35 Tomi Valkeinen wrote:
> On 23/05/14 16:13, Alexander Shiyan wrote:
> 
> >> Would it be possible to add the new driver along the old driver, and use
> >> the new driver only for the boards you have, and for boards for which
> >> it's clear the the old driver is not working? This could be merged for 3.16.
> > 
> > At this time yes, we can. But since I plan to add multiplatform support
> > for this SOC, this seems not possible.
> > I can try to make multiplatform support optional, then it could be done...
> 
> Hmm, why is that not possible with multiplatform support? What do you
> mean with multiplatform support here?

We are migrating all ARM platforms to allow building them into a single
kernel. However, that means that device drivers cannot access platform
specific header files any more and have to get hardware information from
DT or through platform data. The existing driver however uses mach/hardware.h.

	Arnd

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On 23/05/14 17:14, Arnd Bergmann wrote:
> On Friday 23 May 2014 17:10:35 Tomi Valkeinen wrote:
>> On 23/05/14 16:13, Alexander Shiyan wrote:
>>
>>>> Would it be possible to add the new driver along the old driver, and use
>>>> the new driver only for the boards you have, and for boards for which
>>>> it's clear the the old driver is not working? This could be merged for 3.16.
>>>
>>> At this time yes, we can. But since I plan to add multiplatform support
>>> for this SOC, this seems not possible.
>>> I can try to make multiplatform support optional, then it could be done...
>>
>> Hmm, why is that not possible with multiplatform support? What do you
>> mean with multiplatform support here?
> 
> We are migrating all ARM platforms to allow building them into a single
> kernel. However, that means that device drivers cannot access platform
> specific header files any more and have to get hardware information from
> DT or through platform data. The existing driver however uses mach/hardware.h.

Ah, I see. So the problem is not with two different drivers for the same
hardware device as such, but that in this case the older driver just
doesn't play well with multiplatform.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 1/3] video: clps711x: Add new Cirrus Logic CLPS711X framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On 23/05/14 17:27, Alexander Shiyan wrote:

>> But I don't want to make this more difficult for you than it needs to.
>> As I said, I'm fine with the current patches, if we skip 3.16 and get
>> them to linux-next right after the merge window. If nobody would use the
>> new driver in 3.16 anyway (in your proposal above), would this be the
>> easiest way?
> 
> This is the only way to announce initial multiplatform support (in case we will
> use two differrent drivers).
> Another (easiest) way is to use the current patchset...

If the old driver doesn't even play well with multiplatform, and is thus
blocking moving the SoC to multiplatform, I'd just get done with it in
one go.

So I would recommend pushing the new driver and the removal of the old
one for 3.17, and having those changes in linux-next right after the
3.16 merge window.

Is that ok?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]