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@xxxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxx>
>
> 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
>>
--
Regards
--
Kieran
