Re: [PATCH v7 12/34] i2c: tegra: Use clk-bulk helpers

[Thierry Reding]

On Mon, Sep 21, 2020 at 02:15:09PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 21, 2020 at 2:02 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> > On Thu, Sep 17, 2020 at 04:54:28PM +0300, Andy Shevchenko wrote:
> > > On Thu, Sep 17, 2020 at 2:38 PM Thierry Reding <thierry.reding@xxxxxxxxx> wrote:
> > > > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote:
> 
> ...
> 
> > > > This is tempting from a diffstat point of view, but the downside is that
> > > > we can now no longer validate that all of the necessary clocks are given
> > > > in device tree.
> > > >
> > > > Previously the driver would fail to probe the I2C controller if any of
> > > > the expected clocks were not defined in device tree, but now it's just
> > > > going to continue without it and not give any indication as to what's
> > > > wrong.
> > >
> > > You may print an error in the error path as previously. Since both
> > > clocks are mandatory (as far as I understood the code) user will need
> > > to check DT in any case.
> >
> > The problem is that the number of required clocks depends on the variant
> > of the IP block that's implemented. Some require just one clock and
> > others require two or three. With this patch the driver is just going to
> > pick whatever clocks are given in device tree, but it removes any
> > possibility of detecting whether the device trees contain the correct
> > clocks. So we may very well run into a situation where the driver now
> > successfully probes but then malfunctions because one or more of the
> > clocks were not specified in device tree.
> >
> > Thierry
> 
> I still failed to get this. Are you suggesting that CCF bulk
> operations are fundamentally broken?

No, I'm not suggesting that. All I'm saying is the way that they are
used here is causing the driver to behave differently that it was
before.

Taking for example the VI I2C controller instantiation. That requires
the "slow" clock to be specified. Previously if the VI I2C device tree
node didn't have that "slow" clock specified, the I2C driver probe would
exit with an error code. After this change it will simply not see the
"slow" clock and then just continue without it as if it was optional.

In other words, after this patch we have no way of saying which clocks
are required and which are optional. They all become optional, basically
and the driver would attempt to continue (and most likely hang) if no
clocks at all had been specified in device tree.

> In the above case one may add more checks. AFAICS is_vi won't be
> removed, so can be easily checked.
> Basically that for-loop for div_clk is questionable. I agree on that.

But we need that one to find which of the clocks is the divider clock so
that we can call clk_set_rate() on it later on. It's a bit odd that we'd
just continue even if we didn't find the divider clock. I think the CCF
does handle this transparently and will just no-op all the calls for
NULL clocks, but it still means that we won't be running the I2C bus at
the right frequency if the divider clock was not specified in device
tree.

Thierry

Attachment: signature.asc
Description: PGP signature