Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757848AbbBGRvG (ORCPT ); Sat, 7 Feb 2015 12:51:06 -0500 Received: from sauhun.de ([89.238.76.85]:45361 "EHLO pokefinder.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755854AbbBGRvD (ORCPT ); Sat, 7 Feb 2015 12:51:03 -0500 Date: Sat, 7 Feb 2015 18:50:39 +0100 From: Wolfram Sang To: Ray Jui Cc: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= , Arend van Spriel , Kevin Cernekee , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Grant Likely , Christian Daudt , Matt Porter , Florian Fainelli , Russell King , Scott Branden , Dmitry Torokhov , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, bcm-kernel-feedback-list@broadcom.com, devicetree@vger.kernel.org Subject: Re: [PATCH v8 2/3] i2c: iproc: Add Broadcom iProc I2C Driver Message-ID: <20150207175039.GB6263@katana> References: <1423272507-18459-1-git-send-email-rjui@broadcom.com> <1423272507-18459-3-git-send-email-rjui@broadcom.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="cvVnyQ+4j833TQvp" Content-Disposition: inline In-Reply-To: <1423272507-18459-3-git-send-email-rjui@broadcom.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4595 Lines: 156 --cvVnyQ+4j833TQvp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Ray, On Fri, Feb 06, 2015 at 05:28:26PM -0800, Ray Jui wrote: > Add initial support to the Broadcom iProc I2C controller found in the > iProc family of SoCs. >=20 > The iProc I2C controller has separate internal TX and RX FIFOs, each has > a size of 64 bytes. The iProc I2C controller supports two bus speeds > including standard mode (100kHz) and fast mode (400kHz) Mostly looking good. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Please sort the includes. > +static bool bcm_iproc_i2c_bus_busy(struct bcm_iproc_i2c_dev *iproc_i2c) > +{ > + if (readl(iproc_i2c->base + M_CMD_OFFSET) & > + (1 << M_CMD_START_BUSY_SHIFT)) > + return true; > + else > + return false; > +} Minor: return !!(readl(...))? You decide. > + > +static int bcm_iproc_i2c_format_addr(struct bcm_iproc_i2c_dev *iproc_i2c, > + struct i2c_msg *msg, u8 *addr) > +{ > + *addr =3D msg->addr << 1; > + > + if (msg->flags & I2C_M_RD) > + *addr |=3D 1; > + > + return 0; > +} I'd suggest a oneliner. *addr =3D msg->addr << 1 | (msg->flags & I2C_M_RD ? 1 : 0) Or use !! like above. Don't do an extra function for that. It is only used once and it also doesn't need to be int since it can't fail anyhow. (Note to self: I should make a macro for that in i2c.h) > + /* need to reserve one byte in the FIFO for the slave address */ > + if (msg->len > M_TX_RX_FIFO_SIZE - 1) { > + dev_err(iproc_i2c->device, > + "only support data length up to %u bytes\n", > + M_TX_RX_FIFO_SIZE - 1); > + return -EINVAL; -EOPNOTSUPP Is it really a HW limitation? Could the driver later be extended to continue filling the FIFO if a certain threshold is reached? > + dev_dbg(iproc_i2c->device, "xfer %c, addr=3D0x%02x, len=3D%d\n", > + (msg->flags & I2C_M_RD) ? 'R' : 'W', msg->addr, > + msg->len); > + dev_dbg(iproc_i2c->device, "*** data: %*ph\n", msg->len, msg->buf); Not really needed. We have tracing for that. > + if (bus_speed < 100000) { > + dev_err(iproc_i2c->device, "%d Hz bus speed not supported\n", > + bus_speed); > + dev_err(iproc_i2c->device, > + "valid speeds are 100khz and 400khz\n"); > + return -EINVAL; > + } else if (bus_speed < 400000) { > + bus_speed =3D 100000; > + speed_bit =3D 0; > + } else { > + bus_speed =3D 400000; > + speed_bit =3D 1; > + } > + > + val =3D readl(iproc_i2c->base + TIM_CFG_OFFSET); > + val &=3D ~(1 << TIM_CFG_MODE_400_SHIFT); > + val |=3D speed_bit << TIM_CFG_MODE_400_SHIFT; val |=3D (bus_speed =3D=3D 400000) ... and skip speed_bit? You decide. > +static void bcm_iproc_i2c_enable(struct bcm_iproc_i2c_dev *iproc_i2c) > +{ > + u32 val; > + > + val =3D readl(iproc_i2c->base + CFG_OFFSET); > + val |=3D 1 << CFG_EN_SHIFT; > + writel(val, iproc_i2c->base + CFG_OFFSET); > +} > + > +static void bcm_iproc_i2c_disable(struct bcm_iproc_i2c_dev *iproc_i2c) > +{ > + u32 val; > + > + val =3D readl(iproc_i2c->base + CFG_OFFSET); > + val &=3D ~(1 << CFG_EN_SHIFT); > + writel(val, iproc_i2c->base + CFG_OFFSET); > +} Extra functions? They are self explaining and only used once. You decide. Rest looks fine, thanks! --cvVnyQ+4j833TQvp Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU1lBvAAoJEBQN5MwUoCm2wroQAJYxctKO7RB+0o4wHtHRBapd 0UrhX+9Csiz912f/k08xP+D9RX+IVYNtlC7KJhgHniERSIWOPU6A8Cq7Ph+rIhW4 Ln7klovQfRsFCKiiYhJ5wpJ8+S/99TQ9vYaMODPKFfMmqAAasKbaHf6b9G8We1VV 8tppgu1ZOeGgTbMVaItjwKh/ND0srSAZxXaBs2dyKV8PRpL9vU3ZbC3vPwe4BcdW cFhseYWbtHpgR9cN3XW69PV+zNHsGYrm8g/brq4NGUOmE2lV0bvdIO1WciMPirtm Jdq51AA4NNG1/eju/evr5HHJJyzaTfHLqSn52tHobfnpKB0agV+XgOtA6Ny/kHK7 6EbggzWoss1cW/YTIDYeskecQelAFdcNffsXc3ASOZZ+5kU1w2EdkugObD7ZkOfJ 7mtQSm8IGEGx+3fyHWNAF3RGrs/6Pim6PRNmfhMBg8PebS0gipmzfpSI+yohwXVW 9v+SPz6IhA46YbuXae+wzSQTt/sl5IjXD+HteDPWpYefSuc3BkLymGX7ZvetxzMe msFriTk5wBb3xLnDoaT5gdAxlsY9VRqqEu7Rh7YhjptcY9mWZq6f+pTyG49RcChE pkfz1ty9TnELfAcUeqjTTu2inkSTq0bjGppRPnP0uvvIkNDH/j4IjISGhpcT754V tg5UpDCk5Nc3xFWfo0+z =iYks -----END PGP SIGNATURE----- --cvVnyQ+4j833TQvp-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/