[PATCH v3 0/5] Add board support for TS-4800

[PATCH v3 0/5] Add board support for TS-4800

[Damien Riegel]

This patch serie adds support for TS-4800 board. This board,                                                                              
manufactured by Technologic Systems, is based on an IMX515.

The first stage bootloader, called TS-BOOTROM, enables the watchdog,
so a watchdog driver is required to prevent board from rebooting.

The current device tree is minimal but it allows to get a shell on the
board.

Changes in v3:
 - Rebased on v4.3
 - Changed vendor prefix from "ts" to "technologic"
 - Added a DT option to generic syscon driver to allow regmap configuration
 - Dropped custom mfd driver, use generic syscon driver instead.

Changes in v2:
 - Added a mfd driver to handle syscon registers
 - The watchdog driver now uses the regmap (created by the mfd driver)
   to access the feed register
 - Remove watchdog's dependency on SOC_IMX51

Damien Riegel (5):
  of: add vendor prefix for Technologic Systems
  mfd: syscon: add a DT property to set value width
  watchdog: ts4800: add driver for TS-4800 watchdog
  ARM: imx_v6_v7_defconfig: add TS-4800 watchdog
  ARM: dts: TS-4800: add basic device tree

 .../devicetree/bindings/arm/technologic.txt        |   6 +
 Documentation/devicetree/bindings/mfd/syscon.txt   |   3 +
 .../devicetree/bindings/vendor-prefixes.txt        |   1 +
 .../devicetree/bindings/watchdog/ts4800-wdt.txt    |  21 ++
 arch/arm/boot/dts/Makefile                         |   3 +-
 arch/arm/boot/dts/imx51-ts4800.dts                 | 190 ++++++++++++++++++
 arch/arm/configs/imx_v6_v7_defconfig               |   1 +
 drivers/mfd/syscon.c                               |  12 ++
 drivers/watchdog/Kconfig                           |  10 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/ts4800_wdt.c                      | 219 +++++++++++++++++++++
 11 files changed, 466 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/technologic.txt
 create mode 100644 Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
 create mode 100644 arch/arm/boot/dts/imx51-ts4800.dts
 create mode 100644 drivers/watchdog/ts4800_wdt.c

-- 
2.5.0

[PATCH v3 1/5] of: add vendor prefix for Technologic Systems

[Damien Riegel]

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 82d2ac9..0a70537 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -223,6 +223,7 @@ toshiba	Toshiba Corporation
 toumaz	Toumaz
 tplink	TP-LINK Technologies Co., Ltd.
 truly	Truly Semiconductors Limited
+technologic	Technologic Systems
 usi	Universal Scientific Industrial Co., Ltd.
 v3	V3 Semiconductor
 variscite	Variscite Ltd.
-- 
2.5.0

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Damien Riegel]

Currently syscon has a fixed configuration of 32 bits for register and
values widths. In some cases, it would be desirable to be able to
customize the value width.

For example, certain boards (like the ones manufactured by Technologic
Systems) have a FPGA that is memory-mapped, but its registers are only
16-bit wide.

This patch adds an optional "bus-width" DT binding for syscon that
allows to change the width for the data bus (i.e. val_bits). If this
property is provided, it will also adjust the register stride to
bus-width / 8. If not provided, the default configuration is used.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
 drivers/mfd/syscon.c                             | 12 ++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
index fe8150b..4c9d187 100644
--- a/Documentation/devicetree/bindings/mfd/syscon.txt
+++ b/Documentation/devicetree/bindings/mfd/syscon.txt
@@ -13,6 +13,9 @@ Required properties:
 - compatible: Should contain "syscon".
 - reg: the register region can be accessed from syscon
 
