[V2] media: i2c: Add ADV761X support

  This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
support. Only one HDMI port is supported on ADV7612.

The code is based on the ADV7604 driver, and ADV7612 patch by
Shinobu Uehara <shinobu.uehara.xc@renesas.com>

Changes in version 2:
* Used platform data for I2C addresses setup. The driver
  should work with both SoC and non-SoC camera models.
* Dropped unnecessary code and unsupported callbacks.
* Implemented IRQ handling for format change detection.

Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
---
 drivers/media/i2c/Kconfig   |   11 +
 drivers/media/i2c/Makefile  |    1 +
 drivers/media/i2c/adv761x.c | 1009 +++++++++++++++++++++++++++++++++++++++++++
 include/media/adv761x.h     |   38 ++
 4 files changed, 1059 insertions(+)
 create mode 100644 drivers/media/i2c/adv761x.c
 create mode 100644 include/media/adv761x.h
  

Hi Valentine,

I don't entirely understand how you use this driver with soc-camera.
Since soc-camera doesn't support FMT_CHANGE notifies it can't really
act on it. Did you hack soc-camera to do this?

The way it stands I would prefer to see a version of the driver without
soc-camera support. I wouldn't have a problem merging that as this driver
is a good base for further development.

You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
control. It's easy to implement and that way apps can be notified when
the hotplug changes value.

Regards,

	Hans

On 11/15/13 13:54, Valentine Barshak wrote:
> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
> support. Only one HDMI port is supported on ADV7612.
> 
> The code is based on the ADV7604 driver, and ADV7612 patch by
> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
> 
> Changes in version 2:
> * Used platform data for I2C addresses setup. The driver
>   should work with both SoC and non-SoC camera models.
> * Dropped unnecessary code and unsupported callbacks.
> * Implemented IRQ handling for format change detection.
> 
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> ---
>  drivers/media/i2c/Kconfig   |   11 +
>  drivers/media/i2c/Makefile  |    1 +
>  drivers/media/i2c/adv761x.c | 1009 +++++++++++++++++++++++++++++++++++++++++++
>  include/media/adv761x.h     |   38 ++
>  4 files changed, 1059 insertions(+)
>  create mode 100644 drivers/media/i2c/adv761x.c
>  create mode 100644 include/media/adv761x.h
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 75c8a03..2388642 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -206,6 +206,17 @@ config VIDEO_ADV7604
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adv7604.
>  
> +config VIDEO_ADV761X
> +	tristate "Analog Devices ADV761X decoder"
> +	depends on VIDEO_V4L2 && I2C
> +	---help---
> +	  Support for the Analog Devices ADV7611/ADV7612 video decoder.
> +
> +	  This is an Analog Devices Xpressview HDMI Receiver.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called adv761x.
> +
>  config VIDEO_ADV7842
>  	tristate "Analog Devices ADV7842 decoder"
>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index e03f177..d78d627 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_VIDEO_ADV7183) += adv7183.o
>  obj-$(CONFIG_VIDEO_ADV7343) += adv7343.o
>  obj-$(CONFIG_VIDEO_ADV7393) += adv7393.o
>  obj-$(CONFIG_VIDEO_ADV7604) += adv7604.o
> +obj-$(CONFIG_VIDEO_ADV761X) += adv761x.o
>  obj-$(CONFIG_VIDEO_ADV7842) += adv7842.o
>  obj-$(CONFIG_VIDEO_AD9389B) += ad9389b.o
>  obj-$(CONFIG_VIDEO_ADV7511) += adv7511.o
> diff --git a/drivers/media/i2c/adv761x.c b/drivers/media/i2c/adv761x.c
> new file mode 100644
> index 0000000..95939f5
> --- /dev/null
> +++ b/drivers/media/i2c/adv761x.c
> @@ -0,0 +1,1009 @@
> +/*
> + * adv761x Analog Devices ADV761X HDMI receiver driver
> + *
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/rwsem.h>
> +#include <linux/slab.h>
> +#include <linux/videodev2.h>
> +#include <media/adv761x.h>
> +#include <media/soc_camera.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +
> +#define ADV761X_DRIVER_NAME "adv761x"
> +
> +/* VERT_FILTER_LOCKED and DE_REGEN_FILTER_LOCKED flags */
> +#define ADV761X_HDMI_F_LOCKED(v)	(((v) & 0xa0) == 0xa0)
> +
> +/* Maximum supported resolution */
> +#define ADV761X_MAX_WIDTH		1920
> +#define ADV761X_MAX_HEIGHT		1080
> +
> +/* Use SoC camera subdev desc private data for platform_data */
> +#define ADV761X_SOC_CAM_QUIRK		0x1
> +
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +struct adv761x_color_format {
> +	enum v4l2_mbus_pixelcode code;
> +	enum v4l2_colorspace colorspace;
> +};
> +
> +/* Supported color format list */
> +static const struct adv761x_color_format adv761x_cfmts[] = {
> +	{
> +		.code		= V4L2_MBUS_FMT_RGB888_1X24,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +	},
> +};
> +
> +/* ADV761X descriptor structure */
> +struct adv761x_state {
> +	struct v4l2_subdev			sd;
> +	struct media_pad			pad;
> +	struct v4l2_ctrl_handler		ctrl_hdl;
> +
> +	u8					edid[256];
> +	unsigned				edid_blocks;
> +
> +	struct rw_semaphore			rwsem;
> +	const struct adv761x_color_format	*cfmt;
> +	u32					width;
> +	u32					height;
> +	enum v4l2_field				scanmode;
> +	u32					status;
> +
> +	int					gpio;
> +	int					irq;
> +
> +	struct workqueue_struct			*work_queue;
> +	struct delayed_work			enable_hotplug;
> +	struct work_struct			interrupt_service;
> +
> +	struct i2c_client			*i2c_cec;
> +	struct i2c_client			*i2c_inf;
> +	struct i2c_client			*i2c_dpll;
> +	struct i2c_client			*i2c_rep;
> +	struct i2c_client			*i2c_edid;
> +	struct i2c_client			*i2c_hdmi;
> +	struct i2c_client			*i2c_cp;
> +};
> +
> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
> +{
> +	return &container_of(ctrl->handler, struct adv761x_state, ctrl_hdl)->sd;
> +}
> +
> +static inline struct adv761x_state *to_state(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct adv761x_state, sd);
> +}
> +
> +/* I2C I/O operations */
> +static s32 adv_smbus_read_byte_data(struct i2c_client *client, u8 command)
> +{
> +	s32 ret, i;
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = i2c_smbus_read_byte_data(client, command);
> +		if (ret >= 0)
> +			return ret;
> +	}
> +
> +	v4l_err(client, "Reading addr:%02x reg:%02x\n failed",
> +		client->addr, command);
> +	return ret;
> +}
> +
> +static s32 adv_smbus_write_byte_data(struct i2c_client *client, u8 command,
> +				     u8 value)
> +{
> +	s32 ret, i;
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = i2c_smbus_write_byte_data(client, command, value);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	v4l_err(client, "Writing addr:%02x reg:%02x val:%02x failed\n",
> +		client->addr, command, value);
> +	return ret;
> +}
> +
> +static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client, u8 command,
> +					  u8 length, const u8 *values)
> +{
> +	s32 ret, i;
> +
> +	ret = i2c_smbus_write_i2c_block_data(client, command, length, values);
> +	if (!ret)
> +		return 0;
> +
> +	for (i = 0; i < length; i++) {
> +		ret = adv_smbus_write_byte_data(client, command + i, values[i]);
> +		if (ret)
> +			break;
> +	}
> +
> +	return ret;
> +}
> +
> +static inline int io_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	return adv_smbus_read_byte_data(client, reg);
> +}
> +
> +static inline int io_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	return adv_smbus_write_byte_data(client, reg, val);
> +}
> +
> +static inline int cec_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_cec, reg);
> +}
> +
> +static inline int cec_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_cec, reg, val);
> +}
> +
> +static inline int infoframe_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_inf, reg);
> +}
> +
> +static inline int infoframe_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_inf, reg, val);
> +}
> +
> +static inline int dpll_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_dpll, reg);
> +}
> +
> +static inline int dpll_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_dpll, reg, val);
> +}
> +
> +static inline int rep_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_rep, reg);
> +}
> +
> +static inline int rep_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_rep, reg, val);
> +}
> +
> +static inline int edid_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_edid, reg);
> +}
> +
> +static inline int edid_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_edid, reg, val);
> +}
> +
> +static inline int edid_write_block(struct v4l2_subdev *sd,
> +				   unsigned len, const u8 *val)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +	struct adv761x_state *state = to_state(sd);
> +	int ret = 0;
> +	int i;
> +
> +	v4l2_dbg(2, debug, sd, "Writing EDID block (%d bytes)\n", len);
> +
> +	v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
> +
> +	/* Disable I2C access to internal EDID ram from DDC port */
> +	rep_write(sd, 0x74, 0x0);
> +
> +	for (i = 0; !ret && i < len; i += I2C_SMBUS_BLOCK_MAX)
> +		ret = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
> +				I2C_SMBUS_BLOCK_MAX, val + i);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * ADV761x calculates the checksums and enables I2C access
> +	 * to internal EDID ram from DDC port.
> +	 */
> +	rep_write(sd, 0x74, 0x01);
> +
> +	for (i = 0; i < 1000; i++) {
> +		if (rep_read(sd, 0x76) & 0x1) {
> +			/* Enable hotplug after 100 ms */
> +			queue_delayed_work(state->work_queue,
> +					   &state->enable_hotplug, HZ / 10);
> +			return 0;
> +		}
> +		schedule();
> +	}
> +
> +	v4l_err(client, "Enabling EDID failed\n");
> +	return -EIO;
> +}
> +
> +static inline int hdmi_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_hdmi, reg);
> +}
> +
> +static inline int hdmi_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_hdmi, reg, val);
> +}
> +
> +static inline int cp_read(struct v4l2_subdev *sd, u8 reg)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_read_byte_data(state->i2c_cp, reg);
> +}
> +
> +static inline int cp_write(struct v4l2_subdev *sd, u8 reg, u8 val)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	return adv_smbus_write_byte_data(state->i2c_cp, reg, val);
> +}
> +
> +static inline int adv761x_power_off(struct v4l2_subdev *sd)
> +{
> +	return io_write(sd, 0x0c, 0x62);
> +}
> +
> +static int adv761x_core_init(struct v4l2_subdev *sd)
> +{
> +	io_write(sd, 0x01, 0x06);	/* V-FREQ = 60Hz */
> +					/* Prim_Mode = HDMI-GR */
> +	io_write(sd, 0x02, 0xf2);	/* Auto CSC, RGB out */
> +					/* Disable op_656 bit */
> +	io_write(sd, 0x03, 0x42);	/* 36 bit SDR 444 Mode 0 */
> +	io_write(sd, 0x05, 0x28);	/* AV Codes Off */
> +	io_write(sd, 0x0b, 0x44);	/* Power up part */
> +	io_write(sd, 0x0c, 0x42);	/* Power up part */
> +	io_write(sd, 0x14, 0x7f);	/* Max Drive Strength */
> +	io_write(sd, 0x15, 0x80);	/* Disable Tristate of Pins */
> +					/* (Audio output pins active) */
> +	io_write(sd, 0x19, 0x83);	/* LLC DLL phase */
> +	io_write(sd, 0x33, 0x40);	/* LLC DLL enable */
> +
> +	cp_write(sd, 0xba, 0x01);	/* Set HDMI FreeRun */
> +	cp_write(sd, 0x3e, 0x80);	/* Enable color adjustments */
> +
> +	hdmi_write(sd, 0x9b, 0x03);	/* ADI recommended setting */
> +	hdmi_write(sd, 0x00, 0x08);	/* Set HDMI Input Port A */
> +					/* (BG_MEAS_PORT_SEL = 001b) */
> +	hdmi_write(sd, 0x02, 0x03);	/* Enable Ports A & B in */
> +					/* background mode */
> +	hdmi_write(sd, 0x6d, 0x80);	/* Enable TDM mode */
> +	hdmi_write(sd, 0x03, 0x18);	/* I2C mode 24 bits */
> +	hdmi_write(sd, 0x83, 0xfc);	/* Enable clock terminators */
> +					/* for port A & B */
> +	hdmi_write(sd, 0x6f, 0x0c);	/* ADI recommended setting */
> +	hdmi_write(sd, 0x85, 0x1f);	/* ADI recommended setting */
> +	hdmi_write(sd, 0x87, 0x70);	/* ADI recommended setting */
> +	hdmi_write(sd, 0x8d, 0x04);	/* LFG Port A */
> +	hdmi_write(sd, 0x8e, 0x1e);	/* HFG Port A */
> +	hdmi_write(sd, 0x1a, 0x8a);	/* unmute audio */
> +	hdmi_write(sd, 0x57, 0xda);	/* ADI recommended setting */
> +	hdmi_write(sd, 0x58, 0x01);	/* ADI recommended setting */
> +	hdmi_write(sd, 0x75, 0x10);	/* DDC drive strength */
> +	hdmi_write(sd, 0x90, 0x04);	/* LFG Port B */
> +	hdmi_write(sd, 0x91, 0x1e);	/* HFG Port B */
> +	hdmi_write(sd, 0x04, 0x03);
> +	hdmi_write(sd, 0x14, 0x00);
> +	hdmi_write(sd, 0x15, 0x00);
> +	hdmi_write(sd, 0x16, 0x00);
> +
> +	rep_write(sd, 0x40, 0x81);	/* Disable HDCP 1.1 features */
> +	rep_write(sd, 0x74, 0x00);	/* Disable the Internal EDID */
> +					/* for all ports */
> +
> +	/* Setup interrupts */
> +	io_write(sd, 0x40, 0xc2);	/* Active high until cleared */
> +	io_write(sd, 0x6e, 0x03);	/* INT1 HDMI DE_REGEN and V_LOCK */
> +
> +	return v4l2_ctrl_handler_setup(sd->ctrl_handler);
> +}
> +
> +static int adv761x_hdmi_info(struct v4l2_subdev *sd, enum v4l2_field *scanmode,
> +			     u32 *width, u32 *height)
> +{
> +	int msb, val;
> +
> +	/* Line width */
> +	msb = hdmi_read(sd, 0x07);
> +	if (msb < 0)
> +		return msb;
> +
> +	if (!ADV761X_HDMI_F_LOCKED(msb))
> +		return -EAGAIN;
> +
> +	/* Interlaced or progressive */
> +	val = hdmi_read(sd, 0x0b);
> +	if (val < 0)
> +		return val;
> +
> +	*scanmode = (val & 0x20) ? V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
> +	val = hdmi_read(sd, 0x08);
> +	if (val < 0)
> +		return val;
> +
> +	val |= (msb & 0x1f) << 8;
> +	*width = val;
> +
> +	/* Lines per frame */
> +	msb = hdmi_read(sd, 0x09);
> +	if (msb < 0)
> +		return msb;
> +
> +	val = hdmi_read(sd, 0x0a);
> +	if (val < 0)
> +		return val;
> +
> +	val |= (msb & 0x1f) << 8;
> +	if (*scanmode == V4L2_FIELD_INTERLACED)
> +		val <<= 1;
> +	*height = val;
> +
> +	if (*width == 0 || *height == 0)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +/* Hotplug work */
> +static void adv761x_enable_hotplug(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct adv761x_state *state = container_of(dwork, struct adv761x_state,
> +						   enable_hotplug);
> +	struct v4l2_subdev *sd = &state->sd;
> +
> +	v4l2_dbg(2, debug, sd, "Enable hotplug\n");
> +	v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)1);
> +}
> +
> +/* IRQ work */
> +static void adv761x_interrupt_service(struct work_struct *work)
> +{
> +	struct adv761x_state *state = container_of(work, struct adv761x_state,
> +						   interrupt_service);
> +	struct v4l2_subdev *sd = &state->sd;
> +	enum v4l2_field scanmode;
> +	u32 width, height;
> +	u32 status = 0;
> +	int ret;
> +
> +	/* Clear HDMI interrupts */
> +	io_write(sd, 0x6c, 0xff);
> +
> +	ret = adv761x_hdmi_info(sd, &scanmode, &width, &height);
> +	if (ret) {
> +		if (state->status == V4L2_IN_ST_NO_SIGNAL)
> +			return;
> +
> +		width = ADV761X_MAX_WIDTH;
> +		height = ADV761X_MAX_HEIGHT;
> +		scanmode = V4L2_FIELD_NONE;
> +		status = V4L2_IN_ST_NO_SIGNAL;
> +	}
> +
> +	if (status)
> +		v4l2_dbg(2, debug, sd, "No HDMI video input detected\n");
> +	else
> +		v4l2_dbg(2, debug, sd, "HDMI video input detected (%ux%u%c)\n",
> +			 width, height,
> +			 scanmode == V4L2_FIELD_NONE ? 'p' : 'i');
> +
> +	down_write(&state->rwsem);
> +	state->width = width;
> +	state->height = height;
> +	state->scanmode = scanmode;
> +	state->status = status;
> +	up_write(&state->rwsem);
> +
> +	v4l2_subdev_notify(sd, ADV761X_FMT_CHANGE, NULL);
> +}
> +
> +/* IRQ handler */
> +static irqreturn_t adv761x_irq_handler(int irq, void *devid)
> +{
> +	struct adv761x_state *state = devid;
> +
> +	queue_work(state->work_queue, &state->interrupt_service);
> +	return IRQ_HANDLED;
> +}
> +
> +/* v4l2_subdev_core_ops */
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static void adv761x_inv_register(struct v4l2_subdev *sd)
> +{
> +	v4l2_info(sd, "0x000-0x0ff: IO Map\n");
> +	v4l2_info(sd, "0x100-0x1ff: CEC Map\n");
> +	v4l2_info(sd, "0x200-0x2ff: InfoFrame Map\n");
> +	v4l2_info(sd, "0x300-0x3ff: DPLL Map\n");
> +	v4l2_info(sd, "0x400-0x4ff: Repeater Map\n");
> +	v4l2_info(sd, "0x500-0x5ff: EDID Map\n");
> +	v4l2_info(sd, "0x600-0x6ff: HDMI Map\n");
> +	v4l2_info(sd, "0x700-0x7ff: CP Map\n");
> +}
> +
> +static int adv761x_g_register(struct v4l2_subdev *sd,
> +			      struct v4l2_dbg_register *reg)
> +{
> +	reg->size = 1;
> +	switch (reg->reg >> 8) {
> +	case 0:
> +		reg->val = io_read(sd, reg->reg & 0xff);
> +		break;
> +	case 1:
> +		reg->val = cec_read(sd, reg->reg & 0xff);
> +		break;
> +	case 2:
> +		reg->val = infoframe_read(sd, reg->reg & 0xff);
> +		break;
> +	case 3:
> +		reg->val = dpll_read(sd, reg->reg & 0xff);
> +		break;
> +	case 4:
> +		reg->val = rep_read(sd, reg->reg & 0xff);
> +		break;
> +	case 5:
> +		reg->val = edid_read(sd, reg->reg & 0xff);
> +		break;
> +	case 6:
> +		reg->val = hdmi_read(sd, reg->reg & 0xff);
> +		break;
> +	case 7:
> +		reg->val = cp_read(sd, reg->reg & 0xff);
> +		break;
> +	default:
> +		v4l2_info(sd, "Register %03llx not supported\n", reg->reg);
> +		adv761x_inv_register(sd);
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int adv761x_s_register(struct v4l2_subdev *sd,
> +			      const struct v4l2_dbg_register *reg)
> +{
> +	switch (reg->reg >> 8) {
> +	case 0:
> +		io_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 1:
> +		cec_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 2:
> +		infoframe_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 3:
> +		dpll_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 4:
> +		rep_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 5:
> +		edid_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 6:
> +		hdmi_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	case 7:
> +		cp_write(sd, reg->reg & 0xff, reg->val & 0xff);
> +		break;
> +	default:
> +		v4l2_info(sd, "Register %03llx not supported\n", reg->reg);
> +		adv761x_inv_register(sd);
> +		break;
> +	}
> +	return 0;
> +}
> +#endif	/* CONFIG_VIDEO_ADV_DEBUG */
> +
> +/* v4l2_subdev_video_ops */
> +static int adv761x_g_input_status(struct v4l2_subdev *sd, u32 *status)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	down_read(&state->rwsem);
> +	*status = state->status;
> +	up_read(&state->rwsem);
> +	return 0;
> +}
> +
> +static int adv761x_g_mbus_fmt(struct v4l2_subdev *sd,
> +			      struct v4l2_mbus_framefmt *mf)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	down_read(&state->rwsem);
> +	mf->width = state->width;
> +	mf->height = state->height;
> +	mf->field = state->scanmode;
> +	mf->code = state->cfmt->code;
> +	mf->colorspace = state->cfmt->colorspace;
> +	up_read(&state->rwsem);
> +	return 0;
> +}
> +
> +static int adv761x_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
> +				 enum v4l2_mbus_pixelcode *code)
> +{
> +	/* Check requested format index is within range */
> +	if (index >= ARRAY_SIZE(adv761x_cfmts))
> +		return -EINVAL;
> +
> +	*code = adv761x_cfmts[index].code;
> +
> +	return 0;
> +}
> +
> +static int adv761x_g_mbus_config(struct v4l2_subdev *sd,
> +				 struct v4l2_mbus_config *cfg)
> +{
> +	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
> +		V4L2_MBUS_VSYNC_ACTIVE_LOW | V4L2_MBUS_HSYNC_ACTIVE_LOW |
> +		V4L2_MBUS_DATA_ACTIVE_HIGH;
> +	cfg->type = V4L2_MBUS_PARALLEL;
> +
> +	return 0;
> +}
> +
> +/* v4l2_subdev_pad_ops */
> +static int adv761x_get_edid(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_edid *edid)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +
> +	if (edid->pad != 0)
> +		return -EINVAL;
> +
> +	if (edid->blocks == 0)
> +		return -EINVAL;
> +
> +	if (edid->start_block >= state->edid_blocks)
> +		return -EINVAL;
> +
> +	if (edid->start_block + edid->blocks > state->edid_blocks)
> +		edid->blocks = state->edid_blocks - edid->start_block;
> +	if (!edid->edid)
> +		return -EINVAL;
> +
> +	memcpy(edid->edid + edid->start_block * 128,
> +	       state->edid + edid->start_block * 128,
> +	       edid->blocks * 128);
> +	return 0;
> +}
> +
> +static int adv761x_set_edid(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_edid *edid)
> +{
> +	struct adv761x_state *state = to_state(sd);
> +	int ret;
> +
> +	if (edid->pad != 0)
> +		return -EINVAL;
> +
> +	if (edid->start_block != 0)
> +		return -EINVAL;
> +
> +	if (edid->blocks == 0) {
> +		/* Pull down the hotplug pin */
> +		v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
> +		/* Disable I2C access to internal EDID RAM from DDC port */
> +		rep_write(sd, 0x74, 0x0);
> +		state->edid_blocks = 0;
> +		return 0;
> +	}
> +
> +	if (edid->blocks > 2)
> +		return -E2BIG;
> +
> +	if (!edid->edid)
> +		return -EINVAL;
> +
> +	memcpy(state->edid, edid->edid, 128 * edid->blocks);
> +	state->edid_blocks = edid->blocks;
> +
> +	ret = edid_write_block(sd, 128 * edid->blocks, state->edid);
> +	if (ret < 0)
> +		v4l2_err(sd, "Writing EDID failed\n");
> +
> +	return ret;
> +}
> +
> +/* v4l2_ctrl_ops */
> +static int adv761x_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_subdev *sd = to_sd(ctrl);
> +	u8 val = ctrl->val;
> +	int ret;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		ret = cp_write(sd, 0x3c, val);
> +		break;
> +	case V4L2_CID_CONTRAST:
> +		ret = cp_write(sd, 0x3a, val);
> +		break;
> +	case V4L2_CID_SATURATION:
> +		ret = cp_write(sd, 0x3b, val);
> +		break;
> +	case V4L2_CID_HUE:
> +		ret = cp_write(sd, 0x3d, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +/* V4L structures */
> +static const struct v4l2_subdev_core_ops adv761x_core_ops = {
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +	.g_register	= adv761x_g_register,
> +	.s_register	= adv761x_s_register,
> +#endif
> +};
> +
> +static const struct v4l2_subdev_video_ops adv761x_video_ops = {
> +	.g_input_status = adv761x_g_input_status,
> +	.g_mbus_fmt	= adv761x_g_mbus_fmt,
> +	.try_mbus_fmt	= adv761x_g_mbus_fmt,
> +	.s_mbus_fmt	= adv761x_g_mbus_fmt,
> +	.enum_mbus_fmt	= adv761x_enum_mbus_fmt,
> +	.g_mbus_config	= adv761x_g_mbus_config,
> +};
> +
> +static const struct v4l2_subdev_pad_ops adv761x_pad_ops = {
> +	.get_edid = adv761x_get_edid,
> +	.set_edid = adv761x_set_edid,
> +};
> +
> +static const struct v4l2_subdev_ops adv761x_ops = {
> +	.core = &adv761x_core_ops,
> +	.video = &adv761x_video_ops,
> +	.pad = &adv761x_pad_ops,
> +};
> +
> +static const struct v4l2_ctrl_ops adv761x_ctrl_ops = {
> +	.s_ctrl = adv761x_s_ctrl,
> +};
> +
> +/* Device initialization and clean-up */
> +static void adv761x_unregister_clients(struct adv761x_state *state)
> +{
> +	if (state->i2c_cec)
> +		i2c_unregister_device(state->i2c_cec);
> +	if (state->i2c_inf)
> +		i2c_unregister_device(state->i2c_inf);
> +	if (state->i2c_dpll)
> +		i2c_unregister_device(state->i2c_dpll);
> +	if (state->i2c_rep)
> +		i2c_unregister_device(state->i2c_rep);
> +	if (state->i2c_edid)
> +		i2c_unregister_device(state->i2c_edid);
> +	if (state->i2c_hdmi)
> +		i2c_unregister_device(state->i2c_hdmi);
> +	if (state->i2c_cp)
> +		i2c_unregister_device(state->i2c_cp);
> +}
> +
> +static struct i2c_client *adv761x_dummy_client(struct v4l2_subdev *sd,
> +					       u8 addr, u8 def_addr, u8 io_reg)
> +{
> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> +	if (!addr)
> +		addr = def_addr;
> +
> +	io_write(sd, io_reg, addr << 1);
> +	return i2c_new_dummy(client->adapter, addr);
> +}
> +
> +static inline int adv761x_check_rev(struct i2c_client *client)
> +{
> +	int msb, rev;
> +
> +	msb = adv_smbus_read_byte_data(client, 0xea);
> +	if (msb < 0)
> +		return msb;
> +
> +	rev = adv_smbus_read_byte_data(client, 0xeb);
> +	if (rev < 0)
> +		return rev;
> +
> +	rev |= msb << 8;
> +
> +	switch (rev) {
> +	case 0x2051:
> +		return 7611;
> +	case 0x2041:
> +		return 7612;
> +	default:
> +		break;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int adv761x_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct adv761x_platform_data *pdata;
> +	struct adv761x_state *state;
> +	struct v4l2_ctrl_handler *ctrl_hdl;
> +	struct v4l2_subdev *sd;
> +	int irq, ret;
> +
> +	/* Check if the adapter supports the needed features */
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -EIO;
> +
> +	/* Check chip revision */
> +	ret = adv761x_check_rev(client);
> +	if (ret < 0)
> +		return ret;
> +
> +	v4l_info(client, "Chip found @ 0x%02x (adv%d)\n", client->addr, ret);
> +
> +	/* Get platform data */
> +	if (id->driver_data == ADV761X_SOC_CAM_QUIRK) {
> +		struct soc_camera_subdev_desc *ssdd;
> +
> +		v4l_info(client, "Using SoC camera glue\n");
> +		ssdd = soc_camera_i2c_to_desc(client);
> +		pdata = ssdd ? ssdd->drv_priv : NULL;
> +	} else {
> +		pdata = client->dev.platform_data;
> +	}
> +
> +	if (!pdata) {
> +		v4l_err(client, "No platform data found\n");
> +		return -ENODEV;
> +	}
> +
> +	state = devm_kzalloc(&client->dev, sizeof(*state), GFP_KERNEL);
> +	if (state == NULL) {
> +		v4l_err(client, "Memory allocation failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	init_rwsem(&state->rwsem);
> +
> +	/* Setup default values */
> +	state->cfmt = &adv761x_cfmts[0];
> +	state->width = ADV761X_MAX_WIDTH;
> +	state->height = ADV761X_MAX_HEIGHT;
> +	state->scanmode = V4L2_FIELD_NONE;
> +	state->status = V4L2_IN_ST_NO_SIGNAL;
> +	state->gpio = -1;
> +
> +	/* Setup subdev */
> +	sd = &state->sd;
> +	v4l2_i2c_subdev_init(sd, client, &adv761x_ops);
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +	/* Setup I2C clients */
> +	state->i2c_cec = adv761x_dummy_client(sd, pdata->i2c_cec, 0x40, 0xf4);
> +	state->i2c_inf = adv761x_dummy_client(sd, pdata->i2c_inf, 0x3e, 0xf5);
> +	state->i2c_dpll = adv761x_dummy_client(sd, pdata->i2c_dpll, 0x26, 0xf8);
> +	state->i2c_rep = adv761x_dummy_client(sd, pdata->i2c_rep, 0x32, 0xf9);
> +	state->i2c_edid = adv761x_dummy_client(sd, pdata->i2c_edid, 0x36, 0xfa);
> +	state->i2c_hdmi = adv761x_dummy_client(sd, pdata->i2c_hdmi, 0x34, 0xfb);
> +	state->i2c_cp = adv761x_dummy_client(sd, pdata->i2c_cp, 0x22, 0xfd);
> +	if (!state->i2c_cec || !state->i2c_inf || !state->i2c_dpll ||
> +	    !state->i2c_rep || !state->i2c_edid ||
> +	    !state->i2c_hdmi || !state->i2c_cp) {
> +		ret = -ENODEV;
> +		v4l2_err(sd, "I2C clients setup failed\n");
> +		goto err_i2c;
> +	}
> +
> +	/* Setup control handlers */
> +	ctrl_hdl = &state->ctrl_hdl;
> +	v4l2_ctrl_handler_init(ctrl_hdl, 4);
> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
> +			  V4L2_CID_BRIGHTNESS, -128, 127, 1, 0);
> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
> +			  V4L2_CID_CONTRAST, 0, 255, 1, 128);
> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
> +			  V4L2_CID_SATURATION, 0, 255, 1, 128);
> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
> +			  V4L2_CID_HUE, 0, 255, 1, 0);
> +	sd->ctrl_handler = ctrl_hdl;
> +	if (ctrl_hdl->error) {
> +		ret = ctrl_hdl->error;
> +		v4l2_err(sd, "Control handlers setup failed\n");
> +		goto err_hdl;
> +	}
> +
> +	/* Setup media entity */
> +	state->pad.flags = MEDIA_PAD_FL_SOURCE;
> +	ret = media_entity_init(&sd->entity, 1, &state->pad, 0);
> +	if (ret) {
> +		v4l2_err(sd, "Media entity setup failed\n");
> +		goto err_hdl;
> +	}
> +
> +	/* Setup work queue */
> +	state->work_queue = create_singlethread_workqueue(client->name);
> +	if (!state->work_queue) {
> +		ret = -ENOMEM;
> +		v4l2_err(sd, "Work queue setup failed\n");
> +		goto err_entity;
> +	}
> +
> +	INIT_DELAYED_WORK(&state->enable_hotplug, adv761x_enable_hotplug);
> +	INIT_WORK(&state->interrupt_service, adv761x_interrupt_service);
> +
> +	/* Setup IRQ */
> +	irq = client->irq;
> +	if (irq <= 0) {
> +		v4l_info(client, "Using GPIO IRQ\n");
> +		ret = gpio_request_one(pdata->gpio, GPIOF_IN,
> +				       ADV761X_DRIVER_NAME);
> +		if (ret) {
> +			v4l_err(client, "GPIO setup failed\n");
> +			goto err_work;
> +		}
> +
> +		state->gpio = pdata->gpio;
> +		irq = gpio_to_irq(pdata->gpio);
> +	}
> +
> +	if (irq <= 0) {
> +		ret = -ENODEV;
> +		v4l_err(client, "IRQ not found\n");
> +		goto err_gpio;
> +	}
> +
> +	ret = request_irq(irq, adv761x_irq_handler, IRQF_TRIGGER_RISING,
> +			  ADV761X_DRIVER_NAME, state);
> +	if (ret) {
> +		v4l_err(client, "IRQ setup failed\n");
> +		goto err_gpio;
> +	}
> +
> +	state->irq = irq;
> +
> +	/* Setup core registers */
> +	ret = adv761x_core_init(sd);
> +	if (ret < 0) {
> +		v4l_err(client, "Core setup failed\n");
> +		goto err_core;
> +	}
> +
> +	return 0;
> +
> +err_core:
> +	adv761x_power_off(sd);
> +	free_irq(state->irq, state);
> +err_gpio:
> +	if (gpio_is_valid(state->gpio))
> +		gpio_free(state->gpio);
> +err_work:
> +	cancel_work_sync(&state->interrupt_service);
> +	cancel_delayed_work_sync(&state->enable_hotplug);
> +	destroy_workqueue(state->work_queue);
> +err_entity:
> +	media_entity_cleanup(&sd->entity);
> +err_hdl:
> +	v4l2_ctrl_handler_free(ctrl_hdl);
> +err_i2c:
> +	adv761x_unregister_clients(state);
> +	return ret;
> +}
> +
> +static int adv761x_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct adv761x_state *state = to_state(sd);
> +
> +	/* Release IRQ/GPIO */
> +	free_irq(state->irq, state);
> +	if (gpio_is_valid(state->gpio))
> +		gpio_free(state->gpio);
> +
> +	/* Destroy workqueue */
> +	cancel_work_sync(&state->interrupt_service);
> +	cancel_delayed_work_sync(&state->enable_hotplug);
> +	destroy_workqueue(state->work_queue);
> +
> +	/* Power off */
> +	adv761x_power_off(sd);
> +
> +	/* Clean up*/
> +	v4l2_device_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +	adv761x_unregister_clients(state);
> +	return 0;
> +}
> +
> +static const struct i2c_device_id adv761x_id[] = {
> +	{ "adv761x", 0 },
> +	{ "adv761x-soc_cam", ADV761X_SOC_CAM_QUIRK },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, adv761x_id);
> +
> +static struct i2c_driver adv761x_driver = {
> +	.driver = {
> +		.owner	= THIS_MODULE,
> +		.name	= ADV761X_DRIVER_NAME,
> +	},
> +	.probe		= adv761x_probe,
> +	.remove		= adv761x_remove,
> +	.id_table	= adv761x_id,
> +};
> +
> +module_i2c_driver(adv761x_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("ADV761X HDMI receiver video decoder driver");
> +MODULE_AUTHOR("Valentine Barshak <valentine.barshak@cogentembedded.com>");
> diff --git a/include/media/adv761x.h b/include/media/adv761x.h
> new file mode 100644
> index 0000000..ec54361
> --- /dev/null
> +++ b/include/media/adv761x.h
> @@ -0,0 +1,38 @@
> +/*
> + * adv761x Analog Devices ADV761X HDMI receiver driver
> + *
> + * Copyright (C) 2013 Cogent Embedded, Inc.
> + * Copyright (C) 2013 Renesas Electronics Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _ADV761X_H_
> +#define _ADV761X_H_
> +
> +struct adv761x_platform_data {
> +	/* INT1 GPIO IRQ */
> +	int gpio;
> +
> +	/* I2C addresses: 0 == use default */
> +	u8 i2c_cec;
> +	u8 i2c_inf;
> +	u8 i2c_dpll;
> +	u8 i2c_rep;
> +	u8 i2c_edid;
> +	u8 i2c_hdmi;
> +	u8 i2c_cp;
> +};
> +
> +/* Notify events */
> +#define ADV761X_HOTPLUG		1
> +#define ADV761X_FMT_CHANGE	2
> +
> +#endif	/* _ADV761X_H_ */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/19/2013 01:50 PM, Hans Verkuil wrote:
> Hi Valentine,

