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
