| 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 *
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:27AM +0000, Daniel Scally wrote: > ACPI devices with _HID INT3472 are currently matched to the tps68470 > driver, however this does not cover all situations in which that _HID > occurs. We've encountered three possibilities: > > 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing > a physical tps68470 device) that requires a GPIO and OpRegion driver > 2. On devices designed for Windows, an ACPI device with _HID INT3472 > (again representing a physical tps68470 device) which requires GPIO, > Clock and Regulator drivers. > 3. On other devices designed for Windows, an ACPI device with _HID > INT3472 which does NOT represent a physical tps68470, and is instead > used as a dummy device to group some system GPIO lines which are meant > to be consumed by the sensor that is dependent on this entry. > > This commit adds a new module, registering a platform driver to deal > with the 3rd scenario plus an i2c-driver to deal with #1 and #2, by > querying the CLDB buffer found against INT3472 entries to determine > which is most appropriate. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Switched to a module registering a platform driver to run > the dummy ACPI devices, plus an i2c driver to replace and extend > the existing tps68470 driver > - Added clock handling functions to the int3472-discrete driver > - A whole bunch of other changes too numerous to enumerate > MAINTAINERS | 5 + > drivers/platform/x86/Kconfig | 25 + > drivers/platform/x86/Makefile | 4 + > .../platform/x86/intel_skl_int3472_common.c | 100 ++++ > .../platform/x86/intel_skl_int3472_common.h | 100 ++++ > .../platform/x86/intel_skl_int3472_discrete.c | 496 ++++++++++++++++++ > .../platform/x86/intel_skl_int3472_tps68470.c | 145 +++++ > 7 files changed, 875 insertions(+) > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h > create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a091b496fdd8..c4ed8c3bc58e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9147,6 +9147,11 @@ S: Maintained > F: arch/x86/include/asm/intel_scu_ipc.h > F: drivers/platform/x86/intel_scu_* > > +INTEL SKL INT3472 ACPI DEVICE DRIVER > +M: Daniel Scally <djrscally@gmail.com> > +S: Maintained > +F: drivers/platform/x86/intel_skl_int3472_* > + > INTEL SPEED SELECT TECHNOLOGY > M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > L: platform-driver-x86@vger.kernel.org > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 91e6176cdfbd..916b077df2d5 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -844,6 +844,31 @@ config INTEL_CHT_INT33FE > device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m > for Type-C device. > > +config INTEL_SKL_INT3472 > + tristate "Intel SkyLake ACPI INT3472 Driver" > + depends on X86 && ACPI > + select REGMAP_I2C > + help > + This driver adds support for the INT3472 ACPI devices found on some > + Intel SkyLake devices. > + > + There are 3 kinds of INT3472 ACPI device possible; two for devices > + designed for Windows (either with or without a physical tps68470 I'd capitalize TPS, chip names are usually written in upper case, and driver names in lower case. > + PMIC) and one designed for Chrome OS. How about expanding this a bit to explain what the INT3472 stands for ? The INT3472 is an Intel camera power controller, a logical device found on some Skylake-based systems that can map to different hardware devices depending on the platform. On machines designed for Chrome OS, it maps to a TPS68470 camera PMIC. On machines designed for Windows, it maps to either a TP68470 camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs and power gates. > This driver handles all three > + situations by discovering information it needs to discern them at > + runtime. > + > + If your device was designed for Chrome OS, this driver will provide > + an ACPI operation region, which must be available before any of the > + devices using this are probed. For this reason, you should select Y > + if your device was designed for ChromeOS. This option also configures > + the designware-i2c driver to be built-in, for the same reason. Is the last sentence a leftover ? > + > + Say Y or M here if you have a SkyLake device designed for use > + with Windows or ChromeOS. Say N here if you are not sure. > + > + The module will be named "intel-skl-int3472" > + > config INTEL_HID_EVENT > tristate "INTEL HID Event" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 581475f59819..ae29c66842ca 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -86,6 +86,10 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o > obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o > obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o > obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o > +obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472.o > +intel_skl_int3472-objs := intel_skl_int3472_common.o \ > + intel_skl_int3472_discrete.o \ > + intel_skl_int3472_tps68470.o > obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o > > # MSI > diff --git a/drivers/platform/x86/intel_skl_int3472_common.c b/drivers/platform/x86/intel_skl_int3472_common.c > new file mode 100644 > index 000000000000..08cb9d3c06aa > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_common.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/acpi.h> > +#include <linux/i2c.h> > +#include <linux/platform_device.h> #include <linux/slab.h> for kfree(). > + > +#include "intel_skl_int3472_common.h" > + > +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > + struct int3472_cldb *cldb) > +{ > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_handle handle = adev->handle; > + union acpi_object *obj; > + acpi_status status; > + int ret = 0; > + > + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + obj = buffer.pointer; > + if (!obj) { > + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); Is this the code path that is taken on Chrome OS ? If so an error message isn't appropriate. I'd drop this message, and instead add an error message in the discrete PMIC code. > + return -ENODEV; > + } > + > + if (obj->type != ACPI_TYPE_BUFFER) { > + dev_err(&adev->dev, "CLDB object is not an ACPI buffer\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + if (obj->buffer.length > sizeof(*cldb)) { > + dev_err(&adev->dev, "The CLDB buffer is too large\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + memcpy(cldb, obj->buffer.pointer, obj->buffer.length); > + > +out_free_buff: > + kfree(buffer.pointer); > + return ret; > +} > + > +static const struct acpi_device_id int3472_device_id[] = { > + { "INT3472", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, int3472_device_id); > + > +static struct platform_driver int3472_discrete = { > + .driver = { > + .name = "int3472-discrete", > + .acpi_match_table = int3472_device_id, > + }, > + .probe = skl_int3472_discrete_probe, > + .remove = skl_int3472_discrete_remove, > +}; > + > +static struct i2c_driver int3472_tps68470 = { > + .driver = { > + .name = "int3472-tps68470", > + .acpi_match_table = int3472_device_id, > + }, > + .probe_new = skl_int3472_tps68470_probe, > +}; > + > +static int skl_int3472_init(void) > +{ > + int ret = 0; > + > + ret = platform_driver_register(&int3472_discrete); > + if (ret) > + return ret; > + > + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470); > + if (ret) > + goto err_unregister_plat_drv; You could call platform_driver_unregister() directly here and return ret. I don't expect this function will need to be extended with a more complex error handling code path. > + > + return 0; > + > +err_unregister_plat_drv: > + platform_driver_unregister(&int3472_discrete); > + return ret; > +} > +module_init(skl_int3472_init); > + > +static void skl_int3472_exit(void) > +{ > + platform_driver_unregister(&int3472_discrete); > + i2c_del_driver(&int3472_tps68470); > +} > +module_exit(skl_int3472_exit); > + > +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver"); > +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/platform/x86/intel_skl_int3472_common.h b/drivers/platform/x86/intel_skl_int3472_common.h > new file mode 100644 > index 000000000000..9acd0ce7aed9 > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_common.h > @@ -0,0 +1,100 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Author: Dan Scally <djrscally@gmail.com> */ Missing blank line, and missing header guard. > +#include <linux/regulator/machine.h> Alphabetically sorted please. > +#include <linux/clk-provider.h> > +#include <linux/gpio/machine.h> You can drop this and forward-declare struct gpio_desc. > +#include <linux/regulator/driver.h> > +#include <linux/types.h> > + > +/* PMIC GPIO Types */ > +#define INT3472_GPIO_TYPE_RESET 0x00 > +#define INT3472_GPIO_TYPE_POWERDOWN 0x01 > +#define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c > +#define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b Should the macros be sorted by value ? > +#define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d Blank line here ? > +#define INT3472_PDEV_MAX_NAME_LEN 23 > +#define INT3472_MAX_SENSOR_GPIOS 3 And blank line here too ? > +#define GPIO_REGULATOR_NAME_LENGTH 27 > +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 > + > +#define INT3472_REGULATOR(_NAME, _SUPPLY, _OPS) \ Macro arguments don't need to be capitalized. > + (const struct regulator_desc) { \ > + .name = _NAME, \ > + .supply_name = _SUPPLY, \ > + .id = 0, \ > + .type = REGULATOR_VOLTAGE, \ > + .ops = _OPS, \ > + .owner = THIS_MODULE, \ > + } > + > +#define INT3472_GPIO_FUNCTION_REMAP(_PIN, _FUNCTION) \ > + (const struct int3472_gpio_function_remap) { \ > + .documented = _PIN, \ > + .actual = _FUNCTION \ > + } > + > +#define to_int3472_clk(hw) \ > + container_of(hw, struct int3472_gpio_clock, clk_hw) > + > +struct int3472_cldb { > + u8 version; > + /* > + * control logic type > + * 0: UNKNOWN > + * 1: DISCRETE(CRD-D) > + * 2: PMIC TPS68470 > + * 3: PMIC uP6641 > + */ > + u8 control_logic_type; > + u8 control_logic_id; > + u8 sensor_card_sku; > + u8 reserved[28]; > +}; > + > +struct int3472_gpio_regulator { > + char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > + char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; > + struct gpio_desc *gpio; > + struct regulator_dev *rdev; > + struct regulator_desc rdesc; > +}; > + > +struct int3472_gpio_clock { > + struct clk *clk; > + struct clk_hw clk_hw; > + struct clk_lookup *cl; > + struct gpio_desc *gpio; > +}; > + > +struct int3472_device { Maybe int3472_discrete_device ? > + struct acpi_device *adev; > + struct platform_device *pdev; > + struct acpi_device *sensor; > + char *sensor_name; const ? > + > + unsigned int n_gpios; /* how many GPIOs have we seen */ > + > + struct int3472_gpio_regulator regulator; > + struct int3472_gpio_clock clock; You don't necessarily need to define separate structures for this, you could also write struct { char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; struct gpio_desc *gpio; struct regulator_dev *rdev; struct regulator_desc rdesc; } regulator; struct { struct clk *clk; struct clk_hw clk_hw; struct clk_lookup *cl; struct gpio_desc *gpio; } clock; It's entirely up to you. > + > + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ > + bool gpios_mapped; > + struct gpiod_lookup_table gpios; > +}; > + > +struct int3472_gpio_function_remap { > + char *documented; > + char *actual; const for both ? > +}; > + > +struct int3472_sensor_config { > + char *sensor_module_name; > + struct regulator_consumer_supply supply_map; > + const struct int3472_gpio_function_remap *function_maps; > +}; > + > +int skl_int3472_discrete_probe(struct platform_device *pdev); > +int skl_int3472_discrete_remove(struct platform_device *pdev); > +int skl_int3472_tps68470_probe(struct i2c_client *client); > +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > + struct int3472_cldb *cldb); struct platform_device, struct i2c_client and strct acpi_device should be forward-declared at the top. > diff --git a/drivers/platform/x86/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel_skl_int3472_discrete.c > new file mode 100644 > index 000000000000..93d250dca92f > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_discrete.c > @@ -0,0 +1,496 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/acpi.h> > +#include <linux/clkdev.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/driver.h> > + > +#include "intel_skl_int3472_common.h" > + > +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */ > +static const guid_t int3472_gpio_guid = > + GUID_INIT(0x79234640, 0x9e10, 0x4fea, > + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); > + > +/* 822ace8f-2814-4174-a56b-5f029fe079ee */ > +static const guid_t cio2_sensor_module_guid = > + GUID_INIT(0x822ace8f, 0x2814, 0x4174, > + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); A comment that explains what those DSM functions do would be useful for reference. It has taken lots of time to figure it out, let's spare the pain to the next person who tries to understand this :-) > + > +/* > + * Here follows platform specific mapping information that we can pass to > + * the functions mapping resources to the sensors. Where the sensors have > + * a power enable pin defined in DSDT we need to provide a supply name so > + * the sensor drivers can find the regulator. Optionally, we can provide a > + * NULL terminated array of function name mappings to deal with any platform > + * specific deviations from the documented behaviour of GPIOs. > + * > + * Map a GPIO function name to NULL to prevent the driver from mapping that > + * GPIO at all. > + */ > + > +static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = { > + INT3472_GPIO_FUNCTION_REMAP("reset", NULL), Does the macro increase readability compared to writing { "reset", NULL }, ? I think I'd drop it. > + INT3472_GPIO_FUNCTION_REMAP("powerdown", "reset"), > + { } > +}; > + > +static struct int3472_sensor_config int3472_sensor_configs[] = { static const struct > + /* Lenovo Miix 510-12ISK - OV2680, Front */ > + { "GNDF140809R", { 0 }, ov2680_gpio_function_remaps}, > + /* Lenovo Miix 510-12ISK - OV5648, Rear */ > + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, > + /* Surface Go 1&2 - OV5693, Front */ > + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, > +}; > + > +/* > + * The regulators have to have .ops to be valid, but the only ops we actually > + * support are .enable and .disable which are handled via .ena_gpiod. Pass an > + * empty struct to clear the check without lying about capabilities. > + */ > +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 }; > + > +static int skl_int3472_clk_enable(struct clk_hw *hw) > +{ > + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > + > + gpiod_set_value(clk->gpio, 1); The clock enable() and disable() methods are not supposed to sleep, while setting a GPIO value may sleep in the general case. Should this be moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable() and skl_int3472_clk_unprepare(). > + > + return 0; > +} > + > +static void skl_int3472_clk_disable(struct clk_hw *hw) > +{ > + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > + > + gpiod_set_value(clk->gpio, 0); > +} > + > +static int skl_int3472_clk_prepare(struct clk_hw *hw) > +{ > + /* > + * We're just turning a GPIO on to enable, so nothing to do here, but > + * we want to provide the op so prepare_enable() works. > + */ > + return 0; > +} > + > +static void skl_int3472_clk_unprepare(struct clk_hw *hw) > +{ > + /* Likewise, nothing to do here... */ > +} > + > +static const struct clk_ops skl_int3472_clock_ops = { > + .prepare = skl_int3472_clk_prepare, > + .unprepare = skl_int3472_clk_unprepare, > + .enable = skl_int3472_clk_enable, > + .disable = skl_int3472_clk_disable, > +}; > + > +static struct int3472_sensor_config * > +skl_int3472_get_sensor_module_config(struct int3472_device *int3472) > +{ > + unsigned int i = ARRAY_SIZE(int3472_sensor_configs); > + struct int3472_sensor_config *ret; > + union acpi_object *obj; > + > + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, > + &cio2_sensor_module_guid, 0x00, > + 0x01, NULL, ACPI_TYPE_STRING); > + > + if (!obj) { > + dev_err(&int3472->pdev->dev, > + "Failed to get sensor module string from _DSM\n"); > + return ERR_PTR(-ENODEV); > + } > + > + if (obj->string.type != ACPI_TYPE_STRING) { > + dev_err(&int3472->pdev->dev, > + "Sensor _DSM returned a non-string value\n"); > + ret = ERR_PTR(-EINVAL); > + goto out_free_obj; > + } > + > + ret = ERR_PTR(-ENODEV); > + while (i--) { Unless there's a specific need to iterate backward over the array, for (i = 0; i < ARRAY_SIZE(int3472_sensor_configs); ++i) { > + if (!strcmp(int3472_sensor_configs[i].sensor_module_name, > + obj->string.pointer)) { > + ret = &int3472_sensor_configs[i]; Just break; > + goto out_free_obj; > + } > + } > + > +out_free_obj: > + ACPI_FREE(obj); > + return ret; > +} > + > +static int skl_int3472_map_gpio_to_sensor(struct int3472_device *int3472, > + struct acpi_resource *ares, > + char *func, u32 polarity) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; const > + struct int3472_sensor_config *sensor_config; > + struct gpiod_lookup table_entry; > + struct acpi_device *adev; > + acpi_handle handle; > + acpi_status status; > + int ret; > + > + sensor_config = skl_int3472_get_sensor_module_config(int3472); > + if (!IS_ERR(sensor_config) && sensor_config->function_maps) { > + unsigned int i = 0; const struct int3472_gpio_function_remap *remap = sensor_config->function_maps; for (; remap->documented; ++remap) { if (!strcmp(func, remap->documented)) { func = remap->actual; > + > + while (sensor_config->function_maps[i].documented) { > + if (!strcmp(func, sensor_config->function_maps[i].documented)) { > + func = sensor_config->function_maps[i].actual; > + Extra blank line. > + break; > + } > + > + i++; > + } > + } > + > + if (!func) > + return 0; I initially thought this wasn't right, as if no entry was found in the mapping table, func would still have its non-NULL value as passed to this function. I then realized that you're checking if the match that was found is NULL. A comment to explain this would be useful. > + > + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) { > + dev_warn(&int3472->pdev->dev, "Too many GPIOs mapped\n"); > + return -EINVAL; > + } You can move this to the top. > + > + status = acpi_get_handle(NULL, path, &handle); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + ret = acpi_bus_get_device(handle, &adev); > + if (ret) > + return -ENODEV; > + > + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > + ares->data.gpio.pin_table[0], > + func, 0, polarity); I wonder if table_entry.key = acpi_dev_name(adev); table_entry.chip_hwnum = ares->data.gpio.pin_table[0]; table_entry.con_id = func; table_entry.idx = 0; table_entry.flags = polarity; (with struct gpiod_lookup table_entry = { }; above) would be more readable. Up to you. > + > + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, > + sizeof(table_entry)); Ah, or maybe struct gpio_lookup *table_entry; table_entry = &int3472->gpios.table[int3472->n_sensor_gpios]; table_entry->key = acpi_dev_name(adev); table_entry->chip_hwnum = ares->data.gpio.pin_table[0]; table_entry->con_id = func; table_entry->idx = 0; table_entry->flags = polarity; (no need to memset() to 0 first as the whole structure has been allocated with kzalloc()). > + > + int3472->n_sensor_gpios++; > + > + return 0; > +} > + > +static int skl_int3472_register_clock(struct int3472_device *int3472, > + struct acpi_resource *ares) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; const > + struct clk_init_data init = { }; struct clk_init_data init = { .ops = &skl_int3472_clock_ops, }; and drop it from below (up to you). > + int ret = 0; > + > + init.name = kasprintf(GFP_KERNEL, "%s-clk", > + acpi_dev_name(int3472->adev)); You need to check for NULL and return -ENOMEM. > + init.ops = &skl_int3472_clock_ops; > + > + int3472->clock.gpio = acpi_get_gpiod(path, > + ares->data.gpio.pin_table[0]); > + if (IS_ERR(int3472->clock.gpio)) { > + ret = PTR_ERR(int3472->clock.gpio); > + goto out_free_init_name; > + } > + > + int3472->clock.clk_hw.init = &init; > + int3472->clock.clk = clk_register(&int3472->adev->dev, > + &int3472->clock.clk_hw); > + if (IS_ERR(int3472->clock.clk)) { > + ret = PTR_ERR(int3472->clock.clk); > + goto err_put_gpio; > + } > + > + int3472->clock.cl = clkdev_create(int3472->clock.clk, "xvclk", > + int3472->sensor_name); > + if (IS_ERR_OR_NULL(int3472->clock.cl)) if (!int3472->clock.cl) as the function never return an error pointer. > + goto err_unregister_clk; If this fails, you will end up calling clk_unregister() and clkdev_drop() in skl_int3472_discrete_remove(). You should replace the check in the remove function with if (!int3472->clock.cl) { > + > + goto out_free_init_name; > + > +err_unregister_clk: > + clk_unregister(int3472->clock.clk); > +err_put_gpio: > + gpiod_put(int3472->clock.gpio); > +out_free_init_name: > + kfree(init.name); > + > + return ret; > +} > + > +static int skl_int3472_register_regulator(struct int3472_device *int3472, > + struct acpi_resource *ares) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; const > + struct int3472_sensor_config *sensor_config; > + struct regulator_init_data init_data = { }; > + struct regulator_config cfg = { }; > + int ret; > + > + sensor_config = skl_int3472_get_sensor_module_config(int3472); > + if (IS_ERR_OR_NULL(sensor_config)) { The function never returns NULL, you can use IS_ERR(). > + dev_err(&int3472->pdev->dev, "No sensor module config\n"); > + return PTR_ERR(sensor_config); > + } Would it make sense to call this in skl_int3472_discrete_probe() or skl_int3472_parse_crs() and cache the config pointer ? > + > + if (!sensor_config->supply_map.supply) { > + dev_err(&int3472->pdev->dev, "No supply name defined\n"); > + return -ENODEV; > + } > + > + init_data.supply_regulator = NULL; Not needed, you initialize init_data to 0 when declaring it. > + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; > + init_data.num_consumer_supplies = 1; > + init_data.consumer_supplies = &sensor_config->supply_map; > + > + snprintf(int3472->regulator.regulator_name, > + GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator"); s/GPIO_REGULATOR_NAME_LENGTH/sizeof(int3472->regulator.regulator_name)/ Do regulator names need to be unique ? If so you may have a problem if a platform has two discrete INT3472. > + snprintf(int3472->regulator.supply_name, > + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0"); > + > + int3472->regulator.rdesc = INT3472_REGULATOR( > + int3472->regulator.regulator_name, > + int3472->regulator.supply_name, > + &int3472_gpio_regulator_ops); > + > + int3472->regulator.gpio = acpi_get_gpiod(path, > + ares->data.gpio.pin_table[0]); > + if (IS_ERR(int3472->regulator.gpio)) { > + dev_err(&int3472->pdev->dev, "Failed to get GPIO line\n"); s/GPIO line/regulator GPIO lines/ ? > + return PTR_ERR(int3472->regulator.gpio); > + } > + > + cfg.dev = &int3472->adev->dev; > + cfg.init_data = &init_data; > + cfg.ena_gpiod = int3472->regulator.gpio; > + > + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, > + &cfg); > + if (IS_ERR(int3472->regulator.rdev)) { > + ret = PTR_ERR(int3472->regulator.rdev); > + goto err_free_gpio; > + } > + > + return 0; > + > +err_free_gpio: > + gpiod_put(int3472->regulator.gpio); > + > + return ret; > +} > + > +/** > + * skl_int3472_handle_gpio_resources: maps PMIC resources to consuming sensor s/: maps/ - Map/ > + * @ares: A pointer to a &struct acpi_resource > + * @data: A pointer to a &struct int3472_device > + * > + * This function handles GPIO resources that are against an INT3472 > + * ACPI device, by checking the value of the corresponding _DSM entry. > + * This will return a 32bit int, where the lowest byte represents the > + * function of the GPIO pin: > + * > + * 0x00 Reset > + * 0x01 Power down > + * 0x0b Power enable > + * 0x0c Clock enable > + * 0x0d Privacy LED > + * > + * There are some known platform specific quirks where that does not quite > + * hold up; for example where a pin with type 0x01 (Power down) is mapped to > + * a sensor pin that performs a reset function or entries in _CRS and _DSM that > + * do not actually correspond to a physical connection. These will be handled by > + * the mapping sub-functions. > + * > + * GPIOs will either be mapped directly to the sensor device or else used > + * to create clocks and regulators via the usual frameworks. > + * > + * Return: > + * * 0 - When all resources found are handled properly. > + * * -EINVAL - If the resource is not a GPIO IO resource > + * * -ENODEV - If the resource has no corresponding _DSM entry > + * * -Other - Errors propagated from one of the sub-functions. > + */ > +static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > + void *data) > +{ > + struct int3472_device *int3472 = data; > + union acpi_object *obj; > + int ret = 0; > + > + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || > + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) > + return EINVAL; /* Deliberately positive so parsing continues */ Maybe 1 instead of EINVAL ? > + > + /* > + * n_gpios + 2 because the index of this _DSM function is 1-based and > + * the first function is just a count. > + */ > + obj = acpi_evaluate_dsm_typed(int3472->adev->handle, > + &int3472_gpio_guid, 0x00, > + int3472->n_gpios + 2, > + NULL, ACPI_TYPE_INTEGER); > + > + if (!obj) { > + dev_warn(&int3472->pdev->dev, > + "No _DSM entry for this GPIO pin\n"); s/this GPIO pin/GPIO pin %u/ (and add the corresponding argument). > + return ENODEV; return 1; > + } > + > + switch (obj->integer.value & 0xff) { > + case INT3472_GPIO_TYPE_RESET: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "reset", > + GPIO_ACTIVE_LOW); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map reset pin to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_POWERDOWN: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "powerdown", > + GPIO_ACTIVE_LOW); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map powerdown pin to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_CLK_ENABLE: > + ret = skl_int3472_register_clock(int3472, ares); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map clock to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_POWER_ENABLE: > + ret = skl_int3472_register_regulator(int3472, ares); > + if (ret) { > + dev_err(&int3472->pdev->dev, > + "Failed to map regulator to sensor\n"); > + } > + > + break; > + case INT3472_GPIO_TYPE_PRIVACY_LED: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, > + "indicator-led", > + GPIO_ACTIVE_HIGH); Mapping the indicator LED to the sensor isn't great, as all sensor drivers would then need to handle it. Could it be handled in the regulator instead, so that it would be turned on automatically when the sensor is powered up ? Another option, more complicated, would be to handle it in the CIO2 driver (but I'm not sure how we would map it). > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map indicator led to sensor\n"); > + > + break; > + default: > + dev_warn(&int3472->pdev->dev, > + "GPIO type 0x%llx unknown; the sensor may not work\n", > + (obj->integer.value & 0xff)); No need for parentheses. > + ret = EINVAL; ret = 1; break; > + } > + > + int3472->n_gpios++; > + ACPI_FREE(obj); > + > + return ret; > +} > + > +static int skl_int3472_parse_crs(struct int3472_device *int3472) > +{ > + struct list_head resource_list; > + int ret = 0; No need to initialize ret to 0. > + > + INIT_LIST_HEAD(&resource_list); > + > + ret = acpi_dev_get_resources(int3472->adev, &resource_list, > + skl_int3472_handle_gpio_resources, > + int3472); > + You can drop this blank line. > + if (!ret) { > + gpiod_add_lookup_table(&int3472->gpios); > + int3472->gpios_mapped = true; > + } > + > + acpi_dev_free_resource_list(&resource_list); > + > + return ret; > +} > + > +int skl_int3472_discrete_probe(struct platform_device *pdev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); > + struct int3472_device *int3472; > + struct int3472_cldb cldb; > + int ret = 0; No need to initialize ret to 0. > + > + ret = skl_int3472_get_cldb_buffer(adev, &cldb); > + if (ret || cldb.control_logic_type != 1) An error message is needed for unsupported control_logic_type values. > + return -EINVAL; > + > + int3472 = kzalloc(sizeof(*int3472) + > + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), One more space for the indentation, and the outer parentheses are not needed. > + GFP_KERNEL); > + if (!int3472) > + return -ENOMEM; > + > + int3472->adev = adev; > + int3472->pdev = pdev; > + platform_set_drvdata(pdev, int3472); > + > + int3472->sensor = acpi_dev_get_next_dep_dev(adev, NULL); > + if (!int3472->sensor) { > + dev_err(&pdev->dev, > + "This INT3472 entry seems to have no dependents.\n"); s/This INT3472 entry/INT3472/ > + ret = -ENODEV; > + goto err_free_int3472; > + } > + int3472->sensor_name = i2c_acpi_dev_name(int3472->sensor); > + int3472->gpios.dev_id = int3472->sensor_name; > + > + ret = skl_int3472_parse_crs(int3472); > + if (ret) { > + skl_int3472_discrete_remove(pdev); > + goto err_return_ret; Just return ret; will do. > + } > + > + return 0; > + > +err_free_int3472: > + kfree(int3472); > +err_return_ret: > + return ret; > +} > + > +int skl_int3472_discrete_remove(struct platform_device *pdev) > +{ > + struct int3472_device *int3472; > + > + int3472 = platform_get_drvdata(pdev); struct int3472_device *int3472 = platform_get_drvdata(pdev); > + > + if (int3472->gpios_mapped) > + gpiod_remove_lookup_table(&int3472->gpios); You could avoid the need for the gpios_mapped field by checking for if (int3472->gpios.list.next) Up to you. > + > + if (!IS_ERR_OR_NULL(int3472->regulator.rdev)) { > + gpiod_put(int3472->regulator.gpio); > + regulator_unregister(int3472->regulator.rdev); You should call regulator_unregister(), in case unregistering the regulator tries to turn it off. > + } > + > + if (!IS_ERR_OR_NULL(int3472->clock.clk)) { > + gpiod_put(int3472->clock.gpio); > + clk_unregister(int3472->clock.clk); > + clkdev_drop(int3472->clock.cl); Same here, gpiod_put() should go last. > + } > + > + acpi_dev_put(int3472->sensor); > + > + kfree(int3472->sensor_name); > + kfree(int3472); > + > + return 0; > +} > diff --git a/drivers/platform/x86/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel_skl_int3472_tps68470.c > new file mode 100644 > index 000000000000..9bda65d34987 > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_tps68470.c > @@ -0,0 +1,145 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/i2c.h> > +#include <linux/mfd/tps68470.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include "intel_skl_int3472_common.h" > + > +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); 0x%02x > + > + return 0; > +} > + > +static struct platform_device * > +skl_int3472_register_pdev(const char *name, struct device *parent) > +{ > + struct platform_device *pdev; > + int ret; > + > + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE); > + if (IS_ERR_OR_NULL(pdev)) > + return ERR_PTR(-ENOMEM); > + > + pdev->dev.parent = parent; > + pdev->driver_override = kstrndup(pdev->name, INT3472_PDEV_MAX_NAME_LEN, > + GFP_KERNEL); > + > + ret = platform_device_add(pdev); > + if (ret) { > + platform_device_put(pdev); > + return ERR_PTR(ret); > + } > + > + return pdev; > +} > + > +int skl_int3472_tps68470_probe(struct i2c_client *client) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&client->dev); > + struct platform_device *regulator_dev; > + struct platform_device *opregion_dev; > + struct platform_device *gpio_dev; > + struct int3472_cldb cldb = { 0 }; > + struct platform_device *clk_dev; > + bool cldb_present = true; > + struct regmap *regmap; > + int ret = 0; > + > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(&client->dev, "devm_regmap_init_i2c Error %ld\n", s/Error/error/ I'd actually write "Failed to create regmap" or something similar. > + PTR_ERR(regmap)); > + return PTR_ERR(regmap); > + } > + > + i2c_set_clientdata(client, regmap); > + > + ret = tps68470_chip_init(&client->dev, regmap); > + if (ret < 0) { > + dev_err(&client->dev, "TPS68470 Init Error %d\n", ret); s/Init Error/init error/ > + return ret; > + } > + > + /* > + * Check CLDB buffer against the PMIC's adev. If present, then we check > + * the value of control_logic_type field and follow one of the > + * following scenarios: > + * > + * 1. No CLDB - likely ACPI tables designed for ChromeOS. We > + * create platform devices for the GPIOs and OpRegion drivers. > + * > + * 2. CLDB, with control_logic_type = 2 - probably ACPI tables > + * made for Windows 2-in-1 platforms. Register pdevs for GPIO, > + * Clock and Regulator drivers to bind to. > + * > + * 3. Any other value in control_logic_type, we should never have > + * gotten to this point; crash and burn. Crash is a bit hard, and that's not what you do below. > + */ > + ret = skl_int3472_get_cldb_buffer(adev, &cldb); > + if (!ret && cldb.control_logic_type != 2) You should print an error message here (at least to catch an unsupported control_logic_type value, skl_int3472_get_cldb_buffer() errors are already logged in that function). > + return -EINVAL; > + > + if (ret) > + cldb_present = false; > + > + gpio_dev = skl_int3472_register_pdev("tps68470-gpio", &client->dev); > + if (IS_ERR(gpio_dev)) > + return PTR_ERR(gpio_dev); > + > + if (cldb_present) { > + clk_dev = skl_int3472_register_pdev("tps68470-clk", > + &client->dev); > + if (IS_ERR(clk_dev)) { > + ret = PTR_ERR(clk_dev); > + goto err_free_gpio; > + } > + > + regulator_dev = skl_int3472_register_pdev("tps68470-regulator", > + &client->dev); > + if (IS_ERR(regulator_dev)) { > + ret = PTR_ERR(regulator_dev); > + goto err_free_clk; > + } > + } else { > + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", > + &client->dev); > + if (IS_ERR(opregion_dev)) { > + ret = PTR_ERR(opregion_dev); > + goto err_free_gpio; > + } > + } I wonder if this could be simplified by using devm_mfd_add_devices. You could have two arrays of mfd_cell, one for each case. > + > + return 0; > + > +err_free_clk: > + platform_device_put(clk_dev); > +err_free_gpio: > + platform_device_put(gpio_dev); > + > + return ret; > +}
Hi Daniel, On Mon, Jan 18, 2021 at 12:34:24AM +0000, Daniel Scally wrote: > Some places in the kernel allow users to map resources to a device > using device name (for example, 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> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > - Used format macro in i2c_dev_set_name() instead of sub func > > drivers/i2c/i2c-core-base.c | 4 ++-- > include/linux/i2c.h | 3 +++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 63ebf722a424..547b8926cac8 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -811,12 +811,12 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, > struct acpi_device *adev = ACPI_COMPANION(&client->dev); > > if (info && info->dev_name) { > - dev_set_name(&client->dev, "i2c-%s", info->dev_name); > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name); > return; > } > > if (adev) { > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); Over 80, please wrap. With that, Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > return; > } > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 56622658b215..4d40a4b46810 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -39,6 +39,9 @@ enum i2c_slave_event; > typedef int (*i2c_slave_cb_t)(struct i2c_client *client, > enum i2c_slave_event event, u8 *val); > > +/* I2C Device Name Format - to maintain consistency outside the i2c layer */ > +#define I2C_DEV_NAME_FORMAT "i2c-%s" > + > /* I2C Frequency Modes */ > #define I2C_MAX_STANDARD_MODE_FREQ 100000 > #define I2C_MAX_FAST_MODE_FREQ 400000 > -- > 2.25.1 >
> > if (adev) { > > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > > Over 80, please wrap. I am not that strict with the 80, and I don't think it will improve readability here. It can stay this time IMO.
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. Fix prefix to be "ACPI / utils: " and rebase on top of function name changes as suggested by Laurent. ... > +/** > + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev Are you expecting some kind of for_each_*() macro to be added and used? Otherwise there is probably no need to have it "_next_" in the name as it will be a bit confusing. > + * @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. > + */
On Mon, Jan 18, 2021 at 12:34:24AM +0000, Daniel Scally wrote: > Some places in the kernel allow users to map resources to a device > using device name (for example, gpiod_lookup_table). Currently "...in the struct gpiod_lookup_table)." ? > this involves waiting for the i2c_client to have been registered so we I²C client ? > 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 I²C device ? > create a macro for the format that's accessible outside the i2c layer I²C layer ? > and use it in i2c_dev_set_name() Period at the end. For the record, I do not like wrapping below to 80 limit and agree with Wolfram. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > - Used format macro in i2c_dev_set_name() instead of sub func > > drivers/i2c/i2c-core-base.c | 4 ++-- > include/linux/i2c.h | 3 +++ > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c > index 63ebf722a424..547b8926cac8 100644 > --- a/drivers/i2c/i2c-core-base.c > +++ b/drivers/i2c/i2c-core-base.c > @@ -811,12 +811,12 @@ static void i2c_dev_set_name(struct i2c_adapter *adap, > struct acpi_device *adev = ACPI_COMPANION(&client->dev); > > if (info && info->dev_name) { > - dev_set_name(&client->dev, "i2c-%s", info->dev_name); > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, info->dev_name); > return; > } > > if (adev) { > - dev_set_name(&client->dev, "i2c-%s", acpi_dev_name(adev)); > + dev_set_name(&client->dev, I2C_DEV_NAME_FORMAT, acpi_dev_name(adev)); > return; > } > > diff --git a/include/linux/i2c.h b/include/linux/i2c.h > index 56622658b215..4d40a4b46810 100644 > --- a/include/linux/i2c.h > +++ b/include/linux/i2c.h > @@ -39,6 +39,9 @@ enum i2c_slave_event; > typedef int (*i2c_slave_cb_t)(struct i2c_client *client, > enum i2c_slave_event event, u8 *val); > > +/* I2C Device Name Format - to maintain consistency outside the i2c layer */ > +#define I2C_DEV_NAME_FORMAT "i2c-%s" > + > /* I2C Frequency Modes */ > #define I2C_MAX_STANDARD_MODE_FREQ 100000 > #define I2C_MAX_FAST_MODE_FREQ 400000 > -- > 2.25.1 >
On 18/01/2021 12:33, Andy Shevchenko wrote: > 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. > Fix prefix to be "ACPI / utils: " and rebase on top of function name changes as > suggested by Laurent. OK. >> +/** >> + * acpi_dev_get_next_dep_dev - Return next ACPI device dependent on input dev > Are you expecting some kind of for_each_*() macro to be added and used? > Otherwise there is probably no need to have it "_next_" in the name as it will > be a bit confusing. I thought that somebody might want to do that in the future yes, although I've no need for it at the minute because each of the dummy INT3472 devices only has one dependent sensor that we've seen so far. But for the INT3472 devices representing a physical TPS68470, there are platforms where 2 sensors are called out as dependent on the same PMIC ACPI device (Surface Go2 for example). It can be renamed and just cross that bridge when we come to it though, I have no problem with that. > >> + * @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. >> + */
On Mon, Jan 18, 2021 at 03:45:02PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 18, 2021 at 12:34:26AM +0000, Daniel Scally wrote:
And prefix: "gpiolib: acpi: ".
On Mon, Jan 18, 2021 at 11:12:34AM +0000, Barnabás Pőcze wrote: > 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta: > Have you considered putting the source (and header) files into a dedicated > folder? I think it'd help manageability in the long run, and it'd be immediately > obvious that these source files form a single "unit". What would be the folder name? Because, for example, intel_cht_int33fe* have no folder (yet?) and here it's kinda similar case when HID describes something else than just one IP.
2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: > On Mon, Jan 18, 2021 at 11:12:34AM +0000, Barnabás Pőcze wrote: > > 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta: > > > Have you considered putting the source (and header) files into a dedicated > > folder? I think it'd help manageability in the long run, and it'd be immediately > > obvious that these source files form a single "unit". > > What would be the folder name? Because, for example, intel_cht_int33fe* have no > folder (yet?) and here it's kinda similar case when HID describes something > else than just one IP. I think "intel_skl_int3472" would not be a bad name for the folder. And I believe "intel_cht_int33fe" could be given its own folder as well. Regards, Barnabás Pőcze
On Mon, Jan 18, 2021 at 02:51:30PM +0000, Barnabás Pőcze wrote: > 2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: > > > On Mon, Jan 18, 2021 at 11:12:34AM +0000, Barnabás Pőcze wrote: > > > 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta: > > > > > Have you considered putting the source (and header) files into a dedicated > > > folder? I think it'd help manageability in the long run, and it'd be immediately > > > obvious that these source files form a single "unit". > > > > What would be the folder name? Because, for example, intel_cht_int33fe* have no > > folder (yet?) and here it's kinda similar case when HID describes something > > else than just one IP. > > I think "intel_skl_int3472" would not be a bad name for the folder. And I believe > "intel_cht_int33fe" could be given its own folder as well. I;m not objecting (at some point in the past I had proposed moving Intel stuff to a separate folder, but at that time PDx86 has no folders at all and Darren was kinda not in favour of creating ones, but things changed), just let's hear Hans on this.
Hi, On 1/18/21 4:23 PM, andriy.shevchenko@linux.intel.com wrote: > On Mon, Jan 18, 2021 at 02:51:30PM +0000, Barnabás Pőcze wrote: >> 2021. január 18., hétfő 14:51 keltezéssel, Andy Shevchenko írta: >> >>> On Mon, Jan 18, 2021 at 11:12:34AM +0000, Barnabás Pőcze wrote: >>>> 2021. január 18., hétfő 1:34 keltezéssel, Daniel Scally írta: >>> >>>> Have you considered putting the source (and header) files into a dedicated >>>> folder? I think it'd help manageability in the long run, and it'd be immediately >>>> obvious that these source files form a single "unit". >>> >>> What would be the folder name? Because, for example, intel_cht_int33fe* have no >>> folder (yet?) and here it's kinda similar case when HID describes something >>> else than just one IP. >> >> I think "intel_skl_int3472" would not be a bad name for the folder. And I believe >> "intel_cht_int33fe" could be given its own folder as well. > > I;m not objecting (at some point in the past I had proposed moving Intel stuff > to a separate folder, but at that time PDx86 has no folders at all and Darren > was kinda not in favour of creating ones, but things changed), just let's hear > Hans on this. I'm in favor of using a folder for this and "intel_skl_int3472" is fine with me. With that said I'm not entirely sure if I'm in favor of the _skl_ part of the folder and driver name or not. The INT3472 ACPI device is used with other CPUs, e.g. Apollo Lake too and I think the driver should work fine with those. The intel_cht_int33fe case is special because the driver only applies to some models with an INT33FE ACPI device (the whole INT33FE ACPI device is a horrible thing which seems to stem from Intel rushing Bay Trail to market to try and compete on the tablet market). With that all said SKL probably is the first SoC to feature this and I guess future IPUs may still use INT3472 given Intel's BAD habit of re-using ACPI HIDs for multiple incompatible generations. So I guess that keeping it is fine; and if we then need an incompatible INT3472 driver for newer IPUs we can use a different prefix for those. TL;DR: 1. Using a folder is fine, desirable even 2. I've some concerns about the name, but I'm not really objecting, just giving my 2 cents. Regards, Hans
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? 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? > 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 >
Hi Laurent, thanks for the comments - really appreciate the detail. Some specific responses below but assume a general "will do" to everything you mentioned otherwise... On 18/01/2021 09:15, Laurent Pinchart wrote: >> + PMIC) and one designed for Chrome OS. > How about expanding this a bit to explain what the INT3472 stands for ? > > The INT3472 is an Intel camera power controller, a logical device > found on some Skylake-based systems that can map to different > hardware devices depending on the platform. On machines > designed for Chrome OS, it maps to a TPS68470 camera PMIC. On > machines designed for Windows, it maps to either a TP68470 > camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs > and power gates. Yeah sure ok >> This driver handles all three >> + situations by discovering information it needs to discern them at >> + runtime. >> + >> + If your device was designed for Chrome OS, this driver will provide >> + an ACPI operation region, which must be available before any of the >> + devices using this are probed. For this reason, you should select Y >> + if your device was designed for ChromeOS. This option also configures >> + the designware-i2c driver to be built-in, for the same reason. > Is the last sentence a leftover ? Oops - it is, but it was supposed to remind me to double check that that was still necessary. I'll take a look, thanks. >> + >> +#include "intel_skl_int3472_common.h" >> + >> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, >> + struct int3472_cldb *cldb) >> +{ >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >> + acpi_handle handle = adev->handle; >> + union acpi_object *obj; >> + acpi_status status; >> + int ret = 0; >> + >> + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer); >> + if (ACPI_FAILURE(status)) >> + return -ENODEV; >> + >> + obj = buffer.pointer; >> + if (!obj) { >> + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); > Is this the code path that is taken on Chrome OS ? If so an error > message isn't appropriate. I'd drop this message, and instead add an > error message in the discrete PMIC code. Ah yes of course, thanks, I'll move the error message. >> + >> + unsigned int n_gpios; /* how many GPIOs have we seen */ >> + >> + struct int3472_gpio_regulator regulator; >> + struct int3472_gpio_clock clock; > You don't necessarily need to define separate structures for this, you > could also write > > struct { > char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; > struct gpio_desc *gpio; > struct regulator_dev *rdev; > struct regulator_desc rdesc; > } regulator; > > struct { > struct clk *clk; > struct clk_hw clk_hw; > struct clk_lookup *cl; > struct gpio_desc *gpio; > } clock; > > It's entirely up to you. Ooh yeah I like that more, thanks very much. >> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */ >> +static const guid_t int3472_gpio_guid = >> + GUID_INIT(0x79234640, 0x9e10, 0x4fea, >> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); >> + >> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */ >> +static const guid_t cio2_sensor_module_guid = >> + GUID_INIT(0x822ace8f, 0x2814, 0x4174, >> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); > A comment that explains what those DSM functions do would be useful for > reference. It has taken lots of time to figure it out, let's spare the > pain to the next person who tries to understand this :-) Hah - good point, well made. I'll explain what they're for then. >> +static int skl_int3472_clk_enable(struct clk_hw *hw) >> +{ >> + struct int3472_gpio_clock *clk = to_int3472_clk(hw); >> + >> + gpiod_set_value(clk->gpio, 1); > The clock enable() and disable() methods are not supposed to sleep, > while setting a GPIO value may sleep in the general case. Should this be > moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable() > and skl_int3472_clk_unprepare(). I was under the assumption the difference between gpiod_set_value() and gpiod_set_value_cansleep() was that gpiod_set_value() _can't_ sleep, but actually reading the function's comments it seems it will just complain if it turns out it can sleep: * This function can be called from contexts where we cannot sleep, and will * complain if the GPIO chip functions potentially sleep. It doesn't complain, on either of my devices, but I guess that can't be guaranteed for _every_ device, so these calls probably are safer in (un)prepare() yes. >> + } >> + >> + i++; >> + } >> + } >> + >> + if (!func) >> + return 0; > I initially thought this wasn't right, as if no entry was found in the > mapping table, func would still have its non-NULL value as passed to > this function. I then realized that you're checking if the match that > was found is NULL. A comment to explain this would be useful. Yep ok - I actually had one and decided it was superfluous and removed it - my bad. >> + >> + status = acpi_get_handle(NULL, path, &handle); >> + if (ACPI_FAILURE(status)) >> + return -EINVAL; >> + >> + ret = acpi_bus_get_device(handle, &adev); >> + if (ret) >> + return -ENODEV; >> + >> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), >> + ares->data.gpio.pin_table[0], >> + func, 0, polarity); > I wonder if > > table_entry.key = acpi_dev_name(adev); > table_entry.chip_hwnum = ares->data.gpio.pin_table[0]; > table_entry.con_id = func; > table_entry.idx = 0; > table_entry.flags = polarity; > > (with struct gpiod_lookup table_entry = { }; above) would be more > readable. Up to you. > >> + >> + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, >> + sizeof(table_entry)); > Ah, or maybe > > struct gpio_lookup *table_entry; > > table_entry = &int3472->gpios.table[int3472->n_sensor_gpios]; > table_entry->key = acpi_dev_name(adev); > table_entry->chip_hwnum = ares->data.gpio.pin_table[0]; > table_entry->con_id = func; > table_entry->idx = 0; > table_entry->flags = polarity; > > (no need to memset() to 0 first as the whole structure has been > allocated with kzalloc()). Yeah you're right, this looks much nicer - thanks. >> + int ret = 0; >> + >> + init.name = kasprintf(GFP_KERNEL, "%s-clk", >> + acpi_dev_name(int3472->adev)); > You need to check for NULL and return -ENOMEM. Oops, of course, thanks >> + goto err_unregister_clk; > If this fails, you will end up calling clk_unregister() and > clkdev_drop() in skl_int3472_discrete_remove(). You should replace the > check in the remove function with > > if (!int3472->clock.cl) { You're right, good spot, thank you. >> + dev_err(&int3472->pdev->dev, "No sensor module config\n"); >> + return PTR_ERR(sensor_config); >> + } > Would it make sense to call this in skl_int3472_discrete_probe() or > skl_int3472_parse_crs() and cache the config pointer ? Yes, probably actually, and then the GPIO mapping function can just check for its presence. >> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; >> + init_data.num_consumer_supplies = 1; >> + init_data.consumer_supplies = &sensor_config->supply_map; >> + >> + snprintf(int3472->regulator.regulator_name, >> + GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator"); > s/GPIO_REGULATOR_NAME_LENGTH/sizeof(int3472->regulator.regulator_name)/ > > Do regulator names need to be unique ? If so you may have a problem if a > platform has two discrete INT3472. Unlike clocks, the regulator framework doesn't shout at you when you do this, but I agree it's suboptimal at the very least, I'll set it to ..."%s-regulator", acpi_dev_name(int3472->adev)... as with the clock. >> + case INT3472_GPIO_TYPE_PRIVACY_LED: >> + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, >> + "indicator-led", >> + GPIO_ACTIVE_HIGH); > Mapping the indicator LED to the sensor isn't great, as all sensor > drivers would then need to handle it. Could it be handled in the > regulator instead, so that it would be turned on automatically when the > sensor is powered up ? Another option, more complicated, would be to > handle it in the CIO2 driver (but I'm not sure how we would map it). Not with the regulator, because it turns out only the 0x0b pin is one of those and those appear on very few devices in scope, so it wouldn't be called on a Surface Book 2 for example. Perhaps as part of clock prepare/enable? I don't much like the idea of it running in the CIO2 driver to be honest, feels a bit out of place. >> + >> + if (int3472->gpios_mapped) >> + gpiod_remove_lookup_table(&int3472->gpios); > You could avoid the need for the gpios_mapped field by checking for > > if (int3472->gpios.list.next) > > Up to you. Thank you! I was scratching my head trying to figure out a better way of doing that.
Hi Andy - thanks as always for the comments Some responses below, but if not mentioned I'll follow your suggestion of course On 18/01/2021 14:46, Andy Shevchenko wrote: > On Mon, Jan 18, 2021 at 11:15:21AM +0200, Laurent Pinchart wrote: >> On Mon, Jan 18, 2021 at 12:34:27AM +0000, Daniel Scally wrote: > My comments on top of Laurent's to avoid dups. > > First of all, PDx86 has its own prefix pattern: "platform/x86: ..." Sorry, I probably should have put more effort into figuring out the right pattern for those >>> +config INTEL_SKL_INT3472 >>> + tristate "Intel SkyLake ACPI INT3472 Driver" >>> + depends on X86 && ACPI > X86 is a dup. Entire lot of PDx86 is a priory dependent on it (find "if X86" > line in Kconfig). Ah, oh yeah - thanks. I'll watch for that in future >>> +static struct i2c_driver int3472_tps68470 = { >>> + .driver = { >>> + .name = "int3472-tps68470", >>> + .acpi_match_table = int3472_device_id, >>> + }, >>> + .probe_new = skl_int3472_tps68470_probe, >>> +}; > I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... Sorry, I'm a bit confused by this. The i2c device is already present...we just want the driver to bind to them, so what role do those functions have there? >>> +struct int3472_gpio_clock { >>> + struct clk *clk; >>> + struct clk_hw clk_hw; >>> + struct clk_lookup *cl; >>> + struct gpio_desc *gpio; >>> +}; > Wondering if this has some similarities with and actually can utilize clk-gpio > driver. > > > >>> +static const struct clk_ops skl_int3472_clock_ops = { >>> + .prepare = skl_int3472_clk_prepare, >>> + .unprepare = skl_int3472_clk_unprepare, >>> + .enable = skl_int3472_clk_enable, >>> + .disable = skl_int3472_clk_disable, >>> +}; > Yeah, sounds like reinventing clk-gpio.c. > > static const struct clk_ops clk_gpio_gate_ops = { > .enable = clk_gpio_gate_enable, > .disable = clk_gpio_gate_disable, > .is_enabled = clk_gpio_gate_is_enabled, > }; > > (Or is it mux? It has support there as well. > Hmm, yeah, this looks like it would work actually. So I think I'd need to: 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver 2. Register a platform device to bind to the clk-gpio driver 3. Register a gpio lookup table so that the clk-gpio driver can find the gpio in question using gpiod_get() And that looks like it will work; I'll try it. >>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */ >>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, >>> + /* Surface Go 1&2 - OV5693, Front */ >>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, > I'm wondering if you should use same I2C format macro and create this > dynamically? Or rather find a corresponding ACPI device instance and > copy it's name? ... The supply name needs hard-coding really, but the device name I suppose can come from int3472->sensor_name. >>> +static struct int3472_sensor_config * >>> +skl_int3472_get_sensor_module_config(struct int3472_device *int3472) >>> +{ >>> + unsigned int i = ARRAY_SIZE(int3472_sensor_configs); >>> + struct int3472_sensor_config *ret; >>> + union acpi_object *obj; >>> + >>> + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, >>> + &cio2_sensor_module_guid, 0x00, >>> + 0x01, NULL, ACPI_TYPE_STRING); >>> + >>> + if (!obj) { >>> + dev_err(&int3472->pdev->dev, >>> + "Failed to get sensor module string from _DSM\n"); >>> + return ERR_PTR(-ENODEV); >>> + } >>> + >>> + if (obj->string.type != ACPI_TYPE_STRING) { >>> + dev_err(&int3472->pdev->dev, >>> + "Sensor _DSM returned a non-string value\n"); >>> + ret = ERR_PTR(-EINVAL); >>> + goto out_free_obj; > And after below change you can turn this to > > ACPI_FREE(obj); > return ERR_PTR(-EINVAL); > > and remove label completely, but it's up to you. > >>> + } >>> + ret = ERR_PTR(-ENODEV); > This seems redundant. Or are you expecting ARRAY_SIZE() to be 0? > If no, you may add static_assert() near to the array definition. It **could** become 0, if the entries I've added are removed in future because the sensors are no longer supported or something. There might be no sensor_module_config for a given device. We only need to supply one if a) The platform has a 0x0b type GPIO, which means we need to define a supply name the driver is expecting b) The GPIO functions deviate from documented purpose, which means we need to supply a remapping struct Otherwise, there's no need for it. >>> + >>> + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, >>> + &cfg); >>> + if (IS_ERR(int3472->regulator.rdev)) { >>> + ret = PTR_ERR(int3472->regulator.rdev); >>> + goto err_free_gpio; >>> + } > Similar here, can we utilize gpio-regulator.c? > Also yes probably, with the same steps as for the clocks. Again, I'll try that out, thanks very much. >>> + dev_warn(&int3472->pdev->dev, >>> + "GPIO type 0x%llx unknown; the sensor may not work\n", >>> + (obj->integer.value & 0xff)); >> No need for parentheses. > And instead of "%llx" with " & 0xff" you may use "%x" with "(u8)" cast. > However, I don't think we need to show only last byte, because it may give > wrong impression on values like "0x100". But in this case only the last byte holds the type information, second lowest byte is the pin number. So as we understand it, 0x100 would be invalid anyway. >>> + int3472 = kzalloc(sizeof(*int3472) + >>> + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), >> One more space for the indentation, and the outer parentheses are not >> needed. > And use struct_size() from overflow.h. TIL! Thank you >>> + if (int3472->gpios_mapped) >>> + gpiod_remove_lookup_table(&int3472->gpios); >> You could avoid the need for the gpios_mapped field by checking for >> >> if (int3472->gpios.list.next) > I think this is an intrusion to GPIO realm. > Instead would you consider to drop the check completely and use ->gpios to be > NULL / not-NULL (and yes, you won't need gpios_mapped flag)? > > d321ad1286d2 ("gpiolib: Follow usual pattern for gpiod_remove_lookup_table() call") > > in I²C tree makes it possible. That's also fine by me; or I guess also check int3472->gpiods.dev_id, and only set the pointer the first time a GPIO is mapped. I'll rework it, thanks. >>> + return -EINVAL; >>> + >>> + if (ret) >>> + cldb_present = false; >>> + >>> + gpio_dev = skl_int3472_register_pdev("tps68470-gpio", &client->dev); >>> + if (IS_ERR(gpio_dev)) >>> + return PTR_ERR(gpio_dev); >>> + >>> + if (cldb_present) { >>> + clk_dev = skl_int3472_register_pdev("tps68470-clk", >>> + &client->dev); >>> + if (IS_ERR(clk_dev)) { >>> + ret = PTR_ERR(clk_dev); >>> + goto err_free_gpio; >>> + } >>> + >>> + regulator_dev = skl_int3472_register_pdev("tps68470-regulator", >>> + &client->dev); >>> + if (IS_ERR(regulator_dev)) { >>> + ret = PTR_ERR(regulator_dev); >>> + goto err_free_clk; >>> + } >>> + } else { >>> + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", >>> + &client->dev); >>> + if (IS_ERR(opregion_dev)) { >>> + ret = PTR_ERR(opregion_dev); >>> + goto err_free_gpio; >>> + } >>> + } >> I wonder if this could be simplified by using devm_mfd_add_devices. You >> could have two arrays of mfd_cell, one for each case. > Yeah, which effectively means that we should have some kind of mfd/tps68470 in > place. Can you expand on what you mean by that a little, please?
Hi Andy, Laurent On 18/01/2021 21:19, Daniel Scally wrote: >>>> +static const struct clk_ops skl_int3472_clock_ops = { >>>> + .prepare = skl_int3472_clk_prepare, >>>> + .unprepare = skl_int3472_clk_unprepare, >>>> + .enable = skl_int3472_clk_enable, >>>> + .disable = skl_int3472_clk_disable, >>>> +}; >> Yeah, sounds like reinventing clk-gpio.c. >> >> static const struct clk_ops clk_gpio_gate_ops = { >> .enable = clk_gpio_gate_enable, >> .disable = clk_gpio_gate_disable, >> .is_enabled = clk_gpio_gate_is_enabled, >> }; >> >> (Or is it mux? It has support there as well. >> > Hmm, yeah, this looks like it would work actually. So I think I'd need to: > > > 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver > > 2. Register a platform device to bind to the clk-gpio driver > > 3. Register a gpio lookup table so that the clk-gpio driver can find the > gpio in question using gpiod_get() > > > And that looks like it will work; I'll try it. I'm more and more confident that this will work, but it has some knock-on effects: The both clk and regulator gpio driver expects to be able to fetch the GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of course, so we need to add another GPIO lookup table so those drivers can see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will be so we can set the .dev_id member of a gpiod_lookup_table to that value, but that isn't set until _after_ the pdev is registered (because it has to figure out the id, we can't manually set the IDs because there could be more than one instance of int3472-discrete bound to multiple PMIC devices, and we don't know which id the current one should have). Finally, we can't wait until the device is registered because it immediately probes, can't find the GPIO and then fails probe. It's similar problem that causes us to need the i2c-acpi name format macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) Solving it is a bit of a sticky one; perhaps something like moving the dev_set_name() part of platform_device_add() [1] to its own function, that's called in both platform_device_alloc() and platform_device_register(). That way it would be available before the device itself was registered, meaning we could create the lookup table before it probes the driver. (also, Laurent, if we did it this way we wouldn't be able to also handle the led-indicator GPIO here without some fairly major rework) [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L563
Hi Daniel, On Mon, Jan 18, 2021 at 08:46:34PM +0000, Daniel Scally wrote: > Hi Laurent, thanks for the comments - really appreciate the detail. > > Some specific responses below but assume a general "will do" to > everything you mentioned otherwise... > > On 18/01/2021 09:15, Laurent Pinchart wrote: > >> + PMIC) and one designed for Chrome OS. > > > > How about expanding this a bit to explain what the INT3472 stands for ? > > > > The INT3472 is an Intel camera power controller, a logical device > > found on some Skylake-based systems that can map to different > > hardware devices depending on the platform. On machines > > designed for Chrome OS, it maps to a TPS68470 camera PMIC. On > > machines designed for Windows, it maps to either a TP68470 > > camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs > > and power gates. > > Yeah sure ok > > >> This driver handles all three > >> + situations by discovering information it needs to discern them at > >> + runtime. > >> + > >> + If your device was designed for Chrome OS, this driver will provide > >> + an ACPI operation region, which must be available before any of the > >> + devices using this are probed. For this reason, you should select Y > >> + if your device was designed for ChromeOS. This option also configures > >> + the designware-i2c driver to be built-in, for the same reason. > > > > Is the last sentence a leftover ? > > Oops - it is, but it was supposed to remind me to double check that that > was still necessary. I'll take a look, thanks. > > >> + > >> +#include "intel_skl_int3472_common.h" > >> + > >> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > >> + struct int3472_cldb *cldb) > >> +{ > >> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > >> + acpi_handle handle = adev->handle; > >> + union acpi_object *obj; > >> + acpi_status status; > >> + int ret = 0; > >> + > >> + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer); > >> + if (ACPI_FAILURE(status)) > >> + return -ENODEV; > >> + > >> + obj = buffer.pointer; > >> + if (!obj) { > >> + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); > > > > Is this the code path that is taken on Chrome OS ? If so an error > > message isn't appropriate. I'd drop this message, and instead add an > > error message in the discrete PMIC code. > > Ah yes of course, thanks, I'll move the error message. > > >> + > >> + unsigned int n_gpios; /* how many GPIOs have we seen */ > >> + > >> + struct int3472_gpio_regulator regulator; > >> + struct int3472_gpio_clock clock; > > > > You don't necessarily need to define separate structures for this, you > > could also write > > > > struct { > > char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > > char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; > > struct gpio_desc *gpio; > > struct regulator_dev *rdev; > > struct regulator_desc rdesc; > > } regulator; > > > > struct { > > struct clk *clk; > > struct clk_hw clk_hw; > > struct clk_lookup *cl; > > struct gpio_desc *gpio; > > } clock; > > > > It's entirely up to you. > > Ooh yeah I like that more, thanks very much. > > >> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */ > >> +static const guid_t int3472_gpio_guid = > >> + GUID_INIT(0x79234640, 0x9e10, 0x4fea, > >> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); > >> + > >> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */ > >> +static const guid_t cio2_sensor_module_guid = > >> + GUID_INIT(0x822ace8f, 0x2814, 0x4174, > >> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); > > > > A comment that explains what those DSM functions do would be useful for > > reference. It has taken lots of time to figure it out, let's spare the > > pain to the next person who tries to understand this :-) > > Hah - good point, well made. I'll explain what they're for then. > > >> +static int skl_int3472_clk_enable(struct clk_hw *hw) > >> +{ > >> + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > >> + > >> + gpiod_set_value(clk->gpio, 1); > > > > The clock enable() and disable() methods are not supposed to sleep, > > while setting a GPIO value may sleep in the general case. Should this be > > moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable() > > and skl_int3472_clk_unprepare(). > > I was under the assumption the difference between gpiod_set_value() and > gpiod_set_value_cansleep() was that gpiod_set_value() _can't_ sleep, but > actually reading the function's comments it seems it will just complain > if it turns out it can sleep: > > * This function can be called from contexts where we cannot sleep, and will > * complain if the GPIO chip functions potentially sleep. It doesn't > complain, on either of my devices, but I guess that can't be guaranteed > for _every_ device, so these calls probably are safer in (un)prepare() yes. If we could guarantee that the GPIOs are connected to the SoC, we could keep using the code above, as there should be no need to sleep. The question is whether this can be guaranteed or not. It's true that I would be surprised if the GPIOs were connected, for instance, to an I2C GPIO expander.. > >> + } > >> + > >> + i++; > >> + } > >> + } > >> + > >> + if (!func) > >> + return 0; > > > > I initially thought this wasn't right, as if no entry was found in the > > mapping table, func would still have its non-NULL value as passed to > > this function. I then realized that you're checking if the match that > > was found is NULL. A comment to explain this would be useful. > > Yep ok - I actually had one and decided it was superfluous and removed > it - my bad. > > >> + > >> + status = acpi_get_handle(NULL, path, &handle); > >> + if (ACPI_FAILURE(status)) > >> + return -EINVAL; > >> + > >> + ret = acpi_bus_get_device(handle, &adev); > >> + if (ret) > >> + return -ENODEV; > >> + > >> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > >> + ares->data.gpio.pin_table[0], > >> + func, 0, polarity); > > > > I wonder if > > > > table_entry.key = acpi_dev_name(adev); > > table_entry.chip_hwnum = ares->data.gpio.pin_table[0]; > > table_entry.con_id = func; > > table_entry.idx = 0; > > table_entry.flags = polarity; > > > > (with struct gpiod_lookup table_entry = { }; above) would be more > > readable. Up to you. > > > >> + > >> + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, > >> + sizeof(table_entry)); > > > > Ah, or maybe > > > > struct gpio_lookup *table_entry; > > > > table_entry = &int3472->gpios.table[int3472->n_sensor_gpios]; > > table_entry->key = acpi_dev_name(adev); > > table_entry->chip_hwnum = ares->data.gpio.pin_table[0]; > > table_entry->con_id = func; > > table_entry->idx = 0; > > table_entry->flags = polarity; > > > > (no need to memset() to 0 first as the whole structure has been > > allocated with kzalloc()). > > Yeah you're right, this looks much nicer - thanks. > > >> + int ret = 0; > >> + > >> + init.name = kasprintf(GFP_KERNEL, "%s-clk", > >> + acpi_dev_name(int3472->adev)); > > > > You need to check for NULL and return -ENOMEM. > > Oops, of course, thanks > > >> + goto err_unregister_clk; > > > > If this fails, you will end up calling clk_unregister() and > > clkdev_drop() in skl_int3472_discrete_remove(). You should replace the > > check in the remove function with > > > > if (!int3472->clock.cl) { > > You're right, good spot, thank you. > > >> + dev_err(&int3472->pdev->dev, "No sensor module config\n"); > >> + return PTR_ERR(sensor_config); > >> + } > > > > Would it make sense to call this in skl_int3472_discrete_probe() or > > skl_int3472_parse_crs() and cache the config pointer ? > > Yes, probably actually, and then the GPIO mapping function can just > check for its presence. > > >> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; > >> + init_data.num_consumer_supplies = 1; > >> + init_data.consumer_supplies = &sensor_config->supply_map; > >> + > >> + snprintf(int3472->regulator.regulator_name, > >> + GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator"); > > > > s/GPIO_REGULATOR_NAME_LENGTH/sizeof(int3472->regulator.regulator_name)/ > > > > Do regulator names need to be unique ? If so you may have a problem if a > > platform has two discrete INT3472. > > Unlike clocks, the regulator framework doesn't shout at you when you do > this, but I agree it's suboptimal at the very least, I'll set it to > ..."%s-regulator", acpi_dev_name(int3472->adev)... as with the clock. > > >> + case INT3472_GPIO_TYPE_PRIVACY_LED: > >> + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, > >> + "indicator-led", > >> + GPIO_ACTIVE_HIGH); > > > > Mapping the indicator LED to the sensor isn't great, as all sensor > > drivers would then need to handle it. Could it be handled in the > > regulator instead, so that it would be turned on automatically when the > > sensor is powered up ? Another option, more complicated, would be to > > handle it in the CIO2 driver (but I'm not sure how we would map it). > > Not with the regulator, because it turns out only the 0x0b pin is one of > those and those appear on very few devices in scope, so it wouldn't be > called on a Surface Book 2 for example. Perhaps as part of clock > prepare/enable? I don't much like the idea of it running in the CIO2 > driver to be honest, feels a bit out of place. The clock is another option, but could there be platforms where the clock GPIO isn't present ? Another option would be to let userspace handle that GPIO, but we then need to convey it to userspace. > >> + > >> + if (int3472->gpios_mapped) > >> + gpiod_remove_lookup_table(&int3472->gpios); > > > > You could avoid the need for the gpios_mapped field by checking for > > > > if (int3472->gpios.list.next) > > > > Up to you. > > Thank you! I was scratching my head trying to figure out a better way of > doing that.
Hi Daniel, On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > On 18/01/2021 21:19, Daniel Scally wrote: > >>>> +static const struct clk_ops skl_int3472_clock_ops = { > >>>> + .prepare = skl_int3472_clk_prepare, > >>>> + .unprepare = skl_int3472_clk_unprepare, > >>>> + .enable = skl_int3472_clk_enable, > >>>> + .disable = skl_int3472_clk_disable, > >>>> +}; > >> > >> Yeah, sounds like reinventing clk-gpio.c. > >> > >> static const struct clk_ops clk_gpio_gate_ops = { > >> .enable = clk_gpio_gate_enable, > >> .disable = clk_gpio_gate_disable, > >> .is_enabled = clk_gpio_gate_is_enabled, > >> }; > >> > >> (Or is it mux? It has support there as well. > >> > > Hmm, yeah, this looks like it would work actually. So I think I'd need to: > > > > 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver > > > > 2. Register a platform device to bind to the clk-gpio driver > > > > 3. Register a gpio lookup table so that the clk-gpio driver can find the > > gpio in question using gpiod_get() > > > > And that looks like it will work; I'll try it. > > I'm more and more confident that this will work, but it has some > knock-on effects: > > The both clk and regulator gpio driver expects to be able to fetch the > GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of > course, so we need to add another GPIO lookup table so those drivers can > see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will > be so we can set the .dev_id member of a gpiod_lookup_table to that > value, but that isn't set until _after_ the pdev is registered (because > it has to figure out the id, we can't manually set the IDs because there > could be more than one instance of int3472-discrete bound to multiple > PMIC devices, and we don't know which id the current one should have). > Finally, we can't wait until the device is registered because it > immediately probes, can't find the GPIO and then fails probe. > > It's similar problem that causes us to need the i2c-acpi name format > macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) > > Solving it is a bit of a sticky one; perhaps something like moving the > dev_set_name() part of platform_device_add() [1] to its own function, > that's called in both platform_device_alloc() and > platform_device_register(). That way it would be available before the > device itself was registered, meaning we could create the lookup table > before it probes the driver. > > (also, Laurent, if we did it this way we wouldn't be able to also handle > the led-indicator GPIO here without some fairly major rework) Given the additional complexity I don't think it's worth it, your implementation is fine and code duplication with clk-gpio is minimal. > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/base/platform.c#L563
Hi Laurent On 19/01/2021 06:19, Laurent Pinchart wrote: > Hi Daniel, > > On Mon, Jan 18, 2021 at 08:46:34PM +0000, Daniel Scally wrote: >> Hi Laurent, thanks for the comments - really appreciate the detail. >> >> Some specific responses below but assume a general "will do" to >> everything you mentioned otherwise... >> >> On 18/01/2021 09:15, Laurent Pinchart wrote: >>>> + PMIC) and one designed for Chrome OS. >>> How about expanding this a bit to explain what the INT3472 stands for ? >>> >>> The INT3472 is an Intel camera power controller, a logical device >>> found on some Skylake-based systems that can map to different >>> hardware devices depending on the platform. On machines >>> designed for Chrome OS, it maps to a TPS68470 camera PMIC. On >>> machines designed for Windows, it maps to either a TP68470 >>> camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs >>> and power gates. >> Yeah sure ok >> >>>> This driver handles all three >>>> + situations by discovering information it needs to discern them at >>>> + runtime. >>>> + >>>> + If your device was designed for Chrome OS, this driver will provide >>>> + an ACPI operation region, which must be available before any of the >>>> + devices using this are probed. For this reason, you should select Y >>>> + if your device was designed for ChromeOS. This option also configures >>>> + the designware-i2c driver to be built-in, for the same reason. >>> Is the last sentence a leftover ? >> Oops - it is, but it was supposed to remind me to double check that that >> was still necessary. I'll take a look, thanks. >> >>>> + >>>> +#include "intel_skl_int3472_common.h" >>>> + >>>> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, >>>> + struct int3472_cldb *cldb) >>>> +{ >>>> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; >>>> + acpi_handle handle = adev->handle; >>>> + union acpi_object *obj; >>>> + acpi_status status; >>>> + int ret = 0; >>>> + >>>> + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer); >>>> + if (ACPI_FAILURE(status)) >>>> + return -ENODEV; >>>> + >>>> + obj = buffer.pointer; >>>> + if (!obj) { >>>> + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); >>> Is this the code path that is taken on Chrome OS ? If so an error >>> message isn't appropriate. I'd drop this message, and instead add an >>> error message in the discrete PMIC code. >> Ah yes of course, thanks, I'll move the error message. >> >>>> + >>>> + unsigned int n_gpios; /* how many GPIOs have we seen */ >>>> + >>>> + struct int3472_gpio_regulator regulator; >>>> + struct int3472_gpio_clock clock; >>> You don't necessarily need to define separate structures for this, you >>> could also write >>> >>> struct { >>> char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; >>> char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; >>> struct gpio_desc *gpio; >>> struct regulator_dev *rdev; >>> struct regulator_desc rdesc; >>> } regulator; >>> >>> struct { >>> struct clk *clk; >>> struct clk_hw clk_hw; >>> struct clk_lookup *cl; >>> struct gpio_desc *gpio; >>> } clock; >>> >>> It's entirely up to you. >> Ooh yeah I like that more, thanks very much. >> >>>> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */ >>>> +static const guid_t int3472_gpio_guid = >>>> + GUID_INIT(0x79234640, 0x9e10, 0x4fea, >>>> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); >>>> + >>>> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */ >>>> +static const guid_t cio2_sensor_module_guid = >>>> + GUID_INIT(0x822ace8f, 0x2814, 0x4174, >>>> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); >>> A comment that explains what those DSM functions do would be useful for >>> reference. It has taken lots of time to figure it out, let's spare the >>> pain to the next person who tries to understand this :-) >> Hah - good point, well made. I'll explain what they're for then. >> >>>> +static int skl_int3472_clk_enable(struct clk_hw *hw) >>>> +{ >>>> + struct int3472_gpio_clock *clk = to_int3472_clk(hw); >>>> + >>>> + gpiod_set_value(clk->gpio, 1); >>> The clock enable() and disable() methods are not supposed to sleep, >>> while setting a GPIO value may sleep in the general case. Should this be >>> moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable() >>> and skl_int3472_clk_unprepare(). >> I was under the assumption the difference between gpiod_set_value() and >> gpiod_set_value_cansleep() was that gpiod_set_value() _can't_ sleep, but >> actually reading the function's comments it seems it will just complain >> if it turns out it can sleep: >> >> * This function can be called from contexts where we cannot sleep, and will >> * complain if the GPIO chip functions potentially sleep. It doesn't >> complain, on either of my devices, but I guess that can't be guaranteed >> for _every_ device, so these calls probably are safer in (un)prepare() yes. > If we could guarantee that the GPIOs are connected to the SoC, we could > keep using the code above, as there should be no need to sleep. The > question is whether this can be guaranteed or not. It's true that I > would be surprised if the GPIOs were connected, for instance, to an I2C > GPIO expander.. Is that the deciding factor? I'd say that's unlikely, but what do I know? Then again, is there actually any downside to calling gpiod_set_value() in the prepare() function instead? If not, may as well be safe. >>>> + } >>>> + >>>> + i++; >>>> + } >>>> + } >>>> + >>>> + if (!func) >>>> + return 0; >>> I initially thought this wasn't right, as if no entry was found in the >>> mapping table, func would still have its non-NULL value as passed to >>> this function. I then realized that you're checking if the match that >>> was found is NULL. A comment to explain this would be useful. >> Yep ok - I actually had one and decided it was superfluous and removed >> it - my bad. >> >>>> + >>>> + status = acpi_get_handle(NULL, path, &handle); >>>> + if (ACPI_FAILURE(status)) >>>> + return -EINVAL; >>>> + >>>> + ret = acpi_bus_get_device(handle, &adev); >>>> + if (ret) >>>> + return -ENODEV; >>>> + >>>> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), >>>> + ares->data.gpio.pin_table[0], >>>> + func, 0, polarity); >>> I wonder if >>> >>> table_entry.key = acpi_dev_name(adev); >>> table_entry.chip_hwnum = ares->data.gpio.pin_table[0]; >>> table_entry.con_id = func; >>> table_entry.idx = 0; >>> table_entry.flags = polarity; >>> >>> (with struct gpiod_lookup table_entry = { }; above) would be more >>> readable. Up to you. >>> >>>> + >>>> + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, >>>> + sizeof(table_entry)); >>> Ah, or maybe >>> >>> struct gpio_lookup *table_entry; >>> >>> table_entry = &int3472->gpios.table[int3472->n_sensor_gpios]; >>> table_entry->key = acpi_dev_name(adev); >>> table_entry->chip_hwnum = ares->data.gpio.pin_table[0]; >>> table_entry->con_id = func; >>> table_entry->idx = 0; >>> table_entry->flags = polarity; >>> >>> (no need to memset() to 0 first as the whole structure has been >>> allocated with kzalloc()). >> Yeah you're right, this looks much nicer - thanks. >> >>>> + int ret = 0; >>>> + >>>> + init.name = kasprintf(GFP_KERNEL, "%s-clk", >>>> + acpi_dev_name(int3472->adev)); >>> You need to check for NULL and return -ENOMEM. >> Oops, of course, thanks >> >>>> + goto err_unregister_clk; >>> If this fails, you will end up calling clk_unregister() and >>> clkdev_drop() in skl_int3472_discrete_remove(). You should replace the >>> check in the remove function with >>> >>> if (!int3472->clock.cl) { >> You're right, good spot, thank you. >> >>>> + dev_err(&int3472->pdev->dev, "No sensor module config\n"); >>>> + return PTR_ERR(sensor_config); >>>> + } >>> Would it make sense to call this in skl_int3472_discrete_probe() or >>> skl_int3472_parse_crs() and cache the config pointer ? >> Yes, probably actually, and then the GPIO mapping function can just >> check for its presence. >> >>>> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; >>>> + init_data.num_consumer_supplies = 1; >>>> + init_data.consumer_supplies = &sensor_config->supply_map; >>>> + >>>> + snprintf(int3472->regulator.regulator_name, >>>> + GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator"); >>> s/GPIO_REGULATOR_NAME_LENGTH/sizeof(int3472->regulator.regulator_name)/ >>> >>> Do regulator names need to be unique ? If so you may have a problem if a >>> platform has two discrete INT3472. >> Unlike clocks, the regulator framework doesn't shout at you when you do >> this, but I agree it's suboptimal at the very least, I'll set it to >> ..."%s-regulator", acpi_dev_name(int3472->adev)... as with the clock. >> >>>> + case INT3472_GPIO_TYPE_PRIVACY_LED: >>>> + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, >>>> + "indicator-led", >>>> + GPIO_ACTIVE_HIGH); >>> Mapping the indicator LED to the sensor isn't great, as all sensor >>> drivers would then need to handle it. Could it be handled in the >>> regulator instead, so that it would be turned on automatically when the >>> sensor is powered up ? Another option, more complicated, would be to >>> handle it in the CIO2 driver (but I'm not sure how we would map it). >> Not with the regulator, because it turns out only the 0x0b pin is one of >> those and those appear on very few devices in scope, so it wouldn't be >> called on a Surface Book 2 for example. Perhaps as part of clock >> prepare/enable? I don't much like the idea of it running in the CIO2 >> driver to be honest, feels a bit out of place. > The clock is another option, but could there be platforms where the > clock GPIO isn't present ? I haven't ever seen a DSDT that didn't include a 0x0c pin to enable the clock, though that doesn't necessarily mean they're always there. Plenty of driver datasheets say they're happy for the external clock to be free running, so it could just be always active I suppose. > Another option would be to let userspace handle that GPIO, but we then > need to convey it to userspace. Can you point me to an example of that to look at perhaps? >>>> + >>>> + if (int3472->gpios_mapped) >>>> + gpiod_remove_lookup_table(&int3472->gpios); >>> You could avoid the need for the gpios_mapped field by checking for >>> >>> if (int3472->gpios.list.next) >>> >>> Up to you. >> Thank you! I was scratching my head trying to figure out a better way of >> doing that.
On Mon, Jan 18, 2021 at 09:19:52PM +0000, Daniel Scally wrote: > On 18/01/2021 14:46, Andy Shevchenko wrote: > > On Mon, Jan 18, 2021 at 11:15:21AM +0200, Laurent Pinchart wrote: > >> On Mon, Jan 18, 2021 at 12:34:27AM +0000, Daniel Scally wrote: ... > >>> +static struct i2c_driver int3472_tps68470 = { > >>> + .driver = { > >>> + .name = "int3472-tps68470", > >>> + .acpi_match_table = int3472_device_id, > >>> + }, > >>> + .probe_new = skl_int3472_tps68470_probe, > >>> +}; > > I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > Sorry, I'm a bit confused by this. The i2c device is already > present...we just want the driver to bind to them, so what role do those > functions have there? What I meant is something like *_i2c.c real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI involved (and it sounds like it should be mfd/tps one, in which you just cut out ACPI IDs and convert to pure I²C one, that what I had suggested in the first place) *_proxy.c GPIO proxy as library *.c platform driver with ACPI ID, in which ->probe() we actually instantiate above via calling i2c_acpi_new_device(), *if needed*, along with GPIO proxy ... > >>> +struct int3472_gpio_clock { > >>> + struct clk *clk; > >>> + struct clk_hw clk_hw; > >>> + struct clk_lookup *cl; > >>> + struct gpio_desc *gpio; > >>> +}; > >>> +static const struct clk_ops skl_int3472_clock_ops = { > >>> + .prepare = skl_int3472_clk_prepare, > >>> + .unprepare = skl_int3472_clk_unprepare, > >>> + .enable = skl_int3472_clk_enable, > >>> + .disable = skl_int3472_clk_disable, > >>> +}; > > Wondering if this has some similarities with and actually can utilize clk-gpio > > driver. > > Yeah, sounds like reinventing clk-gpio.c. > > > > static const struct clk_ops clk_gpio_gate_ops = { > > .enable = clk_gpio_gate_enable, > > .disable = clk_gpio_gate_disable, > > .is_enabled = clk_gpio_gate_is_enabled, > > }; > > > > Or is it mux? It has support there as well. > > > Hmm, yeah, this looks like it would work actually. So I think I'd need to: > > > 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver > > 2. Register a platform device to bind to the clk-gpio driver > > 3. Register a gpio lookup table so that the clk-gpio driver can find the > gpio in question using gpiod_get() > > And that looks like it will work; I'll try it. You need to modify clk-gpio.c to export clk_hw_register_gpio_gate() clk_hw_register_gpio_mux() (perhaps it will require to add *_unregister() counterparts) and call it from your code. See, for example, how clk_hw_unregister_fixed_rate() is being used. Another case is to add a helper directly into clk-gpio and call it instead of clk_hw_*() one, see how clk_register_fractional_divider() is implemented and used. ... > >>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */ > >>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, > >>> + /* Surface Go 1&2 - OV5693, Front */ > >>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, > > I'm wondering if you should use same I2C format macro and create this > > dynamically? Or rather find a corresponding ACPI device instance and > > copy it's name? ... > > The supply name needs hard-coding really, but the device name I suppose > can come from int3472->sensor_name. To be strict in terms you are using "device instance name" in the REGULATOR_SUPPLY() second parameter. Because "device name" is generic and doesn't point to the actual *instance* of the device in the system. So, and "device name instance" we may get only by traversing through the (ACPI) bus and find corresponding struct device and derive name from it. Same way like you have done in previous patch series. Because there is no guarantee that, e.g., i2c-INT33BE:00 will be present on the system and moreover there is no guarantee that if two INT33BE or more devices are present you will get :00 always for the one you need! ... > >>> + ret = ERR_PTR(-ENODEV); > > This seems redundant. Or are you expecting ARRAY_SIZE() to be 0? > > If no, you may add static_assert() near to the array definition. > > It **could** become 0, if the entries I've added are removed in future > because the sensors are no longer supported or something. There might be > no sensor_module_config for a given device. We only need to supply one if > > a) The platform has a 0x0b type GPIO, which means we need to define a > supply name the driver is expecting > > b) The GPIO functions deviate from documented purpose, which means we > need to supply a remapping struct > > Otherwise, there's no need for it. I see. So, up to you then. > > > >>> + > >>> + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, > >>> + &cfg); > >>> + if (IS_ERR(int3472->regulator.rdev)) { > >>> + ret = PTR_ERR(int3472->regulator.rdev); > >>> + goto err_free_gpio; > >>> + } > > Similar here, can we utilize gpio-regulator.c? > > > > Also yes probably, with the same steps as for the clocks. Again, I'll > try that out, thanks very much. You may look at arch/x86/platform/intel-mid/device_libs/platform_bcm43xx.c (but in your case you probably don't need a GPIO lookup table, just check). ... > >>> + dev_warn(&int3472->pdev->dev, > >>> + "GPIO type 0x%llx unknown; the sensor may not work\n", > >>> + (obj->integer.value & 0xff)); > >> No need for parentheses. > > And instead of "%llx" with " & 0xff" you may use "%x" with "(u8)" cast. > > However, I don't think we need to show only last byte, because it may give > > wrong impression on values like "0x100". > > But in this case only the last byte holds the type information, second > lowest byte is the pin number. So as we understand it, 0x100 would be > invalid anyway. Ah, indeed. So, I think the best is to have %hhx and see if you can supply just obj->integer.value and compiler won't warn about type mismatch. ... > >>> + } else { > >>> + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", > >>> + &client->dev); > >>> + if (IS_ERR(opregion_dev)) { > >>> + ret = PTR_ERR(opregion_dev); > >>> + goto err_free_gpio; > >>> + } > >>> + } > >> I wonder if this could be simplified by using devm_mfd_add_devices. You > >> could have two arrays of mfd_cell, one for each case. > > Yeah, which effectively means that we should have some kind of mfd/tps68470 in > > place. > > Can you expand on what you mean by that a little, please? The very first comment in this reply should hopefully shed a light on my idea.
On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > On 18/01/2021 21:19, Daniel Scally wrote: > I'm more and more confident that this will work, but it has some > knock-on effects: > > The both clk and regulator gpio driver expects to be able to fetch the > GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of > course, so we need to add another GPIO lookup table so those drivers can > see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will > be so we can set the .dev_id member of a gpiod_lookup_table to that > value, but that isn't set until _after_ the pdev is registered (because > it has to figure out the id, we can't manually set the IDs because there > could be more than one instance of int3472-discrete bound to multiple > PMIC devices, and we don't know which id the current one should have). > Finally, we can't wait until the device is registered because it > immediately probes, can't find the GPIO and then fails probe. > > It's similar problem that causes us to need the i2c-acpi name format > macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) > > Solving it is a bit of a sticky one; perhaps something like moving the > dev_set_name() part of platform_device_add() [1] to its own function, > that's called in both platform_device_alloc() and > platform_device_register(). That way it would be available before the > device itself was registered, meaning we could create the lookup table > before it probes the driver. See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of methods to be able to use it as a library. > (also, Laurent, if we did it this way we wouldn't be able to also handle > the led-indicator GPIO here without some fairly major rework) LED indicators are done as LED class devices (see plenty of examples in PDx86 drivers: drivers/platform/x86/)
Morning Andy On 19/01/2021 09:33, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: >> On 18/01/2021 21:19, Daniel Scally wrote: >> I'm more and more confident that this will work, but it has some >> knock-on effects: >> >> The both clk and regulator gpio driver expects to be able to fetch the >> GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of >> course, so we need to add another GPIO lookup table so those drivers can >> see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will >> be so we can set the .dev_id member of a gpiod_lookup_table to that >> value, but that isn't set until _after_ the pdev is registered (because >> it has to figure out the id, we can't manually set the IDs because there >> could be more than one instance of int3472-discrete bound to multiple >> PMIC devices, and we don't know which id the current one should have). >> Finally, we can't wait until the device is registered because it >> immediately probes, can't find the GPIO and then fails probe. >> >> It's similar problem that causes us to need the i2c-acpi name format >> macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) >> >> Solving it is a bit of a sticky one; perhaps something like moving the >> dev_set_name() part of platform_device_add() [1] to its own function, >> that's called in both platform_device_alloc() and >> platform_device_register(). That way it would be available before the >> device itself was registered, meaning we could create the lookup table >> before it probes the driver. > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > methods to be able to use it as a library. Ack! Ok, I thought about that the wrong way. I'll take another look tonight then. >> (also, Laurent, if we did it this way we wouldn't be able to also handle >> the led-indicator GPIO here without some fairly major rework) > LED indicators are done as LED class devices (see plenty of examples in PDx86 > drivers: drivers/platform/x86/) And this too - thanks very much
On Tue, Jan 19, 2021 at 08:21:23AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > On 18/01/2021 21:19, Daniel Scally wrote: ... > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > the led-indicator GPIO here without some fairly major rework) > > Given the additional complexity I don't think it's worth it, your > implementation is fine and code duplication with clk-gpio is minimal. Making clk-gpio.c available as a library is a win in long term and reduces a possible duplication by others in the future. I bet we even might find already clk-gpio parts in some drivers.
On 19/01/2021 09:24, Andy Shevchenko wrote: >>>>> +static struct i2c_driver int3472_tps68470 = { >>>>> + .driver = { >>>>> + .name = "int3472-tps68470", >>>>> + .acpi_match_table = int3472_device_id, >>>>> + }, >>>>> + .probe_new = skl_int3472_tps68470_probe, >>>>> +}; >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... >> Sorry, I'm a bit confused by this. The i2c device is already >> present...we just want the driver to bind to them, so what role do those >> functions have there? > What I meant is something like > > *_i2c.c > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > involved (and it sounds like it should be mfd/tps one, in which you > just cut out ACPI IDs and convert to pure I²C one, that what I had > suggested in the first place) Ahh; sorry - i misunderstood what you meant there. I understand now I think, but there is one complication; the ACPI subsystem already creates a client for that i2c adapter and address; i2c_new_client_device() includes a check to see whether that adapter / address combination has an i2c device already. So we would have to have the platform driver with ACPI ID first find the existing i2c_client and unregister it before registering the new one...the existing clients have a name matching the ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an i2c_device_id of course. > > *_proxy.c > GPIO proxy as library > > *.c > platform driver with ACPI ID, in which ->probe() we actually instantiate > above via calling i2c_acpi_new_device(), *if needed*, along with GPIO > proxy > > ... > >>>>> +struct int3472_gpio_clock { >>>>> + struct clk *clk; >>>>> + struct clk_hw clk_hw; >>>>> + struct clk_lookup *cl; >>>>> + struct gpio_desc *gpio; >>>>> +}; >>>>> +static const struct clk_ops skl_int3472_clock_ops = { >>>>> + .prepare = skl_int3472_clk_prepare, >>>>> + .unprepare = skl_int3472_clk_unprepare, >>>>> + .enable = skl_int3472_clk_enable, >>>>> + .disable = skl_int3472_clk_disable, >>>>> +}; >>> Wondering if this has some similarities with and actually can utilize clk-gpio >>> driver. >>> Yeah, sounds like reinventing clk-gpio.c. >>> >>> static const struct clk_ops clk_gpio_gate_ops = { >>> .enable = clk_gpio_gate_enable, >>> .disable = clk_gpio_gate_disable, >>> .is_enabled = clk_gpio_gate_is_enabled, >>> }; >>> >>> Or is it mux? It has support there as well. >>> >> Hmm, yeah, this looks like it would work actually. So I think I'd need to: >> >> >> 1. Make enabling INTEL_SKL_INT3472 also enable the clk-gpio driver >> >> 2. Register a platform device to bind to the clk-gpio driver >> >> 3. Register a gpio lookup table so that the clk-gpio driver can find the >> gpio in question using gpiod_get() >> >> And that looks like it will work; I'll try it. > You need to modify clk-gpio.c to export > > clk_hw_register_gpio_gate() > clk_hw_register_gpio_mux() > > (perhaps it will require to add *_unregister() counterparts) and call it from > your code. > > See, for example, how clk_hw_unregister_fixed_rate() is being used. Another > case is to add a helper directly into clk-gpio and call it instead of > clk_hw_*() one, see how clk_register_fractional_divider() is implemented and > used. I'll take a look, thanks > ... > >>>>> + /* Lenovo Miix 510-12ISK - OV5648, Rear */ >>>>> + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, >>>>> + /* Surface Go 1&2 - OV5693, Front */ >>>>> + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, >>> I'm wondering if you should use same I2C format macro and create this >>> dynamically? Or rather find a corresponding ACPI device instance and >>> copy it's name? ... >> The supply name needs hard-coding really, but the device name I suppose >> can come from int3472->sensor_name. > To be strict in terms you are using "device instance name" in the > REGULATOR_SUPPLY() second parameter. Because "device name" is generic and > doesn't point to the actual *instance* of the device in the system. > > So, and "device name instance" we may get only by traversing through the (ACPI) > bus and find corresponding struct device and derive name from it. Same way like > you have done in previous patch series. > > Because there is no guarantee that, e.g., i2c-INT33BE:00 will be present on > the system and moreover there is no guarantee that if two INT33BE or more > devices are present you will get :00 always for the one you need! Mm, good point, hadn't considered two identical sensors on the same platform. Alright; I'll think about this in more detail, thank you. >>>>> + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", >>>>> + &client->dev); >>>>> + if (IS_ERR(opregion_dev)) { >>>>> + ret = PTR_ERR(opregion_dev); >>>>> + goto err_free_gpio; >>>>> + } >>>>> + } >>>> I wonder if this could be simplified by using devm_mfd_add_devices. You >>>> could have two arrays of mfd_cell, one for each case. >>> Yeah, which effectively means that we should have some kind of mfd/tps68470 in >>> place. >> Can you expand on what you mean by that a little, please? > The very first comment in this reply should hopefully shed a light on my idea. > It did, thanks
Hi Daniel, On 18/01/2021 00:34, Daniel Scally wrote: > ACPI devices with _HID INT3472 are currently matched to the tps68470 > driver, however this does not cover all situations in which that _HID > occurs. We've encountered three possibilities: > > 1. On Chrome OS devices, an ACPI device with _HID INT3472 (representing > a physical tps68470 device) that requires a GPIO and OpRegion driver > 2. On devices designed for Windows, an ACPI device with _HID INT3472 > (again representing a physical tps68470 device) which requires GPIO, > Clock and Regulator drivers. > 3. On other devices designed for Windows, an ACPI device with _HID > INT3472 which does NOT represent a physical tps68470, and is instead > used as a dummy device to group some system GPIO lines which are meant > to be consumed by the sensor that is dependent on this entry. > > This commit adds a new module, registering a platform driver to deal > with the 3rd scenario plus an i2c-driver to deal with #1 and #2, by > querying the CLDB buffer found against INT3472 entries to determine > which is most appropriate. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Scally <djrscally@gmail.com> > --- > Changes in v2: > > - Switched to a module registering a platform driver to run > the dummy ACPI devices, plus an i2c driver to replace and extend > the existing tps68470 driver > - Added clock handling functions to the int3472-discrete driver > - A whole bunch of other changes too numerous to enumerate > MAINTAINERS | 5 + > drivers/platform/x86/Kconfig | 25 + > drivers/platform/x86/Makefile | 4 + > .../platform/x86/intel_skl_int3472_common.c | 100 ++++ > .../platform/x86/intel_skl_int3472_common.h | 100 ++++ > .../platform/x86/intel_skl_int3472_discrete.c | 496 ++++++++++++++++++ > .../platform/x86/intel_skl_int3472_tps68470.c | 145 +++++ > 7 files changed, 875 insertions(+) > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_common.h > create mode 100644 drivers/platform/x86/intel_skl_int3472_discrete.c > create mode 100644 drivers/platform/x86/intel_skl_int3472_tps68470.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a091b496fdd8..c4ed8c3bc58e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9147,6 +9147,11 @@ S: Maintained > F: arch/x86/include/asm/intel_scu_ipc.h > F: drivers/platform/x86/intel_scu_* > > +INTEL SKL INT3472 ACPI DEVICE DRIVER > +M: Daniel Scally <djrscally@gmail.com> > +S: Maintained > +F: drivers/platform/x86/intel_skl_int3472_* > + > INTEL SPEED SELECT TECHNOLOGY > M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > L: platform-driver-x86@vger.kernel.org > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 91e6176cdfbd..916b077df2d5 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -844,6 +844,31 @@ config INTEL_CHT_INT33FE > device and CONFIG_TYPEC_FUSB302=m and CONFIG_BATTERY_MAX17042=m > for Type-C device. > > +config INTEL_SKL_INT3472 > + tristate "Intel SkyLake ACPI INT3472 Driver" > + depends on X86 && ACPI > + select REGMAP_I2C I've tried compiling this as a built in and a module and on my minimal config I had failures on both for regulator_register and regulator_unregister. I suspect this needs to have either a selects or a depends upon CONFIG_REGULATOR -- Regards Kieran > + help > + This driver adds support for the INT3472 ACPI devices found on some > + Intel SkyLake devices. > + > + There are 3 kinds of INT3472 ACPI device possible; two for devices > + designed for Windows (either with or without a physical tps68470 > + PMIC) and one designed for Chrome OS. This driver handles all three > + situations by discovering information it needs to discern them at > + runtime. > + > + If your device was designed for Chrome OS, this driver will provide > + an ACPI operation region, which must be available before any of the > + devices using this are probed. For this reason, you should select Y > + if your device was designed for ChromeOS. This option also configures > + the designware-i2c driver to be built-in, for the same reason. > + > + Say Y or M here if you have a SkyLake device designed for use > + with Windows or ChromeOS. Say N here if you are not sure. > + > + The module will be named "intel-skl-int3472" > + > config INTEL_HID_EVENT > tristate "INTEL HID Event" > depends on ACPI > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 581475f59819..ae29c66842ca 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -86,6 +86,10 @@ obj-$(CONFIG_INTEL_HID_EVENT) += intel-hid.o > obj-$(CONFIG_INTEL_INT0002_VGPIO) += intel_int0002_vgpio.o > obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o > obj-$(CONFIG_INTEL_OAKTRAIL) += intel_oaktrail.o > +obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472.o > +intel_skl_int3472-objs := intel_skl_int3472_common.o \ > + intel_skl_int3472_discrete.o \ > + intel_skl_int3472_tps68470.o > obj-$(CONFIG_INTEL_VBTN) += intel-vbtn.o > > # MSI > diff --git a/drivers/platform/x86/intel_skl_int3472_common.c b/drivers/platform/x86/intel_skl_int3472_common.c > new file mode 100644 > index 000000000000..08cb9d3c06aa > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_common.c > @@ -0,0 +1,100 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/acpi.h> > +#include <linux/i2c.h> > +#include <linux/platform_device.h> > + > +#include "intel_skl_int3472_common.h" > + > +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > + struct int3472_cldb *cldb) > +{ > + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > + acpi_handle handle = adev->handle; > + union acpi_object *obj; > + acpi_status status; > + int ret = 0; > + > + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer); > + if (ACPI_FAILURE(status)) > + return -ENODEV; > + > + obj = buffer.pointer; > + if (!obj) { > + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); > + return -ENODEV; > + } > + > + if (obj->type != ACPI_TYPE_BUFFER) { > + dev_err(&adev->dev, "CLDB object is not an ACPI buffer\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + if (obj->buffer.length > sizeof(*cldb)) { > + dev_err(&adev->dev, "The CLDB buffer is too large\n"); > + ret = -EINVAL; > + goto out_free_buff; > + } > + > + memcpy(cldb, obj->buffer.pointer, obj->buffer.length); > + > +out_free_buff: > + kfree(buffer.pointer); > + return ret; > +} > + > +static const struct acpi_device_id int3472_device_id[] = { > + { "INT3472", 0 }, > + { }, > +}; > +MODULE_DEVICE_TABLE(acpi, int3472_device_id); > + > +static struct platform_driver int3472_discrete = { > + .driver = { > + .name = "int3472-discrete", > + .acpi_match_table = int3472_device_id, > + }, > + .probe = skl_int3472_discrete_probe, > + .remove = skl_int3472_discrete_remove, > +}; > + > +static struct i2c_driver int3472_tps68470 = { > + .driver = { > + .name = "int3472-tps68470", > + .acpi_match_table = int3472_device_id, > + }, > + .probe_new = skl_int3472_tps68470_probe, > +}; > + > +static int skl_int3472_init(void) > +{ > + int ret = 0; > + > + ret = platform_driver_register(&int3472_discrete); > + if (ret) > + return ret; > + > + ret = i2c_register_driver(THIS_MODULE, &int3472_tps68470); > + if (ret) > + goto err_unregister_plat_drv; > + > + return 0; > + > +err_unregister_plat_drv: > + platform_driver_unregister(&int3472_discrete); > + return ret; > +} > +module_init(skl_int3472_init); > + > +static void skl_int3472_exit(void) > +{ > + platform_driver_unregister(&int3472_discrete); > + i2c_del_driver(&int3472_tps68470); > +} > +module_exit(skl_int3472_exit); > + > +MODULE_DESCRIPTION("Intel SkyLake INT3472 ACPI Device Driver"); > +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>"); > +MODULE_LICENSE("GPL v2"); > diff --git a/drivers/platform/x86/intel_skl_int3472_common.h b/drivers/platform/x86/intel_skl_int3472_common.h > new file mode 100644 > index 000000000000..9acd0ce7aed9 > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_common.h > @@ -0,0 +1,100 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Author: Dan Scally <djrscally@gmail.com> */ > +#include <linux/regulator/machine.h> > +#include <linux/clk-provider.h> > +#include <linux/gpio/machine.h> > +#include <linux/regulator/driver.h> > +#include <linux/types.h> > + > +/* PMIC GPIO Types */ > +#define INT3472_GPIO_TYPE_RESET 0x00 > +#define INT3472_GPIO_TYPE_POWERDOWN 0x01 > +#define INT3472_GPIO_TYPE_CLK_ENABLE 0x0c > +#define INT3472_GPIO_TYPE_POWER_ENABLE 0x0b > +#define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d > +#define INT3472_PDEV_MAX_NAME_LEN 23 > +#define INT3472_MAX_SENSOR_GPIOS 3 > +#define GPIO_REGULATOR_NAME_LENGTH 27 > +#define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 > + > +#define INT3472_REGULATOR(_NAME, _SUPPLY, _OPS) \ > + (const struct regulator_desc) { \ > + .name = _NAME, \ > + .supply_name = _SUPPLY, \ > + .id = 0, \ > + .type = REGULATOR_VOLTAGE, \ > + .ops = _OPS, \ > + .owner = THIS_MODULE, \ > + } > + > +#define INT3472_GPIO_FUNCTION_REMAP(_PIN, _FUNCTION) \ > + (const struct int3472_gpio_function_remap) { \ > + .documented = _PIN, \ > + .actual = _FUNCTION \ > + } > + > +#define to_int3472_clk(hw) \ > + container_of(hw, struct int3472_gpio_clock, clk_hw) > + > +struct int3472_cldb { > + u8 version; > + /* > + * control logic type > + * 0: UNKNOWN > + * 1: DISCRETE(CRD-D) > + * 2: PMIC TPS68470 > + * 3: PMIC uP6641 > + */ > + u8 control_logic_type; > + u8 control_logic_id; > + u8 sensor_card_sku; > + u8 reserved[28]; > +}; > + > +struct int3472_gpio_regulator { > + char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > + char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; > + struct gpio_desc *gpio; > + struct regulator_dev *rdev; > + struct regulator_desc rdesc; > +}; > + > +struct int3472_gpio_clock { > + struct clk *clk; > + struct clk_hw clk_hw; > + struct clk_lookup *cl; > + struct gpio_desc *gpio; > +}; > + > +struct int3472_device { > + struct acpi_device *adev; > + struct platform_device *pdev; > + struct acpi_device *sensor; > + char *sensor_name; > + > + unsigned int n_gpios; /* how many GPIOs have we seen */ > + > + struct int3472_gpio_regulator regulator; > + struct int3472_gpio_clock clock; > + > + unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ > + bool gpios_mapped; > + struct gpiod_lookup_table gpios; > +}; > + > +struct int3472_gpio_function_remap { > + char *documented; > + char *actual; > +}; > + > +struct int3472_sensor_config { > + char *sensor_module_name; > + struct regulator_consumer_supply supply_map; > + const struct int3472_gpio_function_remap *function_maps; > +}; > + > +int skl_int3472_discrete_probe(struct platform_device *pdev); > +int skl_int3472_discrete_remove(struct platform_device *pdev); > +int skl_int3472_tps68470_probe(struct i2c_client *client); > +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > + struct int3472_cldb *cldb); > diff --git a/drivers/platform/x86/intel_skl_int3472_discrete.c b/drivers/platform/x86/intel_skl_int3472_discrete.c > new file mode 100644 > index 000000000000..93d250dca92f > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_discrete.c > @@ -0,0 +1,496 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/acpi.h> > +#include <linux/clkdev.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regulator/driver.h> > + > +#include "intel_skl_int3472_common.h" > + > +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */ > +static const guid_t int3472_gpio_guid = > + GUID_INIT(0x79234640, 0x9e10, 0x4fea, > + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); > + > +/* 822ace8f-2814-4174-a56b-5f029fe079ee */ > +static const guid_t cio2_sensor_module_guid = > + GUID_INIT(0x822ace8f, 0x2814, 0x4174, > + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); > + > +/* > + * Here follows platform specific mapping information that we can pass to > + * the functions mapping resources to the sensors. Where the sensors have > + * a power enable pin defined in DSDT we need to provide a supply name so > + * the sensor drivers can find the regulator. Optionally, we can provide a > + * NULL terminated array of function name mappings to deal with any platform > + * specific deviations from the documented behaviour of GPIOs. > + * > + * Map a GPIO function name to NULL to prevent the driver from mapping that > + * GPIO at all. > + */ > + > +static const struct int3472_gpio_function_remap ov2680_gpio_function_remaps[] = { > + INT3472_GPIO_FUNCTION_REMAP("reset", NULL), > + INT3472_GPIO_FUNCTION_REMAP("powerdown", "reset"), > + { } > +}; > + > +static struct int3472_sensor_config int3472_sensor_configs[] = { > + /* Lenovo Miix 510-12ISK - OV2680, Front */ > + { "GNDF140809R", { 0 }, ov2680_gpio_function_remaps}, > + /* Lenovo Miix 510-12ISK - OV5648, Rear */ > + { "GEFF150023R", REGULATOR_SUPPLY("avdd", "i2c-OVTI5648:00"), NULL}, > + /* Surface Go 1&2 - OV5693, Front */ > + { "YHCU", REGULATOR_SUPPLY("avdd", "i2c-INT33BE:00"), NULL}, > +}; > + > +/* > + * The regulators have to have .ops to be valid, but the only ops we actually > + * support are .enable and .disable which are handled via .ena_gpiod. Pass an > + * empty struct to clear the check without lying about capabilities. > + */ > +static const struct regulator_ops int3472_gpio_regulator_ops = { 0 }; > + > +static int skl_int3472_clk_enable(struct clk_hw *hw) > +{ > + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > + > + gpiod_set_value(clk->gpio, 1); > + > + return 0; > +} > + > +static void skl_int3472_clk_disable(struct clk_hw *hw) > +{ > + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > + > + gpiod_set_value(clk->gpio, 0); > +} > + > +static int skl_int3472_clk_prepare(struct clk_hw *hw) > +{ > + /* > + * We're just turning a GPIO on to enable, so nothing to do here, but > + * we want to provide the op so prepare_enable() works. > + */ > + return 0; > +} > + > +static void skl_int3472_clk_unprepare(struct clk_hw *hw) > +{ > + /* Likewise, nothing to do here... */ > +} > + > +static const struct clk_ops skl_int3472_clock_ops = { > + .prepare = skl_int3472_clk_prepare, > + .unprepare = skl_int3472_clk_unprepare, > + .enable = skl_int3472_clk_enable, > + .disable = skl_int3472_clk_disable, > +}; > + > +static struct int3472_sensor_config * > +skl_int3472_get_sensor_module_config(struct int3472_device *int3472) > +{ > + unsigned int i = ARRAY_SIZE(int3472_sensor_configs); > + struct int3472_sensor_config *ret; > + union acpi_object *obj; > + > + obj = acpi_evaluate_dsm_typed(int3472->sensor->handle, > + &cio2_sensor_module_guid, 0x00, > + 0x01, NULL, ACPI_TYPE_STRING); > + > + if (!obj) { > + dev_err(&int3472->pdev->dev, > + "Failed to get sensor module string from _DSM\n"); > + return ERR_PTR(-ENODEV); > + } > + > + if (obj->string.type != ACPI_TYPE_STRING) { > + dev_err(&int3472->pdev->dev, > + "Sensor _DSM returned a non-string value\n"); > + ret = ERR_PTR(-EINVAL); > + goto out_free_obj; > + } > + > + ret = ERR_PTR(-ENODEV); > + while (i--) { > + if (!strcmp(int3472_sensor_configs[i].sensor_module_name, > + obj->string.pointer)) { > + ret = &int3472_sensor_configs[i]; > + goto out_free_obj; > + } > + } > + > +out_free_obj: > + ACPI_FREE(obj); > + return ret; > +} > + > +static int skl_int3472_map_gpio_to_sensor(struct int3472_device *int3472, > + struct acpi_resource *ares, > + char *func, u32 polarity) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct int3472_sensor_config *sensor_config; > + struct gpiod_lookup table_entry; > + struct acpi_device *adev; > + acpi_handle handle; > + acpi_status status; > + int ret; > + > + sensor_config = skl_int3472_get_sensor_module_config(int3472); > + if (!IS_ERR(sensor_config) && sensor_config->function_maps) { > + unsigned int i = 0; > + > + while (sensor_config->function_maps[i].documented) { > + if (!strcmp(func, sensor_config->function_maps[i].documented)) { > + func = sensor_config->function_maps[i].actual; > + > + break; > + } > + > + i++; > + } > + } > + > + if (!func) > + return 0; > + > + if (int3472->n_sensor_gpios >= INT3472_MAX_SENSOR_GPIOS) { > + dev_warn(&int3472->pdev->dev, "Too many GPIOs mapped\n"); > + return -EINVAL; > + } > + > + status = acpi_get_handle(NULL, path, &handle); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + > + ret = acpi_bus_get_device(handle, &adev); > + if (ret) > + return -ENODEV; > + > + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > + ares->data.gpio.pin_table[0], > + func, 0, polarity); > + > + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, > + sizeof(table_entry)); > + > + int3472->n_sensor_gpios++; > + > + return 0; > +} > + > +static int skl_int3472_register_clock(struct int3472_device *int3472, > + struct acpi_resource *ares) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct clk_init_data init = { }; > + int ret = 0; > + > + init.name = kasprintf(GFP_KERNEL, "%s-clk", > + acpi_dev_name(int3472->adev)); > + init.ops = &skl_int3472_clock_ops; > + > + int3472->clock.gpio = acpi_get_gpiod(path, > + ares->data.gpio.pin_table[0]); > + if (IS_ERR(int3472->clock.gpio)) { > + ret = PTR_ERR(int3472->clock.gpio); > + goto out_free_init_name; > + } > + > + int3472->clock.clk_hw.init = &init; > + int3472->clock.clk = clk_register(&int3472->adev->dev, > + &int3472->clock.clk_hw); > + if (IS_ERR(int3472->clock.clk)) { > + ret = PTR_ERR(int3472->clock.clk); > + goto err_put_gpio; > + } > + > + int3472->clock.cl = clkdev_create(int3472->clock.clk, "xvclk", > + int3472->sensor_name); > + if (IS_ERR_OR_NULL(int3472->clock.cl)) > + goto err_unregister_clk; > + > + goto out_free_init_name; > + > +err_unregister_clk: > + clk_unregister(int3472->clock.clk); > +err_put_gpio: > + gpiod_put(int3472->clock.gpio); > +out_free_init_name: > + kfree(init.name); > + > + return ret; > +} > + > +static int skl_int3472_register_regulator(struct int3472_device *int3472, > + struct acpi_resource *ares) > +{ > + char *path = ares->data.gpio.resource_source.string_ptr; > + struct int3472_sensor_config *sensor_config; > + struct regulator_init_data init_data = { }; > + struct regulator_config cfg = { }; > + int ret; > + > + sensor_config = skl_int3472_get_sensor_module_config(int3472); > + if (IS_ERR_OR_NULL(sensor_config)) { > + dev_err(&int3472->pdev->dev, "No sensor module config\n"); > + return PTR_ERR(sensor_config); > + } > + > + if (!sensor_config->supply_map.supply) { > + dev_err(&int3472->pdev->dev, "No supply name defined\n"); > + return -ENODEV; > + } > + > + init_data.supply_regulator = NULL; > + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; > + init_data.num_consumer_supplies = 1; > + init_data.consumer_supplies = &sensor_config->supply_map; > + > + snprintf(int3472->regulator.regulator_name, > + GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator"); > + snprintf(int3472->regulator.supply_name, > + GPIO_REGULATOR_SUPPLY_NAME_LENGTH, "supply-0"); > + > + int3472->regulator.rdesc = INT3472_REGULATOR( > + int3472->regulator.regulator_name, > + int3472->regulator.supply_name, > + &int3472_gpio_regulator_ops); > + > + int3472->regulator.gpio = acpi_get_gpiod(path, > + ares->data.gpio.pin_table[0]); > + if (IS_ERR(int3472->regulator.gpio)) { > + dev_err(&int3472->pdev->dev, "Failed to get GPIO line\n"); > + return PTR_ERR(int3472->regulator.gpio); > + } > + > + cfg.dev = &int3472->adev->dev; > + cfg.init_data = &init_data; > + cfg.ena_gpiod = int3472->regulator.gpio; > + > + int3472->regulator.rdev = regulator_register(&int3472->regulator.rdesc, > + &cfg); > + if (IS_ERR(int3472->regulator.rdev)) { > + ret = PTR_ERR(int3472->regulator.rdev); > + goto err_free_gpio; > + } > + > + return 0; > + > +err_free_gpio: > + gpiod_put(int3472->regulator.gpio); > + > + return ret; > +} > + > +/** > + * skl_int3472_handle_gpio_resources: maps PMIC resources to consuming sensor > + * @ares: A pointer to a &struct acpi_resource > + * @data: A pointer to a &struct int3472_device > + * > + * This function handles GPIO resources that are against an INT3472 > + * ACPI device, by checking the value of the corresponding _DSM entry. > + * This will return a 32bit int, where the lowest byte represents the > + * function of the GPIO pin: > + * > + * 0x00 Reset > + * 0x01 Power down > + * 0x0b Power enable > + * 0x0c Clock enable > + * 0x0d Privacy LED > + * > + * There are some known platform specific quirks where that does not quite > + * hold up; for example where a pin with type 0x01 (Power down) is mapped to > + * a sensor pin that performs a reset function or entries in _CRS and _DSM that > + * do not actually correspond to a physical connection. These will be handled by > + * the mapping sub-functions. > + * > + * GPIOs will either be mapped directly to the sensor device or else used > + * to create clocks and regulators via the usual frameworks. > + * > + * Return: > + * * 0 - When all resources found are handled properly. > + * * -EINVAL - If the resource is not a GPIO IO resource > + * * -ENODEV - If the resource has no corresponding _DSM entry > + * * -Other - Errors propagated from one of the sub-functions. > + */ > +static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > + void *data) > +{ > + struct int3472_device *int3472 = data; > + union acpi_object *obj; > + int ret = 0; > + > + if (ares->type != ACPI_RESOURCE_TYPE_GPIO || > + ares->data.gpio.connection_type != ACPI_RESOURCE_GPIO_TYPE_IO) > + return EINVAL; /* Deliberately positive so parsing continues */ > + > + /* > + * n_gpios + 2 because the index of this _DSM function is 1-based and > + * the first function is just a count. > + */ > + obj = acpi_evaluate_dsm_typed(int3472->adev->handle, > + &int3472_gpio_guid, 0x00, > + int3472->n_gpios + 2, > + NULL, ACPI_TYPE_INTEGER); > + > + if (!obj) { > + dev_warn(&int3472->pdev->dev, > + "No _DSM entry for this GPIO pin\n"); > + return ENODEV; > + } > + > + switch (obj->integer.value & 0xff) { > + case INT3472_GPIO_TYPE_RESET: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "reset", > + GPIO_ACTIVE_LOW); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map reset pin to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_POWERDOWN: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, "powerdown", > + GPIO_ACTIVE_LOW); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map powerdown pin to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_CLK_ENABLE: > + ret = skl_int3472_register_clock(int3472, ares); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map clock to sensor\n"); > + > + break; > + case INT3472_GPIO_TYPE_POWER_ENABLE: > + ret = skl_int3472_register_regulator(int3472, ares); > + if (ret) { > + dev_err(&int3472->pdev->dev, > + "Failed to map regulator to sensor\n"); > + } > + > + break; > + case INT3472_GPIO_TYPE_PRIVACY_LED: > + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, > + "indicator-led", > + GPIO_ACTIVE_HIGH); > + if (ret) > + dev_err(&int3472->pdev->dev, > + "Failed to map indicator led to sensor\n"); > + > + break; > + default: > + dev_warn(&int3472->pdev->dev, > + "GPIO type 0x%llx unknown; the sensor may not work\n", > + (obj->integer.value & 0xff)); > + ret = EINVAL; > + } > + > + int3472->n_gpios++; > + ACPI_FREE(obj); > + > + return ret; > +} > + > +static int skl_int3472_parse_crs(struct int3472_device *int3472) > +{ > + struct list_head resource_list; > + int ret = 0; > + > + INIT_LIST_HEAD(&resource_list); > + > + ret = acpi_dev_get_resources(int3472->adev, &resource_list, > + skl_int3472_handle_gpio_resources, > + int3472); > + > + if (!ret) { > + gpiod_add_lookup_table(&int3472->gpios); > + int3472->gpios_mapped = true; > + } > + > + acpi_dev_free_resource_list(&resource_list); > + > + return ret; > +} > + > +int skl_int3472_discrete_probe(struct platform_device *pdev) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); > + struct int3472_device *int3472; > + struct int3472_cldb cldb; > + int ret = 0; > + > + ret = skl_int3472_get_cldb_buffer(adev, &cldb); > + if (ret || cldb.control_logic_type != 1) > + return -EINVAL; > + > + int3472 = kzalloc(sizeof(*int3472) + > + ((INT3472_MAX_SENSOR_GPIOS + 1) * sizeof(struct gpiod_lookup)), > + GFP_KERNEL); > + if (!int3472) > + return -ENOMEM; > + > + int3472->adev = adev; > + int3472->pdev = pdev; > + platform_set_drvdata(pdev, int3472); > + > + int3472->sensor = acpi_dev_get_next_dep_dev(adev, NULL); > + if (!int3472->sensor) { > + dev_err(&pdev->dev, > + "This INT3472 entry seems to have no dependents.\n"); > + ret = -ENODEV; > + goto err_free_int3472; > + } > + int3472->sensor_name = i2c_acpi_dev_name(int3472->sensor); > + int3472->gpios.dev_id = int3472->sensor_name; > + > + ret = skl_int3472_parse_crs(int3472); > + if (ret) { > + skl_int3472_discrete_remove(pdev); > + goto err_return_ret; > + } > + > + return 0; > + > +err_free_int3472: > + kfree(int3472); > +err_return_ret: > + return ret; > +} > + > +int skl_int3472_discrete_remove(struct platform_device *pdev) > +{ > + struct int3472_device *int3472; > + > + int3472 = platform_get_drvdata(pdev); > + > + if (int3472->gpios_mapped) > + gpiod_remove_lookup_table(&int3472->gpios); > + > + if (!IS_ERR_OR_NULL(int3472->regulator.rdev)) { > + gpiod_put(int3472->regulator.gpio); > + regulator_unregister(int3472->regulator.rdev); > + } > + > + if (!IS_ERR_OR_NULL(int3472->clock.clk)) { > + gpiod_put(int3472->clock.gpio); > + clk_unregister(int3472->clock.clk); > + clkdev_drop(int3472->clock.cl); > + } > + > + acpi_dev_put(int3472->sensor); > + > + kfree(int3472->sensor_name); > + kfree(int3472); > + > + return 0; > +} > diff --git a/drivers/platform/x86/intel_skl_int3472_tps68470.c b/drivers/platform/x86/intel_skl_int3472_tps68470.c > new file mode 100644 > index 000000000000..9bda65d34987 > --- /dev/null > +++ b/drivers/platform/x86/intel_skl_int3472_tps68470.c > @@ -0,0 +1,145 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Author: Dan Scally <djrscally@gmail.com> */ > + > +#include <linux/i2c.h> > +#include <linux/mfd/tps68470.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include "intel_skl_int3472_common.h" > + > +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 struct platform_device * > +skl_int3472_register_pdev(const char *name, struct device *parent) > +{ > + struct platform_device *pdev; > + int ret; > + > + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE); > + if (IS_ERR_OR_NULL(pdev)) > + return ERR_PTR(-ENOMEM); > + > + pdev->dev.parent = parent; > + pdev->driver_override = kstrndup(pdev->name, INT3472_PDEV_MAX_NAME_LEN, > + GFP_KERNEL); > + > + ret = platform_device_add(pdev); > + if (ret) { > + platform_device_put(pdev); > + return ERR_PTR(ret); > + } > + > + return pdev; > +} > + > +int skl_int3472_tps68470_probe(struct i2c_client *client) > +{ > + struct acpi_device *adev = ACPI_COMPANION(&client->dev); > + struct platform_device *regulator_dev; > + struct platform_device *opregion_dev; > + struct platform_device *gpio_dev; > + struct int3472_cldb cldb = { 0 }; > + struct platform_device *clk_dev; > + bool cldb_present = true; > + struct regmap *regmap; > + int ret = 0; > + > + regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(&client->dev, "devm_regmap_init_i2c Error %ld\n", > + PTR_ERR(regmap)); > + return PTR_ERR(regmap); > + } > + > + i2c_set_clientdata(client, regmap); > + > + ret = tps68470_chip_init(&client->dev, regmap); > + if (ret < 0) { > + dev_err(&client->dev, "TPS68470 Init Error %d\n", ret); > + return ret; > + } > + > + /* > + * Check CLDB buffer against the PMIC's adev. If present, then we check > + * the value of control_logic_type field and follow one of the > + * following scenarios: > + * > + * 1. No CLDB - likely ACPI tables designed for ChromeOS. We > + * create platform devices for the GPIOs and OpRegion drivers. > + * > + * 2. CLDB, with control_logic_type = 2 - probably ACPI tables > + * made for Windows 2-in-1 platforms. Register pdevs for GPIO, > + * Clock and Regulator drivers to bind to. > + * > + * 3. Any other value in control_logic_type, we should never have > + * gotten to this point; crash and burn. > + */ > + ret = skl_int3472_get_cldb_buffer(adev, &cldb); > + if (!ret && cldb.control_logic_type != 2) > + return -EINVAL; > + > + if (ret) > + cldb_present = false; > + > + gpio_dev = skl_int3472_register_pdev("tps68470-gpio", &client->dev); > + if (IS_ERR(gpio_dev)) > + return PTR_ERR(gpio_dev); > + > + if (cldb_present) { > + clk_dev = skl_int3472_register_pdev("tps68470-clk", > + &client->dev); > + if (IS_ERR(clk_dev)) { > + ret = PTR_ERR(clk_dev); > + goto err_free_gpio; > + } > + > + regulator_dev = skl_int3472_register_pdev("tps68470-regulator", > + &client->dev); > + if (IS_ERR(regulator_dev)) { > + ret = PTR_ERR(regulator_dev); > + goto err_free_clk; > + } > + } else { > + opregion_dev = skl_int3472_register_pdev("tps68470_pmic_opregion", > + &client->dev); > + if (IS_ERR(opregion_dev)) { > + ret = PTR_ERR(opregion_dev); > + goto err_free_gpio; > + } > + } > + > + return 0; > + > +err_free_clk: > + platform_device_put(clk_dev); > +err_free_gpio: > + platform_device_put(gpio_dev); > + > + return ret; > +} >
On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > On 19/01/2021 09:24, Andy Shevchenko wrote: > >>>>> +static struct i2c_driver int3472_tps68470 = { > >>>>> + .driver = { > >>>>> + .name = "int3472-tps68470", > >>>>> + .acpi_match_table = int3472_device_id, > >>>>> + }, > >>>>> + .probe_new = skl_int3472_tps68470_probe, > >>>>> +}; > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > >> Sorry, I'm a bit confused by this. The i2c device is already > >> present...we just want the driver to bind to them, so what role do those > >> functions have there? > > What I meant is something like > > > > *_i2c.c > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > involved (and it sounds like it should be mfd/tps one, in which you > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > suggested in the first place) > > Ahh; sorry - i misunderstood what you meant there. I understand now I > think, but there is one complication; the ACPI subsystem already creates > a client for that i2c adapter and address; i2c_new_client_device() > includes a check to see whether that adapter / address combination has > an i2c device already. So we would have to have the platform driver > with ACPI ID first find the existing i2c_client and unregister it before > registering the new one...the existing clients have a name matching the > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > i2c_device_id of course. See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 static const struct acpi_device_id i2c_multi_instantiate_ids[] = { {"BSG1160", }, {"BSG2150", }, {"INT33FE", }, {"INT3515", }, {} }; So, we quirklist it here and instantiate manually from platform driver (new coming one). ... > > You need to modify clk-gpio.c to export > > > > clk_hw_register_gpio_gate() > > clk_hw_register_gpio_mux() > > > > (perhaps it will require to add *_unregister() counterparts) and call it from > > your code. > > > > See, for example, how clk_hw_unregister_fixed_rate() is being used. Another Here I meant of course clk_hw_register_fixed_rate(). > > case is to add a helper directly into clk-gpio and call it instead of > > clk_hw_*() one, see how clk_register_fractional_divider() is implemented and > > used. > > I'll take a look, thanks
On Tue, Jan 19, 2021 at 10:56:17AM +0000, Kieran Bingham wrote: > On 18/01/2021 00:34, Daniel Scally wrote: ... > > +config INTEL_SKL_INT3472 > > + tristate "Intel SkyLake ACPI INT3472 Driver" > > + depends on X86 && ACPI > > + select REGMAP_I2C > > I've tried compiling this as a built in and a module and on my minimal > config I had failures on both for regulator_register and > regulator_unregister. > > I suspect this needs to have either a selects or a depends upon > CONFIG_REGULATOR Valid point, although it seems no consensus on which is better to use. It seems to me that in this case we need to select it.
On 19/01/2021 11:11, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 10:56:17AM +0000, Kieran Bingham wrote: >> On 18/01/2021 00:34, Daniel Scally wrote: > ... > >>> +config INTEL_SKL_INT3472 >>> + tristate "Intel SkyLake ACPI INT3472 Driver" >>> + depends on X86 && ACPI >>> + select REGMAP_I2C >> I've tried compiling this as a built in and a module and on my minimal >> config I had failures on both for regulator_register and >> regulator_unregister. >> >> I suspect this needs to have either a selects or a depends upon >> CONFIG_REGULATOR > Valid point, although it seems no consensus on which is better to use. It seems > to me that in this case we need to select it. > Yeah; it will be necessary for the gpio-regulator too anyway I expect. Thanks Kieran; I missed that entirely.
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 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 Daniel, On Tue, Jan 19, 2021 at 08:43:43AM +0000, Daniel Scally wrote: > On 19/01/2021 06:19, Laurent Pinchart wrote: > > On Mon, Jan 18, 2021 at 08:46:34PM +0000, Daniel Scally wrote: > >> Hi Laurent, thanks for the comments - really appreciate the detail. > >> > >> Some specific responses below but assume a general "will do" to > >> everything you mentioned otherwise... > >> > >> On 18/01/2021 09:15, Laurent Pinchart wrote: > >>>> + PMIC) and one designed for Chrome OS. > >>> How about expanding this a bit to explain what the INT3472 stands for ? > >>> > >>> The INT3472 is an Intel camera power controller, a logical device > >>> found on some Skylake-based systems that can map to different > >>> hardware devices depending on the platform. On machines > >>> designed for Chrome OS, it maps to a TPS68470 camera PMIC. On > >>> machines designed for Windows, it maps to either a TP68470 > >>> camera PMIC, a uP6641Q sensor PMIC, or a set of discrete GPIOs > >>> and power gates. > >> Yeah sure ok > >> > >>>> This driver handles all three > >>>> + situations by discovering information it needs to discern them at > >>>> + runtime. > >>>> + > >>>> + If your device was designed for Chrome OS, this driver will provide > >>>> + an ACPI operation region, which must be available before any of the > >>>> + devices using this are probed. For this reason, you should select Y > >>>> + if your device was designed for ChromeOS. This option also configures > >>>> + the designware-i2c driver to be built-in, for the same reason. > >>> Is the last sentence a leftover ? > >> Oops - it is, but it was supposed to remind me to double check that that > >> was still necessary. I'll take a look, thanks. > >> > >>>> + > >>>> +#include "intel_skl_int3472_common.h" > >>>> + > >>>> +int skl_int3472_get_cldb_buffer(struct acpi_device *adev, > >>>> + struct int3472_cldb *cldb) > >>>> +{ > >>>> + struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL }; > >>>> + acpi_handle handle = adev->handle; > >>>> + union acpi_object *obj; > >>>> + acpi_status status; > >>>> + int ret = 0; > >>>> + > >>>> + status = acpi_evaluate_object(handle, "CLDB", NULL, &buffer); > >>>> + if (ACPI_FAILURE(status)) > >>>> + return -ENODEV; > >>>> + > >>>> + obj = buffer.pointer; > >>>> + if (!obj) { > >>>> + dev_err(&adev->dev, "ACPI device has no CLDB object\n"); > >>> Is this the code path that is taken on Chrome OS ? If so an error > >>> message isn't appropriate. I'd drop this message, and instead add an > >>> error message in the discrete PMIC code. > >> Ah yes of course, thanks, I'll move the error message. > >> > >>>> + > >>>> + unsigned int n_gpios; /* how many GPIOs have we seen */ > >>>> + > >>>> + struct int3472_gpio_regulator regulator; > >>>> + struct int3472_gpio_clock clock; > >>> You don't necessarily need to define separate structures for this, you > >>> could also write > >>> > >>> struct { > >>> char regulator_name[GPIO_REGULATOR_NAME_LENGTH]; > >>> char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH]; > >>> struct gpio_desc *gpio; > >>> struct regulator_dev *rdev; > >>> struct regulator_desc rdesc; > >>> } regulator; > >>> > >>> struct { > >>> struct clk *clk; > >>> struct clk_hw clk_hw; > >>> struct clk_lookup *cl; > >>> struct gpio_desc *gpio; > >>> } clock; > >>> > >>> It's entirely up to you. > >> Ooh yeah I like that more, thanks very much. > >> > >>>> +/* 79234640-9e10-4fea-a5c1-b5aa8b19756f */ > >>>> +static const guid_t int3472_gpio_guid = > >>>> + GUID_INIT(0x79234640, 0x9e10, 0x4fea, > >>>> + 0xa5, 0xc1, 0xb5, 0xaa, 0x8b, 0x19, 0x75, 0x6f); > >>>> + > >>>> +/* 822ace8f-2814-4174-a56b-5f029fe079ee */ > >>>> +static const guid_t cio2_sensor_module_guid = > >>>> + GUID_INIT(0x822ace8f, 0x2814, 0x4174, > >>>> + 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); > >>> A comment that explains what those DSM functions do would be useful for > >>> reference. It has taken lots of time to figure it out, let's spare the > >>> pain to the next person who tries to understand this :-) > >> Hah - good point, well made. I'll explain what they're for then. > >> > >>>> +static int skl_int3472_clk_enable(struct clk_hw *hw) > >>>> +{ > >>>> + struct int3472_gpio_clock *clk = to_int3472_clk(hw); > >>>> + > >>>> + gpiod_set_value(clk->gpio, 1); > >>> The clock enable() and disable() methods are not supposed to sleep, > >>> while setting a GPIO value may sleep in the general case. Should this be > >>> moved to skl_int3472_clk_prepare() ? Same for skl_int3472_clk_disable() > >>> and skl_int3472_clk_unprepare(). > >> I was under the assumption the difference between gpiod_set_value() and > >> gpiod_set_value_cansleep() was that gpiod_set_value() _can't_ sleep, but > >> actually reading the function's comments it seems it will just complain > >> if it turns out it can sleep: > >> > >> * This function can be called from contexts where we cannot sleep, and will > >> * complain if the GPIO chip functions potentially sleep. It doesn't > >> complain, on either of my devices, but I guess that can't be guaranteed > >> for _every_ device, so these calls probably are safer in (un)prepare() yes. > > If we could guarantee that the GPIOs are connected to the SoC, we could > > keep using the code above, as there should be no need to sleep. The > > question is whether this can be guaranteed or not. It's true that I > > would be surprised if the GPIOs were connected, for instance, to an I2C > > GPIO expander.. > > Is that the deciding factor? I'd say that's unlikely, but what do I > know? Then again, is there actually any downside to calling > gpiod_set_value() in the prepare() function instead? If not, may as well > be safe. The downside is that prepare() is meant to be called earlier than enable() when the consumer needs to call enable() in a context that can't sleep. This can sometimes cause the clock to be enabled for longer than necessary. In this case I don't think it's an issue, sensor drivers will use clk_prepare_enable() anyway. > >>>> + } > >>>> + > >>>> + i++; > >>>> + } > >>>> + } > >>>> + > >>>> + if (!func) > >>>> + return 0; > >>> I initially thought this wasn't right, as if no entry was found in the > >>> mapping table, func would still have its non-NULL value as passed to > >>> this function. I then realized that you're checking if the match that > >>> was found is NULL. A comment to explain this would be useful. > >> Yep ok - I actually had one and decided it was superfluous and removed > >> it - my bad. > >> > >>>> + > >>>> + status = acpi_get_handle(NULL, path, &handle); > >>>> + if (ACPI_FAILURE(status)) > >>>> + return -EINVAL; > >>>> + > >>>> + ret = acpi_bus_get_device(handle, &adev); > >>>> + if (ret) > >>>> + return -ENODEV; > >>>> + > >>>> + table_entry = (struct gpiod_lookup)GPIO_LOOKUP_IDX(acpi_dev_name(adev), > >>>> + ares->data.gpio.pin_table[0], > >>>> + func, 0, polarity); > >>> I wonder if > >>> > >>> table_entry.key = acpi_dev_name(adev); > >>> table_entry.chip_hwnum = ares->data.gpio.pin_table[0]; > >>> table_entry.con_id = func; > >>> table_entry.idx = 0; > >>> table_entry.flags = polarity; > >>> > >>> (with struct gpiod_lookup table_entry = { }; above) would be more > >>> readable. Up to you. > >>> > >>>> + > >>>> + memcpy(&int3472->gpios.table[int3472->n_sensor_gpios], &table_entry, > >>>> + sizeof(table_entry)); > >>> Ah, or maybe > >>> > >>> struct gpio_lookup *table_entry; > >>> > >>> table_entry = &int3472->gpios.table[int3472->n_sensor_gpios]; > >>> table_entry->key = acpi_dev_name(adev); > >>> table_entry->chip_hwnum = ares->data.gpio.pin_table[0]; > >>> table_entry->con_id = func; > >>> table_entry->idx = 0; > >>> table_entry->flags = polarity; > >>> > >>> (no need to memset() to 0 first as the whole structure has been > >>> allocated with kzalloc()). > >> Yeah you're right, this looks much nicer - thanks. > >> > >>>> + int ret = 0; > >>>> + > >>>> + init.name = kasprintf(GFP_KERNEL, "%s-clk", > >>>> + acpi_dev_name(int3472->adev)); > >>> You need to check for NULL and return -ENOMEM. > >> Oops, of course, thanks > >> > >>>> + goto err_unregister_clk; > >>> If this fails, you will end up calling clk_unregister() and > >>> clkdev_drop() in skl_int3472_discrete_remove(). You should replace the > >>> check in the remove function with > >>> > >>> if (!int3472->clock.cl) { > >> You're right, good spot, thank you. > >> > >>>> + dev_err(&int3472->pdev->dev, "No sensor module config\n"); > >>>> + return PTR_ERR(sensor_config); > >>>> + } > >>> Would it make sense to call this in skl_int3472_discrete_probe() or > >>> skl_int3472_parse_crs() and cache the config pointer ? > >> Yes, probably actually, and then the GPIO mapping function can just > >> check for its presence. > >> > >>>> + init_data.constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS; > >>>> + init_data.num_consumer_supplies = 1; > >>>> + init_data.consumer_supplies = &sensor_config->supply_map; > >>>> + > >>>> + snprintf(int3472->regulator.regulator_name, > >>>> + GPIO_REGULATOR_NAME_LENGTH, "int3472-discrete-regulator"); > >>> s/GPIO_REGULATOR_NAME_LENGTH/sizeof(int3472->regulator.regulator_name)/ > >>> > >>> Do regulator names need to be unique ? If so you may have a problem if a > >>> platform has two discrete INT3472. > >> Unlike clocks, the regulator framework doesn't shout at you when you do > >> this, but I agree it's suboptimal at the very least, I'll set it to > >> ..."%s-regulator", acpi_dev_name(int3472->adev)... as with the clock. > >> > >>>> + case INT3472_GPIO_TYPE_PRIVACY_LED: > >>>> + ret = skl_int3472_map_gpio_to_sensor(int3472, ares, > >>>> + "indicator-led", > >>>> + GPIO_ACTIVE_HIGH); > >>> Mapping the indicator LED to the sensor isn't great, as all sensor > >>> drivers would then need to handle it. Could it be handled in the > >>> regulator instead, so that it would be turned on automatically when the > >>> sensor is powered up ? Another option, more complicated, would be to > >>> handle it in the CIO2 driver (but I'm not sure how we would map it). > >> Not with the regulator, because it turns out only the 0x0b pin is one of > >> those and those appear on very few devices in scope, so it wouldn't be > >> called on a Surface Book 2 for example. Perhaps as part of clock > >> prepare/enable? I don't much like the idea of it running in the CIO2 > >> driver to be honest, feels a bit out of place. > > The clock is another option, but could there be platforms where the > > clock GPIO isn't present ? > > I haven't ever seen a DSDT that didn't include a 0x0c pin to enable the > clock, though that doesn't necessarily mean they're always there. Plenty > of driver datasheets say they're happy for the external clock to be free > running, so it could just be always active I suppose. Maybe we can handle this later if such a platform is found. You should then print a warning message if no clock is present. > > Another option would be to let userspace handle that GPIO, but we then > > need to convey it to userspace. > > Can you point me to an example of that to look at perhaps? I don't think there's any :-) We'd have to design the mechanism. > >>>> + > >>>> + if (int3472->gpios_mapped) > >>>> + gpiod_remove_lookup_table(&int3472->gpios); > >>> You could avoid the need for the gpios_mapped field by checking for > >>> > >>> if (int3472->gpios.list.next) > >>> > >>> Up to you. > >> Thank you! I was scratching my head trying to figure out a better way of > >> doing that.
Hi Andy, On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > On 18/01/2021 21:19, Daniel Scally wrote: > > > > I'm more and more confident that this will work, but it has some > > knock-on effects: > > > > The both clk and regulator gpio driver expects to be able to fetch the > > GPIO using devm_gpiod_get(&pdev->dev, "enable", ...). That won't work of > > course, so we need to add another GPIO lookup table so those drivers can > > see the GPIOs. For that, we need to know what dev_name(&pdev->dev) will > > be so we can set the .dev_id member of a gpiod_lookup_table to that > > value, but that isn't set until _after_ the pdev is registered (because > > it has to figure out the id, we can't manually set the IDs because there > > could be more than one instance of int3472-discrete bound to multiple > > PMIC devices, and we don't know which id the current one should have). > > Finally, we can't wait until the device is registered because it > > immediately probes, can't find the GPIO and then fails probe. > > > > It's similar problem that causes us to need the i2c-acpi name format > > macros, but complicated by the dynamic ID part of dev_name(&pdev->dev) > > > > Solving it is a bit of a sticky one; perhaps something like moving the > > dev_set_name() part of platform_device_add() [1] to its own function, > > that's called in both platform_device_alloc() and > > platform_device_register(). That way it would be available before the > > device itself was registered, meaning we could create the lookup table > > before it probes the driver. > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > methods to be able to use it as a library. That seems really overkill given the very simple implementation of the clock provided here. > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > the led-indicator GPIO here without some fairly major rework) > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > drivers: drivers/platform/x86/) How do you expose the link between the sensor and its indicator LED to userspace ? Isn't it better to handle it in the kernel to avoid rogue userspace turning the camera on without notifying the user ?
On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > >>>>> + .driver = { > > >>>>> + .name = "int3472-tps68470", > > >>>>> + .acpi_match_table = int3472_device_id, > > >>>>> + }, > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > >>>>> +}; > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > >> Sorry, I'm a bit confused by this. The i2c device is already > > >> present...we just want the driver to bind to them, so what role do those > > >> functions have there? > > > What I meant is something like > > > > > > *_i2c.c > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > involved (and it sounds like it should be mfd/tps one, in which you > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > suggested in the first place) > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > think, but there is one complication; the ACPI subsystem already creates > > a client for that i2c adapter and address; i2c_new_client_device() > > includes a check to see whether that adapter / address combination has > > an i2c device already. So we would have to have the platform driver > > with ACPI ID first find the existing i2c_client and unregister it before > > registering the new one...the existing clients have a name matching the > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > i2c_device_id of course. > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > {"BSG1160", }, > {"BSG2150", }, > {"INT33FE", }, > {"INT3515", }, > {} > }; > > So, we quirklist it here and instantiate manually from platform driver (new > coming one). This is documented as used for devices that have multiple I2cSerialBus resources. That's not the case for the INT3472 as far as I can tell. I don't think we should abuse this mechanism. Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it has to register an OpRegion for ACPI-based Chrome OS devices. On other platforms (including DT platforms), it should only register regulators, clocks and GPIOs. Given the differences between those platforms, I don't think a TPS68470 driver that would fake being unaware of being probed through ACPI would be a good idea. We can always refactor the code later when we'll have a non-ACPI based platform using the TPS68470, without such a platform there's no way we can test the I2C driver without ACPI anyway. > ... > > > > You need to modify clk-gpio.c to export > > > > > > clk_hw_register_gpio_gate() > > > clk_hw_register_gpio_mux() > > > > > > (perhaps it will require to add *_unregister() counterparts) and call it from > > > your code. > > > > > > See, for example, how clk_hw_unregister_fixed_rate() is being used. Another > > Here I meant of course clk_hw_register_fixed_rate(). > > > > case is to add a helper directly into clk-gpio and call it instead of > > > clk_hw_*() one, see how clk_register_fractional_divider() is implemented and > > > used. > > > > I'll take a look, thanks
On Tue, Jan 19, 2021 at 11:35:42AM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 08:21:23AM +0200, Laurent Pinchart wrote: > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > On 18/01/2021 21:19, Daniel Scally wrote: > > ... > > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > the led-indicator GPIO here without some fairly major rework) > > > > Given the additional complexity I don't think it's worth it, your > > implementation is fine and code duplication with clk-gpio is minimal. > > Making clk-gpio.c available as a library is a win in long term and reduces a > possible duplication by others in the future. I bet we even might find already > clk-gpio parts in some drivers. How about you submit a patch on top then ? :-) Let's avoid yak shaving. -- Regards, Laurent Pinchart
On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > On 18/01/2021 21:19, Daniel Scally wrote: ... > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > > methods to be able to use it as a library. > > That seems really overkill given the very simple implementation of the > clock provided here. Less code in the end is called an overkill? Hmm... I think since we in Linux it's better to utilize what it provides. Do you want me to prepare a patch to show that there is no overkill at all? ... > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > the led-indicator GPIO here without some fairly major rework) > > > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > > drivers: drivers/platform/x86/) > > How do you expose the link between the sensor and its indicator LED to > userspace ? Isn't it better to handle it in the kernel to avoid rogue > userspace turning the camera on without notifying the user ? I didn't get this. It's completely a LED handling driver business. We may expose it to user space or not, but it's orthogonal to the usage of LED class IIUC. Am I mistaken here?
On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > > >>>>> + .driver = { > > > >>>>> + .name = "int3472-tps68470", > > > >>>>> + .acpi_match_table = int3472_device_id, > > > >>>>> + }, > > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > > >>>>> +}; > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > > >> Sorry, I'm a bit confused by this. The i2c device is already > > > >> present...we just want the driver to bind to them, so what role do those > > > >> functions have there? > > > > What I meant is something like > > > > > > > > *_i2c.c > > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > > involved (and it sounds like it should be mfd/tps one, in which you > > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > > suggested in the first place) > > > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > > think, but there is one complication; the ACPI subsystem already creates > > > a client for that i2c adapter and address; i2c_new_client_device() > > > includes a check to see whether that adapter / address combination has > > > an i2c device already. So we would have to have the platform driver > > > with ACPI ID first find the existing i2c_client and unregister it before > > > registering the new one...the existing clients have a name matching the > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > > i2c_device_id of course. > > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > > {"BSG1160", }, > > {"BSG2150", }, > > {"INT33FE", }, > > {"INT3515", }, > > {} > > }; > > > > So, we quirklist it here and instantiate manually from platform driver (new > > coming one). > > This is documented as used for devices that have multiple I2cSerialBus > resources. That's not the case for the INT3472 as far as I can tell. I > don't think we should abuse this mechanism. This is quite a similar case to that one. Let's avoid yak shaving, right? > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it > has to register an OpRegion for ACPI-based Chrome OS devices. On other > platforms (including DT platforms), it should only register regulators, > clocks and GPIOs. Given the differences between those platforms, I don't > think a TPS68470 driver that would fake being unaware of being probed > through ACPI would be a good idea. We can always refactor the code later > when we'll have a non-ACPI based platform using the TPS68470, without > such a platform there's no way we can test the I2C driver without ACPI > anyway. Are you agree that MFD approach should stay? How then we can manage to have an MFD driver cohabit with our new driver? I proposed a clean solution which will handle all possible cases via quirk driver. Having two drivers enumerated by different scenarios is a call for troubles (we have already with one of that sensors). And there is no "faking" anything, it's rather gating it depending on the platform. -- With Best Regards, Andy Shevchenko
Hi Andy, On Tue, Jan 19, 2021 at 07:43:15PM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: > > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > > On 18/01/2021 21:19, Daniel Scally wrote: > > ... > > > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > > > methods to be able to use it as a library. > > > > That seems really overkill given the very simple implementation of the > > clock provided here. > > Less code in the end is called an overkill? Hmm... > I think since we in Linux it's better to utilize what it provides. Do you want > me to prepare a patch to show that there is no overkill at all? The amount of code we would save it very small. It's not necessarily a bad idea, but I think such an improvement could be made on top, it shouldn't block this series. > ... > > > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > > the led-indicator GPIO here without some fairly major rework) > > > > > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > > > drivers: drivers/platform/x86/) > > > > How do you expose the link between the sensor and its indicator LED to > > userspace ? Isn't it better to handle it in the kernel to avoid rogue > > userspace turning the camera on without notifying the user ? > > I didn't get this. It's completely a LED handling driver business. We may > expose it to user space or not, but it's orthogonal to the usage of LED class > IIUC. Am I mistaken here? If it stays internal to the kernel and is solely controlled from the int3472 driver, there's no need to involve the LED class. If we want to expose the privacy LED to userspace then the LED framework is the way to go, but we will also need to find a way to expose the link between the camera sensor and the LED to userspace. If there are two privacy LEDs, one for the front sensor and one for the back sensor, userspace will need to know which is which.
Hi Andy, On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: > On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: > > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > > > >>>>> + .driver = { > > > > >>>>> + .name = "int3472-tps68470", > > > > >>>>> + .acpi_match_table = int3472_device_id, > > > > >>>>> + }, > > > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > > > >>>>> +}; > > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > > > >> Sorry, I'm a bit confused by this. The i2c device is already > > > > >> present...we just want the driver to bind to them, so what role do those > > > > >> functions have there? > > > > > What I meant is something like > > > > > > > > > > *_i2c.c > > > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > > > involved (and it sounds like it should be mfd/tps one, in which you > > > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > > > suggested in the first place) > > > > > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > > > think, but there is one complication; the ACPI subsystem already creates > > > > a client for that i2c adapter and address; i2c_new_client_device() > > > > includes a check to see whether that adapter / address combination has > > > > an i2c device already. So we would have to have the platform driver > > > > with ACPI ID first find the existing i2c_client and unregister it before > > > > registering the new one...the existing clients have a name matching the > > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > > > i2c_device_id of course. > > > > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > > > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > > > {"BSG1160", }, > > > {"BSG2150", }, > > > {"INT33FE", }, > > > {"INT3515", }, > > > {} > > > }; > > > > > > So, we quirklist it here and instantiate manually from platform driver (new > > > coming one). > > > > This is documented as used for devices that have multiple I2cSerialBus > > resources. That's not the case for the INT3472 as far as I can tell. I > > don't think we should abuse this mechanism. > > This is quite a similar case to that one. Let's avoid yak shaving, right? Exactly my point, that's why I think this patch is good overall, I don't think it requires a complete rewrite. > > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it > > has to register an OpRegion for ACPI-based Chrome OS devices. On other > > platforms (including DT platforms), it should only register regulators, > > clocks and GPIOs. Given the differences between those platforms, I don't > > think a TPS68470 driver that would fake being unaware of being probed > > through ACPI would be a good idea. We can always refactor the code later > > when we'll have a non-ACPI based platform using the TPS68470, without > > such a platform there's no way we can test the I2C driver without ACPI > > anyway. > > Are you agree that MFD approach should stay? How then we can manage to have an > MFD driver cohabit with our new driver? I proposed a clean solution which will > handle all possible cases via quirk driver. Having two drivers enumerated by > different scenarios is a call for troubles (we have already with one of that > sensors). I think we should solve this problem when it will arise. Solving problems with complex architectures without a platform to test the code on is a pretty sure way to get the architecture design wrong. Let's get this merged, it's an improvement compared to the current situation, and then let's improve it further on top when we'll need to support more use cases. > And there is no "faking" anything, it's rather gating it depending on the > platform.
On Wed, Jan 20, 2021 at 06:18:53AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 07:43:15PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 06:36:31PM +0200, Laurent Pinchart wrote: > > > On Tue, Jan 19, 2021 at 11:33:58AM +0200, Andy Shevchenko wrote: > > > > On Tue, Jan 19, 2021 at 12:11:40AM +0000, Daniel Scally wrote: > > > > > On 18/01/2021 21:19, Daniel Scally wrote: ... > > > > See my previous reply. TL;DR: you have to modify clk-gpio.c to export couple of > > > > methods to be able to use it as a library. > > > > > > That seems really overkill given the very simple implementation of the > > > clock provided here. > > > > Less code in the end is called an overkill? Hmm... > > I think since we in Linux it's better to utilize what it provides. Do you want > > me to prepare a patch to show that there is no overkill at all? > > The amount of code we would save it very small. It's not necessarily a > bad idea, but I think such an improvement could be made on top, it > shouldn't block this series. Okay, let's wait what Dan will say on this. I can probably help to achieve this improvement sooner than later. ... > > > > > (also, Laurent, if we did it this way we wouldn't be able to also handle > > > > > the led-indicator GPIO here without some fairly major rework) > > > > > > > > LED indicators are done as LED class devices (see plenty of examples in PDx86 > > > > drivers: drivers/platform/x86/) > > > > > > How do you expose the link between the sensor and its indicator LED to > > > userspace ? Isn't it better to handle it in the kernel to avoid rogue > > > userspace turning the camera on without notifying the user ? > > > > I didn't get this. It's completely a LED handling driver business. We may > > expose it to user space or not, but it's orthogonal to the usage of LED class > > IIUC. Am I mistaken here? > > If it stays internal to the kernel and is solely controlled from the > int3472 driver, there's no need to involve the LED class. If we want to > expose the privacy LED to userspace then the LED framework is the way to > go, but we will also need to find a way to expose the link between the > camera sensor and the LED to userspace. If there are two privacy LEDs, > one for the front sensor and one for the back sensor, userspace will > need to know which is which. I see. For now we probably can keep GPIO LED implementation internally. -- With Best Regards, Andy Shevchenko
On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote: > On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: > > On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: > > > On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: > > > > On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: > > > > > On 19/01/2021 09:24, Andy Shevchenko wrote: > > > > > >>>>> +static struct i2c_driver int3472_tps68470 = { > > > > > >>>>> + .driver = { > > > > > >>>>> + .name = "int3472-tps68470", > > > > > >>>>> + .acpi_match_table = int3472_device_id, > > > > > >>>>> + }, > > > > > >>>>> + .probe_new = skl_int3472_tps68470_probe, > > > > > >>>>> +}; > > > > > >>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can > > > > > >>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it > > > > > >>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... > > > > > >> Sorry, I'm a bit confused by this. The i2c device is already > > > > > >> present...we just want the driver to bind to them, so what role do those > > > > > >> functions have there? > > > > > > What I meant is something like > > > > > > > > > > > > *_i2c.c > > > > > > real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI > > > > > > involved (and it sounds like it should be mfd/tps one, in which you > > > > > > just cut out ACPI IDs and convert to pure I²C one, that what I had > > > > > > suggested in the first place) > > > > > > > > > > Ahh; sorry - i misunderstood what you meant there. I understand now I > > > > > think, but there is one complication; the ACPI subsystem already creates > > > > > a client for that i2c adapter and address; i2c_new_client_device() > > > > > includes a check to see whether that adapter / address combination has > > > > > an i2c device already. So we would have to have the platform driver > > > > > with ACPI ID first find the existing i2c_client and unregister it before > > > > > registering the new one...the existing clients have a name matching the > > > > > ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an > > > > > i2c_device_id of course. > > > > > > > > See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 > > > > > > > > static const struct acpi_device_id i2c_multi_instantiate_ids[] = { > > > > {"BSG1160", }, > > > > {"BSG2150", }, > > > > {"INT33FE", }, > > > > {"INT3515", }, > > > > {} > > > > }; > > > > > > > > So, we quirklist it here and instantiate manually from platform driver (new > > > > coming one). > > > > > > This is documented as used for devices that have multiple I2cSerialBus > > > resources. That's not the case for the INT3472 as far as I can tell. I > > > don't think we should abuse this mechanism. > > > > This is quite a similar case to that one. Let's avoid yak shaving, right? > > Exactly my point, that's why I think this patch is good overall, I don't > think it requires a complete rewrite. The approach in the series is to reinvent the MFD driver which I against of. I don;t think we need to kill it there and reborn in a new form and dragging code from there to here to there. On top of that the approach with a quirk driver in the middle seems to me cleaner than using different paths how the two drivers are being initialized. In the proposed approach there will be one making decision point and easy to understand what's going on. The bad example of two making decision points is acpi_lpss.c vs. individual drivers (however in that case it have different ID's, i.e. ACPI vs. PCI), > > > Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it > > > has to register an OpRegion for ACPI-based Chrome OS devices. On other > > > platforms (including DT platforms), it should only register regulators, > > > clocks and GPIOs. Given the differences between those platforms, I don't > > > think a TPS68470 driver that would fake being unaware of being probed > > > through ACPI would be a good idea. We can always refactor the code later > > > when we'll have a non-ACPI based platform using the TPS68470, without > > > such a platform there's no way we can test the I2C driver without ACPI > > > anyway. > > > > Are you agree that MFD approach should stay? How then we can manage to have an > > MFD driver cohabit with our new driver? I proposed a clean solution which will > > handle all possible cases via quirk driver. Having two drivers enumerated by > > different scenarios is a call for troubles (we have already with one of that > > sensors). > > I think we should solve this problem when it will arise. Solving > problems with complex architectures without a platform to test the code > on is a pretty sure way to get the architecture design wrong. Let's get > this merged, it's an improvement compared to the current situation, and > then let's improve it further on top when we'll need to support more use > cases. But this is problem already here right now. The submitted code is to support a new platform that needs a quirk and treats INT3472 differently. The usual way is to refactor the existing solution to make them both to have a best compromise. > > And there is no "faking" anything, it's rather gating it depending on the > > platform.
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?
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 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? Or just export helpers to call each of the callbacks (one to fetch the next dependent device, one to decrement the unmet dependencies counter) 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.
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.
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 >> 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. Yeah that's what I mean by helper; I'll do that then; thanks
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.
Hello Andy, Laurent On 21/01/2021 00:18, Daniel Scally wrote: > On 20/01/2021 12:57, Andy Shevchenko wrote: >> On Wed, Jan 20, 2021 at 06:21:41AM +0200, Laurent Pinchart wrote: >>> On Tue, Jan 19, 2021 at 07:51:14PM +0200, Andy Shevchenko wrote: >>>> On Tue, Jan 19, 2021 at 06:48:15PM +0200, Laurent Pinchart wrote: >>>>> On Tue, Jan 19, 2021 at 01:08:37PM +0200, Andy Shevchenko wrote: >>>>>> On Tue, Jan 19, 2021 at 10:40:42AM +0000, Daniel Scally wrote: >>>>>>> On 19/01/2021 09:24, Andy Shevchenko wrote: >>>>>>>>>>>> +static struct i2c_driver int3472_tps68470 = { >>>>>>>>>>>> + .driver = { >>>>>>>>>>>> + .name = "int3472-tps68470", >>>>>>>>>>>> + .acpi_match_table = int3472_device_id, >>>>>>>>>>>> + }, >>>>>>>>>>>> + .probe_new = skl_int3472_tps68470_probe, >>>>>>>>>>>> +}; >>>>>>>>>> I'm not sure we want to have like this. If I'm not mistaken the I²C driver can >>>>>>>>>> be separated without ACPI IDs (just having I²C IDs) and you may instantiate it >>>>>>>>>> via i2c_new_client_device() or i2c_acpi_new_device() whichever suits better... >>>>>>>>> Sorry, I'm a bit confused by this. The i2c device is already >>>>>>>>> present...we just want the driver to bind to them, so what role do those >>>>>>>>> functions have there? >>>>>>>> What I meant is something like >>>>>>>> >>>>>>>> *_i2c.c >>>>>>>> real I²C driver for the TPS chip, but solely with I²C ID table, no ACPI >>>>>>>> involved (and it sounds like it should be mfd/tps one, in which you >>>>>>>> just cut out ACPI IDs and convert to pure I²C one, that what I had >>>>>>>> suggested in the first place) >>>>>>> Ahh; sorry - i misunderstood what you meant there. I understand now I >>>>>>> think, but there is one complication; the ACPI subsystem already creates >>>>>>> a client for that i2c adapter and address; i2c_new_client_device() >>>>>>> includes a check to see whether that adapter / address combination has >>>>>>> an i2c device already. So we would have to have the platform driver >>>>>>> with ACPI ID first find the existing i2c_client and unregister it before >>>>>>> registering the new one...the existing clients have a name matching the >>>>>>> ACPI device instance name (e.g i2c-INT3472:00) which we can't use as an >>>>>>> i2c_device_id of course. >>>>>> See how INT33FE is being handled. Hint: drivers/acpi/scan.c:~1600 >>>>>> >>>>>> static const struct acpi_device_id i2c_multi_instantiate_ids[] = { >>>>>> {"BSG1160", }, >>>>>> {"BSG2150", }, >>>>>> {"INT33FE", }, >>>>>> {"INT3515", }, >>>>>> {} >>>>>> }; >>>>>> >>>>>> So, we quirklist it here and instantiate manually from platform driver (new >>>>>> coming one). >>>>> This is documented as used for devices that have multiple I2cSerialBus >>>>> resources. That's not the case for the INT3472 as far as I can tell. I >>>>> don't think we should abuse this mechanism. >>>> This is quite a similar case to that one. Let's avoid yak shaving, right? >>> Exactly my point, that's why I think this patch is good overall, I don't >>> think it requires a complete rewrite. >> The approach in the series is to reinvent the MFD driver which I against of. >> I don;t think we need to kill it there and reborn in a new form and dragging >> code from there to here to there. >> >> On top of that the approach with a quirk driver in the middle seems to me >> cleaner than using different paths how the two drivers are being initialized. >> In the proposed approach there will be one making decision point and easy to >> understand what's going on. >> >> The bad example of two making decision points is acpi_lpss.c vs. individual >> drivers (however in that case it have different ID's, i.e. ACPI vs. PCI), > > Right; so if I understand correctly, the proposal is: > > 1. Add INT3472 to the i2c_multi_instantiate_ids, which blocks it getting > created as an i2c device > 2. instead of intel-skl-int3472 registering an i2c and a platform > driver, just register a platform driver that binds to the INT3472 > acpi_device_id. We can check hardware type like in > intel_cht_int33fe_common.c and call either discrete probe that does what > the discrete driver is doing now, or else call tps68470 which is just a > stub driver registering an i2c device like intel_cht_int33fe_microb.c > 3. Change the existing tps68470 mfd driver to match to that created i2c > device instead of ACPI match, and move the code from > intel_skl_int3472_tps68470.c to that driver instead > > I think I finally got what you meant there, Andy, but correct me if I'm > wrong please. > > I'm not sure that one's better than the other, to be honest. Either the > multi-function device functionality lives in the conventional place, or > else _all_ of the int3472 handling code lives together in one module. Can we come to a consensus on this? I would rather be in agreement than leave it hanging...I do see the argument that it's better not to rebirth the MFD driver away from that subsystem, but at the moment I lean towards the view that having all the code handling this particular _HID in one place is probably preferable, if only to make it easier for anyone coming in the future to understand what's happening. That said; I'm not particularly precious about it, I'd just like to agree an approach so I can move forward with the next version >>>>> Don't forget that the TPS68470 I2C driver needs to be ACPI-aware, as it >>>>> has to register an OpRegion for ACPI-based Chrome OS devices. On other >>>>> platforms (including DT platforms), it should only register regulators, >>>>> clocks and GPIOs. Given the differences between those platforms, I don't >>>>> think a TPS68470 driver that would fake being unaware of being probed >>>>> through ACPI would be a good idea. We can always refactor the code later >>>>> when we'll have a non-ACPI based platform using the TPS68470, without >>>>> such a platform there's no way we can test the I2C driver without ACPI >>>>> anyway. >>>> Are you agree that MFD approach should stay? How then we can manage to have an >>>> MFD driver cohabit with our new driver? I proposed a clean solution which will >>>> handle all possible cases via quirk driver. Having two drivers enumerated by >>>> different scenarios is a call for troubles (we have already with one of that >>>> sensors). > What kind of troubles do you anticipate here? > >>> I think we should solve this problem when it will arise. Solving >>> problems with complex architectures without a platform to test the code >>> on is a pretty sure way to get the architecture design wrong. Let's get >>> this merged, it's an improvement compared to the current situation, and >>> then let's improve it further on top when we'll need to support more use >>> cases. >> But this is problem already here right now. The submitted code is to support >> a new platform that needs a quirk and treats INT3472 differently. The usual >> way is to refactor the existing solution to make them both to have a best >> compromise. >> >>>> And there is no "faking" anything, it's rather gating it depending on the >>>> platform.
On Sun, Feb 7, 2021 at 1:00 PM Daniel Scally <djrscally@gmail.com> wrote: > On 21/01/2021 00:18, Daniel Scally wrote: > > On 20/01/2021 12:57, Andy Shevchenko wrote: ... > > I'm not sure that one's better than the other, to be honest. Either the > > multi-function device functionality lives in the conventional place, or > > else _all_ of the int3472 handling code lives together in one module. > Can we come to a consensus on this? I would rather be in agreement than > leave it hanging...I do see the argument that it's better not to rebirth > the MFD driver away from that subsystem, but at the moment I lean > towards the view that having all the code handling this particular _HID > in one place is probably preferable, if only to make it easier for > anyone coming in the future to understand what's happening. That said; > I'm not particularly precious about it, I'd just like to agree an > approach so I can move forward with the next version So, my priorities of rejection (first is higher) - open-coding MFD subsystem (that said, if you shuffle the code, at least leave it as an MFD driver) - moving out from MFD (although I understand intentions) Summarize, go ahead with your view (leaving it as an MFD driver) and look forward to what maintainer(s) will say.