Hi Hans,
Thanks for your work.
On 2021-03-25 11:39:37 +0100, Hans Verkuil wrote:
> While the adv7604/11/12 hardware supported EDIDs up to 4 blocks, the
> driver didn't. This patch adds support for this. It also improves support
> for EDIDs that do not have a Source Physical Address: in that case the
> spa location is set to the first byte of the second block, and the
> 'physical address' is just the two bytes at that location. This is per
> the suggestion in the adv76xx documentation for such EDIDs.
>
> Tested with an adv7604 and adv7612.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
This one works as expected,
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> Change since v2' remove ADV7604 type test, was spurious code that should
> have been removed. Thanks to Niklas for catching that!
> ---
> diff --git a/drivers/media/i2c/adv7604.c b/drivers/media/i2c/adv7604.c
> index 18184297e2f0..7547afc85eb1 100644
> --- a/drivers/media/i2c/adv7604.c
> +++ b/drivers/media/i2c/adv7604.c
> @@ -73,6 +73,8 @@ MODULE_LICENSE("GPL");
>
> #define ADV76XX_MAX_ADDRS (3)
>
> +#define ADV76XX_MAX_EDID_BLOCKS 4
> +
> enum adv76xx_type {
> ADV7604,
> ADV7611,
> @@ -108,6 +110,11 @@ struct adv76xx_chip_info {
>
> unsigned int edid_enable_reg;
> unsigned int edid_status_reg;
> + unsigned int edid_segment_reg;
> + unsigned int edid_segment_mask;
> + unsigned int edid_spa_loc_reg;
> + unsigned int edid_spa_loc_msb_mask;
> + unsigned int edid_spa_port_b_reg;
> unsigned int lcf_reg;
>
> unsigned int cable_det_mask;
> @@ -176,7 +183,7 @@ struct adv76xx_state {
> const struct adv76xx_format_info *format;
>
> struct {
> - u8 edid[256];
> + u8 edid[ADV76XX_MAX_EDID_BLOCKS * 128];
> u32 present;
> unsigned blocks;
> } edid;
> @@ -2327,15 +2334,25 @@ static int adv76xx_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
> __func__, edid->pad, state->edid.present);
> return 0;
> }
> - if (edid->blocks > 2) {
> - edid->blocks = 2;
> + if (edid->blocks > ADV76XX_MAX_EDID_BLOCKS) {
> + edid->blocks = ADV76XX_MAX_EDID_BLOCKS;
> return -E2BIG;
> }
> +
> pa = v4l2_get_edid_phys_addr(edid->edid, edid->blocks * 128, &spa_loc);
> err = v4l2_phys_addr_validate(pa, &parent_pa, NULL);
> if (err)
> return err;
>
> + if (!spa_loc) {
> + /*
> + * There is no SPA, so just set spa_loc to 128 and pa to whatever
> + * data is there.
> + */
> + spa_loc = 128;
> + pa = (edid->edid[spa_loc] << 8) | edid->edid[spa_loc + 1];
> + }
> +
> v4l2_dbg(2, debug, sd, "%s: write EDID pad %d, edid.present = 0x%x\n",
> __func__, edid->pad, state->edid.present);
>
> @@ -2344,59 +2361,63 @@ static int adv76xx_set_edid(struct v4l2_subdev *sd, struct v4l2_edid *edid)
> adv76xx_set_hpd(state, 0);
> rep_write_clr_set(sd, info->edid_enable_reg, 0x0f, 0x00);
>
> - /*
> - * Return an error if no location of the source physical address
> - * was found.
> - */
> - if (edid->blocks > 1 && spa_loc == 0)
> - return -EINVAL;
> -
> switch (edid->pad) {
> case ADV76XX_PAD_HDMI_PORT_A:
> state->spa_port_a[0] = pa >> 8;
> state->spa_port_a[1] = pa & 0xff;
> break;
> case ADV7604_PAD_HDMI_PORT_B:
> - rep_write(sd, 0x70, pa >> 8);
> - rep_write(sd, 0x71, pa & 0xff);
> + rep_write(sd, info->edid_spa_port_b_reg, pa >> 8);
> + rep_write(sd, info->edid_spa_port_b_reg + 1, pa & 0xff);
> break;
> case ADV7604_PAD_HDMI_PORT_C:
> - rep_write(sd, 0x72, pa >> 8);
> - rep_write(sd, 0x73, pa & 0xff);
> + rep_write(sd, info->edid_spa_port_b_reg + 2, pa >> 8);
> + rep_write(sd, info->edid_spa_port_b_reg + 3, pa & 0xff);
> break;
> case ADV7604_PAD_HDMI_PORT_D:
> - rep_write(sd, 0x74, pa >> 8);
> - rep_write(sd, 0x75, pa & 0xff);
> + rep_write(sd, info->edid_spa_port_b_reg + 4, pa >> 8);
> + rep_write(sd, info->edid_spa_port_b_reg + 5, pa & 0xff);
> break;
> default:
> return -EINVAL;
> }
>
> - if (info->type == ADV7604) {
> - rep_write(sd, 0x76, spa_loc & 0xff);
> - rep_write_clr_set(sd, 0x77, 0x40, (spa_loc & 0x100) >> 2);
> - } else {
> - /* ADV7612 Software Manual Rev. A, p. 15 */
> - rep_write(sd, 0x70, spa_loc & 0xff);
> - rep_write_clr_set(sd, 0x71, 0x01, (spa_loc & 0x100) >> 8);
> - }
> + if (info->edid_spa_loc_reg) {
> + u8 mask = info->edid_spa_loc_msb_mask;
>
> - if (spa_loc) {
> - edid->edid[spa_loc] = state->spa_port_a[0];
> - edid->edid[spa_loc + 1] = state->spa_port_a[1];
> + rep_write(sd, info->edid_spa_loc_reg, spa_loc & 0xff);
> + rep_write_clr_set(sd, info->edid_spa_loc_reg + 1,
> + mask, (spa_loc & 0x100) ? mask : 0);
> }
>
> + edid->edid[spa_loc] = state->spa_port_a[0];
> + edid->edid[spa_loc + 1] = state->spa_port_a[1];
> +
> memcpy(state->edid.edid, edid->edid, 128 * edid->blocks);
> state->edid.blocks = edid->blocks;
> state->aspect_ratio = v4l2_calc_aspect_ratio(edid->edid[0x15],
> edid->edid[0x16]);
> state->edid.present |= 1 << edid->pad;
>
> - err = edid_write_block(sd, 128 * edid->blocks, state->edid.edid);
> + rep_write_clr_set(sd, info->edid_segment_reg,
> + info->edid_segment_mask, 0);
> + err = edid_write_block(sd, 128 * min(edid->blocks, 2U), state->edid.edid);
> if (err < 0) {
> v4l2_err(sd, "error %d writing edid pad %d\n", err, edid->pad);
> return err;
> }
> + if (edid->blocks > 2) {
> + rep_write_clr_set(sd, info->edid_segment_reg,
> + info->edid_segment_mask,
> + info->edid_segment_mask);
> + err = edid_write_block(sd, 128 * (edid->blocks - 2),
> + state->edid.edid + 256);
> + if (err < 0) {
> + v4l2_err(sd, "error %d writing edid pad %d\n",
> + err, edid->pad);
> + return err;
> + }
> + }
>
> /* adv76xx calculates the checksums and enables I2C access to internal
> EDID RAM from DDC port. */
> @@ -2989,6 +3010,11 @@ static const struct adv76xx_chip_info adv76xx_chip_info[] = {
> .num_dv_ports = 4,
> .edid_enable_reg = 0x77,
> .edid_status_reg = 0x7d,
> + .edid_segment_reg = 0x77,
> + .edid_segment_mask = 0x10,
> + .edid_spa_loc_reg = 0x76,
> + .edid_spa_loc_msb_mask = 0x40,
> + .edid_spa_port_b_reg = 0x70,
> .lcf_reg = 0xb3,
> .tdms_lock_mask = 0xe0,
> .cable_det_mask = 0x1e,
> @@ -3039,6 +3065,8 @@ static const struct adv76xx_chip_info adv76xx_chip_info[] = {
> .num_dv_ports = 1,
> .edid_enable_reg = 0x74,
> .edid_status_reg = 0x76,
> + .edid_segment_reg = 0x7a,
> + .edid_segment_mask = 0x01,
> .lcf_reg = 0xa3,
> .tdms_lock_mask = 0x43,
> .cable_det_mask = 0x01,
> @@ -3083,6 +3111,11 @@ static const struct adv76xx_chip_info adv76xx_chip_info[] = {
> .num_dv_ports = 1, /* normally 2 */
> .edid_enable_reg = 0x74,
> .edid_status_reg = 0x76,
> + .edid_segment_reg = 0x7a,
> + .edid_segment_mask = 0x01,
> + .edid_spa_loc_reg = 0x70,
> + .edid_spa_loc_msb_mask = 0x01,
> + .edid_spa_port_b_reg = 0x52,
> .lcf_reg = 0xa3,
> .tdms_lock_mask = 0x43,
> .cable_det_mask = 0x01,
--
Regards,
Niklas Söderlund
