lkml.org 
[lkml]   [2016]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
From
Hi Jonathan,

On 20 March 2016 at 18:25, Jonathan Cameron <jic23@kernel.org> 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.


regards,
Joachim Eastwood

\
 
 \ /
  Last update: 2016-03-20 19:41    [W:0.050 / U:0.396 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site