Re: [PATCH v9 5/5] docs: i2c: i2c-topology: add section about bus speed

[Wolfram Sang]

> What makes the topology bad is D2. Without that device, it should
> be OK. So, the question is which is more important?
> 1. protect ignorant system designers from creating a topology
>    that includes problematic devices like D2
> 2. allow multi-level mux-locked muxes with variable bus-speed at all

My take is: as a generic operating system, we should try hard to
maintain a reliable system. If a device gets stalled and blocks the bus
because of a too high frequency, this is really bad.

Now, if this is a flaw of the HW design, we can only do so much but not
more. If the HW design is OK, we should not introduce risks. This is why
I want to disable this feature for MUX_IDLE_AS_IS. It cannot work in
this setup, but the bus itself is likely designed OK and would work at
the lowest speed.

> Checking the rules at run-time feels complicated to me, as devices
> may come and go. Also, naming people "ignorant" over a mistake such
> as the above in 1 is perhaps not all that fair. But, it feels sad
> to disallow things that in fact do work.

Like I said, as a generic OS, we should play safe. If someone wants to
implement a more risky version, it can be built on top of what we have.
This is agreeing to "I want this and I know I am on my own now".
Obviously, it reduces maintenance burden a lot.

> > ... unlike here. We have MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT defined
> > already. And I'd think we should only allow bus speed switching for
> > MUX_IDLE_DISCONNECT to avoid out-of-spec scenarios. Opinions?
> 
> It is probably not unwise to disallow MUX_IDLE_AS_IS in this context,
> at least until someone actually needs it. Which does not seem all that
> likely?

Exactly.

> However, it seems like it should be fairly OK to allow a predefined
> idle channel, as long as that channel is not lowering the bus speed
> compared to the parent. But maybe supporting that can also wait for
> an actual user?

I agree as well. I hope that Marcus' use-case is MUX_IDLE_DISCONNECT.

> > > +Supported controllers
> > > +-----------------------
> > > +
> > > +Not all I2C controllers support setting the bus speed dynamically.
> > > +At the time of writing, the following controllers have support:
> > > +
> > > +============================   =============================================
> > > +i2c-davinci                    Supports dynamic bus speed
> > > +============================   =============================================
> > 
> > This paragaph is easy to get outdated. We can document that only
> > controller drivers with the callback function implemented will work.
> > People then can find out if that applies for their driver...
> 
> There are other such hard-to-maintain lists elsewhere in the text.
> Not saying that this makes neither those nor this list a good idea,
> but it is an explanation for adding one more.

You mean the list if a driver uses parent-locked or mux-locked. What
about putting that information into the driver? The core could enforce
that it is set to a meaningful value and not left uninitialized?

Happy hacking everyone,

   Wolfram