- Fix national,lm75 compatible string
- Replace obsolete fsl,spi-num-chipselects with num-cs
Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM")
Signed-off-by: Matthias Schiffer <[email protected]>
---
arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 2 +-
arch/arm/boot/dts/imx6qdl-tqma6a.dtsi | 2 +-
arch/arm/boot/dts/imx6qdl-tqma6b.dtsi | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
index 9513020ddd1a..7aaae83c1fae 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
@@ -20,7 +20,7 @@
&ecspi1 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_ecspi1>;
- fsl,spi-num-chipselects = <1>;
+ num-cs = <1>;
cs-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>;
status = "okay";
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
index c18a06cf7929..b679bec78e6c 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
index a7460075f517..49c472285c06 100644
--- a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi
@@ -16,7 +16,7 @@
};
sensor@48 {
- compatible = "lm75";
+ compatible = "national,lm75";
reg = <0x48>;
};
--
2.17.1
Hi Matthias,
On Mon, Aug 24, 2020 at 6:10 AM Matthias Schiffer
<[email protected]> wrote:
> diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> index 9513020ddd1a..7aaae83c1fae 100644
> --- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> @@ -20,7 +20,7 @@
> &ecspi1 {
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_ecspi1>;
> - fsl,spi-num-chipselects = <1>;
> + num-cs = <1>;
You could simply remove fsl,spi-num-chipselects without passing num-cs.
The spi core is able to count the number of chipselects passed via
cs-gpios in the device tree.
On Mon, 2020-08-24 at 18:36 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Mon, Aug 24, 2020 at 6:10 AM Matthias Schiffer
> <[email protected]> wrote:
>
> > diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > index 9513020ddd1a..7aaae83c1fae 100644
> > --- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi
> > @@ -20,7 +20,7 @@
> > &ecspi1 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&pinctrl_ecspi1>;
> > - fsl,spi-num-chipselects = <1>;
> > + num-cs = <1>;
>
> You could simply remove fsl,spi-num-chipselects without passing num-
> cs.
>
> The spi core is able to count the number of chipselects passed via
> cs-gpios in the device tree.
Hmm, unless I'm overlooking something, this is not going to work:
- spi_get_gpio_descs() sets num_chipselect to the maximum of the
num_chipselect set in the driver and the number of cs-gpios
- spi_imx_probe() sets num_chipselect to 3 if not specified in the
device tree
So I think we would end up with 3 instead of 1 chipselect.
Kind regards,
Matthias
Hi Matthias,
On Tue, Aug 25, 2020 at 4:22 AM Matthias Schiffer
<[email protected]> wrote:
> Hmm, unless I'm overlooking something, this is not going to work:
>
> - spi_get_gpio_descs() sets num_chipselect to the maximum of the
> num_chipselect set in the driver and the number of cs-gpios
>
> - spi_imx_probe() sets num_chipselect to 3 if not specified in the
> device tree
>
> So I think we would end up with 3 instead of 1 chipselect.
Oh, this has changed recently in 8cdcd8aeee281 ("spi: imx/fsl-lpspi:
Convert to GPIO descriptors"):
....
- } else {
- u32 num_cs;
-
- if (!of_property_read_u32(np, "num-cs", &num_cs))
- master->num_chipselect = num_cs;
- /* If not preset, default value of 1 is used */
Initially, if num-cs was not present the default value for num_chipselect was 1.
- }
+ /*
+ * Get number of chip selects from device properties. This can be
+ * coming from device tree or boardfiles, if it is not defined,
+ * a default value of 3 chip selects will be used, as all the legacy
+ * board files have <= 3 chip selects.
+ */
+ if (!device_property_read_u32(&pdev->dev, "num-cs", &val))
+ master->num_chipselect = val;
+ else
+ master->num_chipselect = 3;
Now it became 3.
I think this is a driver issue and we should fix the driver instead of
requiring to pass num-cs to the device tree.
num-cs is not even documented in the spi-imx binding.
On Tue, 2020-08-25 at 11:24 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Tue, Aug 25, 2020 at 4:22 AM Matthias Schiffer
> <[email protected]> wrote:
>
> > Hmm, unless I'm overlooking something, this is not going to work:
> >
> > - spi_get_gpio_descs() sets num_chipselect to the maximum of the
> > num_chipselect set in the driver and the number of cs-gpios
> >
> > - spi_imx_probe() sets num_chipselect to 3 if not specified in the
> > device tree
> >
> > So I think we would end up with 3 instead of 1 chipselect.
>
> Oh, this has changed recently in 8cdcd8aeee281 ("spi: imx/fsl-lpspi:
> Convert to GPIO descriptors"):
> ....
>
> - } else {
> - u32 num_cs;
> -
> - if (!of_property_read_u32(np, "num-cs", &num_cs))
> - master->num_chipselect = num_cs;
> - /* If not preset, default value of 1 is used */
>
> Initially, if num-cs was not present the default value for
> num_chipselect was 1.
>
> - }
> + /*
> + * Get number of chip selects from device properties. This
> can be
> + * coming from device tree or boardfiles, if it is not
> defined,
> + * a default value of 3 chip selects will be used, as all the
> legacy
> + * board files have <= 3 chip selects.
> + */
> + if (!device_property_read_u32(&pdev->dev, "num-cs", &val))
> + master->num_chipselect = val;
> + else
> + master->num_chipselect = 3;
>
> Now it became 3.
>
> I think this is a driver issue and we should fix the driver instead
> of
> requiring to pass num-cs to the device tree.
>
>
> num-cs is not even documented in the spi-imx binding.
Makes sense. Does the following logic sound correct?
- If num-cs is set, use that (and add it to the docs)
- If num-cs is unset, use the number of cs-gpios
- If num-cs is unset and no cs-gpios are defined, use a driver-provided
default
I'm not sure if 3 is a particularly useful default either, but it seems
it was chosen to accommodate boards that previously set this via
platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) have 4
internal CS pins per ECSPI instance, so maybe the driver should use
that as its default instead?
On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer
<[email protected]> wrote:
> Makes sense. Does the following logic sound correct?
>
> - If num-cs is set, use that (and add it to the docs)
I would not add num-cs to the docs. As far as I can see there is no
imx dts that uses num-cs currently.
> - If num-cs is unset, use the number of cs-gpios
> - If num-cs is unset and no cs-gpios are defined, use a driver-provided
> default
>
>
> I'm not sure if 3 is a particularly useful default either, but it seems
> it was chosen to accommodate boards that previously set this via
> platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) have 4
> internal CS pins per ECSPI instance, so maybe the driver should use
> that as its default instead?
I think it is time to get rid of i.MX board files. I will try to work
on this when I have a chance.
bout using 4 as default chip select number, please also check some
older SoCs like imx25, imx35, imx51, imx53, etc
On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote:
> On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer
> <[email protected]> wrote:
>
> > Makes sense. Does the following logic sound correct?
> >
> > - If num-cs is set, use that (and add it to the docs)
>
> I would not add num-cs to the docs. As far as I can see there is no
> imx dts that uses num-cs currently.
But the previous platform data that was removed in 8cdcd8aeee281 ("spi:
imx/fsl-lpspi: Convert to GPIO descriptors") set different values for
different boards. So maybe some DTS should be using num-cs?
>
> > - If num-cs is unset, use the number of cs-gpios
> > - If num-cs is unset and no cs-gpios are defined, use a driver-
> > provided
> > default
> >
> >
> > I'm not sure if 3 is a particularly useful default either, but it
> > seems
> > it was chosen to accommodate boards that previously set this via
> > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7)
> > have 4
> > internal CS pins per ECSPI instance, so maybe the driver should use
> > that as its default instead?
>
> I think it is time to get rid of i.MX board files. I will try to work
> on this when I have a chance.
>
> bout using 4 as default chip select number, please also check some
> older SoCs like imx25, imx35, imx51, imx53, etc
Hmm, I just checked i.MX28, and it has only 3 chip selects per
instance.
Hi Matthias,
On Wed, Aug 26, 2020 at 7:32 AM Matthias Schiffer
<[email protected]> wrote:
> But the previous platform data that was removed in 8cdcd8aeee281 ("spi:
> imx/fsl-lpspi: Convert to GPIO descriptors") set different values for
> different boards. So maybe some DTS should be using num-cs?
Could you provide an example of an imx dts that should use num-cs?
On Wed, 2020-08-26 at 07:59 -0300, Fabio Estevam wrote:
> Hi Matthias,
>
> On Wed, Aug 26, 2020 at 7:32 AM Matthias Schiffer
> <[email protected]> wrote:
>
> > But the previous platform data that was removed in 8cdcd8aeee281
> > ("spi:
> > imx/fsl-lpspi: Convert to GPIO descriptors") set different values
> > for
> > different boards. So maybe some DTS should be using num-cs?
>
> Could you provide an example of an imx dts that should use num-cs?
I'm not sure, does anything break when num_chipselect is set too high?
Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 for
spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that it
would make sense to add this as num-cs to
arch/arm/boot/dts/imx31-lite.dts.
On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer
<[email protected]> wrote:
> Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 for
> spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that it
> would make sense to add this as num-cs to
> arch/arm/boot/dts/imx31-lite.dts.
Or just pass cs-gpios instead?
On Wed, 2020-08-26 at 10:01 -0300, Fabio Estevam wrote:
> On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer
> <[email protected]> wrote:
>
> > Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1
> > for
> > spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that
> > it
> > would make sense to add this as num-cs to
> > arch/arm/boot/dts/imx31-lite.dts.
>
> Or just pass cs-gpios instead?
Using GPIOs for chipselect would require different pinmuxing. Also, why
use GPIOs, when the SPI controller has this built in?
On Wed, 2020-08-26 at 12:32 +0200, Matthias Schiffer wrote:
> On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote:
> > On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer
> > <[email protected]> wrote:
> >
> > > Makes sense. Does the following logic sound correct?
> > >
> > > - If num-cs is set, use that (and add it to the docs)
> >
> > I would not add num-cs to the docs. As far as I can see there is no
> > imx dts that uses num-cs currently.
>
> But the previous platform data that was removed in 8cdcd8aeee281
> ("spi:
> imx/fsl-lpspi: Convert to GPIO descriptors") set different values for
> different boards. So maybe some DTS should be using num-cs?
>
>
> >
> > > - If num-cs is unset, use the number of cs-gpios
> > > - If num-cs is unset and no cs-gpios are defined, use a driver-
> > > provided
> > > default
> > >
> > >
> > > I'm not sure if 3 is a particularly useful default either, but it
> > > seems
> > > it was chosen to accommodate boards that previously set this via
> > > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7)
> > > have 4
> > > internal CS pins per ECSPI instance, so maybe the driver should
> > > use
> > > that as its default instead?
> >
> > I think it is time to get rid of i.MX board files. I will try to
> > work
> > on this when I have a chance.
> >
> > bout using 4 as default chip select number, please also check some
> > older SoCs like imx25, imx35, imx51, imx53, etc
>
> Hmm, I just checked i.MX28, and it has only 3 chip selects per
> instance.
Ah sorry, I got confused, the i.MX28 has a different SPI IP.
On Wed, Aug 26, 2020 at 10:13 AM Matthias Schiffer
<[email protected]> wrote:
> Using GPIOs for chipselect would require different pinmuxing. Also, why
> use GPIOs, when the SPI controller has this built in?
In the initial chips with the ECSPI controller there was a bug with
the native chipselect controller and we had to use GPIO.
On Wed, 2020-08-26 at 10:49 -0300, Fabio Estevam wrote:
> On Wed, Aug 26, 2020 at 10:13 AM Matthias Schiffer
> <[email protected]> wrote:
>
> > Using GPIOs for chipselect would require different pinmuxing. Also,
> > why
> > use GPIOs, when the SPI controller has this built in?
>
> In the initial chips with the ECSPI controller there was a bug with
> the native chipselect controller and we had to use GPIO.
Ah, I see.
Nevertheless, hardware that uses the native chipselects of newer chips
exists (for example our TQ-Systems starterkit mainboards, the DTS of
which we're currently preparing for mainline submission). Shouldn't
num-cs be set for boards (or SoM DTSI) where not all CS pins of the SoC
are usable?
In any case, my original question was about the intended logic for
num_chipselects for SPI drivers. My proposal was:
- If num-cs is set, use that
- If num-cs is unset, use the number of cs-gpios
- If num-cs is unset and no cs-gpios are defined, use a driver-provided
default
How do other SPI controller drivers deal with this?
Hi Matthias,
(Your mailer is messing the threading.)
On Thu, Aug 27, 2020 at 4:31 AM Matthias Schiffer
<[email protected]> wrote:
> Ah, I see.
>
> Nevertheless, hardware that uses the native chipselects of newer chips
> exists (for example our TQ-Systems starterkit mainboards, the DTS of
> which we're currently preparing for mainline submission). Shouldn't
> num-cs be set for boards (or SoM DTSI) where not all CS pins of the SoC
> are usable?
>
> In any case, my original question was about the intended logic for
> num_chipselects for SPI drivers. My proposal was:
>
> - If num-cs is set, use that
> - If num-cs is unset, use the number of cs-gpios
> - If num-cs is unset and no cs-gpios are defined, use a driver-provided
> default
>
> How do other SPI controller drivers deal with this?
I think it would be better to discuss this in the spi mailing list
with Mark Brown and the dt maintainer, Rob Herring.