On Fri 2018-08-24 14:06:04, Jan Kundrát wrote:
> 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.)
Yeah, but I guess working hardware has RESET line connecting... and
reset sequence is crazy enough to be of concern.
> 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.
The default should be not to reset... because that's how it works in v4.18.
> 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.
Seems wonky enough to me :-).
> 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
You already have that problem, right?
> 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".
I'd say this is "ti,our-hw-designers-forgot-to-connect-reset-line",
and I see nothing OS specific about that... Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Attachment:
signature.asc
Description: Digital signature
