Hi Matthias,
it's nice to see that you consider most of the comments, unfortunately I still
have some left ;)
> +/*
> + * tpm_st33_spi_init initialize driver
> + * @return: 0 if successful, else non zero value.
> + */
> +static int __init tpm_st33_spi_init(void)
> +{
> + return spi_register_driver(&tpm_st33_spi_driver);
> +}
> +
> +/*
> + * tpm_st33_spi_exit The kernel calls this function during unloading the
> + * module or during shut down process
> + */
> +static void __exit tpm_st33_spi_exit(void)
> +{
> + spi_unregister_driver(&tpm_st33_spi_driver);
> +}
> +
> +module_init(tpm_st33_spi_init);
> +module_exit(tpm_st33_spi_exit);
Please use module_spi_driver(&tpm_st33_spi_driver); instead
Boilerplate code sucks.
> + u8 *data_buffer = platform_data->tpm_spi_buffer[0];
> + struct spi_transfer xfer = {
> + .tx_buf = data_buffer,
> + .rx_buf = platform_data->tpm_spi_buffer[1],
> + };
> + struct spi_message msg;
>...
> + xfer.len = total_length;
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> + value = spi_sync(dev, &msg);
Doesn't spi_write fit here ?
static inline int
spi_write(struct spi_device *spi, const void *buf, size_t len)
{
struct spi_transfer t = {
.tx_buf = buf,
.len = len,
};
struct spi_message m;
spi_message_init(&m);
spi_message_add_tail(&t, &m);
return spi_sync(spi, &m);
}
Seems pretty identical.
-> This would save some lines of code,
and again - boilerplate code sucks.
Same applies to spi_read,
The driver generates some gcc warnings:
drivers/char/tpm/tpm_spi_stm_st33.c: In function 'read8_reg':
drivers/char/tpm/tpm_spi_stm_st33.c:180:5: warning: unused variable 'latency'
[-Wunused-variable]
drivers/char/tpm/tpm_spi_stm_st33.c: In function 'tpm_stm_spi_status':
drivers/char/tpm/tpm_spi_stm_st33.c:353:2: warning: 'data' is used
uninitialized in this function [-Wuninitialized]
drivers/char/tpm/tpm_spi_stm_st33.c: In function 'get_burstcount':
drivers/char/tpm/tpm_spi_stm_st33.c:450:12: warning: 'temp' is used
uninitialized in this function [-Wuninitialized]
drivers/char/tpm/tpm_spi_stm_st33.c: In function 'check_locality':
drivers/char/tpm/tpm_spi_stm_st33.c:369:22: warning: 'data' may be used
uninitialized in this function [-Wuninitialized]
The driver does give me some smatch errors:
$ LANG=C make -j16 C=1 CHECK=smatch
CHECK drivers/char/tpm/tpm_spi_stm_st33.c
drivers/char/tpm/tpm_spi_stm_st33.c:882 tpm_st33_spi_probe() warn: variable
dereferenced before check 'platform_data' (see line 781)
drivers/char/tpm/tpm_spi_stm_st33.c:980 tpm_st33_spi_pm_resume() warn: should
this be a bitwise op?
drivers/char/tpm/tpm_spi_stm_st33.c:980 tpm_st33_spi_pm_resume() warn: should
this be a bitwise op?
and some sparse errors:
drivers/char/tpm/tpm_spi_stm_st33.c:102:35: warning: cast removes address
space of expression
drivers/char/tpm/tpm_spi_stm_st33.c:171:35: warning: cast removes address
space of expression
drivers/char/tpm/tpm_spi_stm_st33.c:299:19: warning: cast removes address
space of expression
drivers/char/tpm/tpm_spi_stm_st33.c:311:15: warning: symbol
'wait_for_serirq_timeout' was not declared. Should it be static?
drivers/char/tpm/tpm_spi_stm_st33.c:546:19: warning: cast removes address
space of expression
And here are some other, rather subjective remarks:
> + for (; nbr_dummy_bytes < total_length &&
> + ((u8 *)xfer.rx_buf)[nbr_dummy_bytes] == 0;
> + nbr_dummy_bytes++)
> + ;
Not sure if it would be easier to read using a while and putting the increment
into the loop body
>+ while (nbr_dummy_bytes < total_length &&
>+ ((u8 *)xfer.rx_buf)[nbr_dummy_bytes] == 0)
>+ nbr_dummy_bytes++;
I usually don't like empty loop bodies, as they tend to be overlooked.
> +static u32 SPI_WRITE_DATA(struct tpm_chip *tpm, u8 tpm_register,
> + u8 *tpm_data, u16 tpm_size)
> +{
> + u8 value = 0;
> + value = write8_reg(tpm, tpm_register, tpm_data, tpm_size);
> +
> + switch (value) {
> + case TPM_ST_SPI_OK:
> + return TPM_ST_SPI_OK;
> + case 0:
> + return 0;
> + default:
> + return -EBUSY;
> + }
> +} /* SPI_WRITE_DATA() */
Why do you need a wrapper function here for the return code?
write8_reg is only called from this location, so why doesn't it return the
correct error code directly?
*confused*
Same applies to read8_reg.
> +static int _wait_for_interrupt_serirq_timeout(struct tpm_chip *chip,
> + unsigned long timeout)
Is only called from wait_for_serirq_timeout - I'm not really sure if it helps
readability to have a separate function.
> + .resume = tpm_st33_spi_pm_resume,
> + .suspend = tpm_st33_spi_pm_suspend,
Maybe use
SIMPLE_DEV_PM_OPS
instead as it is (afaik) the new standard way for PM_OPS.
The conversion should be pretty similar to the one in your I2C TPM driver:
https://github.com/shpedoikal/linux/commit/d459335381eca1cb91fefb87021d3d172342e55a
Regards,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
