| Message ID | 20210222130735.1313443-1-djrscally@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce intel_skl_int3472 module | expand |
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 >
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
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 >
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 >
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 >
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 >>
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?
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.
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.
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()
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);
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.
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 >
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
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
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/