Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-24 Thread Jonathan Cameron 
On 21/11/16 19:52, David Lechner wrote:
> On 11/20/2016 12:28 PM, David Lechner wrote:
>> This adds a new driver for the TI ADS7950 family of ADC chips. These
>> communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
>> varieties.
>>
>> Signed-off-by: David Lechner 
>> ---
>>
>> v2 changes:
>>
>> * Got rid of XX wildcards - using ADS7950 everywhere
>> * Fixed some macro parentheses issues
>> * Added TI_ prefix to macros to match ti_ prefixes used elsewhere
>> * Added space in rx_buf for holding timestamp
>> * Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
>>   helper functions
>> * Don't use dev_info() at end of probe
>> * Minor spelling and code style fixes
>>
>>  drivers/iio/adc/Kconfig  |  13 ++
>>  drivers/iio/adc/Makefile |   1 +
>>  drivers/iio/adc/ti-ads7950.c | 488 
>> +++
>>  3 files changed, 502 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-ads7950.c
>>
> 
> ...
> 
>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>> new file mode 100644
>> index 000..d0b76bd
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads7950.c
> 
> ...
> 
>> +static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>> +{
>> +struct iio_poll_func *pf = p;
>> +struct iio_dev *indio_dev = pf->indio_dev;
>> +struct ti_ads7950_state *st = iio_priv(indio_dev);
>> +int b_sent;
>> +
>> +b_sent = spi_sync(st->spi, >ring_msg);
> 
> hmm, I copied this from another driver, but spi_sync() in IRQ handler does 
> not sound like a good idea (spi_sync() can sleep). I will replace it with 
> spi_async().
Umm.. spi_async doesn't make any sense here...

Key thing is that here you are in a threaded interrupt so sleeping is fine and
here I'd imagine it leads to simpler code.

It's a bit old but https://lwn.net/Articles/302043/ has a good description.

> 
> 
>> +if (b_sent)
>> +goto done;
>> +
>> +iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
>> +   iio_get_time_ns(indio_dev));
>> +
>> +done:
>> +iio_trigger_notify_done(indio_dev->trig);
>> +
>> +return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>> +   struct iio_chan_spec const *chan,
>> +   int *val, int *val2, long m)
>> +{
>> +struct ti_ads7950_state *st = iio_priv(indio_dev);
>> +int ret;
>> +
>> +switch (m) {
>> +case IIO_CHAN_INFO_RAW:
>> +
>> +ret = iio_device_claim_direct_mode(indio_dev);
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = ti_ads7950_scan_direct(st, chan->address);
>> +iio_device_release_direct_mode(indio_dev);
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>> +return -EIO;
>> +
>> +*val = TI_ADS7950_EXTRACT(ret, 0, 12);
> 
> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
> versions of this chip. The 8- and 10-bit versions still return a
> 12-bit number where the last 4 or 2 bits are always 0. Should I be
> shifting the 12-bit value here based on the chip being used so that
> *val is 0-255 for 8-bit and 0-1023 for 10-bit? Or should this be
> *really* raw and not even use TI_ADS7950_EXTRACT() to mask the
> channel address bits?
>> +
>> +return IIO_VAL_INT;
>> +case IIO_CHAN_INFO_SCALE:
>> +ret = ti_ads7950_get_range(st);
>> +if (ret < 0)
>> +return ret;
>> +
>> +*val = ret;
>> +*val2 = chan->scan_type.realbits;
>> +
>> +return IIO_VAL_FRACTIONAL_LOG2;
>> +}
>> +
>> +return -EINVAL;
>> +}
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-24 Thread Jonathan Cameron 
On 21/11/16 19:52, David Lechner wrote:
> On 11/20/2016 12:28 PM, David Lechner wrote:
>> This adds a new driver for the TI ADS7950 family of ADC chips. These
>> communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
>> varieties.
>>
>> Signed-off-by: David Lechner 
>> ---
>>
>> v2 changes:
>>
>> * Got rid of XX wildcards - using ADS7950 everywhere
>> * Fixed some macro parentheses issues
>> * Added TI_ prefix to macros to match ti_ prefixes used elsewhere
>> * Added space in rx_buf for holding timestamp
>> * Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
>>   helper functions
>> * Don't use dev_info() at end of probe
>> * Minor spelling and code style fixes
>>
>>  drivers/iio/adc/Kconfig  |  13 ++
>>  drivers/iio/adc/Makefile |   1 +
>>  drivers/iio/adc/ti-ads7950.c | 488 
>> +++
>>  3 files changed, 502 insertions(+)
>>  create mode 100644 drivers/iio/adc/ti-ads7950.c
>>
> 
> ...
> 
>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>> new file mode 100644
>> index 000..d0b76bd
>> --- /dev/null
>> +++ b/drivers/iio/adc/ti-ads7950.c
> 
> ...
> 
>> +static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>> +{
>> +struct iio_poll_func *pf = p;
>> +struct iio_dev *indio_dev = pf->indio_dev;
>> +struct ti_ads7950_state *st = iio_priv(indio_dev);
>> +int b_sent;
>> +
>> +b_sent = spi_sync(st->spi, >ring_msg);
> 
> hmm, I copied this from another driver, but spi_sync() in IRQ handler does 
> not sound like a good idea (spi_sync() can sleep). I will replace it with 
> spi_async().
Umm.. spi_async doesn't make any sense here...

Key thing is that here you are in a threaded interrupt so sleeping is fine and
here I'd imagine it leads to simpler code.

It's a bit old but https://lwn.net/Articles/302043/ has a good description.

> 
> 
>> +if (b_sent)
>> +goto done;
>> +
>> +iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
>> +   iio_get_time_ns(indio_dev));
>> +
>> +done:
>> +iio_trigger_notify_done(indio_dev->trig);
>> +
>> +return IRQ_HANDLED;
>> +}
> 
> ...
> 
>> +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>> +   struct iio_chan_spec const *chan,
>> +   int *val, int *val2, long m)
>> +{
>> +struct ti_ads7950_state *st = iio_priv(indio_dev);
>> +int ret;
>> +
>> +switch (m) {
>> +case IIO_CHAN_INFO_RAW:
>> +
>> +ret = iio_device_claim_direct_mode(indio_dev);
>> +if (ret < 0)
>> +return ret;
>> +
>> +ret = ti_ads7950_scan_direct(st, chan->address);
>> +iio_device_release_direct_mode(indio_dev);
>> +if (ret < 0)
>> +return ret;
>> +
>> +if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>> +return -EIO;
>> +
>> +*val = TI_ADS7950_EXTRACT(ret, 0, 12);
> 
> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
> versions of this chip. The 8- and 10-bit versions still return a
> 12-bit number where the last 4 or 2 bits are always 0. Should I be
> shifting the 12-bit value here based on the chip being used so that
> *val is 0-255 for 8-bit and 0-1023 for 10-bit? Or should this be
> *really* raw and not even use TI_ADS7950_EXTRACT() to mask the
> channel address bits?
>> +
>> +return IIO_VAL_INT;
>> +case IIO_CHAN_INFO_SCALE:
>> +ret = ti_ads7950_get_range(st);
>> +if (ret < 0)
>> +return ret;
>> +
>> +*val = ret;
>> +*val2 = chan->scan_type.realbits;
>> +
>> +return IIO_VAL_FRACTIONAL_LOG2;
>> +}
>> +
>> +return -EINVAL;
>> +}
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-22 Thread David Lechner 

