From: Jani Nikula <jani.nikula@linux.intel.com>
To: Douglas Anderson <dianders@chromium.org>,
dri-devel@lists.freedesktop.org
Cc: Douglas Anderson <dianders@chromium.org>,
Sankeerth Billakanti <quic_sbillaka@quicinc.com>,
Philip Chen <philipchen@chromium.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
David Airlie <airlied@linux.ie>,
linux-kernel@vger.kernel.org,
Abhinav Kumar <quic_abhinavk@quicinc.com>,
Robert Foss <robert.foss@linaro.org>,
Stephen Boyd <swboyd@chromium.org>,
Hsin-Yi Wang <hsinyi@chromium.org>,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Subject: Re: [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly
Date: Mon, 11 Apr 2022 11:34:16 +0300 [thread overview]
Message-ID: <87o818hvcn.fsf@intel.com> (raw)
In-Reply-To: <20220408193536.RFC.1.I4182ae27e00792842cb86f1433990a0ef9c0a073@changeid>
On Fri, 08 Apr 2022, Douglas Anderson <dianders@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@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
next prev parent reply other threads:[~2022-04-11 8:34 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-09 2:36 [RFC PATCH 0/6] drm/dp: Improvements for DP AUX channel Douglas Anderson
2022-04-09 2:36 ` [RFC PATCH 1/6] drm/dp: Helpers to make it easier for drivers to use DP AUX bus properly Douglas Anderson
2022-04-11 8:34 ` Jani Nikula [this message]
2022-04-11 13:37 ` Doug Anderson
2022-04-14 23:51 ` Stephen Boyd
2022-04-15 21:13 ` Doug Anderson
2022-04-15 0:46 ` Dmitry Baryshkov
2022-04-15 21:13 ` Doug Anderson
2022-04-15 22:44 ` Dmitry Baryshkov
2022-04-16 0:09 ` Doug Anderson
2022-04-16 0:54 ` Dmitry Baryshkov
2022-04-18 23:10 ` Doug Anderson
2022-05-03 22:45 ` Doug Anderson
2022-05-03 23:23 ` Doug Anderson
2022-04-09 2:36 ` [RFC PATCH 2/6] drm/bridge: parade-ps8640: Break probe in two to handle DP AUX better Douglas Anderson
2022-04-09 2:36 ` [RFC PATCH 3/6] drm/dp: Add is_hpd_asserted() callback to struct drm_dp_aux Douglas Anderson
2022-04-15 0:48 ` Dmitry Baryshkov
2022-04-09 2:36 ` [RFC PATCH 4/6] drm/panel-edp: Take advantage of is_hpd_asserted() in " Douglas Anderson
2022-04-15 0:51 ` Dmitry Baryshkov
2022-04-15 21:17 ` Doug Anderson
2022-04-15 22:11 ` Dmitry Baryshkov
2022-04-16 0:12 ` Doug Anderson
2022-04-16 0:14 ` Dmitry Baryshkov
2022-04-18 17:18 ` Doug Anderson
2022-04-09 2:36 ` [RFC PATCH 5/6] drm/panel: atna33xc20: " Douglas Anderson
2022-04-09 2:36 ` [RFC PATCH 6/6] drm/bridge: parade-ps8640: Provide " Douglas Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o818hvcn.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=airlied@linux.ie \
--cc=dianders@chromium.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=hsinyi@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=philipchen@chromium.org \
--cc=quic_abhinavk@quicinc.com \
--cc=quic_sbillaka@quicinc.com \
--cc=robert.foss@linaro.org \
--cc=swboyd@chromium.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).