Re: [PATCH V2] Intel Lewisburg device IDs for SMBus

[Alexandra Yates <alexandra.yates@linux.intel.com>]

Hi Jean,

On 11/05/2015 12:45 AM, Jean Delvare wrote:
> Hi Alexandra,
>
> On Wed,  4 Nov 2015 11:09:16 -0800, Alexandra Yates wrote:
>> Adding Intel codename Lewisburg platform device IDs for SMBus.
>>
>> Signed-off-by: Alexandra Yates <alexandra.yates@linux.intel.com>
>> ---
>>   Documentation/i2c/busses/i2c-i801 | 1 +
>>   drivers/i2c/busses/Kconfig        | 1 +
>>   drivers/i2c/busses/i2c-i801.c     | 6 ++++++
>>   3 files changed, 8 insertions(+)
>>
>> diff --git a/Documentation/i2c/busses/i2c-i801 b/Documentation/i2c/busses/i2c-i801
>> index 6a4b1af..1bba38d 100644
>> --- a/Documentation/i2c/busses/i2c-i801
>> +++ b/Documentation/i2c/busses/i2c-i801
>> @@ -32,6 +32,7 @@ Supported adapters:
>>     * Intel Sunrise Point-LP (PCH)
>>     * Intel DNV (SOC)
>>     * Intel Broxton (SOC)
>> +  * Intel Lewisburg (PCH)
>>      Datasheets: Publicly available at the Intel website
>>
>>   On Intel Patsburg and later chipsets, both the normal host SMBus controller
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index e24c2b6..7b0aa82 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -126,6 +126,7 @@ config I2C_I801
>>   	    Sunrise Point-LP (PCH)
>>   	    DNV (SOC)
>>   	    Broxton (SOC)
>> +	    Lewisburg (PCH)
>>
>>   	  This driver can also be built as a module.  If so, the module
>>   	  will be called i2c-i801.
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index c306751..76fcef4 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -62,6 +62,8 @@
>>    * Sunrise Point-LP (PCH)	0x9d23	32	hard	yes	yes	yes
>>    * DNV (SOC)			0x19df	32	hard	yes	yes	yes
>>    * Broxton (SOC)		0x5ad4	32	hard	yes	yes	yes
>> + * Lewisburg (PCH)		0xA1A3	32	hard	yes	yes	yes
>> + * Lewisburg Supersku (PCH)	0xA223	32	hard	yes	yes	yes
>
> It's a bit unfortunate that you used upper-case letters for the
> hexadecimal numbers when all other entries used lower-case letters.
>
>>    *
>>    * Features supported by this driver:
>>    * Software PEC				no
>> @@ -181,6 +183,8 @@
>>   				 STATUS_ERROR_FLAGS)
>>
>>   /* Older devices have their ID defined in <linux/pci_ids.h> */
>> +#define PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS		0xA1A3
>> +#define PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS	0xA223
>
> Same here. Plus, why are you adding the IDs at the top of the list here
> while you added them at the bottom of the first list...
>
>>   #define PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS		0x0f12
>>   #define PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS		0x2292
>>   #define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS		0x1c22
>> @@ -864,6 +868,8 @@ static const struct pci_device_id i801_ids[] = {
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WILDCATPOINT_SMBUS) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_WILDCATPOINT_LP_SMBUS) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BAYTRAIL_SMBUS) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SMBUS) },
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_LEWISBURG_SSKU_SMBUS) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BRASWELL_SMBUS) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_SMBUS) },
>>   	{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_SMBUS) },
>
> ... and now in the middle of the last list? I would appreciate more
> consistency.
>
> These are all details of course, overall the changes look good, so you
> can add:
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>

Thank you for your review.   I sent the V3 of this patch with the 
changes you suggested.

-- 
Thank you,
<Alexandra>