On 11/22/2016 01:23 AM, Jonathan Cameron wrote:



On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler 
 wrote:



+static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
+  struct iio_chan_spec const *chan,
+  int *val, int *val2, long m)
+{
+   struct ti_ads7950_state *st = iio_priv(indio_dev);
+   int ret;
+
+   switch (m) {
+   case IIO_CHAN_INFO_RAW:
+
+   ret = iio_device_claim_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   ret = ti_ads7950_scan_direct(st, chan->address);
+   iio_device_release_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
+   return -EIO;
+
+   *val = TI_ADS7950_EXTRACT(ret, 0, 12);


I'm not sure if I am doing this right. There are 8- 10- and 12-bit

versions of

this chip. The 8- and 10-bit versions still return a 12-bit number

where the

last 4 or 2 bits are always 0. Should I be shifting the 12-bit value

here

based on the chip being used so that *val is 0-255 for 8-bit and

0-1023 for

10-bit? Or should this be *really* raw and not even use

TI_ADS7950_EXTRACT()

to mask the channel address bits?


I'd shift and adjust _SCALE so that *val * scale gives mV

It would also be fine to not do anything and let userspace deal with shifting 
and masking for
buffered data.  Non buffered obviously still needs shifting and masking though!


I have sent a v3 patch already that does exactly this. Buffered data is 
untouched and unbuffered data is now correct with shifting and masking.




I'd slightly prefer the doing nothing route but don't really care as both are 
valid uses of the ABI.

Jonathan



+
+   return IIO_VAL_INT;
+   case IIO_CHAN_INFO_SCALE:
+   ret = ti_ads7950_get_range(st);
+   if (ret < 0)
+   return ret;
+
+   *val = ret;
+   *val2 = chan->scan_type.realbits;
+
+   return IIO_VAL_FRACTIONAL_LOG2;
+   }
+
+   return -EINVAL;
+}

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-22 Thread David Lechner 

