Linux Framebuffer Layer development
 help / color / mirror / Atom feed
* Re: [PATCH] RFC: framebuffer: provide generic get_fb_unmapped_area
From: Uwe Kleine-König @ 2013-11-18 19:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20131118185959.GE19318@pengutronix.de>

Hello again,

On Mon, Nov 18, 2013 at 07:59:59PM +0100, Uwe Kleine-König wrote:
> 	if (pgoff > screen_size || pgoff + len > screen_size)
This must be:

	if (pgoff > screen_size || len > screen_size - pgoff)

to do what I intended.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

^ permalink raw reply

* [PATCH] video: kyro: fix incorrect sizes when copying to userspace
From: Sasha Levin @ 2013-11-19 19:25 UTC (permalink / raw)
  To: plagnioj, tomi.valkeinen; +Cc: gregkh, linux-fbdev, linux-kernel, Sasha Levin

kyro would copy u32s and specify sizeof(unsigned long) as the size to copy.

This would copy more data than intended and cause memory corruption and might
leak kernel memory.

Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
---
 drivers/video/kyro/fbdev.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/kyro/fbdev.c b/drivers/video/kyro/fbdev.c
index 50c8574..65041e1 100644
--- a/drivers/video/kyro/fbdev.c
+++ b/drivers/video/kyro/fbdev.c
@@ -624,15 +624,15 @@ static int kyrofb_ioctl(struct fb_info *info,
 			return -EINVAL;
 		}
 	case KYRO_IOCTL_UVSTRIDE:
-		if (copy_to_user(argp, &deviceInfo.ulOverlayUVStride, sizeof(unsigned long)))
+		if (copy_to_user(argp, &deviceInfo.ulOverlayUVStride, sizeof(deviceInfo.ulOverlayUVStride)))
 			return -EFAULT;
 		break;
 	case KYRO_IOCTL_STRIDE:
-		if (copy_to_user(argp, &deviceInfo.ulOverlayStride, sizeof(unsigned long)))
+		if (copy_to_user(argp, &deviceInfo.ulOverlayStride, sizeof(deviceInfo.ulOverlayStride)))
 			return -EFAULT;
 		break;
 	case KYRO_IOCTL_OVERLAY_OFFSET:
-		if (copy_to_user(argp, &deviceInfo.ulOverlayOffset, sizeof(unsigned long)))
+		if (copy_to_user(argp, &deviceInfo.ulOverlayOffset, sizeof(deviceInfo.ulOverlayOffset)))
 			return -EFAULT;
 		break;
 	}
-- 
1.7.10.4


^ permalink raw reply related

* Re: [PATCHv9][ 1/3] Input: tsc2007: Add device tree support.
From: Dmitry Torokhov @ 2013-11-19 21:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1383916659-9988-1-git-send-email-denis@eukrea.com>

Hi Denis,

On Fri, Nov 08, 2013 at 02:17:37PM +0100, Denis Carikli wrote:
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v8->v9:
> - Added Grant Likely in the Cc list.
> - Removed the mention of the pinctrl properties in the documentation.
> 
> ChangeLog v7->v8:
> - Fixed the lack of x and z fuzz properties.
> - The pendown gpio is better documented.
> - Added Shawn Guo in the cc list.

Does the device still work if you drop the patch below on top of yours?

Thanks!

-- 
Dmitry

Input: tsc2007 misc fixes

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 arch/arm/mach-imx/mach-cpuimx35.c    |    2 -
 arch/arm/mach-imx/mach-cpuimx51sd.c  |    2 -
 arch/sh/boards/mach-ecovec24/setup.c |    2 -
 drivers/input/touchscreen/tsc2007.c  |  126 +++++++++++++---------------------
 include/linux/i2c/tsc2007.h          |    6 +-
 5 files changed, 55 insertions(+), 83 deletions(-)

diff --git a/arch/arm/mach-imx/mach-cpuimx35.c b/arch/arm/mach-imx/mach-cpuimx35.c
index 771362d..65e4c53 100644
--- a/arch/arm/mach-imx/mach-cpuimx35.c
+++ b/arch/arm/mach-imx/mach-cpuimx35.c
@@ -53,7 +53,7 @@ static const struct imxi2c_platform_data
 };
 
 #define TSC2007_IRQGPIO		IMX_GPIO_NR(3, 2)
-static int tsc2007_get_pendown_state(void)
+static int tsc2007_get_pendown_state(struct device *dev)
 {
 	return !gpio_get_value(TSC2007_IRQGPIO);
 }
diff --git a/arch/arm/mach-imx/mach-cpuimx51sd.c b/arch/arm/mach-imx/mach-cpuimx51sd.c
index 9b5ddf5..1fba2b8 100644
--- a/arch/arm/mach-imx/mach-cpuimx51sd.c
+++ b/arch/arm/mach-imx/mach-cpuimx51sd.c
@@ -121,7 +121,7 @@ static const struct imxuart_platform_data uart_pdata __initconst = {
 	.flags = IMXUART_HAVE_RTSCTS,
 };
 
-static int tsc2007_get_pendown_state(void)
+static int tsc2007_get_pendown_state(struct device *dev)
 {
 	if (mx51_revision() < IMX_CHIP_REVISION_3_0)
 		return !gpio_get_value(TSC2007_IRQGPIO_REV2);
diff --git a/arch/sh/boards/mach-ecovec24/setup.c b/arch/sh/boards/mach-ecovec24/setup.c
index 1fa8be4..23d7e45 100644
--- a/arch/sh/boards/mach-ecovec24/setup.c
+++ b/arch/sh/boards/mach-ecovec24/setup.c
@@ -501,7 +501,7 @@ static struct platform_device keysc_device = {
 /* TouchScreen */
 #define IRQ0 evt2irq(0x600)
 
-static int ts_get_pendown_state(void)
+static int ts_get_pendown_state(struct device *dev)
 {
 	int val = 0;
 	gpio_free(GPIO_FN_INTC_IRQ0);
diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index 3168a99..390148c 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -88,15 +88,10 @@ struct tsc2007 {
 	wait_queue_head_t	wait;
 	bool			stopped;
 
-	int			(*get_pendown_state)(void);
+	int			(*get_pendown_state)(struct device *);
 	void			(*clear_penirq)(void);
 };
 
-static int tsc2007_get_pendown_state_dt(struct tsc2007 *ts)
-{
-	return !gpio_get_value(ts->gpio);
-}
-
 static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
 {
 	s32 data;
@@ -155,14 +150,6 @@ static u32 tsc2007_calculate_pressure(struct tsc2007 *tsc, struct ts_event *tc)
 	return rt;
 }
 
-static bool tsc2007_is_pen_down_valid(struct tsc2007 *ts)
-{
-	if (ts->of)
-		return gpio_is_valid(ts->gpio);
-	else
-		return ts->get_pendown_state ? true : false;
-}
-
 static bool tsc2007_is_pen_down(struct tsc2007 *ts)
 {
 	/*
@@ -179,13 +166,10 @@ static bool tsc2007_is_pen_down(struct tsc2007 *ts)
 	 * to fall back on the pressure reading.
 	 */
 
-	if (!tsc2007_is_pen_down_valid(ts))
+	if (!ts->get_pendown_state)
 		return true;
 
-	if (ts->of)
-		return tsc2007_get_pendown_state_dt(ts);
-	else
-		return ts->get_pendown_state();
+	return ts->get_pendown_state(&ts->client->dev);
 }
 
 static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
@@ -202,7 +186,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
 
 		rt = tsc2007_calculate_pressure(ts, &tc);
 
-		if (!rt && !tsc2007_is_pen_down_valid(ts)) {
+		if (!rt && !ts->get_pendown_state) {
 			/*
 			 * If pressure reported is 0 and we don't have
 			 * callback to check pendown state, we have to
@@ -298,13 +282,25 @@ static void tsc2007_close(struct input_dev *input_dev)
 }
 
 #ifdef CONFIG_OF
-static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts,
-			    struct device_node *np)
+static int tsc2007_get_pendown_state_gpio(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct tsc2007 *ts = i2c_get_clientdata(client);
+
+	return !gpio_get_value(ts->gpio);
+}
+
+static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 {
-	int err = 0;
+	struct device_node *np = client->dev.of_node;
 	u32 val32;
 	u64 val64;
 
+	if (!np) {
+		dev_err(&client->dev, "missing device tree data\n");
+		return -EINVAL;
+	}
+
 	if (!of_property_read_u32(np, "ti,max-rt", &val32))
 		ts->max_rt = val32;
 	else
@@ -327,42 +323,32 @@ static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts,
 	if (!of_property_read_u32(np, "ti,x-plate-ohms", &val32)) {
 		ts->x_plate_ohms = val32;
 	} else {
-		dev_err(&client->dev,
-			"Error: lacking ti,x-plate-ohms devicetree property. (err %d).",
-			err);
+		dev_err(&client->dev, "missing ti,x-plate-ohms devicetree property.");
 		return -EINVAL;
 	}
 
 	ts->gpio = of_get_gpio(np, 0);
-	if (!gpio_is_valid(ts->gpio))
-		dev_err(&client->dev,
-			"GPIO not found (of_get_gpio returned %d)\n",
-			ts->gpio);
-
-	/* Used to detect if it is probed trough the device tree,
-	 * in order to be able to use that information in the IRQ handler.
-	 */
-	ts->of = 1;
+	if (gpio_is_valid(ts->gpio))
+		ts->get_pendown_state = tsc2007_get_pendown_state_gpio;
+	else
+		dev_warn(&client->dev,
+			 "GPIO not specified in DT (of_get_gpio returned %d)\n",
+			 ts->gpio);
 
 	return 0;
 }
 #else
-static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts,
-			    struct device_node *np)
+static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts)
 {
-	return -ENODEV;
+	dev_err(&client->dev, "platform data is required!\n");
+	return -EINVAL;
 }
 #endif
 
 static int tsc2007_probe_pdev(struct i2c_client *client, struct tsc2007 *ts,
-			      struct tsc2007_platform_data *pdata,
+			      const struct tsc2007_platform_data *pdata,
 			      const struct i2c_device_id *id)
 {
-	if (!pdata) {
-		dev_err(&client->dev, "platform data is required!\n");
-		return -EINVAL;
-	}
-
 	ts->model             = pdata->model;
 	ts->x_plate_ohms      = pdata->x_plate_ohms;
 	ts->max_rt            = pdata->max_rt ? : MAX_12BIT;
@@ -379,45 +365,40 @@ static int tsc2007_probe_pdev(struct i2c_client *client, struct tsc2007 *ts,
 		return -EINVAL;
 	}
 
-	/* Used to detect if it is probed trough the device tree,
-	 * in order to be able to use that information in the IRQ handler.
-	 */
-	ts->of = 0;
-
 	return 0;
 }
 
 static int tsc2007_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
-	struct device_node *np = client->dev.of_node;
-	struct tsc2007_platform_data *pdata = client->dev.platform_data;
+	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct tsc2007 *ts;
 	struct input_dev *input_dev;
-	int err = 0;
+	int err;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_WORD_DATA))
+		return -EIO;
 
 	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
 	if (!ts)
 		return -ENOMEM;
 
-	if (np)
-		err = tsc2007_probe_dt(client, ts, np);
-	else
+	if (pdata)
 		err = tsc2007_probe_pdev(client, ts, pdata, id);
-
+	else
+		err = tsc2007_probe_dt(client, ts);
 	if (err)
 		return err;
 
-	if (!i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_READ_WORD_DATA))
-		return -EIO;
-
 	input_dev = input_allocate_device();
 	if (!input_dev) {
 		err = -ENOMEM;
 		goto err_free_input;
 	};
 
+	i2c_set_clientdata(client, ts);
+
 	ts->client = client;
 	ts->irq = client->irq;
 	ts->input = input_dev;
@@ -443,10 +424,8 @@ static int tsc2007_probe(struct i2c_client *client,
 	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT,
 			     ts->fuzzz, 0);
 
-	if (!np) {
-		if (pdata->init_platform_hw)
-			pdata->init_platform_hw();
-	}
+	if (pdata && pdata->init_platform_hw)
+		pdata->init_platform_hw();
 
 	err = request_threaded_irq(ts->irq, tsc2007_hard_irq, tsc2007_soft_irq,
 				   IRQF_ONESHOT, client->dev.driver->name, ts);
