Hi Kieran,
On Fri, Nov 29, 2019 at 11:13:00AM +0000, Kieran Bingham wrote:
> Hi Jacopo,
>
> Thanks for reviewing this :D
>
> On 29/11/2019 09:14, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Thu, Nov 28, 2019 at 04:27:06PM +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 ith a FIXME and is difficult to interpret the meaning of the
> >
> > s/ith/with
>
> ack.
>
> >
> >> values 0, 1, 2.
> >>
> >> Introduce an enum to declare the intent of each state, and comment the
> >> states accordingly.
> >>
> >> This state transition is only needed for the multi-channel support, and
> >> will not be included in the single-channel max9286 posting.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
> >> ---
> >> drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++--------------
> >> 1 file changed, 40 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> >> index eed00ff1dee4..a9c3e7411bda 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,16 +221,39 @@ static int max9286_write(struct max9286_priv *priv, u8 reg, u8 val)
> >> * I2C Multiplexer
> >> */
> >>
> >> +enum max9286_i2c_mux_state {
> >
> > Let the bikeshedding begin here
> >
> >> + /*
> >> + * The I2C Mux will enable only a single channel (both forward, and
> >
> > s/Mux/mux
>
> Ack.
>
> >
> >> + * reverse) at a time, and to reduce I2C bus bandwidth, it will only
> >> + * reconfigure the channel when a write is requested to a different
> >> + * channel.
> >
> > I won't here explain what a mux channel select does
>
> I was trying to explain what /this state/ does ...
> I can streamline this.
>
> >
> >> + */
> >> + MAX9286_I2C_MUX_STATE_CHANNEL = 0,
> >
> > To me, this should be the initial state of the mux, where all channels
> > are closed.
> >
>
> I actually started with a _CLOSED here, but I determined that _CLOSED
> was redundant, as _CLOSED is simply _CHANNEL with a channel id of -1.
>
> And when in _CLOSED, the next 'write' should be of type _CHANNEL, as in
> it should configure only a single channel, and open only that channel.
>
>
> > The state machine to me should look like:
> >
> > init() -> i2c_mux_close() -> mux_state = CLOSED;
> > all transaction selects/deselect a single channel>
> > s_stream(1) -> mux_state = REQUEST_OPEN
>
> I also had a REQUEST_OPEN, but I deemed it also to be a bit redundant,
> as the external (not mux components) which adapt the mux_state should
> only care about two states - Either it's open or on channel.
>
> I almost wonder if I should put in a helper function to make mux_state
> private to the i2c_mux functions ... but I think that's overkill.
>
>
> > first transaction opens all channels -> mux_state = OPEN
> > all successive transactions with (mux_state = OPEN) are nop
> >
> > s_stream(0) -> i2c_mux_close() -> mux_state = CLOSED
> > all transaction selects/deselect a single channel until next s_stream(1)
> >
> > For this state I propose MAX9286_I2C_MUX_STATE_CLOSED
> >
> >> +
> >> + /*
> >> + * The I2C mux will be configured with all ports open. All I2C writes
> >> + * will be transmitted to all remote I2C devices, and where multiple
> >> + * devices have the same address, the write will be received by all of
> >> + * them.
> >> + */
> >> + MAX9286_I2C_MUX_STATE_OPEN,
> >
> > I propose MAX9286_I2C_MUX_STATE_REQUEST_OPEN
> >
> >> +
> >> + /*
> >> + * The I2C mux is configured with all ports open.
> >> + *
> >> + * No reconfiguration of the I2C mux channel select is required.
> >> + */
> >> + MAX9286_I2C_MUX_STATE_DISABLE_SELECT,
> >
> > I propose MAX9286_I2C_MUX_STATE_OPEN
>
> I had this as 'MUX_STATE_OPENED', but it felt like what it was really
> doing was just 'disabling select' operations, hence I renamed it.
>
> It's also 'internal' and I wouldn't expect the no nmax9286_i2c_mux_
> functions to reference this enum value.
>
> My further reasoning to keep this as DISABLE_SELECT is that ifsomeone
> set this state (not that anyone ever should), when the mux was closed -
> it would remain closed.
>
Ok, let's see what other thinks... anyway, that's your code so if you
feel strong about this, I'm fine with what you have
>
> > Could all these be shorten to MAX9286_MUX_.... ?
>
> I think so, I was just following the function naming.
>
>
> >> +};
> >> +
> >> static int max9286_i2c_mux_close(struct max9286_priv *priv)
> >> {
> >> - /* FIXME: See note in max9286_i2c_mux_select() */
> >> - if (priv->streaming)
> >> - return 0;
> >
> > I don't get why we had this one here. Do you agree it was not
> > necessary ? I guess so, since you dropped it...
>
>
> Exactly, I couldn't see any reason for this to be here, and I also
> couldn't see it being used, as _close It doesn't get called after an
> s_stream operation as far as I can tell. It's only currently closed
> during _probe and _init.
>
> However, if at some other point, someone wanted to call _close, I would
> expect it to close all of the channels.
>
> >
> >> /*
> >> * 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.
> >> + * mux, and that the channel state and ID is invalidated to ensure we
> >> + * reconfigure on the next max9286_i2c_mux_select() call.
> >> */
> >> + priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
>
> Note here that we set the mux_channel to -1, and thus the state is set
> to _CHANNEL as discussed above, because on the next operation - we
> expect either the write to go to the specific channel, /or/ if someone
> has set MAX9286_I2C_MUX_STATE_OPEN explicitly the select call will send
> it to all channels.
>
> Those are the only two options as far as I can tell, so adding extra
> states of '_CLOSED' seems redundant, and adds unecessary complexity to me.
_CLOSED was meant to replace _CHANNEL in my proposal.
No worries though
>
> >> priv->mux_channel = -1;
> >> max9286_write(priv, 0x0a, 0x00);
> >> usleep_range(3000, 5000);
> >> @@ -242,23 +265,19 @@ 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;
> >> + /* channel select is disabled when configured in the opened state. */
> >
> > Channel
>
> Ack.
>
>
> >
> >> + if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT)
> >> return 0;
> >> - } else if (priv->streaming == 2) {
> >> +
> >> + if (priv->mux_state == MAX9286_I2C_MUX_STATE_OPEN) {
> >> + /* Open all channels on the MAX9286 */
> >> + max9286_write(priv, 0x0a, 0xff);
> >> + priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT;
> >
> > Shouldn't we sleep 3-5ms when changing the forward/reverse channel
> > configuration ?
>
> Based on the comments below, we probably do - and this has been missing.
>
Yes, was not there in first place
> >
> >> return 0;
> >> }
> >>
> >> + /* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */
> >> +
> >
> > Empty line
> > Do you need this comment ?
>
> I wanted to clarify that of the 3 states, the lines above explicitly
> handle the MAX9286_I2C_MUX_STATE_DISABLE_SELECT, and the
> MAX9286_I2C_MUX_STATE_OPEN states, so it's left 'implicit' that the code
> below is MAX9286_I2C_MUX_STATE_CHANNEL.
>
> I added the comment to make it more explicit.
>
> I didn't want to move all the code into a switch statement which would
> be my only alternative otherwise I think.
>
>
>
>
> >> if (priv->mux_channel == chan)
> >> return 0;
> >>
> >> @@ -441,8 +460,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;
> >> + priv->mux_state = MAX9286_I2C_MUX_STATE_OPEN;
> >>
> >> /* Start all cameras. */
> >> for_each_source(priv, source) {
> >> @@ -490,8 +508,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;
> >> + priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL;
> >
> > Shouldn't we call i2c_mux_close() here, and let it close all channels
> > and reset the mux state ? If the mux is not closed by writing
> > 0x0a = 0x00 but the state is here reset to STATE_CHANNEL all
> > successive i2c_mux_select() call will re-open channel-by-channel a mux
> > that is already open, won't they ?
>
> I have not modified the actual state transitions from your original
> implementation, so I think you're the expert here. This is your code,
> just renamed.
I know, this was a question not strictly related to your changes
>
> (Ok perhaps that's not true, I removed the state check at
> max9286_i2c_mux_close above)
>
> So - thinking it through ... Yes, you are right - this will leave all
> the channels open. This is the behaviour we had before this patch though
> so I wonder if this could explain any of the issues we've had.
>
> I don't /think/ so - because
> A) we probably reset the board a lot,
> B) I don't think we've had issues starting and stopping streams, but
> we haven't done enough testing there.
Also, it won't hurt to have the mux open all the time after we have
configured addresses properly. It just does not feel 'right'
>
>
>
> > Overall, I very much agree we need this patch, so thanks for having
> > addressed it!
>
> No problem, I needed to go through to understand what the three states
> (0, 1, 2) were, so this is what I came up with.
>
> Thanks for your comments, I'll await any further comments to the above
> then do a respin before collapsing it into the multi-stream support patch.
>
> Or do you think we should keep things as separate patches on top of the
> 'single' camera support? I don't want to end up in a GMSL==100 patches
> to track case again if we can avoid it .., So I'd like to keep it down
> to three managable topics :
>
> Patch 1) A single camera support, (should apply and run on linux-media)
> Patch 2) Support for multiple streams (requires v4l2-mux)
> Patch 3) Support for 2 MAX9286 on one bus (not upstreamable currently)
I very much agree with this plan!
Thanks
j
>
> --
> Kieran
>
>
> > Thanks
> > j
> >
> >> }
> >>
> >> return 0;
> >> --
> >> 2.20.1
> >>
Attachment:
signature.asc
Description: PGP signature
