| Message ID | 20230111163012.310945-2-macroalpha82@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: Add Magnachip D53E6EA8966 Panel Controller | expand |
Hi, On Wed, Jan 11, 2023 at 10:30:09AM -0600, Chris Morgan wrote: > From: Chris Morgan <macromorgan@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@hotmail.com> > Signed-off-by: Maya Matuszczyk <maccraft123mc@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. > + /* > + * 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. Maxime
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@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@hotmail.com> > > Signed-off-by: Maya Matuszczyk <maccraft123mc@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.
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); + + /* + * 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); + 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); + if (IS_ERR_OR_NULL(info->node)) + goto error; + + of_node_put(endpoint); + return dsi_host; + +error: + of_node_put(endpoint); + return ERR_PTR(-ENODEV); +} +EXPORT_SYMBOL_GPL(drm_of_get_dsi_bus); diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h index 10ab58c40746..705ea2caa494 100644 --- a/include/drm/drm_of.h +++ b/include/drm/drm_of.h @@ -15,6 +15,8 @@ struct drm_encoder; struct drm_panel; struct drm_bridge; struct device_node; +struct mipi_dsi_device_info; +struct mipi_dsi_host; /** * enum drm_lvds_dual_link_pixels - Pixel order of an LVDS dual-link connection @@ -56,6 +58,8 @@ int drm_of_get_data_lanes_count_ep(const struct device_node *port, int port_reg, int reg, const unsigned int min, const unsigned int max); +struct mipi_dsi_host *drm_of_get_dsi_bus(struct device *dev, + struct mipi_dsi_device_info *info); #else static inline uint32_t drm_of_crtc_port_mask(struct drm_device *dev, struct device_node *port) @@ -127,6 +131,12 @@ drm_of_get_data_lanes_count_ep(const struct device_node *port, { return -EINVAL; } +static inline struct +mipi_dsi_host *drm_of_get_dsi_bus(struct device *dev, + struct mipi_dsi_device_info *info) +{ + return ERR_PTR(-EINVAL); +} #endif /*