Hi everyone,
This is v3 of my "of: Introduce hardware prober driver" [1] series.
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. I tried to include
everyone from the original Cc: list. Please let me know if you would
like to be dropped from future submissions.
Changes since v2:
- Added of_changeset_update_prop_string()
- Moved generic I2C code to the I2C core
- Moved remaining platform specific code to platform/chrome/
- Switched to of_node_is_available() to check if node is enabled.
- Switched to OF changeset API to update status property
- I2C probe helper function now accepts "struct device *dev" instead to
reduce line length and dereferences
- Moved "ret = 0" to just before for_each_child_of_node(i2c_node, node)
- Depend on rather than select CONFIG_I2C
- Copied machine check to driver init function
- Explicitly mentioned "device tree" or OF in driver name, description
and Kconfig symbol
- Dropped filename from inside the file
- Made loop variable size_t (instead of unsigned int as Andy asked)
- Switched to PLATFORM_DEVID_NONE instead of raw -1
- Switched to standard goto error path pattern in hw_prober_driver_init()
- Dropped device class from status property
Patches removed from v3 and saved for later:
- of: base: Add of_device_is_fail
- of: hw_prober: Support Chromebook SKU ID based component selection
- dt-bindings: arm: mediatek: Remove SKU specific compatibles for Google Krane
- arm64: dts: mediatek: mt8183-kukui: Merge Krane device trees
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 ribbon cable and always have the same resources. The driver as
implemented currently doesn't deal with regulators or GPIO pins,
since in the existing device trees they are either always on for
regulators, or have GPIO hogs or pinmux and pinconfig directly
tied to the pin controller.
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.
I've left it out of v3 for now.
Patch 1 adds of_changeset_update_prop_string(), as requested by Rob.
Patch 2 implements probing the I2C bus for presence of components as
a helper function in the I2C core.
Patch 3 adds a ChromeOS specific DT hardware prober. This initial
version targets the Hana Chromebooks, probing its I2C trackpads and
touchscreens.
Patch 4 modifies the Hana device tree and marks the touchscreens
and trackpads as "fail-needs-probe", ready for the driver to probe.
Patch 5 adds a missing touchscreen variant to Hana. This patch
conflicts with another one in flight [3] that is almost the same.
Assuming this is acceptable to folks, because there are compile
time dependencies, I think it would be easier for the code bits
(patches 1 through 4) to go through either the OF tree or I2C
tree. Patches 5 and 6 can go through the soc tree via the mediatek
tree.
Thanks
ChenYu
Background as given in Doug's cover letter:
Support for multiple "equivalent" sources for components (also known
as second sourcing components) is a standard practice that helps keep
cost down and also makes sure that if one component is unavailable due
to a shortage that we don't need to stop production for the whole
product.
Some components are very easy to second source. eMMC, for instance, is
fully discoverable and probable so you can stuff a wide variety of
similar eMMC chips on your board and things will work without a hitch.
Some components are more difficult to second source, specifically
because it's difficult for software to probe what component is present
on any given board. In cases like this software is provided
supplementary information to help it, like a GPIO strap or a SKU ID
programmed into an EEPROM. This helpful information can allow the
bootloader to select a different device tree. The various different
"SKUs" of different Chromebooks are examples of this.
Some components are somewhere in between. These in-between components
are the subject of this patch. Specifically, these components are
easily "probeable" but not easily "discoverable".
A good example of a probeable but undiscoverable device is an
i2c-connected touchscreen or trackpad. Two separate components may be
electrically compatible with each other and may have compatible power
sequencing requirements but may require different software. If
software is told about the different possible components (because it
can't discover them), it can safely probe them to figure out which
ones are present.
On systems using device tree, if we want to tell the OS about all of
the different components we need to list them all in the device
tree. This leads to a problem. The multiple sources for components
likely use the same resources (GPIOs, interrupts, regulators). If the
OS tries to probe all of these components at the same time then it
will detect a resource conflict and that's a fatal error.
The fact that Linux can't handle these probeable but undiscoverable
devices well has had a few consequences:
1. In some cases, we've abandoned the idea of second sourcing
components for a given board, which increases cost / generates
manufacturing headaches.
2. In some cases, we've been forced to add some sort of strapping /
EEPROM to indicate which component is present. This adds difficulty
to manufacturing / refurb processes.
3. In some cases, we've managed to make things work by the skin of our
teeth through slightly hacky solutions. Specifically, if we remove
the "pinctrl" entry from the various options then it won't
conflict. Regulators inherently can have more than one consumer, so
as long as there are no GPIOs involved in power sequencing and
probing devices then things can work. This is how
"sc8280xp-lenovo-thinkpad-x13s" works and also how
"mt8173-elm-hana" works.
End of background from Doug's cover letter.
[1] https://lore.kernel.org/linux-arm-kernel/[email protected]/
[2] https://lore.kernel.org/all/20230921102420.RFC.1.I9dddd99ccdca175e3ceb1b9fa1827df0928c5101@changeid/
[3] https://lore.kernel.org/linux-mediatek/[email protected]/
Chen-Yu Tsai (5):
of: dynamic: Add of_changeset_update_prop_string
i2c: of: Introduce component probe function
platform/chrome: Introduce device tree hardware prober
arm64: dts: mediatek: mt8173-elm-hana: Mark touchscreens and trackpads
as fail
arm64: dts: mediatek: mt8173-elm-hana: Add G2touch G7500 touchscreen
.../boot/dts/mediatek/mt8173-elm-hana.dtsi | 20 ++++
drivers/i2c/i2c-core-of.c | 110 ++++++++++++++++++
drivers/of/dynamic.c | 47 ++++++++
drivers/platform/chrome/Kconfig | 11 ++
drivers/platform/chrome/Makefile | 1 +
.../platform/chrome/chromeos_of_hw_prober.c | 89 ++++++++++++++
include/linux/i2c.h | 4 +
include/linux/of.h | 3 +
8 files changed, 285 insertions(+)
create mode 100644 drivers/platform/chrome/chromeos_of_hw_prober.c
--
2.43.0.rc1.413.gea7ed67945-goog
This introduces yet another second-source touchscreen found on Hana.
This is a G2touch G7500 touchscreen, which is compatible with HID over
I2C.
Add a device node for it. In keeping with the new scheme for second
source components, mark it as "failed-needs-probe".
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
Changes since v2:
- Drop class from status
---
arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
index 1d68bc6834e4..216d8b43be05 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
@@ -31,6 +31,15 @@ touchscreen3: touchscreen@20 {
interrupts = <88 IRQ_TYPE_LEVEL_LOW>;
status = "fail-needs-probe";
};
+
+ touchscreen4: touchscreen@40 {
+ compatible = "hid-over-i2c";
+ reg = <0x40>;
+ hid-descr-addr = <0x0001>;
+ interrupt-parent = <&pio>;
+ interrupts = <88 IRQ_TYPE_LEVEL_LOW>;
+ status = "fail-needs-probe";
+ };
};
&i2c4 {
--
2.43.0.rc1.413.gea7ed67945-goog
Some devices are designed and manufactured with some components having
multiple drop-in replacement options. These components are often
connected to the mainboard via ribbon cables, having the same signals
and pin assignments across all options. These may include the display
panel and touchscreen on laptops and tablets, and the trackpad on
laptops. Sometimes which component option is used in a particular device
can be detected by some firmware provided identifier, other times that
information is not available, and the kernel has to try to probe each
device.
This change attempts to make the "probe each device" case cleaner. The
current approach is to have all options added and enabled in the device
tree. The kernel would then bind each device and run each driver's probe
function. This works, but has been broken before due to the introduction
of asynchronous probing, causing multiple instances requesting "shared"
resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
time, with only one instance succeeding. Work arounds for these include
moving the pinmux to the parent I2C controller, using GPIO hogs or
pinmux settings to keep the GPIO pins in some fixed configuration, and
requesting the interrupt line very late. Such configurations can be seen
on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
Lenovo Thinkpad 13S.
Instead of this delicate dance between drivers and device tree quirks,
this change introduces a simple I2C component prober. For any given
class of devices on the same I2C bus, it will go through all of them,
doing a simple I2C read transfer and see which one of them responds.
It will then enable the device that responds.
This requires some minor modifications in the existing device tree.
The status for all the device nodes for the component options must be
set to "failed-needs-probe". This makes it clear that some mechanism is
needed to enable one of them, and also prevents the prober and device
drivers running at the same time.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
Changes since v2:
- Addressed Rob's comments
- Move remaining driver code to drivers/platform/chrome/
- Depend on rather than select CONFIG_I2C
- Copy machine check to driver init function
- Addressed Andy's comments
- Explicitly mention "device tree" or OF in driver name, description
and Kconfig symbol
- Drop filename from inside the file
- Switch to passing "struct device *" to shorten lines
- Move "ret = 0" to just before for_each_child_of_node(i2c_node, node)
- Make loop variable size_t (instead of unsigned int as Andy asked)
- Use PLATFORM_DEVID_NONE instead of raw -1
- Use standard goto error path pattern in hw_prober_driver_init()
- Changes since v1:
- New patch
---
drivers/platform/chrome/Kconfig | 11 +++
drivers/platform/chrome/Makefile | 1 +
.../platform/chrome/chromeos_of_hw_prober.c | 89 +++++++++++++++++++
3 files changed, 101 insertions(+)
create mode 100644 drivers/platform/chrome/chromeos_of_hw_prober.c
diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig
index 7a83346bfa53..aa161f2f09e3 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
+ bool "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..13aaad6382aa
--- /dev/null
+++ b/drivers/platform/chrome/chromeos_of_hw_prober.c
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ChromeOS Device Tree Hardware Prober
+ *
+ * Copyright (c) 2023 Google LLC
+ */
+
+#include <linux/array_size.h>
+#include <linux/i2c.h>
+#include <linux/of.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;
+};
+
+static int chromeos_i2c_component_prober(struct device *dev, const void *data)
+{
+ const char *type = data;
+
+ return i2c_of_probe_component(dev, type);
+}
+
+static const struct hw_prober_entry hw_prober_platforms[] = {
+ { .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = "touchscreen" },
+ { .compatible = "google,hana", .prober = chromeos_i2c_component_prober, .data = "trackpad" },
+};
+
+static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
+{
+ for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
+ if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
+ int ret;
+
+ ret = hw_prober_platforms[i].prober(&pdev->dev,
+ hw_prober_platforms[i].data);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static struct platform_driver chromeos_of_hw_prober_driver = {
+ .probe = chromeos_of_hw_prober_probe,
+ .driver = {
+ .name = DRV_NAME,
+ },
+};
+
+static int __init chromeos_of_hw_prober_driver_init(void)
+{
+ struct platform_device *pdev;
+ 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 0;
+
+ ret = platform_driver_register(&chromeos_of_hw_prober_driver);
+ if (ret)
+ return ret;
+
+ pdev = platform_device_register_simple(DRV_NAME, PLATFORM_DEVID_NONE, NULL, 0);
+ if (IS_ERR(pdev))
+ goto err;
+
+ return 0;
+
+err:
+ platform_driver_unregister(&chromeos_of_hw_prober_driver);
+
+ return PTR_ERR(pdev);
+}
+device_initcall(chromeos_of_hw_prober_driver_init);
--
2.43.0.rc1.413.gea7ed67945-goog
Add a helper function to add string property updates to an OF changeset.
This is similar to of_changeset_add_prop_string(), but instead of adding
the property (and failing if it exists), it will update the property.
This shall be used later in the DT hardware prober.
Signed-off-by: Chen-Yu Tsai <[email protected]>
---
New patch added in v3.
---
drivers/of/dynamic.c | 47 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/of.h | 3 +++
2 files changed, 50 insertions(+)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index f63250c650ca..d22aad938667 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -1039,3 +1039,50 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
return ret;
}
EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
+
+static int of_changeset_update_prop_helper(struct of_changeset *ocs,
+ struct device_node *np,
+ const struct property *pp)
+{
+ struct property *new_pp;
+ int ret;
+
+ new_pp = __of_prop_dup(pp, GFP_KERNEL);
+ if (!new_pp)
+ return -ENOMEM;
+
+ ret = of_changeset_update_property(ocs, np, new_pp);
+ if (ret) {
+ kfree(new_pp->name);
+ kfree(new_pp->value);
+ kfree(new_pp);
+ }
+
+ return ret;
+}
+
+/**
+ * of_changeset_update_prop_string - Add a string property update to a changeset
+ *
+ * @ocs: changeset pointer
+ * @np: device node pointer
+ * @prop_name: name of the property to be updated
+ * @str: pointer to null terminated string
+ *
+ * Create a string property to be updated and add it to a changeset.
+ *
+ * Return: 0 on success, a negative error value in case of an error.
+ */
+int of_changeset_update_prop_string(struct of_changeset *ocs,
+ struct device_node *np,
+ const char *prop_name, const char *str)
+{
+ struct property prop;
+
+ prop.name = (char *)prop_name;
+ prop.length = strlen(str) + 1;
+ prop.value = (void *)str;
+
+ return of_changeset_update_prop_helper(ocs, np, &prop);
+}
+EXPORT_SYMBOL_GPL(of_changeset_update_prop_string);
diff --git a/include/linux/of.h b/include/linux/of.h
index 6a9ddf20e79a..c69bc7da380e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1601,6 +1601,9 @@ static inline int of_changeset_add_prop_u32(struct of_changeset *ocs,
{
return of_changeset_add_prop_u32_array(ocs, np, prop_name, &val, 1);
}
+int of_changeset_update_prop_string(struct of_changeset *ocs,
+ struct device_node *np,
+ const char *prop_name, const char *str);
#else /* CONFIG_OF_DYNAMIC */
static inline int of_reconfig_notifier_register(struct notifier_block *nb)
--
2.43.0.rc1.413.gea7ed67945-goog
On Tue, Nov 28, 2023 at 04:42:32PM +0800, Chen-Yu Tsai wrote:
> Some devices are designed and manufactured with some components having
> multiple drop-in replacement options. These components are often
> connected to the mainboard via ribbon cables, having the same signals
> and pin assignments across all options. These may include the display
> panel and touchscreen on laptops and tablets, and the trackpad on
> laptops. Sometimes which component option is used in a particular device
> can be detected by some firmware provided identifier, other times that
> information is not available, and the kernel has to try to probe each
> device.
>
> This change attempts to make the "probe each device" case cleaner. The
> current approach is to have all options added and enabled in the device
> tree. The kernel would then bind each device and run each driver's probe
> function. This works, but has been broken before due to the introduction
> of asynchronous probing, causing multiple instances requesting "shared"
> resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> time, with only one instance succeeding. Work arounds for these include
> moving the pinmux to the parent I2C controller, using GPIO hogs or
> pinmux settings to keep the GPIO pins in some fixed configuration, and
> requesting the interrupt line very late. Such configurations can be seen
> on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> Lenovo Thinkpad 13S.
>
> Instead of this delicate dance between drivers and device tree quirks,
> this change introduces a simple I2C component prober. For any given
> class of devices on the same I2C bus, it will go through all of them,
> doing a simple I2C read transfer and see which one of them responds.
> It will then enable the device that responds.
>
> This requires some minor modifications in the existing device tree.
> The status for all the device nodes for the component options must be
> set to "failed-needs-probe". This makes it clear that some mechanism is
> needed to enable one of them, and also prevents the prober and device
> drivers running at the same time.
...
> +#include <linux/array_size.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
init.h for init calls.
> +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> +{
> + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> + int ret;
Perhaps
if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
continue;
?
> + ret = hw_prober_platforms[i].prober(&pdev->dev,
> + hw_prober_platforms[i].data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 29, 2023 at 12:26 AM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Nov 28, 2023 at 04:42:32PM +0800, Chen-Yu Tsai wrote:
> > Some devices are designed and manufactured with some components having
> > multiple drop-in replacement options. These components are often
> > connected to the mainboard via ribbon cables, having the same signals
> > and pin assignments across all options. These may include the display
> > panel and touchscreen on laptops and tablets, and the trackpad on
> > laptops. Sometimes which component option is used in a particular device
> > can be detected by some firmware provided identifier, other times that
> > information is not available, and the kernel has to try to probe each
> > device.
> >
> > This change attempts to make the "probe each device" case cleaner. The
> > current approach is to have all options added and enabled in the device
> > tree. The kernel would then bind each device and run each driver's probe
> > function. This works, but has been broken before due to the introduction
> > of asynchronous probing, causing multiple instances requesting "shared"
> > resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> > time, with only one instance succeeding. Work arounds for these include
> > moving the pinmux to the parent I2C controller, using GPIO hogs or
> > pinmux settings to keep the GPIO pins in some fixed configuration, and
> > requesting the interrupt line very late. Such configurations can be seen
> > on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> > Lenovo Thinkpad 13S.
> >
> > Instead of this delicate dance between drivers and device tree quirks,
> > this change introduces a simple I2C component prober. For any given
> > class of devices on the same I2C bus, it will go through all of them,
> > doing a simple I2C read transfer and see which one of them responds.
> > It will then enable the device that responds.
> >
> > This requires some minor modifications in the existing device tree.
> > The status for all the device nodes for the component options must be
> > set to "failed-needs-probe". This makes it clear that some mechanism is
> > needed to enable one of them, and also prevents the prober and device
> > drivers running at the same time.
>
> ...
>
> > +#include <linux/array_size.h>
> > +#include <linux/i2c.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
>
> init.h for init calls.
Added to next version.
> > +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> > +{
> > + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> > + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> > + int ret;
>
> Perhaps
>
> if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
> continue;
>
> ?
Works for me. One less level of indentation.
Thanks
ChenYu
> > + ret = hw_prober_platforms[i].prober(&pdev->dev,
> > + hw_prober_platforms[i].data);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Hi,
On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <[email protected]> wrote:
>
> @@ -1039,3 +1039,50 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
> return ret;
> }
> EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
> +
> +static int of_changeset_update_prop_helper(struct of_changeset *ocs,
> + struct device_node *np,
> + const struct property *pp)
> +{
> + struct property *new_pp;
> + int ret;
> +
> + new_pp = __of_prop_dup(pp, GFP_KERNEL);
> + if (!new_pp)
> + return -ENOMEM;
> +
> + ret = of_changeset_update_property(ocs, np, new_pp);
> + if (ret) {
> + kfree(new_pp->name);
> + kfree(new_pp->value);
> + kfree(new_pp);
Given that this is the 3rd copy of the freeing logic, does it make
sense to make __of_prop_free() that's documented to free what was
returned by __of_prop_dupe()?
Hi,
On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <[email protected]> wrote:
>
> @@ -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
> + bool "ChromeOS Device Tree Hardware Prober"
Any reason that it can't be a module?
> + depends on OF
> + depends on I2C
> + select OF_DYNAMIC
> + default OF
You probably don't want "default OF". This means that everyone will
automatically get this new driver enabled which is unlikely to be
right.
> +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> +{
> + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> + int ret;
> +
> + ret = hw_prober_platforms[i].prober(&pdev->dev,
> + hw_prober_platforms[i].data);
> + if (ret)
Should it only check for -EPROBE_DEFER here? ...and then maybe warn
for other cases and go through the loop? If there's some error
enabling the touchscreen I'd still want the trackpad to probe...
> + return ret;
> + }
> +
> + return 0;
Random thought: once we get here, the driver is useless / just wasting
memory. Any way to have it freed? ;-)
On Sat, Dec 2, 2023 at 9:01 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <[email protected]> wrote:
> >
> > @@ -1039,3 +1039,50 @@ int of_changeset_add_prop_u32_array(struct of_changeset *ocs,
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(of_changeset_add_prop_u32_array);
> > +
> > +static int of_changeset_update_prop_helper(struct of_changeset *ocs,
> > + struct device_node *np,
> > + const struct property *pp)
> > +{
> > + struct property *new_pp;
> > + int ret;
> > +
> > + new_pp = __of_prop_dup(pp, GFP_KERNEL);
> > + if (!new_pp)
> > + return -ENOMEM;
> > +
> > + ret = of_changeset_update_property(ocs, np, new_pp);
> > + if (ret) {
> > + kfree(new_pp->name);
> > + kfree(new_pp->value);
> > + kfree(new_pp);
>
> Given that this is the 3rd copy of the freeing logic, does it make
> sense to make __of_prop_free() that's documented to free what was
> returned by __of_prop_dupe()?
Makes sense. There's also one in property_list_free(). I'll add a patch
for it.
ChenYu
On Sat, Dec 2, 2023 at 8:58 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <[email protected]> wrote:
> >
> > @@ -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
> > + bool "ChromeOS Device Tree Hardware Prober"
>
> Any reason that it can't be a module?
No technical one. However if it's a module, the user has to manually load
it. So I think it's more of a usability thing.
OOTH I think this needs to be a module if I2C is built as a module.
Somehow I had thought of it at one point but then it slipped my mind.
> > + depends on OF
> > + depends on I2C
> > + select OF_DYNAMIC
> > + default OF
>
> You probably don't want "default OF". This means that everyone will
> automatically get this new driver enabled which is unlikely to be
> right.
I thought this whole section was guarded behind KCONFIG_CHROME_PLATFORMS.
So if the user has CHROME_PLATFORMS enabled and has OF enabled, they
likely need the prober.
> > +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> > +{
> > + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> > + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> > + int ret;
> > +
> > + ret = hw_prober_platforms[i].prober(&pdev->dev,
> > + hw_prober_platforms[i].data);
> > + if (ret)
>
> Should it only check for -EPROBE_DEFER here? ...and then maybe warn
> for other cases and go through the loop? If there's some error
> enabling the touchscreen I'd still want the trackpad to probe...
Makes sense. However there's no extra information to give in the
warning though.
> > + return ret;
> > + }
> > +
> > + return 0;
>
> Random thought: once we get here, the driver is useless / just wasting
> memory. Any way to have it freed? ;-)
I don't think there is a good way to do that, except maybe marking all
the functions as __init? But that likely doesn't work in combination
with deferred probing (say the i2c driver is a module).
ChenYu
On Mon, Dec 4, 2023 at 3:24 PM Chen-Yu Tsai <[email protected]> wrote:
>
> On Sat, Dec 2, 2023 at 8:58 AM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <[email protected]> wrote:
> > >
> > > @@ -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
> > > + bool "ChromeOS Device Tree Hardware Prober"
> >
> > Any reason that it can't be a module?
>
> No technical one. However if it's a module, the user has to manually load
> it. So I think it's more of a usability thing.
We could probably manually add module aliases. I thought about aliases
against the machine compatibles, but there doesn't seem to be a device
for it to trigger. Or target something common to ChromeOS devices like
the EC? It's really hacky though.
ChenYu
> OOTH I think this needs to be a module if I2C is built as a module.
> Somehow I had thought of it at one point but then it slipped my mind.
>
> > > + depends on OF
> > > + depends on I2C
> > > + select OF_DYNAMIC
> > > + default OF
> >
> > You probably don't want "default OF". This means that everyone will
> > automatically get this new driver enabled which is unlikely to be
> > right.
>
> I thought this whole section was guarded behind KCONFIG_CHROME_PLATFORMS.
> So if the user has CHROME_PLATFORMS enabled and has OF enabled, they
> likely need the prober.
>
> > > +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> > > +{
> > > + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> > > + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> > > + int ret;
> > > +
> > > + ret = hw_prober_platforms[i].prober(&pdev->dev,
> > > + hw_prober_platforms[i].data);
> > > + if (ret)
> >
> > Should it only check for -EPROBE_DEFER here? ...and then maybe warn
> > for other cases and go through the loop? If there's some error
> > enabling the touchscreen I'd still want the trackpad to probe...
>
> Makes sense. However there's no extra information to give in the
> warning though.
>
> > > + return ret;
> > > + }
> > > +
> > > + return 0;
> >
> > Random thought: once we get here, the driver is useless / just wasting
> > memory. Any way to have it freed? ;-)
>
> I don't think there is a good way to do that, except maybe marking all
> the functions as __init? But that likely doesn't work in combination
> with deferred probing (say the i2c driver is a module).
>
> ChenYu