Hi Hans,
thanks for your review.

>
> I don't entirely understand how you use this driver with soc-camera.
> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
> act on it. Did you hack soc-camera to do this?

I did not. The format is queried before reading the frame by the user-space.
I'm not sure if there's some kind of generic interface to notify the camera
layer about format change events. Different subdevices use different FMT_CHANGE
defines for that. I've implemented the format change notifier based on the adv7604
in hope that it may be useful later.

>
> The way it stands I would prefer to see a version of the driver without
> soc-camera support. I wouldn't have a problem merging that as this driver
> is a good base for further development.

I've tried to implement the driver base good enough to work with both SoC
and non-SoC cameras since I don't think having 2 separate drivers for
different camera models is a good idea.

The problem is that I'm using it with R-Car VIN SoC camera driver and don't
have any other h/w. Having a platform data quirk for SoC camera in
the subdevice driver seemed simple and clean enough.

Hacking SoC camera to make it support both generic and SoC cam subdevices
doesn't seem that straightforward to me.

Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
involves a lot of regression testing with other R-Car boards that use different
subdevices with VIN.

What would you suggest?

>
> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
> control. It's easy to implement and that way apps can be notified when
> the hotplug changes value.

OK, thanks.

>
> Regards,
>
> 	Hans

Thanks,
Val.

