Re: [PATCHv2 1/4] mfd: tps65912: Add new mfd device

[Jack Stone <jwjstone@fastmail.fm>]

On 13/05/2011 21:53, Margarita Olaya wrote:
>>> +static int tps65912_spi_read(struct tps65912 *tps65912, u8 addr,
>>> +                                                     int bytes, void *dest)
>>> +{
>>> +     struct spi_device *spi = tps65912->spi_device;
>>> +
>>> +     unsigned long spi_data = 0 << 23 | addr << 15;
>>
>> Is the 0 << 23 meant to be 1 << 23? Like the write function.
>>
> No, It should be zero, I will remove.

Is it a read vs write bit?

You don't have to remove it but a comment explaining that bit 23 is the
read bit would help.

> 
>>> +     struct spi_transfer xfer;
>>> +     struct spi_message msg;
>>> +     int ret;
>>> +     u8 *data = (u8 *) dest;
>> Shouldn't need to cast a void *
>>
>>> +     u32 tx_buf = 0, rx_buf = 0;
>> These are initialized below.
>>
>>> +     tx_buf = spi_data;
>>> +     rx_buf = 0;
>>> +
>>> +     xfer.tx_buf     = &tx_buf;
>>> +     xfer.rx_buf     = &rx_buf;
>>> +     xfer.len        = sizeof(unsigned long);
>>> +     xfer.bits_per_word = 24;
>>> +
>>> +     spi_message_init(&msg);
>>> +     spi_message_add_tail(&xfer, &msg);
>>> +
>>> +     if (spi == NULL)
>>> +             return 0;
>>> +
>>> +     ret = spi_sync(spi, &msg);
>>> +     if (ret == 0)
>>> +             *data = (u8) (rx_buf & 0xFF);
>>> +     return ret;
>>> +}
>>> +
>> The spi read/write functions both ignore the bytes argument and only
>> transfer one byte, whereas the i2c versions appear to read/write
>> multiple bytes. Which one is correct?
>>
> Both are correct, I'm passing bytes argument to spi read/write to make
> it compatible with the declaration of tps65912_read/write functions
> pointer.
> 

But if anyone ever calls the tps65912_read/write functions with bytes !=
1 then this will fail, but only on the SPI case.

If they are only ever called with bytes == 1 then you can simply remove
the bytes argument.

Thanks,

Jack