[PATCH v3 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[PATCH v3 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[Ken Sloat]

From: Ken Sloat <ksloat@aampglobal.com>

The ISC driver currently supports ITU-R 601 encoding which
utilizes the external hysync and vsync signals. ITU-R 656
format removes the need for these pins by embedding the
sync pulses within the data packet.

To support this feature, enable necessary register bits
when this feature is enabled via device tree.

Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
---
 drivers/media/platform/atmel/atmel-isc-regs.h | 2 ++
 drivers/media/platform/atmel/atmel-isc.c      | 7 ++++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h
index 2aadc19235ea..d730693f299c 100644
--- a/drivers/media/platform/atmel/atmel-isc-regs.h
+++ b/drivers/media/platform/atmel/atmel-isc-regs.h
@@ -24,6 +24,8 @@
 #define ISC_PFE_CFG0_HPOL_LOW   BIT(0)
 #define ISC_PFE_CFG0_VPOL_LOW   BIT(1)
 #define ISC_PFE_CFG0_PPOL_LOW   BIT(2)
+#define ISC_PFE_CFG0_CCIR656    BIT(9)
+#define ISC_PFE_CFG0_CCIR_CRC   BIT(10)
 
 #define ISC_PFE_CFG0_MODE_PROGRESSIVE   (0x0 << 4)
 #define ISC_PFE_CFG0_MODE_MASK          GENMASK(6, 4)
diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index 50178968b8a6..9a399aa7ca92 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -1095,7 +1095,8 @@ static int isc_configure(struct isc_device *isc)
 	pfe_cfg0  |= subdev->pfe_cfg0 | ISC_PFE_CFG0_MODE_PROGRESSIVE;
 	mask = ISC_PFE_CFG0_BPS_MASK | ISC_PFE_CFG0_HPOL_LOW |
 	       ISC_PFE_CFG0_VPOL_LOW | ISC_PFE_CFG0_PPOL_LOW |
-	       ISC_PFE_CFG0_MODE_MASK;
+	       ISC_PFE_CFG0_MODE_MASK | ISC_PFE_CFG0_CCIR_CRC |
+		   ISC_PFE_CFG0_CCIR656;
 
 	regmap_update_bits(regmap, ISC_PFE_CFG0, mask, pfe_cfg0);
 
@@ -2084,6 +2085,10 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 		if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
 			subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
 
+		if (v4l2_epn.bus_type == V4L2_MBUS_BT656)
+			subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC |
+					ISC_PFE_CFG0_CCIR656;
+
 		subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
 		subdev_entity->asd->match.fwnode =
 			of_fwnode_handle(rem);
-- 
2.17.1

[PATCH v3 2/2] media: atmel-isc: Update device tree binding documentation

[Ken Sloat]

From: Ken Sloat <ksloat@aampglobal.com>

Update device tree binding documentation specifying how to
enable BT656 with CRC decoding and specify properties for
default parallel bus type.

Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
---
 Changes in v2:
 -Use correct media "bus-type" dt property.

 Changes in v3:
 -Specify default bus type.
 -Document optional parallel bus flags.

 .../devicetree/bindings/media/atmel-isc.txt       | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
index bbe0e87c6188..db3749a3964f 100644
--- a/Documentation/devicetree/bindings/media/atmel-isc.txt
+++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
@@ -21,6 +21,21 @@ Required properties for ISC:
 - pinctrl-names, pinctrl-0
 	Please refer to pinctrl-bindings.txt.
 
+Optional properties for ISC:
+- bus-type
+	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
+	is enabled. If omitted, then the default bus-type is parallel and
+	the additional properties to follow can be specified:
+- hsync-active
+	Active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
+	If unspecified, this signal is set as active HIGH.
+- vsync-active
+	Active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
+	If unspecified, this signal is set as active HIGH.
+- pclk-sample
+	Sample data on rising (1) or falling (0) edge of the pixel clock
+	signal. If unspecified, data is sampled on the rising edge.
+
 ISC supports a single port node with parallel bus. It should contain one
 'port' child node with child 'endpoint' node. Please refer to the bindings
 defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
-- 
2.17.1

Re: [PATCH v3 1/2] media: atmel-isc: Add support for BT656 with CRC decoding

[sakari.ailus]

Hi Ken,

Thanks for the update.

On Mon, Feb 04, 2019 at 02:18:13PM +0000, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> The ISC driver currently supports ITU-R 601 encoding which
> utilizes the external hysync and vsync signals. ITU-R 656
> format removes the need for these pins by embedding the
> sync pulses within the data packet.
> 
> To support this feature, enable necessary register bits
> when this feature is enabled via device tree.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> ---
>  drivers/media/platform/atmel/atmel-isc-regs.h | 2 ++
>  drivers/media/platform/atmel/atmel-isc.c      | 7 ++++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isc-regs.h b/drivers/media/platform/atmel/atmel-isc-regs.h
> index 2aadc19235ea..d730693f299c 100644
> --- a/drivers/media/platform/atmel/atmel-isc-regs.h
> +++ b/drivers/media/platform/atmel/atmel-isc-regs.h
> @@ -24,6 +24,8 @@
>  #define ISC_PFE_CFG0_HPOL_LOW   BIT(0)
>  #define ISC_PFE_CFG0_VPOL_LOW   BIT(1)
>  #define ISC_PFE_CFG0_PPOL_LOW   BIT(2)
> +#define ISC_PFE_CFG0_CCIR656    BIT(9)
> +#define ISC_PFE_CFG0_CCIR_CRC   BIT(10)
>  
>  #define ISC_PFE_CFG0_MODE_PROGRESSIVE   (0x0 << 4)
>  #define ISC_PFE_CFG0_MODE_MASK          GENMASK(6, 4)
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index 50178968b8a6..9a399aa7ca92 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -1095,7 +1095,8 @@ static int isc_configure(struct isc_device *isc)
>  	pfe_cfg0  |= subdev->pfe_cfg0 | ISC_PFE_CFG0_MODE_PROGRESSIVE;
>  	mask = ISC_PFE_CFG0_BPS_MASK | ISC_PFE_CFG0_HPOL_LOW |
>  	       ISC_PFE_CFG0_VPOL_LOW | ISC_PFE_CFG0_PPOL_LOW |
> -	       ISC_PFE_CFG0_MODE_MASK;
> +	       ISC_PFE_CFG0_MODE_MASK | ISC_PFE_CFG0_CCIR_CRC |
> +		   ISC_PFE_CFG0_CCIR656;

This could be aligned more nicely.

>  
>  	regmap_update_bits(regmap, ISC_PFE_CFG0, mask, pfe_cfg0);
>  
> @@ -2084,6 +2085,10 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>  		if (flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)
>  			subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
>  
> +		if (v4l2_epn.bus_type == V4L2_MBUS_BT656)
> +			subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_CCIR_CRC |
> +					ISC_PFE_CFG0_CCIR656;

Could you also set the bus_type field for the v4l2_epn to the parallel bus
and if you get -ENXIO, try Bt.656 as well? The semantics changes (i.e. you
need to set the defaults before calling v4l2_fwnode_endpoint_parse()) with
setting the bus_type field; please see v4l2_fwnode_endpoint_parse()
documentation in include/media/v4l2-fwnode.h .

> +
>  		subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
>  		subdev_entity->asd->match.fwnode =
>  			of_fwnode_handle(rem);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 2/2] media: atmel-isc: Update device tree binding documentation

[sakari.ailus]

On Mon, Feb 04, 2019 at 02:18:14PM +0000, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>
> 
> Update device tree binding documentation specifying how to
> enable BT656 with CRC decoding and specify properties for
> default parallel bus type.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>

The patch looks nice; I'll just wait for Rob's ack.

-- 
Sakari Ailus

Re: [PATCH v3 2/2] media: atmel-isc: Update device tree binding documentation

[Rob Herring]

On Mon, Feb 04, 2019 at 02:18:14PM +0000, Ken Sloat wrote:
> From: Ken Sloat <ksloat@aampglobal.com>

Needs a better subject, not one that applies to any change. Update with 
what?

> Update device tree binding documentation specifying how to
> enable BT656 with CRC decoding and specify properties for
> default parallel bus type.
> 
> Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> ---
>  Changes in v2:
>  -Use correct media "bus-type" dt property.
> 
>  Changes in v3:
>  -Specify default bus type.
>  -Document optional parallel bus flags.
> 
>  .../devicetree/bindings/media/atmel-isc.txt       | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> index bbe0e87c6188..db3749a3964f 100644
> --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> @@ -21,6 +21,21 @@ Required properties for ISC:
>  - pinctrl-names, pinctrl-0
>  	Please refer to pinctrl-bindings.txt.
>  
> +Optional properties for ISC:
> +- bus-type
> +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> +	is enabled. If omitted, then the default bus-type is parallel and
> +	the additional properties to follow can be specified:
> +- hsync-active
> +	Active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> +	If unspecified, this signal is set as active HIGH.
> +- vsync-active
> +	Active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> +	If unspecified, this signal is set as active HIGH.
> +- pclk-sample
> +	Sample data on rising (1) or falling (0) edge of the pixel clock
> +	signal. If unspecified, data is sampled on the rising edge.

These are all common properties, right? No need to redefine them. Just 
reference the common doc. Maybe the default needs to be stated here if 
different or not defined.

> +
>  ISC supports a single port node with parallel bus. It should contain one
>  'port' child node with child 'endpoint' node. Please refer to the bindings
>  defined in Documentation/devicetree/bindings/media/video-interfaces.txt.
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/2] media: atmel-isc: Update device tree binding documentation

[sakari.ailus]

On Thu, Feb 14, 2019 at 09:44:22AM -0600, Rob Herring wrote:
> On Mon, Feb 04, 2019 at 02:18:14PM +0000, Ken Sloat wrote:
> > From: Ken Sloat <ksloat@aampglobal.com>
> 
> Needs a better subject, not one that applies to any change. Update with 
> what?
> 
> > Update device tree binding documentation specifying how to
> > enable BT656 with CRC decoding and specify properties for
> > default parallel bus type.
> > 
> > Signed-off-by: Ken Sloat <ksloat@aampglobal.com>
> > ---
> >  Changes in v2:
> >  -Use correct media "bus-type" dt property.
> > 
> >  Changes in v3:
> >  -Specify default bus type.
> >  -Document optional parallel bus flags.
> > 
> >  .../devicetree/bindings/media/atmel-isc.txt       | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/atmel-isc.txt b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > index bbe0e87c6188..db3749a3964f 100644
> > --- a/Documentation/devicetree/bindings/media/atmel-isc.txt
> > +++ b/Documentation/devicetree/bindings/media/atmel-isc.txt
> > @@ -21,6 +21,21 @@ Required properties for ISC:
> >  - pinctrl-names, pinctrl-0
> >  	Please refer to pinctrl-bindings.txt.
> >  
> > +Optional properties for ISC:
> > +- bus-type
> > +	When set to 6, Bt.656 decoding (embedded sync) with CRC decoding
> > +	is enabled. If omitted, then the default bus-type is parallel and
> > +	the additional properties to follow can be specified:
> > +- hsync-active
> > +	Active state of the HSYNC signal, 0/1 for LOW/HIGH respectively.
> > +	If unspecified, this signal is set as active HIGH.
> > +- vsync-active
> > +	Active state of the VSYNC signal, 0/1 for LOW/HIGH respectively.
> > +	If unspecified, this signal is set as active HIGH.
> > +- pclk-sample
> > +	Sample data on rising (1) or falling (0) edge of the pixel clock
> > +	signal. If unspecified, data is sampled on the rising edge.
> 
> These are all common properties, right? No need to redefine them. Just 
> reference the common doc. Maybe the default needs to be stated here if 
> different or not defined.

Yeah, video-interfaces.txt does not define the defaults; it's hardware
specific.

-- 
Sakari Ailus