Re: [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI

Andy Shevchenko
2013-05-23T08:05:40Z

On Thu, May 23, 2013 at 10:43 AM, Mathias LEBLANC
<Mathias.LEBLANC@st.com> wrote:

> Thanks for your support, I will fix these code style problem.

I left below the comments I think should be addressed besides style.
Please, comment what you think about them.

> However in a first time, can we publish this SPI driver?

It's up to SPI subsystem maintainer, though I couldn't consider the
quality of the driver is enough to include to upstream. You may try to
ask Greg to go to staging if you have real demand of this.

> I think that it will be preferable to submit it and apply some patch if it's only coding style error.

I don't support the way of submitting patch on top of submiting
something that must be just fixed.

> I have fix errors in this patch that has been discovered in the I2C patch, so I don't know what's stop this submission.
> I think that's driver is more criticized than the I2C driver although it's the same base.

I hope you understand it's not an exuce.

> I know that's important to have good code in the kernel source and I'm agree about that,

Good.

> I propose it be published as a first release and I fix coding style problem in a second time.

See above.

>> +++ b/drivers/char/tpm/tpm_spi_stm_st33.c

>> +enum stm33zp24_int_flags {
>> + TPM_GLOBAL_INT_ENABLE = 0x80,
>> + TPM_INTF_CMD_READY_INT = 0x080,
>
> What the difference? It looks like first constant is not belong to this enum.
>
>> +static int spi_write8_reg(struct tpm_chip *tpm, u8 tpm_register,
>> + u8 *tpm_data, u16 tpm_size) {
>> + u8 data = 0;
>> + int total_length = 0, nbr_dummy_bytes;
>> + int value = 0;
>> + struct spi_device *dev =
>> + (struct spi_device __force *)tpm->vendor.iobase;
>> + struct st33zp24_platform_data *platform_data = dev->dev.platform_data;
>> + u8 *data_buffer = platform_data->tpm_spi_buffer[0];
>
> It seems a bad idea to have buffers in platform_data. I bet the buffers should be part of other struct. What did I miss?
>
>> + struct spi_transfer xfer = {
>> + .tx_buf = data_buffer,
>> + .rx_buf = platform_data->tpm_spi_buffer[1],
>> + };
>
> ... even this entire structure.
> Can you consider to use spi_message API ?

>> +static unsigned long wait_for_serirq_timeout(struct tpm_chip *chip,
>> + bool condition, unsigned long timeout) {
>> + long status = 0;
>> + struct spi_device *client;
>> + struct st33zp24_platform_data *pin_infos;
>> +
>> + client = (struct spi_device __force *)chip->vendor.iobase;
>
> Is there any better storage for this pointer? It seems an abuse of iobase member.

>> + chip->vendor.iobase = (void __iomem *)dev;
>
> Don't like this one. Try to find better way to drag pointer.

--
With Best Regards,
Andy Shevchenko