[PATCH v9 1/4] drm: of: Add drm_of_get_dsi_bus helper function

Chris Morgan macromorgan at hotmail.com
Wed Jan 11 17:34:36 UTC 2023


On Wed, Jan 11, 2023 at 05:39:26PM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Jan 11, 2023 at 10:30:09AM -0600, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan at hotmail.com>
> > 
> > Add helper function to find DSI host for devices where DSI panel is not
> > a minor of a DSI bus (such as the Samsung AMS495QA01 panel or the
> > official Raspberry Pi touchscreen display).
> > 
> > Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> > Signed-off-by: Maya Matuszczyk <maccraft123mc at gmail.com>
> > ---
> >  drivers/gpu/drm/drm_of.c | 70 ++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_of.h     | 10 ++++++
> >  2 files changed, 80 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index 7bbcb999bb75..4ebb5bc4b595 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -10,6 +10,7 @@
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_device.h>
> >  #include <drm/drm_encoder.h>
> > +#include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> >  
> > @@ -493,3 +494,72 @@ int drm_of_get_data_lanes_count_ep(const struct device_node *port,
> >  	return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(drm_of_get_data_lanes_count_ep);
> > +
> > +/**
> > + * drm_of_get_dsi_bus - find the DSI bus for a given device
> > + * @dev: parent device of display (SPI, I2C)
> > + * @info: DSI device info to be updated with correct DSI node
> > + *
> > + * Gets parent DSI bus for a DSI device controlled through a bus other
> > + * than MIPI-DCS (SPI, I2C, etc.) using the Device Tree.
> > + *
> > + * Returns pointer to mipi_dsi_host if successful, -EINVAL if the
> > + * request is unsupported, -EPROBE_DEFER if the DSI host is found but
> > + * not available, or -ENODEV otherwise.
> > + */
> > +struct mipi_dsi_host *drm_of_get_dsi_bus(struct device *dev,
> > +					 struct mipi_dsi_device_info *info)
> > +{
> > +	struct mipi_dsi_host *dsi_host;
> > +	struct device_node *endpoint, *dsi_host_node;
> > +
> > +	/*
> > +	 * Exit immediately if we attempt to call this function when
> > +	 * DRM_MIPI_DSI is not enabled, in the event CONFIG_OF is
> > +	 * enabled.
> > +	 */
> > +	if (!IS_ENABLED(CONFIG_DRM_MIPI_DSI))
> > +		return ERR_PTR(-EINVAL);
> 
> The commit log isn't super clear on why this is needed, but it would be
> more consistent to add an ifdef and only compile the entire function if
> DRM_MIPI_DSI is there, just like you did for OF already.

Thank you. I can do that, I just wasn't sure if "#ifdefs" were frowned
upon or not. That would probably be the easiest way to do this though.

> 
> > +	/*
> > +	 * Get first endpoint child from device.
> > +	 */
> > +	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> > +	if (!endpoint)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	/*
> > +	 * Follow the first endpoint to get the DSI host node.
> > +	 */
> > +	dsi_host_node = of_graph_get_remote_port_parent(endpoint);
> 
> There's no need to hold the reference to endpoint after that call. call
> of_node_put(endpoint) here, and it will simplify the error path.
> 
> > +	if (!dsi_host_node)
> > +		goto error;
> > +
> > +	/*
> > +	 * Get the DSI host from the DSI host node. If we get an error
> > +	 * or the return is null assume we're not ready to probe just
> > +	 * yet. Release the DSI host node since we're done with it.
> > +	 */
> > +	dsi_host = of_find_mipi_dsi_host_by_node(dsi_host_node);
> > +	of_node_put(dsi_host_node);
> > +	if (IS_ERR_OR_NULL(dsi_host)) {
> > +		of_node_put(endpoint);
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +	}
> > +
> > +	/*
> > +	 * Set the node of the mipi_dsi_device_info to the correct node
> > +	 * and then release the endpoint node since we're done with it.
> > +	 */
> > +	info->node = of_graph_get_remote_port(endpoint);
> 
> Ah, you're using it there.
> 
> I think I'd rework the function to:
> 
> - retrieve the endpoint
> - retrieve the remote port, give up the endpoint
> - retrieve the remote port parent
> 
> Also, I'm not entirely sure what you had in mind, but info might not be
> there at all and it would be fine imho.
> 

What if I make it optional and if a NULL is passed skip this step, but
otherwise populate the info node?

> Maxime

Thank you for your input. I'll wait for the bots to see if the previous
errors are really finally fixed, then I'll make these changes and
resubmit.


More information about the dri-devel mailing list