Re: [PATCH 1/1] et8ek8: Decrease stack usage

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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



-- 
Sakari Ailus
sakari.ailus@linux.intel.com