* [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).