| Message ID | 20180904111310.4049-1-radu_nicolae.pirea@upb.ro (mailing list archive) |
|---|---|
| Headers | show |
| Series | Driver for at91 usart in spi mode | expand |
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
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 >
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.
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?
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.
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
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.
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
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?
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.
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
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
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?
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. >
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.
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/
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.
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.
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.
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
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
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.
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.
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.
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.
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 :).
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 :). >
Hi, 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. Currently I am not working for Microchip, but I will continue to maintain and improve this driver. So yes, this is the reason that I changed my email address from MAINTAINERS file. Changes in v12: - fixed interrupts property in example from bindings of SPI driver - changed email address in MAINTAINERS - added static to declaration of few functions in SPI driver(kbuild-robot warning) Changes in v11: - removed "depends on HAS_DMA" from drivers/spi/Kconfig because the driver has no dma support - changed "selects MFD_AT91_USART" to "depends on MFD_AT91_USART" in drivers/spi/Kconfig - changed comment style in spi-at91-usart.c Changes in v10: -fixed kbuild test robot warning Changes in v9: - minor changes - rebased on top of broonie/for-4.19 Changes in v8: - fixed passing an empty mfd cell if "atmel,usart-mode" value is invalid Changes in v7: - synced up SPDIX license with module license - numbering of usart modes starts from 0 insteand of 1 Changes in v6: - removed unused compatible strings from serial and spi drivers Changes in v5: - fixed usage of stdout-path property with atmel_serial driver Changes in v4: - modified the spi driver to use cs gpio support form spi subsystem - fixed dma transfers for serial driver - squashed binding for spi and serial and moved them to mfd/atmel-usart.txt Changes in v3: - fixed spi slaves probing Changes in v2: - added at91-usart mfd driver - modified spi-at91-usart driver to work as mfd driver child - modified atmel_serial driver to work as mfd driver child Changes in v1: - added spi-at91-usart driver 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