[PATCH 0/3] c8sectpfe

[PATCH 0/3] c8sectpfe

[Peter Griffin]

Hi Mauro,

This series includes a couple of fixes for the c8sectpfe Linux dvb driver.

One was caused by omitting a patch from the original c8sectpfe series which
defined the ssc2 and ssc3 dt nodes, which was then used by the later DT patch.

This patch is included, along with the original patch which you reverted.

Also Valentin Rothberg spotted LIBELF32 Kconfig symbol I was selecting in the
Kconfig, this isn't required upstream and is left over legacy so I've removed
it.

Sorry for the delay in sending these fixes, I've been on holiday for the last
3 weeks.

kind regards,

Peter.

Peter Griffin (3):
  ARM: DT: STi: stihxxx-b2120: Add pulse-width properties to ssc2 & ssc3
  ARM: DT: STi: STiH407: Add c8sectpfe LinuxDVB DT node.
  [media] c8sectpfe: Remove select on undefined LIBELF_32

 arch/arm/boot/dts/stihxxx-b2120.dtsi         | 48 ++++++++++++++++++++++++++--
 drivers/media/platform/sti/c8sectpfe/Kconfig |  1 -
 2 files changed, 46 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH 1/3] ARM: DT: STi: stihxxx-b2120: Add pulse-width properties to ssc2 & ssc3

[Peter Griffin]

Adding these properties makes the I2C bus to the demodulators much
more reliable, and we no longer suffer from I2C errors when tuning.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm/boot/dts/stihxxx-b2120.dtsi | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/stihxxx-b2120.dtsi b/arch/arm/boot/dts/stihxxx-b2120.dtsi
index f589fe4..62994ae 100644
--- a/arch/arm/boot/dts/stihxxx-b2120.dtsi
+++ b/arch/arm/boot/dts/stihxxx-b2120.dtsi
@@ -27,12 +27,18 @@
 			};
 		};
 
-		i2c@9842000 {
+		ssc2: i2c@9842000 {
 			status = "okay";
+			clock-frequency = <100000>;
+			st,i2c-min-scl-pulse-width-us = <0>;
+			st,i2c-min-sda-pulse-width-us = <5>;
 		};
 
-		i2c@9843000 {
+		ssc3: i2c@9843000 {
 			status = "okay";
+			clock-frequency = <100000>;
+			st,i2c-min-scl-pulse-width-us = <0>;
+			st,i2c-min-sda-pulse-width-us = <5>;
 		};
 
 		i2c@9844000 {
-- 
1.9.1

[PATCH 2/3] ARM: DT: STi: STiH407: Add c8sectpfe LinuxDVB DT node.

[Peter Griffin]

This patch adds in the required DT node for the c8sectpfe
Linux DVB demux driver which allows the tsin channels
to be used on an upstream kernel.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm/boot/dts/stihxxx-b2120.dtsi | 38 ++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/boot/dts/stihxxx-b2120.dtsi b/arch/arm/boot/dts/stihxxx-b2120.dtsi
index 62994ae..1bc018e 100644
--- a/arch/arm/boot/dts/stihxxx-b2120.dtsi
+++ b/arch/arm/boot/dts/stihxxx-b2120.dtsi
@@ -6,6 +6,10 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
+#include <dt-bindings/clock/stih407-clks.h>
+#include <dt-bindings/media/c8sectpfe.h>
+
 / {
 	soc {
 		sbc_serial0: serial@9530000 {
@@ -85,5 +89,39 @@
 			status = "okay";
 		};
 
+		c8sectpfe@08a20000 {
+			compatible = "st,stih407-c8sectpfe";
+			status = "okay";
+			reg = <0x08a20000 0x10000>, <0x08a00000 0x4000>;
+			reg-names = "c8sectpfe", "c8sectpfe-ram";
+
+			interrupts = <0 34 0>, <0 35 0>;
+			interrupt-names = "c8sectpfe-error-irq",
+					  "c8sectpfe-idle-irq";
+
+			pinctrl-names	= "tsin0-serial", "tsin0-parallel",
+					  "tsin3-serial", "tsin4-serial",
+					  "tsin5-serial";
+
+			pinctrl-0	= <&pinctrl_tsin0_serial>;
+			pinctrl-1	= <&pinctrl_tsin0_parallel>;
+			pinctrl-2	= <&pinctrl_tsin3_serial>;
+			pinctrl-3	= <&pinctrl_tsin4_serial_alt3>;
+			pinctrl-4	= <&pinctrl_tsin5_serial_alt1>;
+
+			clocks = <&clk_s_c0_flexgen CLK_PROC_STFE>;
+			clock-names = "c8sectpfe";
+
+			/* tsin0 is TSA on NIMA */
+			tsin0: port@0 {
+
+				tsin-num = <0>;
+				serial-not-parallel;
+				i2c-bus = <&ssc2>;
+				rst-gpio = <&pio15 4 0>;
+
+				dvb-card = <STV0367_TDA18212_NIMA_1>;
+			};
+		};
 	};
 };
-- 
1.9.1

[PATCH 3/3] [media] c8sectpfe: Remove select on undefined LIBELF_32

[Peter Griffin]

LIBELF_32 is not defined in Kconfig, and is left over legacy
which is not required in the upstream driver, so remove it.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Suggested-by: Valentin Rothberg <valentinrothberg@gmail.com>
---
 drivers/media/platform/sti/c8sectpfe/Kconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/sti/c8sectpfe/Kconfig b/drivers/media/platform/sti/c8sectpfe/Kconfig
index d1bfd4c..b9ec667 100644
--- a/drivers/media/platform/sti/c8sectpfe/Kconfig
+++ b/drivers/media/platform/sti/c8sectpfe/Kconfig
@@ -1,7 +1,6 @@
 config DVB_C8SECTPFE
 	tristate "STMicroelectronics C8SECTPFE DVB support"
 	depends on DVB_CORE && I2C && (ARCH_STI || ARCH_MULTIPLATFORM)
-	select LIBELF_32
 	select FW_LOADER
 	select FW_LOADER_USER_HELPER_FALLBACK
 	select DEBUG_FS
-- 
1.9.1

Re: [PATCH 2/3] ARM: DT: STi: STiH407: Add c8sectpfe LinuxDVB DT node.

[Lee Jones]

On Tue, 25 Aug 2015, Peter Griffin wrote:

> This patch adds in the required DT node for the c8sectpfe
> Linux DVB demux driver which allows the tsin channels
> to be used on an upstream kernel.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  arch/arm/boot/dts/stihxxx-b2120.dtsi | 38 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stihxxx-b2120.dtsi b/arch/arm/boot/dts/stihxxx-b2120.dtsi
> index 62994ae..1bc018e 100644
> --- a/arch/arm/boot/dts/stihxxx-b2120.dtsi
> +++ b/arch/arm/boot/dts/stihxxx-b2120.dtsi
> @@ -6,6 +6,10 @@
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +
> +#include <dt-bindings/clock/stih407-clks.h>
> +#include <dt-bindings/media/c8sectpfe.h>
> +

Looks like some of the STi files are a little wavering on convention,
but I suggest the extra '\n's are superfluous.

>  / {
>  	soc {
>  		sbc_serial0: serial@9530000 {
> @@ -85,5 +89,39 @@
>  			status = "okay";
>  		};
>  
> +		c8sectpfe@08a20000 {

This should be a device type, not a model number.

demodulator@abcdabcd {

> +			compatible = "st,stih407-c8sectpfe";

To the uninitiated c8sectpfe is pretty indecipherable.

How about *-c8sectpfe-demod?

> +			status = "okay";
> +			reg = <0x08a20000 0x10000>, <0x08a00000 0x4000>;
> +			reg-names = "c8sectpfe", "c8sectpfe-ram";
> +
> +			interrupts = <0 34 0>, <0 35 0>;

Please use the predetermined DEFINES for the flags cell.

> +			interrupt-names = "c8sectpfe-error-irq",
> +					  "c8sectpfe-idle-irq";
> +
> +			pinctrl-names	= "tsin0-serial", "tsin0-parallel",
> +					  "tsin3-serial", "tsin4-serial",
> +					  "tsin5-serial";

I would put *-names *under* the properties they pertain to and in the
same format i.e. one per line in this case, for easy eye-match.

> +			pinctrl-0	= <&pinctrl_tsin0_serial>;
> +			pinctrl-1	= <&pinctrl_tsin0_parallel>;
> +			pinctrl-2	= <&pinctrl_tsin3_serial>;
> +			pinctrl-3	= <&pinctrl_tsin4_serial_alt3>;
> +			pinctrl-4	= <&pinctrl_tsin5_serial_alt1>;
> +
> +			clocks = <&clk_s_c0_flexgen CLK_PROC_STFE>;
> +			clock-names = "c8sectpfe";
> +
> +			/* tsin0 is TSA on NIMA */
> +			tsin0: port@0 {
> +

Why the '\n'?

> +				tsin-num = <0>;
> +				serial-not-parallel;
> +				i2c-bus = <&ssc2>;

If you are adding this property, I would get Wolfram or one of the DT
guys to Ack it.

> +				rst-gpio = <&pio15 4 0>;

Why not use the whole name "reset"?

"-gpio" should be "-gpios".

So, in full: "reset-gpios"?

Flags: GPIO_ACTIVE_HIGH ?

> +				dvb-card = <STV0367_TDA18212_NIMA_1>;
> +			};
> +		};
>  	};
>  };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] ARM: DT: STi: STiH407: Add c8sectpfe LinuxDVB DT node.

[Peter Griffin]

Hi Lee,

On Wed, 26 Aug 2015, Lee Jones wrote:

> On Tue, 25 Aug 2015, Peter Griffin wrote:
> 
> > This patch adds in the required DT node for the c8sectpfe
> > Linux DVB demux driver which allows the tsin channels
> > to be used on an upstream kernel.
> > 
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  arch/arm/boot/dts/stihxxx-b2120.dtsi | 38 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/stihxxx-b2120.dtsi b/arch/arm/boot/dts/stihxxx-b2120.dtsi
> > index 62994ae..1bc018e 100644
> > --- a/arch/arm/boot/dts/stihxxx-b2120.dtsi
> > +++ b/arch/arm/boot/dts/stihxxx-b2120.dtsi
> > @@ -6,6 +6,10 @@
> >   * it under the terms of the GNU General Public License version 2 as
> >   * published by the Free Software Foundation.
> >   */
> > +
> > +#include <dt-bindings/clock/stih407-clks.h>
> > +#include <dt-bindings/media/c8sectpfe.h>
> > +
> 
> Looks like some of the STi files are a little wavering on convention,
> but I suggest the extra '\n's are superfluous.

Ok I will remove them in v2.

> 
> >  / {
> >  	soc {
> >  		sbc_serial0: serial@9530000 {
> > @@ -85,5 +89,39 @@
> >  			status = "okay";
> >  		};
> >  
> > +		c8sectpfe@08a20000 {
> 
> This should be a device type, not a model number.
> 
> demodulator@abcdabcd {

Ok will change to demux@08a20000 in v2

> 
> > +			compatible = "st,stih407-c8sectpfe";
> 
> To the uninitiated c8sectpfe is pretty indecipherable.

It is the name of the IP block in the datasheet. It stands
for SECurity TRansport Processor FrontEnd

> 
> How about *-c8sectpfe-demod?

That doesn't really make sense, the demod is a seperate device
typically controlled over I2C bus and connected to this IP block
via the TS pins. Also although a tsin channel is "normally" connected
to a demodulator device there is no reason it couldn't be connected
for example to another STi chipset which is doing tsout.

Also this IP block can do more than accept tsin, it can
also do tsout, and merge TS channels together (some coming from
DDR, external TS pins). Which are features we hope to add
to the driver in the future so the name needs to be very generic.

So with that in mind I would prefer to leave the compatible as
the name of the IP block from the SoC datasheet.

> 
> > +			status = "okay";
> > +			reg = <0x08a20000 0x10000>, <0x08a00000 0x4000>;
> > +			reg-names = "c8sectpfe", "c8sectpfe-ram";
> > +
> > +			interrupts = <0 34 0>, <0 35 0>;
> 
> Please use the predetermined DEFINES for the flags cell.

Will fix in V2

> 
> > +			interrupt-names = "c8sectpfe-error-irq",
> > +					  "c8sectpfe-idle-irq";
> > +
> > +			pinctrl-names	= "tsin0-serial", "tsin0-parallel",
> > +					  "tsin3-serial", "tsin4-serial",
> > +					  "tsin5-serial";
> 
> I would put *-names *under* the properties they pertain to and in the
> same format i.e. one per line in this case, for easy eye-match.

Ok will change in V2.
> 
> > +			pinctrl-0	= <&pinctrl_tsin0_serial>;
> > +			pinctrl-1	= <&pinctrl_tsin0_parallel>;
> > +			pinctrl-2	= <&pinctrl_tsin3_serial>;
> > +			pinctrl-3	= <&pinctrl_tsin4_serial_alt3>;
> > +			pinctrl-4	= <&pinctrl_tsin5_serial_alt1>;
> > +
> > +			clocks = <&clk_s_c0_flexgen CLK_PROC_STFE>;
> > +			clock-names = "c8sectpfe";
> > +
> > +			/* tsin0 is TSA on NIMA */
> > +			tsin0: port@0 {
> > +
> 
> Why the '\n'?

Removed in v2
> 
> > +				tsin-num = <0>;
> > +				serial-not-parallel;
> > +				i2c-bus = <&ssc2>;
> 
> If you are adding this property, I would get Wolfram or one of the DT
> guys to Ack it.

This driver is actually already accepted, I just missed off one patch from
the v2 series which meant this patch broke the build when Mauro applied it,
which is the reason for re-sending it.

This binding is the same way the drm display drivers use DT via ddc-i2c-bus
property to get the i2c bus for EDID control, so I think uncontroversial.
> 
> > +				rst-gpio = <&pio15 4 0>;
> 
> Why not use the whole name "reset"?
> 
> "-gpio" should be "-gpios".
> 
> So, in full: "reset-gpios"?
> 
> Flags: GPIO_ACTIVE_HIGH ?

Doing a grep, that does seem to be "more standard". Will fix in V2 as a single
atomic commit.

regards,

Peter.

Re: [PATCH 2/3] ARM: DT: STi: STiH407: Add c8sectpfe LinuxDVB DT node.

[Lee Jones]

On Wed, 26 Aug 2015, Peter Griffin wrote:
> On Wed, 26 Aug 2015, Lee Jones wrote:
> > On Tue, 25 Aug 2015, Peter Griffin wrote:
> > 
> > > This patch adds in the required DT node for the c8sectpfe
> > > Linux DVB demux driver which allows the tsin channels
> > > to be used on an upstream kernel.
> > > 
> > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > > ---
> > >  arch/arm/boot/dts/stihxxx-b2120.dtsi | 38 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/stihxxx-b2120.dtsi b/arch/arm/boot/dts/stihxxx-b2120.dtsi
> > > index 62994ae..1bc018e 100644
> > > --- a/arch/arm/boot/dts/stihxxx-b2120.dtsi
> > > +++ b/arch/arm/boot/dts/stihxxx-b2120.dtsi

[...]

> > How about *-c8sectpfe-demod?
> 
> That doesn't really make sense, the demod is a seperate device
> typically controlled over I2C bus and connected to this IP block
> via the TS pins. Also although a tsin channel is "normally" connected
> to a demodulator device there is no reason it couldn't be connected
> for example to another STi chipset which is doing tsout.
> 
> Also this IP block can do more than accept tsin, it can
> also do tsout, and merge TS channels together (some coming from
> DDR, external TS pins). Which are features we hope to add
> to the driver in the future so the name needs to be very generic.
> 
> So with that in mind I would prefer to leave the compatible as
> the name of the IP block from the SoC datasheet.

Understood.  Thanks for the explanation.

[...]

> > > +				tsin-num = <0>;
> > > +				serial-not-parallel;
> > > +				i2c-bus = <&ssc2>;
> > 
> > If you are adding this property, I would get Wolfram or one of the DT
> > guys to Ack it.
> 
> This driver is actually already accepted, I just missed off one patch from
> the v2 series which meant this patch broke the build when Mauro applied it,
> which is the reason for re-sending it.
> 
> This binding is the same way the drm display drivers use DT via ddc-i2c-bus
> property to get the i2c bus for EDID control, so I think uncontroversial.

You're still adding a non-vendor property.  If you think it's generic
enough not to require a "<vendor>," prefix, please attempt to document
it in a generic binding document.

> > > +				rst-gpio = <&pio15 4 0>;
> > 
> > Why not use the whole name "reset"?
> > 
> > "-gpio" should be "-gpios".
> > 
> > So, in full: "reset-gpios"?
> > 
> > Flags: GPIO_ACTIVE_HIGH ?
> 
> Doing a grep, that does seem to be "more standard". Will fix in V2 as a single
> atomic commit.

It's not just 'more standard', it's documented in the GPIO binding.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog