| Message ID | 20210118003428.568892-1-djrscally@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce intel_skl_int3472 driver | expand |
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:23AM +0000, Daniel Scally wrote: > In some ACPI tables we encounter, devices use the _DEP method to assert > a dependence on other ACPI devices as opposed to the OpRegions that the > specification intends. We need to be able to find those devices "from" > the dependee, so add a function to parse all ACPI Devices and check if > the include the handle of the dependee device in their _DEP buffer. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > - Used acpi_lpss_dep() as Andy suggested. > > drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ > include/acpi/acpi_bus.h | 2 ++ > 2 files changed, 36 insertions(+) > > diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c > index 78b38775f18b..ec6a2406a886 100644 > --- a/drivers/acpi/utils.c > +++ b/drivers/acpi/utils.c > @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) > return false; > } > > +static int acpi_dev_match_by_dep(struct device *dev, const void *data) > +{ > + struct acpi_device *adev = to_acpi_device(dev); > + const struct acpi_device *dependee = data; > + acpi_handle handle = dependee->handle; > + > + if (acpi_lpss_dep(adev, handle)) > + return 1; > + > + return 0; > +} > + I think I'd move this just before acpi_dev_get_next_dep_dev() to keep the two together. > /** > * acpi_dev_present - Detect that a given ACPI device is present > * @hid: Hardware ID of the device. > @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) > } > EXPORT_SYMBOL(acpi_dev_present); > > +/** > + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev Maybe acpi_dev_get_next_dependent_dev() ? "dep" could mean either dependent or dependency. > + * @adev: Pointer to the dependee device > + * @prev: Pointer to the previous dependent device (or NULL for first match) > + * > + * Return the next ACPI device which declares itself dependent on @adev in > + * the _DEP buffer. > + * > + * The caller is responsible to call put_device() on the returned device. > + */ > +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, > + struct acpi_device *prev) > +{ > + struct device *start = prev ? &prev->dev : NULL; > + struct device *dev; > + > + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); Having to loop over all ACPI devices is quite inefficient, but if we need a reverse lookup, we don't really have a choice. We could create a reverse map, but I don't think it's worth it. > + > + return dev ? to_acpi_device(dev) : NULL; > +} > +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); I would have used EXPORT_SYMBOL_GPL. I'm not sure what the policy is in the ACPI subsystem, and it's also a personal choice. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > /** > * acpi_dev_get_next_match_dev - Return the next match of ACPI device > * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index 02a716a0af5d..33deb22294f2 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -683,6 +683,8 @@ 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); > > +struct acpi_device * > +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); > struct acpi_device * > acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); > struct acpi_device *
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:28AM +0000, 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. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Introduced > > drivers/acpi/pmic/Kconfig | 1 - > drivers/gpio/Kconfig | 1 - > drivers/mfd/Kconfig | 18 -------- > drivers/mfd/Makefile | 1 - > drivers/mfd/tps68470.c | 97 --------------------------------------- > 5 files changed, 118 deletions(-) > delete mode 100644 drivers/mfd/tps68470.c > > diff --git a/drivers/acpi/pmic/Kconfig b/drivers/acpi/pmic/Kconfig > index 56bbcb2ce61b..e27d8ef3a32c 100644 > --- a/drivers/acpi/pmic/Kconfig > +++ b/drivers/acpi/pmic/Kconfig > @@ -52,7 +52,6 @@ endif # PMIC_OPREGION > > config TPS68470_PMIC_OPREGION > bool "ACPI operation region support for TPS68470 PMIC" > - depends on MFD_TPS68470 Should this now depend 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..07ff8f24b0d9 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -1343,7 +1343,6 @@ config GPIO_TPS65912 > > config GPIO_TPS68470 > bool "TPS68470 GPIO" > - depends on MFD_TPS68470 Same here. This won't deal with the case where th TPS68470 is instantiated through DT, but that's not supported yet, so it can be dealt with it later when the need arises. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > 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);
Morning Laurent On 18/01/2021 07:34, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Mon, Jan 18, 2021 at 12:34:23AM +0000, Daniel Scally wrote: >> In some ACPI tables we encounter, devices use the _DEP method to assert >> a dependence on other ACPI devices as opposed to the OpRegions that the >> specification intends. We need to be able to find those devices "from" >> the dependee, so add a function to parse all ACPI Devices and check if >> the include the handle of the dependee device in their _DEP buffer. >> >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> - Used acpi_lpss_dep() as Andy suggested. >> >> drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ >> include/acpi/acpi_bus.h | 2 ++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 78b38775f18b..ec6a2406a886 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> return false; >> } >> >> +static int acpi_dev_match_by_dep(struct device *dev, const void *data) >> +{ >> + struct acpi_device *adev = to_acpi_device(dev); >> + const struct acpi_device *dependee = data; >> + acpi_handle handle = dependee->handle; >> + >> + if (acpi_lpss_dep(adev, handle)) >> + return 1; >> + >> + return 0; >> +} >> + > I think I'd move this just before acpi_dev_get_next_dep_dev() to keep > the two together. Will do > >> /** >> * acpi_dev_present - Detect that a given ACPI device is present >> * @hid: Hardware ID of the device. >> @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) >> } >> EXPORT_SYMBOL(acpi_dev_present); >> >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev > Maybe acpi_dev_get_next_dependent_dev() ? "dep" could mean either > dependent or dependency. Yes, good point, I agree. > >> + * @adev: Pointer to the dependee device >> + * @prev: Pointer to the previous dependent device (or NULL for first match) >> + * >> + * Return the next ACPI device which declares itself dependent on @adev in >> + * the _DEP buffer. >> + * >> + * The caller is responsible to call put_device() on the returned device. >> + */ >> +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, >> + struct acpi_device *prev) >> +{ >> + struct device *start = prev ? &prev->dev : NULL; >> + struct device *dev; >> + >> + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); > Having to loop over all ACPI devices is quite inefficient, but if we > need a reverse lookup, we don't really have a choice. We could create a > reverse map, but I don't think it's worth it. > >> + >> + return dev ? to_acpi_device(dev) : NULL; >> +} >> +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); > I would have used EXPORT_SYMBOL_GPL. I'm not sure what the policy is in > the ACPI subsystem, and it's also a personal choice. EXPORT_SYMBOL_GPL would be my usual choice, but the other functions in the file all use EXPORT_SYMBOL, so I assumed there was some policy that that be used (since basically everywhere else I've touched in the kernel so far defaults to the GPL version) > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! > >> + >> /** >> * acpi_dev_get_next_match_dev - Return the next match of ACPI device >> * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 02a716a0af5d..33deb22294f2 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -683,6 +683,8 @@ 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); >> >> +struct acpi_device * >> +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); >> struct acpi_device * >> acpi_dev_get_next_match_dev(struct acpi_device *adev, const char *hid, const char *uid, s64 hrv); >> struct acpi_device *
Hi Daniel, Thank you for the patch. On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > We want to refer to an i2c device by name before it has been s/i2c device/acpi i2c device/ ? > created by the kernel; add a function that constructs the name > from the acpi device instead. > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Stopped using devm_kasprintf() > > drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ > include/linux/i2c.h | 5 +++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 37c510d9347a..98c3ba9a2350 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > } > EXPORT_SYMBOL_GPL(i2c_acpi_new_device); > > +/** > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > + * @adev: ACPI device to construct the name for > + * > + * Constructs the name of an i2c device matching the format used by > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > + * before they have been instantiated. > + * > + * The caller is responsible for freeing the returned pointer. > + */ > +char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); There's a real danger of a memory leak, as the function name sounds very similar to dev_name() or acpi_dev_name() and those don't allocate memory. I'm not sure what a better name would be, but given that this function is only used in patch 6/7 and not in the I2C subsystem itself, I wonder if we should inline this kasprintf() call in the caller and drop this patch. > +} > +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name); > + > #ifdef CONFIG_ACPI_I2C_OPREGION > static int acpi_gsb_i2c_read_bytes(struct i2c_client *client, > u8 cmd, u8 *data, u8 data_len) > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 4d40a4b46810..b82aac05b17f 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -998,6 +998,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > u32 i2c_acpi_find_bus_speed(struct device *dev); > struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > struct i2c_board_info *info); > +char *i2c_acpi_dev_name(struct acpi_device *adev); > struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle); > #else > static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > @@ -1014,6 +1015,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev, > { > return ERR_PTR(-ENODEV); > } > +static inline char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return NULL; > +} > static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle) > { > return NULL;
On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > We want to refer to an i2c device by name before it has been I²C > created by the kernel; add a function that constructs the name > from the acpi device instead. acpi -> ACPI Prefix: "i2c: core: " With above Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Stopped using devm_kasprintf() > > drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ > include/linux/i2c.h | 5 +++++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > index 37c510d9347a..98c3ba9a2350 100644 > --- a/drivers/i2c/i2c-core-acpi.c > +++ b/drivers/i2c/i2c-core-acpi.c > @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > } > EXPORT_SYMBOL_GPL(i2c_acpi_new_device); > > +/** > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > + * @adev: ACPI device to construct the name for > + * > + * Constructs the name of an i2c device matching the format used by > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > + * before they have been instantiated. > + * > + * The caller is responsible for freeing the returned pointer. > + */ > +char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > +} > +EXPORT_SYMBOL_GPL(i2c_acpi_dev_name); > + > #ifdef CONFIG_ACPI_I2C_OPREGION > static int acpi_gsb_i2c_read_bytes(struct i2c_client *client, > u8 cmd, u8 *data, u8 data_len) > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 4d40a4b46810..b82aac05b17f 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -998,6 +998,7 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > u32 i2c_acpi_find_bus_speed(struct device *dev); > struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > struct i2c_board_info *info); > +char *i2c_acpi_dev_name(struct acpi_device *adev); > struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle); > #else > static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares, > @@ -1014,6 +1015,10 @@ static inline struct i2c_client *i2c_acpi_new_device(struct device *dev, > { > return ERR_PTR(-ENODEV); > } > +static inline char *i2c_acpi_dev_name(struct acpi_device *adev) > +{ > + return NULL; > +} > static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle) > { > return NULL; > -- > 2.25.1 >
On Mon, Jan 18, 2021 at 12:34:26AM +0000, Daniel Scally wrote: > I need to be able to translate GPIO resources in an acpi_device's _CRS ACPI device's > into gpio_descs. Those are represented in _CRS as a pathname to a GPIO into GPIO descriptor array > device plus the pin's index number: this function is perfect for that > purpose. ... > diff --git a/include/linux/acpi.h b/include/linux/acpi.h Wrong header. Please use gpio/consumer.h.
On Mon, Jan 18, 2021 at 12:34:28AM +0000, 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. Prefix: "mfd: tps68470: ". Rationale: easier to look for specific commits, by, for example, running `git log --grep tps68470`.
On Mon, 2021-01-18 at 15:39 +0200, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > > We want to refer to an i2c device by name before it has been > > I²C Andy, are you next going to suggest renaming all the files with i2c? $ git ls-files | grep i2c | wc -l 953 Please do not use the pedantic I²C, 7 bit ascii is just fine here. My keyboard does not have a superscripted 2 key, and yes, I know how to use it with multiple keypresses but it's irrelevant. > > created by the kernel; add a function that constructs the name > > from the acpi device instead. > > acpi -> ACPI Same deal with acpi filenames. Everyone already recognizes acpi is actually ACPI and there isn't any confusion in anyone's mind. $ git ls-files | grep acpi | wc -l 533 > Prefix: "i2c: core: " Please stop being a pedant on these trivial things. It's unimportant and has almost no value.
On Mon, Jan 18, 2021 at 08:56:44PM +0200, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 10:43:14AM -0800, Joe Perches wrote: > > On Mon, 2021-01-18 at 15:39 +0200, Andy Shevchenko wrote: > > > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: ... > > > Prefix: "i2c: core: " > > > > Please stop being a pedant on these trivial things. > > It's unimportant and has almost no value. > > This series is going to have a new version, that's why I did those comments. > If it had been the only comments, I would have not posted them. And actually to the rationale: it makes slightly easier to grep for patches against same driver / group of drivers / subsystem. I bet the `... --grep 'i2c: core:' will give different results to the `... -- drivers/i2c/i2c-* include/i2c*` besides being shorter.
On Mon, 2021-01-18 at 15:53 +0200, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:28AM +0000, 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. > > Prefix: "mfd: tps68470: ". Rationale: easier to look for specific commits, by, > for example, running `git log --grep tps68470`. It's also reasonable to grep by path instead $ git log --pretty=oneline --grep 'tps68470' cf2e8c544cd3b33e9e403b7b72404c221bf888d1 Merge tag 'mfd-next-5.1' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd 66265e719b4603ef9a1b8a6c876bcb542c021496 mfd: tps68470: Drop unused MODULE_DEVICE_TABLE 883cad5ba8cc2d9b740b4ad0a8a91063c99c75a3 Merge tag 'mfd-next-4.18' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd ca34b4f0bed802e1c8612ef08456b20992aeb02a gpio: tps68470: Update to SPDX license identifier vs $ git log --pretty=oneline -- '*tps68470*' e42615ec233b30dfaf117b108d4cb49455b4df1d gpio: Use new GPIO_LINE_DIRECTION 66265e719b4603ef9a1b8a6c876bcb542c021496 mfd: tps68470: Drop unused MODULE_DEVICE_TABLE 36b835176fe014197639f335d9d35424b7805027 ACPI / PMIC: Sort headers alphabetically 37c089d1facaf03969f66a5469c169a2c73429f6 mfd: Update to SPDX license identifier 1b2951dd99af3970c1c1a8385a12b90236b837de Merge tag 'gpio-v4.17-1' of git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio ca34b4f0bed802e1c8612ef08456b20992aeb02a gpio: tps68470: Update to SPDX license identifier 66444f460e68d641a63f0787627bac6c1ee340b5 ACPI / PMIC: Replace license boilerplate with SPDX license identifier e13452ac379070f038c264618e35559434252175 ACPI / PMIC: Add TI PMIC TPS68470 operation region driver 968c61f7da3cf6d58a49587cfe00d899ca72c1ad Merge tag 'mfd-next-4.14' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd 9bbf6a15ce19dd947b7fa6ad4095931ab3682da8 mfd: Add support for TPS68470 device 275b13a65547e2dc39c75d660d2e0f0fddde90f6 gpio: Add support for TPS68470 GPIOs
On 18/01/2021 16:14, Rafael J. Wysocki wrote: > On Mon, Jan 18, 2021 at 1:37 AM 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 function to parse all ACPI Devices and check if >> the include the handle of the dependee device in their _DEP buffer. > What exactly do you need this for? So, in our DSDT we have devices with _HID INT3472, plus sensors which refer to those INT3472's in their _DEP method. The driver binds to the INT3472 device, we need to find the sensors dependent on them. > Would it be practical to look up the suppliers in acpi_dep_list instead? > > Note that supplier drivers may remove entries from there, but does > that matter for your use case? Ah - that may work, yes. Thank you, let me test that. > >> Signed-off-by: Daniel Scally <djrscally@gmail.com> >> --- >> Changes in v2: >> - Used acpi_lpss_dep() as Andy suggested. >> >> drivers/acpi/utils.c | 34 ++++++++++++++++++++++++++++++++++ >> include/acpi/acpi_bus.h | 2 ++ >> 2 files changed, 36 insertions(+) >> >> diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c >> index 78b38775f18b..ec6a2406a886 100644 >> --- a/drivers/acpi/utils.c >> +++ b/drivers/acpi/utils.c >> @@ -831,6 +831,18 @@ bool acpi_lpss_dep(struct acpi_device *adev, acpi_handle handle) >> return false; >> } >> >> +static int acpi_dev_match_by_dep(struct device *dev, const void *data) >> +{ >> + struct acpi_device *adev = to_acpi_device(dev); >> + const struct acpi_device *dependee = data; >> + acpi_handle handle = dependee->handle; >> + >> + if (acpi_lpss_dep(adev, handle)) >> + return 1; >> + >> + return 0; >> +} >> + >> /** >> * acpi_dev_present - Detect that a given ACPI device is present >> * @hid: Hardware ID of the device. >> @@ -866,6 +878,28 @@ bool acpi_dev_present(const char *hid, const char *uid, s64 hrv) >> } >> EXPORT_SYMBOL(acpi_dev_present); >> >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev >> + * @adev: Pointer to the dependee device >> + * @prev: Pointer to the previous dependent device (or NULL for first match) >> + * >> + * Return the next ACPI device which declares itself dependent on @adev in >> + * the _DEP buffer. >> + * >> + * The caller is responsible to call put_device() on the returned device. >> + */ >> +struct acpi_device *acpi_dev_get_next_dep_dev(struct acpi_device *adev, >> + struct acpi_device *prev) >> +{ >> + struct device *start = prev ? &prev->dev : NULL; >> + struct device *dev; >> + >> + dev = bus_find_device(&acpi_bus_type, start, adev, acpi_dev_match_by_dep); >> + >> + return dev ? to_acpi_device(dev) : NULL; >> +} >> +EXPORT_SYMBOL(acpi_dev_get_next_dep_dev); >> + >> /** >> * acpi_dev_get_next_match_dev - Return the next match of ACPI device >> * @adev: Pointer to the previous acpi_device matching this @hid, @uid and @hrv >> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h >> index 02a716a0af5d..33deb22294f2 100644 >> --- a/include/acpi/acpi_bus.h >> +++ b/include/acpi/acpi_bus.h >> @@ -683,6 +683,8 @@ 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); >> >> +struct acpi_device * >> +acpi_dev_get_next_dep_dev(struct acpi_device *adev, struct acpi_device *prev); >> 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 >>
On 18/01/2021 13:45, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 12:34:26AM +0000, Daniel Scally wrote: >> I need to be able to translate GPIO resources in an acpi_device's _CRS > > ACPI device's > >> into gpio_descs. Those are represented in _CRS as a pathname to a GPIO > > into GPIO descriptor array > >> device plus the pin's index number: this function is perfect for that >> purpose. > > ... > >> diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > Wrong header. Please use gpio/consumer.h. > Ack to all - thanks.
On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: > > On 18/01/2021 16:14, Rafael J. Wysocki wrote: > > On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: > >> In some ACPI tables we encounter, devices use the _DEP method to assert > >> a dependence on other ACPI devices as opposed to the OpRegions that the > >> specification intends. We need to be able to find those devices "from" > >> the dependee, so add a function to parse all ACPI Devices and check if > >> the include the handle of the dependee device in their _DEP buffer. > > What exactly do you need this for? > > So, in our DSDT we have devices with _HID INT3472, plus sensors which > refer to those INT3472's in their _DEP method. The driver binds to the > INT3472 device, we need to find the sensors dependent on them. > Well, this is an interesting concept. :-) Why does _DEP need to be used for that? Isn't there any other way to look up the dependent sensors? > > > Would it be practical to look up the suppliers in acpi_dep_list instead? > > > > Note that supplier drivers may remove entries from there, but does > > that matter for your use case? > > Ah - that may work, yes. Thank you, let me test that. Even if that doesn't work right away, but it can be made work, I would very much prefer that to the driver parsing _DEP for every device in the namespace by itself.
On Mon, Jan 18, 2021 at 9:55 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > Thank you for the patch. > > On Mon, Jan 18, 2021 at 12:34:25AM +0000, Daniel Scally wrote: > > We want to refer to an i2c device by name before it has been > > s/i2c device/acpi i2c device/ ? > > > created by the kernel; add a function that constructs the name > > from the acpi device instead. > > > > Signed-off-by: Daniel Scally <djrscally@gmail.com> > > --- > > Changes in v2: > > > > - Stopped using devm_kasprintf() > > > > drivers/i2c/i2c-core-acpi.c | 16 ++++++++++++++++ > > include/linux/i2c.h | 5 +++++ > > 2 files changed, 21 insertions(+) > > > > diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c > > index 37c510d9347a..98c3ba9a2350 100644 > > --- a/drivers/i2c/i2c-core-acpi.c > > +++ b/drivers/i2c/i2c-core-acpi.c > > @@ -497,6 +497,22 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index, > > } > > EXPORT_SYMBOL_GPL(i2c_acpi_new_device); > > > > +/** > > + * i2c_acpi_dev_name - Construct i2c device name for devs sourced from ACPI > > + * @adev: ACPI device to construct the name for > > + * > > + * Constructs the name of an i2c device matching the format used by > > + * i2c_dev_set_name() to allow users to refer to an i2c device by name even > > + * before they have been instantiated. > > + * > > + * The caller is responsible for freeing the returned pointer. > > + */ > > +char *i2c_acpi_dev_name(struct acpi_device *adev) > > +{ > > + return kasprintf(GFP_KERNEL, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > > There's a real danger of a memory leak, as the function name sounds very > similar to dev_name() or acpi_dev_name() and those don't allocate > memory. I'm not sure what a better name would be, but given that this > function is only used in patch 6/7 and not in the I2C subsystem itself, > I wonder if we should inline this kasprintf() call in the caller and > drop this patch. IMO if this is a one-off usage, it's better to open-code it.
On 19/01/2021 13:15, Rafael J. Wysocki wrote: > On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: >> On 18/01/2021 16:14, Rafael J. Wysocki wrote: >>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: >>>> In some ACPI tables we encounter, devices use the _DEP method to assert >>>> a dependence on other ACPI devices as opposed to the OpRegions that the >>>> specification intends. We need to be able to find those devices "from" >>>> the dependee, so add a function to parse all ACPI Devices and check if >>>> the include the handle of the dependee device in their _DEP buffer. >>> What exactly do you need this for? >> So, in our DSDT we have devices with _HID INT3472, plus sensors which >> refer to those INT3472's in their _DEP method. The driver binds to the >> INT3472 device, we need to find the sensors dependent on them. >> > Well, this is an interesting concept. :-) > > Why does _DEP need to be used for that? Isn't there any other way to > look up the dependent sensors? If there is, I'm not aware of it, I don't see a reference to the sensor in the INT3472 device (named "PMI0", with the corresponding sensor being "CAM0") in DSDT [1] >>> Would it be practical to look up the suppliers in acpi_dep_list instead? >>> >>> Note that supplier drivers may remove entries from there, but does >>> that matter for your use case? >> Ah - that may work, yes. Thank you, let me test that. > Even if that doesn't work right away, but it can be made work, I would > very much prefer that to the driver parsing _DEP for every device in > the namespace by itself. Alright; I haven't looked too closely yet, but I think an iterator over acpi_dep_list exported from the ACPI subsystem would also work in a pretty similar way to the function introduced in this patch does, without much work [1] https://gist.githubusercontent.com/djrscally/e64d112180517352fa3392878b0f4a7d/raw/88b90b3ea4204fd7845257b6666fdade47cc2981/dsdt.dsl
Hi Rafael On 19/01/2021 13:15, Rafael J. Wysocki wrote: > On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: >> On 18/01/2021 16:14, Rafael J. Wysocki wrote: >>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: >>>> In some ACPI tables we encounter, devices use the _DEP method to assert >>>> a dependence on other ACPI devices as opposed to the OpRegions that the >>>> specification intends. We need to be able to find those devices "from" >>>> the dependee, so add a function to parse all ACPI Devices and check if >>>> the include the handle of the dependee device in their _DEP buffer. >>> What exactly do you need this for? >> So, in our DSDT we have devices with _HID INT3472, plus sensors which >> refer to those INT3472's in their _DEP method. The driver binds to the >> INT3472 device, we need to find the sensors dependent on them. >> > Well, this is an interesting concept. :-) > > Why does _DEP need to be used for that? Isn't there any other way to > look up the dependent sensors? > >>> Would it be practical to look up the suppliers in acpi_dep_list instead? >>> >>> Note that supplier drivers may remove entries from there, but does >>> that matter for your use case? >> Ah - that may work, yes. Thank you, let me test that. > Even if that doesn't work right away, but it can be made work, I would > very much prefer that to the driver parsing _DEP for every device in > the namespace by itself. This does work; do you prefer it in scan.c, or in utils.c (in which case with acpi_dep_list declared as external var in internal.h)?
On 21/01/2021 11:58, Rafael J. Wysocki wrote: > On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@gmail.com> wrote: >> Hi Rafael >> >> On 19/01/2021 13:15, Rafael J. Wysocki wrote: >>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: >>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote: >>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: >>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert >>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the >>>>>> specification intends. We need to be able to find those devices "from" >>>>>> the dependee, so add a function to parse all ACPI Devices and check if >>>>>> the include the handle of the dependee device in their _DEP buffer. >>>>> What exactly do you need this for? >>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which >>>> refer to those INT3472's in their _DEP method. The driver binds to the >>>> INT3472 device, we need to find the sensors dependent on them. >>>> >>> Well, this is an interesting concept. :-) >>> >>> Why does _DEP need to be used for that? Isn't there any other way to >>> look up the dependent sensors? >>> >>>>> Would it be practical to look up the suppliers in acpi_dep_list instead? >>>>> >>>>> Note that supplier drivers may remove entries from there, but does >>>>> that matter for your use case? >>>> Ah - that may work, yes. Thank you, let me test that. >>> Even if that doesn't work right away, but it can be made work, I would >>> very much prefer that to the driver parsing _DEP for every device in >>> the namespace by itself. >> >> This does work; do you prefer it in scan.c, or in utils.c (in which case >> with acpi_dep_list declared as external var in internal.h)? > Let's put it in scan.c for now, because there is the lock protecting > the list in there too. > > How do you want to implement this? Something like "walk the list and > run a callback for the matching entries" or do you have something else > in mind? Something like this (though with a mutex_lock()). It could be simplified by dropping the prev stuff, but we have seen INT3472 devices with multiple sensors declaring themselves dependent on the same device struct acpi_device * acpi_dev_get_next_dependent_dev(struct acpi_device *supplier, struct acpi_device *prev) { struct acpi_dep_data *dep; struct acpi_device *adev; int ret; if (!supplier) return ERR_PTR(-EINVAL); if (prev) { /* * We need to find the previous device in the list, so we know * where to start iterating from. */ list_for_each_entry(dep, &acpi_dep_list, node) if (dep->consumer == prev->handle && dep->supplier == supplier->handle) break; dep = list_next_entry(dep, node); } else { dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data, node); } list_for_each_entry_from(dep, &acpi_dep_list, node) { if (dep->supplier == supplier->handle) { ret = acpi_bus_get_device(dep->consumer, &adev); if (ret) return ERR_PTR(ret); return adev; } } return NULL; }
On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally@gmail.com> wrote: > > > On 21/01/2021 11:58, Rafael J. Wysocki wrote: > > On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@gmail.com> wrote: > >> Hi Rafael > >> > >> On 19/01/2021 13:15, Rafael J. Wysocki wrote: > >>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: > >>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote: > >>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: > >>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert > >>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the > >>>>>> specification intends. We need to be able to find those devices "from" > >>>>>> the dependee, so add a function to parse all ACPI Devices and check if > >>>>>> the include the handle of the dependee device in their _DEP buffer. > >>>>> What exactly do you need this for? > >>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which > >>>> refer to those INT3472's in their _DEP method. The driver binds to the > >>>> INT3472 device, we need to find the sensors dependent on them. > >>>> > >>> Well, this is an interesting concept. :-) > >>> > >>> Why does _DEP need to be used for that? Isn't there any other way to > >>> look up the dependent sensors? > >>> > >>>>> Would it be practical to look up the suppliers in acpi_dep_list instead? > >>>>> > >>>>> Note that supplier drivers may remove entries from there, but does > >>>>> that matter for your use case? > >>>> Ah - that may work, yes. Thank you, let me test that. > >>> Even if that doesn't work right away, but it can be made work, I would > >>> very much prefer that to the driver parsing _DEP for every device in > >>> the namespace by itself. > >> > >> This does work; do you prefer it in scan.c, or in utils.c (in which case > >> with acpi_dep_list declared as external var in internal.h)? > > Let's put it in scan.c for now, because there is the lock protecting > > the list in there too. > > > > How do you want to implement this? Something like "walk the list and > > run a callback for the matching entries" or do you have something else > > in mind? > > > Something like this (though with a mutex_lock()). It could be simplified > by dropping the prev stuff, but we have seen INT3472 devices with > multiple sensors declaring themselves dependent on the same device > > > struct acpi_device * > acpi_dev_get_next_dependent_dev(struct acpi_device *supplier, > struct acpi_device *prev) > { > struct acpi_dep_data *dep; > struct acpi_device *adev; > int ret; > > if (!supplier) > return ERR_PTR(-EINVAL); > > if (prev) { > /* > * We need to find the previous device in the list, so we know > * where to start iterating from. > */ > list_for_each_entry(dep, &acpi_dep_list, node) > if (dep->consumer == prev->handle && > dep->supplier == supplier->handle) > break; > > dep = list_next_entry(dep, node); > } else { > dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data, > node); > } > > > list_for_each_entry_from(dep, &acpi_dep_list, node) { > if (dep->supplier == supplier->handle) { > ret = acpi_bus_get_device(dep->consumer, &adev); > if (ret) > return ERR_PTR(ret); > > return adev; > } > } > > return NULL; > } That would work I think, but would it be practical to modify acpi_walk_dep_device_list() so that it runs a callback for every consumer found instead of or in addition to the "delete from the list and free the entry" operation?
On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally <djrscally@gmail.com> wrote: > > > On 21/01/2021 14:39, Rafael J. Wysocki wrote: > > On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally@gmail.com> wrote: > >> > >> On 21/01/2021 11:58, Rafael J. Wysocki wrote: > >>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@gmail.com> wrote: > >>>> Hi Rafael > >>>> > >>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote: > >>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: > >>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote: > >>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: > >>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert > >>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the > >>>>>>>> specification intends. We need to be able to find those devices "from" > >>>>>>>> the dependee, so add a function to parse all ACPI Devices and check if > >>>>>>>> the include the handle of the dependee device in their _DEP buffer. > >>>>>>> What exactly do you need this for? > >>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which > >>>>>> refer to those INT3472's in their _DEP method. The driver binds to the > >>>>>> INT3472 device, we need to find the sensors dependent on them. > >>>>>> > >>>>> Well, this is an interesting concept. :-) > >>>>> > >>>>> Why does _DEP need to be used for that? Isn't there any other way to > >>>>> look up the dependent sensors? > >>>>> > >>>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead? > >>>>>>> > >>>>>>> Note that supplier drivers may remove entries from there, but does > >>>>>>> that matter for your use case? > >>>>>> Ah - that may work, yes. Thank you, let me test that. > >>>>> Even if that doesn't work right away, but it can be made work, I would > >>>>> very much prefer that to the driver parsing _DEP for every device in > >>>>> the namespace by itself. > >>>> This does work; do you prefer it in scan.c, or in utils.c (in which case > >>>> with acpi_dep_list declared as external var in internal.h)? > >>> Let's put it in scan.c for now, because there is the lock protecting > >>> the list in there too. > >>> > >>> How do you want to implement this? Something like "walk the list and > >>> run a callback for the matching entries" or do you have something else > >>> in mind? > >> > >> Something like this (though with a mutex_lock()). It could be simplified > >> by dropping the prev stuff, but we have seen INT3472 devices with > >> multiple sensors declaring themselves dependent on the same device > >> > >> > >> struct acpi_device * > >> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier, > >> struct acpi_device *prev) > >> { > >> struct acpi_dep_data *dep; > >> struct acpi_device *adev; > >> int ret; > >> > >> if (!supplier) > >> return ERR_PTR(-EINVAL); > >> > >> if (prev) { > >> /* > >> * We need to find the previous device in the list, so we know > >> * where to start iterating from. > >> */ > >> list_for_each_entry(dep, &acpi_dep_list, node) > >> if (dep->consumer == prev->handle && > >> dep->supplier == supplier->handle) > >> break; > >> > >> dep = list_next_entry(dep, node); > >> } else { > >> dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data, > >> node); > >> } > >> > >> > >> list_for_each_entry_from(dep, &acpi_dep_list, node) { > >> if (dep->supplier == supplier->handle) { > >> ret = acpi_bus_get_device(dep->consumer, &adev); > >> if (ret) > >> return ERR_PTR(ret); > >> > >> return adev; > >> } > >> } > >> > >> return NULL; > >> } > > That would work I think, but would it be practical to modify > > acpi_walk_dep_device_list() so that it runs a callback for every > > consumer found instead of or in addition to the "delete from the list > > and free the entry" operation? > > > I think that this would work fine, if that's the way you want to go. > We'd just need to move everything inside the if (dep->supplier == > handle) block to a new callback, and for my purposes I think also add a > way to stop parsing the list from the callback (so like have the > callbacks return int and stop parsing on a non-zero return). Do you want > to expose that ability to pass a callback outside of ACPI? Yes. > Or just export helpers to call each of the callbacks (one to fetch the next > dependent device, one to decrement the unmet dependencies counter) If you can run a callback for every matching entry, you don't really need to have a callback to return the next matching entry. You can do stuff for all of them in one go (note that it probably is not a good idea to run the callback under the lock, so the for loop currently in there is not really suitable for that). > Otherwise, I'd just need to update the 5 users of that function either > to use the new helper or else to also pass the decrement dependencies > callback. Or have a wrapper around it passing the decrement dependencies callback for the "typical" users.
> > There's a real danger of a memory leak, as the function name sounds very > > similar to dev_name() or acpi_dev_name() and those don't allocate > > memory. I'm not sure what a better name would be, but given that this > > function is only used in patch 6/7 and not in the I2C subsystem itself, > > I wonder if we should inline this kasprintf() call in the caller and > > drop this patch. > > IMO if this is a one-off usage, it's better to open-code it. Sounds reasonable to me, too.
> Just to clarify; "open-code" meaning inline it in the caller like > Laurent said, right? Yes.
On 28/01/2021 09:17, Wolfram Sang wrote: >> Just to clarify; "open-code" meaning inline it in the caller like >> Laurent said, right? > Yes. > Thanks - will do that and drop this one then
Hi Rafael On 21/01/2021 21:06, Daniel Scally wrote: > > On 21/01/2021 18:08, Rafael J. Wysocki wrote: >> On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally <djrscally@gmail.com> wrote: >>> >>> On 21/01/2021 14:39, Rafael J. Wysocki wrote: >>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally@gmail.com> wrote: >>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote: >>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@gmail.com> wrote: >>>>>>> Hi Rafael >>>>>>> >>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote: >>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: >>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote: >>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: >>>>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert >>>>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the >>>>>>>>>>> specification intends. We need to be able to find those devices "from" >>>>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check if >>>>>>>>>>> the include the handle of the dependee device in their _DEP buffer. >>>>>>>>>> What exactly do you need this for? >>>>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which >>>>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the >>>>>>>>> INT3472 device, we need to find the sensors dependent on them. >>>>>>>>> >>>>>>>> Well, this is an interesting concept. :-) >>>>>>>> >>>>>>>> Why does _DEP need to be used for that? Isn't there any other way to >>>>>>>> look up the dependent sensors? >>>>>>>> >>>>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead? >>>>>>>>>> >>>>>>>>>> Note that supplier drivers may remove entries from there, but does >>>>>>>>>> that matter for your use case? >>>>>>>>> Ah - that may work, yes. Thank you, let me test that. >>>>>>>> Even if that doesn't work right away, but it can be made work, I would >>>>>>>> very much prefer that to the driver parsing _DEP for every device in >>>>>>>> the namespace by itself. >>>>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case >>>>>>> with acpi_dep_list declared as external var in internal.h)? >>>>>> Let's put it in scan.c for now, because there is the lock protecting >>>>>> the list in there too. >>>>>> >>>>>> How do you want to implement this? Something like "walk the list and >>>>>> run a callback for the matching entries" or do you have something else >>>>>> in mind? >>>>> Something like this (though with a mutex_lock()). It could be simplified >>>>> by dropping the prev stuff, but we have seen INT3472 devices with >>>>> multiple sensors declaring themselves dependent on the same device >>>>> >>>>> >>>>> struct acpi_device * >>>>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier, >>>>> struct acpi_device *prev) >>>>> { >>>>> struct acpi_dep_data *dep; >>>>> struct acpi_device *adev; >>>>> int ret; >>>>> >>>>> if (!supplier) >>>>> return ERR_PTR(-EINVAL); >>>>> >>>>> if (prev) { >>>>> /* >>>>> * We need to find the previous device in the list, so we know >>>>> * where to start iterating from. >>>>> */ >>>>> list_for_each_entry(dep, &acpi_dep_list, node) >>>>> if (dep->consumer == prev->handle && >>>>> dep->supplier == supplier->handle) >>>>> break; >>>>> >>>>> dep = list_next_entry(dep, node); >>>>> } else { >>>>> dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data, >>>>> node); >>>>> } >>>>> >>>>> >>>>> list_for_each_entry_from(dep, &acpi_dep_list, node) { >>>>> if (dep->supplier == supplier->handle) { >>>>> ret = acpi_bus_get_device(dep->consumer, &adev); >>>>> if (ret) >>>>> return ERR_PTR(ret); >>>>> >>>>> return adev; >>>>> } >>>>> } >>>>> >>>>> return NULL; >>>>> } >>>> That would work I think, but would it be practical to modify >>>> acpi_walk_dep_device_list() so that it runs a callback for every >>>> consumer found instead of or in addition to the "delete from the list >>>> and free the entry" operation? >>> >>> I think that this would work fine, if that's the way you want to go. >>> We'd just need to move everything inside the if (dep->supplier == >>> handle) block to a new callback, and for my purposes I think also add a >>> way to stop parsing the list from the callback (so like have the >>> callbacks return int and stop parsing on a non-zero return). Do you want >>> to expose that ability to pass a callback outside of ACPI? >> Yes. >> >>> Or just export helpers to call each of the callbacks (one to fetch the next >>> dependent device, one to decrement the unmet dependencies counter) >> If you can run a callback for every matching entry, you don't really >> need to have a callback to return the next matching entry. You can do >> stuff for all of them in one go > > Well it my case it's more to return a pointer to the dep->consumer's > acpi_device for a matching entry, so my idea was where there's multiple > dependents you could use this as an iterator...but it could just be > extended to that if needed later; I don't actually need to do it right now. > > >> note that it probably is not a good >> idea to run the callback under the lock, so the for loop currently in >> there is not really suitable for that > > No problem; I'll tweak that then Slightly walking back my "No problem" here; as I understand this there's kinda two options: 1. Walk over the (locked) list, when a match is found unlock, run the callback and re-lock. The problem with that idea is unless I'm mistaken there's no guarantee that the .next pointer is still valid then (even using the *_safe() methods) because either the next or the next + 1 entry could have been removed whilst the list was unlocked and the callback was being ran, so this seems a little unsafe. 2. Walk over the (locked) list twice, the first time counting matching entries and using that to allocate a temporary buffer, then walk again to store the matching entries into the buffer. Finally, run the callback for everything in the buffer, free it and return. Obviously that's a lot less efficient than the current function, which isn't particularly palatable. Apologies if I've missed a better option that would work fine; but failing that do you still want me to go ahead and change acpi_walk_dep_device_list() to do this (I'd choose #2 of the above), or fallback to using acpi_dev_get_next_dependent_dev() described above? If the latter, does acpi_walk_dep_device_list() maybe need re-naming to make clear it's not a generalised function?
On Tue, Feb 02, 2021 at 09:58:17AM +0000, Daniel Scally wrote: > On 21/01/2021 21:06, Daniel Scally wrote: > > On 21/01/2021 18:08, Rafael J. Wysocki wrote: ... > > No problem; I'll tweak that then > > Slightly walking back my "No problem" here; as I understand this there's > kinda two options: > > 1. Walk over the (locked) list, when a match is found unlock, run the > callback and re-lock. > > The problem with that idea is unless I'm mistaken there's no guarantee > that the .next pointer is still valid then (even using the *_safe() > methods) because either the next or the next + 1 entry could have been > removed whilst the list was unlocked and the callback was being ran, so > this seems a little unsafe. It's easy to solve. See an example in deferred_probe_work_func(). https://elixir.bootlin.com/linux/latest/source/drivers/base/dd.c#L75 > 2. Walk over the (locked) list twice, the first time counting matching > entries and using that to allocate a temporary buffer, then walk again > to store the matching entries into the buffer. Finally, run the callback > for everything in the buffer, free it and return. > > Obviously that's a lot less efficient than the current function, which > isn't particularly palatable. > > Apologies if I've missed a better option that would work fine; but > failing that do you still want me to go ahead and change > acpi_walk_dep_device_list() to do this (I'd choose #2 of the above), or > fallback to using acpi_dev_get_next_dependent_dev() described above? If > the latter, does acpi_walk_dep_device_list() maybe need re-naming to > make clear it's not a generalised function?
On Tue, Feb 2, 2021 at 10:58 AM Daniel Scally <djrscally@gmail.com> wrote: > > Hi Rafael > > On 21/01/2021 21:06, Daniel Scally wrote: > > > > On 21/01/2021 18:08, Rafael J. Wysocki wrote: > >> On Thu, Jan 21, 2021 at 5:34 PM Daniel Scally <djrscally@gmail.com> wrote: > >>> > >>> On 21/01/2021 14:39, Rafael J. Wysocki wrote: > >>>> On Thu, Jan 21, 2021 at 1:04 PM Daniel Scally <djrscally@gmail.com> wrote: > >>>>> On 21/01/2021 11:58, Rafael J. Wysocki wrote: > >>>>>> On Thu, Jan 21, 2021 at 10:47 AM Daniel Scally <djrscally@gmail.com> wrote: > >>>>>>> Hi Rafael > >>>>>>> > >>>>>>> On 19/01/2021 13:15, Rafael J. Wysocki wrote: > >>>>>>>> On Mon, Jan 18, 2021 at 9:51 PM Daniel Scally <djrscally@gmail.com> wrote: > >>>>>>>>> On 18/01/2021 16:14, Rafael J. Wysocki wrote: > >>>>>>>>>> On Mon, Jan 18, 2021 at 1:37 AM Daniel Scally <djrscally@gmail.com> wrote: > >>>>>>>>>>> In some ACPI tables we encounter, devices use the _DEP method to assert > >>>>>>>>>>> a dependence on other ACPI devices as opposed to the OpRegions that the > >>>>>>>>>>> specification intends. We need to be able to find those devices "from" > >>>>>>>>>>> the dependee, so add a function to parse all ACPI Devices and check if > >>>>>>>>>>> the include the handle of the dependee device in their _DEP buffer. > >>>>>>>>>> What exactly do you need this for? > >>>>>>>>> So, in our DSDT we have devices with _HID INT3472, plus sensors which > >>>>>>>>> refer to those INT3472's in their _DEP method. The driver binds to the > >>>>>>>>> INT3472 device, we need to find the sensors dependent on them. > >>>>>>>>> > >>>>>>>> Well, this is an interesting concept. :-) > >>>>>>>> > >>>>>>>> Why does _DEP need to be used for that? Isn't there any other way to > >>>>>>>> look up the dependent sensors? > >>>>>>>> > >>>>>>>>>> Would it be practical to look up the suppliers in acpi_dep_list instead? > >>>>>>>>>> > >>>>>>>>>> Note that supplier drivers may remove entries from there, but does > >>>>>>>>>> that matter for your use case? > >>>>>>>>> Ah - that may work, yes. Thank you, let me test that. > >>>>>>>> Even if that doesn't work right away, but it can be made work, I would > >>>>>>>> very much prefer that to the driver parsing _DEP for every device in > >>>>>>>> the namespace by itself. > >>>>>>> This does work; do you prefer it in scan.c, or in utils.c (in which case > >>>>>>> with acpi_dep_list declared as external var in internal.h)? > >>>>>> Let's put it in scan.c for now, because there is the lock protecting > >>>>>> the list in there too. > >>>>>> > >>>>>> How do you want to implement this? Something like "walk the list and > >>>>>> run a callback for the matching entries" or do you have something else > >>>>>> in mind? > >>>>> Something like this (though with a mutex_lock()). It could be simplified > >>>>> by dropping the prev stuff, but we have seen INT3472 devices with > >>>>> multiple sensors declaring themselves dependent on the same device > >>>>> > >>>>> > >>>>> struct acpi_device * > >>>>> acpi_dev_get_next_dependent_dev(struct acpi_device *supplier, > >>>>> struct acpi_device *prev) > >>>>> { > >>>>> struct acpi_dep_data *dep; > >>>>> struct acpi_device *adev; > >>>>> int ret; > >>>>> > >>>>> if (!supplier) > >>>>> return ERR_PTR(-EINVAL); > >>>>> > >>>>> if (prev) { > >>>>> /* > >>>>> * We need to find the previous device in the list, so we know > >>>>> * where to start iterating from. > >>>>> */ > >>>>> list_for_each_entry(dep, &acpi_dep_list, node) > >>>>> if (dep->consumer == prev->handle && > >>>>> dep->supplier == supplier->handle) > >>>>> break; > >>>>> > >>>>> dep = list_next_entry(dep, node); > >>>>> } else { > >>>>> dep = list_first_entry(&acpi_dep_list, struct acpi_dep_data, > >>>>> node); > >>>>> } > >>>>> > >>>>> > >>>>> list_for_each_entry_from(dep, &acpi_dep_list, node) { > >>>>> if (dep->supplier == supplier->handle) { > >>>>> ret = acpi_bus_get_device(dep->consumer, &adev); > >>>>> if (ret) > >>>>> return ERR_PTR(ret); > >>>>> > >>>>> return adev; > >>>>> } > >>>>> } > >>>>> > >>>>> return NULL; > >>>>> } > >>>> That would work I think, but would it be practical to modify > >>>> acpi_walk_dep_device_list() so that it runs a callback for every > >>>> consumer found instead of or in addition to the "delete from the list > >>>> and free the entry" operation? > >>> > >>> I think that this would work fine, if that's the way you want to go. > >>> We'd just need to move everything inside the if (dep->supplier == > >>> handle) block to a new callback, and for my purposes I think also add a > >>> way to stop parsing the list from the callback (so like have the > >>> callbacks return int and stop parsing on a non-zero return). Do you want > >>> to expose that ability to pass a callback outside of ACPI? > >> Yes. > >> > >>> Or just export helpers to call each of the callbacks (one to fetch the next > >>> dependent device, one to decrement the unmet dependencies counter) > >> If you can run a callback for every matching entry, you don't really > >> need to have a callback to return the next matching entry. You can do > >> stuff for all of them in one go > > > > Well it my case it's more to return a pointer to the dep->consumer's > > acpi_device for a matching entry, so my idea was where there's multiple > > dependents you could use this as an iterator...but it could just be > > extended to that if needed later; I don't actually need to do it right now. > > > > > >> note that it probably is not a good > >> idea to run the callback under the lock, so the for loop currently in > >> there is not really suitable for that > > > > No problem; I'll tweak that then > > Slightly walking back my "No problem" here; as I understand this there's > kinda two options: > > 1. Walk over the (locked) list, when a match is found unlock, run the > callback and re-lock. That's what I was thinking about. > The problem with that idea is unless I'm mistaken there's no guarantee > that the .next pointer is still valid then (even using the *_safe() > methods) because either the next or the next + 1 entry could have been > removed whilst the list was unlocked and the callback was being ran, so > this seems a little unsafe. This can be addressed by rotating the list while walking it, but that becomes problematic if there are concurrent walkers. OK, I guess running the callback under the lock is not really a big deal (and for the deletion case this is actually necessary), so let's do that.