Hi Boris,

Thank you for the patch.

On Wed, Oct 23, 2019 at 05:45:07PM +0200, Boris Brezillon wrote:
> Some LVDS encoder might support several input/output bus formats. Add

s/encoder/encoders/

> a way to describe the one used on a specific design by adding optional

s/the one/the format/

> 'data-mapping' properties to the input/output ports.
> 
> Signed-off-by: Boris Brezillon <boris.brezil...@collabora.com>
> ---
> Changes in v3:
> * Use bus-width for the rgb24/rgb18 distinction
> * Adjust code to match core changes
> * Get rid of of_get_data_mapping()
> * Don't implement ->atomic_check() (the core now takes care of bus
>   flags propagation)
> 
> Changes in v2:
> * Make the bus-format negotiation logic more generic
> ---
>  drivers/gpu/drm/bridge/lvds-encoder.c | 72 +++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/lvds-encoder.c 
> b/drivers/gpu/drm/bridge/lvds-encoder.c
> index e2132a8d5106..a2a8f7f4ac97 100644
> --- a/drivers/gpu/drm/bridge/lvds-encoder.c
> +++ b/drivers/gpu/drm/bridge/lvds-encoder.c
> @@ -6,6 +6,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/platform_device.h>
>  
> @@ -16,6 +17,8 @@ struct lvds_encoder {
>       struct drm_bridge bridge;
>       struct drm_bridge *panel_bridge;
>       struct gpio_desc *powerdown_gpio;
> +     u32 output_fmt;
> +     u32 input_fmt;
>  };
>  
>  static int lvds_encoder_attach(struct drm_bridge *bridge)
> @@ -48,10 +51,40 @@ static void lvds_encoder_disable(struct drm_bridge 
> *bridge)
>               gpiod_set_value_cansleep(lvds_encoder->powerdown_gpio, 1);
>  }
>  
> +static u32 *lvds_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> +                                        struct drm_bridge_state 
> *bridge_state,
> +                                        struct drm_crtc_state *crtc_state,
> +                                        struct drm_connector_state 
> *conn_state,
> +                                        u32 output_fmt,
> +                                        unsigned int *num_input_fmts)
> +{
> +     struct lvds_encoder *lvds_encoder = container_of(bridge,
> +                                                      struct lvds_encoder,
> +                                                      bridge);
> +     u32 *input_fmts;
> +
> +     if (output_fmt == MEDIA_BUS_FMT_FIXED ||
> +         output_fmt == lvds_encoder->output_fmt)
> +             *num_input_fmts = 1;
> +     else
> +             *num_input_fmts = 0;
> +
> +     if (!*num_input_fmts)
> +             return NULL;
> +
> +     input_fmts = kcalloc(*num_input_fmts, sizeof(*input_fmts), GFP_KERNEL);

As *num_input_fmts == 1, you can

        input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);

> +     if (!input_fmts)
> +             return NULL;
> +
> +     input_fmts[0] = lvds_encoder->input_fmt;
> +     return input_fmts;
> +}
> +
>  static struct drm_bridge_funcs funcs = {
>       .attach = lvds_encoder_attach,
>       .enable = lvds_encoder_enable,
>       .disable = lvds_encoder_disable,
> +     .atomic_get_input_bus_fmts = lvds_atomic_get_input_bus_fmts,
>  };
>  
>  static int lvds_encoder_probe(struct platform_device *pdev)
> @@ -62,11 +95,16 @@ static int lvds_encoder_probe(struct platform_device 
> *pdev)
>       struct device_node *panel_node;
>       struct drm_panel *panel;
>       struct lvds_encoder *lvds_encoder;
> +     const char *output_data_mapping = NULL;
> +     u32 input_bus_width = 0;
>  
>       lvds_encoder = devm_kzalloc(dev, sizeof(*lvds_encoder), GFP_KERNEL);
>       if (!lvds_encoder)
>               return -ENOMEM;
>  
> +     lvds_encoder->input_fmt = MEDIA_BUS_FMT_FIXED;
> +     lvds_encoder->output_fmt = MEDIA_BUS_FMT_FIXED;
> +
>       lvds_encoder->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>                                                              GPIOD_OUT_HIGH);
>       if (IS_ERR(lvds_encoder->powerdown_gpio)) {
> @@ -77,6 +115,25 @@ static int lvds_encoder_probe(struct platform_device 
> *pdev)
>               return err;
>       }
>  
> +     port = of_graph_get_port_by_id(dev->of_node, 0);
> +     if (!port) {
> +             dev_dbg(dev, "port 0 not found\n");
> +             return -ENXIO;
> +     }
> +
> +     of_node_put(port);
> +
> +     if (of_property_read_u32(port, "bus-width", &input_bus_width)) {

You're using port after calling of_node_put().

> +             lvds_encoder->input_fmt = MEDIA_BUS_FMT_FIXED;
> +     } else if (input_bus_width == 18) {
> +             lvds_encoder->input_fmt = MEDIA_BUS_FMT_RGB666_1X18;
> +     } else if (input_bus_width == 24) {
> +             lvds_encoder->input_fmt = MEDIA_BUS_FMT_RGB888_1X24;
> +     } else {
> +             dev_dbg(dev, "unsupported bus-width)\n");

s/bus-width)/bus-width/

but I think you should make the error message a bit clearer

                dev_dbg(dev, "unsupported bus-width value %u on port 0\n",
                        input_bus_width);

> +             return -ENOTSUPP;
> +     }
> +
>       /* Locate the panel DT node. */
>       port = of_graph_get_port_by_id(dev->of_node, 1);
>       if (!port) {
> @@ -84,6 +141,21 @@ static int lvds_encoder_probe(struct platform_device 
> *pdev)
>               return -ENXIO;
>       }
>  
> +     of_property_read_string(port, "data-mapping", &output_data_mapping);
> +     if (!output_data_mapping) {
> +             lvds_encoder->output_fmt = MEDIA_BUS_FMT_FIXED;
> +     } else if (!strcmp(output_data_mapping, "jeida-18")) {
> +             lvds_encoder->output_fmt = MEDIA_BUS_FMT_RGB666_1X7X3_SPWG;
> +     } else if (!strcmp(output_data_mapping, "jeida-24")) {
> +             lvds_encoder->output_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA;
> +     } else if (!strcmp(output_data_mapping, "vesa-24")) {
> +             lvds_encoder->output_fmt = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG;
> +     } else {
> +             of_node_put(port);
> +             dev_dbg(dev, "unsupported output data-mapping\n");
> +             return -ENOTSUPP;
> +     }
> +
>       endpoint = of_get_child_by_name(port, "endpoint");
>       of_node_put(port);
>       if (!endpoint) {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to