max9286: Improve mux-state readbility [v2]
diff mbox series

Message ID 20191129132643.6429-1-kieran.bingham@ideasonboard.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • max9286: Improve mux-state readbility [v2]
Related show

Commit Message

Kieran Bingham Nov. 29, 2019, 1:26 p.m. UTC
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(-)

Comments

Jacopo Mondi Dec. 3, 2019, 8:19 a.m. UTC | #1
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
>
Kieran Bingham Dec. 6, 2019, 1:52 p.m. UTC | #2
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
>>

Patch
diff mbox series

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;