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