* [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
@ 2020-12-24 6:18 Marek Vasut
2021-01-24 17:04 ` Marek Vasut
2021-03-22 1:14 ` Laurent Pinchart
0 siblings, 2 replies; 8+ messages in thread
From: Marek Vasut @ 2020-12-24 6:18 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, Benjamin Gaignard, Antonio Borneo, Vincent Abriou,
Philippe Cornu, Yannick Fertre, Andrzej Hajda, Laurent Pinchart,
Maxime Coquelin, Biju Das, Sam Ravnborg, linux-stm32,
linux-arm-kernel, Alexandre Torgue
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to
select input pixel data sampling edge. Add DT property "pixelclk-active",
same as the one used by display timings, and configure bus flags based on
this DT property.
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Antonio Borneo <antonio.borneo@st.com>
Cc: Benjamin Gaignard <benjamin.gaignard@st.com>
Cc: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Philippe Cornu <philippe.cornu@st.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Vincent Abriou <vincent.abriou@st.com>
Cc: Yannick Fertre <yannick.fertre@st.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-stm32@st-md-mailman.stormreply.com
To: dri-devel@lists.freedesktop.org
---
V2: - Limit the pixelclk-active to encoders only
- Update DT binding document
---
.../bindings/display/bridge/lvds-codec.yaml | 7 +++
drivers/gpu/drm/bridge/lvds-codec.c | 52 +++++++++++++------
2 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
index e5e3c72630cf..399a6528780a 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -74,6 +74,13 @@ properties:
additionalProperties: false
+ pixelclk-active:
+ description: |
+ Data sampling on rising or falling edge.
+ Use 0 to sample pixel data on rising edge and
+ Use 1 to sample pixel data on falling edge and
+ enum: [0, 1]
+
powerdown-gpios:
description:
The GPIO used to control the power down line of this device.
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
index dcf579a4cf83..cab81ccd895d 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -15,13 +15,18 @@
#include <drm/drm_bridge.h>
#include <drm/drm_panel.h>
+struct lvds_codec_data {
+ u32 connector_type;
+ bool is_encoder;
+};
+
struct lvds_codec {
struct device *dev;
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct drm_bridge_timings timings;
struct regulator *vcc;
struct gpio_desc *powerdown_gpio;
- u32 connector_type;
};
static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge)
@@ -76,17 +81,20 @@ static const struct drm_bridge_funcs funcs = {
static int lvds_codec_probe(struct platform_device *pdev)
{
+ const struct lvds_codec_data *data;
struct device *dev = &pdev->dev;
struct device_node *panel_node;
struct drm_panel *panel;
struct lvds_codec *lvds_codec;
+ u32 val;
lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
if (!lvds_codec)
return -ENOMEM;
+ data = of_device_get_match_data(dev);
+
lvds_codec->dev = &pdev->dev;
- lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->vcc = devm_regulator_get(lvds_codec->dev, "power");
if (IS_ERR(lvds_codec->vcc))
@@ -115,10 +123,22 @@ 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);
+ data->connector_type);
if (IS_ERR(lvds_codec->panel_bridge))
return PTR_ERR(lvds_codec->panel_bridge);
+ /*
+ * Encoder might sample data on different clock edge than the display,
+ * for example OnSemi FIN3385 has a dedicated strapping pin to select
+ * the sampling edge.
+ */
+ if (data->is_encoder &&
+ !of_property_read_u32(dev->of_node, "pixelclk-active", &val)) {
+ lvds_codec->timings.input_bus_flags = val ?
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE :
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;
+ }
+
/*
* The panel_bridge bridge is attached to the panel's of_node,
* but we need a bridge attached to our of_node for our user
@@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
*/
lvds_codec->bridge.of_node = dev->of_node;
lvds_codec->bridge.funcs = &funcs;
+ lvds_codec->bridge.timings = &lvds_codec->timings;
drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
@@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev)
return 0;
}
+static const struct lvds_codec_data decoder_data = {
+ .connector_type = DRM_MODE_CONNECTOR_DPI,
+ .is_encoder = false,
+};
+
+static const struct lvds_codec_data encoder_data = {
+ .connector_type = DRM_MODE_CONNECTOR_LVDS,
+ .is_encoder = true,
+};
+
static const struct of_device_id lvds_codec_match[] = {
- {
- .compatible = "lvds-decoder",
- .data = (void *)DRM_MODE_CONNECTOR_DPI,
- },
- {
- .compatible = "lvds-encoder",
- .data = (void *)DRM_MODE_CONNECTOR_LVDS,
- },
- {
- .compatible = "thine,thc63lvdm83d",
- .data = (void *)DRM_MODE_CONNECTOR_LVDS,
- },
+ { .compatible = "lvds-decoder", .data = &decoder_data, },
+ { .compatible = "lvds-encoder", .data = &encoder_data, },
+ { .compatible = "thine,thc63lvdm83d", .data = &encoder_data, },
{},
};
MODULE_DEVICE_TABLE(of, lvds_codec_match);
--
2.29.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2020-12-24 6:18 [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select Marek Vasut
@ 2021-01-24 17:04 ` Marek Vasut
2021-03-19 18:20 ` Marek Vasut
2021-03-22 1:14 ` Laurent Pinchart
1 sibling, 1 reply; 8+ messages in thread
From: Marek Vasut @ 2021-01-24 17:04 UTC (permalink / raw)
To: dri-devel
Cc: Alexandre Torgue, Antonio Borneo, Vincent Abriou, Philippe Cornu,
Yannick Fertre, Andrzej Hajda, Laurent Pinchart, Maxime Coquelin,
Biju Das, Sam Ravnborg, linux-stm32, linux-arm-kernel,
Benjamin Gaignard
On 12/24/20 7:18 AM, Marek Vasut wrote:
> The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to
> select input pixel data sampling edge. Add DT property "pixelclk-active",
> same as the one used by display timings, and configure bus flags based on
> this DT property.
Bump ?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2021-01-24 17:04 ` Marek Vasut
@ 2021-03-19 18:20 ` Marek Vasut
0 siblings, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2021-03-19 18:20 UTC (permalink / raw)
To: dri-devel
Cc: Alexandre Torgue, Andrzej Hajda, Antonio Borneo,
Benjamin Gaignard, Biju Das, Laurent Pinchart, Maxime Coquelin,
Philippe Cornu, Sam Ravnborg, Vincent Abriou, Yannick Fertre,
linux-arm-kernel, linux-stm32
On 1/24/21 6:04 PM, Marek Vasut wrote:
> On 12/24/20 7:18 AM, Marek Vasut wrote:
>> The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to
>> select input pixel data sampling edge. Add DT property "pixelclk-active",
>> same as the one used by display timings, and configure bus flags based on
>> this DT property.
>
> Bump ?
Any news on this patch ?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2020-12-24 6:18 [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select Marek Vasut
2021-01-24 17:04 ` Marek Vasut
@ 2021-03-22 1:14 ` Laurent Pinchart
2021-03-22 10:29 ` Marek Vasut
1 sibling, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2021-03-22 1:14 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Alexandre Torgue, Andrzej Hajda, Antonio Borneo,
Benjamin Gaignard, Biju Das, Maxime Coquelin, Philippe Cornu,
Sam Ravnborg, Vincent Abriou, Yannick Fertre, linux-arm-kernel,
linux-stm32
Hi Marek,
All my apologies for the awfully delayed review, and thank you for
pinging me.
On Thu, Dec 24, 2020 at 07:18:32AM +0100, Marek Vasut wrote:
> The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to
> select input pixel data sampling edge. Add DT property "pixelclk-active",
> same as the one used by display timings, and configure bus flags based on
> this DT property.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Antonio Borneo <antonio.borneo@st.com>
> Cc: Benjamin Gaignard <benjamin.gaignard@st.com>
> Cc: Biju Das <biju.das.jz@bp.renesas.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Philippe Cornu <philippe.cornu@st.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: Yannick Fertre <yannick.fertre@st.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: dri-devel@lists.freedesktop.org
> ---
> V2: - Limit the pixelclk-active to encoders only
> - Update DT binding document
> ---
> .../bindings/display/bridge/lvds-codec.yaml | 7 +++
> drivers/gpu/drm/bridge/lvds-codec.c | 52 +++++++++++++------
> 2 files changed, 44 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> index e5e3c72630cf..399a6528780a 100644
> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> @@ -74,6 +74,13 @@ properties:
>
> additionalProperties: false
>
> + pixelclk-active:
> + description: |
> + Data sampling on rising or falling edge.
> + Use 0 to sample pixel data on rising edge and
> + Use 1 to sample pixel data on falling edge and
> + enum: [0, 1]
The idea is good, but instead of adding a custom property, how about
reusing the pclk-sample property defined in
../../media/video-interfaces.yaml ?
The property is only valid for encoders, so I would at least mention
that in the description, or, better, handle this based on the compatible
string to allow validation.
> +
> powerdown-gpios:
> description:
> The GPIO used to control the power down line of this device.
> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> index dcf579a4cf83..cab81ccd895d 100644
> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> @@ -15,13 +15,18 @@
> #include <drm/drm_bridge.h>
> #include <drm/drm_panel.h>
>
> +struct lvds_codec_data {
> + u32 connector_type;
> + bool is_encoder;
> +};
> +
> struct lvds_codec {
> struct device *dev;
> struct drm_bridge bridge;
> struct drm_bridge *panel_bridge;
> + struct drm_bridge_timings timings;
> struct regulator *vcc;
> struct gpio_desc *powerdown_gpio;
> - u32 connector_type;
> };
>
> static inline struct lvds_codec *to_lvds_codec(struct drm_bridge *bridge)
> @@ -76,17 +81,20 @@ static const struct drm_bridge_funcs funcs = {
>
> static int lvds_codec_probe(struct platform_device *pdev)
> {
> + const struct lvds_codec_data *data;
> struct device *dev = &pdev->dev;
> struct device_node *panel_node;
> struct drm_panel *panel;
> struct lvds_codec *lvds_codec;
> + u32 val;
>
> lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
> if (!lvds_codec)
> return -ENOMEM;
>
> + data = of_device_get_match_data(dev);
> +
> lvds_codec->dev = &pdev->dev;
> - lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>
> lvds_codec->vcc = devm_regulator_get(lvds_codec->dev, "power");
> if (IS_ERR(lvds_codec->vcc))
> @@ -115,10 +123,22 @@ 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);
> + data->connector_type);
> if (IS_ERR(lvds_codec->panel_bridge))
> return PTR_ERR(lvds_codec->panel_bridge);
>
> + /*
> + * Encoder might sample data on different clock edge than the display,
> + * for example OnSemi FIN3385 has a dedicated strapping pin to select
> + * the sampling edge.
> + */
> + if (data->is_encoder &&
> + !of_property_read_u32(dev->of_node, "pixelclk-active", &val)) {
> + lvds_codec->timings.input_bus_flags = val ?
> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE :
> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;
> + }
> +
> /*
> * The panel_bridge bridge is attached to the panel's of_node,
> * but we need a bridge attached to our of_node for our user
> @@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> */
> lvds_codec->bridge.of_node = dev->of_node;
> lvds_codec->bridge.funcs = &funcs;
> + lvds_codec->bridge.timings = &lvds_codec->timings;
> drm_bridge_add(&lvds_codec->bridge);
>
> platform_set_drvdata(pdev, lvds_codec);
> @@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct lvds_codec_data decoder_data = {
> + .connector_type = DRM_MODE_CONNECTOR_DPI,
> + .is_encoder = false,
The two fields are a bit redundant, as the decoder is always
LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but
maybe we could drop the connector_type field, and derive the connector
type from is_encoder ?
One may then say that we could drop the lvds_codec_data structure as it
contains a single field, but I foresee a need to have device-specific
timings at some point, so I think it's a good addition.
The patch otherwise looks good.
> +};
> +
> +static const struct lvds_codec_data encoder_data = {
> + .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .is_encoder = true,
> +};
> +
> static const struct of_device_id lvds_codec_match[] = {
> - {
> - .compatible = "lvds-decoder",
> - .data = (void *)DRM_MODE_CONNECTOR_DPI,
> - },
> - {
> - .compatible = "lvds-encoder",
> - .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> - },
> - {
> - .compatible = "thine,thc63lvdm83d",
> - .data = (void *)DRM_MODE_CONNECTOR_LVDS,
> - },
> + { .compatible = "lvds-decoder", .data = &decoder_data, },
> + { .compatible = "lvds-encoder", .data = &encoder_data, },
> + { .compatible = "thine,thc63lvdm83d", .data = &encoder_data, },
> {},
> };
> MODULE_DEVICE_TABLE(of, lvds_codec_match);
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2021-03-22 1:14 ` Laurent Pinchart
@ 2021-03-22 10:29 ` Marek Vasut
2021-03-22 10:37 ` Laurent Pinchart
2021-04-07 9:55 ` Marek Vasut
0 siblings, 2 replies; 8+ messages in thread
From: Marek Vasut @ 2021-03-22 10:29 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, Alexandre Torgue, Andrzej Hajda, Antonio Borneo,
Benjamin Gaignard, Biju Das, Maxime Coquelin, Philippe Cornu,
Sam Ravnborg, Vincent Abriou, Yannick Fertre, linux-arm-kernel,
linux-stm32
On 3/22/21 2:14 AM, Laurent Pinchart wrote:
> Hi Marek,
Hi,
[...]
>> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>> index e5e3c72630cf..399a6528780a 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>> @@ -74,6 +74,13 @@ properties:
>>
>> additionalProperties: false
>>
>> + pixelclk-active:
>> + description: |
>> + Data sampling on rising or falling edge.
>> + Use 0 to sample pixel data on rising edge and
>> + Use 1 to sample pixel data on falling edge and
>> + enum: [0, 1]
>
> The idea is good, but instead of adding a custom property, how about
> reusing the pclk-sample property defined in
> ../../media/video-interfaces.yaml ?
Repeating myself from V1 discussion ... Either is fine by me, but I
think pixelclk-active, which comes from panel-timings.yaml is closer to
the video than multimedia bindings.
> The property is only valid for encoders, so I would at least mention
> that in the description, or, better, handle this based on the compatible
> string to allow validation.
How does that work in the Yaml file ?
>> +
>> powerdown-gpios:
>> description:
>> The GPIO used to control the power down line of this device.
>> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
>> index dcf579a4cf83..cab81ccd895d 100644
>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
[...]
>> @@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
>> */
>> lvds_codec->bridge.of_node = dev->of_node;
>> lvds_codec->bridge.funcs = &funcs;
>> + lvds_codec->bridge.timings = &lvds_codec->timings;
>> drm_bridge_add(&lvds_codec->bridge);
>>
>> platform_set_drvdata(pdev, lvds_codec);
>> @@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +static const struct lvds_codec_data decoder_data = {
>> + .connector_type = DRM_MODE_CONNECTOR_DPI,
>> + .is_encoder = false,
>
> The two fields are a bit redundant, as the decoder is always
> LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but
> maybe we could drop the connector_type field, and derive the connector
> type from is_encoder ?
Or the other way around instead ? That is, if the connector_type is
LVDS, then it is encoder , otherwise its decoder ?
> One may then say that we could drop the lvds_codec_data structure as it
> contains a single field, but I foresee a need to have device-specific
> timings at some point, so I think it's a good addition.
[...]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2021-03-22 10:29 ` Marek Vasut
@ 2021-03-22 10:37 ` Laurent Pinchart
2021-04-16 6:49 ` Laurent Pinchart
2021-04-07 9:55 ` Marek Vasut
1 sibling, 1 reply; 8+ messages in thread
From: Laurent Pinchart @ 2021-03-22 10:37 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, Alexandre Torgue, Andrzej Hajda, Antonio Borneo,
Benjamin Gaignard, Biju Das, Maxime Coquelin, Philippe Cornu,
Sam Ravnborg, Vincent Abriou, Yannick Fertre, linux-arm-kernel,
linux-stm32, Rob Herring, devicetree
Hi Marek,
(CC'ing Ron and the DT mailing list for the DT discussion)
On Mon, Mar 22, 2021 at 11:29:04AM +0100, Marek Vasut wrote:
> On 3/22/21 2:14 AM, Laurent Pinchart wrote:
> > Hi Marek,
>
> Hi,
>
> [...]
>
> >> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >> index e5e3c72630cf..399a6528780a 100644
> >> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> >> @@ -74,6 +74,13 @@ properties:
> >>
> >> additionalProperties: false
> >>
> >> + pixelclk-active:
> >> + description: |
> >> + Data sampling on rising or falling edge.
> >> + Use 0 to sample pixel data on rising edge and
> >> + Use 1 to sample pixel data on falling edge and
> >> + enum: [0, 1]
> >
> > The idea is good, but instead of adding a custom property, how about
> > reusing the pclk-sample property defined in
> > ../../media/video-interfaces.yaml ?
>
> Repeating myself from V1 discussion ... Either is fine by me, but I
> think pixelclk-active, which comes from panel-timings.yaml is closer to
> the video than multimedia bindings.
That's a good point. The part that bothers me a bit is that it would be
nice to define the property in a single YAML schema, referenced by
individual bindings. video-interfaces.yaml is there for that purpose. We
could do something similar on the display side, or consider the
pixelclk-active usage in panel-timings.yaml an exception that we can't
switch to video-interfaces.yaml as backward compatibility must be
preserved.
I don't have a too strong preference, whatever Rob prefers would be fine
with me.
> > The property is only valid for encoders, so I would at least mention
> > that in the description, or, better, handle this based on the compatible
> > string to allow validation.
>
> How does that work in the Yaml file ?
Something along the lines of
if:
not:
properties:
compatible:
contains:
const: lvds-encoder
then:
properties:
pixelclk-active: false
My YAML schema foo is a bit rusty though, I apologize if this doesn't
work as-is. There are lots of similar examples in DT bindings that
should hopefully be right :-)
> >> +
> >> powerdown-gpios:
> >> description:
> >> The GPIO used to control the power down line of this device.
> >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> >> index dcf579a4cf83..cab81ccd895d 100644
> >> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
>
> [...]
>
> >> @@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> >> */
> >> lvds_codec->bridge.of_node = dev->of_node;
> >> lvds_codec->bridge.funcs = &funcs;
> >> + lvds_codec->bridge.timings = &lvds_codec->timings;
> >> drm_bridge_add(&lvds_codec->bridge);
> >>
> >> platform_set_drvdata(pdev, lvds_codec);
> >> @@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev)
> >> return 0;
> >> }
> >>
> >> +static const struct lvds_codec_data decoder_data = {
> >> + .connector_type = DRM_MODE_CONNECTOR_DPI,
> >> + .is_encoder = false,
> >
> > The two fields are a bit redundant, as the decoder is always
> > LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but
> > maybe we could drop the connector_type field, and derive the connector
> > type from is_encoder ?
>
> Or the other way around instead ? That is, if the connector_type is
> LVDS, then it is encoder , otherwise its decoder ?
Either way works for me.
> > One may then say that we could drop the lvds_codec_data structure as it
> > contains a single field, but I foresee a need to have device-specific
> > timings at some point, so I think it's a good addition.
>
> [...]
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2021-03-22 10:29 ` Marek Vasut
2021-03-22 10:37 ` Laurent Pinchart
@ 2021-04-07 9:55 ` Marek Vasut
1 sibling, 0 replies; 8+ messages in thread
From: Marek Vasut @ 2021-04-07 9:55 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, Alexandre Torgue, Andrzej Hajda, Antonio Borneo,
Benjamin Gaignard, Biju Das, Maxime Coquelin, Philippe Cornu,
Sam Ravnborg, Vincent Abriou, Yannick Fertre, linux-arm-kernel,
linux-stm32
On 3/22/21 11:29 AM, Marek Vasut wrote:
> On 3/22/21 2:14 AM, Laurent Pinchart wrote:
>> Hi Marek,
>
> Hi,
>
> [...]
>
>>> diff --git
>>> a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>>> b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>>> index e5e3c72630cf..399a6528780a 100644
>>> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>>> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
>>> @@ -74,6 +74,13 @@ properties:
>>> additionalProperties: false
>>> + pixelclk-active:
>>> + description: |
>>> + Data sampling on rising or falling edge.
>>> + Use 0 to sample pixel data on rising edge and
>>> + Use 1 to sample pixel data on falling edge and
>>> + enum: [0, 1]
>>
>> The idea is good, but instead of adding a custom property, how about
>> reusing the pclk-sample property defined in
>> ../../media/video-interfaces.yaml ?
>
> Repeating myself from V1 discussion ... Either is fine by me, but I
> think pixelclk-active, which comes from panel-timings.yaml is closer to
> the video than multimedia bindings.
Was there ever any reply from Rob on this ?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
2021-03-22 10:37 ` Laurent Pinchart
@ 2021-04-16 6:49 ` Laurent Pinchart
0 siblings, 0 replies; 8+ messages in thread
From: Laurent Pinchart @ 2021-04-16 6:49 UTC (permalink / raw)
To: Rob Herring
Cc: Marek Vasut, devicetree, Alexandre Torgue, Antonio Borneo,
Vincent Abriou, Philippe Cornu, dri-devel, Yannick Fertre,
Andrzej Hajda, Maxime Coquelin, Biju Das, Sam Ravnborg,
linux-stm32, linux-arm-kernel, Benjamin Gaignard
Hi Rob,
There's a question for you below.
On Mon, Mar 22, 2021 at 12:37:47PM +0200, Laurent Pinchart wrote:
> Hi Marek,
>
> (CC'ing Ron and the DT mailing list for the DT discussion)
>
> On Mon, Mar 22, 2021 at 11:29:04AM +0100, Marek Vasut wrote:
> > On 3/22/21 2:14 AM, Laurent Pinchart wrote:
> > > Hi Marek,
> >
> > Hi,
> >
> > [...]
> >
> > >> diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > >> index e5e3c72630cf..399a6528780a 100644
> > >> --- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > >> +++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
> > >> @@ -74,6 +74,13 @@ properties:
> > >>
> > >> additionalProperties: false
> > >>
> > >> + pixelclk-active:
> > >> + description: |
> > >> + Data sampling on rising or falling edge.
> > >> + Use 0 to sample pixel data on rising edge and
> > >> + Use 1 to sample pixel data on falling edge and
> > >> + enum: [0, 1]
> > >
> > > The idea is good, but instead of adding a custom property, how about
> > > reusing the pclk-sample property defined in
> > > ../../media/video-interfaces.yaml ?
> >
> > Repeating myself from V1 discussion ... Either is fine by me, but I
> > think pixelclk-active, which comes from panel-timings.yaml is closer to
> > the video than multimedia bindings.
>
> That's a good point. The part that bothers me a bit is that it would be
> nice to define the property in a single YAML schema, referenced by
> individual bindings. video-interfaces.yaml is there for that purpose. We
> could do something similar on the display side, or consider the
> pixelclk-active usage in panel-timings.yaml an exception that we can't
> switch to video-interfaces.yaml as backward compatibility must be
> preserved.
>
> I don't have a too strong preference, whatever Rob prefers would be fine
> with me.
>
> > > The property is only valid for encoders, so I would at least mention
> > > that in the description, or, better, handle this based on the compatible
> > > string to allow validation.
> >
> > How does that work in the Yaml file ?
>
> Something along the lines of
>
> if:
> not:
> properties:
> compatible:
> contains:
> const: lvds-encoder
> then:
> properties:
> pixelclk-active: false
>
> My YAML schema foo is a bit rusty though, I apologize if this doesn't
> work as-is. There are lots of similar examples in DT bindings that
> should hopefully be right :-)
>
> > >> +
> > >> powerdown-gpios:
> > >> description:
> > >> The GPIO used to control the power down line of this device.
> > >> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c
> > >> index dcf579a4cf83..cab81ccd895d 100644
> > >> --- a/drivers/gpu/drm/bridge/lvds-codec.c
> > >> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
> >
> > [...]
> >
> > >> @@ -126,6 +146,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
> > >> */
> > >> lvds_codec->bridge.of_node = dev->of_node;
> > >> lvds_codec->bridge.funcs = &funcs;
> > >> + lvds_codec->bridge.timings = &lvds_codec->timings;
> > >> drm_bridge_add(&lvds_codec->bridge);
> > >>
> > >> platform_set_drvdata(pdev, lvds_codec);
> > >> @@ -142,19 +163,20 @@ static int lvds_codec_remove(struct platform_device *pdev)
> > >> return 0;
> > >> }
> > >>
> > >> +static const struct lvds_codec_data decoder_data = {
> > >> + .connector_type = DRM_MODE_CONNECTOR_DPI,
> > >> + .is_encoder = false,
> > >
> > > The two fields are a bit redundant, as the decoder is always
> > > LVDS-to-DPI, and the encoder DPI-to-LVDS. I don't mind too much, but
> > > maybe we could drop the connector_type field, and derive the connector
> > > type from is_encoder ?
> >
> > Or the other way around instead ? That is, if the connector_type is
> > LVDS, then it is encoder , otherwise its decoder ?
>
> Either way works for me.
>
> > > One may then say that we could drop the lvds_codec_data structure as it
> > > contains a single field, but I foresee a need to have device-specific
> > > timings at some point, so I think it's a good addition.
> >
> > [...]
--
Regards,
Laurent Pinchart
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-16 7:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-24 6:18 [PATCH V2] drm/bridge: lvds-codec: Add support for pixel data sampling edge select Marek Vasut
2021-01-24 17:04 ` Marek Vasut
2021-03-19 18:20 ` Marek Vasut
2021-03-22 1:14 ` Laurent Pinchart
2021-03-22 10:29 ` Marek Vasut
2021-03-22 10:37 ` Laurent Pinchart
2021-04-16 6:49 ` Laurent Pinchart
2021-04-07 9:55 ` Marek Vasut
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).