On pátek 24. srpna 2018 11:45:00 CEST, Pavel Machek wrote:
Ok, resetting seems like good idea, but the crazy way this reset is
done tells me it should probably be opt-in via device-tree option?
Hi Pavel,
I was also thinking about this, but in the end I decided to always perform
the reset unconditionally. Here's a bunch of reasons:
1) I don't see a scenario where it is legitimate *not* to reset. If we
don't reset, then even a simple reboot on a platform with no RESET signal
propagation can cause the kernel to report values via sysfs that do not
reflect reality. (Been there, saw that.)
2) What should be the default? If it's to reset, then one has to come up
with a way of coordinating the reset among several instances so that a
reset "of a device probed later" won't affect those already configured.
This is hard, and that's why my patch keeps a reference to the i2c_client
of the reset address. And I do believe that the default should be to reset
because that way we always work with a chip in a known good state.
3) The sequence is not that wonky considering that we're talking about a
LED driver :). These chips quite often have several group-call addresses
and other, eh, features which do not map well with Linux' abstractions. I
don't understand why they did not put in that functionality over the
*regular* I2C address, but then I don't design chips for a living.
4) Coming from the SPI subsystem, people say that the DT bindings should
describe hardware instead of talking about compatibility quirks which might
be Linux specific (recall that "spidev" discussion where the code complains
if the DT bindings say that s given device is just "spidev"). Think about
how the DT bindings with this feature would look like on a board with
several chips:
leds@0x60 {
compatible = "ti,tlc59116";
reg = <0x60>;
ti,reset;
};
leds@0x61 {
compatible = "ti,tlc59116";
reg = <0x61>;
};
Only one of these devices should invoke the reset, the rest of them must be
somehow prevented from doing it because the reset affects all chips on the
bus, not just the one we are "talking to". The SWRESET I2C address is
hardcoded. In my opinion, this would be an OS-specific quirk where the DT
says "reset everyone when the code probes for this device".
Perhaps I am missing something, though.
Cheers,
Jan
