All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/7] Introduce intel_skl_int3472 driver
@ 2021-01-18  0:34 Daniel Scally
  2021-01-18  0:34 ` [PATCH v2 1/7] acpi: utils: move acpi_lpss_dep() to utils Daniel Scally
                   ` (6 more replies)
  0 siblings, 7 replies; 107+ messages in thread
From: Daniel Scally @ 2021-01-18  0:34 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-gpio, linux-i2c,
	platform-driver-x86, devel
  Cc: rjw, lenb, andy, mika.westerberg, linus.walleij, bgolaszewski,
	wsa, lee.jones, hdegoede, mgross, robert.moore, erik.kaneda,
	sakari.ailus, andriy.shevchenko, laurent.pinchart, kieran.bingham

Hello all

v1 for this series was originally 14-18 of this series:
https://lore.kernel.org/linux-media/20201130133129.1024662-1-djrscally@gmail.com/T/#m91934e12e3d033da2e768e952ea3b4a125ee3e67

At the moment in the kernel the ACPI _HID INT3472 is taken by the tps68470
MFD driver, but that driver can only handle some of the cases of that _HID
that we see. There are at least these three possibilities:

1. INT3472 devices that provide GPIOs through the usual framework and run
   power and clocks through an operation region; this is the situation that
   the current module handles and is seen on ChromeOS devices
2. INT3472 devices that provide GPIOs, plus clocks and regulators that are
   meant to be driven through the usual frameworks, usually seen on devices
   designed to run Windows
3. INT3472 devices that don't actually represent a physical tps68470, but
   are being used as a convenient way of grouping a bunch of system GPIO
   lines that are intended to enable power and clocks for sensors which
   are called out as dependent on them. Also seen on devices designed to
   run Windows.

This series introduces a new module which registers:

1. An i2c driver that determines which scenario (#1 or #2) applies to the
   machine and registers platform devices to be bound to GPIO, OpRegion,
   clock and regulator drivers as appropriate.
2. A platform driver that binds to the dummy INT3472 devices described in
   #3

The platform driver for the dummy device registers the GPIO lines that
enable the clocks and regulators to the sensors via those frameworks so
that sensor drivers can consume them in the usual fashion. The existing
GPIO and OpRegion tps68470 drivers will work with the i2c driver that's
registered. Clock and regulator drivers are currently in the works.

The existing mfd/tps68470.c driver being thus superseded, it is removed.

This has been tested on a number of devices; but currently **not** on a
ChromeOS, which we ideally need to do to ensure no regression caused by
replacing the tps68470 MFD driver.

Thanks
Dan

Daniel Scally (7):
  acpi: utils: move acpi_lpss_dep() to utils
  acpi: utils: Add function to fetch dependent acpi_devices
  i2c: i2c-core-base: Use format macro in i2c_dev_set_name()
  i2c: i2c-core-acpi: Add i2c_acpi_dev_name()
  gpio: gpiolib-acpi: Export acpi_get_gpiod()
  platform: x86: Add intel_skl_int3472 driver
  mfd: Remove tps68470 MFD driver

 MAINTAINERS                                   |   5 +
 drivers/acpi/acpi_lpss.c                      |  24 -
 drivers/acpi/internal.h                       |   1 +
 drivers/acpi/pmic/Kconfig                     |   1 -
 drivers/acpi/utils.c                          |  58 ++
 drivers/gpio/Kconfig                          |   1 -
 drivers/gpio/gpiolib-acpi.c                   |   3 +-
 drivers/i2c/i2c-core-acpi.c                   |  16 +
 drivers/i2c/i2c-core-base.c                   |   4 +-
 drivers/mfd/Kconfig                           |  18 -
 drivers/mfd/Makefile                          |   1 -
 drivers/mfd/tps68470.c                        |  97 ----
 drivers/platform/x86/Kconfig                  |  25 +
 drivers/platform/x86/Makefile                 |   4 +
 .../platform/x86/intel_skl_int3472_common.c   | 100 ++++
 .../platform/x86/intel_skl_int3472_common.h   | 100 ++++
 .../platform/x86/intel_skl_int3472_discrete.c | 496 ++++++++++++++++++
 .../platform/x86/intel_skl_int3472_tps68470.c | 145 +++++
 include/acpi/acpi_bus.h                       |   2 +
 include/linux/acpi.h                          |   5 +
 include/linux/i2c.h                           |   8 +
 21 files changed, 969 insertions(+), 145 deletions(-)
 delete mode 100644 drivers/mfd/tps68470.c
 create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c
 create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h
 create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c
 create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c

-- 
2.25.1


^ permalink raw reply	[flat|nested] 107+ messages in thread
* [Devel] Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices
  2021-02-02  9:58                       ` Daniel Scally
@ 2021-02-02 14:02 ` Rafael J. Wysocki
  0 siblings, 0 replies; 107+ messages in thread
From: Rafael J. Wysocki @ 2021-02-02 14:02 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 6871 bytes --]

