Re: [PATCH] i2c: i801: Fix resume bug

[Jean Delvare <jdelvare@suse.de>]

Hi Bjorn, Wolfram,

On Fri, 28 Aug 2020 11:40:36 -0500, Bjorn Helgaas wrote:
> On Fri, Aug 28, 2020 at 10:57:53AM +0200, Wolfram Sang wrote:
> > On Tue, Aug 25, 2020 at 07:13:03PM +0200, Jean Delvare wrote:  
> > > +static unsigned char i801_setup_hstcfg(unsigned char hstcfg)
> > > +{
> > > +	hstcfg &= ~SMBHSTCFG_I2C_EN;	/* SMBus timing */
> > > +	hstcfg |= SMBHSTCFG_HST_EN;
> > > +	return hstcfg;
> > > +}  
> > 
> > What about putting the write to SMBHSTCFG here, too. When I read the
> > function name, I assumed it will do that.  
> 
> From the point of view of suspend/resume, I think it's nice to have a
> write in i801_resume() to match the one in i801_suspend().

I agree that symmetry is nice in general, but I must agree with
Wolfram, a function named "setup" which doesn't actually perform any
action is kind of confusing. I believe that moving the write in there
makes sense as both callers will do exactly that immediately after.

> Putting the write inside i801_setup_hstcfg() would mean i801_probe()
> would do *two* writes instead of the one it currently does.

I can't see why that would be the case.

-- 
Jean Delvare
SUSE L3 Support