Hi Peter, Wofram,
W dniu 17.05.2020 o 23:32, Peter Rosin pisze:
> On 2020-05-15 23:19, Peter Rosin wrote:
>
> Ok, now I see what you mean, and I realize that you were talking
> about a new flag for i2c_bus_lock() all along. Brain-fart on my
> part. Sorry.
>
> Yes, by adding the flag to i2c_bus_(un)lock() you get something that
> is much closer to existing semantics.
>
>>> 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:
>> This is not true, you would have to do it in i2c_parent_(un)lock_bus
>> too. The I2C client drivers should not need to know (or care) if the
>> muxes are parent-locked or mux-locked.
Yes, I agree. That was what I was thinking about - I meant that it is only relevant to callbacks in i2c-mux.c
>> Hmmm, on second thought, maybe the select/deselect should be part of
>> lock/unlock unconditionally? The big problem with that is that
>> "locking" may fail. And, I now realize that this is the big issue
>> with the whole approach. You need to add a return value to
>> i2c_lock_operations.lock_bus, and that feels like a *big* change.
>> Maybe not technically, the there will be a signal that the function
>> may fail. I.e. every client will, with that return value, get pressure
>> to check and act on locking failures when it is in fact only needed in
>> this new situation.
This is indeed a bigger problem I didn't think about earlier. But at I guess we agree that having such a possibility is
useful, we just have to find out the best way to implement this.
>>
>> I guess you could do the select/deselect thing conditionally, like
>> you suggested, but make it an error to try to use it with
>> i2c_lock_bus(), and only allow it from i2c_trylock_bus() which
>> already has a return value. But with that, I feel that you need to
>> somehow separate the cases when you want to actually wait for the lock
>> and when you want to give up immediately if the lock is taken.
But the problem is that currently the select callback does not have a "trylock" semantics - it tries for quite some time
before giving up. We would need to introduce some kind of flag to distinguish between the two. But the caller then would
have to most likely implement his own timeout loop which is not elegant solution either.
> The return value could be added to i2c_lock_operations.lock_bus, but not
> be exposed when called through i2c_lock_bus(). That wouldn't bee too bad
> I guess. Then a separate set of functions could be added for lock+select
> that do expose the return value (and adds the new flag of course). I.e.
> i2c_lock_select_bus, i2c_trylock_select_bus and t2c_deselect_unlock_bus.
> Or something like that.
That would be ok for me but Wolfram would have to comment, I guess.
> However, I have the feeling that *most* client drivers that do their
> own i2c_lock_adapter/__i2c_transfer/.../i2c_unlock_adapter really would
> like to also keep arbitrators locked during the operation to prevent not
> only the local master from clobbering the chip status.
I'm not sure how useful this is for generic case (if it's "most" or only "some") but as you pointed out, it is not a
simple extension of a lock as now the client would have to deal with the fail possibility so I think such a lock should
still be considered an opt-in. For this, a new API is good idea.
> Hmm. New idea. Maybe the arbitrator ->select should remain with the xfer
> as it is today, but then you set a flag that the arbitrator is selected,
> and delay the deselect until the unlock (instead of doing it last in the
> xfer). You would of course need to check the flag before doing the select
> in the xfer, so that follow-up xfers don't redo the select.
>
> Something like this (untested, written in the mail client):
>
> static int __i2c_mux_master_xfer(struct i2c_adapter *adap,
> struct i2c_msg msgs[], int num)
> {
> struct i2c_mux_priv *priv = adap->algo_data;
> struct i2c_mux_core *muxc = priv->muxc;
> struct i2c_adapter *parent = muxc->parent;
>
> /* Switch to the right mux port and perform the transfer. */
>
> if (!muxc->selected) {
> int ret = muxc->select(muxc, priv->chan_id);
> muxc->selected = true;
> if (ret < 0)
> return ret;
> }
>
> return __i2c_transfer(parent, msgs, num);
> }
>
> static void i2c_parent_unlock_bus(struct i2c_adapter *adapter,
> unsigned int flags)
> {
> struct i2c_mux_priv *priv = adapter->algo_data;
> struct i2c_mux_core *muxc = priv->muxc;
> struct i2c_adapter *parent = muxc->parent;
>
> if (muxc->selected && muxc->deselect)
> muxc->deselect(muxc, priv->chan_id);
> muxc->selected = false;
> i2c_unlock_bus(parent, flags);
> rt_mutex_unlock(&parent->mux_lock);
> }
>
> (i2c_mux_master_xfer(), __i2c_mux_smbus_xfer(), i2c_mux_smbus_xfer() and
> i2c_mux_unlock_bus() need corresponding changes)
Something like this was my initial idea. But I'm not that sure if such a "deeper lock" should indeed be default option
for all callers. Could that introduce some regressions? Especially if we somehow get deselected automatically by the
device (like noted below).
> One more thing to keep in mind. Some arbs/muxes/gates auto-deselects after
> the Nth xfer (always the 1st?), so arb/mux/gate drivers must probably have
> some way to opt out of whatever we do to support "I2C units" across
> arbs/muxes/gates. Maybe that property mostly applies to gates? Anyway, the
> above is not a complete solution.
Also there is a possibility that a mux could be deselected after some idle time as well. I think the PCA9461 does
support something like that but I would have to check.
Maybe a hybrid approach would be best, then? The user could provide a flag to i2c_bus_lock() to opt-in for a deeper lock
(to avoid possible regressions and changed behavior) but the actual select would be called in __i2c_mux_mater_xfer and
deselect in unlock_bus as you suggested above (but only if the flag was provided to i2c_bus_{un,}lock(). I'm not sure if
this is better than introducing i2c_{un,}lock_select_bus() as suggested previously, however. Wolfram, what are your
thoughts? I think an additional point of view would be useful here.
Krzysztof
