Message-ID: <ZwfvuA2WhD_0P3gL@smile.fi.intel.com>
Date: Thu, 10 Oct 2024 18:16:08 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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,
	Douglas Anderson <dianders@...omium.org>,
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v8 4/8] i2c: Introduce OF component probe function

On Tue, Oct 08, 2024 at 03:34:23PM +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 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.

Fresh reading of the commit message make me think why the firmware or
bootloader on such a device can't form a dynamic OF (overlay?) to fulfill
the need?

Another question is that we have the autoprobing mechanism for I2C for ages,
why that one can't be (re-)used / extended to cover these cases?

...

> +#ifndef _LINUX_I2C_OF_PROBER_H
> +#define _LINUX_I2C_OF_PROBER_H

Missing kconfig.h.

> +struct device;
> +struct device_node;
> +
> +/**
> + * struct i2c_of_probe_ops - I2C OF component prober callbacks
> + *
> + * A set of callbacks to be used by i2c_of_probe_component().
> + *
> + * All callbacks are optional. Callbacks are called only once per run, and are
> + * used in the order they are defined in this structure.
> + *
> + * All callbacks that have return values shall return %0 on success,
> + * or a negative error number on failure.
> + *
> + * The @dev parameter passed to the callbacks is the same as @dev passed to
> + * i2c_of_probe_component(). It should only be used for dev_printk() calls
> + * and nothing else, especially not managed device resource (devres) APIs.
> + */
> +struct i2c_of_probe_ops {
> +	/**
> +	 * @enable: Retrieve and enable resources so that the components respond to probes.
> +	 *
> +	 * Resources should be reverted to their initial state before returning if this fails.
> +	 */
> +	int (*enable)(struct device *dev, struct device_node *bus_node, void *data);
> +
> +	/**
> +	 * @cleanup_early: Release exclusive resources prior to enabling component.
> +	 *
> +	 * Only called if a matching component is actually found. If none are found,
> +	 * resources that would have been released in this callback should be released in
> +	 * @free_resourcs_late instead.
> +	 */
> +	void (*cleanup_early)(struct device *dev, void *data);
> +
> +	/**
> +	 * @cleanup: Opposite of @enable to balance refcounts and free resources after probing.
> +	 *
> +	 * Should check if resources were already freed by @cleanup_early.
> +	 */
> +	void (*cleanup)(struct device *dev, void *data);
> +};
> +
> +/**
> + * struct i2c_of_probe_cfg - I2C OF component prober configuration
> + * @ops: Callbacks for the prober to use.
> + * @type: A string to match the device node name prefix to probe for.
> + */
> +struct i2c_of_probe_cfg {
> +	const struct i2c_of_probe_ops *ops;
> +	const char *type;
> +};
> +
> +#if IS_ENABLED(CONFIG_OF_DYNAMIC)
> +
> +int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cfg, void *ctx);
> +
> +#endif /* IS_ENABLED(CONFIG_OF_DYNAMIC) */
> +
> +#endif /* _LINUX_I2C_OF_PROBER_H */

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists