[RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Jani Nikula
jani.nikula at linux.intel.com
Mon Apr 11 08:34:16 UTC 2022
On Fri, 08 Apr 2022, Douglas Anderson <dianders at chromium.org> wrote:
> As talked about in the kerneldoc for "struct dp_aux_ep_client" in this
> patch and also in the past in commit a1e3667a9835 ("drm/bridge:
> ti-sn65dsi86: Promote the AUX channel to its own sub-dev"), to use the
> DP AUX bus properly we really need two "struct device"s. One "struct
> device" is in charge of providing the DP AUX bus and the other is
> where we'll try to get a reference to the newly probed endpoint
> devices.
>
> In ti-sn65dsi86 this wasn't too difficult to accomplish. That driver
> is already broken up into several "struct devices" anyway because it
> also provides a PWM and some GPIOs. Adding one more wasn't that
> difficult / ugly.
>
> When I tried to do the same solution in parade-ps8640, it felt like I
> was copying too much boilerplate code. I made the realization that I
> didn't _really_ need a separate "driver" for each person that wanted
> to do the same thing. By putting all the "driver" related code in a
> common place then we could save a bit of hassle. This change
> effectively adds a new "ep_client" driver that can be used by
> anyone. The devices instantiated by this driver will just call through
> to the probe/remove/shutdown calls provided.
>
> At the moment, the "ep_client" driver is backed by the Linux auxiliary
> bus (unfortunate naming--this has nothing to do with DP AUX). I didn't
> want to expose this to clients, though, so as far as clients are
> concerned they get a vanilla "struct device".
>
> Signed-off-by: Douglas Anderson <dianders at chromium.org>
What is an "EP" client or device?
BR,
Jani.
> ---
>
> drivers/gpu/drm/dp/drm_dp_aux_bus.c | 165 +++++++++++++++++++++++++++-
> include/drm/dp/drm_dp_aux_bus.h | 58 ++++++++++
> 2 files changed, 222 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/dp/drm_dp_aux_bus.c b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> index 415afce3cf96..5386ceacf133 100644
> --- a/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> +++ b/drivers/gpu/drm/dp/drm_dp_aux_bus.c
> @@ -12,6 +12,7 @@
> * to perform transactions on that bus.
> */
>
> +#include <linux/auxiliary_bus.h>
> #include <linux/init.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> @@ -299,6 +300,163 @@ void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *drv)
> }
> EXPORT_SYMBOL_GPL(dp_aux_dp_driver_unregister);
>
> +/* -----------------------------------------------------------------------------
> + * DP AUX EP Client
> + */
> +
> +struct dp_aux_ep_client_data {
> + struct dp_aux_ep_client *client;
> + struct auxiliary_device adev;
> +};
> +
> +static int dp_aux_ep_client_probe(struct auxiliary_device *adev,
> + const struct auxiliary_device_id *id)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (!data->client->probe)
> + return 0;
> +
> + return data->client->probe(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_remove(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->remove)
> + data->client->remove(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_shutdown(struct auxiliary_device *adev)
> +{
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + if (data->client->shutdown)
> + data->client->shutdown(&adev->dev, data->client);
> +}
> +
> +static void dp_aux_ep_client_dev_release(struct device *dev)
> +{
> + struct auxiliary_device *adev = to_auxiliary_dev(dev);
> + struct dp_aux_ep_client_data *data =
> + container_of(adev, struct dp_aux_ep_client_data, adev);
> +
> + kfree(data);
> +}
> +
> +/**
> + * dp_aux_register_ep_client() - Register an DP AUX EP client
> + * @client: The structure describing the client. It's the client's
> + * responsibility to keep this memory around until
> + * dp_aux_unregister_ep_client() is called, either explicitly or
> + * implicitly via devm.
> + *
> + * See the description of "struct dp_aux_ep_client" for a full explanation
> + * of when you should use this and why.
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data;
> + int ret;
> +
> + data = kzalloc(sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + data->adev.name = "ep_client";
> + data->adev.dev.parent = client->aux->dev;
> + data->adev.dev.release = dp_aux_ep_client_dev_release;
> + device_set_of_node_from_dev(&data->adev.dev, client->aux->dev);
> +
> + ret = auxiliary_device_init(&data->adev);
> + if (ret) {
> + /*
> + * NOTE: if init doesn't fail then it takes ownership
> + * of memory and this kfree() is magically part of
> + * auxiliary_device_uninit().
> + */
> + kfree(data);
> + return ret;
> + }
> +
> + ret = auxiliary_device_add(&data->adev);
> + if (ret)
> + goto err_did_init;
> +
> + client->_opaque = data;
> +
> + return 0;
> +
> +err_did_init:
> + auxiliary_device_uninit(&data->adev);
> + return ret;
> +}
> +
> +/**
> + * dp_aux_unregister_ep_client() - Inverse of dp_aux_register_ep_client()
> + * @client: The structure describing the client.
> + *
> + * If dp_aux_register_ep_client() returns no error then you should call this
> + * to free resources.
> + */
> +void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client)
> +{
> + struct dp_aux_ep_client_data *data = client->_opaque;
> +
> + auxiliary_device_delete(&data->adev);
> + auxiliary_device_uninit(&data->adev);
> +}
> +
> +static void dp_aux_unregister_ep_client_void(void *data)
> +{
> + dp_aux_unregister_ep_client(data);
> +}
> +
> +/**
> + * devm_dp_aux_register_ep_client() - devm wrapper for dp_aux_register_ep_client()
> + * @client: The structure describing the client.
> + *
> + * Handles freeing w/ devm on the device "client->aux->dev".
> + *
> + * Return: 0 if no error or negative error code.
> + */
> +int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client)
> +{
> + int ret;
> +
> + ret = dp_aux_register_ep_client(client);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(client->aux->dev,
> + dp_aux_unregister_ep_client_void,
> + client);
> +}
> +
> +static const struct auxiliary_device_id dp_aux_ep_client_id_table[] = {
> + { .name = "drm_dp_aux_bus.ep_client", },
> + {},
> +};
> +
> +static struct auxiliary_driver dp_aux_ep_client_driver = {
> + .name = "ep_client",
> + .probe = dp_aux_ep_client_probe,
> + .remove = dp_aux_ep_client_remove,
> + .shutdown = dp_aux_ep_client_shutdown,
> + .id_table = dp_aux_ep_client_id_table,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Module init
> + */
> +
> static int __init dp_aux_bus_init(void)
> {
> int ret;
> @@ -307,11 +465,16 @@ static int __init dp_aux_bus_init(void)
> if (ret)
> return ret;
>
> - return 0;
> + ret = auxiliary_driver_register(&dp_aux_ep_client_driver);
> + if (ret)
> + bus_unregister(&dp_aux_bus_type);
> +
> + return ret;
> }
>
> static void __exit dp_aux_bus_exit(void)
> {
> + auxiliary_driver_unregister(&dp_aux_ep_client_driver);
> bus_unregister(&dp_aux_bus_type);
> }
>
> diff --git a/include/drm/dp/drm_dp_aux_bus.h b/include/drm/dp/drm_dp_aux_bus.h
> index 4f19b20b1dd6..ecf68b6873bd 100644
> --- a/include/drm/dp/drm_dp_aux_bus.h
> +++ b/include/drm/dp/drm_dp_aux_bus.h
> @@ -54,4 +54,62 @@ int __dp_aux_dp_driver_register(struct dp_aux_ep_driver *aux_ep_drv,
> struct module *owner);
> void dp_aux_dp_driver_unregister(struct dp_aux_ep_driver *aux_ep_drv);
>
> +/**
> + * struct dp_aux_ep_device - Helper for clients of DP AUX EP devices
> + *
> + * The DP AUX bus can be a bit tricky to use properly. Usually, the way
> + * things work is that:
> + * - The DP controller driver provides the DP AUX bus and would like to probe
> + * the endpoints on the DP AUX bus (AKA the panel) as part of its probe
> + * routine.
> + * - The DP controller driver would also like to acquire a reference to the
> + * DP AUX endpoints (AKA the panel) as part of its probe.
> + *
> + * The problem is that the DP AUX endpoints aren't guaranteed to complete their
> + * probe right away. They could be probing asynchronously or they simply might
> + * fail to acquire some resource and return -EPROBE_DEFER.
> + *
> + * The best way to solve this is to break the DP controller's probe into
> + * two parts. The first part will create the DP AUX bus. The second part will
> + * acquire the reference to the DP AUX endpoint. The first part can complete
> + * finish with no problems and be "done" even if the second part ends up
> + * deferring while waiting for the DP AUX endpoint.
> + *
> + * The dp_aux_ep_client structure and associated functions help with managing
> + * this common case. They will create/add a second "struct device" for you.
> + * In the probe for this second "struct device" (known as the "clientdev" here)
> + * you can acquire references to the AUX DP endpoints and can freely return
> + * -EPROBE_DEFER if they're not ready yet.
> + *
> + * A few notes:
> + * - The parent of the clientdev is guaranteed to be aux->dev
> + * - The of_node of the clientdev is guaranteed to be the same as the of_node
> + * of aux->dev, copied with device_set_of_node_from_dev().
> + * - If you're doing "devm" type things in the clientdev's probe you should
> + * use the clientdev. This makes lifetimes be managed properly.
> + *
> + * NOTE: there's no requirement to use these helpers if you have solved the
> + * problem described above in some other way.
> + */
> +struct dp_aux_ep_client {
> + /** @probe: The second half of the probe */
> + int (*probe)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @remove: Remove function corresponding to the probe */
> + void (*remove)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @shutdown: Shutdown function corresponding to the probe */
> + void (*shutdown)(struct device *clientdev, struct dp_aux_ep_client *client);
> +
> + /** @aux: The AUX bus */
> + struct drm_dp_aux *aux;
> +
> + /** @_opaque: Used by the implementation */
> + void *_opaque;
> +};
> +
> +int dp_aux_register_ep_client(struct dp_aux_ep_client *client);
> +void dp_aux_unregister_ep_client(struct dp_aux_ep_client *client);
> +int devm_dp_aux_register_ep_client(struct dp_aux_ep_client *client);
> +
> #endif /* _DP_AUX_BUS_H_ */
--
Jani Nikula, Intel Open Source Graphics Center
More information about the dri-devel
mailing list