| Message ID | 20170816081305.GA19601@amd (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1dhtSD-0005Rm-Gr; Wed, 16 Aug 2017 08:13:29 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751781AbdHPINJ (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 16 Aug 2017 04:13:09 -0400 Received: from atrey.karlin.mff.cuni.cz ([195.113.26.193]:34106 "EHLO atrey.karlin.mff.cuni.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751588AbdHPINH (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 16 Aug 2017 04:13:07 -0400 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 512) id 3DFBD827FE; Wed, 16 Aug 2017 10:13:06 +0200 (CEST) Date: Wed, 16 Aug 2017 10:13:05 +0200 From: Pavel Machek <pavel@ucw.cz> To: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: linux-media@vger.kernel.org, kernel list <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 1/1] et8ek8: Decrease stack usage Message-ID: <20170816081305.GA19601@amd> References: <1502868825-4531-1-git-send-email-sakari.ailus@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="d6Gm4EdcadzBjdND" Content-Disposition: inline In-Reply-To: <1502868825-4531-1-git-send-email-sakari.ailus@linux.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Pavel Machek
Aug. 16, 2017, 8:13 a.m. UTC
On Wed 2017-08-16 10:33:45, Sakari Ailus wrote: > The et8ek8 driver combines I²C register writes to a single array that it > passes to i2c_transfer(). The maximum number of writes is 48 at once, > decrease it to 8 and make more transfers if needed, thus avoiding a > warning on stack usage. Dunno. Slowing down code to save stack does not sound attractive. What about this one? Way simpler, too... (Unless there's some rule about i2c, DMA and static buffers. Is it?) Signed-off-by: Pavel Machek <pavel@ucw.cz> (untested) Pavel > --- > Pavel: this is just compile tested. Could you test it on N900, please? > > drivers/media/i2c/et8ek8/et8ek8_driver.c | 26 +++++++++++++++++--------- > 1 file changed, 17 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c > index f39f517..c14f0fd 100644 > --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c > +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c > @@ -43,7 +43,7 @@ > > #define ET8EK8_NAME "et8ek8" > #define ET8EK8_PRIV_MEM_SIZE 128 > -#define ET8EK8_MAX_MSG 48 > +#define ET8EK8_MAX_MSG 8 > > struct et8ek8_sensor { > struct v4l2_subdev subdev; > @@ -220,7 +220,8 @@ static void et8ek8_i2c_create_msg(struct i2c_client *client, u16 len, u16 reg, > > /* > * A buffered write method that puts the wanted register write > - * commands in a message list and passes the list to the i2c framework > + * commands in smaller number of message lists and passes the lists to > + * the i2c framework > */ > static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > const struct et8ek8_reg *wnext, > @@ -231,11 +232,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > int wcnt = 0; > u16 reg, data_length; > u32 val; > - > - if (WARN_ONCE(cnt > ET8EK8_MAX_MSG, > - ET8EK8_NAME ": %s: too many messages.\n", __func__)) { > - return -EINVAL; > - } > + int rval; > > /* Create new write messages for all writes */ > while (wcnt < cnt) { > @@ -249,10 +246,21 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > > /* Update write count */ > wcnt++; > + > + if (wcnt < ET8EK8_MAX_MSG) > + continue; > + > + rval = i2c_transfer(client->adapter, msg, wcnt); > + if (rval < 0) > + return rval; > + > + cnt -= wcnt; > + wcnt = 0; > } > > - /* Now we send everything ... */ > - return i2c_transfer(client->adapter, msg, wcnt); > + rval = i2c_transfer(client->adapter, msg, wcnt); > + > + return rval < 0 ? rval : 0; > } > > /*
Comments
On Wed, Aug 16, 2017 at 10:13:05AM +0200, Pavel Machek wrote: > On Wed 2017-08-16 10:33:45, Sakari Ailus wrote: > > The et8ek8 driver combines I²C register writes to a single array that it > > passes to i2c_transfer(). The maximum number of writes is 48 at once, > > decrease it to 8 and make more transfers if needed, thus avoiding a > > warning on stack usage. > > Dunno. Slowing down code to save stack does not sound attractive. > > What about this one? Way simpler, too... (Unless there's some rule > about i2c, DMA and static buffers. Is it?) > > Signed-off-by: Pavel Machek <pavel@ucw.cz> > > (untested) > Pavel > > diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c > index f39f517..64da731 100644 > --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c > +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c > @@ -227,7 +227,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > int cnt) > { > struct i2c_msg msg[ET8EK8_MAX_MSG]; > - unsigned char data[ET8EK8_MAX_MSG][6]; > + static unsigned char data[ET8EK8_MAX_MSG][6]; Works, but we'll need to serialise calls to the function then. I'm not really sure if passing multiple messages to i2c_transfer() really even helps here. I think it could be removed altogether as well. > int wcnt = 0; > u16 reg, data_length; > u32 val; > > > > > --- > > Pavel: this is just compile tested. Could you test it on N900, please? > > > > drivers/media/i2c/et8ek8/et8ek8_driver.c | 26 +++++++++++++++++--------- > > 1 file changed, 17 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c > > index f39f517..c14f0fd 100644 > > --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c > > +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c > > @@ -43,7 +43,7 @@ > > > > #define ET8EK8_NAME "et8ek8" > > #define ET8EK8_PRIV_MEM_SIZE 128 > > -#define ET8EK8_MAX_MSG 48 > > +#define ET8EK8_MAX_MSG 8 > > > > struct et8ek8_sensor { > > struct v4l2_subdev subdev; > > @@ -220,7 +220,8 @@ static void et8ek8_i2c_create_msg(struct i2c_client *client, u16 len, u16 reg, > > > > /* > > * A buffered write method that puts the wanted register write > > - * commands in a message list and passes the list to the i2c framework > > + * commands in smaller number of message lists and passes the lists to > > + * the i2c framework > > */ > > static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > > const struct et8ek8_reg *wnext, > > @@ -231,11 +232,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > > int wcnt = 0; > > u16 reg, data_length; > > u32 val; > > - > > - if (WARN_ONCE(cnt > ET8EK8_MAX_MSG, > > - ET8EK8_NAME ": %s: too many messages.\n", __func__)) { > > - return -EINVAL; > > - } > > + int rval; > > > > /* Create new write messages for all writes */ > > while (wcnt < cnt) { > > @@ -249,10 +246,21 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, > > > > /* Update write count */ > > wcnt++; > > + > > + if (wcnt < ET8EK8_MAX_MSG) > > + continue; > > + > > + rval = i2c_transfer(client->adapter, msg, wcnt); > > + if (rval < 0) > > + return rval; > > + > > + cnt -= wcnt; > > + wcnt = 0; > > } > > > > - /* Now we send everything ... */ > > - return i2c_transfer(client->adapter, msg, wcnt); > > + rval = i2c_transfer(client->adapter, msg, wcnt); > > + > > + return rval < 0 ? rval : 0; > > } > > > > /* > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
diff --git a/drivers/media/i2c/et8ek8/et8ek8_driver.c b/drivers/media/i2c/et8ek8/et8ek8_driver.c index f39f517..64da731 100644 --- a/drivers/media/i2c/et8ek8/et8ek8_driver.c +++ b/drivers/media/i2c/et8ek8/et8ek8_driver.c @@ -227,7 +227,7 @@ static int et8ek8_i2c_buffered_write_regs(struct i2c_client *client, int cnt) { struct i2c_msg msg[ET8EK8_MAX_MSG]; - unsigned char data[ET8EK8_MAX_MSG][6]; + static unsigned char data[ET8EK8_MAX_MSG][6]; int wcnt = 0; u16 reg, data_length; u32 val;