* [PATCH v12 1/6] MAINTAINERS: add at91 usart mfd driver
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
@ 2018-09-04 11:13 ` Radu Pirea
2018-09-04 11:13 ` [PATCH v12 2/6] dt-bindings: add binding for atmel-usart in SPI mode Radu Pirea
` (7 subsequent siblings)
8 siblings, 0 replies; 38+ messages in thread
From: Radu Pirea @ 2018-09-04 11:13 UTC (permalink / raw)
To: lee.jones, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, gregkh, broonie, jslaby, richard.genoud,
davem, mchehab+samsung, akpm, arnd
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi, Radu Pirea
From: Radu Pirea <radu.pirea@microchip.com>
Added entry for at91 usart mfd driver.
Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 544cac829cf4..8aeaa2cc3e14 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9322,6 +9322,7 @@ M: Richard Genoud <richard.genoud@gmail.com>
S: Maintained
F: drivers/tty/serial/atmel_serial.c
F: drivers/tty/serial/atmel_serial.h
+F: Documentation/devicetree/bindings/mfd/atmel-usart.txt
MICROCHIP / ATMEL DMA DRIVER
M: Ludovic Desroches <ludovic.desroches@microchip.com>
@@ -9354,6 +9355,14 @@ S: Supported
F: drivers/mtd/nand/raw/atmel/*
F: Documentation/devicetree/bindings/mtd/atmel-nand.txt
+MICROCHIP AT91 USART MFD DRIVER
+M: Radu Pirea <radu_nicolae.pirea@upb.ro>
+L: linux-kernel@vger.kernel.org
+S: Supported
+F: drivers/mfd/at91-usart.c
+F: include/dt-bindings/mfd/at91-usart.h
+F: Documentation/devicetree/bindings/mfd/atmel-usart.txt
+
MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER
M: Woojung Huh <Woojung.Huh@microchip.com>
M: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v12 2/6] dt-bindings: add binding for atmel-usart in SPI mode
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
2018-09-04 11:13 ` [PATCH v12 1/6] MAINTAINERS: add at91 usart mfd driver Radu Pirea
@ 2018-09-04 11:13 ` Radu Pirea
2018-09-04 11:13 ` [PATCH v12 3/6] mfd: at91-usart: added mfd driver for usart Radu Pirea
` (6 subsequent siblings)
8 siblings, 0 replies; 38+ messages in thread
From: Radu Pirea @ 2018-09-04 11:13 UTC (permalink / raw)
To: lee.jones, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, gregkh, broonie, jslaby, richard.genoud,
davem, mchehab+samsung, akpm, arnd
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi, Radu Pirea
From: Radu Pirea <radu.pirea@microchip.com>
This patch moves the bindings for serial from serial/atmel-usart.txt to
mfd/atmel-usart.txt and adds bindings for USART in SPI mode.
Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
.../bindings/{serial => mfd}/atmel-usart.txt | 25 +++++++++++++++++--
include/dt-bindings/mfd/at91-usart.h | 17 +++++++++++++
2 files changed, 40 insertions(+), 2 deletions(-)
rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
create mode 100644 include/dt-bindings/mfd/at91-usart.h
diff --git a/Documentation/devicetree/bindings/serial/atmel-usart.txt b/Documentation/devicetree/bindings/mfd/atmel-usart.txt
similarity index 76%
rename from Documentation/devicetree/bindings/serial/atmel-usart.txt
rename to Documentation/devicetree/bindings/mfd/atmel-usart.txt
index 7c0d6b2f53e4..7f0cd72f47d2 100644
--- a/Documentation/devicetree/bindings/serial/atmel-usart.txt
+++ b/Documentation/devicetree/bindings/mfd/atmel-usart.txt
@@ -1,6 +1,6 @@
* Atmel Universal Synchronous Asynchronous Receiver/Transmitter (USART)
-Required properties:
+Required properties for USART:
- compatible: Should be "atmel,<chip>-usart" or "atmel,<chip>-dbgu"
The compatible <chip> indicated will be the first SoC to support an
additional mode or an USART new feature.
@@ -11,7 +11,13 @@ Required properties:
Required elements: "usart"
- clocks: phandles to input clocks.
-Optional properties:
+Required properties for USART in SPI mode:
+- #size-cells : Must be <0>
+- #address-cells : Must be <1>
+- cs-gpios: chipselects (internal cs not supported)
+- atmel,usart-mode : Must be <AT91_USART_MODE_SPI> (found in dt-bindings/mfd/at91-usart.h)
+
+Optional properties in serial mode:
- atmel,use-dma-rx: use of PDC or DMA for receiving data
- atmel,use-dma-tx: use of PDC or DMA for transmitting data
- {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD line respectively.
@@ -62,3 +68,18 @@ Example:
dma-names = "tx", "rx";
atmel,fifo-size = <32>;
};
+
+- SPI mode:
+ #include <dt-bindings/mfd/at91-usart.h>
+
+ spi0: spi@f001c000 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "atmel,at91rm9200-usart", "atmel,at91sam9260-usart";
+ atmel,usart-mode = <AT91_USART_MODE_SPI>;
+ reg = <0xf001c000 0x100>;
+ interrupts = <12 IRQ_TYPE_LEVEL_HIGH 5>;
+ clocks = <&usart0_clk>;
+ clock-names = "usart";
+ cs-gpios = <&pioB 3 0>;
+ };
diff --git a/include/dt-bindings/mfd/at91-usart.h b/include/dt-bindings/mfd/at91-usart.h
new file mode 100644
index 000000000000..2de5bc312e1e
--- /dev/null
+++ b/include/dt-bindings/mfd/at91-usart.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides macros for AT91 USART DT bindings.
+ *
+ * Copyright (C) 2018 Microchip Technology
+ *
+ * Author: Radu Pirea <radu.pirea@microchip.com>
+ *
+ */
+
+#ifndef __DT_BINDINGS_AT91_USART_H__
+#define __DT_BINDINGS_AT91_USART_H__
+
+#define AT91_USART_MODE_SERIAL 0
+#define AT91_USART_MODE_SPI 1
+
+#endif /* __DT_BINDINGS_AT91_USART_H__ */
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v12 3/6] mfd: at91-usart: added mfd driver for usart
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
2018-09-04 11:13 ` [PATCH v12 1/6] MAINTAINERS: add at91 usart mfd driver Radu Pirea
2018-09-04 11:13 ` [PATCH v12 2/6] dt-bindings: add binding for atmel-usart in SPI mode Radu Pirea
@ 2018-09-04 11:13 ` Radu Pirea
2018-09-04 11:13 ` [PATCH v12 4/6] MAINTAINERS: add at91 usart spi driver Radu Pirea
` (5 subsequent siblings)
8 siblings, 0 replies; 38+ messages in thread
From: Radu Pirea @ 2018-09-04 11:13 UTC (permalink / raw)
To: lee.jones, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, gregkh, broonie, jslaby, richard.genoud,
davem, mchehab+samsung, akpm, arnd
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi, Radu Pirea
From: Radu Pirea <radu.pirea@microchip.com>
This mfd driver is just a wrapper over atmel_serial driver and
spi-at91-usart driver. Selection of one of the drivers is based on a
property from device tree. If the property is not specified, the default
driver is atmel_serial.
Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Rob Herring <robh@kernel.org>
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
drivers/mfd/Kconfig | 9 +++++
drivers/mfd/Makefile | 1 +
drivers/mfd/at91-usart.c | 71 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 81 insertions(+)
create mode 100644 drivers/mfd/at91-usart.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b860eb5aa194..a886672b960d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -99,6 +99,15 @@ config MFD_AAT2870_CORE
additional drivers must be enabled in order to use the
functionality of the device.
+config MFD_AT91_USART
+ tristate "AT91 USART Driver"
+ select MFD_CORE
+ help
+ Select this to get support for AT91 USART IP. This is a wrapper
+ over at91-usart-serial driver and usart-spi-driver. 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_FLEXCOM
tristate "Atmel Flexcom (Flexible Serial Communication Unit)"
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index e9fd20dba18d..c7e66928abb9 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -184,6 +184,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_AT91_USART) += at91-usart.o
obj-$(CONFIG_MFD_ATMEL_FLEXCOM) += atmel-flexcom.o
obj-$(CONFIG_MFD_ATMEL_HLCDC) += atmel-hlcdc.o
obj-$(CONFIG_MFD_ATMEL_SMC) += atmel-smc.o
diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
new file mode 100644
index 000000000000..a4b9929c156f
--- /dev/null
+++ b/drivers/mfd/at91-usart.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for AT91 USART
+ *
+ * Copyright (C) 2018 Microchip Technology
+ *
+ * Author: Radu Pirea <radu.pirea@microchip.com>
+ *
+ */
+
+#include <dt-bindings/mfd/at91-usart.h>
+
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/property.h>
+
+static struct mfd_cell at91_usart_spi_subdev = {
+ .name = "at91_usart_spi",
+ .of_compatible = "microchip,at91sam9g45-usart-spi",
+ };
+
+static struct mfd_cell at91_usart_serial_subdev = {
+ .name = "atmel_usart_serial",
+ .of_compatible = "atmel,at91rm9200-usart-serial",
+ };
+
+static int at91_usart_mode_probe(struct platform_device *pdev)
+{
+ struct mfd_cell cell;
+ u32 opmode = AT91_USART_MODE_SERIAL;
+
+ device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
+
+ switch (opmode) {
+ case AT91_USART_MODE_SPI:
+ cell = at91_usart_spi_subdev;
+ break;
+ case AT91_USART_MODE_SERIAL:
+ cell = at91_usart_serial_subdev;
+ break;
+ default:
+ dev_err(&pdev->dev, "atmel,usart-mode has an invalid value %u\n",
+ opmode);
+ return -EINVAL;
+ }
+
+ return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO, &cell, 1,
+ NULL, 0, NULL);
+}
+
+static const struct of_device_id at91_usart_mode_of_match[] = {
+ { .compatible = "atmel,at91rm9200-usart" },
+ { .compatible = "atmel,at91sam9260-usart" },
+ { /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, at91_usart_mode_of_match);
+
+static struct platform_driver at91_usart_mfd = {
+ .probe = at91_usart_mode_probe,
+ .driver = {
+ .name = "at91_usart_mode",
+ .of_match_table = at91_usart_mode_of_match,
+ },
+};
+
+module_platform_driver(at91_usart_mfd);
+
+MODULE_AUTHOR("Radu Pirea <radu.pirea@microchip.com>");
+MODULE_DESCRIPTION("AT91 USART MFD driver");
+MODULE_LICENSE("GPL v2");
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v12 4/6] MAINTAINERS: add at91 usart spi driver
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
` (2 preceding siblings ...)
2018-09-04 11:13 ` [PATCH v12 3/6] mfd: at91-usart: added mfd driver for usart Radu Pirea
@ 2018-09-04 11:13 ` Radu Pirea
2018-09-04 11:13 ` [PATCH v12 5/6] spi: at91-usart: add driver for at91-usart as spi Radu Pirea
` (4 subsequent siblings)
8 siblings, 0 replies; 38+ messages in thread
From: Radu Pirea @ 2018-09-04 11:13 UTC (permalink / raw)
To: lee.jones, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, gregkh, broonie, jslaby, richard.genoud,
davem, mchehab+samsung, akpm, arnd
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi, Radu Pirea
From: Radu Pirea <radu.pirea@microchip.com>
Added entry for at91 usart mfd driver.
Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 8aeaa2cc3e14..1bb477aab33b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9363,6 +9363,13 @@ F: drivers/mfd/at91-usart.c
F: include/dt-bindings/mfd/at91-usart.h
F: Documentation/devicetree/bindings/mfd/atmel-usart.txt
+MICROCHIP AT91 USART SPI DRIVER
+M: Radu Pirea <radu_nicolae.pirea@upb.ro>
+L: linux-spi@vger.kernel.org
+S: Supported
+F: drivers/spi/spi-at91-usart.c
+F: Documentation/devicetree/bindings/mfd/atmel-usart.txt
+
MICROCHIP KSZ SERIES ETHERNET SWITCH DRIVER
M: Woojung Huh <Woojung.Huh@microchip.com>
M: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v12 5/6] spi: at91-usart: add driver for at91-usart as spi
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
` (3 preceding siblings ...)
2018-09-04 11:13 ` [PATCH v12 4/6] MAINTAINERS: add at91 usart spi driver Radu Pirea
@ 2018-09-04 11:13 ` Radu Pirea
2018-09-04 11:13 ` [PATCH v12 6/6] tty/serial: atmel: change the driver to work under at91-usart mfd Radu Pirea
` (3 subsequent siblings)
8 siblings, 0 replies; 38+ messages in thread
From: Radu Pirea @ 2018-09-04 11:13 UTC (permalink / raw)
To: lee.jones, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, gregkh, broonie, jslaby, richard.genoud,
davem, mchehab+samsung, akpm, arnd
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi, Radu Pirea
From: Radu Pirea <radu.pirea@microchip.com>
This is the driver for at91-usart in spi mode. The USART IP can be configured
to work in many modes and one of them is SPI.
The driver was tested on sama5d3-xplained and sama5d4-xplained boards with
enc28j60 ethernet controller as slave.
Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviwed-by: Mark Brown <broonie@kernel.org>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
---
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-at91-usart.c | 432 +++++++++++++++++++++++++++++++++++
3 files changed, 441 insertions(+)
create mode 100644 drivers/spi/spi-at91-usart.c
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ad5d68e1dab7..6d9e309fab67 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -83,6 +83,14 @@ config SPI_ATMEL
This selects a driver for the Atmel SPI Controller, present on
many AT91 ARM chips.
+config SPI_AT91_USART
+ tristate "Atmel USART Controller SPI driver"
+ depends on (ARCH_AT91 || COMPILE_TEST)
+ depends on MFD_AT91_USART
+ help
+ This selects a driver for the AT91 USART Controller as SPI Master,
+ present on AT91 and SAMA5 SoC series.
+
config SPI_AU1550
tristate "Au1550/Au1200/Au1300 SPI Controller"
depends on MIPS_ALCHEMY
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index cb1f4378b87c..901ff606e6e5 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_SPI_LOOPBACK_TEST) += spi-loopback-test.o
obj-$(CONFIG_SPI_ALTERA) += spi-altera.o
obj-$(CONFIG_SPI_ARMADA_3700) += spi-armada-3700.o
obj-$(CONFIG_SPI_ATMEL) += spi-atmel.o
+obj-$(CONFIG_SPI_AT91_USART) += spi-at91-usart.o
obj-$(CONFIG_SPI_ATH79) += spi-ath79.o
obj-$(CONFIG_SPI_AU1550) += spi-au1550.o
obj-$(CONFIG_SPI_AXI_SPI_ENGINE) += spi-axi-spi-engine.o
diff --git a/drivers/spi/spi-at91-usart.c b/drivers/spi/spi-at91-usart.c
new file mode 100644
index 000000000000..a924657642fa
--- /dev/null
+++ b/drivers/spi/spi-at91-usart.c
@@ -0,0 +1,432 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Driver for AT91 USART Controllers as SPI
+//
+// Copyright (C) 2018 Microchip Technology Inc.
+//
+// Author: Radu Pirea <radu.pirea@microchip.com>
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+
+#include <linux/spi/spi.h>
+
+#define US_CR 0x00
+#define US_MR 0x04
+#define US_IER 0x08
+#define US_IDR 0x0C
+#define US_CSR 0x14
+#define US_RHR 0x18
+#define US_THR 0x1C
+#define US_BRGR 0x20
+#define US_VERSION 0xFC
+
+#define US_CR_RSTRX BIT(2)
+#define US_CR_RSTTX BIT(3)
+#define US_CR_RXEN BIT(4)
+#define US_CR_RXDIS BIT(5)
+#define US_CR_TXEN BIT(6)
+#define US_CR_TXDIS BIT(7)
+
+#define US_MR_SPI_MASTER 0x0E
+#define US_MR_CHRL GENMASK(7, 6)
+#define US_MR_CPHA BIT(8)
+#define US_MR_CPOL BIT(16)
+#define US_MR_CLKO BIT(18)
+#define US_MR_WRDBT BIT(20)
+#define US_MR_LOOP BIT(15)
+
+#define US_IR_RXRDY BIT(0)
+#define US_IR_TXRDY BIT(1)
+#define US_IR_OVRE BIT(5)
+
+#define US_BRGR_SIZE BIT(16)
+
+#define US_MIN_CLK_DIV 0x06
+#define US_MAX_CLK_DIV BIT(16)
+
+#define US_RESET (US_CR_RSTRX | US_CR_RSTTX)
+#define US_DISABLE (US_CR_RXDIS | US_CR_TXDIS)
+#define US_ENABLE (US_CR_RXEN | US_CR_TXEN)
+#define US_OVRE_RXRDY_IRQS (US_IR_OVRE | US_IR_RXRDY)
+
+#define US_INIT \
+ (US_MR_SPI_MASTER | US_MR_CHRL | US_MR_CLKO | US_MR_WRDBT)
+
+/* Register access macros */
+#define at91_usart_spi_readl(port, reg) \
+ readl_relaxed((port)->regs + US_##reg)
+#define at91_usart_spi_writel(port, reg, value) \
+ writel_relaxed((value), (port)->regs + US_##reg)
+
+#define at91_usart_spi_readb(port, reg) \
+ readb_relaxed((port)->regs + US_##reg)
+#define at91_usart_spi_writeb(port, reg, value) \
+ writeb_relaxed((value), (port)->regs + US_##reg)
+
+struct at91_usart_spi {
+ struct spi_transfer *current_transfer;
+ void __iomem *regs;
+ struct device *dev;
+ struct clk *clk;
+
+ /*used in interrupt to protect data reading*/
+ spinlock_t lock;
+
+ int irq;
+ unsigned int current_tx_remaining_bytes;
+ unsigned int current_rx_remaining_bytes;
+
+ u32 spi_clk;
+ u32 status;
+
+ bool xfer_failed;
+};
+
+static inline u32 at91_usart_spi_tx_ready(struct at91_usart_spi *aus)
+{
+ return aus->status & US_IR_TXRDY;
+}
+
+static inline u32 at91_usart_spi_rx_ready(struct at91_usart_spi *aus)
+{
+ return aus->status & US_IR_RXRDY;
+}
+
+static inline u32 at91_usart_spi_check_overrun(struct at91_usart_spi *aus)
+{
+ return aus->status & US_IR_OVRE;
+}
+
+static inline u32 at91_usart_spi_read_status(struct at91_usart_spi *aus)
+{
+ aus->status = at91_usart_spi_readl(aus, CSR);
+ return aus->status;
+}
+
+static inline void at91_usart_spi_tx(struct at91_usart_spi *aus)
+{
+ unsigned int len = aus->current_transfer->len;
+ unsigned int remaining = aus->current_tx_remaining_bytes;
+ const u8 *tx_buf = aus->current_transfer->tx_buf;
+
+ if (!remaining)
+ return;
+
+ if (at91_usart_spi_tx_ready(aus)) {
+ at91_usart_spi_writeb(aus, THR, tx_buf[len - remaining]);
+ aus->current_tx_remaining_bytes--;
+ }
+}
+
+static inline void at91_usart_spi_rx(struct at91_usart_spi *aus)
+{
+ int len = aus->current_transfer->len;
+ int remaining = aus->current_rx_remaining_bytes;
+ u8 *rx_buf = aus->current_transfer->rx_buf;
+
+ if (!remaining)
+ return;
+
+ rx_buf[len - remaining] = at91_usart_spi_readb(aus, RHR);
+ aus->current_rx_remaining_bytes--;
+}
+
+static inline void
+at91_usart_spi_set_xfer_speed(struct at91_usart_spi *aus,
+ struct spi_transfer *xfer)
+{
+ at91_usart_spi_writel(aus, BRGR,
+ DIV_ROUND_UP(aus->spi_clk, xfer->speed_hz));
+}
+
+static irqreturn_t at91_usart_spi_interrupt(int irq, void *dev_id)
+{
+ struct spi_controller *controller = dev_id;
+ struct at91_usart_spi *aus = spi_master_get_devdata(controller);
+
+ spin_lock(&aus->lock);
+ at91_usart_spi_read_status(aus);
+
+ if (at91_usart_spi_check_overrun(aus)) {
+ aus->xfer_failed = true;
+ at91_usart_spi_writel(aus, IDR, US_IR_OVRE | US_IR_RXRDY);
+ spin_unlock(&aus->lock);
+ return IRQ_HANDLED;
+ }
+
+ if (at91_usart_spi_rx_ready(aus)) {
+ at91_usart_spi_rx(aus);
+ spin_unlock(&aus->lock);
+ return IRQ_HANDLED;
+ }
+
+ spin_unlock(&aus->lock);
+
+ return IRQ_NONE;
+}
+
+static int at91_usart_spi_setup(struct spi_device *spi)
+{
+ struct at91_usart_spi *aus = spi_master_get_devdata(spi->controller);
+ u32 *ausd = spi->controller_state;
+ unsigned int mr = at91_usart_spi_readl(aus, MR);
+ u8 bits = spi->bits_per_word;
+
+ if (bits != 8) {
+ dev_dbg(&spi->dev, "Only 8 bits per word are supported\n");
+ return -EINVAL;
+ }
+
+ if (spi->mode & SPI_CPOL)
+ mr |= US_MR_CPOL;
+ else
+ mr &= ~US_MR_CPOL;
+
+ if (spi->mode & SPI_CPHA)
+ mr |= US_MR_CPHA;
+ else
+ mr &= ~US_MR_CPHA;
+
+ if (spi->mode & SPI_LOOP)
+ mr |= US_MR_LOOP;
+ else
+ mr &= ~US_MR_LOOP;
+
+ if (!ausd) {
+ ausd = kzalloc(sizeof(*ausd), GFP_KERNEL);
+ if (!ausd)
+ return -ENOMEM;
+
+ spi->controller_state = ausd;
+ }
+
+ *ausd = mr;
+
+ dev_dbg(&spi->dev,
+ "setup: bpw %u mode 0x%x -> mr %d %08x\n",
+ bits, spi->mode, spi->chip_select, mr);
+
+ return 0;
+}
+
+static int at91_usart_spi_transfer_one(struct spi_controller *ctlr,
+ struct spi_device *spi,
+ struct spi_transfer *xfer)
+{
+ struct at91_usart_spi *aus = spi_master_get_devdata(ctlr);
+
+ at91_usart_spi_set_xfer_speed(aus, xfer);
+ aus->xfer_failed = false;
+ aus->current_transfer = xfer;
+ aus->current_tx_remaining_bytes = xfer->len;
+ aus->current_rx_remaining_bytes = xfer->len;
+
+ while ((aus->current_tx_remaining_bytes ||
+ aus->current_rx_remaining_bytes) && !aus->xfer_failed) {
+ at91_usart_spi_read_status(aus);
+ at91_usart_spi_tx(aus);
+ cpu_relax();
+ }
+
+ if (aus->xfer_failed) {
+ dev_err(aus->dev, "Overrun!\n");
+ return -EIO;
+ }
+
+ return 0;
+}
+
+static int at91_usart_spi_prepare_message(struct spi_controller *ctlr,
+ struct spi_message *message)
+{
+ struct at91_usart_spi *aus = spi_master_get_devdata(ctlr);
+ struct spi_device *spi = message->spi;
+ u32 *ausd = spi->controller_state;
+
+ at91_usart_spi_writel(aus, CR, US_ENABLE);
+ at91_usart_spi_writel(aus, IER, US_OVRE_RXRDY_IRQS);
+ at91_usart_spi_writel(aus, MR, *ausd);
+
+ return 0;
+}
+
+static int at91_usart_spi_unprepare_message(struct spi_controller *ctlr,
+ struct spi_message *message)
+{
+ struct at91_usart_spi *aus = spi_master_get_devdata(ctlr);
+
+ at91_usart_spi_writel(aus, CR, US_RESET | US_DISABLE);
+ at91_usart_spi_writel(aus, IDR, US_OVRE_RXRDY_IRQS);
+
+ return 0;
+}
+
+static void at91_usart_spi_cleanup(struct spi_device *spi)
+{
+ struct at91_usart_spi_device *ausd = spi->controller_state;
+
+ spi->controller_state = NULL;
+ kfree(ausd);
+}
+
+static void at91_usart_spi_init(struct at91_usart_spi *aus)
+{
+ at91_usart_spi_writel(aus, MR, US_INIT);
+ at91_usart_spi_writel(aus, CR, US_RESET | US_DISABLE);
+}
+
+static int at91_usart_gpio_setup(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.parent->of_node;
+ int i;
+ int ret;
+ int nb;
+
+ if (!np)
+ return -EINVAL;
+
+ nb = of_gpio_named_count(np, "cs-gpios");
+ for (i = 0; i < nb; i++) {
+ int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
+
+ if (cs_gpio < 0)
+ return cs_gpio;
+
+ if (gpio_is_valid(cs_gpio)) {
+ ret = devm_gpio_request_one(&pdev->dev, cs_gpio,
+ GPIOF_DIR_OUT,
+ dev_name(&pdev->dev));
+ if (ret)
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int at91_usart_spi_probe(struct platform_device *pdev)
+{
+ struct resource *regs;
+ struct spi_controller *controller;
+ struct at91_usart_spi *aus;
+ struct clk *clk;
+ int irq;
+ int ret;
+
+ regs = platform_get_resource(to_platform_device(pdev->dev.parent),
+ IORESOURCE_MEM, 0);
+ if (!regs)
+ return -EINVAL;
+
+ irq = platform_get_irq(to_platform_device(pdev->dev.parent), 0);
+ if (irq < 0)
+ return irq;
+
+ clk = devm_clk_get(pdev->dev.parent, "usart");
+ if (IS_ERR(clk))
+ return PTR_ERR(clk);
+
+ ret = -ENOMEM;
+ controller = spi_alloc_master(&pdev->dev, sizeof(*aus));
+ if (!controller)
+ goto at91_usart_spi_probe_fail;
+
+ ret = at91_usart_gpio_setup(pdev);
+ if (ret)
+ goto at91_usart_spi_probe_fail;
+
+ controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LOOP | SPI_CS_HIGH;
+ controller->dev.of_node = pdev->dev.parent->of_node;
+ controller->bits_per_word_mask = SPI_BPW_MASK(8);
+ controller->setup = at91_usart_spi_setup;
+ controller->flags = SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX;
+ controller->transfer_one = at91_usart_spi_transfer_one;
+ controller->prepare_message = at91_usart_spi_prepare_message;
+ controller->unprepare_message = at91_usart_spi_unprepare_message;
+ controller->cleanup = at91_usart_spi_cleanup;
+ controller->max_speed_hz = DIV_ROUND_UP(clk_get_rate(clk),
+ US_MIN_CLK_DIV);
+ controller->min_speed_hz = DIV_ROUND_UP(clk_get_rate(clk),
+ US_MAX_CLK_DIV);
+ platform_set_drvdata(pdev, controller);
+
+ aus = spi_master_get_devdata(controller);
+
+ aus->dev = &pdev->dev;
+ aus->regs = devm_ioremap_resource(&pdev->dev, regs);
+ if (IS_ERR(aus->regs)) {
+ ret = PTR_ERR(aus->regs);
+ goto at91_usart_spi_probe_fail;
+ }
+
+ aus->irq = irq;
+ aus->clk = clk;
+
+ ret = devm_request_irq(&pdev->dev, irq, at91_usart_spi_interrupt, 0,
+ dev_name(&pdev->dev), controller);
+ if (ret)
+ goto at91_usart_spi_probe_fail;
+
+ ret = clk_prepare_enable(clk);
+ if (ret)
+ goto at91_usart_spi_probe_fail;
+
+ aus->spi_clk = clk_get_rate(clk);
+ at91_usart_spi_init(aus);
+
+ spin_lock_init(&aus->lock);
+ ret = devm_spi_register_master(&pdev->dev, controller);
+ if (ret)
+ goto at91_usart_fail_register_master;
+
+ dev_info(&pdev->dev,
+ "AT91 USART SPI Controller version 0x%x at %pa (irq %d)\n",
+ at91_usart_spi_readl(aus, VERSION),
+ ®s->start, irq);
+
+ return 0;
+
+at91_usart_fail_register_master:
+ clk_disable_unprepare(clk);
+at91_usart_spi_probe_fail:
+ spi_master_put(controller);
+ return ret;
+}
+
+static int at91_usart_spi_remove(struct platform_device *pdev)
+{
+ struct spi_controller *ctlr = platform_get_drvdata(pdev);
+ struct at91_usart_spi *aus = spi_master_get_devdata(ctlr);
+
+ clk_disable_unprepare(aus->clk);
+
+ return 0;
+}
+
+static const struct of_device_id at91_usart_spi_dt_ids[] = {
+ { .compatible = "microchip,at91sam9g45-usart-spi"},
+ { /* sentinel */}
+};
+
+MODULE_DEVICE_TABLE(of, at91_usart_spi_dt_ids);
+
+static struct platform_driver at91_usart_spi_driver = {
+ .driver = {
+ .name = "at91_usart_spi",
+ },
+ .probe = at91_usart_spi_probe,
+ .remove = at91_usart_spi_remove,
+};
+
+module_platform_driver(at91_usart_spi_driver);
+
+MODULE_DESCRIPTION("Microchip AT91 USART SPI Controller driver");
+MODULE_AUTHOR("Radu Pirea <radu.pirea@microchip.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:at91_usart_spi");
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH v12 6/6] tty/serial: atmel: change the driver to work under at91-usart mfd
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
` (4 preceding siblings ...)
2018-09-04 11:13 ` [PATCH v12 5/6] spi: at91-usart: add driver for at91-usart as spi Radu Pirea
@ 2018-09-04 11:13 ` Radu Pirea
2018-09-10 9:48 ` [PATCH v12 0/6] Driver for at91 usart in spi mode Lee Jones
` (2 subsequent siblings)
8 siblings, 0 replies; 38+ messages in thread
From: Radu Pirea @ 2018-09-04 11:13 UTC (permalink / raw)
To: lee.jones, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, gregkh, broonie, jslaby, richard.genoud,
davem, mchehab+samsung, akpm, arnd
Cc: devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi, Radu Pirea
From: Radu Pirea <radu.pirea@microchip.com>
This patch modifies the place where resources and device tree properties
are searched.
Signed-off-by: Radu Pirea <radu.pirea@microchip.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Richard Genoud <richard.genoud@gmail.com>
Acked-by: Nicolas Ferre <nicolas.ferre@microchip.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/atmel_serial.c | 42 ++++++++++++++++++++-----------
2 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index df8bd0c7b97d..32886c304641 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -118,6 +118,7 @@ config SERIAL_ATMEL
depends on ARCH_AT91 || COMPILE_TEST
select SERIAL_CORE
select SERIAL_MCTRL_GPIO if GPIOLIB
+ select MFD_AT91_USART
help
This enables the driver for the on-chip UARTs of the Atmel
AT91 processors.
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 8e4428725848..267d4d1de3f8 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -193,8 +193,7 @@ static struct console atmel_console;
#if defined(CONFIG_OF)
static const struct of_device_id atmel_serial_dt_ids[] = {
- { .compatible = "atmel,at91rm9200-usart" },
- { .compatible = "atmel,at91sam9260-usart" },
+ { .compatible = "atmel,at91rm9200-usart-serial" },
{ /* sentinel */ }
};
#endif
@@ -915,6 +914,7 @@ static void atmel_tx_dma(struct uart_port *port)
static int atmel_prepare_tx_dma(struct uart_port *port)
{
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ struct device *mfd_dev = port->dev->parent;
dma_cap_mask_t mask;
struct dma_slave_config config;
int ret, nent;
@@ -922,7 +922,7 @@ static int atmel_prepare_tx_dma(struct uart_port *port)
dma_cap_zero(mask);
dma_cap_set(DMA_SLAVE, mask);
- atmel_port->chan_tx = dma_request_slave_channel(port->dev, "tx");
+ atmel_port->chan_tx = dma_request_slave_channel(mfd_dev, "tx");
if (atmel_port->chan_tx == NULL)
goto chan_err;
dev_info(port->dev, "using %s for tx DMA transfers\n",
@@ -1093,6 +1093,7 @@ static void atmel_rx_from_dma(struct uart_port *port)
static int atmel_prepare_rx_dma(struct uart_port *port)
{
struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ struct device *mfd_dev = port->dev->parent;
struct dma_async_tx_descriptor *desc;
dma_cap_mask_t mask;
struct dma_slave_config config;
@@ -1104,7 +1105,7 @@ static int atmel_prepare_rx_dma(struct uart_port *port)
dma_cap_zero(mask);
dma_cap_set(DMA_CYCLIC, mask);
- atmel_port->chan_rx = dma_request_slave_channel(port->dev, "rx");
+ atmel_port->chan_rx = dma_request_slave_channel(mfd_dev, "rx");
if (atmel_port->chan_rx == NULL)
goto chan_err;
dev_info(port->dev, "using %s for rx DMA transfers\n",
@@ -2222,8 +2223,8 @@ static const char *atmel_type(struct uart_port *port)
*/
static void atmel_release_port(struct uart_port *port)
{
- struct platform_device *pdev = to_platform_device(port->dev);
- int size = pdev->resource[0].end - pdev->resource[0].start + 1;
+ struct platform_device *mpdev = to_platform_device(port->dev->parent);
+ int size = resource_size(mpdev->resource);
release_mem_region(port->mapbase, size);
@@ -2238,8 +2239,8 @@ static void atmel_release_port(struct uart_port *port)
*/
static int atmel_request_port(struct uart_port *port)
{
- struct platform_device *pdev = to_platform_device(port->dev);
- int size = pdev->resource[0].end - pdev->resource[0].start + 1;
+ struct platform_device *mpdev = to_platform_device(port->dev->parent);
+ int size = resource_size(mpdev->resource);
if (!request_mem_region(port->mapbase, size, "atmel_serial"))
return -EBUSY;
@@ -2341,27 +2342,28 @@ static int atmel_init_port(struct atmel_uart_port *atmel_port,
{
int ret;
struct uart_port *port = &atmel_port->uart;
+ struct platform_device *mpdev = to_platform_device(pdev->dev.parent);
atmel_init_property(atmel_port, pdev);
atmel_set_ops(port);
- uart_get_rs485_mode(&pdev->dev, &port->rs485);
+ uart_get_rs485_mode(&mpdev->dev, &port->rs485);
port->iotype = UPIO_MEM;
port->flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP;
port->ops = &atmel_pops;
port->fifosize = 1;
port->dev = &pdev->dev;
- port->mapbase = pdev->resource[0].start;
- port->irq = pdev->resource[1].start;
+ port->mapbase = mpdev->resource[0].start;
+ port->irq = mpdev->resource[1].start;
port->rs485_config = atmel_config_rs485;
- port->membase = NULL;
+ port->membase = NULL;
memset(&atmel_port->rx_ring, 0, sizeof(atmel_port->rx_ring));
/* for console, the clock could already be configured */
if (!atmel_port->clk) {
- atmel_port->clk = clk_get(&pdev->dev, "usart");
+ atmel_port->clk = clk_get(&mpdev->dev, "usart");
if (IS_ERR(atmel_port->clk)) {
ret = PTR_ERR(atmel_port->clk);
atmel_port->clk = NULL;
@@ -2694,13 +2696,22 @@ static void atmel_serial_probe_fifos(struct atmel_uart_port *atmel_port,
static int atmel_serial_probe(struct platform_device *pdev)
{
struct atmel_uart_port *atmel_port;
- struct device_node *np = pdev->dev.of_node;
+ struct device_node *np = pdev->dev.parent->of_node;
void *data;
int ret = -ENODEV;
bool rs485_enabled;
BUILD_BUG_ON(ATMEL_SERIAL_RINGSIZE & (ATMEL_SERIAL_RINGSIZE - 1));
+ /*
+ * In device tree there is no node with "atmel,at91rm9200-usart-serial"
+ * as compatible string. This driver is probed by at91-usart mfd driver
+ * which is just a wrapper over the atmel_serial driver and
+ * spi-at91-usart driver. All attributes needed by this driver are
+ * found in of_node of parent.
+ */
+ pdev->dev.of_node = np;
+
ret = of_alias_get_id(np, "serial");
if (ret < 0)
/* port id not found in platform data nor device-tree aliases:
@@ -2836,6 +2847,7 @@ static int atmel_serial_remove(struct platform_device *pdev)
clk_put(atmel_port->clk);
atmel_port->clk = NULL;
+ pdev->dev.of_node = NULL;
return ret;
}
@@ -2846,7 +2858,7 @@ static struct platform_driver atmel_serial_driver = {
.suspend = atmel_serial_suspend,
.resume = atmel_serial_resume,
.driver = {
- .name = "atmel_usart",
+ .name = "atmel_usart_serial",
.of_match_table = of_match_ptr(atmel_serial_dt_ids),
},
};
--
2.18.0
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
` (5 preceding siblings ...)
2018-09-04 11:13 ` [PATCH v12 6/6] tty/serial: atmel: change the driver to work under at91-usart mfd Radu Pirea
@ 2018-09-10 9:48 ` Lee Jones
2018-09-10 9:51 ` Nicolas Ferre
2018-09-11 9:33 ` Lee Jones
2018-09-11 11:18 ` [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window Lee Jones
8 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-10 9:48 UTC (permalink / raw)
To: Radu Pirea
Cc: robh+dt, mark.rutland, nicolas.ferre, alexandre.belloni, gregkh,
broonie, jslaby, richard.genoud, davem, mchehab+samsung, akpm,
arnd, devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi
On Tue, 04 Sep 2018, Radu Pirea wrote:
> Well, this is the 12th version of this patch series.
> In this version I fixed a warning from kbuild-robot and I have no idea
> how I forgot to add static in declaration of that functions. Also I
> fixed the example for the SPI driver in bindings.
Okay great. So which patches still require Acks?
[...]
> Radu Pirea (6):
> MAINTAINERS: add at91 usart mfd driver
> dt-bindings: add binding for atmel-usart in SPI mode
> mfd: at91-usart: added mfd driver for usart
> MAINTAINERS: add at91 usart spi driver
> spi: at91-usart: add driver for at91-usart as spi
> tty/serial: atmel: change the driver to work under at91-usart mfd
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-10 9:48 ` [PATCH v12 0/6] Driver for at91 usart in spi mode Lee Jones
@ 2018-09-10 9:51 ` Nicolas Ferre
2018-09-10 13:43 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Nicolas Ferre @ 2018-09-10 9:51 UTC (permalink / raw)
To: Lee Jones, Radu Pirea
Cc: robh+dt, mark.rutland, alexandre.belloni, gregkh, broonie,
jslaby, richard.genoud, davem, mchehab+samsung, akpm, arnd,
devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi
Hi Lee,
On 10/09/2018 at 11:48, Lee Jones wrote:
> On Tue, 04 Sep 2018, Radu Pirea wrote:
>> Well, this is the 12th version of this patch series.
>> In this version I fixed a warning from kbuild-robot and I have no idea
>> how I forgot to add static in declaration of that functions. Also I
>> fixed the example for the SPI driver in bindings.
>
> Okay great. So which patches still require Acks?
None I would say: everything's ready to be integrated into your MFD tree
as you proposed some time ago.
We are looking forward seeing this series integrated in linux-next as
soon as possible.
Best regards,
Nicolas
>> Radu Pirea (6):
>> MAINTAINERS: add at91 usart mfd driver
>> dt-bindings: add binding for atmel-usart in SPI mode
>> mfd: at91-usart: added mfd driver for usart
>> MAINTAINERS: add at91 usart spi driver
>> spi: at91-usart: add driver for at91-usart as spi
>> tty/serial: atmel: change the driver to work under at91-usart mfd
>
--
Nicolas Ferre
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-10 9:51 ` Nicolas Ferre
@ 2018-09-10 13:43 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2018-09-10 13:43 UTC (permalink / raw)
To: Nicolas Ferre
Cc: Radu Pirea, robh+dt, mark.rutland, alexandre.belloni, gregkh,
broonie, jslaby, richard.genoud, davem, mchehab+samsung, akpm,
arnd, devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi
On Mon, 10 Sep 2018, Nicolas Ferre wrote:
> Hi Lee,
>
> On 10/09/2018 at 11:48, Lee Jones wrote:
> > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > Well, this is the 12th version of this patch series.
> > > In this version I fixed a warning from kbuild-robot and I have no idea
> > > how I forgot to add static in declaration of that functions. Also I
> > > fixed the example for the SPI driver in bindings.
> >
> > Okay great. So which patches still require Acks?
>
> None I would say: everything's ready to be integrated into your MFD tree as
> you proposed some time ago.
>
> We are looking forward seeing this series integrated in linux-next as soon
> as possible.
Very well.
I'm just catching up after an extended vacation - only 300 mails to go!
Please bear with me.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
` (6 preceding siblings ...)
2018-09-10 9:48 ` [PATCH v12 0/6] Driver for at91 usart in spi mode Lee Jones
@ 2018-09-11 9:33 ` Lee Jones
2018-09-11 9:39 ` Alexandre Belloni
2018-09-11 11:18 ` [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window Lee Jones
8 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-11 9:33 UTC (permalink / raw)
To: Radu Pirea
Cc: robh+dt, mark.rutland, nicolas.ferre, alexandre.belloni, gregkh,
broonie, jslaby, richard.genoud, davem, mchehab+samsung, akpm,
arnd, devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi
On Tue, 04 Sep 2018, Radu Pirea wrote:
> Radu Pirea (6):
> MAINTAINERS: add at91 usart mfd driver
> dt-bindings: add binding for atmel-usart in SPI mode
> mfd: at91-usart: added mfd driver for usart
> MAINTAINERS: add at91 usart spi driver
> spi: at91-usart: add driver for at91-usart as spi
> tty/serial: atmel: change the driver to work under at91-usart mfd
>
> .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> MAINTAINERS | 16 +
> drivers/mfd/Kconfig | 9 +
> drivers/mfd/Makefile | 1 +
> drivers/mfd/at91-usart.c | 71 +++
> drivers/spi/Kconfig | 8 +
> drivers/spi/Makefile | 1 +
> drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> drivers/tty/serial/Kconfig | 1 +
> drivers/tty/serial/atmel_serial.c | 42 +-
> include/dt-bindings/mfd/at91-usart.h | 17 +
> 11 files changed, 606 insertions(+), 17 deletions(-)
> rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> create mode 100644 drivers/mfd/at91-usart.c
> create mode 100644 drivers/spi/spi-at91-usart.c
> create mode 100644 include/dt-bindings/mfd/at91-usart.h
Seeing as this patch-set has caused some issues this morning, I took
the liberty to peruse back into its history to figure out where things
started to go wrong. I also re-reviewed the MFD driver - and I'm glad
I did!
My Acked-by has been attached to the MFD portion since v5, which is
why the code hasn't caught my eye before today. I reviewed the
relocation of the *binding document* (serial => mfd with no changes)
in v4 and nothing else. It appears as though you mistakenly added it
to the *MFD driver* instead. This explains my confusion in v10 when I
told you I'd already reviewed the binding document.
As I said, I have re-reviewed the MFD driver and I'm afraid to say
that I do not like what I see. Besides the missing header file and
the whitespace tabbing errors, I do not agree with the implementation.
Using MFD as a shim to hack around driver selection is not a valid
use-case.
What's stopping you from just using the compatible string directly to
select which driver you need to probe?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 9:33 ` Lee Jones
@ 2018-09-11 9:39 ` Alexandre Belloni
2018-09-11 14:59 ` Geert Uytterhoeven
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-11 9:39 UTC (permalink / raw)
To: Lee Jones
Cc: Radu Pirea, robh+dt, mark.rutland, nicolas.ferre, gregkh,
broonie, jslaby, richard.genoud, davem, mchehab+samsung, akpm,
arnd, devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi
On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> On Tue, 04 Sep 2018, Radu Pirea wrote:
> > Radu Pirea (6):
> > MAINTAINERS: add at91 usart mfd driver
> > dt-bindings: add binding for atmel-usart in SPI mode
> > mfd: at91-usart: added mfd driver for usart
> > MAINTAINERS: add at91 usart spi driver
> > spi: at91-usart: add driver for at91-usart as spi
> > tty/serial: atmel: change the driver to work under at91-usart mfd
> >
> > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > MAINTAINERS | 16 +
> > drivers/mfd/Kconfig | 9 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/at91-usart.c | 71 +++
> > drivers/spi/Kconfig | 8 +
> > drivers/spi/Makefile | 1 +
> > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > drivers/tty/serial/Kconfig | 1 +
> > drivers/tty/serial/atmel_serial.c | 42 +-
> > include/dt-bindings/mfd/at91-usart.h | 17 +
> > 11 files changed, 606 insertions(+), 17 deletions(-)
> > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > create mode 100644 drivers/mfd/at91-usart.c
> > create mode 100644 drivers/spi/spi-at91-usart.c
> > create mode 100644 include/dt-bindings/mfd/at91-usart.h
>
> Seeing as this patch-set has caused some issues this morning, I took
> the liberty to peruse back into its history to figure out where things
> started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> I did!
>
> My Acked-by has been attached to the MFD portion since v5, which is
> why the code hasn't caught my eye before today. I reviewed the
> relocation of the *binding document* (serial => mfd with no changes)
> in v4 and nothing else. It appears as though you mistakenly added it
> to the *MFD driver* instead. This explains my confusion in v10 when I
> told you I'd already reviewed the binding document.
>
> As I said, I have re-reviewed the MFD driver and I'm afraid to say
> that I do not like what I see. Besides the missing header file and
> the whitespace tabbing errors, I do not agree with the implementation.
> Using MFD as a shim to hack around driver selection is not a valid
> use-case.
>
> What's stopping you from just using the compatible string directly to
> select which driver you need to probe?
>
Then you'd have multiple compatible strings for the same IP which is a
big no-no.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 9:39 ` Alexandre Belloni
@ 2018-09-11 14:59 ` Geert Uytterhoeven
2018-09-11 15:36 ` Alexandre Belloni
0 siblings, 1 reply; 38+ messages in thread
From: Geert Uytterhoeven @ 2018-09-11 14:59 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Lee Jones, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
Hi Alexandre,
On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > Radu Pirea (6):
> > > MAINTAINERS: add at91 usart mfd driver
> > > dt-bindings: add binding for atmel-usart in SPI mode
> > > mfd: at91-usart: added mfd driver for usart
> > > MAINTAINERS: add at91 usart spi driver
> > > spi: at91-usart: add driver for at91-usart as spi
> > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > >
> > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > MAINTAINERS | 16 +
> > > drivers/mfd/Kconfig | 9 +
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/at91-usart.c | 71 +++
> > > drivers/spi/Kconfig | 8 +
> > > drivers/spi/Makefile | 1 +
> > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > drivers/tty/serial/Kconfig | 1 +
> > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > create mode 100644 drivers/mfd/at91-usart.c
> > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> >
> > Seeing as this patch-set has caused some issues this morning, I took
> > the liberty to peruse back into its history to figure out where things
> > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > I did!
> >
> > My Acked-by has been attached to the MFD portion since v5, which is
> > why the code hasn't caught my eye before today. I reviewed the
> > relocation of the *binding document* (serial => mfd with no changes)
> > in v4 and nothing else. It appears as though you mistakenly added it
> > to the *MFD driver* instead. This explains my confusion in v10 when I
> > told you I'd already reviewed the binding document.
> >
> > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > that I do not like what I see. Besides the missing header file and
> > the whitespace tabbing errors, I do not agree with the implementation.
> > Using MFD as a shim to hack around driver selection is not a valid
> > use-case.
> >
> > What's stopping you from just using the compatible string directly to
> > select which driver you need to probe?
> >
>
> Then you'd have multiple compatible strings for the same IP which is a
> big no-no.
It's still the same hardware device, isn't?
What if the SPI or UART slave is not on-board, but on an expansion board?
Then the SoC-specific .dtsi has no idea what mode should be used.
Hence shouldn't the software derive the hardware mode from the full
hardware description in DT? If that's impossible (I didn't look into detail
whether an SPI bus can easily be distinguished from a UART bus), perhaps
a mode property should be added?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 14:59 ` Geert Uytterhoeven
@ 2018-09-11 15:36 ` Alexandre Belloni
2018-09-11 16:23 ` Geert Uytterhoeven
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-11 15:36 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Lee Jones, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> Hi Alexandre,
>
> On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > Radu Pirea (6):
> > > > MAINTAINERS: add at91 usart mfd driver
> > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > mfd: at91-usart: added mfd driver for usart
> > > > MAINTAINERS: add at91 usart spi driver
> > > > spi: at91-usart: add driver for at91-usart as spi
> > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > >
> > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > MAINTAINERS | 16 +
> > > > drivers/mfd/Kconfig | 9 +
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/at91-usart.c | 71 +++
> > > > drivers/spi/Kconfig | 8 +
> > > > drivers/spi/Makefile | 1 +
> > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > drivers/tty/serial/Kconfig | 1 +
> > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > >
> > > Seeing as this patch-set has caused some issues this morning, I took
> > > the liberty to peruse back into its history to figure out where things
> > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > I did!
> > >
> > > My Acked-by has been attached to the MFD portion since v5, which is
> > > why the code hasn't caught my eye before today. I reviewed the
> > > relocation of the *binding document* (serial => mfd with no changes)
> > > in v4 and nothing else. It appears as though you mistakenly added it
> > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > told you I'd already reviewed the binding document.
> > >
> > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > that I do not like what I see. Besides the missing header file and
> > > the whitespace tabbing errors, I do not agree with the implementation.
> > > Using MFD as a shim to hack around driver selection is not a valid
> > > use-case.
> > >
> > > What's stopping you from just using the compatible string directly to
> > > select which driver you need to probe?
> > >
> >
> > Then you'd have multiple compatible strings for the same IP which is a
> > big no-no.
>
> It's still the same hardware device, isn't?
> What if the SPI or UART slave is not on-board, but on an expansion board?
> Then the SoC-specific .dtsi has no idea what mode should be used.
>
> Hence shouldn't the software derive the hardware mode from the full
> hardware description in DT? If that's impossible (I didn't look into detail
> whether an SPI bus can easily be distinguished from a UART bus), perhaps
> a mode property should be added?
>
Yes, this is exactly what is done:
https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
Only one compatbile for the IP and a property to know what is the mode.
That property should indeed be set in the board dts and not the SoC
dtsi.
the other, less robust alternative was to look for child nodes and
decide that if some where present it would indicate an SPI bus. But I
think at some point we may have child nodes under a UART node.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 15:36 ` Alexandre Belloni
@ 2018-09-11 16:23 ` Geert Uytterhoeven
2018-09-11 18:39 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Geert Uytterhoeven @ 2018-09-11 16:23 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Lee Jones, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > Radu Pirea (6):
> > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > mfd: at91-usart: added mfd driver for usart
> > > > > MAINTAINERS: add at91 usart spi driver
> > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > >
> > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > MAINTAINERS | 16 +
> > > > > drivers/mfd/Kconfig | 9 +
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > drivers/spi/Kconfig | 8 +
> > > > > drivers/spi/Makefile | 1 +
> > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > >
> > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > the liberty to peruse back into its history to figure out where things
> > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > I did!
> > > >
> > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > why the code hasn't caught my eye before today. I reviewed the
> > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > told you I'd already reviewed the binding document.
> > > >
> > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > that I do not like what I see. Besides the missing header file and
> > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > use-case.
> > > >
> > > > What's stopping you from just using the compatible string directly to
> > > > select which driver you need to probe?
> > > >
> > >
> > > Then you'd have multiple compatible strings for the same IP which is a
> > > big no-no.
> >
> > It's still the same hardware device, isn't?
> > What if the SPI or UART slave is not on-board, but on an expansion board?
> > Then the SoC-specific .dtsi has no idea what mode should be used.
> >
> > Hence shouldn't the software derive the hardware mode from the full
> > hardware description in DT? If that's impossible (I didn't look into detail
> > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > a mode property should be added?
>
> Yes, this is exactly what is done:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
OK.
I guess the main "hackish" part is that the mfd_cell uses of_compatible,
which thus requires having additional compatible values?
I think those can just be removed. AFAICS, the SPI and serial drivers already
match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
names?
> Only one compatbile for the IP and a property to know what is the mode.
> That property should indeed be set in the board dts and not the SoC
> dtsi.
>
> the other, less robust alternative was to look for child nodes and
> decide that if some where present it would indicate an SPI bus. But I
> think at some point we may have child nodes under a UART node.
Indeed, using the "new" serial bus.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 16:23 ` Geert Uytterhoeven
@ 2018-09-11 18:39 ` Lee Jones
2018-09-11 18:58 ` Alexandre Belloni
2018-09-11 18:59 ` Geert Uytterhoeven
0 siblings, 2 replies; 38+ messages in thread
From: Lee Jones @ 2018-09-11 18:39 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > Radu Pirea (6):
> > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > >
> > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > MAINTAINERS | 16 +
> > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > drivers/mfd/Makefile | 1 +
> > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > drivers/spi/Kconfig | 8 +
> > > > > > drivers/spi/Makefile | 1 +
> > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > >
> > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > the liberty to peruse back into its history to figure out where things
> > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > I did!
> > > > >
> > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > told you I'd already reviewed the binding document.
> > > > >
> > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > that I do not like what I see. Besides the missing header file and
> > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > use-case.
> > > > >
> > > > > What's stopping you from just using the compatible string directly to
> > > > > select which driver you need to probe?
> > > > >
> > > >
> > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > big no-no.
> > >
> > > It's still the same hardware device, isn't?
> > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > >
> > > Hence shouldn't the software derive the hardware mode from the full
> > > hardware description in DT? If that's impossible (I didn't look into detail
> > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > a mode property should be added?
> >
> > Yes, this is exactly what is done:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
>
> OK.
>
> I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> which thus requires having additional compatible values?
>
> I think those can just be removed. AFAICS, the SPI and serial drivers already
> match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> names?
The hackish part of this driver is that it's using MFD for something
which is clearly not an MFD. It's a USART device. Nothing more,
nothing less.
Does anyone have the datasheet to hand?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 18:39 ` Lee Jones
@ 2018-09-11 18:58 ` Alexandre Belloni
2018-09-11 19:04 ` Geert Uytterhoeven
2018-09-11 22:44 ` Lee Jones
2018-09-11 18:59 ` Geert Uytterhoeven
1 sibling, 2 replies; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-11 18:58 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
>
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > >
> > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > MAINTAINERS | 16 +
> > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> >
> > OK.
> >
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> >
> > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > names?
>
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD. It's a USART device. Nothing more,
> nothing less.
>
> Does anyone have the datasheet to hand?
>
It is not a simple usart, it is either a usart or a full blown SPI
controller with registers changing layout depending on the selected
mode. Otherwise, I'm not sure how you would get a USART to do SPI.
Datasheet here:
http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
USART doc starting p572, registers p621.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 18:58 ` Alexandre Belloni
@ 2018-09-11 19:04 ` Geert Uytterhoeven
2018-09-11 22:44 ` Lee Jones
1 sibling, 0 replies; 38+ messages in thread
From: Geert Uytterhoeven @ 2018-09-11 19:04 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Lee Jones, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
Hi Alexandre,
On Tue, Sep 11, 2018 at 8:58 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> >
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD. It's a USART device. Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
>
> It is not a simple usart, it is either a usart or a full blown SPI
> controller with registers changing layout depending on the selected
> mode. Otherwise, I'm not sure how you would get a USART to do SPI.
Note the "S" in USART. SPI is just synchronous serial with a shared clock
for transmit and receive. So the hardware is not that unrelated.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 18:58 ` Alexandre Belloni
2018-09-11 19:04 ` Geert Uytterhoeven
@ 2018-09-11 22:44 ` Lee Jones
2018-09-11 22:54 ` Lee Jones
1 sibling, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-11 22:44 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Tue, 11 Sep 2018, Alexandre Belloni wrote:
> On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> >
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > > >
> > > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > > MAINTAINERS | 16 +
> > > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD. It's a USART device. Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
> >
>
> It is not a simple usart, it is either a usart or a full blown SPI
> controller with registers changing layout depending on the selected
> mode. Otherwise, I'm not sure how you would get a USART to do SPI.
Make up your mind. Either the IP is different, or it's not. ;)
> Datasheet here:
Great. Thank you.
> http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
>
> USART doc starting p572, registers p621.
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 22:44 ` Lee Jones
@ 2018-09-11 22:54 ` Lee Jones
2018-09-12 7:33 ` Alexandre Belloni
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-11 22:54 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Tue, 11 Sep 2018, Lee Jones wrote:
> On Tue, 11 Sep 2018, Alexandre Belloni wrote:
>
> > On 11/09/2018 19:39:30+0100, Lee Jones wrote:
> > > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > >
> > > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > > Radu Pirea (6):
> > > > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > > > >
> > > > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > > > MAINTAINERS | 16 +
> > > > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > > >
> > > > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > > > I did!
> > > > > > > >
> > > > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > > > told you I'd already reviewed the binding document.
> > > > > > > >
> > > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > > use-case.
> > > > > > > >
> > > > > > > > What's stopping you from just using the compatible string directly to
> > > > > > > > select which driver you need to probe?
> > > > > > > >
> > > > > > >
> > > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > > big no-no.
> > > > > >
> > > > > > It's still the same hardware device, isn't?
> > > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > > >
> > > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > > a mode property should be added?
> > > > >
> > > > > Yes, this is exactly what is done:
> > > > >
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > > >
> > > > OK.
> > > >
> > > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > > which thus requires having additional compatible values?
> > > >
> > > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > > names?
> > >
> > > The hackish part of this driver is that it's using MFD for something
> > > which is clearly not an MFD. It's a USART device. Nothing more,
> > > nothing less.
> > >
> > > Does anyone have the datasheet to hand?
> > >
> >
> > It is not a simple usart, it is either a usart or a full blown SPI
> > controller with registers changing layout depending on the selected
> > mode. Otherwise, I'm not sure how you would get a USART to do SPI.
>
> Make up your mind. Either the IP is different, or it's not. ;)
>
> > Datasheet here:
>
> Great. Thank you.
>
> > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> >
> > USART doc starting p572, registers p621.
After looking at the datasheet, I don't see any reason why one of the
two drivers can't be selected using different compatible strings.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 22:54 ` Lee Jones
@ 2018-09-12 7:33 ` Alexandre Belloni
2018-09-12 8:41 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-12 7:33 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > >
> > > USART doc starting p572, registers p621.
>
> After looking at the datasheet, I don't see any reason why one of the
> two drivers can't be selected using different compatible strings.
Because there is only one IP and we don't use the device tree to selecet
linux specific drivers.
If you are not happy having that in MFD, I guess we can move it out
somewhere else.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 7:33 ` Alexandre Belloni
@ 2018-09-12 8:41 ` Lee Jones
2018-09-12 9:43 ` Geert Uytterhoeven
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-12 8:41 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > >
> > > > USART doc starting p572, registers p621.
> >
> > After looking at the datasheet, I don't see any reason why one of the
> > two drivers can't be selected using different compatible strings.
>
> Because there is only one IP and we don't use the device tree to selecet
> linux specific drivers.
We do it all the time. There are loads of MFDs (def: same IP, with
different functions) which have separate compatibles for their various
functions. If you wish this IP to operate as an SPI controller, it
should have an SPI compatible, if you wish it to operate as a U(S)ART,
then it should have a UART compatible. It's what we do for most of
the other MFDs in the kernel.
> If you are not happy having that in MFD, I guess we can move it out
> somewhere else.
My issue isn't pertaining to where the hack lives, it's that there is
a hack in the first place.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 8:41 ` Lee Jones
@ 2018-09-12 9:43 ` Geert Uytterhoeven
2018-09-12 10:54 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Geert Uytterhoeven @ 2018-09-12 9:43 UTC (permalink / raw)
To: Lee Jones
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
Hi Lee,
On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > >
> > > > > USART doc starting p572, registers p621.
> > >
> > > After looking at the datasheet, I don't see any reason why one of the
> > > two drivers can't be selected using different compatible strings.
> >
> > Because there is only one IP and we don't use the device tree to selecet
> > linux specific drivers.
>
> We do it all the time. There are loads of MFDs (def: same IP, with
> different functions) which have separate compatibles for their various
> functions. If you wish this IP to operate as an SPI controller, it
> should have an SPI compatible, if you wish it to operate as a U(S)ART,
> then it should have a UART compatible. It's what we do for most of
> the other MFDs in the kernel.
There is a big difference: MFD functions are(more or less) independent
functions, which can be used at the same time. It makes perfect sense for a
single IP block that has both SPI and UART interfaces, that can be used at
the same time.
In this case, there is a single piece of hardware that can perform
different functions, but not at the same time. Performing a different
function means configuring the hardware for that function, hence using a
different driver (from a different subsystem).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 9:43 ` Geert Uytterhoeven
@ 2018-09-12 10:54 ` Lee Jones
2018-09-12 11:17 ` Alexandre Belloni
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-12 10:54 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > >
> > > > > > USART doc starting p572, registers p621.
> > > >
> > > > After looking at the datasheet, I don't see any reason why one of the
> > > > two drivers can't be selected using different compatible strings.
> > >
> > > Because there is only one IP and we don't use the device tree to selecet
> > > linux specific drivers.
> >
> > We do it all the time. There are loads of MFDs (def: same IP, with
> > different functions) which have separate compatibles for their various
> > functions. If you wish this IP to operate as an SPI controller, it
> > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > then it should have a UART compatible. It's what we do for most of
> > the other MFDs in the kernel.
>
> There is a big difference: MFD functions are(more or less) independent
> functions, which can be used at the same time. It makes perfect sense for a
> single IP block that has both SPI and UART interfaces, that can be used at
> the same time.
>
> In this case, there is a single piece of hardware that can perform
> different functions, but not at the same time. Performing a different
> function means configuring the hardware for that function, hence using a
> different driver (from a different subsystem).
Yes, I can see that PoV.
But ... we can't have it both ways. *Either* it's a true MFD, in
which case it can/should have 2 separate compatible strings which can
be specified directly from the DT. *Or* it's not an MFD. In the
latter case, which I think we're all agreeing on (else we'd have 2
compatible strings), MFD is not the place to handle this (my original
point).
So ... this is a USART device which can do SPI, right?
My current thinking is that; as this is a USART device first &
foremost, the USART should be probed in the first instance regardless,
then if SPI mode is specified it (the USART driver) registers the SPI
platform driver (as MFD does currently) and exits gracefully, allowing
the SPI driver to take over.
Spanner in the works: is it physically possible to change the mode at
run-time? :s
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 10:54 ` Lee Jones
@ 2018-09-12 11:17 ` Alexandre Belloni
2018-09-12 11:43 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-12 11:17 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > >
> > > > > > > USART doc starting p572, registers p621.
> > > > >
> > > > > After looking at the datasheet, I don't see any reason why one of the
> > > > > two drivers can't be selected using different compatible strings.
> > > >
> > > > Because there is only one IP and we don't use the device tree to selecet
> > > > linux specific drivers.
> > >
> > > We do it all the time. There are loads of MFDs (def: same IP, with
> > > different functions) which have separate compatibles for their various
> > > functions. If you wish this IP to operate as an SPI controller, it
> > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > then it should have a UART compatible. It's what we do for most of
> > > the other MFDs in the kernel.
> >
> > There is a big difference: MFD functions are(more or less) independent
> > functions, which can be used at the same time. It makes perfect sense for a
> > single IP block that has both SPI and UART interfaces, that can be used at
> > the same time.
> >
> > In this case, there is a single piece of hardware that can perform
> > different functions, but not at the same time. Performing a different
> > function means configuring the hardware for that function, hence using a
> > different driver (from a different subsystem).
>
> Yes, I can see that PoV.
>
> But ... we can't have it both ways. *Either* it's a true MFD, in
> which case it can/should have 2 separate compatible strings which can
> be specified directly from the DT. *Or* it's not an MFD. In the
> latter case, which I think we're all agreeing on (else we'd have 2
> compatible strings), MFD is not the place to handle this (my original
> point).
>
If that is what bothers you, then let's move it out of mfd.
> So ... this is a USART device which can do SPI, right?
>
> My current thinking is that; as this is a USART device first &
> foremost, the USART should be probed in the first instance regardless,
> then if SPI mode is specified it (the USART driver) registers the SPI
> platform driver (as MFD does currently) and exits gracefully, allowing
> the SPI driver to take over.
>
> Spanner in the works: is it physically possible to change the mode at
> run-time? :s
Yes it is possible but on Linux that will not happen without probing
the drivers again. I think DT overlays will be the only possible use
case because on SPI, you'd still have to provide nodes for the connected
SPI devices.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 11:17 ` Alexandre Belloni
@ 2018-09-12 11:43 ` Lee Jones
2018-09-12 12:14 ` Alexandre Belloni
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-12 11:43 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 12/09/2018 11:54:07+0100, Lee Jones wrote:
> > On Wed, 12 Sep 2018, Geert Uytterhoeven wrote:
> > > On Wed, Sep 12, 2018 at 10:41 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> > > > > On 11/09/2018 23:54:40+0100, Lee Jones wrote:
> > > > > > > > http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-6438-32-bit-ARM926-Embedded-Microprocessor-SAM9G45_Datasheet.pdf
> > > > > > > >
> > > > > > > > USART doc starting p572, registers p621.
> > > > > >
> > > > > > After looking at the datasheet, I don't see any reason why one of the
> > > > > > two drivers can't be selected using different compatible strings.
> > > > >
> > > > > Because there is only one IP and we don't use the device tree to selecet
> > > > > linux specific drivers.
> > > >
> > > > We do it all the time. There are loads of MFDs (def: same IP, with
> > > > different functions) which have separate compatibles for their various
> > > > functions. If you wish this IP to operate as an SPI controller, it
> > > > should have an SPI compatible, if you wish it to operate as a U(S)ART,
> > > > then it should have a UART compatible. It's what we do for most of
> > > > the other MFDs in the kernel.
> > >
> > > There is a big difference: MFD functions are(more or less) independent
> > > functions, which can be used at the same time. It makes perfect sense for a
> > > single IP block that has both SPI and UART interfaces, that can be used at
> > > the same time.
> > >
> > > In this case, there is a single piece of hardware that can perform
> > > different functions, but not at the same time. Performing a different
> > > function means configuring the hardware for that function, hence using a
> > > different driver (from a different subsystem).
> >
> > Yes, I can see that PoV.
> >
> > But ... we can't have it both ways. *Either* it's a true MFD, in
> > which case it can/should have 2 separate compatible strings which can
> > be specified directly from the DT. *Or* it's not an MFD. In the
> > latter case, which I think we're all agreeing on (else we'd have 2
> > compatible strings), MFD is not the place to handle this (my original
> > point).
> >
>
> If that is what bothers you, then let's move it out of mfd.
As I've already mentioned. I don't just want it moved out of MFD and
shoved somewhere else. My aim is to fix this properly.
> > So ... this is a USART device which can do SPI, right?
> >
> > My current thinking is that; as this is a USART device first &
> > foremost, the USART should be probed in the first instance regardless,
> > then if SPI mode is specified it (the USART driver) registers the SPI
> > platform driver (as MFD does currently) and exits gracefully, allowing
> > the SPI driver to take over.
> >
> > Spanner in the works: is it physically possible to change the mode at
> > run-time? :s
>
> Yes it is possible but on Linux that will not happen without probing
> the drivers again.
Not sure I understand what you mean.
I'm suggesting that you use the same platform_* interfaces MFD uses to
register the SPI driver if SPI mode has been selected. Only do so
from the appropriate driver i.e. USART.
> I think DT overlays will be the only possible use
> case because on SPI, you'd still have to provide nodes for the connected
> SPI devices.
Since SPI is a function of the USART you should describe is as such
via a child node.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 11:43 ` Lee Jones
@ 2018-09-12 12:14 ` Alexandre Belloni
2018-09-12 13:12 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-12 12:14 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > But ... we can't have it both ways. *Either* it's a true MFD, in
> > > which case it can/should have 2 separate compatible strings which can
> > > be specified directly from the DT. *Or* it's not an MFD. In the
> > > latter case, which I think we're all agreeing on (else we'd have 2
> > > compatible strings), MFD is not the place to handle this (my original
> > > point).
> > >
> >
> > If that is what bothers you, then let's move it out of mfd.
>
> As I've already mentioned. I don't just want it moved out of MFD and
> shoved somewhere else. My aim is to fix this properly.
>
If it is out of MFD, then I'm not sure why you would care too much about
it as you won't be maintaining that code. And I still this what was done
was correct but I'm open to test what you suggest.
> > > So ... this is a USART device which can do SPI, right?
> > >
> > > My current thinking is that; as this is a USART device first &
> > > foremost, the USART should be probed in the first instance regardless,
> > > then if SPI mode is specified it (the USART driver) registers the SPI
> > > platform driver (as MFD does currently) and exits gracefully, allowing
> > > the SPI driver to take over.
> > >
> > > Spanner in the works: is it physically possible to change the mode at
> > > run-time? :s
> >
> > Yes it is possible but on Linux that will not happen without probing
> > the drivers again.
>
> Not sure I understand what you mean.
>
I was just commenting on changing the mode at runtime.
> I'm suggesting that you use the same platform_* interfaces MFD uses to
> register the SPI driver if SPI mode has been selected. Only do so
> from the appropriate driver i.e. USART.
>
Yeah, I understood that but I didn't comment because I'm not sure this
will work yet.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 12:14 ` Alexandre Belloni
@ 2018-09-12 13:12 ` Lee Jones
2018-09-12 19:36 ` Radu Pirea
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-12 13:12 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > > But ... we can't have it both ways. *Either* it's a true MFD, in
> > > > which case it can/should have 2 separate compatible strings which can
> > > > be specified directly from the DT. *Or* it's not an MFD. In the
> > > > latter case, which I think we're all agreeing on (else we'd have 2
> > > > compatible strings), MFD is not the place to handle this (my original
> > > > point).
> > > >
> > >
> > > If that is what bothers you, then let's move it out of mfd.
> >
> > As I've already mentioned. I don't just want it moved out of MFD and
> > shoved somewhere else. My aim is to fix this properly.
> >
>
> If it is out of MFD, then I'm not sure why you would care too much about
> it as you won't be maintaining that code. And I still this what was done
> was correct but I'm open to test what you suggest.
I care for the kernel in general, not just the areas I'm responsible
for. I guess I'm just that kinda guy! ;)
> > > > So ... this is a USART device which can do SPI, right?
> > > >
> > > > My current thinking is that; as this is a USART device first &
> > > > foremost, the USART should be probed in the first instance regardless,
> > > > then if SPI mode is specified it (the USART driver) registers the SPI
> > > > platform driver (as MFD does currently) and exits gracefully, allowing
> > > > the SPI driver to take over.
> > > >
> > > > Spanner in the works: is it physically possible to change the mode at
> > > > run-time? :s
> > >
> > > Yes it is possible but on Linux that will not happen without probing
> > > the drivers again.
> >
> > Not sure I understand what you mean.
>
> I was just commenting on changing the mode at runtime.
Oh I see. My question was relating to whether the H/W is physically
capable of changing modes on-the-fly, rather than how Linux would
handle that. If this is something we'd wish to support, then it would
have to be a single driver, which is why I was asking. By separating
the drivers this way, we are blocking that as a possibility. Although
I guess the OP has already thought about that and made the decision
not to support it.
> > I'm suggesting that you use the same platform_* interfaces MFD uses to
> > register the SPI driver if SPI mode has been selected. Only do so
> > from the appropriate driver i.e. USART.
>
> Yeah, I understood that but I didn't comment because I'm not sure this
> will work yet.
Other drivers already do this.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 13:12 ` Lee Jones
@ 2018-09-12 19:36 ` Radu Pirea
2018-10-09 9:04 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Radu Pirea @ 2018-09-12 19:36 UTC (permalink / raw)
To: Lee Jones, Alexandre Belloni
Cc: Geert Uytterhoeven, Rob Herring, Mark Rutland, Nicolas Ferre,
Greg KH, Mark Brown, Jiri Slaby, Richard Genoud, David S. Miller,
Mauro Carvalho Chehab, Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 2018-09-12 at 14:12 +0100, Lee Jones wrote:
> On Wed, 12 Sep 2018, Alexandre Belloni wrote:
>
> > On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > > > But ... we can't have it both ways. *Either* it's a true
> > > > > MFD, in
> > > > > which case it can/should have 2 separate compatible strings
> > > > > which can
> > > > > be specified directly from the DT. *Or* it's not an MFD. In
> > > > > the
> > > > > latter case, which I think we're all agreeing on (else we'd
> > > > > have 2
> > > > > compatible strings), MFD is not the place to handle this (my
> > > > > original
> > > > > point).
> > > > >
> > > >
> > > > If that is what bothers you, then let's move it out of mfd.
> > >
> > > As I've already mentioned. I don't just want it moved out of MFD
> > > and
> > > shoved somewhere else. My aim is to fix this properly.
> > >
> >
> > If it is out of MFD, then I'm not sure why you would care too much
> > about
> > it as you won't be maintaining that code. And I still this what was
> > done
> > was correct but I'm open to test what you suggest.
>
> I care for the kernel in general, not just the areas I'm responsible
> for. I guess I'm just that kinda guy! ;)
Well, Lee, like you, I think this driver should not be a MFD driver,
but Alex has a good point of view.
>
> > > > > So ... this is a USART device which can do SPI, right?
> > > > >
> > > > > My current thinking is that; as this is a USART device first
> > > > > &
> > > > > foremost, the USART should be probed in the first instance
> > > > > regardless,
> > > > > then if SPI mode is specified it (the USART driver) registers
> > > > > the SPI
> > > > > platform driver (as MFD does currently) and exits gracefully,
> > > > > allowing
> > > > > the SPI driver to take over.
> > > > >
> > > > > Spanner in the works: is it physically possible to change the
> > > > > mode at
> > > > > run-time? :s
> > > >
> > > > Yes it is possible but on Linux that will not happen without
> > > > probing
> > > > the drivers again.
> > >
> > > Not sure I understand what you mean.
> >
> > I was just commenting on changing the mode at runtime.
>
> Oh I see. My question was relating to whether the H/W is physically
> capable of changing modes on-the-fly, rather than how Linux would
> handle that. If this is something we'd wish to support, then it
> would
> have to be a single driver, which is why I was asking. By separating
> the drivers this way, we are blocking that as a
> possibility. Although
> I guess the OP has already thought about that and made the decision
> not to support it.
Is possible to change modes on-the-fly, but you have no reason to do
that. On the PCB you will have a SPI slave or a serial console :)
Anyway, the current form of the driver, and through this I want to say
"this ugly hack", allows the user to switch from serial to SPI mode by
adding only one property to the device tree node of USART. If the
driver were in his first form, a simple SPI driver, how you will make a
dtsi file for an IP like this? You will add two nodes for the same IP
in dtsi and will take care to enable correct node in dts?
I think this driver is only a tradeoff between having an ugly hack in
kernel or having an messy device tree.
>
> > > I'm suggesting that you use the same platform_* interfaces MFD
> > > uses to
> > > register the SPI driver if SPI mode has been selected. Only do
> > > so
> > > from the appropriate driver i.e. USART.
> >
> > Yeah, I understood that but I didn't comment because I'm not sure
> > this
> > will work yet.
>
> Other drivers already do this.
Can you give me an example please?
I am open to suggestions.
Sorry for that acked-by. There was a lot of "reviewed-by", "acked-by",
etc in a single version and I messed up :).
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 19:36 ` Radu Pirea
@ 2018-10-09 9:04 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2018-10-09 9:04 UTC (permalink / raw)
To: Radu Pirea
Cc: Alexandre Belloni, Geert Uytterhoeven, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 12 Sep 2018, Radu Pirea wrote:
> On Wed, 2018-09-12 at 14:12 +0100, Lee Jones wrote:
> > On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> >
> > > On 12/09/2018 12:43:52+0100, Lee Jones wrote:
> > > > > > But ... we can't have it both ways. *Either* it's a true
> > > > > > MFD, in
> > > > > > which case it can/should have 2 separate compatible strings
> > > > > > which can
> > > > > > be specified directly from the DT. *Or* it's not an MFD. In
> > > > > > the
> > > > > > latter case, which I think we're all agreeing on (else we'd
> > > > > > have 2
> > > > > > compatible strings), MFD is not the place to handle this (my
> > > > > > original
> > > > > > point).
> > > > > >
> > > > >
> > > > > If that is what bothers you, then let's move it out of mfd.
> > > >
> > > > As I've already mentioned. I don't just want it moved out of MFD
> > > > and
> > > > shoved somewhere else. My aim is to fix this properly.
> > > >
> > >
> > > If it is out of MFD, then I'm not sure why you would care too much
> > > about
> > > it as you won't be maintaining that code. And I still this what was
> > > done
> > > was correct but I'm open to test what you suggest.
> >
> > I care for the kernel in general, not just the areas I'm responsible
> > for. I guess I'm just that kinda guy! ;)
>
> Well, Lee, like you, I think this driver should not be a MFD driver,
> but Alex has a good point of view.
>
> >
> > > > > > So ... this is a USART device which can do SPI, right?
> > > > > >
> > > > > > My current thinking is that; as this is a USART device first
> > > > > > &
> > > > > > foremost, the USART should be probed in the first instance
> > > > > > regardless,
> > > > > > then if SPI mode is specified it (the USART driver) registers
> > > > > > the SPI
> > > > > > platform driver (as MFD does currently) and exits gracefully,
> > > > > > allowing
> > > > > > the SPI driver to take over.
> > > > > >
> > > > > > Spanner in the works: is it physically possible to change the
> > > > > > mode at
> > > > > > run-time? :s
> > > > >
> > > > > Yes it is possible but on Linux that will not happen without
> > > > > probing
> > > > > the drivers again.
> > > >
> > > > Not sure I understand what you mean.
> > >
> > > I was just commenting on changing the mode at runtime.
> >
> > Oh I see. My question was relating to whether the H/W is physically
> > capable of changing modes on-the-fly, rather than how Linux would
> > handle that. If this is something we'd wish to support, then it
> > would
> > have to be a single driver, which is why I was asking. By separating
> > the drivers this way, we are blocking that as a
> > possibility. Although
> > I guess the OP has already thought about that and made the decision
> > not to support it.
>
> Is possible to change modes on-the-fly, but you have no reason to do
> that. On the PCB you will have a SPI slave or a serial console :)
> Anyway, the current form of the driver, and through this I want to say
> "this ugly hack", allows the user to switch from serial to SPI mode by
> adding only one property to the device tree node of USART. If the
> driver were in his first form, a simple SPI driver, how you will make a
> dtsi file for an IP like this? You will add two nodes for the same IP
> in dtsi and will take care to enable correct node in dts?
> I think this driver is only a tradeoff between having an ugly hack in
> kernel or having an messy device tree.
>
> >
> > > > I'm suggesting that you use the same platform_* interfaces MFD
> > > > uses to
> > > > register the SPI driver if SPI mode has been selected. Only do
> > > > so
> > > > from the appropriate driver i.e. USART.
> > >
> > > Yeah, I understood that but I didn't comment because I'm not sure
> > > this
> > > will work yet.
> >
> > Other drivers already do this.
>
> Can you give me an example please?
Sorry for the delay, I have been on vacation.
Grep for 'platform_device_add' in drivers/
> I am open to suggestions.
>
> Sorry for that acked-by. There was a lot of "reviewed-by", "acked-by",
> etc in a single version and I messed up :).
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 18:39 ` Lee Jones
2018-09-11 18:58 ` Alexandre Belloni
@ 2018-09-11 18:59 ` Geert Uytterhoeven
2018-09-11 22:43 ` Lee Jones
1 sibling, 1 reply; 38+ messages in thread
From: Geert Uytterhoeven @ 2018-09-11 18:59 UTC (permalink / raw)
To: Lee Jones
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
Hi Lee,
On Tue, Sep 11, 2018 at 8:40 PM Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > Radu Pirea (6):
> > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > >
> > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > MAINTAINERS | 16 +
> > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > >
> > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > I did!
> > > > > >
> > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > told you I'd already reviewed the binding document.
> > > > > >
> > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > use-case.
> > > > > >
> > > > > > What's stopping you from just using the compatible string directly to
> > > > > > select which driver you need to probe?
> > > > > >
> > > > >
> > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > big no-no.
> > > >
> > > > It's still the same hardware device, isn't?
> > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > >
> > > > Hence shouldn't the software derive the hardware mode from the full
> > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > a mode property should be added?
> > >
> > > Yes, this is exactly what is done:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> >
> > OK.
> >
> > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > which thus requires having additional compatible values?
> >
> > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > names?
>
> The hackish part of this driver is that it's using MFD for something
> which is clearly not an MFD. It's a USART device. Nothing more,
> nothing less.
>
> Does anyone have the datasheet to hand?
I haven't read it, but I believe it's not unlike Renesas SCIF, which is
served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
But the latter is not used from DT, so we haven't experienced (and solved)
the similar issue yet.
Would it work if the UART driver and SPI driver would match against the
same compatible value, but the UART driver would do in its probe()
function:
device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
if (opmode != AT91_USART_MODE_SERIAL)
return ENODEV;
while the SPI driver would do:
device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
if (opmode != AT91_USART_MODE_SPI)
return ENODEV;
? No MFD driver involved.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 18:59 ` Geert Uytterhoeven
@ 2018-09-11 22:43 ` Lee Jones
2018-09-12 7:30 ` Alexandre Belloni
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-11 22:43 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Alexandre Belloni, radu_nicolae.pirea, Rob Herring, Mark Rutland,
Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby, Richard Genoud,
David S. Miller, Mauro Carvalho Chehab, Andrew Morton,
Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> On Tue, Sep 11, 2018 at 8:40 PM Lee Jones <lee.jones@linaro.org> wrote:
> > On Tue, 11 Sep 2018, Geert Uytterhoeven wrote:
> > > On Tue, Sep 11, 2018 at 5:36 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:
> > > > On 11/09/2018 16:59:09+0200, Geert Uytterhoeven wrote:
> > > > > On Tue, Sep 11, 2018 at 11:40 AM Alexandre Belloni
> > > > > <alexandre.belloni@bootlin.com> wrote:
> > > > > > On 11/09/2018 10:33:56+0100, Lee Jones wrote:
> > > > > > > On Tue, 04 Sep 2018, Radu Pirea wrote:
> > > > > > > > Radu Pirea (6):
> > > > > > > > MAINTAINERS: add at91 usart mfd driver
> > > > > > > > dt-bindings: add binding for atmel-usart in SPI mode
> > > > > > > > mfd: at91-usart: added mfd driver for usart
> > > > > > > > MAINTAINERS: add at91 usart spi driver
> > > > > > > > spi: at91-usart: add driver for at91-usart as spi
> > > > > > > > tty/serial: atmel: change the driver to work under at91-usart mfd
> > > > > > > >
> > > > > > > > .../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
> > > > > > > > MAINTAINERS | 16 +
> > > > > > > > drivers/mfd/Kconfig | 9 +
> > > > > > > > drivers/mfd/Makefile | 1 +
> > > > > > > > drivers/mfd/at91-usart.c | 71 +++
> > > > > > > > drivers/spi/Kconfig | 8 +
> > > > > > > > drivers/spi/Makefile | 1 +
> > > > > > > > drivers/spi/spi-at91-usart.c | 432 ++++++++++++++++++
> > > > > > > > drivers/tty/serial/Kconfig | 1 +
> > > > > > > > drivers/tty/serial/atmel_serial.c | 42 +-
> > > > > > > > include/dt-bindings/mfd/at91-usart.h | 17 +
> > > > > > > > 11 files changed, 606 insertions(+), 17 deletions(-)
> > > > > > > > rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
> > > > > > > > create mode 100644 drivers/mfd/at91-usart.c
> > > > > > > > create mode 100644 drivers/spi/spi-at91-usart.c
> > > > > > > > create mode 100644 include/dt-bindings/mfd/at91-usart.h
> > > > > > >
> > > > > > > Seeing as this patch-set has caused some issues this morning, I took
> > > > > > > the liberty to peruse back into its history to figure out where things
> > > > > > > started to go wrong. I also re-reviewed the MFD driver - and I'm glad
> > > > > > > I did!
> > > > > > >
> > > > > > > My Acked-by has been attached to the MFD portion since v5, which is
> > > > > > > why the code hasn't caught my eye before today. I reviewed the
> > > > > > > relocation of the *binding document* (serial => mfd with no changes)
> > > > > > > in v4 and nothing else. It appears as though you mistakenly added it
> > > > > > > to the *MFD driver* instead. This explains my confusion in v10 when I
> > > > > > > told you I'd already reviewed the binding document.
> > > > > > >
> > > > > > > As I said, I have re-reviewed the MFD driver and I'm afraid to say
> > > > > > > that I do not like what I see. Besides the missing header file and
> > > > > > > the whitespace tabbing errors, I do not agree with the implementation.
> > > > > > > Using MFD as a shim to hack around driver selection is not a valid
> > > > > > > use-case.
> > > > > > >
> > > > > > > What's stopping you from just using the compatible string directly to
> > > > > > > select which driver you need to probe?
> > > > > > >
> > > > > >
> > > > > > Then you'd have multiple compatible strings for the same IP which is a
> > > > > > big no-no.
> > > > >
> > > > > It's still the same hardware device, isn't?
> > > > > What if the SPI or UART slave is not on-board, but on an expansion board?
> > > > > Then the SoC-specific .dtsi has no idea what mode should be used.
> > > > >
> > > > > Hence shouldn't the software derive the hardware mode from the full
> > > > > hardware description in DT? If that's impossible (I didn't look into detail
> > > > > whether an SPI bus can easily be distinguished from a UART bus), perhaps
> > > > > a mode property should be added?
> > > >
> > > > Yes, this is exactly what is done:
> > > >
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/tree/drivers/mfd/at91-usart.c?h=ib-mfd-spi-tty-4.20-1#n33
> > >
> > > OK.
> > >
> > > I guess the main "hackish" part is that the mfd_cell uses of_compatible,
> > > which thus requires having additional compatible values?
> > >
> > > I think those can just be removed. AFAICS, the SPI and serial drivers already
> > > match against the "at91_usart_spi" resp. "atmel_usart_serial" platform device
> > > names?
> >
> > The hackish part of this driver is that it's using MFD for something
> > which is clearly not an MFD. It's a USART device. Nothing more,
> > nothing less.
> >
> > Does anyone have the datasheet to hand?
>
> I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> But the latter is not used from DT, so we haven't experienced (and solved)
> the similar issue yet.
>
> Would it work if the UART driver and SPI driver would match against the
> same compatible value, but the UART driver would do in its probe()
> function:
>
> device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> if (opmode != AT91_USART_MODE_SERIAL)
> return ENODEV;
>
> while the SPI driver would do:
>
> device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> if (opmode != AT91_USART_MODE_SPI)
> return ENODEV;
>
> ? No MFD driver involved.
I haven't looked at the code in a while, but if memory serves I
believe platform code gives up once it has found its first match, so
by doing this, one of the drivers will never be matched/probed.
It's midnight here, so cracking out the datasheet isn't going to
happen just now, but it's my current belief that if the IP serves 2
very different modes of operation, even if the registers are in a
shared space, they could have their own compatible strings in DT.
That is what the MFD driver provides after all. Why would it be okay
to allocate different compatible strings from the MFD, but not in the
Device Tree?
It would be the easiest solution.
Has Rob commented on this yet?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-11 22:43 ` Lee Jones
@ 2018-09-12 7:30 ` Alexandre Belloni
2018-09-12 8:36 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Alexandre Belloni @ 2018-09-12 7:30 UTC (permalink / raw)
To: Lee Jones
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > But the latter is not used from DT, so we haven't experienced (and solved)
> > the similar issue yet.
> >
> > Would it work if the UART driver and SPI driver would match against the
> > same compatible value, but the UART driver would do in its probe()
> > function:
> >
> > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > if (opmode != AT91_USART_MODE_SERIAL)
> > return ENODEV;
> >
> > while the SPI driver would do:
> >
> > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > if (opmode != AT91_USART_MODE_SPI)
> > return ENODEV;
> >
> > ? No MFD driver involved.
>
> I haven't looked at the code in a while, but if memory serves I
> believe platform code gives up once it has found its first match, so
> by doing this, one of the drivers will never be matched/probed.
>
> It's midnight here, so cracking out the datasheet isn't going to
> happen just now, but it's my current belief that if the IP serves 2
> very different modes of operation, even if the registers are in a
> shared space, they could have their own compatible strings in DT.
>
> That is what the MFD driver provides after all. Why would it be okay
> to allocate different compatible strings from the MFD, but not in the
> Device Tree?
>
> It would be the easiest solution.
>
> Has Rob commented on this yet?
>
V4 of the bindings were acked by Rob and you:
https://patchwork.kernel.org/patch/10428087/
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH v12 0/6] Driver for at91 usart in spi mode
2018-09-12 7:30 ` Alexandre Belloni
@ 2018-09-12 8:36 ` Lee Jones
0 siblings, 0 replies; 38+ messages in thread
From: Lee Jones @ 2018-09-12 8:36 UTC (permalink / raw)
To: Alexandre Belloni
Cc: Geert Uytterhoeven, radu_nicolae.pirea, Rob Herring,
Mark Rutland, Nicolas Ferre, Greg KH, Mark Brown, Jiri Slaby,
Richard Genoud, David S. Miller, Mauro Carvalho Chehab,
Andrew Morton, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, Linux Kernel Mailing List, open list:SERIAL DRIVERS,
linux-spi
On Wed, 12 Sep 2018, Alexandre Belloni wrote:
> On 11/09/2018 23:43:02+0100, Lee Jones wrote:
> > > I haven't read it, but I believe it's not unlike Renesas SCIF, which is
> > > served by both drivers/tty/serial/sh-sci.c and drivers/spi/spi-sh-sci.c.
> > > But the latter is not used from DT, so we haven't experienced (and solved)
> > > the similar issue yet.
> > >
> > > Would it work if the UART driver and SPI driver would match against the
> > > same compatible value, but the UART driver would do in its probe()
> > > function:
> > >
> > > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > > if (opmode != AT91_USART_MODE_SERIAL)
> > > return ENODEV;
> > >
> > > while the SPI driver would do:
> > >
> > > device_property_read_u32(&pdev->dev, "atmel,usart-mode", &opmode);
> > > if (opmode != AT91_USART_MODE_SPI)
> > > return ENODEV;
> > >
> > > ? No MFD driver involved.
> >
> > I haven't looked at the code in a while, but if memory serves I
> > believe platform code gives up once it has found its first match, so
> > by doing this, one of the drivers will never be matched/probed.
> >
> > It's midnight here, so cracking out the datasheet isn't going to
> > happen just now, but it's my current belief that if the IP serves 2
> > very different modes of operation, even if the registers are in a
> > shared space, they could have their own compatible strings in DT.
> >
> > That is what the MFD driver provides after all. Why would it be okay
> > to allocate different compatible strings from the MFD, but not in the
> > Device Tree?
> >
> > It would be the easiest solution.
> >
> > Has Rob commented on this yet?
>
> V4 of the bindings were acked by Rob and you:
> https://patchwork.kernel.org/patch/10428087/
We didn't Ack the bindings. We Acked the location change.
I mean, has Rob specifically spoken out about using a compatible
string for each function.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window
2018-09-04 11:13 [PATCH v12 0/6] Driver for at91 usart in spi mode Radu Pirea
` (7 preceding siblings ...)
2018-09-11 9:33 ` Lee Jones
@ 2018-09-11 11:18 ` Lee Jones
2018-09-11 14:04 ` Greg KH
8 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-11 11:18 UTC (permalink / raw)
To: Radu Pirea
Cc: robh+dt, mark.rutland, nicolas.ferre, alexandre.belloni, gregkh,
broonie, jslaby, richard.genoud, davem, mchehab+samsung, akpm,
arnd, devicetree, linux-arm-kernel, linux-kernel, linux-serial,
linux-spi
[In case you missed it]
This new pull-request contains the following changes since v1:
- Uplift the submission version from v11 to v12
- Include fix for build breakage due to missing Device Tree include file
Since it is based on the original pull-request, it can be pulled
in either directly on top of it or as a pull in its own right.
The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3:
Linux 4.19-rc1 (2018-08-26 14:11:59 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-spi-tty-v4.20-1
for you to fetch changes up to 65b80dfffeabd4eb253b93d07eadde4d89c18511:
mfd: at91-usart: Include Device Tree header (2018-09-11 11:44:56 +0100)
----------------------------------------------------------------
Immutable branch between MFD, SPI and TTY due for the v4.20 merge window (v2)
----------------------------------------------------------------
Lee Jones (4):
dt-bindings: mfd: atmel-usart: Correct interrupts property to include IRQ number
MAINTAINERS: Change Radu's email address
spi: at91-usart: Make local functions static
mfd: at91-usart: Include Device Tree header
Radu Pirea (6):
MAINTAINERS: Add AT91 USART MFD entry
dt-bindings: Add binding for atmel-usart in SPI mode
mfd: at91-usart: Add MFD driver for USART
MAINTAINERS: Add AT91 USART SPI entry
spi: at91-usart: Add driver for at91-usart as SPI
tty/serial: atmel: Change the driver to work under at91-usart MFD
.../bindings/{serial => mfd}/atmel-usart.txt | 25 +-
MAINTAINERS | 16 +
drivers/mfd/Kconfig | 9 +
drivers/mfd/Makefile | 1 +
drivers/mfd/at91-usart.c | 72 ++++
drivers/spi/Kconfig | 8 +
drivers/spi/Makefile | 1 +
drivers/spi/spi-at91-usart.c | 432 +++++++++++++++++++++
drivers/tty/serial/Kconfig | 1 +
drivers/tty/serial/atmel_serial.c | 42 +-
include/dt-bindings/mfd/at91-usart.h | 17 +
11 files changed, 607 insertions(+), 17 deletions(-)
rename Documentation/devicetree/bindings/{serial => mfd}/atmel-usart.txt (76%)
create mode 100644 drivers/mfd/at91-usart.c
create mode 100644 drivers/spi/spi-at91-usart.c
create mode 100644 include/dt-bindings/mfd/at91-usart.h
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window
2018-09-11 11:18 ` [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window Lee Jones
@ 2018-09-11 14:04 ` Greg KH
2018-09-11 14:07 ` Lee Jones
0 siblings, 1 reply; 38+ messages in thread
From: Greg KH @ 2018-09-11 14:04 UTC (permalink / raw)
To: Lee Jones
Cc: Radu Pirea, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, broonie, jslaby, richard.genoud, davem,
mchehab+samsung, akpm, arnd, devicetree, linux-arm-kernel,
linux-kernel, linux-serial, linux-spi
On Tue, Sep 11, 2018 at 12:18:50PM +0100, Lee Jones wrote:
> [In case you missed it]
>
> This new pull-request contains the following changes since v1:
>
> - Uplift the submission version from v11 to v12
> - Include fix for build breakage due to missing Device Tree include file
>
> Since it is based on the original pull-request, it can be pulled
> in either directly on top of it or as a pull in its own right.
>
> The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3:
>
> Linux 4.19-rc1 (2018-08-26 14:11:59 -0700)
>
> are available in the Git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-spi-tty-v4.20-1
Ok, let's try this again, I've dropped that other patch and taken this
branch into my tree.
Hopefully this all works well :)
thanks,
greg k-h
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window
2018-09-11 14:04 ` Greg KH
@ 2018-09-11 14:07 ` Lee Jones
2018-09-11 14:12 ` Greg KH
0 siblings, 1 reply; 38+ messages in thread
From: Lee Jones @ 2018-09-11 14:07 UTC (permalink / raw)
To: Greg KH
Cc: Radu Pirea, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, broonie, jslaby, richard.genoud, davem,
mchehab+samsung, akpm, arnd, devicetree, linux-arm-kernel,
linux-kernel, linux-serial, linux-spi
On Tue, 11 Sep 2018, Greg KH wrote:
> On Tue, Sep 11, 2018 at 12:18:50PM +0100, Lee Jones wrote:
> > [In case you missed it]
> >
> > This new pull-request contains the following changes since v1:
> >
> > - Uplift the submission version from v11 to v12
> > - Include fix for build breakage due to missing Device Tree include file
> >
> > Since it is based on the original pull-request, it can be pulled
> > in either directly on top of it or as a pull in its own right.
> >
> > The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3:
> >
> > Linux 4.19-rc1 (2018-08-26 14:11:59 -0700)
> >
> > are available in the Git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-spi-tty-v4.20-1
>
> Ok, let's try this again, I've dropped that other patch and taken this
> branch into my tree.
What happened to not rebasing? :D
> Hopefully this all works well :)
Me too. :)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [GIT PULL v2] Immutable branch between MFD, SPI and TTY due for the v4.20 merge window
2018-09-11 14:07 ` Lee Jones
@ 2018-09-11 14:12 ` Greg KH
0 siblings, 0 replies; 38+ messages in thread
From: Greg KH @ 2018-09-11 14:12 UTC (permalink / raw)
To: Lee Jones
Cc: Radu Pirea, robh+dt, mark.rutland, nicolas.ferre,
alexandre.belloni, broonie, jslaby, richard.genoud, davem,
mchehab+samsung, akpm, arnd, devicetree, linux-arm-kernel,
linux-kernel, linux-serial, linux-spi
On Tue, Sep 11, 2018 at 03:07:33PM +0100, Lee Jones wrote:
> On Tue, 11 Sep 2018, Greg KH wrote:
>
> > On Tue, Sep 11, 2018 at 12:18:50PM +0100, Lee Jones wrote:
> > > [In case you missed it]
> > >
> > > This new pull-request contains the following changes since v1:
> > >
> > > - Uplift the submission version from v11 to v12
> > > - Include fix for build breakage due to missing Device Tree include file
> > >
> > > Since it is based on the original pull-request, it can be pulled
> > > in either directly on top of it or as a pull in its own right.
> > >
> > > The following changes since commit 5b394b2ddf0347bef56e50c69a58773c94343ff3:
> > >
> > > Linux 4.19-rc1 (2018-08-26 14:11:59 -0700)
> > >
> > > are available in the Git repository at:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-spi-tty-v4.20-1
> >
> > Ok, let's try this again, I've dropped that other patch and taken this
> > branch into my tree.
>
> What happened to not rebasing? :D
Turned out I hadn't pushed to my immutable branch yet, we got lucky :)
^ permalink raw reply [flat|nested] 38+ messages in thread