On 09/25/2013 12:21 PM, Laurent Pinchart wrote:
> Hi Valentine,
>
> Thank you for the patch.
>
> Please see below for a couple of comments (in addition to Hans' and Guennadi's
> comments).
>
> On Tuesday 24 September 2013 17:38:34 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>
>> ---
>> 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
>
> [snip]
>
>> 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
>
> [snip]
>
>> +/* Supported color format list */
>> +static const struct adv761x_color_format adv761x_cfmts[] = {
>> + {
>> + .code = V4L2_MBUS_FMT_RGB888_1X24,
>> + .colorspace = V4L2_COLORSPACE_SRGB,
>> + },
>> +};
>
> Do you plan to add support for more formats later ?
>
> [snip]
>
>> +static inline int edid_write_block(struct v4l2_subdev *sd,
>> + unsigned len, const u8 *val)
>
> I would pass a pointer to adv761x_state to the internal functions instead of
> getting it from the subdev pointer each time, but that's up to you.
>
>> +{
>> + struct i2c_client *client = v4l2_get_subdevdata(sd);
>> + struct adv761x_state *state = to_state(sd);
>> + int ret = 0;
>> + int i;
>
> i is used as an unsigned number, please make it unsigned int. Same comment for
> all the locations below where i is used as unsigned.
>
>> +
>> + v4l2_dbg(2, debug, sd, "writing EDID block (%d bytes)\n", len);
>> +
>> + v4l2_subdev_notify(sd, ADV761X_HOTPLUG, (void *)0);
>
> This means that the master V4L2 driver will need to handle ADV761x-specific
> events, which doesn't sound very good. What do you use the event for ? Could
> you create a standard interface for this ?
>
>> + /* Disable I2C access to internal EDID ram from DDC port */
>> + rep_write(sd, 0x74, 0x0);
>
> Could you #define constants for register addresses and values instead of
> hardcoding them ?
These days I would probably use the regmap API instead.
That said, I've always hated using defines for register addresses since all the
datasheets are always organized around addresses, not names. Using defines means
I need to do a double-lookup: from define to address, from address to the right
database page that documents it.
I'd rather see a useful comment.
>
>> + 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();
>
> I haven't checked the datasheet, but can't the chip generate an interrupt that
> could replace the busy-loop ?
There isn't one in the adv7604, so I assume it's the same for this chip.
>
>> + }
>> +
>> + v4l_err(client, "error enabling edid\n");
>> + return -EIO;
>> +}
Regards,
Hans
--
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