On Tue, Feb 2, 2021 at 10:58 AM Daniel Scally <djrscally(a)gmail.com> wrote:
>
> Hi Rafael
>
> On 21/01/2021 21:06, Daniel Scally wrote:
> >
> > On 21/01/2021 18:08, Rafael J. Wysocki wrote:
> >> On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally <djrscally(a)gmail.com> wrote:
> >>>
> >>> On 21/01/2021 14:39, Rafael J. Wysocki wrote:
> >>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally(a)gmail.com> wrote:
> >>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote:
> >>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally(a)gmail.com> wrote:
> >>>>>>> Hi Rafael
> >>>>>>>
> >>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote:
> >>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally(a)gmail.com> wrote:
> >>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote:
> >>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally(a)gmail.com> wrote:
> >>>>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert
> >>>>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the
> >>>>>>>>>>> specification intends. We need to be able to find those devices "from"
> >>>>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check if
> >>>>>>>>>>> the include the handle of the dependee device in their _DEP buffer.
> >>>>>>>>>> What exactly do you need this for?
> >>>>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which
> >>>>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the
> >>>>>>>>> INT3472 device, we need to find the sensors dependent on them.
> >>>>>>>>>
> >>>>>>>> Well, this is an interesting concept. :-)
> >>>>>>>>
> >>>>>>>> Why does _DEP need to be used for that?  Isn't there any other way to
> >>>>>>>> look up the dependent sensors?
> >>>>>>>>
> >>>>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead?
> >>>>>>>>>>
> >>>>>>>>>> Note that supplier drivers may remove entries from there, but does
> >>>>>>>>>> that matter for your use case?
> >>>>>>>>> Ah - that may work, yes. Thank you, let me test that.
> >>>>>>>> Even if that doesn't work right away, but it can be made work, I would
> >>>>>>>> very much prefer that to the driver parsing _DEP for every device in
> >>>>>>>> the namespace by itself.
> >>>>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case
> >>>>>>> with acpi_dep_list declared as external var in internal.h)?
> >>>>>> Let's put it in scan.c for now, because there is the lock protecting
> >>>>>> the list in there too.
> >>>>>>
> >>>>>> How do you want to implement this?  Something like "walk the list and
> >>>>>> run a callback for the matching entries" or do you have something else
> >>>>>> in mind?
> >>>>> Something like this (though with a mutex_lock()). It could be simplified
> >>>>> by dropping the prev stuff, but we have seen INT3472 devices with
> >>>>> multiple sensors declaring themselves dependent on the same device
> >>>>>
> >>>>>
> >>>>> struct acpi_device *
> >>>>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier,
> >>>>>                 struct acpi_device *prev)
> >>>>> {
> >>>>>     struct acpi_dep_data *dep;
> >>>>>     struct acpi_device *adev;
> >>>>>     int ret;
> >>>>>
> >>>>>     if (!supplier)
> >>>>>         return ERR_PTR(-EINVAL);
> >>>>>
> >>>>>     if (prev) {
> >>>>>         /*
> >>>>>          * We need to find the previous device in the list, so we know
> >>>>>          * where to start iterating from.
> >>>>>          */
> >>>>>         list_for_each_entry(dep, &acpi_dep_list, node)
> >>>>>             if (dep->consumer == prev->handle &&
> >>>>>                 dep->supplier == supplier->handle)
> >>>>>                 break;
> >>>>>
> >>>>>         dep = list_next_entry(dep, node);
> >>>>>     } else {
> >>>>>         dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data,
> >>>>>                        node);
> >>>>>     }
> >>>>>
> >>>>>
> >>>>>     list_for_each_entry_from(dep, &acpi_dep_list, node) {
> >>>>>         if (dep->supplier == supplier->handle) {
> >>>>>             ret = acpi_bus_get_device(dep->consumer, &adev);
> >>>>>             if (ret)
> >>>>>                 return ERR_PTR(ret);
> >>>>>
> >>>>>             return adev;
> >>>>>         }
> >>>>>     }
> >>>>>
> >>>>>     return NULL;
> >>>>> }
> >>>> That would work I think, but would it be practical to modify
> >>>> acpi_walk_dep_device_list() so that it runs a callback for every
> >>>> consumer found instead of or in addition to the "delete from the list
> >>>> and free the entry" operation?
> >>>
> >>> I think that this would work fine, if that's the way you want to go.
> >>> We'd just need to move everything inside the if (dep->supplier ==
> >>> handle) block to a new callback, and for my purposes I think also add a
> >>> way to stop parsing the list from the callback (so like have the
> >>> callbacks return int and stop parsing on a non-zero return). Do you want
> >>> to expose that ability to pass a callback outside of ACPI?
> >> Yes.
> >>
> >>> Or just export helpers to call each of the callbacks (one to fetch the next
> >>> dependent device, one to decrement the unmet dependencies counter)
> >> If you can run a callback for every matching entry, you don't really
> >> need to have a callback to return the next matching entry.  You can do
> >> stuff for all of them in one go
> >
> > Well it my case it's more to return a pointer to the dep->consumer's
> > acpi_device for a matching entry, so my idea was where there's multiple
> > dependents you could use this as an iterator...but it could just be
> > extended to that if needed later; I don't actually need to do it right now.
> >
> >
> >> note that it probably is not a good
> >> idea to run the callback under the lock, so the for loop currently in
> >> there is not really suitable for that
> >
> > No problem;  I'll tweak that then
>
> Slightly walking back my "No problem" here; as I understand this there's
> kinda two options:
>
> 1. Walk over the (locked) list, when a match is found unlock, run the
> callback and re-lock.

