[PATCH v6 0/2] mfd: flexcom: add a driver for Flexcom

[PATCH v6 0/2] mfd: flexcom: add a driver for Flexcom

[Cyrille Pitchen]

ChangeLog

v6:
- select the operating mode according to the "compatible" DT property of
  the first available child node (should be unique).
- remove the "atmel,flexcom-mode" DT property so the need of a header file
  defining macros for the possible values of this deprecated property.

v5:
- create a header file containing macros used by DT bindings
- use numeric constants instead of strings to select the Flexcom mode
- change the license to "GPL v2"
- update the DT binding documentation to make it more readable and add
  references to USART, SPI and I2C DT binding documentations. remove the
  useless label in the Example section.
- change the register prefix from FX_ to FLEX_ to match the Flexcom
  programmer datasheet.
- rename some variables to make them more understandable.

v4:
- check clk_prepare_enable() return code in atmel_flexcom_probe()
- add a commit message to the DT binding patch

v3:
- remove MODULE_ALIAS()
- add Acked-by from Boris Brezillon and Alexandre Belloni

v2:
- enhance the documentation of DT bindings and change the way the "ranges"
  property is used.
- replace __raw_readl() and __raw_writel() by readl() and writel().
- change the module license to "GPL" for v2 or later
- print the selected flexcom mode after the hardware version

v1:
This series of patches a support to the Atmel Flexcom, a wrapper which
integrates an USART, a SPI controller and a TWI controller. Only one
peripheral can be used at a time. The active function is selected though
the Flexcom Mode Register.

Cyrille Pitchen (2):
  mfd: devicetree: add bindings for Atmel Flexcom
  mfd: atmel-flexcom: add a driver for Atmel Flexible Serial
    Communication Unit

 .../devicetree/bindings/mfd/atmel-flexcom.txt      |  67 +++++++++++
 drivers/mfd/Kconfig                                |  11 ++
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/atmel-flexcom.c                        | 126 +++++++++++++++++++++
 4 files changed, 205 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
 create mode 100644 drivers/mfd/atmel-flexcom.c

-- 
1.8.2.2

[PATCH v6 1/2] mfd: devicetree: add bindings for Atmel Flexcom

[Cyrille Pitchen]

This patch documents the DT bindings for the Atmel Flexcom which will be
introduced by sama5d2x SoCs. These bindings will be used by the actual
Flexcom driver to be sent in another patch.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 .../devicetree/bindings/mfd/atmel-flexcom.txt      | 67 ++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt

diff --git a/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt b/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
new file mode 100644
index 000000000000..87a366b95ecf
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
@@ -0,0 +1,67 @@
+* Device tree bindings for Atmel Flexcom (Flexible Serial Communication Unit)
+
+The Atmel Flexcom is just a wrapper which embeds a SPI controller, an I2C
+controller and an USART. Only one function can be used at a time and is chosen
+at boot time according to the device tree.
+
+Required properties:
+- compatible:		"atmel,sama5d2-flexcom"
+- reg:			shall be the offset/length value for Flexcom dedicated
+			I/O registers (without USART, TWI or SPI registers).
+- clocks:		shall be the Flexcom peripheral clock from PMC.
+- #address-cells:	should be <1>
+- #size-cells:		should be <1>
+- ranges:		one range for the full I/O register region (including
+			USART, TWI and SPI registers).
+
+Required child:
+a single available child for the serial controller to enable.
+
+One of the following substrings must be found inside the compatible property of
+this child to enable the right serial controller:
+- "-usart"
+- "-spi"
+- "-i2c"
+
+The reg property of this child should be:
+- <0x200 0x200> for USART
+- <0x400 0x200> for SPI
+- <0x600 0x200> for I2C
+
+The phandle provided by the clocks property of the child is the same as one for
+the Flexcom parent.
+
+Other properties remain unchanged. See documentation of the respective device:
+- ../serial/atmel-usart.txt
+- ../spi/spi_atmel.txt
+- ../i2c/i2c-at91.txt
+
+Example:
+
+flexcom@f8034000 {
+	compatible = "atmel,sama5d2-flexcom";
+	reg = <0xf8034000 0x200>;
+	clocks = <&flx0_clk>;
+	#address-cells = <1>;
+	#size-cells = <1>;
+	ranges = <0x0 0xf8034000 0x800>;
+
+	spi@f8034400 {
+		compatible = "atmel,at91rm9200-spi";
+		reg = <0x400 0x200>;
+		interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_flx0_default>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clocks = <&flx0_clk>;
+		clock-names = "spi_clk";
+		atmel,fifo-size = <32>;
+
+		mtd_dataflash@0 {
+			compatible = "atmel,at25f512b";
+			reg = <0>;
+			spi-max-frequency = <20000000>;
+		};
+	};
+};
-- 
1.8.2.2

[PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Cyrille Pitchen]

This driver supports the new Atmel Flexcom. The Flexcom is a wrapper which
integrates one SPI controller, one I2C controller and one USART. Only one
function can be enabled at a time. This driver selects the function once
for all, when the Flexcom is probed, finding the first (should be unique)
available DT child node which compatible property contains one of the
following strings:
- "-usart"
- "-spi"
- "-i2c"

This driver has chosen to present the Flexcom to the system as a MFD so
the implementation is seamless for the existing Atmel SPI, I2C and USART
drivers.

Also the Flexcom embeds FIFOs: the latest patches of the SPI, I2C and
USART drivers take advantage of this new feature.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/mfd/Kconfig         |  11 ++++
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/atmel-flexcom.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/mfd/atmel-flexcom.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index d5ad04dad081..9b33ad0653d1 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -59,6 +59,17 @@ config MFD_AAT2870_CORE
 	  additional drivers must be enabled in order to use the
 	  functionality of the device.
 
+config MFD_ATMEL_FLEXCOM
+	tristate "Atmel Flexcom (Flexible Serial Communication Unit)"
+	select MFD_CORE
+	depends on OF
+	help
+	  Select this to get support for Atmel Flexcom. This is a wrapper
+	  which embeds a SPI controller, a I2C controller and a USART. Only
+	  one function can be used at a time. The choice is done at boot time
+	  by the probe function of this MFD driver according to a device tree
+	  property.
+
 config MFD_ATMEL_HLCDC
 	tristate "Atmel HLCDC (High-end LCD Controller)"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 0e5cfeba107c..c666bf51abf3 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -160,6 +160,7 @@ obj-$(CONFIG_MFD_SPMI_PMIC)	+= qcom-spmi-pmic.o
 obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
+obj-$(CONFIG_MFD_ATMEL_FLEXCOM)	+= atmel-flexcom.o
 obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
 obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
 obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
