| Message ID | 20200409121202.11130-12-kieran.bingham+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286 v8 - modifications | expand |
Hi Kieran, On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote: > Provide a function to control setting of the overlap window, but disable > it by default. > > The function will allow the value to be easily updated in the future, > either statically in the code, or via an external control mechanism. > > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > index 008a93910300..61178ae363d6 100644 > --- a/drivers/media/i2c/max9286.c > +++ b/drivers/media/i2c/max9286.c > @@ -118,6 +118,9 @@ > #define MAX9286_REV_FLEN(n) ((n) - 20) > /* Register 0x49 */ > #define MAX9286_VIDEO_DETECT_MASK 0x0f > +/* Register 0x64 */ > +#define MAX9286_ENFSINLAST BIT(5) > +#define MAX9286_OVLP_WINDOWH_MASK GENMASK(4, 0) > /* Register 0x69 */ > #define MAX9286_LFLTBMONMASKED BIT(7) > #define MAX9286_LOCKMONMASKED BIT(6) > @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > return 0; > } > > +/* > + * The overlap window is a 13 bit value with the low byte in register 0x63 and > + * the high byte(5bits) stored in the least significant bits of register 0x64. ^ space But I'm not sure this is useful comment. The register layout is actually the only documented thing of this register :) > + */ > +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window) > +{ > + int ret; > + u8 val; > + > + ret = max9286_read(priv, 0x64); > + if (ret < 0) > + return -EIO; > + > + max9286_write(priv, 0x63, window & 0xff); > + > + /* > + * Process the high byte, while preserve existing bits set in 0x64. > + * TODO: Convert this all to regmap so we can utilise regmap_update_bits > + */ > + window >>= 8; > + val = ret & ~MAX9286_OVLP_WINDOWH_MASK; > + val |= window & MAX9286_OVLP_WINDOWH_MASK; > + > + max9286_write(priv, 0x64, val); > + > + return 0; > +} > + > static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate) > { > if (!priv->pixelrate) > @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv) > max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS | > MAX9286_HVSRC_D14); > > + /* > + * The overlap window seems to provide additional validation by tracking > + * the delay between vsync and frame sync, generating an error if the > + * delay is bigger than the programmed window, though it's not yet clear > + * what value should be set. > + * > + * As it's an optional value and can be disabled, we do so by setting > + * a 0 overlap value. > + */ This is useful instead :) > + max9286_set_overlap_window(priv, 0); > + > /* > * Wait for 2ms to allow the link to resynchronize after the > * configuration change. > -- > 2.20.1 >
Hi Jacopo, On 09/04/2020 17:32, Jacopo Mondi wrote: > Hi Kieran, > > On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote: >> Provide a function to control setting of the overlap window, but disable >> it by default. >> >> The function will allow the value to be easily updated in the future, >> either statically in the code, or via an external control mechanism. >> >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> >> --- >> drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >> index 008a93910300..61178ae363d6 100644 >> --- a/drivers/media/i2c/max9286.c >> +++ b/drivers/media/i2c/max9286.c >> @@ -118,6 +118,9 @@ >> #define MAX9286_REV_FLEN(n) ((n) - 20) >> /* Register 0x49 */ >> #define MAX9286_VIDEO_DETECT_MASK 0x0f >> +/* Register 0x64 */ >> +#define MAX9286_ENFSINLAST BIT(5) >> +#define MAX9286_OVLP_WINDOWH_MASK GENMASK(4, 0) >> /* Register 0x69 */ >> #define MAX9286_LFLTBMONMASKED BIT(7) >> #define MAX9286_LOCKMONMASKED BIT(6) >> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) >> return 0; >> } >> >> +/* >> + * The overlap window is a 13 bit value with the low byte in register 0x63 and >> + * the high byte(5bits) stored in the least significant bits of register 0x64. > ^ space > > But I'm not sure this is useful comment. The register layout is > actually the only documented thing of this register :) Sure, but readers won't necessarily have the documentation :-) I think I was just trying to explain what was happening because we have so many unidentified registers (i.e. they're all just numbers, so we can't do, max9286_write(priv, MAX9286_OVERLAP_LOW, overlap & 0xff); max9286_write(priv, MAX9286_OVERLAP_HIGH, (overlap >> SHIFT) & MASK); I can drop this comment all the same, although I'd like to document clearly that it's a 13-bit value as an input, (not a full 16 bit) Will this be better suited? /* * The overlap window is a 13 bit value stored across two registers. * The definition and units of the value is undocumented. */ >> + */ >> +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window) >> +{ >> + int ret; >> + u8 val; >> + >> + ret = max9286_read(priv, 0x64); >> + if (ret < 0) >> + return -EIO; >> + >> + max9286_write(priv, 0x63, window & 0xff); >> + >> + /* >> + * Process the high byte, while preserve existing bits set in 0x64. >> + * TODO: Convert this all to regmap so we can utilise regmap_update_bits >> + */ >> + window >>= 8; >> + val = ret & ~MAX9286_OVLP_WINDOWH_MASK; >> + val |= window & MAX9286_OVLP_WINDOWH_MASK; >> + >> + max9286_write(priv, 0x64, val); >> + >> + return 0; >> +} >> + >> static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate) >> { >> if (!priv->pixelrate) >> @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv) >> max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS | >> MAX9286_HVSRC_D14); >> >> + /* >> + * The overlap window seems to provide additional validation by tracking >> + * the delay between vsync and frame sync, generating an error if the >> + * delay is bigger than the programmed window, though it's not yet clear >> + * what value should be set. >> + * >> + * As it's an optional value and can be disabled, we do so by setting >> + * a 0 overlap value. >> + */ > > This is useful instead :) :-) >> + max9286_set_overlap_window(priv, 0); >> + >> /* >> * Wait for 2ms to allow the link to resynchronize after the >> * configuration change. >> -- >> 2.20.1 >>
Hi Kieran, On Fri, Apr 10, 2020 at 08:14:18AM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 09/04/2020 17:32, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Thu, Apr 09, 2020 at 01:12:00PM +0100, Kieran Bingham wrote: > >> Provide a function to control setting of the overlap window, but disable > >> it by default. > >> > >> The function will allow the value to be easily updated in the future, > >> either statically in the code, or via an external control mechanism. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > >> --- > >> drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 42 insertions(+) > >> > >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c > >> index 008a93910300..61178ae363d6 100644 > >> --- a/drivers/media/i2c/max9286.c > >> +++ b/drivers/media/i2c/max9286.c > >> @@ -118,6 +118,9 @@ > >> #define MAX9286_REV_FLEN(n) ((n) - 20) > >> /* Register 0x49 */ > >> #define MAX9286_VIDEO_DETECT_MASK 0x0f > >> +/* Register 0x64 */ > >> +#define MAX9286_ENFSINLAST BIT(5) > >> +#define MAX9286_OVLP_WINDOWH_MASK GENMASK(4, 0) > >> /* Register 0x69 */ > >> #define MAX9286_LFLTBMONMASKED BIT(7) > >> #define MAX9286_LOCKMONMASKED BIT(6) > >> @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) > >> return 0; > >> } > >> > >> +/* > >> + * The overlap window is a 13 bit value with the low byte in register 0x63 and > >> + * the high byte(5bits) stored in the least significant bits of register 0x64. > > ^ space > > > > But I'm not sure this is useful comment. The register layout is > > actually the only documented thing of this register :) > > Sure, but readers won't necessarily have the documentation :-) I think I > was just trying to explain what was happening because we have so many > unidentified registers (i.e. they're all just numbers, so we can't do, > > max9286_write(priv, MAX9286_OVERLAP_LOW, overlap & 0xff); > max9286_write(priv, MAX9286_OVERLAP_HIGH, (overlap >> SHIFT) & MASK); > > I can drop this comment all the same, although I'd like to document > clearly that it's a 13-bit value as an input, (not a full 16 bit) > > Will this be better suited? > > > /* > * The overlap window is a 13 bit value stored across two registers. > * The definition and units of the value is undocumented. > */ Oh no worries, if you want to keep that comment the first version is perfectly fine.
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 008a93910300..61178ae363d6 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -118,6 +118,9 @@ #define MAX9286_REV_FLEN(n) ((n) - 20) /* Register 0x49 */ #define MAX9286_VIDEO_DETECT_MASK 0x0f +/* Register 0x64 */ +#define MAX9286_ENFSINLAST BIT(5) +#define MAX9286_OVLP_WINDOWH_MASK GENMASK(4, 0) /* Register 0x69 */ #define MAX9286_LFLTBMONMASKED BIT(7) #define MAX9286_LOCKMONMASKED BIT(6) @@ -632,6 +635,34 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) return 0; } +/* + * The overlap window is a 13 bit value with the low byte in register 0x63 and + * the high byte(5bits) stored in the least significant bits of register 0x64. + */ +static int max9286_set_overlap_window(struct max9286_priv *priv, u16 window) +{ + int ret; + u8 val; + + ret = max9286_read(priv, 0x64); + if (ret < 0) + return -EIO; + + max9286_write(priv, 0x63, window & 0xff); + + /* + * Process the high byte, while preserve existing bits set in 0x64. + * TODO: Convert this all to regmap so we can utilise regmap_update_bits + */ + window >>= 8; + val = ret & ~MAX9286_OVLP_WINDOWH_MASK; + val |= window & MAX9286_OVLP_WINDOWH_MASK; + + max9286_write(priv, 0x64, val); + + return 0; +} + static int max9286_set_pixelrate(struct max9286_priv *priv, s64 rate) { if (!priv->pixelrate) @@ -942,6 +973,17 @@ static int max9286_setup(struct max9286_priv *priv) max9286_write(priv, 0x0c, MAX9286_HVEN | MAX9286_INVVS | MAX9286_HVSRC_D14); + /* + * The overlap window seems to provide additional validation by tracking + * the delay between vsync and frame sync, generating an error if the + * delay is bigger than the programmed window, though it's not yet clear + * what value should be set. + * + * As it's an optional value and can be disabled, we do so by setting + * a 0 overlap value. + */ + max9286_set_overlap_window(priv, 0); + /* * Wait for 2ms to allow the link to resynchronize after the * configuration change.
Provide a function to control setting of the overlap window, but disable it by default. The function will allow the value to be easily updated in the future, either statically in the code, or via an external control mechanism. Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> --- drivers/media/i2c/max9286.c | 42 +++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)