That's what I was thinking about.

> The problem with that idea is unless I'm mistaken there's no guarantee
> that the .next pointer is still valid then (even using the *_safe()
> methods) because either the next or the next + 1 entry could have been
> removed whilst the list was unlocked and the callback was being ran, so
> this seems a little unsafe.

This can be addressed by rotating the list while walking it, but that
becomes problematic if there are concurrent walkers.

OK, I guess running the callback under the lock is not really a big
deal (and for the deletion case this is actually necessary), so let's
do that.

^ permalink raw reply	[flat|nested] 107+ messages in thread
* [Devel] Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver
  2021-02-07 11:00                         ` Daniel Scally
@ 2021-02-07 11:56 ` Andy Shevchenko
  -1 siblings, 0 replies; 107+ messages in thread
From: Andy Shevchenko @ 2021-02-07 11:56 UTC (permalink / raw)
  To: devel

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

On Sun, Feb 7, 2021 at 1:00 PM Daniel Scally <djrscally(a)gmail.com> wrote:
> On 21/01/2021 00:18, Daniel Scally wrote:
> > On 20/01/2021 12:57, Andy Shevchenko wrote:

...

> > I'm not sure that one's better than the other, to be honest. Either the
> > multi-function device functionality lives in the conventional place, or
> > else _all_ of the int3472 handling code lives together in one module.
> Can we come to a consensus on this? I would rather be in agreement than
> leave it hanging...I do see the argument that it's better not to rebirth
> the MFD driver away from that subsystem, but at the moment I lean
> towards the view that having all the code handling this particular _HID
> in one place is probably preferable, if only to make it easier for
> anyone coming in the future to understand what's happening. That said;
> I'm not particularly precious about it, I'd just like to agree an
> approach so I can move forward with the next version

