From owner-svn-src-all@freebsd.org Thu Oct 13 07:50:01 2016
Return-Path: <owner-svn-src-all@freebsd.org>
Delivered-To: svn-src-all@mailman.ysv.freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org
[IPv6:2001:1900:2254:206a::19:1])
by mailman.ysv.freebsd.org (Postfix) with ESMTP id 95EB0C0F6F9;
Thu, 13 Oct 2016 07:50:01 +0000 (UTC) (envelope-from avg@FreeBSD.org)
Received: from citapm.icyb.net.ua (citapm.icyb.net.ua [212.40.38.140])
by mx1.freebsd.org (Postfix) with ESMTP id 580C7962;
Thu, 13 Oct 2016 07:49:59 +0000 (UTC) (envelope-from avg@FreeBSD.org)
Received: from porto.starpoint.kiev.ua (porto-e.starpoint.kiev.ua
[212.40.38.100])
by citapm.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id KAA03802;
Thu, 13 Oct 2016 10:49:58 +0300 (EEST)
(envelope-from avg@FreeBSD.org)
Received: from localhost ([127.0.0.1])
by porto.starpoint.kiev.ua with esmtp (Exim 4.34 (FreeBSD))
id 1buam5-000JA7-Oo; Thu, 13 Oct 2016 10:49:58 +0300
Subject: Re: svn commit: r307195 - head/sys/dev/iicbus
To: src-committers@FreeBSD.org, svn-src-all@FreeBSD.org,
svn-src-head@FreeBSD.org
References: <201610130725.u9D7PIA0015270@repo.freebsd.org>
From: Andriy Gapon <avg@FreeBSD.org>
Message-ID: <f3e826fb-65dc-de2c-e516-ec67f1660b59@FreeBSD.org>
Date: Thu, 13 Oct 2016 10:49:21 +0300
User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101
Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <201610130725.u9D7PIA0015270@repo.freebsd.org>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.23
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for "
user" and " projects" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Thu, 13 Oct 2016 07:50:01 -0000
On 13/10/2016 10:25, Andriy Gapon wrote:
> Author: avg
> Date: Thu Oct 13 07:25:18 2016
> New Revision: 307195
> URL: https://svnweb.freebsd.org/changeset/base/307195
>
> Log:
> convert iicsmb to use iicbus_transfer for all operations
>
> Previously the driver used more low level operations like iicbus_start
> and iicbus_write. The problem is that those operations are not
> implemented by iicbus(4) and the calls were effectively routed to
> a driver to which the bus is attached.
> But not all of the controllers implement such low level operations
> while all of the drivers are expected to have iicbus_transfer.
>
> While there fix incorrect implementation of iicsmb_bwrite and iicsmb_bread.
> The former should send a byte count before the actual bytes, while the
> latter should first receive the byte count and then receive the bytes.
Just a thought. It would be much easier to implement iicsmb_bread() if we had a
flag like I2C_M_RECV_LEN in Linux. The flag signals that the first byte
received from slave is a count of how many more bytes we should read from that
slave. But adding support for a new flag to all controller drivers is not fun.
> I have tested only these commands:
> - quick (r/w)
> - send byte
> - receive byte
> - read byte
> - write byte
>
> MFC after: 1 month
> Differential Revision: https://reviews.freebsd.org/D8170
>
> Modified:
> head/sys/dev/iicbus/iicsmb.c
>
> Modified: head/sys/dev/iicbus/iicsmb.c
> ==============================================================================
> --- head/sys/dev/iicbus/iicsmb.c Thu Oct 13 07:22:13 2016 (r307194)
> +++ head/sys/dev/iicbus/iicsmb.c Thu Oct 13 07:25:18 2016 (r307195)
> @@ -131,8 +131,6 @@ static driver_t iicsmb_driver = {
> sizeof(struct iicsmb_softc),
> };
>
> -#define IICBUS_TIMEOUT 100 /* us */
> -
> static void
> iicsmb_identify(driver_t *driver, device_t parent)
> {
> @@ -276,237 +274,213 @@ iicsmb_callback(device_t dev, int index,
> }
>
> static int
> +iic2smb_error(int error)
> +{
> + switch (error) {
> + case IIC_NOERR:
> + return (SMB_ENOERR);
> + case IIC_EBUSERR:
> + return (SMB_EBUSERR);
> + case IIC_ENOACK:
> + return (SMB_ENOACK);
> + case IIC_ETIMEOUT:
> + return (SMB_ETIMEOUT);
> + case IIC_EBUSBSY:
> + return (SMB_EBUSY);
> + case IIC_ESTATUS:
> + return (SMB_EBUSERR);
> + case IIC_EUNDERFLOW:
> + return (SMB_EBUSERR);
> + case IIC_EOVERFLOW:
> + return (SMB_EBUSERR);
> + case IIC_ENOTSUPP:
> + return (SMB_ENOTSUPP);
> + case IIC_ENOADDR:
> + return (SMB_EBUSERR);
> + case IIC_ERESOURCE:
> + return (SMB_EBUSERR);
> + default:
> + return (SMB_EBUSERR);
> + }
> +}
> +
> +#define TRANSFER_MSGS(dev, msgs) iicbus_transfer(dev, msgs, nitems(msgs))
> +
> +static int
> iicsmb_quick(device_t dev, u_char slave, int how)
> {
> - device_t parent = device_get_parent(dev);
> + struct iic_msg msgs[] = {
> + { slave, how == SMB_QWRITE ? IIC_M_WR : IIC_M_RD, 0, NULL },
> + };
> int error;
>
> switch (how) {
> case SMB_QWRITE:
> - error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT);
> - break;
> -
> case SMB_QREAD:
> - error = iicbus_start(parent, slave | LSB, IICBUS_TIMEOUT);
> break;
> -
> default:
> - error = EINVAL;
> - break;
> + return (SMB_EINVAL);
> }
>
> - if (!error)
> - error = iicbus_stop(parent);
> -
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_sendb(device_t dev, u_char slave, char byte)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent;
> -
> - error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT);
> -
> - if (!error) {
> - error = iicbus_write(parent, &byte, 1, &sent, IICBUS_TIMEOUT);
> -
> - iicbus_stop(parent);
> - }
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR, 1, &byte },
> + };
> + int error;
>
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_recvb(device_t dev, u_char slave, char *byte)
> {
> - device_t parent = device_get_parent(dev);
> - int error, read;
> -
> - error = iicbus_start(parent, slave | LSB, 0);
> -
> - if (!error) {
> - error = iicbus_read(parent, byte, 1, &read, IIC_LAST_READ, IICBUS_TIMEOUT);
> -
> - iicbus_stop(parent);
> - }
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_RD, 1, byte },
> + };
> + int error;
>
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_writeb(device_t dev, u_char slave, char cmd, char byte)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent;
> -
> - error = iicbus_start(parent, slave & ~LSB, 0);
> -
> - if (!error) {
> - if (!(error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - error = iicbus_write(parent, &byte, 1, &sent, IICBUS_TIMEOUT);
> -
> - iicbus_stop(parent);
> - }
> + uint8_t bytes[] = { cmd, byte };
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR, nitems(bytes), bytes },
> + };
> + int error;
>
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_writew(device_t dev, u_char slave, char cmd, short word)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent;
> -
> - char low = (char)(word & 0xff);
> - char high = (char)((word & 0xff00) >> 8);
> -
> - error = iicbus_start(parent, slave & ~LSB, 0);
> -
> - if (!error) {
> - if (!(error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - if (!(error = iicbus_write(parent, &low, 1, &sent, IICBUS_TIMEOUT)))
> - error = iicbus_write(parent, &high, 1, &sent, IICBUS_TIMEOUT);
> -
> - iicbus_stop(parent);
> - }
> + uint8_t bytes[] = { cmd, word & 0xff, word >> 8 };
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR, nitems(bytes), bytes },
> + };
> + int error;
>
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_readb(device_t dev, u_char slave, char cmd, char *byte)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent, read;
> -
> - if ((error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT)))
> - return (error);
> -
> - if ((error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_repeated_start(parent, slave | LSB, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_read(parent, byte, 1, &read, IIC_LAST_READ, IICBUS_TIMEOUT)))
> - goto error;
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR | IIC_M_NOSTOP, 1, &cmd },
> + { slave, IIC_M_RD, 1, byte },
> + };
> + int error;
>
> -error:
> - iicbus_stop(parent);
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> -#define BUF2SHORT(low,high) \
> - ((short)(((high) & 0xff) << 8) | (short)((low) & 0xff))
> -
> static int
> iicsmb_readw(device_t dev, u_char slave, char cmd, short *word)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent, read;
> - char buf[2];
> -
> - if ((error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT)))
> - return (error);
> -
> - if ((error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_repeated_start(parent, slave | LSB, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_read(parent, buf, 2, &read, IIC_LAST_READ, IICBUS_TIMEOUT)))
> - goto error;
> -
> - /* first, receive low, then high byte */
> - *word = BUF2SHORT(buf[0], buf[1]);
> + uint8_t buf[2];
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR | IIC_M_NOSTOP, 1, &cmd },
> + { slave, IIC_M_RD, nitems(buf), buf },
> + };
> + int error;
>
> -error:
> - iicbus_stop(parent);
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + if (error == 0)
> + *word = ((uint16_t)buf[1] << 8) | buf[0];
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_pcall(device_t dev, u_char slave, char cmd, short sdata, short *rdata)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent, read;
> - char buf[2];
> -
> - if ((error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT)))
> - return (error);
> -
> - if ((error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - /* first, send low, then high byte */
> - buf[0] = (char)(sdata & 0xff);
> - buf[1] = (char)((sdata & 0xff00) >> 8);
> -
> - if ((error = iicbus_write(parent, buf, 2, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_repeated_start(parent, slave | LSB, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_read(parent, buf, 2, &read, IIC_LAST_READ, IICBUS_TIMEOUT)))
> - goto error;
> -
> - /* first, receive low, then high byte */
> - *rdata = BUF2SHORT(buf[0], buf[1]);
> + uint8_t in[3] = { cmd, sdata & 0xff, sdata >> 8 };
> + uint8_t out[2];
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR | IIC_M_NOSTOP, nitems(in), in },
> + { slave, IIC_M_RD, nitems(out), out },
> + };
> + int error;
>
> -error:
> - iicbus_stop(parent);
> - return (error);
> + error = TRANSFER_MSGS(dev, msgs);
> + if (error == 0)
> + *rdata = ((uint16_t)out[1] << 8) | out[0];
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_bwrite(device_t dev, u_char slave, char cmd, u_char count, char *buf)
> {
> - device_t parent = device_get_parent(dev);
> - int error, sent;
> -
> - if ((error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_write(parent, buf, (int)count, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_stop(parent)))
> - goto error;
> + uint8_t bytes[2] = { cmd, count };
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR | IIC_M_NOSTOP, nitems(bytes), bytes },
> + { slave, IIC_M_WR | IIC_M_NOSTART, count, buf },
> + };
> + int error;
>
> -error:
> - return (error);
> + if (count > 32 || count == 0)
> + return (SMB_EINVAL);
> + error = TRANSFER_MSGS(dev, msgs);
> + return (iic2smb_error(error));
> }
>
> static int
> iicsmb_bread(device_t dev, u_char slave, char cmd, u_char *count, char *buf)
> {
> + struct iic_msg msgs[] = {
> + { slave, IIC_M_WR | IIC_M_NOSTOP, 1, &cmd },
> + { slave, IIC_M_RD | IIC_M_NOSTOP, 1, count },
> + };
> + struct iic_msg block_msg[] = {
> + { slave, IIC_M_RD | IIC_M_NOSTART, 0, buf },
> + };
> device_t parent = device_get_parent(dev);
> - int error, sent, read;
> -
> - if ((error = iicbus_start(parent, slave & ~LSB, IICBUS_TIMEOUT)))
> - return (error);
> -
> - if ((error = iicbus_write(parent, &cmd, 1, &sent, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_repeated_start(parent, slave | LSB, IICBUS_TIMEOUT)))
> - goto error;
> -
> - if ((error = iicbus_read(parent, buf, (int)*count, &read,
> - IIC_LAST_READ, IICBUS_TIMEOUT)))
> - goto error;
> - *count = read;
> + int error;
> + u_char bufsz;
>
> -error:
> - iicbus_stop(parent);
> - return (error);
> + /* Stash output buffer size before overwriting it. */
> + bufsz = *count;
> + if (bufsz == 0)
> + return (SMB_EINVAL);
> +
> + /* Have to do this because the command is split in two transfers. */
> + error = iicbus_request_bus(parent, dev, IIC_WAIT);
> + if (error == 0)
> + error = TRANSFER_MSGS(dev, msgs);
> + if (error == 0) {
> + /*
> + * If the slave offers an empty or a too long reply,
> + * read one byte to generate the stop or abort.
> + * XXX 32 is hardcoded until SMB_MAXBLOCKSIZE is restored
> + * to sanity.
> + */
> + if (*count > 32 || *count == 0)
> + block_msg[0].len = 1;
> + /* If longer than the buffer, then clamp at the buffer size. */
> + if (*count > bufsz)
> + block_msg[0].len = bufsz;
> + else
> + block_msg[0].len = *count;
> + error = TRANSFER_MSGS(dev, block_msg);
> + if (*count > 32 || *count == 0)
> + error = SMB_EINVAL;
> + }
> + (void)iicbus_release_bus(parent, dev);
> + return (iic2smb_error(error));
> }
>
> DRIVER_MODULE(iicsmb, iicbus, iicsmb_driver, iicsmb_devclass, 0, 0);
>
--
Andriy Gapon