* [PATCH v3 0/6] Introduce intel_skl_int3472 module
@ 2021-02-22 13:07 Daniel Scally
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
` (8 more replies)
0 siblings, 9 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
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
v2 was here:
https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrscally@gmail.com/
Series level changelog:
- Dropped the patch moving acpi_lpss_dep() to utils since it's not used
in acpi_dev_get_dependent_dev() anymore.
- Replaced it with a patch extending acpi_walk_dep_device_list() to be
able to apply a given callback against each device in acpi_dep_list
- Dropped the patch creating acpi_i2c_dev_name() and simply open coded
that functionality.
This has been tested on a number of devices, but currently **not** on a device
designed for ChromeOS, which we ideally need to do to ensure no regression
caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way
you could help with that? Unfortunately, I don't have a device to test it on
myself.
Original cover letter:
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 available but have not so far been
tested, so aren't part of this series.
The existing mfd/tps68470.c driver being thus superseded, it is removed.
Thanks
Dan
Daniel Scally (6):
ACPI: scan: Extend acpi_walk_dep_device_list()
ACPI: scan: Add function to fetch dependent of acpi device
i2c: core: Add a format macro for I2C device names
gpiolib: acpi: Export acpi_get_gpiod()
platform/x86: Add intel_skl_int3472 driver
mfd: tps68470: Remove tps68470 MFD driver
MAINTAINERS | 5 +
drivers/acpi/ec.c | 2 +-
drivers/acpi/pmic/Kconfig | 2 +-
drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
drivers/acpi/scan.c | 92 ++-
drivers/gpio/Kconfig | 2 +-
drivers/gpio/gpiolib-acpi.c | 38 +-
drivers/i2c/i2c-core-acpi.c | 2 +-
drivers/i2c/i2c-core-base.c | 4 +-
drivers/mfd/Kconfig | 18 -
drivers/mfd/Makefile | 1 -
drivers/mfd/tps68470.c | 97 ---
drivers/platform/surface/surface3_power.c | 2 +-
drivers/platform/x86/Kconfig | 2 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel-int3472/Kconfig | 31 +
drivers/platform/x86/intel-int3472/Makefile | 4 +
.../intel-int3472/intel_skl_int3472_common.c | 106 ++++
.../intel-int3472/intel_skl_int3472_common.h | 110 ++++
.../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
.../intel_skl_int3472_tps68470.c | 113 ++++
include/acpi/acpi_bus.h | 8 +
include/linux/acpi.h | 4 +-
include/linux/gpio/consumer.h | 7 +
include/linux/i2c.h | 3 +
25 files changed, 1100 insertions(+), 148 deletions(-)
delete mode 100644 drivers/mfd/tps68470.c
create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
create mode 100644 drivers/platform/x86/intel-int3472/Makefile
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
--
2.25.1
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
@ 2021-02-22 13:07 ` Daniel Scally
2021-02-22 13:34 ` Andy Shevchenko
` (2 more replies)
2021-02-22 13:07 ` [PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device Daniel Scally
` (7 subsequent siblings)
8 siblings, 3 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel, Rafael J . Wysocki
The acpi_walk_dep_device_list() is not as generalisable as its name
implies, serving only to decrement the dependency count for each
dependent device of the input. Extend the function to instead accept
a callback which can be applied to all the dependencies in acpi_dep_list.
Replace all existing calls to the function with calls to a wrapper, passing
a callback that applies the same dependency reduction.
Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:
- patch introduced
drivers/acpi/ec.c | 2 +-
drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
drivers/acpi/scan.c | 58 ++++++++++++++++-------
drivers/gpio/gpiolib-acpi.c | 2 +-
drivers/i2c/i2c-core-acpi.c | 2 +-
drivers/platform/surface/surface3_power.c | 2 +-
include/acpi/acpi_bus.h | 7 +++
include/linux/acpi.h | 4 +-
8 files changed, 55 insertions(+), 24 deletions(-)
diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index 13565629ce0a..a258db713bd2 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -1627,7 +1627,7 @@ static int acpi_ec_add(struct acpi_device *device)
WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
/* Reprobe devices depending on the EC */
- acpi_walk_dep_device_list(ec->handle);
+ acpi_dev_flag_dependency_met(ec->handle);
acpi_handle_debug(ec->handle, "enumerated.\n");
return 0;
diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
index a5101b07611a..59cca504325e 100644
--- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
+++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
@@ -117,7 +117,7 @@ static int chtdc_ti_pmic_opregion_probe(struct platform_device *pdev)
return err;
/* Re-enumerate devices depending on PMIC */
- acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent));
+ acpi_dev_flag_dependency_met(ACPI_HANDLE(pdev->dev.parent));
return 0;
}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 80b668c80073..c9e4190316ef 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -49,12 +49,6 @@ static DEFINE_MUTEX(acpi_hp_context_lock);
*/
static u64 spcr_uart_addr;
-struct acpi_dep_data {
- struct list_head node;
- acpi_handle supplier;
- acpi_handle consumer;
-};
-
void acpi_scan_lock_acquire(void)
{
mutex_lock(&acpi_scan_lock);
@@ -2099,30 +2093,58 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
device->handler->hotplug.notify_online(device);
}
-void acpi_walk_dep_device_list(acpi_handle handle)
+static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
+ void *data)
{
- struct acpi_dep_data *dep, *tmp;
struct acpi_device *adev;
+ acpi_bus_get_device(dep->consumer, &adev);
+ if (!adev)
+ return 0;
+
+ adev->dep_unmet--;
+ if (!adev->dep_unmet)
+ acpi_bus_attach(adev, true);
+
+ list_del(&dep->node);
+ kfree(dep);
+ return 0;
+}
+
+void acpi_walk_dep_device_list(acpi_handle handle,
+ int (*callback)(struct acpi_dep_data *, void *),
+ void *data)
+{
+ struct acpi_dep_data *dep, *tmp;
+ int ret;
+
mutex_lock(&acpi_dep_list_lock);
list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
if (dep->supplier == handle) {
- acpi_bus_get_device(dep->consumer, &adev);
- if (!adev)
- continue;
-
- adev->dep_unmet--;
- if (!adev->dep_unmet)
- acpi_bus_attach(adev, true);
-
- list_del(&dep->node);
- kfree(dep);
+ ret = callback(dep, data);
+ if (ret)
+ break;
}
}
mutex_unlock(&acpi_dep_list_lock);
}
EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
+/**
+ * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device
+ * is now active
+ * @handle: acpi_handle for the supplier device
+ *
+ * This function walks through the dependencies list and informs each consumer
+ * of @handle that their dependency upon it is now met. Devices with no more
+ * unmet dependencies will be attached to the acpi bus.
+ */
+void acpi_dev_flag_dependency_met(acpi_handle handle)
+{
+ acpi_walk_dep_device_list(handle, __acpi_dev_flag_dependency_met, NULL);
+}
+EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
+
/**
* acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
* @handle: Root of the namespace scope to scan.
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e37a57d0a2f0..e4d728fda982 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -1254,7 +1254,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
acpi_gpiochip_request_regions(acpi_gpio);
acpi_gpiochip_scan_gpios(acpi_gpio);
- acpi_walk_dep_device_list(handle);
+ acpi_dev_flag_dependency_met(handle);
}
void acpi_gpiochip_remove(struct gpio_chip *chip)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 37c510d9347a..38647cf34bde 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -283,7 +283,7 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap)
if (!handle)
return;
- acpi_walk_dep_device_list(handle);
+ acpi_dev_flag_dependency_met(handle);
}
static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
index cc4f9cba6856..ad895285d3e9 100644
--- a/drivers/platform/surface/surface3_power.c
+++ b/drivers/platform/surface/surface3_power.c
@@ -478,7 +478,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
return -ENOMEM;
}
- acpi_walk_dep_device_list(handle);
+ acpi_dev_flag_dependency_met(handle);
return 0;
}
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 02a716a0af5d..91172af3a04d 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -278,6 +278,12 @@ struct acpi_device_power {
struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */
};
+struct acpi_dep_data {
+ struct list_head node;
+ acpi_handle supplier;
+ acpi_handle consumer;
+};
+
/* Performance Management */
struct acpi_device_perf_flags {
@@ -683,6 +689,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
+void acpi_dev_flag_dependency_met(acpi_handle handle);
struct acpi_device *
acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
struct acpi_device *
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 2630c2e953f7..2d5e6e88e8a0 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -655,7 +655,9 @@ extern bool acpi_driver_match_device(struct device *dev,
const struct device_driver *drv);
int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
int acpi_device_modalias(struct device *, char *, int);
-void acpi_walk_dep_device_list(acpi_handle handle);
+void acpi_walk_dep_device_list(acpi_handle handle,
+ int (*callback)(struct acpi_dep_data *, void *),
+ void *data);
struct platform_device *acpi_create_platform_device(struct acpi_device *,
struct property_entry *);
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
@ 2021-02-22 13:07 ` Daniel Scally
2021-02-22 13:41 ` Andy Shevchenko
2021-02-22 13:07 ` [PATCH v3 3/6] i2c: core: Add a format macro for I2C device names Daniel Scally
` (6 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
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 callback and a wrapper to walk over the
acpi_dep_list and return the dependent ACPI device.
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:
- Switched from a standalone function to a callback passed to
acpi_walk_dep_device_list().
drivers/acpi/scan.c | 34 ++++++++++++++++++++++++++++++++++
include/acpi/acpi_bus.h | 1 +
2 files changed, 35 insertions(+)
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index c9e4190316ef..55626925261c 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2093,6 +2093,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
device->handler->hotplug.notify_online(device);
}
+static int __acpi_dev_get_dependent_dev(struct acpi_dep_data *dep, void *data)
+{
+ struct acpi_device *adev;
+ int ret;
+
+ ret = acpi_bus_get_device(dep->consumer, &adev);
+ if (ret)
+ /* If we don't find an adev then we want to continue parsing */
+ return 0;
+
+ *(struct acpi_device **)data = adev;
+
+ return 1;
+}
+
static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
void *data)
{
@@ -2145,6 +2160,25 @@ void acpi_dev_flag_dependency_met(acpi_handle handle)
}
EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
+/**
+ * acpi_dev_get_dependent_dev - Return ACPI device dependent on @adev
+ * @adev: Pointer to the dependee device
+ *
+ * Returns the first &struct acpi_device which declares itself dependent on
+ * @adev via the _DEP buffer, parsed from the acpi_dep_list.
+ */
+struct acpi_device *
+acpi_dev_get_dependent_dev(struct acpi_device *supplier)
+{
+ struct acpi_device *adev = NULL;
+
+ acpi_walk_dep_device_list(supplier->handle,
+ __acpi_dev_get_dependent_dev, &adev);
+
+ return adev;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_get_dependent_dev);
+
/**
* acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
* @handle: Root of the namespace scope to scan.
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 91172af3a04d..5b14a9ae4ed5 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -690,6 +690,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
void acpi_dev_flag_dependency_met(acpi_handle handle);
+struct acpi_device *acpi_dev_get_dependent_dev(struct acpi_device *supplier);
struct acpi_device *
acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
struct acpi_device *
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 3/6] i2c: core: Add a format macro for I2C device names
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
2021-02-22 13:07 ` [PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device Daniel Scally
@ 2021-02-22 13:07 ` Daniel Scally
2021-02-22 13:38 ` Wolfram Sang
2021-02-22 13:07 ` [PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod() Daniel Scally
` (5 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel, Andy Shevchenko
Some places in the kernel allow users to map resources to a device
using device name (for example, in the struct gpiod_lookup_table).
Currently this involves waiting for the I2C client to have been registered
so we can use dev_name(&client->dev). We want to add a function to allow
users to refer to an I2C device by name before it has been instantiated,
so create a macro for the format that's accessible outside the I2C layer
and use it in i2c_dev_set_name().
Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:
- None
drivers/i2c/i2c-core-base.c | 4 ++--
include/linux/i2c.h | 3 +++
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf722a424..547b8926cac8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -811,12 +811,12 @@ static void i2c_dev_set_name(struct i2c_adapter *adap,
struct acpi_device *adev = ACPI_COMPANION(&client->dev);
if (info && info->dev_name) {
- dev_set_name(&client->dev, "i2c-%s", info->dev_name);
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name);
return;
}
if (adev) {
- dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev));
+ dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev));
return;
}
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 56622658b215..4d40a4b46810 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -39,6 +39,9 @@ enum i2c_slave_event;
typedef int (*i2c_slave_cb_t)(struct i2c_client *client,
enum i2c_slave_event event, u8 *val);
+/* I2C Device Name Format - to maintain consistency outside the i2c layer */
+#define I2C_DEV_NAME_FORMAT "i2c-%s"
+
/* I2C Frequency Modes */
#define I2C_MAX_STANDARD_MODE_FREQ 100000
#define I2C_MAX_FAST_MODE_FREQ 400000
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod()
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (2 preceding siblings ...)
2021-02-22 13:07 ` [PATCH v3 3/6] i2c: core: Add a format macro for I2C device names Daniel Scally
@ 2021-02-22 13:07 ` Daniel Scally
2021-02-22 13:54 ` Andy Shevchenko
2021-02-22 13:07 ` [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver Daniel Scally
` (4 subsequent siblings)
8 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
I need to be able to translate GPIO resources in an ACPI device's _CRS
into GPIO descriptor array. Those are represented in _CRS as a pathname
to a GPIO device plus the pin's index number: this function is perfect
for that purpose.
As it's currently only used internally within the GPIO layer, provide and
export a wrapper function that additionally holds a reference to the GPIO
device.
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:
- Having realised that it wasn't taking a reference to the GPIO device,
I decided the best thing to do was leave the existing function as-is
(apart from renaming) and provide a wrapper that simply passes
through to the original and takes a reference before returning the
struct gpio_desc
Because of the change to that functionality, I dropped the R-b's from
the last version.
drivers/gpio/gpiolib-acpi.c | 36 +++++++++++++++++++++++++++++++----
include/linux/gpio/consumer.h | 7 +++++++
2 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e4d728fda982..0cc7cc327757 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -102,7 +102,8 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
}
/**
- * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
+ * __acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
+ * GPIO API
* @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
* @pin: ACPI GPIO pin number (0-based, controller-relative)
*
@@ -111,7 +112,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
* controller does not have GPIO chip registered at the moment. This is to
* support probe deferral.
*/
-static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+static struct gpio_desc *__acpi_get_gpiod(char *path, int pin)
{
struct gpio_chip *chip;
acpi_handle handle;
@@ -128,6 +129,33 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
return gpiochip_get_desc(chip, pin);
}
+/**
+ * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
+ * GPIO API, and hold a refcount to the GPIO device.
+ * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
+ * @pin: ACPI GPIO pin number (0-based, controller-relative)
+ * @label: Label to pass to gpiod_request()
+ *
+ * This function is a simple pass-through to __acpi_get_gpiod(), except that as
+ * it is intended for use outside of the GPIO layer (in a similar fashion to
+ * gpiod_get_index() for example) it also holds a reference to the GPIO device.
+ */
+struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label)
+{
+ struct gpio_desc *gpio = __acpi_get_gpiod(path, pin);
+ int ret;
+
+ if (IS_ERR(gpio))
+ return gpio;
+
+ ret = gpiod_request(gpio, label);
+ if (ret)
+ return ERR_PTR(ret);
+
+ return gpio;
+}
+EXPORT_SYMBOL_GPL(acpi_get_gpiod);
+
static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
{
struct acpi_gpio_event *event = data;
@@ -689,8 +717,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
if (pin_index >= agpio->pin_table_length)
return 1;
- lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
- agpio->pin_table[pin_index]);
+ lookup->desc = __acpi_get_gpiod(agpio->resource_source.string_ptr,
+ agpio->pin_table[pin_index]);
lookup->info.pin_config = agpio->pin_config;
lookup->info.debounce = agpio->debounce_timeout;
lookup->info.gpioint = gpioint;
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index ef49307611d2..6eee751f44dd 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -690,6 +690,8 @@ int devm_acpi_dev_add_driver_gpios(struct device *dev,
const struct acpi_gpio_mapping *gpios);
void devm_acpi_dev_remove_driver_gpios(struct device *dev);
+struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label);
+
#else /* CONFIG_GPIOLIB && CONFIG_ACPI */
struct acpi_device;
@@ -708,6 +710,11 @@ static inline int devm_acpi_dev_add_driver_gpios(struct device *dev,
}
static inline void devm_acpi_dev_remove_driver_gpios(struct device *dev) {}
+struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label)
+{
+ return NULL;
+}
+
#endif /* CONFIG_GPIOLIB && CONFIG_ACPI */
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (3 preceding siblings ...)
2021-02-22 13:07 ` [PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod() Daniel Scally
@ 2021-02-22 13:07 ` Daniel Scally
2021-02-22 13:19 ` Daniel Scally
` (2 more replies)
2021-02-22 13:07 ` [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver Daniel Scally
` (3 subsequent siblings)
8 siblings, 3 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
ACPI devices with _HID INT3472 are currently matched to the tps68470
driver, however this does not cover all situations in which that _HID
occurs. We've encountered three possibilities:
1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
a physical TPS68470 device) that requires a GPIO and OpRegion driver
2. On devices designed for Windows, an ACPI device with _HID INT3472
(again representing a physical TPS68470 device) which requires GPIO,
Clock and Regulator drivers.
3. On other devices designed for Windows, an ACPI device with _HID
INT3472 which does **not** represent a physical TPS68470, and is instead
used as a dummy device to group some system GPIO lines which are meant
to be consumed by the sensor that is dependent on this entry.
This commit adds a new module, registering a platform driver to deal
with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
querying the CLDB buffer found against INT3472 entries to determine
which is most appropriate.
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:
- Added clk_recalc_rate() operation to the clk that's registered, plus
some associated functions / revisions.
- Moved source and header files to their own folder within platform/x86
- Switched the GPIO toggling for the clock from enable/disable to
prepare/unprepare to avoid any sleep problems
- Switched handling of the privacy LED GPIO from map-to-sensor to being
toggled along with the clk enable GPIO; register the clock in
intel_skl_int3472_parse_crs() instead of during handle_gpio_resources
- Better commenting in a lot of places
- Used the sensor_name formed from acpi_dev_name(int3472->sensor) and
the i2c name format macro as the dev_id in regulator init data rather
than hardcoding an instance name.
- Fetched the sensor module config a single time rather than once per
GPIO
- Switched int3472-tps68470 driver to use MFD framework properly rather
than open coding the same functionality
- A myriad of other fixes too minor to call out.
Suggested changes (from Andy) that I didn't follow:
- Using clk-gpio.c as a library: The requirement to add clk_recalc_rate
so that clk_get_rate() would be supported, along with driving the
privacy LED on clk_enable() meant this wouldn't work I think
- Leave the MFD driver in its usual place, and prevent the INT3472 from
probing as an i2c device from ACPI. I definitely see the argument for
this, but in the end I think probably having all the code for the HID
within a single place is probably a bit preferable.
MAINTAINERS | 5 +
drivers/platform/x86/Kconfig | 2 +
drivers/platform/x86/Makefile | 1 +
drivers/platform/x86/intel-int3472/Kconfig | 31 +
drivers/platform/x86/intel-int3472/Makefile | 4 +
.../intel-int3472/intel_skl_int3472_common.c | 106 ++++
.../intel-int3472/intel_skl_int3472_common.h | 110 ++++
.../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
.../intel_skl_int3472_tps68470.c | 113 ++++
9 files changed, 964 insertions(+)
create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
create mode 100644 drivers/platform/x86/intel-int3472/Makefile
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a091b496fdd8..cf44b3e77b90 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9147,6 +9147,11 @@ S: Maintained
F: arch/x86/include/asm/intel_scu_ipc.h
F: drivers/platform/x86/intel_scu_*
+INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER
+M: Daniel Scally <djrscally@gmail.com>
+S: Maintained
+F: drivers/platform/x86/intel-int3472/intel_skl_int3472_*
+
INTEL SPEED SELECT TECHNOLOGY
M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
L: platform-driver-x86@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 91e6176cdfbd..9739d30951b6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -844,6 +844,8 @@ config INTEL_CHT_INT33FE
device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m
for Type-C device.
+source "drivers/platform/x86/intel-int3472/Kconfig"
+
config INTEL_HID_EVENT
tristate "INTEL HID Event"
depends on ACPI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 581475f59819..2293b6c3d1c2 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o
obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o
obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o
obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o
+obj-$(CONFIG_INTEL_SKL_INT3472) += intel-int3472/
obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o
# MSI
diff --git a/drivers/platform/x86/intel-int3472/Kconfig b/drivers/platform/x86/intel-int3472/Kconfig
new file mode 100644
index 000000000000..b94622245c21
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/Kconfig
@@ -0,0 +1,31 @@
+config INTEL_SKL_INT3472
+ tristate "Intel SkyLake ACPI INT3472 Driver"
+ depends on ACPI
+ depends on REGULATOR
+ depends on GPIOLIB
+ depends on COMMON_CLK && CLKDEV_LOOKUP
+ depends on I2C
+ select MFD_CORE
+ select REGMAP_I2C
+ help
+ This driver adds support for the INT3472 ACPI devices found on some
+ Intel SkyLake devices.
+
+ The INT3472 is an Intel camera power controller, a logical device
+ found on some Skylake-based systems that can map to different
+ hardware devices depending on the platform. On machines
+ designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
+ machines designed for Windows, it maps to either a TP68470
+ camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
+ and power gates.
+
+ If your device was designed for Chrome OS, this driver will provide
+ an ACPI OpRegion, which must be available before any of the devices
+ using it are probed. For this reason, you should select Y if your
+ device was designed for ChromeOS. For the same reason the
+ I2C_DESIGNWARE_PLATFORM option must be set to Y too.
+
+ Say Y or M here if you have a SkyLake device designed for use
+ with Windows or ChromeOS. Say N here if you are not sure.
+
+ The module will be named "intel-skl-int3472"
diff --git a/drivers/platform/x86/intel-int3472/Makefile b/drivers/platform/x86/intel-int3472/Makefile
new file mode 100644
index 000000000000..c887ee7d52ca
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/Makefile
@@ -0,0 +1,4 @@
+obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472.o
+intel_skl_int3472-objs := intel_skl_int3472_common.o \
+ intel_skl_int3472_discrete.o \
+ intel_skl_int3472_tps68470.o
diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
new file mode 100644
index 000000000000..f61166b6c497
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/acpi.h>
+#include <linux/i2c.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "intel_skl_int3472_common.h"
+
+union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev,
+ char *id)
+{
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ acpi_handle handle = adev->handle;
+ union acpi_object *obj;
+ acpi_status status;
+
+ status = acpi_evaluate_object(handle, id, NULL, &buffer);
+ if (ACPI_FAILURE(status))
+ return ERR_PTR(-ENODEV);
+
+ obj = buffer.pointer;
+ if (!obj)
+ return ERR_PTR(-ENODEV);
+
+ if (obj->type != ACPI_TYPE_BUFFER) {
+ dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
+ kfree(obj);
+ return ERR_PTR(-EINVAL);
+ }
+
+ return obj;
+}
+
+int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
+{
+ union acpi_object *obj;
+ int ret = 0;
+
+ obj = skl_int3472_get_acpi_buffer(adev, "CLDB");
+ if (IS_ERR(obj))
+ return PTR_ERR(obj);
+
+ if (obj->buffer.length > sizeof(*cldb)) {
+ dev_err(&adev->dev, "The CLDB buffer is too large\n");
+ ret = -EINVAL;
+ goto out_free_obj;
+ }
+
+ memcpy(cldb, obj->buffer.pointer, obj->buffer.length);
+
+out_free_obj:
+ kfree(obj);
+ return ret;
+}
+
+static const struct acpi_device_id int3472_device_id[] = {
+ { "INT3472", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, int3472_device_id);
+
+static struct platform_driver int3472_discrete = {
+ .driver = {
+ .name = "int3472-discrete",
+ .acpi_match_table = int3472_device_id,
+ },
+ .probe = skl_int3472_discrete_probe,
+ .remove = skl_int3472_discrete_remove,
+};
+
+static struct i2c_driver int3472_tps68470 = {
+ .driver = {
+ .name = "int3472-tps68470",
+ .acpi_match_table = int3472_device_id,
+ },
+ .probe_new = skl_int3472_tps68470_probe,
+};
+
+static int skl_int3472_init(void)
+{
+ int ret = 0;
+
+ ret = platform_driver_register(&int3472_discrete);
+ if (ret)
+ return ret;
+
+ ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
+ if (ret)
+ platform_driver_unregister(&int3472_discrete);
+
+ return ret;
+}
+module_init(skl_int3472_init);
+
+static void skl_int3472_exit(void)
+{
+ platform_driver_unregister(&int3472_discrete);
+ i2c_del_driver(&int3472_tps68470);
+}
+module_exit(skl_int3472_exit);
+
+MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver");
+MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
new file mode 100644
index 000000000000..9169356cd522
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
@@ -0,0 +1,110 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#ifndef _INTEL_SKL_INT3472_H
+#define _INTEL_SKL_INT3472_H
+
+#include <linux/clk-provider.h>
+#include <linux/gpio/machine.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/types.h>
+
+/* PMIC GPIO Types */
+#define INT3472_GPIO_TYPE_RESET 0x00
+#define INT3472_GPIO_TYPE_POWERDOWN 0x01
+#define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b
+#define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c
+#define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d
+
+#define INT3472_PDEV_MAX_NAME_LEN 23
+#define INT3472_MAX_SENSOR_GPIOS 3
+
+#define GPIO_REGULATOR_NAME_LENGTH 21
+#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9
+
+#define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET 86
+
+#define INT3472_REGULATOR(_name, _supply, _ops) \
+ (const struct regulator_desc) { \
+ .name = _name, \
+ .supply_name = _supply, \
+ .type = REGULATOR_VOLTAGE, \
+ .ops = _ops, \
+ .owner = THIS_MODULE, \
+ }
+
+#define to_int3472_clk(hw) \
+ container_of(hw, struct int3472_gpio_clock, clk_hw)
+
+#define to_int3472_device(clk) \
+ container_of(clk, struct int3472_discrete_device, clock)
+
+struct platform_device;
+struct i2c_client;
+struct acpi_device;
+
+struct int3472_cldb {
+ u8 version;
+ /*
+ * control logic type
+ * 0: UNKNOWN
+ * 1: DISCRETE(CRD-D)
+ * 2: PMIC TPS68470
+ * 3: PMIC uP6641
+ */
+ u8 control_logic_type;
+ u8 control_logic_id;
+ u8 sensor_card_sku;
+ u8 reserved[28];
+};
+
+struct int3472_gpio_function_remap {
+ char *documented;
+ char *actual;
+};
+
+struct int3472_sensor_config {
+ const char *sensor_module_name;
+ struct regulator_consumer_supply supply_map;
+ const struct int3472_gpio_function_remap *function_maps;
+};
+
+struct int3472_discrete_device {
+ struct acpi_device *adev;
+ struct device *dev;
+ struct acpi_device *sensor;
+ const char *sensor_name;
+
+ struct int3472_sensor_config *sensor_config;
+
+ struct int3472_gpio_regulator {
+ char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
+ char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
+ struct gpio_desc *gpio;
+ struct regulator_dev *rdev;
+ struct regulator_desc rdesc;
+ } regulator;
+
+ struct int3472_gpio_clock {
+ struct clk *clk;
+ struct clk_hw clk_hw;
+ struct clk_lookup *cl;
+ struct gpio_desc *ena_gpio;
+ struct gpio_desc *led_gpio;
+ u32 frequency;
+ } clock;
+
+ unsigned int n_gpios; /* how many GPIOs have we seen */
+ unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
+ struct gpiod_lookup_table gpios;
+};
+
+int skl_int3472_discrete_probe(struct platform_device *pdev);
+int skl_int3472_discrete_remove(struct platform_device *pdev);
+int skl_int3472_tps68470_probe(struct i2c_client *client);
+union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev,
+ char *id);
+int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb);
+
+#endif
diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
new file mode 100644
index 000000000000..40652161bbbf
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
@@ -0,0 +1,592 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/acpi.h>
+#include <linux/clkdev.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/slab.h>
+
+#include "intel_skl_int3472_common.h"
+
+/*
+ * 79234640-9e10-4fea-a5c1-b5aa8b19756f
+ * This _DSM GUID returns information about the GPIO lines mapped to a
+ * discrete INT3472 device. Function number 1 returns a count of the GPIO
+ * lines that are mapped. Subsequent functions return 32 bit ints encoding
+ * information about the GPIO line, including its purpose.
+ */
+static const guid_t int3472_gpio_guid =
+ GUID_INIT(0x79234640, 0x9e10, 0x4fea,
+ 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
+
+/*
+ * 822ace8f-2814-4174-a56b-5f029fe079ee
+ * This _DSM GUID returns a string from the sensor device, which acts as a
+ * module identifier.
+ */
+static const guid_t cio2_sensor_module_guid =
+ GUID_INIT(0x822ace8f, 0x2814, 0x4174,
+ 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
+
+/*
+ * Here follows platform specific mapping information that we can pass to
+ * the functions mapping resources to the sensors. Where the sensors have
+ * a power enable pin defined in DSDT we need to provide a supply name so
+ * the sensor drivers can find the regulator. The device name will be derived
+ * from the sensor's ACPI device within the code. Optionally, we can provide a
+ * NULL terminated array of function name mappings to deal with any platform
+ * specific deviations from the documented behaviour of GPIOs.
+ *
+ * Map a GPIO function name to NULL to prevent the driver from mapping that
+ * GPIO at all.
+ */
+
+static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
+ { "reset", NULL },
+ { "powerdown", "reset" },
+ { }
+};
+
+static struct int3472_sensor_config int3472_sensor_configs[] = {
+ /* Lenovo Miix 510-12ISK - OV2680, Front */
+ { "GNDF140809R", { 0 }, ov2680_gpio_function_remaps},
+ /* Lenovo Miix 510-12ISK - OV5648, Rear */
+ { "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL},
+ /* Surface Go 1&2 - OV5693, Front */
+ { "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL},
+};
+
+/*
+ * The regulators have to have .ops to be valid, but the only ops we actually
+ * support are .enable and .disable which are handled via .ena_gpiod. Pass an
+ * empty struct to clear the check without lying about capabilities.
+ */
+static const struct regulator_ops int3472_gpio_regulator_ops = { 0 };
+
+static int skl_int3472_clk_prepare(struct clk_hw *hw)
+{
+ struct int3472_gpio_clock *clk = to_int3472_clk(hw);
+
+ gpiod_set_value(clk->ena_gpio, 1);
+ if (clk->led_gpio)
+ gpiod_set_value(clk->led_gpio, 1);
+
+ return 0;
+}
+
+static void skl_int3472_clk_unprepare(struct clk_hw *hw)
+{
+ struct int3472_gpio_clock *clk = to_int3472_clk(hw);
+
+ gpiod_set_value(clk->ena_gpio, 0);
+ if (clk->led_gpio)
+ gpiod_set_value(clk->led_gpio, 0);
+}
+
+static int skl_int3472_clk_enable(struct clk_hw *hw)
+{
+ /*
+ * We're just turning a GPIO on to enable, which operation has the
+ * potential to sleep. Given enable cannot sleep, but prepare can,
+ * we toggle the GPIO in prepare instead. Thus, nothing to do here.
+ */
+ return 0;
+}
+
+static void skl_int3472_clk_disable(struct clk_hw *hw)
+{
+ /* Likewise, nothing to do here... */
+}
+
+static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
+{
+ union acpi_object *obj;
+ unsigned int ret = 0;
+
+ obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
+ if (IS_ERR(obj))
+ return 0; /* report rate as 0 on error */
+
+ if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
+ dev_err(int3472->dev, "The buffer is too small\n");
+ goto out_free_buff;
+ }
+
+ ret = *(u32 *)(obj->buffer.pointer + CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET);
+
+out_free_buff:
+ kfree(obj);
+ return ret;
+}
+
+static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct int3472_gpio_clock *clk = to_int3472_clk(hw);
+ struct int3472_discrete_device *int3472 = to_int3472_device(clk);
+
+ return int3472->clock.frequency;
+}
+
+static const struct clk_ops skl_int3472_clock_ops = {
+ .prepare = skl_int3472_clk_prepare,
+ .unprepare = skl_int3472_clk_unprepare,
+ .enable = skl_int3472_clk_enable,
+ .disable = skl_int3472_clk_disable,
+ .recalc_rate = skl_int3472_clk_recalc_rate,
+};
+
+static struct int3472_sensor_config *
+skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
+{
+ struct int3472_sensor_config *ret;
+ union acpi_object *obj;
+ unsigned int i;
+
+ obj = acpi_evaluate_dsm_typed(int3472->sensor->handle,
+ &cio2_sensor_module_guid, 0x00,
+ 0x01, NULL, ACPI_TYPE_STRING);
+
+ if (!obj) {
+ dev_err(int3472->dev,
+ "Failed to get sensor module string from _DSM\n");
+ return ERR_PTR(-ENODEV);
+ }
+
+ if (obj->string.type != ACPI_TYPE_STRING) {
+ dev_err(int3472->dev,
+ "Sensor _DSM returned a non-string value\n");
+ ret = ERR_PTR(-EINVAL);
+ goto out_free_obj;
+ }
+
+ ret = NULL;
+ for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); i++) {
+ if (!strcmp(int3472_sensor_configs[i].sensor_module_name,
+ obj->string.pointer)) {
+ ret = &int3472_sensor_configs[i];
+ break;
+ }
+ }
+
+out_free_obj:
+ ACPI_FREE(obj);
+ return ret;
+}
+
+static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
+ struct acpi_resource *ares,
+ char *func, u32 polarity)
+{
+ char *path = ares->data.gpio.resource_source.string_ptr;
+ const struct int3472_sensor_config *sensor_config;
+ struct gpiod_lookup *table_entry;
+ struct acpi_device *adev;
+ acpi_handle handle;
+ acpi_status status;
+ int ret;
+
+ if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) {
+ dev_warn(int3472->dev, "Too many GPIOs mapped\n");
+ return -EINVAL;
+ }
+
+ sensor_config = int3472->sensor_config;
+ if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
+ const struct int3472_gpio_function_remap *remap =
+ sensor_config->function_maps;
+
+ for (; remap->documented; ++remap)
+ if (!strcmp(func, remap->documented)) {
+ func = remap->actual;
+ break;
+ }
+ }
+
+ /* Functions mapped to NULL should not be mapped to the sensor */
+ if (!func)
+ return 0;
+
+ status = acpi_get_handle(NULL, path, &handle);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+
+ ret = acpi_bus_get_device(handle, &adev);
+ if (ret)
+ return -ENODEV;
+
+ table_entry = &int3472->gpios.table[int3472->n_sensor_gpios];
+ table_entry->key = acpi_dev_name(adev);
+ table_entry->chip_hwnum = ares->data.gpio.pin_table[0];
+ table_entry->con_id = func;
+ table_entry->idx = 0;
+ table_entry->flags = polarity;
+
+ int3472->n_sensor_gpios++;
+
+ return 0;
+}
+
+static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
+ struct acpi_resource *ares, u8 type)
+{
+ char *path = ares->data.gpio.resource_source.string_ptr;
+ struct gpio_desc *gpio;
+
+ switch (type) {
+ case INT3472_GPIO_TYPE_CLK_ENABLE:
+ gpio = acpi_get_gpiod(path, ares->data.gpio.pin_table[0],
+ "int3472,clk-enable");
+ if (IS_ERR(gpio))
+ return (PTR_ERR(gpio));
+
+ int3472->clock.ena_gpio = gpio;
+ break;
+ case INT3472_GPIO_TYPE_PRIVACY_LED:
+ gpio = acpi_get_gpiod(path, ares->data.gpio.pin_table[0],
+ "int3472,privacy-led");
+ if (IS_ERR(gpio))
+ return (PTR_ERR(gpio));
+
+ int3472->clock.led_gpio = gpio;
+ break;
+ default:
+ dev_err(int3472->dev, "Invalid GPIO type 0x%hx for clock\n",
+ type);
+ break;
+ }
+
+ return 0;
+}
+
+static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
+{
+ struct clk_init_data init = {
+ .ops = &skl_int3472_clock_ops,
+ .flags = CLK_GET_RATE_NOCACHE,
+ };
+ int ret = 0;
+
+ init.name = kasprintf(GFP_KERNEL, "%s-clk",
+ acpi_dev_name(int3472->adev));
+ if (!init.name)
+ return -ENOMEM;
+
+ int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
+
+ int3472->clock.clk_hw.init = &init;
+ int3472->clock.clk = clk_register(&int3472->adev->dev,
+ &int3472->clock.clk_hw);
+ if (IS_ERR(int3472->clock.clk)) {
+ ret = PTR_ERR(int3472->clock.clk);
+ goto out_free_init_name;
+ }
+
+ int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
+ int3472->sensor_name);
+ if (!int3472->clock.cl) {
+ ret = -ENOMEM;
+ goto err_unregister_clk;
+ }
+
+ goto out_free_init_name;
+
+err_unregister_clk:
+ clk_unregister(int3472->clock.clk);
+out_free_init_name:
+ kfree(init.name);
+
+ return ret;
+}
+
+static int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
+ struct acpi_resource *ares)
+{
+ char *path = ares->data.gpio.resource_source.string_ptr;
+ struct int3472_sensor_config *sensor_config;
+ struct regulator_init_data init_data = { };
+ struct regulator_config cfg = { };
+ int ret;
+
+ sensor_config = int3472->sensor_config;
+ if (IS_ERR_OR_NULL(sensor_config)) {
+ dev_err(int3472->dev, "No sensor module config\n");
+ return PTR_ERR(sensor_config);
+ }
+
+ if (!sensor_config->supply_map.supply) {
+ dev_err(int3472->dev, "No supply name defined\n");
+ return -ENODEV;
+ }
+
+ init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
+ init_data.num_consumer_supplies = 1;
+ sensor_config->supply_map.dev_name = int3472->sensor_name;
+ init_data.consumer_supplies = &sensor_config->supply_map;
+
+ snprintf(int3472->regulator.regulator_name,
+ sizeof(int3472->regulator.regulator_name), "%s-regulator",
+ acpi_dev_name(int3472->adev));
+ snprintf(int3472->regulator.supply_name,
+ GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
+
+ int3472->regulator.rdesc = INT3472_REGULATOR(
+ int3472->regulator.regulator_name,
+ int3472->regulator.supply_name,
+ &int3472_gpio_regulator_ops);
+
+ int3472->regulator.gpio = acpi_get_gpiod(path,
+ ares->data.gpio.pin_table[0],
+ "int3472,regulator");
+ if (IS_ERR(int3472->regulator.gpio)) {
+ dev_err(int3472->dev, "Failed to get regulator GPIO lines\n");
+ return PTR_ERR(int3472->regulator.gpio);
+ }
+
+ cfg.dev = &int3472->adev->dev;
+ cfg.init_data = &init_data;
+ cfg.ena_gpiod = int3472->regulator.gpio;
+
+ int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc,
+ &cfg);
+ if (IS_ERR(int3472->regulator.rdev)) {
+ ret = PTR_ERR(int3472->regulator.rdev);
+ goto err_free_gpio;
+ }
+
+ return 0;
+
+err_free_gpio:
+ gpiod_put(int3472->regulator.gpio);
+
+ return ret;
+}
+
+/**
+ * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
+ * @ares: A pointer to a &struct acpi_resource
+ * @data: A pointer to a &struct int3472_discrete_device
+ *
+ * This function handles GPIO resources that are against an INT3472
+ * ACPI device, by checking the value of the corresponding _DSM entry.
+ * This will return a 32bit int, where the lowest byte represents the
+ * function of the GPIO pin:
+ *
+ * 0x00 Reset
+ * 0x01 Power down
+ * 0x0b Power enable
+ * 0x0c Clock enable
+ * 0x0d Privacy LED
+ *
+ * There are some known platform specific quirks where that does not quite
+ * hold up; for example where a pin with type 0x01 (Power down) is mapped to
+ * a sensor pin that performs a reset function or entries in _CRS and _DSM that
+ * do not actually correspond to a physical connection. These will be handled
+ * by the mapping sub-functions.
+ *
+ * GPIOs will either be mapped directly to the sensor device or else used
+ * to create clocks and regulators via the usual frameworks.
+ *
+ * Return:
+ * * 0 - When all resources found are handled properly.
+ * * -EINVAL - If the resource is not a GPIO IO resource
+ * * -ENODEV - If the resource has no corresponding _DSM entry
+ * * -Other - Errors propagated from one of the sub-functions.
+ */
+static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
+ void *data)
+{
+ struct int3472_discrete_device *int3472 = data;
+ u16 pin = ares->data.gpio.pin_table[0];
+ union acpi_object *obj;
+ char *err_msg;
+ int ret = 0;
+ u8 type;
+
+ if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
+ ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
+ return 1; /* Deliberately positive so parsing continues */
+
+ /*
+ * n_gpios + 2 because the index of this _DSM function is 1-based and
+ * the first function is just a count.
+ */
+ obj = acpi_evaluate_dsm_typed(int3472->adev->handle,
+ &int3472_gpio_guid, 0x00,
+ int3472->n_gpios + 2,
+ NULL, ACPI_TYPE_INTEGER);
+
+ if (!obj) {
+ dev_warn(int3472->dev, "No _DSM entry for GPIO pin %u\n", pin);
+ return 1;
+ }
+
+ type = obj->integer.value & 0xff;
+
+ switch (type) {
+ case INT3472_GPIO_TYPE_RESET:
+ ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "reset",
+ GPIO_ACTIVE_LOW);
+ if (ret)
+ err_msg = "Failed to map reset pin to sensor\n";
+
+ break;
+ case INT3472_GPIO_TYPE_POWERDOWN:
+ ret = skl_int3472_map_gpio_to_sensor(int3472, ares,
+ "powerdown",
+ GPIO_ACTIVE_LOW);
+ if (ret)
+ err_msg = "Failed to map powerdown pin to sensor\n";
+
+ break;
+ case INT3472_GPIO_TYPE_CLK_ENABLE:
+ case INT3472_GPIO_TYPE_PRIVACY_LED:
+ ret = skl_int3472_map_gpio_to_clk(int3472, ares, type);
+ if (ret)
+ err_msg = "Failed to map GPIO to clock\n";
+
+ break;
+ case INT3472_GPIO_TYPE_POWER_ENABLE:
+ ret = skl_int3472_register_regulator(int3472, ares);
+ if (ret)
+ err_msg = "Failed to map regulator to sensor\n";
+
+ break;
+ default:
+ dev_warn(int3472->dev,
+ "GPIO type 0x%02x unknown; the sensor may not work\n",
+ type);
+ ret = 1;
+ break;
+ }
+
+ if (ret < 0 && ret != -EPROBE_DEFER)
+ dev_err(int3472->dev, err_msg);
+
+ int3472->n_gpios++;
+ ACPI_FREE(obj);
+
+ return ret;
+}
+
+static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
+{
+ struct list_head resource_list;
+ int ret;
+
+ INIT_LIST_HEAD(&resource_list);
+
+ int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
+
+ ret = acpi_dev_get_resources(int3472->adev, &resource_list,
+ skl_int3472_handle_gpio_resources,
+ int3472);
+ if (ret)
+ goto out_free_res_list;
+
+ if (int3472->clock.ena_gpio) {
+ ret = skl_int3472_register_clock(int3472);
+ if (ret)
+ goto out_free_res_list;
+ } else {
+ if (int3472->clock.led_gpio)
+ dev_warn(int3472->dev,
+ "No clk GPIO. The privacy LED won't work\n");
+ }
+
+ int3472->gpios.dev_id = int3472->sensor_name;
+ gpiod_add_lookup_table(&int3472->gpios);
+
+out_free_res_list:
+ acpi_dev_free_resource_list(&resource_list);
+
+ return ret;
+}
+
+int skl_int3472_discrete_probe(struct platform_device *pdev)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+ struct int3472_discrete_device *int3472;
+ struct int3472_cldb cldb;
+ int ret;
+
+ ret = skl_int3472_fill_cldb(adev, &cldb);
+ if (ret) {
+ dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
+ return ret;
+ }
+
+ if (cldb.control_logic_type != 1) {
+ dev_err(&pdev->dev, "Unsupported control logic type %u\n",
+ cldb.control_logic_type);
+ return -EINVAL;
+ }
+
+ /* Max num GPIOs we've seen plus a terminator */
+ int3472 = kzalloc(struct_size(int3472, gpios.table,
+ INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
+ if (!int3472)
+ return -ENOMEM;
+
+ int3472->adev = adev;
+ int3472->dev = &pdev->dev;
+ platform_set_drvdata(pdev, int3472);
+
+ int3472->sensor = acpi_dev_get_dependent_dev(adev);
+ if (IS_ERR_OR_NULL(int3472->sensor)) {
+ dev_err(&pdev->dev,
+ "INT3472 seems to have no dependents.\n");
+ ret = -ENODEV;
+ goto err_free_int3472;
+ }
+ get_device(&int3472->sensor->dev);
+
+ int3472->sensor_name = kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT,
+ acpi_dev_name(int3472->sensor));
+
+ ret = skl_int3472_parse_crs(int3472);
+ if (ret) {
+ skl_int3472_discrete_remove(pdev);
+ return ret;
+ }
+
+ return 0;
+
+err_free_int3472:
+ kfree(int3472);
+ return ret;
+}
+
+int skl_int3472_discrete_remove(struct platform_device *pdev)
+{
+ struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
+
+ if (int3472->gpios.dev_id)
+ gpiod_remove_lookup_table(&int3472->gpios);
+
+ if (!IS_ERR(int3472->regulator.rdev))
+ regulator_unregister(int3472->regulator.rdev);
+
+ if (!IS_ERR(int3472->clock.clk))
+ clk_unregister(int3472->clock.clk);
+
+ if (int3472->clock.cl)
+ clkdev_drop(int3472->clock.cl);
+
+ gpiod_put(int3472->regulator.gpio);
+ gpiod_put(int3472->clock.ena_gpio);
+ gpiod_put(int3472->clock.led_gpio);
+
+ acpi_dev_put(int3472->sensor);
+
+ kfree(int3472->sensor_name);
+ kfree(int3472);
+
+ return 0;
+}
diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
new file mode 100644
index 000000000000..d0d2391e263f
--- /dev/null
+++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Author: Dan Scally <djrscally@gmail.com> */
+
+#include <linux/i2c.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/tps68470.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include "intel_skl_int3472_common.h"
+
+static const struct mfd_cell tps68470_c[] = {
+ { .name = "tps68470-gpio" },
+ { .name = "tps68470_pmic_opregion" },
+};
+
+static const struct mfd_cell tps68470_w[] = {
+ { .name = "tps68470-gpio" },
+ { .name = "tps68470-clk" },
+ { .name = "tps68470-regulator"},
+};
+
+static const struct regmap_config tps68470_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = TPS68470_REG_MAX,
+};
+
+static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
+{
+ unsigned int version;
+ int ret;
+
+ /* Force software reset */
+ ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
+ if (ret) {
+ dev_err(dev, "Failed to read revision register: %d\n", ret);
+ return ret;
+ }
+
+ dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
+
+ return 0;
+}
+
+int skl_int3472_tps68470_probe(struct i2c_client *client)
+{
+ struct acpi_device *adev = ACPI_COMPANION(&client->dev);
+ struct int3472_cldb cldb = { 0 };
+ bool cldb_present = true;
+ struct regmap *regmap;
+ int ret;
+
+ regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(&client->dev, "Failed to create regmap: %ld\n",
+ PTR_ERR(regmap));
+ return PTR_ERR(regmap);
+ }
+
+ i2c_set_clientdata(client, regmap);
+
+ ret = tps68470_chip_init(&client->dev, regmap);
+ if (ret < 0) {
+ dev_err(&client->dev, "TPS68470 init error %d\n", ret);
+ return ret;
+ }
+
+ /*
+ * Check CLDB buffer against the PMIC's adev. If present, then we check
+ * the value of control_logic_type field and follow one of the
+ * following scenarios:
+ *
+ * 1. No CLDB - likely ACPI tables designed for ChromeOS. We
+ * create platform devices for the GPIOs and OpRegion drivers.
+ *
+ * 2. CLDB, with control_logic_type = 2 - probably ACPI tables
+ * made for Windows 2-in-1 platforms. Register pdevs for GPIO,
+ * Clock and Regulator drivers to bind to.
+ *
+ * 3. Any other value in control_logic_type, we should never have
+ * gotten to this point; fail probe and return.
+ */
+ ret = skl_int3472_fill_cldb(adev, &cldb);
+ if (!ret && cldb.control_logic_type != 2) {
+ dev_err(&client->dev, "Unsupported control logic type %u\n",
+ cldb.control_logic_type);
+ return -EINVAL;
+ }
+
+ if (ret)
+ cldb_present = false;
+
+ if (cldb_present)
+ ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
+ tps68470_w, ARRAY_SIZE(tps68470_w),
+ NULL, 0, NULL);
+ else
+ ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
+ tps68470_c, ARRAY_SIZE(tps68470_c),
+ NULL, 0, NULL);
+
+ if (ret) {
+ dev_err(&client->dev, "Failed to add MFD devices\n");
+ return ret;
+ }
+
+ return 0;
+}
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (4 preceding siblings ...)
2021-02-22 13:07 ` [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver Daniel Scally
@ 2021-02-22 13:07 ` Daniel Scally
2021-02-22 14:12 ` Andy Shevchenko
2021-03-10 9:33 ` Lee Jones
2021-02-22 13:11 ` [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (2 subsequent siblings)
8 siblings, 2 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:07 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
This driver only covered one scenario in which ACPI devices with _HID
INT3472 are found, and its functionality has been taken over by the
intel-skl-int3472 module, so remove it.
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes in v3:
- Replaced Kconfig dependencies with INTEL_SKL_INT3472 for the tps68470
OpRegion and GPIO drivers.
drivers/acpi/pmic/Kconfig | 2 +-
drivers/gpio/Kconfig | 2 +-
drivers/mfd/Kconfig | 18 --------
drivers/mfd/Makefile | 1 -
drivers/mfd/tps68470.c | 97 ---------------------------------------
5 files changed, 2 insertions(+), 118 deletions(-)
delete mode 100644 drivers/mfd/tps68470.c
diff --git a/drivers/acpi/pmic/Kconfig b/drivers/acpi/pmic/Kconfig
index 56bbcb2ce61b..f84b8f6038dc 100644
--- a/drivers/acpi/pmic/Kconfig
+++ b/drivers/acpi/pmic/Kconfig
@@ -52,7 +52,7 @@ endif # PMIC_OPREGION
config TPS68470_PMIC_OPREGION
bool "ACPI operation region support for TPS68470 PMIC"
- depends on MFD_TPS68470
+ depends on INTEL_SKL_INT3472
help
This config adds ACPI operation region support for TI TPS68470 PMIC.
TPS68470 device is an advanced power management unit that powers
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c70f46e80a3b..998898c72af8 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1343,7 +1343,7 @@ config GPIO_TPS65912
config GPIO_TPS68470
bool "TPS68470 GPIO"
- depends on MFD_TPS68470
+ depends on INTEL_SKL_INT3472
help
Select this option to enable GPIO driver for the TPS68470
chip family.
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index bdfce7b15621..9a1f648efde0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1520,24 +1520,6 @@ config MFD_TPS65217
This driver can also be built as a module. If so, the module
will be called tps65217.
-config MFD_TPS68470
- bool "TI TPS68470 Power Management / LED chips"
- depends on ACPI && PCI && I2C=y
- depends on I2C_DESIGNWARE_PLATFORM=y
- select MFD_CORE
- select REGMAP_I2C
- help
- If you say yes here you get support for the TPS68470 series of
- Power Management / LED chips.
-
- These include voltage regulators, LEDs and other features
- that are often used in portable devices.
-
- This option is a bool as it provides an ACPI operation
- region, which must be available before any of the devices
- using this are probed. This option also configures the
- designware-i2c driver to be built-in, for the same reason.
-
config MFD_TI_LP873X
tristate "TI LP873X Power Management IC"
depends on I2C
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 14fdb188af02..5994e812f479 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -105,7 +105,6 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
-obj-$(CONFIG_MFD_TPS68470) += tps68470.o
obj-$(CONFIG_MFD_TPS80031) += tps80031.o
obj-$(CONFIG_MENELAUS) += menelaus.o
diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
deleted file mode 100644
index 4a4df4ffd18c..000000000000
--- a/drivers/mfd/tps68470.c
+++ /dev/null
@@ -1,97 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * TPS68470 chip Parent driver
- *
- * Copyright (C) 2017 Intel Corporation
- *
- * Authors:
- * Rajmohan Mani <rajmohan.mani@intel.com>
- * Tianshu Qiu <tian.shu.qiu@intel.com>
- * Jian Xu Zheng <jian.xu.zheng@intel.com>
- * Yuning Pu <yuning.pu@intel.com>
- */
-
-#include <linux/acpi.h>
-#include <linux/delay.h>
-#include <linux/i2c.h>
-#include <linux/init.h>
-#include <linux/mfd/core.h>
-#include <linux/mfd/tps68470.h>
-#include <linux/regmap.h>
-
-static const struct mfd_cell tps68470s[] = {
- { .name = "tps68470-gpio" },
- { .name = "tps68470_pmic_opregion" },
-};
-
-static const struct regmap_config tps68470_regmap_config = {
- .reg_bits = 8,
- .val_bits = 8,
- .max_register = TPS68470_REG_MAX,
-};
-
-static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
-{
- unsigned int version;
- int ret;
-
- /* Force software reset */
- ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
- if (ret)
- return ret;
-
- ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
- if (ret) {
- dev_err(dev, "Failed to read revision register: %d\n", ret);
- return ret;
- }
-
- dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
-
- return 0;
-}
-
-static int tps68470_probe(struct i2c_client *client)
-{
- struct device *dev = &client->dev;
- struct regmap *regmap;
- int ret;
-
- regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
- if (IS_ERR(regmap)) {
- dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
- PTR_ERR(regmap));
- return PTR_ERR(regmap);
- }
-
- i2c_set_clientdata(client, regmap);
-
- ret = tps68470_chip_init(dev, regmap);
- if (ret < 0) {
- dev_err(dev, "TPS68470 Init Error %d\n", ret);
- return ret;
- }
-
- ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, tps68470s,
- ARRAY_SIZE(tps68470s), NULL, 0, NULL);
- if (ret < 0) {
- dev_err(dev, "devm_mfd_add_devices failed: %d\n", ret);
- return ret;
- }
-
- return 0;
-}
-
-static const struct acpi_device_id tps68470_acpi_ids[] = {
- {"INT3472"},
- {},
-};
-
-static struct i2c_driver tps68470_driver = {
- .driver = {
- .name = "tps68470",
- .acpi_match_table = tps68470_acpi_ids,
- },
- .probe_new = tps68470_probe,
-};
-builtin_i2c_driver(tps68470_driver);
--
2.25.1
^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (5 preceding siblings ...)
2021-02-22 13:07 ` [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver Daniel Scally
@ 2021-02-22 13:11 ` Daniel Scally
2021-02-22 14:15 ` Andy Shevchenko
2021-03-04 13:37 ` Hans de Goede
8 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:11 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andriy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
+cc Andy, who's email address I managed to screw up - sorry.
On 22/02/2021 13:07, Daniel Scally wrote:
> 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
>
> v2 was here:
> https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrscally@gmail.com/
>
> Series level changelog:
>
> - Dropped the patch moving acpi_lpss_dep() to utils since it's not used
> in acpi_dev_get_dependent_dev() anymore.
> - Replaced it with a patch extending acpi_walk_dep_device_list() to be
> able to apply a given callback against each device in acpi_dep_list
> - Dropped the patch creating acpi_i2c_dev_name() and simply open coded
> that functionality.
>
> This has been tested on a number of devices, but currently **not** on a device
> designed for ChromeOS, which we ideally need to do to ensure no regression
> caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way
> you could help with that? Unfortunately, I don't have a device to test it on
> myself.
>
> Original cover letter:
>
> 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 available but have not so far been
> tested, so aren't part of this series.
>
> The existing mfd/tps68470.c driver being thus superseded, it is removed.
>
> Thanks
> Dan
>
> Daniel Scally (6):
> ACPI: scan: Extend acpi_walk_dep_device_list()
> ACPI: scan: Add function to fetch dependent of acpi device
> i2c: core: Add a format macro for I2C device names
> gpiolib: acpi: Export acpi_get_gpiod()
> platform/x86: Add intel_skl_int3472 driver
> mfd: tps68470: Remove tps68470 MFD driver
>
> MAINTAINERS | 5 +
> drivers/acpi/ec.c | 2 +-
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
> drivers/acpi/scan.c | 92 ++-
> drivers/gpio/Kconfig | 2 +-
> drivers/gpio/gpiolib-acpi.c | 38 +-
> drivers/i2c/i2c-core-acpi.c | 2 +-
> drivers/i2c/i2c-core-base.c | 4 +-
> drivers/mfd/Kconfig | 18 -
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---
> drivers/platform/surface/surface3_power.c | 2 +-
> drivers/platform/x86/Kconfig | 2 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel-int3472/Kconfig | 31 +
> drivers/platform/x86/intel-int3472/Makefile | 4 +
> .../intel-int3472/intel_skl_int3472_common.c | 106 ++++
> .../intel-int3472/intel_skl_int3472_common.h | 110 ++++
> .../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
> .../intel_skl_int3472_tps68470.c | 113 ++++
> include/acpi/acpi_bus.h | 8 +
> include/linux/acpi.h | 4 +-
> include/linux/gpio/consumer.h | 7 +
> include/linux/i2c.h | 3 +
> 25 files changed, 1100 insertions(+), 148 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
> create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
> create mode 100644 drivers/platform/x86/intel-int3472/Makefile
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 13:07 ` [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver Daniel Scally
@ 2021-02-22 13:19 ` Daniel Scally
2021-02-22 13:27 ` Hans de Goede
2021-02-22 14:58 ` Andy Shevchenko
2021-02-23 20:04 ` Laurent Pinchart
2 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 13:19 UTC (permalink / raw)
To: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones
Cc: Andy Shevchenko, kieran.bingham+renesas, laurent.pinchart,
hdegoede, mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
Hi all
On 22/02/2021 13:07, Daniel Scally wrote:
> diff --git a/drivers/platform/x86/intel-int3472/Kconfig b/drivers/platform/x86/intel-int3472/Kconfig
> new file mode 100644
> index 000000000000..b94622245c21
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/Kconfig
> @@ -0,0 +1,31 @@
> +config INTEL_SKL_INT3472
> + tristate "Intel SkyLake ACPI INT3472 Driver"
> + depends on ACPI
> + depends on REGULATOR
> + depends on GPIOLIB
> + depends on COMMON_CLK && CLKDEV_LOOKUP
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + This driver adds support for the INT3472 ACPI devices found on some
> + Intel SkyLake devices.
> +
> + The INT3472 is an Intel camera power controller, a logical device
> + found on some Skylake-based systems that can map to different
> + hardware devices depending on the platform. On machines
> + designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
> + machines designed for Windows, it maps to either a TP68470
> + camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
> + and power gates.
> +
> + If your device was designed for Chrome OS, this driver will provide
> + an ACPI OpRegion, which must be available before any of the devices
> + using it are probed. For this reason, you should select Y if your
> + device was designed for ChromeOS. For the same reason the
> + I2C_DESIGNWARE_PLATFORM option must be set to Y too.
> +
> + Say Y or M here if you have a SkyLake device designed for use
> + with Windows or ChromeOS. Say N here if you are not sure.
> +
> + The module will be named "intel-skl-int3472"
The Kconfig option for the existing tps68470 driver is a bool which
depends on I2C_DESIGNWARE_PLATFORM=y, giving the following reason:
This option is a bool as it provides an ACPI operation
region, which must be available before any of the devices
using this are probed. This option also configures the
designware-i2c driver to be built-in, for the same reason.
One problem I've faced is that that scenario only applies to some
devices that this new driver can support, so hard-coding it as built in
didn't make much sense. For that reason I opted to set it tristate, but
of course that issue still exists for ChromeOS devices where the
OpRegion will be registered. I opted for simply documenting that
requirement, as is done in aaac4a2eadaa6: "mfd: axp20x-i2c: Document
that this must be builtin on x86", but that's not entirely satisfactory.
Possible alternatives might be setting "depends on
I2C_DESIGNWARE_PLATFORM=y if CHROME_PLATFORMS" or something similar,
though of course the User would still have to realise they need to
build-in the INTEL_SKL_INT3472 Kconfig option too.
Feedback around this issue would be particularly welcome, as I'm not
sure what the best approach might be.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 13:19 ` Daniel Scally
@ 2021-02-22 13:27 ` Hans de Goede
2021-02-22 22:50 ` Daniel Scally
0 siblings, 1 reply; 44+ messages in thread
From: Hans de Goede @ 2021-02-22 13:27 UTC (permalink / raw)
To: Daniel Scally, tfiga, sakari.ailus, rajmohan.mani, rjw, lenb,
mika.westerberg, linus.walleij, bgolaszewski, wsa, lee.jones
Cc: Andy Shevchenko, kieran.bingham+renesas, laurent.pinchart,
mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
Hi,
On 2/22/21 2:19 PM, Daniel Scally wrote:
> Hi all
>
> On 22/02/2021 13:07, Daniel Scally wrote:
>> diff --git a/drivers/platform/x86/intel-int3472/Kconfig b/drivers/platform/x86/intel-int3472/Kconfig
>> new file mode 100644
>> index 000000000000..b94622245c21
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel-int3472/Kconfig
>> @@ -0,0 +1,31 @@
>> +config INTEL_SKL_INT3472
>> + tristate "Intel SkyLake ACPI INT3472 Driver"
>> + depends on ACPI
>> + depends on REGULATOR
>> + depends on GPIOLIB
>> + depends on COMMON_CLK && CLKDEV_LOOKUP
>> + depends on I2C
>> + select MFD_CORE
>> + select REGMAP_I2C
>> + help
>> + This driver adds support for the INT3472 ACPI devices found on some
>> + Intel SkyLake devices.
>> +
>> + The INT3472 is an Intel camera power controller, a logical device
>> + found on some Skylake-based systems that can map to different
>> + hardware devices depending on the platform. On machines
>> + designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
>> + machines designed for Windows, it maps to either a TP68470
>> + camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
>> + and power gates.
>> +
>> + If your device was designed for Chrome OS, this driver will provide
>> + an ACPI OpRegion, which must be available before any of the devices
>> + using it are probed. For this reason, you should select Y if your
>> + device was designed for ChromeOS. For the same reason the
>> + I2C_DESIGNWARE_PLATFORM option must be set to Y too.
>> +
>> + Say Y or M here if you have a SkyLake device designed for use
>> + with Windows or ChromeOS. Say N here if you are not sure.
>> +
>> + The module will be named "intel-skl-int3472"
> The Kconfig option for the existing tps68470 driver is a bool which
> depends on I2C_DESIGNWARE_PLATFORM=y, giving the following reason:
>
> This option is a bool as it provides an ACPI operation
> region, which must be available before any of the devices
> using this are probed. This option also configures the
> designware-i2c driver to be built-in, for the same reason.
>
> One problem I've faced is that that scenario only applies to some
> devices that this new driver can support, so hard-coding it as built in
> didn't make much sense. For that reason I opted to set it tristate, but
> of course that issue still exists for ChromeOS devices where the
> OpRegion will be registered. I opted for simply documenting that
> requirement, as is done in aaac4a2eadaa6: "mfd: axp20x-i2c: Document
> that this must be builtin on x86", but that's not entirely satisfactory.
> Possible alternatives might be setting "depends on
> I2C_DESIGNWARE_PLATFORM=y if CHROME_PLATFORMS" or something similar,
> though of course the User would still have to realise they need to
> build-in the INTEL_SKL_INT3472 Kconfig option too.
>
> Feedback around this issue would be particularly welcome, as I'm not
> sure what the best approach might be.
This is a tricky area, I actually wrote the "mfd: axp20x-i2c: Document
that this must be builtin on x86" patch you refer to. At first I tried
to express the dependency in Kconfig language but things got too complex
and Kconfig sometimes became unhappy about circular deps (or something
like that).
The most important thing here is to make sure that the generic configs
shipped by distros get this right; and we can hope that people creating
those configs at least read the help text...
So all in all I believe that just documenting the requirement is fine.
The alternative would be to just change I2C_DESIGNWARE_PLATFORM (and the
core) to a bool, or at least make it not selectable as module when
X86 and ACPI are set... That would be a bit of a big hammer but might
not be the worst idea actually.
Regards,
Hans
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
@ 2021-02-22 13:34 ` Andy Shevchenko
2021-03-07 13:36 ` Daniel Scally
2021-02-22 13:38 ` Wolfram Sang
2021-03-08 17:46 ` Rafael J. Wysocki
2 siblings, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-22 13:34 UTC (permalink / raw)
To: Daniel Scally, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel, Rafael J . Wysocki
On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> The acpi_walk_dep_device_list() is not as generalisable as its name
> implies, serving only to decrement the dependency count for each
> dependent device of the input. Extend the function to instead accept
> a callback which can be applied to all the dependencies in acpi_dep_list.
> Replace all existing calls to the function with calls to a wrapper, passing
> a callback that applies the same dependency reduction.
The code looks okay to me, if it was the initial idea, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
See a few nit-picks below.
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
> - patch introduced
>
> drivers/acpi/ec.c | 2 +-
> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
> drivers/acpi/scan.c | 58 ++++++++++++++++-------
> drivers/gpio/gpiolib-acpi.c | 2 +-
> drivers/i2c/i2c-core-acpi.c | 2 +-
> drivers/platform/surface/surface3_power.c | 2 +-
> include/acpi/acpi_bus.h | 7 +++
> include/linux/acpi.h | 4 +-
> 8 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 13565629ce0a..a258db713bd2 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1627,7 +1627,7 @@ static int acpi_ec_add(struct acpi_device *device)
> WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
>
> /* Reprobe devices depending on the EC */
> - acpi_walk_dep_device_list(ec->handle);
> + acpi_dev_flag_dependency_met(ec->handle);
>
> acpi_handle_debug(ec->handle, "enumerated.\n");
> return 0;
> diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> index a5101b07611a..59cca504325e 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> @@ -117,7 +117,7 @@ static int chtdc_ti_pmic_opregion_probe(struct platform_device *pdev)
> return err;
>
> /* Re-enumerate devices depending on PMIC */
> - acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent));
> + acpi_dev_flag_dependency_met(ACPI_HANDLE(pdev->dev.parent));
> return 0;
> }
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 80b668c80073..c9e4190316ef 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -49,12 +49,6 @@ static DEFINE_MUTEX(acpi_hp_context_lock);
> */
> static u64 spcr_uart_addr;
>
> -struct acpi_dep_data {
> - struct list_head node;
> - acpi_handle supplier;
> - acpi_handle consumer;
> -};
> -
> void acpi_scan_lock_acquire(void)
> {
> mutex_lock(&acpi_scan_lock);
> @@ -2099,30 +2093,58 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
> device->handler->hotplug.notify_online(device);
> }
>
> -void acpi_walk_dep_device_list(acpi_handle handle)
> +static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
> + void *data)
> {
> - struct acpi_dep_data *dep, *tmp;
> struct acpi_device *adev;
>
> + acpi_bus_get_device(dep->consumer, &adev);
> + if (!adev)
> + return 0;
> +
> + adev->dep_unmet--;
> + if (!adev->dep_unmet)
> + acpi_bus_attach(adev, true);
> +
> + list_del(&dep->node);
> + kfree(dep);
> + return 0;
> +}
> +
> +void acpi_walk_dep_device_list(acpi_handle handle,
> + int (*callback)(struct acpi_dep_data *, void *),
> + void *data)
> +{
> + struct acpi_dep_data *dep, *tmp;
> + int ret;
> +
> mutex_lock(&acpi_dep_list_lock);
> list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
> if (dep->supplier == handle) {
> - acpi_bus_get_device(dep->consumer, &adev);
> - if (!adev)
> - continue;
> -
> - adev->dep_unmet--;
> - if (!adev->dep_unmet)
> - acpi_bus_attach(adev, true);
> -
> - list_del(&dep->node);
> - kfree(dep);
> + ret = callback(dep, data);
> + if (ret)
> + break;
> }
> }
> mutex_unlock(&acpi_dep_list_lock);
> }
> EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
>
> +/**
> + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device
> + * is now active
... - Inform consumers that the device is now active
(and it will be on one line)
> + * @handle: acpi_handle for the supplier device
ACPI handle
> + *
> + * This function walks through the dependencies list and informs each consumer
> + * of @handle that their dependency upon it is now met. Devices with no more
> + * unmet dependencies will be attached to the acpi bus.
acpi -> ACPI ?
> + */
> +void acpi_dev_flag_dependency_met(acpi_handle handle)
> +{
Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
> + acpi_walk_dep_device_list(handle, __acpi_dev_flag_dependency_met, NULL);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
> +
> /**
> * acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
> * @handle: Root of the namespace scope to scan.
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index e37a57d0a2f0..e4d728fda982 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1254,7 +1254,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
>
> acpi_gpiochip_request_regions(acpi_gpio);
> acpi_gpiochip_scan_gpios(acpi_gpio);
> - acpi_walk_dep_device_list(handle);
> + acpi_dev_flag_dependency_met(handle);
> }
>
> void acpi_gpiochip_remove(struct gpio_chip *chip)
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 37c510d9347a..38647cf34bde 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -283,7 +283,7 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap)
> if (!handle)
> return;
>
> - acpi_walk_dep_device_list(handle);
> + acpi_dev_flag_dependency_met(handle);
> }
>
> static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
> index cc4f9cba6856..ad895285d3e9 100644
> --- a/drivers/platform/surface/surface3_power.c
> +++ b/drivers/platform/surface/surface3_power.c
> @@ -478,7 +478,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
> return -ENOMEM;
> }
>
> - acpi_walk_dep_device_list(handle);
> + acpi_dev_flag_dependency_met(handle);
> return 0;
> }
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 02a716a0af5d..91172af3a04d 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -278,6 +278,12 @@ struct acpi_device_power {
> struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */
> };
>
> +struct acpi_dep_data {
> + struct list_head node;
> + acpi_handle supplier;
> + acpi_handle consumer;
> +};
> +
> /* Performance Management */
>
> struct acpi_device_perf_flags {
> @@ -683,6 +689,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
>
> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
>
> +void acpi_dev_flag_dependency_met(acpi_handle handle);
> struct acpi_device *
> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
> struct acpi_device *
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 2630c2e953f7..2d5e6e88e8a0 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -655,7 +655,9 @@ extern bool acpi_driver_match_device(struct device *dev,
> const struct device_driver *drv);
> int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
> int acpi_device_modalias(struct device *, char *, int);
> -void acpi_walk_dep_device_list(acpi_handle handle);
> +void acpi_walk_dep_device_list(acpi_handle handle,
> + int (*callback)(struct acpi_dep_data *, void *),
> + void *data);
>
> struct platform_device *acpi_create_platform_device(struct acpi_device *,
> struct property_entry *);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
2021-02-22 13:34 ` Andy Shevchenko
@ 2021-02-22 13:38 ` Wolfram Sang
2021-03-08 17:46 ` Rafael J. Wysocki
2 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2021-02-22 13:38 UTC (permalink / raw)
To: Daniel Scally
Cc: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, lee.jones, andy.shevchenko,
kieran.bingham+renesas, laurent.pinchart, hdegoede, mgross,
luzmaximilian, robert.moore, erik.kaneda, me, linux-kernel,
linux-acpi, linux-gpio, linux-i2c, platform-driver-x86, devel,
Rafael J . Wysocki
[-- Attachment #1: Type: text/plain, Size: 766 bytes --]
On Mon, Feb 22, 2021 at 01:07:30PM +0000, Daniel Scally wrote:
> The acpi_walk_dep_device_list() is not as generalisable as its name
> implies, serving only to decrement the dependency count for each
> dependent device of the input. Extend the function to instead accept
> a callback which can be applied to all the dependencies in acpi_dep_list.
> Replace all existing calls to the function with calls to a wrapper, passing
> a callback that applies the same dependency reduction.
>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
I trust you guys:
Acked-by: Wolfram Sang <wsa@kernel.org> # for changing I2C core
I might need an immutable branch, though, if this is somehow possible.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 3/6] i2c: core: Add a format macro for I2C device names
2021-02-22 13:07 ` [PATCH v3 3/6] i2c: core: Add a format macro for I2C device names Daniel Scally
@ 2021-02-22 13:38 ` Wolfram Sang
0 siblings, 0 replies; 44+ messages in thread
From: Wolfram Sang @ 2021-02-22 13:38 UTC (permalink / raw)
To: Daniel Scally
Cc: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, lee.jones, andy.shevchenko,
kieran.bingham+renesas, laurent.pinchart, hdegoede, mgross,
luzmaximilian, robert.moore, erik.kaneda, me, linux-kernel,
linux-acpi, linux-gpio, linux-i2c, platform-driver-x86, devel,
Andy Shevchenko
[-- Attachment #1: Type: text/plain, Size: 940 bytes --]
On Mon, Feb 22, 2021 at 01:07:32PM +0000, Daniel Scally wrote:
> Some places in the kernel allow users to map resources to a device
> using device name (for example, in the struct gpiod_lookup_table).
> Currently this involves waiting for the I2C client to have been registered
> so we can use dev_name(&client->dev). We want to add a function to allow
> users to refer to an I2C device by name before it has been instantiated,
> so create a macro for the format that's accessible outside the I2C layer
> and use it in i2c_dev_set_name().
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
Acked-by: Wolfram Sang <wsa@kernel.org> # for changing I2C core
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device
2021-02-22 13:07 ` [PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device Daniel Scally
@ 2021-02-22 13:41 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-22 13:41 UTC (permalink / raw)
To: Daniel Scally, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On Mon, Feb 22, 2021 at 3:11 PM Daniel Scally <djrscally@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 callback and a wrapper to walk over the
> acpi_dep_list and return the dependent ACPI device.
>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Nit-picks below as usual :-)
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
> - Switched from a standalone function to a callback passed to
> acpi_walk_dep_device_list().
>
> drivers/acpi/scan.c | 34 ++++++++++++++++++++++++++++++++++
> include/acpi/acpi_bus.h | 1 +
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index c9e4190316ef..55626925261c 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2093,6 +2093,21 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
> device->handler->hotplug.notify_online(device);
> }
>
> +static int __acpi_dev_get_dependent_dev(struct acpi_dep_data *dep, void *data)
> +{
> + struct acpi_device *adev;
> + int ret;
> +
> + ret = acpi_bus_get_device(dep->consumer, &adev);
> + if (ret)
> + /* If we don't find an adev then we want to continue parsing */
> + return 0;
> +
> + *(struct acpi_device **)data = adev;
Hmm... I'm wondering if
*(void **data) = adev;
will compile and work.
But on second thought the current code is more specific and explicit,
which is good.
> +
> + return 1;
> +}
> +
> static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
> void *data)
> {
> @@ -2145,6 +2160,25 @@ void acpi_dev_flag_dependency_met(acpi_handle handle)
> }
> EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
>
> +/**
> + * acpi_dev_get_dependent_dev - Return ACPI device dependent on @adev
> + * @adev: Pointer to the dependee device
> + *
> + * Returns the first &struct acpi_device which declares itself dependent on
> + * @adev via the _DEP buffer, parsed from the acpi_dep_list.
> + */
> +struct acpi_device *
> +acpi_dev_get_dependent_dev(struct acpi_device *supplier)
I believe it will be okay to have it on one line
> +{
> + struct acpi_device *adev = NULL;
> + acpi_walk_dep_device_list(supplier->handle,
> + __acpi_dev_get_dependent_dev, &adev);
Ditto.
> + return adev;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_get_dependent_dev);
> +
> /**
> * acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
> * @handle: Root of the namespace scope to scan.
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 91172af3a04d..5b14a9ae4ed5 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -690,6 +690,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
>
> void acpi_dev_flag_dependency_met(acpi_handle handle);
> +struct acpi_device *acpi_dev_get_dependent_dev(struct acpi_device *supplier);
> struct acpi_device *
> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
> struct acpi_device *
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod()
2021-02-22 13:07 ` [PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod() Daniel Scally
@ 2021-02-22 13:54 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-22 13:54 UTC (permalink / raw)
To: Daniel Scally, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On Mon, Feb 22, 2021 at 3:13 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> I need to be able to translate GPIO resources in an ACPI device's _CRS
I -> we
> into GPIO descriptor array. Those are represented in _CRS as a pathname
> to a GPIO device plus the pin's index number: this function is perfect
Which one? "the acpi_...() function"
> for that purpose.
>
> As it's currently only used internally within the GPIO layer, provide and
> export a wrapper function that additionally holds a reference to the GPIO
> device.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
after addressing above and beyond :-)
>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
>
> - Having realised that it wasn't taking a reference to the GPIO device,
> I decided the best thing to do was leave the existing function as-is
> (apart from renaming) and provide a wrapper that simply passes
> through to the original and takes a reference before returning the
> struct gpio_desc
>
> Because of the change to that functionality, I dropped the R-b's from
> the last version.
>
> drivers/gpio/gpiolib-acpi.c | 36 +++++++++++++++++++++++++++++++----
> include/linux/gpio/consumer.h | 7 +++++++
> 2 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index e4d728fda982..0cc7cc327757 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -102,7 +102,8 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> }
>
> /**
> - * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
> + * __acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
Can we rename it better, i.e. w/o __, like "acpi_gpio_pin_to_gpiod()" or so?
> + * GPIO API
> * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
> * @pin: ACPI GPIO pin number (0-based, controller-relative)
> *
> @@ -111,7 +112,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> * controller does not have GPIO chip registered at the moment. This is to
> * support probe deferral.
> */
> -static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> +static struct gpio_desc *__acpi_get_gpiod(char *path, int pin)
> {
> struct gpio_chip *chip;
> acpi_handle handle;
> @@ -128,6 +129,33 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> return gpiochip_get_desc(chip, pin);
> }
>
> +/**
> + * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with
> + * GPIO API, and hold a refcount to the GPIO device.
> + * @path: ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
> + * @pin: ACPI GPIO pin number (0-based, controller-relative)
> + * @label: Label to pass to gpiod_request()
> + *
> + * This function is a simple pass-through to __acpi_get_gpiod(), except that as
> + * it is intended for use outside of the GPIO layer (in a similar fashion to
> + * gpiod_get_index() for example) it also holds a reference to the GPIO device.
> + */
> +struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label)
Name better to reflect the action, perhaps
"acpi_gpio_get_and_request_desc()" or so.
> +{
> + struct gpio_desc *gpio = __acpi_get_gpiod(path, pin);
Please, split it, so the assignment goes...
> + int ret;
...here.
> + if (IS_ERR(gpio))
> + return gpio;
> +
> + ret = gpiod_request(gpio, label);
> + if (ret)
> + return ERR_PTR(ret);
> +
> + return gpio;
> +}
> +EXPORT_SYMBOL_GPL(acpi_get_gpiod);
> +
> static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> {
> struct acpi_gpio_event *event = data;
> @@ -689,8 +717,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
> if (pin_index >= agpio->pin_table_length)
> return 1;
>
> - lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
> - agpio->pin_table[pin_index]);
> + lookup->desc = __acpi_get_gpiod(agpio->resource_source.string_ptr,
> + agpio->pin_table[pin_index]);
> lookup->info.pin_config = agpio->pin_config;
> lookup->info.debounce = agpio->debounce_timeout;
> lookup->info.gpioint = gpioint;
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index ef49307611d2..6eee751f44dd 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -690,6 +690,8 @@ int devm_acpi_dev_add_driver_gpios(struct device *dev,
> const struct acpi_gpio_mapping *gpios);
> void devm_acpi_dev_remove_driver_gpios(struct device *dev);
>
> +struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label);
> +
> #else /* CONFIG_GPIOLIB && CONFIG_ACPI */
>
> struct acpi_device;
> @@ -708,6 +710,11 @@ static inline int devm_acpi_dev_add_driver_gpios(struct device *dev,
> }
> static inline void devm_acpi_dev_remove_driver_gpios(struct device *dev) {}
>
> +struct gpio_desc *acpi_get_gpiod(char *path, int pin, char *label)
> +{
> + return NULL;
> +}
> +
> #endif /* CONFIG_GPIOLIB && CONFIG_ACPI */
>
>
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
2021-02-22 13:07 ` [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver Daniel Scally
@ 2021-02-22 14:12 ` Andy Shevchenko
2021-02-22 22:37 ` Daniel Scally
2021-03-10 9:33 ` Lee Jones
1 sibling, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-22 14:12 UTC (permalink / raw)
To: Daniel Scally
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, andy.shevchenko, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> This driver only covered one scenario in which ACPI devices with _HID
> INT3472 are found, and its functionality has been taken over by the
> intel-skl-int3472 module, so remove it.
As long as patch 5 accepted
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
> - Replaced Kconfig dependencies with INTEL_SKL_INT3472 for the tps68470
> OpRegion and GPIO drivers.
>
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/gpio/Kconfig | 2 +-
> drivers/mfd/Kconfig | 18 --------
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---------------------------------------
> 5 files changed, 2 insertions(+), 118 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
>
> diff --git a/drivers/acpi/pmic/Kconfig b/drivers/acpi/pmic/Kconfig
> index 56bbcb2ce61b..f84b8f6038dc 100644
> --- a/drivers/acpi/pmic/Kconfig
> +++ b/drivers/acpi/pmic/Kconfig
> @@ -52,7 +52,7 @@ endif # PMIC_OPREGION
>
> config TPS68470_PMIC_OPREGION
> bool "ACPI operation region support for TPS68470 PMIC"
> - depends on MFD_TPS68470
> + depends on INTEL_SKL_INT3472
> help
> This config adds ACPI operation region support for TI TPS68470 PMIC.
> TPS68470 device is an advanced power management unit that powers
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index c70f46e80a3b..998898c72af8 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1343,7 +1343,7 @@ config GPIO_TPS65912
>
> config GPIO_TPS68470
> bool "TPS68470 GPIO"
> - depends on MFD_TPS68470
> + depends on INTEL_SKL_INT3472
> help
> Select this option to enable GPIO driver for the TPS68470
> chip family.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bdfce7b15621..9a1f648efde0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1520,24 +1520,6 @@ config MFD_TPS65217
> This driver can also be built as a module. If so, the module
> will be called tps65217.
>
> -config MFD_TPS68470
> - bool "TI TPS68470 Power Management / LED chips"
> - depends on ACPI && PCI && I2C=y
> - depends on I2C_DESIGNWARE_PLATFORM=y
> - select MFD_CORE
> - select REGMAP_I2C
> - help
> - If you say yes here you get support for the TPS68470 series of
> - Power Management / LED chips.
> -
> - These include voltage regulators, LEDs and other features
> - that are often used in portable devices.
> -
> - This option is a bool as it provides an ACPI operation
> - region, which must be available before any of the devices
> - using this are probed. This option also configures the
> - designware-i2c driver to be built-in, for the same reason.
> -
> config MFD_TI_LP873X
> tristate "TI LP873X Power Management IC"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 14fdb188af02..5994e812f479 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -105,7 +105,6 @@ obj-$(CONFIG_MFD_TPS65910) += tps65910.o
> obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> obj-$(CONFIG_MFD_TPS65912_SPI) += tps65912-spi.o
> -obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> obj-$(CONFIG_MENELAUS) += menelaus.o
>
> diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c
> deleted file mode 100644
> index 4a4df4ffd18c..000000000000
> --- a/drivers/mfd/tps68470.c
> +++ /dev/null
> @@ -1,97 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * TPS68470 chip Parent driver
> - *
> - * Copyright (C) 2017 Intel Corporation
> - *
> - * Authors:
> - * Rajmohan Mani <rajmohan.mani@intel.com>
> - * Tianshu Qiu <tian.shu.qiu@intel.com>
> - * Jian Xu Zheng <jian.xu.zheng@intel.com>
> - * Yuning Pu <yuning.pu@intel.com>
> - */
> -
> -#include <linux/acpi.h>
> -#include <linux/delay.h>
> -#include <linux/i2c.h>
> -#include <linux/init.h>
> -#include <linux/mfd/core.h>
> -#include <linux/mfd/tps68470.h>
> -#include <linux/regmap.h>
> -
> -static const struct mfd_cell tps68470s[] = {
> - { .name = "tps68470-gpio" },
> - { .name = "tps68470_pmic_opregion" },
> -};
> -
> -static const struct regmap_config tps68470_regmap_config = {
> - .reg_bits = 8,
> - .val_bits = 8,
> - .max_register = TPS68470_REG_MAX,
> -};
> -
> -static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
> -{
> - unsigned int version;
> - int ret;
> -
> - /* Force software reset */
> - ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
> - if (ret)
> - return ret;
> -
> - ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> - if (ret) {
> - dev_err(dev, "Failed to read revision register: %d\n", ret);
> - return ret;
> - }
> -
> - dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> -
> - return 0;
> -}
> -
> -static int tps68470_probe(struct i2c_client *client)
> -{
> - struct device *dev = &client->dev;
> - struct regmap *regmap;
> - int ret;
> -
> - regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> - if (IS_ERR(regmap)) {
> - dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> - PTR_ERR(regmap));
> - return PTR_ERR(regmap);
> - }
> -
> - i2c_set_clientdata(client, regmap);
> -
> - ret = tps68470_chip_init(dev, regmap);
> - if (ret < 0) {
> - dev_err(dev, "TPS68470 Init Error %d\n", ret);
> - return ret;
> - }
> -
> - ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, tps68470s,
> - ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> - if (ret < 0) {
> - dev_err(dev, "devm_mfd_add_devices failed: %d\n", ret);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static const struct acpi_device_id tps68470_acpi_ids[] = {
> - {"INT3472"},
> - {},
> -};
> -
> -static struct i2c_driver tps68470_driver = {
> - .driver = {
> - .name = "tps68470",
> - .acpi_match_table = tps68470_acpi_ids,
> - },
> - .probe_new = tps68470_probe,
> -};
> -builtin_i2c_driver(tps68470_driver);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (6 preceding siblings ...)
2021-02-22 13:11 ` [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
@ 2021-02-22 14:15 ` Andy Shevchenko
2021-03-04 13:37 ` Hans de Goede
8 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-22 14:15 UTC (permalink / raw)
To: Daniel Scally, Andy Shevchenko, Dmitry Torokhov, Guenter Roeck
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On Mon, Feb 22, 2021 at 3:11 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> 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
>
> v2 was here:
> https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrscally@gmail.com/
>
> Series level changelog:
>
> - Dropped the patch moving acpi_lpss_dep() to utils since it's not used
> in acpi_dev_get_dependent_dev() anymore.
> - Replaced it with a patch extending acpi_walk_dep_device_list() to be
> able to apply a given callback against each device in acpi_dep_list
> - Dropped the patch creating acpi_i2c_dev_name() and simply open coded
> that functionality.
>
> This has been tested on a number of devices, but currently **not** on a device
> designed for ChromeOS, which we ideally need to do to ensure no regression
> caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way
> you could help with that? Unfortunately, I don't have a device to test it on
> myself.
+Cc: Dmitry and Guenter. Guys, do you know by a chance who can help
with the above request from Daniel?
> Original cover letter:
>
> 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 available but have not so far been
> tested, so aren't part of this series.
>
> The existing mfd/tps68470.c driver being thus superseded, it is removed.
>
> Thanks
> Dan
>
> Daniel Scally (6):
> ACPI: scan: Extend acpi_walk_dep_device_list()
> ACPI: scan: Add function to fetch dependent of acpi device
> i2c: core: Add a format macro for I2C device names
> gpiolib: acpi: Export acpi_get_gpiod()
> platform/x86: Add intel_skl_int3472 driver
> mfd: tps68470: Remove tps68470 MFD driver
>
> MAINTAINERS | 5 +
> drivers/acpi/ec.c | 2 +-
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
> drivers/acpi/scan.c | 92 ++-
> drivers/gpio/Kconfig | 2 +-
> drivers/gpio/gpiolib-acpi.c | 38 +-
> drivers/i2c/i2c-core-acpi.c | 2 +-
> drivers/i2c/i2c-core-base.c | 4 +-
> drivers/mfd/Kconfig | 18 -
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---
> drivers/platform/surface/surface3_power.c | 2 +-
> drivers/platform/x86/Kconfig | 2 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel-int3472/Kconfig | 31 +
> drivers/platform/x86/intel-int3472/Makefile | 4 +
> .../intel-int3472/intel_skl_int3472_common.c | 106 ++++
> .../intel-int3472/intel_skl_int3472_common.h | 110 ++++
> .../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
> .../intel_skl_int3472_tps68470.c | 113 ++++
> include/acpi/acpi_bus.h | 8 +
> include/linux/acpi.h | 4 +-
> include/linux/gpio/consumer.h | 7 +
> include/linux/i2c.h | 3 +
> 25 files changed, 1100 insertions(+), 148 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
> create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
> create mode 100644 drivers/platform/x86/intel-int3472/Makefile
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 13:07 ` [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver Daniel Scally
2021-02-22 13:19 ` Daniel Scally
@ 2021-02-22 14:58 ` Andy Shevchenko
2021-02-22 22:35 ` Daniel Scally
2021-05-17 21:43 ` Daniel Scally
2021-02-23 20:04 ` Laurent Pinchart
2 siblings, 2 replies; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-22 14:58 UTC (permalink / raw)
To: Daniel Scally, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> ACPI devices with _HID INT3472 are currently matched to the tps68470
> driver, however this does not cover all situations in which that _HID
> occurs. We've encountered three possibilities:
>
> 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
> a physical TPS68470 device) that requires a GPIO and OpRegion driver
> 2. On devices designed for Windows, an ACPI device with _HID INT3472
> (again representing a physical TPS68470 device) which requires GPIO,
> Clock and Regulator drivers.
> 3. On other devices designed for Windows, an ACPI device with _HID
> INT3472 which does **not** represent a physical TPS68470, and is instead
> used as a dummy device to group some system GPIO lines which are meant
> to be consumed by the sensor that is dependent on this entry.
>
> This commit adds a new module, registering a platform driver to deal
> with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
> querying the CLDB buffer found against INT3472 entries to determine
> which is most appropriate.
Can you split CLK parts (and maybe regulators as well) to something
like intel_skl_int3472_clk.c?
...
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> + dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
Perhaps acpi_handle_err() et al. instead of dev_*(&adev->dev, ...)
where it's applicable?
...
> + if (obj->buffer.length > sizeof(*cldb)) {
> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
> + ret = -EINVAL;
ENOSPC? ENOMEM?
> + goto out_free_obj;
> + }
...
> +static int skl_int3472_init(void)
> +{
> + int ret = 0;
Redundant assignment.
> + ret = platform_driver_register(&int3472_discrete);
> + if (ret)
> + return ret;
> +
> + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
> + if (ret)
> + platform_driver_unregister(&int3472_discrete);
Not a fan of the above, but let's see what others will say...
> + return ret;
> +}
> +module_init(skl_int3472_init);
...
> +#include <linux/clk-provider.h>
This is definitely not for *.h. (Not all C files needed this)
> +#include <linux/gpio/machine.h>
Ditto.
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
Ditto.
...
> +/*
> + * 79234640-9e10-4fea-a5c1-b5aa8b19756f
> + * This _DSM GUID returns information about the GPIO lines mapped to a
> + * discrete INT3472 device. Function number 1 returns a count of the GPIO
> + * lines that are mapped. Subsequent functions return 32 bit ints encoding
> + * information about the GPIO line, including its purpose.
> + */
> +static const guid_t int3472_gpio_guid =
> + GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
uuid.h ?
...
> +/*
> + * The regulators have to have .ops to be valid, but the only ops we actually
> + * support are .enable and .disable which are handled via .ena_gpiod. Pass an
> + * empty struct to clear the check without lying about capabilities.
> + */
> +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 };
{ 0 } is implied by the static keyword and C standard.
...
> +static int skl_int3472_clk_prepare(struct clk_hw *hw)
> +{
> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> +
> + gpiod_set_value(clk->ena_gpio, 1);
> + if (clk->led_gpio)
Make it optional and drop this check. Same for other places of use of this GPIO.
> + gpiod_set_value(clk->led_gpio, 1);
> +
> + return 0;
> +}
...
> +static int skl_int3472_clk_enable(struct clk_hw *hw)
> +{
> + /*
> + * We're just turning a GPIO on to enable, which operation has the
> + * potential to sleep. Given enable cannot sleep, but prepare can,
> + * we toggle the GPIO in prepare instead. Thus, nothing to do here.
> + */
Missed . and / or () in some words? (Describing callbacks, personally
I use the form "->callback()" in such cases)
> + return 0;
> +}
...
> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
> +{
> + union acpi_object *obj;
> + unsigned int ret = 0;
unsigned for ret is unusual. Looking into the code, first of all it
doesn't need this assignment; second, it probably can gain a better
name: "frequency"?
> + obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
> + if (IS_ERR(obj))
> + return 0; /* report rate as 0 on error */
> +
> + if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
> + dev_err(int3472->dev, "The buffer is too small\n");
> + goto out_free_buff;
> + }
> +
> + ret = *(u32 *)(obj->buffer.pointer + CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET);
> +
> +out_free_buff:
> + kfree(obj);
> + return ret;
> +}
...
> + sensor_config = int3472->sensor_config;
> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
Hmm...
Would
if (IS_ERR_OR_NULL(sensor_config))
return 0;
if (!_maps)
return 0;
with respective comments working here?
> + const struct int3472_gpio_function_remap *remap =
> + sensor_config->function_maps;
Split assignment so we can see what is the initial for-loop iterator value.
> + for (; remap->documented; ++remap)
remap++
> + if (!strcmp(func, remap->documented)) {
> + func = remap->actual;
> + break;
> + }
> + }
> +
> + /* Functions mapped to NULL should not be mapped to the sensor */
> + if (!func)
> + return 0;
...
> +static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
> +{
> + struct clk_init_data init = {
> + .ops = &skl_int3472_clock_ops,
> + .flags = CLK_GET_RATE_NOCACHE,
> + };
> + int ret = 0;
> +
> + init.name = kasprintf(GFP_KERNEL, "%s-clk",
> + acpi_dev_name(int3472->adev));
devm_*() ? Or is the lifetime different?
> + if (!init.name)
> + return -ENOMEM;
> +
> + int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
> +
> + int3472->clock.clk_hw.init = &init;
> + int3472->clock.clk = clk_register(&int3472->adev->dev,
> + &int3472->clock.clk_hw);
> + if (IS_ERR(int3472->clock.clk)) {
> + ret = PTR_ERR(int3472->clock.clk);
> + goto out_free_init_name;
> + }
> +
> + int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
> + int3472->sensor_name);
> + if (!int3472->clock.cl) {
> + ret = -ENOMEM;
> + goto err_unregister_clk;
> + }
> +
> + goto out_free_init_name;
> +
> +err_unregister_clk:
> + clk_unregister(int3472->clock.clk);
> +out_free_init_name:
> + kfree(init.name);
> +
> + return ret;
> +}
...
> + sensor_config = int3472->sensor_config;
> + if (IS_ERR_OR_NULL(sensor_config)) {
> + dev_err(int3472->dev, "No sensor module config\n");
> + return PTR_ERR(sensor_config);
NULL -> 0. Is it okay?
> + }
...
> + int ret = 0;
Seems redundant assignment.
...
> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
> + return 1; /* Deliberately positive so parsing continues */
I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
spreading it over kernel code (yes, I know about one existing TS
case).
Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().
...
> + if (ret < 0 && ret != -EPROBE_DEFER)
> + dev_err(int3472->dev, err_msg);
dev_err_probe() will make the above conditional go away. And you may even do...
> + int3472->n_gpios++;
> + ACPI_FREE(obj);
> + return ret;
...here
return dev_err_probe(...);
...
> + struct list_head resource_list;
> + INIT_LIST_HEAD(&resource_list);
LIST_HEAD(resource_list);
will do two in one.
...
> + if (int3472->clock.ena_gpio) {
Not sure you need this here.
> + ret = skl_int3472_register_clock(int3472);
> + if (ret)
> + goto out_free_res_list;
> + } else {
> + if (int3472->clock.led_gpio)
Ditto.
> + dev_warn(int3472->dev,
> + "No clk GPIO. The privacy LED won't work\n");
> + }
...
> + /* Max num GPIOs we've seen plus a terminator */
> + int3472 = kzalloc(struct_size(int3472, gpios.table,
> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
Wonder of you can use devm_*() APIs in this function.
> + if (!int3472)
> + return -ENOMEM;
...
> + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> + if (IS_ERR_OR_NULL(int3472->sensor)) {
> + dev_err(&pdev->dev,
> + "INT3472 seems to have no dependents.\n");
> + ret = -ENODEV;
Don't shadow error code when you got IS_ERR() case.
> + goto err_free_int3472;
> + }
...
> +int skl_int3472_discrete_remove(struct platform_device *pdev)
> +{
> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
> + if (int3472->gpios.dev_id)
> + gpiod_remove_lookup_table(&int3472->gpios);
gpiod_remove_lookup_table() is now NULL-aware.
But in any case I guess you don't need the above check.
> + if (!IS_ERR(int3472->regulator.rdev))
> + regulator_unregister(int3472->regulator.rdev);
Shouldn't it be the pointer to the regulator itself?
> + if (!IS_ERR(int3472->clock.clk))
If you get it optional, you won't need this additional check.
> + clk_unregister(int3472->clock.clk);
> +
> + if (int3472->clock.cl)
> + clkdev_drop(int3472->clock.cl);
> +
> + gpiod_put(int3472->regulator.gpio);
> + gpiod_put(int3472->clock.ena_gpio);
> + gpiod_put(int3472->clock.led_gpio);
> +
> + acpi_dev_put(int3472->sensor);
> +
> + kfree(int3472->sensor_name);
> + kfree(int3472);
> +
> + return 0;
> +}
...
> + ret = skl_int3472_fill_cldb(adev, &cldb);
> + if (!ret && cldb.control_logic_type != 2) {
> + dev_err(&client->dev, "Unsupported control logic type %u\n",
> + cldb.control_logic_type);
> + return -EINVAL;
> + }
> +
> + if (ret)
> + cldb_present = false;
if (ret)
...
else if (...) {
...
return ...;
}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 14:58 ` Andy Shevchenko
@ 2021-02-22 22:35 ` Daniel Scally
2021-02-23 12:01 ` Andy Shevchenko
2021-05-17 21:43 ` Daniel Scally
1 sibling, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 22:35 UTC (permalink / raw)
To: Andy Shevchenko, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
Hi Andy - thanks for comments!
On 22/02/2021 14:58, Andy Shevchenko wrote:
> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>> ACPI devices with _HID INT3472 are currently matched to the tps68470
>> driver, however this does not cover all situations in which that _HID
>> occurs. We've encountered three possibilities:
>>
>> 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
>> a physical TPS68470 device) that requires a GPIO and OpRegion driver
>> 2. On devices designed for Windows, an ACPI device with _HID INT3472
>> (again representing a physical TPS68470 device) which requires GPIO,
>> Clock and Regulator drivers.
>> 3. On other devices designed for Windows, an ACPI device with _HID
>> INT3472 which does **not** represent a physical TPS68470, and is instead
>> used as a dummy device to group some system GPIO lines which are meant
>> to be consumed by the sensor that is dependent on this entry.
>>
>> This commit adds a new module, registering a platform driver to deal
>> with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
>> querying the CLDB buffer found against INT3472 entries to determine
>> which is most appropriate.
> Can you split CLK parts (and maybe regulators as well) to something
> like intel_skl_int3472_clk.c?
Sure, no problem
>
>> +#include <linux/acpi.h>
>> +#include <linux/i2c.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> + dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
> Perhaps acpi_handle_err() et al. instead of dev_*(&adev->dev, ...)
> where it's applicable?
Ah - yes, ok, thanks. TIL those exist
>> + if (obj->buffer.length > sizeof(*cldb)) {
>> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
>> + ret = -EINVAL;
> ENOSPC? ENOMEM?
I still think EINVAL actually, as in this case the problem isn't that
space couldn't be allocated but that the buffer in the SSDB is larger
than I expect it to be, which means the definition of it has changed /
this device isn't actually supported.
>> + ret = platform_driver_register(&int3472_discrete);
>> + if (ret)
>> + return ret;
>> +
>> + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
>> + if (ret)
>> + platform_driver_unregister(&int3472_discrete);
> Not a fan of the above, but let's see what others will say...
Yeah; happy to discuss this more if needed.
>> +#include <linux/clk-provider.h>
> This is definitely not for *.h. (Not all C files needed this)
>
>> +#include <linux/gpio/machine.h>
> Ditto.
>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
> Ditto.
Yep; I'll move them to *_clk.c and *_regulator.c files.
>> +static int skl_int3472_clk_prepare(struct clk_hw *hw)
>> +{
>> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> +
>> + gpiod_set_value(clk->ena_gpio, 1);
>> + if (clk->led_gpio)
> Make it optional and drop this check. Same for other places of use of this GPIO.
Oops, of course, thanks
>> +static int skl_int3472_clk_enable(struct clk_hw *hw)
>> +{
>> + /*
>> + * We're just turning a GPIO on to enable, which operation has the
>> + * potential to sleep. Given enable cannot sleep, but prepare can,
>> + * we toggle the GPIO in prepare instead. Thus, nothing to do here.
>> + */
> Missed . and / or () in some words? (Describing callbacks, personally
> I use the form "->callback()" in such cases)
OK, I'll fix the comment to match that style.
>> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
>> +{
>> + union acpi_object *obj;
>>
>> + unsigned int ret = 0;
> unsigned for ret is unusual. Looking into the code, first of all it
> doesn't need this assignment; second, it probably can gain a better
> name: "frequency"?
Yep ok, I'll rename to freq/frequency
>> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
> Hmm...
>
> Would
>
> if (IS_ERR_OR_NULL(sensor_config))
> return 0;
>
> if (!_maps)
> return 0;
>
> with respective comments working here?
No, because the absence of either sensor_config or
sensor_config->function_maps is not a failure mode. We only need to
provide sensor_configs for some platforms, and function_maps for even
fewer. So if that check is false, the rest of the function should still
execute.
>> +static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
>> +{
>> + struct clk_init_data init = {
>> + .ops = &skl_int3472_clock_ops,
>> + .flags = CLK_GET_RATE_NOCACHE,
>> + };
>> + int ret = 0;
>> +
>> + init.name = kasprintf(GFP_KERNEL, "%s-clk",
>> + acpi_dev_name(int3472->adev));
> devm_*() ? Or is the lifetime different?
No it's not; I'll use devm_*(), thanks
>> + sensor_config = int3472->sensor_config;
>> + if (IS_ERR_OR_NULL(sensor_config)) {
>> + dev_err(int3472->dev, "No sensor module config\n");
>> + return PTR_ERR(sensor_config);
> NULL -> 0. Is it okay?
Ah, no it's not - good catch thank you.
>> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
>> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
>> + return 1; /* Deliberately positive so parsing continues */
> I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
> spreading it over kernel code (yes, I know about one existing TS
> case).
> Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().
Sure, but I probably name it acpi_gpio_is_io_resource() - a function
named "get" which returns a bool seems a bit funny to me.
>> + if (ret < 0 && ret != -EPROBE_DEFER)
>> + dev_err(int3472->dev, err_msg);
> dev_err_probe() will make the above conditional go away. And you may even do...
Ah-ha - thought that must exist but couldn't find it - thank you.
>> + if (int3472->clock.ena_gpio) {
> Not sure you need this here.
We haven't seen a device that lacks a clock enable GPIO it's true, but
since all the other kinds seem optional it didn't seem impossible that
that one is optional too. I can remove if you prefer and we can just
deal with it when we encounter one like that though?
>> + /* Max num GPIOs we've seen plus a terminator */
>> + int3472 = kzalloc(struct_size(int3472, gpios.table,
>> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> Wonder of you can use devm_*() APIs in this function.
Yeah I can, I'll switch to that.
>> +int skl_int3472_discrete_remove(struct platform_device *pdev)
>> +{
>> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
>> + if (int3472->gpios.dev_id)
>> + gpiod_remove_lookup_table(&int3472->gpios);
> gpiod_remove_lookup_table() is now NULL-aware.
> But in any case I guess you don't need the above check.
Sorry; forgot to call out that I didn't follow that suggestion;
int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios
won't be NULL, even if I haven't filled anything in to there yet because
it failed before it got to that point. So, not sure that it quite works
there.
>
>> + if (!IS_ERR(int3472->regulator.rdev))
>> + regulator_unregister(int3472->regulator.rdev);
> Shouldn't it be the pointer to the regulator itself?
int3472->regulator is type struct int3472_gpio_regulator, the .rdev is
the normal regulator_dev
>
>> + if (!IS_ERR(int3472->clock.clk))
> If you get it optional, you won't need this additional check.
Yes - here it will definitely work; thanks, I'll add that patch
>> + ret = skl_int3472_fill_cldb(adev, &cldb);
>> + if (!ret && cldb.control_logic_type != 2) {
>> + dev_err(&client->dev, "Unsupported control logic type %u\n",
>> + cldb.control_logic_type);
>> + return -EINVAL;
>> + }
>> +
>> + if (ret)
>> + cldb_present = false;
> if (ret)
> ...
> else if (...) {
> ...
> return ...;
> }
Oh yeah...now you point that out I have no idea what I was thinking there...
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
2021-02-22 14:12 ` Andy Shevchenko
@ 2021-02-22 22:37 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 22:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, andy.shevchenko, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On 22/02/2021 14:12, Andy Shevchenko wrote:
> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>> This driver only covered one scenario in which ACPI devices with _HID
>> INT3472 are found, and its functionality has been taken over by the
>> intel-skl-int3472 module, so remove it.
> As long as patch 5 accepted
> Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Great - thank you! And likewise for the R-bs on the previous patches,
I'll follow all the comments for those
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 13:27 ` Hans de Goede
@ 2021-02-22 22:50 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-22 22:50 UTC (permalink / raw)
To: Hans de Goede, tfiga, sakari.ailus, rajmohan.mani, rjw, lenb,
mika.westerberg, linus.walleij, bgolaszewski, wsa, lee.jones
Cc: Andy Shevchenko, kieran.bingham+renesas, laurent.pinchart,
mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
Hi Hans, thanks for the input
On 22/02/2021 13:27, Hans de Goede wrote:
> Hi,
>
> On 2/22/21 2:19 PM, Daniel Scally wrote:
>> Hi all
>>
>> On 22/02/2021 13:07, Daniel Scally wrote:
>>> diff --git a/drivers/platform/x86/intel-int3472/Kconfig b/drivers/platform/x86/intel-int3472/Kconfig
>>> new file mode 100644
>>> index 000000000000..b94622245c21
>>> --- /dev/null
>>> +++ b/drivers/platform/x86/intel-int3472/Kconfig
>>> @@ -0,0 +1,31 @@
>>> +config INTEL_SKL_INT3472
>>> + tristate "Intel SkyLake ACPI INT3472 Driver"
>>> + depends on ACPI
>>> + depends on REGULATOR
>>> + depends on GPIOLIB
>>> + depends on COMMON_CLK && CLKDEV_LOOKUP
>>> + depends on I2C
>>> + select MFD_CORE
>>> + select REGMAP_I2C
>>> + help
>>> + This driver adds support for the INT3472 ACPI devices found on some
>>> + Intel SkyLake devices.
>>> +
>>> + The INT3472 is an Intel camera power controller, a logical device
>>> + found on some Skylake-based systems that can map to different
>>> + hardware devices depending on the platform. On machines
>>> + designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
>>> + machines designed for Windows, it maps to either a TP68470
>>> + camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
>>> + and power gates.
>>> +
>>> + If your device was designed for Chrome OS, this driver will provide
>>> + an ACPI OpRegion, which must be available before any of the devices
>>> + using it are probed. For this reason, you should select Y if your
>>> + device was designed for ChromeOS. For the same reason the
>>> + I2C_DESIGNWARE_PLATFORM option must be set to Y too.
>>> +
>>> + Say Y or M here if you have a SkyLake device designed for use
>>> + with Windows or ChromeOS. Say N here if you are not sure.
>>> +
>>> + The module will be named "intel-skl-int3472"
>> The Kconfig option for the existing tps68470 driver is a bool which
>> depends on I2C_DESIGNWARE_PLATFORM=y, giving the following reason:
>>
>> This option is a bool as it provides an ACPI operation
>> region, which must be available before any of the devices
>> using this are probed. This option also configures the
>> designware-i2c driver to be built-in, for the same reason.
>>
>> One problem I've faced is that that scenario only applies to some
>> devices that this new driver can support, so hard-coding it as built in
>> didn't make much sense. For that reason I opted to set it tristate, but
>> of course that issue still exists for ChromeOS devices where the
>> OpRegion will be registered. I opted for simply documenting that
>> requirement, as is done in aaac4a2eadaa6: "mfd: axp20x-i2c: Document
>> that this must be builtin on x86", but that's not entirely satisfactory.
>> Possible alternatives might be setting "depends on
>> I2C_DESIGNWARE_PLATFORM=y if CHROME_PLATFORMS" or something similar,
>> though of course the User would still have to realise they need to
>> build-in the INTEL_SKL_INT3472 Kconfig option too.
>>
>> Feedback around this issue would be particularly welcome, as I'm not
>> sure what the best approach might be.
> This is a tricky area, I actually wrote the "mfd: axp20x-i2c: Document
> that this must be builtin on x86" patch you refer to. At first I tried
> to express the dependency in Kconfig language but things got too complex
> and Kconfig sometimes became unhappy about circular deps (or something
> like that).
Yes, I had a go too; with similar results
> The most important thing here is to make sure that the generic configs
> shipped by distros get this right; and we can hope that people creating
> those configs at least read the help text...
>
> So all in all I believe that just documenting the requirement is fine.
OK - that's what I'm hoping is the consensus, as I don't think it can be
made _entirely_ seamless through dependencies or whatever anyway, in
which case documenting it seems like the cleanest approach to me.
>
> The alternative would be to just change I2C_DESIGNWARE_PLATFORM (and the
> core) to a bool, or at least make it not selectable as module when
> X86 and ACPI are set... That would be a bit of a big hammer but might
> not be the worst idea actually.
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 22:35 ` Daniel Scally
@ 2021-02-23 12:01 ` Andy Shevchenko
2021-02-23 13:06 ` Daniel Scally
0 siblings, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-23 12:01 UTC (permalink / raw)
To: Daniel Scally
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On Mon, Feb 22, 2021 at 10:35:44PM +0000, Daniel Scally wrote:
> On 22/02/2021 14:58, Andy Shevchenko wrote:
> > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
...
> >> + if (obj->buffer.length > sizeof(*cldb)) {
> >> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
> >> + ret = -EINVAL;
> > ENOSPC? ENOMEM?
>
> I still think EINVAL actually, as in this case the problem isn't that
> space couldn't be allocated but that the buffer in the SSDB is larger
> than I expect it to be, which means the definition of it has changed /
> this device isn't actually supported.
OK!
...
> >> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
> > Hmm...
> >
> > Would
> >
> > if (IS_ERR_OR_NULL(sensor_config))
> > return 0;
> >
> > if (!_maps)
> > return 0;
> >
> > with respective comments working here?
>
> No, because the absence of either sensor_config or
> sensor_config->function_maps is not a failure mode. We only need to
> provide sensor_configs for some platforms, and function_maps for even
> fewer. So if that check is false, the rest of the function should still
> execute.
I see, thanks for elaboration.
...
> >> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
> >> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
> >> + return 1; /* Deliberately positive so parsing continues */
> > I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
> > spreading it over kernel code (yes, I know about one existing TS
> > case).
> > Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().
>
> Sure, but I probably name it acpi_gpio_is_io_resource() - a function
> named "get" which returns a bool seems a bit funny to me.
But don't you need the resource itself?
You may extract and check resource at the same time as
acpi_gpio_get_irq_resource() does.
...
> >> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
> >> + if (int3472->gpios.dev_id)
> >> + gpiod_remove_lookup_table(&int3472->gpios);
> > gpiod_remove_lookup_table() is now NULL-aware.
> > But in any case I guess you don't need the above check.
>
> Sorry; forgot to call out that I didn't follow that suggestion;
> int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios
> won't be NULL, even if I haven't filled anything in to there yet because
> it failed before it got to that point. So, not sure that it quite works
> there.
I think if you initialize the ->list member you can remove without check.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-23 12:01 ` Andy Shevchenko
@ 2021-02-23 13:06 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-23 13:06 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
On 23/02/2021 12:01, Andy Shevchenko wrote:
>>>> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
>>>> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
>>>> + return 1; /* Deliberately positive so parsing continues */
>>> I don't like to lose control over ACPI_RESOURCE_TYPE_GPIO, i.e.
>>> spreading it over kernel code (yes, I know about one existing TS
>>> case).
>>> Consider to provide a helper in analogue to acpi_gpio_get_irq_resource().
>> Sure, but I probably name it acpi_gpio_is_io_resource() - a function
>> named "get" which returns a bool seems a bit funny to me.
> But don't you need the resource itself?
>
> You may extract and check resource at the same time as
> acpi_gpio_get_irq_resource() does.
Oh! Reading comprehension fail; I didn't notice it was returning the
pointer through agpio; you're right of course.
>
> ...
>
>>>> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
>>>> + if (int3472->gpios.dev_id)
>>>> + gpiod_remove_lookup_table(&int3472->gpios);
>>> gpiod_remove_lookup_table() is now NULL-aware.
>>> But in any case I guess you don't need the above check.
>> Sorry; forgot to call out that I didn't follow that suggestion;
>> int3472->gpios is a _struct_ rather than a pointer, so &int3472->gpios
>> won't be NULL, even if I haven't filled anything in to there yet because
>> it failed before it got to that point. So, not sure that it quite works
>> there.
> I think if you initialize the ->list member you can remove without check.
I'll give that a try - thanks
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 13:07 ` [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver Daniel Scally
2021-02-22 13:19 ` Daniel Scally
2021-02-22 14:58 ` Andy Shevchenko
@ 2021-02-23 20:04 ` Laurent Pinchart
2021-02-23 22:36 ` Daniel Scally
2 siblings, 1 reply; 44+ messages in thread
From: Laurent Pinchart @ 2021-02-23 20:04 UTC (permalink / raw)
To: Daniel Scally
Cc: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones, andy.shevchenko,
kieran.bingham+renesas, hdegoede, mgross, luzmaximilian,
robert.moore, erik.kaneda, me, linux-kernel, linux-acpi,
linux-gpio, linux-i2c, platform-driver-x86, devel
Hi Daniel,
Thank you for the patch.
On Mon, Feb 22, 2021 at 01:07:34PM +0000, Daniel Scally wrote:
> ACPI devices with _HID INT3472 are currently matched to the tps68470
> driver, however this does not cover all situations in which that _HID
> occurs. We've encountered three possibilities:
>
> 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing
> a physical TPS68470 device) that requires a GPIO and OpRegion driver
> 2. On devices designed for Windows, an ACPI device with _HID INT3472
> (again representing a physical TPS68470 device) which requires GPIO,
> Clock and Regulator drivers.
> 3. On other devices designed for Windows, an ACPI device with _HID
> INT3472 which does **not** represent a physical TPS68470, and is instead
> used as a dummy device to group some system GPIO lines which are meant
> to be consumed by the sensor that is dependent on this entry.
>
> This commit adds a new module, registering a platform driver to deal
> with the 3rd scenario plus an i2c driver to deal with #1 and #2, by
> querying the CLDB buffer found against INT3472 entries to determine
> which is most appropriate.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
>
> - Added clk_recalc_rate() operation to the clk that's registered, plus
> some associated functions / revisions.
> - Moved source and header files to their own folder within platform/x86
> - Switched the GPIO toggling for the clock from enable/disable to
> prepare/unprepare to avoid any sleep problems
> - Switched handling of the privacy LED GPIO from map-to-sensor to being
> toggled along with the clk enable GPIO; register the clock in
> intel_skl_int3472_parse_crs() instead of during handle_gpio_resources
> - Better commenting in a lot of places
> - Used the sensor_name formed from acpi_dev_name(int3472->sensor) and
> the i2c name format macro as the dev_id in regulator init data rather
> than hardcoding an instance name.
> - Fetched the sensor module config a single time rather than once per
> GPIO
> - Switched int3472-tps68470 driver to use MFD framework properly rather
> than open coding the same functionality
> - A myriad of other fixes too minor to call out.
>
> Suggested changes (from Andy) that I didn't follow:
>
> - Using clk-gpio.c as a library: The requirement to add clk_recalc_rate
> so that clk_get_rate() would be supported, along with driving the
> privacy LED on clk_enable() meant this wouldn't work I think
> - Leave the MFD driver in its usual place, and prevent the INT3472 from
> probing as an i2c device from ACPI. I definitely see the argument for
> this, but in the end I think probably having all the code for the HID
> within a single place is probably a bit preferable.
>
> MAINTAINERS | 5 +
> drivers/platform/x86/Kconfig | 2 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel-int3472/Kconfig | 31 +
> drivers/platform/x86/intel-int3472/Makefile | 4 +
> .../intel-int3472/intel_skl_int3472_common.c | 106 ++++
> .../intel-int3472/intel_skl_int3472_common.h | 110 ++++
> .../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
> .../intel_skl_int3472_tps68470.c | 113 ++++
> 9 files changed, 964 insertions(+)
> create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
> create mode 100644 drivers/platform/x86/intel-int3472/Makefile
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a091b496fdd8..cf44b3e77b90 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9147,6 +9147,11 @@ S: Maintained
> F: arch/x86/include/asm/intel_scu_ipc.h
> F: drivers/platform/x86/intel_scu_*
>
> +INTEL SKYLAKE INT3472 ACPI DEVICE DRIVER
> +M: Daniel Scally <djrscally@gmail.com>
> +S: Maintained
> +F: drivers/platform/x86/intel-int3472/intel_skl_int3472_*
> +
> INTEL SPEED SELECT TECHNOLOGY
> M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> L: platform-driver-x86@vger.kernel.org
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 91e6176cdfbd..9739d30951b6 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -844,6 +844,8 @@ config INTEL_CHT_INT33FE
> device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m
> for Type-C device.
>
> +source "drivers/platform/x86/intel-int3472/Kconfig"
> +
> config INTEL_HID_EVENT
> tristate "INTEL HID Event"
> depends on ACPI
> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
> index 581475f59819..2293b6c3d1c2 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o
> obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o
> obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o
> obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o
> +obj-$(CONFIG_INTEL_SKL_INT3472) += intel-int3472/
> obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o
>
> # MSI
> diff --git a/drivers/platform/x86/intel-int3472/Kconfig b/drivers/platform/x86/intel-int3472/Kconfig
> new file mode 100644
> index 000000000000..b94622245c21
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/Kconfig
> @@ -0,0 +1,31 @@
> +config INTEL_SKL_INT3472
> + tristate "Intel SkyLake ACPI INT3472 Driver"
> + depends on ACPI
> + depends on REGULATOR
> + depends on GPIOLIB
> + depends on COMMON_CLK && CLKDEV_LOOKUP
> + depends on I2C
Maybe these could be sorted alphabetically ?
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + This driver adds support for the INT3472 ACPI devices found on some
> + Intel SkyLake devices.
> +
> + The INT3472 is an Intel camera power controller, a logical device
> + found on some Skylake-based systems that can map to different
> + hardware devices depending on the platform. On machines
> + designed for Chrome OS, it maps to a TPS68470 camera PMIC. On
> + machines designed for Windows, it maps to either a TP68470
> + camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs
> + and power gates.
> +
> + If your device was designed for Chrome OS, this driver will provide
> + an ACPI OpRegion, which must be available before any of the devices
> + using it are probed. For this reason, you should select Y if your
> + device was designed for ChromeOS. For the same reason the
> + I2C_DESIGNWARE_PLATFORM option must be set to Y too.
> +
> + Say Y or M here if you have a SkyLake device designed for use
> + with Windows or ChromeOS. Say N here if you are not sure.
> +
> + The module will be named "intel-skl-int3472"
> diff --git a/drivers/platform/x86/intel-int3472/Makefile b/drivers/platform/x86/intel-int3472/Makefile
> new file mode 100644
> index 000000000000..c887ee7d52ca
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/Makefile
> @@ -0,0 +1,4 @@
> +obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472.o
> +intel_skl_int3472-objs := intel_skl_int3472_common.o \
> + intel_skl_int3472_discrete.o \
> + intel_skl_int3472_tps68470.o
> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> new file mode 100644
> index 000000000000..f61166b6c497
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +
> +#include <linux/acpi.h>
> +#include <linux/i2c.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include "intel_skl_int3472_common.h"
> +
> +union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev,
> + char *id)
> +{
> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> + acpi_handle handle = adev->handle;
> + union acpi_object *obj;
> + acpi_status status;
> +
> + status = acpi_evaluate_object(handle, id, NULL, &buffer);
> + if (ACPI_FAILURE(status))
> + return ERR_PTR(-ENODEV);
> +
> + obj = buffer.pointer;
> + if (!obj)
> + return ERR_PTR(-ENODEV);
> +
> + if (obj->type != ACPI_TYPE_BUFFER) {
> + dev_err(&adev->dev, "%s object is not an ACPI buffer\n", id);
> + kfree(obj);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return obj;
> +}
> +
> +int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb)
> +{
> + union acpi_object *obj;
> + int ret = 0;
> +
> + obj = skl_int3472_get_acpi_buffer(adev, "CLDB");
> + if (IS_ERR(obj))
> + return PTR_ERR(obj);
> +
> + if (obj->buffer.length > sizeof(*cldb)) {
> + dev_err(&adev->dev, "The CLDB buffer is too large\n");
> + ret = -EINVAL;
> + goto out_free_obj;
> + }
> +
> + memcpy(cldb, obj->buffer.pointer, obj->buffer.length);
> +
> +out_free_obj:
> + kfree(obj);
> + return ret;
> +}
> +
> +static const struct acpi_device_id int3472_device_id[] = {
> + { "INT3472", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, int3472_device_id);
> +
> +static struct platform_driver int3472_discrete = {
> + .driver = {
> + .name = "int3472-discrete",
> + .acpi_match_table = int3472_device_id,
> + },
> + .probe = skl_int3472_discrete_probe,
> + .remove = skl_int3472_discrete_remove,
> +};
> +
> +static struct i2c_driver int3472_tps68470 = {
> + .driver = {
> + .name = "int3472-tps68470",
> + .acpi_match_table = int3472_device_id,
> + },
> + .probe_new = skl_int3472_tps68470_probe,
> +};
> +
> +static int skl_int3472_init(void)
> +{
> + int ret = 0;
> +
> + ret = platform_driver_register(&int3472_discrete);
> + if (ret)
> + return ret;
> +
> + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470);
> + if (ret)
> + platform_driver_unregister(&int3472_discrete);
> +
> + return ret;
> +}
> +module_init(skl_int3472_init);
> +
> +static void skl_int3472_exit(void)
> +{
> + platform_driver_unregister(&int3472_discrete);
> + i2c_del_driver(&int3472_tps68470);
> +}
> +module_exit(skl_int3472_exit);
> +
> +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver");
> +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> new file mode 100644
> index 000000000000..9169356cd522
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +
> +#ifndef _INTEL_SKL_INT3472_H
> +#define _INTEL_SKL_INT3472_H
> +
> +#include <linux/clk-provider.h>
> +#include <linux/gpio/machine.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/types.h>
> +
> +/* PMIC GPIO Types */
> +#define INT3472_GPIO_TYPE_RESET 0x00
> +#define INT3472_GPIO_TYPE_POWERDOWN 0x01
> +#define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b
> +#define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c
> +#define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d
> +
> +#define INT3472_PDEV_MAX_NAME_LEN 23
> +#define INT3472_MAX_SENSOR_GPIOS 3
> +
> +#define GPIO_REGULATOR_NAME_LENGTH 21
> +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9
> +
> +#define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET 86
> +
> +#define INT3472_REGULATOR(_name, _supply, _ops) \
> + (const struct regulator_desc) { \
> + .name = _name, \
> + .supply_name = _supply, \
> + .type = REGULATOR_VOLTAGE, \
> + .ops = _ops, \
> + .owner = THIS_MODULE, \
> + }
> +
> +#define to_int3472_clk(hw) \
> + container_of(hw, struct int3472_gpio_clock, clk_hw)
> +
> +#define to_int3472_device(clk) \
> + container_of(clk, struct int3472_discrete_device, clock)
> +
> +struct platform_device;
> +struct i2c_client;
> +struct acpi_device;
Alphabetical order ?
> +
> +struct int3472_cldb {
> + u8 version;
> + /*
> + * control logic type
> + * 0: UNKNOWN
> + * 1: DISCRETE(CRD-D)
> + * 2: PMIC TPS68470
> + * 3: PMIC uP6641
> + */
> + u8 control_logic_type;
> + u8 control_logic_id;
> + u8 sensor_card_sku;
> + u8 reserved[28];
> +};
> +
> +struct int3472_gpio_function_remap {
> + char *documented;
> + char *actual;
const char for both ?
> +};
> +
> +struct int3472_sensor_config {
> + const char *sensor_module_name;
> + struct regulator_consumer_supply supply_map;
> + const struct int3472_gpio_function_remap *function_maps;
> +};
> +
> +struct int3472_discrete_device {
> + struct acpi_device *adev;
> + struct device *dev;
> + struct acpi_device *sensor;
> + const char *sensor_name;
> +
> + struct int3472_sensor_config *sensor_config;
const
> +
> + struct int3472_gpio_regulator {
> + char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
> + char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
> + struct gpio_desc *gpio;
> + struct regulator_dev *rdev;
> + struct regulator_desc rdesc;
> + } regulator;
> +
> + struct int3472_gpio_clock {
> + struct clk *clk;
> + struct clk_hw clk_hw;
> + struct clk_lookup *cl;
> + struct gpio_desc *ena_gpio;
> + struct gpio_desc *led_gpio;
> + u32 frequency;
> + } clock;
> +
> + unsigned int n_gpios; /* how many GPIOs have we seen */
> + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
> + struct gpiod_lookup_table gpios;
> +};
> +
> +int skl_int3472_discrete_probe(struct platform_device *pdev);
> +int skl_int3472_discrete_remove(struct platform_device *pdev);
> +int skl_int3472_tps68470_probe(struct i2c_client *client);
> +union acpi_object *skl_int3472_get_acpi_buffer(struct acpi_device *adev,
> + char *id);
> +int skl_int3472_fill_cldb(struct acpi_device *adev, struct int3472_cldb *cldb);
> +
> +#endif
> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> new file mode 100644
> index 000000000000..40652161bbbf
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> @@ -0,0 +1,592 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +
> +#include <linux/acpi.h>
> +#include <linux/clkdev.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/overflow.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/slab.h>
> +
> +#include "intel_skl_int3472_common.h"
> +
> +/*
> + * 79234640-9e10-4fea-a5c1-b5aa8b19756f
> + * This _DSM GUID returns information about the GPIO lines mapped to a
> + * discrete INT3472 device. Function number 1 returns a count of the GPIO
> + * lines that are mapped. Subsequent functions return 32 bit ints encoding
> + * information about the GPIO line, including its purpose.
> + */
> +static const guid_t int3472_gpio_guid =
> + GUID_INIT(0x79234640, 0x9e10, 0x4fea,
> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f);
> +
> +/*
> + * 822ace8f-2814-4174-a56b-5f029fe079ee
> + * This _DSM GUID returns a string from the sensor device, which acts as a
> + * module identifier.
> + */
> +static const guid_t cio2_sensor_module_guid =
> + GUID_INIT(0x822ace8f, 0x2814, 0x4174,
> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee);
> +
> +/*
> + * Here follows platform specific mapping information that we can pass to
> + * the functions mapping resources to the sensors. Where the sensors have
> + * a power enable pin defined in DSDT we need to provide a supply name so
> + * the sensor drivers can find the regulator. The device name will be derived
> + * from the sensor's ACPI device within the code. Optionally, we can provide a
> + * NULL terminated array of function name mappings to deal with any platform
> + * specific deviations from the documented behaviour of GPIOs.
> + *
> + * Map a GPIO function name to NULL to prevent the driver from mapping that
> + * GPIO at all.
> + */
> +
> +static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
> + { "reset", NULL },
> + { "powerdown", "reset" },
> + { }
> +};
> +
> +static struct int3472_sensor_config int3472_sensor_configs[] = {
This should be static const (and there will be some fallout due to that,
as skl_int3472_register_regulator() modifies the supply_map, so I think
you'll have a copy of supply_map in int3472_discrete_device).
> + /* Lenovo Miix 510-12ISK - OV2680, Front */
> + { "GNDF140809R", { 0 }, ov2680_gpio_function_remaps},
Missing space before } (and below too).
> + /* Lenovo Miix 510-12ISK - OV5648, Rear */
> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", NULL), NULL},
> + /* Surface Go 1&2 - OV5693, Front */
> + { "YHCU", REGULATOR_SUPPLY("avdd", NULL), NULL},
> +};
> +
> +/*
> + * The regulators have to have .ops to be valid, but the only ops we actually
> + * support are .enable and .disable which are handled via .ena_gpiod. Pass an
> + * empty struct to clear the check without lying about capabilities.
> + */
> +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 };
> +
> +static int skl_int3472_clk_prepare(struct clk_hw *hw)
> +{
> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> +
> + gpiod_set_value(clk->ena_gpio, 1);
> + if (clk->led_gpio)
> + gpiod_set_value(clk->led_gpio, 1);
> +
> + return 0;
> +}
> +
> +static void skl_int3472_clk_unprepare(struct clk_hw *hw)
> +{
> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> +
> + gpiod_set_value(clk->ena_gpio, 0);
> + if (clk->led_gpio)
> + gpiod_set_value(clk->led_gpio, 0);
> +}
> +
> +static int skl_int3472_clk_enable(struct clk_hw *hw)
> +{
> + /*
> + * We're just turning a GPIO on to enable, which operation has the
> + * potential to sleep. Given enable cannot sleep, but prepare can,
> + * we toggle the GPIO in prepare instead. Thus, nothing to do here.
> + */
> + return 0;
> +}
> +
> +static void skl_int3472_clk_disable(struct clk_hw *hw)
> +{
> + /* Likewise, nothing to do here... */
> +}
> +
> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
> +{
> + union acpi_object *obj;
> + unsigned int ret = 0;
> +
> + obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
> + if (IS_ERR(obj))
> + return 0; /* report rate as 0 on error */
> +
> + if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
Should we define an ssdb structure instead of peeking into the buffer
with an offset ?
> + dev_err(int3472->dev, "The buffer is too small\n");
> + goto out_free_buff;
> + }
> +
> + ret = *(u32 *)(obj->buffer.pointer + CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET);
> +
> +out_free_buff:
> + kfree(obj);
> + return ret;
> +}
> +
> +static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> + struct int3472_discrete_device *int3472 = to_int3472_device(clk);
> +
> + return int3472->clock.frequency;
Maybe just
struct int3472_gpio_clock *clk = to_int3472_clk(hw);
return clk->frequency;
> +}
> +
> +static const struct clk_ops skl_int3472_clock_ops = {
> + .prepare = skl_int3472_clk_prepare,
> + .unprepare = skl_int3472_clk_unprepare,
> + .enable = skl_int3472_clk_enable,
> + .disable = skl_int3472_clk_disable,
> + .recalc_rate = skl_int3472_clk_recalc_rate,
> +};
> +
> +static struct int3472_sensor_config *
> +skl_int3472_get_sensor_module_config(struct int3472_discrete_device *int3472)
> +{
> + struct int3472_sensor_config *ret;
> + union acpi_object *obj;
> + unsigned int i;
> +
> + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle,
> + &cio2_sensor_module_guid, 0x00,
> + 0x01, NULL, ACPI_TYPE_STRING);
> +
> + if (!obj) {
> + dev_err(int3472->dev,
> + "Failed to get sensor module string from _DSM\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + if (obj->string.type != ACPI_TYPE_STRING) {
> + dev_err(int3472->dev,
> + "Sensor _DSM returned a non-string value\n");
> + ret = ERR_PTR(-EINVAL);
> + goto out_free_obj;
> + }
> +
> + ret = NULL;
> + for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); i++) {
> + if (!strcmp(int3472_sensor_configs[i].sensor_module_name,
> + obj->string.pointer)) {
> + ret = &int3472_sensor_configs[i];
> + break;
> + }
> + }
> +
> +out_free_obj:
> + ACPI_FREE(obj);
> + return ret;
> +}
> +
> +static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int3472,
> + struct acpi_resource *ares,
> + char *func, u32 polarity)
> +{
> + char *path = ares->data.gpio.resource_source.string_ptr;
> + const struct int3472_sensor_config *sensor_config;
> + struct gpiod_lookup *table_entry;
> + struct acpi_device *adev;
> + acpi_handle handle;
> + acpi_status status;
> + int ret;
> +
> + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) {
> + dev_warn(int3472->dev, "Too many GPIOs mapped\n");
> + return -EINVAL;
> + }
> +
> + sensor_config = int3472->sensor_config;
> + if (!IS_ERR_OR_NULL(sensor_config) && sensor_config->function_maps) {
> + const struct int3472_gpio_function_remap *remap =
> + sensor_config->function_maps;
> +
> + for (; remap->documented; ++remap)
> + if (!strcmp(func, remap->documented)) {
> + func = remap->actual;
> + break;
> + }
No strictly required, but I'd use curly braces for the for loop. Up to
you.
> + }
> +
> + /* Functions mapped to NULL should not be mapped to the sensor */
> + if (!func)
> + return 0;
> +
> + status = acpi_get_handle(NULL, path, &handle);
> + if (ACPI_FAILURE(status))
> + return -EINVAL;
> +
> + ret = acpi_bus_get_device(handle, &adev);
> + if (ret)
> + return -ENODEV;
> +
> + table_entry = &int3472->gpios.table[int3472->n_sensor_gpios];
> + table_entry->key = acpi_dev_name(adev);
> + table_entry->chip_hwnum = ares->data.gpio.pin_table[0];
> + table_entry->con_id = func;
> + table_entry->idx = 0;
> + table_entry->flags = polarity;
> +
> + int3472->n_sensor_gpios++;
> +
> + return 0;
> +}
> +
> +static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472,
> + struct acpi_resource *ares, u8 type)
> +{
> + char *path = ares->data.gpio.resource_source.string_ptr;
> + struct gpio_desc *gpio;
> +
> + switch (type) {
> + case INT3472_GPIO_TYPE_CLK_ENABLE:
> + gpio = acpi_get_gpiod(path, ares->data.gpio.pin_table[0],
> + "int3472,clk-enable");
> + if (IS_ERR(gpio))
> + return (PTR_ERR(gpio));
> +
> + int3472->clock.ena_gpio = gpio;
> + break;
> + case INT3472_GPIO_TYPE_PRIVACY_LED:
> + gpio = acpi_get_gpiod(path, ares->data.gpio.pin_table[0],
> + "int3472,privacy-led");
> + if (IS_ERR(gpio))
> + return (PTR_ERR(gpio));
> +
> + int3472->clock.led_gpio = gpio;
> + break;
> + default:
> + dev_err(int3472->dev, "Invalid GPIO type 0x%hx for clock\n",
> + type);
type being a u8, it should be %hhx, but I'd just use 0x%02x.
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int skl_int3472_register_clock(struct int3472_discrete_device *int3472)
> +{
> + struct clk_init_data init = {
> + .ops = &skl_int3472_clock_ops,
> + .flags = CLK_GET_RATE_NOCACHE,
> + };
> + int ret = 0;
> +
> + init.name = kasprintf(GFP_KERNEL, "%s-clk",
> + acpi_dev_name(int3472->adev));
> + if (!init.name)
> + return -ENOMEM;
> +
> + int3472->clock.frequency = skl_int3472_get_clk_frequency(int3472);
> +
> + int3472->clock.clk_hw.init = &init;
> + int3472->clock.clk = clk_register(&int3472->adev->dev,
> + &int3472->clock.clk_hw);
> + if (IS_ERR(int3472->clock.clk)) {
> + ret = PTR_ERR(int3472->clock.clk);
> + goto out_free_init_name;
> + }
> +
> + int3472->clock.cl = clkdev_create(int3472->clock.clk, NULL,
> + int3472->sensor_name);
> + if (!int3472->clock.cl) {
> + ret = -ENOMEM;
> + goto err_unregister_clk;
> + }
> +
> + goto out_free_init_name;
> +
> +err_unregister_clk:
> + clk_unregister(int3472->clock.clk);
> +out_free_init_name:
> + kfree(init.name);
> +
> + return ret;
> +}
> +
> +static int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
> + struct acpi_resource *ares)
> +{
> + char *path = ares->data.gpio.resource_source.string_ptr;
> + struct int3472_sensor_config *sensor_config;
> + struct regulator_init_data init_data = { };
> + struct regulator_config cfg = { };
> + int ret;
> +
> + sensor_config = int3472->sensor_config;
> + if (IS_ERR_OR_NULL(sensor_config)) {
> + dev_err(int3472->dev, "No sensor module config\n");
> + return PTR_ERR(sensor_config);
> + }
> +
> + if (!sensor_config->supply_map.supply) {
> + dev_err(int3472->dev, "No supply name defined\n");
> + return -ENODEV;
> + }
> +
> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> + init_data.num_consumer_supplies = 1;
> + sensor_config->supply_map.dev_name = int3472->sensor_name;
> + init_data.consumer_supplies = &sensor_config->supply_map;
> +
> + snprintf(int3472->regulator.regulator_name,
> + sizeof(int3472->regulator.regulator_name), "%s-regulator",
> + acpi_dev_name(int3472->adev));
> + snprintf(int3472->regulator.supply_name,
> + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
> +
> + int3472->regulator.rdesc = INT3472_REGULATOR(
> + int3472->regulator.regulator_name,
> + int3472->regulator.supply_name,
> + &int3472_gpio_regulator_ops);
> +
> + int3472->regulator.gpio = acpi_get_gpiod(path,
> + ares->data.gpio.pin_table[0],
> + "int3472,regulator");
> + if (IS_ERR(int3472->regulator.gpio)) {
> + dev_err(int3472->dev, "Failed to get regulator GPIO lines\n");
s/lines/line/ (sorry, it was a typo in my review of v2)
> + return PTR_ERR(int3472->regulator.gpio);
> + }
> +
> + cfg.dev = &int3472->adev->dev;
> + cfg.init_data = &init_data;
> + cfg.ena_gpiod = int3472->regulator.gpio;
> +
> + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc,
> + &cfg);
> + if (IS_ERR(int3472->regulator.rdev)) {
> + ret = PTR_ERR(int3472->regulator.rdev);
> + goto err_free_gpio;
> + }
> +
> + return 0;
> +
> +err_free_gpio:
> + gpiod_put(int3472->regulator.gpio);
> +
> + return ret;
> +}
> +
> +/**
> + * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor
> + * @ares: A pointer to a &struct acpi_resource
> + * @data: A pointer to a &struct int3472_discrete_device
> + *
> + * This function handles GPIO resources that are against an INT3472
> + * ACPI device, by checking the value of the corresponding _DSM entry.
> + * This will return a 32bit int, where the lowest byte represents the
> + * function of the GPIO pin:
> + *
> + * 0x00 Reset
> + * 0x01 Power down
> + * 0x0b Power enable
> + * 0x0c Clock enable
> + * 0x0d Privacy LED
> + *
> + * There are some known platform specific quirks where that does not quite
> + * hold up; for example where a pin with type 0x01 (Power down) is mapped to
> + * a sensor pin that performs a reset function or entries in _CRS and _DSM that
> + * do not actually correspond to a physical connection. These will be handled
> + * by the mapping sub-functions.
> + *
> + * GPIOs will either be mapped directly to the sensor device or else used
> + * to create clocks and regulators via the usual frameworks.
> + *
> + * Return:
> + * * 0 - When all resources found are handled properly.
> + * * -EINVAL - If the resource is not a GPIO IO resource
> + * * -ENODEV - If the resource has no corresponding _DSM entry
> + * * -Other - Errors propagated from one of the sub-functions.
> + */
> +static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares,
> + void *data)
> +{
> + struct int3472_discrete_device *int3472 = data;
> + u16 pin = ares->data.gpio.pin_table[0];
> + union acpi_object *obj;
> + char *err_msg;
> + int ret = 0;
> + u8 type;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO ||
> + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO)
> + return 1; /* Deliberately positive so parsing continues */
> +
> + /*
> + * n_gpios + 2 because the index of this _DSM function is 1-based and
> + * the first function is just a count.
> + */
> + obj = acpi_evaluate_dsm_typed(int3472->adev->handle,
> + &int3472_gpio_guid, 0x00,
> + int3472->n_gpios + 2,
> + NULL, ACPI_TYPE_INTEGER);
> +
> + if (!obj) {
> + dev_warn(int3472->dev, "No _DSM entry for GPIO pin %u\n", pin);
> + return 1;
> + }
> +
> + type = obj->integer.value & 0xff;
> +
> + switch (type) {
> + case INT3472_GPIO_TYPE_RESET:
> + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "reset",
> + GPIO_ACTIVE_LOW);
> + if (ret)
> + err_msg = "Failed to map reset pin to sensor\n";
> +
> + break;
> + case INT3472_GPIO_TYPE_POWERDOWN:
> + ret = skl_int3472_map_gpio_to_sensor(int3472, ares,
> + "powerdown",
> + GPIO_ACTIVE_LOW);
> + if (ret)
> + err_msg = "Failed to map powerdown pin to sensor\n";
> +
> + break;
> + case INT3472_GPIO_TYPE_CLK_ENABLE:
> + case INT3472_GPIO_TYPE_PRIVACY_LED:
> + ret = skl_int3472_map_gpio_to_clk(int3472, ares, type);
> + if (ret)
> + err_msg = "Failed to map GPIO to clock\n";
> +
> + break;
> + case INT3472_GPIO_TYPE_POWER_ENABLE:
> + ret = skl_int3472_register_regulator(int3472, ares);
> + if (ret)
> + err_msg = "Failed to map regulator to sensor\n";
> +
> + break;
> + default:
> + dev_warn(int3472->dev,
> + "GPIO type 0x%02x unknown; the sensor may not work\n",
> + type);
> + ret = 1;
> + break;
> + }
> +
> + if (ret < 0 && ret != -EPROBE_DEFER)
> + dev_err(int3472->dev, err_msg);
> +
> + int3472->n_gpios++;
> + ACPI_FREE(obj);
> +
> + return ret;
> +}
> +
> +static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
> +{
> + struct list_head resource_list;
> + int ret;
> +
> + INIT_LIST_HEAD(&resource_list);
> +
> + int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
I have forgotten some of the context I'm afraid :-/ Are there valid use
cases for not checking for an error here, or should we do so and drop
the error checks in other functions above ?
> +
> + ret = acpi_dev_get_resources(int3472->adev, &resource_list,
> + skl_int3472_handle_gpio_resources,
> + int3472);
> + if (ret)
> + goto out_free_res_list;
> +
> + if (int3472->clock.ena_gpio) {
> + ret = skl_int3472_register_clock(int3472);
> + if (ret)
> + goto out_free_res_list;
> + } else {
> + if (int3472->clock.led_gpio)
> + dev_warn(int3472->dev,
> + "No clk GPIO. The privacy LED won't work\n");
> + }
> +
> + int3472->gpios.dev_id = int3472->sensor_name;
> + gpiod_add_lookup_table(&int3472->gpios);
> +
> +out_free_res_list:
> + acpi_dev_free_resource_list(&resource_list);
> +
> + return ret;
> +}
> +
> +int skl_int3472_discrete_probe(struct platform_device *pdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + struct int3472_discrete_device *int3472;
> + struct int3472_cldb cldb;
> + int ret;
> +
> + ret = skl_int3472_fill_cldb(adev, &cldb);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
> + return ret;
> + }
> +
> + if (cldb.control_logic_type != 1) {
> + dev_err(&pdev->dev, "Unsupported control logic type %u\n",
> + cldb.control_logic_type);
> + return -EINVAL;
> + }
> +
> + /* Max num GPIOs we've seen plus a terminator */
> + int3472 = kzalloc(struct_size(int3472, gpios.table,
> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> + if (!int3472)
> + return -ENOMEM;
> +
> + int3472->adev = adev;
> + int3472->dev = &pdev->dev;
> + platform_set_drvdata(pdev, int3472);
> +
> + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> + if (IS_ERR_OR_NULL(int3472->sensor)) {
> + dev_err(&pdev->dev,
> + "INT3472 seems to have no dependents.\n");
> + ret = -ENODEV;
> + goto err_free_int3472;
> + }
> + get_device(&int3472->sensor->dev);
I see no corresponding put_device(), am I missing something ? I'm also
not sure why this is needed.
> +
> + int3472->sensor_name = kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT,
> + acpi_dev_name(int3472->sensor));
This needs a NULL check.
> +
> + ret = skl_int3472_parse_crs(int3472);
> + if (ret) {
> + skl_int3472_discrete_remove(pdev);
> + return ret;
> + }
> +
> + return 0;
> +
> +err_free_int3472:
> + kfree(int3472);
> + return ret;
> +}
> +
> +int skl_int3472_discrete_remove(struct platform_device *pdev)
> +{
> + struct int3472_discrete_device *int3472 = platform_get_drvdata(pdev);
> +
> + if (int3472->gpios.dev_id)
> + gpiod_remove_lookup_table(&int3472->gpios);
> +
> + if (!IS_ERR(int3472->regulator.rdev))
> + regulator_unregister(int3472->regulator.rdev);
> +
> + if (!IS_ERR(int3472->clock.clk))
> + clk_unregister(int3472->clock.clk);
> +
> + if (int3472->clock.cl)
> + clkdev_drop(int3472->clock.cl);
> +
> + gpiod_put(int3472->regulator.gpio);
> + gpiod_put(int3472->clock.ena_gpio);
> + gpiod_put(int3472->clock.led_gpio);
> +
> + acpi_dev_put(int3472->sensor);
> +
> + kfree(int3472->sensor_name);
> + kfree(int3472);
> +
> + return 0;
> +}
> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> new file mode 100644
> index 000000000000..d0d2391e263f
> --- /dev/null
> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Dan Scally <djrscally@gmail.com> */
> +
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/tps68470.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "intel_skl_int3472_common.h"
> +
> +static const struct mfd_cell tps68470_c[] = {
> + { .name = "tps68470-gpio" },
> + { .name = "tps68470_pmic_opregion" },
> +};
> +
> +static const struct mfd_cell tps68470_w[] = {
Maybe more explicit names than _c and _w could be nice ?
> + { .name = "tps68470-gpio" },
> + { .name = "tps68470-clk" },
> + { .name = "tps68470-regulator"},
Missing space before }.
> +};
> +
> +static const struct regmap_config tps68470_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = TPS68470_REG_MAX,
> +};
> +
> +static int tps68470_chip_init(struct device *dev, struct regmap *regmap)
> +{
> + unsigned int version;
> + int ret;
> +
> + /* Force software reset */
> + ret = regmap_write(regmap, TPS68470_REG_RESET, TPS68470_REG_RESET_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(regmap, TPS68470_REG_REVID, &version);
> + if (ret) {
> + dev_err(dev, "Failed to read revision register: %d\n", ret);
> + return ret;
> + }
> +
> + dev_info(dev, "TPS68470 REVID: 0x%02x\n", version);
> +
> + return 0;
> +}
> +
> +int skl_int3472_tps68470_probe(struct i2c_client *client)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&client->dev);
> + struct int3472_cldb cldb = { 0 };
> + bool cldb_present = true;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Failed to create regmap: %ld\n",
> + PTR_ERR(regmap));
> + return PTR_ERR(regmap);
> + }
> +
> + i2c_set_clientdata(client, regmap);
> +
> + ret = tps68470_chip_init(&client->dev, regmap);
> + if (ret < 0) {
> + dev_err(&client->dev, "TPS68470 init error %d\n", ret);
> + return ret;
> + }
> +
> + /*
> + * Check CLDB buffer against the PMIC's adev. If present, then we check
> + * the value of control_logic_type field and follow one of the
> + * following scenarios:
> + *
> + * 1. No CLDB - likely ACPI tables designed for ChromeOS. We
> + * create platform devices for the GPIOs and OpRegion drivers.
> + *
> + * 2. CLDB, with control_logic_type = 2 - probably ACPI tables
> + * made for Windows 2-in-1 platforms. Register pdevs for GPIO,
> + * Clock and Regulator drivers to bind to.
> + *
> + * 3. Any other value in control_logic_type, we should never have
> + * gotten to this point; fail probe and return.
> + */
> + ret = skl_int3472_fill_cldb(adev, &cldb);
> + if (!ret && cldb.control_logic_type != 2) {
> + dev_err(&client->dev, "Unsupported control logic type %u\n",
> + cldb.control_logic_type);
> + return -EINVAL;
> + }
> +
> + if (ret)
> + cldb_present = false;
> +
> + if (cldb_present)
> + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> + tps68470_w, ARRAY_SIZE(tps68470_w),
> + NULL, 0, NULL);
> + else
> + ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> + tps68470_c, ARRAY_SIZE(tps68470_c),
> + NULL, 0, NULL);
> +
> + if (ret) {
> + dev_err(&client->dev, "Failed to add MFD devices\n");
> + return ret;
> + }
> +
> + return 0;
> +}
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-23 20:04 ` Laurent Pinchart
@ 2021-02-23 22:36 ` Daniel Scally
2021-02-24 10:13 ` Laurent Pinchart
0 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-02-23 22:36 UTC (permalink / raw)
To: Laurent Pinchart
Cc: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones, andy.shevchenko,
kieran.bingham+renesas, hdegoede, mgross, luzmaximilian,
robert.moore, erik.kaneda, me, linux-kernel, linux-acpi,
linux-gpio, linux-i2c, platform-driver-x86, devel
Hi Laurent
On 23/02/2021 20:04, Laurent Pinchart wrote:
> +
> +/*
> + * Here follows platform specific mapping information that we can pass to
> + * the functions mapping resources to the sensors. Where the sensors have
> + * a power enable pin defined in DSDT we need to provide a supply name so
> + * the sensor drivers can find the regulator. The device name will be derived
> + * from the sensor's ACPI device within the code. Optionally, we can provide a
> + * NULL terminated array of function name mappings to deal with any platform
> + * specific deviations from the documented behaviour of GPIOs.
> + *
> + * Map a GPIO function name to NULL to prevent the driver from mapping that
> + * GPIO at all.
> + */
> +
> +static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
> + { "reset", NULL },
> + { "powerdown", "reset" },
> + { }
> +};
> +
> +static struct int3472_sensor_config int3472_sensor_configs[] = {
> This should be static const (and there will be some fallout due to that,
> as skl_int3472_register_regulator() modifies the supply_map, so I think
> you'll have a copy of supply_map in int3472_discrete_device).
Ack to all of the constness; you mentioned that last time too - not sure
how I missed doing those! I think I can just having a local struct
regulator_consumer_supply in skl_int3472_register_regulator and fill it
from int3472->sensor_config.supply_map
>> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
>> +{
>> + union acpi_object *obj;
>> + unsigned int ret = 0;
>> +
>> + obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
>> + if (IS_ERR(obj))
>> + return 0; /* report rate as 0 on error */
>> +
>> + if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
> Should we define an ssdb structure instead of peeking into the buffer
> with an offset ?
I thought about that, but in the end decided it didn't seem worth
defining the whole SSDB structure just to use one field. Particularly
since we use it in cio2-bridge already, so if we're going to do that it
really ought to just live in a header that's included in both - and that
seemed even less worthwhile.
I don't have a strong feeling though, so if you think it's better to
define the struct I'm happy to.
>> +static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>> + struct int3472_discrete_device *int3472 = to_int3472_device(clk);
>> +
>> + return int3472->clock.frequency;
> Maybe just
>
> struct int3472_gpio_clock *clk = to_int3472_clk(hw);
>
> return clk->frequency;
Oops, of course.
>> +static int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
>> + struct acpi_resource *ares)
>> +{
>> + char *path = ares->data.gpio.resource_source.string_ptr;
>> + struct int3472_sensor_config *sensor_config;
>> + struct regulator_init_data init_data = { };
>> + struct regulator_config cfg = { };
>> + int ret;
>> +
>> + sensor_config = int3472->sensor_config;
>> + if (IS_ERR_OR_NULL(sensor_config)) {
>> + dev_err(int3472->dev, "No sensor module config\n");
>> + return PTR_ERR(sensor_config);
>> + }
>> +
>> + if (!sensor_config->supply_map.supply) {
>> + dev_err(int3472->dev, "No supply name defined\n");
>> + return -ENODEV;
>> + }
>> +
>> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>> + init_data.num_consumer_supplies = 1;
>> + sensor_config->supply_map.dev_name = int3472->sensor_name;
>> + init_data.consumer_supplies = &sensor_config->supply_map;
>> +
>> + snprintf(int3472->regulator.regulator_name,
>> + sizeof(int3472->regulator.regulator_name), "%s-regulator",
>> + acpi_dev_name(int3472->adev));
>> + snprintf(int3472->regulator.supply_name,
>> + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
>> +
>> + int3472->regulator.rdesc = INT3472_REGULATOR(
>> + int3472->regulator.regulator_name,
>> + int3472->regulator.supply_name,
>> + &int3472_gpio_regulator_ops);
>> +
>> + int3472->regulator.gpio = acpi_get_gpiod(path,
>> + ares->data.gpio.pin_table[0],
>> + "int3472,regulator");
>> + if (IS_ERR(int3472->regulator.gpio)) {
>> + dev_err(int3472->dev, "Failed to get regulator GPIO lines\n");
> s/lines/line/ (sorry, it was a typo in my review of v2)
No problem!
>> +static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
>> +{
>> + struct list_head resource_list;
>> + int ret;
>> +
>> + INIT_LIST_HEAD(&resource_list);
>> +
>> + int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
> I have forgotten some of the context I'm afraid :-/ Are there valid use
> cases for not checking for an error here, or should we do so and drop
> the error checks in other functions above ?
Not all platforms need a sensor_config; only those which have either a
regulator pin or need a GPIO function to be remapped; the rest will do
without it.
So, we need to not check for an error here because the absence of a
sensor_config isn't necessarily an error, we won't know till later.
> +int skl_int3472_discrete_probe(struct platform_device *pdev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> + struct int3472_discrete_device *int3472;
> + struct int3472_cldb cldb;
> + int ret;
> +
> + ret = skl_int3472_fill_cldb(adev, &cldb);
> + if (ret) {
> + dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
> + return ret;
> + }
> +
> + if (cldb.control_logic_type != 1) {
> + dev_err(&pdev->dev, "Unsupported control logic type %u\n",
> + cldb.control_logic_type);
> + return -EINVAL;
> + }
> +
> + /* Max num GPIOs we've seen plus a terminator */
> + int3472 = kzalloc(struct_size(int3472, gpios.table,
> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> + if (!int3472)
> + return -ENOMEM;
> +
> + int3472->adev = adev;
> + int3472->dev = &pdev->dev;
> + platform_set_drvdata(pdev, int3472);
> +
> + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> + if (IS_ERR_OR_NULL(int3472->sensor)) {
> + dev_err(&pdev->dev,
> + "INT3472 seems to have no dependents.\n");
> + ret = -ENODEV;
> + goto err_free_int3472;
> + }
> + get_device(&int3472->sensor->dev);
> I see no corresponding put_device(), am I missing something ? I'm also
> not sure why this is needed.
>
The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
to be no acpi_dev_get() for some reason. We use the sensor acpi_device
to get the clock frequency, and to fetch the sensor module string, so I
thought it ought to hold a reference on those grounds.
>> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>> new file mode 100644
>> index 000000000000..d0d2391e263f
>> --- /dev/null
>> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>> @@ -0,0 +1,113 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Author: Dan Scally <djrscally@gmail.com> */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/tps68470.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +
>> +#include "intel_skl_int3472_common.h"
>> +
>> +static const struct mfd_cell tps68470_c[] = {
>> + { .name = "tps68470-gpio" },
>> + { .name = "tps68470_pmic_opregion" },
>> +};
>> +
>> +static const struct mfd_cell tps68470_w[] = {
> Maybe more explicit names than _c and _w could be nice ?
_chrome and _windows was in my mind - sound ok?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-23 22:36 ` Daniel Scally
@ 2021-02-24 10:13 ` Laurent Pinchart
2021-02-24 10:18 ` Andy Shevchenko
0 siblings, 1 reply; 44+ messages in thread
From: Laurent Pinchart @ 2021-02-24 10:13 UTC (permalink / raw)
To: Daniel Scally
Cc: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, lee.jones, andy.shevchenko,
kieran.bingham+renesas, hdegoede, mgross, luzmaximilian,
robert.moore, erik.kaneda, me, linux-kernel, linux-acpi,
linux-gpio, linux-i2c, platform-driver-x86, devel
Hi Daniel,
On Tue, Feb 23, 2021 at 10:36:18PM +0000, Daniel Scally wrote:
> On 23/02/2021 20:04, Laurent Pinchart wrote:
> >> +
> >> +/*
> >> + * Here follows platform specific mapping information that we can pass to
> >> + * the functions mapping resources to the sensors. Where the sensors have
> >> + * a power enable pin defined in DSDT we need to provide a supply name so
> >> + * the sensor drivers can find the regulator. The device name will be derived
> >> + * from the sensor's ACPI device within the code. Optionally, we can provide a
> >> + * NULL terminated array of function name mappings to deal with any platform
> >> + * specific deviations from the documented behaviour of GPIOs.
> >> + *
> >> + * Map a GPIO function name to NULL to prevent the driver from mapping that
> >> + * GPIO at all.
> >> + */
> >> +
> >> +static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = {
> >> + { "reset", NULL },
> >> + { "powerdown", "reset" },
> >> + { }
> >> +};
> >> +
> >> +static struct int3472_sensor_config int3472_sensor_configs[] = {
> >
> > This should be static const (and there will be some fallout due to that,
> > as skl_int3472_register_regulator() modifies the supply_map, so I think
> > you'll have a copy of supply_map in int3472_discrete_device).
>
> Ack to all of the constness; you mentioned that last time too - not sure
> how I missed doing those! I think I can just having a local struct
> regulator_consumer_supply in skl_int3472_register_regulator and fill it
> from int3472->sensor_config.supply_map
>
> >> +static unsigned int skl_int3472_get_clk_frequency(struct int3472_discrete_device *int3472)
> >> +{
> >> + union acpi_object *obj;
> >> + unsigned int ret = 0;
> >> +
> >> + obj = skl_int3472_get_acpi_buffer(int3472->sensor, "SSDB");
> >> + if (IS_ERR(obj))
> >> + return 0; /* report rate as 0 on error */
> >> +
> >> + if (obj->buffer.length < CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET + sizeof(u32)) {
> >
> > Should we define an ssdb structure instead of peeking into the buffer
> > with an offset ?
>
> I thought about that, but in the end decided it didn't seem worth
> defining the whole SSDB structure just to use one field. Particularly
> since we use it in cio2-bridge already, so if we're going to do that it
> really ought to just live in a header that's included in both - and that
> seemed even less worthwhile.
>
> I don't have a strong feeling though, so if you think it's better to
> define the struct I'm happy to.
If the structure is available already, sharing it in a common header
would be best I think, but that's not a blocker. It can be done on top
of this series.
> >> +static unsigned long skl_int3472_clk_recalc_rate(struct clk_hw *hw,
> >> + unsigned long parent_rate)
> >> +{
> >> + struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> >> + struct int3472_discrete_device *int3472 = to_int3472_device(clk);
> >> +
> >> + return int3472->clock.frequency;
> >
> > Maybe just
> >
> > struct int3472_gpio_clock *clk = to_int3472_clk(hw);
> >
> > return clk->frequency;
>
> Oops, of course.
>
> >> +static int skl_int3472_register_regulator(struct int3472_discrete_device *int3472,
> >> + struct acpi_resource *ares)
> >> +{
> >> + char *path = ares->data.gpio.resource_source.string_ptr;
> >> + struct int3472_sensor_config *sensor_config;
> >> + struct regulator_init_data init_data = { };
> >> + struct regulator_config cfg = { };
> >> + int ret;
> >> +
> >> + sensor_config = int3472->sensor_config;
> >> + if (IS_ERR_OR_NULL(sensor_config)) {
> >> + dev_err(int3472->dev, "No sensor module config\n");
> >> + return PTR_ERR(sensor_config);
> >> + }
> >> +
> >> + if (!sensor_config->supply_map.supply) {
> >> + dev_err(int3472->dev, "No supply name defined\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> >> + init_data.num_consumer_supplies = 1;
> >> + sensor_config->supply_map.dev_name = int3472->sensor_name;
> >> + init_data.consumer_supplies = &sensor_config->supply_map;
> >> +
> >> + snprintf(int3472->regulator.regulator_name,
> >> + sizeof(int3472->regulator.regulator_name), "%s-regulator",
> >> + acpi_dev_name(int3472->adev));
> >> + snprintf(int3472->regulator.supply_name,
> >> + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0");
> >> +
> >> + int3472->regulator.rdesc = INT3472_REGULATOR(
> >> + int3472->regulator.regulator_name,
> >> + int3472->regulator.supply_name,
> >> + &int3472_gpio_regulator_ops);
> >> +
> >> + int3472->regulator.gpio = acpi_get_gpiod(path,
> >> + ares->data.gpio.pin_table[0],
> >> + "int3472,regulator");
> >> + if (IS_ERR(int3472->regulator.gpio)) {
> >> + dev_err(int3472->dev, "Failed to get regulator GPIO lines\n");
> >
> > s/lines/line/ (sorry, it was a typo in my review of v2)
>
> No problem!
>
> >> +static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472)
> >> +{
> >> + struct list_head resource_list;
> >> + int ret;
> >> +
> >> + INIT_LIST_HEAD(&resource_list);
> >> +
> >> + int3472->sensor_config = skl_int3472_get_sensor_module_config(int3472);
> >
> > I have forgotten some of the context I'm afraid :-/ Are there valid use
> > cases for not checking for an error here, or should we do so and drop
> > the error checks in other functions above ?
>
> Not all platforms need a sensor_config; only those which have either a
> regulator pin or need a GPIO function to be remapped; the rest will do
> without it.
>
> So, we need to not check for an error here because the absence of a
> sensor_config isn't necessarily an error, we won't know till later.
>
> >> +int skl_int3472_discrete_probe(struct platform_device *pdev)
> >> +{
> >> + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> >> + struct int3472_discrete_device *int3472;
> >> + struct int3472_cldb cldb;
> >> + int ret;
> >> +
> >> + ret = skl_int3472_fill_cldb(adev, &cldb);
> >> + if (ret) {
> >> + dev_err(&pdev->dev, "Couldn't fill CLDB structure\n");
> >> + return ret;
> >> + }
> >> +
> >> + if (cldb.control_logic_type != 1) {
> >> + dev_err(&pdev->dev, "Unsupported control logic type %u\n",
> >> + cldb.control_logic_type);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + /* Max num GPIOs we've seen plus a terminator */
> >> + int3472 = kzalloc(struct_size(int3472, gpios.table,
> >> + INT3472_MAX_SENSOR_GPIOS + 1), GFP_KERNEL);
> >> + if (!int3472)
> >> + return -ENOMEM;
> >> +
> >> + int3472->adev = adev;
> >> + int3472->dev = &pdev->dev;
> >> + platform_set_drvdata(pdev, int3472);
> >> +
> >> + int3472->sensor = acpi_dev_get_dependent_dev(adev);
> >> + if (IS_ERR_OR_NULL(int3472->sensor)) {
> >> + dev_err(&pdev->dev,
> >> + "INT3472 seems to have no dependents.\n");
> >> + ret = -ENODEV;
> >> + goto err_free_int3472;
> >> + }
> >> + get_device(&int3472->sensor->dev);
> >
> > I see no corresponding put_device(), am I missing something ? I'm also
> > not sure why this is needed.
>
> The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
> to be no acpi_dev_get() for some reason. We use the sensor acpi_device
> to get the clock frequency, and to fetch the sensor module string, so I
> thought it ought to hold a reference on those grounds.
Shouldn't acpi_dev_get_dependent_dev() increase the reference count
then, instead of doing it manually here ?
> >> diff --git a/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> >> new file mode 100644
> >> index 000000000000..d0d2391e263f
> >> --- /dev/null
> >> +++ b/drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
> >> @@ -0,0 +1,113 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/* Author: Dan Scally <djrscally@gmail.com> */
> >> +
> >> +#include <linux/i2c.h>
> >> +#include <linux/mfd/core.h>
> >> +#include <linux/mfd/tps68470.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +
> >> +#include "intel_skl_int3472_common.h"
> >> +
> >> +static const struct mfd_cell tps68470_c[] = {
> >> + { .name = "tps68470-gpio" },
> >> + { .name = "tps68470_pmic_opregion" },
> >> +};
> >> +
> >> +static const struct mfd_cell tps68470_w[] = {
> >
> > Maybe more explicit names than _c and _w could be nice ?
>
> _chrome and _windows was in my mind - sound ok?
As Andy mentioned, _cros is better, and _windows_ or _win both work for
me.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-24 10:13 ` Laurent Pinchart
@ 2021-02-24 10:18 ` Andy Shevchenko
2021-02-24 10:20 ` Daniel Scally
0 siblings, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-02-24 10:18 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Daniel Scally, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones, andy.shevchenko,
kieran.bingham+renesas, Hans de Goede, Mark Gross,
Maximilian Luz, Robert Moore, Erik Kaneda, me,
Linux Kernel Mailing List, ACPI Devel Maling List,
open list:GPIO SUBSYSTEM, linux-i2c, Platform Driver, devel
On Wed, Feb 24, 2021 at 12:16 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, Feb 23, 2021 at 10:36:18PM +0000, Daniel Scally wrote:
> > On 23/02/2021 20:04, Laurent Pinchart wrote:
...
> > >> + get_device(&int3472->sensor->dev);
> > >
> > > I see no corresponding put_device(), am I missing something ? I'm also
> > > not sure why this is needed.
> >
> > The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
> > to be no acpi_dev_get() for some reason. We use the sensor acpi_device
> > to get the clock frequency, and to fetch the sensor module string, so I
> > thought it ought to hold a reference on those grounds.
>
> Shouldn't acpi_dev_get_dependent_dev() increase the reference count
> then, instead of doing it manually here ?
That's what I expected as well.
We have plenty of acpi_dev_get_*() and they do increase the reference
counter one way or the other.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-24 10:18 ` Andy Shevchenko
@ 2021-02-24 10:20 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-02-24 10:20 UTC (permalink / raw)
To: Andy Shevchenko, Laurent Pinchart
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, andy.shevchenko, kieran.bingham+renesas,
Hans de Goede, Mark Gross, Maximilian Luz, Robert Moore,
Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
Hi Andy, Laurent
On 24/02/2021 10:18, Andy Shevchenko wrote:
> On Wed, Feb 24, 2021 at 12:16 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> On Tue, Feb 23, 2021 at 10:36:18PM +0000, Daniel Scally wrote:
>>> On 23/02/2021 20:04, Laurent Pinchart wrote:
> ...
>
>>>>> + get_device(&int3472->sensor->dev);
>>>> I see no corresponding put_device(), am I missing something ? I'm also
>>>> not sure why this is needed.
>>> The put is acpi_dev_put() in skl_int3472_discrete_remove(); there seems
>>> to be no acpi_dev_get() for some reason. We use the sensor acpi_device
>>> to get the clock frequency, and to fetch the sensor module string, so I
>>> thought it ought to hold a reference on those grounds.
>> Shouldn't acpi_dev_get_dependent_dev() increase the reference count
>> then, instead of doing it manually here ?
> That's what I expected as well.
> We have plenty of acpi_dev_get_*() and they do increase the reference
> counter one way or the other.
>
Okedokey, I'll move the get() to that function and drop it from here.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
` (7 preceding siblings ...)
2021-02-22 14:15 ` Andy Shevchenko
@ 2021-03-04 13:37 ` Hans de Goede
2021-03-04 13:49 ` Daniel Scally
8 siblings, 1 reply; 44+ messages in thread
From: Hans de Goede @ 2021-03-04 13:37 UTC (permalink / raw)
To: Daniel Scally, tfiga, sakari.ailus, rajmohan.mani, rjw, lenb,
mika.westerberg, linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
Hi,
On 2/22/21 2:07 PM, Daniel Scally wrote:
> 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
>
> v2 was here:
> https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrscally@gmail.com/
>
> Series level changelog:
>
> - Dropped the patch moving acpi_lpss_dep() to utils since it's not used
> in acpi_dev_get_dependent_dev() anymore.
> - Replaced it with a patch extending acpi_walk_dep_device_list() to be
> able to apply a given callback against each device in acpi_dep_list
> - Dropped the patch creating acpi_i2c_dev_name() and simply open coded
> that functionality.
>
> This has been tested on a number of devices, but currently **not** on a device
> designed for ChromeOS, which we ideally need to do to ensure no regression
> caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way
> you could help with that? Unfortunately, I don't have a device to test it on
> myself.
>
> Original cover letter:
>
> 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 available but have not so far been
> tested, so aren't part of this series.
>
> The existing mfd/tps68470.c driver being thus superseded, it is removed.
Thank you for this patch series. Since there have already been a whole
bunch of review-comments, I've not taken a detailed look at this yet.
I do wonder if you have thought about how this series should be merged?
This series is spread over quite a few subsytems and since there are
various interdependencies in the patches it is probably best if it gets
merged in its entirety through a single tree.
I guess that merging though either Rafael's (drivers/acpi) tree or
Lee's (drivers/mfd) tree makes the most sense.
As drivers/platform/x86 maintainer I'm happy with whatever solution
works for the other subsystem maintainers.
Regards,
Hans
>
> Thanks
> Dan
>
> Daniel Scally (6):
> ACPI: scan: Extend acpi_walk_dep_device_list()
> ACPI: scan: Add function to fetch dependent of acpi device
> i2c: core: Add a format macro for I2C device names
> gpiolib: acpi: Export acpi_get_gpiod()
> platform/x86: Add intel_skl_int3472 driver
> mfd: tps68470: Remove tps68470 MFD driver
>
> MAINTAINERS | 5 +
> drivers/acpi/ec.c | 2 +-
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
> drivers/acpi/scan.c | 92 ++-
> drivers/gpio/Kconfig | 2 +-
> drivers/gpio/gpiolib-acpi.c | 38 +-
> drivers/i2c/i2c-core-acpi.c | 2 +-
> drivers/i2c/i2c-core-base.c | 4 +-
> drivers/mfd/Kconfig | 18 -
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---
> drivers/platform/surface/surface3_power.c | 2 +-
> drivers/platform/x86/Kconfig | 2 +
> drivers/platform/x86/Makefile | 1 +
> drivers/platform/x86/intel-int3472/Kconfig | 31 +
> drivers/platform/x86/intel-int3472/Makefile | 4 +
> .../intel-int3472/intel_skl_int3472_common.c | 106 ++++
> .../intel-int3472/intel_skl_int3472_common.h | 110 ++++
> .../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
> .../intel_skl_int3472_tps68470.c | 113 ++++
> include/acpi/acpi_bus.h | 8 +
> include/linux/acpi.h | 4 +-
> include/linux/gpio/consumer.h | 7 +
> include/linux/i2c.h | 3 +
> 25 files changed, 1100 insertions(+), 148 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
> create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
> create mode 100644 drivers/platform/x86/intel-int3472/Makefile
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
2021-03-04 13:37 ` Hans de Goede
@ 2021-03-04 13:49 ` Daniel Scally
2021-03-29 15:03 ` Andy Shevchenko
0 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-03-04 13:49 UTC (permalink / raw)
To: Hans de Goede, tfiga, sakari.ailus, rajmohan.mani, rjw, lenb,
mika.westerberg, linus.walleij, bgolaszewski, wsa, lee.jones
Cc: andriy.shevchenko, kieran.bingham+renesas, laurent.pinchart,
mgross, luzmaximilian, robert.moore, erik.kaneda, me,
linux-kernel, linux-acpi, linux-gpio, linux-i2c,
platform-driver-x86, devel
Hi Hans
On 04/03/2021 13:37, Hans de Goede wrote:
> Hi,
>
> On 2/22/21 2:07 PM, Daniel Scally wrote:
>> 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
>>
>> v2 was here:
>> https://lore.kernel.org/platform-driver-x86/20210118003428.568892-1-djrscally@gmail.com/
>>
>> Series level changelog:
>>
>> - Dropped the patch moving acpi_lpss_dep() to utils since it's not used
>> in acpi_dev_get_dependent_dev() anymore.
>> - Replaced it with a patch extending acpi_walk_dep_device_list() to be
>> able to apply a given callback against each device in acpi_dep_list
>> - Dropped the patch creating acpi_i2c_dev_name() and simply open coded
>> that functionality.
>>
>> This has been tested on a number of devices, but currently **not** on a device
>> designed for ChromeOS, which we ideally need to do to ensure no regression
>> caused by replacing the tps68470 MFD driver. Sakari / Tomasz, is there any way
>> you could help with that? Unfortunately, I don't have a device to test it on
>> myself.
>>
>> Original cover letter:
>>
>> 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 available but have not so far been
>> tested, so aren't part of this series.
>>
>> The existing mfd/tps68470.c driver being thus superseded, it is removed.
> Thank you for this patch series. Since there have already been a whole
> bunch of review-comments, I've not taken a detailed look at this yet.
No problem, I'm hoping to do a v3 over the weekend anyway.
> I do wonder if you have thought about how this series should be merged?
> This series is spread over quite a few subsytems and since there are
> various interdependencies in the patches it is probably best if it gets
> merged in its entirety through a single tree.
>
> I guess that merging though either Rafael's (drivers/acpi) tree or
> Lee's (drivers/mfd) tree makes the most sense.
>
> As drivers/platform/x86 maintainer I'm happy with whatever solution
> works for the other subsystem maintainers.
I also think it's a good idea to go through a single tree, and my plan
was to raise that probably after the next review round or so, but I
hadn't gotten as far as thinking about whos tree it should be or
anything yet. To be honest I'm not sure what factors dictate which
choice is best in that regard; handling complex git merges is a bit
outside my experience.
>
> Regards,
>
> Hans
>
>
>
>
>> Thanks
>> Dan
>>
>> Daniel Scally (6):
>> ACPI: scan: Extend acpi_walk_dep_device_list()
>> ACPI: scan: Add function to fetch dependent of acpi device
>> i2c: core: Add a format macro for I2C device names
>> gpiolib: acpi: Export acpi_get_gpiod()
>> platform/x86: Add intel_skl_int3472 driver
>> mfd: tps68470: Remove tps68470 MFD driver
>>
>> MAINTAINERS | 5 +
>> drivers/acpi/ec.c | 2 +-
>> drivers/acpi/pmic/Kconfig | 2 +-
>> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
>> drivers/acpi/scan.c | 92 ++-
>> drivers/gpio/Kconfig | 2 +-
>> drivers/gpio/gpiolib-acpi.c | 38 +-
>> drivers/i2c/i2c-core-acpi.c | 2 +-
>> drivers/i2c/i2c-core-base.c | 4 +-
>> drivers/mfd/Kconfig | 18 -
>> drivers/mfd/Makefile | 1 -
>> drivers/mfd/tps68470.c | 97 ---
>> drivers/platform/surface/surface3_power.c | 2 +-
>> drivers/platform/x86/Kconfig | 2 +
>> drivers/platform/x86/Makefile | 1 +
>> drivers/platform/x86/intel-int3472/Kconfig | 31 +
>> drivers/platform/x86/intel-int3472/Makefile | 4 +
>> .../intel-int3472/intel_skl_int3472_common.c | 106 ++++
>> .../intel-int3472/intel_skl_int3472_common.h | 110 ++++
>> .../intel_skl_int3472_discrete.c | 592 ++++++++++++++++++
>> .../intel_skl_int3472_tps68470.c | 113 ++++
>> include/acpi/acpi_bus.h | 8 +
>> include/linux/acpi.h | 4 +-
>> include/linux/gpio/consumer.h | 7 +
>> include/linux/i2c.h | 3 +
>> 25 files changed, 1100 insertions(+), 148 deletions(-)
>> delete mode 100644 drivers/mfd/tps68470.c
>> create mode 100644 drivers/platform/x86/intel-int3472/Kconfig
>> create mode 100644 drivers/platform/x86/intel-int3472/Makefile
>> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.c
>> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_common.h
>> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_discrete.c
>> create mode 100644 drivers/platform/x86/intel-int3472/intel_skl_int3472_tps68470.c
>>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-02-22 13:34 ` Andy Shevchenko
@ 2021-03-07 13:36 ` Daniel Scally
2021-03-07 20:39 ` Andy Shevchenko
0 siblings, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-03-07 13:36 UTC (permalink / raw)
To: Andy Shevchenko, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel, Rafael J . Wysocki
Hi Andy
On 22/02/2021 13:34, Andy Shevchenko wrote:
> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>> The acpi_walk_dep_device_list() is not as generalisable as its name
>> implies, serving only to decrement the dependency count for each
>> dependent device of the input. Extend the function to instead accept
>> a callback which can be applied to all the dependencies in acpi_dep_list.
>> Replace all existing calls to the function with calls to a wrapper, passing
>> a callback that applies the same dependency reduction.
> The code looks okay to me, if it was the initial idea, feel free to add
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Thank you!
>> + */
>> +void acpi_dev_flag_dependency_met(acpi_handle handle)
>> +{
> Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
I can do this, but I avoided it because in most of the uses in the
kernel currently there's no struct acpi_device, they're just passing
ACPI_HANDLE(dev) instead, so I'd need to get the adev with
ACPI_COMPANION() in each place. It didn't seem worth it...but happy to
do it if you'd prefer it that way?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-07 13:36 ` Daniel Scally
@ 2021-03-07 20:39 ` Andy Shevchenko
2021-03-08 13:36 ` Rafael J. Wysocki
0 siblings, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-03-07 20:39 UTC (permalink / raw)
To: Daniel Scally
Cc: Andy Shevchenko, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones,
kieran.bingham+renesas, Laurent Pinchart, Hans de Goede,
Mark Gross, Maximilian Luz, Robert Moore, Erik Kaneda, me,
Linux Kernel Mailing List, ACPI Devel Maling List,
open list:GPIO SUBSYSTEM, linux-i2c, Platform Driver, devel,
Rafael J . Wysocki
On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote:
> On 22/02/2021 13:34, Andy Shevchenko wrote:
> > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
> >> The acpi_walk_dep_device_list() is not as generalisable as its name
> >> implies, serving only to decrement the dependency count for each
> >> dependent device of the input. Extend the function to instead accept
> >> a callback which can be applied to all the dependencies in acpi_dep_list.
> >> Replace all existing calls to the function with calls to a wrapper, passing
> >> a callback that applies the same dependency reduction.
> > The code looks okay to me, if it was the initial idea, feel free to add
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
>
> Thank you!
>
>
> >> + */
> >> +void acpi_dev_flag_dependency_met(acpi_handle handle)
> >> +{
> > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
>
>
> I can do this, but I avoided it because in most of the uses in the
> kernel currently there's no struct acpi_device, they're just passing
> ACPI_HANDLE(dev) instead, so I'd need to get the adev with
> ACPI_COMPANION() in each place. It didn't seem worth it...but happy to
> do it if you'd prefer it that way?
I see, let Rafael decide then. I'm not pushing here.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-07 20:39 ` Andy Shevchenko
@ 2021-03-08 13:36 ` Rafael J. Wysocki
2021-03-08 13:57 ` Andy Shevchenko
0 siblings, 1 reply; 44+ messages in thread
From: Rafael J. Wysocki @ 2021-03-08 13:36 UTC (permalink / raw)
To: Andy Shevchenko, Daniel Scally
Cc: Andy Shevchenko, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones, Kieran Bingham,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Rafael J . Wysocki
On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote:
> > On 22/02/2021 13:34, Andy Shevchenko wrote:
> > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
> > >> The acpi_walk_dep_device_list() is not as generalisable as its name
> > >> implies, serving only to decrement the dependency count for each
> > >> dependent device of the input. Extend the function to instead accept
> > >> a callback which can be applied to all the dependencies in acpi_dep_list.
> > >> Replace all existing calls to the function with calls to a wrapper, passing
> > >> a callback that applies the same dependency reduction.
> > > The code looks okay to me, if it was the initial idea, feel free to add
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> >
> > Thank you!
> >
> >
> > >> + */
> > >> +void acpi_dev_flag_dependency_met(acpi_handle handle)
> > >> +{
> > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
> >
> >
> > I can do this, but I avoided it because in most of the uses in the
> > kernel currently there's no struct acpi_device, they're just passing
> > ACPI_HANDLE(dev) instead, so I'd need to get the adev with
> > ACPI_COMPANION() in each place. It didn't seem worth it...
It may not even be possible sometimes, because that function may be
called before creating all of the struct acpi_device objects (like in
the case of deferred enumeration).
> > but happy to
> > do it if you'd prefer it that way?
>
> I see, let Rafael decide then. I'm not pushing here.
Well, it's a matter of correctness.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-08 13:36 ` Rafael J. Wysocki
@ 2021-03-08 13:57 ` Andy Shevchenko
2021-03-08 15:45 ` Rafael J. Wysocki
0 siblings, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-03-08 13:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Daniel Scally, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones, Kieran Bingham,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA)
On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote:
> On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote:
> > > On 22/02/2021 13:34, Andy Shevchenko wrote:
> > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
> > > >> The acpi_walk_dep_device_list() is not as generalisable as its name
> > > >> implies, serving only to decrement the dependency count for each
> > > >> dependent device of the input. Extend the function to instead accept
> > > >> a callback which can be applied to all the dependencies in acpi_dep_list.
> > > >> Replace all existing calls to the function with calls to a wrapper, passing
> > > >> a callback that applies the same dependency reduction.
> > > > The code looks okay to me, if it was the initial idea, feel free to add
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
...
> > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle)
> > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
> > >
> > > I can do this, but I avoided it because in most of the uses in the
> > > kernel currently there's no struct acpi_device, they're just passing
> > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with
> > > ACPI_COMPANION() in each place. It didn't seem worth it...
>
> It may not even be possible sometimes, because that function may be
> called before creating all of the struct acpi_device objects (like in
> the case of deferred enumeration).
>
> > > but happy to
> > > do it if you'd prefer it that way?
> >
> > I see, let Rafael decide then. I'm not pushing here.
>
> Well, it's a matter of correctness.
Looking at your above comment it is indeed. Thanks for clarification!
But should we have acpi_dev_*() namespace for this function if it takes handle?
For time being nothing better than following comes to my mind:
__acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met()
acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met()
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-08 13:57 ` Andy Shevchenko
@ 2021-03-08 15:45 ` Rafael J. Wysocki
2021-03-08 17:23 ` Rafael J. Wysocki
0 siblings, 1 reply; 44+ messages in thread
From: Rafael J. Wysocki @ 2021-03-08 15:45 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Rafael J. Wysocki, Daniel Scally, Tomasz Figa, Sakari Ailus,
Rajmohan Mani, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, Bartosz Golaszewski, Wolfram Sang, Lee Jones,
Kieran Bingham, Laurent Pinchart, Hans de Goede, Mark Gross,
Maximilian Luz, Robert Moore, Erik Kaneda, me,
Linux Kernel Mailing List, ACPI Devel Maling List,
open list:GPIO SUBSYSTEM, linux-i2c, Platform Driver,
open list:ACPI COMPONENT ARCHITECTURE (ACPICA)
On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote:
> > On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote:
> > > > On 22/02/2021 13:34, Andy Shevchenko wrote:
> > > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
> > > > >> The acpi_walk_dep_device_list() is not as generalisable as its name
> > > > >> implies, serving only to decrement the dependency count for each
> > > > >> dependent device of the input. Extend the function to instead accept
> > > > >> a callback which can be applied to all the dependencies in acpi_dep_list.
> > > > >> Replace all existing calls to the function with calls to a wrapper, passing
> > > > >> a callback that applies the same dependency reduction.
> > > > > The code looks okay to me, if it was the initial idea, feel free to add
> > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> ...
>
> > > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle)
>
> > > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
> > > >
> > > > I can do this, but I avoided it because in most of the uses in the
> > > > kernel currently there's no struct acpi_device, they're just passing
> > > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with
> > > > ACPI_COMPANION() in each place. It didn't seem worth it...
> >
> > It may not even be possible sometimes, because that function may be
> > called before creating all of the struct acpi_device objects (like in
> > the case of deferred enumeration).
> >
> > > > but happy to
> > > > do it if you'd prefer it that way?
> > >
> > > I see, let Rafael decide then. I'm not pushing here.
> >
> > Well, it's a matter of correctness.
>
> Looking at your above comment it is indeed. Thanks for clarification!
Well, actually, the struct device for the object passed to this
function should be there already, because otherwise it wouldn't make
sense to update the list. So my comment above is not really
applicable to this particular device and the function could take a
struct acpi_device pointer argument. Sorry for the confusion.
> But should we have acpi_dev_*() namespace for this function if it takes handle?
It takes a device object handle.
Anyway, as per the above, it can take a struct acpi_device pointer
argument in which case the "acpi_dev_" prefix should be fine.
> For time being nothing better than following comes to my mind:
>
> __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met()
> acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met()
The above said, the name is somewhat confusing overall IMV.
Something like acpi_dev_clear_dependencies() might be better.
So lets make it something like
void acpi_dev_clear_dependencies(struct acpi_device *supplier);
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-08 15:45 ` Rafael J. Wysocki
@ 2021-03-08 17:23 ` Rafael J. Wysocki
2021-03-08 20:49 ` Daniel Scally
0 siblings, 1 reply; 44+ messages in thread
From: Rafael J. Wysocki @ 2021-03-08 17:23 UTC (permalink / raw)
To: Daniel Scally
Cc: Andy Shevchenko, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones, Kieran Bingham,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA)
On Mon, Mar 8, 2021 at 4:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote:
> > > On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:
> > > > On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote:
> > > > > On 22/02/2021 13:34, Andy Shevchenko wrote:
> > > > > > On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
> > > > > >> The acpi_walk_dep_device_list() is not as generalisable as its name
> > > > > >> implies, serving only to decrement the dependency count for each
> > > > > >> dependent device of the input. Extend the function to instead accept
> > > > > >> a callback which can be applied to all the dependencies in acpi_dep_list.
> > > > > >> Replace all existing calls to the function with calls to a wrapper, passing
> > > > > >> a callback that applies the same dependency reduction.
> > > > > > The code looks okay to me, if it was the initial idea, feel free to add
> > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> >
> > ...
> >
> > > > > >> +void acpi_dev_flag_dependency_met(acpi_handle handle)
> >
> > > > > > Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
> > > > >
> > > > > I can do this, but I avoided it because in most of the uses in the
> > > > > kernel currently there's no struct acpi_device, they're just passing
> > > > > ACPI_HANDLE(dev) instead, so I'd need to get the adev with
> > > > > ACPI_COMPANION() in each place. It didn't seem worth it...
> > >
> > > It may not even be possible sometimes, because that function may be
> > > called before creating all of the struct acpi_device objects (like in
> > > the case of deferred enumeration).
> > >
> > > > > but happy to
> > > > > do it if you'd prefer it that way?
> > > >
> > > > I see, let Rafael decide then. I'm not pushing here.
> > >
> > > Well, it's a matter of correctness.
> >
> > Looking at your above comment it is indeed. Thanks for clarification!
>
> Well, actually, the struct device for the object passed to this
> function should be there already, because otherwise it wouldn't make
> sense to update the list. So my comment above is not really
> applicable to this particular device and the function could take a
> struct acpi_device pointer argument. Sorry for the confusion.
>
> > But should we have acpi_dev_*() namespace for this function if it takes handle?
>
> It takes a device object handle.
>
> Anyway, as per the above, it can take a struct acpi_device pointer
> argument in which case the "acpi_dev_" prefix should be fine.
>
> > For time being nothing better than following comes to my mind:
> >
> > __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met()
> > acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met()
>
> The above said, the name is somewhat confusing overall IMV.
>
> Something like acpi_dev_clear_dependencies() might be better.
>
> So lets make it something like
>
> void acpi_dev_clear_dependencies(struct acpi_device *supplier);
To be precise, there are two functions in the patch,
acpi_dev_flag_dependency_met() which invokes
acpi_walk_dep_device_list() and __acpi_dev_flag_dependency_met()
invoked by the latter as a callback.
Above I was talking about the first one.
The callback should still take a struct acpi_dep_data pointer argument
and I would call it acpi_scan_clear_dep() or similar.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
2021-02-22 13:34 ` Andy Shevchenko
2021-02-22 13:38 ` Wolfram Sang
@ 2021-03-08 17:46 ` Rafael J. Wysocki
2021-03-08 20:40 ` Daniel Scally
2 siblings, 1 reply; 44+ messages in thread
From: Rafael J. Wysocki @ 2021-03-08 17:46 UTC (permalink / raw)
To: Daniel Scally
Cc: Tomasz Figa, Sakari Ailus, Mani, Rajmohan, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, andy.shevchenko, Kieran Bingham,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA),
Rafael J . Wysocki
On Mon, Feb 22, 2021 at 2:07 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> The acpi_walk_dep_device_list() is not as generalisable as its name
> implies, serving only to decrement the dependency count for each
> dependent device of the input. Extend the function to instead accept
> a callback which can be applied to all the dependencies in acpi_dep_list.
> Replace all existing calls to the function with calls to a wrapper, passing
> a callback that applies the same dependency reduction.
>
> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
> - patch introduced
>
> drivers/acpi/ec.c | 2 +-
> drivers/acpi/pmic/intel_pmic_chtdc_ti.c | 2 +-
> drivers/acpi/scan.c | 58 ++++++++++++++++-------
> drivers/gpio/gpiolib-acpi.c | 2 +-
> drivers/i2c/i2c-core-acpi.c | 2 +-
> drivers/platform/surface/surface3_power.c | 2 +-
> include/acpi/acpi_bus.h | 7 +++
> include/linux/acpi.h | 4 +-
> 8 files changed, 55 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 13565629ce0a..a258db713bd2 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -1627,7 +1627,7 @@ static int acpi_ec_add(struct acpi_device *device)
> WARN(!ret, "Could not request EC cmd io port 0x%lx", ec->command_addr);
>
> /* Reprobe devices depending on the EC */
> - acpi_walk_dep_device_list(ec->handle);
> + acpi_dev_flag_dependency_met(ec->handle);
>
> acpi_handle_debug(ec->handle, "enumerated.\n");
> return 0;
> diff --git a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> index a5101b07611a..59cca504325e 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtdc_ti.c
> @@ -117,7 +117,7 @@ static int chtdc_ti_pmic_opregion_probe(struct platform_device *pdev)
> return err;
>
> /* Re-enumerate devices depending on PMIC */
> - acpi_walk_dep_device_list(ACPI_HANDLE(pdev->dev.parent));
> + acpi_dev_flag_dependency_met(ACPI_HANDLE(pdev->dev.parent));
> return 0;
> }
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 80b668c80073..c9e4190316ef 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -49,12 +49,6 @@ static DEFINE_MUTEX(acpi_hp_context_lock);
> */
> static u64 spcr_uart_addr;
>
> -struct acpi_dep_data {
> - struct list_head node;
> - acpi_handle supplier;
> - acpi_handle consumer;
> -};
> -
> void acpi_scan_lock_acquire(void)
> {
> mutex_lock(&acpi_scan_lock);
> @@ -2099,30 +2093,58 @@ static void acpi_bus_attach(struct acpi_device *device, bool first_pass)
> device->handler->hotplug.notify_online(device);
> }
>
> -void acpi_walk_dep_device_list(acpi_handle handle)
> +static int __acpi_dev_flag_dependency_met(struct acpi_dep_data *dep,
> + void *data)
> {
> - struct acpi_dep_data *dep, *tmp;
> struct acpi_device *adev;
>
> + acpi_bus_get_device(dep->consumer, &adev);
> + if (!adev)
> + return 0;
> +
> + adev->dep_unmet--;
> + if (!adev->dep_unmet)
> + acpi_bus_attach(adev, true);
> +
> + list_del(&dep->node);
> + kfree(dep);
> + return 0;
> +}
> +
> +void acpi_walk_dep_device_list(acpi_handle handle,
> + int (*callback)(struct acpi_dep_data *, void *),
> + void *data)
> +{
> + struct acpi_dep_data *dep, *tmp;
> + int ret;
> +
> mutex_lock(&acpi_dep_list_lock);
> list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
> if (dep->supplier == handle) {
> - acpi_bus_get_device(dep->consumer, &adev);
> - if (!adev)
> - continue;
> -
> - adev->dep_unmet--;
> - if (!adev->dep_unmet)
> - acpi_bus_attach(adev, true);
The above code in the mainline has changed recently, so you need to
rebase the above and adjust for the change of behavior.
> -
> - list_del(&dep->node);
> - kfree(dep);
> + ret = callback(dep, data);
> + if (ret)
> + break;
> }
> }
> mutex_unlock(&acpi_dep_list_lock);
> }
> EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
>
> +/**
> + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device
> + * is now active
No parens here, please, and make it fit one line.
Also the description should be something like "Clear dependencies on
the given device."
> + * @handle: acpi_handle for the supplier device
> + *
> + * This function walks through the dependencies list and informs each consumer
> + * of @handle that their dependency upon it is now met. Devices with no more
> + * unmet dependencies will be attached to the acpi bus.
> + */
> +void acpi_dev_flag_dependency_met(acpi_handle handle)
> +{
> + acpi_walk_dep_device_list(handle, __acpi_dev_flag_dependency_met, NULL);
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_flag_dependency_met);
> +
> /**
> * acpi_bus_scan - Add ACPI device node objects in a given namespace scope.
> * @handle: Root of the namespace scope to scan.
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index e37a57d0a2f0..e4d728fda982 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -1254,7 +1254,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
>
> acpi_gpiochip_request_regions(acpi_gpio);
> acpi_gpiochip_scan_gpios(acpi_gpio);
> - acpi_walk_dep_device_list(handle);
> + acpi_dev_flag_dependency_met(handle);
> }
>
> void acpi_gpiochip_remove(struct gpio_chip *chip)
> diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
> index 37c510d9347a..38647cf34bde 100644
> --- a/drivers/i2c/i2c-core-acpi.c
> +++ b/drivers/i2c/i2c-core-acpi.c
> @@ -283,7 +283,7 @@ void i2c_acpi_register_devices(struct i2c_adapter *adap)
> if (!handle)
> return;
>
> - acpi_walk_dep_device_list(handle);
> + acpi_dev_flag_dependency_met(handle);
> }
>
> static const struct acpi_device_id i2c_acpi_force_400khz_device_ids[] = {
> diff --git a/drivers/platform/surface/surface3_power.c b/drivers/platform/surface/surface3_power.c
> index cc4f9cba6856..ad895285d3e9 100644
> --- a/drivers/platform/surface/surface3_power.c
> +++ b/drivers/platform/surface/surface3_power.c
> @@ -478,7 +478,7 @@ static int mshw0011_install_space_handler(struct i2c_client *client)
> return -ENOMEM;
> }
>
> - acpi_walk_dep_device_list(handle);
> + acpi_dev_flag_dependency_met(handle);
> return 0;
> }
>
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 02a716a0af5d..91172af3a04d 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -278,6 +278,12 @@ struct acpi_device_power {
> struct acpi_device_power_state states[ACPI_D_STATE_COUNT]; /* Power states (D0-D3Cold) */
> };
>
> +struct acpi_dep_data {
> + struct list_head node;
> + acpi_handle supplier;
> + acpi_handle consumer;
> +};
> +
> /* Performance Management */
>
> struct acpi_device_perf_flags {
> @@ -683,6 +689,7 @@ static inline bool acpi_device_can_poweroff(struct acpi_device *adev)
>
> bool acpi_dev_hid_uid_match(struct acpi_device *adev, const char *hid2, const char *uid2);
>
> +void acpi_dev_flag_dependency_met(acpi_handle handle);
> struct acpi_device *
> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv);
> struct acpi_device *
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 2630c2e953f7..2d5e6e88e8a0 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -655,7 +655,9 @@ extern bool acpi_driver_match_device(struct device *dev,
> const struct device_driver *drv);
> int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
> int acpi_device_modalias(struct device *, char *, int);
> -void acpi_walk_dep_device_list(acpi_handle handle);
> +void acpi_walk_dep_device_list(acpi_handle handle,
> + int (*callback)(struct acpi_dep_data *, void *),
> + void *data);
>
> struct platform_device *acpi_create_platform_device(struct acpi_device *,
> struct property_entry *);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-08 17:46 ` Rafael J. Wysocki
@ 2021-03-08 20:40 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-03-08 20:40 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tomasz Figa, Sakari Ailus, Mani, Rajmohan, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, andy.shevchenko, Kieran Bingham,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA)
Hi Rafael
On 08/03/2021 17:46, Rafael J. Wysocki wrote:
>> +void acpi_walk_dep_device_list(acpi_handle handle,
>> + int (*callback)(struct acpi_dep_data *, void *),
>> + void *data)
>> +{
>> + struct acpi_dep_data *dep, *tmp;
>> + int ret;
>> +
>> mutex_lock(&acpi_dep_list_lock);
>> list_for_each_entry_safe(dep, tmp, &acpi_dep_list, node) {
>> if (dep->supplier == handle) {
>> - acpi_bus_get_device(dep->consumer, &adev);
>> - if (!adev)
>> - continue;
>> -
>> - adev->dep_unmet--;
>> - if (!adev->dep_unmet)
>> - acpi_bus_attach(adev, true);
> The above code in the mainline has changed recently, so you need to
> rebase the above and adjust for the change of behavior.
Yeah, I'll rebase onto 5.12-rc2 before next submission.
>
>> -
>> - list_del(&dep->node);
>> - kfree(dep);
>> + ret = callback(dep, data);
>> + if (ret)
>> + break;
>> }
>> }
>> mutex_unlock(&acpi_dep_list_lock);
>> }
>> EXPORT_SYMBOL_GPL(acpi_walk_dep_device_list);
>>
>> +/**
>> + * acpi_dev_flag_dependency_met() - Inform consumers of @handle that the device
>> + * is now active
> No parens here, please, and make it fit one line.
>
> Also the description should be something like "Clear dependencies on
> the given device."
OK - no problem
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list()
2021-03-08 17:23 ` Rafael J. Wysocki
@ 2021-03-08 20:49 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-03-08 20:49 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Andy Shevchenko, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones, Kieran Bingham,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA)
Hi Rafael
On 08/03/2021 17:23, Rafael J. Wysocki wrote:
> On Mon, Mar 8, 2021 at 4:45 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>> On Mon, Mar 8, 2021 at 2:57 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Mon, Mar 08, 2021 at 02:36:27PM +0100, Rafael J. Wysocki wrote:
>>>> On Sun, Mar 7, 2021 at 9:39 PM Andy Shevchenko
>>>> <andy.shevchenko@gmail.com> wrote:
>>>>> On Sun, Mar 7, 2021 at 3:36 PM Daniel Scally <djrscally@gmail.com> wrote:
>>>>>> On 22/02/2021 13:34, Andy Shevchenko wrote:
>>>>>>> On Mon, Feb 22, 2021 at 3:12 PM Daniel Scally <djrscally@gmail.com> wrote:
>>>>>>>> The acpi_walk_dep_device_list() is not as generalisable as its name
>>>>>>>> implies, serving only to decrement the dependency count for each
>>>>>>>> dependent device of the input. Extend the function to instead accept
>>>>>>>> a callback which can be applied to all the dependencies in acpi_dep_list.
>>>>>>>> Replace all existing calls to the function with calls to a wrapper, passing
>>>>>>>> a callback that applies the same dependency reduction.
>>>>>>> The code looks okay to me, if it was the initial idea, feel free to add
>>>>>>> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>>> ...
>>>
>>>>>>>> +void acpi_dev_flag_dependency_met(acpi_handle handle)
>>>>>>> Since it's acpi_dev_* namespace, perhaps it should take struct acpi_device here?
>>>>>> I can do this, but I avoided it because in most of the uses in the
>>>>>> kernel currently there's no struct acpi_device, they're just passing
>>>>>> ACPI_HANDLE(dev) instead, so I'd need to get the adev with
>>>>>> ACPI_COMPANION() in each place. It didn't seem worth it...
>>>> It may not even be possible sometimes, because that function may be
>>>> called before creating all of the struct acpi_device objects (like in
>>>> the case of deferred enumeration).
>>>>
>>>>>> but happy to
>>>>>> do it if you'd prefer it that way?
>>>>> I see, let Rafael decide then. I'm not pushing here.
>>>> Well, it's a matter of correctness.
>>> Looking at your above comment it is indeed. Thanks for clarification!
>> Well, actually, the struct device for the object passed to this
>> function should be there already, because otherwise it wouldn't make
>> sense to update the list. So my comment above is not really
>> applicable to this particular device and the function could take a
>> struct acpi_device pointer argument. Sorry for the confusion.
>>
>>> But should we have acpi_dev_*() namespace for this function if it takes handle?
>> It takes a device object handle.
>>
>> Anyway, as per the above, it can take a struct acpi_device pointer
>> argument in which case the "acpi_dev_" prefix should be fine.
OK, so the conclusion there is change the argument to a struct
acpi_device pointer and update all the uses.
>>> For time being nothing better than following comes to my mind:
>>>
>>> __acpi_dev_flag_dependency_met() => __acpi_flag_device_dependency_met()
>>> acpi_dev_flag_dependency_met() => acpi_flag_device_dependency_met()
>> The above said, the name is somewhat confusing overall IMV.
>>
>> Something like acpi_dev_clear_dependencies() might be better.
>>
>> So lets make it something like
>>
>> void acpi_dev_clear_dependencies(struct acpi_device *supplier);
> To be precise, there are two functions in the patch,
> acpi_dev_flag_dependency_met() which invokes
> acpi_walk_dep_device_list() and __acpi_dev_flag_dependency_met()
> invoked by the latter as a callback.
>
> Above I was talking about the first one.
>
> The callback should still take a struct acpi_dep_data pointer argument
> and I would call it acpi_scan_clear_dep() or similar.
OK, works for me, I'll make those changes - thanks
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver
2021-02-22 13:07 ` [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver Daniel Scally
2021-02-22 14:12 ` Andy Shevchenko
@ 2021-03-10 9:33 ` Lee Jones
1 sibling, 0 replies; 44+ messages in thread
From: Lee Jones @ 2021-03-10 9:33 UTC (permalink / raw)
To: Daniel Scally
Cc: tfiga, sakari.ailus, rajmohan.mani, rjw, lenb, mika.westerberg,
linus.walleij, bgolaszewski, wsa, andy.shevchenko,
kieran.bingham+renesas, laurent.pinchart, hdegoede, mgross,
luzmaximilian, robert.moore, erik.kaneda, me, linux-kernel,
linux-acpi, linux-gpio, linux-i2c, platform-driver-x86, devel
On Mon, 22 Feb 2021, Daniel Scally wrote:
> This driver only covered one scenario in which ACPI devices with _HID
> INT3472 are found, and its functionality has been taken over by the
> intel-skl-int3472 module, so remove it.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes in v3:
> - Replaced Kconfig dependencies with INTEL_SKL_INT3472 for the tps68470
> OpRegion and GPIO drivers.
>
> drivers/acpi/pmic/Kconfig | 2 +-
> drivers/gpio/Kconfig | 2 +-
> drivers/mfd/Kconfig | 18 --------
> drivers/mfd/Makefile | 1 -
> drivers/mfd/tps68470.c | 97 ---------------------------------------
> 5 files changed, 2 insertions(+), 118 deletions(-)
> delete mode 100644 drivers/mfd/tps68470.c
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
2021-03-04 13:49 ` Daniel Scally
@ 2021-03-29 15:03 ` Andy Shevchenko
2021-03-29 20:37 ` Daniel Scally
0 siblings, 1 reply; 44+ messages in thread
From: Andy Shevchenko @ 2021-03-29 15:03 UTC (permalink / raw)
To: Daniel Scally
Cc: Hans de Goede, tfiga, sakari.ailus, rajmohan.mani, rjw, lenb,
mika.westerberg, linus.walleij, bgolaszewski, wsa, lee.jones,
kieran.bingham+renesas, laurent.pinchart, mgross, luzmaximilian,
robert.moore, erik.kaneda, me, linux-kernel, linux-acpi,
linux-gpio, linux-i2c, platform-driver-x86, devel
On Thu, Mar 04, 2021 at 01:49:14PM +0000, Daniel Scally wrote:
> On 04/03/2021 13:37, Hans de Goede wrote:
> > On 2/22/21 2:07 PM, Daniel Scally wrote:
...
> >> The existing mfd/tps68470.c driver being thus superseded, it is removed.
> > Thank you for this patch series. Since there have already been a whole
> > bunch of review-comments, I've not taken a detailed look at this yet.
>
> No problem, I'm hoping to do a v3 over the weekend anyway.
Do you mean v4?
I'm just wondering if you need any help.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 0/6] Introduce intel_skl_int3472 module
2021-03-29 15:03 ` Andy Shevchenko
@ 2021-03-29 20:37 ` Daniel Scally
0 siblings, 0 replies; 44+ messages in thread
From: Daniel Scally @ 2021-03-29 20:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Hans de Goede, tfiga, sakari.ailus, rajmohan.mani, rjw, lenb,
mika.westerberg, linus.walleij, bgolaszewski, wsa, lee.jones,
kieran.bingham+renesas, laurent.pinchart, mgross, luzmaximilian,
robert.moore, erik.kaneda, me, linux-kernel, linux-acpi,
linux-gpio, linux-i2c, platform-driver-x86, devel
Hi Andy
On 29/03/2021 16:03, Andy Shevchenko wrote:
> On Thu, Mar 04, 2021 at 01:49:14PM +0000, Daniel Scally wrote:
>> On 04/03/2021 13:37, Hans de Goede wrote:
>>> On 2/22/21 2:07 PM, Daniel Scally wrote:
> ...
>
>>>> The existing mfd/tps68470.c driver being thus superseded, it is removed.
>>> Thank you for this patch series. Since there have already been a whole
>>> bunch of review-comments, I've not taken a detailed look at this yet.
>> No problem, I'm hoping to do a v3 over the weekend anyway.
> Do you mean v4?
Oops, I do indeed.
> I'm just wondering if you need any help.
Thanks - I don't think so; I've just not been working on it very much
lately. I got sidetracked with a sensor driver [1] that was pretty fun,
so I've been focused on that instead. I'm just finishing up a v2 for
that, and then I'll come back to this.
[1]
https://lore.kernel.org/linux-media/20210312103239.279523-2-djrscally@gmail.com/
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-02-22 14:58 ` Andy Shevchenko
2021-02-22 22:35 ` Daniel Scally
@ 2021-05-17 21:43 ` Daniel Scally
2021-05-17 21:47 ` Andy Shevchenko
1 sibling, 1 reply; 44+ messages in thread
From: Daniel Scally @ 2021-05-17 21:43 UTC (permalink / raw)
To: Andy Shevchenko, Andy Shevchenko
Cc: Tomasz Figa, Sakari Ailus, Rajmohan Mani, Rafael J. Wysocki,
Len Brown, Mika Westerberg, Linus Walleij, Bartosz Golaszewski,
Wolfram Sang, Lee Jones, kieran.bingham+renesas,
Laurent Pinchart, Hans de Goede, Mark Gross, Maximilian Luz,
Robert Moore, Erik Kaneda, me, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, devel
Hi Andy
On 22/02/2021 14:58, Andy Shevchenko wrote
>> +#include <linux/clk-provider.h>
>
> This is definitely not for *.h. (Not all C files needed this)
>
>> +#include <linux/gpio/machine.h>
>
> Ditto.
>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>
> Ditto.
Bit more delayed than I wanted to be, but I'm just finishing off the v4
of this. For these includes, I'm using the actual structs from them
rather than pointers, so removing these would mean moving the definition
of struct int3472_discrete_device into one of the source files; you're
happy with that?
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver
2021-05-17 21:43 ` Daniel Scally
@ 2021-05-17 21:47 ` Andy Shevchenko
0 siblings, 0 replies; 44+ messages in thread
From: Andy Shevchenko @ 2021-05-17 21:47 UTC (permalink / raw)
To: Daniel Scally
Cc: Andy Shevchenko, Tomasz Figa, Sakari Ailus, Rajmohan Mani,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
Bartosz Golaszewski, Wolfram Sang, Lee Jones,
kieran.bingham+renesas, Laurent Pinchart, Hans de Goede,
Mark Gross, Maximilian Luz, Robert Moore, Erik Kaneda,
Fabian Wüthrich, Linux Kernel Mailing List,
ACPI Devel Maling List, open list:GPIO SUBSYSTEM, linux-i2c,
Platform Driver, open list:ACPI COMPONENT ARCHITECTURE (ACPICA)
On Tue, May 18, 2021 at 12:43 AM Daniel Scally <djrscally@gmail.com> wrote:
>
> Hi Andy
>
> On 22/02/2021 14:58, Andy Shevchenko wrote
> >> +#include <linux/clk-provider.h>
> >
> > This is definitely not for *.h. (Not all C files needed this)
> >
> >> +#include <linux/gpio/machine.h>
> >
> > Ditto.
> >
> >> +#include <linux/regulator/driver.h>
> >> +#include <linux/regulator/machine.h>
> >
> > Ditto.
>
> Bit more delayed than I wanted to be, but I'm just finishing off the v4
> of this. For these includes, I'm using the actual structs from them
> rather than pointers, so removing these would mean moving the definition
> of struct int3472_discrete_device into one of the source files; you're
> happy with that?
Either way, please send a v4 and we start over from a fresh view.
Thanks!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2021-05-17 21:48 UTC | newest]
Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-22 13:07 [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
2021-02-22 13:07 ` [PATCH v3 1/6] ACPI: scan: Extend acpi_walk_dep_device_list() Daniel Scally
2021-02-22 13:34 ` Andy Shevchenko
2021-03-07 13:36 ` Daniel Scally
2021-03-07 20:39 ` Andy Shevchenko
2021-03-08 13:36 ` Rafael J. Wysocki
2021-03-08 13:57 ` Andy Shevchenko
2021-03-08 15:45 ` Rafael J. Wysocki
2021-03-08 17:23 ` Rafael J. Wysocki
2021-03-08 20:49 ` Daniel Scally
2021-02-22 13:38 ` Wolfram Sang
2021-03-08 17:46 ` Rafael J. Wysocki
2021-03-08 20:40 ` Daniel Scally
2021-02-22 13:07 ` [PATCH v3 2/6] ACPI: scan: Add function to fetch dependent of acpi device Daniel Scally
2021-02-22 13:41 ` Andy Shevchenko
2021-02-22 13:07 ` [PATCH v3 3/6] i2c: core: Add a format macro for I2C device names Daniel Scally
2021-02-22 13:38 ` Wolfram Sang
2021-02-22 13:07 ` [PATCH v3 4/6] gpiolib: acpi: Export acpi_get_gpiod() Daniel Scally
2021-02-22 13:54 ` Andy Shevchenko
2021-02-22 13:07 ` [PATCH v3 5/6] platform/x86: Add intel_skl_int3472 driver Daniel Scally
2021-02-22 13:19 ` Daniel Scally
2021-02-22 13:27 ` Hans de Goede
2021-02-22 22:50 ` Daniel Scally
2021-02-22 14:58 ` Andy Shevchenko
2021-02-22 22:35 ` Daniel Scally
2021-02-23 12:01 ` Andy Shevchenko
2021-02-23 13:06 ` Daniel Scally
2021-05-17 21:43 ` Daniel Scally
2021-05-17 21:47 ` Andy Shevchenko
2021-02-23 20:04 ` Laurent Pinchart
2021-02-23 22:36 ` Daniel Scally
2021-02-24 10:13 ` Laurent Pinchart
2021-02-24 10:18 ` Andy Shevchenko
2021-02-24 10:20 ` Daniel Scally
2021-02-22 13:07 ` [PATCH v3 6/6] mfd: tps68470: Remove tps68470 MFD driver Daniel Scally
2021-02-22 14:12 ` Andy Shevchenko
2021-02-22 22:37 ` Daniel Scally
2021-03-10 9:33 ` Lee Jones
2021-02-22 13:11 ` [PATCH v3 0/6] Introduce intel_skl_int3472 module Daniel Scally
2021-02-22 14:15 ` Andy Shevchenko
2021-03-04 13:37 ` Hans de Goede
2021-03-04 13:49 ` Daniel Scally
2021-03-29 15:03 ` Andy Shevchenko
2021-03-29 20:37 ` Daniel Scally
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 NNTP newsgroup(s).