* Re: [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency [not found] ` <20180605134950.6605-2-linus.walleij@linaro.org> @ 2018-06-11 9:46 ` Tudor Ambarus 2018-06-28 8:47 ` Linus Walleij 0 siblings, 1 reply; 2+ messages in thread From: Tudor Ambarus @ 2018-06-11 9:46 UTC (permalink / raw) To: Linus Walleij, wsa; +Cc: linux-i2c, linux-crypto, linux-arm-kernel, Herbert Xu 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; > ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency 2018-06-11 9:46 ` [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency Tudor Ambarus @ 2018-06-28 8:47 ` Linus Walleij 0 siblings, 0 replies; 2+ messages in thread From: Linus Walleij @ 2018-06-28 8:47 UTC (permalink / raw) To: Tudor Ambarus Cc: linux-i2c, Herbert Xu, linux-crypto, Linux ARM, Wolfram Sang 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 ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-06-28 8:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180605134950.6605-1-linus.walleij@linaro.org>
[not found] ` <20180605134950.6605-2-linus.walleij@linaro.org>
2018-06-11 9:46 ` [PATCH 2/9] crypto: atmel-ecc: Silently ignore missing clock frequency Tudor Ambarus
2018-06-28 8:47 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).