Messages in this thread | | | Date | Thu, 23 May 2013 11:05:33 +0300 | Subject | Re: [PATCH 1/1] TPM: STMicroelectronics st33 driver SPI | From | Andy Shevchenko <> |
| |
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
| |