On 2016-07-25 12:00, Kachalov Anton wrote:
> You are welcome.
>
> In my implementation in PCA954x I did the following things that I would like to highlight:
>
> 1. handle slave register and store i2c_client data to the internal array
> (length equal to number of channels). First obstacle – get the right offset in
> the array (channel number). It's better to expose "chan_id" field from
> i2c_mux_priv as a public function. I did a hack during init by writing down
> first muxc->adapter's number ("nr" field store in private adap_off).
The i2c-mux.c file is not sacred, nor set in stone, just add the accessor
if you need it. (But the function would be kind of dangerous as there is
no way that I know of to verify that the passed adapter is in fact a mux
child adapter. So, if you'd pass something else, you'd get to keep all
the pieces...)
HOWEVER, chan_id is not necessarily the same as the index into the
muxc->adapter array. The index in the muxc->adapter array is just the
order in which the mux child adapters were registered, and bears no
relation to the chan_id. At least none that can be relied upon.
So, a better (but totally different) addition to the mux core might be
to add a function to set the reg_slave and unreg_slave pointers in
i2c_mux_priv->algo, instead of copying the whole algo struct.
Another thing is that it seems that data->last_chan represents the same
thing as your new variable data->cur_chan, but in a different form (bitmask
vs. enumeration), and they are not always kept in sync. Please use just
one variable to prevent nasty surprises, as two variables are bound to get
out of sync.
> 2. during handling of select_chan I'm trying to register new i2c dummy
> device with newly selected slave addr corresponding to the channel
> number (if it has a slave). This is the second problem – after I've registered
> dummy device with some slave address, I'm not able to register slave
> devices for other mux'ed busses with the same slave addr. In other words,
> I have to populate all the slave devices (with the same slave addr)
> right before I'm going to use the bus first time.
That description doesn't really match the code (or maybe I'm misunderstanding
or the code has changed or something). What do you mean that you have to
populate all the slave devices beforehand? The code only creates one dummy
device and reuses that one for all "slave proxying".
Anyway, one thing that I do not like about that code is that it, on mux
changes, clobbers dummy->addr before the slave proxy in unregistered. It's
not really pretty to clobber it at all, but it would feel better if the addr
was only clobbered when the slave was not registered.
I'm also a bit surprised that you are allowed to create a new (dummy) device
with an address that is already taken on a downstream mux client adapter.
Is that a feature in the i2c core, or is it a bug? Wolfram?
> 3. registering and/or unregister first (if applicable) slave device on the parent
> bus adapter. This is the third obstacle. While the bus is already locked,
> calling generic i2c_slave_{register,unregister} lead to the deadlock.
> I did it directly via the reg_slave/unreg_slave calls of appropriate data structure.
BTW, your open-coded unlocked versions of i2c_slave_register and
i2c_slave_unregister do not handle errors and skips some tests. Maybe add
unlocked versions to i2c-core.c instead? Also, why do you need to forbid
10-bit addresses in pca954x_reg_slave? Is that a property of the mux, or
is it really a property of your root adapter?
> Notes:
> Slave works only with keeping channel selected while generic i2c-mux logic
> is to deselect channel after each i2c transfers.
>
> 25.07.2016, 12:41, "Wolfram Sang" <wsa@xxxxxxxxxxxxx>:
>>> There is no way to switch mux and request to BMC #2. Logically, this shouldn't
>>> happen too.
>>
>> I see. I think I have the big picture now, thanks for the heads up.
>>
>> However, I am not sure what recommendations I can give to you. I'd start
>> with adding reg_slave to the muxed adapters and then try to reconfigure
>> the real adapter when the mux switches (I assume you did that already).
>> But I don't have the bandwidth to dive into that and see what gory
>> details come up. Maybe Peter Rosin has something to add, though?
The whole thing seems a bit fragile, as Wolfram already stated...
Let me see if I got how this works; You have some process that accesses some
device on the different mux segments, with some amount of time between each
access. The mux leaves the segment selected after this access. During this
time slot, when the mux segment is selected but idle, there might come in a
slave access from that mux segment.
This all depends on the external process that round-robins the mux. I don't
like that. It should be some system process with a much closer tie to the
kernel (maybe it should be in-kernel?) that is responsible for the
round-robin of the mux. And, AFAIU, there is no need for any access to the
child side of the mux to enable these slave accesses, this process may just
flip the mux selection and that should be enough, no? This process can also
step past any mux segments that do not have any slave registered so that
the waiting time can be reduced, and it can be completely disabled if there
are no slaves registered.
A bigger issue is that the process also needs to consider other muxes in
case there is a large mux tree on the root adapter. So, I think it needs to
be a central process connected to the root adapter, and not something local
to the mux for it to be generic? This seems to get nastier the more I think
about it...
*time passes*
I realize that in the large mux tree case, your code doesn't work very
well at all. I.e. if you have
dev1
/
mux1 dev2
/ \ /
root mux2
\ \
etc dev3
then accesses to dev1 will flip mux1 and prevent any slave accesses from
dev2 and dev3. I guess this just shows the fundamental fragility with the
approach of having multiple devices doing slave accesses to the same
root adapter in some ad-hoc manner. I guess you could refuse slave
registration in case the mux isn't sitting directly on the root adapter,
but I don't know how to detect and prohibit slave accesses from parallel
muxes on a root adapter.
Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
