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

Reply via email to