[PATCH 0/2] Add MediaTek MT8365 SPI support

[PATCH 0/2] Add MediaTek MT8365 SPI support

[Alexandre Mergnat]

Hi,
This patch series adds SPI support for MT8365-EVK board.
The SPIDEV is enabled, it can be used through the board pin header,
as described directly on the PCB.

This series depends to another one which add support for
MT8365 SoC and EVK board. Link [1].

Test:
- Loopback MOSI and MISO pins
- Issue the following command:
spidev_test -D /dev/spidev0.0 -v
- RX line should be the same as TX line.

Regards,
Alex

[1]: https://lore.kernel.org/linux-mediatek/20230101220149.3035048-1-bero@baylibre.com/

To: Rob Herring <robh+dt@kernel.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
To: Matthias Brugger <matthias.bgg@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-mediatek@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-spi@vger.kernel.org
Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

---
Alexandre Mergnat (2):
      arm64: dts: mediatek: add spidev support for mt8365-evk board
      spi: spidev: add new mediatek support

 arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 24 ++++++++++++++++++++++++
 drivers/spi/spidev.c                        |  2 ++
 2 files changed, 26 insertions(+)
---
base-commit: 8b6cfcce3ce939db11166680a57253c39110f07e
change-id: 20230118-mt8365-spi-support-0d96bc55a4a0

Best regards,
-- 
Alexandre Mergnat <amergnat@baylibre.com>

[PATCH 1/2] arm64: dts: mediatek: add spidev support for mt8365-evk board

[Alexandre Mergnat]

Add SPI pins and spidev support for mt8365-evk.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
index 275ea3a0e708..c1d603cb129a 100644
--- a/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8365-evk.dts
@@ -142,6 +142,17 @@ pins {
 				 <MT8365_PIN_116_I2S_BCK__FUNC_PWM_C>;
 		};
 	};
