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