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