Re: [PATCH v3 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/23/24 3:56 PM, Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 6:35 PM, Pali Rohár wrote:
>> On Saturday 22 June 2024 17:12:50 Pali Rohár wrote:
>>> On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
>>>> Hi Pali,
>>>>
>>>> On 6/22/24 4:20 PM, Pali Rohár wrote:
>>>>> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>>>>>> Hi Pali,
>>>>>>
>>>>>> On 6/22/24 3:16 PM, Pali Rohár wrote:
>>>>>>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>>>>>>>> It is not necessary to handle the Dell specific instantiation of
>>>>>>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>>>>>>>> inside the generic i801 I2C adapter driver.
>>>>>>>>
>>>>>>>> The kernel already instantiates platform_device-s for these ACPI devices
>>>>>>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>>>>>>>> platform drivers.
>>>>>>>>
>>>>>>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>>>>>>>> the SMO88xx specific dell-smo8800 driver.
>>>>>>>
>>>>>>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
>>>>>>> and freefall code for smo88xx are basically independent.
>>>>>>>
>>>>>>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
>>>>>>> detection. The only thing which have these "drivers" common is the ACPI
>>>>>>> detection mechanism based on presence of SMO88?? identifiers from
>>>>>>> acpi_smo8800_ids[] array.
>>>>>>>
>>>>>>> I think it makes both "drivers" cleaner if they are put into separate
>>>>>>> files as they are independent of each one.
>>>>>>>
>>>>>>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
>>>>>>> instead (or similar name)? And just share list of ACPI ids via some
>>>>>>> header file (or something like that).
>>>>>>
>>>>>> Interesting idea, but that will not work, only 1 driver can bind to
>>>>>> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
>>>>>
>>>>> And it is required to bind lis3 device to ACPI code? What is needed is
>>>>> just to check if system matches DMI strings and ACPI strings. You are
>>>>> not binding device to DMI strings, so I think there is no need to bind
>>>>> it neither to ACPI strings.
>>>>
>>>> The driver needs to bind to something ...
>>>
>>> Why?
>>>
>>> Currently in i2c-i801.c file was called just
>>> register_dell_lis3lv02d_i2c_device() function and there was no binding
>>> to anything, no binding to DMI structure and neither no binding to ACPI
>>> structures.
>>>
>>> And if I'm looking correctly at your new function
>>> smo8800_instantiate_i2c_client() it does not bind device neither.
>>> And smo8800_i2c_bus_notify() does not depend on binding.
>>>
>>> So I do not see where is that binding requirement.
>>
>> Now I have tried to do it, to move code into dell-lis3lv02d.c file.
>>
>> Below is example how it could look like. I reused most of your code.
>> I have not tested it. It is just an idea.
> 
> Thank you for going through the trouble of writing this proof
> of concept.
> 
> The problem with DMI matching, instead of binding to the ACPI
> SMO88xx platform_device is that this will now only load on
> laptops for which we already have the DMI ids.

I guess we could put all of this from the current dell-smo8800.c
code in a .h :

static const struct acpi_device_id smo8800_ids[] = {
        { "SMO8800", 0 },
        { "SMO8801", 0 },
        { "SMO8810", 0 },
        { "SMO8811", 0 },
        { "SMO8820", 0 },
        { "SMO8821", 0 },
        { "SMO8830", 0 },
        { "SMO8831", 0 },
        { "", 0 },
};
MODULE_DEVICE_TABLE(acpi, smo8800_ids);

Andy, I guess this is what you ment with your MODULE_DEVICE_TABLE()
comment?

And then include that .h from both modules. Then both modules
will auto-load and in the dell-lis3lv02d module we can use
module_init() / module_exit() to register the bus-notifier.

Pali, since you have expressed a clear preference for splitting
things and since this solution should not impact functionality
in anyway, I'll do this split for v4 of this series.

I do plan to re-use the existing CONFIG_DELL_SMO8800 Kconfig
value to decide whether or not to build the new dell-lis3lv02d
module. Having 2 separate Kconfig options for this seems unnecessary.

Regards,

Hans




> 
> So this looses the warning about the i2c address info missing
> which we currently give when there is a SMO88xx ACPI device
> on laptops not in the DMI table (the current i2c-i801.c code
> has this already). Not giving the warning in turn means we
> cannot inform users about trying the new probe option, which is
> the whole reason to do this patch-set in the first place.
> 
> I still believe that keeping this new code in dell-smo8800.c
> is best:
> 
> 1. This very much is about handling the SMO88xx ACPI devices
> which makes putting it in the smo8800.c driver the logical /
> the right thing to do.
> 
> 2. This only adds 400 lines of code. After this all of
> dell-smo8800.c is only 600 lines. That is very small so
> I really so no pressing need to spread this out over 2 files.
> 
> 3. You claim the freefall IRQ and the i2c_client instantiation
> are 2 different things, but they are both for the same chip
> and normally would both be described in the same ACPI device
> node. The manual i2c_client instantation is done to address
> a shortcoming of the SMO88xx ACPI device node, so handling it
> belongs in the smo8800 driver.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>> #include <linux/acpi.h>
>> #include <linux/dmi.h>
>> #include <linux/i2c.h>
>> #include <linux/module.h>
>> #include <linux/notifier.h>
>> #include <linux/workqueue.h>
>>
>> static struct work_struct dell_lis3lv02d_i2c_work;
>> static struct i2c_client *dell_lis3lv02d_i2c_dev;
>> static unsigned short dell_lis3lv02d_i2c_addr;
>>
>> static int dell_lis3lv02d_find_i801(struct device *dev, void *data)
>> {
>> 	struct i2c_adapter *adap, **adap_ret = data;
>>
>> 	adap = i2c_verify_adapter(dev);
>> 	if (!adap)
>> 		return 0;
>>
>> 	if (!strstarts(adap->name, "SMBus I801 adapter"))
>> 		return 0;
>>
>> 	*adap_ret = i2c_get_adapter(adap->nr);
>> 	return 1;
>> }
>>
>> static void dell_lis3lv02d_instantiate_i2c_client(struct work_struct *work)
>> {
>> 	struct i2c_board_info info = { };
>> 	struct i2c_adapter *adap = NULL;
>> 	struct i2c_client *client;
>>
>> 	if (dell_lis3lv02d_i2c_dev)
>> 		return;
>>
>> 	bus_for_each_dev(&i2c_bus_type, NULL, &adap, dell_lis3lv02d_find_i801);
>> 	if (!adap)
>> 		return;
>>
>> 	info.addr = dell_lis3lv02d_i2c_addr;
>> 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>>
>> 	client = i2c_new_client_device(adap, &info);
>> 	if (IS_ERR(client)) {
>> 		pr_err("error %ld registering %s i2c_client\n",
>> 			PTR_ERR(client), info.type);
>> 		return;
>> 	}
>>
>> 	dell_lis3lv02d_i2c_dev = client;
>>
>> 	pr_err("registered %s i2c_client on address 0x%02x\n",
>> 		info.type, info.addr);
>> }
>>
>> static int dell_lis3lv02d_i2c_bus_notify(struct notifier_block *nb,
>> 					 unsigned long action, void *data)
>> {
>> 	struct device *dev = data;
>> 	struct i2c_client *client;
>> 	struct i2c_adapter *adap;
>>
>> 	switch (action) {
>> 	case BUS_NOTIFY_ADD_DEVICE:
>> 		adap = i2c_verify_adapter(dev);
>> 		if (!adap)
>> 			break;
>>
>> 		if (strstarts(adap->name, "SMBus I801 adapter"))
>> 			queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
>> 		break;
>> 	case BUS_NOTIFY_REMOVED_DEVICE:
>> 		client = i2c_verify_client(dev);
>> 		if (!client)
>> 			break;
>>
>> 		if (dell_lis3lv02d_i2c_dev == client) {
>> 			pr_debug("accelerometer i2c_client removed\n");
>> 			dell_lis3lv02d_i2c_dev = NULL;
>> 		}
>> 		break;
>> 	default:
>> 		break;
>> 	}
>>
>> 	return 0;
>> }
>>
>> // TODO: move this array into dell-smo8800.h header file
>> static const char *const acpi_smo8800_ids[] __initconst = {
>> 	"SMO8800",
>> 	"SMO8801",
>> 	"SMO8810",
>> 	"SMO8811",
>> 	"SMO8820",
>> 	"SMO8821",
>> 	"SMO8830",
>> 	"SMO8831",
>> };
>>
>> static acpi_status __init 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_FAILURE(status))
>> 		return AE_OK;
>>
>> 	if (!(info->valid & ACPI_VALID_HID))
>> 		goto smo88xx_not_found;
>>
>> 	hid = info->hardware_id.string;
>> 	if (!hid)
>> 		goto smo88xx_not_found;
>>
>> 	i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid);
>> 	if (i < 0)
>> 		goto smo88xx_not_found;
>>
>> 	kfree(info);
>>
>> 	*return_value = NULL;
>> 	return AE_CTRL_TERMINATE;
>>
>> smo88xx_not_found:
>> 	kfree(info);
>> 	return AE_OK;
>> }
>>
>> static bool __init has_acpi_smo88xx_device(void)
>> {
>> 	void *err = ERR_PTR(-ENOENT);
>>
>> 	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
>> 	return !IS_ERR(err);
>> }
>>
>> /*
>>  * Accelerometer's I2C address is not specified in DMI nor ACPI,
>>  * so it is needed to define mapping table based on DMI product names.
>>  */
>> static const struct dmi_system_id dell_lis3lv02d_devices[] __initconst = {
>> 	/*
>> 	 * Dell platform team told us that these Latitude devices have
>> 	 * ST microelectronics accelerometer at I2C address 0x29.
>> 	 */
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	/*
>> 	 * Additional individual entries were added after verification.
>> 	 */
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
>> 		},
>> 		.driver_data = (void *)0x1d,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{ }
>> };
>> MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);
>>
>> static struct notifier_block dell_lis3lv02d_i2c_nb = {
>> 	.notifier_call = dell_lis3lv02d_i2c_bus_notify,
>> };
>>
>> static int __init dell_lis3lv02d_init(void)
>> {
>> 	const struct dmi_system_id *lis3lv02d_dmi_id;
>> 	int err;
>>
>> 	if (!has_acpi_smo88xx_device())
>> 		return -ENODEV;
>>
>> 	lis3lv02d_dmi_id = dmi_first_match(dell_lis3lv02d_devices);
>> 	if (!lis3lv02d_dmi_id)
>> 		return -ENODEV;
>>
>> 	dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;
>>
>> 	err = bus_register_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
>> 	if (err)
>> 		return err;
>>
>> 	INIT_WORK(&dell_lis3lv02d_i2c_work, dell_lis3lv02d_instantiate_i2c_client);
>> 	queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
>>
>> 	return 0;
>> }
>>
>> static void __exit dell_lis3lv02d_exit(void)
>> {
>> 	bus_unregister_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
>> 	cancel_work_sync(&dell_lis3lv02d_i2c_work);
>> 	if (dell_lis3lv02d_i2c_dev)
>> 		i2c_unregister_device(dell_lis3lv02d_i2c_dev);
>> }
>>
>> module_init(dell_lis3lv02d_init);
>> module_exit(dell_lis3lv02d_exit);
>>
>> MODULE_LICENSE("GPL");
>>
>