| Message ID | 20180605134950.6605-3-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi, Linus, On 06/05/2018 04:49 PM, Linus Walleij wrote: > Report errors once when they happen on the I2C bus so we > get good information in cases such as when the wrong I2C > address is used. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/crypto/atmel-ecc.c | 27 +++++++++++++++++++++------ > 1 file changed, 21 insertions(+), 6 deletions(-) > > diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c > index 145ab3a39a56..214b0572bf8b 100644 > --- a/drivers/crypto/atmel-ecc.c > +++ b/drivers/crypto/atmel-ecc.c > @@ -310,29 +310,41 @@ static int atmel_ecc_send_receive(struct i2c_client *client, > mutex_lock(&i2c_priv->lock); > > ret = atmel_ecc_wakeup(client); > - if (ret) > + if (ret) { > + dev_err(&client->dev, "wakeup failed\n"); > goto err; > + } > > /* send the command */ I guess that this comment will become superfluous if you're going to add an error message. > ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE); > - if (ret < 0) > + if (ret < 0) { > + dev_err(&client->dev, "command send failed\n"); > goto err; > + } > > /* delay the appropriate amount of time for command to execute */ > msleep(cmd->msecs); > > /* receive the response */ > ret = i2c_master_recv(client, cmd->data, cmd->rxsize); > - if (ret < 0) > + if (ret < 0) { > + dev_err(&client->dev, "getting response failed\n"); > goto err; > + } > > /* put the device into low-power mode */ > ret = atmel_ecc_sleep(client); > - if (ret < 0) > + if (ret < 0) { > + dev_err(&client->dev, "putting to sleep failed\n"); > goto err; > + } > > mutex_unlock(&i2c_priv->lock); > - return atmel_ecc_status(&client->dev, cmd->data); > + ret = atmel_ecc_status(&client->dev, cmd->data); atmel_ecc_status already prints errors when needed. > + if (ret < 0) > + dev_err(&client->dev, "ECC status parse failed\n"); > + > + return ret; > err: > mutex_unlock(&i2c_priv->lock); > return ret; > @@ -624,8 +636,11 @@ static int device_sanity_check(struct i2c_client *client) > atmel_ecc_init_read_cmd(cmd); > > ret = atmel_ecc_send_receive(client, cmd); > - if (ret) > + if (ret) { > + dev_err(&client->dev, > + "failed to send ECC init command\n"); Here we will have two errors reported, the first being from the atmel_ecc_send_receive(). I would go with just an error reported. Do we really care what happened with the i2c transfer, or it's enough to report that the init failed? Thanks, ta > goto free_cmd; > + } > > /* > * It is vital that the Configuration, Data and OTP zones be locked >
On Tue, Jun 12, 2018 at 2:36 PM Tudor Ambarus <tudor.ambarus@microchip.com> wrote: > On 06/05/2018 04:49 PM, Linus Walleij wrote: > > /* send the command */ > > I guess that this comment will become superfluous if you're going to add > an error message. OK stripped obvious comments. > > - return atmel_ecc_status(&client->dev, cmd->data); > > + ret = atmel_ecc_status(&client->dev, cmd->data); > > atmel_ecc_status already prints errors when needed. OK skipped this change. > > ret = atmel_ecc_send_receive(client, cmd); > > - if (ret) > > + if (ret) { > > + dev_err(&client->dev, > > + "failed to send ECC init command\n"); > > Here we will have two errors reported, the first being from the > atmel_ecc_send_receive(). I would go with just an error reported. Do we > really care what happened with the i2c transfer, or it's enough to > report that the init failed? The more help the better. I think it is relevant to have both: you will read the log and say "OK init failed because this I2C transfer is not getting across as it should", that is helpful. Yours, Linus Walleij
diff --git a/drivers/crypto/atmel-ecc.c b/drivers/crypto/atmel-ecc.c index 145ab3a39a56..214b0572bf8b 100644 --- a/drivers/crypto/atmel-ecc.c +++ b/drivers/crypto/atmel-ecc.c @@ -310,29 +310,41 @@ static int atmel_ecc_send_receive(struct i2c_client *client, mutex_lock(&i2c_priv->lock); ret = atmel_ecc_wakeup(client); - if (ret) + if (ret) { + dev_err(&client->dev, "wakeup failed\n"); goto err; + } /* send the command */ ret = i2c_master_send(client, (u8 *)cmd, cmd->count + WORD_ADDR_SIZE); - if (ret < 0) + if (ret < 0) { + dev_err(&client->dev, "command send failed\n"); goto err; + } /* delay the appropriate amount of time for command to execute */ msleep(cmd->msecs); /* receive the response */ ret = i2c_master_recv(client, cmd->data, cmd->rxsize); - if (ret < 0) + if (ret < 0) { + dev_err(&client->dev, "getting response failed\n"); goto err; + } /* put the device into low-power mode */ ret = atmel_ecc_sleep(client); - if (ret < 0) + if (ret < 0) { + dev_err(&client->dev, "putting to sleep failed\n"); goto err; + } mutex_unlock(&i2c_priv->lock); - return atmel_ecc_status(&client->dev, cmd->data); + ret = atmel_ecc_status(&client->dev, cmd->data); + if (ret < 0) + dev_err(&client->dev, "ECC status parse failed\n"); + + return ret; err: mutex_unlock(&i2c_priv->lock); return ret; @@ -624,8 +636,11 @@ static int device_sanity_check(struct i2c_client *client) atmel_ecc_init_read_cmd(cmd); ret = atmel_ecc_send_receive(client, cmd); - if (ret) + if (ret) { + dev_err(&client->dev, + "failed to send ECC init command\n"); goto free_cmd; + } /* * It is vital that the Configuration, Data and OTP zones be locked
Report errors once when they happen on the I2C bus so we get good information in cases such as when the wrong I2C address is used. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/crypto/atmel-ecc.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-)