Re: Two separate i2c transfers

["Adamski, Krzysztof (Nokia - PL/Wrocław)" <krzysztof.adamski@nokia.com>]

Hi Peter,

W dniu 15.05.2020 o 10:02, Peter Rosin pisze:
> 
> 
> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>> Hi Wolfram, Peter,
>>
>> Thank you for the quick response.
>>
>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze:
>>> Hi Krzysztof,
>>>
>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote:
>>>
>>> Adding Peter as the mux maintainer to CC.
>>>
>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an
>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus
>>>> master selector channels. Both masters must do such a sequence before performing long operation:
>>>
>>> I need a diagram of that setup. What is the BMS? A chip? Some software?
>>> Can you draw a graph and give names of chips etc...?
>>>
>>> And, of course, why on earth do you need to access the same chip from
>>> two masters within one Linux? :) (That's how I understood it)
>>>
> 
> *snip* useful layout.
> 
>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make
>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already
>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to
>> do this kind of deeper lock on the bus.
> 
> Yes, that definitely makes it clearer. So, what you need is something
> more complex than an I2C xfer between select/deselect. Your proposal
> to add a flag to not do the deselect should probably have a matching
> flag to not do the select on the following xfer. Otherwise that second
> xfer is likely to do useless work. This should probably also be two
> independent flags, so that you can add intermediate xfers that neither
> select nor deselect. But you also need an explicit deselect mechanism
> for use if there is a problem half way through...
> 
> But, I think all that exposes the too much to the users, and while it
> is certainly possible (most things are), I not a huge fan of it.
> Maintainers of other subsystems will not know the rules, and drivers
> are, over time, bound to start using these facilities in half-baked
> ways.
> 
> I would rather have some generic I2C mechanism to request more than
> a single xfer as a unit, such that the muxes/arbs/gates can then lock
> the mux/arb/gate, do the select, feed the unit to the parent adapter
> and finally deselect and unlock the mux/arb/gate. With the locking
> and selecting centralized instead of spread out to all users. This
> helps avoid bugs where things are not cleaned up on error paths etc.
> 

Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not
want to be disturbed by anyone on local system, you do:

i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);
 __i2c_transfer(client->adapter, ...);
 some_logic
 __ice_transfer(client->adapter, ...);
i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER);

right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change
existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in):

i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);
 __i2c_transfer(client->adapter, ...)
 some_logic
 __ice_transfer(client->adapter, ...)
i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR);


The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do:
if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
    priv->muxc->select(muxc, priv->chan_id);
    priv->muxc->arbitrator_selected = true;
}

and unlock could do the opposite:
if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) {
    priv->muxc->select(muxc, priv->chan_id);
    priv->muxc->arbitrator_selected = true;
}

Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set,
it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock
and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could
make sense for other types of muxes as well..

Would that really be confusing / un-intutive ?

Best regards,