new file mode 100644
index 000000000000..8ba1862fbb74
--- /dev/null
+++ b/drivers/mfd/atmel-flexcom.c
@@ -0,0 +1,126 @@
+/*
+ * Driver for Atmel Flexcom
+ *
+ * Copyright (C) 2015 Atmel Corporation
+ *
+ * Author: Cyrille Pitchen <cyrille.pitchen@atmel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+
+/* I/O register offsets */
+#define FLEX_MR		0x0	/* Mode Register */
+#define FLEX_VERSION	0xfc	/* Version Register */
+
+/* Mode Register bit fields */
+#define FLEX_MR_OPMODE_MASK	0x3
+#define FLEX_MR_OPMODE_NO_COM	0x0
+#define FLEX_MR_OPMODE_USART	0x1
+#define FLEX_MR_OPMODE_SPI	0x2
+#define FLEX_MR_OPMODE_TWI	0x3
+
+
+static int atmel_flexcom_probe(struct platform_device *pdev)
+{
+	struct device_node *child, *np = pdev->dev.of_node;
+	struct clk *clk;
+	struct resource *res;
+	void __iomem *base;
+	u32 opmode = FLEX_MR_OPMODE_NO_COM;
+	int err;
+
+	for_each_child_of_node(np, child) {
+		const char *compatible;
+		int cplen;
+
+		if (!of_device_is_available(child))
+			continue;
+
+		compatible = of_get_property(child, "compatible", &cplen);
+		if (!compatible || strlen(compatible) > cplen)
+			continue;
+
+		if (strstr(compatible, "-usart")) {
+			opmode = FLEX_MR_OPMODE_USART;
+			break;
+		}
+
+		if (strstr(compatible, "-spi")) {
+			opmode = FLEX_MR_OPMODE_SPI;
+			break;
+		}
+
+		if (strstr(compatible, "-i2c")) {
+			opmode = FLEX_MR_OPMODE_TWI;
+			break;
+		}
+	}
+
+	if (opmode == FLEX_MR_OPMODE_NO_COM)
+		return -EINVAL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	err = clk_prepare_enable(clk);
+	if (err)
+		return err;
+
+	/*
+	 * Set the Operating Mode in the Mode Register: only the selected device
+	 * is clocked. Hence, registers of the other serial devices remain
+	 * inaccessible and are read as zero. Also the external I/O lines of the
+	 * Flexcom are muxed to reach the selected device.
+	 */
+	writel(opmode, base + FLEX_MR);
+
+	clk_disable_unprepare(clk);
+
+	return of_platform_populate(np, NULL, NULL, &pdev->dev);
+}
+
+static const struct of_device_id atmel_flexcom_of_match[] = {
+	{ .compatible = "atmel,sama5d2-flexcom" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
+
+static struct platform_driver atmel_flexcom_driver = {
+	.driver		= {
+		.name	= "atmel_flexcom",
+		.of_match_table = atmel_flexcom_of_match,
+	},
+	.probe		= atmel_flexcom_probe,
+};
+
+module_platform_driver(atmel_flexcom_driver);
+
+MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@atmel.com>");
+MODULE_DESCRIPTION("Atmel Flexcom MFD driver");
+MODULE_LICENSE("GPL v2");
-- 
1.8.2.2

Re: [PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Lee Jones]

On Wed, 22 Jul 2015, Cyrille Pitchen wrote:

> This driver supports the new Atmel Flexcom. The Flexcom is a wrapper which
> integrates one SPI controller, one I2C controller and one USART. Only one
> function can be enabled at a time. This driver selects the function once
> for all, when the Flexcom is probed, finding the first (should be unique)
> available DT child node which compatible property contains one of the
> following strings:
> - "-usart"
> - "-spi"
> - "-i2c"
> 
> This driver has chosen to present the Flexcom to the system as a MFD so
> the implementation is seamless for the existing Atmel SPI, I2C and USART
> drivers.
> 
> Also the Flexcom embeds FIFOs: the latest patches of the SPI, I2C and
> USART drivers take advantage of this new feature.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> ---
>  drivers/mfd/Kconfig         |  11 ++++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/atmel-flexcom.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 138 insertions(+)
>  create mode 100644 drivers/mfd/atmel-flexcom.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index d5ad04dad081..9b33ad0653d1 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -59,6 +59,17 @@ config MFD_AAT2870_CORE
>  	  additional drivers must be enabled in order to use the
>  	  functionality of the device.
>  
> +config MFD_ATMEL_FLEXCOM
> +	tristate "Atmel Flexcom (Flexible Serial Communication Unit)"
> +	select MFD_CORE
> +	depends on OF
> +	help
> +	  Select this to get support for Atmel Flexcom. This is a wrapper
> +	  which embeds a SPI controller, a I2C controller and a USART. Only
> +	  one function can be used at a time. The choice is done at boot time
> +	  by the probe function of this MFD driver according to a device tree
> +	  property.
> +
>  config MFD_ATMEL_HLCDC
>  	tristate "Atmel HLCDC (High-end LCD Controller)"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 0e5cfeba107c..c666bf51abf3 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -160,6 +160,7 @@ obj-$(CONFIG_MFD_SPMI_PMIC)	+= qcom-spmi-pmic.o
>  obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
>  obj-$(CONFIG_MFD_TPS65090)	+= tps65090.o
>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
> +obj-$(CONFIG_MFD_ATMEL_FLEXCOM)	+= atmel-flexcom.o
>  obj-$(CONFIG_MFD_ATMEL_HLCDC)	+= atmel-hlcdc.o
>  obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_PALMAS)	+= palmas.o
> diff --git a/drivers/mfd/atmel-flexcom.c b/drivers/mfd/atmel-flexcom.c
> new file mode 100644
> index 000000000000..8ba1862fbb74
> --- /dev/null
> +++ b/drivers/mfd/atmel-flexcom.c
> @@ -0,0 +1,126 @@
> +/*
> + * Driver for Atmel Flexcom
> + *
> + * Copyright (C) 2015 Atmel Corporation
> + *
> + * Author: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +
> +/* I/O register offsets */
> +#define FLEX_MR		0x0	/* Mode Register */
> +#define FLEX_VERSION	0xfc	/* Version Register */
> +
> +/* Mode Register bit fields */
> +#define FLEX_MR_OPMODE_MASK	0x3
> +#define FLEX_MR_OPMODE_NO_COM	0x0
> +#define FLEX_MR_OPMODE_USART	0x1
> +#define FLEX_MR_OPMODE_SPI	0x2
> +#define FLEX_MR_OPMODE_TWI	0x3
> +
> +
> +static int atmel_flexcom_probe(struct platform_device *pdev)
> +{
> +	struct device_node *child, *np = pdev->dev.of_node;
> +	struct clk *clk;
> +	struct resource *res;
> +	void __iomem *base;
> +	u32 opmode = FLEX_MR_OPMODE_NO_COM;
> +	int err;
> +
> +	for_each_child_of_node(np, child) {
> +		const char *compatible;
> +		int cplen;
> +
> +		if (!of_device_is_available(child))
> +			continue;
> +
> +		compatible = of_get_property(child, "compatible", &cplen);
> +		if (!compatible || strlen(compatible) > cplen)
> +			continue;
> +
> +		if (strstr(compatible, "-usart")) {
> +			opmode = FLEX_MR_OPMODE_USART;
> +			break;
> +		}
> +
> +		if (strstr(compatible, "-spi")) {
> +			opmode = FLEX_MR_OPMODE_SPI;
> +			break;
> +		}
> +
> +		if (strstr(compatible, "-i2c")) {
> +			opmode = FLEX_MR_OPMODE_TWI;
> +			break;
> +		}
> +	}

>From what I understand Flexcom is a wrapper which can sit above any
number of SPI, I2C and/or UART devices.  Devices which you don't
really have any control over (source code wise).  So wouldn't it be
better to match on the details you do have control over i.e. the node
name, rather than the compatible string?

I would personally match on of_find_node_by_name() to future-proof
your implementation.

> +	if (opmode == FLEX_MR_OPMODE_NO_COM)
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	err = clk_prepare_enable(clk);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Set the Operating Mode in the Mode Register: only the selected device
> +	 * is clocked. Hence, registers of the other serial devices remain
> +	 * inaccessible and are read as zero. Also the external I/O lines of the
> +	 * Flexcom are muxed to reach the selected device.
> +	 */
> +	writel(opmode, base + FLEX_MR);
> +
> +	clk_disable_unprepare(clk);
> +
> +	return of_platform_populate(np, NULL, NULL, &pdev->dev);
> +}
> +
> +static const struct of_device_id atmel_flexcom_of_match[] = {
> +	{ .compatible = "atmel,sama5d2-flexcom" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, atmel_flexcom_of_match);
> +
> +static struct platform_driver atmel_flexcom_driver = {
> +	.driver		= {
> +		.name	= "atmel_flexcom",
> +		.of_match_table = atmel_flexcom_of_match,
> +	},
> +	.probe		= atmel_flexcom_probe,
> +};

This way of lining code up looks weird.  I do like straight lines when
code has the same indentation, but you're attempting to match lines at
different levels (and not matching on lines that are on the same
level).

> +module_platform_driver(atmel_flexcom_driver);
> +
> +MODULE_AUTHOR("Cyrille Pitchen <cyrille.pitchen@atmel.com>");
> +MODULE_DESCRIPTION("Atmel Flexcom MFD driver");
> +MODULE_LICENSE("GPL v2");

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

Re: [PATCH v6 1/2] mfd: devicetree: add bindings for Atmel Flexcom

[Lee Jones]

On Wed, 22 Jul 2015, Cyrille Pitchen wrote:

> This patch documents the DT bindings for the Atmel Flexcom which will be
> introduced by sama5d2x SoCs. These bindings will be used by the actual
> Flexcom driver to be sent in another patch.
> 
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> ---
>  .../devicetree/bindings/mfd/atmel-flexcom.txt      | 67 ++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> 
> diff --git a/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt b/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> new file mode 100644
> index 000000000000..87a366b95ecf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/atmel-flexcom.txt
> @@ -0,0 +1,67 @@
> +* Device tree bindings for Atmel Flexcom (Flexible Serial Communication Unit)
> +
> +The Atmel Flexcom is just a wrapper which embeds a SPI controller, an I2C
> +controller and an USART. Only one function can be used at a time and is chosen
> +at boot time according to the device tree.
> +
> +Required properties:
> +- compatible:		"atmel,sama5d2-flexcom"
> +- reg:			shall be the offset/length value for Flexcom dedicated
> +			I/O registers (without USART, TWI or SPI registers).
> +- clocks:		shall be the Flexcom peripheral clock from PMC.
> +- #address-cells:	should be <1>
> +- #size-cells:		should be <1>
> +- ranges:		one range for the full I/O register region (including
> +			USART, TWI and SPI registers).
> +
> +Required child:
> +a single available child for the serial controller to enable.

Not a fan of uppercase characters I see.

Nevertheless, documentation looks fine and doesn't introduce any new
properties, which we like.

Acked-by: Lee Jones <lee.jones@linaro.org>

> +One of the following substrings must be found inside the compatible property of
> +this child to enable the right serial controller:
> +- "-usart"
> +- "-spi"
> +- "-i2c"
> +
> +The reg property of this child should be:
> +- <0x200 0x200> for USART
> +- <0x400 0x200> for SPI
> +- <0x600 0x200> for I2C
> +
> +The phandle provided by the clocks property of the child is the same as one for
> +the Flexcom parent.
> +
> +Other properties remain unchanged. See documentation of the respective device:
> +- ../serial/atmel-usart.txt
> +- ../spi/spi_atmel.txt
> +- ../i2c/i2c-at91.txt
> +
> +Example:
> +
> +flexcom@f8034000 {
> +	compatible = "atmel,sama5d2-flexcom";
> +	reg = <0xf8034000 0x200>;
> +	clocks = <&flx0_clk>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	ranges = <0x0 0xf8034000 0x800>;
> +
> +	spi@f8034400 {
> +		compatible = "atmel,at91rm9200-spi";
> +		reg = <0x400 0x200>;
> +		interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_flx0_default>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clocks = <&flx0_clk>;
> +		clock-names = "spi_clk";
> +		atmel,fifo-size = <32>;
> +
> +		mtd_dataflash@0 {
> +			compatible = "atmel,at25f512b";
> +			reg = <0>;
> +			spi-max-frequency = <20000000>;
> +		};
> +	};
> +};

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

Re: [PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Boris Brezillon]

Hi Lee,

On Thu, 23 Jul 2015 08:32:17 +0100
Lee Jones <lee.jones@linaro.org> wrote:

> On Wed, 22 Jul 2015, Cyrille Pitchen wrote:
> > +	for_each_child_of_node(np, child) {
> > +		const char *compatible;
> > +		int cplen;
> > +
> > +		if (!of_device_is_available(child))
> > +			continue;
> > +
> > +		compatible = of_get_property(child, "compatible", &cplen);
> > +		if (!compatible || strlen(compatible) > cplen)
> > +			continue;
> > +
> > +		if (strstr(compatible, "-usart")) {
> > +			opmode = FLEX_MR_OPMODE_USART;
> > +			break;
> > +		}
> > +
> > +		if (strstr(compatible, "-spi")) {
> > +			opmode = FLEX_MR_OPMODE_SPI;
> > +			break;
> > +		}
> > +
> > +		if (strstr(compatible, "-i2c")) {
> > +			opmode = FLEX_MR_OPMODE_TWI;
> > +			break;
> > +		}
> > +	}
> 
> From what I understand Flexcom is a wrapper which can sit above any
> number of SPI, I2C and/or UART devices.  Devices which you don't
> really have any control over (source code wise).  So wouldn't it be
> better to match on the details you do have control over i.e. the node
> name, rather than the compatible string?
> 
> I would personally match on of_find_node_by_name() to future-proof
> your implementation.

Actually, I think using compatible strings is more future-proof than
using the node names, because nothing in the DT bindings doc enforce the
node name, and usually what we use to attach a node to a specific
driver is the compatible string (this one is specified in the bindings
doc).

Regarding the implementation itself, I would match the child node with
an of_device_id table rather than trying to find a specific substring
in the compatible string, but I think that's only a matter of taste.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Lee Jones]

On Thu, 23 Jul 2015, Boris Brezillon wrote:

> Hi Lee,
> 
> On Thu, 23 Jul 2015 08:32:17 +0100
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Wed, 22 Jul 2015, Cyrille Pitchen wrote:
> > > +	for_each_child_of_node(np, child) {
> > > +		const char *compatible;
> > > +		int cplen;
> > > +
> > > +		if (!of_device_is_available(child))
> > > +			continue;
> > > +
> > > +		compatible = of_get_property(child, "compatible", &cplen);
> > > +		if (!compatible || strlen(compatible) > cplen)
> > > +			continue;
> > > +
> > > +		if (strstr(compatible, "-usart")) {
> > > +			opmode = FLEX_MR_OPMODE_USART;
> > > +			break;
> > > +		}
> > > +
> > > +		if (strstr(compatible, "-spi")) {
> > > +			opmode = FLEX_MR_OPMODE_SPI;
> > > +			break;
> > > +		}
> > > +
> > > +		if (strstr(compatible, "-i2c")) {
> > > +			opmode = FLEX_MR_OPMODE_TWI;
> > > +			break;
> > > +		}
> > > +	}
> > 
> > From what I understand Flexcom is a wrapper which can sit above any
> > number of SPI, I2C and/or UART devices.  Devices which you don't
> > really have any control over (source code wise).  So wouldn't it be
> > better to match on the details you do have control over i.e. the node
> > name, rather than the compatible string?
> > 
> > I would personally match on of_find_node_by_name() to future-proof
> > your implementation.
> 
> Actually, I think using compatible strings is more future-proof than
> using the node names, because nothing in the DT bindings doc enforce the
> node name, and usually what we use to attach a node to a specific
> driver is the compatible string (this one is specified in the bindings
> doc).

I know what you're saying, but what if someone uses the Flexcom driver
to wrap a different type of SPI driver where (for instance) the
compatible string used is "<name>-<newtype>".  Then we'd have to keep
adding more lines here to accommodate.

Whereas if we used the child node name which only pertains to _this_
driver, we would then have full control and know that (unless it
Flexcom is used for a completely different type of serial controller)
we wouldn't have to keep expanding the code to accommodate.

> Regarding the implementation itself, I would match the child node with
> an of_device_id table rather than trying to find a specific substring
> in the compatible string, but I think that's only a matter of taste.

You mean duplicate each of the supported device's compatible strings
in this driver, then fetch the attributed of_match_device()->data
value?

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

Re: [PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Boris Brezillon]

On Thu, 23 Jul 2015 10:13:11 +0100
Lee Jones <lee.jones@linaro.org> wrote:

> On Thu, 23 Jul 2015, Boris Brezillon wrote:
> 
> > Hi Lee,
> > 
> > On Thu, 23 Jul 2015 08:32:17 +0100
> > Lee Jones <lee.jones@linaro.org> wrote:
> > 
> > > On Wed, 22 Jul 2015, Cyrille Pitchen wrote:
> > > > +	for_each_child_of_node(np, child) {
> > > > +		const char *compatible;
> > > > +		int cplen;
> > > > +
> > > > +		if (!of_device_is_available(child))
> > > > +			continue;
> > > > +
> > > > +		compatible = of_get_property(child, "compatible", &cplen);
> > > > +		if (!compatible || strlen(compatible) > cplen)
> > > > +			continue;
> > > > +
> > > > +		if (strstr(compatible, "-usart")) {
> > > > +			opmode = FLEX_MR_OPMODE_USART;
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		if (strstr(compatible, "-spi")) {
> > > > +			opmode = FLEX_MR_OPMODE_SPI;
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		if (strstr(compatible, "-i2c")) {
> > > > +			opmode = FLEX_MR_OPMODE_TWI;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > 
> > > From what I understand Flexcom is a wrapper which can sit above any
> > > number of SPI, I2C and/or UART devices.  Devices which you don't
> > > really have any control over (source code wise).  So wouldn't it be
> > > better to match on the details you do have control over i.e. the node
> > > name, rather than the compatible string?
> > > 
> > > I would personally match on of_find_node_by_name() to future-proof
> > > your implementation.
> > 
> > Actually, I think using compatible strings is more future-proof than
> > using the node names, because nothing in the DT bindings doc enforce the
> > node name, and usually what we use to attach a node to a specific
> > driver is the compatible string (this one is specified in the bindings
> > doc).
> 
> I know what you're saying, but what if someone uses the Flexcom driver
> to wrap a different type of SPI driver where (for instance) the
> compatible string used is "<name>-<newtype>".  Then we'd have to keep
> adding more lines here to accommodate.
> 
> Whereas if we used the child node name which only pertains to _this_
> driver, we would then have full control and know that (unless it
> Flexcom is used for a completely different type of serial controller)
> we wouldn't have to keep expanding the code to accommodate.

You're right about the complexity implied by the compat string
maintenance, but I still think using node names to detect the mode is
a bad idea.

Let's take another example making both solution unsuitable: what if
the flexcom-v2 exposes 2 devices of the same type, they will both have
the same name and the same compatible string, and we'll have no way to
detect the appropriate mode. That's why I think none of our suggestion
is future-proof.

> 
> > Regarding the implementation itself, I would match the child node with
> > an of_device_id table rather than trying to find a specific substring
> > in the compatible string, but I think that's only a matter of taste.
> 
> You mean duplicate each of the supported device's compatible strings
> in this driver, then fetch the attributed of_match_device()->data
> value?
> 

Yes, and that's definitely not a good idea, but I think Cyrille has
found a better approach (I'll let him explain).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Cyrille Pitchen]

Hi all,

Le 23/07/2015 14:50, Boris Brezillon a écrit :
> On Thu, 23 Jul 2015 10:13:11 +0100
> Lee Jones <lee.jones@linaro.org> wrote:
> 
>> On Thu, 23 Jul 2015, Boris Brezillon wrote:
>>
>>> Hi Lee,
>>>
>>> On Thu, 23 Jul 2015 08:32:17 +0100
>>> Lee Jones <lee.jones@linaro.org> wrote:
>>>
>>>> On Wed, 22 Jul 2015, Cyrille Pitchen wrote:
>>>>> +	for_each_child_of_node(np, child) {
>>>>> +		const char *compatible;
>>>>> +		int cplen;
>>>>> +
>>>>> +		if (!of_device_is_available(child))
>>>>> +			continue;
>>>>> +
>>>>> +		compatible = of_get_property(child, "compatible", &cplen);
>>>>> +		if (!compatible || strlen(compatible) > cplen)
>>>>> +			continue;
>>>>> +
>>>>> +		if (strstr(compatible, "-usart")) {
>>>>> +			opmode = FLEX_MR_OPMODE_USART;
>>>>> +			break;
>>>>> +		}
>>>>> +
>>>>> +		if (strstr(compatible, "-spi")) {
>>>>> +			opmode = FLEX_MR_OPMODE_SPI;
>>>>> +			break;
>>>>> +		}
>>>>> +
>>>>> +		if (strstr(compatible, "-i2c")) {
>>>>> +			opmode = FLEX_MR_OPMODE_TWI;
>>>>> +			break;
>>>>> +		}
>>>>> +	}
>>>>
>>>> From what I understand Flexcom is a wrapper which can sit above any
>>>> number of SPI, I2C and/or UART devices.  Devices which you don't
>>>> really have any control over (source code wise).  So wouldn't it be
>>>> better to match on the details you do have control over i.e. the node
>>>> name, rather than the compatible string?
>>>>
>>>> I would personally match on of_find_node_by_name() to future-proof
>>>> your implementation.
>>>
>>> Actually, I think using compatible strings is more future-proof than
>>> using the node names, because nothing in the DT bindings doc enforce the
>>> node name, and usually what we use to attach a node to a specific
>>> driver is the compatible string (this one is specified in the bindings
>>> doc).
>>
>> I know what you're saying, but what if someone uses the Flexcom driver
>> to wrap a different type of SPI driver where (for instance) the
>> compatible string used is "<name>-<newtype>".  Then we'd have to keep
>> adding more lines here to accommodate.
>>
>> Whereas if we used the child node name which only pertains to _this_
>> driver, we would then have full control and know that (unless it
>> Flexcom is used for a completely different type of serial controller)
>> we wouldn't have to keep expanding the code to accommodate.
> 
> You're right about the complexity implied by the compat string
> maintenance, but I still think using node names to detect the mode is
> a bad idea.
> 
> Let's take another example making both solution unsuitable: what if
> the flexcom-v2 exposes 2 devices of the same type, they will both have
> the same name and the same compatible string, and we'll have no way to
> detect the appropriate mode. That's why I think none of our suggestion
> is future-proof.
> 
>>
>>> Regarding the implementation itself, I would match the child node with
>>> an of_device_id table rather than trying to find a specific substring
>>> in the compatible string, but I think that's only a matter of taste.
>>
>> You mean duplicate each of the supported device's compatible strings
>> in this driver, then fetch the attributed of_match_device()->data
>> value?
>>
> 
> Yes, and that's definitely not a good idea, but I think Cyrille has
> found a better approach (I'll let him explain).

Indeed, what about taking advantage of the "ranges" property?

For the Flexcom:
#address-cells = <2>;
#size-cells = <1>;
ranges = <1 0 0xf8034200 0x200    /* opmode 1: USART */
          2 0 0xf8034400 0x200    /* opmode 2: SPI */
          3 0 0xf8034600 0x200>;  /* opmode 3: I2C */

Then for the single available child (for instance the SPI controller):
reg = <2 0 0x200>;

So the Operating Mode to be set into the Flexcom Mode Register is read from
the very first u32 of the "reg" property of the child.

No need to introduce any new DT property and the mapping remains easy to
maintain to follow hardware upgrades.

More details in v7 series.

> 
> Best Regards,
> 
> Boris
> 

Best Regards,

Cyrille

Re: [PATCH v6 2/2] mfd: atmel-flexcom: add a driver for Atmel Flexible Serial Communication Unit

[Lee Jones]

On Thu, 23 Jul 2015, Cyrille Pitchen wrote:

> Hi all,
> 
> Le 23/07/2015 14:50, Boris Brezillon a écrit :
> > On Thu, 23 Jul 2015 10:13:11 +0100
> > Lee Jones <lee.jones@linaro.org> wrote:
> > 
> >> On Thu, 23 Jul 2015, Boris Brezillon wrote:
> >>
> >>> Hi Lee,
> >>>
> >>> On Thu, 23 Jul 2015 08:32:17 +0100
> >>> Lee Jones <lee.jones@linaro.org> wrote:
> >>>
> >>>> On Wed, 22 Jul 2015, Cyrille Pitchen wrote:
> >>>>> +	for_each_child_of_node(np, child) {
> >>>>> +		const char *compatible;
> >>>>> +		int cplen;
> >>>>> +
> >>>>> +		if (!of_device_is_available(child))
> >>>>> +			continue;
> >>>>> +
> >>>>> +		compatible = of_get_property(child, "compatible", &cplen);
> >>>>> +		if (!compatible || strlen(compatible) > cplen)
> >>>>> +			continue;
> >>>>> +
> >>>>> +		if (strstr(compatible, "-usart")) {
> >>>>> +			opmode = FLEX_MR_OPMODE_USART;
> >>>>> +			break;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (strstr(compatible, "-spi")) {
> >>>>> +			opmode = FLEX_MR_OPMODE_SPI;
> >>>>> +			break;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (strstr(compatible, "-i2c")) {
> >>>>> +			opmode = FLEX_MR_OPMODE_TWI;
> >>>>> +			break;
> >>>>> +		}
> >>>>> +	}
> >>>>
> >>>> From what I understand Flexcom is a wrapper which can sit above any
> >>>> number of SPI, I2C and/or UART devices.  Devices which you don't
> >>>> really have any control over (source code wise).  So wouldn't it be
> >>>> better to match on the details you do have control over i.e. the node
> >>>> name, rather than the compatible string?
> >>>>
> >>>> I would personally match on of_find_node_by_name() to future-proof
> >>>> your implementation.
> >>>
> >>> Actually, I think using compatible strings is more future-proof than
> >>> using the node names, because nothing in the DT bindings doc enforce the
> >>> node name, and usually what we use to attach a node to a specific
> >>> driver is the compatible string (this one is specified in the bindings
> >>> doc).
> >>
> >> I know what you're saying, but what if someone uses the Flexcom driver
> >> to wrap a different type of SPI driver where (for instance) the
> >> compatible string used is "<name>-<newtype>".  Then we'd have to keep
> >> adding more lines here to accommodate.
> >>
> >> Whereas if we used the child node name which only pertains to _this_
> >> driver, we would then have full control and know that (unless it
> >> Flexcom is used for a completely different type of serial controller)
> >> we wouldn't have to keep expanding the code to accommodate.
> > 
> > You're right about the complexity implied by the compat string
> > maintenance, but I still think using node names to detect the mode is
> > a bad idea.
> > 
> > Let's take another example making both solution unsuitable: what if
> > the flexcom-v2 exposes 2 devices of the same type, they will both have
> > the same name and the same compatible string, and we'll have no way to
> > detect the appropriate mode. That's why I think none of our suggestion
> > is future-proof.
> > 
> >>
> >>> Regarding the implementation itself, I would match the child node with
> >>> an of_device_id table rather than trying to find a specific substring
> >>> in the compatible string, but I think that's only a matter of taste.
> >>
> >> You mean duplicate each of the supported device's compatible strings
> >> in this driver, then fetch the attributed of_match_device()->data
> >> value?
> >>
> > 
> > Yes, and that's definitely not a good idea, but I think Cyrille has
> > found a better approach (I'll let him explain).
> 
> Indeed, what about taking advantage of the "ranges" property?
> 
> For the Flexcom:
> #address-cells = <2>;
> #size-cells = <1>;
> ranges = <1 0 0xf8034200 0x200    /* opmode 1: USART */
>           2 0 0xf8034400 0x200    /* opmode 2: SPI */
>           3 0 0xf8034600 0x200>;  /* opmode 3: I2C */
> 
> Then for the single available child (for instance the SPI controller):
> reg = <2 0 0x200>;
> 
> So the Operating Mode to be set into the Flexcom Mode Register is read from
> the very first u32 of the "reg" property of the child.
> 
> No need to introduce any new DT property and the mapping remains easy to
> maintain to follow hardware upgrades.

When we do things like this we normally do:

    reg <base base_size>, <mode mode_size>;
    reg-names "base", "mode";

Then use:

     platform_get_resource_byname()

.... to fetch them.

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