linux-kernel - Re: [PATCH v3] iio: adc: ti-ads7950: add GPIO support

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Feb 2019 10:48:49 -0600
From:   David Lechner <david@...hnology.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     justinpopo6@...il.com, linux-iio@...r.kernel.org,
        linux-gpio@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        f.fainelli@...il.com, bgolaszewski@...libre.com,
        linus.walleij@...aro.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] iio: adc: ti-ads7950: add GPIO support

On 2/20/19 6:00 AM, Jonathan Cameron wrote:
> On Wed, 13 Feb 2019 09:10:53 +0100
> David Lechner <david@...hnology.com> wrote:
> 
>> On 2/12/19 9:57 PM, justinpopo6@...il.com wrote:
>>> From: Justin Chen <justinpopo6@...il.com>
>>>
>>> The ADS79XX has GPIO pins that can be used. Add support for the GPIO
>>> pins using the GPIO chip framework.
>>>
>>> Signed-off-by: Justin Chen <justinpopo6@...il.com>
>>> ---
>>
>> It will be better to split this up into two patches[1]. One to replace
>> all uses of indio_dev->mlock with the new local lock and then another to
>> add GPIO support.
>>
>> How are you using/testing this patch? Do we need device tree bindings?
>>
>> It will also help reviewers if you add a note about what you changed in
>> each revision of the patch when you resubmit. The usual way to do this
>> is something like:
>>
>>       v3 changes:
>>
>>       - Fixed unlocking mutex too many times in ti_ads7950_init_gpio()
>>
>> It also is nice to wait a few days at least before submitting the next
>> revision to give people some time to respond.
> 
> Agreed with all comments except the endian one.
> SPI doesn't define an endianness of data on the wire, so we may need
> to convert to match whatever ordering the ti chip expects.
> I would expect things to be thoroughly broken if we remove those
> conversions - particularly as I doubt this is being tested with a
> big endian host!
> 
> Jonathan

I'm a bit confused then. I got this idea from include/linux/spi.h, which
says:

  * In-memory data values are always in native CPU byte order, translated
  * from the wire byte order (big-endian except with SPI_LSB_FIRST).  So
  * for example when bits_per_word is sixteen, buffers are 2N bytes long
  * (@len = 2N) and hold N sixteen bit words in CPU byte order.

And in the most recent patches to the ti-ads7950 driver where we switched
from 8-bit words to 16-bit words, I had to remove the calls to cpu_to_be16()
to keep things working.

I realize that I am only using one SPI controller, so I may be making a bad
assumption here, but it seems to me that it is up to the SPI controller to
make sure the bits get sent over the wire in the correct order and we
shouldn't have to worry about it here. We are implicitly telling the SPI
controller that we need big-endian over the wire by omitting the SPI_LSB_FIRST
flag here:

	spi->bits_per_word = 16;
	spi->mode |= SPI_CS_WORD;
	ret = spi_setup(spi);