Message-ID: <CAD=FV=Vy6q-nPZ99k7t8-G_osefg=0jxPUWm8F1uhtVw0xDvxg@mail.gmail.com>
Date: Thu, 17 Oct 2024 14:02:36 -0700
From: Doug Anderson <dianders@...omium.org>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>, 
	Matthias Brugger <matthias.bgg@...il.com>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Wolfram Sang <wsa@...nel.org>, 
	Benson Leung <bleung@...omium.org>, Tzung-Bi Shih <tzungbi@...nel.org>, 
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-mediatek@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Johan Hovold <johan@...nel.org>, 
	Jiri Kosina <jikos@...nel.org>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, 
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v9 3/7] i2c: Introduce OF component probe function

Hi,

On Thu, Oct 17, 2024 at 2:42 AM Chen-Yu Tsai <wenst@...omium.org> 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 probe function. For a
> 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 "fail-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 <wenst@...omium.org>
> ---
> Changes since v8:
> - Added missing include of <linux/kconfig.h> to i2c-of-prober.h
> - Expanded description of i2c_of_probe_ops::enable to mention that
>   returning -EPROBE_DEFER is valid (Doug)
> - Reworded descrption of i2c_of_probe_ops::cleanup_early to avoid
>   confusion with i2c_of_probe_ops::enable (Doug)
> - Reworked i2c_of_probe_get_i2c_node() to return NULL on failure,
>   matching most functions that return |struct device_node *|.
>
> Changes since v7:
> - Dropped log level of "enabling component" to debug
> - Dropped file name from header file
> - Reverted to __free() cleanup for i2c bus node
> - Corrected "failed-needs-probe" to "fail-needs-probe" in commit message
> - Fixed incorrectly positioned period ('.') in commit message
> - Expanded description of i2c_of_probe_component()
> - Expanded comment explaining check for "available" devices to note that
>   if such a device is found then the i2c probe function becomes a no-op
> - Simplified check for "available" devices for-each loop
> - Expanded description of @free_resources_early callback to explicitly
>   state that it is not called if no working components are found
> - Dropped !cfg check
> - Replaced "fail" with "fail-needs-probe" in i2c_of_probe_component()
>   kernel doc
> - Combined callbacks (.get_resources with .enable; .cleanup with
>   .free_resources_late); .free_resources_early renamed to .cleanup_early
>
> Changes since v6:
> - Correctly replaced for_each_child_of_node_scoped() with
>   for_each_child_of_node_with_prefix()
> - Added namespace for exported symbol
> - 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
>
> Changes since v5:
> - Fixed indent in Makefile
> - Split regulator and GPIO TODO items
> - Reversed final conditional in i2c_of_probe_enable_node()
>
> Changes since v4:
> - Split code into helper functions
> - Use scoped helpers and __free() to reduce error path
>
> Changes since v3:
> - Complete kernel-doc
> - Return different error if I2C controller is disabled
> - Expand comment to explain assumptions and constraints
> - Split for-loop finding target node and operations on target node
> - Add missing i2c_put_adapter()
> - Move prober code to separate file
>
> Rob also asked why there was a limitation of "exactly one touchscreen
> will be enabled across the whole tree".
>
> The use case this prober currently targets is a component on consumer
> electronics (tablet or laptop) being swapped out due to cost or supply
> reasons. Designs with multiple components of the same type are pretty
> rare. The way the next patch is written also assumes this for efficiency
> reasons.
>
> Changes since v2:
> - New patch split out from "of: Introduce hardware prober driver"
> - Addressed Rob's comments
>   - Move i2c prober to i2c subsystem
>   - Use of_node_is_available() to check if node is enabled.
>   - Use OF changeset API to update status property
> - Addressed Andy's comments
>   - Probe function now accepts "struct device *dev" instead to reduce
>     line length and dereferences
>   - Move "ret = 0" to just before for_each_child_of_node(i2c_node, node)
> ---
>  MAINTAINERS                      |   8 ++
>  drivers/i2c/Makefile             |   1 +
>  drivers/i2c/i2c-core-of-prober.c | 182 +++++++++++++++++++++++++++++++
>  include/linux/i2c-of-prober.h    |  75 +++++++++++++
>  4 files changed, 266 insertions(+)

Reviewed-by: Douglas Anderson <dianders@...omium.org>

Powered by blists - more mailing lists