[PATCH v5 2/2] drm/bridge: lvds-codec: Add support for LVDS data mapping select

[Sam Ravnborg]

Hi Laurent,

> > > +
> > > +	/*
> > > +	 * Decoder input LVDS format is a property of the decoder chip or even
> > > +	 * its strapping. Handle data-mapping the same way lvds-panel does. In
> > > +	 * case data-mapping is not present, do nothing, since there are still
> > > +	 * legacy bindings which do not specify this property.
> >
> > The missing data-mapping property is reported as an error, but this
> > comments says it is OK. Info?
> 
> It's not a fatal error, probe still continues in backward-compatibility
> mode. The message is there to tell that the DT should be updated. Would
> you downgrade that to a warning ?
Warning would IMO be better as this is not an error that stops it from
working.

> 
> > > +	 */
> > > +	if (lvds_codec->connector_type != DRM_MODE_CONNECTOR_LVDS) {
> > > +		bus_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> >
> > Are there any reg specified in the binding? If not then the second
> > parameter should be -1 here.
> 
> Yes, the DT node has two ports, with port 0 being the input. For LVDS
> decoders, that's where the LVDS bus is.
OK.

> 
> > > +		if (!bus_node) {
> > > +			dev_dbg(dev, "bus DT node not found\n");
> > > +			return -ENXIO;
> > > +		}
> > > +
> > > +		ret = of_property_read_string(bus_node, "data-mapping",
> > > +					      &mapping);
> > > +		of_node_put(bus_node);
> > > +		if (ret < 0) {
> > > +			dev_err(dev, "missing 'data-mapping' DT property\n");
> > > +		} else {
> >
> > It would be nice with a helper for the below code if we need this a third
> > time.
> 
> Where would you store it ?
drm_connector.c seems to be a good place.
Or maybe a static inline in drm_connector.h.
> 
> > > +			if (!strcmp(mapping, "jeida-18")) {
> > > +				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
> > > +			} else if (!strcmp(mapping, "jeida-24")) {
> > > +				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> > > +			} else if (!strcmp(mapping, "vesa-24")) {
> > > +				lvds_codec->bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> > > +			} else {
> > > +				dev_err(dev, "invalid 'data-mapping' DT property\n");
> > > +				return -EINVAL;
> > > +			}

	Sam