From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>,
David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
Rob Herring <robh+dt@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Andrzej Hajda <a.hajda@samsung.com>,
Simon Horman <horms@verge.net.au>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Magnus Damm <magnus.damm@gmail.com>,
Peter Rosin <peda@axentia.se>,
dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
linux-renesas-soc@vger.kernel.org,
Chris Paterson <Chris.Paterson2@renesas.com>,
Biju Das <biju.das@bp.renesas.com>,
Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
Jacopo Mondi <jacopo+renesas@jmondi.org>
Subject: Re: [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings
Date: Tue, 19 Nov 2019 02:16:16 +0200 [thread overview]
Message-ID: <20191119001616.GL5171@pendragon.ideasonboard.com> (raw)
In-Reply-To: <1573660292-10629-13-git-send-email-fabrizio.castro@bp.renesas.com>
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@bp.renesas.com>
>
> ---
> 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
next prev parent reply other threads:[~2019-11-19 0:16 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-13 15:51 [PATCH v4 00/13] Add LCD panel support to iwg20d Fabrizio Castro
2019-11-13 15:51 ` [PATCH v4 01/13] dt-bindings: display: bridge: Convert lvds-transmitter binding to json-schema Fabrizio Castro
2019-11-18 21:35 ` Rob Herring
2019-11-18 23:51 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 02/13] dt-bindings: display: bridge: lvds-transmitter: Document powerdown-gpios Fabrizio Castro
2019-11-18 21:36 ` Rob Herring
2019-11-18 23:55 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 03/13] dt-bindings: display: bridge: lvds-transmitter: Absorb ti,ds90c185.txt Fabrizio Castro
2019-11-18 21:38 ` Rob Herring
2019-11-18 23:56 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 04/13] dt-bindings: display: bridge: lvds-transmitter: Document "ti,sn75lvds83" Fabrizio Castro
2019-11-18 21:38 ` Rob Herring
2019-11-18 23:57 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 05/13] drm/bridge: Repurpose lvds-encoder.c Fabrizio Castro
2019-11-19 0:00 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 06/13] drm/bridge: lvds-codec: Add "lvds-decoder" support Fabrizio Castro
2019-11-19 0:02 ` Laurent Pinchart
2019-12-13 17:10 ` Laurent Pinchart
2019-12-16 21:22 ` Laurent Pinchart
2019-12-17 11:02 ` Fabrizio Castro
2019-12-17 12:21 ` Geert Uytterhoeven
2019-12-17 12:31 ` Fabrizio Castro
2019-12-17 12:38 ` Geert Uytterhoeven
2019-12-17 13:54 ` Laurent Pinchart
2019-12-17 22:06 ` Fabrizio Castro
2019-12-17 23:08 ` Laurent Pinchart
2019-12-18 12:00 ` Fabrizio Castro
2019-11-13 15:51 ` [PATCH v4 07/13] drm/bridge: lvds-codec: Simplify panel DT node localisation Fabrizio Castro
2019-11-19 0:03 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 08/13] dt-bindings: display: bridge: Repurpose lvds-encoder Fabrizio Castro
2019-11-18 21:40 ` Rob Herring
2019-11-19 0:07 ` Laurent Pinchart
2019-11-19 10:35 ` Fabrizio Castro
2019-11-13 15:51 ` [PATCH v4 09/13] dt-bindings: display: bridge: lvds-codec: Document ti,ds90cf384a Fabrizio Castro
2019-11-18 21:41 ` Rob Herring
2019-11-19 0:09 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 10/13] ARM: dts: iwg20d-q7-common: Add LCD support Fabrizio Castro
2019-11-19 0:10 ` Laurent Pinchart
2019-11-25 10:30 ` Geert Uytterhoeven
2019-11-13 15:51 ` [PATCH v4 11/13] ARM: shmobile_defconfig: Enable support for panels from EDT Fabrizio Castro
2019-11-19 0:12 ` Laurent Pinchart
2019-11-25 10:30 ` Geert Uytterhoeven
2019-11-13 15:51 ` [PATCH v4 12/13] [HACK] drm/bridge: lvds-codec: Enforce device specific compatible strings Fabrizio Castro
2019-11-19 0:16 ` Laurent Pinchart [this message]
2019-11-19 11:17 ` Fabrizio Castro
2019-11-19 21:51 ` Laurent Pinchart
2019-11-21 16:00 ` Fabrizio Castro
2019-11-22 8:16 ` Geert Uytterhoeven
2019-11-25 11:17 ` Fabrizio Castro
2019-12-02 9:42 ` Laurent Pinchart
2019-11-13 15:51 ` [PATCH v4 13/13] [HACK] dt-bindings: display: bridge: lvds-codec: Absorb thine,thc63lvdm83d.txt Fabrizio Castro
2019-11-18 21:51 ` Rob Herring
2019-11-19 0:19 ` Laurent Pinchart
2019-12-18 11:52 ` [PATCH v4 00/13] Add LCD panel support to iwg20d Neil Armstrong
2019-12-18 14:05 ` Fabrizio Castro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191119001616.GL5171@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=Chris.Paterson2@renesas.com \
--cc=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=biju.das@bp.renesas.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=fabrizio.castro@bp.renesas.com \
--cc=geert+renesas@glider.be \
--cc=horms@verge.net.au \
--cc=jacopo+renesas@jmondi.org \
--cc=kieran.bingham+renesas@ideasonboard.com \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=magnus.damm@gmail.com \
--cc=mark.rutland@arm.com \
--cc=narmstrong@baylibre.com \
--cc=peda@axentia.se \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).