Re: [PATCH v3] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

[Pali RohÃr]

Hi!

On Tuesday 04 June 2019 16:57:29 Jean Delvare wrote:
> Hi Pali,
> 
> Next time, please start a new thread for a new version of a patch.

Ok!

> On Sun,  2 Jun 2019 15:20:03 +0200, Pali RohÃr wrote:
> > Dell platform team told us that some (DMI whitelisted) Dell Latitude
> > machines have ST microelectronics accelerometer at i2c address 0x29.
> > 
> > Presence of that ST microelectronics accelerometer is verified by existence
> > of SMO88xx ACPI device which represent that accelerometer. Unfortunately
> > ACPI device does not specify i2c address.
> > 
> > This patch registers lis3lv02d device for selected Dell Latitude machines
> > at i2c address 0x29 after detection. And for Dell Vostro V131 machine at
> > i2c address 0x1d which was manually detected.
> > 
> > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to
> > conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so
> > lis3lv02d correctly initialize accelerometer.
> > 
> > Tested on Dell Latitude E6440.
> > 
> > Signed-off-by: Pali RohÃr <pali.rohar@xxxxxxxxx>
> > 
> > ---
> > Changes since v2:
> >  * Use explicit list of SMOxx ACPI devices
> > 
> > Changes since v1:
> >  * Added Dell Vostro V131 based on MichaÅ KÄpieÅ testing
> >  * Changed DMI product structure to include also i2c address
> > ---
> >  drivers/i2c/busses/i2c-i801.c       | 118 ++++++++++++++++++++++++++++++++++++
> >  drivers/platform/x86/dell-smo8800.c |   1 +
> >  2 files changed, 119 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ac7f7817dc89..2ac8ff41cc24 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1134,6 +1134,121 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
> >  	}
> >  }
> >  
> > +/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
> > +static const struct acpi_device_id acpi_smo8800_ids[] = {
> > +	{ "SMO8800", 0 },
> > +	{ "SMO8801", 0 },
> > +	{ "SMO8810", 0 },
> > +	{ "SMO8811", 0 },
> > +	{ "SMO8820", 0 },
> > +	{ "SMO8821", 0 },
> > +	{ "SMO8830", 0 },
> > +	{ "SMO8831", 0 },
> > +};
> > +
> > +static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> > +					     u32 nesting_level,
> > +					     void *context,
> > +					     void **return_value)
> > +{
> > +	struct acpi_device_info *info;
> > +	acpi_status status;
> > +	char *hid;
> > +	int i;
> > +
> > +	status = acpi_get_object_info(obj_handle, &info);
> > +	if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
> > +		return AE_OK;
> > +
> > +	hid = info->hardware_id.string;
> > +	if (!hid)
> > +		return AE_OK;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
> > +		if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
> > +			*((bool *)return_value) = true;
> > +			return AE_CTRL_TERMINATE;
> > +		}
> > +	}
> > +
> > +	return AE_OK;
> > +
> 
> Unneeded blank line.

Ok.

> > +}
> > +
> > +static bool is_dell_system_with_lis3lv02d(void)
> > +{
> > +	bool found;
> > +	acpi_status status;
> > +	const char *vendor;
> > +
> > +	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
> > +	if (strcmp(vendor, "Dell Inc.") != 0)
> > +		return false;
> > +
> > +	/*
> > +	 * Check that ACPI device SMO88xx exists and is enabled. That ACPI
> 
> I see how you check that the device exists, but not that it is enabled.

Hm.. you are right. I will add missing check.

> > +	 * device represent our ST microelectronics lis3lv02d accelerometer but
> > +	 * unfortunately without any other information (like i2c address).
> 
> I2C

I will change i2c to I2C in whole patch.

> > +	 */
> > +	found = false;
> > +	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > +				  (void **)&found);
> > +	if (!ACPI_SUCCESS(status) || !found)
> > +		return false;
> > +
> > +	return true;
> 
> Looks more complex than it needs to be. You don't really care about the
> status, as in the end you return the same on error as you do when no
> device is found. So I think you can simply go with:
> 
> 	found = false;
> 	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> 				  (void **)&found);
> 
> 	return found;

Ok, it really simplify that check.

> 
> > +}
> > +
> > +/*
> > + * Accelerometer's i2c address is not specified in DMI nor ACPI,
> 
> I2C
> 
> > + * so it is needed to define mapping table based on DMI product names.
> > + */
> > +static struct {
> 
> Any reason to not make it const?

No, I will make it const.

> > +	const char *dmi_product_name;
> > +	unsigned short i2c_addr;
> > +} dell_lis3lv02d_devices[] = {
> > +	/*
> > +	 * Dell platform team told us that these Latitude devices have
> > +	 * ST microelectronics accelerometer at i2c address 0x29.
> 
> I2C
> 
> > +	 */
> > +	{ "Latitude E5250",     0x29 },
> > +	{ "Latitude E5450",     0x29 },
> > +	{ "Latitude E5550",     0x29 },
> > +	{ "Latitude E6440",     0x29 },
> > +	{ "Latitude E6440 ATG", 0x29 },
> > +	{ "Latitude E6540",     0x29 },
> > +	/*
> > +	 * Additional individual entries were added after verification.
> > +	 */
> > +	{ "Vostro V131",        0x1d },
> > +};
> > +
> > +static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
> > +{
> > +	struct i2c_board_info info;
> > +	const char *dmi_product_name;
> > +	int i;
> > +
> > +	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > +	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
> > +		if (strcmp(dmi_product_name,
> > +			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
> > +			break;
> > +	}
> > +
> > +	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
> > +		dev_warn(&priv->pci_dev->dev,
> > +			 "Accelerometer lis3lv02d is present on i2c bus but its"
> 
> i2c bus -> SMBus
> 
> > +			 " i2c address is unknown, skipping registration...\n");
> 
> I2C (or just s/i2c //, as it's kind of redundant)
> 
> Suspension points not really needed IMHO.

Ok, it will be in V4 which I sent in few minutes.

> > +		return;
> > +	}
> > + (...)
> 
> All the rest looks good to me.
> 
> Thanks,

-- 
Pali RohÃr
pali.rohar@xxxxxxxxx

Attachment: signature.asc
Description: PGP signature