On 11/22/2016 01:23 AM, Jonathan Cameron wrote:



On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler 
 wrote:



+static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
+  struct iio_chan_spec const *chan,
+  int *val, int *val2, long m)
+{
+   struct ti_ads7950_state *st = iio_priv(indio_dev);
+   int ret;
+
+   switch (m) {
+   case IIO_CHAN_INFO_RAW:
+
+   ret = iio_device_claim_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   ret = ti_ads7950_scan_direct(st, chan->address);
+   iio_device_release_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
+   return -EIO;
+
+   *val = TI_ADS7950_EXTRACT(ret, 0, 12);


I'm not sure if I am doing this right. There are 8- 10- and 12-bit

versions of

this chip. The 8- and 10-bit versions still return a 12-bit number

where the

last 4 or 2 bits are always 0. Should I be shifting the 12-bit value

here

based on the chip being used so that *val is 0-255 for 8-bit and

0-1023 for

10-bit? Or should this be *really* raw and not even use

TI_ADS7950_EXTRACT()

to mask the channel address bits?


I'd shift and adjust _SCALE so that *val * scale gives mV

It would also be fine to not do anything and let userspace deal with shifting 
and masking for
buffered data.  Non buffered obviously still needs shifting and masking though!


I have sent a v3 patch already that does exactly this. Buffered data is 
untouched and unbuffered data is now correct with shifting and masking.




I'd slightly prefer the doing nothing route but don't really care as both are 
valid uses of the ABI.

Jonathan



+
+   return IIO_VAL_INT;
+   case IIO_CHAN_INFO_SCALE:
+   ret = ti_ads7950_get_range(st);
+   if (ret < 0)
+   return ret;
+
+   *val = ret;
+   *val2 = chan->scan_type.realbits;
+
+   return IIO_VAL_FRACTIONAL_LOG2;
+   }
+
+   return -EINVAL;
+}

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-21 Thread Jonathan Cameron 


On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler 
 wrote:
>
>> > +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>> > + struct iio_chan_spec const *chan,
>> > + int *val, int *val2, long m)
>> > +{
>> > +  struct ti_ads7950_state *st = iio_priv(indio_dev);
>> > +  int ret;
>> > +
>> > +  switch (m) {
>> > +  case IIO_CHAN_INFO_RAW:
>> > +
>> > +  ret = iio_device_claim_direct_mode(indio_dev);
>> > +  if (ret < 0)
>> > +  return ret;
>> > +
>> > +  ret = ti_ads7950_scan_direct(st, chan->address);
>> > +  iio_device_release_direct_mode(indio_dev);
>> > +  if (ret < 0)
>> > +  return ret;
>> > +
>> > +  if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>> > +  return -EIO;
>> > +
>> > +  *val = TI_ADS7950_EXTRACT(ret, 0, 12);
>> 
>> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
>versions of
>> this chip. The 8- and 10-bit versions still return a 12-bit number
>where the
>> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value
>here
>> based on the chip being used so that *val is 0-255 for 8-bit and
>0-1023 for
>> 10-bit? Or should this be *really* raw and not even use
>TI_ADS7950_EXTRACT()
>> to mask the channel address bits?
>
>I'd shift and adjust _SCALE so that *val * scale gives mV
It would also be fine to not do anything and let userspace deal with shifting 
and masking for
buffered data.  Non buffered obviously still needs shifting and masking though!

I'd slightly prefer the doing nothing route but don't really care as both are 
valid uses of the ABI.

Jonathan
> 
>> > +
>> > +  return IIO_VAL_INT;
>> > +  case IIO_CHAN_INFO_SCALE:
>> > +  ret = ti_ads7950_get_range(st);
>> > +  if (ret < 0)
>> > +  return ret;
>> > +
>> > +  *val = ret;
>> > +  *val2 = chan->scan_type.realbits;
>> > +
>> > +  return IIO_VAL_FRACTIONAL_LOG2;
>> > +  }
>> > +
>> > +  return -EINVAL;
>> > +}

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-21 Thread Jonathan Cameron 


