| Message ID | 20191128162706.704-1-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | max9286: Improve mux-state readbility | expand |
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 > 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@ideasonboard.com> > --- > 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 > + * 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 > + */ > + MAX9286_I2C_MUX_STATE_CHANNEL = 0, To me, this should be the initial state of the mux, where all channels are closed. 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 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 Could all these be shorten to MAX9286_MUX_.... ? > +}; > + > 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... > /* > * 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; > 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 > + 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 ? > return 0; > } > > + /* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */ > + Empty line Do you need this comment ? > 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 ? Overall, I very much agree we need this patch, so thanks for having addressed it! Thanks j > } > > return 0; > -- > 2.20.1 >
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@ideasonboard.com> >> --- >> 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. > 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. >> 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. > >> 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. (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. > 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) -- Kieran > Thanks > j > >> } >> >> return 0; >> -- >> 2.20.1 >>
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@ideasonboard.com> > >> --- > >> 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 > >>
Hi Jacopo, On 29/11/2019 11:35, Jacopo Mondi wrote: > 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@ideasonboard.com> >>>> --- >>>> 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 Ok, >>> 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 Ah I see, as in just a different name. My concern with _CLOSED though is that we do not close the channel after each write, and _CLOSED could potentially give the impression that we do ... >>>> 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' Indeed, and while it may not hurt, it leads to a point where we get the following conditions : (O=Open, _=Closed) 1 2 3 4 O _ _ _ // Write to 1 _ O _ _ // Write to 2 _ _ O _ // Write to 3 _ _ _ O // Write to 4 O O O O // StreamOn // MAX9286_I2C_MUX_STATE_OPEN O O O O // StreamOff // MAX9286_I2C_MUX_STATE_CHANNEL O O O O // Write to 1 _ O O O // Write to 2 _ _ O O // Write to 3 _ _ _ O // Write to 4 O _ _ _ // Write to 1 O O O O // StreamOn // MAX9286_I2C_MUX_STATE_OPEN O O O O // StreamOff // MAX9286_I2C_MUX_STATE_CHANNEL ... Continues I will indeed fix this by calling mux_close when we stop streaming. At least then we will have a consistent state as we expect. In fact from all that I think that means we would likely be better to have: /* Opens all channels on the MUX and disables individual select lines */ static int max9286_i2c_mux_open(struct max9286_priv *priv) { /* Open all channels on the MAX9286 */ max9286_write(priv, 0x0a, 0xff); usleep_range(3000, 5000); priv->mux_state = MAX9286_I2C_MUX_STATE_DISABLE_SELECT; return 0; } static int max9286_i2c_mux_close(struct max9286_priv *priv) { priv->mux_state = MAX9286_I2C_MUX_STATE_CHANNEL; priv->mux_channel = -1; max9286_write(priv, 0x0a, 0x00); usleep_range(3000, 5000); return 0; } static int max9286_i2c_mux_select(struct i2c_mux_core *muxc, u32 chan) { struct max9286_priv *priv = i2c_mux_priv(muxc); /* channel select is disabled when configured in the opend state. */ if (priv->mux_state == MAX9286_I2C_MUX_STATE_DISABLE_SELECT) return 0; /* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */ if (priv->mux_channel == chan) return 0; ... } Which makes the intentions much clearer, and stops other sections of the driver from directly modifying the 'private' mux_state. >>> 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! Great, I'm on the right track then. > Thanks > j >
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 { + /* + * The I2C Mux will enable only a single channel (both forward, and + * 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. + */ + MAX9286_I2C_MUX_STATE_CHANNEL = 0, + + /* + * 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, + + /* + * 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, +}; + static int 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. + * 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; 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. */ + 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; return 0; } + /* Handling for MAX9286_I2C_MUX_STATE_CHANNEL */ + 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; } 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 ith a FIXME and is difficult to interpret the meaning of the 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@ideasonboard.com> --- drivers/media/i2c/max9286.c | 63 +++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 23 deletions(-)