+Optional property:
+- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
+
 Examples:
 gpr: iomuxc-gpr at 020e0000 {
 	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 176bf0f..00b0995 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	struct syscon *syscon;
 	struct regmap *regmap;
 	void __iomem *base;
+	u32 bus_width;
 	int ret;
 	struct regmap_config syscon_config = syscon_regmap_config;
 
@@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
 	 else if (of_property_read_bool(np, "little-endian"))
 		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
 
+	 ret = of_property_read_u32(np, "bus-width", &bus_width);
+	 if (!ret) {
+		 /*
+		  * the property has been provided, regmap_init_mmio
+		  * will return an error if these values are invalid
+		  * so there is no need to check them here.
+		  */
+		 syscon_config.val_bits = bus_width;
+		 syscon_config.reg_stride = syscon_config.val_bits / 8;
+	 }
+
 	regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	if (IS_ERR(regmap)) {
 		pr_err("regmap init failed\n");
-- 
2.5.0

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Lee Jones]

On Mon, 16 Nov 2015, Damien Riegel wrote:

> Currently syscon has a fixed configuration of 32 bits for register and
> values widths. In some cases, it would be desirable to be able to
> customize the value width.
> 
> For example, certain boards (like the ones manufactured by Technologic
> Systems) have a FPGA that is memory-mapped, but its registers are only
> 16-bit wide.
> 
> This patch adds an optional "bus-width" DT binding for syscon that
> allows to change the width for the data bus (i.e. val_bits). If this
> property is provided, it will also adjust the register stride to
> bus-width / 8. If not provided, the default configuration is used.
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
>  drivers/mfd/syscon.c                             | 12 ++++++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> index fe8150b..4c9d187 100644
> --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> @@ -13,6 +13,9 @@ Required properties:
>  - compatible: Should contain "syscon".
>  - reg: the register region can be accessed from syscon
>  
> +Optional property:
> +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> +
>  Examples:
>  gpr: iomuxc-gpr at 020e0000 {
>  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 176bf0f..00b0995 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
>  	struct syscon *syscon;
>  	struct regmap *regmap;
>  	void __iomem *base;
> +	u32 bus_width;
>  	int ret;
>  	struct regmap_config syscon_config = syscon_regmap_config;
>  
> @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
>  	 else if (of_property_read_bool(np, "little-endian"))
>  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
>  
> +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> +	 if (!ret) {

This syntax is confusing, as we normally associate it with an error
condition.  Instead, I'd use:

  if (of_property_read_u32(np, "bus-width", &bus_width) == 0)

Or, for more clarity:

  of_property_read_u32(np, "bus-width", &bus_width);
  if (bus_width)

If you choose this version (which I think is my preferred method, don't
forget to initialise 'bus_width' to zero.

> +		 /*
> +		  * the property has been provided, regmap_init_mmio
> +		  * will return an error if these values are invalid
> +		  * so there is no need to check them here.
> +		  */
> +		 syscon_config.val_bits = bus_width;
> +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> +	 }
> +
>  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
>  	if (IS_ERR(regmap)) {
>  		pr_err("regmap init failed\n");

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

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Lee Jones]

Looks like you forgot to Cc Arnd too.

Doing that for you now.

On Tue, 17 Nov 2015, Lee Jones wrote:
> On Mon, 16 Nov 2015, Damien Riegel wrote:
> 
> > Currently syscon has a fixed configuration of 32 bits for register and
> > values widths. In some cases, it would be desirable to be able to
> > customize the value width.
> > 
> > For example, certain boards (like the ones manufactured by Technologic
> > Systems) have a FPGA that is memory-mapped, but its registers are only
> > 16-bit wide.
> > 
> > This patch adds an optional "bus-width" DT binding for syscon that
> > allows to change the width for the data bus (i.e. val_bits). If this
> > property is provided, it will also adjust the register stride to
> > bus-width / 8. If not provided, the default configuration is used.
> > 
> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
> >  drivers/mfd/syscon.c                             | 12 ++++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > index fe8150b..4c9d187 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > @@ -13,6 +13,9 @@ Required properties:
> >  - compatible: Should contain "syscon".
> >  - reg: the register region can be accessed from syscon
> >  
> > +Optional property:
> > +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> > +
> >  Examples:
> >  gpr: iomuxc-gpr at 020e0000 {
> >  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 176bf0f..00b0995 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	struct syscon *syscon;
> >  	struct regmap *regmap;
> >  	void __iomem *base;
> > +	u32 bus_width;
> >  	int ret;
> >  	struct regmap_config syscon_config = syscon_regmap_config;
> >  
> > @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	 else if (of_property_read_bool(np, "little-endian"))
> >  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> >  
> > +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> > +	 if (!ret) {
> 
> This syntax is confusing, as we normally associate it with an error
> condition.  Instead, I'd use:
> 
>   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> 
> Or, for more clarity:
> 
>   of_property_read_u32(np, "bus-width", &bus_width);
>   if (bus_width)
> 
> If you choose this version (which I think is my preferred method, don't
> forget to initialise 'bus_width' to zero.
> 
> > +		 /*
> > +		  * the property has been provided, regmap_init_mmio
> > +		  * will return an error if these values are invalid
> > +		  * so there is no need to check them here.
> > +		  */
> > +		 syscon_config.val_bits = bus_width;
> > +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> > +	 }
> > +
> >  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> >  	if (IS_ERR(regmap)) {
> >  		pr_err("regmap init failed\n");
> 

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

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Guenter Roeck]

On Tue, Nov 17, 2015 at 09:19:46AM +0000, Lee Jones wrote:
> On Mon, 16 Nov 2015, Damien Riegel wrote:
> 
> > Currently syscon has a fixed configuration of 32 bits for register and
> > values widths. In some cases, it would be desirable to be able to
> > customize the value width.
> > 
> > For example, certain boards (like the ones manufactured by Technologic
> > Systems) have a FPGA that is memory-mapped, but its registers are only
> > 16-bit wide.
> > 
> > This patch adds an optional "bus-width" DT binding for syscon that
> > allows to change the width for the data bus (i.e. val_bits). If this
> > property is provided, it will also adjust the register stride to
> > bus-width / 8. If not provided, the default configuration is used.
> > 
> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > ---
> >  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
> >  drivers/mfd/syscon.c                             | 12 ++++++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > index fe8150b..4c9d187 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > @@ -13,6 +13,9 @@ Required properties:
> >  - compatible: Should contain "syscon".
> >  - reg: the register region can be accessed from syscon
> >  
> > +Optional property:
> > +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> > +
> >  Examples:
> >  gpr: iomuxc-gpr at 020e0000 {
> >  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 176bf0f..00b0995 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	struct syscon *syscon;
> >  	struct regmap *regmap;
> >  	void __iomem *base;
> > +	u32 bus_width;
> >  	int ret;
> >  	struct regmap_config syscon_config = syscon_regmap_config;
> >  
> > @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
> >  	 else if (of_property_read_bool(np, "little-endian"))
> >  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> >  
> > +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> > +	 if (!ret) {
> 
> This syntax is confusing, as we normally associate it with an error
> condition.  Instead, I'd use:
> 
>   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)

Or maybe better

	if (!of_property_read_u32(np, "bus-width", &bus_width))

> 
> Or, for more clarity:
> 
>   of_property_read_u32(np, "bus-width", &bus_width);
>   if (bus_width)
> 
> If you choose this version (which I think is my preferred method, don't
> forget to initialise 'bus_width' to zero.
> 
Ignoring an error and depending on bus_width==0 to determine if the property
was provided seems odd, especially since it would "hide" if the bus-width
property is set to 0. In the original code, this would be detected as error.

Guenter

> > +		 /*
> > +		  * the property has been provided, regmap_init_mmio
> > +		  * will return an error if these values are invalid
> > +		  * so there is no need to check them here.
> > +		  */
> > +		 syscon_config.val_bits = bus_width;
> > +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> > +	 }
> > +
> >  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> >  	if (IS_ERR(regmap)) {
> >  		pr_err("regmap init failed\n");
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Lee Jones]

On Tue, 17 Nov 2015, Guenter Roeck wrote:

> On Tue, Nov 17, 2015 at 09:19:46AM +0000, Lee Jones wrote:
> > On Mon, 16 Nov 2015, Damien Riegel wrote:
> > 
> > > Currently syscon has a fixed configuration of 32 bits for register and
> > > values widths. In some cases, it would be desirable to be able to
> > > customize the value width.
> > > 
> > > For example, certain boards (like the ones manufactured by Technologic
> > > Systems) have a FPGA that is memory-mapped, but its registers are only
> > > 16-bit wide.
> > > 
> > > This patch adds an optional "bus-width" DT binding for syscon that
> > > allows to change the width for the data bus (i.e. val_bits). If this
> > > property is provided, it will also adjust the register stride to
> > > bus-width / 8. If not provided, the default configuration is used.
> > > 
> > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > > ---
> > >  Documentation/devicetree/bindings/mfd/syscon.txt |  3 +++
> > >  drivers/mfd/syscon.c                             | 12 ++++++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > index fe8150b..4c9d187 100644
> > > --- a/Documentation/devicetree/bindings/mfd/syscon.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/syscon.txt
> > > @@ -13,6 +13,9 @@ Required properties:
> > >  - compatible: Should contain "syscon".
> > >  - reg: the register region can be accessed from syscon
> > >  
> > > +Optional property:
> > > +- bus-width: width of the data bus. Can be <8>, <16>, <32>, or <64>.
> > > +
> > >  Examples:
> > >  gpr: iomuxc-gpr at 020e0000 {
> > >  	compatible = "fsl,imx6q-iomuxc-gpr", "syscon";
> > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > > index 176bf0f..00b0995 100644
> > > --- a/drivers/mfd/syscon.c
> > > +++ b/drivers/mfd/syscon.c
> > > @@ -47,6 +47,7 @@ static struct syscon *of_syscon_register(struct device_node *np)
> > >  	struct syscon *syscon;
> > >  	struct regmap *regmap;
> > >  	void __iomem *base;
> > > +	u32 bus_width;
> > >  	int ret;
> > >  	struct regmap_config syscon_config = syscon_regmap_config;
> > >  
> > > @@ -69,6 +70,17 @@ static struct syscon *of_syscon_register(struct device_node *np)
> > >  	 else if (of_property_read_bool(np, "little-endian"))
> > >  		syscon_config.val_format_endian = REGMAP_ENDIAN_LITTLE;
> > >  
> > > +	 ret = of_property_read_u32(np, "bus-width", &bus_width);
> > > +	 if (!ret) {
> > 
> > This syntax is confusing, as we normally associate it with an error
> > condition.  Instead, I'd use:
> > 
> >   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> 
> Or maybe better
> 
> 	if (!of_property_read_u32(np, "bus-width", &bus_width))

Not sure if it's better, but it's also suitable.

> > Or, for more clarity:
> > 
> >   of_property_read_u32(np, "bus-width", &bus_width);
> >   if (bus_width)
> > 
> > If you choose this version (which I think is my preferred method, don't
> > forget to initialise 'bus_width' to zero.
> > 
> Ignoring an error and depending on bus_width==0 to determine if the property
> was provided seems odd, especially since it would "hide" if the bus-width
> property is set to 0. In the original code, this would be detected as error.

I'm not sure what you mean.  If bus_width==0, then a problem has
occurred and we will not use the value.  If bus_width!=0 then we can
assume that it's been set and (as the comment describes) the value
will be checked for errors in regmap_init_mmio().

> > > +		 /*
> > > +		  * the property has been provided, regmap_init_mmio
> > > +		  * will return an error if these values are invalid
> > > +		  * so there is no need to check them here.
> > > +		  */
> > > +		 syscon_config.val_bits = bus_width;
> > > +		 syscon_config.reg_stride = syscon_config.val_bits / 8;
> > > +	 }
> > > +
> > >  	regmap = regmap_init_mmio(NULL, base, &syscon_config);
> > >  	if (IS_ERR(regmap)) {
> > >  		pr_err("regmap init failed\n");
> > 

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

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Guenter Roeck]

On 11/18/2015 12:21 AM, Lee Jones wrote:
[ ... ]
>>> Or, for more clarity:
>>>
>>>    of_property_read_u32(np, "bus-width", &bus_width);
>>>    if (bus_width)
>>>
>>> If you choose this version (which I think is my preferred method, don't
>>> forget to initialise 'bus_width' to zero.
>>>
>> Ignoring an error and depending on bus_width==0 to determine if the property
>> was provided seems odd, especially since it would "hide" if the bus-width
>> property is set to 0. In the original code, this would be detected as error.
>
> I'm not sure what you mean.  If bus_width==0, then a problem has
> occurred and we will not use the value.  If bus_width!=0 then we can
> assume that it's been set and (as the comment describes) the value
> will be checked for errors in regmap_init_mmio().
>

Your proposed code does not distinguish a missing property from "bus-width = <0>;".
It will silently ignore this case.

Guenter

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Lee Jones]

On Wed, 18 Nov 2015, Guenter Roeck wrote:

> On 11/18/2015 12:21 AM, Lee Jones wrote:
> [ ... ]
> >>>Or, for more clarity:
> >>>
> >>>   of_property_read_u32(np, "bus-width", &bus_width);
> >>>   if (bus_width)
> >>>
> >>>If you choose this version (which I think is my preferred method, don't
> >>>forget to initialise 'bus_width' to zero.
> >>>
> >>Ignoring an error and depending on bus_width==0 to determine if the property
> >>was provided seems odd, especially since it would "hide" if the bus-width
> >>property is set to 0. In the original code, this would be detected as error.
> >
> >I'm not sure what you mean.  If bus_width==0, then a problem has
> >occurred and we will not use the value.  If bus_width!=0 then we can
> >assume that it's been set and (as the comment describes) the value
> >will be checked for errors in regmap_init_mmio().
> >
> 
> Your proposed code does not distinguish a missing property from "bus-width = <0>;".
> It will silently ignore this case.

Okay, so what you're saying is, you'd prefer to throw and error
instead of using the default.  Fair point.  If this is an issue then
feel free to use either of the other two suggestions.

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

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Arnd Bergmann]

On Tuesday 17 November 2015 09:26:49 Guenter Roeck wrote:
> > 
> > This syntax is confusing, as we normally associate it with an error
> > condition.  Instead, I'd use:
> > 
> >   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> 
> Or maybe better
> 
>         if (!of_property_read_u32(np, "bus-width", &bus_width))

I would also prefer the latter, but it doesn't matter much either way.

> > 
> > Or, for more clarity:
> > 
> >   of_property_read_u32(np, "bus-width", &bus_width);
> >   if (bus_width)
> > 
> > If you choose this version (which I think is my preferred method, don't
> > forget to initialise 'bus_width' to zero.
> > 
> Ignoring an error and depending on bus_width==0 to determine if the property
> was provided seems odd, especially since it would "hide" if the bus-width
> property is set to 0. In the original code, this would be detected as error.

Right.

Another option would be

	ret = of_property_read_u32(np, "bus-width", &bus_width);
	/* no bus width provided, default to 32-bit */
	if (ret)
		bus_width = 32;

	syscon_config.val_bits = bus_width;
	syscon_config.reg_stride = syscon_config.val_bits / 8;

which has the same effect but seems a little clearer to me.

	Arnd

[PATCH v3 2/5] mfd: syscon: add a DT property to set value width

[Lee Jones]

On Sat, 21 Nov 2015, Arnd Bergmann wrote:

> On Tuesday 17 November 2015 09:26:49 Guenter Roeck wrote:
> > > 
> > > This syntax is confusing, as we normally associate it with an error
> > > condition.  Instead, I'd use:
> > > 
> > >   if (of_property_read_u32(np, "bus-width", &bus_width) == 0)
> > 
> > Or maybe better
> > 
> >         if (!of_property_read_u32(np, "bus-width", &bus_width))
> 
> I would also prefer the latter, but it doesn't matter much either way.
> 
> > > 
> > > Or, for more clarity:
> > > 
> > >   of_property_read_u32(np, "bus-width", &bus_width);
> > >   if (bus_width)
> > > 
> > > If you choose this version (which I think is my preferred method, don't
> > > forget to initialise 'bus_width' to zero.
> > > 
> > Ignoring an error and depending on bus_width==0 to determine if the property
> > was provided seems odd, especially since it would "hide" if the bus-width
> > property is set to 0. In the original code, this would be detected as error.
> 
> Right.
> 
> Another option would be
> 
> 	ret = of_property_read_u32(np, "bus-width", &bus_width);
> 	/* no bus width provided, default to 32-bit */
> 	if (ret)
> 		bus_width = 32;
> 
> 	syscon_config.val_bits = bus_width;
> 	syscon_config.reg_stride = syscon_config.val_bits / 8;
> 
> which has the same effect but seems a little clearer to me.

Works for me.

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

[PATCH v3 3/5] watchdog: ts4800: add driver for TS-4800 watchdog

[Damien Riegel]

This watchdog is instantiated in a FPGA that is memory mapped. It is
made of only one register, called the feed register. Writing to this
register will re-arm the watchdog for a given time (and enable it if it
was disable). It can be disabled by writing a special value into it.

It is part of a syscon block, and the watchdog register offset in this
block varies from board to board. This offset is passed in the syscon
property after the phandle to the syscon node.

This watchdog is present on several of Technologic Systems' boards, and
having a kernel driver allows to use these boards without relying on a
userspace tool to ping their watchdog.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 .../devicetree/bindings/watchdog/ts4800-wdt.txt    |  21 ++
 drivers/watchdog/Kconfig                           |  10 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/ts4800_wdt.c                      | 219 +++++++++++++++++++++
 4 files changed, 251 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
 create mode 100644 drivers/watchdog/ts4800_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
new file mode 100644
index 0000000..7d5dcfb
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
@@ -0,0 +1,21 @@
+Technologic Systems Watchdog
+
+Required properties:
+- compatible: must be "technologic,ts4800-wdt"
+- syscon: phandle / integer array that points to the syscon node which
+          describes the FPGA's syscon registers.
+          - phandle to FPGA's syscon
+          - offset to the watchdog register
+
+Example:
+
+syscon: syscon at b0010000 {
+	compatible = "syscon", "simple-mfd";
+	reg = <0xb0010000 0x3d>;
+	bus-width = <16>;
+
+	wdt at e {
+		compatible = "technologic,ts4800-wdt";
+		syscon = <&syscon 0xe>;
+	};
+}
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 79e1aa1..2914594 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -426,6 +426,16 @@ config NUC900_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called nuc900_wdt.
 
+config TS4800_WATCHDOG
+	tristate "TS-4800 Watchdog"
+	depends on OF
+	select WATCHDOG_CORE
+	select MFD_SYSCON
+	help
+	  Technologic Systems TS-4800 has watchdog timer implemented in
+	  an external FPGA. Say Y here if you want to support for the
+	  watchdog timer on TS-4800 board.
+
 config TS72XX_WATCHDOG
 	tristate "TS-72XX SBC Watchdog"
 	depends on MACH_TS72XX
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c616e3..3863ce0 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o
 obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
 obj-$(CONFIG_STMP3XXX_RTC_WATCHDOG) += stmp3xxx_rtc_wdt.o
 obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
+obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
 obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
diff --git a/drivers/watchdog/ts4800_wdt.c b/drivers/watchdog/ts4800_wdt.c
new file mode 100644
index 0000000..658d0ca
--- /dev/null
+++ b/drivers/watchdog/ts4800_wdt.c
@@ -0,0 +1,219 @@
+/*
+ * Watchdog driver for TS-4800 based boards
+ *
+ * Copyright (c) 2015 - Savoir-faire Linux
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/watchdog.h>
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout,
+	"Watchdog cannot be stopped once started (default="
+	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/* possible feed values */
+#define TS4800_WDT_FEED_2S       0x1
+#define TS4800_WDT_FEED_10S      0x2
+#define TS4800_WDT_DISABLE       0x3
+
+struct ts4800_wdt {
+	struct watchdog_device  wdd;
+	struct regmap           *regmap;
+	u32                     feed_offset;
+	u16                     feed_val;
+};
+
+/*
+ * TS-4800 supports the following timeout values:
+ *
+ *   value desc
+ *   ---------------------
+ *     0    feed for 338ms
+ *     1    feed for 2.706s
+ *     2    feed for 10.824s
+ *     3    disable watchdog
+ *
+ * Keep the regmap/timeout map ordered by timeout
+ */
+static const struct {
+	const int timeout;
+	const int regval;
+} ts4800_wdt_map[] = {
+	{ 2,  TS4800_WDT_FEED_2S },
+	{ 10, TS4800_WDT_FEED_10S },
+};
+
+static int timeout_to_regval(struct watchdog_device *wdd, int new_timeout)
+{
+	int i;
+
+	new_timeout = clamp_val(new_timeout,
+				wdd->min_timeout, wdd->max_timeout);
+
+	for (i = 0; i < ARRAY_SIZE(ts4800_wdt_map); i++) {
+		if (ts4800_wdt_map[i].timeout >= new_timeout)
+			return ts4800_wdt_map[i].timeout;
+	}
+
+	return -EINVAL;
+}
+
+static void ts4800_write_feed(struct ts4800_wdt *wdt, u16 val)
+{
+	regmap_write(wdt->regmap, wdt->feed_offset, val);
+}
+
+static int ts4800_wdt_start(struct watchdog_device *wdd)
+{
+	struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	ts4800_write_feed(wdt, wdt->feed_val);
+	return 0;
+}
+
+static int ts4800_wdt_stop(struct watchdog_device *wdd)
+{
+	struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	ts4800_write_feed(wdt, TS4800_WDT_DISABLE);
+	return 0;
+}
+
+static int ts4800_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int new_timeout)
+{
+	struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd);
+	int val = timeout_to_regval(wdd, new_timeout);
+
+	if (val < 0)
+		return val;
+
+	wdt->feed_val = (u16) val;
+
+	return 0;
+}
+
+static const struct watchdog_ops ts4800_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = ts4800_wdt_start,
+	.stop = ts4800_wdt_stop,
+	.set_timeout = ts4800_wdt_set_timeout,
+};
+
+static const struct watchdog_info ts4800_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
+	.identity = "TS-4800 Watchdog",
+};
+
+static int ts4800_wdt_probe(struct platform_device *pdev)
+{
+	const int max_timeout_index = ARRAY_SIZE(ts4800_wdt_map) - 1;
+	struct device_node *np = pdev->dev.of_node;
+	struct device_node *syscon_np;
+	struct watchdog_device *wdd;
+	struct ts4800_wdt *wdt;
+	u32 reg;
+	int ret;
+
+	syscon_np = of_parse_phandle(np, "syscon", 0);
+	if (!syscon_np) {
+		dev_err(&pdev->dev, "no syscon property\n");
+		return -ENODEV;
+	}
+
+	ret = of_property_read_u32_index(np, "syscon", 1, &reg);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "no offset in syscon\n");
+		return -EINVAL;
+	}
+
+	/* allocate memory for watchdog struct */
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	/* set regmap and offset to know where to write */
+	wdt->feed_offset = reg;
+	wdt->regmap = syscon_node_to_regmap(syscon_np);
+	if (!wdt->regmap) {
+		dev_err(&pdev->dev, "cannot get parent's regmap\n");
+		return -EINVAL;
+	}
+
+	/* Initialize struct watchdog_device */
+	wdd = &wdt->wdd;
+	wdd->parent = &pdev->dev;
+	wdd->info = &ts4800_wdt_info;
+	wdd->ops = &ts4800_wdt_ops;
+	wdd->min_timeout = ts4800_wdt_map[0].timeout;
+	wdd->max_timeout = ts4800_wdt_map[max_timeout_index].timeout;
+	wdd->timeout = wdd->max_timeout;
+	wdt->feed_val = ts4800_wdt_map[max_timeout_index].regval;
+
+	watchdog_set_drvdata(wdd, wdt);
+	watchdog_set_nowayout(wdd, nowayout);
+
+	/*
+	 * Must be called after watchdog_set_drvdata to dereference a valid
+	 * pointer. The feed register is write-only, so it is not possible to
+	 * determine if watchdog is already started or not. Disable it to be in
+	 * a known state.
+	 */
+	ts4800_wdt_stop(wdd);
+
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"failed to register watchdog device\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	dev_info(&pdev->dev,
+		 "initialized (timeout = %d sec, nowayout = %d)\n",
+		 wdd->timeout, nowayout);
+
+	return 0;
+}
+
+static int ts4800_wdt_remove(struct platform_device *pdev)
+{
+	struct ts4800_wdt *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&wdt->wdd);
+
+	return 0;
+}
+
+static const struct of_device_id ts4800_wdt_of_match[] = {
+	{ .compatible = "technologic,ts4800-wdt", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ts4800_wdt_of_match);
+
+static struct platform_driver ts4800_wdt_driver = {
+	.probe		= ts4800_wdt_probe,
+	.remove		= ts4800_wdt_remove,
+	.driver		= {
+		.name	= "ts4800_wdt",
+		.of_match_table = ts4800_wdt_of_match,
+	},
+};
+
+module_platform_driver(ts4800_wdt_driver);
+
+MODULE_AUTHOR("Damien Riegel <damien.riegel@savoirfairelinux.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:ts4800_wdt");
-- 
2.5.0

[PATCH v3 3/5] watchdog: ts4800: add driver for TS-4800 watchdog

[Guenter Roeck]

On 11/16/2015 11:06 AM, Damien Riegel wrote:
> This watchdog is instantiated in a FPGA that is memory mapped. It is
> made of only one register, called the feed register. Writing to this
> register will re-arm the watchdog for a given time (and enable it if it
> was disable). It can be disabled by writing a special value into it.
>
> It is part of a syscon block, and the watchdog register offset in this
> block varies from board to board. This offset is passed in the syscon
> property after the phandle to the syscon node.
>
> This watchdog is present on several of Technologic Systems' boards, and
> having a kernel driver allows to use these boards without relying on a
> userspace tool to ping their watchdog.
>

Last part should be obvious.

> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>   .../devicetree/bindings/watchdog/ts4800-wdt.txt    |  21 ++
>   drivers/watchdog/Kconfig                           |  10 +
>   drivers/watchdog/Makefile                          |   1 +
>   drivers/watchdog/ts4800_wdt.c                      | 219 +++++++++++++++++++++
>   4 files changed, 251 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
>   create mode 100644 drivers/watchdog/ts4800_wdt.c
>
> diff --git a/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
> new file mode 100644
> index 0000000..7d5dcfb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/ts4800-wdt.txt
> @@ -0,0 +1,21 @@
> +Technologic Systems Watchdog
> +
> +Required properties:
> +- compatible: must be "technologic,ts4800-wdt"
> +- syscon: phandle / integer array that points to the syscon node which
> +          describes the FPGA's syscon registers.
> +          - phandle to FPGA's syscon
> +          - offset to the watchdog register
> +
> +Example:
> +
> +syscon: syscon at b0010000 {
> +	compatible = "syscon", "simple-mfd";
> +	reg = <0xb0010000 0x3d>;
> +	bus-width = <16>;
> +
> +	wdt at e {
> +		compatible = "technologic,ts4800-wdt";
> +		syscon = <&syscon 0xe>;
> +	};
> +}
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 79e1aa1..2914594 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -426,6 +426,16 @@ config NUC900_WATCHDOG
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called nuc900_wdt.
>
> +config TS4800_WATCHDOG
> +	tristate "TS-4800 Watchdog"
> +	depends on OF
> +	select WATCHDOG_CORE
> +	select MFD_SYSCON
> +	help
> +	  Technologic Systems TS-4800 has watchdog timer implemented in
> +	  an external FPGA. Say Y here if you want to support for the
> +	  watchdog timer on TS-4800 board.
> +
>   config TS72XX_WATCHDOG
>   	tristate "TS-72XX SBC Watchdog"
>   	depends on MACH_TS72XX
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c616e3..3863ce0 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_RN5T618_WATCHDOG) += rn5t618_wdt.o
>   obj-$(CONFIG_COH901327_WATCHDOG) += coh901327_wdt.o
>   obj-$(CONFIG_STMP3XXX_RTC_WATCHDOG) += stmp3xxx_rtc_wdt.o
>   obj-$(CONFIG_NUC900_WATCHDOG) += nuc900_wdt.o
> +obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
>   obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>   obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
>   obj-$(CONFIG_UX500_WATCHDOG) += ux500_wdt.o
> diff --git a/drivers/watchdog/ts4800_wdt.c b/drivers/watchdog/ts4800_wdt.c
> new file mode 100644
> index 0000000..658d0ca
> --- /dev/null
> +++ b/drivers/watchdog/ts4800_wdt.c
> @@ -0,0 +1,219 @@
> +/*
> + * Watchdog driver for TS-4800 based boards
> + *
> + * Copyright (c) 2015 - Savoir-faire Linux
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout,
> +	"Watchdog cannot be stopped once started (default="
> +	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +/* possible feed values */
> +#define TS4800_WDT_FEED_2S       0x1
> +#define TS4800_WDT_FEED_10S      0x2
> +#define TS4800_WDT_DISABLE       0x3
> +
> +struct ts4800_wdt {
> +	struct watchdog_device  wdd;
> +	struct regmap           *regmap;
> +	u32                     feed_offset;
> +	u16                     feed_val;

u16 doesn't save you anything here. In many cases it actually makes
the code more complex. Might as well use u32.

> +};
> +
> +/*
> + * TS-4800 supports the following timeout values:
> + *
> + *   value desc
> + *   ---------------------
> + *     0    feed for 338ms
> + *     1    feed for 2.706s
> + *     2    feed for 10.824s
> + *     3    disable watchdog
> + *
> + * Keep the regmap/timeout map ordered by timeout
> + */
> +static const struct {
> +	const int timeout;
> +	const int regval;
> +} ts4800_wdt_map[] = {
> +	{ 2,  TS4800_WDT_FEED_2S },
> +	{ 10, TS4800_WDT_FEED_10S },
> +};
> +
> +static int timeout_to_regval(struct watchdog_device *wdd, int new_timeout)
> +{
> +	int i;
> +
> +	new_timeout = clamp_val(new_timeout,
> +				wdd->min_timeout, wdd->max_timeout);
> +
This should not be necessary. The value is already checked by the infrastructure.

> +	for (i = 0; i < ARRAY_SIZE(ts4800_wdt_map); i++) {
> +		if (ts4800_wdt_map[i].timeout >= new_timeout)
> +			return ts4800_wdt_map[i].timeout;
> +	}
> +
> +	return -EINVAL;

I think it would be easier here to just return TS4800_WDT_FEED_10S.

Overall, there are three "defenses" against bad values:
The infrastructure code, the clamp_val above, and the error return here.
One should be enough.

> +}
> +
> +static void ts4800_write_feed(struct ts4800_wdt *wdt, u16 val)
> +{
> +	regmap_write(wdt->regmap, wdt->feed_offset, val);
> +}
> +
> +static int ts4800_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	ts4800_write_feed(wdt, wdt->feed_val);
> +	return 0;
> +}
> +
> +static int ts4800_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	ts4800_write_feed(wdt, TS4800_WDT_DISABLE);
> +	return 0;
> +}
> +
> +static int ts4800_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int new_timeout)
> +{
> +	struct ts4800_wdt *wdt = watchdog_get_drvdata(wdd);
> +	int val = timeout_to_regval(wdd, new_timeout);
> +
> +	if (val < 0)
> +		return val;
> +
This should never happen, since the range was already verified by the infrastructure.
Another reason to not return an error from timeout_to_regval().

> +	wdt->feed_val = (u16) val;
> +
Then you could just make this

	wdt->feed_val = timeout_to_regval(wdd, new_timeout);

Also, you need to set the timeout value in the wdd structure to the actually
selected timeout, to let user space know which timeout was selected.

	wdt->timeout = ???;

This can be important in situations where the requested timeout is, say,
3 seconds, but the code actually sets it to 10 seconds. User space might
need to know about that.

> +	return 0;
> +}
> +
> +static const struct watchdog_ops ts4800_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = ts4800_wdt_start,
> +	.stop = ts4800_wdt_stop,
> +	.set_timeout = ts4800_wdt_set_timeout,
> +};
> +
> +static const struct watchdog_info ts4800_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> +	.identity = "TS-4800 Watchdog",
> +};
> +
> +static int ts4800_wdt_probe(struct platform_device *pdev)
> +{
> +	const int max_timeout_index = ARRAY_SIZE(ts4800_wdt_map) - 1;

Might as well use

#define MAX_TIMEOUT_INDEX	(ARRAY_SIZE(ts4800_wdt_map) - 1)

and reduce runtime complexity.

> +	struct device_node *np = pdev->dev.of_node;
> +	struct device_node *syscon_np;
> +	struct watchdog_device *wdd;
> +	struct ts4800_wdt *wdt;
> +	u32 reg;
> +	int ret;
> +
> +	syscon_np = of_parse_phandle(np, "syscon", 0);
> +	if (!syscon_np) {
> +		dev_err(&pdev->dev, "no syscon property\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = of_property_read_u32_index(np, "syscon", 1, &reg);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "no offset in syscon\n");
> +		return -EINVAL;

return -EINVAL or ret ? I think some static checkers may complain
if you don't return ret.

> +	}
> +
> +	/* allocate memory for watchdog struct */
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	/* set regmap and offset to know where to write */
> +	wdt->feed_offset = reg;
> +	wdt->regmap = syscon_node_to_regmap(syscon_np);
> +	if (!wdt->regmap) {

syscon_node_to_regmap() returns an error pointer.

> +		dev_err(&pdev->dev, "cannot get parent's regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Initialize struct watchdog_device */
> +	wdd = &wdt->wdd;
> +	wdd->parent = &pdev->dev;
> +	wdd->info = &ts4800_wdt_info;
> +	wdd->ops = &ts4800_wdt_ops;
> +	wdd->min_timeout = ts4800_wdt_map[0].timeout;
> +	wdd->max_timeout = ts4800_wdt_map[max_timeout_index].timeout;
> +	wdd->timeout = wdd->max_timeout;
> +	wdt->feed_val = ts4800_wdt_map[max_timeout_index].regval;
> +
> +	watchdog_set_drvdata(wdd, wdt);
> +	watchdog_set_nowayout(wdd, nowayout);
> +

Calling watchdog_init_timeout() might be useful as well, to get a timeout value
from devicetree if provided.

> +	/*
> +	 * Must be called after watchdog_set_drvdata to dereference a valid
> +	 * pointer. The feed register is write-only, so it is not possible to
> +	 * determine if watchdog is already started or not. Disable it to be in
> +	 * a known state.

Annoying :-(. I keep wondering who comes up with those HW implementations.

> +	 */
> +	ts4800_wdt_stop(wdd);
> +
> +	ret = watchdog_register_device(wdd);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	dev_info(&pdev->dev,
> +		 "initialized (timeout = %d sec, nowayout = %d)\n",
> +		 wdd->timeout, nowayout);
> +
> +	return 0;
> +}
> +
> +static int ts4800_wdt_remove(struct platform_device *pdev)
> +{
> +	struct ts4800_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ts4800_wdt_of_match[] = {
> +	{ .compatible = "technologic,ts4800-wdt", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ts4800_wdt_of_match);
> +
> +static struct platform_driver ts4800_wdt_driver = {
> +	.probe		= ts4800_wdt_probe,
> +	.remove		= ts4800_wdt_remove,
> +	.driver		= {
> +		.name	= "ts4800_wdt",
> +		.of_match_table = ts4800_wdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(ts4800_wdt_driver);
> +
> +MODULE_AUTHOR("Damien Riegel <damien.riegel@savoirfairelinux.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:ts4800_wdt");
>

[PATCH v3 4/5] ARM: imx_v6_v7_defconfig: add TS-4800 watchdog

[Damien Riegel]

The TS-4800, based on an IMX.515, needs its watchdog support in order to
work.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 79194c6..8968166 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -195,6 +195,7 @@ CONFIG_THERMAL=y
 CONFIG_CPU_THERMAL=y
 CONFIG_IMX_THERMAL=y
 CONFIG_WATCHDOG=y
+CONFIG_TS4800_WATCHDOG=y
 CONFIG_IMX2_WDT=y
 CONFIG_MFD_DA9052_I2C=y
 CONFIG_MFD_MC13XXX_SPI=y
-- 
2.5.0

[PATCH v3 5/5] ARM: dts: TS-4800: add basic device tree

[Damien Riegel]

This device tree adds support for TS-4800 by Technologic Systems. This
board is based on MX51-babbage, but there are some subtle differences in
the pins used, and there is an additional FPGA that is memory-mapped.

More details here:
  http://wiki.embeddedarm.com/wiki/TS-4800

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 .../devicetree/bindings/arm/technologic.txt        |   6 +
 arch/arm/boot/dts/Makefile                         |   3 +-
 arch/arm/boot/dts/imx51-ts4800.dts                 | 190 +++++++++++++++++++++
 3 files changed, 198 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/technologic.txt
 create mode 100644 arch/arm/boot/dts/imx51-ts4800.dts

diff --git a/Documentation/devicetree/bindings/arm/technologic.txt b/Documentation/devicetree/bindings/arm/technologic.txt
new file mode 100644
index 0000000..8422988
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/technologic.txt
@@ -0,0 +1,6 @@
+Technologic Systems Platforms Device Tree Bindings
+--------------------------------------------------
+
+TS-4800 board
+Required root node properties:
+	- compatible = "technologic,imx51-ts4800", "fsl,imx51";
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index bb8fa02..41b9985 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -258,7 +258,8 @@ dtb-$(CONFIG_SOC_IMX51) += \
 	imx51-apf51dev.dtb \
 	imx51-babbage.dtb \
 	imx51-digi-connectcore-jsk.dtb \
-	imx51-eukrea-mbimxsd51-baseboard.dtb
+	imx51-eukrea-mbimxsd51-baseboard.dtb \
+	imx51-ts4800.dtb
 dtb-$(CONFIG_SOC_IMX53) += \
 	imx53-ard.dtb \
 	imx53-m53evk.dtb \
diff --git a/arch/arm/boot/dts/imx51-ts4800.dts b/arch/arm/boot/dts/imx51-ts4800.dts
new file mode 100644
index 0000000..fac2058
--- /dev/null
+++ b/arch/arm/boot/dts/imx51-ts4800.dts
@@ -0,0 +1,190 @@
+/*
+ * Copyright 2015 Savoir-faire Linux
+ *
+ * This device tree is based on imx51-babbage.dts
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/dts-v1/;
+#include "imx51.dtsi"
+
+/ {
+	model = "Technologic Systems TS-4800";
+	compatible = "technologic,imx51-ts4800", "fsl,imx51";
+
+	chosen {
+		stdout-path = &uart1;
+	};
+
+	memory {
+		reg = <0x90000000 0x10000000>;
+	};
+
+	soc {
+		fpga {
+			compatible = "simple-bus";
+			reg = <0xb0000000 0x1d000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+
+			syscon: syscon at b0010000 {
+				compatible = "syscon", "simple-mfd";
+				reg = <0xb0010000 0x3d>;
+				bus-width = <16>;
+
+				wdt at e {
+					compatible = "technologic,ts4800-wdt";
+					syscon = <&syscon 0xe>;
+				};
+			};
+		};
+	};
+
+	clocks {
+		ckih1 {
+			clock-frequency = <22579200>;
+		};
+
+		ckih2 {
+			clock-frequency = <24576000>;
+		};
+	};
+};
+
+&esdhc1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_esdhc1>;
+	cd-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
+	wp-gpios = <&gpio1 1 GPIO_ACTIVE_HIGH>;
+	status = "okay";
+};
+
+&fec {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_fec>;
+	phy-mode = "mii";
+	phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>;
+	phy-reset-duration = <1>;
+	status = "okay";
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1>;
+	status = "okay";
+};
+
+&uart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart2>;
+	status = "okay";
+};
+
+&uart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart3>;
+	status = "okay";
+};
+
+&i2c2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c2>;
+	status = "okay";
+
+	rtc: m41t00 at 68 {
+		compatible = "stm,m41t00";
+		reg = <0x68>;
+	};
+};
+
+
+&iomuxc {
+	imx51-ts4800 {
+
+		pinctrl_ecspi1: ecspi1grp {
+			fsl,pins = <
+				MX51_PAD_CSPI1_MISO__ECSPI1_MISO	0x185
+				MX51_PAD_CSPI1_MOSI__ECSPI1_MOSI	0x185
+				MX51_PAD_CSPI1_SCLK__ECSPI1_SCLK	0x185
+				MX51_PAD_CSPI1_SS0__GPIO4_24		0x85 /* CS0 */
+			>;
+		};
+
+		pinctrl_esdhc1: esdhc1grp {
+			fsl,pins = <
+				MX51_PAD_SD1_CMD__SD1_CMD		0x400020d5
+				MX51_PAD_SD1_CLK__SD1_CLK		0x20d5
+				MX51_PAD_SD1_DATA0__SD1_DATA0		0x20d5
+				MX51_PAD_SD1_DATA1__SD1_DATA1		0x20d5
+				MX51_PAD_SD1_DATA2__SD1_DATA2		0x20d5
+				MX51_PAD_SD1_DATA3__SD1_DATA3		0x20d5
+				MX51_PAD_GPIO1_0__GPIO1_0		0x100
+				MX51_PAD_GPIO1_1__GPIO1_1		0x100
+			>;
+		};
+
+		pinctrl_fec: fecgrp {
+			fsl,pins = <
+				MX51_PAD_EIM_EB2__FEC_MDIO		0x000001f5
+				MX51_PAD_EIM_EB3__FEC_RDATA1		0x00000085
+				MX51_PAD_EIM_CS2__FEC_RDATA2		0x00000085
+				MX51_PAD_EIM_CS3__FEC_RDATA3		0x00000085
+				MX51_PAD_EIM_CS4__FEC_RX_ER		0x00000180
+				MX51_PAD_EIM_CS5__FEC_CRS		0x00000180
+				MX51_PAD_DISP2_DAT10__FEC_COL		0x00000180
+				MX51_PAD_DISP2_DAT11__FEC_RX_CLK		0x00000180
+				MX51_PAD_DISP2_DAT14__FEC_RDATA0		0x00002180
+				MX51_PAD_DISP2_DAT15__FEC_TDATA0		0x00002004
+				MX51_PAD_NANDF_CS2__FEC_TX_ER		0x00002004
+				MX51_PAD_DI2_PIN2__FEC_MDC		0x00002004
+				MX51_PAD_DISP2_DAT6__FEC_TDATA1		0x00002004
+				MX51_PAD_DISP2_DAT7__FEC_TDATA2		0x00002004
+				MX51_PAD_DISP2_DAT8__FEC_TDATA3		0x00002004
+				MX51_PAD_DISP2_DAT9__FEC_TX_EN		0x00002004
+				MX51_PAD_DISP2_DAT13__FEC_TX_CLK	0x00002180
+				MX51_PAD_DISP2_DAT12__FEC_RX_DV		0x000020a4
+				MX51_PAD_EIM_A20__GPIO2_14		0x00000085 /* Phy Reset */
+			>;
+		};
+
+		pinctrl_i2c2: i2c2grp {
+			fsl,pins = <
+				MX51_PAD_KEY_COL4__I2C2_SCL		0x400001ed
+				MX51_PAD_KEY_COL5__I2C2_SDA		0x400001ed
+			>;
+		};
+
+		pinctrl_uart1: uart1grp {
+			fsl,pins = <
+				MX51_PAD_UART1_RXD__UART1_RXD		0x1c5
+				MX51_PAD_UART1_TXD__UART1_TXD		0x1c5
+				MX51_PAD_UART1_RTS__UART1_RTS		0x1c5
+				MX51_PAD_UART1_CTS__UART1_CTS		0x1c5
+			>;
+		};
+
+		pinctrl_uart2: uart2grp {
+			fsl,pins = <
+				MX51_PAD_UART2_RXD__UART2_RXD		0x1c5
+				MX51_PAD_UART2_TXD__UART2_TXD		0x1c5
+			>;
+		};
+
+		pinctrl_uart3: uart3grp {
+			fsl,pins = <
+				MX51_PAD_EIM_D25__UART3_RXD		0x1c5
+				MX51_PAD_EIM_D26__UART3_TXD		0x1c5
+				MX51_PAD_EIM_D27__UART3_RTS		0x1c5
+				MX51_PAD_EIM_D24__UART3_CTS		0x1c5
+			>;
+		};
+
+	};
+};
-- 
2.5.0