| Message ID | 20240911072751.365361-1-wenst@chromium.org |
|---|---|
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
On Wed, Sep 11, 2024 at 03:27:41PM +0800, Chen-Yu Tsai wrote: > _regulator_get() contains a lot of common code doing checks prior to the > regulator lookup and housekeeping work after the lookup. Almost all the > code could be shared with a OF-specific variant of _regulator_get(). > > Split out the common parts so that they can be reused. The OF-specific > version of _regulator_get() will be added in a subsequent patch. > No functional changes were made. I think this patch makes sense on its own, but it of course up to Mark. FWIW, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On Wed, Sep 11, 2024 at 03:27:45PM +0800, Chen-Yu Tsai wrote: > Add helpers to do regulator management for the I2C OF component prober. > Components that the prober intends to probe likely require their > regulator supplies be enabled, and GPIOs be toggled to enable them or > bring them out of reset before they will respond to probe attempts. > GPIOs will be handled in the next patch. > > The assumption is that the same class of components to be probed are > always connected in the same fashion with the same regulator supply > and GPIO. The names may vary due to binding differences, but the > physical layout does not change. > > This set of helpers supports at most one regulator supply. The user > must specify the node from which the supply is retrieved. The supply > name and the amount of time to wait after the supply is enabled are > also given by the user. ... > +static int i2c_of_probe_simple_get_supply(struct device *dev, struct device_node *node, > + struct i2c_of_probe_simple_ctx *ctx) > +{ > + const char *supply_name; > + struct regulator *supply; > + > + /* > + * It's entirely possible for the component's device node to not have regulator > + * supplies. While it does not make sense from a hardware perspective, the > + * supplies could be always on or otherwise not modeled in the device tree, but > + * the device would still work. > + */ I would reformat as /* * It's entirely possible for the component's device node to not have the * regulator supplies. While it does not make sense from a hardware perspective, * the supplies could be always on or otherwise not modeled in the device tree, * but the device would still work. */ > + supply_name = ctx->opts->supply_name; > + if (!supply_name) > + return 0; > + > + supply = of_regulator_get_optional(dev, node, supply_name); > + if (IS_ERR(supply)) { > + return dev_err_probe(dev, PTR_ERR(supply), > + "Failed to get regulator supply \"%s\" from %pOF\n", > + supply_name, node); > + } > + > + ctx->supply = supply; > + > + return 0; > +} ... > +int i2c_of_probe_simple_get_res(struct device *dev, struct device_node *bus_node, void *data) > +{ > + struct i2c_of_probe_simple_ctx *ctx = data; > + struct device_node *node; > + const char *compat; > + int ret; > + > + dev_dbg(dev, "Requesting resources for components under I2C bus %pOF\n", bus_node); > + > + if (!ctx || !ctx->opts) > + return -EINVAL; > + > + compat = ctx->opts->res_node_compatible; > + if (!compat) > + return -EINVAL; > + node = of_get_compatible_child(bus_node, compat); __free(of_node_put) ? > + if (!node) > + return dev_err_probe(dev, -ENODEV, "No device compatible with \"%s\" found\n", > + compat); > + > + ret = i2c_of_probe_simple_get_supply(dev, node, ctx); > + if (ret) > + goto out_put_node; > + > + return 0; > + > +out_put_node: > + of_node_put(node); > + return ret; > +} ... > + * @post_power_on_delay_ms: Delay in ms after regulators are powered on. Passed to msleep(). No need to duplicate the units as it's obvious from the variable name and msleep(). * @post_power_on_delay_ms: Delay after regulators are powered on. Passed to msleep().
Hi Chen-Yu, On Wed, Sep 11, 2024 at 03:27:43PM GMT, Chen-Yu Tsai wrote: > Some lines in the Makefile have a space before tabs. Remove those. > > Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Closes: https://lore.kernel.org/all/ZsdE0PxKnGRjzChl@smile.fi.intel.com/ > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
On Wed, 11 Sep 2024 15:27:38 +0800, Chen-Yu Tsai wrote: > This is v7 of my "of: Introduce hardware prober driver" [1] series. > v7 mainly refactors the code into a series of helpers. The scope of > supported components is also reduced to those with at most one regulator > supply and one GPIO pin. Also the helpers expect these to be named and > so the "bulk get" API changes have been dropped. > > Also, a pull request to document the "fail-needs-probe" status has been > sent: https://github.com/devicetree-org/dt-schema/pull/141 > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next Thanks! [03/10] regulator: Split up _regulator_get() commit: 2a1de5678944147c2a41b6006127d2d0b618e83b All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Hi, On Wed, Sep 11, 2024 at 12:28 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > +static int i2c_of_probe_simple_enable_regulator(struct device *dev, struct i2c_of_probe_simple_ctx *ctx) > +{ > + int ret; > + > + if (!ctx->supply) > + return 0; > + > + dev_dbg(dev, "Enabling regulator supply \"%s\"\n", ctx->opts->supply_name); > + > + ret = regulator_enable(ctx->supply); > + if (ret) > + return ret; > + > + msleep(ctx->opts->post_power_on_delay_ms); Presumably you want an "if (ctx->opts->post_power_on_delay_ms)" before the call to msleep() since it doesn't check that for you. > +/** > + * i2c_of_probe_simple_enable - Enable resources for I2C OF prober simple helpers > + * @dev: Pointer to the &struct device of the caller, only used for dev_printk() messages > + * @data: Pointer to &struct i2c_of_probe_simple_ctx helper context. > + * > + * If a regulator supply was found, enable that regulator. > + * > + * Return: %0 on success or no-op, or a negative error number on failure. > + */ > +int i2c_of_probe_simple_enable(struct device *dev, void *data) > +{ > + struct i2c_of_probe_simple_ctx *ctx = data; > + int ret; > + > + ret = i2c_of_probe_simple_enable_regulator(dev, ctx); > + if (ret) > + return ret; > + > + return 0; Instead of the above, just: return i2c_of_probe_simple_enable_regulator(dev, ctx); I guess maybe you'd have to undo it in the next patch, but it does make this patch stand by itself better.. Although I'd also say that if it were me I might just get rid of the helpers and inline the stuff. The helpers don't _really_ add too much. 3 of the 4 callers are just simple wrappers of the helper and I don't think it would be terrible to inline the last one. I guess with the next patch when you add GPIOs it maybe makes more sense, but even then it feels like a stretch to me. Anyway, feel free to ignore if you want. > +/** > + * DOC: I2C OF component prober simple helpers > + * > + * Components such as trackpads are commonly connected to a devices baseboard > + * with a 6-pin ribbon cable. That gives at most one voltage supply and one > + * GPIO besides the I2C bus, interrupt pin, and common ground. Touchscreens, Maybe speculate here that the GPIO is often an enable or reset line? Otherwise you leave the reader wondering what this mysterious GPIO is for.
Hi, On Wed, Sep 11, 2024 at 12:29 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > @@ -8,6 +8,7 @@ obj-$(CONFIG_CHROMEOS_ACPI) += chromeos_acpi.o > obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o > obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN) += chromeos_privacy_screen.o > obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.o > +obj-$(CONFIG_CHROMEOS_OF_HW_PROBER) += chromeos_of_hw_prober.o "o" sorts before "p" so "of" should sort before "privacy"? I guess it's not exactly all sorted, but this small section is. Since it's arbitrary you could preserve the existing sorting. :-P > +static const struct hw_prober_entry hw_prober_platforms[] = { > + { .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = &chromeos_i2c_probe_dumb_touchscreen }, > + { .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = &chromeos_i2c_probe_dumb_trackpad }, The fact that the example is only using "dumb" probers doesn't make it quite as a compelling proof that the code will scale up. Any chance you could add something that requires a bit more oomph? ;-) > +static int chromeos_of_hw_prober_driver_init(void) > +{ > + size_t i; > + int ret; > + > + for (i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) > + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) > + break; > + if (i == ARRAY_SIZE(hw_prober_platforms)) > + return -ENODEV; > + > + ret = platform_driver_register(&chromeos_of_hw_prober_driver); > + if (ret) > + return ret; > + > + chromeos_of_hw_prober_pdev = > + platform_device_register_simple(DRV_NAME, PLATFORM_DEVID_NONE, NULL, 0); > + if (IS_ERR(chromeos_of_hw_prober_pdev)) > + goto err; FWIW if you didn't want to see your prober called over and over again if one of the devices deferred you could just register one device per type, right? Then each device would be able to defer separately. Dunno if it's worth it but figured I'd mention it... Also: as a high level comment, this all looks much nicer to me now that it's parameterized. :-)
On Sat, Sep 14, 2024 at 1:43 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Wed, Sep 11, 2024 at 12:29 AM Chen-Yu Tsai <wenst@chromium.org> wrote: > > > > @@ -8,6 +8,7 @@ obj-$(CONFIG_CHROMEOS_ACPI) += chromeos_acpi.o > > obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o > > obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN) += chromeos_privacy_screen.o > > obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.o > > +obj-$(CONFIG_CHROMEOS_OF_HW_PROBER) += chromeos_of_hw_prober.o > > "o" sorts before "p" so "of" should sort before "privacy"? > > I guess it's not exactly all sorted, but this small section is. Since > it's arbitrary you could preserve the existing sorting. :-P To me it seemed more like they are just sorted in the order they were added. > > +static const struct hw_prober_entry hw_prober_platforms[] = { > > + { .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = &chromeos_i2c_probe_dumb_touchscreen }, > > + { .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = &chromeos_i2c_probe_dumb_trackpad }, > > The fact that the example is only using "dumb" probers doesn't make it > quite as a compelling proof that the code will scale up. Any chance > you could add something that requires a bit more oomph? ;-) I only have a hacked up thing right now. This is the one I'm using to test things: http://git.kernel.org/wens/c/5c2c920429167a15b990a7cf8427705eec321134 About this one, it seems we can at least merge the device trees of each product into just one. The touchscreen or trackpad (or lack thereof) is probed by the kernel. This one I only started looking into: http://git.kernel.org/wens/c/42c21929aeb3c7ca7b0ce9cacb1d0ff915d3c783 About the second one, AFAIK the device tree descriptions are incomplete. Only one of the trackpad options has the regulator supply described. The regulator itself is marked as always on, so things currently work. Some work will need to be put in to research the schematics and test whether things do work correctly. > > +static int chromeos_of_hw_prober_driver_init(void) > > +{ > > + size_t i; > > + int ret; > > + > > + for (i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) > > + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) > > + break; > > + if (i == ARRAY_SIZE(hw_prober_platforms)) > > + return -ENODEV; > > + > > + ret = platform_driver_register(&chromeos_of_hw_prober_driver); > > + if (ret) > > + return ret; > > + > > + chromeos_of_hw_prober_pdev = > > + platform_device_register_simple(DRV_NAME, PLATFORM_DEVID_NONE, NULL, 0); > > + if (IS_ERR(chromeos_of_hw_prober_pdev)) > > + goto err; > > FWIW if you didn't want to see your prober called over and over again > if one of the devices deferred you could just register one device per > type, right? Then each device would be able to defer separately. Dunno > if it's worth it but figured I'd mention it... I think that adds some unnecessary complexity, and more lingering devices in the system. These platform devices are not removed. > Also: as a high level comment, this all looks much nicer to me now > that it's parameterized. :-) Thanks! ChenYu
On Mon, Sep 16, 2024 at 04:58:51PM +0200, Chen-Yu Tsai wrote: > On Sat, Sep 14, 2024 at 1:43 AM Doug Anderson <dianders@chromium.org> wrote: > > On Wed, Sep 11, 2024 at 12:29 AM Chen-Yu Tsai <wenst@chromium.org> wrote: ... > > > obj-$(CONFIG_CHROMEOS_LAPTOP) += chromeos_laptop.o > > > obj-$(CONFIG_CHROMEOS_PRIVACY_SCREEN) += chromeos_privacy_screen.o > > > obj-$(CONFIG_CHROMEOS_PSTORE) += chromeos_pstore.o > > > +obj-$(CONFIG_CHROMEOS_OF_HW_PROBER) += chromeos_of_hw_prober.o > > > > "o" sorts before "p" so "of" should sort before "privacy"? > > > > I guess it's not exactly all sorted, but this small section is. Since > > it's arbitrary you could preserve the existing sorting. :-P > > To me it seemed more like they are just sorted in the order they were > added. If we can make it more ordered I'm for it. Just my 2c.
Hi everyone, This is v7 of my "of: Introduce hardware prober driver" [1] series. v7 mainly refactors the code into a series of helpers. The scope of supported components is also reduced to those with at most one regulator supply and one GPIO pin. Also the helpers expect these to be named and so the "bulk get" API changes have been dropped. Also, a pull request to document the "fail-needs-probe" status has been sent: https://github.com/devicetree-org/dt-schema/pull/141 v2 continued Doug's "of: device: Support 2nd sources of probeable but undiscoverable devices" [2] series, but follows the scheme suggested by Rob, marking all second source component device nodes as "fail-needs-probe", and having a hardware prober driver enable the one of them. Changes since v6: - Link to v6: https://lore.kernel.org/all/20240904090016.2841572-1-wenst@chromium.org/ - Dropped patch "gpiolib: Add gpio_property_name_length()" No longer needed - Dropped patch "regulator: Move OF-specific regulator lookup code to of_regulator.c" Already merged - Patch 2 "of: base: Add for_each_child_of_node_with_prefix()" - Changed helper name to "for_each_child_of_node_with_prefix()" - Patch 4 "regulator: Add of_regulator_get_optional() for pure DT regulator lookup" - Was "regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()" - Changed reference [1] to Link: tag - Rebased on top of commit 401d078eaf2e ("regulator: of: Refactor of_get_*regulator() to decrease indentation") - Exported of_regulator_get_optional() - Changed commit message to focus on "of_regulator_get_optional()" - Dropped change to of_regulator_bulk_get_all() - Patch 5 "i2c: core: Remove extra space in Makefile" - Collected Andy's Reviewed-by - Patch 6 "i2c: Introduce OF component probe function" - Correctly replaced for_each_child_of_node_scoped() with for_each_child_of_node_with_prefix() - Added namespace for exported symbols - Made the probe function a framework with hooks - Split out a new header file - Added MAINTAINERS entry - Reworded kernel-doc - Dropped usage of __free from i2c_of_probe_component() since error path cleanup is needed anyway - Patch 7 "i2c: of-prober: Add simple helpers for regulator support" - Moved change of of_get_next_child_scoped() to of_get_next_child_with_prefix() to previous patch - Restructured into helpers for the I2C OF component prober - Reduced to only handle one regulator - Commit message updated - Patch 8 "i2c: of-prober: Add GPIO support to simple helpers" - Restructured into helpers for the I2C OF component prober - Reduced to only handle one GPIO - Set GPIO to input on (failure) cleanup - Updated commit message - Patch 9 "platform/chrome: Introduce device tree hardware prober" - Adapted to new I2C OF prober interface - Collected Acked-by tag Changes since v5: - Link to v5: https://lore.kernel.org/all/20240822092006.3134096-1-wenst@chromium.org/ - Patch 1 "of: dynamic: Add of_changeset_update_prop_string" - Collected Rob's reviewed-by - Patch 2 "of: base: Add for_each_child_of_node_with_prefix_scoped()" - New patch - Patch 3 "regulator: Move OF-specific regulator lookup code to of_regulator.c" - Fix kerneldoc format of of_regulator_dev_lookup() - Fix stub compile error for !CONFIG_OF in drivers/regulator/internal.h - Patch 4 "regulator: Split up _regulator_get()" - Fixed kerneldoc "Return" section format for _regulator_get_common() - Slightly reworded return value description - Patch 5 "regulator: Do pure DT regulator lookup in of_regulator_bulk_get_all()" - Used "dev_of_node(dev)" instead of "dev->of_node" - Replaced "dev_printk" with "dev_printk()" in kerneldoc mentions - Fixed kerneldoc "Return" section format for of_regulator_get_optional() - Fix @np parameter name in of_regulator_dev_lookup() kerneldoc - Patch 6 "gpiolib: Add gpio_property_name_length()" - Changed function name to "gpio_get_property_name_length()" - Changed argument name to "propname" - Clarified return value for "*-<GPIO suffix>" case - Reworked according to Andy's suggestion - Added stub function - Patch 7 "i2c: core: Remove extra space in Makefile" - New patch - Patch 8 "i2c: Introduce OF component probe function" - Fixed indent in Makefile - Split regulator and GPIO TODO items - Reversed final conditional in i2c_of_probe_enable_node() - Patch 9 "i2c: of-prober: Add regulator support" - Split of_regulator_bulk_get_all() return value check and explain "ret == 0" case - Switched to of_get_next_child_with_prefix_scoped() where applicable - Used krealloc_array() instead of directly calculating size - copy whole regulator array in one memcpy() call - Drop "0" from struct zeroing initializer - Split out regulator helper from i2c_of_probe_enable_res() to keep code cleaner when combined with the next patch - Added options for customizing power sequencing delay - Rename i2c_of_probe_get_regulator() to i2c_of_probe_get_regulators() - Add i2c_of_probe_free_regulator() helper - Patch 10 "i2c: of-prober: Add GPIO support" - Renamed "con" to "propname" in i2c_of_probe_get_gpiod() - Copy string first and check return value of strscpy() for overflow in i2c_of_probe_get_gpiod() - Add parenthesis around "enable" and "reset" GPIO names in comments - Split resource count debug message into two separate lines - Split out GPIO helper from i2c_of_probe_enable_res() to keep code cleaner following the previous patch - Adopted options for customizing power sequencing delay following previous patch - Patch 11 "platform/chrome: Introduce device tree hardware prober" - Adapt to new i2c_of_probe_component() parameters - Patch 12 "arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and trackpads as fail" - None See v5 cover letter for previous change logs. For the I2C component (touchscreens and trackpads) case from the original series, the hardware prober driver finds the particular class of device in the device tree, gets its parent I2C adapter, and tries to initiate a simple I2C read for each device under that I2C bus. When it finds one that responds, it considers that one present, marks it as "okay", and returns, letting the driver core actually probe the device. This works fine in most cases since these components are connected via a ribbon cable and always have the same resources. The prober will also grab these resources and enable them. The other case, selecting a display panel to use based on the SKU ID from the firmware, hit a bit of an issue with fixing the OF graph. It has been left out since v3. Patch 1 adds of_changeset_update_prop_string(), as requested by Rob. Patch 2 adds for_each_child_of_node_with_prefix(), as suggested by Andy. Patches 3 through 4 reorganize the OF-specific regulator core code and adds a new of_regulator_get_optional() function to look up regulator supplies solely using device tree nodes. Patch 5 cleans up some extra spaces in the i2c core Makefile Patch 6 implements probing the I2C bus for presence of components as a hookable helper function in the I2C core. Patch 7 implements regulator supply support as a set of simple helpers for the I2C component prober. Patch 8 implements GPIO support for the I2C component prober simple helpers. Patch 9 adds a ChromeOS specific DT hardware prober. This initial version targets the Hana Chromebooks, probing its I2C trackpads and touchscreens. Patch 10 modifies the Hana device tree and marks the touchscreens and trackpads as "fail-needs-probe", ready for the driver to probe. The patch and build time dependencies for this series is now quite complicated: regulator cleanups in -next -> regulator patches here ---- | v platform/chrome device tree hardware prober <--- i2c of-prober The regulator patches in this series depend on other cleanup patches [1] that are already in -next. Patches 6 through 8 introducting i2c of-prober depend on the first 5 patches. Patch 11, The chrome prober, depends on patch 6 for now. I think it would be easier if the respective maintainers take the first four patches for -rc1. Wolfram has agreed to take the remaining i2c and chrome patches through the i2c tree once the other bits have landed, Patch 12 can go in only after everything else is in. This should be better than having an immutable branch on top of some commit in -next for other trees to consume. This might be the last revision I send out before ELCE / Plumbers, as I'm traveling to Austria a few day earlier. If there are more concerns about the design, maybe we could discuss it in person then if all concerned parties are present. Thanks ChenYu Chen-Yu Tsai (10): of: dynamic: Add of_changeset_update_prop_string of: base: Add for_each_child_of_node_with_prefix() regulator: Split up _regulator_get() regulator: Add of_regulator_get_optional() for pure DT regulator lookup i2c: core: Remove extra space in Makefile i2c: Introduce OF component probe function i2c: of-prober: Add simple helpers for regulator support i2c: of-prober: Add GPIO support to simple helpers platform/chrome: Introduce device tree hardware prober arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and trackpads as fail MAINTAINERS | 8 + .../boot/dts/mediatek/mt8173-elm-hana.dtsi | 13 + arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi | 4 +- drivers/i2c/Makefile | 7 +- drivers/i2c/i2c-core-of-prober.c | 455 ++++++++++++++++++ drivers/of/base.c | 35 ++ drivers/of/dynamic.c | 44 ++ drivers/platform/chrome/Kconfig | 11 + drivers/platform/chrome/Makefile | 1 + .../platform/chrome/chromeos_of_hw_prober.c | 125 +++++ drivers/regulator/core.c | 58 ++- drivers/regulator/internal.h | 6 + drivers/regulator/of_regulator.c | 51 +- include/linux/i2c-of-prober.h | 131 +++++ include/linux/of.h | 13 + include/linux/regulator/consumer.h | 4 + 16 files changed, 942 insertions(+), 24 deletions(-) create mode 100644 drivers/i2c/i2c-core-of-prober.c create mode 100644 drivers/platform/chrome/chromeos_of_hw_prober.c create mode 100644 include/linux/i2c-of-prober.h