Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Laurent Pinchart]

Hi Jacopo,

On Monday, 23 April 2018 10:41:56 EEST jacopo mondi wrote:
> On Sun, Apr 22, 2018 at 10:02:51PM +0200, Peter Rosin wrote:
> > On 2018-04-19 11:31, Jacopo Mondi wrote:
> >> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> >> modes, selectable by means of an external pin.
> >> 
> >> Add support for configurable LVDS input mapping modes, using the newly
> >> introduced support for bridge input image formats.
> >> 
> >> Signed-off-by: Jacopo Mondi 
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 
> >>  1 file changed, 41 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> b/drivers/gpu/drm/bridge/thc63lvd1024.c index 48527f8..a3071a1 100644
> >> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> >> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c

[snip]

> >> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> >> +{
> >> +  u32 bus_fmt;
> >> +  u32 map;
> >> +  int ret;
> >> +
> >> +  ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
> >> +  if (ret) {
> >> +  dev_err(thc63->dev,
> >> +  "Unable to parse property \"thine,map\": %d\n", ret);
> >> +  return ret;
> >> +  }
> >> +
> >> +  switch (map) {
> >> +  case THC63_LVDS_MAP_MODE1:
> >> +  bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> >> +  break;
> >> +  case THC63_LVDS_MAP_MODE2:
> >> +  bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> > 
> > Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
> > or rgb101010/1x7x5, no?
> 
> I should combine the 'map' pin input mode property with the 'bus_width' one
> to find that out probably.

Yes, but that could also be left for later, when the need to support those 
formats arise, especially given that include/uapi/linux/media-bus-format.h has 
no 1x7x5 formats yet.

> >> +  break;
> >> +  default:
> >> +  dev_err(thc63->dev,
> >> +  "Invalid value for property \"thine,map\": %u\n", map);
> >> +  return -EINVAL;
> >> +  }
> >> +
> >> +  drm_bridge_set_bus_formats(>bridge, _fmt, 1);
> >> +
> >> +  return 0;
> >> +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thursday, 19 April 2018 12:31:04 EEST Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> modes, selectable by means of an external pin.
> 
> Add support for configurable LVDS input mapping modes, using the newly
> introduced support for bridge input image formats.
> 
> Signed-off-by: Jacopo Mondi 
> ---
> drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +
> 1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c
> b/drivers/gpu/drm/bridge/thc63lvd1024.c index 48527f8..a3071a1 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -10,9 +10,15 @@
>  #include 
> 
>  #include 
> +#include 
>  #include 
>  #include 
> 
> +enum thc63_lvds_mapping_mode {
> + THC63_LVDS_MAP_MODE2,
> + THC63_LVDS_MAP_MODE1,
> +};
> +
>  enum thc63_ports {
>   THC63_LVDS_IN0,
>   THC63_LVDS_IN1,
> @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>   return 0;
>  }
> 
> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> +{
> + u32 bus_fmt;
> + u32 map;
> + int ret;
> +
> + ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
> + if (ret) {
> + dev_err(thc63->dev,
> + "Unable to parse property \"thine,map\": %d\n", ret);
> + return ret;

Given that the property is currently not defined, I think you should select a 
default instead of returning an error to preserve backward compatibility. You 
can print a warning message to get the DT updated, maybe something like

u32 map = 0;
...

ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
if (ret)
dev_warn(thc63->dev,
 "Unable to parse thine,map property (%d), defaulting 
to 0\n",
 ret);

> + }
> +
> + switch (map) {
> + case THC63_LVDS_MAP_MODE1:
> + bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> + break;
> + case THC63_LVDS_MAP_MODE2:
> + bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> + break;
> + default:
> + dev_err(thc63->dev,
> + "Invalid value for property \"thine,map\": %u\n", map);
> + return -EINVAL;

This is fin as invalid property values should be rejected.

> + }
> +
> + drm_bridge_set_bus_formats(>bridge, _fmt, 1);
> +
> + return 0;
> +}
> +
>  static int thc63_gpio_init(struct thc63_dev *thc63)
>  {
>   thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
>   if (ret)
>   return ret;
> 
> + ret = thc63_set_bus_fmt(thc63);
> + if (ret)
> + return ret;
> +

Or you could move the code to thc63_parse_dt() as you're parsing DT.

>   thc63->bridge.driver_private = thc63;
>   thc63->bridge.of_node = pdev->dev.of_node;
>   thc63->bridge.funcs = _bridge_func;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[jacopo mondi]

Hi Peter,

On Sun, Apr 22, 2018 at 10:02:51PM +0200, Peter Rosin wrote:
> On 2018-04-19 11:31, Jacopo Mondi wrote:
> > The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> > modes, selectable by means of an external pin.
> >
> > Add support for configurable LVDS input mapping modes, using the newly
> > introduced support for bridge input image formats.
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 
> > +++
> >  1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> > b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > index 48527f8..a3071a1 100644
> > --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -10,9 +10,15 @@
> >  #include 
> >
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >
> > +enum thc63_lvds_mapping_mode {
> > +   THC63_LVDS_MAP_MODE2,
> > +   THC63_LVDS_MAP_MODE1,
> > +};
> > +
> >  enum thc63_ports {
> > THC63_LVDS_IN0,
> > THC63_LVDS_IN1,
> > @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
> > return 0;
> >  }
> >
> > +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> > +{
> > +   u32 bus_fmt;
> > +   u32 map;
> > +   int ret;
> > +
> > +   ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
> > +   if (ret) {
> > +   dev_err(thc63->dev,
> > +   "Unable to parse property \"thine,map\": %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   switch (map) {
> > +   case THC63_LVDS_MAP_MODE1:
> > +   bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> > +   break;
> > +   case THC63_LVDS_MAP_MODE2:
> > +   bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
>
> Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
> or rgb101010/1x7x5, no?

I should combine the 'map' pin input mode property with the 'bus_width' one
to find that out probably.

Thanks
   j


>
> Cheers,
> Peter
>
> > +   break;
> > +   default:
> > +   dev_err(thc63->dev,
> > +   "Invalid value for property \"thine,map\": %u\n", map);
> > +   return -EINVAL;
> > +   }
> > +
> > +   drm_bridge_set_bus_formats(>bridge, _fmt, 1);
> > +
> > +   return 0;
> > +}
> > +
> >  static int thc63_gpio_init(struct thc63_dev *thc63)
> >  {
> > thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> > @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > +   ret = thc63_set_bus_fmt(thc63);
> > +   if (ret)
> > +   return ret;
> > +
> > thc63->bridge.driver_private = thc63;
> > thc63->bridge.of_node = pdev->dev.of_node;
> > thc63->bridge.funcs = _bridge_func;
> >
>


signature.asc
Description: PGP signature

Re: [PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Peter Rosin]

On 2018-04-19 11:31, Jacopo Mondi wrote:
> The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
> modes, selectable by means of an external pin.
> 
> Add support for configurable LVDS input mapping modes, using the newly
> introduced support for bridge input image formats.
> 
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 41 
> +++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> index 48527f8..a3071a1 100644
> --- a/drivers/gpu/drm/bridge/thc63lvd1024.c
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -10,9 +10,15 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  
> +enum thc63_lvds_mapping_mode {
> + THC63_LVDS_MAP_MODE2,
> + THC63_LVDS_MAP_MODE1,
> +};
> +
>  enum thc63_ports {
>   THC63_LVDS_IN0,
>   THC63_LVDS_IN1,
> @@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
>   return 0;
>  }
>  
> +static int thc63_set_bus_fmt(struct thc63_dev *thc63)
> +{
> + u32 bus_fmt;
> + u32 map;
> + int ret;
> +
> + ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
> + if (ret) {
> + dev_err(thc63->dev,
> + "Unable to parse property \"thine,map\": %d\n", ret);
> + return ret;
> + }
> +
> + switch (map) {
> + case THC63_LVDS_MAP_MODE1:
> + bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> + break;
> + case THC63_LVDS_MAP_MODE2:
> + bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;

Why do you assume rgb888/1x7x4 here? It might as well be rgb666/1x7x3
or rgb101010/1x7x5, no?

Cheers,
Peter

> + break;
> + default:
> + dev_err(thc63->dev,
> + "Invalid value for property \"thine,map\": %u\n", map);
> + return -EINVAL;
> + }
> +
> + drm_bridge_set_bus_formats(>bridge, _fmt, 1);
> +
> + return 0;
> +}
> +
>  static int thc63_gpio_init(struct thc63_dev *thc63)
>  {
>   thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> @@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
>   if (ret)
>   return ret;
>  
> + ret = thc63_set_bus_fmt(thc63);
> + if (ret)
> + return ret;
> +
>   thc63->bridge.driver_private = thc63;
>   thc63->bridge.of_node = pdev->dev.of_node;
>   thc63->bridge.funcs = _bridge_func;
> 

[PATCH 3/8] drm: bridge: thc63lvd1024: Add support for LVDS mode map

[Jacopo Mondi]

The THC63LVD1024 LVDS to RGB bridge supports two different LVDS mapping
modes, selectable by means of an external pin.

Add support for configurable LVDS input mapping modes, using the newly
introduced support for bridge input image formats.

Signed-off-by: Jacopo Mondi 
---
 drivers/gpu/drm/bridge/thc63lvd1024.c | 41 +++
 1 file changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
b/drivers/gpu/drm/bridge/thc63lvd1024.c
index 48527f8..a3071a1 100644
--- a/drivers/gpu/drm/bridge/thc63lvd1024.c
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -10,9 +10,15 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 
+enum thc63_lvds_mapping_mode {
+   THC63_LVDS_MAP_MODE2,
+   THC63_LVDS_MAP_MODE1,
+};
+
 enum thc63_ports {
THC63_LVDS_IN0,
THC63_LVDS_IN1,
@@ -116,6 +122,37 @@ static int thc63_parse_dt(struct thc63_dev *thc63)
return 0;
 }
 
+static int thc63_set_bus_fmt(struct thc63_dev *thc63)
+{
+   u32 bus_fmt;
+   u32 map;
+   int ret;
+
+   ret = of_property_read_u32(thc63->dev->of_node, "thine,map", );
+   if (ret) {
+   dev_err(thc63->dev,
+   "Unable to parse property \"thine,map\": %d\n", ret);
+   return ret;
+   }
+
+   switch (map) {
+   case THC63_LVDS_MAP_MODE1:
+   bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
+   break;
+   case THC63_LVDS_MAP_MODE2:
+   bus_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
+   break;
+   default:
+   dev_err(thc63->dev,
+   "Invalid value for property \"thine,map\": %u\n", map);
+   return -EINVAL;
+   }
+
+   drm_bridge_set_bus_formats(>bridge, _fmt, 1);
+
+   return 0;
+}
+
 static int thc63_gpio_init(struct thc63_dev *thc63)
 {
thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
@@ -166,6 +203,10 @@ static int thc63_probe(struct platform_device *pdev)
if (ret)
return ret;
 
+   ret = thc63_set_bus_fmt(thc63);
+   if (ret)
+   return ret;
+
thc63->bridge.driver_private = thc63;
thc63->bridge.of_node = pdev->dev.of_node;
thc63->bridge.funcs = _bridge_func;
-- 
2.7.4