| Message ID | 20240911072751.365361-10-wenst@chromium.org |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Wolfram Sang |
| Headers | show |
| Series | platform/chrome: Introduce DT hardware prober | expand |
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.
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig index 7dbeb786352a..b7dbaf77b6db 100644 --- a/drivers/platform/chrome/Kconfig +++ b/drivers/platform/chrome/Kconfig @@ -61,6 +61,17 @@ config CHROMEOS_TBMC To compile this driver as a module, choose M here: the module will be called chromeos_tbmc. +config CHROMEOS_OF_HW_PROBER + tristate "ChromeOS Device Tree Hardware Prober" + depends on OF + depends on I2C + select OF_DYNAMIC + default OF + help + This option enables the device tree hardware prober for ChromeOS + devices. The driver will probe the correct component variant in + devices that have multiple drop-in options for one component. + config CROS_EC tristate "ChromeOS Embedded Controller" select CROS_EC_PROTO diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile index 2dcc6ccc2302..21a9d5047053 100644 --- a/drivers/platform/chrome/Makefile +++ b/drivers/platform/chrome/Makefile @@ -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 obj-$(CONFIG_CHROMEOS_TBMC) += chromeos_tbmc.o obj-$(CONFIG_CROS_EC) += cros_ec.o obj-$(CONFIG_CROS_EC_I2C) += cros_ec_i2c.o diff --git a/drivers/platform/chrome/chromeos_of_hw_prober.c b/drivers/platform/chrome/chromeos_of_hw_prober.c new file mode 100644 index 000000000000..6d9667c40cc7 --- /dev/null +++ b/drivers/platform/chrome/chromeos_of_hw_prober.c @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ChromeOS Device Tree Hardware Prober + * + * Copyright (c) 2024 Google LLC + */ + +#include <linux/array_size.h> +#include <linux/errno.h> +#include <linux/i2c-of-prober.h> +#include <linux/module.h> +#include <linux/platform_device.h> + +#define DRV_NAME "chromeos_of_hw_prober" + +/** + * struct hw_prober_entry - Holds an entry for the hardware prober + * + * @compatible: compatible string to match against the machine + * @prober: prober function to call when machine matches + * @data: extra data for the prober function + */ +struct hw_prober_entry { + const char *compatible; + int (*prober)(struct device *dev, const void *data); + const void *data; +}; + +struct chromeos_i2c_probe_data { + const struct i2c_of_probe_cfg *cfg; + const struct i2c_of_probe_simple_opts *opts; +}; + +static int chromeos_i2c_component_prober(struct device *dev, const void *_data) +{ + const struct chromeos_i2c_probe_data *data = _data; + struct i2c_of_probe_simple_ctx ctx = { + .opts = data->opts + }; + + return i2c_of_probe_component(dev, data->cfg, &ctx); +} + +static const struct chromeos_i2c_probe_data chromeos_i2c_probe_dumb_touchscreen = { + .cfg = &(const struct i2c_of_probe_cfg) { + .type = "touchscreen" + } +}; + +static const struct chromeos_i2c_probe_data chromeos_i2c_probe_dumb_trackpad = { + .cfg = &(const struct i2c_of_probe_cfg) { + .type = "trackpad" + } +}; + +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 }, +}; + +static int chromeos_of_hw_prober_probe(struct platform_device *pdev) +{ + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++) { + int ret; + + if (!of_machine_is_compatible(hw_prober_platforms[i].compatible)) + continue; + + ret = hw_prober_platforms[i].prober(&pdev->dev, hw_prober_platforms[i].data); + /* Ignore unrecoverable errors and keep going through other probers */ + if (ret == -EPROBE_DEFER) + return ret; + } + + return 0; +} + +static struct platform_driver chromeos_of_hw_prober_driver = { + .probe = chromeos_of_hw_prober_probe, + .driver = { + .name = DRV_NAME, + }, +}; + +static struct platform_device *chromeos_of_hw_prober_pdev; + +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; + + return 0; + +err: + platform_driver_unregister(&chromeos_of_hw_prober_driver); + + return PTR_ERR(chromeos_of_hw_prober_pdev); +} +module_init(chromeos_of_hw_prober_driver_init); + +static void chromeos_of_hw_prober_driver_exit(void) +{ + platform_device_unregister(chromeos_of_hw_prober_pdev); + platform_driver_unregister(&chromeos_of_hw_prober_driver); +} +module_exit(chromeos_of_hw_prober_driver_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("ChromeOS device tree hardware prober"); +MODULE_IMPORT_NS(I2C_OF_PROBER);