On Tue, Jun 12, 2018 at 3:13 PM Tudor Ambarus
<tudor.ambarus@xxxxxxxxxxxxx> wrote:
> > +#include <linux/random.h>
>
> includes should be ordered alphabetically.
OK fixed.
> > +static int atmel_ecc_get_serial(struct i2c_client *client)
> > +{
> > + struct atmel_ecc_cmd *cmd;
> > + u8 serial[9];
>
> int i, ret; before serial to avoid stack padding.
>
> > + int ret;
> > + int i;
Is your point trying to help the compiler to lay things out on
the stack? Isn't that premature optimization? I was under the
impression that we should leave this stuff to the compiler.
But sure, it is a small change so I did it anyways :)
> > +
> > + cmd = kmalloc(sizeof(*cmd), GFP_KERNEL);
> > + if (!cmd)
> > + return -ENOMEM;
> > +
> > + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_0_3);
> > + ret = atmel_ecc_send_receive(client, cmd);
> > + if (ret) {
>
> free(cmd);
Whoops added a goto contruction to do proper free:ing of
cmd, thanks.
> > + dev_err(&client->dev,
> > + "failed to read serial byte 0-3\n");
> > + return ret;
> > + }
> > + for (i = 0; i < 4; i++)
> > + serial[0+i] = cmd->data[RSP_DATA_IDX + i];
>
> serial[i]?
OK.
> > + atmel_ecc_init_read_config_word(cmd, CONFIG_ZONE_SERIAL_4_7);
>
> can't we read the serial_no once?
Sorry not following. Do you mean we need an accessor to
read several config words in succession? I think I read somewhere
that the device only supports reading one config word at the
time and the serial number is spread out over three config words...
> > + for (i = 0; i < 4; i++)
> > + serial[4+i] = cmd->data[RSP_DATA_IDX + i];
>
> spaces preferred around that '+'
OK.
> > + dev_info(&client->dev,
> > + "serial number: %02x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > + serial[0], serial[1], serial[2], serial[3], serial[4],
> > + serial[5], serial[6], serial[7], serial[8]);
>
> Why do you need the serial number printed out?
Cuddly feeling I guess. If there is a problem with the device the
user might want to report the serial number to the
manufacturer?
Yours,
Linus Walleij
