Re: [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency

[Linus Walleij]

On Mon, Jun 11, 2018 at 11:46 AM Tudor Ambarus
 wrote:
> On 06/05/2018 04:49 PM, Linus Walleij wrote:
> > The Atmel ECC driver contains a check for the I2C bus clock
> > frequency, so as to check that the I2C adapter in use
> > satisfies the device specs.
> >
> > If the device is connected to a device tree node that does not
> > contain a clock frequency setting, such as an I2C mux or gate,
> > this blocks the probe. Make the probe continue silently if
> > no clock frequency can be found, assuming all is safe.
>
> I don't think it's safe. We use bus_clk_rate to compute the ecc508's
> wake token. If you can't wake the device, it will ignore all your
> commands.

I see. I wonder why it works so well for me then?
Could we just print a warning and continue?
The general advice for the kernel is not to bail out if
hardware is not optimally configured, but continue with
a warning that maybe not everything is all right.

> My proposal was to introduce a bus_freq_hz member in the i2c_adapter
> structure (https://patchwork.kernel.org/patch/10307637/), but I haven't
> received feedback yet, Wolfram?
>
> I would say first to check the bus_freq_hz from adapter (if it will be
> accepted) else to look into dt and, in the last case, if nowhere
> provided, to block the probe.

So blocking the probe because we are not 100% sure the hardware
will work is against established practice: the established practice
is to be liberal with letting probe() continue, but emit a warning.

In my case that is good because it makes the hardware probe
and work without any visible problems.

I *can* try to traverse the device tree upwards from the mux node
to find the parent I2C controller and its "clock-frequency"
property. I felt that was hackish, but if you prefer the hack I
can try to use that.

Yours,
Linus Walleij

Re: [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency

[Tudor Ambarus]

Hi, Linus, Wolfram,

On 06/05/2018 04:49 PM, Linus Walleij wrote:

The Atmel ECC driver contains a check for the I2C bus clock
frequency, so as to check that the I2C adapter in use
satisfies the device specs.

If the device is connected to a device tree node that does not
contain a clock frequency setting, such as an I2C mux or gate,
this blocks the probe. Make the probe continue silently if
no clock frequency can be found, assuming all is safe.


I don't think it's safe. We use bus_clk_rate to compute the ecc508's
wake token. If you can't wake the device, it will ignore all your
commands.

My proposal was to introduce a bus_freq_hz member in the i2c_adapter
structure (https://patchwork.kernel.org/patch/10307637/), but I haven't
received feedback yet, Wolfram?

I would say first to check the bus_freq_hz from adapter (if it will be
accepted) else to look into dt and, in the last case, if nowhere
provided, to block the probe.

Thanks,
ta



Signed-off-by: Linus Walleij 
---
  drivers/crypto/atmel-ecc.c | 11 +--
  1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c
index e66f18a0ddd0..145ab3a39a56 100644
--- a/drivers/crypto/atmel-ecc.c
+++ b/drivers/crypto/atmel-ecc.c
@@ -657,14 +657,13 @@ static int atmel_ecc_probe(struct i2c_client *client,
return -ENODEV;
}
  
+	/*

+* Silently assume all is fine if there is no
+* "clock-frequency" property.
+*/
ret = of_property_read_u32(client->adapter->dev.of_node,
   "clock-frequency", _clk_rate);
-   if (ret) {
-   dev_err(dev, "of: failed to read clock-frequency property\n");
-   return ret;
-   }
-
-   if (bus_clk_rate > 100L) {
+   if (!ret && (bus_clk_rate > 100L)) {
dev_err(dev, "%d exceeds maximum supported clock frequency 
(1MHz)\n",
bus_clk_rate);
return -EINVAL;