From: Jonathan Cameron <jic23@cam.ac.uk>
To: Barry Song <21cnbao@gmail.com>
Cc: gregkh@suse.de, linux-iio@vger.kernel.org
Subject: Re: [PATCH 1/3] adis16300: fix some minor issues and clean-up
Date: Fri, 04 Jun 2010 11:56:05 +0100 [thread overview]
Message-ID: <4C08DBC5.1040206@cam.ac.uk> (raw)
In-Reply-To: <1275643194-25328-2-git-send-email-21cnbao@gmail.com>
On 06/04/10 10:19, Barry Song wrote:
> 1. add delay between spi transfers
> 2. move burst read to ring function
> 3. clean-up
I think I'm right in saying that, this lot will appear in the commit message.
Comments like this should go below.
Otherwise, looks fine to me.
>
> Signed-off-by: Barry Song <21cnbao@gmail.com>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
Comments here.
> drivers/staging/iio/imu/adis16300.h | 6 -
> drivers/staging/iio/imu/adis16300_core.c | 151 +++++++++++++-----------------
> drivers/staging/iio/imu/adis16300_ring.c | 54 ++++++++++-
> 3 files changed, 119 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/staging/iio/imu/adis16300.h b/drivers/staging/iio/imu/adis16300.h
> index 1c7ea5c..b050067 100644
> --- a/drivers/staging/iio/imu/adis16300.h
> +++ b/drivers/staging/iio/imu/adis16300.h
> @@ -115,14 +115,8 @@ struct adis16300_state {
> struct mutex buf_lock;
> };
>
> -int adis16300_spi_read_burst(struct device *dev, u8 *rx);
> -
> int adis16300_set_irq(struct device *dev, bool enable);
>
> -int adis16300_reset(struct device *dev);
> -
> -int adis16300_check_status(struct device *dev);
> -
> #ifdef CONFIG_IIO_RING_BUFFER
> /* At the moment triggers are only used for ring buffer
> * filling. This may change!
> diff --git a/drivers/staging/iio/imu/adis16300_core.c b/drivers/staging/iio/imu/adis16300_core.c
> index 5a7e5ef..28667e8 100644
> --- a/drivers/staging/iio/imu/adis16300_core.c
> +++ b/drivers/staging/iio/imu/adis16300_core.c
> @@ -29,10 +29,7 @@
>
> #define DRIVER_NAME "adis16300"
>
> -/* At the moment the spi framework doesn't allow global setting of cs_change.
> - * It's in the likely to be added comment at the top of spi.h.
> - * This means that use cannot be made of spi_write etc.
> - */
> +static int adis16300_check_status(struct device *dev);
>
> /**
> * adis16300_spi_write_reg_8() - write single byte to a register
> @@ -79,11 +76,13 @@ static int adis16300_spi_write_reg_16(struct device *dev,
> .bits_per_word = 8,
> .len = 2,
> .cs_change = 1,
> + .delay_usecs = 75,
> }, {
> .tx_buf = st->tx + 2,
> .bits_per_word = 8,
> .len = 2,
> .cs_change = 1,
> + .delay_usecs = 75,
> },
> };
>
> @@ -122,12 +121,14 @@ static int adis16300_spi_read_reg_16(struct device *dev,
> .tx_buf = st->tx,
> .bits_per_word = 8,
> .len = 2,
> - .cs_change = 0,
> + .cs_change = 1,
> + .delay_usecs = 75,
> }, {
> .rx_buf = st->rx,
> .bits_per_word = 8,
> .len = 2,
> - .cs_change = 0,
> + .cs_change = 1,
> + .delay_usecs = 75,
> },
> };
>
> @@ -154,54 +155,6 @@ error_ret:
> return ret;
> }
>
> -/**
> - * adis16300_spi_read_burst() - read all data registers
> - * @dev: device associated with child of actual device (iio_dev or iio_trig)
> - * @rx: somewhere to pass back the value read (min size is 24 bytes)
> - **/
> -int adis16300_spi_read_burst(struct device *dev, u8 *rx)
> -{
> - struct spi_message msg;
> - struct iio_dev *indio_dev = dev_get_drvdata(dev);
> - struct adis16300_state *st = iio_dev_get_devdata(indio_dev);
> - u32 old_speed_hz = st->us->max_speed_hz;
> - int ret;
> -
> - struct spi_transfer xfers[] = {
> - {
> - .tx_buf = st->tx,
> - .bits_per_word = 8,
> - .len = 2,
> - .cs_change = 0,
> - }, {
> - .rx_buf = rx,
> - .bits_per_word = 8,
> - .len = 18,
> - .cs_change = 0,
> - },
> - };
> -
> - mutex_lock(&st->buf_lock);
> - st->tx[0] = ADIS16300_READ_REG(ADIS16300_GLOB_CMD);
> - st->tx[1] = 0;
> -
> - spi_message_init(&msg);
> - spi_message_add_tail(&xfers[0], &msg);
> - spi_message_add_tail(&xfers[1], &msg);
> -
> - st->us->max_speed_hz = min(ADIS16300_SPI_BURST, old_speed_hz);
> - spi_setup(st->us);
> -
> - ret = spi_sync(st->us, &msg);
> - if (ret)
> - dev_err(&st->us->dev, "problem when burst reading");
> -
> - st->us->max_speed_hz = old_speed_hz;
> - spi_setup(st->us);
> - mutex_unlock(&st->buf_lock);
> - return ret;
> -}
> -
> static ssize_t adis16300_spi_read_signed(struct device *dev,
> struct device_attribute *attr,
> char *buf,
> @@ -240,6 +193,24 @@ static ssize_t adis16300_read_12bit_unsigned(struct device *dev,
> return sprintf(buf, "%u\n", val & 0x0FFF);
> }
>
> +static ssize_t adis16300_read_14bit_unsigned(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int ret;
> + u16 val = 0;
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + ret = adis16300_spi_read_reg_16(dev, this_attr->address, &val);
> + if (ret)
> + return ret;
> +
> + if (val & ADIS16300_ERROR_ACTIVE)
> + adis16300_check_status(dev);
> +
> + return sprintf(buf, "%u\n", val & 0x3FFF);
> +}
> +
> static ssize_t adis16300_read_14bit_signed(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -356,6 +327,18 @@ static ssize_t adis16300_write_frequency(struct device *dev,
> return ret ? ret : len;
> }
>
> +static int adis16300_reset(struct device *dev)
> +{
> + int ret;
> + ret = adis16300_spi_write_reg_8(dev,
> + ADIS16300_GLOB_CMD,
> + ADIS16300_GLOB_CMD_SW_RESET);
> + if (ret)
> + dev_err(dev, "problem resetting device");
> +
> + return ret;
> +}
> +
> static ssize_t adis16300_write_reset(struct device *dev,
> struct device_attribute *attr,
> const char *buf, size_t len)
> @@ -371,8 +354,6 @@ static ssize_t adis16300_write_reset(struct device *dev,
> return -1;
> }
>
> -
> -
> int adis16300_set_irq(struct device *dev, bool enable)
> {
> int ret;
> @@ -396,32 +377,37 @@ error_ret:
> return ret;
> }
>
> -int adis16300_reset(struct device *dev)
> +/* Power down the device */
> +static int adis16300_stop_device(struct device *dev)
> {
> int ret;
> - ret = adis16300_spi_write_reg_8(dev,
> - ADIS16300_GLOB_CMD,
> - ADIS16300_GLOB_CMD_SW_RESET);
> + u16 val = ADIS16300_SLP_CNT_POWER_OFF;
> +
> + ret = adis16300_spi_write_reg_16(dev, ADIS16300_SLP_CNT, val);
> if (ret)
> - dev_err(dev, "problem resetting device");
> + dev_err(dev, "problem with turning device off: SLP_CNT");
>
> return ret;
> }
>
> -/* Power down the device */
> -static int adis16300_stop_device(struct device *dev)
> +static int adis16300_self_test(struct device *dev)
> {
> int ret;
> - u16 val = ADIS16300_SLP_CNT_POWER_OFF;
> + ret = adis16300_spi_write_reg_16(dev,
> + ADIS16300_MSC_CTRL,
> + ADIS16300_MSC_CTRL_MEM_TEST);
> + if (ret) {
> + dev_err(dev, "problem starting self test");
> + goto err_ret;
> + }
>
> - ret = adis16300_spi_write_reg_16(dev, ADIS16300_SLP_CNT, val);
> - if (ret)
> - dev_err(dev, "problem with turning device off: SLP_CNT");
> + adis16300_check_status(dev);
>
> +err_ret:
> return ret;
> }
>
> -int adis16300_check_status(struct device *dev)
> +static int adis16300_check_status(struct device *dev)
> {
> u16 status;
> int ret;
> @@ -483,6 +469,11 @@ static int adis16300_initial_setup(struct adis16300_state *st)
> }
>
> /* Do self test */
> + ret = adis16300_self_test(dev);
> + if (ret) {
> + dev_err(dev, "self test failure");
> + goto err_ret;
> + }
>
> /* Read status register to check the result */
> ret = adis16300_check_status(dev);
> @@ -526,7 +517,7 @@ static IIO_DEV_ATTR_ACCEL_Z_OFFSET(S_IWUSR | S_IRUGO,
> adis16300_write_16bit,
> ADIS16300_ZACCL_OFF);
>
> -static IIO_DEV_ATTR_IN_NAMED_RAW(supply, adis16300_read_14bit_signed,
> +static IIO_DEV_ATTR_IN_NAMED_RAW(supply, adis16300_read_14bit_unsigned,
> ADIS16300_SUPPLY_OUT);
> static IIO_CONST_ATTR(in_supply_scale, "0.00242");
>
> @@ -548,7 +539,7 @@ static IIO_DEV_ATTR_INCLI_Y(adis16300_read_13bit_signed,
> ADIS16300_YINCLI_OUT);
> static IIO_CONST_ATTR(incli_scale, "0.044 d");
>
> -static IIO_DEV_ATTR_TEMP_RAW(adis16300_read_12bit_signed);
> +static IIO_DEV_ATTR_TEMP_RAW(adis16300_read_12bit_unsigned);
> static IIO_CONST_ATTR(temp_offset, "198.16 K");
> static IIO_CONST_ATTR(temp_scale, "0.14 K");
>
> @@ -659,15 +650,7 @@ static int __devinit adis16300_probe(struct spi_device *spi)
> goto error_unreg_ring_funcs;
> }
>
> - if (spi->irq && gpio_is_valid(irq_to_gpio(spi->irq)) > 0) {
> -#if 0 /* fixme: here we should support */
> - iio_init_work_cont(&st->work_cont_thresh,
> - NULL,
> - adis16300_thresh_handler_bh_no_check,
> - 0,
> - 0,
> - st);
> -#endif
> + if (spi->irq) {
> ret = iio_register_interrupt_line(spi->irq,
> st->indio_dev,
> 0,
> @@ -688,10 +671,9 @@ static int __devinit adis16300_probe(struct spi_device *spi)
> return 0;
>
> error_remove_trigger:
> - if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
> - adis16300_remove_trigger(st->indio_dev);
> + adis16300_remove_trigger(st->indio_dev);
> error_unregister_line:
> - if (st->indio_dev->modes & INDIO_RING_TRIGGERED)
> + if (spi->irq)
> iio_unregister_interrupt_line(st->indio_dev, 0);
> error_uninitialize_ring:
> adis16300_uninitialize_ring(st->indio_dev->ring);
> @@ -712,7 +694,6 @@ error_ret:
> return ret;
> }
>
> -/* fixme, confirm ordering in this function */
> static int adis16300_remove(struct spi_device *spi)
> {
> int ret;
> @@ -726,12 +707,12 @@ static int adis16300_remove(struct spi_device *spi)
> flush_scheduled_work();
>
> adis16300_remove_trigger(indio_dev);
> - if (spi->irq && gpio_is_valid(irq_to_gpio(spi->irq)) > 0)
> + if (spi->irq)
> iio_unregister_interrupt_line(indio_dev, 0);
>
> adis16300_uninitialize_ring(indio_dev->ring);
> - adis16300_unconfigure_ring(indio_dev);
> iio_device_unregister(indio_dev);
> + adis16300_unconfigure_ring(indio_dev);
> kfree(st->tx);
> kfree(st->rx);
> kfree(st);
> diff --git a/drivers/staging/iio/imu/adis16300_ring.c b/drivers/staging/iio/imu/adis16300_ring.c
> index 76cf8a6..17ceb72 100644
> --- a/drivers/staging/iio/imu/adis16300_ring.c
> +++ b/drivers/staging/iio/imu/adis16300_ring.c
> @@ -26,7 +27,7 @@ static inline u16 combine_8_to_16(u8 lower, u8 upper)
> return _lower | (_upper << 8);
> }
>
> -static IIO_SCAN_EL_C(supply, ADIS16300_SCAN_SUPPLY, IIO_SIGNED(14),
> +static IIO_SCAN_EL_C(supply, ADIS16300_SCAN_SUPPLY, IIO_UNSIGNED(14),
> ADIS16300_SUPPLY_OUT, NULL);
>
> static IIO_SCAN_EL_C(gyro_x, ADIS16300_SCAN_GYRO_X, IIO_SIGNED(14),
> @@ -39,9 +40,9 @@ static IIO_SCAN_EL_C(accel_y, ADIS16300_SCAN_ACC_Y, IIO_SIGNED(14),
> static IIO_SCAN_EL_C(accel_z, ADIS16300_SCAN_ACC_Z, IIO_SIGNED(14),
> ADIS16300_ZACCL_OUT, NULL);
>
> -static IIO_SCAN_EL_C(temp, ADIS16300_SCAN_TEMP, IIO_SIGNED(12),
> +static IIO_SCAN_EL_C(temp, ADIS16300_SCAN_TEMP, IIO_UNSIGNED(12),
> ADIS16300_TEMP_OUT, NULL);
> -static IIO_SCAN_EL_C(adc_0, ADIS16300_SCAN_ADC_0, IIO_SIGNED(12),
> +static IIO_SCAN_EL_C(adc_0, ADIS16300_SCAN_ADC_0, IIO_UNSIGNED(12),
> ADIS16300_AUX_ADC, NULL);
>
> static IIO_SCAN_EL_C(incli_x, ADIS16300_SCAN_INCLI_X, IIO_SIGNED(12),
> @@ -87,6 +88,54 @@ static void adis16300_poll_func_th(struct iio_dev *indio_dev)
> */
> }
>
> +/**
> + * adis16300_spi_read_burst() - read all data registers
> + * @dev: device associated with child of actual device (iio_dev or iio_trig)
> + * @rx: somewhere to pass back the value read (min size is 24 bytes)
> + **/
> +static int adis16300_spi_read_burst(struct device *dev, u8 *rx)
> +{
> + struct spi_message msg;
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct adis16300_state *st = iio_dev_get_devdata(indio_dev);
> + u32 old_speed_hz = st->us->max_speed_hz;
> + int ret;
> +
> + struct spi_transfer xfers[] = {
> + {
> + .tx_buf = st->tx,
> + .bits_per_word = 8,
> + .len = 2,
> + .cs_change = 0,
> + }, {
> + .rx_buf = rx,
> + .bits_per_word = 8,
> + .len = 18,
> + .cs_change = 0,
> + },
> + };
> +
> + mutex_lock(&st->buf_lock);
> + st->tx[0] = ADIS16300_READ_REG(ADIS16300_GLOB_CMD);
> + st->tx[1] = 0;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfers[0], &msg);
> + spi_message_add_tail(&xfers[1], &msg);
> +
> + st->us->max_speed_hz = ADIS16300_SPI_BURST;
> + spi_setup(st->us);
> +
> + ret = spi_sync(st->us, &msg);
> + if (ret)
> + dev_err(&st->us->dev, "problem when burst reading");
> +
> + st->us->max_speed_hz = old_speed_hz;
> + spi_setup(st->us);
> + mutex_unlock(&st->buf_lock);
> + return ret;
> +}
> +
> /* Whilst this makes a lot of calls to iio_sw_ring functions - it is to device
> * specific to be rolled into the core.
> */
next prev parent reply other threads:[~2010-06-04 10:55 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-06-04 9:19 [PATCH 0/3] adis16xxx: fix some minor issues and clean-up Barry Song
2010-06-04 9:19 ` [PATCH 1/3] adis16300: " Barry Song
2010-06-04 9:19 ` [PATCH 2/3] adis16400: " Barry Song
2010-06-04 9:19 ` [PATCH 3/3] adis16209/220/240/350: tuning spi delay to make hardware more stable Barry Song
2010-06-04 10:59 ` Jonathan Cameron
2010-06-04 10:58 ` [PATCH 2/3] adis16400: fix some minor issues and clean-up Jonathan Cameron
2010-06-04 15:28 ` Greg KH
2010-06-04 15:39 ` Jonathan Cameron
2010-06-04 10:56 ` Jonathan Cameron [this message]
2010-06-04 15:28 ` [PATCH 1/3] adis16300: " Greg KH
2010-06-05 0:43 ` Barry Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C08DBC5.1040206@cam.ac.uk \
--to=jic23@cam.ac.uk \
--cc=21cnbao@gmail.com \
--cc=gregkh@suse.de \
--cc=linux-iio@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox