* [PATCH v4 0/2] DCMI BT656 parallel bus mode support
@ 2020-10-20 10:14 Hugues Fruchet
2020-10-20 10:14 ` [PATCH v4 1/2] media: stm32-dcmi: add support of BT656 bus Hugues Fruchet
2020-10-20 10:14 ` [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656 Hugues Fruchet
0 siblings, 2 replies; 11+ messages in thread
From: Hugues Fruchet @ 2020-10-20 10:14 UTC (permalink / raw)
To: Alexandre Torgue, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, Rob Herring
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-media,
linux-stm32, Hugues Fruchet, Alain Volmat, Yannick Fertre,
Philippe CORNU
Add support of BT656 embedded synchronization bus.
This mode allows to save hardware synchro lines hsync & vsync
by replacing them with synchro codes embedded in data stream.
This mode is enabled when hsync-active & vsync-active
fields are not specified in devicetree.
===========
= history =
===========
version 4:
- Fix typo in commit message
version 3:
- Fix bus_width print to %u as per Sakari comment
version 2:
- As per Sakari remark, revisit commit message and document
BT656 parallel bus mode in bindings
version 1:
- Initial submission
Hugues Fruchet (2):
media: stm32-dcmi: add support of BT656 bus
media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
.../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++
drivers/media/platform/stm32/stm32-dcmi.c | 37 ++++++++++++++++++++--
2 files changed, 65 insertions(+), 2 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 1/2] media: stm32-dcmi: add support of BT656 bus
2020-10-20 10:14 [PATCH v4 0/2] DCMI BT656 parallel bus mode support Hugues Fruchet
@ 2020-10-20 10:14 ` Hugues Fruchet
2020-10-20 10:14 ` [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656 Hugues Fruchet
1 sibling, 0 replies; 11+ messages in thread
From: Hugues Fruchet @ 2020-10-20 10:14 UTC (permalink / raw)
To: Alexandre Torgue, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, Rob Herring
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-media,
linux-stm32, Hugues Fruchet, Alain Volmat, Yannick Fertre,
Philippe CORNU
Add support of BT656 embedded synchronization bus.
This mode allows to save hardware synchro lines hsync & vsync by replacing
them with synchro codes embedded in data stream.
This bus type is only compatible with 8 bits width data bus.
Due to reserved values 0x00 & 0xff used for synchro codes, valid data vary
from 0x1 to 0xfe, this is up to sensor to clip accordingly pixel data. As
a consequence of this clipping, JPEG is not supported with this bus type.
DCMI crop feature is also not available with this bus type.
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
drivers/media/platform/stm32/stm32-dcmi.c | 37 +++++++++++++++++++++++++++++--
1 file changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index fd1c41c..7705683 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -157,6 +157,7 @@ struct stm32_dcmi {
struct vb2_queue queue;
struct v4l2_fwnode_bus_parallel bus;
+ enum v4l2_mbus_type bus_type;
struct completion complete;
struct clk *mclk;
enum state state;
@@ -777,6 +778,23 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
val |= CR_PCKPOL;
+ /*
+ * BT656 embedded synchronisation bus mode.
+ *
+ * Default SAV/EAV mode is supported here with default codes
+ * SAV=0xff000080 & EAV=0xff00009d.
+ * With DCMI this means LSC=SAV=0x80 & LEC=EAV=0x9d.
+ */
+ if (dcmi->bus_type == V4L2_MBUS_BT656) {
+ val |= CR_ESS;
+
+ /* Unmask all codes */
+ reg_write(dcmi->regs, DCMI_ESUR, 0xffffffff);/* FEC:LEC:LSC:FSC */
+
+ /* Trig on LSC=0x80 & LEC=0x9d codes, ignore FSC and FEC */
+ reg_write(dcmi->regs, DCMI_ESCR, 0xff9d80ff);/* FEC:LEC:LSC:FSC */
+ }
+
reg_write(dcmi->regs, DCMI_CR, val);
/* Set crop */
@@ -1067,8 +1085,9 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
if (ret)
return ret;
- /* Disable crop if JPEG is requested */
- if (pix->pixelformat == V4L2_PIX_FMT_JPEG)
+ /* Disable crop if JPEG is requested or BT656 bus is selected */
+ if (pix->pixelformat == V4L2_PIX_FMT_JPEG &&
+ dcmi->bus_type != V4L2_MBUS_BT656)
dcmi->do_crop = false;
/* pix to mbus format */
@@ -1592,6 +1611,11 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
if (dcmi_formats[i].mbus_code != mbus_code.code)
continue;
+ /* Exclude JPEG if BT656 bus is selected */
+ if (dcmi_formats[i].fourcc == V4L2_PIX_FMT_JPEG &&
+ dcmi->bus_type == V4L2_MBUS_BT656)
+ continue;
+
/* Code supported, have we got this fourcc yet? */
for (j = 0; j < num_fmts; j++)
if (sd_fmts[j]->fourcc ==
@@ -1873,9 +1897,18 @@ static int dcmi_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "CSI bus not supported\n");
return -ENODEV;
}
+
+ if (ep.bus_type == V4L2_MBUS_BT656 &&
+ ep.bus.parallel.bus_width != 8) {
+ dev_err(&pdev->dev, "BT656 bus conflicts with %u bits bus width (8 bits required)\n",
+ ep.bus.parallel.bus_width);
+ return -ENODEV;
+ }
+
dcmi->bus.flags = ep.bus.parallel.flags;
dcmi->bus.bus_width = ep.bus.parallel.bus_width;
dcmi->bus.data_shift = ep.bus.parallel.data_shift;
+ dcmi->bus_type = ep.bus_type;
irq = platform_get_irq(pdev, 0);
if (irq <= 0)
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-20 10:14 [PATCH v4 0/2] DCMI BT656 parallel bus mode support Hugues Fruchet
2020-10-20 10:14 ` [PATCH v4 1/2] media: stm32-dcmi: add support of BT656 bus Hugues Fruchet
@ 2020-10-20 10:14 ` Hugues Fruchet
2020-10-21 13:00 ` Sakari Ailus
1 sibling, 1 reply; 11+ messages in thread
From: Hugues Fruchet @ 2020-10-20 10:14 UTC (permalink / raw)
To: Alexandre Torgue, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, Rob Herring
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-media,
linux-stm32, Hugues Fruchet, Alain Volmat, Yannick Fertre,
Philippe CORNU
Add support of BT656 parallel bus mode in DCMI.
This mode is enabled when hsync-active & vsync-active
fields are not specified.
Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
.../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
index 3fe778c..1ee521a 100644
--- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
+++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
@@ -44,6 +44,36 @@ properties:
bindings defined in
Documentation/devicetree/bindings/media/video-interfaces.txt.
+ properties:
+ endpoint:
+ type: object
+
+ properties:
+ bus-width: true
+
+ hsync-active:
+ description:
+ If both HSYNC and VSYNC polarities are not specified, BT656
+ embedded synchronization is selected.
+ default: 0
+
+ vsync-active:
+ description:
+ If both HSYNC and VSYNC polarities are not specified, BT656
+ embedded synchronization is selected.
+ default: 0
+
+ pclk-sample: true
+
+ remote-endpoint: true
+
+ required:
+ - remote-endpoint
+
+ additionalProperties: false
+
+ additionalProperties: false
+
required:
- compatible
- reg
--
2.7.4
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-20 10:14 ` [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656 Hugues Fruchet
@ 2020-10-21 13:00 ` Sakari Ailus
2020-10-21 14:24 ` Hugues FRUCHET
0 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2020-10-21 13:00 UTC (permalink / raw)
To: Hugues Fruchet
Cc: Alexandre Torgue, Mauro Carvalho Chehab, Hans Verkuil,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-media, linux-stm32, Alain Volmat, Yannick Fertre,
Philippe CORNU
Hi Hugues,
On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> Add support of BT656 parallel bus mode in DCMI.
> This mode is enabled when hsync-active & vsync-active
> fields are not specified.
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> index 3fe778c..1ee521a 100644
> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> @@ -44,6 +44,36 @@ properties:
> bindings defined in
> Documentation/devicetree/bindings/media/video-interfaces.txt.
>
> + properties:
> + endpoint:
> + type: object
> +
> + properties:
> + bus-width: true
> +
> + hsync-active:
> + description:
> + If both HSYNC and VSYNC polarities are not specified, BT656
> + embedded synchronization is selected.
> + default: 0
> +
> + vsync-active:
> + description:
> + If both HSYNC and VSYNC polarities are not specified, BT656
> + embedded synchronization is selected.
> + default: 0
Should I understand this as if the polarities were not specified, BT.656
will be used? The bindings previously documented BT.601 (parallel) only, so
it was somewhat ambigious to begin with. Is there a risk of interpreting
old BT.601 bindings as BT.656? With bus-type property, I believe you could
avoid at least that risk.
Also not specifying at least one of the default values leads to BT.656
without bus-type. That could be addressed by removing the defaults.
> +
> + pclk-sample: true
> +
> + remote-endpoint: true
> +
> + required:
> + - remote-endpoint
> +
> + additionalProperties: false
> +
> + additionalProperties: false
> +
> required:
> - compatible
> - reg
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-21 13:00 ` Sakari Ailus
@ 2020-10-21 14:24 ` Hugues FRUCHET
2020-10-21 21:40 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: Hugues FRUCHET @ 2020-10-21 14:24 UTC (permalink / raw)
To: Sakari Ailus
Cc: Alexandre TORGUE, Mauro Carvalho Chehab, Hans Verkuil,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-media, linux-stm32, Alain VOLMAT, Yannick FERTRE,
Philippe CORNU
Hi Sakari,
On 10/21/20 3:00 PM, Sakari Ailus wrote:
> Hi Hugues,
>
> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
>> Add support of BT656 parallel bus mode in DCMI.
>> This mode is enabled when hsync-active & vsync-active
>> fields are not specified.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
>> 1 file changed, 30 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>> index 3fe778c..1ee521a 100644
>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>> @@ -44,6 +44,36 @@ properties:
>> bindings defined in
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>
>> + properties:
>> + endpoint:
>> + type: object
>> +
>> + properties:
>> + bus-width: true
>> +
>> + hsync-active:
>> + description:
>> + If both HSYNC and VSYNC polarities are not specified, BT656
>> + embedded synchronization is selected.
>> + default: 0
>> +
>> + vsync-active:
>> + description:
>> + If both HSYNC and VSYNC polarities are not specified, BT656
>> + embedded synchronization is selected.
>> + default: 0
>
> Should I understand this as if the polarities were not specified, BT.656
> will be used?
Yes, this is what is documented in video-interfaces.txt:
"
Note, that if HSYNC and VSYNC polarities are not specified, embedded
synchronization may be required, where supported.
"
and
"
/* If hsync-active/vsync-active are missing,
embedded BT.656 sync is used */
hsync-active = <0>; /* Active low */
vsync-active = <0>; /* Active low */
"
and I found also this in
Documentation/devicetree/bindings/media/renesas,vin.yaml
"
hsync-active:
description:
If both HSYNC and VSYNC polarities are not specified,
embedded
synchronization is selected.
default: 1
vsync-active:
description:
If both HSYNC and VSYNC polarities are not specified,
embedded
synchronization is selected.
default: 1
"
In the other hand I've found few occurences of "bus-type"
(marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
way to go versus previous way to signal BT656 (without hsync/vsync) ?
As explained previously, I prefer this last way for backward compatibility.
The bindings previously documented BT.601 (parallel) only, so
> it was somewhat ambigious to begin with. Is there a risk of interpreting
> old BT.601 bindings as BT.656?
I don't think so.
With bus-type property, I believe you could
> avoid at least that risk.
yes but as explained, I'll prefer not to amend current boards device
tree files.
>
> Also not specifying at least one of the default values leads to BT.656
> without bus-type. That could be addressed by removing the defaults.
>
I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
drop the "default: 1" lines ?
>> +
>> + pclk-sample: true
>> +
>> + remote-endpoint: true
>> +
>> + required:
>> + - remote-endpoint
>> +
>> + additionalProperties: false
>> +
>> + additionalProperties: false
>> +
>> required:
>> - compatible
>> - reg
>
BR,
Hugues.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-21 14:24 ` Hugues FRUCHET
@ 2020-10-21 21:40 ` Sakari Ailus
2020-10-22 14:56 ` Hugues FRUCHET
0 siblings, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2020-10-21 21:40 UTC (permalink / raw)
To: Hugues FRUCHET
Cc: Alexandre TORGUE, Mauro Carvalho Chehab, Hans Verkuil,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-media, linux-stm32, Alain VOLMAT, Yannick FERTRE,
Philippe CORNU
Hi Hugues,
On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
>
> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > Hi Hugues,
> >
> > On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> >> Add support of BT656 parallel bus mode in DCMI.
> >> This mode is enabled when hsync-active & vsync-active
> >> fields are not specified.
> >>
> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >> ---
> >> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> >> 1 file changed, 30 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >> index 3fe778c..1ee521a 100644
> >> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >> @@ -44,6 +44,36 @@ properties:
> >> bindings defined in
> >> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>
> >> + properties:
> >> + endpoint:
> >> + type: object
> >> +
> >> + properties:
> >> + bus-width: true
> >> +
> >> + hsync-active:
> >> + description:
> >> + If both HSYNC and VSYNC polarities are not specified, BT656
> >> + embedded synchronization is selected.
> >> + default: 0
> >> +
> >> + vsync-active:
> >> + description:
> >> + If both HSYNC and VSYNC polarities are not specified, BT656
> >> + embedded synchronization is selected.
> >> + default: 0
> >
> > Should I understand this as if the polarities were not specified, BT.656
> > will be used?
>
> Yes, this is what is documented in video-interfaces.txt:
> "
> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> synchronization may be required, where supported.
> "
> and
> "
> /* If hsync-active/vsync-active are missing,
> embedded BT.656 sync is used */
> hsync-active = <0>; /* Active low */
> vsync-active = <0>; /* Active low */
> "
> and I found also this in
> Documentation/devicetree/bindings/media/renesas,vin.yaml
> "
> hsync-active:
> description:
> If both HSYNC and VSYNC polarities are not specified,
> embedded
> synchronization is selected.
> default: 1
>
> vsync-active:
> description:
> If both HSYNC and VSYNC polarities are not specified,
> embedded
> synchronization is selected.
> default: 1
Having the defaults leads to somewhat weird behaviour: specifying the
default value on either property changes the bus type.
> "
>
> In the other hand I've found few occurences of "bus-type"
> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> As explained previously, I prefer this last way for backward compatibility.
If you have a default for bus-type (BT.601), this won't be a problem.
The old DT bindings were somewhat, well, opportunistic. The v4l2-of
framework-let did its best and sometimes it worked. The behaviour is still
supported but not encouraged in new bindings.
>
>
> The bindings previously documented BT.601 (parallel) only, so
> > it was somewhat ambigious to begin with. Is there a risk of interpreting
> > old BT.601 bindings as BT.656?
> I don't think so.
>
> With bus-type property, I believe you could
> > avoid at least that risk.
> yes but as explained, I'll prefer not to amend current boards device
> tree files.
I don't think it matters from this point of view --- you can have a
default bus-type.
>
> >
> > Also not specifying at least one of the default values leads to BT.656
> > without bus-type. That could be addressed by removing the defaults.
> >
> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> drop the "default: 1" lines ?
That's one option, yes. Then you have to have those for BT.601 and it's no
longer ambiguous.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-21 21:40 ` Sakari Ailus
@ 2020-10-22 14:56 ` Hugues FRUCHET
2020-10-26 14:17 ` Rob Herring
0 siblings, 1 reply; 11+ messages in thread
From: Hugues FRUCHET @ 2020-10-22 14:56 UTC (permalink / raw)
To: Sakari Ailus, Jacopo Mondi
Cc: Alexandre TORGUE, Mauro Carvalho Chehab, Hans Verkuil,
Rob Herring, devicetree, linux-arm-kernel, linux-kernel,
linux-media, linux-stm32, Alain VOLMAT, Yannick FERTRE,
Philippe CORNU
Hi Sakari,
+ Jacopo for his work on ov772x binding related to BT656
On 10/21/20 11:40 PM, Sakari Ailus wrote:
> Hi Hugues,
>
> On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>> On 10/21/20 3:00 PM, Sakari Ailus wrote:
>>> Hi Hugues,
>>>
>>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
>>>> Add support of BT656 parallel bus mode in DCMI.
>>>> This mode is enabled when hsync-active & vsync-active
>>>> fields are not specified.
>>>>
>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>> ---
>>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
>>>> 1 file changed, 30 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>>>> index 3fe778c..1ee521a 100644
>>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>>>> @@ -44,6 +44,36 @@ properties:
>>>> bindings defined in
>>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>>>>
>>>> + properties:
>>>> + endpoint:
>>>> + type: object
>>>> +
>>>> + properties:
>>>> + bus-width: true
>>>> +
>>>> + hsync-active:
>>>> + description:
>>>> + If both HSYNC and VSYNC polarities are not specified, BT656
>>>> + embedded synchronization is selected.
>>>> + default: 0
>>>> +
>>>> + vsync-active:
>>>> + description:
>>>> + If both HSYNC and VSYNC polarities are not specified, BT656
>>>> + embedded synchronization is selected.
>>>> + default: 0
>>>
>>> Should I understand this as if the polarities were not specified, BT.656
>>> will be used?
>>
>> Yes, this is what is documented in video-interfaces.txt:
>> "
>> Note, that if HSYNC and VSYNC polarities are not specified, embedded
>> synchronization may be required, where supported.
>> "
>> and
>> "
>> /* If hsync-active/vsync-active are missing,
>> embedded BT.656 sync is used */
>> hsync-active = <0>; /* Active low */
>> vsync-active = <0>; /* Active low */
>> "
>> and I found also this in
>> Documentation/devicetree/bindings/media/renesas,vin.yaml
>> "
>> hsync-active:
>> description:
>> If both HSYNC and VSYNC polarities are not specified,
>> embedded
>> synchronization is selected.
>> default: 1
>>
>> vsync-active:
>> description:
>> If both HSYNC and VSYNC polarities are not specified,
>> embedded
>> synchronization is selected.
>> default: 1
>
> Having the defaults leads to somewhat weird behaviour: specifying the
> default value on either property changes the bus type.
>
>> "
>>
>> In the other hand I've found few occurences of "bus-type"
>> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
>> way to go versus previous way to signal BT656 (without hsync/vsync) ?
>> As explained previously, I prefer this last way for backward compatibility.
>
> If you have a default for bus-type (BT.601), this won't be a problem.
>
> The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> framework-let did its best and sometimes it worked. The behaviour is still
> supported but not encouraged in new bindings.
>
OK, so let's go for the new way.
I've found an interesting patch from Jacopo that is of great help:
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20200910162055.614089-4-jacopo+renesas@jmondi.org/
Here is a draft proposal before I push a new version, please comment:
properties:
bus-type:
enum: [5, 6]
default: 5
bus-width:
enum: [8, 10, 12, 14]
default: 8
hsync-active:
enum: [0, 1]
default: 0
vsync-active:
enum: [0, 1]
default: 0
pclk-sample:
enum: [0, 1]
default: 0
remote-endpoint: true
allOf:
- if:
properties:
bus-type:
const: 6
then:
properties:
hsync-active: false
vsync-active: false
bus-width:
enum: [8]
required:
- remote-endpoint
unevaluatedProperties: false
Unfortunately, the "default: 5" for bus-type is not working !!
If we don't specify "bus-type" in example, dt_binding_check is failing
as if default was 6, it's hardly understandable (see below) !
port {
dcmi_0: endpoint {
remote-endpoint = <&ov5640_0>;
bus-width = <10>;
hsync-active = <0>;
vsync-active = <0>;
pclk-sample = <1>;
};
=> this should be OK but error claimed:
DTC
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
CHECK
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
=> if "bus-type" is explicitly set to 5, all is fine (see below) !
port {
dcmi_0: endpoint {
remote-endpoint = <&ov5640_0>;
bus-type = <5>;
bus-width = <10>;
hsync-active = <0>;
vsync-active = <0>;
pclk-sample = <1>;
};
};
DTC
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
CHECK
Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
~/.../media_tree$
>>
>>
>> The bindings previously documented BT.601 (parallel) only, so
>>> it was somewhat ambigious to begin with. Is there a risk of interpreting
>>> old BT.601 bindings as BT.656?
>> I don't think so.
>>
>> With bus-type property, I believe you could
>>> avoid at least that risk.
>> yes but as explained, I'll prefer not to amend current boards device
>> tree files.
>
> I don't think it matters from this point of view --- you can have a
> default bus-type.
>
>>
>>>
>>> Also not specifying at least one of the default values leads to BT.656
>>> without bus-type. That could be addressed by removing the defaults.
>>>
>> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
>> drop the "default: 1" lines ?
>
> That's one option, yes. Then you have to have those for BT.601 and it's no
> longer ambiguous.
>
BR,
Hugues.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-22 14:56 ` Hugues FRUCHET
@ 2020-10-26 14:17 ` Rob Herring
2020-10-30 17:42 ` Sakari Ailus
0 siblings, 1 reply; 11+ messages in thread
From: Rob Herring @ 2020-10-26 14:17 UTC (permalink / raw)
To: Hugues FRUCHET
Cc: Sakari Ailus, Jacopo Mondi, Alexandre TORGUE,
Mauro Carvalho Chehab, Hans Verkuil, devicetree,
linux-arm-kernel, linux-kernel, linux-media, linux-stm32,
Alain VOLMAT, Yannick FERTRE, Philippe CORNU
On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
>
> + Jacopo for his work on ov772x binding related to BT656
>
> On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > Hi Hugues,
> >
> > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> >> Hi Sakari,
> >>
> >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> >>> Hi Hugues,
> >>>
> >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> >>>> Add support of BT656 parallel bus mode in DCMI.
> >>>> This mode is enabled when hsync-active & vsync-active
> >>>> fields are not specified.
> >>>>
> >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> >>>> ---
> >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> >>>> 1 file changed, 30 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >>>> index 3fe778c..1ee521a 100644
> >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >>>> @@ -44,6 +44,36 @@ properties:
> >>>> bindings defined in
> >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> >>>>
> >>>> + properties:
> >>>> + endpoint:
> >>>> + type: object
> >>>> +
> >>>> + properties:
> >>>> + bus-width: true
> >>>> +
> >>>> + hsync-active:
> >>>> + description:
> >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> >>>> + embedded synchronization is selected.
> >>>> + default: 0
> >>>> +
> >>>> + vsync-active:
> >>>> + description:
> >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> >>>> + embedded synchronization is selected.
> >>>> + default: 0
> >>>
> >>> Should I understand this as if the polarities were not specified, BT.656
> >>> will be used?
> >>
> >> Yes, this is what is documented in video-interfaces.txt:
> >> "
> >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> >> synchronization may be required, where supported.
> >> "
> >> and
> >> "
> >> /* If hsync-active/vsync-active are missing,
> >> embedded BT.656 sync is used */
> >> hsync-active = <0>; /* Active low */
> >> vsync-active = <0>; /* Active low */
> >> "
> >> and I found also this in
> >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> >> "
> >> hsync-active:
> >> description:
> >> If both HSYNC and VSYNC polarities are not specified,
> >> embedded
> >> synchronization is selected.
> >> default: 1
> >>
> >> vsync-active:
> >> description:
> >> If both HSYNC and VSYNC polarities are not specified,
> >> embedded
> >> synchronization is selected.
> >> default: 1
> >
> > Having the defaults leads to somewhat weird behaviour: specifying the
> > default value on either property changes the bus type.
> >
> >> "
> >>
> >> In the other hand I've found few occurences of "bus-type"
> >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> >> As explained previously, I prefer this last way for backward compatibility.
> >
> > If you have a default for bus-type (BT.601), this won't be a problem.
> >
> > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > framework-let did its best and sometimes it worked. The behaviour is still
> > supported but not encouraged in new bindings.
> >
>
> OK, so let's go for the new way.
> I've found an interesting patch from Jacopo that is of great help:
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20200910162055.614089-4-jacopo+renesas@jmondi.org/
>
> Here is a draft proposal before I push a new version, please comment:
>
> properties:
> bus-type:
> enum: [5, 6]
> default: 5
>
> bus-width:
> enum: [8, 10, 12, 14]
> default: 8
>
> hsync-active:
> enum: [0, 1]
For common properties, you can assume there's a common schema. As 0 and
1 are the only possible values, you don't need to define them here
unless only a subset is valid for this device.
> default: 0
>
> vsync-active:
> enum: [0, 1]
> default: 0
>
> pclk-sample:
> enum: [0, 1]
> default: 0
>
> remote-endpoint: true
>
> allOf:
> - if:
> properties:
> bus-type:
> const: 6
To fix the error, you need:
required:
- bus-type
The problem is the above schema is also true if the property
is not present.
> then:
> properties:
> hsync-active: false
> vsync-active: false
> bus-width:
> enum: [8]
>
> required:
> - remote-endpoint
>
> unevaluatedProperties: false
>
>
> Unfortunately, the "default: 5" for bus-type is not working !!
> If we don't specify "bus-type" in example, dt_binding_check is failing
> as if default was 6, it's hardly understandable (see below) !
> port {
> dcmi_0: endpoint {
> remote-endpoint = <&ov5640_0>;
> bus-width = <10>;
> hsync-active = <0>;
> vsync-active = <0>;
> pclk-sample = <1>;
> };
> => this should be OK but error claimed:
> DTC
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> CHECK
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
> dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
> dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
> dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
> From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
>
> => if "bus-type" is explicitly set to 5, all is fine (see below) !
> port {
> dcmi_0: endpoint {
> remote-endpoint = <&ov5640_0>;
> bus-type = <5>;
> bus-width = <10>;
> hsync-active = <0>;
> vsync-active = <0>;
> pclk-sample = <1>;
> };
> };
>
> DTC
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> CHECK
> Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> ~/.../media_tree$
>
>
> >>
> >>
> >> The bindings previously documented BT.601 (parallel) only, so
> >>> it was somewhat ambigious to begin with. Is there a risk of interpreting
> >>> old BT.601 bindings as BT.656?
> >> I don't think so.
> >>
> >> With bus-type property, I believe you could
> >>> avoid at least that risk.
> >> yes but as explained, I'll prefer not to amend current boards device
> >> tree files.
> >
> > I don't think it matters from this point of view --- you can have a
> > default bus-type.
> >
> >>
> >>>
> >>> Also not specifying at least one of the default values leads to BT.656
> >>> without bus-type. That could be addressed by removing the defaults.
> >>>
> >> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> >> drop the "default: 1" lines ?
> >
> > That's one option, yes. Then you have to have those for BT.601 and it's no
> > longer ambiguous.
> >
>
> BR,
> Hugues.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-26 14:17 ` Rob Herring
@ 2020-10-30 17:42 ` Sakari Ailus
2020-10-30 18:00 ` Jacopo Mondi
2020-10-30 20:09 ` Rob Herring
0 siblings, 2 replies; 11+ messages in thread
From: Sakari Ailus @ 2020-10-30 17:42 UTC (permalink / raw)
To: Rob Herring
Cc: Hugues FRUCHET, Jacopo Mondi, Alexandre TORGUE,
Mauro Carvalho Chehab, Hans Verkuil, devicetree,
linux-arm-kernel, linux-kernel, linux-media, linux-stm32,
Alain VOLMAT, Yannick FERTRE, Philippe CORNU
Hi Rob,
On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> > Hi Sakari,
> >
> > + Jacopo for his work on ov772x binding related to BT656
> >
> > On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > > Hi Hugues,
> > >
> > > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> > >> Hi Sakari,
> > >>
> > >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > >>> Hi Hugues,
> > >>>
> > >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> > >>>> Add support of BT656 parallel bus mode in DCMI.
> > >>>> This mode is enabled when hsync-active & vsync-active
> > >>>> fields are not specified.
> > >>>>
> > >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > >>>> ---
> > >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> > >>>> 1 file changed, 30 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >>>> index 3fe778c..1ee521a 100644
> > >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >>>> @@ -44,6 +44,36 @@ properties:
> > >>>> bindings defined in
> > >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> > >>>>
> > >>>> + properties:
> > >>>> + endpoint:
> > >>>> + type: object
> > >>>> +
> > >>>> + properties:
> > >>>> + bus-width: true
> > >>>> +
> > >>>> + hsync-active:
> > >>>> + description:
> > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > >>>> + embedded synchronization is selected.
> > >>>> + default: 0
> > >>>> +
> > >>>> + vsync-active:
> > >>>> + description:
> > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > >>>> + embedded synchronization is selected.
> > >>>> + default: 0
> > >>>
> > >>> Should I understand this as if the polarities were not specified, BT.656
> > >>> will be used?
> > >>
> > >> Yes, this is what is documented in video-interfaces.txt:
> > >> "
> > >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> > >> synchronization may be required, where supported.
> > >> "
> > >> and
> > >> "
> > >> /* If hsync-active/vsync-active are missing,
> > >> embedded BT.656 sync is used */
> > >> hsync-active = <0>; /* Active low */
> > >> vsync-active = <0>; /* Active low */
> > >> "
> > >> and I found also this in
> > >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> > >> "
> > >> hsync-active:
> > >> description:
> > >> If both HSYNC and VSYNC polarities are not specified,
> > >> embedded
> > >> synchronization is selected.
> > >> default: 1
> > >>
> > >> vsync-active:
> > >> description:
> > >> If both HSYNC and VSYNC polarities are not specified,
> > >> embedded
> > >> synchronization is selected.
> > >> default: 1
> > >
> > > Having the defaults leads to somewhat weird behaviour: specifying the
> > > default value on either property changes the bus type.
> > >
> > >> "
> > >>
> > >> In the other hand I've found few occurences of "bus-type"
> > >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> > >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> > >> As explained previously, I prefer this last way for backward compatibility.
> > >
> > > If you have a default for bus-type (BT.601), this won't be a problem.
> > >
> > > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > > framework-let did its best and sometimes it worked. The behaviour is still
> > > supported but not encouraged in new bindings.
> > >
> >
> > OK, so let's go for the new way.
> > I've found an interesting patch from Jacopo that is of great help:
> > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20200910162055.614089-4-jacopo+renesas@jmondi.org/
I wonder if Jacopo tested it. The idea seems interesting nonetheless.
> >
> > Here is a draft proposal before I push a new version, please comment:
> >
> > properties:
> > bus-type:
> > enum: [5, 6]
> > default: 5
> >
> > bus-width:
> > enum: [8, 10, 12, 14]
> > default: 8
> >
> > hsync-active:
> > enum: [0, 1]
>
> For common properties, you can assume there's a common schema. As 0 and
> 1 are the only possible values, you don't need to define them here
> unless only a subset is valid for this device.
>
> > default: 0
> >
> > vsync-active:
> > enum: [0, 1]
> > default: 0
> >
> > pclk-sample:
> > enum: [0, 1]
> > default: 0
> >
> > remote-endpoint: true
> >
> > allOf:
> > - if:
> > properties:
> > bus-type:
> > const: 6
>
> To fix the error, you need:
>
> required:
> - bus-type
>
> The problem is the above schema is also true if the property
> is not present.
Hmm. The idea was that we could keep this consistent with old bindings that
only documented parallel mode, and thus didn't need bus-type. This is
actually quite common --- adding support for something that wasn't known or
cared for during the original review.
I guess this could be done in the driver, too, adding a comment that the
bindings earlier did not require it.
>
> > then:
> > properties:
> > hsync-active: false
> > vsync-active: false
> > bus-width:
> > enum: [8]
> >
> > required:
> > - remote-endpoint
> >
> > unevaluatedProperties: false
> >
> >
> > Unfortunately, the "default: 5" for bus-type is not working !!
> > If we don't specify "bus-type" in example, dt_binding_check is failing
> > as if default was 6, it's hardly understandable (see below) !
> > port {
> > dcmi_0: endpoint {
> > remote-endpoint = <&ov5640_0>;
> > bus-width = <10>;
> > hsync-active = <0>;
> > vsync-active = <0>;
> > pclk-sample = <1>;
> > };
> > => this should be OK but error claimed:
> > DTC
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > CHECK
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
> > dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
> > dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
> > dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
> > From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> >
> > => if "bus-type" is explicitly set to 5, all is fine (see below) !
> > port {
> > dcmi_0: endpoint {
> > remote-endpoint = <&ov5640_0>;
> > bus-type = <5>;
> > bus-width = <10>;
> > hsync-active = <0>;
> > vsync-active = <0>;
> > pclk-sample = <1>;
> > };
> > };
> >
> > DTC
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > CHECK
> > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > ~/.../media_tree$
> >
> >
> > >>
> > >>
> > >> The bindings previously documented BT.601 (parallel) only, so
> > >>> it was somewhat ambigious to begin with. Is there a risk of interpreting
> > >>> old BT.601 bindings as BT.656?
> > >> I don't think so.
> > >>
> > >> With bus-type property, I believe you could
> > >>> avoid at least that risk.
> > >> yes but as explained, I'll prefer not to amend current boards device
> > >> tree files.
> > >
> > > I don't think it matters from this point of view --- you can have a
> > > default bus-type.
> > >
> > >>
> > >>>
> > >>> Also not specifying at least one of the default values leads to BT.656
> > >>> without bus-type. That could be addressed by removing the defaults.
> > >>>
> > >> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> > >> drop the "default: 1" lines ?
> > >
> > > That's one option, yes. Then you have to have those for BT.601 and it's no
> > > longer ambiguous.
> > >
> >
> > BR,
> > Hugues.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-30 17:42 ` Sakari Ailus
@ 2020-10-30 18:00 ` Jacopo Mondi
2020-10-30 20:09 ` Rob Herring
1 sibling, 0 replies; 11+ messages in thread
From: Jacopo Mondi @ 2020-10-30 18:00 UTC (permalink / raw)
To: Sakari Ailus
Cc: Rob Herring, Hugues FRUCHET, Alexandre TORGUE,
Mauro Carvalho Chehab, Hans Verkuil, devicetree,
linux-arm-kernel, linux-kernel, linux-media, linux-stm32,
Alain VOLMAT, Yannick FERTRE, Philippe CORNU
Hi Sakari,
On Fri, Oct 30, 2020 at 07:42:36PM +0200, Sakari Ailus wrote:
> Hi Rob,
>
> On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> > On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> > > Hi Sakari,
> > >
> > > + Jacopo for his work on ov772x binding related to BT656
> > >
> > > On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > > > Hi Hugues,
> > > >
> > > > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> > > >> Hi Sakari,
> > > >>
> > > >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > > >>> Hi Hugues,
> > > >>>
> > > >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> > > >>>> Add support of BT656 parallel bus mode in DCMI.
> > > >>>> This mode is enabled when hsync-active & vsync-active
> > > >>>> fields are not specified.
> > > >>>>
> > > >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > > >>>> ---
> > > >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> > > >>>> 1 file changed, 30 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> index 3fe778c..1ee521a 100644
> > > >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> @@ -44,6 +44,36 @@ properties:
> > > >>>> bindings defined in
> > > >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > >>>>
> > > >>>> + properties:
> > > >>>> + endpoint:
> > > >>>> + type: object
> > > >>>> +
> > > >>>> + properties:
> > > >>>> + bus-width: true
> > > >>>> +
> > > >>>> + hsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>> +
> > > >>>> + vsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>
> > > >>> Should I understand this as if the polarities were not specified, BT.656
> > > >>> will be used?
> > > >>
> > > >> Yes, this is what is documented in video-interfaces.txt:
> > > >> "
> > > >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> > > >> synchronization may be required, where supported.
> > > >> "
> > > >> and
> > > >> "
> > > >> /* If hsync-active/vsync-active are missing,
> > > >> embedded BT.656 sync is used */
> > > >> hsync-active = <0>; /* Active low */
> > > >> vsync-active = <0>; /* Active low */
> > > >> "
> > > >> and I found also this in
> > > >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > >> "
> > > >> hsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >>
> > > >> vsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >
> > > > Having the defaults leads to somewhat weird behaviour: specifying the
> > > > default value on either property changes the bus type.
> > > >
> > > >> "
> > > >>
> > > >> In the other hand I've found few occurences of "bus-type"
> > > >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> > > >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> > > >> As explained previously, I prefer this last way for backward compatibility.
> > > >
> > > > If you have a default for bus-type (BT.601), this won't be a problem.
> > > >
> > > > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > > > framework-let did its best and sometimes it worked. The behaviour is still
> > > > supported but not encouraged in new bindings.
> > > >
> > >
> > > OK, so let's go for the new way.
> > > I've found an interesting patch from Jacopo that is of great help:
> > > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20200910162055.614089-4-jacopo+renesas@jmondi.org/
>
> I wonder if Jacopo tested it. The idea seems interesting nonetheless.
>
I think I did, but in my series bus-type was required so I guess I don't
get bitten by the bus-type mismatch, if I got the below error right
> > >
> > > Here is a draft proposal before I push a new version, please comment:
> > >
> > > properties:
> > > bus-type:
> > > enum: [5, 6]
> > > default: 5
> > >
> > > bus-width:
> > > enum: [8, 10, 12, 14]
> > > default: 8
> > >
> > > hsync-active:
> > > enum: [0, 1]
> >
> > For common properties, you can assume there's a common schema. As 0 and
> > 1 are the only possible values, you don't need to define them here
> > unless only a subset is valid for this device.
> >
> > > default: 0
> > >
> > > vsync-active:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > pclk-sample:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > remote-endpoint: true
> > >
> > > allOf:
> > > - if:
> > > properties:
> > > bus-type:
> > > const: 6
> >
> > To fix the error, you need:
> >
> > required:
> > - bus-type
> >
> > The problem is the above schema is also true if the property
> > is not present.
>
> Hmm. The idea was that we could keep this consistent with old bindings that
> only documented parallel mode, and thus didn't need bus-type. This is
> actually quite common --- adding support for something that wasn't known or
> cared for during the original review.
>
> I guess this could be done in the driver, too, adding a comment that the
> bindings earlier did not require it.
>
> >
> > > then:
> > > properties:
> > > hsync-active: false
> > > vsync-active: false
> > > bus-width:
> > > enum: [8]
> > >
> > > required:
> > > - remote-endpoint
> > >
> > > unevaluatedProperties: false
> > >
> > >
> > > Unfortunately, the "default: 5" for bus-type is not working !!
> > > If we don't specify "bus-type" in example, dt_binding_check is failing
> > > as if default was 6, it's hardly understandable (see below) !
> > > port {
> > > dcmi_0: endpoint {
> > > remote-endpoint = <&ov5640_0>;
> > > bus-width = <10>;
> > > hsync-active = <0>;
> > > vsync-active = <0>;
> > > pclk-sample = <1>;
> > > };
> > > => this should be OK but error claimed:
> > > DTC
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > CHECK
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml:
> > > dcmi@4c006000: port:endpoint:vsync-active: False schema does not allow [[0]]
> > > dcmi@4c006000: port:endpoint:hsync-active: False schema does not allow [[0]]
> > > dcmi@4c006000: port:endpoint:bus-width:0:0: 10 is not one of [8]
> > > From schema: Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > >
> > > => if "bus-type" is explicitly set to 5, all is fine (see below) !
> > > port {
> > > dcmi_0: endpoint {
> > > remote-endpoint = <&ov5640_0>;
> > > bus-type = <5>;
> > > bus-width = <10>;
> > > hsync-active = <0>;
> > > vsync-active = <0>;
> > > pclk-sample = <1>;
> > > };
> > > };
> > >
> > > DTC
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > CHECK
> > > Documentation/devicetree/bindings/media/st,stm32-dcmi.example.dt.yaml
> > > ~/.../media_tree$
> > >
> > >
> > > >>
> > > >>
> > > >> The bindings previously documented BT.601 (parallel) only, so
> > > >>> it was somewhat ambigious to begin with. Is there a risk of interpreting
> > > >>> old BT.601 bindings as BT.656?
> > > >> I don't think so.
> > > >>
> > > >> With bus-type property, I believe you could
> > > >>> avoid at least that risk.
> > > >> yes but as explained, I'll prefer not to amend current boards device
> > > >> tree files.
> > > >
> > > > I don't think it matters from this point of view --- you can have a
> > > > default bus-type.
> > > >
> > > >>
> > > >>>
> > > >>> Also not specifying at least one of the default values leads to BT.656
> > > >>> without bus-type. That could be addressed by removing the defaults.
> > > >>>
> > > >> I'm new to yaml, I've taken that from renesas,vin.yaml. Should I just
> > > >> drop the "default: 1" lines ?
> > > >
> > > > That's one option, yes. Then you have to have those for BT.601 and it's no
> > > > longer ambiguous.
> > > >
> > >
> > > BR,
> > > Hugues.
>
> --
> Kind regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656
2020-10-30 17:42 ` Sakari Ailus
2020-10-30 18:00 ` Jacopo Mondi
@ 2020-10-30 20:09 ` Rob Herring
1 sibling, 0 replies; 11+ messages in thread
From: Rob Herring @ 2020-10-30 20:09 UTC (permalink / raw)
To: Sakari Ailus
Cc: Hugues FRUCHET, Jacopo Mondi, Alexandre TORGUE,
Mauro Carvalho Chehab, Hans Verkuil, devicetree,
linux-arm-kernel, linux-kernel, linux-media, linux-stm32,
Alain VOLMAT, Yannick FERTRE, Philippe CORNU
On Fri, Oct 30, 2020 at 12:42 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Rob,
>
> On Mon, Oct 26, 2020 at 09:17:14AM -0500, Rob Herring wrote:
> > On Thu, Oct 22, 2020 at 02:56:17PM +0000, Hugues FRUCHET wrote:
> > > Hi Sakari,
> > >
> > > + Jacopo for his work on ov772x binding related to BT656
> > >
> > > On 10/21/20 11:40 PM, Sakari Ailus wrote:
> > > > Hi Hugues,
> > > >
> > > > On Wed, Oct 21, 2020 at 02:24:08PM +0000, Hugues FRUCHET wrote:
> > > >> Hi Sakari,
> > > >>
> > > >> On 10/21/20 3:00 PM, Sakari Ailus wrote:
> > > >>> Hi Hugues,
> > > >>>
> > > >>> On Tue, Oct 20, 2020 at 12:14:49PM +0200, Hugues Fruchet wrote:
> > > >>>> Add support of BT656 parallel bus mode in DCMI.
> > > >>>> This mode is enabled when hsync-active & vsync-active
> > > >>>> fields are not specified.
> > > >>>>
> > > >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> > > >>>> ---
> > > >>>> .../devicetree/bindings/media/st,stm32-dcmi.yaml | 30 ++++++++++++++++++++++
> > > >>>> 1 file changed, 30 insertions(+)
> > > >>>>
> > > >>>> diff --git a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> index 3fe778c..1ee521a 100644
> > > >>>> --- a/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> +++ b/Documentation/devicetree/bindings/media/st,stm32-dcmi.yaml
> > > >>>> @@ -44,6 +44,36 @@ properties:
> > > >>>> bindings defined in
> > > >>>> Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > >>>>
> > > >>>> + properties:
> > > >>>> + endpoint:
> > > >>>> + type: object
> > > >>>> +
> > > >>>> + properties:
> > > >>>> + bus-width: true
> > > >>>> +
> > > >>>> + hsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>> +
> > > >>>> + vsync-active:
> > > >>>> + description:
> > > >>>> + If both HSYNC and VSYNC polarities are not specified, BT656
> > > >>>> + embedded synchronization is selected.
> > > >>>> + default: 0
> > > >>>
> > > >>> Should I understand this as if the polarities were not specified, BT.656
> > > >>> will be used?
> > > >>
> > > >> Yes, this is what is documented in video-interfaces.txt:
> > > >> "
> > > >> Note, that if HSYNC and VSYNC polarities are not specified, embedded
> > > >> synchronization may be required, where supported.
> > > >> "
> > > >> and
> > > >> "
> > > >> /* If hsync-active/vsync-active are missing,
> > > >> embedded BT.656 sync is used */
> > > >> hsync-active = <0>; /* Active low */
> > > >> vsync-active = <0>; /* Active low */
> > > >> "
> > > >> and I found also this in
> > > >> Documentation/devicetree/bindings/media/renesas,vin.yaml
> > > >> "
> > > >> hsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >>
> > > >> vsync-active:
> > > >> description:
> > > >> If both HSYNC and VSYNC polarities are not specified,
> > > >> embedded
> > > >> synchronization is selected.
> > > >> default: 1
> > > >
> > > > Having the defaults leads to somewhat weird behaviour: specifying the
> > > > default value on either property changes the bus type.
> > > >
> > > >> "
> > > >>
> > > >> In the other hand I've found few occurences of "bus-type"
> > > >> (marvell,mmp2-ccic.yaml), it is why I asked you if "bus-type" is the new
> > > >> way to go versus previous way to signal BT656 (without hsync/vsync) ?
> > > >> As explained previously, I prefer this last way for backward compatibility.
> > > >
> > > > If you have a default for bus-type (BT.601), this won't be a problem.
> > > >
> > > > The old DT bindings were somewhat, well, opportunistic. The v4l2-of
> > > > framework-let did its best and sometimes it worked. The behaviour is still
> > > > supported but not encouraged in new bindings.
> > > >
> > >
> > > OK, so let's go for the new way.
> > > I've found an interesting patch from Jacopo that is of great help:
> > > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20200910162055.614089-4-jacopo+renesas@jmondi.org/
>
> I wonder if Jacopo tested it. The idea seems interesting nonetheless.
>
> > >
> > > Here is a draft proposal before I push a new version, please comment:
> > >
> > > properties:
> > > bus-type:
> > > enum: [5, 6]
> > > default: 5
> > >
> > > bus-width:
> > > enum: [8, 10, 12, 14]
> > > default: 8
> > >
> > > hsync-active:
> > > enum: [0, 1]
> >
> > For common properties, you can assume there's a common schema. As 0 and
> > 1 are the only possible values, you don't need to define them here
> > unless only a subset is valid for this device.
> >
> > > default: 0
> > >
> > > vsync-active:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > pclk-sample:
> > > enum: [0, 1]
> > > default: 0
> > >
> > > remote-endpoint: true
> > >
> > > allOf:
> > > - if:
> > > properties:
> > > bus-type:
> > > const: 6
> >
> > To fix the error, you need:
> >
> > required:
> > - bus-type
> >
> > The problem is the above schema is also true if the property
> > is not present.
>
> Hmm. The idea was that we could keep this consistent with old bindings that
> only documented parallel mode, and thus didn't need bus-type. This is
> actually quite common --- adding support for something that wasn't known or
> cared for during the original review.
TBC, the 'required' here is required for the 'if' in the if/then
schema to work as you want. It's not making 'bus-type' a required
property.
Rob
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-30 20:09 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-20 10:14 [PATCH v4 0/2] DCMI BT656 parallel bus mode support Hugues Fruchet
2020-10-20 10:14 ` [PATCH v4 1/2] media: stm32-dcmi: add support of BT656 bus Hugues Fruchet
2020-10-20 10:14 ` [PATCH v4 2/2] media: dt-bindings: media: st,stm32-dcmi: Add support of BT656 Hugues Fruchet
2020-10-21 13:00 ` Sakari Ailus
2020-10-21 14:24 ` Hugues FRUCHET
2020-10-21 21:40 ` Sakari Ailus
2020-10-22 14:56 ` Hugues FRUCHET
2020-10-26 14:17 ` Rob Herring
2020-10-30 17:42 ` Sakari Ailus
2020-10-30 18:00 ` Jacopo Mondi
2020-10-30 20:09 ` Rob Herring
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).