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

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

Hi,

On 6/24/24 8:14 PM, Pali Rohár wrote:
> On Monday 24 June 2024 13:15:16 Hans de Goede wrote:
>> +static int match_acpi_device_ids(struct device *dev, const void *data)
>> +{
> 
> You can mark this function as __init as it is called only from
> dell_lis3lv02d_init to free space.
> 
>> +	const struct acpi_device_id *ids = data;
>> +
>> +	return acpi_match_device(ids, dev) ? 1 : 0;
>> +}
>> +
>> +static int __init dell_lis3lv02d_init(void)
>> +{
>> +	struct device *dev;
>> +	int err;
>> +
>> +	/*
>> +	 * First check for a matching platform_device. This protects against
>> +	 * SMO88xx ACPI fwnodes which actually do have an I2C resource, which
>> +	 * will already have an i2c_client instantiated (not a platform_device).
>> +	 */
>> +	dev = bus_find_device(&platform_bus_type, NULL, smo8800_ids, match_acpi_device_ids);
>> +	if (!dev) {
>> +		pr_debug("No SMO88xx platform-device found\n");
>> +		return 0;
> 
> Is zero return value expected? Should not be it something like -ENODEV?

This is a module_init() function returning non 0 leads to an error getting
logged and the modprobe command to return a non 0 value which is not what
we want.

>> +	}
>> +	put_device(dev);
>> +
>> +	lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
>> +	if (!lis3lv02d_dmi_id) {
> 
> You can cache the value lis3lv02d_dmi_id->driver_data instead of caching
> lis3lv02d_dmi_id pointer and then you can mark lis3lv02d_devices array
> as __initconst to free additional space not needed at runtime on x86
> machines without accelerometer where CONFIG_DELL_SMO8800=y.

This is a good idea, done for v5.

Regards,

Hans


> 
>> +		pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * Register i2c-bus notifier + queue initial scan for lis3lv02d
>> +	 * i2c_client instantiation.
>> +	 */
>> +	err = bus_register_notifier(&i2c_bus_type, &i2c_nb);
>> +	if (err)
>> +		return err;
>> +
>> +	notifier_registered = true;
>> +
>> +	queue_work(system_long_wq, &i2c_work);
>> +	return 0;
>> +}
>> +module_init(dell_lis3lv02d_init);
>> +
>> +static void __exit dell_lis3lv02d_module_exit(void)
>> +{
>> +	if (!notifier_registered)
>> +		return;
>> +
>> +	bus_unregister_notifier(&i2c_bus_type, &i2c_nb);
>> +	cancel_work_sync(&i2c_work);
>> +	i2c_unregister_device(i2c_dev);
>> +}
>> +module_exit(dell_lis3lv02d_module_exit);
>> +
>> +MODULE_DESCRIPTION("lis3lv02d i2c-client instantiation for ACPI SMO88xx devices");
>> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.45.1
>>
>