>
> On 11/15/13 13:54, Valentine Barshak wrote:
>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>> support. Only one HDMI port is supported on ADV7612.
>>
>> The code is based on the ADV7604 driver, and ADV7612 patch by
>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>
>> Changes in version 2:
>> * Used platform data for I2C addresses setup. The driver
>>    should work with both SoC and non-SoC camera models.
>> * Dropped unnecessary code and unsupported callbacks.
>> * Implemented IRQ handling for format change detection.
>>
>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>> ---
>>   drivers/media/i2c/Kconfig   |   11 +
>>   drivers/media/i2c/Makefile  |    1 +
>>   drivers/media/i2c/adv761x.c | 1009 +++++++++++++++++++++++++++++++++++++++++++
>>   include/media/adv761x.h     |   38 ++
>>   4 files changed, 1059 insertions(+)
>>   create mode 100644 drivers/media/i2c/adv761x.c
>>   create mode 100644 include/media/adv761x.h
>>
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 75c8a03..2388642 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -206,6 +206,17 @@ config VIDEO_ADV7604
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called adv7604.
>>
>> +config VIDEO_ADV761X
>> +	tristate "Analog Devices ADV761X decoder"
>> +	depends on VIDEO_V4L2 && I2C
>> +	---help---
>> +	  Support for the Analog Devices ADV7611/ADV7612 video decoder.
>> +
>> +	  This is an Analog Devices Xpressview HDMI Receiver.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called adv761x.
>> +
>>   config VIDEO_ADV7842
>>   	tristate "Analog Devices ADV7842 decoder"
>>   	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index e03f177..d78d627 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_VIDEO_ADV7183) += adv7183.o
>>   obj-$(CONFIG_VIDEO_ADV7343) += adv7343.o
>>   obj-$(CONFIG_VIDEO_ADV7393) += adv7393.o
>>   obj-$(CONFIG_VIDEO_ADV7604) += adv7604.o
>> +obj-$(CONFIG_VIDEO_ADV761X) += adv761x.o
>>   obj-$(CONFIG_VIDEO_ADV7842) += adv7842.o
>>   obj-$(CONFIG_VIDEO_AD9389B) += ad9389b.o
>>   obj-$(CONFIG_VIDEO_ADV7511) += adv7511.o
>> diff --git a/drivers/media/i2c/adv761x.c b/drivers/media/i2c/adv761x.c
>> new file mode 100644
>> index 0000000..95939f5
>> --- /dev/null
>> +++ b/drivers/media/i2c/adv761x.c
>> @@ -0,0 +1,1009 @@
>> +/*
>> + * adv761x Analog Devices ADV761X HDMI receiver driver
>> + *
>> + * Copyright (C) 2013 Cogent Embedded, Inc.
>> + * Copyright (C) 2013 Renesas Electronics Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/gpio.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/rwsem.h>
>> +#include <linux/slab.h>
>> +#include <linux/videodev2.h>
>> +#include <media/adv761x.h>
>> +#include <media/soc_camera.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-ioctl.h>
>> +
>> +#define ADV761X_DRIVER_NAME "adv761x"
>> +
>> +/* VERT_FILTER_LOCKED and DE_REGEN_FILTER_LOCKED flags */
>> +#define ADV761X_HDMI_F_LOCKED(v)	(((v) & 0xa0) == 0xa0)
>> +
>> +/* Maximum supported resolution */
>> +#define ADV761X_MAX_WIDTH		1920
>> +#define ADV761X_MAX_HEIGHT		1080
>> +
>> +/* Use SoC camera subdev desc private data for platform_data */
>> +#define ADV761X_SOC_CAM_QUIRK		0x1
>> +
>> +static int debug;
>> +module_param(debug, int, 0644);
>> +MODULE_PARM_DESC(debug, "debug level (0-2)");
>> +
>> +struct adv761x_color_format {
>> +	enum v4l2_mbus_pixelcode code;
>> +	enum v4l2_colorspace colorspace;
>> +};
>> +
>> +/* Supported color format list */
>> +static const struct adv761x_color_format adv761x_cfmts[] = {
>> +	{
>> +		.code		= V4L2_MBUS_FMT_RGB888_1X24,
>> +		.colorspace	= V4L2_COLORSPACE_SRGB,
>> +	},
>> +};
>> +
>> +/* ADV761X descriptor structure */
>> +struct adv761x_state {
>> +	struct v4l2_subdev			sd;
>> +	struct media_pad			pad;
>> +	struct v4l2_ctrl_handler		ctrl_hdl;
>> +
>> +	u8					edid[256];
>> +	unsigned				edid_blocks;
>> +
>> +	struct rw_semaphore			rwsem;
>> +	const struct adv761x_color_format	*cfmt;
>> +	u32					width;
>> +	u32					height;
>> +	enum v4l2_field				scanmode;
>> +	u32					status;
>> +
>> +	int					gpio;
>> +	int					irq;
>> +
>> +	struct workqueue_struct			*work_queue;
>> +	struct delayed_work			enable_hotplug;
>> +	struct work_struct			interrupt_service;
>> +
>> +	struct i2c_client			*i2c_cec;
>> +	struct i2c_client			*i2c_inf;
>> +	struct i2c_client			*i2c_dpll;
>> +	struct i2c_client			*i2c_rep;
>> +	struct i2c_client			*i2c_edid;
>> +	struct i2c_client			*i2c_hdmi;
>> +	struct i2c_client			*i2c_cp;
>> +};
>> +
>> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
>> +{
>> +	return &container_of(ctrl->handler, struct adv761x_state, ctrl_hdl)->sd;
>> +}
>> +
>> +static inline struct adv761x_state *to_state(struct v4l2_subdev *sd)
>> +{
>> +	return container_of(sd, struct adv761x_state, sd);
>> +}
>> +
>> +/* I2C I/O operations */
>> +static s32 adv_smbus_read_byte_data(struct i2c_client *client, u8 command)
>> +{
>> +	s32 ret, i;
>> +
>> +	for (i = 0; i < 3; i++) {
>> +		ret = i2c_smbus_read_byte_data(client, command);
>> +		if (ret >= 0)
>> +			return ret;
>> +	}
>> +
>> +	v4l_err(client, "Reading addr:%02x reg:%02x\n failed",
>> +		client->addr, command);
>> +	return ret;
>> +}
>> +
>> +static s32 adv_smbus_write_byte_data(struct i2c_client *client, u8 command,
>> +				     u8 value)
>> +{
>> +	s32 ret, i;
>> +
>> +	for (i = 0; i < 3; i++) {
>> +		ret = i2c_smbus_write_byte_data(client, command, value);
>> +		if (!ret)
>> +			return 0;
>> +	}
>> +
>> +	v4l_err(client, "Writing addr:%02x reg:%02x val:%02x failed\n",
>> +		client->addr, command, value);
>> +	return ret;
>> +}
>> +
>> +static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client, u8 command,
>> +					  u8 length, const u8 *values)
>> +{
>> +	s32 ret, i;
>> +
>> +	ret = i2c_smbus_write_i2c_block_data(client, command, length, values);
>> +	if (!ret)
>> +		return 0;
>> +
>> +	for (i = 0; i < length; i++) {
>> +		ret = adv_smbus_write_byte_data(client, command + i, values[i]);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static inline int io_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +	return adv_smbus_read_byte_data(client, reg);
>> +}
>> +
>> +static inline int io_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +	return adv_smbus_write_byte_data(client, reg, val);
>> +}
>> +
>> +static inline int cec_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_cec, reg);
>> +}
>> +
>> +static inline int cec_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_cec, reg, val);
>> +}
>> +
>> +static inline int infoframe_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_inf, reg);
>> +}
>> +
>> +static inline int infoframe_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_inf, reg, val);
>> +}
>> +
>> +static inline int dpll_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_dpll, reg);
>> +}
>> +
>> +static inline int dpll_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_dpll, reg, val);
>> +}
>> +
>> +static inline int rep_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_rep, reg);
>> +}
>> +
>> +static inline int rep_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_rep, reg, val);
>> +}
>> +
>> +static inline int edid_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_edid, reg);
>> +}
>> +
>> +static inline int edid_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_edid, reg, val);
>> +}
>> +
>> +static inline int edid_write_block(struct v4l2_subdev *sd,
>> +				   unsigned len, const u8 *val)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +	struct adv761x_state *state = to_state(sd);
>> +	int ret = 0;
>> +	int i;
>> +
>> +	v4l2_dbg(2, debug, sd, "Writing EDID block (%d bytes)\n", len);
>> +
>> +	v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
>> +
>> +	/* Disable I2C access to internal EDID ram from DDC port */
>> +	rep_write(sd, 0x74, 0x0);
>> +
>> +	for (i = 0; !ret && i < len; i += I2C_SMBUS_BLOCK_MAX)
>> +		ret = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
>> +				I2C_SMBUS_BLOCK_MAX, val + i);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * ADV761x calculates the checksums and enables I2C access
>> +	 * to internal EDID ram from DDC port.
>> +	 */
>> +	rep_write(sd, 0x74, 0x01);
>> +
>> +	for (i = 0; i < 1000; i++) {
>> +		if (rep_read(sd, 0x76) & 0x1) {
>> +			/* Enable hotplug after 100 ms */
>> +			queue_delayed_work(state->work_queue,
>> +					   &state->enable_hotplug, HZ / 10);
>> +			return 0;
>> +		}
>> +		schedule();
>> +	}
>> +
>> +	v4l_err(client, "Enabling EDID failed\n");
>> +	return -EIO;
>> +}
>> +
>> +static inline int hdmi_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_hdmi, reg);
>> +}
>> +
>> +static inline int hdmi_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_hdmi, reg, val);
>> +}
>> +
>> +static inline int cp_read(struct v4l2_subdev *sd, u8 reg)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_read_byte_data(state->i2c_cp, reg);
>> +}
>> +
>> +static inline int cp_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	return adv_smbus_write_byte_data(state->i2c_cp, reg, val);
>> +}
>> +
>> +static inline int adv761x_power_off(struct v4l2_subdev *sd)
>> +{
>> +	return io_write(sd, 0x0c, 0x62);
>> +}
>> +
>> +static int adv761x_core_init(struct v4l2_subdev *sd)
>> +{
>> +	io_write(sd, 0x01, 0x06);	/* V-FREQ = 60Hz */
>> +					/* Prim_Mode = HDMI-GR */
>> +	io_write(sd, 0x02, 0xf2);	/* Auto CSC, RGB out */
>> +					/* Disable op_656 bit */
>> +	io_write(sd, 0x03, 0x42);	/* 36 bit SDR 444 Mode 0 */
>> +	io_write(sd, 0x05, 0x28);	/* AV Codes Off */
>> +	io_write(sd, 0x0b, 0x44);	/* Power up part */
>> +	io_write(sd, 0x0c, 0x42);	/* Power up part */
>> +	io_write(sd, 0x14, 0x7f);	/* Max Drive Strength */
>> +	io_write(sd, 0x15, 0x80);	/* Disable Tristate of Pins */
>> +					/* (Audio output pins active) */
>> +	io_write(sd, 0x19, 0x83);	/* LLC DLL phase */
>> +	io_write(sd, 0x33, 0x40);	/* LLC DLL enable */
>> +
>> +	cp_write(sd, 0xba, 0x01);	/* Set HDMI FreeRun */
>> +	cp_write(sd, 0x3e, 0x80);	/* Enable color adjustments */
>> +
>> +	hdmi_write(sd, 0x9b, 0x03);	/* ADI recommended setting */
>> +	hdmi_write(sd, 0x00, 0x08);	/* Set HDMI Input Port A */
>> +					/* (BG_MEAS_PORT_SEL = 001b) */
>> +	hdmi_write(sd, 0x02, 0x03);	/* Enable Ports A & B in */
>> +					/* background mode */
>> +	hdmi_write(sd, 0x6d, 0x80);	/* Enable TDM mode */
>> +	hdmi_write(sd, 0x03, 0x18);	/* I2C mode 24 bits */
>> +	hdmi_write(sd, 0x83, 0xfc);	/* Enable clock terminators */
>> +					/* for port A & B */
>> +	hdmi_write(sd, 0x6f, 0x0c);	/* ADI recommended setting */
>> +	hdmi_write(sd, 0x85, 0x1f);	/* ADI recommended setting */
>> +	hdmi_write(sd, 0x87, 0x70);	/* ADI recommended setting */
>> +	hdmi_write(sd, 0x8d, 0x04);	/* LFG Port A */
>> +	hdmi_write(sd, 0x8e, 0x1e);	/* HFG Port A */
>> +	hdmi_write(sd, 0x1a, 0x8a);	/* unmute audio */
>> +	hdmi_write(sd, 0x57, 0xda);	/* ADI recommended setting */
>> +	hdmi_write(sd, 0x58, 0x01);	/* ADI recommended setting */
>> +	hdmi_write(sd, 0x75, 0x10);	/* DDC drive strength */
>> +	hdmi_write(sd, 0x90, 0x04);	/* LFG Port B */
>> +	hdmi_write(sd, 0x91, 0x1e);	/* HFG Port B */
>> +	hdmi_write(sd, 0x04, 0x03);
>> +	hdmi_write(sd, 0x14, 0x00);
>> +	hdmi_write(sd, 0x15, 0x00);
>> +	hdmi_write(sd, 0x16, 0x00);
>> +
>> +	rep_write(sd, 0x40, 0x81);	/* Disable HDCP 1.1 features */
>> +	rep_write(sd, 0x74, 0x00);	/* Disable the Internal EDID */
>> +					/* for all ports */
>> +
>> +	/* Setup interrupts */
>> +	io_write(sd, 0x40, 0xc2);	/* Active high until cleared */
>> +	io_write(sd, 0x6e, 0x03);	/* INT1 HDMI DE_REGEN and V_LOCK */
>> +
>> +	return v4l2_ctrl_handler_setup(sd->ctrl_handler);
>> +}
>> +
>> +static int adv761x_hdmi_info(struct v4l2_subdev *sd, enum v4l2_field *scanmode,
>> +			     u32 *width, u32 *height)
>> +{
>> +	int msb, val;
>> +
>> +	/* Line width */
>> +	msb = hdmi_read(sd, 0x07);
>> +	if (msb < 0)
>> +		return msb;
>> +
>> +	if (!ADV761X_HDMI_F_LOCKED(msb))
>> +		return -EAGAIN;
>> +
>> +	/* Interlaced or progressive */
>> +	val = hdmi_read(sd, 0x0b);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	*scanmode = (val & 0x20) ? V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
>> +	val = hdmi_read(sd, 0x08);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	val |= (msb & 0x1f) << 8;
>> +	*width = val;
>> +
>> +	/* Lines per frame */
>> +	msb = hdmi_read(sd, 0x09);
>> +	if (msb < 0)
>> +		return msb;
>> +
>> +	val = hdmi_read(sd, 0x0a);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	val |= (msb & 0x1f) << 8;
>> +	if (*scanmode == V4L2_FIELD_INTERLACED)
>> +		val <<= 1;
>> +	*height = val;
>> +
>> +	if (*width == 0 || *height == 0)
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Hotplug work */
>> +static void adv761x_enable_hotplug(struct work_struct *work)
>> +{
>> +	struct delayed_work *dwork = to_delayed_work(work);
>> +	struct adv761x_state *state = container_of(dwork, struct adv761x_state,
>> +						   enable_hotplug);
>> +	struct v4l2_subdev *sd = &state->sd;
>> +
>> +	v4l2_dbg(2, debug, sd, "Enable hotplug\n");
>> +	v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)1);
>> +}
>> +
>> +/* IRQ work */
>> +static void adv761x_interrupt_service(struct work_struct *work)
>> +{
>> +	struct adv761x_state *state = container_of(work, struct adv761x_state,
>> +						   interrupt_service);
>> +	struct v4l2_subdev *sd = &state->sd;
>> +	enum v4l2_field scanmode;
>> +	u32 width, height;
>> +	u32 status = 0;
>> +	int ret;
>> +
>> +	/* Clear HDMI interrupts */
>> +	io_write(sd, 0x6c, 0xff);
>> +
>> +	ret = adv761x_hdmi_info(sd, &scanmode, &width, &height);
>> +	if (ret) {
>> +		if (state->status == V4L2_IN_ST_NO_SIGNAL)
>> +			return;
>> +
>> +		width = ADV761X_MAX_WIDTH;
>> +		height = ADV761X_MAX_HEIGHT;
>> +		scanmode = V4L2_FIELD_NONE;
>> +		status = V4L2_IN_ST_NO_SIGNAL;
>> +	}
>> +
>> +	if (status)
>> +		v4l2_dbg(2, debug, sd, "No HDMI video input detected\n");
>> +	else
>> +		v4l2_dbg(2, debug, sd, "HDMI video input detected (%ux%u%c)\n",
>> +			 width, height,
>> +			 scanmode == V4L2_FIELD_NONE ? 'p' : 'i');
>> +
>> +	down_write(&state->rwsem);
>> +	state->width = width;
>> +	state->height = height;
>> +	state->scanmode = scanmode;
>> +	state->status = status;
>> +	up_write(&state->rwsem);
>> +
>> +	v4l2_subdev_notify(sd, ADV761X_FMT_CHANGE, NULL);
>> +}
>> +
>> +/* IRQ handler */
>> +static irqreturn_t adv761x_irq_handler(int irq, void *devid)
>> +{
>> +	struct adv761x_state *state = devid;
>> +
>> +	queue_work(state->work_queue, &state->interrupt_service);
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +/* v4l2_subdev_core_ops */
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +static void adv761x_inv_register(struct v4l2_subdev *sd)
>> +{
>> +	v4l2_info(sd, "0x000-0x0ff: IO Map\n");
>> +	v4l2_info(sd, "0x100-0x1ff: CEC Map\n");
>> +	v4l2_info(sd, "0x200-0x2ff: InfoFrame Map\n");
>> +	v4l2_info(sd, "0x300-0x3ff: DPLL Map\n");
>> +	v4l2_info(sd, "0x400-0x4ff: Repeater Map\n");
>> +	v4l2_info(sd, "0x500-0x5ff: EDID Map\n");
>> +	v4l2_info(sd, "0x600-0x6ff: HDMI Map\n");
>> +	v4l2_info(sd, "0x700-0x7ff: CP Map\n");
>> +}
>> +
>> +static int adv761x_g_register(struct v4l2_subdev *sd,
>> +			      struct v4l2_dbg_register *reg)
>> +{
>> +	reg->size = 1;
>> +	switch (reg->reg >> 8) {
>> +	case 0:
>> +		reg->val = io_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 1:
>> +		reg->val = cec_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 2:
>> +		reg->val = infoframe_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 3:
>> +		reg->val = dpll_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 4:
>> +		reg->val = rep_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 5:
>> +		reg->val = edid_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 6:
>> +		reg->val = hdmi_read(sd, reg->reg & 0xff);
>> +		break;
>> +	case 7:
>> +		reg->val = cp_read(sd, reg->reg & 0xff);
>> +		break;
>> +	default:
>> +		v4l2_info(sd, "Register %03llx not supported\n", reg->reg);
>> +		adv761x_inv_register(sd);
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int adv761x_s_register(struct v4l2_subdev *sd,
>> +			      const struct v4l2_dbg_register *reg)
>> +{
>> +	switch (reg->reg >> 8) {
>> +	case 0:
>> +		io_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 1:
>> +		cec_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 2:
>> +		infoframe_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 3:
>> +		dpll_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 4:
>> +		rep_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 5:
>> +		edid_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 6:
>> +		hdmi_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	case 7:
>> +		cp_write(sd, reg->reg & 0xff, reg->val & 0xff);
>> +		break;
>> +	default:
>> +		v4l2_info(sd, "Register %03llx not supported\n", reg->reg);
>> +		adv761x_inv_register(sd);
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +#endif	/* CONFIG_VIDEO_ADV_DEBUG */
>> +
>> +/* v4l2_subdev_video_ops */
>> +static int adv761x_g_input_status(struct v4l2_subdev *sd, u32 *status)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	down_read(&state->rwsem);
>> +	*status = state->status;
>> +	up_read(&state->rwsem);
>> +	return 0;
>> +}
>> +
>> +static int adv761x_g_mbus_fmt(struct v4l2_subdev *sd,
>> +			      struct v4l2_mbus_framefmt *mf)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	down_read(&state->rwsem);
>> +	mf->width = state->width;
>> +	mf->height = state->height;
>> +	mf->field = state->scanmode;
>> +	mf->code = state->cfmt->code;
>> +	mf->colorspace = state->cfmt->colorspace;
>> +	up_read(&state->rwsem);
>> +	return 0;
>> +}
>> +
>> +static int adv761x_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
>> +				 enum v4l2_mbus_pixelcode *code)
>> +{
>> +	/* Check requested format index is within range */
>> +	if (index >= ARRAY_SIZE(adv761x_cfmts))
>> +		return -EINVAL;
>> +
>> +	*code = adv761x_cfmts[index].code;
>> +
>> +	return 0;
>> +}
>> +
>> +static int adv761x_g_mbus_config(struct v4l2_subdev *sd,
>> +				 struct v4l2_mbus_config *cfg)
>> +{
>> +	cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>> +		V4L2_MBUS_VSYNC_ACTIVE_LOW | V4L2_MBUS_HSYNC_ACTIVE_LOW |
>> +		V4L2_MBUS_DATA_ACTIVE_HIGH;
>> +	cfg->type = V4L2_MBUS_PARALLEL;
>> +
>> +	return 0;
>> +}
>> +
>> +/* v4l2_subdev_pad_ops */
>> +static int adv761x_get_edid(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_edid *edid)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	if (edid->pad != 0)
>> +		return -EINVAL;
>> +
>> +	if (edid->blocks == 0)
>> +		return -EINVAL;
>> +
>> +	if (edid->start_block >= state->edid_blocks)
>> +		return -EINVAL;
>> +
>> +	if (edid->start_block + edid->blocks > state->edid_blocks)
>> +		edid->blocks = state->edid_blocks - edid->start_block;
>> +	if (!edid->edid)
>> +		return -EINVAL;
>> +
>> +	memcpy(edid->edid + edid->start_block * 128,
>> +	       state->edid + edid->start_block * 128,
>> +	       edid->blocks * 128);
>> +	return 0;
>> +}
>> +
>> +static int adv761x_set_edid(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_edid *edid)
>> +{
>> +	struct adv761x_state *state = to_state(sd);
>> +	int ret;
>> +
>> +	if (edid->pad != 0)
>> +		return -EINVAL;
>> +
>> +	if (edid->start_block != 0)
>> +		return -EINVAL;
>> +
>> +	if (edid->blocks == 0) {
>> +		/* Pull down the hotplug pin */
>> +		v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
>> +		/* Disable I2C access to internal EDID RAM from DDC port */
>> +		rep_write(sd, 0x74, 0x0);
>> +		state->edid_blocks = 0;
>> +		return 0;
>> +	}
>> +
>> +	if (edid->blocks > 2)
>> +		return -E2BIG;
>> +
>> +	if (!edid->edid)
>> +		return -EINVAL;
>> +
>> +	memcpy(state->edid, edid->edid, 128 * edid->blocks);
>> +	state->edid_blocks = edid->blocks;
>> +
>> +	ret = edid_write_block(sd, 128 * edid->blocks, state->edid);
>> +	if (ret < 0)
>> +		v4l2_err(sd, "Writing EDID failed\n");
>> +
>> +	return ret;
>> +}
>> +
>> +/* v4l2_ctrl_ops */
>> +static int adv761x_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct v4l2_subdev *sd = to_sd(ctrl);
>> +	u8 val = ctrl->val;
>> +	int ret;
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_BRIGHTNESS:
>> +		ret = cp_write(sd, 0x3c, val);
>> +		break;
>> +	case V4L2_CID_CONTRAST:
>> +		ret = cp_write(sd, 0x3a, val);
>> +		break;
>> +	case V4L2_CID_SATURATION:
>> +		ret = cp_write(sd, 0x3b, val);
>> +		break;
>> +	case V4L2_CID_HUE:
>> +		ret = cp_write(sd, 0x3d, val);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/* V4L structures */
>> +static const struct v4l2_subdev_core_ops adv761x_core_ops = {
>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>> +	.g_register	= adv761x_g_register,
>> +	.s_register	= adv761x_s_register,
>> +#endif
>> +};
>> +
>> +static const struct v4l2_subdev_video_ops adv761x_video_ops = {
>> +	.g_input_status = adv761x_g_input_status,
>> +	.g_mbus_fmt	= adv761x_g_mbus_fmt,
>> +	.try_mbus_fmt	= adv761x_g_mbus_fmt,
>> +	.s_mbus_fmt	= adv761x_g_mbus_fmt,
>> +	.enum_mbus_fmt	= adv761x_enum_mbus_fmt,
>> +	.g_mbus_config	= adv761x_g_mbus_config,
>> +};
>> +
>> +static const struct v4l2_subdev_pad_ops adv761x_pad_ops = {
>> +	.get_edid = adv761x_get_edid,
>> +	.set_edid = adv761x_set_edid,
>> +};
>> +
>> +static const struct v4l2_subdev_ops adv761x_ops = {
>> +	.core = &adv761x_core_ops,
>> +	.video = &adv761x_video_ops,
>> +	.pad = &adv761x_pad_ops,
>> +};
>> +
>> +static const struct v4l2_ctrl_ops adv761x_ctrl_ops = {
>> +	.s_ctrl = adv761x_s_ctrl,
>> +};
>> +
>> +/* Device initialization and clean-up */
>> +static void adv761x_unregister_clients(struct adv761x_state *state)
>> +{
>> +	if (state->i2c_cec)
>> +		i2c_unregister_device(state->i2c_cec);
>> +	if (state->i2c_inf)
>> +		i2c_unregister_device(state->i2c_inf);
>> +	if (state->i2c_dpll)
>> +		i2c_unregister_device(state->i2c_dpll);
>> +	if (state->i2c_rep)
>> +		i2c_unregister_device(state->i2c_rep);
>> +	if (state->i2c_edid)
>> +		i2c_unregister_device(state->i2c_edid);
>> +	if (state->i2c_hdmi)
>> +		i2c_unregister_device(state->i2c_hdmi);
>> +	if (state->i2c_cp)
>> +		i2c_unregister_device(state->i2c_cp);
>> +}
>> +
>> +static struct i2c_client *adv761x_dummy_client(struct v4l2_subdev *sd,
>> +					       u8 addr, u8 def_addr, u8 io_reg)
>> +{
>> +	struct i2c_client *client = v4l2_get_subdevdata(sd);
>> +
>> +	if (!addr)
>> +		addr = def_addr;
>> +
>> +	io_write(sd, io_reg, addr << 1);
>> +	return i2c_new_dummy(client->adapter, addr);
>> +}
>> +
>> +static inline int adv761x_check_rev(struct i2c_client *client)
>> +{
>> +	int msb, rev;
>> +
>> +	msb = adv_smbus_read_byte_data(client, 0xea);
>> +	if (msb < 0)
>> +		return msb;
>> +
>> +	rev = adv_smbus_read_byte_data(client, 0xeb);
>> +	if (rev < 0)
>> +		return rev;
>> +
>> +	rev |= msb << 8;
>> +
>> +	switch (rev) {
>> +	case 0x2051:
>> +		return 7611;
>> +	case 0x2041:
>> +		return 7612;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -ENODEV;
>> +}
>> +
>> +static int adv761x_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *id)
>> +{
>> +	struct adv761x_platform_data *pdata;
>> +	struct adv761x_state *state;
>> +	struct v4l2_ctrl_handler *ctrl_hdl;
>> +	struct v4l2_subdev *sd;
>> +	int irq, ret;
>> +
>> +	/* Check if the adapter supports the needed features */
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>> +		return -EIO;
>> +
>> +	/* Check chip revision */
>> +	ret = adv761x_check_rev(client);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	v4l_info(client, "Chip found @ 0x%02x (adv%d)\n", client->addr, ret);
>> +
>> +	/* Get platform data */
>> +	if (id->driver_data == ADV761X_SOC_CAM_QUIRK) {
>> +		struct soc_camera_subdev_desc *ssdd;
>> +
>> +		v4l_info(client, "Using SoC camera glue\n");
>> +		ssdd = soc_camera_i2c_to_desc(client);
>> +		pdata = ssdd ? ssdd->drv_priv : NULL;
>> +	} else {
>> +		pdata = client->dev.platform_data;
>> +	}
>> +
>> +	if (!pdata) {
>> +		v4l_err(client, "No platform data found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	state = devm_kzalloc(&client->dev, sizeof(*state), GFP_KERNEL);
>> +	if (state == NULL) {
>> +		v4l_err(client, "Memory allocation failed\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	init_rwsem(&state->rwsem);
>> +
>> +	/* Setup default values */
>> +	state->cfmt = &adv761x_cfmts[0];
>> +	state->width = ADV761X_MAX_WIDTH;
>> +	state->height = ADV761X_MAX_HEIGHT;
>> +	state->scanmode = V4L2_FIELD_NONE;
>> +	state->status = V4L2_IN_ST_NO_SIGNAL;
>> +	state->gpio = -1;
>> +
>> +	/* Setup subdev */
>> +	sd = &state->sd;
>> +	v4l2_i2c_subdev_init(sd, client, &adv761x_ops);
>> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +
>> +	/* Setup I2C clients */
>> +	state->i2c_cec = adv761x_dummy_client(sd, pdata->i2c_cec, 0x40, 0xf4);
>> +	state->i2c_inf = adv761x_dummy_client(sd, pdata->i2c_inf, 0x3e, 0xf5);
>> +	state->i2c_dpll = adv761x_dummy_client(sd, pdata->i2c_dpll, 0x26, 0xf8);
>> +	state->i2c_rep = adv761x_dummy_client(sd, pdata->i2c_rep, 0x32, 0xf9);
>> +	state->i2c_edid = adv761x_dummy_client(sd, pdata->i2c_edid, 0x36, 0xfa);
>> +	state->i2c_hdmi = adv761x_dummy_client(sd, pdata->i2c_hdmi, 0x34, 0xfb);
>> +	state->i2c_cp = adv761x_dummy_client(sd, pdata->i2c_cp, 0x22, 0xfd);
>> +	if (!state->i2c_cec || !state->i2c_inf || !state->i2c_dpll ||
>> +	    !state->i2c_rep || !state->i2c_edid ||
>> +	    !state->i2c_hdmi || !state->i2c_cp) {
>> +		ret = -ENODEV;
>> +		v4l2_err(sd, "I2C clients setup failed\n");
>> +		goto err_i2c;
>> +	}
>> +
>> +	/* Setup control handlers */
>> +	ctrl_hdl = &state->ctrl_hdl;
>> +	v4l2_ctrl_handler_init(ctrl_hdl, 4);
>> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>> +			  V4L2_CID_BRIGHTNESS, -128, 127, 1, 0);
>> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>> +			  V4L2_CID_CONTRAST, 0, 255, 1, 128);
>> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>> +			  V4L2_CID_SATURATION, 0, 255, 1, 128);
>> +	v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>> +			  V4L2_CID_HUE, 0, 255, 1, 0);
>> +	sd->ctrl_handler = ctrl_hdl;
>> +	if (ctrl_hdl->error) {
>> +		ret = ctrl_hdl->error;
>> +		v4l2_err(sd, "Control handlers setup failed\n");
>> +		goto err_hdl;
>> +	}
>> +
>> +	/* Setup media entity */
>> +	state->pad.flags = MEDIA_PAD_FL_SOURCE;
>> +	ret = media_entity_init(&sd->entity, 1, &state->pad, 0);
>> +	if (ret) {
>> +		v4l2_err(sd, "Media entity setup failed\n");
>> +		goto err_hdl;
>> +	}
>> +
>> +	/* Setup work queue */
>> +	state->work_queue = create_singlethread_workqueue(client->name);
>> +	if (!state->work_queue) {
>> +		ret = -ENOMEM;
>> +		v4l2_err(sd, "Work queue setup failed\n");
>> +		goto err_entity;
>> +	}
>> +
>> +	INIT_DELAYED_WORK(&state->enable_hotplug, adv761x_enable_hotplug);
>> +	INIT_WORK(&state->interrupt_service, adv761x_interrupt_service);
>> +
>> +	/* Setup IRQ */
>> +	irq = client->irq;
>> +	if (irq <= 0) {
>> +		v4l_info(client, "Using GPIO IRQ\n");
>> +		ret = gpio_request_one(pdata->gpio, GPIOF_IN,
>> +				       ADV761X_DRIVER_NAME);
>> +		if (ret) {
>> +			v4l_err(client, "GPIO setup failed\n");
>> +			goto err_work;
>> +		}
>> +
>> +		state->gpio = pdata->gpio;
>> +		irq = gpio_to_irq(pdata->gpio);
>> +	}
>> +
>> +	if (irq <= 0) {
>> +		ret = -ENODEV;
>> +		v4l_err(client, "IRQ not found\n");
>> +		goto err_gpio;
>> +	}
>> +
>> +	ret = request_irq(irq, adv761x_irq_handler, IRQF_TRIGGER_RISING,
>> +			  ADV761X_DRIVER_NAME, state);
>> +	if (ret) {
>> +		v4l_err(client, "IRQ setup failed\n");
>> +		goto err_gpio;
>> +	}
>> +
>> +	state->irq = irq;
>> +
>> +	/* Setup core registers */
>> +	ret = adv761x_core_init(sd);
>> +	if (ret < 0) {
>> +		v4l_err(client, "Core setup failed\n");
>> +		goto err_core;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_core:
>> +	adv761x_power_off(sd);
>> +	free_irq(state->irq, state);
>> +err_gpio:
>> +	if (gpio_is_valid(state->gpio))
>> +		gpio_free(state->gpio);
>> +err_work:
>> +	cancel_work_sync(&state->interrupt_service);
>> +	cancel_delayed_work_sync(&state->enable_hotplug);
>> +	destroy_workqueue(state->work_queue);
>> +err_entity:
>> +	media_entity_cleanup(&sd->entity);
>> +err_hdl:
>> +	v4l2_ctrl_handler_free(ctrl_hdl);
>> +err_i2c:
>> +	adv761x_unregister_clients(state);
>> +	return ret;
>> +}
>> +
>> +static int adv761x_remove(struct i2c_client *client)
>> +{
>> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
>> +	struct adv761x_state *state = to_state(sd);
>> +
>> +	/* Release IRQ/GPIO */
>> +	free_irq(state->irq, state);
>> +	if (gpio_is_valid(state->gpio))
>> +		gpio_free(state->gpio);
>> +
>> +	/* Destroy workqueue */
>> +	cancel_work_sync(&state->interrupt_service);
>> +	cancel_delayed_work_sync(&state->enable_hotplug);
>> +	destroy_workqueue(state->work_queue);
>> +
>> +	/* Power off */
>> +	adv761x_power_off(sd);
>> +
>> +	/* Clean up*/
>> +	v4l2_device_unregister_subdev(sd);
>> +	media_entity_cleanup(&sd->entity);
>> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
>> +	adv761x_unregister_clients(state);
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id adv761x_id[] = {
>> +	{ "adv761x", 0 },
>> +	{ "adv761x-soc_cam", ADV761X_SOC_CAM_QUIRK },
>> +	{ },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, adv761x_id);
>> +
>> +static struct i2c_driver adv761x_driver = {
>> +	.driver = {
>> +		.owner	= THIS_MODULE,
>> +		.name	= ADV761X_DRIVER_NAME,
>> +	},
>> +	.probe		= adv761x_probe,
>> +	.remove		= adv761x_remove,
>> +	.id_table	= adv761x_id,
>> +};
>> +
>> +module_i2c_driver(adv761x_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("ADV761X HDMI receiver video decoder driver");
>> +MODULE_AUTHOR("Valentine Barshak <valentine.barshak@cogentembedded.com>");
>> diff --git a/include/media/adv761x.h b/include/media/adv761x.h
>> new file mode 100644
>> index 0000000..ec54361
>> --- /dev/null
>> +++ b/include/media/adv761x.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * adv761x Analog Devices ADV761X HDMI receiver driver
>> + *
>> + * Copyright (C) 2013 Cogent Embedded, Inc.
>> + * Copyright (C) 2013 Renesas Electronics Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef _ADV761X_H_
>> +#define _ADV761X_H_
>> +
>> +struct adv761x_platform_data {
>> +	/* INT1 GPIO IRQ */
>> +	int gpio;
>> +
>> +	/* I2C addresses: 0 == use default */
>> +	u8 i2c_cec;
>> +	u8 i2c_inf;
>> +	u8 i2c_dpll;
>> +	u8 i2c_rep;
>> +	u8 i2c_edid;
>> +	u8 i2c_hdmi;
>> +	u8 i2c_cp;
>> +};
>> +
>> +/* Notify events */
>> +#define ADV761X_HOTPLUG		1
>> +#define ADV761X_FMT_CHANGE	2
>> +
>> +#endif	/* _ADV761X_H_ */
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Valentine,

On 11/20/13 11:14, Valentine wrote:
> On 11/19/2013 01:50 PM, Hans Verkuil wrote:
>> Hi Valentine,
> 
> Hi Hans,
> thanks for your review.
> 
>>
>> I don't entirely understand how you use this driver with soc-camera.
>> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
>> act on it. Did you hack soc-camera to do this?
> 
> I did not. The format is queried before reading the frame by the user-space.
> I'm not sure if there's some kind of generic interface to notify the camera
> layer about format change events. Different subdevices use different FMT_CHANGE
> defines for that. I've implemented the format change notifier based on the adv7604
> in hope that it may be useful later.

Yes, I need to generalize the FMT_CHANGE event.

But what happens if you are streaming and the HDMI connector is unplugged?
Or plugged back in again, possibly with a larger resolution? I'm not sure
if the soc_camera driver supports such scenarios.

> 
>>
>> The way it stands I would prefer to see a version of the driver without
>> soc-camera support. I wouldn't have a problem merging that as this driver
>> is a good base for further development.
> 
> I've tried to implement the driver base good enough to work with both SoC
> and non-SoC cameras since I don't think having 2 separate drivers for
> different camera models is a good idea.
> 
> The problem is that I'm using it with R-Car VIN SoC camera driver and don't
> have any other h/w. Having a platform data quirk for SoC camera in
> the subdevice driver seemed simple and clean enough.

I hate it, but it isn't something you can do anything about. So it will have
to do for now.

> Hacking SoC camera to make it support both generic and SoC cam subdevices
> doesn't seem that straightforward to me.

Guennadi, what is the status of this? I'm getting really tired of soc-camera
infecting sub-devices. Subdev drivers should be independent of any bridge
driver using them, but soc-camera keeps breaking that. It's driving me nuts.

I'll be honest, it's getting to the point that I want to just NACK any
future subdev drivers that depend on soc-camera, just to force a solution.
There is no technical reason for this dependency, it just takes some time
to fix soc-camera.

> Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
> involves a lot of regression testing with other R-Car boards that use different
> subdevices with VIN.
> 
> What would you suggest?

Let's leave it as-is for now :-(

I'm not happy, but as I said, it's not your fault.

Regards,

	Hans

> 
>>
>> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
>> control. It's easy to implement and that way apps can be notified when
>> the hotplug changes value.
> 
> OK, thanks.
> 
>>
>> Regards,
>>
>>     Hans
> 
> Thanks,
> Val.
> 
>>
>> On 11/15/13 13:54, Valentine Barshak wrote:
>>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>>> support. Only one HDMI port is supported on ADV7612.
>>>
>>> The code is based on the ADV7604 driver, and ADV7612 patch by
>>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>>
>>> Changes in version 2:
>>> * Used platform data for I2C addresses setup. The driver
>>>    should work with both SoC and non-SoC camera models.
>>> * Dropped unnecessary code and unsupported callbacks.
>>> * Implemented IRQ handling for format change detection.
>>>
>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>>> ---
>>>   drivers/media/i2c/Kconfig   |   11 +
>>>   drivers/media/i2c/Makefile  |    1 +
>>>   drivers/media/i2c/adv761x.c | 1009 +++++++++++++++++++++++++++++++++++++++++++
>>>   include/media/adv761x.h     |   38 ++
>>>   4 files changed, 1059 insertions(+)
>>>   create mode 100644 drivers/media/i2c/adv761x.c
>>>   create mode 100644 include/media/adv761x.h
>>>
>>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>>> index 75c8a03..2388642 100644
>>> --- a/drivers/media/i2c/Kconfig
>>> +++ b/drivers/media/i2c/Kconfig
>>> @@ -206,6 +206,17 @@ config VIDEO_ADV7604
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called adv7604.
>>>
>>> +config VIDEO_ADV761X
>>> +    tristate "Analog Devices ADV761X decoder"
>>> +    depends on VIDEO_V4L2 && I2C
>>> +    ---help---
>>> +      Support for the Analog Devices ADV7611/ADV7612 video decoder.
>>> +
>>> +      This is an Analog Devices Xpressview HDMI Receiver.
>>> +
>>> +      To compile this driver as a module, choose M here: the
>>> +      module will be called adv761x.
>>> +
>>>   config VIDEO_ADV7842
>>>       tristate "Analog Devices ADV7842 decoder"
>>>       depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API
>>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>>> index e03f177..d78d627 100644
>>> --- a/drivers/media/i2c/Makefile
>>> +++ b/drivers/media/i2c/Makefile
>>> @@ -26,6 +26,7 @@ obj-$(CONFIG_VIDEO_ADV7183) += adv7183.o
>>>   obj-$(CONFIG_VIDEO_ADV7343) += adv7343.o
>>>   obj-$(CONFIG_VIDEO_ADV7393) += adv7393.o
>>>   obj-$(CONFIG_VIDEO_ADV7604) += adv7604.o
>>> +obj-$(CONFIG_VIDEO_ADV761X) += adv761x.o
>>>   obj-$(CONFIG_VIDEO_ADV7842) += adv7842.o
>>>   obj-$(CONFIG_VIDEO_AD9389B) += ad9389b.o
>>>   obj-$(CONFIG_VIDEO_ADV7511) += adv7511.o
>>> diff --git a/drivers/media/i2c/adv761x.c b/drivers/media/i2c/adv761x.c
>>> new file mode 100644
>>> index 0000000..95939f5
>>> --- /dev/null
>>> +++ b/drivers/media/i2c/adv761x.c
>>> @@ -0,0 +1,1009 @@
>>> +/*
>>> + * adv761x Analog Devices ADV761X HDMI receiver driver
>>> + *
>>> + * Copyright (C) 2013 Cogent Embedded, Inc.
>>> + * Copyright (C) 2013 Renesas Electronics Corporation
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/errno.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/rwsem.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/videodev2.h>
>>> +#include <media/adv761x.h>
>>> +#include <media/soc_camera.h>
>>> +#include <media/v4l2-ctrls.h>
>>> +#include <media/v4l2-device.h>
>>> +#include <media/v4l2-ioctl.h>
>>> +
>>> +#define ADV761X_DRIVER_NAME "adv761x"
>>> +
>>> +/* VERT_FILTER_LOCKED and DE_REGEN_FILTER_LOCKED flags */
>>> +#define ADV761X_HDMI_F_LOCKED(v)    (((v) & 0xa0) == 0xa0)
>>> +
>>> +/* Maximum supported resolution */
>>> +#define ADV761X_MAX_WIDTH        1920
>>> +#define ADV761X_MAX_HEIGHT        1080
>>> +
>>> +/* Use SoC camera subdev desc private data for platform_data */
>>> +#define ADV761X_SOC_CAM_QUIRK        0x1
>>> +
>>> +static int debug;
>>> +module_param(debug, int, 0644);
>>> +MODULE_PARM_DESC(debug, "debug level (0-2)");
>>> +
>>> +struct adv761x_color_format {
>>> +    enum v4l2_mbus_pixelcode code;
>>> +    enum v4l2_colorspace colorspace;
>>> +};
>>> +
>>> +/* Supported color format list */
>>> +static const struct adv761x_color_format adv761x_cfmts[] = {
>>> +    {
>>> +        .code        = V4L2_MBUS_FMT_RGB888_1X24,
>>> +        .colorspace    = V4L2_COLORSPACE_SRGB,
>>> +    },
>>> +};
>>> +
>>> +/* ADV761X descriptor structure */
>>> +struct adv761x_state {
>>> +    struct v4l2_subdev            sd;
>>> +    struct media_pad            pad;
>>> +    struct v4l2_ctrl_handler        ctrl_hdl;
>>> +
>>> +    u8                    edid[256];
>>> +    unsigned                edid_blocks;
>>> +
>>> +    struct rw_semaphore            rwsem;
>>> +    const struct adv761x_color_format    *cfmt;
>>> +    u32                    width;
>>> +    u32                    height;
>>> +    enum v4l2_field                scanmode;
>>> +    u32                    status;
>>> +
>>> +    int                    gpio;
>>> +    int                    irq;
>>> +
>>> +    struct workqueue_struct            *work_queue;
>>> +    struct delayed_work            enable_hotplug;
>>> +    struct work_struct            interrupt_service;
>>> +
>>> +    struct i2c_client            *i2c_cec;
>>> +    struct i2c_client            *i2c_inf;
>>> +    struct i2c_client            *i2c_dpll;
>>> +    struct i2c_client            *i2c_rep;
>>> +    struct i2c_client            *i2c_edid;
>>> +    struct i2c_client            *i2c_hdmi;
>>> +    struct i2c_client            *i2c_cp;
>>> +};
>>> +
>>> +static inline struct v4l2_subdev *to_sd(struct v4l2_ctrl *ctrl)
>>> +{
>>> +    return &container_of(ctrl->handler, struct adv761x_state, ctrl_hdl)->sd;
>>> +}
>>> +
>>> +static inline struct adv761x_state *to_state(struct v4l2_subdev *sd)
>>> +{
>>> +    return container_of(sd, struct adv761x_state, sd);
>>> +}
>>> +
>>> +/* I2C I/O operations */
>>> +static s32 adv_smbus_read_byte_data(struct i2c_client *client, u8 command)
>>> +{
>>> +    s32 ret, i;
>>> +
>>> +    for (i = 0; i < 3; i++) {
>>> +        ret = i2c_smbus_read_byte_data(client, command);
>>> +        if (ret >= 0)
>>> +            return ret;
>>> +    }
>>> +
>>> +    v4l_err(client, "Reading addr:%02x reg:%02x\n failed",
>>> +        client->addr, command);
>>> +    return ret;
>>> +}
>>> +
>>> +static s32 adv_smbus_write_byte_data(struct i2c_client *client, u8 command,
>>> +                     u8 value)
>>> +{
>>> +    s32 ret, i;
>>> +
>>> +    for (i = 0; i < 3; i++) {
>>> +        ret = i2c_smbus_write_byte_data(client, command, value);
>>> +        if (!ret)
>>> +            return 0;
>>> +    }
>>> +
>>> +    v4l_err(client, "Writing addr:%02x reg:%02x val:%02x failed\n",
>>> +        client->addr, command, value);
>>> +    return ret;
>>> +}
>>> +
>>> +static s32 adv_smbus_write_i2c_block_data(struct i2c_client *client, u8 command,
>>> +                      u8 length, const u8 *values)
>>> +{
>>> +    s32 ret, i;
>>> +
>>> +    ret = i2c_smbus_write_i2c_block_data(client, command, length, values);
>>> +    if (!ret)
>>> +        return 0;
>>> +
>>> +    for (i = 0; i < length; i++) {
>>> +        ret = adv_smbus_write_byte_data(client, command + i, values[i]);
>>> +        if (ret)
>>> +            break;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static inline int io_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(client, reg);
>>> +}
>>> +
>>> +static inline int io_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(client, reg, val);
>>> +}
>>> +
>>> +static inline int cec_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_cec, reg);
>>> +}
>>> +
>>> +static inline int cec_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_cec, reg, val);
>>> +}
>>> +
>>> +static inline int infoframe_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_inf, reg);
>>> +}
>>> +
>>> +static inline int infoframe_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_inf, reg, val);
>>> +}
>>> +
>>> +static inline int dpll_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_dpll, reg);
>>> +}
>>> +
>>> +static inline int dpll_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_dpll, reg, val);
>>> +}
>>> +
>>> +static inline int rep_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_rep, reg);
>>> +}
>>> +
>>> +static inline int rep_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_rep, reg, val);
>>> +}
>>> +
>>> +static inline int edid_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_edid, reg);
>>> +}
>>> +
>>> +static inline int edid_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_edid, reg, val);
>>> +}
>>> +
>>> +static inline int edid_write_block(struct v4l2_subdev *sd,
>>> +                   unsigned len, const u8 *val)
>>> +{
>>> +    struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +    struct adv761x_state *state = to_state(sd);
>>> +    int ret = 0;
>>> +    int i;
>>> +
>>> +    v4l2_dbg(2, debug, sd, "Writing EDID block (%d bytes)\n", len);
>>> +
>>> +    v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
>>> +
>>> +    /* Disable I2C access to internal EDID ram from DDC port */
>>> +    rep_write(sd, 0x74, 0x0);
>>> +
>>> +    for (i = 0; !ret && i < len; i += I2C_SMBUS_BLOCK_MAX)
>>> +        ret = adv_smbus_write_i2c_block_data(state->i2c_edid, i,
>>> +                I2C_SMBUS_BLOCK_MAX, val + i);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    /*
>>> +     * ADV761x calculates the checksums and enables I2C access
>>> +     * to internal EDID ram from DDC port.
>>> +     */
>>> +    rep_write(sd, 0x74, 0x01);
>>> +
>>> +    for (i = 0; i < 1000; i++) {
>>> +        if (rep_read(sd, 0x76) & 0x1) {
>>> +            /* Enable hotplug after 100 ms */
>>> +            queue_delayed_work(state->work_queue,
>>> +                       &state->enable_hotplug, HZ / 10);
>>> +            return 0;
>>> +        }
>>> +        schedule();
>>> +    }
>>> +
>>> +    v4l_err(client, "Enabling EDID failed\n");
>>> +    return -EIO;
>>> +}
>>> +
>>> +static inline int hdmi_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_hdmi, reg);
>>> +}
>>> +
>>> +static inline int hdmi_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_hdmi, reg, val);
>>> +}
>>> +
>>> +static inline int cp_read(struct v4l2_subdev *sd, u8 reg)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_read_byte_data(state->i2c_cp, reg);
>>> +}
>>> +
>>> +static inline int cp_write(struct v4l2_subdev *sd, u8 reg, u8 val)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    return adv_smbus_write_byte_data(state->i2c_cp, reg, val);
>>> +}
>>> +
>>> +static inline int adv761x_power_off(struct v4l2_subdev *sd)
>>> +{
>>> +    return io_write(sd, 0x0c, 0x62);
>>> +}
>>> +
>>> +static int adv761x_core_init(struct v4l2_subdev *sd)
>>> +{
>>> +    io_write(sd, 0x01, 0x06);    /* V-FREQ = 60Hz */
>>> +                    /* Prim_Mode = HDMI-GR */
>>> +    io_write(sd, 0x02, 0xf2);    /* Auto CSC, RGB out */
>>> +                    /* Disable op_656 bit */
>>> +    io_write(sd, 0x03, 0x42);    /* 36 bit SDR 444 Mode 0 */
>>> +    io_write(sd, 0x05, 0x28);    /* AV Codes Off */
>>> +    io_write(sd, 0x0b, 0x44);    /* Power up part */
>>> +    io_write(sd, 0x0c, 0x42);    /* Power up part */
>>> +    io_write(sd, 0x14, 0x7f);    /* Max Drive Strength */
>>> +    io_write(sd, 0x15, 0x80);    /* Disable Tristate of Pins */
>>> +                    /* (Audio output pins active) */
>>> +    io_write(sd, 0x19, 0x83);    /* LLC DLL phase */
>>> +    io_write(sd, 0x33, 0x40);    /* LLC DLL enable */
>>> +
>>> +    cp_write(sd, 0xba, 0x01);    /* Set HDMI FreeRun */
>>> +    cp_write(sd, 0x3e, 0x80);    /* Enable color adjustments */
>>> +
>>> +    hdmi_write(sd, 0x9b, 0x03);    /* ADI recommended setting */
>>> +    hdmi_write(sd, 0x00, 0x08);    /* Set HDMI Input Port A */
>>> +                    /* (BG_MEAS_PORT_SEL = 001b) */
>>> +    hdmi_write(sd, 0x02, 0x03);    /* Enable Ports A & B in */
>>> +                    /* background mode */
>>> +    hdmi_write(sd, 0x6d, 0x80);    /* Enable TDM mode */
>>> +    hdmi_write(sd, 0x03, 0x18);    /* I2C mode 24 bits */
>>> +    hdmi_write(sd, 0x83, 0xfc);    /* Enable clock terminators */
>>> +                    /* for port A & B */
>>> +    hdmi_write(sd, 0x6f, 0x0c);    /* ADI recommended setting */
>>> +    hdmi_write(sd, 0x85, 0x1f);    /* ADI recommended setting */
>>> +    hdmi_write(sd, 0x87, 0x70);    /* ADI recommended setting */
>>> +    hdmi_write(sd, 0x8d, 0x04);    /* LFG Port A */
>>> +    hdmi_write(sd, 0x8e, 0x1e);    /* HFG Port A */
>>> +    hdmi_write(sd, 0x1a, 0x8a);    /* unmute audio */
>>> +    hdmi_write(sd, 0x57, 0xda);    /* ADI recommended setting */
>>> +    hdmi_write(sd, 0x58, 0x01);    /* ADI recommended setting */
>>> +    hdmi_write(sd, 0x75, 0x10);    /* DDC drive strength */
>>> +    hdmi_write(sd, 0x90, 0x04);    /* LFG Port B */
>>> +    hdmi_write(sd, 0x91, 0x1e);    /* HFG Port B */
>>> +    hdmi_write(sd, 0x04, 0x03);
>>> +    hdmi_write(sd, 0x14, 0x00);
>>> +    hdmi_write(sd, 0x15, 0x00);
>>> +    hdmi_write(sd, 0x16, 0x00);
>>> +
>>> +    rep_write(sd, 0x40, 0x81);    /* Disable HDCP 1.1 features */
>>> +    rep_write(sd, 0x74, 0x00);    /* Disable the Internal EDID */
>>> +                    /* for all ports */
>>> +
>>> +    /* Setup interrupts */
>>> +    io_write(sd, 0x40, 0xc2);    /* Active high until cleared */
>>> +    io_write(sd, 0x6e, 0x03);    /* INT1 HDMI DE_REGEN and V_LOCK */
>>> +
>>> +    return v4l2_ctrl_handler_setup(sd->ctrl_handler);
>>> +}
>>> +
>>> +static int adv761x_hdmi_info(struct v4l2_subdev *sd, enum v4l2_field *scanmode,
>>> +                 u32 *width, u32 *height)
>>> +{
>>> +    int msb, val;
>>> +
>>> +    /* Line width */
>>> +    msb = hdmi_read(sd, 0x07);
>>> +    if (msb < 0)
>>> +        return msb;
>>> +
>>> +    if (!ADV761X_HDMI_F_LOCKED(msb))
>>> +        return -EAGAIN;
>>> +
>>> +    /* Interlaced or progressive */
>>> +    val = hdmi_read(sd, 0x0b);
>>> +    if (val < 0)
>>> +        return val;
>>> +
>>> +    *scanmode = (val & 0x20) ? V4L2_FIELD_INTERLACED : V4L2_FIELD_NONE;
>>> +    val = hdmi_read(sd, 0x08);
>>> +    if (val < 0)
>>> +        return val;
>>> +
>>> +    val |= (msb & 0x1f) << 8;
>>> +    *width = val;
>>> +
>>> +    /* Lines per frame */
>>> +    msb = hdmi_read(sd, 0x09);
>>> +    if (msb < 0)
>>> +        return msb;
>>> +
>>> +    val = hdmi_read(sd, 0x0a);
>>> +    if (val < 0)
>>> +        return val;
>>> +
>>> +    val |= (msb & 0x1f) << 8;
>>> +    if (*scanmode == V4L2_FIELD_INTERLACED)
>>> +        val <<= 1;
>>> +    *height = val;
>>> +
>>> +    if (*width == 0 || *height == 0)
>>> +        return -EIO;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Hotplug work */
>>> +static void adv761x_enable_hotplug(struct work_struct *work)
>>> +{
>>> +    struct delayed_work *dwork = to_delayed_work(work);
>>> +    struct adv761x_state *state = container_of(dwork, struct adv761x_state,
>>> +                           enable_hotplug);
>>> +    struct v4l2_subdev *sd = &state->sd;
>>> +
>>> +    v4l2_dbg(2, debug, sd, "Enable hotplug\n");
>>> +    v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)1);
>>> +}
>>> +
>>> +/* IRQ work */
>>> +static void adv761x_interrupt_service(struct work_struct *work)
>>> +{
>>> +    struct adv761x_state *state = container_of(work, struct adv761x_state,
>>> +                           interrupt_service);
>>> +    struct v4l2_subdev *sd = &state->sd;
>>> +    enum v4l2_field scanmode;
>>> +    u32 width, height;
>>> +    u32 status = 0;
>>> +    int ret;
>>> +
>>> +    /* Clear HDMI interrupts */
>>> +    io_write(sd, 0x6c, 0xff);
>>> +
>>> +    ret = adv761x_hdmi_info(sd, &scanmode, &width, &height);
>>> +    if (ret) {
>>> +        if (state->status == V4L2_IN_ST_NO_SIGNAL)
>>> +            return;
>>> +
>>> +        width = ADV761X_MAX_WIDTH;
>>> +        height = ADV761X_MAX_HEIGHT;
>>> +        scanmode = V4L2_FIELD_NONE;
>>> +        status = V4L2_IN_ST_NO_SIGNAL;
>>> +    }
>>> +
>>> +    if (status)
>>> +        v4l2_dbg(2, debug, sd, "No HDMI video input detected\n");
>>> +    else
>>> +        v4l2_dbg(2, debug, sd, "HDMI video input detected (%ux%u%c)\n",
>>> +             width, height,
>>> +             scanmode == V4L2_FIELD_NONE ? 'p' : 'i');
>>> +
>>> +    down_write(&state->rwsem);
>>> +    state->width = width;
>>> +    state->height = height;
>>> +    state->scanmode = scanmode;
>>> +    state->status = status;
>>> +    up_write(&state->rwsem);
>>> +
>>> +    v4l2_subdev_notify(sd, ADV761X_FMT_CHANGE, NULL);
>>> +}
>>> +
>>> +/* IRQ handler */
>>> +static irqreturn_t adv761x_irq_handler(int irq, void *devid)
>>> +{
>>> +    struct adv761x_state *state = devid;
>>> +
>>> +    queue_work(state->work_queue, &state->interrupt_service);
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* v4l2_subdev_core_ops */
>>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>>> +static void adv761x_inv_register(struct v4l2_subdev *sd)
>>> +{
>>> +    v4l2_info(sd, "0x000-0x0ff: IO Map\n");
>>> +    v4l2_info(sd, "0x100-0x1ff: CEC Map\n");
>>> +    v4l2_info(sd, "0x200-0x2ff: InfoFrame Map\n");
>>> +    v4l2_info(sd, "0x300-0x3ff: DPLL Map\n");
>>> +    v4l2_info(sd, "0x400-0x4ff: Repeater Map\n");
>>> +    v4l2_info(sd, "0x500-0x5ff: EDID Map\n");
>>> +    v4l2_info(sd, "0x600-0x6ff: HDMI Map\n");
>>> +    v4l2_info(sd, "0x700-0x7ff: CP Map\n");
>>> +}
>>> +
>>> +static int adv761x_g_register(struct v4l2_subdev *sd,
>>> +                  struct v4l2_dbg_register *reg)
>>> +{
>>> +    reg->size = 1;
>>> +    switch (reg->reg >> 8) {
>>> +    case 0:
>>> +        reg->val = io_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 1:
>>> +        reg->val = cec_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 2:
>>> +        reg->val = infoframe_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 3:
>>> +        reg->val = dpll_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 4:
>>> +        reg->val = rep_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 5:
>>> +        reg->val = edid_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 6:
>>> +        reg->val = hdmi_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    case 7:
>>> +        reg->val = cp_read(sd, reg->reg & 0xff);
>>> +        break;
>>> +    default:
>>> +        v4l2_info(sd, "Register %03llx not supported\n", reg->reg);
>>> +        adv761x_inv_register(sd);
>>> +        break;
>>> +    }
>>> +    return 0;
>>> +}
>>> +
>>> +static int adv761x_s_register(struct v4l2_subdev *sd,
>>> +                  const struct v4l2_dbg_register *reg)
>>> +{
>>> +    switch (reg->reg >> 8) {
>>> +    case 0:
>>> +        io_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 1:
>>> +        cec_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 2:
>>> +        infoframe_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 3:
>>> +        dpll_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 4:
>>> +        rep_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 5:
>>> +        edid_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 6:
>>> +        hdmi_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    case 7:
>>> +        cp_write(sd, reg->reg & 0xff, reg->val & 0xff);
>>> +        break;
>>> +    default:
>>> +        v4l2_info(sd, "Register %03llx not supported\n", reg->reg);
>>> +        adv761x_inv_register(sd);
>>> +        break;
>>> +    }
>>> +    return 0;
>>> +}
>>> +#endif    /* CONFIG_VIDEO_ADV_DEBUG */
>>> +
>>> +/* v4l2_subdev_video_ops */
>>> +static int adv761x_g_input_status(struct v4l2_subdev *sd, u32 *status)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    down_read(&state->rwsem);
>>> +    *status = state->status;
>>> +    up_read(&state->rwsem);
>>> +    return 0;
>>> +}
>>> +
>>> +static int adv761x_g_mbus_fmt(struct v4l2_subdev *sd,
>>> +                  struct v4l2_mbus_framefmt *mf)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    down_read(&state->rwsem);
>>> +    mf->width = state->width;
>>> +    mf->height = state->height;
>>> +    mf->field = state->scanmode;
>>> +    mf->code = state->cfmt->code;
>>> +    mf->colorspace = state->cfmt->colorspace;
>>> +    up_read(&state->rwsem);
>>> +    return 0;
>>> +}
>>> +
>>> +static int adv761x_enum_mbus_fmt(struct v4l2_subdev *sd, unsigned int index,
>>> +                 enum v4l2_mbus_pixelcode *code)
>>> +{
>>> +    /* Check requested format index is within range */
>>> +    if (index >= ARRAY_SIZE(adv761x_cfmts))
>>> +        return -EINVAL;
>>> +
>>> +    *code = adv761x_cfmts[index].code;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int adv761x_g_mbus_config(struct v4l2_subdev *sd,
>>> +                 struct v4l2_mbus_config *cfg)
>>> +{
>>> +    cfg->flags = V4L2_MBUS_PCLK_SAMPLE_RISING | V4L2_MBUS_MASTER |
>>> +        V4L2_MBUS_VSYNC_ACTIVE_LOW | V4L2_MBUS_HSYNC_ACTIVE_LOW |
>>> +        V4L2_MBUS_DATA_ACTIVE_HIGH;
>>> +    cfg->type = V4L2_MBUS_PARALLEL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* v4l2_subdev_pad_ops */
>>> +static int adv761x_get_edid(struct v4l2_subdev *sd,
>>> +                struct v4l2_subdev_edid *edid)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    if (edid->pad != 0)
>>> +        return -EINVAL;
>>> +
>>> +    if (edid->blocks == 0)
>>> +        return -EINVAL;
>>> +
>>> +    if (edid->start_block >= state->edid_blocks)
>>> +        return -EINVAL;
>>> +
>>> +    if (edid->start_block + edid->blocks > state->edid_blocks)
>>> +        edid->blocks = state->edid_blocks - edid->start_block;
>>> +    if (!edid->edid)
>>> +        return -EINVAL;
>>> +
>>> +    memcpy(edid->edid + edid->start_block * 128,
>>> +           state->edid + edid->start_block * 128,
>>> +           edid->blocks * 128);
>>> +    return 0;
>>> +}
>>> +
>>> +static int adv761x_set_edid(struct v4l2_subdev *sd,
>>> +                struct v4l2_subdev_edid *edid)
>>> +{
>>> +    struct adv761x_state *state = to_state(sd);
>>> +    int ret;
>>> +
>>> +    if (edid->pad != 0)
>>> +        return -EINVAL;
>>> +
>>> +    if (edid->start_block != 0)
>>> +        return -EINVAL;
>>> +
>>> +    if (edid->blocks == 0) {
>>> +        /* Pull down the hotplug pin */
>>> +        v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
>>> +        /* Disable I2C access to internal EDID RAM from DDC port */
>>> +        rep_write(sd, 0x74, 0x0);
>>> +        state->edid_blocks = 0;
>>> +        return 0;
>>> +    }
>>> +
>>> +    if (edid->blocks > 2)
>>> +        return -E2BIG;
>>> +
>>> +    if (!edid->edid)
>>> +        return -EINVAL;
>>> +
>>> +    memcpy(state->edid, edid->edid, 128 * edid->blocks);
>>> +    state->edid_blocks = edid->blocks;
>>> +
>>> +    ret = edid_write_block(sd, 128 * edid->blocks, state->edid);
>>> +    if (ret < 0)
>>> +        v4l2_err(sd, "Writing EDID failed\n");
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +/* v4l2_ctrl_ops */
>>> +static int adv761x_s_ctrl(struct v4l2_ctrl *ctrl)
>>> +{
>>> +    struct v4l2_subdev *sd = to_sd(ctrl);
>>> +    u8 val = ctrl->val;
>>> +    int ret;
>>> +
>>> +    switch (ctrl->id) {
>>> +    case V4L2_CID_BRIGHTNESS:
>>> +        ret = cp_write(sd, 0x3c, val);
>>> +        break;
>>> +    case V4L2_CID_CONTRAST:
>>> +        ret = cp_write(sd, 0x3a, val);
>>> +        break;
>>> +    case V4L2_CID_SATURATION:
>>> +        ret = cp_write(sd, 0x3b, val);
>>> +        break;
>>> +    case V4L2_CID_HUE:
>>> +        ret = cp_write(sd, 0x3d, val);
>>> +        break;
>>> +    default:
>>> +        ret = -EINVAL;
>>> +        break;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +/* V4L structures */
>>> +static const struct v4l2_subdev_core_ops adv761x_core_ops = {
>>> +#ifdef CONFIG_VIDEO_ADV_DEBUG
>>> +    .g_register    = adv761x_g_register,
>>> +    .s_register    = adv761x_s_register,
>>> +#endif
>>> +};
>>> +
>>> +static const struct v4l2_subdev_video_ops adv761x_video_ops = {
>>> +    .g_input_status = adv761x_g_input_status,
>>> +    .g_mbus_fmt    = adv761x_g_mbus_fmt,
>>> +    .try_mbus_fmt    = adv761x_g_mbus_fmt,
>>> +    .s_mbus_fmt    = adv761x_g_mbus_fmt,
>>> +    .enum_mbus_fmt    = adv761x_enum_mbus_fmt,
>>> +    .g_mbus_config    = adv761x_g_mbus_config,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_pad_ops adv761x_pad_ops = {
>>> +    .get_edid = adv761x_get_edid,
>>> +    .set_edid = adv761x_set_edid,
>>> +};
>>> +
>>> +static const struct v4l2_subdev_ops adv761x_ops = {
>>> +    .core = &adv761x_core_ops,
>>> +    .video = &adv761x_video_ops,
>>> +    .pad = &adv761x_pad_ops,
>>> +};
>>> +
>>> +static const struct v4l2_ctrl_ops adv761x_ctrl_ops = {
>>> +    .s_ctrl = adv761x_s_ctrl,
>>> +};
>>> +
>>> +/* Device initialization and clean-up */
>>> +static void adv761x_unregister_clients(struct adv761x_state *state)
>>> +{
>>> +    if (state->i2c_cec)
>>> +        i2c_unregister_device(state->i2c_cec);
>>> +    if (state->i2c_inf)
>>> +        i2c_unregister_device(state->i2c_inf);
>>> +    if (state->i2c_dpll)
>>> +        i2c_unregister_device(state->i2c_dpll);
>>> +    if (state->i2c_rep)
>>> +        i2c_unregister_device(state->i2c_rep);
>>> +    if (state->i2c_edid)
>>> +        i2c_unregister_device(state->i2c_edid);
>>> +    if (state->i2c_hdmi)
>>> +        i2c_unregister_device(state->i2c_hdmi);
>>> +    if (state->i2c_cp)
>>> +        i2c_unregister_device(state->i2c_cp);
>>> +}
>>> +
>>> +static struct i2c_client *adv761x_dummy_client(struct v4l2_subdev *sd,
>>> +                           u8 addr, u8 def_addr, u8 io_reg)
>>> +{
>>> +    struct i2c_client *client = v4l2_get_subdevdata(sd);
>>> +
>>> +    if (!addr)
>>> +        addr = def_addr;
>>> +
>>> +    io_write(sd, io_reg, addr << 1);
>>> +    return i2c_new_dummy(client->adapter, addr);
>>> +}
>>> +
>>> +static inline int adv761x_check_rev(struct i2c_client *client)
>>> +{
>>> +    int msb, rev;
>>> +
>>> +    msb = adv_smbus_read_byte_data(client, 0xea);
>>> +    if (msb < 0)
>>> +        return msb;
>>> +
>>> +    rev = adv_smbus_read_byte_data(client, 0xeb);
>>> +    if (rev < 0)
>>> +        return rev;
>>> +
>>> +    rev |= msb << 8;
>>> +
>>> +    switch (rev) {
>>> +    case 0x2051:
>>> +        return 7611;
>>> +    case 0x2041:
>>> +        return 7612;
>>> +    default:
>>> +        break;
>>> +    }
>>> +
>>> +    return -ENODEV;
>>> +}
>>> +
>>> +static int adv761x_probe(struct i2c_client *client,
>>> +             const struct i2c_device_id *id)
>>> +{
>>> +    struct adv761x_platform_data *pdata;
>>> +    struct adv761x_state *state;
>>> +    struct v4l2_ctrl_handler *ctrl_hdl;
>>> +    struct v4l2_subdev *sd;
>>> +    int irq, ret;
>>> +
>>> +    /* Check if the adapter supports the needed features */
>>> +    if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
>>> +        return -EIO;
>>> +
>>> +    /* Check chip revision */
>>> +    ret = adv761x_check_rev(client);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    v4l_info(client, "Chip found @ 0x%02x (adv%d)\n", client->addr, ret);
>>> +
>>> +    /* Get platform data */
>>> +    if (id->driver_data == ADV761X_SOC_CAM_QUIRK) {
>>> +        struct soc_camera_subdev_desc *ssdd;
>>> +
>>> +        v4l_info(client, "Using SoC camera glue\n");
>>> +        ssdd = soc_camera_i2c_to_desc(client);
>>> +        pdata = ssdd ? ssdd->drv_priv : NULL;
>>> +    } else {
>>> +        pdata = client->dev.platform_data;
>>> +    }
>>> +
>>> +    if (!pdata) {
>>> +        v4l_err(client, "No platform data found\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    state = devm_kzalloc(&client->dev, sizeof(*state), GFP_KERNEL);
>>> +    if (state == NULL) {
>>> +        v4l_err(client, "Memory allocation failed\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    init_rwsem(&state->rwsem);
>>> +
>>> +    /* Setup default values */
>>> +    state->cfmt = &adv761x_cfmts[0];
>>> +    state->width = ADV761X_MAX_WIDTH;
>>> +    state->height = ADV761X_MAX_HEIGHT;
>>> +    state->scanmode = V4L2_FIELD_NONE;
>>> +    state->status = V4L2_IN_ST_NO_SIGNAL;
>>> +    state->gpio = -1;
>>> +
>>> +    /* Setup subdev */
>>> +    sd = &state->sd;
>>> +    v4l2_i2c_subdev_init(sd, client, &adv761x_ops);
>>> +    sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>> +
>>> +    /* Setup I2C clients */
>>> +    state->i2c_cec = adv761x_dummy_client(sd, pdata->i2c_cec, 0x40, 0xf4);
>>> +    state->i2c_inf = adv761x_dummy_client(sd, pdata->i2c_inf, 0x3e, 0xf5);
>>> +    state->i2c_dpll = adv761x_dummy_client(sd, pdata->i2c_dpll, 0x26, 0xf8);
>>> +    state->i2c_rep = adv761x_dummy_client(sd, pdata->i2c_rep, 0x32, 0xf9);
>>> +    state->i2c_edid = adv761x_dummy_client(sd, pdata->i2c_edid, 0x36, 0xfa);
>>> +    state->i2c_hdmi = adv761x_dummy_client(sd, pdata->i2c_hdmi, 0x34, 0xfb);
>>> +    state->i2c_cp = adv761x_dummy_client(sd, pdata->i2c_cp, 0x22, 0xfd);
>>> +    if (!state->i2c_cec || !state->i2c_inf || !state->i2c_dpll ||
>>> +        !state->i2c_rep || !state->i2c_edid ||
>>> +        !state->i2c_hdmi || !state->i2c_cp) {
>>> +        ret = -ENODEV;
>>> +        v4l2_err(sd, "I2C clients setup failed\n");
>>> +        goto err_i2c;
>>> +    }
>>> +
>>> +    /* Setup control handlers */
>>> +    ctrl_hdl = &state->ctrl_hdl;
>>> +    v4l2_ctrl_handler_init(ctrl_hdl, 4);
>>> +    v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>>> +              V4L2_CID_BRIGHTNESS, -128, 127, 1, 0);
>>> +    v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>>> +              V4L2_CID_CONTRAST, 0, 255, 1, 128);
>>> +    v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>>> +              V4L2_CID_SATURATION, 0, 255, 1, 128);
>>> +    v4l2_ctrl_new_std(ctrl_hdl, &adv761x_ctrl_ops,
>>> +              V4L2_CID_HUE, 0, 255, 1, 0);
>>> +    sd->ctrl_handler = ctrl_hdl;
>>> +    if (ctrl_hdl->error) {
>>> +        ret = ctrl_hdl->error;
>>> +        v4l2_err(sd, "Control handlers setup failed\n");
>>> +        goto err_hdl;
>>> +    }
>>> +
>>> +    /* Setup media entity */
>>> +    state->pad.flags = MEDIA_PAD_FL_SOURCE;
>>> +    ret = media_entity_init(&sd->entity, 1, &state->pad, 0);
>>> +    if (ret) {
>>> +        v4l2_err(sd, "Media entity setup failed\n");
>>> +        goto err_hdl;
>>> +    }
>>> +
>>> +    /* Setup work queue */
>>> +    state->work_queue = create_singlethread_workqueue(client->name);
>>> +    if (!state->work_queue) {
>>> +        ret = -ENOMEM;
>>> +        v4l2_err(sd, "Work queue setup failed\n");
>>> +        goto err_entity;
>>> +    }
>>> +
>>> +    INIT_DELAYED_WORK(&state->enable_hotplug, adv761x_enable_hotplug);
>>> +    INIT_WORK(&state->interrupt_service, adv761x_interrupt_service);
>>> +
>>> +    /* Setup IRQ */
>>> +    irq = client->irq;
>>> +    if (irq <= 0) {
>>> +        v4l_info(client, "Using GPIO IRQ\n");
>>> +        ret = gpio_request_one(pdata->gpio, GPIOF_IN,
>>> +                       ADV761X_DRIVER_NAME);
>>> +        if (ret) {
>>> +            v4l_err(client, "GPIO setup failed\n");
>>> +            goto err_work;
>>> +        }
>>> +
>>> +        state->gpio = pdata->gpio;
>>> +        irq = gpio_to_irq(pdata->gpio);
>>> +    }
>>> +
>>> +    if (irq <= 0) {
>>> +        ret = -ENODEV;
>>> +        v4l_err(client, "IRQ not found\n");
>>> +        goto err_gpio;
>>> +    }
>>> +
>>> +    ret = request_irq(irq, adv761x_irq_handler, IRQF_TRIGGER_RISING,
>>> +              ADV761X_DRIVER_NAME, state);
>>> +    if (ret) {
>>> +        v4l_err(client, "IRQ setup failed\n");
>>> +        goto err_gpio;
>>> +    }
>>> +
>>> +    state->irq = irq;
>>> +
>>> +    /* Setup core registers */
>>> +    ret = adv761x_core_init(sd);
>>> +    if (ret < 0) {
>>> +        v4l_err(client, "Core setup failed\n");
>>> +        goto err_core;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +err_core:
>>> +    adv761x_power_off(sd);
>>> +    free_irq(state->irq, state);
>>> +err_gpio:
>>> +    if (gpio_is_valid(state->gpio))
>>> +        gpio_free(state->gpio);
>>> +err_work:
>>> +    cancel_work_sync(&state->interrupt_service);
>>> +    cancel_delayed_work_sync(&state->enable_hotplug);
>>> +    destroy_workqueue(state->work_queue);
>>> +err_entity:
>>> +    media_entity_cleanup(&sd->entity);
>>> +err_hdl:
>>> +    v4l2_ctrl_handler_free(ctrl_hdl);
>>> +err_i2c:
>>> +    adv761x_unregister_clients(state);
>>> +    return ret;
>>> +}
>>> +
>>> +static int adv761x_remove(struct i2c_client *client)
>>> +{
>>> +    struct v4l2_subdev *sd = i2c_get_clientdata(client);
>>> +    struct adv761x_state *state = to_state(sd);
>>> +
>>> +    /* Release IRQ/GPIO */
>>> +    free_irq(state->irq, state);
>>> +    if (gpio_is_valid(state->gpio))
>>> +        gpio_free(state->gpio);
>>> +
>>> +    /* Destroy workqueue */
>>> +    cancel_work_sync(&state->interrupt_service);
>>> +    cancel_delayed_work_sync(&state->enable_hotplug);
>>> +    destroy_workqueue(state->work_queue);
>>> +
>>> +    /* Power off */
>>> +    adv761x_power_off(sd);
>>> +
>>> +    /* Clean up*/
>>> +    v4l2_device_unregister_subdev(sd);
>>> +    media_entity_cleanup(&sd->entity);
>>> +    v4l2_ctrl_handler_free(sd->ctrl_handler);
>>> +    adv761x_unregister_clients(state);
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct i2c_device_id adv761x_id[] = {
>>> +    { "adv761x", 0 },
>>> +    { "adv761x-soc_cam", ADV761X_SOC_CAM_QUIRK },
>>> +    { },
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, adv761x_id);
>>> +
>>> +static struct i2c_driver adv761x_driver = {
>>> +    .driver = {
>>> +        .owner    = THIS_MODULE,
>>> +        .name    = ADV761X_DRIVER_NAME,
>>> +    },
>>> +    .probe        = adv761x_probe,
>>> +    .remove        = adv761x_remove,
>>> +    .id_table    = adv761x_id,
>>> +};
>>> +
>>> +module_i2c_driver(adv761x_driver);
>>> +
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_DESCRIPTION("ADV761X HDMI receiver video decoder driver");
>>> +MODULE_AUTHOR("Valentine Barshak <valentine.barshak@cogentembedded.com>");
>>> diff --git a/include/media/adv761x.h b/include/media/adv761x.h
>>> new file mode 100644
>>> index 0000000..ec54361
>>> --- /dev/null
>>> +++ b/include/media/adv761x.h
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * adv761x Analog Devices ADV761X HDMI receiver driver
>>> + *
>>> + * Copyright (C) 2013 Cogent Embedded, Inc.
>>> + * Copyright (C) 2013 Renesas Electronics Corporation
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#ifndef _ADV761X_H_
>>> +#define _ADV761X_H_
>>> +
>>> +struct adv761x_platform_data {
>>> +    /* INT1 GPIO IRQ */
>>> +    int gpio;
>>> +
>>> +    /* I2C addresses: 0 == use default */
>>> +    u8 i2c_cec;
>>> +    u8 i2c_inf;
>>> +    u8 i2c_dpll;
>>> +    u8 i2c_rep;
>>> +    u8 i2c_edid;
>>> +    u8 i2c_hdmi;
>>> +    u8 i2c_cp;
>>> +};
>>> +
>>> +/* Notify events */
>>> +#define ADV761X_HOTPLUG        1
>>> +#define ADV761X_FMT_CHANGE    2
>>> +
>>> +#endif    /* _ADV761X_H_ */
>>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/20/2013 03:19 PM, Hans Verkuil wrote:
> Hi Valentine,

Hi Hans,

>
> On 11/20/13 11:14, Valentine wrote:
>> On 11/19/2013 01:50 PM, Hans Verkuil wrote:
>>> Hi Valentine,
>>
>> Hi Hans,
>> thanks for your review.
>>
>>>
>>> I don't entirely understand how you use this driver with soc-camera.
>>> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
>>> act on it. Did you hack soc-camera to do this?
>>
>> I did not. The format is queried before reading the frame by the user-space.
>> I'm not sure if there's some kind of generic interface to notify the camera
>> layer about format change events. Different subdevices use different FMT_CHANGE
>> defines for that. I've implemented the format change notifier based on the adv7604
>> in hope that it may be useful later.
>
> Yes, I need to generalize the FMT_CHANGE event.
>
> But what happens if you are streaming and the HDMI connector is unplugged?
> Or plugged back in again, possibly with a larger resolution? I'm not sure
> if the soc_camera driver supports such scenarios.

It doesn't. Currently it's up to the UI to poll the format and do the necessary changes.
Otherwise the picture will be incorrect.

>
>>
>>>
>>> The way it stands I would prefer to see a version of the driver without
>>> soc-camera support. I wouldn't have a problem merging that as this driver
>>> is a good base for further development.
>>
>> I've tried to implement the driver base good enough to work with both SoC
>> and non-SoC cameras since I don't think having 2 separate drivers for
>> different camera models is a good idea.
>>
>> The problem is that I'm using it with R-Car VIN SoC camera driver and don't
>> have any other h/w. Having a platform data quirk for SoC camera in
>> the subdevice driver seemed simple and clean enough.
>
> I hate it, but it isn't something you can do anything about. So it will have
> to do for now.
>
>> Hacking SoC camera to make it support both generic and SoC cam subdevices
>> doesn't seem that straightforward to me.
>
> Guennadi, what is the status of this? I'm getting really tired of soc-camera
> infecting sub-devices. Subdev drivers should be independent of any bridge
> driver using them, but soc-camera keeps breaking that. It's driving me nuts.
>
> I'll be honest, it's getting to the point that I want to just NACK any
> future subdev drivers that depend on soc-camera, just to force a solution.
> There is no technical reason for this dependency, it just takes some time
> to fix soc-camera.
>
>> Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
>> involves a lot of regression testing with other R-Car boards that use different
>> subdevices with VIN.
>>
>> What would you suggest?
>
> Let's leave it as-is for now :-(
>
> I'm not happy, but as I said, it's not your fault.

OK, thanks.
Once a better solution is available we can remove the quirk.

>
> Regards,
>
> 	Hans

Thanks,
Val.

>
>>
>>>
>>> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
>>> control. It's easy to implement and that way apps can be notified when
>>> the hotplug changes value.
>>
>> OK, thanks.
>>
>>>
>>> Regards,
>>>
>>>      Hans
>>
>> Thanks,
>> Val.
>>
>>>
>>> On 11/15/13 13:54, Valentine Barshak wrote:
>>>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>>>> support. Only one HDMI port is supported on ADV7612.
>>>>
>>>> The code is based on the ADV7604 driver, and ADV7612 patch by
>>>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>>>
>>>> Changes in version 2:
>>>> * Used platform data for I2C addresses setup. The driver
>>>>     should work with both SoC and non-SoC camera models.
>>>> * Dropped unnecessary code and unsupported callbacks.
>>>> * Implemented IRQ handling for format change detection.
>>>>
>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Valentine,

Did you ever look at this adv7611 driver:

https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12

It adds adv761x support to the adv7604 in a pretty clean way.

Thinking it over I prefer to use that code (although you will have to
add the soc-camera hack for the time being) over your driver.

Others need adv7611 support as well, but with all the dv_timings etc. features
that are removed in your driver. So I am thinking that it is easier to merge
the xilinx version and add whatever you need on top of that.

Regards,

	Hans

On 11/20/13 13:24, Valentine wrote:
> On 11/20/2013 03:19 PM, Hans Verkuil wrote:
>> Hi Valentine,
> 
> Hi Hans,
> 
>>
>> On 11/20/13 11:14, Valentine wrote:
>>> On 11/19/2013 01:50 PM, Hans Verkuil wrote:
>>>> Hi Valentine,
>>>
>>> Hi Hans,
>>> thanks for your review.
>>>
>>>>
>>>> I don't entirely understand how you use this driver with soc-camera.
>>>> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
>>>> act on it. Did you hack soc-camera to do this?
>>>
>>> I did not. The format is queried before reading the frame by the user-space.
>>> I'm not sure if there's some kind of generic interface to notify the camera
>>> layer about format change events. Different subdevices use different FMT_CHANGE
>>> defines for that. I've implemented the format change notifier based on the adv7604
>>> in hope that it may be useful later.
>>
>> Yes, I need to generalize the FMT_CHANGE event.
>>
>> But what happens if you are streaming and the HDMI connector is unplugged?
>> Or plugged back in again, possibly with a larger resolution? I'm not sure
>> if the soc_camera driver supports such scenarios.
> 
> It doesn't. Currently it's up to the UI to poll the format and do the necessary changes.
> Otherwise the picture will be incorrect.
> 
>>
>>>
>>>>
>>>> The way it stands I would prefer to see a version of the driver without
>>>> soc-camera support. I wouldn't have a problem merging that as this driver
>>>> is a good base for further development.
>>>
>>> I've tried to implement the driver base good enough to work with both SoC
>>> and non-SoC cameras since I don't think having 2 separate drivers for
>>> different camera models is a good idea.
>>>
>>> The problem is that I'm using it with R-Car VIN SoC camera driver and don't
>>> have any other h/w. Having a platform data quirk for SoC camera in
>>> the subdevice driver seemed simple and clean enough.
>>
>> I hate it, but it isn't something you can do anything about. So it will have
>> to do for now.
>>
>>> Hacking SoC camera to make it support both generic and SoC cam subdevices
>>> doesn't seem that straightforward to me.
>>
>> Guennadi, what is the status of this? I'm getting really tired of soc-camera
>> infecting sub-devices. Subdev drivers should be independent of any bridge
>> driver using them, but soc-camera keeps breaking that. It's driving me nuts.
>>
>> I'll be honest, it's getting to the point that I want to just NACK any
>> future subdev drivers that depend on soc-camera, just to force a solution.
>> There is no technical reason for this dependency, it just takes some time
>> to fix soc-camera.
>>
>>> Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
>>> involves a lot of regression testing with other R-Car boards that use different
>>> subdevices with VIN.
>>>
>>> What would you suggest?
>>
>> Let's leave it as-is for now :-(
>>
>> I'm not happy, but as I said, it's not your fault.
> 
> OK, thanks.
> Once a better solution is available we can remove the quirk.
> 
>>
>> Regards,
>>
>>     Hans
> 
> Thanks,
> Val.
> 
>>
>>>
>>>>
>>>> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
>>>> control. It's easy to implement and that way apps can be notified when
>>>> the hotplug changes value.
>>>
>>> OK, thanks.
>>>
>>>>
>>>> Regards,
>>>>
>>>>      Hans
>>>
>>> Thanks,
>>> Val.
>>>
>>>>
>>>> On 11/15/13 13:54, Valentine Barshak wrote:
>>>>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>>>>> support. Only one HDMI port is supported on ADV7612.
>>>>>
>>>>> The code is based on the ADV7604 driver, and ADV7612 patch by
>>>>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>>>>
>>>>> Changes in version 2:
>>>>> * Used platform data for I2C addresses setup. The driver
>>>>>     should work with both SoC and non-SoC camera models.
>>>>> * Dropped unnecessary code and unsupported callbacks.
>>>>> * Implemented IRQ handling for format change detection.
>>>>>
>>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> Hi Valentine,
>
> Did you ever look at this adv7611 driver:
>
> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12

No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one.
I'm going to look closer at the patch and test it.

>
> It adds adv761x support to the adv7604 in a pretty clean way.
>
> Thinking it over I prefer to use that code (although you will have to
> add the soc-camera hack for the time being) over your driver.
>
> Others need adv7611 support as well, but with all the dv_timings etc. features
> that are removed in your driver. So I am thinking that it is easier to merge
> the xilinx version and add whatever you need on top of that.
>

Thanks,
Val.

> Regards,
>
> 	Hans
>
> On 11/20/13 13:24, Valentine wrote:
>> On 11/20/2013 03:19 PM, Hans Verkuil wrote:
>>> Hi Valentine,
>>
>> Hi Hans,
>>
>>>
>>> On 11/20/13 11:14, Valentine wrote:
>>>> On 11/19/2013 01:50 PM, Hans Verkuil wrote:
>>>>> Hi Valentine,
>>>>
>>>> Hi Hans,
>>>> thanks for your review.
>>>>
>>>>>
>>>>> I don't entirely understand how you use this driver with soc-camera.
>>>>> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
>>>>> act on it. Did you hack soc-camera to do this?
>>>>
>>>> I did not. The format is queried before reading the frame by the user-space.
>>>> I'm not sure if there's some kind of generic interface to notify the camera
>>>> layer about format change events. Different subdevices use different FMT_CHANGE
>>>> defines for that. I've implemented the format change notifier based on the adv7604
>>>> in hope that it may be useful later.
>>>
>>> Yes, I need to generalize the FMT_CHANGE event.
>>>
>>> But what happens if you are streaming and the HDMI connector is unplugged?
>>> Or plugged back in again, possibly with a larger resolution? I'm not sure
>>> if the soc_camera driver supports such scenarios.
>>
>> It doesn't. Currently it's up to the UI to poll the format and do the necessary changes.
>> Otherwise the picture will be incorrect.
>>
>>>
>>>>
>>>>>
>>>>> The way it stands I would prefer to see a version of the driver without
>>>>> soc-camera support. I wouldn't have a problem merging that as this driver
>>>>> is a good base for further development.
>>>>
>>>> I've tried to implement the driver base good enough to work with both SoC
>>>> and non-SoC cameras since I don't think having 2 separate drivers for
>>>> different camera models is a good idea.
>>>>
>>>> The problem is that I'm using it with R-Car VIN SoC camera driver and don't
>>>> have any other h/w. Having a platform data quirk for SoC camera in
>>>> the subdevice driver seemed simple and clean enough.
>>>
>>> I hate it, but it isn't something you can do anything about. So it will have
>>> to do for now.
>>>
>>>> Hacking SoC camera to make it support both generic and SoC cam subdevices
>>>> doesn't seem that straightforward to me.
>>>
>>> Guennadi, what is the status of this? I'm getting really tired of soc-camera
>>> infecting sub-devices. Subdev drivers should be independent of any bridge
>>> driver using them, but soc-camera keeps breaking that. It's driving me nuts.
>>>
>>> I'll be honest, it's getting to the point that I want to just NACK any
>>> future subdev drivers that depend on soc-camera, just to force a solution.
>>> There is no technical reason for this dependency, it just takes some time
>>> to fix soc-camera.
>>>
>>>> Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
>>>> involves a lot of regression testing with other R-Car boards that use different
>>>> subdevices with VIN.
>>>>
>>>> What would you suggest?
>>>
>>> Let's leave it as-is for now :-(
>>>
>>> I'm not happy, but as I said, it's not your fault.
>>
>> OK, thanks.
>> Once a better solution is available we can remove the quirk.
>>
>>>
>>> Regards,
>>>
>>>      Hans
>>
>> Thanks,
>> Val.
>>
>>>
>>>>
>>>>>
>>>>> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
>>>>> control. It's easy to implement and that way apps can be notified when
>>>>> the hotplug changes value.
>>>>
>>>> OK, thanks.
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>>       Hans
>>>>
>>>> Thanks,
>>>> Val.
>>>>
>>>>>
>>>>> On 11/15/13 13:54, Valentine Barshak wrote:
>>>>>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>>>>>> support. Only one HDMI port is supported on ADV7612.
>>>>>>
>>>>>> The code is based on the ADV7604 driver, and ADV7612 patch by
>>>>>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>>>>>
>>>>>> Changes in version 2:
>>>>>> * Used platform data for I2C addresses setup. The driver
>>>>>>      should work with both SoC and non-SoC camera models.
>>>>>> * Dropped unnecessary code and unsupported callbacks.
>>>>>> * Implemented IRQ handling for format change detection.
>>>>>>
>>>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/20/2013 07:53 PM, Valentine wrote:
> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>> Hi Valentine,

Hi Hans,

>>
>> Did you ever look at this adv7611 driver:
>>
>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>
> No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one.
> I'm going to look closer at the patch and test it.
>

I've tried the patch and I doubt that it was ever tested on adv7611.
I haven't been able to make it work so far. Here's the description of some of the issues
I've encountered.

The patch does not apply cleanly so I had to make small adjustments just to make it apply
without changing the functionality.

First of all the driver (adv7604_dummy_client function) does not set default I2C slave addresses
in the I/O map in case they are not set in the platform data.
This is not needed for 7604, since the default addresses are already set in the I/O map after chip reset.
However, the map is zeroed on 7611/7612 after power up, and we always have to set it manually.

I had to implement the IRQ handler since the soc_camera model does not use
interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
interrupt routed to a GPIO pin.
So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.

The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
seem to handle them correctly.
According to the docs:
"If an interrupt event occurs, and then a second interrupt event occurs before the system controller
has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."

However, the interrupt_service_routine doesn't account for that.
For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.

I'm also not sure how the dv_timing API should be used.
The internal adv7604 state->timings structure is used when getting mbus format.
However, the driver does not set the structure neither at start-up nor in the interrupt service callback when format changes.
Is it supposed to be set by the upper level camera driver?
For example, when the camera driver receives v4l2_subdev_notify(sd, ADV7604_FMT_CHANGE, NULL);
does it have to do the following:
v4l2_subdev_call(sd, video, query_dv_timings, timings);
v4l2_subdev_call(sd, video, s_dv_timings, timings);?

I don't think that this is how it should work.

Anyways, I've tried to call query_dv_timings to initialize state->timings from the interrupt service workqueue.
I've been able to catch format change events though it looks very sloppy at the moment.

BTW, the driver doesn't provide any locking for reading/writing the state->settings which I believe could cause
some issues reading incomplete format when it changes asynchronously to the subdevice g_mbus_fmt operation.

>>
>> It adds adv761x support to the adv7604 in a pretty clean way.

Doesn't seem that clean to me after having a look at it.
It tries to handle both 7604 and 7611 chips in the same way, though,
I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI receiver with no analog inputs.

>>
>> Thinking it over I prefer to use that code (although you will have to
>> add the soc-camera hack for the time being) over your driver.
>>
>> Others need adv7611 support as well, but with all the dv_timings etc. features
>> that are removed in your driver. So I am thinking that it is easier to merge
>> the xilinx version and add whatever you need on top of that.

To be honest I'm more inclined to drop non-soc camera support from my driver and
move it to media/i2c/soc_camera/ the moment. That would be easier.
I don't have any h/w I could test the xilinx version with non-SoC camera interface.
Currently I'm trying to play with core settings since even though I've managed to glue adv7611 support and
the R-Car VIN SoC camera driver I haven't been able to capture a frame.

>>
>
> Thanks,
> Val.
>
>> Regards,
>>
>>     Hans
>>

Thanks,
Val.

>> On 11/20/13 13:24, Valentine wrote:
>>> On 11/20/2013 03:19 PM, Hans Verkuil wrote:
>>>> Hi Valentine,
>>>
>>> Hi Hans,
>>>
>>>>
>>>> On 11/20/13 11:14, Valentine wrote:
>>>>> On 11/19/2013 01:50 PM, Hans Verkuil wrote:
>>>>>> Hi Valentine,
>>>>>
>>>>> Hi Hans,
>>>>> thanks for your review.
>>>>>
>>>>>>
>>>>>> I don't entirely understand how you use this driver with soc-camera.
>>>>>> Since soc-camera doesn't support FMT_CHANGE notifies it can't really
>>>>>> act on it. Did you hack soc-camera to do this?
>>>>>
>>>>> I did not. The format is queried before reading the frame by the user-space.
>>>>> I'm not sure if there's some kind of generic interface to notify the camera
>>>>> layer about format change events. Different subdevices use different FMT_CHANGE
>>>>> defines for that. I've implemented the format change notifier based on the adv7604
>>>>> in hope that it may be useful later.
>>>>
>>>> Yes, I need to generalize the FMT_CHANGE event.
>>>>
>>>> But what happens if you are streaming and the HDMI connector is unplugged?
>>>> Or plugged back in again, possibly with a larger resolution? I'm not sure
>>>> if the soc_camera driver supports such scenarios.
>>>
>>> It doesn't. Currently it's up to the UI to poll the format and do the necessary changes.
>>> Otherwise the picture will be incorrect.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> The way it stands I would prefer to see a version of the driver without
>>>>>> soc-camera support. I wouldn't have a problem merging that as this driver
>>>>>> is a good base for further development.
>>>>>
>>>>> I've tried to implement the driver base good enough to work with both SoC
>>>>> and non-SoC cameras since I don't think having 2 separate drivers for
>>>>> different camera models is a good idea.
>>>>>
>>>>> The problem is that I'm using it with R-Car VIN SoC camera driver and don't
>>>>> have any other h/w. Having a platform data quirk for SoC camera in
>>>>> the subdevice driver seemed simple and clean enough.
>>>>
>>>> I hate it, but it isn't something you can do anything about. So it will have
>>>> to do for now.
>>>>
>>>>> Hacking SoC camera to make it support both generic and SoC cam subdevices
>>>>> doesn't seem that straightforward to me.
>>>>
>>>> Guennadi, what is the status of this? I'm getting really tired of soc-camera
>>>> infecting sub-devices. Subdev drivers should be independent of any bridge
>>>> driver using them, but soc-camera keeps breaking that. It's driving me nuts.
>>>>
>>>> I'll be honest, it's getting to the point that I want to just NACK any
>>>> future subdev drivers that depend on soc-camera, just to force a solution.
>>>> There is no technical reason for this dependency, it just takes some time
>>>> to fix soc-camera.
>>>>
>>>>> Re-implementing R-Car VIN as a non-SoC model seems quite a big task that
>>>>> involves a lot of regression testing with other R-Car boards that use different
>>>>> subdevices with VIN.
>>>>>
>>>>> What would you suggest?
>>>>
>>>> Let's leave it as-is for now :-(
>>>>
>>>> I'm not happy, but as I said, it's not your fault.
>>>
>>> OK, thanks.
>>> Once a better solution is available we can remove the quirk.
>>>
>>>>
>>>> Regards,
>>>>
>>>>      Hans
>>>
>>> Thanks,
>>> Val.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> You do however have to add support for the V4L2_CID_DV_RX_POWER_PRESENT
>>>>>> control. It's easy to implement and that way apps can be notified when
>>>>>> the hotplug changes value.
>>>>>
>>>>> OK, thanks.
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>       Hans
>>>>>
>>>>> Thanks,
>>>>> Val.
>>>>>
>>>>>>
>>>>>> On 11/15/13 13:54, Valentine Barshak wrote:
>>>>>>> This adds ADV7611/ADV7612 Xpressview  HDMI Receiver base
>>>>>>> support. Only one HDMI port is supported on ADV7612.
>>>>>>>
>>>>>>> The code is based on the ADV7604 driver, and ADV7612 patch by
>>>>>>> Shinobu Uehara <shinobu.uehara.xc@renesas.com>
>>>>>>>
>>>>>>> Changes in version 2:
>>>>>>> * Used platform data for I2C addresses setup. The driver
>>>>>>>      should work with both SoC and non-SoC camera models.
>>>>>>> * Dropped unnecessary code and unsupported callbacks.
>>>>>>> * Implemented IRQ handling for format change detection.
>>>>>>>
>>>>>>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>>>
>>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/26/2013 10:28 PM, Valentine wrote:
> On 11/20/2013 07:53 PM, Valentine wrote:
>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>> Hi Valentine,
> 
> Hi Hans,
> 
>>>
>>> Did you ever look at this adv7611 driver:
>>>
>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>>>
>>
>> No, I missed that one somehow, although I did search for the adv7611/7612
>> before implementing this one.
>> I'm going to look closer at the patch and test it.
>>
> 
> I've tried the patch and I doubt that it was ever tested on adv7611.

It was and it works.

> I haven't been able to make it work so far. Here's the description of some
> of the issues
> I've encountered.
> 
> The patch does not apply cleanly so I had to make small adjustments just to
> make it apply
> without changing the functionality.

I have an updated version of the patch, which I intend to submit soon.

[...]
>>>
>>> It adds adv761x support to the adv7604 in a pretty clean way.
> 
> Doesn't seem that clean to me after having a look at it.
> It tries to handle both 7604 and 7611 chips in the same way, though,
> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI
> receiver with no analog inputs.

It is the same HDMI core (with minor modifications) though. So you end end
up with largely the same code for the 7604 and the 7611.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/2013 01:43 AM, Lars-Peter Clausen wrote:
> On 11/26/2013 10:28 PM, Valentine wrote:
>> On 11/20/2013 07:53 PM, Valentine wrote:
>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>> Hi Valentine,
>>
>> Hi Hans,
>>
>>>>
>>>> Did you ever look at this adv7611 driver:
>>>>
>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>>>>
>>>
>>> No, I missed that one somehow, although I did search for the adv7611/7612
>>> before implementing this one.
>>> I'm going to look closer at the patch and test it.
>>>
>>
>> I've tried the patch and I doubt that it was ever tested on adv7611.
>
> It was and it works.

That's good to hear.

>
>> I haven't been able to make it work so far. Here's the description of some
>> of the issues
>> I've encountered.
>>
>> The patch does not apply cleanly so I had to make small adjustments just to
>> make it apply
>> without changing the functionality.
>
> I have an updated version of the patch, which I intend to submit soon.

That's great, thanks!

>
> [...]

I'd also appreciate your thoughts about the issues I've described,
which have been replaced by [...] here.

>>>>
>>>> It adds adv761x support to the adv7604 in a pretty clean way.
>>
>> Doesn't seem that clean to me after having a look at it.
>> It tries to handle both 7604 and 7611 chips in the same way, though,
>> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI
>> receiver with no analog inputs.
>
> It is the same HDMI core (with minor modifications) though. So you end end
> up with largely the same code for the 7604 and the 7611.

Yes, but that's about half of the code while 7611 doesn't really need the non-HDMI part.
Just thought that we may end up with a bigger mess if we keep adding adv76xx support to 7604 driver.

>
> - Lars
>

Thanks,
Val.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Lars,

On Tuesday 26 November 2013 22:43:32 Lars-Peter Clausen wrote:
> On 11/26/2013 10:28 PM, Valentine wrote:
> > On 11/20/2013 07:53 PM, Valentine wrote:
> >> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> >>> Hi Valentine,
> > 
> > Hi Hans,
> > 
> >>> Did you ever look at this adv7611 driver:
> >>> 
> >>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
> >>> fa42af2daa12
> >> No, I missed that one somehow, although I did search for the adv7611/7612
> >> before implementing this one.
> >> I'm going to look closer at the patch and test it.
> > 
> > I've tried the patch and I doubt that it was ever tested on adv7611.
> 
> It was and it works.
> 
> > I haven't been able to make it work so far. Here's the description of some
> > of the issues I've encountered.
> > 
> > The patch does not apply cleanly so I had to make small adjustments just
> > to make it apply without changing the functionality.
> 
> I have an updated version of the patch, which I intend to submit soon.

Is it publicly available already ?

> [...]
> 
> >>> It adds adv761x support to the adv7604 in a pretty clean way.
> > 
> > Doesn't seem that clean to me after having a look at it.
> > It tries to handle both 7604 and 7611 chips in the same way, though,
> > I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI
> > receiver with no analog inputs.
> 
> It is the same HDMI core (with minor modifications) though. So you end end
> up with largely the same code for the 7604 and the 7611.
  

On 11/26/2013 10:57 PM, Valentine wrote:
[...]
>>
>> [...]
> 
> I'd also appreciate your thoughts about the issues I've described,
> which have been replaced by [...] here.

Those seem to be mostly issues that also apply to the adv7604 and should be
fixed anyway. Hans knows the code much better than me and will hopefully be
able to give a better answer.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/26/2013 11:00 PM, Laurent Pinchart wrote:
> Hi Lars,
> 
> On Tuesday 26 November 2013 22:43:32 Lars-Peter Clausen wrote:
>> On 11/26/2013 10:28 PM, Valentine wrote:
>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>> Hi Valentine,
>>>
>>> Hi Hans,
>>>
>>>>> Did you ever look at this adv7611 driver:
>>>>>
>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
>>>>> fa42af2daa12
>>>> No, I missed that one somehow, although I did search for the adv7611/7612
>>>> before implementing this one.
>>>> I'm going to look closer at the patch and test it.
>>>
>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>
>> It was and it works.
>>
>>> I haven't been able to make it work so far. Here's the description of some
>>> of the issues I've encountered.
>>>
>>> The patch does not apply cleanly so I had to make small adjustments just
>>> to make it apply without changing the functionality.
>>
>> I have an updated version of the patch, which I intend to submit soon.
> 
> Is it publicly available already ?
> 

Just started working on it the other day.

>> [...]
>>
>>>>> It adds adv761x support to the adv7604 in a pretty clean way.
>>>
>>> Doesn't seem that clean to me after having a look at it.
>>> It tries to handle both 7604 and 7611 chips in the same way, though,
>>> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI
>>> receiver with no analog inputs.
>>
>> It is the same HDMI core (with minor modifications) though. So you end end
>> up with largely the same code for the 7604 and the 7611.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Tuesday 26 November 2013 23:03:19 Lars-Peter Clausen wrote:
> On 11/26/2013 11:00 PM, Laurent Pinchart wrote:
> > On Tuesday 26 November 2013 22:43:32 Lars-Peter Clausen wrote:
> >> On 11/26/2013 10:28 PM, Valentine wrote:
> >>> On 11/20/2013 07:53 PM, Valentine wrote:
> >>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> >>>>> Hi Valentine,
> >>> 
> >>> Hi Hans,
> >>> 
> >>>>> Did you ever look at this adv7611 driver:
> >>>>> 
> >>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e
> >>>>> 4afa42af2daa12
> >>>> 
> >>>> No, I missed that one somehow, although I did search for the
> >>>> adv7611/7612 before implementing this one.
> >>>> I'm going to look closer at the patch and test it.
> >>> 
> >>> I've tried the patch and I doubt that it was ever tested on adv7611.
> >> 
> >> It was and it works.
> >> 
> >>> I haven't been able to make it work so far. Here's the description of
> >>> some of the issues I've encountered.
> >>> 
> >>> The patch does not apply cleanly so I had to make small adjustments just
> >>> to make it apply without changing the functionality.
> >> 
> >> I have an updated version of the patch, which I intend to submit soon.
> > 
> > Is it publicly available already ?
> 
> Just started working on it the other day.

I'm working on the same chip, how can we avoid effort duplication ?
  

On 11/26/2013 11:03 PM, Laurent Pinchart wrote:
> On Tuesday 26 November 2013 23:03:19 Lars-Peter Clausen wrote:
>> On 11/26/2013 11:00 PM, Laurent Pinchart wrote:
>>> On Tuesday 26 November 2013 22:43:32 Lars-Peter Clausen wrote:
>>>> On 11/26/2013 10:28 PM, Valentine wrote:
>>>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>>>> Hi Valentine,
>>>>>
>>>>> Hi Hans,
>>>>>
>>>>>>> Did you ever look at this adv7611 driver:
>>>>>>>
>>>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e
>>>>>>> 4afa42af2daa12
>>>>>>
>>>>>> No, I missed that one somehow, although I did search for the
>>>>>> adv7611/7612 before implementing this one.
>>>>>> I'm going to look closer at the patch and test it.
>>>>>
>>>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>>>
>>>> It was and it works.
>>>>
>>>>> I haven't been able to make it work so far. Here's the description of
>>>>> some of the issues I've encountered.
>>>>>
>>>>> The patch does not apply cleanly so I had to make small adjustments just
>>>>> to make it apply without changing the functionality.
>>>>
>>>> I have an updated version of the patch, which I intend to submit soon.
>>>
>>> Is it publicly available already ?
>>
>> Just started working on it the other day.
> 
> I'm working on the same chip, how can we avoid effort duplication ?

I'll have something by the end of the week, latest.

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/26/2013 10:28 PM, Valentine wrote:
> On 11/20/2013 07:53 PM, Valentine wrote:
>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>> Hi Valentine,
> 
> Hi Hans,
> 
>>>
>>> Did you ever look at this adv7611 driver:
>>>
>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>>
>> No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one.
>> I'm going to look closer at the patch and test it.
>>
> 
> I've tried the patch and I doubt that it was ever tested on adv7611.
> I haven't been able to make it work so far. Here's the description of some of the issues
> I've encountered.
> 
> The patch does not apply cleanly so I had to make small adjustments just to make it apply
> without changing the functionality.
> 
> First of all the driver (adv7604_dummy_client function) does not set default I2C slave addresses
> in the I/O map in case they are not set in the platform data.
> This is not needed for 7604, since the default addresses are already set in the I/O map after chip reset.
> However, the map is zeroed on 7611/7612 after power up, and we always have to set it manually.

So, the platform data for the 7611/2 should always give i2c addresses. That seems
reasonable.

> I had to implement the IRQ handler since the soc_camera model does not use
> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
> interrupt routed to a GPIO pin.
> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.

For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead
a register has to be read to figure out which device actually produced the irq. So I
want to keep the interrupt_service_routine(). However, adding a gpio field to the
platform_data that, if set, will tell the driver to request an irq and setup a
workqueue that calls interrupt_service_routine() would be fine with me. That will
benefit a lot of people since using gpios is much more common.

> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
> seem to handle them correctly.
> According to the docs:
> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller
> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."
> 
> However, the interrupt_service_routine doesn't account for that.
> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.

I'm not sure if there is much that can be done about this. The code reads the
interrupt status, then clears the interrupts right after. There is always a
race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter
has a better idea?

What can be improved, though, is to clear not just the interrupts that were
read, but all the interrupts that are unmasked. You are right, you could
loose an interrupt that way.

> I'm also not sure how the dv_timing API should be used.
> The internal adv7604 state->timings structure is used when getting mbus format.
> However, the driver does not set the structure neither at start-up nor in the interrupt service callback when format changes.
> Is it supposed to be set by the upper level camera driver?

It would be nice if the adv7604 would set up an initial timings format. In our
case it is indeed the bridge driver that sets it up, but in the general case it
is better if the i2c driver also sets an initial timings struct. 720p60 is
generally a good initial value.

The irq certainly shouldn't change timings: changing timings will most likely
require changes in the video buffer sizes, which generally requires stopping
streaming, reconfiguring the pipeline and restarting streaming. That's not
something the i2c driver can do.

The confusion you have with mbus vs dv_timings is that soc_camera lacks dv_timings
support. It was designed for sensors, although there is now some support for SDTV
receivers (s/g_std ioctls), but dv_timings support has to be added there as well
along the lines of what is done for s/g_std. Basically it is just a passthrough.

The way s_mbus_fmt is defined in adv7604 today is correct. s_dv_timings should be
called to change the format, s_mbus_fmt should just return the current width/height.
For HDTV there is more involved than just width and height when changing formats,
just as SDTV.

So the right approach is to add support for query/enum/s/g_dv_timings and dv_timings_cap
to soc_camera (just passthroughs). Then you can use it the way you are supposed to.

> For example, when the camera driver receives v4l2_subdev_notify(sd, ADV7604_FMT_CHANGE, NULL);
> does it have to do the following:
> v4l2_subdev_call(sd, video, query_dv_timings, timings);
> v4l2_subdev_call(sd, video, s_dv_timings, timings);?
> 
> I don't think that this is how it should work.

And it shouldn't work like that. The soc_camera driver has to send out a FMT_CHANGE
event to the application. It is the application that will receive that event, and
will have to call QUERY_DV_TIMINGS, stop streaming, allocate new buffers to accomodate
the new buffer size, call S_DV_TIMINGS and STREAMON to restart the newly configured
pipeline.

And we still haven't defined that FMT_CHANGE event. This is literally the first time
that an upstreamed bridge driver has to support this.

I will make an RFC for this today or tomorrow. It's really time that we add it.

> 
> Anyways, I've tried to call query_dv_timings to initialize state->timings from the interrupt service workqueue.

That's absolutely a no-go. Drivers should never change format midstream.

> I've been able to catch format change events though it looks very sloppy at the moment.
> 
> BTW, the driver doesn't provide any locking for reading/writing the state->settings which I believe could cause
> some issues reading incomplete format when it changes asynchronously to the subdevice g_mbus_fmt operation.

That shouldn't happen asynchronously. If you have asynchronous behavior like that, then
that needs a close look.

Another issue you have is how to set up the EDID in the receiver. Currently this
is done through the v4l2-subdevX device node of the subdev and the VIDIOC_SUBDEV_G/S_EDID
ioctls. However, soc_camera does not create those device nodes at the moment.
For simple pipelines this may be overkill anyway. In our (non-upstreamed) bridge
drivers we just implement VIDIOC_SUBDEV_G/S_EDID in the bridge driver and
pass it through to the subdev. I have to think about this a bit more, perhaps
I should create VIDIOC_G/S_EDID ioctls that can be used by standard, simple
v4l2 devices as well. I'll add it to the RFC.

Regardless, soc_camera needs to add support for setting/getting EDIDs one way
or another.

> 
>>>
>>> It adds adv761x support to the adv7604 in a pretty clean way.
> 
> Doesn't seem that clean to me after having a look at it.
> It tries to handle both 7604 and 7611 chips in the same way, though,
> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI receiver with no analog inputs.

The analog support of the adv7604 is pretty separate from the HDMI part, so
I don't see that as a big deal. That said, I do have to reevaluate that when
I see the latest version of this patch from Analog Devices later this week.

> 
>>>
>>> Thinking it over I prefer to use that code (although you will have to
>>> add the soc-camera hack for the time being) over your driver.
>>>
>>> Others need adv7611 support as well, but with all the dv_timings etc. features
>>> that are removed in your driver. So I am thinking that it is easier to merge
>>> the xilinx version and add whatever you need on top of that.
> 
> To be honest I'm more inclined to drop non-soc camera support from my driver and
> move it to media/i2c/soc_camera/ the moment. That would be easier.

I won't accept that, sorry. The issues you have derive more from misunderstanding
the way an HDTV receiver is supposed to work (it's not all that easy to wrap your
head around it) and from missing functionality for HDTV in soc_camera and even in
the V4L2 API (because certain bridge drivers that demonstrate how it works couldn't
be upstreamed and we want to avoid adding API additions without having drivers
using it).

> I don't have any h/w I could test the xilinx version with non-SoC camera interface.

Other than the glue to get hold of the platform_data there is no need for additional
soc_camera changes in the driver, so that's not the issue. But soc_camera itself
needs to be enhanced with dv_timings/FMT_CHANGE/EDID support.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

[...]
>> I had to implement the IRQ handler since the soc_camera model does not use
>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
>> interrupt routed to a GPIO pin.
>> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.
> 
> For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead
> a register has to be read to figure out which device actually produced the irq. So I
> want to keep the interrupt_service_routine(). However, adding a gpio field to the
> platform_data that, if set, will tell the driver to request an irq and setup a
> workqueue that calls interrupt_service_routine() would be fine with me. That will
> benefit a lot of people since using gpios is much more common.

I'll look into adding that since I'm using a GPIO for the interrupt on my
platform as well.

> 
>> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
>> seem to handle them correctly.
>> According to the docs:
>> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller
>> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."
>>
>> However, the interrupt_service_routine doesn't account for that.
>> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
>> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
>> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
>> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.
> 
> I'm not sure if there is much that can be done about this. The code reads the
> interrupt status, then clears the interrupts right after. There is always a
> race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter
> has a better idea?
>

As far as I understand it the interrupts lines are level triggered and will
stay asserted as long as there are unmasked, non-acked IRQS. So the
interrupt handler should be re-entered if there are still pending
interrupts. Is it possible that you setup a edge triggered interrupt, in
that case the handler wouldn't be reentered if the signal stays asserted?

But that's just my understanding from the manual, I'll have to verify
whether the hardware does indeed work like that.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/2013 12:21 PM, Hans Verkuil wrote:
> On 11/26/2013 10:28 PM, Valentine wrote:
>> On 11/20/2013 07:53 PM, Valentine wrote:
>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>> Hi Valentine,

Hi Hans,

>>
>> Hi Hans,
>>
>>>>
>>>> Did you ever look at this adv7611 driver:
>>>>
>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>>>
>>> No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one.
>>> I'm going to look closer at the patch and test it.
>>>
>>
>> I've tried the patch and I doubt that it was ever tested on adv7611.
>> I haven't been able to make it work so far. Here's the description of some of the issues
>> I've encountered.
>>
>> The patch does not apply cleanly so I had to make small adjustments just to make it apply
>> without changing the functionality.
>>
>> First of all the driver (adv7604_dummy_client function) does not set default I2C slave addresses
>> in the I/O map in case they are not set in the platform data.
>> This is not needed for 7604, since the default addresses are already set in the I/O map after chip reset.
>> However, the map is zeroed on 7611/7612 after power up, and we always have to set it manually.
>
> So, the platform data for the 7611/2 should always give i2c addresses. That seems
> reasonable.

Yes, but currently the comment in include/media/adv7604.h says
"i2c addresses: 0 == use default", and this is true for 7604, but
it doesn't work for 7611.

Probably the recommended value from the docs should be set by
the driver in case an I2C address is zero in the platform data.
This will help us to keep the same approach across all 76xx chips.

>
>> I had to implement the IRQ handler since the soc_camera model does not use
>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
>> interrupt routed to a GPIO pin.
>> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.
>
> For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead
> a register has to be read to figure out which device actually produced the irq. So I
> want to keep the interrupt_service_routine(). However, adding a gpio field to the
> platform_data that, if set, will tell the driver to request an irq and setup a
> workqueue that calls interrupt_service_routine() would be fine with me. That will
> benefit a lot of people since using gpios is much more common.
>
>> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
>> seem to handle them correctly.
>> According to the docs:
>> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller
>> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."
>>
>> However, the interrupt_service_routine doesn't account for that.
>> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
>> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
>> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
>> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.
>
> I'm not sure if there is much that can be done about this. The code reads the
> interrupt status, then clears the interrupts right after. There is always a
> race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter
> has a better idea?
>
> What can be improved, though, is to clear not just the interrupts that were
> read, but all the interrupts that are unmasked. You are right, you could
> loose an interrupt that way.
>
>> I'm also not sure how the dv_timing API should be used.
>> The internal adv7604 state->timings structure is used when getting mbus format.
>> However, the driver does not set the structure neither at start-up nor in the interrupt service callback when format changes.
>> Is it supposed to be set by the upper level camera driver?
>
> It would be nice if the adv7604 would set up an initial timings format. In our
> case it is indeed the bridge driver that sets it up, but in the general case it
> is better if the i2c driver also sets an initial timings struct. 720p60 is
> generally a good initial value.
>
> The irq certainly shouldn't change timings: changing timings will most likely
> require changes in the video buffer sizes, which generally requires stopping
> streaming, reconfiguring the pipeline and restarting streaming. That's not
> something the i2c driver can do.
>
> The confusion you have with mbus vs dv_timings is that soc_camera lacks dv_timings
> support. It was designed for sensors, although there is now some support for SDTV
> receivers (s/g_std ioctls), but dv_timings support has to be added there as well
> along the lines of what is done for s/g_std. Basically it is just a passthrough.
>
> The way s_mbus_fmt is defined in adv7604 today is correct. s_dv_timings should be
> called to change the format, s_mbus_fmt should just return the current width/height.
> For HDTV there is more involved than just width and height when changing formats,
> just as SDTV.
>
> So the right approach is to add support for query/enum/s/g_dv_timings and dv_timings_cap
> to soc_camera (just passthroughs). Then you can use it the way you are supposed to.
>
>> For example, when the camera driver receives v4l2_subdev_notify(sd, ADV7604_FMT_CHANGE, NULL);
>> does it have to do the following:
>> v4l2_subdev_call(sd, video, query_dv_timings, timings);
>> v4l2_subdev_call(sd, video, s_dv_timings, timings);?
>>
>> I don't think that this is how it should work.
>
> And it shouldn't work like that. The soc_camera driver has to send out a FMT_CHANGE
> event to the application. It is the application that will receive that event, and
> will have to call QUERY_DV_TIMINGS, stop streaming, allocate new buffers to accomodate
> the new buffer size, call S_DV_TIMINGS and STREAMON to restart the newly configured
> pipeline.
>

IIUC, we can't just use VIDIOC_S_FMT, prepare the buffers and read a frame from HDTV.
We have to query dv_timings instead.
It looks like VIDIOC_G_FMT, VIDIOC_S_FMT, VIDIOC_TRY_FMT may in fact return incorrect
data unless the application queries and sets DV timings before using S/G/TRY FMT ioctls.
What's the use of the FMT ioctls then?

> And we still haven't defined that FMT_CHANGE event. This is literally the first time
> that an upstreamed bridge driver has to support this.
>
> I will make an RFC for this today or tomorrow. It's really time that we add it.

Thanks!

>
>>
>> Anyways, I've tried to call query_dv_timings to initialize state->timings from the interrupt service workqueue.
>
> That's absolutely a no-go. Drivers should never change format midstream.
>
>> I've been able to catch format change events though it looks very sloppy at the moment.
>>
>> BTW, the driver doesn't provide any locking for reading/writing the state->settings which I believe could cause
>> some issues reading incomplete format when it changes asynchronously to the subdevice g_mbus_fmt operation.
>
> That shouldn't happen asynchronously. If you have asynchronous behavior like that, then
> that needs a close look.
>
> Another issue you have is how to set up the EDID in the receiver. Currently this
> is done through the v4l2-subdevX device node of the subdev and the VIDIOC_SUBDEV_G/S_EDID
> ioctls. However, soc_camera does not create those device nodes at the moment.
> For simple pipelines this may be overkill anyway. In our (non-upstreamed) bridge
> drivers we just implement VIDIOC_SUBDEV_G/S_EDID in the bridge driver and
> pass it through to the subdev. I have to think about this a bit more, perhaps
> I should create VIDIOC_G/S_EDID ioctls that can be used by standard, simple
> v4l2 devices as well. I'll add it to the RFC.
>
> Regardless, soc_camera needs to add support for setting/getting EDIDs one way
> or another.
>
>>
>>>>
>>>> It adds adv761x support to the adv7604 in a pretty clean way.
>>
>> Doesn't seem that clean to me after having a look at it.
>> It tries to handle both 7604 and 7611 chips in the same way, though,
>> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI receiver with no analog inputs.
>
> The analog support of the adv7604 is pretty separate from the HDMI part, so
> I don't see that as a big deal. That said, I do have to reevaluate that when
> I see the latest version of this patch from Analog Devices later this week.
>
>>
>>>>
>>>> Thinking it over I prefer to use that code (although you will have to
>>>> add the soc-camera hack for the time being) over your driver.
>>>>
>>>> Others need adv7611 support as well, but with all the dv_timings etc. features
>>>> that are removed in your driver. So I am thinking that it is easier to merge
>>>> the xilinx version and add whatever you need on top of that.
>>
>> To be honest I'm more inclined to drop non-soc camera support from my driver and
>> move it to media/i2c/soc_camera/ the moment. That would be easier.
>
> I won't accept that, sorry.The issues you have derive more from misunderstanding
> the way an HDTV receiver is supposed to work (it's not all that easy to wrap your
> head around it) and from missing functionality for HDTV in soc_camera and even in
> the V4L2 API (because certain bridge drivers that demonstrate how it works couldn't
> be upstreamed and we want to avoid adding API additions without having drivers
> using it).

Thank you very much for your explanations.
Yes, it's kind of hard to get a hold of it with some functionality missing in the API
and the drivers. Your help is much appreciated.

>
>> I don't have any h/w I could test the xilinx version with non-SoC camera interface.
>
> Other than the glue to get hold of the platform_data there is no need for additional
> soc_camera changes in the driver, so that's not the issue. But soc_camera itself
> needs to be enhanced with dv_timings/FMT_CHANGE/EDID support.
>

Thanks,
Val.


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/13 11:29, Valentine wrote:
> On 11/27/2013 12:21 PM, Hans Verkuil wrote:
>> On 11/26/2013 10:28 PM, Valentine wrote:
>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>> Hi Valentine,
> 
> Hi Hans,
> 
>>>
>>> Hi Hans,
>>>
>>>>>
>>>>> Did you ever look at this adv7611 driver:
>>>>>
>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4afa42af2daa12
>>>>
>>>> No, I missed that one somehow, although I did search for the adv7611/7612 before implementing this one.
>>>> I'm going to look closer at the patch and test it.
>>>>
>>>
>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>> I haven't been able to make it work so far. Here's the description of some of the issues
>>> I've encountered.
>>>
>>> The patch does not apply cleanly so I had to make small adjustments just to make it apply
>>> without changing the functionality.
>>>
>>> First of all the driver (adv7604_dummy_client function) does not set default I2C slave addresses
>>> in the I/O map in case they are not set in the platform data.
>>> This is not needed for 7604, since the default addresses are already set in the I/O map after chip reset.
>>> However, the map is zeroed on 7611/7612 after power up, and we always have to set it manually.
>>
>> So, the platform data for the 7611/2 should always give i2c addresses. That seems
>> reasonable.
> 
> Yes, but currently the comment in include/media/adv7604.h says
> "i2c addresses: 0 == use default", and this is true for 7604, but
> it doesn't work for 7611.
> 
> Probably the recommended value from the docs should be set by
> the driver in case an I2C address is zero in the platform data.
> This will help us to keep the same approach across all 76xx chips.

That would work for me.

> 
>>
>>> I had to implement the IRQ handler since the soc_camera model does not use
>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
>>> interrupt routed to a GPIO pin.
>>> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.
>>
>> For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead
>> a register has to be read to figure out which device actually produced the irq. So I
>> want to keep the interrupt_service_routine(). However, adding a gpio field to the
>> platform_data that, if set, will tell the driver to request an irq and setup a
>> workqueue that calls interrupt_service_routine() would be fine with me. That will
>> benefit a lot of people since using gpios is much more common.
>>
>>> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
>>> seem to handle them correctly.
>>> According to the docs:
>>> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller
>>> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."
>>>
>>> However, the interrupt_service_routine doesn't account for that.
>>> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
>>> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
>>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
>>> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
>>> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.
>>
>> I'm not sure if there is much that can be done about this. The code reads the
>> interrupt status, then clears the interrupts right after. There is always a
>> race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter
>> has a better idea?
>>
>> What can be improved, though, is to clear not just the interrupts that were
>> read, but all the interrupts that are unmasked. You are right, you could
>> loose an interrupt that way.
>>
>>> I'm also not sure how the dv_timing API should be used.
>>> The internal adv7604 state->timings structure is used when getting mbus format.
>>> However, the driver does not set the structure neither at start-up nor in the interrupt service callback when format changes.
>>> Is it supposed to be set by the upper level camera driver?
>>
>> It would be nice if the adv7604 would set up an initial timings format. In our
>> case it is indeed the bridge driver that sets it up, but in the general case it
>> is better if the i2c driver also sets an initial timings struct. 720p60 is
>> generally a good initial value.
>>
>> The irq certainly shouldn't change timings: changing timings will most likely
>> require changes in the video buffer sizes, which generally requires stopping
>> streaming, reconfiguring the pipeline and restarting streaming. That's not
>> something the i2c driver can do.
>>
>> The confusion you have with mbus vs dv_timings is that soc_camera lacks dv_timings
>> support. It was designed for sensors, although there is now some support for SDTV
>> receivers (s/g_std ioctls), but dv_timings support has to be added there as well
>> along the lines of what is done for s/g_std. Basically it is just a passthrough.
>>
>> The way s_mbus_fmt is defined in adv7604 today is correct. s_dv_timings should be
>> called to change the format, s_mbus_fmt should just return the current width/height.
>> For HDTV there is more involved than just width and height when changing formats,
>> just as SDTV.
>>
>> So the right approach is to add support for query/enum/s/g_dv_timings and dv_timings_cap
>> to soc_camera (just passthroughs). Then you can use it the way you are supposed to.
>>
>>> For example, when the camera driver receives v4l2_subdev_notify(sd, ADV7604_FMT_CHANGE, NULL);
>>> does it have to do the following:
>>> v4l2_subdev_call(sd, video, query_dv_timings, timings);
>>> v4l2_subdev_call(sd, video, s_dv_timings, timings);?
>>>
>>> I don't think that this is how it should work.
>>
>> And it shouldn't work like that. The soc_camera driver has to send out a FMT_CHANGE
>> event to the application. It is the application that will receive that event, and
>> will have to call QUERY_DV_TIMINGS, stop streaming, allocate new buffers to accomodate
>> the new buffer size, call S_DV_TIMINGS and STREAMON to restart the newly configured
>> pipeline.
>>
> 
> IIUC, we can't just use VIDIOC_S_FMT, prepare the buffers and read a frame from HDTV.
> We have to query dv_timings instead.

Right.

> It looks like VIDIOC_G_FMT, VIDIOC_S_FMT, VIDIOC_TRY_FMT may in fact return incorrect
> data unless the application queries and sets DV timings before using S/G/TRY FMT ioctls.
> What's the use of the FMT ioctls then?

The FMT ioctls define the size of your memory buffers. Depending on your use
case (crop/compose settings, stride, padding) you might want to allocate different
sized buffers than you would expect from just the timings.

Also, different DV timings will map to identical resolutions: different
frame rates/pixelclocks, reduced blanking modes, differences in polarities,
etc.

It's no different than e.g. PAL vs SECAM: same resolution, but very different
otherwise.

In other words: the DV_TIMINGS and STD ioctls negotiate the video signal
detection/setup and the FMT ioctls deal with the memory format. Setting the
video signal (S_STD or S_DV_TIMINGS) must be done first and it will typically
reset the v4l2_format settings. If streaming is in progress S_STD or
S_DV_TIMINGS must return EBUSY (unless there is no change, in which case
they can return 0 immediately).

> 
>> And we still haven't defined that FMT_CHANGE event. This is literally the first time
>> that an upstreamed bridge driver has to support this.
>>
>> I will make an RFC for this today or tomorrow. It's really time that we add it.
> 
> Thanks!
> 
>>
>>>
>>> Anyways, I've tried to call query_dv_timings to initialize state->timings from the interrupt service workqueue.
>>
>> That's absolutely a no-go. Drivers should never change format midstream.
>>
>>> I've been able to catch format change events though it looks very sloppy at the moment.
>>>
>>> BTW, the driver doesn't provide any locking for reading/writing the state->settings which I believe could cause
>>> some issues reading incomplete format when it changes asynchronously to the subdevice g_mbus_fmt operation.
>>
>> That shouldn't happen asynchronously. If you have asynchronous behavior like that, then
>> that needs a close look.
>>
>> Another issue you have is how to set up the EDID in the receiver. Currently this
>> is done through the v4l2-subdevX device node of the subdev and the VIDIOC_SUBDEV_G/S_EDID
>> ioctls. However, soc_camera does not create those device nodes at the moment.
>> For simple pipelines this may be overkill anyway. In our (non-upstreamed) bridge
>> drivers we just implement VIDIOC_SUBDEV_G/S_EDID in the bridge driver and
>> pass it through to the subdev. I have to think about this a bit more, perhaps
>> I should create VIDIOC_G/S_EDID ioctls that can be used by standard, simple
>> v4l2 devices as well. I'll add it to the RFC.
>>
>> Regardless, soc_camera needs to add support for setting/getting EDIDs one way
>> or another.
>>
>>>
>>>>>
>>>>> It adds adv761x support to the adv7604 in a pretty clean way.
>>>
>>> Doesn't seem that clean to me after having a look at it.
>>> It tries to handle both 7604 and 7611 chips in the same way, though,
>>> I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI receiver with no analog inputs.
>>
>> The analog support of the adv7604 is pretty separate from the HDMI part, so
>> I don't see that as a big deal. That said, I do have to reevaluate that when
>> I see the latest version of this patch from Analog Devices later this week.
>>
>>>
>>>>>
>>>>> Thinking it over I prefer to use that code (although you will have to
>>>>> add the soc-camera hack for the time being) over your driver.
>>>>>
>>>>> Others need adv7611 support as well, but with all the dv_timings etc. features
>>>>> that are removed in your driver. So I am thinking that it is easier to merge
>>>>> the xilinx version and add whatever you need on top of that.
>>>
>>> To be honest I'm more inclined to drop non-soc camera support from my driver and
>>> move it to media/i2c/soc_camera/ the moment. That would be easier.
>>
>> I won't accept that, sorry.The issues you have derive more from misunderstanding
>> the way an HDTV receiver is supposed to work (it's not all that easy to wrap your
>> head around it) and from missing functionality for HDTV in soc_camera and even in
>> the V4L2 API (because certain bridge drivers that demonstrate how it works couldn't
>> be upstreamed and we want to avoid adding API additions without having drivers
>> using it).
> 
> Thank you very much for your explanations.
> Yes, it's kind of hard to get a hold of it with some functionality missing in the API
> and the drivers. Your help is much appreciated.

No problem, I'd like to get this sorted as well.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/13 10:59, Lars-Peter Clausen wrote:
> [...]
>>> I had to implement the IRQ handler since the soc_camera model does not use
>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing about adv7612
>>> interrupt routed to a GPIO pin.
>>> So I had to schedule a workqueue and call adv7604_isr from there in case an interrupt happens.
>>
>> For our systems the adv7604 interrupts is not always hooked up to a gpio irq, instead
>> a register has to be read to figure out which device actually produced the irq. So I
>> want to keep the interrupt_service_routine(). However, adding a gpio field to the
>> platform_data that, if set, will tell the driver to request an irq and setup a
>> workqueue that calls interrupt_service_routine() would be fine with me. That will
>> benefit a lot of people since using gpios is much more common.
> 
> I'll look into adding that since I'm using a GPIO for the interrupt on my
> platform as well.
> 
>>
>>> The driver enables multiple interrupts on the chip, however, the adv7604_isr callback doesn't
>>> seem to handle them correctly.
>>> According to the docs:
>>> "If an interrupt event occurs, and then a second interrupt event occurs before the system controller
>>> has cleared or masked the first interrupt event, the ADV7611 does not generate a second interrupt signal."
>>>
>>> However, the interrupt_service_routine doesn't account for that.
>>> For example, in case fmt_change interrupt happens while fmt_change_digital interrupt is being
>>> processed by the adv7604_isr routine. If fmt_change status is set just before we clear fmt_change_digital,
>>> we never clear fmt_change. Thus, we end up with fmt_change interrupt missed and therefore further interrupts disabled.
>>> I've tried to call the adv7604_isr routine in a loop and return from the worlqueue only when all interrupt status bits are cleared.
>>> This did help a bit, but sometimes I started getting lots of I2C read/write errors for some reason.
>>
>> I'm not sure if there is much that can be done about this. The code reads the
>> interrupt status, then clears the interrupts right after. There is always a
>> race condition there since this isn't atomic ('read and clear'). Unless Lars-Peter
>> has a better idea?
>>
> 
> As far as I understand it the interrupts lines are level triggered and will
> stay asserted as long as there are unmasked, non-acked IRQS.

You are correct. If you are using level interrupts, then the current
implementation works fine. However, when using edge interrupts (and we
have one system that apparently has only edge-triggered GPIOs), then
it will fail.

The only way to handle that is to mask all interrupts at the start of
the isr, process the interrupts as usual, and unmask the interrupts
at the end of the isr. AFAICT this method should be safe as well with
level triggered interrupts.

Disregard my comment about clearing all interrupts, that's bogus.

> So the
> interrupt handler should be re-entered if there are still pending
> interrupts. Is it possible that you setup a edge triggered interrupt, in
> that case the handler wouldn't be reentered if the signal stays asserted?
> 
> But that's just my understanding from the manual, I'll have to verify
> whether the hardware does indeed work like that.
> 
> - Lars
> 

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Hans,

On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> On 11/26/2013 10:28 PM, Valentine wrote:
> > On 11/20/2013 07:53 PM, Valentine wrote:
> >> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> >>> Hi Valentine,
> > 
> > Hi Hans,
> > 
> >>> Did you ever look at this adv7611 driver:
> >>> 
> >>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
> >>> fa42af2daa12
> >>
> >> No, I missed that one somehow, although I did search for the adv7611/7612
> >> before implementing this one. I'm going to look closer at the patch and
> >> test it.
> > 
> > I've tried the patch and I doubt that it was ever tested on adv7611.
> > I haven't been able to make it work so far. Here's the description of some
> > of the issues I've encountered.
> > 
> > The patch does not apply cleanly so I had to make small adjustments just
> > to make it apply without changing the functionality.
> > 
> > First of all the driver (adv7604_dummy_client function) does not set
> > default I2C slave addresses in the I/O map in case they are not set in
> > the platform data.
> > This is not needed for 7604, since the default addresses are already set
> > in the I/O map after chip reset. However, the map is zeroed on 7611/7612
> > after power up, and we always have to set it manually.
>
> So, the platform data for the 7611/2 should always give i2c addresses. That
> seems reasonable.
> 
> > I had to implement the IRQ handler since the soc_camera model does not use
> > interrupt_service_routine subdevice callback and R-Car VIN knows nothing
> > about adv7612 interrupt routed to a GPIO pin.
> > So I had to schedule a workqueue and call adv7604_isr from there in case
> > an interrupt happens.
>
> For our systems the adv7604 interrupts is not always hooked up to a gpio
> irq, instead a register has to be read to figure out which device actually
> produced the irq.

Where is that register located ? Shouldn't it be modeled as an interrupt 
controller ?

> So I want to keep the interrupt_service_routine(). However, adding a gpio
> field to the platform_data that, if set, will tell the driver to request an
> irq and setup a workqueue that calls interrupt_service_routine() would be
> fine with me. That will benefit a lot of people since using gpios is much
> more common.

We should use the i2c_board_info.irq field for that, not a field in the 
platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.

> > The driver enables multiple interrupts on the chip, however, the
> > adv7604_isr callback doesn't seem to handle them correctly.
> > According to the docs:
> > "If an interrupt event occurs, and then a second interrupt event occurs
> > before the system controller has cleared or masked the first interrupt
> > event, the ADV7611 does not generate a second interrupt signal."
> > 
> > However, the interrupt_service_routine doesn't account for that.
> > For example, in case fmt_change interrupt happens while fmt_change_digital
> > interrupt is being processed by the adv7604_isr routine. If fmt_change
> > status is set just before we clear fmt_change_digital, we never clear
> > fmt_change. Thus, we end up with fmt_change interrupt missed and
> > therefore further interrupts disabled. I've tried to call the adv7604_isr
> > routine in a loop and return from the worlqueue only when all interrupt
> > status bits are cleared. This did help a bit, but sometimes I started
> > getting lots of I2C read/write errors for some reason.
>
> I'm not sure if there is much that can be done about this. The code reads
> the interrupt status, then clears the interrupts right after. There is
> always a race condition there since this isn't atomic ('read and clear').
> Unless Lars-Peter has a better idea?
> 
> What can be improved, though, is to clear not just the interrupts that were
> read, but all the interrupts that are unmasked. You are right, you could
> loose an interrupt that way.

Wouldn't level-trigerred interrupts fix the issue ?

> > I'm also not sure how the dv_timing API should be used.
> > The internal adv7604 state->timings structure is used when getting mbus
> > format. However, the driver does not set the structure neither at
> > start-up nor in the interrupt service callback when format changes. Is it
> > supposed to be set by the upper level camera driver?
> 
> It would be nice if the adv7604 would set up an initial timings format. In
> our case it is indeed the bridge driver that sets it up, but in the general
> case it is better if the i2c driver also sets an initial timings struct.
> 720p60 is generally a good initial value.
> 
> The irq certainly shouldn't change timings: changing timings will most
> likely require changes in the video buffer sizes, which generally requires
> stopping streaming, reconfiguring the pipeline and restarting streaming.
> That's not something the i2c driver can do.
> 
> The confusion you have with mbus vs dv_timings is that soc_camera lacks
> dv_timings support. It was designed for sensors, although there is now some
> support for SDTV receivers (s/g_std ioctls), but dv_timings support has to
> be added there as well along the lines of what is done for s/g_std.
> Basically it is just a passthrough.
> 
> The way s_mbus_fmt is defined in adv7604 today is correct. s_dv_timings
> should be called to change the format, s_mbus_fmt should just return the
> current width/height. For HDTV there is more involved than just width and
> height when changing formats, just as SDTV.
> 
> So the right approach is to add support for query/enum/s/g_dv_timings and
> dv_timings_cap to soc_camera (just passthroughs). Then you can use it the
> way you are supposed to.
>
> > For example, when the camera driver receives v4l2_subdev_notify(sd,
> > ADV7604_FMT_CHANGE, NULL); does it have to do the following:
> > v4l2_subdev_call(sd, video, query_dv_timings, timings);
> > v4l2_subdev_call(sd, video, s_dv_timings, timings);?
> > 
> > I don't think that this is how it should work.
> 
> And it shouldn't work like that. The soc_camera driver has to send out a
> FMT_CHANGE event to the application. It is the application that will
> receive that event, and will have to call QUERY_DV_TIMINGS, stop streaming,
> allocate new buffers to accomodate the new buffer size, call S_DV_TIMINGS
> and STREAMON to restart the newly configured pipeline.
> 
> And we still haven't defined that FMT_CHANGE event. This is literally the
> first time that an upstreamed bridge driver has to support this.
> 
> I will make an RFC for this today or tomorrow. It's really time that we add
> it.
>
> > Anyways, I've tried to call query_dv_timings to initialize state->timings
> > from the interrupt service workqueue.
>
> That's absolutely a no-go. Drivers should never change format midstream.
> 
> > I've been able to catch format change events though it looks very sloppy
> > at the moment.
> > 
> > BTW, the driver doesn't provide any locking for reading/writing the
> > state->settings which I believe could cause some issues reading
> > incomplete format when it changes asynchronously to the subdevice
> > g_mbus_fmt operation.
>
> That shouldn't happen asynchronously. If you have asynchronous behavior like
> that, then that needs a close look.
> 
> Another issue you have is how to set up the EDID in the receiver. Currently
> this is done through the v4l2-subdevX device node of the subdev and the
> VIDIOC_SUBDEV_G/S_EDID ioctls. However, soc_camera does not create those
> device nodes at the moment. For simple pipelines this may be overkill
> anyway. In our (non-upstreamed) bridge drivers we just implement
> VIDIOC_SUBDEV_G/S_EDID in the bridge driver and pass it through to the
> subdev. I have to think about this a bit more, perhaps I should create
> VIDIOC_G/S_EDID ioctls that can be used by standard, simple v4l2 devices as
> well. I'll add it to the RFC.
> 
> Regardless, soc_camera needs to add support for setting/getting EDIDs one
> way or another.
> 
> >>> It adds adv761x support to the adv7604 in a pretty clean way.
> > 
> > Doesn't seem that clean to me after having a look at it.
> > It tries to handle both 7604 and 7611 chips in the same way, though,
> > I'm not exactly sure if it's a good idea since 7611/12 is a pure HDMI
> > receiver with no analog inputs.
>
> The analog support of the adv7604 is pretty separate from the HDMI part, so
> I don't see that as a big deal. That said, I do have to reevaluate that when
> I see the latest version of this patch from Analog Devices later this week.
>
> >>> Thinking it over I prefer to use that code (although you will have to
> >>> add the soc-camera hack for the time being) over your driver.
> >>> 
> >>> Others need adv7611 support as well, but with all the dv_timings etc.
> >>> features that are removed in your driver. So I am thinking that it is
> >>> easier to merge the xilinx version and add whatever you need on top of
> >>> that.
> > 
> > To be honest I'm more inclined to drop non-soc camera support from my
> > driver and move it to media/i2c/soc_camera/ the moment. That would be
> > easier.
>
> I won't accept that, sorry. The issues you have derive more from
> misunderstanding the way an HDTV receiver is supposed to work (it's not all
> that easy to wrap your head around it) and from missing functionality for
> HDTV in soc_camera and even in the V4L2 API (because certain bridge drivers
> that demonstrate how it works couldn't be upstreamed and we want to avoid
> adding API additions without having drivers using it).
> 
> > I don't have any h/w I could test the xilinx version with non-SoC camera
> > interface.
>
> Other than the glue to get hold of the platform_data there is no need for
> additional soc_camera changes in the driver, so that's not the issue. But
> soc_camera itself needs to be enhanced with dv_timings/FMT_CHANGE/EDID
> support.
  

Hi Laurent,

On 11/27/13 12:39, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
>> On 11/26/2013 10:28 PM, Valentine wrote:
>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>> Hi Valentine,
>>>
>>> Hi Hans,
>>>
>>>>> Did you ever look at this adv7611 driver:
>>>>>
>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
>>>>> fa42af2daa12
>>>>
>>>> No, I missed that one somehow, although I did search for the adv7611/7612
>>>> before implementing this one. I'm going to look closer at the patch and
>>>> test it.
>>>
>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>> I haven't been able to make it work so far. Here's the description of some
>>> of the issues I've encountered.
>>>
>>> The patch does not apply cleanly so I had to make small adjustments just
>>> to make it apply without changing the functionality.
>>>
>>> First of all the driver (adv7604_dummy_client function) does not set
>>> default I2C slave addresses in the I/O map in case they are not set in
>>> the platform data.
>>> This is not needed for 7604, since the default addresses are already set
>>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
>>> after power up, and we always have to set it manually.
>>
>> So, the platform data for the 7611/2 should always give i2c addresses. That
>> seems reasonable.
>>
>>> I had to implement the IRQ handler since the soc_camera model does not use
>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>> about adv7612 interrupt routed to a GPIO pin.
>>> So I had to schedule a workqueue and call adv7604_isr from there in case
>>> an interrupt happens.
>>
>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>> irq, instead a register has to be read to figure out which device actually
>> produced the irq.
> 
> Where is that register located ? Shouldn't it be modeled as an interrupt 
> controller ?

It's a PCIe interrupt whose handler needs to read several FPGA registers
in order to figure out which interrupt was actually triggered. I don't
know enough about interrupt controller to understand whether it can be
modeled as a 'standard' interrupt.

> 
>> So I want to keep the interrupt_service_routine(). However, adding a gpio
>> field to the platform_data that, if set, will tell the driver to request an
>> irq and setup a workqueue that calls interrupt_service_routine() would be
>> fine with me. That will benefit a lot of people since using gpios is much
>> more common.
> 
> We should use the i2c_board_info.irq field for that, not a field in the 
> platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.

Yes, of course. Although the adv7604 has two interrupt lines, so if you
would want to use the second, then that would still have to be specified
through the platform data.

> 
>>> The driver enables multiple interrupts on the chip, however, the
>>> adv7604_isr callback doesn't seem to handle them correctly.
>>> According to the docs:
>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>> before the system controller has cleared or masked the first interrupt
>>> event, the ADV7611 does not generate a second interrupt signal."
>>>
>>> However, the interrupt_service_routine doesn't account for that.
>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>> interrupt is being processed by the adv7604_isr routine. If fmt_change
>>> status is set just before we clear fmt_change_digital, we never clear
>>> fmt_change. Thus, we end up with fmt_change interrupt missed and
>>> therefore further interrupts disabled. I've tried to call the adv7604_isr
>>> routine in a loop and return from the worlqueue only when all interrupt
>>> status bits are cleared. This did help a bit, but sometimes I started
>>> getting lots of I2C read/write errors for some reason.
>>
>> I'm not sure if there is much that can be done about this. The code reads
>> the interrupt status, then clears the interrupts right after. There is
>> always a race condition there since this isn't atomic ('read and clear').
>> Unless Lars-Peter has a better idea?
>>
>> What can be improved, though, is to clear not just the interrupts that were
>> read, but all the interrupts that are unmasked. You are right, you could
>> loose an interrupt that way.
> 
> Wouldn't level-trigerred interrupts fix the issue ?

See my earlier reply.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/2013 04:14 PM, Hans Verkuil wrote:
> Hi Laurent,
>
> On 11/27/13 12:39, Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
>>> On 11/26/2013 10:28 PM, Valentine wrote:
>>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>>> Hi Valentine,
>>>>
>>>> Hi Hans,
>>>>
>>>>>> Did you ever look at this adv7611 driver:
>>>>>>
>>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
>>>>>> fa42af2daa12
>>>>>
>>>>> No, I missed that one somehow, although I did search for the adv7611/7612
>>>>> before implementing this one. I'm going to look closer at the patch and
>>>>> test it.
>>>>
>>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>>> I haven't been able to make it work so far. Here's the description of some
>>>> of the issues I've encountered.
>>>>
>>>> The patch does not apply cleanly so I had to make small adjustments just
>>>> to make it apply without changing the functionality.
>>>>
>>>> First of all the driver (adv7604_dummy_client function) does not set
>>>> default I2C slave addresses in the I/O map in case they are not set in
>>>> the platform data.
>>>> This is not needed for 7604, since the default addresses are already set
>>>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
>>>> after power up, and we always have to set it manually.
>>>
>>> So, the platform data for the 7611/2 should always give i2c addresses. That
>>> seems reasonable.
>>>
>>>> I had to implement the IRQ handler since the soc_camera model does not use
>>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>>> about adv7612 interrupt routed to a GPIO pin.
>>>> So I had to schedule a workqueue and call adv7604_isr from there in case
>>>> an interrupt happens.
>>>
>>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>>> irq, instead a register has to be read to figure out which device actually
>>> produced the irq.
>>
>> Where is that register located ? Shouldn't it be modeled as an interrupt
>> controller ?
>
> It's a PCIe interrupt whose handler needs to read several FPGA registers
> in order to figure out which interrupt was actually triggered. I don't
> know enough about interrupt controller to understand whether it can be
> modeled as a 'standard' interrupt.
>
>>
>>> So I want to keep the interrupt_service_routine(). However, adding a gpio
>>> field to the platform_data that, if set, will tell the driver to request an
>>> irq and setup a workqueue that calls interrupt_service_routine() would be
>>> fine with me. That will benefit a lot of people since using gpios is much
>>> more common.
>>
>> We should use the i2c_board_info.irq field for that, not a field in the
>> platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.
>
> Yes, of course. Although the adv7604 has two interrupt lines, so if you
> would want to use the second, then that would still have to be specified
> through the platform data.

In this case the GPIO should be configured as interrupt source in the platform
code. But this doesn't seem to work with R-Car GPIO since it is initialized
later, and the gpio_to_irq function returns an error.
The simplest way seemed to use a GPIO number in the platform data
to have the adv driver configure the pin and request the IRQ.
I'm not sure how to easily defer I2C board info IRQ setup (and camera/subdevice probing)
until GPIO driver is ready.

>
>>
>>>> The driver enables multiple interrupts on the chip, however, the
>>>> adv7604_isr callback doesn't seem to handle them correctly.
>>>> According to the docs:
>>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>>> before the system controller has cleared or masked the first interrupt
>>>> event, the ADV7611 does not generate a second interrupt signal."
>>>>
>>>> However, the interrupt_service_routine doesn't account for that.
>>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>>> interrupt is being processed by the adv7604_isr routine. If fmt_change
>>>> status is set just before we clear fmt_change_digital, we never clear
>>>> fmt_change. Thus, we end up with fmt_change interrupt missed and
>>>> therefore further interrupts disabled. I've tried to call the adv7604_isr
>>>> routine in a loop and return from the worlqueue only when all interrupt
>>>> status bits are cleared. This did help a bit, but sometimes I started
>>>> getting lots of I2C read/write errors for some reason.
>>>
>>> I'm not sure if there is much that can be done about this. The code reads
>>> the interrupt status, then clears the interrupts right after. There is
>>> always a race condition there since this isn't atomic ('read and clear').
>>> Unless Lars-Peter has a better idea?
>>>
>>> What can be improved, though, is to clear not just the interrupts that were
>>> read, but all the interrupts that are unmasked. You are right, you could
>>> loose an interrupt that way.
>>
>> Wouldn't level-trigerred interrupts fix the issue ?

In this case we need to disable the IRQ line in the IRQ handler and re-enable it in the workqueue.
(we can't call the interrupt service routine from the interrupt context.)

This however didn't seem to work with R-Car GPIO.
Calling disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler doesn't seem
to disable it for some reason.

Also if the isr is called by the upper level camera driver, we assume that it needs
special handling (disabling/enabling) for the ADV76xx interrupt although it uses the API
interrupt_service_routine callback. Not a big deal, but still doesn't look pretty to me.

>
> See my earlier reply.
>
> Regards,
>
> 	Hans
>

Thanks,
Val.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/2013 01:32 PM, Valentine wrote:
> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
>> Hi Laurent,
>>
>> On 11/27/13 12:39, Laurent Pinchart wrote:
>>> Hi Hans,
>>>
>>> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
>>>> On 11/26/2013 10:28 PM, Valentine wrote:
>>>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>>>> Hi Valentine,
>>>>>
>>>>> Hi Hans,
>>>>>
>>>>>>> Did you ever look at this adv7611 driver:
>>>>>>>
>>>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
>>>>>>> fa42af2daa12
>>>>>>
>>>>>> No, I missed that one somehow, although I did search for the adv7611/7612
>>>>>> before implementing this one. I'm going to look closer at the patch and
>>>>>> test it.
>>>>>
>>>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>>>> I haven't been able to make it work so far. Here's the description of some
>>>>> of the issues I've encountered.
>>>>>
>>>>> The patch does not apply cleanly so I had to make small adjustments just
>>>>> to make it apply without changing the functionality.
>>>>>
>>>>> First of all the driver (adv7604_dummy_client function) does not set
>>>>> default I2C slave addresses in the I/O map in case they are not set in
>>>>> the platform data.
>>>>> This is not needed for 7604, since the default addresses are already set
>>>>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
>>>>> after power up, and we always have to set it manually.
>>>>
>>>> So, the platform data for the 7611/2 should always give i2c addresses. That
>>>> seems reasonable.
>>>>
>>>>> I had to implement the IRQ handler since the soc_camera model does not use
>>>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>>>> about adv7612 interrupt routed to a GPIO pin.
>>>>> So I had to schedule a workqueue and call adv7604_isr from there in case
>>>>> an interrupt happens.
>>>>
>>>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>>>> irq, instead a register has to be read to figure out which device actually
>>>> produced the irq.
>>>
>>> Where is that register located ? Shouldn't it be modeled as an interrupt
>>> controller ?
>>
>> It's a PCIe interrupt whose handler needs to read several FPGA registers
>> in order to figure out which interrupt was actually triggered. I don't
>> know enough about interrupt controller to understand whether it can be
>> modeled as a 'standard' interrupt.
>>
>>>
>>>> So I want to keep the interrupt_service_routine(). However, adding a gpio
>>>> field to the platform_data that, if set, will tell the driver to request an
>>>> irq and setup a workqueue that calls interrupt_service_routine() would be
>>>> fine with me. That will benefit a lot of people since using gpios is much
>>>> more common.
>>>
>>> We should use the i2c_board_info.irq field for that, not a field in the
>>> platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.
>>
>> Yes, of course. Although the adv7604 has two interrupt lines, so if you
>> would want to use the second, then that would still have to be specified
>> through the platform data.
> 
> In this case the GPIO should be configured as interrupt source in the platform
> code. But this doesn't seem to work with R-Car GPIO since it is initialized
> later, and the gpio_to_irq function returns an error.
> The simplest way seemed to use a GPIO number in the platform data
> to have the adv driver configure the pin and request the IRQ.
> I'm not sure how to easily defer I2C board info IRQ setup (and
> camera/subdevice probing)
> until GPIO driver is ready.

The GPIO driver should set up the GPIO pin as a interrupt pin when the
interrupt is requested. We should not have to add hacks to adv7604 driver to
workaround a broken GPIO driver.

> 
>>
>>>
>>>>> The driver enables multiple interrupts on the chip, however, the
>>>>> adv7604_isr callback doesn't seem to handle them correctly.
>>>>> According to the docs:
>>>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>>>> before the system controller has cleared or masked the first interrupt
>>>>> event, the ADV7611 does not generate a second interrupt signal."
>>>>>
>>>>> However, the interrupt_service_routine doesn't account for that.
>>>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>>>> interrupt is being processed by the adv7604_isr routine. If fmt_change
>>>>> status is set just before we clear fmt_change_digital, we never clear
>>>>> fmt_change. Thus, we end up with fmt_change interrupt missed and
>>>>> therefore further interrupts disabled. I've tried to call the adv7604_isr
>>>>> routine in a loop and return from the worlqueue only when all interrupt
>>>>> status bits are cleared. This did help a bit, but sometimes I started
>>>>> getting lots of I2C read/write errors for some reason.
>>>>
>>>> I'm not sure if there is much that can be done about this. The code reads
>>>> the interrupt status, then clears the interrupts right after. There is
>>>> always a race condition there since this isn't atomic ('read and clear').
>>>> Unless Lars-Peter has a better idea?
>>>>
>>>> What can be improved, though, is to clear not just the interrupts that were
>>>> read, but all the interrupts that are unmasked. You are right, you could
>>>> loose an interrupt that way.
>>>
>>> Wouldn't level-trigerred interrupts fix the issue ?
> 
> In this case we need to disable the IRQ line in the IRQ handler and
> re-enable it in the workqueue.
> (we can't call the interrupt service routine from the interrupt context.)
> 
> This however didn't seem to work with R-Car GPIO.
> Calling disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler
> doesn't seem
> to disable it for some reason.

Use a threaded interrupt instead of workqueue + disable_irq_nosync, that
should work fine.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/2013 05:07 PM, Lars-Peter Clausen wrote:
> On 11/27/2013 01:32 PM, Valentine wrote:
>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
>>> Hi Laurent,
>>>
>>> On 11/27/13 12:39, Laurent Pinchart wrote:
>>>> Hi Hans,
>>>>
>>>> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
>>>>> On 11/26/2013 10:28 PM, Valentine wrote:
>>>>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>>>>> Hi Valentine,
>>>>>>
>>>>>> Hi Hans,
>>>>>>
>>>>>>>> Did you ever look at this adv7611 driver:
>>>>>>>>
>>>>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e4a
>>>>>>>> fa42af2daa12
>>>>>>>
>>>>>>> No, I missed that one somehow, although I did search for the adv7611/7612
>>>>>>> before implementing this one. I'm going to look closer at the patch and
>>>>>>> test it.
>>>>>>
>>>>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>>>>> I haven't been able to make it work so far. Here's the description of some
>>>>>> of the issues I've encountered.
>>>>>>
>>>>>> The patch does not apply cleanly so I had to make small adjustments just
>>>>>> to make it apply without changing the functionality.
>>>>>>
>>>>>> First of all the driver (adv7604_dummy_client function) does not set
>>>>>> default I2C slave addresses in the I/O map in case they are not set in
>>>>>> the platform data.
>>>>>> This is not needed for 7604, since the default addresses are already set
>>>>>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
>>>>>> after power up, and we always have to set it manually.
>>>>>
>>>>> So, the platform data for the 7611/2 should always give i2c addresses. That
>>>>> seems reasonable.
>>>>>
>>>>>> I had to implement the IRQ handler since the soc_camera model does not use
>>>>>> interrupt_service_routine subdevice callback and R-Car VIN knows nothing
>>>>>> about adv7612 interrupt routed to a GPIO pin.
>>>>>> So I had to schedule a workqueue and call adv7604_isr from there in case
>>>>>> an interrupt happens.
>>>>>
>>>>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>>>>> irq, instead a register has to be read to figure out which device actually
>>>>> produced the irq.
>>>>
>>>> Where is that register located ? Shouldn't it be modeled as an interrupt
>>>> controller ?
>>>
>>> It's a PCIe interrupt whose handler needs to read several FPGA registers
>>> in order to figure out which interrupt was actually triggered. I don't
>>> know enough about interrupt controller to understand whether it can be
>>> modeled as a 'standard' interrupt.
>>>
>>>>
>>>>> So I want to keep the interrupt_service_routine(). However, adding a gpio
>>>>> field to the platform_data that, if set, will tell the driver to request an
>>>>> irq and setup a workqueue that calls interrupt_service_routine() would be
>>>>> fine with me. That will benefit a lot of people since using gpios is much
>>>>> more common.
>>>>
>>>> We should use the i2c_board_info.irq field for that, not a field in the
>>>> platform data structure. The IRQ line could be hooked up to a non-GPIO IRQ.
>>>
>>> Yes, of course. Although the adv7604 has two interrupt lines, so if you
>>> would want to use the second, then that would still have to be specified
>>> through the platform data.
>>
>> In this case the GPIO should be configured as interrupt source in the platform
>> code. But this doesn't seem to work with R-Car GPIO since it is initialized
>> later, and the gpio_to_irq function returns an error.
>> The simplest way seemed to use a GPIO number in the platform data
>> to have the adv driver configure the pin and request the IRQ.
>> I'm not sure how to easily defer I2C board info IRQ setup (and
>> camera/subdevice probing)
>> until GPIO driver is ready.
>
> The GPIO driver should set up the GPIO pin as a interrupt pin when the
> interrupt is requested. We should not have to add hacks to adv7604 driver to
> workaround a broken GPIO driver.

The GPIO driver does set up the pin as IRQ when the interrupt is requested.
The problem is that we can't get the IRQ number from the GPIO pin number until
the GPIO driver is started, which happens after the I2C device is registered
by the platform code.

So, the platform (board) init can't set up the i2c board info IRQ.
Using GPIO numbers in platform data seems a simple solution to that.
Besides, the chip supports two IRQ lines (as Hans has mentioned), while
the I2C board info has only one irq member.

I'm not sure that gpio_to_irq is supposed to always work (even at early board initialization)
and never return -EPROBE_DEFER.
If it is, then it's a GPIO driver issue. Otherwise, this is a question of I2C slave deferred probing,
I think, which is not that simple.

>
>>
>>>
>>>>
>>>>>> The driver enables multiple interrupts on the chip, however, the
>>>>>> adv7604_isr callback doesn't seem to handle them correctly.
>>>>>> According to the docs:
>>>>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>>>>> before the system controller has cleared or masked the first interrupt
>>>>>> event, the ADV7611 does not generate a second interrupt signal."
>>>>>>
>>>>>> However, the interrupt_service_routine doesn't account for that.
>>>>>> For example, in case fmt_change interrupt happens while fmt_change_digital
>>>>>> interrupt is being processed by the adv7604_isr routine. If fmt_change
>>>>>> status is set just before we clear fmt_change_digital, we never clear
>>>>>> fmt_change. Thus, we end up with fmt_change interrupt missed and
>>>>>> therefore further interrupts disabled. I've tried to call the adv7604_isr
>>>>>> routine in a loop and return from the worlqueue only when all interrupt
>>>>>> status bits are cleared. This did help a bit, but sometimes I started
>>>>>> getting lots of I2C read/write errors for some reason.
>>>>>
>>>>> I'm not sure if there is much that can be done about this. The code reads
>>>>> the interrupt status, then clears the interrupts right after. There is
>>>>> always a race condition there since this isn't atomic ('read and clear').
>>>>> Unless Lars-Peter has a better idea?
>>>>>
>>>>> What can be improved, though, is to clear not just the interrupts that were
>>>>> read, but all the interrupts that are unmasked. You are right, you could
>>>>> loose an interrupt that way.
>>>>
>>>> Wouldn't level-trigerred interrupts fix the issue ?
>>
>> In this case we need to disable the IRQ line in the IRQ handler and
>> re-enable it in the workqueue.
>> (we can't call the interrupt service routine from the interrupt context.)
>>
>> This however didn't seem to work with R-Car GPIO.
>> Calling disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler
>> doesn't seem
>> to disable it for some reason.
>
> Use a threaded interrupt instead of workqueue + disable_irq_nosync, that
> should work fine.

I've tried that too. It doesn't work either, perhaps for the same reason.

>
> - Lars
>

Thanks,
Val.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/27/2013 01:14 PM, Hans Verkuil wrote:
[...]
>>> For our systems the adv7604 interrupts is not always hooked up to a gpio
>>> irq, instead a register has to be read to figure out which device actually
>>> produced the irq.
>>
>> Where is that register located ? Shouldn't it be modeled as an interrupt 
>> controller ?
> 
> It's a PCIe interrupt whose handler needs to read several FPGA registers
> in order to figure out which interrupt was actually triggered. I don't
> know enough about interrupt controller to understand whether it can be
> modeled as a 'standard' interrupt.

This sounds as if it should be implemented as a irq_chip driver. There are a
couple of examples in drivers/irqchip/

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Wednesday 27 November 2013 15:50:18 Lars-Peter Clausen wrote:
> On 11/27/2013 01:14 PM, Hans Verkuil wrote:
> [...]
> 
> >>> For our systems the adv7604 interrupts is not always hooked up to a gpio
> >>> irq, instead a register has to be read to figure out which device
> >>> actually produced the irq.
> >> 
> >> Where is that register located ? Shouldn't it be modeled as an interrupt
> >> controller ?
> > 
> > It's a PCIe interrupt whose handler needs to read several FPGA registers
> > in order to figure out which interrupt was actually triggered. I don't
> > know enough about interrupt controller to understand whether it can be
> > modeled as a 'standard' interrupt.
> 
> This sounds as if it should be implemented as a irq_chip driver. There are a
> couple of examples in drivers/irqchip/

Exactly, that was my point. A piece of hardware that takes several interrupt 
inputs, includes mask and flag registers and generate a single interrupt 
towards the system is an interrupt controller and should have be handled by 
the Linux irqchip infrastructure.
  

Hi Hans,

On Wednesday 27 November 2013 13:14:41 Hans Verkuil wrote:
> On 11/27/13 12:39, Laurent Pinchart wrote:
> > On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> >> On 11/26/2013 10:28 PM, Valentine wrote:
> >>> On 11/20/2013 07:53 PM, Valentine wrote:
> >>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
> >>>>> Hi Valentine,
> >>> 
> >>> Hi Hans,
> >>> 
> >>>>> Did you ever look at this adv7611 driver:
> >>>>> 
> >>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e
> >>>>> 4afa42af2daa12
> >>>> 
> >>>> No, I missed that one somehow, although I did search for the
> >>>> adv7611/7612 before implementing this one. I'm going to look closer at
> >>>> the patch and test it.
> >>> 
> >>> I've tried the patch and I doubt that it was ever tested on adv7611.
> >>> I haven't been able to make it work so far. Here's the description of
> >>> some of the issues I've encountered.
> >>> 
> >>> The patch does not apply cleanly so I had to make small adjustments just
> >>> to make it apply without changing the functionality.
> >>> 
> >>> First of all the driver (adv7604_dummy_client function) does not set
> >>> default I2C slave addresses in the I/O map in case they are not set in
> >>> the platform data.
> >>> This is not needed for 7604, since the default addresses are already set
> >>> in the I/O map after chip reset. However, the map is zeroed on 7611/7612
> >>> after power up, and we always have to set it manually.
> >> 
> >> So, the platform data for the 7611/2 should always give i2c addresses.
> >> That seems reasonable.
> >> 
> >>> I had to implement the IRQ handler since the soc_camera model does not
> >>> use interrupt_service_routine subdevice callback and R-Car VIN knows
> >>> nothing about adv7612 interrupt routed to a GPIO pin. So I had to
> >>> schedule a workqueue and call adv7604_isr from there in case an
> >>> interrupt happens.
> >> 
> >> For our systems the adv7604 interrupts is not always hooked up to a gpio
> >> irq, instead a register has to be read to figure out which device
> >> actually produced the irq.
> > 
> > Where is that register located ? Shouldn't it be modeled as an interrupt
> > controller ?
> 
> It's a PCIe interrupt whose handler needs to read several FPGA registers
> in order to figure out which interrupt was actually triggered. I don't
> know enough about interrupt controller to understand whether it can be
> modeled as a 'standard' interrupt.

I've replied to this separately.

> >> So I want to keep the interrupt_service_routine(). However, adding a gpio
> >> field to the platform_data that, if set, will tell the driver to request
> >> an irq and setup a workqueue that calls interrupt_service_routine() would
> >> be fine with me. That will benefit a lot of people since using gpios is
> >> much more common.
> > 
> > We should use the i2c_board_info.irq field for that, not a field in the
> > platform data structure. The IRQ line could be hooked up to a non-GPIO
> > IRQ.
> 
> Yes, of course. Although the adv7604 has two interrupt lines, so if you
> would want to use the second, then that would still have to be specified
> through the platform data.

I believe we should then extend the I2C interrupt support. The reason for 
doing so is that we want to use the interrupt DT bindings for DT platforms, 
and those are handled by the I2C core.

> >>> The driver enables multiple interrupts on the chip, however, the
> >>> adv7604_isr callback doesn't seem to handle them correctly.
> >>> According to the docs:
> >>> "If an interrupt event occurs, and then a second interrupt event occurs
> >>> before the system controller has cleared or masked the first interrupt
> >>> event, the ADV7611 does not generate a second interrupt signal."
> >>> 
> >>> However, the interrupt_service_routine doesn't account for that.
> >>> For example, in case fmt_change interrupt happens while
> >>> fmt_change_digital interrupt is being processed by the adv7604_isr
> >>> routine. If fmt_change status is set just before we clear
> >>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
> >>> fmt_change interrupt missed and therefore further interrupts disabled.
> >>> I've tried to call the adv7604_isr routine in a loop and return from the
> >>> worlqueue only when all interrupt status bits are cleared. This did help
> >>> a bit, but sometimes I started getting lots of I2C read/write errors for
> >>> some reason.
> >> 
> >> I'm not sure if there is much that can be done about this. The code reads
> >> the interrupt status, then clears the interrupts right after. There is
> >> always a race condition there since this isn't atomic ('read and clear').
> >> Unless Lars-Peter has a better idea?
> >> 
> >> What can be improved, though, is to clear not just the interrupts that
> >> were read, but all the interrupts that are unmasked. You are right, you
> >> couldloose an interrupt that way.
> > 
> > Wouldn't level-trigerred interrupts fix the issue ?
> 
> See my earlier reply.
  

Hi Valentine,

(CC'ing Linus Walleij, Wolfram Sang and LAKML)

On Wednesday 27 November 2013 16:32:01 Valentine wrote:
> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
> > On 11/27/13 12:39, Laurent Pinchart wrote:
> >> On Wednesday 27 November 2013 09:21:22 Hans Verkuil wrote:
> >>> On 11/26/2013 10:28 PM, Valentine wrote:
> >>>> On 11/20/2013 07:53 PM, Valentine wrote:
> >>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:

[snip]

> >>> So I want to keep the interrupt_service_routine(). However, adding a
> >>> gpio field to the platform_data that, if set, will tell the driver to
> >>> request an irq and setup a workqueue that calls
> >>> interrupt_service_routine() would be fine with me. That will benefit a
> >>> lot of people since using gpios is much more common.
> >> 
> >> We should use the i2c_board_info.irq field for that, not a field in the
> >> platform data structure. The IRQ line could be hooked up to a non-GPIO
> >> IRQ.
> > 
> > Yes, of course. Although the adv7604 has two interrupt lines, so if you
> > would want to use the second, then that would still have to be specified
> > through the platform data.
> 
> In this case the GPIO should be configured as interrupt source in the
> platform code. But this doesn't seem to work with R-Car GPIO since it is
> initialized later, and the gpio_to_irq function returns an error.
> The simplest way seemed to use a GPIO number in the platform data
> to have the adv driver configure the pin and request the IRQ.
> I'm not sure how to easily defer I2C board info IRQ setup (and
> camera/subdevice probing) until GPIO driver is ready.

Good question. This looks like a core problem to me, not specific to the 
adv761x driver. Linus, Wolfram, do you have a comment on that ?

> >>>> The driver enables multiple interrupts on the chip, however, the
> >>>> adv7604_isr callback doesn't seem to handle them correctly.
> >>>> According to the docs:
> >>>> "If an interrupt event occurs, and then a second interrupt event occurs
> >>>> before the system controller has cleared or masked the first interrupt
> >>>> event, the ADV7611 does not generate a second interrupt signal."
> >>>> 
> >>>> However, the interrupt_service_routine doesn't account for that.
> >>>> For example, in case fmt_change interrupt happens while
> >>>> fmt_change_digital interrupt is being processed by the adv7604_isr
> >>>> routine. If fmt_change status is set just before we clear
> >>>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
> >>>> fmt_change interrupt missed and therefore further interrupts disabled.
> >>>> I've tried to call the adv7604_isr routine in a loop and return from
> >>>> the worlqueue only when all interrupt status bits are cleared. This did
> >>>> help a bit, but sometimes I started getting lots of I2C read/write
> >>>> errors for some reason.
> >>> 
> >>> I'm not sure if there is much that can be done about this. The code
> >>> reads the interrupt status, then clears the interrupts right after.
> >>> There is always a race condition there since this isn't atomic ('read
> >>> and clear'). Unless Lars-Peter has a better idea?
> >>> 
> >>> What can be improved, though, is to clear not just the interrupts that
> >>> were read, but all the interrupts that are unmasked. You are right, you
> >>> could loose an interrupt that way.
> >> 
> >> Wouldn't level-trigerred interrupts fix the issue ?
> 
> In this case we need to disable the IRQ line in the IRQ handler and
> re-enable it in the workqueue. (we can't call the interrupt service routine
> from the interrupt context.)

Can't we just flag the interrupt in a non-threaded IRQ handler, acknowledge 
the interrupt and then schedule work on a workqueue for the bottom half ?

> This however didn't seem to work with R-Car GPIO. Calling
> disable_irq_nosync(irq); from the GPIO LEVEL interrupt handler doesn't seem
> to disable it for some reason.
> 
> Also if the isr is called by the upper level camera driver, we assume that
> it needs special handling (disabling/enabling) for the ADV76xx interrupt
> although it uses the API interrupt_service_routine callback. Not a big
> deal, but still doesn't look pretty to me.
>
> > See my earlier reply.
  

[...]
>>>>>> The driver enables multiple interrupts on the chip, however, the
>>>>>> adv7604_isr callback doesn't seem to handle them correctly.
>>>>>> According to the docs:
>>>>>> "If an interrupt event occurs, and then a second interrupt event occurs
>>>>>> before the system controller has cleared or masked the first interrupt
>>>>>> event, the ADV7611 does not generate a second interrupt signal."
>>>>>>
>>>>>> However, the interrupt_service_routine doesn't account for that.
>>>>>> For example, in case fmt_change interrupt happens while
>>>>>> fmt_change_digital interrupt is being processed by the adv7604_isr
>>>>>> routine. If fmt_change status is set just before we clear
>>>>>> fmt_change_digital, we never clear fmt_change. Thus, we end up with
>>>>>> fmt_change interrupt missed and therefore further interrupts disabled.
>>>>>> I've tried to call the adv7604_isr routine in a loop and return from
>>>>>> the worlqueue only when all interrupt status bits are cleared. This did
>>>>>> help a bit, but sometimes I started getting lots of I2C read/write
>>>>>> errors for some reason.
>>>>>
>>>>> I'm not sure if there is much that can be done about this. The code
>>>>> reads the interrupt status, then clears the interrupts right after.
>>>>> There is always a race condition there since this isn't atomic ('read
>>>>> and clear'). Unless Lars-Peter has a better idea?
>>>>>
>>>>> What can be improved, though, is to clear not just the interrupts that
>>>>> were read, but all the interrupts that are unmasked. You are right, you
>>>>> could loose an interrupt that way.
>>>>
>>>> Wouldn't level-trigerred interrupts fix the issue ?
>>
>> In this case we need to disable the IRQ line in the IRQ handler and
>> re-enable it in the workqueue. (we can't call the interrupt service routine
>> from the interrupt context.)
> 
> Can't we just flag the interrupt in a non-threaded IRQ handler, acknowledge 
> the interrupt and then schedule work on a workqueue for the bottom half ?

Acknowledging the interrupt will require a non IRQ context, since it has to
do I2C transfers.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> (CC'ing Linus Walleij, Wolfram Sang and LAKML)
> On Wednesday 27 November 2013 16:32:01 Valentine wrote:
>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:

>> > Yes, of course. Although the adv7604 has two interrupt lines, so if you
>> > would want to use the second, then that would still have to be specified
>> > through the platform data.
>>
>> In this case the GPIO should be configured as interrupt source in the
>> platform code. But this doesn't seem to work with R-Car GPIO since it is
>> initialized later, and the gpio_to_irq function returns an error.
>> The simplest way seemed to use a GPIO number in the platform data
>> to have the adv driver configure the pin and request the IRQ.
>> I'm not sure how to easily defer I2C board info IRQ setup (and
>> camera/subdevice probing) until GPIO driver is ready.
>
> Good question. This looks like a core problem to me, not specific to the
> adv761x driver. Linus, Wolfram, do you have a comment on that ?

So we recently has a large-ish discussion involving me, Stephen
Warren and Jean-Christophe, leading to the conclusion that the
gpio_chip and irq_chip abstractions are orthogonal, and you should
be able to request a GPIO or IRQ without interacting with the other
subsystem.

Specifically you should be able to request an IRQ from the irq_chip
portions of the driver without first requesting the GPIO line.

Some drivers already support this.

We added an internal API to the gpiolib so that the lib, *internally*
can be made aware that a certain GPIO line is used for IRQs,
see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
"gpio: add API to be strict about GPIO IRQ usage"

So I guess the answer to the question is something like, fix
the GPIO driver to stop requiring the GPIO lines to be requested
and configured before being used as IRQs, delete that code,
and while you're at it add a call to gpiod_lock_as_irq()
to your GPIO driver in the right spot: examples are on the
mailing list and my mark-irqs branch in the GPIO tree.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/29/2013 11:37 AM, Linus Walleij wrote:
> On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> (CC'ing Linus Walleij, Wolfram Sang and LAKML)
>> On Wednesday 27 November 2013 16:32:01 Valentine wrote:
>>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
> 
>>>> Yes, of course. Although the adv7604 has two interrupt lines, so if you
>>>> would want to use the second, then that would still have to be specified
>>>> through the platform data.
>>>
>>> In this case the GPIO should be configured as interrupt source in the
>>> platform code. But this doesn't seem to work with R-Car GPIO since it is
>>> initialized later, and the gpio_to_irq function returns an error.
>>> The simplest way seemed to use a GPIO number in the platform data
>>> to have the adv driver configure the pin and request the IRQ.
>>> I'm not sure how to easily defer I2C board info IRQ setup (and
>>> camera/subdevice probing) until GPIO driver is ready.
>>
>> Good question. This looks like a core problem to me, not specific to the
>> adv761x driver. Linus, Wolfram, do you have a comment on that ?
> 
> So we recently has a large-ish discussion involving me, Stephen
> Warren and Jean-Christophe, leading to the conclusion that the
> gpio_chip and irq_chip abstractions are orthogonal, and you should
> be able to request a GPIO or IRQ without interacting with the other
> subsystem.
> 
> Specifically you should be able to request an IRQ from the irq_chip
> portions of the driver without first requesting the GPIO line.
> 
> Some drivers already support this.
> 
> We added an internal API to the gpiolib so that the lib, *internally*
> can be made aware that a certain GPIO line is used for IRQs,
> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
> "gpio: add API to be strict about GPIO IRQ usage"
> 
> So I guess the answer to the question is something like, fix
> the GPIO driver to stop requiring the GPIO lines to be requested
> and configured before being used as IRQs, delete that code,
> and while you're at it add a call to gpiod_lock_as_irq()
> to your GPIO driver in the right spot: examples are on the
> mailing list and my mark-irqs branch in the GPIO tree.

As far as I understand it this already works more or less with the driver.
The problem is that the IRQ numbers are dynamically allocated, while the
GPIO numbers apparently are not. So the board code knows the the GPIO number
at compile time and can pass this to the diver which then does a gpio_to_irq
to lookup the IRQ number. This of course isn't really a problem with
devicetree, but only with platform board code.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/29/2013 02:45 PM, Lars-Peter Clausen wrote:
> On 11/29/2013 11:37 AM, Linus Walleij wrote:
>> On Wed, Nov 27, 2013 at 5:40 PM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>> (CC'ing Linus Walleij, Wolfram Sang and LAKML)
>>> On Wednesday 27 November 2013 16:32:01 Valentine wrote:
>>>> On 11/27/2013 04:14 PM, Hans Verkuil wrote:
>>
>>>>> Yes, of course. Although the adv7604 has two interrupt lines, so if you
>>>>> would want to use the second, then that would still have to be specified
>>>>> through the platform data.
>>>>
>>>> In this case the GPIO should be configured as interrupt source in the
>>>> platform code. But this doesn't seem to work with R-Car GPIO since it is
>>>> initialized later, and the gpio_to_irq function returns an error.
>>>> The simplest way seemed to use a GPIO number in the platform data
>>>> to have the adv driver configure the pin and request the IRQ.
>>>> I'm not sure how to easily defer I2C board info IRQ setup (and
>>>> camera/subdevice probing) until GPIO driver is ready.
>>>
>>> Good question. This looks like a core problem to me, not specific to the
>>> adv761x driver. Linus, Wolfram, do you have a comment on that ?
>>
>> So we recently has a large-ish discussion involving me, Stephen
>> Warren and Jean-Christophe, leading to the conclusion that the
>> gpio_chip and irq_chip abstractions are orthogonal, and you should
>> be able to request a GPIO or IRQ without interacting with the other
>> subsystem.
>>
>> Specifically you should be able to request an IRQ from the irq_chip
>> portions of the driver without first requesting the GPIO line.
>>
>> Some drivers already support this.
>>
>> We added an internal API to the gpiolib so that the lib, *internally*
>> can be made aware that a certain GPIO line is used for IRQs,
>> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
>> "gpio: add API to be strict about GPIO IRQ usage"
>>
>> So I guess the answer to the question is something like, fix
>> the GPIO driver to stop requiring the GPIO lines to be requested
>> and configured before being used as IRQs, delete that code,
>> and while you're at it add a call to gpiod_lock_as_irq()
>> to your GPIO driver in the right spot: examples are on the
>> mailing list and my mark-irqs branch in the GPIO tree.
>
> As far as I understand it this already works more or less with the driver.
> The problem is that the IRQ numbers are dynamically allocated, while the
> GPIO numbers apparently are not. So the board code knows the the GPIO number
> at compile time and can pass this to the diver which then does a gpio_to_irq
> to lookup the IRQ number.

This is correct.

> This of course isn't really a problem with
> devicetree, but only with platform board code.

I'm not sure what's the difference here and why it is not a problem with devicetree?

The other problem with R-Car GPIO driver is that it apparently does not support level IRQs.

>
> - Lars
>

Thanks,
Val.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/29/2013 11:37 AM, Linus Walleij wrote:
(...)
>> Specifically you should be able to request an IRQ from the irq_chip
>> portions of the driver without first requesting the GPIO line.
>>
>> Some drivers already support this.
>>
>> We added an internal API to the gpiolib so that the lib, *internally*
>> can be made aware that a certain GPIO line is used for IRQs,
>> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
>> "gpio: add API to be strict about GPIO IRQ usage"
>>
>> So I guess the answer to the question is something like, fix
>> the GPIO driver to stop requiring the GPIO lines to be requested
>> and configured before being used as IRQs, delete that code,
>> and while you're at it add a call to gpiod_lock_as_irq()
>> to your GPIO driver in the right spot: examples are on the
>> mailing list and my mark-irqs branch in the GPIO tree.
>
> As far as I understand it this already works more or less with the driver.
> The problem is that the IRQ numbers are dynamically allocated, while the
> GPIO numbers apparently are not. So the board code knows the the GPIO number
> at compile time and can pass this to the diver which then does a gpio_to_irq
> to lookup the IRQ number. This of course isn't really a problem with
> devicetree, but only with platform board code.

This has been solved *also* for platform board code by the new, fresh
GPIO descriptor mechanism, see Documentation/gpio/*
in Torvalds' git HEAD.

In your board file provide something like that:
http://marc.info/?l=linux-gpio&m=138546046203600&w=2

Then switch the driver to use the gpiod_* interface like:
http://marc.info/?l=linux-gpio&m=138546036028076&w=2

Problem solved.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, Nov 29, 2013 at 1:14 PM, Valentine
<valentine.barshak@cogentembedded.com> wrote:
> On 11/29/2013 02:45 PM, Lars-Peter Clausen wrote:
>> On 11/29/2013 11:37 AM, Linus Walleij wrote:

>>> So I guess the answer to the question is something like, fix
>>> the GPIO driver to stop requiring the GPIO lines to be requested
>>> and configured before being used as IRQs, delete that code,
>>> and while you're at it add a call to gpiod_lock_as_irq()
>>> to your GPIO driver in the right spot: examples are on the
>>> mailing list and my mark-irqs branch in the GPIO tree.
>>
>> As far as I understand it this already works more or less with the driver.
>> The problem is that the IRQ numbers are dynamically allocated, while the
>> GPIO numbers apparently are not. So the board code knows the the GPIO
>> number
>> at compile time and can pass this to the diver which then does a
>> gpio_to_irq
>> to lookup the IRQ number.
(...)
>> This of course isn't really a problem with
>> devicetree, but only with platform board code.
>
> I'm not sure what's the difference here and why it is not a problem with
> devicetree?

It's no problem when using devicetree because you can obtain
the GPIOs directly from the node with of_get_gpio()
and of_get_named_gpio() in the special DT probe path.

But don't do that! Instead switch the whole driver, and preferably
the whole platform, to use descriptors.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/29/2013 02:42 PM, Linus Walleij wrote:
> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/29/2013 11:37 AM, Linus Walleij wrote:
> (...)
>>> Specifically you should be able to request an IRQ from the irq_chip
>>> portions of the driver without first requesting the GPIO line.
>>>
>>> Some drivers already support this.
>>>
>>> We added an internal API to the gpiolib so that the lib, *internally*
>>> can be made aware that a certain GPIO line is used for IRQs,
>>> see commit d468bf9ecaabd3bf3a6134e5a369ced82b1d1ca1
>>> "gpio: add API to be strict about GPIO IRQ usage"
>>>
>>> So I guess the answer to the question is something like, fix
>>> the GPIO driver to stop requiring the GPIO lines to be requested
>>> and configured before being used as IRQs, delete that code,
>>> and while you're at it add a call to gpiod_lock_as_irq()
>>> to your GPIO driver in the right spot: examples are on the
>>> mailing list and my mark-irqs branch in the GPIO tree.
>>
>> As far as I understand it this already works more or less with the driver.
>> The problem is that the IRQ numbers are dynamically allocated, while the
>> GPIO numbers apparently are not. So the board code knows the the GPIO number
>> at compile time and can pass this to the diver which then does a gpio_to_irq
>> to lookup the IRQ number. This of course isn't really a problem with
>> devicetree, but only with platform board code.
> 
> This has been solved *also* for platform board code by the new, fresh
> GPIO descriptor mechanism, see Documentation/gpio/*
> in Torvalds' git HEAD.

This works when the GPIO numbers are dynamically allocated (which are static
in this case), but not for IRQ numbers.

- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/29/2013 02:42 PM, Linus Walleij wrote:
>> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>
>>> As far as I understand it this already works more or less with the driver.
>>> The problem is that the IRQ numbers are dynamically allocated, while the
>>> GPIO numbers apparently are not. So the board code knows the the GPIO number
>>> at compile time and can pass this to the diver which then does a gpio_to_irq
>>> to lookup the IRQ number. This of course isn't really a problem with
>>> devicetree, but only with platform board code.
>>
>> This has been solved *also* for platform board code by the new, fresh
>> GPIO descriptor mechanism, see Documentation/gpio/*
>> in Torvalds' git HEAD.
>
> This works when the GPIO numbers are dynamically allocated (which are static
> in this case), but not for IRQ numbers.

Sorry I don't get what you're after here. I'm not the subsystem
maintainer for IRQ chips ...

In the DT boot path for platform or AMBA devices the IRQs
are automatically resolved to resources and passed with the
device so that is certainly not the problem, right?

I guess you may be referring to the problem of instatiating
a dynamic IRQ chip in *board code* and then passing the
obtained dynamic IRQ numbers as resources to the
devices also created in a board file?

That would be like you're asking for a function that would
return the base of an irq_chip, that needs to be discussed
with the irq maintainers, so not much I can say, but maybe
I misunderstood this?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi Linus,

On Friday 29 November 2013 20:52:17 Linus Walleij wrote:
> On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen wrote:
> > On 11/29/2013 02:42 PM, Linus Walleij wrote:
> >> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen wrote:
> >>> As far as I understand it this already works more or less with the
> >>> driver.
> >>> The problem is that the IRQ numbers are dynamically allocated, while the
> >>> GPIO numbers apparently are not. So the board code knows the the GPIO
> >>> number at compile time and can pass this to the diver which then does a
> >>> gpio_to_irq to lookup the IRQ number. This of course isn't really a
> >>> problem with devicetree, but only with platform board code.
> >> 
> >> This has been solved *also* for platform board code by the new, fresh
> >> GPIO descriptor mechanism, see Documentation/gpio/*
> >> in Torvalds' git HEAD.
> > 
> > This works when the GPIO numbers are dynamically allocated (which are
> > static in this case), but not for IRQ numbers.
> 
> Sorry I don't get what you're after here. I'm not the subsystem maintainer
> for IRQ chips ...
> 
> In the DT boot path for platform or AMBA devices the IRQs are automatically
> resolved to resources and passed with the device so that is certainly not
> the problem, right?
> 
> I guess you may be referring to the problem of instatiating a dynamic IRQ
> chip in *board code* and then passing the obtained dynamic IRQ numbers as
> resources to the devices also created in a board file?

What we need is to pass an IRQ number to the device driver through platform 
device resources. When the adv761x IRQ line is connected to a GPIO (for which 
we know the number), the natural way to do so it to call gpio_to_irq() to 
convert the GPIO number to the IRQ number. However, calling this function in 
board code will fail as the GPIO driver hasn't probed the GPIO device.

We could pass the GPIO number to the device through platform data, but if the 
IRQ line is connected to a SoC dedicated IRQ input not handled by a GPIO 
driver that won't work either. Furthermore, this would just be a workaround, 
as the driver needs an IRQ and shouldn't have to care about GPIOs.

> That would be like you're asking for a function that would return the base
> of an irq_chip, that needs to be discussed with the irq maintainers, so not
> much I can say, but maybe I misunderstood this?
  

On 11/29/2013 08:52 PM, Linus Walleij wrote:
> On Fri, Nov 29, 2013 at 2:48 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 11/29/2013 02:42 PM, Linus Walleij wrote:
>>> On Fri, Nov 29, 2013 at 11:45 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>>
>>>> As far as I understand it this already works more or less with the driver.
>>>> The problem is that the IRQ numbers are dynamically allocated, while the
>>>> GPIO numbers apparently are not. So the board code knows the the GPIO number
>>>> at compile time and can pass this to the diver which then does a gpio_to_irq
>>>> to lookup the IRQ number. This of course isn't really a problem with
>>>> devicetree, but only with platform board code.
>>>
>>> This has been solved *also* for platform board code by the new, fresh
>>> GPIO descriptor mechanism, see Documentation/gpio/*
>>> in Torvalds' git HEAD.
>>
>> This works when the GPIO numbers are dynamically allocated (which are static
>> in this case), but not for IRQ numbers.
> 
> Sorry I don't get what you're after here. I'm not the subsystem
> maintainer for IRQ chips ...

I'm trying to explain, that the problem is not about GPIO number lookup, but
rather about IRQ number lookup :)

> 
> In the DT boot path for platform or AMBA devices the IRQs
> are automatically resolved to resources and passed with the
> device so that is certainly not the problem, right?

Yep, what I said earlier, this is a problem that's solved by using DT.

> 
> I guess you may be referring to the problem of instatiating
> a dynamic IRQ chip in *board code* and then passing the
> obtained dynamic IRQ numbers as resources to the
> devices also created in a board file?
> 

Yes.

> That would be like you're asking for a function that would
> return the base of an irq_chip, that needs to be discussed
> with the irq maintainers, so not much I can say, but maybe
> I misunderstood this?

I my opinion the best solution for this problem is to have the same lookup
mechanism we've had for clocks, regulators, etc and now also GPIOs.

- Lars


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 11/26/2013 11:06 PM, Lars-Peter Clausen wrote:
> On 11/26/2013 11:03 PM, Laurent Pinchart wrote:
>> On Tuesday 26 November 2013 23:03:19 Lars-Peter Clausen wrote:
>>> On 11/26/2013 11:00 PM, Laurent Pinchart wrote:
>>>> On Tuesday 26 November 2013 22:43:32 Lars-Peter Clausen wrote:
>>>>> On 11/26/2013 10:28 PM, Valentine wrote:
>>>>>> On 11/20/2013 07:53 PM, Valentine wrote:
>>>>>>> On 11/20/2013 07:42 PM, Hans Verkuil wrote:
>>>>>>>> Hi Valentine,
>>>>>>
>>>>>> Hi Hans,
>>>>>>
>>>>>>>> Did you ever look at this adv7611 driver:
>>>>>>>>
>>>>>>>> https://github.com/Xilinx/linux-xlnx/commit/610b9d5de22ae7c0047c65a07e
>>>>>>>> 4afa42af2daa12
>>>>>>>
>>>>>>> No, I missed that one somehow, although I did search for the
>>>>>>> adv7611/7612 before implementing this one.
>>>>>>> I'm going to look closer at the patch and test it.
>>>>>>
>>>>>> I've tried the patch and I doubt that it was ever tested on adv7611.
>>>>>
>>>>> It was and it works.
>>>>>
>>>>>> I haven't been able to make it work so far. Here's the description of
>>>>>> some of the issues I've encountered.
>>>>>>
>>>>>> The patch does not apply cleanly so I had to make small adjustments just
>>>>>> to make it apply without changing the functionality.
>>>>>
>>>>> I have an updated version of the patch, which I intend to submit soon.
>>>>
>>>> Is it publicly available already ?
>>>
>>> Just started working on it the other day.
>>
>> I'm working on the same chip, how can we avoid effort duplication ?
> 
> I'll have something by the end of the week, latest.

Sorry, this has to wait until next week. Didn't had the time to do further
work on it this week.

- Lars

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Fri, Nov 29, 2013 at 9:05 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 11/29/2013 08:52 PM, Linus Walleij wrote:

>> I guess you may be referring to the problem of instatiating
>> a dynamic IRQ chip in *board code* and then passing the
>> obtained dynamic IRQ numbers as resources to the
>> devices also created in a board file?
>>
>
> Yes.
>
>> That would be like you're asking for a function that would
>> return the base of an irq_chip, that needs to be discussed
>> with the irq maintainers, so not much I can say, but maybe
>> I misunderstood this?
>
> I my opinion the best solution for this problem is to have the same lookup
> mechanism we've had for clocks, regulators, etc and now also GPIOs.

Hm this needs to be discussed with some irq people...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html