Re: [PATCH v2] media: i2c: adv748x: Fix video standard selection register setting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Koji-san, Kieran(-san),

Thanks for your work.

On 2018-12-10 12:29:01 +0000, Kieran Bingham wrote:
> From: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx>
> 
> The ADV7481 Register Control Manual states that bit 2 in the Video
> Standard Selection register is reserved with the value of 1.
> 
> The bit is otherwise undocumented, and currently cleared by the driver
> when setting the video standard selection.
> 
> Define the bit as reserved, and ensure that it is always set when
> writing to the SDP_VID_SEL register.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@xxxxxxxxxxx>
> [Kieran: Updated commit message, utilised BIT macro]
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c | 3 ++-
>  drivers/media/i2c/adv748x/adv748x.h     | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index 71714634efb0..c4d9ffc50702 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -151,7 +151,8 @@ static void adv748x_afe_set_video_standard(struct adv748x_state *state,
>  					  int sdpstd)
>  {
>  	sdp_clrset(state, ADV748X_SDP_VID_SEL, ADV748X_SDP_VID_SEL_MASK,
> -		   (sdpstd & 0xf) << ADV748X_SDP_VID_SEL_SHIFT);
> +		   (sdpstd & 0xf) << ADV748X_SDP_VID_SEL_SHIFT |
> +		   ADV748X_SDP_VID_RESERVED_BIT);

Is this really needed? In practice the adv748x driver never touches the 
reserved bit and this special handling *should* not be needed :-)

  #define sdp_clrset(s, r, m, v) sdp_write(s, r, (sdp_read(s, r) & ~m) | v)

The full 'user_map_rw_reg_02' register where the upper 4 bits are 
vid_sel subregister is read and masked. Then the value is updated with 
the new vid_sel value and written back.

However if this is needed or fixes a real bug I'm not against this 
change but in such case I feel the mask should be updated to reflect 
which bits are touched.

>  }
>  
>  static int adv748x_afe_s_input(struct adv748x_afe *afe, unsigned int input)
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index b482c7fe6957..778aa55a741a 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -265,6 +265,7 @@ struct adv748x_state {
>  #define ADV748X_SDP_INSEL		0x00	/* user_map_rw_reg_00 */
>  
>  #define ADV748X_SDP_VID_SEL		0x02	/* user_map_rw_reg_02 */
> +#define ADV748X_SDP_VID_RESERVED_BIT	BIT(2)	/* undocumented reserved bit */
>  #define ADV748X_SDP_VID_SEL_MASK	0xf0
>  #define ADV748X_SDP_VID_SEL_SHIFT	4
>  
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux