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

["Pali Rohár" <pali@kernel.org>]

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.


#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");