From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfram Sang Subject: Re: [PATCH v2 1/4] i2c: tegra: implement slave mode Date: Mon, 20 Jul 2015 11:50:49 +0200 Message-ID: <20150720095049.GD2551@katana> References: <1427745615-5428-1-git-send-email-danindrey@mail.ru> <1427745615-5428-2-git-send-email-danindrey@mail.ru> <20150403194626.GB2016@katana> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="11Y7aswkeuHtSBEs" Return-path: Content-Disposition: inline In-Reply-To: <20150403194626.GB2016@katana> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Andrey Danin Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, ac100-oU9gvf+ajcQ97yFScArB1dHuzzzSOjJt@public.gmane.org, Laxman Dewangan , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Stephen Warren , Thierry Reding , Alexandre Courbot , Greg Kroah-Hartman , Julian Andres Klode , Marc Dietrich List-Id: linux-tegra@vger.kernel.org --11Y7aswkeuHtSBEs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote: > > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 v= al) > > +{ > > + i2c_writel(i2c_dev, val, I2C_SL_RCVD); > > + > > + /* > > + * TODO: A correct fix needs to be found for this. > > + * > > + * We experience less incomplete messages with this delay than without > > + * it, but we don't know why. Help is appreciated. > > + */ >=20 > Uh oh. So I assume this is another reason for staging? I think we can live with this upstream, no need for staging. > > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb) > > + return -EINVAL; >=20 > The core checks for slave_cb being valid. >=20 > > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy); >=20 > You could use value here, too, or? >=20 > > + if (!tegra_i2c_slave_isr(irq, i2c_dev)) > > + return IRQ_HANDLED; >=20 > Minor nit: I'd prefer =3D=3D 0 here, since it reads "if not slave_isr ret= urn > handled". >=20 > > + if (slave->addr > 0x7F) > > + addr2 =3D (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE; >=20 > Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you > need to check for the flag (slave->flags & I2C_CLIENT_TEN). If you'd fix up these and resend, I'll happily include this patch for 4.3 already. Thanks, Wolfram --11Y7aswkeuHtSBEs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVrMR5AAoJEBQN5MwUoCm29gYP/jk9qWhTjLu3Vyfixpw19nJp Wu3I2+mYowNVxjD27rIihYYAc2NBhxaD/0tXld/vGdDTba0fnfxw+y2APLigsqhh arH/05ae2VVQMVHgOWuA5Ux4Jt/LHbJ2VTY2NXM92iDZgP7pqRBc/qA43YxBYgdA +2PWyf/TAotud+xMZyn3SQjA+Q8LIpLHOYo/txigeQ+irT6AplL9OfWKq3smI3W7 5wyUR9zoYJJu+6OCERU7PXU0IWE4Mw8EXs6STobkd4XIzeBB9stGqyNruGeoK42F 3encICz8cdW7sO9DXEJOJUcag0tyQ0sQE5klwd8QwBjdN4ZJry9v1LVpyYWBdLQ+ vIL0xVPv/lpmoVIONN7pp7EIZbzN6glHDRg6H+MfstWTRfyUIGoNGFicoryCPDsK SvzDlV+4qP65+zhlWbI1u816dhvtKsz9C4T3rFOtva3dZK4Dm3DxJaHzulo5HBzG hXMUZrqNtFu9KPq3oJHIRk4Sm8Fioq0gWAdlvT3XSc4a9G3NEfzhmXiEP0H8ldkm GxWHviF1mPFqjbu5akmV2su531TXvpw3A2nXyC4c7lchyIRCP0T5+9RFOV9gPXVI u39lFNk9hcfqEwdBQX3WKa6uHVZMZOql9Iweb3VE86fNHaJbfapBnHfr+Na4X5QO LT/RMWeGywvHgNh04b6g =2IuQ -----END PGP SIGNATURE----- --11Y7aswkeuHtSBEs-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753505AbbGTJvB (ORCPT ); Mon, 20 Jul 2015 05:51:01 -0400 Received: from sauhun.de ([89.238.76.85]:35336 "EHLO pokefinder.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752596AbbGTJu7 (ORCPT ); Mon, 20 Jul 2015 05:50:59 -0400 Date: Mon, 20 Jul 2015 11:50:49 +0200 From: Wolfram Sang To: Andrey Danin Cc: devicetree@vger.kernel.org, linux-i2c@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, ac100@lists.launchpad.net, Laxman Dewangan , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Russell King , Stephen Warren , Thierry Reding , Alexandre Courbot , Greg Kroah-Hartman , Julian Andres Klode , Marc Dietrich Subject: Re: [PATCH v2 1/4] i2c: tegra: implement slave mode Message-ID: <20150720095049.GD2551@katana> References: <1427745615-5428-1-git-send-email-danindrey@mail.ru> <1427745615-5428-2-git-send-email-danindrey@mail.ru> <20150403194626.GB2016@katana> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="11Y7aswkeuHtSBEs" Content-Disposition: inline In-Reply-To: <20150403194626.GB2016@katana> 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 --11Y7aswkeuHtSBEs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote: > > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 v= al) > > +{ > > + i2c_writel(i2c_dev, val, I2C_SL_RCVD); > > + > > + /* > > + * TODO: A correct fix needs to be found for this. > > + * > > + * We experience less incomplete messages with this delay than without > > + * it, but we don't know why. Help is appreciated. > > + */ >=20 > Uh oh. So I assume this is another reason for staging? I think we can live with this upstream, no need for staging. > > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb) > > + return -EINVAL; >=20 > The core checks for slave_cb being valid. >=20 > > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy); >=20 > You could use value here, too, or? >=20 > > + if (!tegra_i2c_slave_isr(irq, i2c_dev)) > > + return IRQ_HANDLED; >=20 > Minor nit: I'd prefer =3D=3D 0 here, since it reads "if not slave_isr ret= urn > handled". >=20 > > + if (slave->addr > 0x7F) > > + addr2 =3D (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE; >=20 > Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you > need to check for the flag (slave->flags & I2C_CLIENT_TEN). If you'd fix up these and resend, I'll happily include this patch for 4.3 already. Thanks, Wolfram --11Y7aswkeuHtSBEs Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVrMR5AAoJEBQN5MwUoCm29gYP/jk9qWhTjLu3Vyfixpw19nJp Wu3I2+mYowNVxjD27rIihYYAc2NBhxaD/0tXld/vGdDTba0fnfxw+y2APLigsqhh arH/05ae2VVQMVHgOWuA5Ux4Jt/LHbJ2VTY2NXM92iDZgP7pqRBc/qA43YxBYgdA +2PWyf/TAotud+xMZyn3SQjA+Q8LIpLHOYo/txigeQ+irT6AplL9OfWKq3smI3W7 5wyUR9zoYJJu+6OCERU7PXU0IWE4Mw8EXs6STobkd4XIzeBB9stGqyNruGeoK42F 3encICz8cdW7sO9DXEJOJUcag0tyQ0sQE5klwd8QwBjdN4ZJry9v1LVpyYWBdLQ+ vIL0xVPv/lpmoVIONN7pp7EIZbzN6glHDRg6H+MfstWTRfyUIGoNGFicoryCPDsK SvzDlV+4qP65+zhlWbI1u816dhvtKsz9C4T3rFOtva3dZK4Dm3DxJaHzulo5HBzG hXMUZrqNtFu9KPq3oJHIRk4Sm8Fioq0gWAdlvT3XSc4a9G3NEfzhmXiEP0H8ldkm GxWHviF1mPFqjbu5akmV2su531TXvpw3A2nXyC4c7lchyIRCP0T5+9RFOV9gPXVI u39lFNk9hcfqEwdBQX3WKa6uHVZMZOql9Iweb3VE86fNHaJbfapBnHfr+Na4X5QO LT/RMWeGywvHgNh04b6g =2IuQ -----END PGP SIGNATURE----- --11Y7aswkeuHtSBEs-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: wsa@the-dreams.de (Wolfram Sang) Date: Mon, 20 Jul 2015 11:50:49 +0200 Subject: [PATCH v2 1/4] i2c: tegra: implement slave mode In-Reply-To: <20150403194626.GB2016@katana> References: <1427745615-5428-1-git-send-email-danindrey@mail.ru> <1427745615-5428-2-git-send-email-danindrey@mail.ru> <20150403194626.GB2016@katana> Message-ID: <20150720095049.GD2551@katana> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Fri, Apr 03, 2015 at 09:46:26PM +0200, Wolfram Sang wrote: > > +static void tegra_i2c_slave_write(struct tegra_i2c_dev *i2c_dev, u32 val) > > +{ > > + i2c_writel(i2c_dev, val, I2C_SL_RCVD); > > + > > + /* > > + * TODO: A correct fix needs to be found for this. > > + * > > + * We experience less incomplete messages with this delay than without > > + * it, but we don't know why. Help is appreciated. > > + */ > > Uh oh. So I assume this is another reason for staging? I think we can live with this upstream, no need for staging. > > + if (!i2c_dev->slave || !i2c_dev->slave->slave_cb) > > + return -EINVAL; > > The core checks for slave_cb being valid. > > > + i2c_slave_event(i2c_dev->slave, I2C_SLAVE_STOP, &dummy); > > You could use value here, too, or? > > > + if (!tegra_i2c_slave_isr(irq, i2c_dev)) > > + return IRQ_HANDLED; > > Minor nit: I'd prefer == 0 here, since it reads "if not slave_isr return > handled". > > > + if (slave->addr > 0x7F) > > + addr2 = (slave->addr >> 7) | I2C_SL_ADDR2_TEN_BIT_MODE; > > Nope. There are 10 bit encodings of addresses 0x000-0x07f, too. So, you > need to check for the flag (slave->flags & I2C_CLIENT_TEN). If you'd fix up these and resend, I'll happily include this patch for 4.3 already. Thanks, Wolfram -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: Digital signature URL: