[PATCH v2 0/6] Add board support for TS-4600

[PATCH v2 0/6] Add board support for TS-4600

[Sebastien Bourdelin]

This patch serie adds support for the TS-4600 boards rev A and B. These
boards, manufactured by Technologic Systems, are based on an i.MX28.

This serie include the support for the watchdog which could be enable
at Linux boot time depending on the bootloader.

The watchdog and few peripherals are implemented in a FPGA, and can
only be access using a custom GPIOs bit-banged bus which is called the
NBUS by Technologic Systems.
A driver for this bus is also included and used by the watchdog.

Sebastien Bourdelin (6):
  of: documentation: add bindings documentation for TS-4600
  ARM: dts: TS-4600: add basic device tree
  dt-bindings: bus: Add documentation for the Technologic Systems NBUS
  bus: add driver for the Technologic Systems NBUS
  ARM: dts: TS-4600: add NBUS support
  watchdog: ts4600: add driver for TS-4600 watchdog

 .../devicetree/bindings/arm/technologic.txt        |   5 +
 Documentation/devicetree/bindings/bus/ts-nbus.txt  |  50 +++
 .../devicetree/bindings/watchdog/ts4600-wdt.txt    |  16 +
 arch/arm/boot/dts/Makefile                         |   2 +
 arch/arm/boot/dts/imx28-ts4600-common.dtsi         | 126 ++++++++
 arch/arm/boot/dts/imx28-ts4600-rev-a.dts           |  22 ++
 arch/arm/boot/dts/imx28-ts4600-rev-b.dts           |  22 ++
 drivers/bus/Kconfig                                |   8 +
 drivers/bus/Makefile                               |   1 +
 drivers/bus/ts-nbus.c                              | 357 +++++++++++++++++++++
 drivers/watchdog/Kconfig                           |  11 +
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/ts4600_wdt.c                      | 217 +++++++++++++
 include/linux/ts-nbus.h                            |  26 ++
 14 files changed, 864 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/ts-nbus.txt
 create mode 100644 Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
 create mode 100644 arch/arm/boot/dts/imx28-ts4600-common.dtsi
 create mode 100644 arch/arm/boot/dts/imx28-ts4600-rev-a.dts
 create mode 100644 arch/arm/boot/dts/imx28-ts4600-rev-b.dts
 create mode 100644 drivers/bus/ts-nbus.c
 create mode 100644 drivers/watchdog/ts4600_wdt.c
 create mode 100644 include/linux/ts-nbus.h

-- 
2.11.0

[PATCH v2 1/6] of: documentation: add bindings documentation for TS-4600

[Sebastien Bourdelin]

This adds the documentation for the TS-4600 by Technologic Systems.

Acked-by: Rob Herring <robh@kernel.org>
---
Changes v1 -> v2:
  - rebase on master
  - add ack by Rob Herring <robh@kernel.org>

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/arm/technologic.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/technologic.txt b/Documentation/devicetree/bindings/arm/technologic.txt
index 33797acad846..e9d6d6575566 100644
--- a/Documentation/devicetree/bindings/arm/technologic.txt
+++ b/Documentation/devicetree/bindings/arm/technologic.txt
@@ -1,6 +1,11 @@
 Technologic Systems Platforms Device Tree Bindings
 --------------------------------------------------
 
+TS-4600 is a System-on-Module based on the Freescale i.MX28 System-on-Chip.
+It can be mounted on a carrier board providing additional peripheral connectors.
+Required root node properties:
+	- compatible = "technologic,imx28-ts4600", "fsl,imx28"
+
 TS-4800 board
 Required root node properties:
 	- compatible = "technologic,imx51-ts4800", "fsl,imx51";
-- 
2.11.0

Re: [PATCH v2 1/6] of: documentation: add bindings documentation for TS-4600

[Shawn Guo]

On Fri, Feb 03, 2017 at 02:47:24PM -0500, Sebastien Bourdelin wrote:
> This adds the documentation for the TS-4600 by Technologic Systems.
> 
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Changes v1 -> v2:
>   - rebase on master
>   - add ack by Rob Herring <robh@kernel.org>
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>

Your SoB should be put before the first '---'.  Otherwise, it will get
lost during patch applying.

Shawn

> ---
>  Documentation/devicetree/bindings/arm/technologic.txt | 5 +++++
>  1 file changed, 5 insertions(+)

[PATCH v2 2/6] ARM: dts: TS-4600: add basic device tree

[Sebastien Bourdelin]

These device trees add support for the TS-4600 by Technologic Systems.

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

---
Changes v1 -> v2:
  - rebase on master

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
---
 arch/arm/boot/dts/Makefile                 |  2 +
 arch/arm/boot/dts/imx28-ts4600-common.dtsi | 78 ++++++++++++++++++++++++++++++
 arch/arm/boot/dts/imx28-ts4600-rev-a.dts   | 22 +++++++++
 arch/arm/boot/dts/imx28-ts4600-rev-b.dts   | 22 +++++++++
 4 files changed, 124 insertions(+)
 create mode 100644 arch/arm/boot/dts/imx28-ts4600-common.dtsi
 create mode 100644 arch/arm/boot/dts/imx28-ts4600-rev-a.dts
 create mode 100644 arch/arm/boot/dts/imx28-ts4600-rev-b.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index f10fe8526239..be6d5e96cc16 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -482,6 +482,8 @@ dtb-$(CONFIG_ARCH_MXS) += \
 	imx28-m28cu3.dtb \
 	imx28-m28evk.dtb \
 	imx28-sps1.dtb \
+	imx28-ts4600-rev-a.dtb \
+	imx28-ts4600-rev-b.dtb \
 	imx28-tx28.dtb
 dtb-$(CONFIG_ARCH_NOMADIK) += \
 	ste-nomadik-s8815.dtb \
diff --git a/arch/arm/boot/dts/imx28-ts4600-common.dtsi b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
new file mode 100644
index 000000000000..04bd5a5c0cb4
--- /dev/null
+++ b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
@@ -0,0 +1,78 @@
+/*
+ * Copyright (C) 2016 Savoir-Faire Linux
+ * Author: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
+ *
+ * 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 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/dts-v1/;
+#include "imx28.dtsi"
+#include "dt-bindings/gpio/gpio.h"
+
+/ {
+
+	compatible = "technologic,imx28-ts4600", "fsl,imx28";
+
+	apb@80000000 {
+		apbh@80000000 {
+			ssp0: ssp@80010000 {
+				compatible = "fsl,imx28-mmc";
+				pinctrl-names = "default";
+				pinctrl-0 = <&mmc0_4bit_pins_a
+					     &mmc0_sck_cfg
+					     &en_sd_pwr>;
+				broken-cd = <1>;
+				bus-width = <4>;
+				vmmc-supply = <&reg_vddio_sd0>;
+				status = "okay";
+			};
+
+			pinctrl@80018000 {
+				pinctrl-names = "default";
+
+				en_sd_pwr: en_sd_pwr {
+					fsl,pinmux-ids = <
+						MX28_PAD_PWM3__GPIO_3_28
+					>;
+					fsl,drive-strength = <MXS_DRIVE_4mA>;
+					fsl,voltage = <MXS_VOLTAGE_HIGH>;
+					fsl,pull-up = <MXS_PULL_DISABLE>;
+				};
+
+			};
+		};
+
+		apbx@80040000 {
+			pwm: pwm@80064000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&pwm2_pins_a>;
+				status = "okay";
+			};
+
+			duart: serial@80074000 {
+				pinctrl-names = "default";
+				pinctrl-0 = <&duart_pins_a>;
+				status = "okay";
+			};
+		};
+	};
+
+	regulators {
+		compatible = "simple-bus";
+
+		reg_vddio_sd0: vddio-sd0 {
+			compatible = "regulator-fixed";
+			regulator-name = "vddio-sd0";
+			regulator-min-microvolt = <3300000>;
+			regulator-max-microvolt = <3300000>;
+			regulator-boot-on;
+			gpio = <&gpio3 28 0>;
+		};
+	};
+
+};
diff --git a/arch/arm/boot/dts/imx28-ts4600-rev-a.dts b/arch/arm/boot/dts/imx28-ts4600-rev-a.dts
new file mode 100644
index 000000000000..e8cb72988fcf
--- /dev/null
+++ b/arch/arm/boot/dts/imx28-ts4600-rev-a.dts
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2016 Savoir-Faire Linux
+ * Author: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
+ *
+ * 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 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include "imx28-ts4600-common.dtsi"
+
+/ {
+	model = "Technologic Systems i.MX28 TS-4600 Rev A";
+
+	memory {
+		reg = <0x40000000 0x08000000>;   /* 128MB */
+	};
+
+};
diff --git a/arch/arm/boot/dts/imx28-ts4600-rev-b.dts b/arch/arm/boot/dts/imx28-ts4600-rev-b.dts
new file mode 100644
index 000000000000..a115f831fe2b
--- /dev/null
+++ b/arch/arm/boot/dts/imx28-ts4600-rev-b.dts
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2016 Savoir-Faire Linux
+ * Author: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
+ *
+ * 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 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include "imx28-ts4600-common.dtsi"
+
+/ {
+	model = "Technologic Systems i.MX28 TS-4600 Rev B";
+
+	memory {
+		reg = <0x40000000 0x10000000>;   /* 256MB */
+	};
+
+};
-- 
2.11.0

[PATCH v2 3/6] dt-bindings: bus: Add documentation for the Technologic Systems NBUS

[Sebastien Bourdelin]

Add binding documentation for the Technologic Systems NBUS that is used
to interface with peripherals in the FPGA of the TS-4600 SoM.

---
Changes v1 -> v2:
  - rebase on master
  - remove the simple-bus compatibility as the root node will now
  populate child nodes (suggested by Rob Herring)
  - use the ts vendor prefix for gpios (suggested by Rob Herring)

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/bus/ts-nbus.txt | 50 +++++++++++++++++++++++
 1 file changed, 50 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/ts-nbus.txt

diff --git a/Documentation/devicetree/bindings/bus/ts-nbus.txt b/Documentation/devicetree/bindings/bus/ts-nbus.txt
new file mode 100644
index 000000000000..c8a1f2cbe6a0
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/ts-nbus.txt
@@ -0,0 +1,50 @@
+Technologic Systems NBUS
+
+The NBUS is a bus used to interface with peripherals in the Technologic
+Systems FPGA on the TS-4600 SoM.
+
+Required properties :
+ - compatible     : "technologic,ts-nbus"
+ - #address-cells : must be 1
+ - #size-cells    : must be 0
+ - pws            : The PWM binded to the FPGA
+ - data-gpios	  : The GPIO pin connected to the data line on the FPGA
+ - csn-gpios	  : The GPIO pin connected to the csn line on the FPGA
+ - txrx-gpios	  : The GPIO pin connected to the txrx line on the FPGA
+ - strobe-gpios	  : The GPIO pin connected to the stobe line on the FPGA
+ - ale-gpios	  : The GPIO pin connected to the ale line on the FPGA
+ - rdy-gpios	  : The GPIO pin connected to the rdy line on the FPGA
+
+Child nodes:
+
+The NBUS node can contain zero or more child nodes representing peripherals
+on the bus.
+
+Example:
+
+	nbus {
+		compatible = "technologic,ts-nbus";
+		pinctrl-0 = <&nbus_pins>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pwms = <&pwm 2 83>;
+		ts-data-gpios   = <&gpio0 0 GPIO_ACTIVE_HIGH
+				   &gpio0 1 GPIO_ACTIVE_HIGH
+				   &gpio0 2 GPIO_ACTIVE_HIGH
+				   &gpio0 3 GPIO_ACTIVE_HIGH
+				   &gpio0 4 GPIO_ACTIVE_HIGH
+				   &gpio0 5 GPIO_ACTIVE_HIGH
+				   &gpio0 6 GPIO_ACTIVE_HIGH
+				   &gpio0 7 GPIO_ACTIVE_HIGH>;
+		ts-csn-gpios    = <&gpio0 16 GPIO_ACTIVE_HIGH>;
+		ts-txrx-gpios   = <&gpio0 24 GPIO_ACTIVE_HIGH>;
+		ts-strobe-gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
+		ts-ale-gpios    = <&gpio0 26 GPIO_ACTIVE_HIGH>;
+		ts-rdy-gpios    = <&gpio0 21 GPIO_ACTIVE_HIGH>;
+
+		watchdog@2a {
+			compatible = "...";
+
+			/* ... */
+		};
+	};
-- 
2.11.0

Re: [PATCH v2 3/6] dt-bindings: bus: Add documentation for the Technologic Systems NBUS

[Rob Herring]

On Fri, Feb 03, 2017 at 02:47:26PM -0500, Sebastien Bourdelin wrote:
> Add binding documentation for the Technologic Systems NBUS that is used
> to interface with peripherals in the FPGA of the TS-4600 SoM.
> 
> ---
> Changes v1 -> v2:
>   - rebase on master
>   - remove the simple-bus compatibility as the root node will now
>   populate child nodes (suggested by Rob Herring)
>   - use the ts vendor prefix for gpios (suggested by Rob Herring)
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/bus/ts-nbus.txt | 50 +++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/ts-nbus.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/ts-nbus.txt b/Documentation/devicetree/bindings/bus/ts-nbus.txt
> new file mode 100644
> index 000000000000..c8a1f2cbe6a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/ts-nbus.txt
> @@ -0,0 +1,50 @@
> +Technologic Systems NBUS
> +
> +The NBUS is a bus used to interface with peripherals in the Technologic
> +Systems FPGA on the TS-4600 SoM.
> +
> +Required properties :
> + - compatible     : "technologic,ts-nbus"
> + - #address-cells : must be 1
> + - #size-cells    : must be 0
> + - pws            : The PWM binded to the FPGA

Should be pwms?

s/binded/bound/

> + - data-gpios	  : The GPIO pin connected to the data line on the FPGA

lines? How many? Always 8 like the example?

> + - csn-gpios	  : The GPIO pin connected to the csn line on the FPGA
> + - txrx-gpios	  : The GPIO pin connected to the txrx line on the FPGA
> + - strobe-gpios	  : The GPIO pin connected to the stobe line on the FPGA
> + - ale-gpios	  : The GPIO pin connected to the ale line on the FPGA
> + - rdy-gpios	  : The GPIO pin connected to the rdy line on the FPGA
> +
> +Child nodes:
> +
> +The NBUS node can contain zero or more child nodes representing peripherals
> +on the bus.
> +
> +Example:
> +
> +	nbus {
> +		compatible = "technologic,ts-nbus";
> +		pinctrl-0 = <&nbus_pins>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		pwms = <&pwm 2 83>;
> +		ts-data-gpios   = <&gpio0 0 GPIO_ACTIVE_HIGH
> +				   &gpio0 1 GPIO_ACTIVE_HIGH
> +				   &gpio0 2 GPIO_ACTIVE_HIGH
> +				   &gpio0 3 GPIO_ACTIVE_HIGH
> +				   &gpio0 4 GPIO_ACTIVE_HIGH
> +				   &gpio0 5 GPIO_ACTIVE_HIGH
> +				   &gpio0 6 GPIO_ACTIVE_HIGH
> +				   &gpio0 7 GPIO_ACTIVE_HIGH>;
> +		ts-csn-gpios    = <&gpio0 16 GPIO_ACTIVE_HIGH>;
> +		ts-txrx-gpios   = <&gpio0 24 GPIO_ACTIVE_HIGH>;
> +		ts-strobe-gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
> +		ts-ale-gpios    = <&gpio0 26 GPIO_ACTIVE_HIGH>;
> +		ts-rdy-gpios    = <&gpio0 21 GPIO_ACTIVE_HIGH>;
> +
> +		watchdog@2a {
> +			compatible = "...";
> +
> +			/* ... */
> +		};
> +	};
> -- 
> 2.11.0
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/6] bus: add driver for the Technologic Systems NBUS

[Sebastien Bourdelin]

This driver implements a GPIOs bit-banged bus, called the NBUS by
Technologic Systems. It is used to communicate with the peripherals in
the FPGA on the TS-4600 SoM.

---
Changes v1 -> v2:
  - rebase on master
  - the driver now populate its child nodes
  - remove the 'default y' option from the Kconfig
  - rework the driver to not use singleton anymore (suggested by Linus
  Walleij)
  - replace the use of the legacy GPIO API with gpiod (suggested by
  Linus Walleij)
  - use the ts vendor prefix for gpios (suggested by Rob Herring)

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
---
 drivers/bus/Kconfig     |   8 ++
 drivers/bus/Makefile    |   1 +
 drivers/bus/ts-nbus.c   | 357 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/ts-nbus.h |  26 ++++
 4 files changed, 392 insertions(+)
 create mode 100644 drivers/bus/ts-nbus.c
 create mode 100644 include/linux/ts-nbus.h

diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index b9e8cfc93c7e..e573f1c27162 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -157,6 +157,14 @@ config TEGRA_GMI
 	  Driver for the Tegra Generic Memory Interface bus which can be used
 	  to attach devices such as NOR, UART, FPGA and more.
 
+config TS_NBUS
+	tristate "Technologic Systems NBUS Driver"
+	depends on SOC_IMX28
+	depends on OF_GPIO && PWM
+	help
+	  Driver for the Technologic Systems NBUS which is used to interface
+	  with the peripherals in the FPGA of the TS-4600 SoM.
+
 config UNIPHIER_SYSTEM_BUS
 	tristate "UniPhier System Bus driver"
 	depends on ARCH_UNIPHIER && OF
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index cc6364bec054..72377f77651c 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_SUNXI_RSB)		+= sunxi-rsb.o
 obj-$(CONFIG_SIMPLE_PM_BUS)	+= simple-pm-bus.o
 obj-$(CONFIG_TEGRA_ACONNECT)	+= tegra-aconnect.o
 obj-$(CONFIG_TEGRA_GMI)		+= tegra-gmi.o
+obj-$(CONFIG_TS_NBUS)		+= ts-nbus.o
 obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
 obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
 
diff --git a/drivers/bus/ts-nbus.c b/drivers/bus/ts-nbus.c
new file mode 100644
index 000000000000..d502d97adbd5
--- /dev/null
+++ b/drivers/bus/ts-nbus.c
@@ -0,0 +1,357 @@
+/*
+ * NBUS driver for TS-4600 based boards
+ *
+ * Copyright (c) 2016 - Savoir-faire Linux
+ * Author: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
+ *
+ * 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.
+ *
+ * This driver implements a GPIOs bit-banged bus, called the NBUS by Technologic
+ * Systems. It is used to communicate with the peripherals in the FPGA on the
+ * TS-4600 SoM.
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/ts-nbus.h>
+
+static DEFINE_MUTEX(ts_nbus_lock);
+
+#define TS_NBUS_READ_MODE  0
+#define TS_NBUS_WRITE_MODE 1
+#define TS_NBUS_DIRECTION_IN  0
+#define TS_NBUS_DIRECTION_OUT 1
+#define TS_NBUS_WRITE_ADR 0
+#define TS_NBUS_WRITE_VAL 1
+
+/*
+ * request all gpios required by the bus.
+ */
+static int ts_nbus_init_pdata(struct platform_device *pdev, struct ts_nbus
+		*ts_nbus)
+{
+	ts_nbus->data = devm_gpiod_get_array(&pdev->dev, "ts-data",
+			GPIOD_OUT_HIGH);
+	if (IS_ERR(ts_nbus->data)) {
+		dev_err(&pdev->dev, "failed to retrieve ts-data-gpio from dts\n");
+		return PTR_ERR(ts_nbus->data);
+	}
+
+	ts_nbus->csn = devm_gpiod_get(&pdev->dev, "ts-csn", GPIOD_OUT_HIGH);
+	if (IS_ERR(ts_nbus->csn)) {
+		dev_err(&pdev->dev, "failed to retrieve ts-csn-gpio from dts\n");
+		return PTR_ERR(ts_nbus->csn);
+	}
+
+	ts_nbus->txrx = devm_gpiod_get(&pdev->dev, "ts-txrx", GPIOD_OUT_HIGH);
+	if (IS_ERR(ts_nbus->txrx)) {
+		dev_err(&pdev->dev, "failed to retrieve ts-txrx-gpio from dts\n");
+		return PTR_ERR(ts_nbus->txrx);
+	}
+
+	ts_nbus->strobe = devm_gpiod_get(&pdev->dev, "ts-strobe", GPIOD_OUT_HIGH);
+	if (IS_ERR(ts_nbus->strobe)) {
+		dev_err(&pdev->dev, "failed to retrieve ts-strobe-gpio from dts\n");
+		return PTR_ERR(ts_nbus->strobe);
+	}
+
+	ts_nbus->ale = devm_gpiod_get(&pdev->dev, "ts-ale", GPIOD_OUT_HIGH);
+	if (IS_ERR(ts_nbus->ale)) {
+		dev_err(&pdev->dev, "failed to retrieve ts-ale-gpio from dts\n");
+		return PTR_ERR(ts_nbus->ale);
+	}
+
+	ts_nbus->rdy = devm_gpiod_get(&pdev->dev, "ts-rdy", GPIOD_IN);
+	if (IS_ERR(ts_nbus->rdy)) {
+		dev_err(&pdev->dev, "failed to retrieve ts-rdy-gpio from dts\n");
+		return PTR_ERR(ts_nbus->rdy);
+	}
+
+	return 0;
+}
+
+/*
+ * the txrx gpio is used by the FPGA to know if the following transactions
+ * should be handled to read or write a value.
+ */
+static inline void ts_nbus_set_mode(struct ts_nbus *ts_nbus, int mode)
+{
+	if (mode == TS_NBUS_READ_MODE)
+		gpiod_set_value_cansleep(ts_nbus->txrx, 0);
+	else
+		gpiod_set_value_cansleep(ts_nbus->txrx, 1);
+}
+
+/*
+ * the data gpios are used for reading and writing values, their directions
+ * should be adjusted accordingly.
+ */
+static inline void ts_nbus_set_direction(struct ts_nbus *ts_nbus, int direction)
+{
+	int i;
+
+	for (i = 0; i < ts_nbus->data->ndescs; i++) {
+		if (direction == TS_NBUS_DIRECTION_IN)
+			gpiod_direction_input(ts_nbus->data->desc[i]);
+		else
+			gpiod_direction_output(ts_nbus->data->desc[i], 1);
+	}
+}
+
+/*
+ * reset the bus in its initial state.
+ */
+static inline void ts_nbus_reset_bus(struct ts_nbus *ts_nbus)
+{
+	int i;
+	int values[ts_nbus->data->ndescs];
+
+	for (i = 0; i < ts_nbus->data->ndescs; i++)
+		values[i] = 0;
+
+	gpiod_set_array_value_cansleep(ts_nbus->data->ndescs,
+			ts_nbus->data->desc, values);
+	gpiod_set_value_cansleep(ts_nbus->csn, 0);
+	gpiod_set_value_cansleep(ts_nbus->strobe, 0);
+	gpiod_set_value_cansleep(ts_nbus->ale, 0);
+}
+
+/*
+ * let the FPGA knows it can process.
+ */
+static inline void ts_nbus_start_transaction(struct ts_nbus *ts_nbus)
+{
+	gpiod_set_value_cansleep(ts_nbus->strobe, 1);
+}
+
+/*
+ * return the byte value read from the data gpios.
+ */
+static inline u8 ts_nbus_read_byte(struct ts_nbus *ts_nbus)
+{
+	struct gpio_descs *gpios = ts_nbus->data;
+	int i;
+	u8 value = 0;
+
+	for (i = 0; i < gpios->ndescs; i++)
+		if (gpiod_get_value_cansleep(gpios->desc[i]))
+			value |= 1 << i;
+
+	return value;
+}
+
+/*
+ * set the data gpios accordingly to the byte value.
+ */
+static inline void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
+{
+	struct gpio_descs *gpios = ts_nbus->data;
+	int i;
+	int values[gpios->ndescs];
+
+	for (i = 0; i < gpios->ndescs; i++)
+		if (byte & (1 << i))
+			values[i] = 1;
+		else
+			values[i] = 0;
+
+	gpiod_set_array_value_cansleep(gpios->ndescs, gpios->desc, values);
+}
+
+/*
+ * reading the bus consists of resetting the bus, then notifying the FPGA to
+ * send the data in the data gpios and return the read value.
+ */
+static inline u8 ts_nbus_read_bus(struct ts_nbus *ts_nbus)
+{
+	ts_nbus_reset_bus(ts_nbus);
+	ts_nbus_start_transaction(ts_nbus);
+
+	return ts_nbus_read_byte(ts_nbus);
+}
+
+/*
+ * writing to the bus consists of resetting the bus, then define the type of
+ * command (address/value), write the data and notify the FPGA to retrieve the
+ * value in the data gpios.
+ */
+static inline void ts_nbus_write_bus(struct ts_nbus *ts_nbus, int cmd, u8 value)
+{
+	ts_nbus_reset_bus(ts_nbus);
+
+	if (cmd == TS_NBUS_WRITE_ADR)
+		gpiod_set_value_cansleep(ts_nbus->ale, 1);
+
+	ts_nbus_write_byte(ts_nbus, value);
+	ts_nbus_start_transaction(ts_nbus);
+}
+
+/*
+ * read the value in the FPGA register at the given address.
+ */
+u16 ts_nbus_read(struct ts_nbus *ts_nbus, u8 adr)
+{
+	int i;
+	u16 val;
+
+	/* bus access must be atomic */
+	mutex_lock(&ts_nbus_lock);
+
+	/* set the bus in read mode */
+	ts_nbus_set_mode(ts_nbus, TS_NBUS_READ_MODE);
+
+	/* write address */
+	ts_nbus_write_bus(ts_nbus, TS_NBUS_WRITE_ADR, adr);
+
+	/* set the data gpios direction as input before reading */
+	ts_nbus_set_direction(ts_nbus, TS_NBUS_DIRECTION_IN);
+
+	/* reading value MSB first */
+	do {
+		val = 0;
+		for (i = 1; i >= 0; i--)
+			val |= (ts_nbus_read_bus(ts_nbus) << (i * 8));
+		gpiod_set_value_cansleep(ts_nbus->csn, 1);
+	} while (gpiod_get_value_cansleep(ts_nbus->rdy));
+
+	/* restore the data gpios direction as output after reading */
+	ts_nbus_set_direction(ts_nbus, TS_NBUS_DIRECTION_OUT);
+
+	mutex_unlock(&ts_nbus_lock);
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(ts_nbus_read);
+
+/*
+ * write the desired value in the FPGA register at the given address.
+ */
+int ts_nbus_write(struct ts_nbus *ts_nbus, u8 adr, u16 value)
+{
+	int i;
+
+	/* bus access must be atomic */
+	mutex_lock(&ts_nbus_lock);
+
+	/* set the bus in write mode */
+	ts_nbus_set_mode(ts_nbus, TS_NBUS_WRITE_MODE);
+
+	/* write address */
+	ts_nbus_write_bus(ts_nbus, TS_NBUS_WRITE_ADR, adr);
+
+	/* writing value MSB first */
+	for (i = 1; i >= 0; i--)
+		ts_nbus_write_bus(ts_nbus, TS_NBUS_WRITE_VAL, (u8)(value >> (i * 8)));
+
+	/* wait for completion */
+	gpiod_set_value_cansleep(ts_nbus->csn, 1);
+	while (gpiod_get_value_cansleep(ts_nbus->rdy) != 0) {
+		gpiod_set_value_cansleep(ts_nbus->csn, 0);
+		gpiod_set_value_cansleep(ts_nbus->csn, 1);
+	}
+
+	mutex_unlock(&ts_nbus_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ts_nbus_write);
+
+static int ts_nbus_probe(struct platform_device *pdev)
+{
+	struct pwm_device *pwm;
+	struct pwm_args pargs;
+	struct device *dev = &pdev->dev;
+	struct ts_nbus *ts_nbus;
+	int ret;
+
+	ts_nbus = devm_kzalloc(dev, sizeof(*ts_nbus), GFP_KERNEL);
+	if (!ts_nbus)
+		return -ENOMEM;
+
+	ret = ts_nbus_init_pdata(pdev, ts_nbus);
+	if (ret < 0)
+		return ret;
+
+	pwm = devm_pwm_get(dev, NULL);
+	if (IS_ERR(pwm)) {
+		ret = PTR_ERR(pwm);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "unable to request PWM\n");
+		return ret;
+	}
+
+	pwm_get_args(pwm, &pargs);
+	if (!pargs.period) {
+		dev_err(&pdev->dev, "invalid PWM period\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * FIXME: pwm_apply_args() should be removed when switching to
+	 * the atomic PWM API.
+	 */
+	pwm_apply_args(pwm);
+	ret = pwm_config(pwm, pargs.period, pargs.period);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * we can now start the FPGA and populate the peripherals.
+	 */
+	pwm_enable(pwm);
+	ts_nbus->pwm = pwm;
+
+	/*
+	 * let the child nodes retrieve this instance of the ts-nbus.
+	 */
+	dev_set_drvdata(dev, ts_nbus);
+
+	ret = of_platform_populate(dev->of_node, NULL, NULL, dev);
+	if (ret < 0)
+		return ret;
+
+	dev_info(dev, "initialized\n");
+
+	return 0;
+}
+
+static int ts_nbus_remove(struct platform_device *pdev)
+{
+	struct ts_nbus *ts_nbus = dev_get_drvdata(&pdev->dev);
+
+	/* shutdown the FPGA */
+	mutex_lock(&ts_nbus_lock);
+	pwm_disable(ts_nbus->pwm);
+	mutex_unlock(&ts_nbus_lock);
+
+	return 0;
+}
+
+static const struct of_device_id ts_nbus_of_match[] = {
+	{ .compatible = "technologic,ts-nbus", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ts_nbus_of_match);
+
+static struct platform_driver ts_nbus_driver = {
+	.probe		= ts_nbus_probe,
+	.remove		= ts_nbus_remove,
+	.driver		= {
+		.name	= "ts_nbus",
+		.of_match_table = ts_nbus_of_match,
+	},
+};
+
+module_platform_driver(ts_nbus_driver);
+
+MODULE_ALIAS("platform:ts_nbus");
+MODULE_AUTHOR("Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>");
+MODULE_DESCRIPTION("Technologic Systems NBUS");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/ts-nbus.h b/include/linux/ts-nbus.h
new file mode 100644
index 000000000000..c0055d9fc9e6
--- /dev/null
+++ b/include/linux/ts-nbus.h
@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2016 - Savoir-faire Linux
+ * Author: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
+ *
+ * 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.
+ */
+
+#ifndef _TS_NBUS_H
+#define _TS_NBUS_H
+
+struct ts_nbus {
+	struct pwm_device *pwm;
+	struct gpio_descs *data;
+	struct gpio_desc *csn;
+	struct gpio_desc *txrx;
+	struct gpio_desc *strobe;
+	struct gpio_desc *ale;
+	struct gpio_desc *rdy;
+};
+
+extern u16 ts_nbus_read(struct ts_nbus *, u8 adr);
+extern int ts_nbus_write(struct ts_nbus *, u8 adr, u16 value);
+
+#endif /* _TS_NBUS_H */
-- 
2.11.0

Re: [PATCH v2 4/6] bus: add driver for the Technologic Systems NBUS

[Linus Walleij]

On Fri, Feb 3, 2017 at 8:47 PM, Sebastien Bourdelin
<sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org> wrote:

> This driver implements a GPIOs bit-banged bus, called the NBUS by
> Technologic Systems. It is used to communicate with the peripherals in
> the FPGA on the TS-4600 SoM.
>
> ---
> Changes v1 -> v2:
>   - rebase on master
>   - the driver now populate its child nodes
>   - remove the 'default y' option from the Kconfig
>   - rework the driver to not use singleton anymore (suggested by Linus
>   Walleij)
>   - replace the use of the legacy GPIO API with gpiod (suggested by
>   Linus Walleij)
>   - use the ts vendor prefix for gpios (suggested by Rob Herring)
>
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>

This is starting to look nice!

More comments:

First, you sprinked "inline" like cinnamon over everything, skip that. The
compiler will inline selectively as it seems fit if you turn on optimization.

> +static DEFINE_MUTEX(ts_nbus_lock);

Move this into struct ts_nbus, initialize the mutex in probe()
and just mutex_lock(&ts_nbus->lock); etc.

> + * the txrx gpio is used by the FPGA to know if the following transactions
> + * should be handled to read or write a value.
> + */
> +static inline void ts_nbus_set_mode(struct ts_nbus *ts_nbus, int mode)

Why inline? Let the compiler decide about that.

> +{
> +       if (mode == TS_NBUS_READ_MODE)

This mode parameter is too complicated. Isn't it just a bool?
(not superimportant)

> +               gpiod_set_value_cansleep(ts_nbus->txrx, 0);
> +       else
> +               gpiod_set_value_cansleep(ts_nbus->txrx, 1);
> +}

You're certainly just using it as a bool.

> +static inline void ts_nbus_set_direction(struct ts_nbus *ts_nbus, int direction)
> +{
> +       int i;
> +
> +       for (i = 0; i < ts_nbus->data->ndescs; i++) {
> +               if (direction == TS_NBUS_DIRECTION_IN)
> +                       gpiod_direction_input(ts_nbus->data->desc[i]);
> +               else
> +                       gpiod_direction_output(ts_nbus->data->desc[i], 1);

Add a comment here explaining why you driver the line high by default
when setting it to output mode. It certainly makes sense but how
does the electronics work and why?

> +static inline void ts_nbus_reset_bus(struct ts_nbus *ts_nbus)
> +{
> +       int i;
> +       int values[ts_nbus->data->ndescs];
> +
> +       for (i = 0; i < ts_nbus->data->ndescs; i++)
> +               values[i] = 0;
> +
> +       gpiod_set_array_value_cansleep(ts_nbus->data->ndescs,
> +                       ts_nbus->data->desc, values);
> +       gpiod_set_value_cansleep(ts_nbus->csn, 0);
> +       gpiod_set_value_cansleep(ts_nbus->strobe, 0);
> +       gpiod_set_value_cansleep(ts_nbus->ale, 0);

Add a comment about the process taken when this bus is reset. We
see what is happening, but why?

> +/*
> + * let the FPGA knows it can process.
> + */
> +static inline void ts_nbus_start_transaction(struct ts_nbus *ts_nbus)

Why inline?

> +{
> +       gpiod_set_value_cansleep(ts_nbus->strobe, 1);
> +}

For example this is a well commented function. We see what is happening
and stobe has an evident name. Nice work!

> +/*
> + * return the byte value read from the data gpios.
> + */
> +static inline u8 ts_nbus_read_byte(struct ts_nbus *ts_nbus)

Why inline?

Then this cannot fail can it?

I think this accessor should be changed to return an error code.

static int ts_nbus_read_byte(struct ts_nbus *ts_nbus, u8 *val);

Then if the return value from the function is != 0 it failed. Simple
to check, just pass a pointer to the value you are getting in
val.

I KNOW I KNOW, not you have to put in error handling everywhere,
what a DRAG! But we usually follow that pattern because...

> +{
> +       struct gpio_descs *gpios = ts_nbus->data;
> +       int i;
> +       u8 value = 0;
> +
> +       for (i = 0; i < gpios->ndescs; i++)

Using gpios->ndescs is a bit overparametrized right? Isn't it
simply 8 bits? Just putting 8 there is fine IMO. All 8 descs
must be available for the thing to work anyway right?

> +               if (gpiod_get_value_cansleep(gpios->desc[i]))

This can fail and you should return an error if < 0...

You are ignoring that right now. That is why the function should
be returning an error code that is usually 0.

> +                       value |= 1 << i;

Use:
#include <linux/bitops.h>

value |= BIT(i);

> +/*
> + * set the data gpios accordingly to the byte value.
> + */
> +static inline void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
> +{
> +       struct gpio_descs *gpios = ts_nbus->data;
> +       int i;
> +       int values[gpios->ndescs];
> +
> +       for (i = 0; i < gpios->ndescs; i++)
> +               if (byte & (1 << i))
> +                       values[i] = 1;
> +               else
> +                       values[i] = 0;
> +
> +       gpiod_set_array_value_cansleep(gpios->ndescs, gpios->desc, values);

This can also fail and you should check the return code and print an error
message if it does.

> +/*
> + * reading the bus consists of resetting the bus, then notifying the FPGA to
> + * send the data in the data gpios and return the read value.
> + */
> +static inline u8 ts_nbus_read_bus(struct ts_nbus *ts_nbus)

All this inline...

> +/*
> + * writing to the bus consists of resetting the bus, then define the type of
> + * command (address/value), write the data and notify the FPGA to retrieve the
> + * value in the data gpios.
> + */
> +static inline void ts_nbus_write_bus(struct ts_nbus *ts_nbus, int cmd, u8 value)
> +{
> +       ts_nbus_reset_bus(ts_nbus);
> +
> +       if (cmd == TS_NBUS_WRITE_ADR)
> +               gpiod_set_value_cansleep(ts_nbus->ale, 1);
> +
> +       ts_nbus_write_byte(ts_nbus, value);
> +       ts_nbus_start_transaction(ts_nbus);

Error codes?

> +       /* reading value MSB first */
> +       do {
> +               val = 0;
> +               for (i = 1; i >= 0; i--)
> +                       val |= (ts_nbus_read_bus(ts_nbus) << (i * 8));

Hard to deal with errors in this loop!

> +               gpiod_set_value_cansleep(ts_nbus->csn, 1);

Here too.

> +++ b/include/linux/ts-nbus.h
> @@ -0,0 +1,26 @@
> +/*
> + * Copyright (c) 2016 - Savoir-faire Linux
> + * Author: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
> + *
> + * 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.
> + */
> +
> +#ifndef _TS_NBUS_H
> +#define _TS_NBUS_H
> +
> +struct ts_nbus {
> +       struct pwm_device *pwm;
> +       struct gpio_descs *data;
> +       struct gpio_desc *csn;
> +       struct gpio_desc *txrx;
> +       struct gpio_desc *strobe;
> +       struct gpio_desc *ale;
> +       struct gpio_desc *rdy;
> +};

Is any consumer really looking into this struct? If not, why
do you expose it?

Move it to the ts-nbus.c file.

The only thing you need in this header is:

struct ts_nbus;

Just that one line. The rest of the code, like the declarations
below and all call sites, are just dealing with "pointers to something",
they don't need to know what it is. You'll be amazed to see how it compiles.
struct gpio_desc works exactly like that.

> +extern u16 ts_nbus_read(struct ts_nbus *, u8 adr);
> +extern int ts_nbus_write(struct ts_nbus *, u8 adr, u16 value);

I suspect the first function should be augmented to return an error code.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/6] bus: add driver for the Technologic Systems NBUS

[Sebastien Bourdelin]

Hi Linus,

First thanks for your code review!

On 02/04/2017 05:14 AM, Linus Walleij wrote:
> On Fri, Feb 3, 2017 at 8:47 PM, Sebastien Bourdelin
> <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org> wrote:
> 
>> This driver implements a GPIOs bit-banged bus, called the NBUS by
>> Technologic Systems. It is used to communicate with the peripherals in
>> the FPGA on the TS-4600 SoM.
>>
>> ---
>> Changes v1 -> v2:
>>   - rebase on master
>>   - the driver now populate its child nodes
>>   - remove the 'default y' option from the Kconfig
>>   - rework the driver to not use singleton anymore (suggested by Linus
>>   Walleij)
>>   - replace the use of the legacy GPIO API with gpiod (suggested by
>>   Linus Walleij)
>>   - use the ts vendor prefix for gpios (suggested by Rob Herring)
>>
>> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
> 
> This is starting to look nice!
> 
> More comments:
> 
> First, you sprinked "inline" like cinnamon over everything, skip that. The
> compiler will inline selectively as it seems fit if you turn on optimization.
> 
Ok.

>> +static DEFINE_MUTEX(ts_nbus_lock);
> 
> Move this into struct ts_nbus, initialize the mutex in probe()
> and just mutex_lock(&ts_nbus->lock); etc.
> 
Ok.

>> + * the txrx gpio is used by the FPGA to know if the following transactions
>> + * should be handled to read or write a value.
>> + */
>> +static inline void ts_nbus_set_mode(struct ts_nbus *ts_nbus, int mode)
> 
> Why inline? Let the compiler decide about that.
> 
Ok.

>> +{
>> +       if (mode == TS_NBUS_READ_MODE)
> 
> This mode parameter is too complicated. Isn't it just a bool?
> (not superimportant)
> 
>> +               gpiod_set_value_cansleep(ts_nbus->txrx, 0);
>> +       else
>> +               gpiod_set_value_cansleep(ts_nbus->txrx, 1);
>> +}
> 
> You're certainly just using it as a bool.
> 
I will remove this function to simplify the logic.

>> +static inline void ts_nbus_set_direction(struct ts_nbus *ts_nbus, int direction)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < ts_nbus->data->ndescs; i++) {
>> +               if (direction == TS_NBUS_DIRECTION_IN)
>> +                       gpiod_direction_input(ts_nbus->data->desc[i]);
>> +               else
>> +                       gpiod_direction_output(ts_nbus->data->desc[i], 1);
> 
> Add a comment here explaining why you driver the line high by default
> when setting it to output mode. It certainly makes sense but how
> does the electronics work and why?
> 
Ok.

>> +static inline void ts_nbus_reset_bus(struct ts_nbus *ts_nbus)
>> +{
>> +       int i;
>> +       int values[ts_nbus->data->ndescs];
>> +
>> +       for (i = 0; i < ts_nbus->data->ndescs; i++)
>> +               values[i] = 0;
>> +
>> +       gpiod_set_array_value_cansleep(ts_nbus->data->ndescs,
>> +                       ts_nbus->data->desc, values);
>> +       gpiod_set_value_cansleep(ts_nbus->csn, 0);
>> +       gpiod_set_value_cansleep(ts_nbus->strobe, 0);
>> +       gpiod_set_value_cansleep(ts_nbus->ale, 0);
> 
> Add a comment about the process taken when this bus is reset. We
> see what is happening, but why?
> 
Ok.

>> +/*
>> + * let the FPGA knows it can process.
>> + */
>> +static inline void ts_nbus_start_transaction(struct ts_nbus *ts_nbus)
> 
> Why inline?
> 
I will remove all the inline.

>> +{
>> +       gpiod_set_value_cansleep(ts_nbus->strobe, 1);
>> +}
> 
> For example this is a well commented function. We see what is happening
> and stobe has an evident name. Nice work!
> 
>> +/*
>> + * return the byte value read from the data gpios.
>> + */
>> +static inline u8 ts_nbus_read_byte(struct ts_nbus *ts_nbus)
> 
> Why inline?
> 
Ditto.

> Then this cannot fail can it?
> 
> I think this accessor should be changed to return an error code.
> 
> static int ts_nbus_read_byte(struct ts_nbus *ts_nbus, u8 *val);
> 
> Then if the return value from the function is != 0 it failed. Simple
> to check, just pass a pointer to the value you are getting in
> val.
> 
> I KNOW I KNOW, not you have to put in error handling everywhere,
> what a DRAG! But we usually follow that pattern because...
> 
>> +{
>> +       struct gpio_descs *gpios = ts_nbus->data;
>> +       int i;
>> +       u8 value = 0;
>> +
>> +       for (i = 0; i < gpios->ndescs; i++)
> 
> Using gpios->ndescs is a bit overparametrized right? Isn't it
> simply 8 bits? Just putting 8 there is fine IMO. All 8 descs
> must be available for the thing to work anyway right?
> 
>> +               if (gpiod_get_value_cansleep(gpios->desc[i]))
> 
> This can fail and you should return an error if < 0...
> 
> You are ignoring that right now. That is why the function should
> be returning an error code that is usually 0.
> 
You are right,
this can fail, i will propagate the "gpiod_get_value_cansleep(...)"
errno value.

>> +                       value |= 1 << i;
> 
> Use:
> #include <linux/bitops.h>
> 
> value |= BIT(i);
>
Ok.

>> +/*
>> + * set the data gpios accordingly to the byte value.
>> + */
>> +static inline void ts_nbus_write_byte(struct ts_nbus *ts_nbus, u8 byte)
>> +{
>> +       struct gpio_descs *gpios = ts_nbus->data;
>> +       int i;
>> +       int values[gpios->ndescs];
>> +
>> +       for (i = 0; i < gpios->ndescs; i++)
>> +               if (byte & (1 << i))
>> +                       values[i] = 1;
>> +               else
>> +                       values[i] = 0;
>> +
>> +       gpiod_set_array_value_cansleep(gpios->ndescs, gpios->desc, values);
> 
> This can also fail and you should check the return code and print an error
> message if it does.
> 
As far as i understood, the "gpiod_set_array_value_cansleep(...)"
function doesn't return
anything, it will return immediately if gpios->desc is null but nothing
else.
Did i miss something?

>> +/*
>> + * reading the bus consists of resetting the bus, then notifying the FPGA to
>> + * send the data in the data gpios and return the read value.
>> + */
>> +static inline u8 ts_nbus_read_bus(struct ts_nbus *ts_nbus)
> 
> All this inline...
Ok :)

> 
>> +/*
>> + * writing to the bus consists of resetting the bus, then define the type of
>> + * command (address/value), write the data and notify the FPGA to retrieve the
>> + * value in the data gpios.
>> + */
>> +static inline void ts_nbus_write_bus(struct ts_nbus *ts_nbus, int cmd, u8 value)
>> +{
>> +       ts_nbus_reset_bus(ts_nbus);
>> +
>> +       if (cmd == TS_NBUS_WRITE_ADR)
>> +               gpiod_set_value_cansleep(ts_nbus->ale, 1);
>> +
>> +       ts_nbus_write_byte(ts_nbus, value);
>> +       ts_nbus_start_transaction(ts_nbus);
> 
> Error codes?
> 
Same question here, these functions only make call to
"gpiod_set_value_cansleep(...)",
which as far as i understood doesn't return any error code.

>> +       /* reading value MSB first */
>> +       do {
>> +               val = 0;
>> +               for (i = 1; i >= 0; i--)
>> +                       val |= (ts_nbus_read_bus(ts_nbus) << (i * 8));
> 
> Hard to deal with errors in this loop!
I will reformat this part.

> 
>> +               gpiod_set_value_cansleep(ts_nbus->csn, 1);
> 
> Here too.
>
Same question, doesn't seem to have an error code.

>> +++ b/include/linux/ts-nbus.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Copyright (c) 2016 - Savoir-faire Linux
>> + * Author: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef _TS_NBUS_H
>> +#define _TS_NBUS_H
>> +
>> +struct ts_nbus {
>> +       struct pwm_device *pwm;
>> +       struct gpio_descs *data;
>> +       struct gpio_desc *csn;
>> +       struct gpio_desc *txrx;
>> +       struct gpio_desc *strobe;
>> +       struct gpio_desc *ale;
>> +       struct gpio_desc *rdy;
>> +};
> 
> Is any consumer really looking into this struct? If not, why
> do you expose it?
> 
> Move it to the ts-nbus.c file.
> 
> The only thing you need in this header is:
> 
> struct ts_nbus;
> 
> Just that one line. The rest of the code, like the declarations
> below and all call sites, are just dealing with "pointers to something",
> they don't need to know what it is. You'll be amazed to see how it compiles.
> struct gpio_desc works exactly like that.
> 
Ok.

>> +extern u16 ts_nbus_read(struct ts_nbus *, u8 adr);
>> +extern int ts_nbus_write(struct ts_nbus *, u8 adr, u16 value);
> 
> I suspect the first function should be augmented to return an error code.
> 
Yes it will.

> Yours,
> Linus Walleij
> 

Thanks you Linus,

Best Regards,
Sebastien Bourdelin.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/6] bus: add driver for the Technologic Systems NBUS

[Linus Walleij]

On Wed, Feb 22, 2017 at 5:56 PM, Sebastien Bourdelin
<sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org> wrote:
> On 02/04/2017 05:14 AM, Linus Walleij wrote:

>>> +       gpiod_set_array_value_cansleep(gpios->ndescs, gpios->desc, values);
>>
>> This can also fail and you should check the return code and print an error
>> message if it does.
>>
> As far as i understood, the "gpiod_set_array_value_cansleep(...)"
> function doesn't return
> anything, it will return immediately if gpios->desc is null but nothing
> else.
> Did i miss something?

No I did, sorry about that.

We *should* make these functions return errors but currently they do not.

>> Error codes?
>>
> Same question here, these functions only make call to
> "gpiod_set_value_cansleep(...)",
> which as far as i understood doesn't return any error code.

Yeah, forget that.

>>> +               gpiod_set_value_cansleep(ts_nbus->csn, 1);
>>
>> Here too.
>>
> Same question, doesn't seem to have an error code.

That too.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 4/6] bus: add driver for the Technologic Systems NBUS

[Sebastien Bourdelin]

Hi Linus,

On 03/14/2017 09:23 AM, Linus Walleij wrote:
> On Wed, Feb 22, 2017 at 5:56 PM, Sebastien Bourdelin
> <sebastien.bourdelin@savoirfairelinux.com> wrote:
>> On 02/04/2017 05:14 AM, Linus Walleij wrote:
> 
>>>> +       gpiod_set_array_value_cansleep(gpios->ndescs, gpios->desc, values);
>>>
>>> This can also fail and you should check the return code and print an error
>>> message if it does.
>>>
>> As far as i understood, the "gpiod_set_array_value_cansleep(...)"
>> function doesn't return
>> anything, it will return immediately if gpios->desc is null but nothing
>> else.
>> Did i miss something?
> 
> No I did, sorry about that.
> 
> We *should* make these functions return errors but currently they do not.
> 
Ok got it.
Thanks!

>>> Error codes?
>>>
>> Same question here, these functions only make call to
>> "gpiod_set_value_cansleep(...)",
>> which as far as i understood doesn't return any error code.
> 
> Yeah, forget that.
> 
Ok.

>>>> +               gpiod_set_value_cansleep(ts_nbus->csn, 1);
>>>
>>> Here too.
>>>
>> Same question, doesn't seem to have an error code.
> 
> That too.
>
Ok.

> Yours,
> Linus Walleij
> 

Best regards,
Sebastien Bourdelin.

[PATCH v2 5/6] ARM: dts: TS-4600: add NBUS support

[Sebastien Bourdelin]

This commit enables the NBUS on the TS-4600, using the ts-nbus driver.

---
Changes v1 -> v2:
  - rebase on master
  - remove the simple-bus compatibility as the root node will now
  populate the child nodes (suggested by Rob Herring)
  - add the vendor prefix to all gpio (suggested by Rob Herring)

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
---
 arch/arm/boot/dts/imx28-ts4600-common.dtsi | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/arch/arm/boot/dts/imx28-ts4600-common.dtsi b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
index 04bd5a5c0cb4..c6282d5479de 100644
--- a/arch/arm/boot/dts/imx28-ts4600-common.dtsi
+++ b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
@@ -44,6 +44,28 @@
 					fsl,pull-up = <MXS_PULL_DISABLE>;
 				};
 
+				nbus_pins: nbus_pins {
+					fsl,pinmux-ids = <
+						MX28_PAD_GPMI_D00__GPIO_0_0
+						MX28_PAD_GPMI_D01__GPIO_0_1
+						MX28_PAD_GPMI_D02__GPIO_0_2
+						MX28_PAD_GPMI_D03__GPIO_0_3
+						MX28_PAD_GPMI_D04__GPIO_0_4
+						MX28_PAD_GPMI_D05__GPIO_0_5
+						MX28_PAD_GPMI_D06__GPIO_0_6
+						MX28_PAD_GPMI_D07__GPIO_0_7
+						MX28_PAD_GPMI_CE0N__GPIO_0_16
+						MX28_PAD_GPMI_RDY1__GPIO_0_21
+						MX28_PAD_GPMI_RDN__GPIO_0_24
+						MX28_PAD_GPMI_WRN__GPIO_0_25
+						MX28_PAD_GPMI_ALE__GPIO_0_26
+					>;
+					fsl,drive-strength = <MXS_DRIVE_4mA>;
+					fsl,voltage = <MXS_VOLTAGE_HIGH>;
+					fsl,pull-up = <MXS_PULL_DISABLE>;
+
+				};
+
 			};
 		};
 
@@ -75,4 +97,25 @@
 		};
 	};
 
+	nbus {
+		compatible = "technologic,ts-nbus";
+		pinctrl-0 = <&nbus_pins>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		pwms = <&pwm 2 83>;
+		ts-data-gpios   = <&gpio0 0 GPIO_ACTIVE_HIGH
+				   &gpio0 1 GPIO_ACTIVE_HIGH
+				   &gpio0 2 GPIO_ACTIVE_HIGH
+				   &gpio0 3 GPIO_ACTIVE_HIGH
+				   &gpio0 4 GPIO_ACTIVE_HIGH
+				   &gpio0 5 GPIO_ACTIVE_HIGH
+				   &gpio0 6 GPIO_ACTIVE_HIGH
+				   &gpio0 7 GPIO_ACTIVE_HIGH>;
+		ts-csn-gpios    = <&gpio0 16 GPIO_ACTIVE_HIGH>;
+		ts-txrx-gpios   = <&gpio0 24 GPIO_ACTIVE_HIGH>;
+		ts-strobe-gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
+		ts-ale-gpios    = <&gpio0 26 GPIO_ACTIVE_HIGH>;
+		ts-rdy-gpios    = <&gpio0 21 GPIO_ACTIVE_HIGH>;
+	};
+
 };
-- 
2.11.0

[PATCH v2 6/6] watchdog: ts4600: add driver for TS-4600 watchdog

[Sebastien Bourdelin]

This watchdog is instantiated in a FPGA and can only be access using a
GPIOs bit-banged bus, called the NBUS by Technologic Systems.
The watchdog 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.

---
Changes v1 -> v2:
  - rebase on master
  - retrieve the ts_nbus instantiated by the parent node (suggested by
  Linus Walleij)
  - rename the wdt by watchdog in the device tree and in the
  documentation (suggested by Rob Herring)
  - add a dependency to the TS_NBUS driver in the Kconfig (suggested by
  Guenter Roeck)
  - simplify the set_timeout function (suggested by Guenter Roeck)
  - use the max_hw_heartbeat_ms callback instead of the max_timeout
  callback (suggested by Guenter Roeck)

Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
---
 .../devicetree/bindings/watchdog/ts4600-wdt.txt    |  16 ++
 arch/arm/boot/dts/imx28-ts4600-common.dtsi         |   5 +
 drivers/watchdog/Kconfig                           |  11 ++
 drivers/watchdog/Makefile                          |   1 +
 drivers/watchdog/ts4600_wdt.c                      | 217 +++++++++++++++++++++
 5 files changed, 250 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
 create mode 100644 drivers/watchdog/ts4600_wdt.c

diff --git a/Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt b/Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
new file mode 100644
index 000000000000..7de00ceae341
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
@@ -0,0 +1,16 @@
+TS-4600 Technologic Systems Watchdog
+
+Required properties:
+- compatible: must be "technologic,ts4600-wdt"
+- reg: offset to the FPGA's watchdog register
+
+Optional property:
+- timeout-sec: contains the watchdog timeout in seconds.
+
+Example:
+
+watchdog {
+	compatible = "technologic,ts4600-wdt";
+	reg = <0x2a>;
+	timeout-sec = <10>;
+};
diff --git a/arch/arm/boot/dts/imx28-ts4600-common.dtsi b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
index c6282d5479de..092c2eed0fe7 100644
--- a/arch/arm/boot/dts/imx28-ts4600-common.dtsi
+++ b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
@@ -116,6 +116,11 @@
 		ts-strobe-gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
 		ts-ale-gpios    = <&gpio0 26 GPIO_ACTIVE_HIGH>;
 		ts-rdy-gpios    = <&gpio0 21 GPIO_ACTIVE_HIGH>;
+
+		watchdog@2a {
+			compatible = "technologic,ts4600-wdt";
+			reg = <0x2a>;
+		};
 	};
 
 };
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index acb00b53a520..ab1355eefad1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -500,6 +500,17 @@ config NUC900_WATCHDOG
 	  To compile this driver as a module, choose M here: the
 	  module will be called nuc900_wdt.
 
+config TS4600_WATCHDOG
+	tristate "TS-4600 Watchdog"
+	depends on TS_NBUS
+	depends on HAS_IOMEM && OF
+	depends on SOC_IMX28 || COMPILE_TEST
+	select WATCHDOG_CORE
+	help
+	  Technologic Systems TS-4600 has watchdog timer implemented in
+	  an external FPGA. Say Y here if you want to support for the
+	  watchdog timer on TS-4600 board.
+
 config TS4800_WATCHDOG
 	tristate "TS-4800 Watchdog"
 	depends on HAS_IOMEM && OF
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0c3d35e3c334..d5e164d2b4cd 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -61,6 +61,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_TS4600_WATCHDOG) += ts4600_wdt.o
 obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
 obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
 obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
diff --git a/drivers/watchdog/ts4600_wdt.c b/drivers/watchdog/ts4600_wdt.c
new file mode 100644
index 000000000000..d7bf6cc04986
--- /dev/null
+++ b/drivers/watchdog/ts4600_wdt.c
@@ -0,0 +1,217 @@
+/*
+ * Watchdog driver for TS-4600 based boards
+ *
+ * Copyright (c) 2016 - Savoir-faire Linux
+ * Author: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
+ *
+ * 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.
+ *
+ * The watchdog on the TS-4600 based boards is in an FPGA and can only be
+ * accessed using a GPIO bit-banged bus called the NBUS by Technologic Systems.
+ * The logic for the watchdog is the same then for the TS-4800 SoM, only the way
+ * to access it change, therefore this driver is heavely based on the ts4800_wdt
+ * driver from Damien Riegel <damien.riegel@savoirfairelinux.com>.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/ts-nbus.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 TS4600_WDT_FEED_2S       0x1
+#define TS4600_WDT_FEED_10S      0x2
+#define TS4600_WDT_DISABLE       0x3
+
+struct ts4600_wdt {
+	struct watchdog_device  wdd;
+	struct ts_nbus		*ts_nbus;
+	u32                     feed_offset;
+	u32                     feed_val;
+};
+
+/*
+ * TS-4600 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;
+} ts4600_wdt_map[] = {
+	{ 2,  TS4600_WDT_FEED_2S },
+	{ 10, TS4600_WDT_FEED_10S },
+};
+
+#define MAX_TIMEOUT_INDEX       (ARRAY_SIZE(ts4600_wdt_map) - 1)
+
+static void ts4600_write_feed(struct ts4600_wdt *wdt, u32 val)
+{
+	ts_nbus_write(wdt->ts_nbus, wdt->feed_offset, val);
+}
+
+static int ts4600_wdt_start(struct watchdog_device *wdd)
+{
+	struct ts4600_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	ts4600_write_feed(wdt, wdt->feed_val);
+
+	return 0;
+}
+
+static int ts4600_wdt_stop(struct watchdog_device *wdd)
+{
+	struct ts4600_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	ts4600_write_feed(wdt, TS4600_WDT_DISABLE);
+	return 0;
+}
+
+static int ts4600_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int timeout)
+{
+	struct ts4600_wdt *wdt = watchdog_get_drvdata(wdd);
+	int i = 0;
+
+	if (ts4600_wdt_map[i].timeout < timeout)
+		i = MAX_TIMEOUT_INDEX;
+
+	wdd->timeout = ts4600_wdt_map[i].timeout;
+	wdt->feed_val = ts4600_wdt_map[i].regval;
+
+	return 0;
+}
+
+static const struct watchdog_ops ts4600_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = ts4600_wdt_start,
+	.stop = ts4600_wdt_stop,
+	.set_timeout = ts4600_wdt_set_timeout,
+};
+
+static const struct watchdog_info ts4600_wdt_info = {
+	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
+	.identity = "TS-4600 Watchdog",
+};
+
+static int ts4600_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct ts_nbus *ts_nbus;
+	struct watchdog_device *wdd;
+	struct ts4600_wdt *wdt;
+	u32 reg;
+	int ret;
+
+	ret = of_property_read_u32(np, "reg", &reg);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "missing reg property\n");
+		return ret;
+	}
+
+	/* allocate memory for watchdog struct */
+	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	/* set offset to know where to write */
+	wdt->feed_offset = reg;
+
+	/* keep a pointer to the ts_nbus instanciated by the parent node */
+	ts_nbus = dev_get_drvdata(dev->parent);
+	if (!ts_nbus) {
+		dev_err(dev, "missing ts-nbus compatible parent node\n");
+		return PTR_ERR(ts_nbus);
+	}
+	wdt->ts_nbus = ts_nbus;
+
+	/* Initialize struct watchdog_device */
+	wdd = &wdt->wdd;
+	wdd->parent = dev;
+	wdd->info = &ts4600_wdt_info;
+	wdd->ops = &ts4600_wdt_ops;
+	wdd->min_timeout = ts4600_wdt_map[0].timeout;
+	wdd->max_hw_heartbeat_ms =
+		ts4600_wdt_map[MAX_TIMEOUT_INDEX].timeout * 1000;
+
+	watchdog_set_drvdata(wdd, wdt);
+	watchdog_set_nowayout(wdd, nowayout);
+	watchdog_init_timeout(wdd, 0, dev);
+
+	/*
+	 * As this watchdog supports only a few values, ts4600_wdt_set_timeout
+	 * must be called to initialize timeout and feed_val with valid values.
+	 * Default to maximum timeout if none, or an invalid one, is provided in
+	 * device tree.
+	 */
+	if (!wdd->timeout)
+		wdd->timeout = wdd->max_timeout;
+	ts4600_wdt_set_timeout(wdd, wdd->timeout);
+
+	/*
+	 * The feed register is write-only, so it is not possible to determine
+	 * watchdog's state. Disable it to be in a known state.
+	 */
+	ts4600_wdt_stop(wdd);
+
+	ret = watchdog_register_device(wdd);
+	if (ret) {
+		dev_err(dev, "failed to register watchdog device\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
+		 wdd->timeout, nowayout);
+
+	return 0;
+}
+
+static int ts4600_wdt_remove(struct platform_device *pdev)
+{
+	struct ts4600_wdt *wdt = platform_get_drvdata(pdev);
+
+	watchdog_unregister_device(&wdt->wdd);
+
+	return 0;
+}
+
+static const struct of_device_id ts4600_wdt_of_match[] = {
+	{ .compatible = "technologic,ts4600-wdt", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, ts4600_wdt_of_match);
+
+static struct platform_driver ts4600_wdt_driver = {
+	.probe		= ts4600_wdt_probe,
+	.remove		= ts4600_wdt_remove,
+	.driver		= {
+		.name	= "ts4600_wdt",
+		.of_match_table = ts4600_wdt_of_match,
+	},
+};
+
+module_platform_driver(ts4600_wdt_driver);
+
+MODULE_AUTHOR("Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:ts4600_wdt");
-- 
2.11.0

Re: [v2,6/6] watchdog: ts4600: add driver for TS-4600 watchdog

[Guenter Roeck]

On Fri, Feb 03, 2017 at 02:47:29PM -0500, Sebastien Bourdelin wrote:
> This watchdog is instantiated in a FPGA and can only be access using a
> GPIOs bit-banged bus, called the NBUS by Technologic Systems.
> The watchdog 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.
> ---
> Changes v1 -> v2:
>   - rebase on master
>   - retrieve the ts_nbus instantiated by the parent node (suggested by
>   Linus Walleij)
>   - rename the wdt by watchdog in the device tree and in the
>   documentation (suggested by Rob Herring)
>   - add a dependency to the TS_NBUS driver in the Kconfig (suggested by
>   Guenter Roeck)
>   - simplify the set_timeout function (suggested by Guenter Roeck)
>   - use the max_hw_heartbeat_ms callback instead of the max_timeout
>   callback (suggested by Guenter Roeck)
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
> ---
>  .../devicetree/bindings/watchdog/ts4600-wdt.txt    |  16 ++
>  arch/arm/boot/dts/imx28-ts4600-common.dtsi         |   5 +
>  drivers/watchdog/Kconfig                           |  11 ++
>  drivers/watchdog/Makefile                          |   1 +
>  drivers/watchdog/ts4600_wdt.c                      | 217 +++++++++++++++++++++
>  5 files changed, 250 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
>  create mode 100644 drivers/watchdog/ts4600_wdt.c
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt b/Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
> new file mode 100644
> index 000000000000..7de00ceae341
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
> @@ -0,0 +1,16 @@
> +TS-4600 Technologic Systems Watchdog
> +
> +Required properties:
> +- compatible: must be "technologic,ts4600-wdt"
> +- reg: offset to the FPGA's watchdog register
> +
> +Optional property:
> +- timeout-sec: contains the watchdog timeout in seconds.
> +
> +Example:
> +
> +watchdog {
> +	compatible = "technologic,ts4600-wdt";
> +	reg = <0x2a>;
> +	timeout-sec = <10>;
> +};
> diff --git a/arch/arm/boot/dts/imx28-ts4600-common.dtsi b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
> index c6282d5479de..092c2eed0fe7 100644
> --- a/arch/arm/boot/dts/imx28-ts4600-common.dtsi
> +++ b/arch/arm/boot/dts/imx28-ts4600-common.dtsi
> @@ -116,6 +116,11 @@
>  		ts-strobe-gpios = <&gpio0 25 GPIO_ACTIVE_HIGH>;
>  		ts-ale-gpios    = <&gpio0 26 GPIO_ACTIVE_HIGH>;
>  		ts-rdy-gpios    = <&gpio0 21 GPIO_ACTIVE_HIGH>;
> +
> +		watchdog@2a {
> +			compatible = "technologic,ts4600-wdt";
> +			reg = <0x2a>;
> +		};
>  	};
>  
>  };
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index acb00b53a520..ab1355eefad1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -500,6 +500,17 @@ config NUC900_WATCHDOG
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called nuc900_wdt.
>  
> +config TS4600_WATCHDOG
> +	tristate "TS-4600 Watchdog"
> +	depends on TS_NBUS
> +	depends on HAS_IOMEM && OF
> +	depends on SOC_IMX28 || COMPILE_TEST
> +	select WATCHDOG_CORE
> +	help
> +	  Technologic Systems TS-4600 has watchdog timer implemented in
> +	  an external FPGA. Say Y here if you want to support for the
> +	  watchdog timer on TS-4600 board.
> +
>  config TS4800_WATCHDOG
>  	tristate "TS-4800 Watchdog"
>  	depends on HAS_IOMEM && OF
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0c3d35e3c334..d5e164d2b4cd 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -61,6 +61,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_TS4600_WATCHDOG) += ts4600_wdt.o
>  obj-$(CONFIG_TS4800_WATCHDOG) += ts4800_wdt.o
>  obj-$(CONFIG_TS72XX_WATCHDOG) += ts72xx_wdt.o
>  obj-$(CONFIG_IMX2_WDT) += imx2_wdt.o
> diff --git a/drivers/watchdog/ts4600_wdt.c b/drivers/watchdog/ts4600_wdt.c
> new file mode 100644
> index 000000000000..d7bf6cc04986
> --- /dev/null
> +++ b/drivers/watchdog/ts4600_wdt.c
> @@ -0,0 +1,217 @@
> +/*
> + * Watchdog driver for TS-4600 based boards
> + *
> + * Copyright (c) 2016 - Savoir-faire Linux
> + * Author: Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>
> + *
> + * 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.
> + *
> + * The watchdog on the TS-4600 based boards is in an FPGA and can only be
> + * accessed using a GPIO bit-banged bus called the NBUS by Technologic Systems.
> + * The logic for the watchdog is the same then for the TS-4800 SoM, only the way
> + * to access it change, therefore this driver is heavely based on the ts4800_wdt
> + * driver from Damien Riegel <damien.riegel-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/ts-nbus.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 TS4600_WDT_FEED_2S       0x1
> +#define TS4600_WDT_FEED_10S      0x2
> +#define TS4600_WDT_DISABLE       0x3
> +
> +struct ts4600_wdt {
> +	struct watchdog_device  wdd;
> +	struct ts_nbus		*ts_nbus;
> +	u32                     feed_offset;
> +	u32                     feed_val;
> +};
> +
> +/*
> + * TS-4600 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;
> +} ts4600_wdt_map[] = {
> +	{ 2,  TS4600_WDT_FEED_2S },
> +	{ 10, TS4600_WDT_FEED_10S },
> +};

Does this table really do any good ? Seems to me it might be simpler
to just use raw values where needed.

> +
> +#define MAX_TIMEOUT_INDEX       (ARRAY_SIZE(ts4600_wdt_map) - 1)
> +
> +static void ts4600_write_feed(struct ts4600_wdt *wdt, u32 val)
> +{
> +	ts_nbus_write(wdt->ts_nbus, wdt->feed_offset, val);
> +}
> +
> +static int ts4600_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct ts4600_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	ts4600_write_feed(wdt, wdt->feed_val);
> +
> +	return 0;
> +}
> +
> +static int ts4600_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct ts4600_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	ts4600_write_feed(wdt, TS4600_WDT_DISABLE);
> +	return 0;
> +}
> +
> +static int ts4600_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	struct ts4600_wdt *wdt = watchdog_get_drvdata(wdd);
> +	int i = 0;
> +
> +	if (ts4600_wdt_map[i].timeout < timeout)
> +		i = MAX_TIMEOUT_INDEX;
> +
> +	wdd->timeout = ts4600_wdt_map[i].timeout;

This is not correct. you would only set wdd->timeout to the value from
the table if the timeout is <= the requested timeout. Otherwise,
set the requested timeout value. Otherwise the timeout will be set
to a maximum of 10 seconds, and using the core to support larger
timeouts does not work.

> +	wdt->feed_val = ts4600_wdt_map[i].regval;
> +

FWIW, personally I think that
	if (timeout <= 2) {
		wdd->timeout = 2;
		wdt->feed_val = TS4600_WDT_FEED_2S;
	} else {
		wdd->timeout = timeout < 10 ? 10 : timeout;
		wdt->feed_val = TS4600_WDT_FEED_10S;
	}

would be simpler than dealing with the table.

> +	return 0;
> +}
> +
> +static const struct watchdog_ops ts4600_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = ts4600_wdt_start,
> +	.stop = ts4600_wdt_stop,
> +	.set_timeout = ts4600_wdt_set_timeout,
> +};
> +
> +static const struct watchdog_info ts4600_wdt_info = {
> +	.options = WDIOF_SETTIMEOUT | WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING,
> +	.identity = "TS-4600 Watchdog",
> +};
> +
> +static int ts4600_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct ts_nbus *ts_nbus;
> +	struct watchdog_device *wdd;
> +	struct ts4600_wdt *wdt;
> +	u32 reg;
> +	int ret;
> +
> +	ret = of_property_read_u32(np, "reg", &reg);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "missing reg property\n");
> +		return ret;
> +	}
> +
> +	/* allocate memory for watchdog struct */
> +	wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	/* set offset to know where to write */
> +	wdt->feed_offset = reg;
> +
> +	/* keep a pointer to the ts_nbus instanciated by the parent node */
> +	ts_nbus = dev_get_drvdata(dev->parent);
> +	if (!ts_nbus) {
> +		dev_err(dev, "missing ts-nbus compatible parent node\n");
> +		return PTR_ERR(ts_nbus);
> +	}
> +	wdt->ts_nbus = ts_nbus;
> +
> +	/* Initialize struct watchdog_device */
> +	wdd = &wdt->wdd;
> +	wdd->parent = dev;
> +	wdd->info = &ts4600_wdt_info;
> +	wdd->ops = &ts4600_wdt_ops;
> +	wdd->min_timeout = ts4600_wdt_map[0].timeout;
> +	wdd->max_hw_heartbeat_ms =
> +		ts4600_wdt_map[MAX_TIMEOUT_INDEX].timeout * 1000;

Why not 10824 ?

> +
> +	watchdog_set_drvdata(wdd, wdt);
> +	watchdog_set_nowayout(wdd, nowayout);
> +	watchdog_init_timeout(wdd, 0, dev);
> +
> +	/*
> +	 * As this watchdog supports only a few values, ts4600_wdt_set_timeout
> +	 * must be called to initialize timeout and feed_val with valid values.
> +	 * Default to maximum timeout if none, or an invalid one, is provided in
> +	 * device tree.
> +	 */
> +	if (!wdd->timeout)
> +		wdd->timeout = wdd->max_timeout;

There is no max_timeout anymore. Just initialize timeout with a value
of your choice, such as 30 seconds, before calling watchdog_init_timeout().

> +	ts4600_wdt_set_timeout(wdd, wdd->timeout);
> +
> +	/*
> +	 * The feed register is write-only, so it is not possible to determine
> +	 * watchdog's state. Disable it to be in a known state.
> +	 */
> +	ts4600_wdt_stop(wdd);
> +
> +	ret = watchdog_register_device(wdd);

Optimal candidate for devm_wtchdog_register_device(). It lets you drop
the remove function and platform_set_drvdata().

> +	if (ret) {
> +		dev_err(dev, "failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +
> +	dev_info(dev, "initialized (timeout = %d sec, nowayout = %d)\n",
> +		 wdd->timeout, nowayout);
> +
> +	return 0;
> +}
> +
> +static int ts4600_wdt_remove(struct platform_device *pdev)
> +{
> +	struct ts4600_wdt *wdt = platform_get_drvdata(pdev);
> +
> +	watchdog_unregister_device(&wdt->wdd);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ts4600_wdt_of_match[] = {
> +	{ .compatible = "technologic,ts4600-wdt", },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, ts4600_wdt_of_match);
> +
> +static struct platform_driver ts4600_wdt_driver = {
> +	.probe		= ts4600_wdt_probe,
> +	.remove		= ts4600_wdt_remove,
> +	.driver		= {
> +		.name	= "ts4600_wdt",
> +		.of_match_table = ts4600_wdt_of_match,
> +	},
> +};
> +
> +module_platform_driver(ts4600_wdt_driver);
> +
> +MODULE_AUTHOR("Sebastien Bourdelin <sebastien.bourdelin-4ysUXcep3aM1wj+D4I0NRVaTQe2KTcn/@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:ts4600_wdt");
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 6/6] watchdog: ts4600: add driver for TS-4600 watchdog

[Rob Herring]

On Fri, Feb 03, 2017 at 02:47:29PM -0500, Sebastien Bourdelin wrote:
> This watchdog is instantiated in a FPGA and can only be access using a
> GPIOs bit-banged bus, called the NBUS by Technologic Systems.
> The watchdog 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.
> 
> ---
> Changes v1 -> v2:
>   - rebase on master
>   - retrieve the ts_nbus instantiated by the parent node (suggested by
>   Linus Walleij)
>   - rename the wdt by watchdog in the device tree and in the
>   documentation (suggested by Rob Herring)
>   - add a dependency to the TS_NBUS driver in the Kconfig (suggested by
>   Guenter Roeck)
>   - simplify the set_timeout function (suggested by Guenter Roeck)
>   - use the max_hw_heartbeat_ms callback instead of the max_timeout
>   callback (suggested by Guenter Roeck)
> 
> Signed-off-by: Sebastien Bourdelin <sebastien.bourdelin@savoirfairelinux.com>
> ---
>  .../devicetree/bindings/watchdog/ts4600-wdt.txt    |  16 ++
>  arch/arm/boot/dts/imx28-ts4600-common.dtsi         |   5 +

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/watchdog/Kconfig                           |  11 ++
>  drivers/watchdog/Makefile                          |   1 +
>  drivers/watchdog/ts4600_wdt.c                      | 217 +++++++++++++++++++++
>  5 files changed, 250 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/ts4600-wdt.txt
>  create mode 100644 drivers/watchdog/ts4600_wdt.c