Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752947AbcCUHvj (ORCPT <rfc822;w@1wt.eu>);
	Mon, 21 Mar 2016 03:51:39 -0400
Received: from smtpo.poczta.interia.pl ([217.74.65.208]:49420 "EHLO
	smtpo.poczta.interia.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752663AbcCUHve (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 21 Mar 2016 03:51:34 -0400
X-Interia-R: Interia
X-Interia-R-IP: 188.121.17.172
X-Interia-R-Helo: <x220>
Date: Mon, 21 Mar 2016 08:52:09 +0100
From: Slawomir Stepien <sst@poczta.fm>
To: Joachim Eastwood <manabian@gmail.com>
Cc: Jonathan Cameron <jic23@kernel.org>, 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>
Subject: Re: [PATCH v3] iio: add driver for Microchip
 MCP413X/414X/415X/416X/423X/424X/425X/426X
Message-ID: <20160321075209.GA25018@x220>
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-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGhQ9VzmdOgQ6tf_KRtdp8953miSioE=HMgAT+TZ4eotd78n7g@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
X-Interia-Antivirus: OK
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 2791
Lines: 78

On Mar 20, 2016 19:15, Joachim Eastwood 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.

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