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/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.

As for Pali's suggestion of having the i2c-i801 code call a symbol
exported by dell-smo8800 that will cause the dell-smo8800 driver
to load on all x86 devices with an i2c-i801 controller (pretty
much all of them). Slowing the boot and eating memory.

So IMHO just doing the scan for the i2c-i801 adapter as already
done in this version of the patch-set, extended with returning
-EPROBE_DEFER if it is not found is the best solution.

Yes this means losing the i2c_client for the lis3lv02d device
if the i2c-i801 driver is manually unbound or rmmod-ed. But that
requires explicit root user action and putting just the i801
driver back in place again also requires implicit root action.

IMHO it is acceptable that in this exceptional case, which
normal users will never hit, a rmmod + modprobe of dell-smo8800
is required to re-gain the lis3lv02d i2c_client.

Regards,

Hans


1) touchscreen_dmi is builtin only because of this and we really
want to avoid adding more of that. Actually thinking more about this
it would be nice to modify touchscreen_dmi to use a mix of registering
the notifier + doing a scan after registering it for matching devices
which are already present, so that touchscreen_dmi can become a module
auto-loaded using the DMI info which it already contains.











> 
>>>> With this simple change all dell smo8800 code would be in its subdir
>>>> drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code.
>>>>
>>>> This approach does not change any functionality, so should be absolutely
>>>> safe.
>>>>
>>>> Future changes will be done only in drivers/platform/x86/dell/ subdir,
>>>> touching i801 would not be needed at all.
>>>
>>> Still these exported functions are not the best solution we can do,
>>> right? We should be able to decouple them without need for the custom
>>> APIs.
>>
>> Well, what I described here is a simple change which get rid of the one
>> problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx
>> logic (like adding a new device id) requires touching unrelated
>> i2c-i801.c source file.
> 
> `get rid of one problem` --> `replace one by another (but maybe less
> critical, dunno) problem`. The new one is the spread of custom APIs
> for a single user, which also requires an additional, shared header
> file and all hell with the Kconfig dependencies.
> 
>> I like small changes which can be easily reviewed and address one
>> problem. Step by step. That is why I proposed it here.
>>
>> For decoupling it is needed to get newly instanced adapter (if the
>> mentioned notifier is able to tell this information) and also it is
>> needed to check if the adapter is the i801.
> 
> Yes.
> 
>