Re: [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Maxime,

On Wed, Nov 18, 2020 at 06:48:05PM +0100, Maxime Ripard wrote:
> On Mon, Oct 12, 2020 at 02:00:30AM +0300, Laurent Pinchart wrote:
> > > -static int drm_of_lvds_get_remote_pixels_type(
> > > -			const struct device_node *port_node)
> > > +static int drm_of_lvds_get_remote_pixels_type(const struct device_node *endpoint)
> > >  {
> > > -	struct device_node *endpoint = NULL;
> > > -	int pixels_type = -EPIPE;
> > > +	struct device_node *remote_port;
> > > +	int pixels_type;
> > >  
> > > -	for_each_child_of_node(port_node, endpoint) {
> > > -		struct device_node *remote_port;
> > > -		int current_pt;
> > > -
> > > -		if (!of_node_name_eq(endpoint, "endpoint"))
> > > -			continue;
> > > -
> > > -		remote_port = of_graph_get_remote_port(endpoint);
> > > -		if (!remote_port) {
> > > -			of_node_put(remote_port);
> > > -			return -EPIPE;
> > > -		}
> > > -
> > > -		current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
> > > +	remote_port = of_graph_get_remote_port(endpoint);
> > > +	if (!remote_port) {
> > >  		of_node_put(remote_port);
> > 
> > You can drop this line.
> > 
> > > -		if (pixels_type < 0)
> > > -			pixels_type = current_pt;
> > > -
> > > -		/*
> > > -		 * Sanity check, ensure that all remote endpoints have the same
> > > -		 * pixel type. We may lift this restriction later if we need to
> > > -		 * support multiple sinks with different dual-link
> > > -		 * configurations by passing the endpoints explicitly to
> > > -		 * drm_of_lvds_get_dual_link_pixel_order().
> > > -		 */
> > 
> > Shouldn't we keep this check when endpoint_id is -1 in
> > drm_of_lvds_get_dual_link_pixel_order() ?
> 
> I tried to do that, and I'm not quite really sure how to go around it.
> 
> This scans all the endpoints in a given port.
> 
> However, now that we have the device, port id and endpoint id, we need
> to use of_graph_get_port_by_id to get the port matching the device and
> port id, and iterate over all its endpoint as done here.
> 
> The trouble is that of_graph_get_port_by_id expects a !const
> device_node, yet drm_of_lvds_get_dual_link_pixel_order (and seems to be
> doing so rightfully), so that creates a warning because we drop the
> const there.

of_graph_get_port_by_id() doesn't seem to modify the device_node passed
to it, couldn't it be modified to take a const pointer ?

> Changing the prototype to passing only the port device_node doesn't
> really work either, since it would be const, and we would need to call
> of_graph_get_endpoint_by_regs (so having the parent device_node, through
> of_graph_get_port_parent) and of_graph_get_port_parent takes a !const
> port device_node.
> 
> I guess we could drop const entirely from our function, but that doesn't
> look right either..

-- 
Regards,

Laurent Pinchart