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.brezillon@xxxxxxxxxxxxx>
> ---
> 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
