| Message ID | 20191129132643.6429-1-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286: Improve mux-state readbility [v2] | expand |
Hi Kieran,
only a really minor nit, you could fix when squashing this patch
On Fri, Nov 29, 2019 at 01:26:43PM +0000, Kieran Bingham wrote:
> The MAX9286 implements an I2C mux which we maintain in an open state
> while we are streaming from the cameras.
>
> The development code for the MAX9286 uses an integer value with
> arbitrary state values to control these state transitions. This is
> highlighted with a FIXME and is difficult to interpret the meaning of the
> values 0, 1, 2.
>
> Introduce a new function call max9286_i2c_mux_open() to make it clear
> when a component opens all the mux channels, and update the usage
> in s_stream() to max9286_i2c_mux_close() the mux on stop stream.
>
> We previously had missed an occasion to sleep after an update to the I2C
> Fwd/Rev channels, so all writes to this configuration register are moved
> to a helper: max9286_i2c_mux_configure() which guarantees the delay.
>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> drivers/media/i2c/max9286.c | 74 ++++++++++++++++++-------------------
> 1 file changed, 37 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 5b8dfa652d50..b34fb31c6db5 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -144,7 +144,7 @@ struct max9286_priv {
> struct media_pad pads[MAX9286_N_PADS];
> struct regulator *regulator;
> bool poc_enabled;
> - int streaming;
> + int mux_state;
>
> struct i2c_mux_core *mux;
> unsigned int mux_channel;
> @@ -221,57 +221,59 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
> * I2C Multiplexer
> */
>
> -static int max9286_i2c_mux_close(struct max9286_priv *priv)
> +enum max9286_i2c_mux_state {
> + MAX9286_MUX_CLOSED = 0,
> + MAX9286_MUX_OPEN,
> +};
> +
> +static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf)
> +{
> + max9286_write(priv, 0x0a, conf);
> +
> + /*
> + * We must sleep after any change to the forward or reverse channel
> + * configuration.
> + */
> + usleep_range(3000, 5000);
> +}
> +
> +static void max9286_i2c_mux_open(struct max9286_priv *priv)
> +{
> + /* Open all channels on the MAX9286 */
> + max9286_i2c_mux_configure(priv, 0xff);
> +
> + priv->mux_state = MAX9286_MUX_OPEN;
> +}
> +
> +static void max9286_i2c_mux_close(struct max9286_priv *priv)
> {
> - /* FIXME: See note in max9286_i2c_mux_select() */
> - if (priv->streaming)
> - return 0;
> /*
> * Ensure that both the forward and reverse channel are disabled on the
> * mux, and that the channel ID is invalidated to ensure we reconfigure
> - * on the next select call.
> + * on the next max9286_i2c_mux_select() call.
> */
> - priv->mux_channel = -1;
> - max9286_write(priv, 0x0a, 0x00);
> - usleep_range(3000, 5000);
> + max9286_i2c_mux_configure(priv, 0x00);
>
> - return 0;
> + priv->mux_state = MAX9286_MUX_CLOSED;
> + priv->mux_channel = -1;
> }
>
> static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan)
> {
> struct max9286_priv *priv = i2c_mux_priv(muxc);
>
> - /*
> - * FIXME: This state keeping is a hack and do the job. It should
> - * be should be reworked. One option to consider is that once all
> - * cameras are programmed the mux selection logic should be disabled
> - * and all all reverse and forward channels enable all the time.
> - *
> - * In any case this logic with a int that have two states should be
> - * reworked!
> - */
> - if (priv->streaming == 1) {
> - max9286_write(priv, 0x0a, 0xff);
> - priv->streaming = 2;
> - return 0;
> - } else if (priv->streaming == 2) {
> + /* channel select is disabled when configured in the opened state. */
All other comments in this driver start with a capital letter.
Otherwise, I really like this patch! thanks!
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> + if (priv->mux_state == MAX9286_MUX_OPEN)
> return 0;
> - }
>
> if (priv->mux_channel == chan)
> return 0;
>
> priv->mux_channel = chan;
>
> - max9286_write(priv, 0x0a,
> - MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan));
> -
> - /*
> - * We must sleep after any change to the forward or reverse channel
> - * configuration.
> - */
> - usleep_range(3000, 5000);
> + max9286_i2c_mux_configure(priv,
> + MAX9286_FWDCCEN(chan) |
> + MAX9286_REVCCEN(chan));
>
> return 0;
> }
> @@ -441,8 +443,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> int ret;
>
> if (enable) {
> - /* FIXME: See note in max9286_i2c_mux_select() */
> - priv->streaming = 1;
> + max9286_i2c_mux_open(priv);
>
> /* Start all cameras. */
> for_each_source(priv, source) {
> @@ -490,8 +491,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> for_each_source(priv, source)
> v4l2_subdev_call(source->sd, video, s_stream, 0);
>
> - /* FIXME: See note in max9286_i2c_mux_select() */
> - priv->streaming = 0;
> + max9286_i2c_mux_close(priv);
> }
>
> return 0;
> --
> 2.20.1
>
Hi Jacopo, On 03/12/2019 08:19, Jacopo Mondi wrote: > Hi Kieran, > only a really minor nit, you could fix when squashing this patch > > On Fri, Nov 29, 2019 at 01:26:43PM +0000, Kieran Bingham wrote: >> The MAX9286 implements an I2C mux which we maintain in an open state >> while we are streaming from the cameras. >> >> The development code for the MAX9286 uses an integer value with >> arbitrary state values to control these state transitions. This is >> highlighted with a FIXME and is difficult to interpret the meaning of the >> values 0, 1, 2. >> >> Introduce a new function call max9286_i2c_mux_open() to make it clear >> when a component opens all the mux channels, and update the usage >> in s_stream() to max9286_i2c_mux_close() the mux on stop stream. >> >> We previously had missed an occasion to sleep after an update to the I2C >> Fwd/Rev channels, so all writes to this configuration register are moved >> to a helper: max9286_i2c_mux_configure() which guarantees the delay. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> drivers/media/i2c/max9286.c | 74 ++++++++++++++++++------------------- >> 1 file changed, 37 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c >> index 5b8dfa652d50..b34fb31c6db5 100644 >> --- a/drivers/media/i2c/max9286.c >> +++ b/drivers/media/i2c/max9286.c >> @@ -144,7 +144,7 @@ struct max9286_priv { >> struct media_pad pads[MAX9286_N_PADS]; >> struct regulator *regulator; >> bool poc_enabled; >> - int streaming; >> + int mux_state; >> >> struct i2c_mux_core *mux; >> unsigned int mux_channel; >> @@ -221,57 +221,59 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val) >> * I2C Multiplexer >> */ >> >> -static int max9286_i2c_mux_close(struct max9286_priv *priv) >> +enum max9286_i2c_mux_state { >> + MAX9286_MUX_CLOSED = 0, >> + MAX9286_MUX_OPEN, >> +}; This is now down to just two states, so I think this probably warrants being changed to a bool mux_open; I've already folded /this/ patch into the main max9286 driver, so I'll make this small fix and post it here along with the next two fixes I have. >> + >> +static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf) >> +{ >> + max9286_write(priv, 0x0a, conf); >> + >> + /* >> + * We must sleep after any change to the forward or reverse channel >> + * configuration. >> + */ >> + usleep_range(3000, 5000); >> +} >> + >> +static void max9286_i2c_mux_open(struct max9286_priv *priv) >> +{ >> + /* Open all channels on the MAX9286 */ >> + max9286_i2c_mux_configure(priv, 0xff); >> + >> + priv->mux_state = MAX9286_MUX_OPEN; >> +} >> + >> +static void max9286_i2c_mux_close(struct max9286_priv *priv) >> { >> - /* FIXME: See note in max9286_i2c_mux_select() */ >> - if (priv->streaming) >> - return 0; >> /* >> * Ensure that both the forward and reverse channel are disabled on the >> * mux, and that the channel ID is invalidated to ensure we reconfigure >> - * on the next select call. >> + * on the next max9286_i2c_mux_select() call. >> */ >> - priv->mux_channel = -1; >> - max9286_write(priv, 0x0a, 0x00); >> - usleep_range(3000, 5000); >> + max9286_i2c_mux_configure(priv, 0x00); >> >> - return 0; >> + priv->mux_state = MAX9286_MUX_CLOSED; >> + priv->mux_channel = -1; >> } >> >> static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) >> { >> struct max9286_priv *priv = i2c_mux_priv(muxc); >> >> - /* >> - * FIXME: This state keeping is a hack and do the job. It should >> - * be should be reworked. One option to consider is that once all >> - * cameras are programmed the mux selection logic should be disabled >> - * and all all reverse and forward channels enable all the time. >> - * >> - * In any case this logic with a int that have two states should be >> - * reworked! >> - */ >> - if (priv->streaming == 1) { >> - max9286_write(priv, 0x0a, 0xff); >> - priv->streaming = 2; >> - return 0; >> - } else if (priv->streaming == 2) { >> + /* channel select is disabled when configured in the opened state. */ > > All other comments in this driver start with a capital letter. Thanks, this is fixed up. > Otherwise, I really like this patch! thanks! > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > >> + if (priv->mux_state == MAX9286_MUX_OPEN) >> return 0; >> - } >> >> if (priv->mux_channel == chan) >> return 0; >> >> priv->mux_channel = chan; >> >> - max9286_write(priv, 0x0a, >> - MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan)); >> - >> - /* >> - * We must sleep after any change to the forward or reverse channel >> - * configuration. >> - */ >> - usleep_range(3000, 5000); >> + max9286_i2c_mux_configure(priv, >> + MAX9286_FWDCCEN(chan) | >> + MAX9286_REVCCEN(chan)); >> >> return 0; >> } >> @@ -441,8 +443,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) >> int ret; >> >> if (enable) { >> - /* FIXME: See note in max9286_i2c_mux_select() */ >> - priv->streaming = 1; >> + max9286_i2c_mux_open(priv); >> >> /* Start all cameras. */ >> for_each_source(priv, source) { >> @@ -490,8 +491,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) >> for_each_source(priv, source) >> v4l2_subdev_call(source->sd, video, s_stream, 0); >> >> - /* FIXME: See note in max9286_i2c_mux_select() */ >> - priv->streaming = 0; >> + max9286_i2c_mux_close(priv); >> } >> >> return 0; >> -- >> 2.20.1 >>
diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c index 5b8dfa652d50..b34fb31c6db5 100644 --- a/drivers/media/i2c/max9286.c +++ b/drivers/media/i2c/max9286.c @@ -144,7 +144,7 @@ struct max9286_priv { struct media_pad pads[MAX9286_N_PADS]; struct regulator *regulator; bool poc_enabled; - int streaming; + int mux_state; struct i2c_mux_core *mux; unsigned int mux_channel; @@ -221,57 +221,59 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val) * I2C Multiplexer */ -static int max9286_i2c_mux_close(struct max9286_priv *priv) +enum max9286_i2c_mux_state { + MAX9286_MUX_CLOSED = 0, + MAX9286_MUX_OPEN, +}; + +static void max9286_i2c_mux_configure(struct max9286_priv *priv, u8 conf) +{ + max9286_write(priv, 0x0a, conf); + + /* + * We must sleep after any change to the forward or reverse channel + * configuration. + */ + usleep_range(3000, 5000); +} + +static void max9286_i2c_mux_open(struct max9286_priv *priv) +{ + /* Open all channels on the MAX9286 */ + max9286_i2c_mux_configure(priv, 0xff); + + priv->mux_state = MAX9286_MUX_OPEN; +} + +static void max9286_i2c_mux_close(struct max9286_priv *priv) { - /* FIXME: See note in max9286_i2c_mux_select() */ - if (priv->streaming) - return 0; /* * Ensure that both the forward and reverse channel are disabled on the * mux, and that the channel ID is invalidated to ensure we reconfigure - * on the next select call. + * on the next max9286_i2c_mux_select() call. */ - priv->mux_channel = -1; - max9286_write(priv, 0x0a, 0x00); - usleep_range(3000, 5000); + max9286_i2c_mux_configure(priv, 0x00); - return 0; + priv->mux_state = MAX9286_MUX_CLOSED; + priv->mux_channel = -1; } static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) { struct max9286_priv *priv = i2c_mux_priv(muxc); - /* - * FIXME: This state keeping is a hack and do the job. It should - * be should be reworked. One option to consider is that once all - * cameras are programmed the mux selection logic should be disabled - * and all all reverse and forward channels enable all the time. - * - * In any case this logic with a int that have two states should be - * reworked! - */ - if (priv->streaming == 1) { - max9286_write(priv, 0x0a, 0xff); - priv->streaming = 2; - return 0; - } else if (priv->streaming == 2) { + /* channel select is disabled when configured in the opened state. */ + if (priv->mux_state == MAX9286_MUX_OPEN) return 0; - } if (priv->mux_channel == chan) return 0; priv->mux_channel = chan; - max9286_write(priv, 0x0a, - MAX9286_FWDCCEN(chan) | MAX9286_REVCCEN(chan)); - - /* - * We must sleep after any change to the forward or reverse channel - * configuration. - */ - usleep_range(3000, 5000); + max9286_i2c_mux_configure(priv, + MAX9286_FWDCCEN(chan) | + MAX9286_REVCCEN(chan)); return 0; } @@ -441,8 +443,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) int ret; if (enable) { - /* FIXME: See note in max9286_i2c_mux_select() */ - priv->streaming = 1; + max9286_i2c_mux_open(priv); /* Start all cameras. */ for_each_source(priv, source) { @@ -490,8 +491,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable) for_each_source(priv, source) v4l2_subdev_call(source->sd, video, s_stream, 0); - /* FIXME: See note in max9286_i2c_mux_select() */ - priv->streaming = 0; + max9286_i2c_mux_close(priv); } return 0;
The MAX9286 implements an I2C mux which we maintain in an open state while we are streaming from the cameras. The development code for the MAX9286 uses an integer value with arbitrary state values to control these state transitions. This is highlighted with a FIXME and is difficult to interpret the meaning of the values 0, 1, 2. Introduce a new function call max9286_i2c_mux_open() to make it clear when a component opens all the mux channels, and update the usage in s_stream() to max9286_i2c_mux_close() the mux on stop stream. We previously had missed an occasion to sleep after an update to the I2C Fwd/Rev channels, so all writes to this configuration register are moved to a helper: max9286_i2c_mux_configure() which guarantees the delay. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- drivers/media/i2c/max9286.c | 74 ++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 37 deletions(-)