Re: [PATCH 07/15] iio:ad5791: Do not store transfer buffers on the stack

[Lars-Peter Clausen <lars@xxxxxxxxxx>]

On 12/22/2013 06:36 PM, Jonathan Cameron wrote:
> On 11/30/13 11:09, Jonathan Cameron wrote:
> > On 11/25/13 12:41, Lars-Peter Clausen wrote:
> > > Some SPI controllers may not be able to handle transfer buffers that are placed
> > > on the stack.
> > >
> > > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Applied to the togreg branch of iio.git
> >
> > Thanks
>
> I really didn't review this series well enough.  This should probably have
> gone in as a fix rather that through the togreg branch as should the other sp
> driver.  Bit late now though given the size of the patch.
>
> Sorry it has taken me so long to do a pull request to Greg.  I always
> get a bit lazy on the stuff for next as can always do it tomorrow!
>
> Slipped rather long this time though!

I think most SPI driver simply don't care as they don't do any DMA. So as long 
as there is a board with a DMA SPI controller + this chip combination that is 
in active use I don't think the patch needs to go into fixes.

> Jonathan
>
> > > ---
> > >   drivers/iio/dac/ad5791.c | 46 +++++++++++++++++++++-------------------------
> > >   1 file changed, 21 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/iio/dac/ad5791.c b/drivers/iio/dac/ad5791.c
> > > index 1e7f4cd..79bf199 100644
> > > --- a/drivers/iio/dac/ad5791.c
> > > +++ b/drivers/iio/dac/ad5791.c
> > > @@ -91,6 +91,11 @@ struct ad5791_state {
> > >   	unsigned			ctrl;
> > >   	unsigned			pwr_down_mode;
> > >   	bool				pwr_down;
> > > +
> > > +	union {
> > > +		__be32 d32;
> > > +		u8 d8[4];
> > > +	} data[3] ____cacheline_aligned;
> > >   };
> > >
> > >   /**
> > > @@ -104,48 +109,39 @@ enum ad5791_supported_device_ids {
> > >   	ID_AD5791,
> > >   };
> > >
> > > -static int ad5791_spi_write(struct spi_device *spi, u8 addr, u32 val)
> > > +static int ad5791_spi_write(struct ad5791_state *st, u8 addr, u32 val)
> > >   {
> > > -	union {
> > > -		__be32 d32;
> > > -		u8 d8[4];
> > > -	} data;
> > > -
> > > -	data.d32 = cpu_to_be32(AD5791_CMD_WRITE |
> > > +	st->data[0].d32 = cpu_to_be32(AD5791_CMD_WRITE |
> > >   			      AD5791_ADDR(addr) |
> > >   			      (val & AD5791_DAC_MASK));
> > >
> > > -	return spi_write(spi, &data.d8[1], 3);
> > > +	return spi_write(st->spi, &st->data[0].d8[1], 3);
> > >   }
> > >
> > > -static int ad5791_spi_read(struct spi_device *spi, u8 addr, u32 *val)
> > > +static int ad5791_spi_read(struct ad5791_state *st, u8 addr, u32 *val)
> > >   {
> > > -	union {
> > > -		__be32 d32;
> > > -		u8 d8[4];
> > > -	} data[3];
> > >   	int ret;
> > >   	struct spi_transfer xfers[] = {
> > >   		{
> > > -			.tx_buf = &data[0].d8[1],
> > > +			.tx_buf = &st->data[0].d8[1],
> > >   			.bits_per_word = 8,
> > >   			.len = 3,
> > >   			.cs_change = 1,
> > >   		}, {
> > > -			.tx_buf = &data[1].d8[1],
> > > -			.rx_buf = &data[2].d8[1],
> > > +			.tx_buf = &st->data[1].d8[1],
> > > +			.rx_buf = &st->data[2].d8[1],
> > >   			.bits_per_word = 8,
> > >   			.len = 3,
> > >   		},
> > >   	};
> > >
> > > -	data[0].d32 = cpu_to_be32(AD5791_CMD_READ |
> > > +	st->data[0].d32 = cpu_to_be32(AD5791_CMD_READ |
> > >   			      AD5791_ADDR(addr));
> > > -	data[1].d32 = cpu_to_be32(AD5791_ADDR(AD5791_ADDR_NOOP));
> > > +	st->data[1].d32 = cpu_to_be32(AD5791_ADDR(AD5791_ADDR_NOOP));
> > >
> > > -	ret = spi_sync_transfer(spi, xfers, ARRAY_SIZE(xfers));
> > > +	ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
> > >
> > > -	*val = be32_to_cpu(data[2].d32);
> > > +	*val = be32_to_cpu(st->data[2].d32);
> > >
> > >   	return ret;
> > >   }
> > > @@ -210,7 +206,7 @@ static ssize_t ad5791_write_dac_powerdown(struct iio_dev *indio_dev,
> > >   	}
> > >   	st->pwr_down = pwr_down;
> > >
> > > -	ret = ad5791_spi_write(st->spi, AD5791_ADDR_CTRL, st->ctrl);
> > > +	ret = ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl);
> > >
> > >   	return ret ? ret : len;
> > >   }
> > > @@ -263,7 +259,7 @@ static int ad5791_read_raw(struct iio_dev *indio_dev,
> > >
> > >   	switch (m) {
> > >   	case IIO_CHAN_INFO_RAW:
> > > -		ret = ad5791_spi_read(st->spi, chan->address, val);
> > > +		ret = ad5791_spi_read(st, chan->address, val);
> > >   		if (ret)
> > >   			return ret;
> > >   		*val &= AD5791_DAC_MASK;
> > > @@ -330,7 +326,7 @@ static int ad5791_write_raw(struct iio_dev *indio_dev,
> > >   		val &= AD5791_RES_MASK(chan->scan_type.realbits);
> > >   		val <<= chan->scan_type.shift;
> > >
> > > -		return ad5791_spi_write(st->spi, chan->address, val);
> > > +		return ad5791_spi_write(st, chan->address, val);
> > >
> > >   	default:
> > >   		return -EINVAL;
> > > @@ -393,7 +389,7 @@ static int ad5791_probe(struct spi_device *spi)
> > >   		dev_warn(&spi->dev, "reference voltage unspecified\n");
> > >   	}
> > >
> > > -	ret = ad5791_spi_write(spi, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
> > > +	ret = ad5791_spi_write(st, AD5791_ADDR_SW_CTRL, AD5791_SWCTRL_RESET);
> > >   	if (ret)
> > >   		goto error_disable_reg_neg;
> > >
> > > @@ -405,7 +401,7 @@ static int ad5791_probe(struct spi_device *spi)
> > >   		  | ((pdata && pdata->use_rbuf_gain2) ? 0 : AD5791_CTRL_RBUF) |
> > >   		  AD5791_CTRL_BIN2SC;
> > >
> > > -	ret = ad5791_spi_write(spi, AD5791_ADDR_CTRL, st->ctrl |
> > > +	ret = ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl |
> > >   		AD5791_CTRL_OPGND | AD5791_CTRL_DACTRI);
> > >   	if (ret)
> > >   		goto error_disable_reg_neg;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html