@@ -461,16 +440,12 @@ static int tsc2007_probe(struct i2c_client *client,
 	if (err)
 		goto err_free_irq;
 
-	i2c_set_clientdata(client, ts);
-
 	return 0;
 
  err_free_irq:
 	free_irq(ts->irq, ts);
-	if (!np) {
-		if (pdata->exit_platform_hw)
-			pdata->exit_platform_hw();
-	}
+	if (pdata && pdata->exit_platform_hw)
+		pdata->exit_platform_hw();
  err_free_input:
 	input_free_device(input_dev);
 	return err;
@@ -478,16 +453,13 @@ static int tsc2007_probe(struct i2c_client *client,
 
 static int tsc2007_remove(struct i2c_client *client)
 {
-	struct device_node *np = client->dev.of_node;
-	struct tsc2007	*ts = i2c_get_clientdata(client);
-	struct tsc2007_platform_data *pdata = client->dev.platform_data;
+	const struct tsc2007_platform_data *pdata = dev_get_platdata(&client->dev);
+	struct tsc2007 *ts = i2c_get_clientdata(client);
 
 	free_irq(ts->irq, ts);
 
-	if (!np) {
-		if (pdata->exit_platform_hw)
-			pdata->exit_platform_hw();
-	}
+	if (pdata && pdata->exit_platform_hw)
+		pdata->exit_platform_hw();
 
 	input_unregister_device(ts->input);
 	kfree(ts);
diff --git a/include/linux/i2c/tsc2007.h b/include/linux/i2c/tsc2007.h
index 506a9f7..041c8e8 100644
--- a/include/linux/i2c/tsc2007.h
+++ b/include/linux/i2c/tsc2007.h
@@ -14,9 +14,9 @@ struct tsc2007_platform_data {
 	int	fuzzy;
 	int	fuzzz;
 
-	int	(*get_pendown_state)(void);
-	void	(*clear_penirq)(void);		/* If needed, clear 2nd level
-						   interrupt source */
+	int	(*get_pendown_state)(struct device *);
+	/* If needed, clear 2nd level interrupt source */
+	void	(*clear_penirq)(void);
 	int	(*init_platform_hw)(void);
 	void	(*exit_platform_hw)(void);
 };

^ permalink raw reply related

* [PATCH v2] video: add OpenCores VGA/LCD framebuffer driver
From: Stefan Kristiansson @ 2013-11-20  4:13 UTC (permalink / raw)
  To: linux-kernel, linux-fbdev; +Cc: tomi.valkeinen, plagnioj, Stefan Kristiansson

This adds support for the VGA/LCD core available from OpenCores:
http://opencores.org/project,vga_lcd

The driver have been tested together with both OpenRISC and
ARM (socfpga) processors.

Signed-off-by: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
---
Changes in v2:
- Add Microblaze as an example user and fix a typo in Xilinx Zynq
---
 drivers/video/Kconfig  |  17 ++
 drivers/video/Makefile |   1 +
 drivers/video/ocfb.c   | 479 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 497 insertions(+)
 create mode 100644 drivers/video/ocfb.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 84b685f..3b3f31e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -979,6 +979,23 @@ config FB_PVR2
 	  (<file:drivers/video/pvr2fb.c>). Please see the file
 	  <file:Documentation/fb/pvr2fb.txt>.
 
+config FB_OPENCORES
+	tristate "OpenCores VGA/LCD core 2.0 framebuffer support"
+	depends on FB
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	default n
+	help
+	  This enables support for the OpenCores VGA/LCD core.
+
+	  The OpenCores VGA/LCD core is typically used together with
+	  softcore CPUs (e.g. OpenRISC or Microblaze) or hard processor
+	  systems (e.g. Altera socfpga or Xilinx Zynq) on FPGAs.
+
+	  The source code and specification for the core is available at
+	  <http://opencores.org/project,vga_lcd>
+
 config FB_S1D13XXX
 	tristate "Epson S1D13XXX framebuffer support"
 	depends on FB
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index e8bae8d..ae17ddf 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_FB_NUC900)           += nuc900fb.o
 obj-$(CONFIG_FB_JZ4740)		  += jz4740_fb.o
 obj-$(CONFIG_FB_PUV3_UNIGFX)      += fb-puv3.o
 obj-$(CONFIG_FB_HYPERV)		  += hyperv_fb.o
+obj-$(CONFIG_FB_OPENCORES)	  += ocfb.o
 
 # Platform or fallback drivers go here
 obj-$(CONFIG_FB_UVESA)            += uvesafb.o
diff --git a/drivers/video/ocfb.c b/drivers/video/ocfb.c
new file mode 100644
index 0000000..6b01e65
--- /dev/null
+++ b/drivers/video/ocfb.c
@@ -0,0 +1,479 @@
+/*
+ * OpenCores VGA/LCD 2.0 core frame buffer driver
+ *
+ * Copyright (C) 2013 Stefan Kristiansson, stefan.kristiansson@saunalahti.fi
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/mm.h>
+#include <linux/dma-mapping.h>
+#include <linux/fb.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+/* OCFB register defines */
+#define OCFB_CTRL	0x000
+#define OCFB_STAT	0x004
+#define OCFB_HTIM	0x008
+#define OCFB_VTIM	0x00c
+#define OCFB_HVLEN	0x010
+#define OCFB_VBARA	0x014
+#define OCFB_PALETTE	0x800
+
+#define OCFB_CTRL_VEN	0x00000001 /* Video Enable */
+#define OCFB_CTRL_HIE	0x00000002 /* HSync Interrupt Enable */
+#define OCFB_CTRL_PC	0x00000800 /* 8-bit Pseudo Color Enable*/
+#define OCFB_CTRL_CD8	0x00000000 /* Color Depth 8 */
+#define OCFB_CTRL_CD16	0x00000200 /* Color Depth 16 */
+#define OCFB_CTRL_CD24	0x00000400 /* Color Depth 24 */
+#define OCFB_CTRL_CD32	0x00000600 /* Color Depth 32 */
+#define OCFB_CTRL_VBL1	0x00000000 /* Burst Length 1 */
+#define OCFB_CTRL_VBL2	0x00000080 /* Burst Length 2 */
+#define OCFB_CTRL_VBL4	0x00000100 /* Burst Length 4 */
+#define OCFB_CTRL_VBL8	0x00000180 /* Burst Length 8 */
+
+#define PALETTE_SIZE	256
+
+#define OCFB_NAME	"OC VGA/LCD"
+
+static char *mode_option;
+
+static const struct fb_videomode default_mode = {
+	/* 640x480 @ 60 Hz, 31.5 kHz hsync */
+	NULL, 60, 640, 480, 39721, 40, 24, 32, 11, 96, 2,
+	0, FB_VMODE_NONINTERLACED
+};
+
+struct ocfb_dev {
+	struct fb_info info;
+	/* Physical and virtual addresses of control regs */
+	phys_addr_t regs_phys;
+	int regs_phys_size;
+	void __iomem *regs;
+	/* flag indicating whether the regs are little endian accessed */
+	int little_endian;
+	/* Physical and virtual addresses of framebuffer */
+	phys_addr_t fb_phys;
+	void __iomem *fb_virt;
+	u32 pseudo_palette[PALETTE_SIZE];
+};
+
+struct ocfb_par {
+	void __iomem *pal_adr;
+};
+
+static struct ocfb_par ocfb_par_priv;
+
+static struct fb_var_screeninfo ocfb_var;
+static struct fb_fix_screeninfo ocfb_fix;
+
+#ifndef MODULE
+static int __init ocfb_setup(char *options)
+{
+	char *curr_opt;
+
+	if (!options || !*options)
+		return 0;
+
+	while ((curr_opt = strsep(&options, ",")) != NULL) {
+		if (!*curr_opt)
+			continue;
+		mode_option = curr_opt;
+	}
+
+	return 0;
+}
+#endif
+
+static inline u32 ocfb_readreg(struct ocfb_dev *fbdev, loff_t offset)
+{
+	if (fbdev->little_endian)
+		return ioread32(fbdev->regs + offset);
+	else
+		return ioread32be(fbdev->regs + offset);
+}
+
+static void ocfb_writereg(struct ocfb_dev *fbdev, loff_t offset, u32 data)
+{
+	if (fbdev->little_endian)
+		iowrite32(data, fbdev->regs + offset);
+	else
+		iowrite32be(data, fbdev->regs + offset);
+}
+
+static int ocfb_setupfb(struct ocfb_dev *fbdev)
+{
+	unsigned long bpp_config;
+	struct fb_var_screeninfo *var = &fbdev->info.var;
+	struct device *dev = fbdev->info.device;
+	u32 hlen;
+	u32 vlen;
+
+	/* Disable display */
+	ocfb_writereg(fbdev, OCFB_CTRL, 0);
+
+	/* Register framebuffer address */
+	fbdev->little_endian = 0;
+	ocfb_writereg(fbdev, OCFB_VBARA, fbdev->fb_phys);
+
+	/* Detect endianess */
+	if (ocfb_readreg(fbdev, OCFB_VBARA) != fbdev->fb_phys) {
+		fbdev->little_endian = 1;
+		ocfb_writereg(fbdev, OCFB_VBARA, fbdev->fb_phys);
+	}
+
+	/* Horizontal timings */
+	ocfb_writereg(fbdev, OCFB_HTIM, (var->hsync_len - 1) << 24 |
+		      (var->right_margin - 1) << 16 | (var->xres - 1));
+
+	/* Vertical timings */
+	ocfb_writereg(fbdev, OCFB_VTIM, (var->vsync_len - 1) << 24 |
+		      (var->lower_margin - 1) << 16 | (var->yres - 1));
+
+	/* Total length of frame */
+	hlen = var->left_margin + var->right_margin + var->hsync_len +
+		var->xres;
+
+	vlen = var->upper_margin + var->lower_margin + var->vsync_len +
+		var->yres;
+
+	ocfb_writereg(fbdev, OCFB_HVLEN, (hlen - 1) << 16 | (vlen - 1));
+
+	bpp_config = OCFB_CTRL_CD8;
+	switch (var->bits_per_pixel) {
+	case 8:
+		if (!var->grayscale)
+			bpp_config |= OCFB_CTRL_PC;  /* enable palette */
+		break;
+
+	case 16:
+		bpp_config |= OCFB_CTRL_CD16;
+		break;
+
+	case 24:
+		bpp_config |= OCFB_CTRL_CD24;
+		break;
+
+	case 32:
+		bpp_config |= OCFB_CTRL_CD32;
+		break;
+
+	default:
+		dev_err(dev, "no bpp specified\n");
+		break;
+	}
+
+	/* maximum (8) VBL (video memory burst length) */
+	bpp_config |= OCFB_CTRL_VBL8;
+
+	/* Enable output */
+	ocfb_writereg(fbdev, OCFB_CTRL, (OCFB_CTRL_VEN | bpp_config));
+
+	return 0;
+}
+
+static int ocfb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			  unsigned blue, unsigned transp,
+			  struct fb_info *info)
+{
+	struct ocfb_par *par = (struct ocfb_par *)info->par;
+	u32 color;
+
+	if (regno >= info->cmap.len) {
+		dev_err(info->device, "regno >= cmap.len\n");
+		return 1;
+	}
+
+	if (info->var.grayscale) {
+		/* grayscale = 0.30*R + 0.59*G + 0.11*B */
+		red = green = blue = (red * 77 + green * 151 + blue * 28) >> 8;
+	}
+
+	red >>= (16 - info->var.red.length);
+	green >>= (16 - info->var.green.length);
+	blue >>= (16 - info->var.blue.length);
+	transp >>= (16 - info->var.transp.length);
+
+	if (info->var.bits_per_pixel = 8 && !info->var.grayscale) {
+		regno <<= 2;
+		color = (red << 16) | (green << 8) | blue;
+		ocfb_writereg(par->pal_adr, regno, color);
+	} else {
+		((u32 *)(info->pseudo_palette))[regno] +			(red << info->var.red.offset) |
+			(green << info->var.green.offset) |
+			(blue << info->var.blue.offset) |
+			(transp << info->var.transp.offset);
+	}
+
+	return 0;
+}
+
+static int ocfb_init_fix(struct ocfb_dev *fbdev)
+{
+	struct fb_var_screeninfo *var = &fbdev->info.var;
+	struct fb_fix_screeninfo *fix = &fbdev->info.fix;
+
+	strcpy(fix->id, OCFB_NAME);
+
+	fix->line_length = var->xres * var->bits_per_pixel/8;
+	fix->smem_len = fix->line_length * var->yres;
+	fix->type = FB_TYPE_PACKED_PIXELS;
+
+	if (var->bits_per_pixel = 8 && !var->grayscale)
+		fix->visual = FB_VISUAL_PSEUDOCOLOR;
+	else
+		fix->visual = FB_VISUAL_TRUECOLOR;
+
+	return 0;
+}
+
+static int ocfb_init_var(struct ocfb_dev *fbdev)
+{
+	struct fb_var_screeninfo *var = &fbdev->info.var;
+
+	var->accel_flags = FB_ACCEL_NONE;
+	var->activate = FB_ACTIVATE_NOW;
+	var->xres_virtual = var->xres;
+	var->yres_virtual = var->yres;
+
+	switch (var->bits_per_pixel) {
+	case 8:
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		var->red.offset = 0;
+		var->red.length = 8;
+		var->green.offset = 0;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		break;
+
+	case 16:
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		var->red.offset = 11;
+		var->red.length = 5;
+		var->green.offset = 5;
+		var->green.length = 6;
+		var->blue.offset = 0;
+		var->blue.length  = 5;
+		break;
+
+	case 24:
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		var->red.offset = 16;
+		var->red.length = 8;
+		var->green.offset = 8;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		break;
+
+	case 32:
+		var->transp.offset = 24;
+		var->transp.length = 8;
+		var->red.offset = 16;
+		var->red.length = 8;
+		var->green.offset = 8;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		break;
+	}
+
+	return 0;
+}
+
+static struct fb_ops ocfb_ops = {
+	.owner		= THIS_MODULE,
+	.fb_setcolreg	= ocfb_setcolreg,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit,
+};
+
+static int ocfb_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct ocfb_dev *fbdev;
+	struct ocfb_par *par = &ocfb_par_priv;
+	struct resource *res;
+	struct resource *mmio;
+	int fbsize;
+
+	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
+	if (!fbdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, fbdev);
+
+	fbdev->info.fbops = &ocfb_ops;
+	fbdev->info.var = ocfb_var;
+	fbdev->info.fix = ocfb_fix;
+	fbdev->info.device = &pdev->dev;
+	fbdev->info.par = par;
+
+	/* Video mode setup */
+	if (!fb_find_mode(&fbdev->info.var, &fbdev->info, mode_option,
+			  NULL, 0, &default_mode, 16)) {
+		dev_err(&pdev->dev, "No valid video modes found\n");
+		kfree(fbdev);
+		return -EINVAL;
+	}
+	ocfb_init_var(fbdev);
+	ocfb_init_fix(fbdev);
+
+	/* Request I/O resource */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "I/O resource request failed\n");
+		kfree(fbdev);
+		return -ENXIO;
+	}
+	fbdev->regs_phys = res->start;
+	fbdev->regs_phys_size = resource_size(res);
+	mmio = devm_request_mem_region(&pdev->dev, res->start,
+				       resource_size(res), res->name);
+	if (!mmio) {
+		dev_err(&pdev->dev, "I/O memory space request failed\n");
+		kfree(fbdev);
+		return -ENXIO;
+	}
+	fbdev->regs = devm_ioremap_nocache(&pdev->dev, mmio->start,
+					   resource_size(mmio));
+	if (!fbdev->regs) {
+		dev_err(&pdev->dev, "I/O memory remap request failed\n");
+		kfree(fbdev);
+		return -ENXIO;
+	}
+	par->pal_adr = fbdev->regs + OCFB_PALETTE;
+
+	/* Allocate framebuffer memory */
+	fbsize = fbdev->info.fix.smem_len;
+	fbdev->fb_virt = dma_alloc_coherent(&pdev->dev, PAGE_ALIGN(fbsize),
+					    &fbdev->fb_phys, GFP_KERNEL);
+	if (!fbdev->fb_virt) {
+		dev_err(&pdev->dev,
+			"Frame buffer memory allocation failed\n");
+		ret = -ENOMEM;
+		goto err_release;
+	}
+	fbdev->info.fix.smem_start = fbdev->fb_phys;
+	fbdev->info.screen_base = (void __iomem *)fbdev->fb_virt;
+	fbdev->info.pseudo_palette = fbdev->pseudo_palette;
+
+	/* Clear framebuffer */
+	memset_io((void __iomem *)fbdev->fb_virt, 0, fbsize);
+
+	/* Setup and enable the framebuffer */
+	ocfb_setupfb(fbdev);
+
+	if (fbdev->little_endian)
+		fbdev->info.flags |= FBINFO_FOREIGN_ENDIAN;
+
+	/* Allocate color map */
+	ret = fb_alloc_cmap(&fbdev->info.cmap, PALETTE_SIZE, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "Color map allocation failed\n");
+		goto err_dma_free;
+	}
+
+	/* Register framebuffer */
+	ret = register_framebuffer(&fbdev->info);
+	if (ret) {
+		dev_err(&pdev->dev, "Framebuffer registration failed\n");
+		goto err_dealloc_cmap;
+	}
+
+	return 0;
+
+err_dealloc_cmap:
+	fb_dealloc_cmap(&fbdev->info.cmap);
+
+err_dma_free:
+	dma_free_coherent(&pdev->dev, PAGE_ALIGN(fbsize), fbdev->fb_virt,
+			  fbdev->fb_phys);
+err_release:
+	release_mem_region(fbdev->regs_phys, fbdev->regs_phys_size);
+	kfree(fbdev);
+
+	return ret;
+}
+
+static int ocfb_remove(struct platform_device *pdev)
+{
+	struct ocfb_dev *fbdev = platform_get_drvdata(pdev);
+
+	unregister_framebuffer(&fbdev->info);
+	fb_dealloc_cmap(&fbdev->info.cmap);
+	dma_free_coherent(&pdev->dev, PAGE_ALIGN(fbdev->info.fix.smem_len),
+			  fbdev->fb_virt, fbdev->fb_phys);
+
+	/* Disable display */
+	ocfb_writereg(fbdev, OCFB_CTRL, 0);
+
+	release_mem_region(fbdev->regs_phys, fbdev->regs_phys_size);
+	kfree(fbdev);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct of_device_id ocfb_match[] = {
+	{ .compatible = "opencores,ocfb", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ocfb_match);
+
+static struct platform_driver ocfb_driver = {
+	.probe  = ocfb_probe,
+	.remove	= ocfb_remove,
+	.driver = {
+		.name = "ocfb_fb",
+		.of_match_table = ocfb_match,
+	}
+};
+
+/*
+ * Init and exit routines
+ */
+static int __init ocfb_init(void)
+{
+#ifndef MODULE
+	char *option = NULL;
+
+	if (fb_get_options("ocfb", &option))
+		return -ENODEV;
+	ocfb_setup(option);
+#endif
+	return platform_driver_register(&ocfb_driver);
+}
+
+static void __exit ocfb_exit(void)
+{
+	platform_driver_unregister(&ocfb_driver);
+}
+
+module_init(ocfb_init);
+module_exit(ocfb_exit);
+
+#ifdef MODULE
+MODULE_AUTHOR("Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>");
+MODULE_DESCRIPTION("OpenCores VGA/LCD 2.0 frame buffer driver");
+MODULE_LICENSE("GPL v2");
+module_param(mode_option, charp, 0);
+MODULE_PARM_DESC(mode_option, "Video mode ('<xres>x<yres>[-<bpp>][@refresh]')");
+#endif
-- 
1.8.3.2


^ permalink raw reply related

* Re: [PATCH v2] video: add OpenCores VGA/LCD framebuffer driver
From: Jingoo Han @ 2013-11-20  6:20 UTC (permalink / raw)
  To: 'Stefan Kristiansson'
  Cc: linux-kernel, linux-fbdev, 'Tomi Valkeinen',
	'Jean-Christophe PLAGNIOL-VILLARD', 'Jingoo Han'
In-Reply-To: <1384920820-18740-1-git-send-email-stefan.kristiansson@saunalahti.fi>

On Wednesday, November 20, 2013 1:14 PM, Stefan Kristiansson wrote:
> 
> This adds support for the VGA/LCD core available from OpenCores:
> http://opencores.org/project,vga_lcd
> 
> The driver have been tested together with both OpenRISC and
> ARM (socfpga) processors.
> 
> Signed-off-by: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> ---
> Changes in v2:
> - Add Microblaze as an example user and fix a typo in Xilinx Zynq
> ---
>  drivers/video/Kconfig  |  17 ++
>  drivers/video/Makefile |   1 +
>  drivers/video/ocfb.c   | 479 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 497 insertions(+)
>  create mode 100644 drivers/video/ocfb.c
> 

Hi Stefan Kristiansson,

I added trivial comments as below.

[.....]

> --- /dev/null
> +++ b/drivers/video/ocfb.c

[.....]

> +static int ocfb_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;
> +	struct ocfb_dev *fbdev;
> +	struct ocfb_par *par = &ocfb_par_priv;
> +	struct resource *res;
> +	struct resource *mmio;
> +	int fbsize;
> +
> +	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);

Please use devm_kzalloc() instead of kzalloc().
In this case, kfree() can be removed from ocfb_probe()
and ocfb_remove(). Thus, it will make the code smaller.

[.....]

> +
> +#ifdef MODULE

I don't think that '#ifdef MODULE' is necessary.
Is there any reason?

Best regards,
Jingoo Han

> +MODULE_AUTHOR("Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>");
> +MODULE_DESCRIPTION("OpenCores VGA/LCD 2.0 frame buffer driver");
> +MODULE_LICENSE("GPL v2");
> +module_param(mode_option, charp, 0);
> +MODULE_PARM_DESC(mode_option, "Video mode ('<xres>x<yres>[-<bpp>][@refresh]')");
> +#endif
> --
> 1.8.3.2


^ permalink raw reply

* Re: [PATCH] RFC: framebuffer: provide generic get_fb_unmapped_area
From: Geert Uytterhoeven @ 2013-11-20  8:41 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20131118185959.GE19318@pengutronix.de>

On Mon, Nov 18, 2013 at 7:59 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>> > +unsigned long fb_get_unmapped_area(struct file *filp, unsigned long orig_addr,
>> > +               unsigned long len, unsigned long pgoff, unsigned long flags)
>> > +{
>> > +       struct fb_info * const info = filp->private_data;
>> > +       unsigned long screen_size = info->screen_size ?: info->fix.smem_len;
>>
>> Why restrict this to screen_size? Fbtest will map the whole frame buffer memory.
> For me screen_size is zero. The logic to determine the size is copied
> from fb_read.

fb_read() only allows reading the visible screen, not the full frame
buffer memory.
fb_mmap() does allow mapping the full frame buffer memory (and the optional
MMIO registers, but you can't easily do that the same way on nommu, as it's
a discontiguous mapping).
So please use PAGE_ALIGN(info->fix.smem_len) as the limit.

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

^ permalink raw reply

* Re: [PATCH v2] video: add OpenCores VGA/LCD framebuffer driver
From: Stefan Kristiansson @ 2013-11-20 19:13 UTC (permalink / raw)
  To: Jingoo Han
  Cc: linux-kernel, linux-fbdev, 'Tomi Valkeinen',
	'Jean-Christophe PLAGNIOL-VILLARD'
In-Reply-To: <000701cee5b8$9b61e3b0$d225ab10$%han@samsung.com>

On Wed, Nov 20, 2013 at 03:20:28PM +0900, Jingoo Han wrote:
> On Wednesday, November 20, 2013 1:14 PM, Stefan Kristiansson wrote:
> 
> > +static int ocfb_probe(struct platform_device *pdev)
> > +{
> > +	int ret = 0;
> > +	struct ocfb_dev *fbdev;
> > +	struct ocfb_par *par = &ocfb_par_priv;
> > +	struct resource *res;
> > +	struct resource *mmio;
> > +	int fbsize;
> > +
> > +	fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL);
> 
> Please use devm_kzalloc() instead of kzalloc().
> In this case, kfree() can be removed from ocfb_probe()
> and ocfb_remove(). Thus, it will make the code smaller.
> 

Nice, thanks for the hint!
Consider it done.

> [.....]
> 
> > +
> > +#ifdef MODULE
> 
> I don't think that '#ifdef MODULE' is necessary.
> Is there any reason?
> 

You're right, that's not necessary, I'll remove that.

Thanks alot for taking the time to review it!

Stefan

^ permalink raw reply

* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Matthew Garrett @ 2013-11-20 23:40 UTC (permalink / raw)
  To: Henrique de Moraes Holschuh
  Cc: Jingoo Han, 'Kyungmin Park',
	'Henrique de Moraes Holschuh', linux-fbdev, linux-kernel,
	kay, 'Richard Purdie', ibm-acpi-devel,
	platform-driver-x86
In-Reply-To: <20131112005628.GA2914@khazad-dum.debian.net>

On Mon, 2013-11-11 at 22:56 -0200, Henrique de Moraes Holschuh wrote:
> On Tue, 12 Nov 2013, Jingoo Han wrote:
> > 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
> > Henrique, can we remove it?
> 
> Can't you fix this by rate-limiting, or otherwise adding an attribute that
> backlight devices should set when they need to supress change events?

It looks like this is just to force synchronisation to sysfs when using
the /proc interface? In which case we should probably just kill
the /proc interface.

-- 
Matthew Garrett <matthew.garrett@nebula.com>


^ permalink raw reply

* Re: [PATCHv9][ 1/3] Input: tsc2007: Add device tree support.
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-11-21  5:05 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1383916659-9988-1-git-send-email-denis@eukrea.com>

On 14:17 Fri 08 Nov     , Denis Carikli wrote:
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v8->v9:
> - Added Grant Likely in the Cc list.
> - Removed the mention of the pinctrl properties in the documentation.
> 
> ChangeLog v7->v8:
> - Fixed the lack of x and z fuzz properties.
> - The pendown gpio is better documented.
> - Added Shawn Guo in the cc list.
> ---
>  .../bindings/input/touchscreen/tsc2007.txt         |   41 ++++
>  drivers/input/touchscreen/tsc2007.c                |  204 ++++++++++++++++----
>  2 files changed, 204 insertions(+), 41 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> new file mode 100644
> index 0000000..028aba66d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/tsc2007.txt
> @@ -0,0 +1,41 @@
> +* Texas Instruments tsc2007 touchscreen controller
> +
> +Required properties:
> +- compatible: must be "ti,tsc2007".
> +- reg: I2C address of the chip.
> +- ti,x-plate-ohms: X-plate resistance in ohms.
> +
> +Optional properties:
> +- gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
nack use interrupt property this a non-sense that we do today to pass irq via
gpio property the should NEVER known it's a gpio just an irq
> +  The penirq pin goes to low when the panel is touched.
> +  (see GPIO binding[1] for more details).
> +- interrupt-parent: the phandle for the gpio controller
> +  (see interrupt binding[0]).
> +- interrupts: (gpio) interrupt to which the chip is connected
> +  (see interrupt binding[0]).
> +- ti,max-rt: maximum pressure.
> +- ti,fuzzx: specifies the absolute input fuzz x value.
> +  If set, it will permit noise in the data up to +- the value given to the fuzz
> +  parameter, that is used to filter noise from the event stream.
> +- ti,fuzzy: specifies the absolute input fuzz y value.
> +- ti,fuzzz: specifies the absolute input fuzz z value.
fuzz{x,y,z} look a weird name
> +- ti,poll-period: how much time to wait(in millisecond) before reading again the
> +  values from the tsc2007.
so put -ms in the binding to de clear and does not requiere to read the doc

Best Regards,
J.
> +
> +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +[1]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +	&i2c1 {
> +		/* ... */
> +		tsc2007@49 {
> +			compatible = "ti,tsc2007";
> +			reg = <0x49>;
> +			interrupt-parent = <&gpio4>;
> +			interrupts = <0x0 0x8>;
> +			gpios = <&gpio4 0 0>;
> +			ti,x-plate-ohms = <180>;
> +		};
> +
> +		/* ... */
> +	};
> diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
> index 0b67ba4..3168a99 100644
> --- a/drivers/input/touchscreen/tsc2007.c
> +++ b/drivers/input/touchscreen/tsc2007.c
> @@ -26,6 +26,9 @@
>  #include <linux/interrupt.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c/tsc2007.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  
>  #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
>  #define TSC2007_MEASURE_AUX		(0x2 << 4)
> @@ -74,7 +77,12 @@ struct tsc2007 {
>  	u16			max_rt;
>  	unsigned long		poll_delay;
>  	unsigned long		poll_period;
> +	int			fuzzx;
> +	int			fuzzy;
> +	int			fuzzz;
> +	char			of;
>  
> +	unsigned		gpio;
>  	int			irq;
>  
>  	wait_queue_head_t	wait;
> @@ -84,6 +92,11 @@ struct tsc2007 {
>  	void			(*clear_penirq)(void);
>  };
>  
> +static int tsc2007_get_pendown_state_dt(struct tsc2007 *ts)
> +{
> +	return !gpio_get_value(ts->gpio);
> +}
> +
>  static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
>  {
>  	s32 data;
> @@ -142,6 +155,14 @@ static u32 tsc2007_calculate_pressure(struct tsc2007 *tsc, struct ts_event *tc)
>  	return rt;
>  }
>  
> +static bool tsc2007_is_pen_down_valid(struct tsc2007 *ts)
> +{
> +	if (ts->of)
> +		return gpio_is_valid(ts->gpio);
> +	else
> +		return ts->get_pendown_state ? true : false;
> +}
> +
>  static bool tsc2007_is_pen_down(struct tsc2007 *ts)
>  {
>  	/*
> @@ -158,10 +179,13 @@ static bool tsc2007_is_pen_down(struct tsc2007 *ts)
>  	 * to fall back on the pressure reading.
>  	 */
>  
> -	if (!ts->get_pendown_state)
> +	if (!tsc2007_is_pen_down_valid(ts))
>  		return true;
>  
> -	return ts->get_pendown_state();
> +	if (ts->of)
> +		return tsc2007_get_pendown_state_dt(ts);
> +	else
> +		return ts->get_pendown_state();
>  }
>  
>  static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
> @@ -178,7 +202,7 @@ static irqreturn_t tsc2007_soft_irq(int irq, void *handle)
>  
>  		rt = tsc2007_calculate_pressure(ts, &tc);
>  
> -		if (rt = 0 && !ts->get_pendown_state) {
> +		if (!rt && !tsc2007_is_pen_down_valid(ts)) {
>  			/*
>  			 * If pressure reported is 0 and we don't have
>  			 * callback to check pendown state, we have to
> @@ -228,7 +252,7 @@ static irqreturn_t tsc2007_hard_irq(int irq, void *handle)
>  {
>  	struct tsc2007 *ts = handle;
>  
> -	if (!ts->get_pendown_state || likely(ts->get_pendown_state()))
> +	if (tsc2007_is_pen_down(ts))
>  		return IRQ_WAKE_THREAD;
>  
>  	if (ts->clear_penirq)
> @@ -273,34 +297,71 @@ static void tsc2007_close(struct input_dev *input_dev)
>  	tsc2007_stop(ts);
>  }
>  
> -static int tsc2007_probe(struct i2c_client *client,
> -				   const struct i2c_device_id *id)
> +#ifdef CONFIG_OF
> +static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts,
> +			    struct device_node *np)
>  {
> -	struct tsc2007 *ts;
> -	struct tsc2007_platform_data *pdata = client->dev.platform_data;
> -	struct input_dev *input_dev;
> -	int err;
> -
> -	if (!pdata) {
> -		dev_err(&client->dev, "platform data is required!\n");
> +	int err = 0;
> +	u32 val32;
> +	u64 val64;
> +
> +	if (!of_property_read_u32(np, "ti,max-rt", &val32))
> +		ts->max_rt = val32;
> +	else
> +		ts->max_rt = MAX_12BIT;
> +
> +	if (!of_property_read_u32(np, "ti,fuzzx", &val32))
> +		ts->fuzzx = val32;
> +
> +	if (!of_property_read_u32(np, "ti,fuzzy", &val32))
> +		ts->fuzzy = val32;
> +
> +	if (!of_property_read_u32(np, "ti,fuzzz", &val32))
> +		ts->fuzzz = val32;
> +
> +	if (!of_property_read_u64(np, "ti,poll-period", &val64))
> +		ts->poll_period = val64;
> +	else
> +		ts->poll_period = 1;
> +
> +	if (!of_property_read_u32(np, "ti,x-plate-ohms", &val32)) {
> +		ts->x_plate_ohms = val32;
> +	} else {
> +		dev_err(&client->dev,
> +			"Error: lacking ti,x-plate-ohms devicetree property. (err %d).",
> +			err);
>  		return -EINVAL;
>  	}
>  
> -	if (!i2c_check_functionality(client->adapter,
> -				     I2C_FUNC_SMBUS_READ_WORD_DATA))
> -		return -EIO;
> +	ts->gpio = of_get_gpio(np, 0);
> +	if (!gpio_is_valid(ts->gpio))
> +		dev_err(&client->dev,
> +			"GPIO not found (of_get_gpio returned %d)\n",
> +			ts->gpio);
>  
> -	ts = kzalloc(sizeof(struct tsc2007), GFP_KERNEL);
> -	input_dev = input_allocate_device();
> -	if (!ts || !input_dev) {
> -		err = -ENOMEM;
> -		goto err_free_mem;
> -	}
> +	/* Used to detect if it is probed trough the device tree,
> +	 * in order to be able to use that information in the IRQ handler.
> +	 */
> +	ts->of = 1;
>  
> -	ts->client = client;
> -	ts->irq = client->irq;
> -	ts->input = input_dev;
> -	init_waitqueue_head(&ts->wait);
> +	return 0;
> +}
> +#else
> +static int tsc2007_probe_dt(struct i2c_client *client, struct tsc2007 *ts,
> +			    struct device_node *np)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
> +static int tsc2007_probe_pdev(struct i2c_client *client, struct tsc2007 *ts,
> +			      struct tsc2007_platform_data *pdata,
> +			      const struct i2c_device_id *id)
> +{
> +	if (!pdata) {
> +		dev_err(&client->dev, "platform data is required!\n");
> +		return -EINVAL;
> +	}
>  
>  	ts->model             = pdata->model;
>  	ts->x_plate_ohms      = pdata->x_plate_ohms;
> @@ -309,13 +370,59 @@ static int tsc2007_probe(struct i2c_client *client,
>  	ts->poll_period       = pdata->poll_period ? : 1;
>  	ts->get_pendown_state = pdata->get_pendown_state;
>  	ts->clear_penirq      = pdata->clear_penirq;
> +	ts->fuzzx             = pdata->fuzzx;
> +	ts->fuzzy             = pdata->fuzzy;
> +	ts->fuzzz             = pdata->fuzzz;
>  
>  	if (pdata->x_plate_ohms = 0) {
>  		dev_err(&client->dev, "x_plate_ohms is not set up in platform data");
> -		err = -EINVAL;
> -		goto err_free_mem;
> +		return -EINVAL;
>  	}
>  
> +	/* Used to detect if it is probed trough the device tree,
> +	 * in order to be able to use that information in the IRQ handler.
> +	 */
> +	ts->of = 0;
> +
> +	return 0;
> +}
> +
> +static int tsc2007_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	struct tsc2007_platform_data *pdata = client->dev.platform_data;
> +	struct tsc2007 *ts;
> +	struct input_dev *input_dev;
> +	int err = 0;
> +
> +	ts = devm_kzalloc(&client->dev, sizeof(struct tsc2007), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	if (np)
> +		err = tsc2007_probe_dt(client, ts, np);
> +	else
> +		err = tsc2007_probe_pdev(client, ts, pdata, id);
> +
> +	if (err)
> +		return err;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_WORD_DATA))
> +		return -EIO;
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		err = -ENOMEM;
> +		goto err_free_input;
> +	};
> +
> +	ts->client = client;
> +	ts->irq = client->irq;
> +	ts->input = input_dev;
> +	init_waitqueue_head(&ts->wait);
> +
>  	snprintf(ts->phys, sizeof(ts->phys),
>  		 "%s/input0", dev_name(&client->dev));
>  
> @@ -331,19 +438,21 @@ static int tsc2007_probe(struct i2c_client *client,
>  	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
>  	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
>  
> -	input_set_abs_params(input_dev, ABS_X, 0, MAX_12BIT, pdata->fuzzx, 0);
> -	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, pdata->fuzzy, 0);
> +	input_set_abs_params(input_dev, ABS_X, 0, MAX_12BIT, ts->fuzzx, 0);
> +	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, ts->fuzzy, 0);
>  	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT,
> -			pdata->fuzzz, 0);
> +			     ts->fuzzz, 0);
>  
> -	if (pdata->init_platform_hw)
> -		pdata->init_platform_hw();
> +	if (!np) {
> +		if (pdata->init_platform_hw)
> +			pdata->init_platform_hw();
> +	}
>  
>  	err = request_threaded_irq(ts->irq, tsc2007_hard_irq, tsc2007_soft_irq,
>  				   IRQF_ONESHOT, client->dev.driver->name, ts);
>  	if (err < 0) {
>  		dev_err(&client->dev, "irq %d busy?\n", ts->irq);
> -		goto err_free_mem;
> +		goto err_free_input;
>  	}
>  
>  	tsc2007_stop(ts);
> @@ -358,23 +467,27 @@ static int tsc2007_probe(struct i2c_client *client,
>  
>   err_free_irq:
>  	free_irq(ts->irq, ts);
> -	if (pdata->exit_platform_hw)
> -		pdata->exit_platform_hw();
> - err_free_mem:
> +	if (!np) {
> +		if (pdata->exit_platform_hw)
> +			pdata->exit_platform_hw();
> +	}
> + err_free_input:
>  	input_free_device(input_dev);
> -	kfree(ts);
>  	return err;
>  }
>  
>  static int tsc2007_remove(struct i2c_client *client)
>  {
> +	struct device_node *np = client->dev.of_node;
>  	struct tsc2007	*ts = i2c_get_clientdata(client);
>  	struct tsc2007_platform_data *pdata = client->dev.platform_data;
>  
>  	free_irq(ts->irq, ts);
>  
> -	if (pdata->exit_platform_hw)
> -		pdata->exit_platform_hw();
> +	if (!np) {
> +		if (pdata->exit_platform_hw)
> +			pdata->exit_platform_hw();
> +	}
>  
>  	input_unregister_device(ts->input);
>  	kfree(ts);
> @@ -389,10 +502,19 @@ static const struct i2c_device_id tsc2007_idtable[] = {
>  
>  MODULE_DEVICE_TABLE(i2c, tsc2007_idtable);
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id tsc2007_of_match[] = {
> +	{ .compatible = "ti,tsc2007" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tsc2007_of_match);
> +#endif
> +
>  static struct i2c_driver tsc2007_driver = {
>  	.driver = {
>  		.owner	= THIS_MODULE,
> -		.name	= "tsc2007"
> +		.name	= "tsc2007",
> +		.of_match_table = of_match_ptr(tsc2007_of_match),
>  	},
>  	.id_table	= tsc2007_idtable,
>  	.probe		= tsc2007_probe,
> -- 
> 1.7.9.5
> 

^ permalink raw reply

* Re: [PATCHv9][ 3/3] ARM: dts: cpuimx35 Add touchscreen support.
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-11-21  5:09 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1383916659-9988-3-git-send-email-denis@eukrea.com>

On 14:17 Fri 08 Nov     , Denis Carikli wrote:
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v8->v9:
> - Added Grant Likely in the cc list.
> - Adapted to the removal of the pinctrl properties in the tsc2007 documentation.
> - Fixed the gpios property (before, it was set to active high by error).
> 
> ChangeLog v7->v8:
> - Added Shawn Guo in the cc list.
> ---
>  arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi b/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> index b9cb5a5..f25a40f 100644
> --- a/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> +++ b/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> @@ -36,6 +36,27 @@
>  		compatible = "nxp,pcf8563";
>  		reg = <0x51>;
>  	};
> +
> +	tsc2007: tsc2007@48 {
> +		compatible = "ti,tsc2007";
> +		reg = <0x48>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <0x2 0x8>;
> +		gpios = <&gpio3 2 1>;
as explain on the binding drop this gpios this is an IRQ not a gpio

NACK

Best Regards,
J.
> +		ti,x-plate-ohms = <180>;
> +        };
> +
> +};
> +
> +&iomuxc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_hog>;
> +
> +	imx35-eukrea {
> +		pinctrl_hog: hoggrp {
> +			fsl,pins = <MX35_PAD_ATA_DA2__GPIO3_2 0x80000000>;
> +		};
> +	};
>  };
>  
>  &nfc {
> -- 
> 1.7.9.5
> 

^ permalink raw reply

* Re: [PATCHv9][ 2/3] ARM: dts: cpuimx51 Add touchscreen support.
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-11-21  5:10 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1383916659-9988-2-git-send-email-denis@eukrea.com>

On 14:17 Fri 08 Nov     , Denis Carikli wrote:
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: Lothar Waßmann <LW@KARO-electronics.de>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v8->v9:
> - Added Grant Likely in the Cc list.
> - Adapted to the removal of the pinctrl properties in the tsc2007 documentation.
> - Fixed the gpios property (before, it was set to active high by error).
> 
> ChangeLog v7->v8:
> - Added Shawn Guo in the cc list.
> ---
>  arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi b/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> index b22841a..b04c65b 100644
> --- a/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> +++ b/arch/arm/boot/dts/imx51-eukrea-cpuimx51.dtsi
> @@ -42,11 +42,23 @@
>  		compatible = "nxp,pcf8563";
>  		reg = <0x51>;
>  	};
> +
> +	tsc2007: tsc2007@49 {
> +		compatible = "ti,tsc2007";
> +		reg = <0x49>;
> +		interrupt-parent = <&gpio4>;
> +		interrupts = <0x0 0x8>;
> +		gpios = <&gpio4 0 1>;
> +		ti,x-plate-ohms = <180>;
as explain on the binding drop this gpios this is an IRQ not a gpio

NACK

Best Regards,
J.
> +	};
>  };
>  
>  &iomuxc {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_hog>;
> +
>  	imx51-eukrea {
> -		pinctrl_tsc2007_1: tsc2007grp-1 {
> +		pinctrl_hog: hoggrp {
>  			fsl,pins = <
>  				MX51_PAD_GPIO_NAND__GPIO_NAND 0x1f5
>  				MX51_PAD_NANDF_D8__GPIO4_0 0x1f5
> -- 
> 1.7.9.5
> 

^ permalink raw reply

* Re: [PATCHv9][ 3/3] ARM: dts: cpuimx35 Add touchscreen support.
From: Lothar Waßmann @ 2013-11-21  8:00 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20131121050922.GF18477@ns203013.ovh.net>

Hi,

Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 14:17 Fri 08 Nov     , Denis Carikli wrote:
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Stephen Warren <swarren@wwwdotorg.org>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: Grant Likely <grant.likely@linaro.org>
> > Cc: devicetree@vger.kernel.org
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: linux-input@vger.kernel.org
> > Cc: Sascha Hauer <kernel@pengutronix.de>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: Lothar Waßmann <LW@KARO-electronics.de>
> > Cc: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Eric Bénard <eric@eukrea.com>
> > Signed-off-by: Denis Carikli <denis@eukrea.com>
> > ---
> > ChangeLog v8->v9:
> > - Added Grant Likely in the cc list.
> > - Adapted to the removal of the pinctrl properties in the tsc2007 documentation.
> > - Fixed the gpios property (before, it was set to active high by error).
> > 
> > ChangeLog v7->v8:
> > - Added Shawn Guo in the cc list.
> > ---
> >  arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi |   21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi b/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> > index b9cb5a5..f25a40f 100644
> > --- a/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> > +++ b/arch/arm/boot/dts/imx35-eukrea-cpuimx35.dtsi
> > @@ -36,6 +36,27 @@
> >  		compatible = "nxp,pcf8563";
> >  		reg = <0x51>;
> >  	};
> > +
> > +	tsc2007: tsc2007@48 {
> > +		compatible = "ti,tsc2007";
> > +		reg = <0x48>;
> > +		interrupt-parent = <&gpio3>;
> > +		interrupts = <0x2 0x8>;
> > +		gpios = <&gpio3 2 1>;
> as explain on the binding drop this gpios this is an IRQ not a gpio
> 
> NACK
The driver needs an interrupt to get notified about pendown events and
needs to poll the pendown signal to find out when the pen has been
lifted. Both the interrupt and the pendown signal happen to be
signalled on the same pin. Thus, IMO it is perfectly well to have the
interrupt property as well as the gpios property here.

How would you tell the driver which pin to poll for detecting the pen
up event?

Having a platform callback with hardcoded GPIO numbers is not an option.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

^ permalink raw reply

* Re: [PATCHv9][ 1/3] Input: tsc2007: Add device tree support.
From: Eric Bénard @ 2013-11-21  8:03 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20131121050544.GE18477@ns203013.ovh.net>

Hi Jean Christophe,

Le Thu, 21 Nov 2013 06:05:44 +0100,
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> a écrit :
> > +Optional properties:
> > +- gpios: the interrupt gpio the chip is connected to (trough the penirq pin).
> nack use interrupt property this a non-sense that we do today to pass irq via
> gpio property the should NEVER known it's a gpio just an irq

read previous email and you will see the GPIO is used to get the signal
level on the pin and not the IRQ.

Eric

^ permalink raw reply

* Re: [PATCHv9][ 3/3] ARM: dts: cpuimx35 Add touchscreen support.
From: Eric Bénard @ 2013-11-21  8:05 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20131121050922.GF18477@ns203013.ovh.net>

Le Thu, 21 Nov 2013 06:09:22 +0100,
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> a écrit :
> > +	tsc2007: tsc2007@48 {
> > +		compatible = "ti,tsc2007";
> > +		reg = <0x48>;
> > +		interrupt-parent = <&gpio3>;
> > +		interrupts = <0x2 0x8>;
> > +		gpios = <&gpio3 2 1>;
> as explain on the binding drop this gpios this is an IRQ not a gpio
> 
> NACK
> 
please read the tsc2007's code, in the patch there is :

+static int tsc2007_get_pendown_state_dt(struct tsc2007 *ts)
+{
+       return !gpio_get_value(ts->gpio);
+}
+

http://www.mail-archive.com/linux-input@vger.kernel.org/msg06765.html

Eric

^ permalink raw reply

* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Henrique de Moraes Holschuh @ 2013-11-21 11:43 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Jingoo Han, 'Kyungmin Park',
	'Henrique de Moraes Holschuh', linux-fbdev, linux-kernel,
	kay, 'Richard Purdie', ibm-acpi-devel,
	platform-driver-x86
In-Reply-To: <1384990859.20536.4.camel@x230>

On Wed, 20 Nov 2013, Matthew Garrett wrote:
> On Mon, 2013-11-11 at 22:56 -0200, Henrique de Moraes Holschuh wrote:
> > On Tue, 12 Nov 2013, Jingoo Han wrote:
> > > 'thinkpad_acpi.c' uses the 'BACKLIGHT_UPDATE_SYSFS'.
> > > Henrique, can we remove it?
> > 
> > Can't you fix this by rate-limiting, or otherwise adding an attribute that
> > backlight devices should set when they need to supress change events?
> 
> It looks like this is just to force synchronisation to sysfs when using
> the /proc interface? In which case we should probably just kill
> the /proc interface.

Well, we can remove the thinkpad-acpi /proc interface as far as I'm
concerned, and that would do away with the use of BACKLIGHT_UPDATE_SYSFS by
thinkpad-acpi.  It is a major userspace ABI break, but removing everything
under /proc/acpi is one of the very few ABI breaks we actually have the
green light to do.

However, the patchset is not about this.

With this patchset applied, as far as I can tell anything that used to be
uevent-driven by the backlight class will break: when a process changes the
backlight using sysfs, other processes will not be notified of the change
anymore.  This patchset seems to break backlight uevent support in such a
way that basically renders the entire thing useless and you might as well
just remove uevent support entirely.

It is also an userspace ABI break, which we do not do lightly.

So, as far as I'm concerned, this patchset should be rejected in its present
form.  IMO, either one that preserves BACKLIGHT_UPDATE_SYSFS and fixes the
urgent issue, or one that removes uevent support entirely from the backlight
class should be proposed instead.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

^ permalink raw reply

* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Matthew Garrett @ 2013-11-21 14:33 UTC (permalink / raw)
  To: Henrique de Moraes Holschuh
  Cc: Jingoo Han, 'Kyungmin Park',
	'Henrique de Moraes Holschuh', linux-fbdev, linux-kernel,
	kay, 'Richard Purdie', ibm-acpi-devel,
	platform-driver-x86
In-Reply-To: <20131121114332.GA23710@khazad-dum.debian.net>

On Thu, Nov 21, 2013 at 09:43:32AM -0200, Henrique de Moraes Holschuh wrote:

> With this patchset applied, as far as I can tell anything that used to be
> uevent-driven by the backlight class will break: when a process changes the
> backlight using sysfs, other processes will not be notified of the change
> anymore.  This patchset seems to break backlight uevent support in such a
> way that basically renders the entire thing useless and you might as well
> just remove uevent support entirely.

The uevent support was initially added to handle systems where pressing 
a hotkey generates an event (good) but the firmware automatically 
changes the brightness (bad). I have absolutely no idea why I added 
BACKLIGHT_UPDATE_SYSFS - BACKLIGHT_UPDATE_HOTKEY solves the problem I 
was trying to solve. I'm not aware of any userspace that relies on 
BACKLIGHT_UPDATE_SYSFS.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

^ permalink raw reply

* [PATCH v3] video: add OpenCores VGA/LCD framebuffer driver
From: Stefan Kristiansson @ 2013-11-22  4:34 UTC (permalink / raw)
  To: linux-kernel, linux-fbdev; +Cc: tomi.valkeinen, plagnioj, Stefan Kristiansson

This adds support for the VGA/LCD core available from OpenCores:
http://opencores.org/project,vga_lcd

The driver have been tested together with both OpenRISC and
ARM (socfpga) processors.

Signed-off-by: Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
---
Changes in v2:
- Add Microblaze as an example user and fix a typo in Xilinx Zynq

Changes in v3:
- Use devm_kzalloc instead of kzalloc
- Remove superflous MODULE #ifdef
---
 drivers/video/Kconfig  |  17 ++
 drivers/video/Makefile |   1 +
 drivers/video/ocfb.c   | 471 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 489 insertions(+)
 create mode 100644 drivers/video/ocfb.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 84b685f..3b3f31e 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -979,6 +979,23 @@ config FB_PVR2
 	  (<file:drivers/video/pvr2fb.c>). Please see the file
 	  <file:Documentation/fb/pvr2fb.txt>.
 
+config FB_OPENCORES
+	tristate "OpenCores VGA/LCD core 2.0 framebuffer support"
+	depends on FB
+	select FB_CFB_FILLRECT
+	select FB_CFB_COPYAREA
+	select FB_CFB_IMAGEBLIT
+	default n
+	help
+	  This enables support for the OpenCores VGA/LCD core.
+
+	  The OpenCores VGA/LCD core is typically used together with
+	  softcore CPUs (e.g. OpenRISC or Microblaze) or hard processor
+	  systems (e.g. Altera socfpga or Xilinx Zynq) on FPGAs.
+
+	  The source code and specification for the core is available at
+	  <http://opencores.org/project,vga_lcd>
+
 config FB_S1D13XXX
 	tristate "Epson S1D13XXX framebuffer support"
 	depends on FB
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index e8bae8d..ae17ddf 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -150,6 +150,7 @@ obj-$(CONFIG_FB_NUC900)           += nuc900fb.o
 obj-$(CONFIG_FB_JZ4740)		  += jz4740_fb.o
 obj-$(CONFIG_FB_PUV3_UNIGFX)      += fb-puv3.o
 obj-$(CONFIG_FB_HYPERV)		  += hyperv_fb.o
+obj-$(CONFIG_FB_OPENCORES)	  += ocfb.o
 
 # Platform or fallback drivers go here
 obj-$(CONFIG_FB_UVESA)            += uvesafb.o
diff --git a/drivers/video/ocfb.c b/drivers/video/ocfb.c
new file mode 100644
index 0000000..3bf5f67
--- /dev/null
+++ b/drivers/video/ocfb.c
@@ -0,0 +1,471 @@
+/*
+ * OpenCores VGA/LCD 2.0 core frame buffer driver
+ *
+ * Copyright (C) 2013 Stefan Kristiansson, stefan.kristiansson@saunalahti.fi
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2.  This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/mm.h>
+#include <linux/dma-mapping.h>
+#include <linux/fb.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+
+/* OCFB register defines */
+#define OCFB_CTRL	0x000
+#define OCFB_STAT	0x004
+#define OCFB_HTIM	0x008
+#define OCFB_VTIM	0x00c
+#define OCFB_HVLEN	0x010
+#define OCFB_VBARA	0x014
+#define OCFB_PALETTE	0x800
+
+#define OCFB_CTRL_VEN	0x00000001 /* Video Enable */
+#define OCFB_CTRL_HIE	0x00000002 /* HSync Interrupt Enable */
+#define OCFB_CTRL_PC	0x00000800 /* 8-bit Pseudo Color Enable*/
+#define OCFB_CTRL_CD8	0x00000000 /* Color Depth 8 */
+#define OCFB_CTRL_CD16	0x00000200 /* Color Depth 16 */
+#define OCFB_CTRL_CD24	0x00000400 /* Color Depth 24 */
+#define OCFB_CTRL_CD32	0x00000600 /* Color Depth 32 */
+#define OCFB_CTRL_VBL1	0x00000000 /* Burst Length 1 */
+#define OCFB_CTRL_VBL2	0x00000080 /* Burst Length 2 */
+#define OCFB_CTRL_VBL4	0x00000100 /* Burst Length 4 */
+#define OCFB_CTRL_VBL8	0x00000180 /* Burst Length 8 */
+
+#define PALETTE_SIZE	256
+
+#define OCFB_NAME	"OC VGA/LCD"
+
+static char *mode_option;
+
+static const struct fb_videomode default_mode = {
+	/* 640x480 @ 60 Hz, 31.5 kHz hsync */
+	NULL, 60, 640, 480, 39721, 40, 24, 32, 11, 96, 2,
+	0, FB_VMODE_NONINTERLACED
+};
+
+struct ocfb_dev {
+	struct fb_info info;
+	/* Physical and virtual addresses of control regs */
+	phys_addr_t regs_phys;
+	int regs_phys_size;
+	void __iomem *regs;
+	/* flag indicating whether the regs are little endian accessed */
+	int little_endian;
+	/* Physical and virtual addresses of framebuffer */
+	phys_addr_t fb_phys;
+	void __iomem *fb_virt;
+	u32 pseudo_palette[PALETTE_SIZE];
+};
+
+struct ocfb_par {
+	void __iomem *pal_adr;
+};
+
+static struct ocfb_par ocfb_par_priv;
+
+static struct fb_var_screeninfo ocfb_var;
+static struct fb_fix_screeninfo ocfb_fix;
+
+#ifndef MODULE
+static int __init ocfb_setup(char *options)
+{
+	char *curr_opt;
+
+	if (!options || !*options)
+		return 0;
+
+	while ((curr_opt = strsep(&options, ",")) != NULL) {
+		if (!*curr_opt)
+			continue;
+		mode_option = curr_opt;
+	}
+
+	return 0;
+}
+#endif
+
+static inline u32 ocfb_readreg(struct ocfb_dev *fbdev, loff_t offset)
+{
+	if (fbdev->little_endian)
+		return ioread32(fbdev->regs + offset);
+	else
+		return ioread32be(fbdev->regs + offset);
+}
+
+static void ocfb_writereg(struct ocfb_dev *fbdev, loff_t offset, u32 data)
+{
+	if (fbdev->little_endian)
+		iowrite32(data, fbdev->regs + offset);
+	else
+		iowrite32be(data, fbdev->regs + offset);
+}
+
+static int ocfb_setupfb(struct ocfb_dev *fbdev)
+{
+	unsigned long bpp_config;
+	struct fb_var_screeninfo *var = &fbdev->info.var;
+	struct device *dev = fbdev->info.device;
+	u32 hlen;
+	u32 vlen;
+
+	/* Disable display */
+	ocfb_writereg(fbdev, OCFB_CTRL, 0);
+
+	/* Register framebuffer address */
+	fbdev->little_endian = 0;
+	ocfb_writereg(fbdev, OCFB_VBARA, fbdev->fb_phys);
+
+	/* Detect endianess */
+	if (ocfb_readreg(fbdev, OCFB_VBARA) != fbdev->fb_phys) {
+		fbdev->little_endian = 1;
+		ocfb_writereg(fbdev, OCFB_VBARA, fbdev->fb_phys);
+	}
+
+	/* Horizontal timings */
+	ocfb_writereg(fbdev, OCFB_HTIM, (var->hsync_len - 1) << 24 |
+		      (var->right_margin - 1) << 16 | (var->xres - 1));
+
+	/* Vertical timings */
+	ocfb_writereg(fbdev, OCFB_VTIM, (var->vsync_len - 1) << 24 |
+		      (var->lower_margin - 1) << 16 | (var->yres - 1));
+
+	/* Total length of frame */
+	hlen = var->left_margin + var->right_margin + var->hsync_len +
+		var->xres;
+
+	vlen = var->upper_margin + var->lower_margin + var->vsync_len +
+		var->yres;
+
+	ocfb_writereg(fbdev, OCFB_HVLEN, (hlen - 1) << 16 | (vlen - 1));
+
+	bpp_config = OCFB_CTRL_CD8;
+	switch (var->bits_per_pixel) {
+	case 8:
+		if (!var->grayscale)
+			bpp_config |= OCFB_CTRL_PC;  /* enable palette */
+		break;
+
+	case 16:
+		bpp_config |= OCFB_CTRL_CD16;
+		break;
+
+	case 24:
+		bpp_config |= OCFB_CTRL_CD24;
+		break;
+
+	case 32:
+		bpp_config |= OCFB_CTRL_CD32;
+		break;
+
+	default:
+		dev_err(dev, "no bpp specified\n");
+		break;
+	}
+
+	/* maximum (8) VBL (video memory burst length) */
+	bpp_config |= OCFB_CTRL_VBL8;
+
+	/* Enable output */
+	ocfb_writereg(fbdev, OCFB_CTRL, (OCFB_CTRL_VEN | bpp_config));
+
+	return 0;
+}
+
+static int ocfb_setcolreg(unsigned regno, unsigned red, unsigned green,
+			  unsigned blue, unsigned transp,
+			  struct fb_info *info)
+{
+	struct ocfb_par *par = (struct ocfb_par *)info->par;
+	u32 color;
+
+	if (regno >= info->cmap.len) {
+		dev_err(info->device, "regno >= cmap.len\n");
+		return 1;
+	}
+
+	if (info->var.grayscale) {
+		/* grayscale = 0.30*R + 0.59*G + 0.11*B */
+		red = green = blue = (red * 77 + green * 151 + blue * 28) >> 8;
+	}
+
+	red >>= (16 - info->var.red.length);
+	green >>= (16 - info->var.green.length);
+	blue >>= (16 - info->var.blue.length);
+	transp >>= (16 - info->var.transp.length);
+
+	if (info->var.bits_per_pixel = 8 && !info->var.grayscale) {
+		regno <<= 2;
+		color = (red << 16) | (green << 8) | blue;
+		ocfb_writereg(par->pal_adr, regno, color);
+	} else {
+		((u32 *)(info->pseudo_palette))[regno] +			(red << info->var.red.offset) |
+			(green << info->var.green.offset) |
+			(blue << info->var.blue.offset) |
+			(transp << info->var.transp.offset);
+	}
+
+	return 0;
+}
+
+static int ocfb_init_fix(struct ocfb_dev *fbdev)
+{
+	struct fb_var_screeninfo *var = &fbdev->info.var;
+	struct fb_fix_screeninfo *fix = &fbdev->info.fix;
+
+	strcpy(fix->id, OCFB_NAME);
+
+	fix->line_length = var->xres * var->bits_per_pixel/8;
+	fix->smem_len = fix->line_length * var->yres;
+	fix->type = FB_TYPE_PACKED_PIXELS;
+
+	if (var->bits_per_pixel = 8 && !var->grayscale)
+		fix->visual = FB_VISUAL_PSEUDOCOLOR;
+	else
+		fix->visual = FB_VISUAL_TRUECOLOR;
+
+	return 0;
+}
+
+static int ocfb_init_var(struct ocfb_dev *fbdev)
+{
+	struct fb_var_screeninfo *var = &fbdev->info.var;
+
+	var->accel_flags = FB_ACCEL_NONE;
+	var->activate = FB_ACTIVATE_NOW;
+	var->xres_virtual = var->xres;
+	var->yres_virtual = var->yres;
+
+	switch (var->bits_per_pixel) {
+	case 8:
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		var->red.offset = 0;
+		var->red.length = 8;
+		var->green.offset = 0;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		break;
+
+	case 16:
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		var->red.offset = 11;
+		var->red.length = 5;
+		var->green.offset = 5;
+		var->green.length = 6;
+		var->blue.offset = 0;
+		var->blue.length  = 5;
+		break;
+
+	case 24:
+		var->transp.offset = 0;
+		var->transp.length = 0;
+		var->red.offset = 16;
+		var->red.length = 8;
+		var->green.offset = 8;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		break;
+
+	case 32:
+		var->transp.offset = 24;
+		var->transp.length = 8;
+		var->red.offset = 16;
+		var->red.length = 8;
+		var->green.offset = 8;
+		var->green.length = 8;
+		var->blue.offset = 0;
+		var->blue.length = 8;
+		break;
+	}
+
+	return 0;
+}
+
+static struct fb_ops ocfb_ops = {
+	.owner		= THIS_MODULE,
+	.fb_setcolreg	= ocfb_setcolreg,
+	.fb_fillrect	= cfb_fillrect,
+	.fb_copyarea	= cfb_copyarea,
+	.fb_imageblit	= cfb_imageblit,
+};
+
+static int ocfb_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct ocfb_dev *fbdev;
+	struct ocfb_par *par = &ocfb_par_priv;
+	struct resource *res;
+	struct resource *mmio;
+	int fbsize;
+
+	fbdev = devm_kzalloc(&pdev->dev, sizeof(*fbdev), GFP_KERNEL);
+	if (!fbdev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, fbdev);
+
+	fbdev->info.fbops = &ocfb_ops;
+	fbdev->info.var = ocfb_var;
+	fbdev->info.fix = ocfb_fix;
+	fbdev->info.device = &pdev->dev;
+	fbdev->info.par = par;
+
+	/* Video mode setup */
+	if (!fb_find_mode(&fbdev->info.var, &fbdev->info, mode_option,
+			  NULL, 0, &default_mode, 16)) {
+		dev_err(&pdev->dev, "No valid video modes found\n");
+		return -EINVAL;
+	}
+	ocfb_init_var(fbdev);
+	ocfb_init_fix(fbdev);
+
+	/* Request I/O resource */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "I/O resource request failed\n");
+		return -ENXIO;
+	}
+	fbdev->regs_phys = res->start;
+	fbdev->regs_phys_size = resource_size(res);
+	mmio = devm_request_mem_region(&pdev->dev, res->start,
+				       resource_size(res), res->name);
+	if (!mmio) {
+		dev_err(&pdev->dev, "I/O memory space request failed\n");
+		return -ENXIO;
+	}
+	fbdev->regs = devm_ioremap_nocache(&pdev->dev, mmio->start,
+					   resource_size(mmio));
+	if (!fbdev->regs) {
+		dev_err(&pdev->dev, "I/O memory remap request failed\n");
+		return -ENXIO;
+	}
+	par->pal_adr = fbdev->regs + OCFB_PALETTE;
+
+	/* Allocate framebuffer memory */
+	fbsize = fbdev->info.fix.smem_len;
+	fbdev->fb_virt = dma_alloc_coherent(&pdev->dev, PAGE_ALIGN(fbsize),
+					    &fbdev->fb_phys, GFP_KERNEL);
+	if (!fbdev->fb_virt) {
+		dev_err(&pdev->dev,
+			"Frame buffer memory allocation failed\n");
+		ret = -ENOMEM;
+		goto err_release;
+	}
+	fbdev->info.fix.smem_start = fbdev->fb_phys;
+	fbdev->info.screen_base = (void __iomem *)fbdev->fb_virt;
+	fbdev->info.pseudo_palette = fbdev->pseudo_palette;
+
+	/* Clear framebuffer */
+	memset_io((void __iomem *)fbdev->fb_virt, 0, fbsize);
+
+	/* Setup and enable the framebuffer */
+	ocfb_setupfb(fbdev);
+
+	if (fbdev->little_endian)
+		fbdev->info.flags |= FBINFO_FOREIGN_ENDIAN;
+
+	/* Allocate color map */
+	ret = fb_alloc_cmap(&fbdev->info.cmap, PALETTE_SIZE, 0);
+	if (ret) {
+		dev_err(&pdev->dev, "Color map allocation failed\n");
+		goto err_dma_free;
+	}
+
+	/* Register framebuffer */
+	ret = register_framebuffer(&fbdev->info);
+	if (ret) {
+		dev_err(&pdev->dev, "Framebuffer registration failed\n");
+		goto err_dealloc_cmap;
+	}
+
+	return 0;
+
+err_dealloc_cmap:
+	fb_dealloc_cmap(&fbdev->info.cmap);
+
+err_dma_free:
+	dma_free_coherent(&pdev->dev, PAGE_ALIGN(fbsize), fbdev->fb_virt,
+			  fbdev->fb_phys);
+err_release:
+	release_mem_region(fbdev->regs_phys, fbdev->regs_phys_size);
+
+	return ret;
+}
+
+static int ocfb_remove(struct platform_device *pdev)
+{
+	struct ocfb_dev *fbdev = platform_get_drvdata(pdev);
+
+	unregister_framebuffer(&fbdev->info);
+	fb_dealloc_cmap(&fbdev->info.cmap);
+	dma_free_coherent(&pdev->dev, PAGE_ALIGN(fbdev->info.fix.smem_len),
+			  fbdev->fb_virt, fbdev->fb_phys);
+
+	/* Disable display */
+	ocfb_writereg(fbdev, OCFB_CTRL, 0);
+
+	release_mem_region(fbdev->regs_phys, fbdev->regs_phys_size);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct of_device_id ocfb_match[] = {
+	{ .compatible = "opencores,ocfb", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ocfb_match);
+
+static struct platform_driver ocfb_driver = {
+	.probe  = ocfb_probe,
+	.remove	= ocfb_remove,
+	.driver = {
+		.name = "ocfb_fb",
+		.of_match_table = ocfb_match,
+	}
+};
+
+/*
+ * Init and exit routines
+ */
+static int __init ocfb_init(void)
+{
+#ifndef MODULE
+	char *option = NULL;
+
+	if (fb_get_options("ocfb", &option))
+		return -ENODEV;
+	ocfb_setup(option);
+#endif
+	return platform_driver_register(&ocfb_driver);
+}
+
+static void __exit ocfb_exit(void)
+{
+	platform_driver_unregister(&ocfb_driver);
+}
+
+module_init(ocfb_init);
+module_exit(ocfb_exit);
+
+MODULE_AUTHOR("Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>");
+MODULE_DESCRIPTION("OpenCores VGA/LCD 2.0 frame buffer driver");
+MODULE_LICENSE("GPL v2");
+module_param(mode_option, charp, 0);
+MODULE_PARM_DESC(mode_option, "Video mode ('<xres>x<yres>[-<bpp>][@refresh]')");
-- 
1.8.3.2


^ permalink raw reply related

* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Henrique de Moraes Holschuh @ 2013-11-22 11:36 UTC (permalink / raw)
  To: Matthew Garrett
  Cc: Jingoo Han, 'Kyungmin Park',
	'Henrique de Moraes Holschuh', linux-fbdev, linux-kernel,
	kay, 'Richard Purdie', ibm-acpi-devel,
	platform-driver-x86
In-Reply-To: <20131121143326.GA19773@srcf.ucam.org>

On Thu, 21 Nov 2013, Matthew Garrett wrote:
> On Thu, Nov 21, 2013 at 09:43:32AM -0200, Henrique de Moraes Holschuh wrote:
> > With this patchset applied, as far as I can tell anything that used to be
> > uevent-driven by the backlight class will break: when a process changes the
> > backlight using sysfs, other processes will not be notified of the change
> > anymore.  This patchset seems to break backlight uevent support in such a
> > way that basically renders the entire thing useless and you might as well
> > just remove uevent support entirely.
> 
> The uevent support was initially added to handle systems where pressing 
> a hotkey generates an event (good) but the firmware automatically 
> changes the brightness (bad). I have absolutely no idea why I added 
> BACKLIGHT_UPDATE_SYSFS - BACKLIGHT_UPDATE_HOTKEY solves the problem I 
> was trying to solve. I'm not aware of any userspace that relies on 
> BACKLIGHT_UPDATE_SYSFS.

Well, either we have userspace that rely on the uevents, or we don't. 

If we don't have any uevent users of the backlight notifications, we might
as well just rip out the feature entirely and replace it with something with
a proper design.  But that would mean all OSD is being done by time-based
open-read-close polling of sysfs or keyed to input events (and therefore
half-baked).

However, if we do have anything that rely on the uevents, it needs
BACKLIGHT_UPDATE_SYSFS.  Without it, there will be no notifications when the
backlight level is changed through sysfs.  And we *DO* have applications
that change the backlight level through sysfs.  From the top of my head, I
know KDE does when it starts, and also as a response to power management
events.  Also, userspace hotkey daemons do use the sysfs interface.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

^ permalink raw reply

* Re: [Mesa-dev] [PATCH] dri3, i915, i965: Add __DRI_IMAGE_FOURCC_SARGB8888
From: Daniel Vetter @ 2013-11-22 16:17 UTC (permalink / raw)
  To: Keith Packard
  Cc: Linux Fbdev development list, intel-gfx, dri-devel,
	linaro-mm-sig@lists.linaro.org, Mesa Dev,
	linux-media@vger.kernel.org
In-Reply-To: <86d2lsem3m.fsf@miki.keithp.com>

On Fri, Nov 22, 2013 at 12:01 PM, Keith Packard <keithp@keithp.com> wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
>
>> Hm, where do we have the canonical source for all these fourcc codes? I'm
>> asking since we have our own copy in the kernel as drm_fourcc.h, and that
>> one is part of the userspace ABI since we use it to pass around
>> framebuffer formats and format lists.
>
> I think it's the kernel? I really don't know, as the whole notion of
> fourcc codes seems crazy to me...
>
> Feel free to steal this code and stick it in the kernel if you like.

Well, I wasn't ever in favour of using fourcc codes since they're just
not standardized at all, highly redundant in some cases and also miss
lots of stuff we actually need (like all the rgb formats).

Cc'ing the heck out of this to get kernel people to hopefully notice.
Maybe someone takes charge of this ... Otherwise meh.

>> Just afraid to create long-term maintainance madness here with the
>> kernel's iron thou-shalt-not-break-userspace-ever rule ... Not likely
>> we'll ever accept srgb for framebuffers though.
>
> Would suck to collide with something we do want though.

Yeah, it'd suck. But given how fourcc works we probably have that
already, just haven't noticed yet :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply

* Re: [PATCH] video: backlight: Remove backlight sysfs uevent
From: Matthew Garrett @ 2013-11-22 17:15 UTC (permalink / raw)
  To: Henrique de Moraes Holschuh
  Cc: Jingoo Han, 'Kyungmin Park',
	'Henrique de Moraes Holschuh', linux-fbdev, linux-kernel,
	kay, 'Richard Purdie', ibm-acpi-devel,
	platform-driver-x86
In-Reply-To: <20131122113601.GB27196@khazad-dum.debian.net>

On Fri, Nov 22, 2013 at 09:36:01AM -0200, Henrique de Moraes Holschuh wrote:
> On Thu, 21 Nov 2013, Matthew Garrett wrote:
> > The uevent support was initially added to handle systems where pressing 
> > a hotkey generates an event (good) but the firmware automatically 
> > changes the brightness (bad). I have absolutely no idea why I added 
> > BACKLIGHT_UPDATE_SYSFS - BACKLIGHT_UPDATE_HOTKEY solves the problem I 
> > was trying to solve. I'm not aware of any userspace that relies on 
> > BACKLIGHT_UPDATE_SYSFS.
> 
> Well, either we have userspace that rely on the uevents, or we don't. 

We have userspace that relies on uevents of type 
BACKLIGHT_UPDATE_HOTKEY. I don't know that we have userspace that relies 
on uevents of type BACKLIGHT_UPDATE_SYSFS.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

^ permalink raw reply

* Re: [Mesa-dev] [PATCH] dri3, i915, i965: Add __DRI_IMAGE_FOURCC_SARGB8888
From: Ville Syrjälä @ 2013-11-22 17:46 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Keith Packard, Linux Fbdev development list, intel-gfx, dri-devel,
	linaro-mm-sig@lists.linaro.org, Mesa Dev,
	linux-media@vger.kernel.org
In-Reply-To: <CAKMK7uEqHKOmMFXZLKno1q08X1B=U7XcJiExHaHbO9VdMeCihQ@mail.gmail.com>

On Fri, Nov 22, 2013 at 05:17:37PM +0100, Daniel Vetter wrote:
> On Fri, Nov 22, 2013 at 12:01 PM, Keith Packard <keithp@keithp.com> wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> >
> >> Hm, where do we have the canonical source for all these fourcc codes? I'm
> >> asking since we have our own copy in the kernel as drm_fourcc.h, and that
> >> one is part of the userspace ABI since we use it to pass around
> >> framebuffer formats and format lists.
> >
> > I think it's the kernel? I really don't know, as the whole notion of
> > fourcc codes seems crazy to me...
> >
> > Feel free to steal this code and stick it in the kernel if you like.
> 
> Well, I wasn't ever in favour of using fourcc codes since they're just
> not standardized at all, highly redundant in some cases and also miss
> lots of stuff we actually need (like all the rgb formats).

I also argued against them, but some people wanted them for whatever
reason. And since I didn't want to argue for several years about the
subject, I just gave in and made the drm pixel formats fourcss. But
given that I just pulled the fourccs out of my ass, we can't really
cross use them between different subsystems anyway. So if we just
consider all the different fourcc namespaces totally distinct, we're
not going to have any problems.

Personally I can promise that I will _not_ be checking Mesa/whatever
code for conflicting fourccs when I need to add a new one to drm_fourcc.h.
There, now I've given fair warning and if things explode later it won't be
my fault.

However if someone wants to emulate the drm fourcc style for whatever
reason, there is a distinct pattern how I cooked them up. Well, a few
different patterns depending whether it's RGB,YUV,packed,planar etc.

> 
> Cc'ing the heck out of this to get kernel people to hopefully notice.
> Maybe someone takes charge of this ... Otherwise meh.
> 
> >> Just afraid to create long-term maintainance madness here with the
> >> kernel's iron thou-shalt-not-break-userspace-ever rule ... Not likely
> >> we'll ever accept srgb for framebuffers though.
> >
> > Would suck to collide with something we do want though.
> 
> Yeah, it'd suck. But given how fourcc works we probably have that
> already, just haven't noticed yet :(
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply

* Re: [Intel-gfx] [Mesa-dev] [PATCH] dri3, i915, i965: Add __DRI_IMAGE_FOURCC_SARGB8888
From: Kristian Høgsberg @ 2013-11-22 22:12 UTC (permalink / raw)
  To: Daniel Vetter
  Cc: Keith Packard, Linux Fbdev development list, intel-gfx, dri-devel,
	linaro-mm-sig@lists.linaro.org, Mesa Dev,
	linux-media@vger.kernel.org
In-Reply-To: <CAKMK7uEqHKOmMFXZLKno1q08X1B=U7XcJiExHaHbO9VdMeCihQ@mail.gmail.com>

On Fri, Nov 22, 2013 at 05:17:37PM +0100, Daniel Vetter wrote:
> On Fri, Nov 22, 2013 at 12:01 PM, Keith Packard <keithp@keithp.com> wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> >
> >> Hm, where do we have the canonical source for all these fourcc codes? I'm
> >> asking since we have our own copy in the kernel as drm_fourcc.h, and that
> >> one is part of the userspace ABI since we use it to pass around
> >> framebuffer formats and format lists.
> >
> > I think it's the kernel? I really don't know, as the whole notion of
> > fourcc codes seems crazy to me...
> >
> > Feel free to steal this code and stick it in the kernel if you like.
> 
> Well, I wasn't ever in favour of using fourcc codes since they're just
> not standardized at all, highly redundant in some cases and also miss
> lots of stuff we actually need (like all the rgb formats).

These drm codes are not fourcc codes in any other way than that
they're defined by creating a 32 bit value by picking four characters.
I don't know what PTSD triggers people have from hearing "fourcc", but
it seems severe.  Forget all that, these codes are DRM specific
defines that are not inteded to match anything anybody else does.  It
doesn't matter if these match of conflict with v4l, fourcc.org,
wikipedia.org or what the amiga did.  They're just tokens that let us
define succintly what the pixel format of a kms framebuffer is and
tell the kernel.

I don't know what else you'd propose?  Pass an X visual in the ioctl?
An EGL config?  This is our name space, we can add stuff as we need
(as Keith is doing here). include/uapi/drm/drm_fourcc.h is the
canonical source for these values and we should add
DRM_FORMAT_SARGB8888 there to make sure we don't clash.

Why are these codes in mesa (and gbm and wl_drm protocol) then?
Because it turns out that once you have an stable and established
namespace for pixel formats (and a kernel userspace header is about as
stable and established as it gets) it makes a lot of sense to reuse
those values.

I already explained to Keith why we use different sets of format codes
in the DRI interface, but it's always fun to slam other peoples code.
Anyway, it's pretty simple, the __DRI_IMAGE_FORMAT_* defines predate
the introduction of drm_fourcc.h.  When we later added suport for
planar YUV __DRIimages, the kernel had picked up drm_fourcc.h after a
long sad bikeshedding flamewar, which included the planar formats we
needed.  At this point we could continue using our custom
__DRI_IMAGE_FORMAT_* defines or we could switch to the tokens that we
had finally converged on.  But don't let me ruin a good old snide remark.

> Cc'ing the heck out of this to get kernel people to hopefully notice.
> Maybe someone takes charge of this ... Otherwise meh.

I don't know what you want to change.  These values are already kernel
ABI, we use them in drmAddFB2, and again, I don't understand what
problem you're seeing.

Kristian

> >> Just afraid to create long-term maintainance madness here with the
> >> kernel's iron thou-shalt-not-break-userspace-ever rule ... Not likely
> >> we'll ever accept srgb for framebuffers though.
> >
> > Would suck to collide with something we do want though.
> 
> Yeah, it'd suck. But given how fourcc works we probably have that
> already, just haven't noticed yet :(
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply

* Re: [Intel-gfx] [Mesa-dev] [PATCH] dri3, i915, i965: Add __DRI_IMAGE_FOURCC_SARGB8888
From: Ville Syrjälä @ 2013-11-22 23:05 UTC (permalink / raw)
  To: Kristian Høgsberg
  Cc: Daniel Vetter, intel-gfx, Linux Fbdev development list, dri-devel,
	linaro-mm-sig@lists.linaro.org, Mesa Dev,
	linux-media@vger.kernel.org
In-Reply-To: <20131122221213.GA3234@tokamak.local>

On Fri, Nov 22, 2013 at 02:12:13PM -0800, Kristian Høgsberg wrote:
> On Fri, Nov 22, 2013 at 05:17:37PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 22, 2013 at 12:01 PM, Keith Packard <keithp@keithp.com> wrote:
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > >
> > >> Hm, where do we have the canonical source for all these fourcc codes? I'm
> > >> asking since we have our own copy in the kernel as drm_fourcc.h, and that
> > >> one is part of the userspace ABI since we use it to pass around
> > >> framebuffer formats and format lists.
> > >
> > > I think it's the kernel? I really don't know, as the whole notion of
> > > fourcc codes seems crazy to me...
> > >
> > > Feel free to steal this code and stick it in the kernel if you like.
> > 
> > Well, I wasn't ever in favour of using fourcc codes since they're just
> > not standardized at all, highly redundant in some cases and also miss
> > lots of stuff we actually need (like all the rgb formats).
> 
> These drm codes are not fourcc codes in any other way than that
> they're defined by creating a 32 bit value by picking four characters.
> I don't know what PTSD triggers people have from hearing "fourcc", but
> it seems severe.  Forget all that, these codes are DRM specific
> defines that are not inteded to match anything anybody else does.  It
> doesn't matter if these match of conflict with v4l, fourcc.org,
> wikipedia.org or what the amiga did.  They're just tokens that let us
> define succintly what the pixel format of a kms framebuffer is and
> tell the kernel.
> 
> I don't know what else you'd propose?  Pass an X visual in the ioctl?
> An EGL config?  This is our name space, we can add stuff as we need
> (as Keith is doing here). include/uapi/drm/drm_fourcc.h is the
> canonical source for these values and we should add
> DRM_FORMAT_SARGB8888 there to make sure we don't clash.

What is this format anyway? -ENODOCS

If its just an srgb version of ARGB8888, then I wouldn't really want it
in drm_fourcc.h. I expect colorspacy stuff will be handled by various
crtc/plane properties in the kernel so we don't need to encode that
stuff into the fb format.

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply

* Re: [Intel-gfx] [Mesa-dev] [PATCH] dri3, i915, i965: Add __DRI_IMAGE_FOURCC_SARGB8888
From: Keith Packard @ 2013-11-22 23:36 UTC (permalink / raw)
  To: Kristian Høgsberg, Daniel Vetter
  Cc: Linux Fbdev development list, intel-gfx, dri-devel,
	linaro-mm-sig@lists.linaro.org, Mesa Dev,
	linux-media@vger.kernel.org
In-Reply-To: <20131122221213.GA3234@tokamak.local>

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

Kristian Høgsberg <hoegsberg@gmail.com> writes:

> I already explained to Keith why we use different sets of format codes
> in the DRI interface, but it's always fun to slam other peoples code.

As we discussed, my complaint isn't so much about __DRI_IMAGE_FOURCC,
but the fact that the __DRIimage interfaces use *both*
__DRI_IMAGE_FOURCC and __DRI_IMAGE_FORMAT at different times.

Ok, here's a fine thing we can actually fix -- the pattern that mesa
uses all over the place in converting formats looks like this (not to
pick on anyone, it's repeated everywhere, this is just the first one I
found in gbm_dri.c):

	static uint32_t
        gbm_dri_to_gbm_format(uint32_t dri_format)
	{
	   uint32_t ret = 0;
	
	   switch (dri_format) {
	   case __DRI_IMAGE_FORMAT_RGB565:
	      ret = GBM_FORMAT_RGB565;
	      break;
	   case __DRI_IMAGE_FORMAT_XRGB8888:
	      ret = GBM_FORMAT_XRGB8888;
	      break;
	   case __DRI_IMAGE_FORMAT_ARGB8888:
	      ret = GBM_FORMAT_ARGB8888;
	      break;
	   case __DRI_IMAGE_FORMAT_ABGR8888:
	      ret = GBM_FORMAT_ABGR8888;
	      break;
	   default:
	      ret = 0;
	      break;
	   }
	
	   return ret;
	}

The problem here is that any unknown incoming formats get translated to
garbage (0) outgoing. With fourcc codes, there is the slight advantage
that 0 is never a legal value, but it sure would be nice to print a
warning or even abort if you get a format code you don't understand as
there's no way 0 is ever going to do what you want.

Anyone have a preference? Abort? Print an error? Silently continue to do
the wrong thing?

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

^ permalink raw reply

* Re: [Intel-gfx] [Mesa-dev] [PATCH] dri3, i915, i965: Add __DRI_IMAGE_FOURCC_SARGB8888
From: Keith Packard @ 2013-11-22 23:43 UTC (permalink / raw)
  To: Ville Syrjälä, Kristian Høgsberg
  Cc: Linux Fbdev development list, intel-gfx, dri-devel,
	linaro-mm-sig@lists.linaro.org, Mesa Dev,
	linux-media@vger.kernel.org
In-Reply-To: <20131122230504.GK10036@intel.com>

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

Ville Syrjälä <ville.syrjala@linux.intel.com> writes:

> What is this format anyway? -ENODOCS

Same as MESA_FORMAT_SARGB8 and __DRI_IMAGE_FORMAT_SARGB8 :-)

> If its just an srgb version of ARGB8888, then I wouldn't really want it
> in drm_fourcc.h. I expect colorspacy stuff will be handled by various
> crtc/plane properties in the kernel so we don't need to encode that
> stuff into the fb format.

It's not any different from splitting YUV codes from RGB codes; a
different color encoding with the same bitfields. And, for mesa, it's
necessary to differentiate between ARGB and SARGB within the same format
code given how the API is structured. So, we have choices:

 1) Let Mesa define it's own fourcc codes and risk future accidental
    collisions
 
 2) Rewrite piles of mesa code to split out the color encoding from the
    bitfield information and pass it separately.

 3) Add "reasonable" format codes like this to the kernel fourcc list.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox