| Message ID | 20180605134950.6605-2-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
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 <linus.walleij@linaro.org> > --- > 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", &bus_clk_rate); > - if (ret) { > - dev_err(dev, "of: failed to read clock-frequency property\n"); > - return ret; > - } > - > - if (bus_clk_rate > 1000000L) { > + if (!ret && (bus_clk_rate > 1000000L)) { > dev_err(dev, "%d exceeds maximum supported clock frequency (1MHz)\n", > bus_clk_rate); > return -EINVAL; >
On Mon, Jun 11, 2018 at 11:46 AM Tudor Ambarus <tudor.ambarus@microchip.com> 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
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", &bus_clk_rate); - if (ret) { - dev_err(dev, "of: failed to read clock-frequency property\n"); - return ret; - } - - if (bus_clk_rate > 1000000L) { + if (!ret && (bus_clk_rate > 1000000L)) { dev_err(dev, "%d exceeds maximum supported clock frequency (1MHz)\n", bus_clk_rate); return -EINVAL;
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. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/crypto/atmel-ecc.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)