So, my priorities of rejection (first is higher)
- open-coding MFD subsystem (that said, if you shuffle the code, at
least leave it as an MFD driver)
- moving out from MFD (although I understand intentions)

Summarize, go ahead with your view (leaving it as an MFD driver) and
look forward to what maintainer(s) will say.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 107+ messages in thread

end of thread, other threads:[~2021-02-07 11:57 UTC | newest]

Thread overview: 107+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-18  0:34 [PATCH v2 0/7] Introduce intel_skl_int3472 driver Daniel Scally
2021-01-18  0:34 ` [PATCH v2 1/7] acpi: utils: move acpi_lpss_dep() to utils Daniel Scally
2021-01-18  7:24   ` Laurent Pinchart
2021-01-18  8:31     ` Daniel Scally
2021-01-18 12:29     ` Andy Shevchenko
2021-01-18 12:35       ` Daniel Scally
2021-01-18 12:28   ` Andy Shevchenko
2021-01-18 16:06     ` [Devel] " Rafael J. Wysocki
2021-01-18 16:06       ` Rafael J. Wysocki
2021-01-18 16:42       ` Andy Shevchenko
2021-01-18  0:34 ` [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices Daniel Scally
2021-01-18  7:34   ` Laurent Pinchart
2021-01-18  8:37     ` Daniel Scally
2021-01-18 12:33   ` Andy Shevchenko
2021-01-18 13:37     ` Daniel Scally
2021-01-18 16:14   ` [Devel] " Rafael J. Wysocki
2021-01-18 16:14     ` Rafael J. Wysocki
2021-01-18 20:51     ` Daniel Scally
2021-01-19 13:15       ` [Devel] " Rafael J. Wysocki
2021-01-19 13:15         ` Rafael J. Wysocki
2021-01-19 13:28         ` Daniel Scally
2021-01-21  9:47         ` Daniel Scally
2021-01-21 11:58           ` [Devel] " Rafael J. Wysocki
2021-01-21 11:58             ` Rafael J. Wysocki
2021-01-21 12:04             ` Daniel Scally
2021-01-21 14:39               ` [Devel] " Rafael J. Wysocki
2021-01-21 14:39                 ` Rafael J. Wysocki
2021-01-21 16:34                 ` Daniel Scally
2021-01-21 18:08                   ` [Devel] " Rafael J. Wysocki
2021-01-21 18:08                     ` Rafael J. Wysocki
2021-01-21 21:06                     ` Daniel Scally
2021-02-02  9:58                       ` Daniel Scally
2021-02-02 11:27                         ` Andy Shevchenko
2021-01-18  0:34 ` [PATCH v2 3/7] i2c: i2c-core-base: Use format macro in i2c_dev_set_name() Daniel Scally
2021-01-18  7:28   ` Laurent Pinchart
2021-01-18 12:41     ` Andy Shevchenko
2021-01-18  9:41   ` Sakari Ailus
2021-01-18  9:42     ` Sakari Ailus
2021-01-18  9:48     ` Wolfram Sang
2021-01-18 12:39   ` Andy Shevchenko
2021-01-18  0:34 ` [PATCH v2 4/7] i2c: i2c-core-acpi: Add i2c_acpi_dev_name() Daniel Scally
2021-01-18  9:18   ` Laurent Pinchart
2021-01-18 13:41     ` Andy Shevchenko
2021-01-19 13:19     ` [Devel] " Rafael J. Wysocki
2021-01-19 13:19       ` Rafael J. Wysocki
2021-01-28  9:00       ` Wolfram Sang
2021-01-28  9:15         ` Daniel Scally
2021-01-28  9:17           ` Wolfram Sang
2021-01-28  9:22             ` Daniel Scally
2021-01-18 13:39   ` Andy Shevchenko
2021-01-18 18:43     ` Joe Perches
2021-01-18 18:56       ` Andy Shevchenko
2021-01-18 19:00         ` Joe Perches
2021-01-18 19:01         ` Andy Shevchenko
2021-01-18  0:34 ` [PATCH v2 5/7] gpio: gpiolib-acpi: Export acpi_get_gpiod() Daniel Scally
2021-01-18  7:37   ` Laurent Pinchart
2021-01-18 13:45   ` Andy Shevchenko
2021-01-18 13:46     ` Andy Shevchenko
2021-01-18 21:32     ` Daniel Scally
2021-01-18  0:34 ` [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver Daniel Scally
2021-01-18  9:15   ` Laurent Pinchart
2021-01-18 14:46     ` Andy Shevchenko
2021-01-18 21:19       ` Daniel Scally
2021-01-19  0:11         ` Daniel Scally
2021-01-19  6:21           ` Laurent Pinchart
2021-01-19  9:35             ` Andy Shevchenko
2021-01-19 16:49               ` Laurent Pinchart
2021-01-19  9:33           ` Andy Shevchenko
2021-01-19  9:34             ` Daniel Scally
2021-01-19 16:36             ` Laurent Pinchart
2021-01-19 17:43               ` Andy Shevchenko
2021-01-20  4:18                 ` Laurent Pinchart
2021-01-20 11:44                   ` Andy Shevchenko
2021-01-21 21:08                     ` Daniel Scally
2021-01-19  9:24         ` Andy Shevchenko
2021-01-19 10:40           ` Daniel Scally
2021-01-19 11:08             ` Andy Shevchenko
2021-01-19 16:48               ` Laurent Pinchart
2021-01-19 17:51                 ` Andy Shevchenko
2021-01-20  4:21                   ` Laurent Pinchart
2021-01-20 12:57                     ` Andy Shevchenko
2021-01-21  0:18                       ` Daniel Scally
2021-02-07 11:00                         ` Daniel Scally
2021-01-18 20:46     ` Daniel Scally
2021-01-19  6:19       ` Laurent Pinchart
2021-01-19  8:43         ` Daniel Scally
2021-01-19 16:33           ` Laurent Pinchart
2021-01-18 11:12   ` Barnabás Pőcze
2021-01-18 13:51     ` andriy.shevchenko
2021-01-18 14:51       ` Barnabás Pőcze
2021-01-18 15:23         ` andriy.shevchenko
2021-01-18 15:32           ` Hans de Goede
2021-01-18 15:48             ` andriy.shevchenko
2021-01-18 16:00               ` Daniel Scally
2021-01-18 16:03                 ` Hans de Goede
2021-01-18 17:05             ` Laurent Pinchart
2021-01-19 10:56   ` Kieran Bingham
2021-01-19 11:11     ` Andy Shevchenko
2021-01-19 11:12       ` Daniel Scally
2021-01-18  0:34 ` [PATCH v2 7/7] mfd: Remove tps68470 MFD driver Daniel Scally
2021-01-18  7:42   ` Laurent Pinchart
2021-01-18 13:53   ` Andy Shevchenko
2021-01-18 20:07     ` Joe Perches
  -- strict thread matches above, loose matches on Subject: below --
2021-02-02 14:02 [Devel] Re: [PATCH v2 2/7] acpi: utils: Add function to fetch dependent acpi_devices Rafael J. Wysocki
2021-02-02 14:02 ` Rafael J. Wysocki
2021-02-07 11:56 [Devel] Re: [PATCH v2 6/7] platform: x86: Add intel_skl_int3472 driver Andy Shevchenko
2021-02-07 11:56 ` Andy Shevchenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.