From: "Pali Rohár" <pali@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: "Andy Shevchenko" <andy@kernel.org>,
"Hans de Goede" <hdegoede@redhat.com>,
"Jean Delvare" <jdelvare@suse.de>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Paul Menzel" <pmenzel@molgen.mpg.de>,
"Andi Shyti" <andi.shyti@kernel.org>,
eric.piel@tremplin-utc.net, "Marius Hoch" <mail@mariushoch.de>,
Dell.Client.Kernel@dell.com,
"Kai Heng Feng" <kai.heng.feng@canonical.com>,
platform-driver-x86@vger.kernel.org,
"Wolfram Sang" <wsa@kernel.org>,
linux-i2c@vger.kernel.org
Subject: Re: [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
Date: Tue, 27 Feb 2024 22:50:00 +0100 [thread overview]
Message-ID: <20240227215000.gbmn4n2uzd3hyk3b@pali> (raw)
In-Reply-To: <CAHp75VeGaKws35x4u-mrmWP2Rd55T6VcR9OjNfh+PsF_M9GR-g@mail.gmail.com>
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:
> > 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?
I do not know how flexible are notifiers. Can notifier call our callback
when new "struct i2c_adapter *adapter" was instanced?
> > 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.
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.
next prev parent reply other threads:[~2024-02-27 21:50 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-06 16:09 [PATCH v2 0/6] i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 Hans de Goede
2024-01-06 16:09 ` [PATCH v2 1/6] platform/x86: dell-smo8800: Change probe() ordering a bit Hans de Goede
2024-01-06 16:09 ` [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 Hans de Goede
2024-01-06 16:24 ` Andy Shevchenko
2024-01-06 19:54 ` Joe Perches
2024-01-07 16:09 ` Steven Rostedt
2024-01-07 16:20 ` Steven Rostedt
2024-01-06 16:38 ` Pali Rohár
2024-01-07 17:10 ` Pali Rohár
2024-02-13 16:30 ` Jean Delvare
2024-02-17 10:33 ` Hans de Goede
2024-02-19 11:52 ` Andy Shevchenko
2024-02-27 21:04 ` Pali Rohár
2024-02-27 21:19 ` Andy Shevchenko
2024-02-27 21:50 ` Pali Rohár [this message]
2024-02-27 22:37 ` Andy Shevchenko
2024-02-28 12:50 ` Hans de Goede
2024-02-29 20:46 ` Pali Rohár
2024-03-02 11:02 ` Hans de Goede
2024-03-02 11:19 ` Pali Rohár
2024-02-27 21:40 ` Pali Rohár
2024-02-28 13:10 ` Hans de Goede
2024-02-28 16:49 ` Andy Shevchenko
2024-02-29 20:57 ` Pali Rohár
2024-03-02 11:38 ` Hans de Goede
2024-03-03 11:14 ` Andy Shevchenko
2024-01-13 4:42 ` kernel test robot
2024-01-13 7:46 ` kernel test robot
2024-01-06 16:09 ` [PATCH v2 3/6] platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client Hans de Goede
2024-01-06 16:09 ` [PATCH v2 4/6] platform/x86: dell-smo8800: Allow using the IIO st_accel driver Hans de Goede
2024-01-13 9:55 ` kernel test robot
2024-01-13 14:24 ` kernel test robot
2024-01-06 16:09 ` [PATCH v2 5/6] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address Hans de Goede
2024-01-06 16:09 ` [PATCH v2 6/6] platform/x86: dell-smo8800: Add a couple more models to dell_lis3lv02d_devices[] Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240227215000.gbmn4n2uzd3hyk3b@pali \
--to=pali@kernel.org \
--cc=Dell.Client.Kernel@dell.com \
--cc=andi.shyti@kernel.org \
--cc=andy.shevchenko@gmail.com \
--cc=andy@kernel.org \
--cc=eric.piel@tremplin-utc.net \
--cc=hdegoede@redhat.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=jdelvare@suse.de \
--cc=kai.heng.feng@canonical.com \
--cc=linux-i2c@vger.kernel.org \
--cc=mail@mariushoch.de \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pmenzel@molgen.mpg.de \
--cc=wsa@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).