Re: [PATCH 3/4] staging: typec: fusb302: refactor resume retry mechanism

[Rui Miguel Silva <rmfrfs@xxxxxxxxx>]

Hi,
On Thu, May 11, 2017 at 10:32:35AM -0700, Guenter Roeck wrote:
> On Thu, May 11, 2017 at 03:20:22PM +0100, Rui Miguel Silva wrote:
> > The i2c functions need to test the pm_suspend state and do, if needed, some
> > retry before i2c operations. This code was repeated 4x.
> >
> > The isolate this create a new function to check suspend state and call it in
> > every need place.
> >
> > Signed-off-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx>
>
> This and the next patch don't apply without patch 2. Please refactor.
> Also, please change the order of patch 3 and 4 since the latter is
> a bug fix and this one is a cleanup (and thus probably not applicable
> for v4.12).

I will refactor and reorder in v2.

Cheers,
  Rui

> Thanks,
> Guenter
>
> > ---
> >  drivers/staging/typec/fusb302/fusb302.c | 66 +++++++++++++++++----------------
> >  1 file changed, 34 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
> > index 40842b8075e5..26d7e2a387c3 100644
> > --- a/drivers/staging/typec/fusb302/fusb302.c
> > +++ b/drivers/staging/typec/fusb302/fusb302.c
> > @@ -122,22 +122,36 @@ struct fusb302_chip {
> >
> >  #define FUSB302_RESUME_RETRY 10
> >  #define FUSB302_RESUME_RETRY_SLEEP 50
> > -static int fusb302_i2c_write(struct fusb302_chip *chip,
> > -			     u8 address, u8 data)
> > +
> > +static bool fusb302_is_suspended(struct fusb302_chip *chip)
> >  {
> >  	int retry_cnt;
> > -	int ret = 0;
> >
> > -	atomic_set(&chip->i2c_busy, 1);
> >  	for (retry_cnt = 0; retry_cnt < FUSB302_RESUME_RETRY; retry_cnt++) {
> >  		if (atomic_read(&chip->pm_suspend)) {
> >  			dev_info(chip->dev, "i2c: pm suspend, retry %d/%d\n",
> >  				 retry_cnt + 1, FUSB302_RESUME_RETRY);
> >  			msleep(FUSB302_RESUME_RETRY_SLEEP);
> >  		} else {
> > -			break;
> > +			return false;
> >  		}
> >  	}
> > +
> > +	return true;
> > +}
> > +
> > +static int fusb302_i2c_write(struct fusb302_chip *chip,
> > +			     u8 address, u8 data)
> > +{
> > +	int ret = 0;
> > +
> > +	atomic_set(&chip->i2c_busy, 1);
> > +
> > +	if (fusb302_is_suspended(chip)) {
> > +		atomic_set(&chip->i2c_busy, 0);
> > +		return -ETIMEDOUT;
> > +	}
> > +
> >  	ret = i2c_smbus_write_byte_data(chip->i2c_client, address, data);
> >  	if (ret < 0)
> >  		dev_err(chip->dev, "cannot write 0x%02x to 0x%02x: %d\n", data,
> > @@ -150,21 +164,17 @@ static int fusb302_i2c_write(struct fusb302_chip *chip,
> >  static int fusb302_i2c_block_write(struct fusb302_chip *chip, u8 address,
> >  				   u8 length, const u8 *data)
> >  {
> > -	int retry_cnt;
> >  	int ret = 0;
> >
> >  	if (length <= 0)
> >  		return ret;
> >  	atomic_set(&chip->i2c_busy, 1);
> > -	for (retry_cnt = 0; retry_cnt < FUSB302_RESUME_RETRY; retry_cnt++) {
> > -		if (atomic_read(&chip->pm_suspend)) {
> > -			dev_info(chip->dev, "i2c: pm suspend, retry %d/%d\n",
> > -				 retry_cnt + 1, FUSB302_RESUME_RETRY);
> > -			msleep(FUSB302_RESUME_RETRY_SLEEP);
> > -		} else {
> > -			break;
> > -		}
> > +
> > +	if (fusb302_is_suspended(chip)) {
> > +		atomic_set(&chip->i2c_busy, 0);
> > +		return -ETIMEDOUT;
> >  	}
> > +
> >  	ret = i2c_smbus_write_i2c_block_data(chip->i2c_client, address,
> >  					     length, data);
> >  	if (ret < 0)
> > @@ -178,19 +188,15 @@ static int fusb302_i2c_block_write(struct fusb302_chip *chip, u8 address,
> >  static int fusb302_i2c_read(struct fusb302_chip *chip,
> >  			    u8 address, u8 *data)
> >  {
> > -	int retry_cnt;
> >  	int ret = 0;
> >
> >  	atomic_set(&chip->i2c_busy, 1);
> > -	for (retry_cnt = 0; retry_cnt < FUSB302_RESUME_RETRY; retry_cnt++) {
> > -		if (atomic_read(&chip->pm_suspend)) {
> > -			dev_info(chip->dev, "i2c: pm suspend, retry %d/%d\n",
> > -				 retry_cnt + 1, FUSB302_RESUME_RETRY);
> > -			msleep(FUSB302_RESUME_RETRY_SLEEP);
> > -		} else {
> > -			break;
> > -		}
> > +
> > +	if (fusb302_is_suspended(chip)) {
> > +		atomic_set(&chip->i2c_busy, 0);
> > +		return -ETIMEDOUT;
> >  	}
> > +
> >  	ret = i2c_smbus_read_byte_data(chip->i2c_client, address);
> >  	*data = (u8)ret;
> >  	if (ret < 0)
> > @@ -203,21 +209,17 @@ static int fusb302_i2c_read(struct fusb302_chip *chip,
> >  static int fusb302_i2c_block_read(struct fusb302_chip *chip, u8 address,
> >  				  u8 length, u8 *data)
> >  {
> > -	int retry_cnt;
> >  	int ret = 0;
> >
> >  	if (length <= 0)
> >  		return ret;
> >  	atomic_set(&chip->i2c_busy, 1);
> > -	for (retry_cnt = 0; retry_cnt < FUSB302_RESUME_RETRY; retry_cnt++) {
> > -		if (atomic_read(&chip->pm_suspend)) {
> > -			dev_info(chip->dev, "i2c: pm suspend, retry %d/%d\n",
> > -				 retry_cnt + 1, FUSB302_RESUME_RETRY);
> > -			msleep(FUSB302_RESUME_RETRY_SLEEP);
> > -		} else {
> > -			break;
> > -		}
> > +
> > +	if (fusb302_is_suspended(chip)) {
> > +		atomic_set(&chip->i2c_busy, 0);
> > +		return -ETIMEDOUT;
> >  	}
> > +
> >  	ret = i2c_smbus_read_i2c_block_data(chip->i2c_client, address,
> >  					    length, data);
> >  	if (ret < 0) {
> > --
> > 2.12.2
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel