lkml.org 
[lkml]   [2013]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [tpmdd-devel] [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI
Date
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



\
 
 \ /
  Last update: 2013-04-28 03:41    [W:0.121 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site