On 21 November 2016 22:54:24 GMT+00:00, Peter Meerwald-Stadler 
 wrote:
>
>> > +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
>> > + struct iio_chan_spec const *chan,
>> > + int *val, int *val2, long m)
>> > +{
>> > +  struct ti_ads7950_state *st = iio_priv(indio_dev);
>> > +  int ret;
>> > +
>> > +  switch (m) {
>> > +  case IIO_CHAN_INFO_RAW:
>> > +
>> > +  ret = iio_device_claim_direct_mode(indio_dev);
>> > +  if (ret < 0)
>> > +  return ret;
>> > +
>> > +  ret = ti_ads7950_scan_direct(st, chan->address);
>> > +  iio_device_release_direct_mode(indio_dev);
>> > +  if (ret < 0)
>> > +  return ret;
>> > +
>> > +  if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
>> > +  return -EIO;
>> > +
>> > +  *val = TI_ADS7950_EXTRACT(ret, 0, 12);
>> 
>> I'm not sure if I am doing this right. There are 8- 10- and 12-bit
>versions of
>> this chip. The 8- and 10-bit versions still return a 12-bit number
>where the
>> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value
>here
>> based on the chip being used so that *val is 0-255 for 8-bit and
>0-1023 for
>> 10-bit? Or should this be *really* raw and not even use
>TI_ADS7950_EXTRACT()
>> to mask the channel address bits?
>
>I'd shift and adjust _SCALE so that *val * scale gives mV
It would also be fine to not do anything and let userspace deal with shifting 
and masking for
buffered data.  Non buffered obviously still needs shifting and masking though!

I'd slightly prefer the doing nothing route but don't really care as both are 
valid uses of the ABI.

Jonathan
> 
>> > +
>> > +  return IIO_VAL_INT;
>> > +  case IIO_CHAN_INFO_SCALE:
>> > +  ret = ti_ads7950_get_range(st);
>> > +  if (ret < 0)
>> > +  return ret;
>> > +
>> > +  *val = ret;
>> > +  *val2 = chan->scan_type.realbits;
>> > +
>> > +  return IIO_VAL_FRACTIONAL_LOG2;
>> > +  }
>> > +
>> > +  return -EINVAL;
>> > +}

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-21 Thread Peter Meerwald-Stadler 

