From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932665AbcFIJjw (ORCPT ); Thu, 9 Jun 2016 05:39:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39508 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752375AbcFIJju (ORCPT ); Thu, 9 Jun 2016 05:39:50 -0400 Date: Thu, 9 Jun 2016 11:39:45 +0200 From: Benjamin Tissoires To: Corey Minyard Cc: Jean Delvare , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, Corey Minyard Subject: Re: [v2,02/10] i2c-i801: Move hostcfg set/reset to i801_access() Message-ID: <20160609093945.GD24234@mail.corp.redhat.com> References: <1464570544-975-3-git-send-email-minyard@acm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1464570544-975-3-git-send-email-minyard@acm.org> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 09 Jun 2016 09:39:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On May 29 2016 or thereabouts, Corey Minyard wrote: > From: Corey Minyard > > The HSTCFG register save/restore was done in i2c_block_transaction, > but all the checks were already done in i801_access, so move it into > those checks. > > This results in a small savings of code, and moves some special > handing for I2C transactions into code that is already handling > special things for I2C transactions. > > Signed-off-by: Corey Minyard > --- > drivers/i2c/busses/i2c-i801.c | 41 +++++++++++++++++++---------------------- > 1 file changed, 19 insertions(+), 22 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index 818c0c8..205f9d0 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -661,20 +661,6 @@ static int i801_block_transaction(struct i801_priv *priv, > int command, int hwpec) > { > int result = 0; > - unsigned char hostc; > - > - if (command == I2C_SMBUS_I2C_BLOCK_DATA) { > - if (read_write == I2C_SMBUS_WRITE) { > - /* set I2C_EN bit in configuration register */ > - pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); > - pci_write_config_byte(priv->pci_dev, SMBHSTCFG, > - hostc | SMBHSTCFG_I2C_EN); > - } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { > - dev_err(&priv->pci_dev->dev, > - "I2C block read is unsupported!\n"); > - return -EOPNOTSUPP; > - } > - } > > if (read_write == I2C_SMBUS_WRITE > || command == I2C_SMBUS_I2C_BLOCK_DATA) { > @@ -699,11 +685,6 @@ static int i801_block_transaction(struct i801_priv *priv, > read_write, > command); > > - if (command == I2C_SMBUS_I2C_BLOCK_DATA > - && read_write == I2C_SMBUS_WRITE) { > - /* restore saved configuration register value */ > - pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc); > - } > return result; > } > > @@ -715,6 +696,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > int hwpec; > int block = 0; > int ret = 0, xact = 0; > + int hostc = -1; > struct i801_priv *priv = i2c_get_adapdata(adap); > > pm_runtime_get_sync(&priv->pci_dev->dev); > @@ -764,12 +746,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > /* NB: page 240 of ICH5 datasheet shows that the R/#W > * bit should be cleared here, even when reading */ > outb_p((addr & 0x7f) << 1, SMBHSTADD(priv)); > - if (read_write == I2C_SMBUS_READ) { > + if (read_write == I2C_SMBUS_WRITE) { > + unsigned char thostc; > + > + outb_p(command, SMBHSTCMD(priv)); > + /* set I2C_EN bit in configuration register */ > + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &thostc); > + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, > + thostc | SMBHSTCFG_I2C_EN); > + hostc = thostc; > + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { nitpick: I'd prefer seeing: if (write) { foo } else { if (!FEATURE_I2C_BLOCK_READ) { goto out } bar } It's equivalent but I feel like it's easier to understand. Other than that, no strong opinion on the patch itself, but it's correct (the code path stays the same): Reviewed-by: Benjamin Tissoires Cheers, Benjamin > + dev_err(&priv->pci_dev->dev, > + "I2C block read is unsupported!\n"); > + ret = -EOPNOTSUPP; > + goto out; > + } else > /* NB: page 240 of ICH5 datasheet also shows > * that DATA1 is the cmd field when reading */ > outb_p(command, SMBHSTDAT1(priv)); > - } else > - outb_p(command, SMBHSTCMD(priv)); > block = 1; > break; > default: > @@ -798,6 +792,9 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > outb_p(inb_p(SMBAUXCTL(priv)) & > ~(SMBAUXCTL_CRC | SMBAUXCTL_E32B), SMBAUXCTL(priv)); > > + if (hostc >= 0) > + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc); > + > if (block) > goto out; > if (ret)