Re: [PATCH] i2c: i801: fix DNV's SMBCTRL register offset

[Jean Delvare <jdelvare@suse.de>]

Hi Felipe,

On Mon,  3 Sep 2018 08:37:05 +0300, Felipe Balbi wrote:
> DNV's iTCO is slightly different with SMBCTRL sitting at a differnt
> offset when compared to all other devices. Let's fix so that we can
> properly use iTCO watchdog.
> 
> Fixes: 84d7f2ebd70d ("i2c: i801: Add support for Intel DNV")
> Cc: <stable@vger.kernel.org> # v4.4+
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 941c223f6491..390bf253b6ea 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1400,6 +1400,11 @@ static void i801_add_tco(struct i801_priv *priv)
>  
>  	res = &tco_res[ICH_RES_MEM_OFF];
>  	res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;
> +
> +	/* DNV device has SMBCTRL at 0xcf000c */
> +	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
> +		res->start += 0x90000;
> +
>  	res->end = res->start + 3;
>  	res->flags = IORESOURCE_MEM;
>  

Thanks for catching this but this is not the way I want it to be fixed.
Applying an arbitrary offset like that is pretty obscure and fragile
too. The value of SMBCTRL for DNV should instead appear explicitly in
the code. Something like that:

#define SBREG_SMBCTRL		0xc6000c
#define SBREG_SMBCTRL_DNV	0xcf000c

	if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)
		res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL_DNV;
	else
		res->start = (resource_size_t)base64_addr + SBREG_SMBCTRL;

Alternatively you can add a member to struct i801_priv to store the
register address in i801_probe(), and use that in i801_add_tco(). The
above defines could even go away then. Both approaches are fine with me.

-- 
Jean Delvare
SUSE L3 Support