> > +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
> > +  struct iio_chan_spec const *chan,
> > +  int *val, int *val2, long m)
> > +{
> > +   struct ti_ads7950_state *st = iio_priv(indio_dev);
> > +   int ret;
> > +
> > +   switch (m) {
> > +   case IIO_CHAN_INFO_RAW:
> > +
> > +   ret = iio_device_claim_direct_mode(indio_dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = ti_ads7950_scan_direct(st, chan->address);
> > +   iio_device_release_direct_mode(indio_dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
> > +   return -EIO;
> > +
> > +   *val = TI_ADS7950_EXTRACT(ret, 0, 12);
> 
> I'm not sure if I am doing this right. There are 8- 10- and 12-bit versions of
> this chip. The 8- and 10-bit versions still return a 12-bit number where the
> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value here
> based on the chip being used so that *val is 0-255 for 8-bit and 0-1023 for
> 10-bit? Or should this be *really* raw and not even use TI_ADS7950_EXTRACT()
> to mask the channel address bits?

I'd shift and adjust _SCALE so that *val * scale gives mV
 
> > +
> > +   return IIO_VAL_INT;
> > +   case IIO_CHAN_INFO_SCALE:
> > +   ret = ti_ads7950_get_range(st);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   *val = ret;
> > +   *val2 = chan->scan_type.realbits;
> > +
> > +   return IIO_VAL_FRACTIONAL_LOG2;
> > +   }
> > +
> > +   return -EINVAL;
> > +}

-- 

Peter Meerwald-Stadler
+43-664-218 (mobile)

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-21 Thread Peter Meerwald-Stadler 

> > +static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
> > +  struct iio_chan_spec const *chan,
> > +  int *val, int *val2, long m)
> > +{
> > +   struct ti_ads7950_state *st = iio_priv(indio_dev);
> > +   int ret;
> > +
> > +   switch (m) {
> > +   case IIO_CHAN_INFO_RAW:
> > +
> > +   ret = iio_device_claim_direct_mode(indio_dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = ti_ads7950_scan_direct(st, chan->address);
> > +   iio_device_release_direct_mode(indio_dev);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
> > +   return -EIO;
> > +
> > +   *val = TI_ADS7950_EXTRACT(ret, 0, 12);
> 
> I'm not sure if I am doing this right. There are 8- 10- and 12-bit versions of
> this chip. The 8- and 10-bit versions still return a 12-bit number where the
> last 4 or 2 bits are always 0. Should I be shifting the 12-bit value here
> based on the chip being used so that *val is 0-255 for 8-bit and 0-1023 for
> 10-bit? Or should this be *really* raw and not even use TI_ADS7950_EXTRACT()
> to mask the channel address bits?

I'd shift and adjust _SCALE so that *val * scale gives mV
 
> > +
> > +   return IIO_VAL_INT;
> > +   case IIO_CHAN_INFO_SCALE:
> > +   ret = ti_ads7950_get_range(st);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   *val = ret;
> > +   *val2 = chan->scan_type.realbits;
> > +
> > +   return IIO_VAL_FRACTIONAL_LOG2;
> > +   }
> > +
> > +   return -EINVAL;
> > +}

-- 

Peter Meerwald-Stadler
+43-664-218 (mobile)

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-21 Thread David Lechner 

On 11/20/2016 12:28 PM, David Lechner wrote:

This adds a new driver for the TI ADS7950 family of ADC chips. These
communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
varieties.

Signed-off-by: David Lechner 
---

v2 changes:

* Got rid of XX wildcards - using ADS7950 everywhere
* Fixed some macro parentheses issues
* Added TI_ prefix to macros to match ti_ prefixes used elsewhere
* Added space in rx_buf for holding timestamp
* Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
  helper functions
* Don't use dev_info() at end of probe
* Minor spelling and code style fixes

 drivers/iio/adc/Kconfig  |  13 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ti-ads7950.c | 488 +++
 3 files changed, 502 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads7950.c



...


diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
new file mode 100644
index 000..d0b76bd
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7950.c


...


+static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct ti_ads7950_state *st = iio_priv(indio_dev);
+   int b_sent;
+
+   b_sent = spi_sync(st->spi, >ring_msg);


hmm, I copied this from another driver, but spi_sync() in IRQ handler 
does not sound like a good idea (spi_sync() can sleep). I will replace 
it with spi_async().




+   if (b_sent)
+   goto done;
+
+   iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
+  iio_get_time_ns(indio_dev));
+
+done:
+   iio_trigger_notify_done(indio_dev->trig);
+
+   return IRQ_HANDLED;
+}


...


