| Message ID | 20230118-mt8365-spi-support-v1-2-842a21e50494@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add MediaTek MT8365 SPI support | expand |
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.
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); >
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
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
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
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
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
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/
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
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
>
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
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.
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.
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").
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
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);
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(+)