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

Andy Shevchenko
2024-02-27T21:19:57Z

On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
> On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote:
> > On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote:
> > > On 2/13/24 17:30, Jean Delvare wrote:
> > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> > > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> >
> > FWIW, I agree with Hans on the location of the HW quirks.
> > If there is a possible way to make the actual driver cleaner
> > and collect quirks somewhere else, I'm full support for that.
>
> 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
> Put this file under a new hidden "bool" config option which is auto
> enabled when CONFIG_DELL_SMO8800 is used.
>
> i2c-i801.c currently has code:
>
> if (is_dell_system_with_lis3lv02d())
> register_dell_lis3lv02d_i2c_device(priv);
>
> This can be put into a new exported function, e.g.
> void dell_smo8800_scan_i2c(struct i2c_adapter *adapter);
> And i2c-i801.c would call it instead.
>
> register_dell_lis3lv02d_i2c_device just needs "adapter", it does not
> need whole i801 priv struct.

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?

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

--
With Best Regards,
Andy Shevchenko