User-Agent: K-9 Mail for Android
In-Reply-To: <CAGhQ9VzmdOgQ6tf_KRtdp8953miSioE=HMgAT+TZ4eotd78n7g@mail.gmail.com>
References: <20160320143021.GD10728@x220> <CAGhQ9VzO-QR7XYW2fkCDB7jj+hJTC7AU3R7UPLO5RVxoOWmsHA@mail.gmail.com> <56EEDD06.6030202@kernel.org> <CAGhQ9VzmdOgQ6tf_KRtdp8953miSioE=HMgAT+TZ4eotd78n7g@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Content-Type: text/plain;
 charset=UTF-8
Subject: Re: [PATCH v3] iio: add driver for Microchip MCP413X/414X/415X/416X/423X/424X/425X/426X
From: Jonathan Cameron <jic23@jic23.retrosnub.co.uk>
Date: Sun, 20 Mar 2016 18:21:55 +0000
To: Joachim Eastwood <manabian@gmail.com>, Jonathan Cameron <jic23@kernel.org>
CC: Slawomir Stepien <sst@poczta.fm>, Hartmut Knaack <knaack.h@gmx.de>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>, linux-iio@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Message-ID: <5A4D8965-1642-463A-A5FE-4DC292A481BE@jic23.retrosnub.co.uk>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2500
Lines: 76


On 20 March 2016 18:15:50 GMT+00:00, Joachim  Eastwood <manabian@gmail.com> wrote:
>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.
Agreed
>
>
>regards,
>Joachim Eastwood

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