[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Gary Bisson]

Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
---
Hi,

Seems that this configuration is missing, especially since many
device trees are already using "spidev" nodes.

Regards,
Gary
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index cd81bd0..f208ad5 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -192,6 +192,7 @@ CONFIG_I2C_IMX=y
 CONFIG_SPI=y
 CONFIG_SPI_IMX=y
 CONFIG_SPI_FSL_DSPI=y
+CONFIG_SPI_SPIDEV=m
 CONFIG_GPIO_SYSFS=y
 CONFIG_GPIO_MC9S08DZ60=y
 CONFIG_GPIO_PCA953X=y
-- 
2.9.3

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Fabio Estevam]

Hi Gary,

On Fri, Dec 2, 2016 at 12:56 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:
> Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
> ---
> Hi,
>
> Seems that this configuration is missing, especially since many
> device trees are already using "spidev" nodes.

Then they will trigger the following warning below (from the spidev
probe function):

/*
* spidev should never be referenced in DT without a specific
* compatible string, it is a Linux implementation thing
* rather than a description of the hardware.
*/
if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
    dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
    WARN_ON(spi->dev.of_node &&
                       !of_match_device(spidev_dt_ids, &spi->dev));
}

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Gary Bisson]

Hi Fabio,

On Fri, Dec 02, 2016 at 01:18:42PM -0200, Fabio Estevam wrote:
> Hi Gary,
> 
> On Fri, Dec 2, 2016 at 12:56 PM, Gary Bisson
> <gary.bisson@boundarydevices.com> wrote:
> > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
> > ---
> > Hi,
> >
> > Seems that this configuration is missing, especially since many
> > device trees are already using "spidev" nodes.
> 
> Then they will trigger the following warning below (from the spidev
> probe function):
> 
> /*
> * spidev should never be referenced in DT without a specific
> * compatible string, it is a Linux implementation thing
> * rather than a description of the hardware.
> */
> if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
>     dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
>     WARN_ON(spi->dev.of_node &&
>                        !of_match_device(spidev_dt_ids, &spi->dev));
> }

Yes I've seen this WARN_ON when doing the dt-overlay testing. But is
disabling the SPIDEV configuration a solution?

To be honest I disagree with that WARN_ON. Ok it means that the hardware
isn't fully described in the device tree but for development platforms
(such as ours or any rpi-like boards) the user can design his own
daugher board with whatever SPI device on it. Then using the spidev
interface is very convenient, so I don't understand what we are trying
to forbid here.

Regards,
Gary

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Javier Martinez Canillas]

Hello Gary,

On Fri, Dec 2, 2016 at 12:27 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:
> Hi Fabio,
>
> On Fri, Dec 02, 2016 at 01:18:42PM -0200, Fabio Estevam wrote:
>> Hi Gary,
>>
>> On Fri, Dec 2, 2016 at 12:56 PM, Gary Bisson
>> <gary.bisson@boundarydevices.com> wrote:
>> > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
>> > ---
>> > Hi,
>> >
>> > Seems that this configuration is missing, especially since many
>> > device trees are already using "spidev" nodes.
>>

This seems to have been added just because people weren't looking that
closer to DT patches at the beginning, but now is forbidden. That's
why the kernel now warns about it.

>> Then they will trigger the following warning below (from the spidev
>> probe function):
>>
>> /*
>> * spidev should never be referenced in DT without a specific
>> * compatible string, it is a Linux implementation thing
>> * rather than a description of the hardware.
>> */
>> if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
>>     dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
>>     WARN_ON(spi->dev.of_node &&
>>                        !of_match_device(spidev_dt_ids, &spi->dev));
>> }
>
> Yes I've seen this WARN_ON when doing the dt-overlay testing. But is
> disabling the SPIDEV configuration a solution?
>
> To be honest I disagree with that WARN_ON. Ok it means that the hardware
> isn't fully described in the device tree but for development platforms
> (such as ours or any rpi-like boards) the user can design his own
> daugher board with whatever SPI device on it. Then using the spidev
> interface is very convenient, so I don't understand what we are trying
> to forbid here.
>

AFAICT, what we are trying to forbid is to have a Linux implementation
detail to creep into a Device Tree.

It's OK to use the spidev interface but that's orthogonal on how the
device is instantiated from the DT. If you want to do that, the
correct approach is AFAIU to add a OF device ID entry in
drivers/spi/spidev.c and use that compatible string in your DT.