+static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
+  struct iio_chan_spec const *chan,
+  int *val, int *val2, long m)
+{
+   struct ti_ads7950_state *st = iio_priv(indio_dev);
+   int ret;
+
+   switch (m) {
+   case IIO_CHAN_INFO_RAW:
+
+   ret = iio_device_claim_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   ret = ti_ads7950_scan_direct(st, chan->address);
+   iio_device_release_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
+   return -EIO;
+
+   *val = TI_ADS7950_EXTRACT(ret, 0, 12);


I'm not sure if I am doing this right. There are 8- 10- and 12-bit 
versions of this chip. The 8- and 10-bit versions still return a 12-bit 
number where the last 4 or 2 bits are always 0. Should I be shifting the 
12-bit value here based on the chip being used so that *val is 0-255 for 
8-bit and 0-1023 for 10-bit? Or should this be *really* raw and not even 
use TI_ADS7950_EXTRACT() to mask the channel address bits?



+
+   return IIO_VAL_INT;
+   case IIO_CHAN_INFO_SCALE:
+   ret = ti_ads7950_get_range(st);
+   if (ret < 0)
+   return ret;
+
+   *val = ret;
+   *val2 = chan->scan_type.realbits;
+
+   return IIO_VAL_FRACTIONAL_LOG2;
+   }
+
+   return -EINVAL;
+}

