| Message ID | 20181210122901.14600-1-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] media: i2c: adv748x: Fix video standard selection register setting | expand |
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 >
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 >> >
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