Re: [PATCH v2 2/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 2/29/24 21:46, Pali Rohár wrote:
> On Wednesday 28 February 2024 13:50:27 Hans de Goede wrote:
>> Hi,
>>
>> On 2/27/24 23:37, Andy Shevchenko wrote:
>>> On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote:
>>>> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote:
>>>>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
>>>
>>> ...
>>>
>>>>> I'm wondering why we need all this. We have notifiers when a device is
>>>>> added / removed. We can provide a board_info for the device and attach
>>>>> it to the proper adapter, no?
>>>>
>>>> I do not know how flexible are notifiers. Can notifier call our callback
>>>> when new "struct i2c_adapter *adapter" was instanced?
>>>
>>> You can follow notifications of *an* I2C adapter being added /
>>> removed. With that, you can filter which one is that. Based on that
>>> you may attach a saved (at __init as you talked about in the reply to
>>> Hans) board_info with all necessary information.
>>>
>>> Something like this (combined)
>>> https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515
>>> https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194
>>
>> drivers/platform/x86/touchscreen_dmi.c actually already does something
>> like this for i2c-clients. The problem is that this brings probe-ordering
>> problems with it. If the i801 driver is loaded before the dell-smo8800
>> driver then the notifiers will not trigger since the i2c-adapter has
>> already been created (1).
>>
>> So we would still need a "cold-plug" manual scan in smo8800_probe()
>> anyways at which point we might as well just return -EPROBE_DEFER
>> when the adapter is not there.
> 
> And that it example why the current existing solution is better, it does
> not have such problems like the proposed.
> 
>> As for Pali's suggestion of having the i2c-i801 code call a symbol
>> exported by dell-smo8800
> 
> I did not suggest that! Please do not make false statements about me.

In: https://lore.kernel.org/platform-driver-x86/20240227210429.l5o52wuexqqmrpol@pali/

You write:

"Location of the quirks can be moved outside of the i2c-i801.c source
code relatively easily without need to change the way how parent--child
relationship currently works.

Relevant functions is_dell_system_with_lis3lv02d() and
register_dell_lis3lv02d_i2c_device() does not use internals of
i2c-i801 and could be moved into new file, lets say
drivers/platform/x86/dell/dell-smo8800-plat.c"

Ok I see now you suggest moving the code to a new file,
sorry for misreading that.

But the point is that moving the code does not help because
since there is a symbol used from the new code it will still
get loaded on all machines were the i2c-i801.c driver gets
loaded. So it will still be taking up RAM and increases
boot time (loading an extra module consumes time) on machines
which do not have any SMO88xx devices.

And that point is still valid even independent of the code
is moved to the existing dell-smo8800 module or to a new
dell-smo8800-plat module.

Regards,

Hans