Re: [PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-21 Thread David Lechner 

On 11/20/2016 12:28 PM, David Lechner wrote:

This adds a new driver for the TI ADS7950 family of ADC chips. These
communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
varieties.

Signed-off-by: David Lechner 
---

v2 changes:

* Got rid of XX wildcards - using ADS7950 everywhere
* Fixed some macro parentheses issues
* Added TI_ prefix to macros to match ti_ prefixes used elsewhere
* Added space in rx_buf for holding timestamp
* Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
  helper functions
* Don't use dev_info() at end of probe
* Minor spelling and code style fixes

 drivers/iio/adc/Kconfig  |  13 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ti-ads7950.c | 488 +++
 3 files changed, 502 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads7950.c



...


diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
new file mode 100644
index 000..d0b76bd
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7950.c


...


+static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
+{
+   struct iio_poll_func *pf = p;
+   struct iio_dev *indio_dev = pf->indio_dev;
+   struct ti_ads7950_state *st = iio_priv(indio_dev);
+   int b_sent;
+
+   b_sent = spi_sync(st->spi, >ring_msg);


hmm, I copied this from another driver, but spi_sync() in IRQ handler 
does not sound like a good idea (spi_sync() can sleep). I will replace 
it with spi_async().




+   if (b_sent)
+   goto done;
+
+   iio_push_to_buffers_with_timestamp(indio_dev, st->rx_buf,
+  iio_get_time_ns(indio_dev));
+
+done:
+   iio_trigger_notify_done(indio_dev->trig);
+
+   return IRQ_HANDLED;
+}


...


+static int ti_ads7950_read_raw(struct iio_dev *indio_dev,
+  struct iio_chan_spec const *chan,
+  int *val, int *val2, long m)
+{
+   struct ti_ads7950_state *st = iio_priv(indio_dev);
+   int ret;
+
+   switch (m) {
+   case IIO_CHAN_INFO_RAW:
+
+   ret = iio_device_claim_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   ret = ti_ads7950_scan_direct(st, chan->address);
+   iio_device_release_direct_mode(indio_dev);
+   if (ret < 0)
+   return ret;
+
+   if (chan->address != TI_ADS7950_EXTRACT(ret, 12, 4))
+   return -EIO;
+
+   *val = TI_ADS7950_EXTRACT(ret, 0, 12);


I'm not sure if I am doing this right. There are 8- 10- and 12-bit 
versions of this chip. The 8- and 10-bit versions still return a 12-bit 
number where the last 4 or 2 bits are always 0. Should I be shifting the 
12-bit value here based on the chip being used so that *val is 0-255 for 
8-bit and 0-1023 for 10-bit? Or should this be *really* raw and not even 
use TI_ADS7950_EXTRACT() to mask the channel address bits?



+
+   return IIO_VAL_INT;
+   case IIO_CHAN_INFO_SCALE:
+   ret = ti_ads7950_get_range(st);
+   if (ret < 0)
+   return ret;
+
+   *val = ret;
+   *val2 = chan->scan_type.realbits;
+
+   return IIO_VAL_FRACTIONAL_LOG2;
+   }
+
+   return -EINVAL;
+}

[PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-20 Thread David Lechner 
This adds a new driver for the TI ADS7950 family of ADC chips. These
communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
varieties.

Signed-off-by: David Lechner 
---

v2 changes:

* Got rid of XX wildcards - using ADS7950 everywhere
* Fixed some macro parentheses issues
* Added TI_ prefix to macros to match ti_ prefixes used elsewhere
* Added space in rx_buf for holding timestamp
* Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
  helper functions
* Don't use dev_info() at end of probe
* Minor spelling and code style fixes

 drivers/iio/adc/Kconfig  |  13 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ti-ads7950.c | 488 +++
 3 files changed, 502 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads7950.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6bbee0b..26b32f0 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -527,6 +527,19 @@ config TI_ADS1015
  This driver can also be built as a module. If so, the module will be
  called ti-ads1015.
 
+config TI_ADS7950
+   tristate "Texas Instruments ADS7950 ADC driver"
+   depends on SPI
+   select IIO_BUFFER
+   select IIO_TRIGGERED_BUFFER
+   help
+ Say yes here to build support for Texas Instruments ADS7950, ADS7951,
+ ADS7952, ADS7953, ADS7954, ADS7955, ADS7956, ADS7957, ADS7958, 
ADS7959.
+ ADS7960, ADS7961.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ti-ads7950.
+
 config TI_ADS8688
tristate "Texas Instruments ADS8688"
depends on SPI && OF
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9391217..1966801 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
+obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
 obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
new file mode 100644
index 000..d0b76bd
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -0,0 +1,488 @@
+/*
+ * Texas Instruments ADS7950 SPI ADC driver
+ *
+ * Copyright 2016 David Lechner 
+ *
+ * Based on iio/ad7923.c:
+ * Copyright 2011 Analog Devices Inc
+ * Copyright 2012 CS Systemes d'Information
+ *
+ * And also on hwmon/ads79xx.c
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define TI_ADS7950_CR_MANUAL   BIT(12)
+#define TI_ADS7950_CR_WRITEBIT(11)
+#define TI_ADS7950_CR_CHAN(ch) ((ch) << 7)
+#define TI_ADS7950_CR_RANGE_5V BIT(6)
+
+#define TI_ADS7950_MAX_CHAN16
+
+#define TI_ADS7950_TIMESTAMP_SIZE (sizeof(int64_t) / sizeof(__be16))
+
+/* val = value, dec = left shift, bits = number of bits of the mask */
+#define TI_ADS7950_EXTRACT(val, dec, bits) \
+   (((val) >> (dec)) & ((1 << (bits)) - 1))
+
+struct ti_ads7950_state {
+   struct spi_device   *spi;
+   struct spi_transfer ring_xfer[TI_ADS7950_MAX_CHAN + 2];
+   struct spi_transfer scan_single_xfer[3];
+   struct spi_message  ring_msg;
+   struct spi_message  scan_single_msg;
+
+   struct regulator*reg;
+
+   unsigned intsettings;
+
+   /*
+* DMA (thus cache coherency maintenance) requires the
+* transfer buffers to live in their own cache lines.
+*/
+   __be16  rx_buf[TI_ADS7950_MAX_CHAN + TI_ADS7950_TIMESTAMP_SIZE]
+   cacheline_aligned;
+   __be16  tx_buf[TI_ADS7950_MAX_CHAN];
+};
+
+struct ti_ads7950_chip_info {
+   const struct iio_chan_spec *channels;
+   unsigned int num_channels;
+};
+
+enum ti_ads7950_id {
+   TI_ADS7950,
+   TI_ADS7951,
+   TI_ADS7952,
+   TI_ADS7953,
+   TI_ADS7954,
+   TI_ADS7955,
+   TI_ADS7956,
+   TI_ADS7957,
+   TI_ADS7958,
+   TI_ADS7959,
+   TI_ADS7960,
+   TI_ADS7961,
+};
+
+#define TI_ADS7950_V_CHAN(index, bits)

[PATCH v2] iio: adc: New driver for TI ADS7950 chips

2016-11-20 Thread David Lechner 
This adds a new driver for the TI ADS7950 family of ADC chips. These
communicate using SPI and come in 8/10/12-bit and 4/8/12/16 channel
varieties.

Signed-off-by: David Lechner 
---

v2 changes:

* Got rid of XX wildcards - using ADS7950 everywhere
* Fixed some macro parentheses issues
* Added TI_ prefix to macros to match ti_ prefixes used elsewhere
* Added space in rx_buf for holding timestamp
* Use iio_device_claim_direct_mode() and spi_message_init_with_transfers()
  helper functions
* Don't use dev_info() at end of probe
* Minor spelling and code style fixes

 drivers/iio/adc/Kconfig  |  13 ++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/ti-ads7950.c | 488 +++
 3 files changed, 502 insertions(+)
 create mode 100644 drivers/iio/adc/ti-ads7950.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 6bbee0b..26b32f0 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -527,6 +527,19 @@ config TI_ADS1015
  This driver can also be built as a module. If so, the module will be
  called ti-ads1015.
 
+config TI_ADS7950
+   tristate "Texas Instruments ADS7950 ADC driver"
+   depends on SPI
+   select IIO_BUFFER
+   select IIO_TRIGGERED_BUFFER
+   help
+ Say yes here to build support for Texas Instruments ADS7950, ADS7951,
+ ADS7952, ADS7953, ADS7954, ADS7955, ADS7956, ADS7957, ADS7958, 
ADS7959.
+ ADS7960, ADS7961.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ti-ads7950.
+
 config TI_ADS8688
tristate "Texas Instruments ADS8688"
depends on SPI && OF
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 9391217..1966801 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_TI_ADC12138) += ti-adc12138.o
 obj-$(CONFIG_TI_ADC128S052) += ti-adc128s052.o
 obj-$(CONFIG_TI_ADC161S626) += ti-adc161s626.o
 obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
+obj-$(CONFIG_TI_ADS7950) += ti-ads7950.o
 obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
new file mode 100644
index 000..d0b76bd
--- /dev/null
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -0,0 +1,488 @@
+/*
+ * Texas Instruments ADS7950 SPI ADC driver
+ *
+ * Copyright 2016 David Lechner 
+ *
+ * Based on iio/ad7923.c:
+ * Copyright 2011 Analog Devices Inc
+ * Copyright 2012 CS Systemes d'Information
+ *
+ * And also on hwmon/ads79xx.c
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ * Nishanth Menon
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define TI_ADS7950_CR_MANUAL   BIT(12)
+#define TI_ADS7950_CR_WRITEBIT(11)
+#define TI_ADS7950_CR_CHAN(ch) ((ch) << 7)
+#define TI_ADS7950_CR_RANGE_5V BIT(6)
+
+#define TI_ADS7950_MAX_CHAN16
+
+#define TI_ADS7950_TIMESTAMP_SIZE (sizeof(int64_t) / sizeof(__be16))
+
+/* val = value, dec = left shift, bits = number of bits of the mask */
+#define TI_ADS7950_EXTRACT(val, dec, bits) \
+   (((val) >> (dec)) & ((1 << (bits)) - 1))
+
+struct ti_ads7950_state {
+   struct spi_device   *spi;
+   struct spi_transfer ring_xfer[TI_ADS7950_MAX_CHAN + 2];
+   struct spi_transfer scan_single_xfer[3];
+   struct spi_message  ring_msg;
+   struct spi_message  scan_single_msg;
+
+   struct regulator*reg;
+
+   unsigned intsettings;
+
+   /*
+* DMA (thus cache coherency maintenance) requires the
+* transfer buffers to live in their own cache lines.
+*/
+   __be16  rx_buf[TI_ADS7950_MAX_CHAN + TI_ADS7950_TIMESTAMP_SIZE]
+   cacheline_aligned;
+   __be16  tx_buf[TI_ADS7950_MAX_CHAN];
+};
+
+struct ti_ads7950_chip_info {
+   const struct iio_chan_spec *channels;
+   unsigned int num_channels;
+};
+
+enum ti_ads7950_id {
+   TI_ADS7950,
+   TI_ADS7951,
+   TI_ADS7952,
+   TI_ADS7953,
+   TI_ADS7954,
+   TI_ADS7955,
+   TI_ADS7956,
+   TI_ADS7957,
+   TI_ADS7958,
+   TI_ADS7959,
+   TI_ADS7960,
+   TI_ADS7961,
+};
+
+#define TI_ADS7950_V_CHAN(index, bits) \
+{