+
+	spi_pins: spi-pins {
+		pins {
+			pinmux = <MT8365_PIN_26_SPI_CS__FUNC_SPI_CSB>,
+				 <MT8365_PIN_27_SPI_CK__FUNC_SPI_CLK>,
+				 <MT8365_PIN_28_SPI_MI__FUNC_SPI_MI>,
+				 <MT8365_PIN_29_SPI_MO__FUNC_SPI_MO>;
+			bias-disable;
+		};
+	};
+
 };
 
 &pwm {
@@ -150,6 +161,19 @@ &pwm {
 	status = "okay";
 };
 
+&spi {
+	pinctrl-0 = <&spi_pins>;
+	pinctrl-names = "default";
+	mediatek,pad-select = <0>;
+	status = "okay";
+
+	spidev@0 {
+		compatible = "mediatek,genio";
+		spi-max-frequency = <5000000>;
+		reg = <0>;
+	};
+};
+
 &uart0 {
 	pinctrl-0 = <&uart0_pins>;
 	pinctrl-names = "default";

-- 
b4 0.10.1

[PATCH 2/2] spi: spidev: add new mediatek support

[Alexandre Mergnat]

Add the "mediatek,genio" compatible string to support Mediatek
SPI controller on the genio boards.

Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
---
 drivers/spi/spidev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 6313e7d0cdf8..e23b825b8d30 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -702,6 +702,7 @@ static const struct spi_device_id spidev_spi_ids[] = {
 	{ .name = "m53cpld" },
 	{ .name = "spi-petra" },
 	{ .name = "spi-authenta" },
+	{ .name = "genio" },
 	{},
 };
 MODULE_DEVICE_TABLE(spi, spidev_spi_ids);
@@ -728,6 +729,7 @@ static const struct of_device_id spidev_dt_ids[] = {
 	{ .compatible = "menlo,m53cpld", .data = &spidev_of_check },
 	{ .compatible = "cisco,spi-petra", .data = &spidev_of_check },
 	{ .compatible = "micron,spi-authenta", .data = &spidev_of_check },
+	{ .compatible = "mediatek,genio", .data = &spidev_of_check },
 	{},
 };
 MODULE_DEVICE_TABLE(of, spidev_dt_ids);

-- 
b4 0.10.1

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Thu, Jan 19, 2023 at 05:28:50PM +0100, Alexandre Mergnat wrote:
> Add the "mediatek,genio" compatible string to support Mediatek
> SPI controller on the genio boards.

>  	{ .compatible = "cisco,spi-petra", .data = &spidev_of_check },
>  	{ .compatible = "micron,spi-authenta", .data = &spidev_of_check },
> +	{ .compatible = "mediatek,genio", .data = &spidev_of_check },

We need a matching update to the binding document.

This does also seem like a terribly generic name - Google
suggests that this is actually a series of numbered products (eg,
Genio 700), perhaps we should be using the specific numbers here?
I guess users would care which they're talking to.  It really
parses as being "generic I/O" which would be an end run around
describing the actual product though it's not actually that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Matthias Brugger]

On 19/01/2023 17:28, Alexandre Mergnat wrote:
> Add the "mediatek,genio" compatible string to support Mediatek
> SPI controller on the genio boards.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   drivers/spi/spidev.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 6313e7d0cdf8..e23b825b8d30 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -702,6 +702,7 @@ static const struct spi_device_id spidev_spi_ids[] = {
>   	{ .name = "m53cpld" },
>   	{ .name = "spi-petra" },
>   	{ .name = "spi-authenta" },
> +	{ .name = "genio" },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(spi, spidev_spi_ids);
> @@ -728,6 +729,7 @@ static const struct of_device_id spidev_dt_ids[] = {
>   	{ .compatible = "menlo,m53cpld", .data = &spidev_of_check },
>   	{ .compatible = "cisco,spi-petra", .data = &spidev_of_check },
>   	{ .compatible = "micron,spi-authenta", .data = &spidev_of_check },
> +	{ .compatible = "mediatek,genio", .data = &spidev_of_check },
>   	{},
>   };
>   MODULE_DEVICE_TABLE(of, spidev_dt_ids);
> 

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Krzysztof Kozlowski]

On 19/01/2023 17:28, Alexandre Mergnat wrote:
> Add the "mediatek,genio" compatible string to support Mediatek
> SPI controller on the genio boards.
> 
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
>  drivers/spi/spidev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
> index 6313e7d0cdf8..e23b825b8d30 100644
> --- a/drivers/spi/spidev.c
> +++ b/drivers/spi/spidev.c
> @@ -702,6 +702,7 @@ static const struct spi_device_id spidev_spi_ids[] = {
>  	{ .name = "m53cpld" },
>  	{ .name = "spi-petra" },
>  	{ .name = "spi-authenta" },
> +	{ .name = "genio" },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(spi, spidev_spi_ids);
> @@ -728,6 +729,7 @@ static const struct of_device_id spidev_dt_ids[] = {
>  	{ .compatible = "menlo,m53cpld", .data = &spidev_of_check },
>  	{ .compatible = "cisco,spi-petra", .data = &spidev_of_check },
>  	{ .compatible = "micron,spi-authenta", .data = &spidev_of_check },
> +	{ .compatible = "mediatek,genio", .data = &spidev_of_check },

Please run scripts/checkpatch.pl and fix reported warnings.

Best regards,
Krzysztof

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Alexandre Mergnat]

Le jeu. 19 janv. 2023 à 17:49, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> a écrit :
>
> On 19/01/2023 17:28, Alexandre Mergnat wrote:
> >       { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
> > +     { .compatible = "mediatek,genio", .data = &spidev_of_check },
>
> Please run scripts/checkpatch.pl and fix reported warnings.

Actually I did.
I saw: "WARNING: DT compatible string "mediatek,genio" appears
un-documented -- check ./Documentation/devicetree/bindings/"
But there are no bindings for spidev. I've made some grep on already
supported compatible devices like "micron,spi-authenta", but I didn't
find documentation to add "mediatek,genio".
Do you know where I should document it please ?

Regards,
Alex

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Krzysztof Kozlowski]

On 19/01/2023 20:18, Alexandre Mergnat wrote:
> Le jeu. 19 janv. 2023 à 17:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> a écrit :
>>
>> On 19/01/2023 17:28, Alexandre Mergnat wrote:
>>>       { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
>>> +     { .compatible = "mediatek,genio", .data = &spidev_of_check },
>>
>> Please run scripts/checkpatch.pl and fix reported warnings.
> 
> Actually I did.
> I saw: "WARNING: DT compatible string "mediatek,genio" appears
> un-documented -- check ./Documentation/devicetree/bindings/"
> But there are no bindings for spidev. 

There are. Just some other people were as well ignoring warnings. What
is the purpose of having tools if people keep ignoring the warnings, sigh...


Best regards,
Krzysztof

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Krzysztof Kozlowski]

On 19/01/2023 20:18, Alexandre Mergnat wrote:
> Le jeu. 19 janv. 2023 à 17:49, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> a écrit :
>>
>> On 19/01/2023 17:28, Alexandre Mergnat wrote:
>>>       { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
>>> +     { .compatible = "mediatek,genio", .data = &spidev_of_check },
>>
>> Please run scripts/checkpatch.pl and fix reported warnings.
> 
> Actually I did.
> I saw: "WARNING: DT compatible string "mediatek,genio" appears
> un-documented -- check ./Documentation/devicetree/bindings/"
> But there are no bindings for spidev. I've made some grep on already
> supported compatible devices like "micron,spi-authenta", but I didn't
> find documentation to add "mediatek,genio".

Another point - why is this after "micron"? Don't add entries to the end
but in order.

Best regards,
Krzysztof

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Krzysztof Kozlowski]

On 20/01/2023 08:44, Krzysztof Kozlowski wrote:
> On 19/01/2023 20:18, Alexandre Mergnat wrote:
>> Le jeu. 19 janv. 2023 à 17:49, Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> a écrit :
>>>
>>> On 19/01/2023 17:28, Alexandre Mergnat wrote:
>>>>       { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
>>>> +     { .compatible = "mediatek,genio", .data = &spidev_of_check },
>>>
>>> Please run scripts/checkpatch.pl and fix reported warnings.
>>
>> Actually I did.
>> I saw: "WARNING: DT compatible string "mediatek,genio" appears
>> un-documented -- check ./Documentation/devicetree/bindings/"
>> But there are no bindings for spidev. 
> 
> There are. Just some other people were as well ignoring warnings. What
> is the purpose of having tools if people keep ignoring the warnings, sigh...

I documented the missing ones.

Best regards,
Krzysztof

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Michael Walle]

From: Alexandre Mergnat <amergnat@baylibre.com>

> Add the "mediatek,genio" compatible string to support Mediatek
> SPI controller on the genio boards.

What is the use case of having the spidev? What if I want to
connect a device with a linux driver to it? It seems like you
just want to expose the SPI bus on the pin header. There was a
similar discussion for a mikrobus connector [1].

-michael

[1] https://lore.kernel.org/linux-devicetree/YmFo+EntwxIsco%2Ft@robh.at.kernel.org/

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Alexandre Mergnat]

Le ven. 20 janv. 2023 à 09:20, Michael Walle <michael@walle.cc> a écrit :
>
> From: Alexandre Mergnat <amergnat@baylibre.com>
>
> > Add the "mediatek,genio" compatible string to support Mediatek
> > SPI controller on the genio boards.
>
> What is the use case of having the spidev? What if I want to
> connect a device with a linux driver to it? It seems like you
> just want to expose the SPI bus on the pin header. There was a
> similar discussion for a mikrobus connector [1].
>

Hi Michael,

Yes I want to expose the SPI on the pin header for two reasons:
- It's an Evaluation Kit board, I believe exposing SPI helps new
customers to try/understand it.
- This board will join the KernelCI soon, this setup will help to do
SPI non regression tests for a fixed default configuration.

AFAII from [1] , you can easily modify the current spidev@0 (If you
don't want to keep userspace interface) or simply add (in the DTS or
overlay) another node foo@1 with a different compatible (and so on)
according to the chip you plug on the header pin.

Regards,
Alex

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Alexandre Mergnat]

 Because there are no logical order:
    { .compatible = "rohm,dh2228fv", .data = &spidev_of_check },
    { .compatible = "lineartechnology,ltc2488", .data = &spidev_of_check },
    { .compatible = "semtech,sx1301", .data = &spidev_of_check },
    { .compatible = "lwn,bk4", .data = &spidev_of_check },
    { .compatible = "dh,dhcom-board", .data = &spidev_of_check },
    { .compatible = "menlo,m53cpld", .data = &spidev_of_check },
    { .compatible = "cisco,spi-petra", .data = &spidev_of_check },
    { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
    { .compatible = "mediatek,genio", .data = &spidev_of_check },

I can put it first then before "rohm", or before
"micron,spi-authenta" you prefer.

I can also introduce another patch in my serie to re-order everything.

Le ven. 20 janv. 2023 à 08:47, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> a écrit :
>
> On 19/01/2023 20:18, Alexandre Mergnat wrote:
> > Le jeu. 19 janv. 2023 à 17:49, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> a écrit :
> >>
> >> On 19/01/2023 17:28, Alexandre Mergnat wrote:
> >>>       { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
> >>> +     { .compatible = "mediatek,genio", .data = &spidev_of_check },
> >>
> >> Please run scripts/checkpatch.pl and fix reported warnings.
> >
> > Actually I did.
> > I saw: "WARNING: DT compatible string "mediatek,genio" appears
> > un-documented -- check ./Documentation/devicetree/bindings/"
> > But there are no bindings for spidev. I've made some grep on already
> > supported compatible devices like "micron,spi-authenta", but I didn't
> > find documentation to add "mediatek,genio".
>
> Another point - why is this after "micron"? Don't add entries to the end
> but in order.
>
> Best regards,
> Krzysztof
>

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Michael Walle]

Hi,

Am 2023-01-23 10:37, schrieb Alexandre Mergnat:
> Le ven. 20 janv. 2023 à 09:20, Michael Walle <michael@walle.cc> a écrit 
> :
>> 
>> From: Alexandre Mergnat <amergnat@baylibre.com>
>> 
>> > Add the "mediatek,genio" compatible string to support Mediatek
>> > SPI controller on the genio boards.
>> 
>> What is the use case of having the spidev? What if I want to
>> connect a device with a linux driver to it? It seems like you
>> just want to expose the SPI bus on the pin header. There was a
>> similar discussion for a mikrobus connector [1].
>> 
> Yes I want to expose the SPI on the pin header for two reasons:

Then "mediatek,genio" doesn't really describe the hardware, does it?
If you read that linked thread, NXP was also trying exposing the SPI
bus on a pin header. IMHO this is just misusing the userspace spi-dev.

That being said, exposing something on a pinheader (or on a standardized
connector) seems like a common thing and we should be working towards
a good solution. I still think Robs proposal for the mikrobus connector
makes also sense for your case.

-michael

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

On Mon, Jan 23, 2023 at 10:37:58AM +0100, Alexandre Mergnat wrote:

> Yes I want to expose the SPI on the pin header for two reasons:
> - It's an Evaluation Kit board, I believe exposing SPI helps new
> customers to try/understand it.

That's not how this works.  Anyone connecting something to the
SPI header will need to update the DT to reflect whatever they
have connected, if that is something that should be controlled
with spidev then they should add the compatible for that thing
to the driver.  If that is something that has a regular driver
then the regular driver will be used.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Alexandre Mergnat]

Le lun. 23 janv. 2023 à 11:44, Michael Walle <michael@walle.cc> a écrit :
>
> Hi,
>
> Am 2023-01-23 10:37, schrieb Alexandre Mergnat:
> > Le ven. 20 janv. 2023 à 09:20, Michael Walle <michael@walle.cc> a écrit
> > :
> >>
> >> From: Alexandre Mergnat <amergnat@baylibre.com>
> >>
> >> > Add the "mediatek,genio" compatible string to support Mediatek
> >> > SPI controller on the genio boards.
> >>
> >> What is the use case of having the spidev? What if I want to
> >> connect a device with a linux driver to it? It seems like you
> >> just want to expose the SPI bus on the pin header. There was a
> >> similar discussion for a mikrobus connector [1].
> >>
> > Yes I want to expose the SPI on the pin header for two reasons:
>
> Then "mediatek,genio" doesn't really describe the hardware, does it?
> If you read that linked thread, NXP was also trying exposing the SPI
> bus on a pin header. IMHO this is just misusing the userspace spi-dev.
>
> That being said, exposing something on a pinheader (or on a standardized
> connector) seems like a common thing and we should be working towards
> a good solution. I still think Robs proposal for the mikrobus connector
> makes also sense for your case.
>

I don't think this is the same case. For the mikrobus case, it's
consistent to have a connector because it fit with other "add-on"
board which can be plug on the "mother board".
Here I've just a simple debug pin header. I've taken a look at
raspberry pi and Beaglebone black DTS, they don't use connector, but
DTS overlay to enable SPI. I think you're right when you say that I'm
misusing the userspace spi-dev.

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Alexandre Mergnat]

