On Mar 20, 2016 19:15, Joachim Eastwood wrote:
> Hi Jonathan,
>
> On 20 March 2016 at 18:25, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> > On 20/03/16 16:12, Joachim Eastwood wrote:
> >>> +static int mcp4131_exec(struct mcp4131_data *data,
> >>> + u8 addr, u8 cmd,
> >>> + u16 val)
> >>> +{
> >>> + int err;
> >>> + struct spi_device *spi = data->spi;
> >>> +
> >>> + data->xfer.tx_buf = data->buf;
> >>> + data->xfer.rx_buf = data->buf;
> >>> +
> >>> + switch (cmd) {
> >>> + case MCP4131_READ:
> >>> + data->xfer.len = 2; /* Two bytes transfer for this command */
> >>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) | MCP4131_READ;
> >>> + data->buf[1] = 0;
> >>> + break;
> >>> +
> >>> + case MCP4131_WRITE:
> >>> + data->xfer.len = 2;
> >>> + data->buf[0] = (addr << MCP4131_WIPER_SHIFT) |
> >>> + MCP4131_WRITE | (val >> 8);
> >>> + data->buf[1] = val & 0xFF; /* 8 bits here */
> >>> + break;
> >>> +
> >>> + default:
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>> + dev_dbg(&spi->dev, "mcp4131_exec: tx0: 0x%x tx1: 0x%x\n",
> >>> + data->buf[0], data->buf[1]);
> >>> +
> >>> + spi_message_init(&data->msg);
> >>> + spi_message_add_tail(&data->xfer, &data->msg);
> >>> +
> >>> + err = spi_sync(spi, &data->msg);
> >>> + if (err) {
> >>> + dev_err(&spi->dev, "spi_sync(): %d\n", err);
> >>> + return err;
> >>> + }
> >>
> >> Isn't this init, add, sync sequence basically open coding of what
> >> spi_write/spi_read does?
> >> If you could use those you could also get rid transfer/message structs
> >> in priv data.
> > I initially wrote the same comment, then realised it's more nuanced than
> > that. Whilst this initially looks like an w8r8 type cycle it's actually
> > something like w4r12 in the read case anyway. The write case could indeed
> > be done with spi_write.
>
> Indeed. I didn't notice that for the read case.
>
> The read case could almost be copy of spi_read, though. One would only
> need to add ".tx_buf = buf" when setting up the transfer struct, I
> think. Having it in its a own function with a comment would make it
> easier to spot the difference.
Just to see if I get it.
For write case I should use the spi_write as it is:
case MCP4131_WRITE:
spi_write(...);
For read case I should create new function (e.g. mcp4131_read) that will look
like spi_read but with additional tx_buf content so I can read the data on miso?
case MCP4131_READ:
mcp4131_read(...)
Keep the needed buffers (transfer/message) local.
--
Slawomir Stepien
--
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
