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.