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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4DCE512B.50701@fastmail.fm>
Date:	Sat, 14 May 2011 10:53:47 +0100
From:	Jack Stone <jwjstone@...tmail.fm>
To:	Margarita Olaya <magi@...mlogic.co.uk>
CC:	linux-kernel@...r.kernel.org, Liam Girdwood <lrg@...mlogic.co.uk>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	sameo@...ux.intel.com
Subject: Re: [PATCHv2 1/4] mfd: tps65912: Add new mfd device

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/