That way, the DT will describe the hardware instead of just a "spidev"
but you could use the spidev interface to access your SPI device.

> Regards,
> Gary
>

Best regards,
Javier

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Javier Martinez Canillas]

Hello Gary,

On Fri, Dec 2, 2016 at 12:44 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:

[snip]

>
> This seems to have been added just because people weren't looking that
> closer to DT patches at the beginning, but now is forbidden. That's
> why the kernel now warns about it.
>
>>> Then they will trigger the following warning below (from the spidev
>>> probe function):
>>>

I guess I didn't make clear on my previous email, but I'm not against
$SUBJECT. I think that the platforms that currently have nodes using
"spidev" as compatible need to be cleaned up, and so I find it
valuable to have this option enabled so people are aware of the issue.

Best regards,
Javier

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Fabio Estevam]

On Fri, Dec 2, 2016 at 1:50 PM, Javier Martinez Canillas
<javier@dowhile0.org> wrote:

> I guess I didn't make clear on my previous email, but I'm not against
> $SUBJECT. I think that the platforms that currently have nodes using
> "spidev" as compatible need to be cleaned up, and so I find it
> valuable to have this option enabled so people are aware of the issue.

Yes, agreed.

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Gary Bisson]

Hi Javier,

Thanks for the quick feedback on the matter.

On Fri, Dec 02, 2016 at 12:44:42PM -0300, Javier Martinez Canillas wrote:
> Hello Gary,
> 
> On Fri, Dec 2, 2016 at 12:27 PM, Gary Bisson
> <gary.bisson@boundarydevices.com> wrote:
> > Hi Fabio,
> >
> > On Fri, Dec 02, 2016 at 01:18:42PM -0200, Fabio Estevam wrote:
> >> Hi Gary,
> >>
> >> On Fri, Dec 2, 2016 at 12:56 PM, Gary Bisson
> >> <gary.bisson@boundarydevices.com> wrote:
> >> > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
> >> > ---
> >> > Hi,
> >> >
> >> > Seems that this configuration is missing, especially since many
> >> > device trees are already using "spidev" nodes.
> >>
> 
> This seems to have been added just because people weren't looking that
> closer to DT patches at the beginning, but now is forbidden. That's
> why the kernel now warns about it.
> 
> >> Then they will trigger the following warning below (from the spidev
> >> probe function):
> >>
> >> /*
> >> * spidev should never be referenced in DT without a specific
> >> * compatible string, it is a Linux implementation thing
> >> * rather than a description of the hardware.
> >> */
> >> if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
> >>     dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
> >>     WARN_ON(spi->dev.of_node &&
> >>                        !of_match_device(spidev_dt_ids, &spi->dev));
> >> }
> >
> > Yes I've seen this WARN_ON when doing the dt-overlay testing. But is
> > disabling the SPIDEV configuration a solution?
> >
> > To be honest I disagree with that WARN_ON. Ok it means that the hardware
> > isn't fully described in the device tree but for development platforms
> > (such as ours or any rpi-like boards) the user can design his own
> > daugher board with whatever SPI device on it. Then using the spidev
> > interface is very convenient, so I don't understand what we are trying
> > to forbid here.
> >
> 
> AFAICT, what we are trying to forbid is to have a Linux implementation
> detail to creep into a Device Tree.
> 
> It's OK to use the spidev interface but that's orthogonal on how the
> device is instantiated from the DT. If you want to do that, the
> correct approach is AFAIU to add a OF device ID entry in
> drivers/spi/spidev.c and use that compatible string in your DT.

I understand that the device tree isn't supposed to describe such
generic "spidev" concept.

And that argument to me is ok for final products where the hardware is
fully defined. However I still believe that for development platforms
this cannot be applied. My customers want to use the SPI bus to connect
any device they want, and most of those customers don't want to mess
with the kernel. For them, having a spidev node, just like it exists for
i2cdev nodes, is ideal.

> That way, the DT will describe the hardware instead of just a "spidev"
> but you could use the spidev interface to access your SPI device.

