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

[Thierry Reding]

On Thu, Sep 17, 2020 at 06:01:56PM +0300, Dmitry Osipenko wrote:
> 17.09.2020 14:38, Thierry Reding пишет:
> > On Wed, Sep 09, 2020 at 01:39:44AM +0300, Dmitry Osipenko wrote:
> >> Use clk-bulk helpers and factor out clocks initialization into separate
> >> function in order to make code cleaner.
> >>
> >> The clocks initialization now performed after reset-control initialization
> >> in order to avoid a noisy -PROBE_DEFER errors on T186+ from the clk-bulk
> >> helper which doesn't silence this error code. Hence reset_control_get()
> >> now may return -EPROBE_DEFER on newer Tegra SoCs because they use BPMP
> >> driver that provides reset controls and BPMP doesn't come up early during
> >> boot. Previously rst was protected by the clocks retrieval and now this
> >> patch makes dev_err_probe() to be used for the rst error handling.
> >>
> >> Suggested-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> >> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> >> ---
> >>  drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++---------------------
> >>  1 file changed, 67 insertions(+), 120 deletions(-)
> > 
> > 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.
> 
> The clocks can't be missed randomly in a device-tree because they are a
> part of the core tegra.dtsi

You can easily generate a device-tree without using the core DTS
includes.

> The tegra-i2c DT binding isn't converted to YAML, but once it will be,
> then the dtbs_check will tell you about such obvious problems like a
> missing mandatory property.

Once that has happened, yes, I think we may be able to simplify the
driver. But before that happens I don't think we should throw away our
only line of defense against broken device trees.

> Even if clock is missing, then you won't miss this problem since I2C
> shouldn't work in that case.

But the whole point of this error handling here is so that we can make
it easier to find the cause of an error. I2C malfunctioning can be
subtle. You could have some EEPROM on I2C that you normally don't touch,
but then at some point you try to access it from userspace and you read
garbage and then you need to start looking why this is happening. The
whole point of error messages is so that you can easily find the root
cause.

> There is a Qualcomm I2C driver that already uses clk_bulk_get_all() and
> doesn't worry about "accidentally" missing clocks.

Just because one particular driver doesn't care doesn't mean everybody
else should stop caring.

> It's still possible to add the clk-num checking, but it should be
> unpractical. We could always add it later on if there will be a real
> incident. Do you agree?

If there's an incident it's already too late. The whole point of error
checking here is to avoid any accidental breakage.

Thierry

Attachment: signature.asc
Description: PGP signature