On Thu, May 23, 2013 at 10:43 AM, Mathias LEBLANC
<[email protected]> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
