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

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

[Kieran Bingham]

From: Koji Matsuoka <koji.matsuoka.xm@renesas.com>

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@ideasonboard.com>
Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
[Kieran: Updated commit message, utilised BIT macro]
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 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);
 }
 
 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

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

[Niklas Söderlund]

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@renesas.com>
> 
> 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@ideasonboard.com>
> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
> [Kieran: Updated commit message, utilised BIT macro]
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  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

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

[Kieran Bingham]

Hi Niklas,

On 10/12/2018 12:55, Niklas Söderlund wrote:
> 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@renesas.com>
>>
>> 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@ideasonboard.com>
>> Signed-off-by: Koji Matsuoka <koji.matsuoka.xm@renesas.com>
>> [Kieran: Updated commit message, utilised BIT macro]
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  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 :-)


Excellent observation. I somehow assumed we were doing a straight write
here.


>   #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.

The mask is defined as:

#define ADV748X_SDP_VID_SEL_MASK        0xf0

Which indeed covers only the VID_SEL bits, and ensures that the reserved
bit is left alone.

If the hardware initialises this bit, then it will remain set. If not -
then the bit will remain unset. I think that's perfectly acceptable for
an undocumented bit, so lets drop this patch.

--
Regards

Kieran



> 
>>  }
>>  
>>  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
>>
>