But then aren't you afraid that the person will use the first compatible
that shows up (let's say "rohm,dh2228fv") and use it for all his spidev?
In that case the driver is happy although the fact remains that any hw
will be plugged behind. Or at the opposite, if everybody that uses
spidev adds its own compatible I'm not sure it will benefit the drive
code.

Anyway, I appreciate your feedback.

Regards,
Gary

[PATCH] imx_v6_v7_defconfig: enable SPIDEV module

[Javier Martinez Canillas]

Hello Gary,

On Fri, Dec 2, 2016 at 1:13 PM, Gary Bisson
<gary.bisson@boundarydevices.com> wrote:
> Hi Javier,
>
> Thanks for the quick feedback on the matter.
>

You are welcome.

> On Fri, Dec 02, 2016 at 12:44:42PM -0300, Javier Martinez Canillas wrote:
>> Hello Gary,
>>
>> On Fri, Dec 2, 2016 at 12:27 PM, Gary Bisson
>> <gary.bisson@boundarydevices.com> wrote:
>> > Hi Fabio,
>> >
>> > On Fri, Dec 02, 2016 at 01:18:42PM -0200, Fabio Estevam wrote:
>> >> Hi Gary,
>> >>
>> >> On Fri, Dec 2, 2016 at 12:56 PM, Gary Bisson
>> >> <gary.bisson@boundarydevices.com> wrote:
>> >> > Signed-off-by: Gary Bisson <gary.bisson@boundarydevices.com>
>> >> > ---
>> >> > Hi,
>> >> >
>> >> > Seems that this configuration is missing, especially since many
>> >> > device trees are already using "spidev" nodes.
>> >>
>>
>> This seems to have been added just because people weren't looking that
>> closer to DT patches at the beginning, but now is forbidden. That's
>> why the kernel now warns about it.
>>
>> >> Then they will trigger the following warning below (from the spidev
>> >> probe function):
>> >>
>> >> /*
>> >> * spidev should never be referenced in DT without a specific
>> >> * compatible string, it is a Linux implementation thing
>> >> * rather than a description of the hardware.
>> >> */
>> >> if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) {
>> >>     dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n");
>> >>     WARN_ON(spi->dev.of_node &&
>> >>                        !of_match_device(spidev_dt_ids, &spi->dev));
>> >> }
>> >
>> > Yes I've seen this WARN_ON when doing the dt-overlay testing. But is
>> > disabling the SPIDEV configuration a solution?
>> >
>> > To be honest I disagree with that WARN_ON. Ok it means that the hardware
>> > isn't fully described in the device tree but for development platforms
>> > (such as ours or any rpi-like boards) the user can design his own
>> > daugher board with whatever SPI device on it. Then using the spidev
>> > interface is very convenient, so I don't understand what we are trying
>> > to forbid here.
>> >
>>
>> AFAICT, what we are trying to forbid is to have a Linux implementation
>> detail to creep into a Device Tree.
>>
>> It's OK to use the spidev interface but that's orthogonal on how the
>> device is instantiated from the DT. If you want to do that, the
>> correct approach is AFAIU to add a OF device ID entry in
>> drivers/spi/spidev.c and use that compatible string in your DT.
>
> I understand that the device tree isn't supposed to describe such
> generic "spidev" concept.
>
> And that argument to me is ok for final products where the hardware is
> fully defined. However I still believe that for development platforms
> this cannot be applied. My customers want to use the SPI bus to connect
> any device they want, and most of those customers don't want to mess
> with the kernel. For them, having a spidev node, just like it exists for
> i2cdev nodes, is ideal.
>

Yes, they are free to do it in their vendor tree DTBs. They just
shouldn't try to post their DTS for mainline inclusion :)

>> That way, the DT will describe the hardware instead of just a "spidev"
>> but you could use the spidev interface to access your SPI device.
>
> But then aren't you afraid that the person will use the first compatible
> that shows up (let's say "rohm,dh2228fv") and use it for all his spidev?
> In that case the driver is happy although the fact remains that any hw
> will be plugged behind. Or at the opposite, if everybody that uses
> spidev adds its own compatible I'm not sure it will benefit the drive
> code.
>

I agree with you that people adding random compatible strings to the
spidev OF device ID table will not scale either. I don't really have
an answer to that, I just mentioned what the SPI maintainers told me
last time I tried to do something similar for a different platform a
couple of years ago:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/303348.html

> Anyway, I appreciate your feedback.
>
> Regards,
> Gary

Best regards,
Javier