Hi Fabrizio,
Thank you for the patch.
On Wed, Nov 13, 2019 at 03:51:31PM +0000, Fabrizio Castro wrote:
> The lvds-codec driver is a generic stub for transparent LVDS
> encoders and decoders.
> It's good practice to list a device specific compatible string
s/good practice/mandatory/
> before the generic fallback (if any) in the DT node for the relevant
> LVDS encoder/decoder, and it's also required by the dt-bindings.
> A notable exception to the generic fallback mechanism is the case
> of "thine,thc63lvdm83d", as documented in:
> Documentation/devicetree/bindings/display/bridge/thine,thc63lvdm83d.txt
> This patch enforces the adoption of a device specific compatible
> string (as fist string in the list), by using markers for the
s/fist/first/
> compatible string we match against and the index of the matching
> compatible string in the list.
>
> Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
>
> ---
> Hi Laurent,
>
> I don't think we need to do anything in the driver to address your
> comment, as we can "enforce" this with the bindings (please see the
> next patch, as it would help with the "enforcing" of the compatible
> string for the thine device).
> I am sending this patch only so that you can see what a possible
> solution in the driver could look like.
>
> v3->v4:
> * New patch addressing the below comment from Laurent:
> "I think the lvds-decoder driver should error out at probe time if only
> one compatible string is listed."
> ---
> drivers/gpu/drm/bridge/lvds-codec.c | 55 +++++++++++++++++++++++++++++++++----
> 1 file changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index 784bbd3..145c25d 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -14,11 +14,16 @@
> #include <drm/drm_bridge.h>
> #include <drm/drm_panel.h>
>
> +struct lvds_codec_data {
> + u32 connector_type;
> + bool device_specific;
> +};
> +
> struct lvds_codec {
> struct drm_bridge bridge;
> struct drm_bridge *panel_bridge;
> struct gpio_desc *powerdown_gpio;
> - u32 connector_type;
> + const struct lvds_codec_data *data;
> };
>
> static int lvds_codec_attach(struct drm_bridge *bridge)
> @@ -65,7 +70,30 @@ static int lvds_codec_probe(struct platform_device *pdev)
> if (!lvds_codec)
> return -ENOMEM;
>
> - lvds_codec->connector_type = (u32)of_device_get_match_data(&pdev->dev);
> + lvds_codec->data = of_device_get_match_data(&pdev->dev);
> + if (!lvds_codec->data)
> + return -EINVAL;
> +
> + /*
> + * If we haven't matched a device specific compatible string, we need
> + * to work out if the generic compatible string we matched against was
> + * listed first in the compatible property.
> + */
Can't we do this unconditionally, and thus drop the lvds_codec_data
structure ?
> + if (!lvds_codec->data->device_specific) {
> + const struct of_device_id *match;
> + int compatible_index;
> +
> + match = of_match_node(dev->driver->of_match_table,
> + dev->of_node);
> + compatible_index = of_property_match_string(dev->of_node,
> + "compatible",
> + match->compatible);
> + if (compatible_index == 0) {
> + dev_err(dev, "Device specific compatible needed\n");
> + return -EINVAL;
> + }
> + }
> +
> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> GPIOD_OUT_HIGH);
> if (IS_ERR(lvds_codec->powerdown_gpio)) {
> @@ -92,7 +120,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>
> lvds_codec->panel_bridge =
> devm_drm_panel_bridge_add_typed(dev, panel,
> - lvds_codec->connector_type);
> + lvds_codec->data->connector_type);
> if (IS_ERR(lvds_codec->panel_bridge))
> return PTR_ERR(lvds_codec->panel_bridge);
>
> @@ -119,18 +147,33 @@ static int lvds_codec_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct lvds_codec_data lvds_codec_decoder_data = {
> + .connector_type = DRM_MODE_CONNECTOR_DPI,
> + .device_specific = false,
> +};
> +
> +static const struct lvds_codec_data lvds_codec_encoder_data = {
> + .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .device_specific = false,
> +};
> +
> +static const struct lvds_codec_data lvds_codec_thc63lvdm83d_data = {
> + .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .device_specific = true,
> +};
> +
> static const struct of_device_id lvds_codec_match[] = {
> {
> .compatible = "lvds-decoder",
> - .data = (void *)DRM_MODE_CONNECTOR_DPI,
> + .data = &lvds_codec_decoder_data,
> },
> {
> .compatible = "lvds-encoder",
> - .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> + .data = &lvds_codec_encoder_data,
> },
> {
> .compatible = "thine,thc63lvdm83d",
> - .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> + .data = &lvds_codec_thc63lvdm83d_data,
> },
> {},
> };
--
Regards,
Laurent Pinchart