Le lun. 23 janv. 2023 à 13:19, Mark Brown <broonie@kernel.org> a écrit :
>
> On Mon, Jan 23, 2023 at 10:37:58AM +0100, Alexandre Mergnat wrote:
>
> > Yes I want to expose the SPI on the pin header for two reasons:
> > - It's an Evaluation Kit board, I believe exposing SPI helps new
> > customers to try/understand it.
>
> That's not how this works.  Anyone connecting something to the
> SPI header will need to update the DT to reflect whatever they
> have connected, if that is something that should be controlled
> with spidev then they should add the compatible for that thing
> to the driver.  If that is something that has a regular driver
> then the regular driver will be used.

Got it. I think this series should be dropped then. If someone needs
the SPI, then he should use overlay (or modify the DTS locally).
I thought I could use spidev to bring SPI into the userspace, to help
future users play with it ("/dev/spidev0.0").

Re: [PATCH 2/2] spi: spidev: add new mediatek support

[Krzysztof Kozlowski]

On 23/01/2023 11:06, Alexandre Mergnat wrote:
>  Because there are no logical order:
>     { .compatible = "rohm,dh2228fv", .data = &spidev_of_check },
>     { .compatible = "lineartechnology,ltc2488", .data = &spidev_of_check },
>     { .compatible = "semtech,sx1301", .data = &spidev_of_check },
>     { .compatible = "lwn,bk4", .data = &spidev_of_check },
>     { .compatible = "dh,dhcom-board", .data = &spidev_of_check },
>     { .compatible = "menlo,m53cpld", .data = &spidev_of_check },
>     { .compatible = "cisco,spi-petra", .data = &spidev_of_check },
>     { .compatible = "micron,spi-authenta", .data = &spidev_of_check },
>     { .compatible = "mediatek,genio", .data = &spidev_of_check },
> 
> I can put it first then before "rohm", or before
> "micron,spi-authenta" you prefer.

Yeah, I noticed it afterwards.

> 
> I can also introduce another patch in my serie to re-order everything.

I already sent a patch for it.

Best regards,
Krzysztof