First the changes since V1 for those familiar with this series:
* old patch 1 dropped, not needed anymore after some reimplementation
here
* added tags from last revision (except for patches 5+6 because there
were changes in code)
* patch 5 has a better of-iterator which respects now that addresses
might have different #address-cells and such
* patch 6 now puts the device it obtained
* one more "dummy" removed fromt the binding docs
TODO: make sure there are no concurrency issues in patch 6 when handling
the struct i2c_client.
Many thanks to Geert and Luca for the review and discussions!
And here the cover-letter for V1:
One outcome of my dynamic address assignment RFC series[1] was that we
need a way to describe an I2C bus in DT fully. This includes unknown
devices and devices requiring multiple addresses. This series implements
that.
Patches 1 does some preparational refactoring. After patch 2, we can
have child nodes with an address, but no compatible. Those addresses
will be marked busy now. They are handled by the dummy driver as well,
but named "reserved" instead of dummy. Patches 3+4 are again some
preparational refactoring. After patch 5, all addresses in a 'reg' array
are now blocked by the I2C core, also using the dummy driver but named
"reserved". So, we can have something like this:
reserved@13 {
reg = <0x13>, <0x14>;
};
After patch 6 then, i2c_new_ancillary_device is spiced up to look for
such a reserved address and return it as a good-old "dummy" device.
Sanity checks include that only a sibling from the same DT node can
request such an ancillary address. Stealing addresses from other drivers
is not possible anymore. This is something I envisioned for some time
now and I am quite happy with the implementation and how things work.
There is only one thing giving me some headache now. There is a danger
of a regression maybe. If someone has multiple 'reg' entries in the DT
but never used i2c_new_ancillary_device but i2c_new_dummy_device, then
things will break now because i2c_new_dummy_device has not enough
information to convert a "reserved" device to a "dummy" one. It will
just see the address as busy. However, all binding documentations I
found which use 'reg' as an array correctly use
i2c_new_ancillary_device. On the other hand, my search strategy for
finding such bindings and DTs do not feel perfect to me. Maybe there are
also some more corner cases in this area, so this series is still RFC.
And some more documentation is needed. Before that, though, the generic
I2C binding docs need some overhaul, too.
All tested on a Renesas Lager board (R-Car H2). A git branch can be
found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/topic/i2c_alias_device_v2
The I3C list is on CC not only because there is 1-line change in their
subsystem, but maybe also because they need to be aware of these changes
for their I2C fallback? I don't really know, let me know if you are not
interested.
Looking forward to comments!
Happy hacking,
Wolfram
[1] https://www.spinics.net/lists/linux-i2c/msg43291.html
Wolfram Sang (6):
i2c: use DEFINE for the dummy driver name
i2c: allow DT nodes without 'compatible'
i2c: of: remove superfluous parameter from exported function
i2c: of: error message unification
i2c: of: mark a whole array of regs as reserved
i2c: core: hand over reserved devices when requesting ancillary
addresses
.../devicetree/bindings/i2c/i2c-ocores.txt | 2 -
Documentation/devicetree/bindings/i2c/i2c.txt | 4 +-
drivers/i2c/i2c-core-base.c | 33 +++++--
drivers/i2c/i2c-core-of.c | 89 +++++++++++--------
drivers/i2c/i2c-core.h | 3 +
drivers/i3c/master.c | 2 +-
include/linux/i2c.h | 6 +-
7 files changed, 88 insertions(+), 51 deletions(-)
--
2.20.1
> There is only one thing giving me some headache now. There is a danger
> of a regression maybe. If someone has multiple 'reg' entries in the DT
> but never used i2c_new_ancillary_device but i2c_new_dummy_device, then
> things will break now because i2c_new_dummy_device has not enough
> information to convert a "reserved" device to a "dummy" one. It will
> just see the address as busy. However, all binding documentations I
> found which use 'reg' as an array correctly use
> i2c_new_ancillary_device. On the other hand, my search strategy for
> finding such bindings and DTs do not feel perfect to me. Maybe there are
> also some more corner cases in this area, so this series is still RFC.
So, I used another search strategy: I checked every
i2c_new_dummy_device() caller in the kernel tree and made sure they
don't get the address to use from DT. I can confirm this is not the
case. That gives me enough trust to say the above issue is a non-issue.
Still open for comments, of course...
Status update on this series:
> TODO: make sure there are no concurrency issues in patch 6 when handling
> the struct i2c_client.
This turns out to be annoying. How to make sure that we don't modify the
i2c_client while the adapter it is sitting on just gets removed. AFAICS
we need a new locking scheme just for that and I am not convinced this
is the way forward.
Also, there is still this small room for regressing when there are DTs
having multiple addresses specified in the DT and the drivers use
i2c_new_dummy_client on these addresses. I have verified that no in-tree
users of i2c_new_dummy (and friends) do work on extra addresses but
still I'd like to completely avoid this potential regression.
One solution to both problems would be to unregister the reserved device
when its address is requested. I am working on this prototype currently.
However, I am not sure yet if one issue might make this approach messy:
re-registering the reserved device when the probe of the requested
address fails.
We will see...
On 15/04/2020 09:27, Wolfram Sang wrote:
>
> Status update on this series:
>
>> TODO: make sure there are no concurrency issues in patch 6 when handling
>> the struct i2c_client.
>
> This turns out to be annoying. How to make sure that we don't modify the
> i2c_client while the adapter it is sitting on just gets removed. AFAICS
> we need a new locking scheme just for that and I am not convinced this
> is the way forward.
>
> Also, there is still this small room for regressing when there are DTs
> having multiple addresses specified in the DT and the drivers use
> i2c_new_dummy_client on these addresses. I have verified that no in-tree
> users of i2c_new_dummy (and friends) do work on extra addresses but
> still I'd like to completely avoid this potential regression.
>
> One solution to both problems would be to unregister the reserved device
> when its address is requested. I am working on this prototype currently.
> However, I am not sure yet if one issue might make this approach messy:
> re-registering the reserved device when the probe of the requested
> address fails.
If we 'unregister' the existing device, could we then register a new
'well named' device more appropriate to the driver, so it doesn't
continue to show up as 'reserved' in the system, but rather a more
appropriate name to the driver that registered it?
> We will see...
>
Looking forward to it :-)
--
Kieran