Hi Valentine,
On Tue, 24 Sep 2013, Valentine Barshak wrote:
> This adds ADV7611/ADV7612 Dual Port Xpressview
> 225 MHz HDMI Receiver support.
>
> The code is based on the ADV7604 driver, and ADV7612 patch
> by Shinobu Uehara <shinobu.uehara.xc@xxxxxxxxxxx>
>
> Signed-off-by: Valentine Barshak <valentine.barshak@xxxxxxxxxxxxxxxxxx>
IIRC, Laurent is reviewing all new media I2C drivers, I added him to cc. A
couple of minor comments from me too, while at it.
> ---
> drivers/media/i2c/Kconfig | 11 +
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/adv761x.c | 1060 +++++++++++++++++++++++++++++++++++++++++++
> include/media/adv761x.h | 28 ++
> 4 files changed, 1100 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 d18be19..8eede00 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 Dual Port Xpressview HDMI Receiver.
Are you sure this driver can handle all adv761x devices? One of the
differences even between 7611 and 7612 is, that only 7612 is dual-port,
7611 is single-port. What about 7619?
> +
> + 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 9f462df..393eb0c 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..bc3dfc3
> --- /dev/null
> +++ b/drivers/media/i2c/adv761x.c
> @@ -0,0 +1,1060 @@
> +/*
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/videodev2.h>
> +#include <media/adv761x.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
> +
> +static int debug;
> +module_param(debug, int, 0644);
> +MODULE_PARM_DESC(debug, "debug level (0-2)");
> +
> +/* I2C map addresses */
> +static u8 i2c_cec = 0x40;
> +module_param(i2c_cec, byte, 0644);
> +MODULE_PARM_DESC(i2c_cec, "CEC map 7-bit I2C address (default: 0x40)");
> +
> +static u8 i2c_inf = 0x3e;
> +module_param(i2c_inf, byte, 0644);
> +MODULE_PARM_DESC(i2c_inf, "InfoFrame map 7-bit I2C address (default: 0x3e)");
> +
> +static u8 i2c_dpll = 0x26;
> +module_param(i2c_dpll, byte, 0644);
> +MODULE_PARM_DESC(i2c_dpll, "DPLL map 7-bit I2C address (default: 0x20)");
> +
> +static u8 i2c_rep = 0x32;
> +module_param(i2c_rep, byte, 0644);
> +MODULE_PARM_DESC(i2c_rep, "Repeater map 7-bit I2C address (default: 0x32)");
> +
> +static u8 i2c_edid = 0x36;
> +module_param(i2c_edid, byte, 0644);
> +MODULE_PARM_DESC(i2c_edid, "EDID map 7-bit I2C address (default: 0x36)");
> +
> +static u8 i2c_hdmi = 0x34;
> +module_param(i2c_hdmi, byte, 0644);
> +MODULE_PARM_DESC(i2c_hdmi, "HDMI map 7-bit I2C address (default: 0x34)");
> +
> +static u8 i2c_cp = 0x22;
> +module_param(i2c_cp, byte, 0644);
> +MODULE_PARM_DESC(i2c_cp, "CP map 7-bit I2C address (default: 0x22)");
Using module parameters has a disadvantage, that all instances of this
driver will get the same values, and it is quite possible to have several
HDMI receivers in a system, I believe? Wouldn't it be better to pass these
addresses from the platform data / DT?
> +
> +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;
> + const struct adv761x_color_format *cfmt;
> + u32 width;
> + u32 height;
> + enum v4l2_field scanmode;
> +
> + struct workqueue_struct *work_queue;
> + struct delayed_work enable_hotplug;
> +
> + /* I2C clients */
> + struct i2c_client *i2c_cec;
> + struct i2c_client *i2c_infoframe;
> + struct i2c_client *i2c_dpll;
> + struct i2c_client *i2c_repeater;
> + 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);
Uhm, why do you need to do this 3 times?... I see adv7842 does that too...
but e.g. adv7604 dies this only for writing and not for reading...
> + if (ret >= 0)
> + return ret;
> + }
> +
> + v4l_err(client, "error reading addr:%02x reg:%02x\n",
> + 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);
ditto
> + if (!ret)
> + return 0;
> + }
> +
> + v4l_err(client, "error writing addr:%02x reg:%02x val:%02x\n",
> + client->addr, command, value);
> + return ret;
> +}
[snip]
> +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();
This doesn't look pretty to me. Other drivers use at least an msleep(1)
here, which doesn't look particularly exciting, but at least it makes some
sense. Whereas a call to schedule() here doesn't really do much, I think.
> + }
> +
> + v4l_err(client, "error enabling edid\n");
> + return -EIO;
> +}
[snip]
> +static int adv761x_s_mbus_fmt(struct v4l2_subdev *sd,
> + struct v4l2_mbus_framefmt *mf)
> +{
> + struct adv761x_state *state = to_state(sd);
> + int i, ret;
> + u8 progressive;
> + u32 width;
> + u32 height;
> +
> + for (i = 0; i < ARRAY_SIZE(adv761x_cfmts); i++) {
> + if (mf->code == adv761x_cfmts[i].code) {
> + state->cfmt = &adv761x_cfmts[i];
> + break;
> + }
> + }
> + if (i >= ARRAY_SIZE(adv761x_cfmts))
> + return -EINVAL;
> +
> + /* Get video information */
> + ret = adv761x_get_vid_info(sd, &progressive, &width, &height);
> + if (ret < 0) {
> + width = ADV761X_MAX_WIDTH;
> + height = ADV761X_MAX_HEIGHT;
> + progressive = 1;
> + }
> +
> + state->width = width;
> + state->height = height;
> + state->scanmode = (progressive) ?
Superfluous parenthesis
> + V4L2_FIELD_NONE : V4L2_FIELD_INTERLACED;
> +
> + mf->width = state->width;
> + mf->height = state->height;
> + mf->code = state->cfmt->code;
> + mf->field = state->scanmode;
> + mf->colorspace = state->cfmt->colorspace;
> +
> + return 0;
> +}
[snip]
> +static struct i2c_client *adv761x_dummy_client(struct v4l2_subdev *sd,
> + u8 addr, u8 io_reg)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> +
> + io_write(sd, io_reg, addr << 1);
> +
> + return i2c_new_dummy(client->adapter, io_read(sd, io_reg) >> 1);
Do you really have to re-read? Cannot you just use addr?
> +}
[snip]
> +/* Power management operations */
> +#ifdef CONFIG_PM_SLEEP
> +static int adv761x_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +
> + /* Power off */
> + return io_write(sd, 0x0c, 0x62);
> +}
> +
> +static int adv761x_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +
> + /* Initializes ADV761X to its default values */
> + return adv761x_core_init(sd);
What if a system was suspended during capture? Is this enough to resume it
automatically?
> +}
> +
> +static SIMPLE_DEV_PM_OPS(adv761x_pm_ops, adv761x_suspend, adv761x_resume);
> +#define ADV761X_PM_OPS (&adv761x_pm_ops)
> +#else /* CONFIG_PM_SLEEP */
> +#define ADV761X_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static const struct i2c_device_id adv761x_id[] = {
> + { ADV761X_DRIVER_NAME, 0 },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, adv761x_id);
> +
> +static struct i2c_driver adv761x_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = ADV761X_DRIVER_NAME,
> + .pm = ADV761X_PM_OPS,
> + },
> + .probe = adv761x_probe,
> + .remove = adv761x_remove,
> + .id_table = adv761x_id,
> +};
> +
> +module_i2c_driver(adv761x_driver);
> +
> +MODULE_DESCRIPTION("HDMI Receiver ADV761X video decoder driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/media/adv761x.h b/include/media/adv761x.h
> new file mode 100644
> index 0000000..e6e6aea
> --- /dev/null
> +++ b/include/media/adv761x.h
> @@ -0,0 +1,28 @@
> +/*
> + * 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 may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + */
> +
> +#ifndef _ADV761X_H_
> +#define _ADV761X_H_
> +
> +/* notify events */
> +#define ADV761X_HOTPLUG 1
Is this header needed at all and - if so - does it have to be exported
under include/ or would it be enough to put it under drivers/media/?
> +
> +#endif /* _ADV761X_H_ */
> --
> 1.8.3.1
>
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
