This patch set adds device tree support to ad2s90, with standard
device tree id table, adds the respective dt-binding documentation,
solves a codestyle warning and move the driver out of staging.
This patch set completes all the remaining itens listed to be done
before moving the driver out of staging, enumerated in this mail thread:
https://marc.info/?l=linux-iio&m=154028966111330&w=2, except by one
codestyle problem: "CHECK: struct mutex definition without comment". It
seems to be a commonly ignored check for mutexes of device states. If I
am wrong, please, let me know and I will be happy to send a patch to
tackle it.
Matheus Tavares (6):
staging:iio:ad2s90: Add device tree support
staging:iio:ad2s90: Remove spi setup that should be done via dt
staging:iio:ad2s90: Add max frequency check at probe
dt-bindings:iio:resolver: Add docs for ad2s90
staging:iio:ad2s90: Add SPDX license identifier
staging:iio:ad2s90: Move out of staging
.../bindings/iio/resolver/ad2s90.txt | 26 ++++++++++++++++
drivers/iio/resolver/Kconfig | 10 ++++++
drivers/iio/resolver/Makefile | 1 +
drivers/{staging => }/iio/resolver/ad2s90.c | 31 ++++++++++++-------
drivers/staging/iio/resolver/Kconfig | 10 ------
drivers/staging/iio/resolver/Makefile | 1 -
6 files changed, 57 insertions(+), 22 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
rename drivers/{staging => }/iio/resolver/ad2s90.c (81%)
--
2.18.0
This patch adds device tree support to ad2s90 with standard
device tree id table.
Signed-off-by: Matheus Tavares <[email protected]>
---
drivers/staging/iio/resolver/ad2s90.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 3e257ac46f7a..ff32ca76ca00 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -107,6 +107,12 @@ static int ad2s90_probe(struct spi_device *spi)
return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
}
+static const struct of_device_id ad2s90_of_match[] = {
+ { .compatible = "adi,ad2s90", },
+ {}
+};
+MODULE_DEVICE_TABLE(of, ad2s90_of_match);
+
static const struct spi_device_id ad2s90_id[] = {
{ "ad2s90" },
{}
@@ -116,6 +122,7 @@ MODULE_DEVICE_TABLE(spi, ad2s90_id);
static struct spi_driver ad2s90_driver = {
.driver = {
.name = "ad2s90",
+ .of_match_table = of_match_ptr(ad2s90_of_match),
},
.probe = ad2s90_probe,
.id_table = ad2s90_id,
--
2.18.0
The ad2s90 driver currently sets some spi settings (max_speed_hz and
mode) at ad2s90_probe. This should, instead, be handled via device tree.
This patch removes these configurations from the probe function.
Note: The way in which the mentioned spi settings need to be specified
on the ad2s90's node of a device tree will be documented in the future
patch "dt-bindings:iio:resolver: Add docs for ad2s90".
Signed-off-by: Matheus Tavares <[email protected]>
---
drivers/staging/iio/resolver/ad2s90.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index ff32ca76ca00..95c118c48400 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -77,7 +77,6 @@ static int ad2s90_probe(struct spi_device *spi)
{
struct iio_dev *indio_dev;
struct ad2s90_state *st;
- int ret;
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
@@ -94,16 +93,6 @@ static int ad2s90_probe(struct spi_device *spi)
indio_dev->num_channels = 1;
indio_dev->name = spi_get_device_id(spi)->name;
- /* need 600ns between CS and the first falling edge of SCLK */
- spi->max_speed_hz = 830000;
- spi->mode = SPI_MODE_3;
- ret = spi_setup(spi);
-
- if (ret < 0) {
- dev_err(&spi->dev, "spi_setup failed!\n");
- return ret;
- }
-
return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
}
--
2.18.0
This patch adds a max frequency check at the beginning of ad2s90_probe
function so that when it is set to a value above 0.83Mhz, dev_err is
called with an appropriate message and -EINVAL is returned.
The defined limit is 0.83Mhz instead of 2Mhz, which is the chip's max
frequency as specified in the datasheet, because, as also specified in
the datasheet, a 600ns delay is expected between the application of a
logic LO to CS and the application of SCLK. Since the delay is not
implemented in the spi code, to satisfy it, SCLK's period should be at
most 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
gives roughly 830000Hz.
Signed-off-by: Matheus Tavares <[email protected]>
Signed-off-by: Alexandru Ardelean <[email protected]>
---
Alex's S-o-B was added because he gave the code suggestion for this
patch.
drivers/staging/iio/resolver/ad2s90.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 95c118c48400..949ff55ac6b0 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -19,6 +19,12 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+/*
+ * Although chip's max frequency is 2Mhz, it needs 600ns between CS and the
+ * first falling edge of SCLK, so frequency should be at most 1 / (2 * 6e-7)
+ */
+#define AD2S90_MAX_SPI_FREQ_HZ 830000
+
struct ad2s90_state {
struct mutex lock;
struct spi_device *sdev;
@@ -78,6 +84,12 @@ static int ad2s90_probe(struct spi_device *spi)
struct iio_dev *indio_dev;
struct ad2s90_state *st;
+ if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
+ dev_err(&spi->dev, "SPI CLK, %d Hz exceeds %d Hz\n",
+ spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
+ return -EINVAL;
+ }
+
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;
--
2.18.0
This patch adds the device tree binding documentation for the ad2s90
resolver-to-digital converter.
Signed-off-by: Matheus Tavares <[email protected]>
---
.../bindings/iio/resolver/ad2s90.txt | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
new file mode 100644
index 000000000000..b42cc7752ffd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
@@ -0,0 +1,26 @@
+Analog Devices AD2S90 Resolver-to-Digital Converter
+
+https://www.analog.com/en/products/ad2s90.html
+
+Required properties:
+ - compatible : should be "adi,ad2s90"
+ - reg : SPI chip select number for the device
+ - spi-max-frequency : set maximum clock frequency, must be 830000
+ - spi-cpol and spi-cpha : must be defined to enable SPI mode 3
+
+Note about max frequency:
+ Chip's max frequency, as specified in its datasheet, is 2Mhz. But a 600ns
+ delay is expected between the application of a logic LO to CS and the
+ application of SCLK, as also specified. And since the delay is not
+ implemented in the spi code, to satisfy it, SCLK's period should be at most
+ 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which gives
+ roughly 830000Hz.
+
+Example:
+resolver@0 {
+ compatible = "adi,ad2s90";
+ reg = <0>;
+ spi-max-frequency = <830000>;
+ spi-cpol;
+ spi-cpha;
+};
--
2.18.0
This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c,
which solves the checkpatch.pl warning:
"WARNING: Missing or malformed SPDX-License-Identifier tag in line 1".
Signed-off-by: Matheus Tavares <[email protected]>
---
drivers/staging/iio/resolver/ad2s90.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
index 949ff55ac6b0..f439da721df8 100644
--- a/drivers/staging/iio/resolver/ad2s90.c
+++ b/drivers/staging/iio/resolver/ad2s90.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
/*
* ad2s90.c simple support for the ADI Resolver to Digital Converters: AD2S90
*
--
2.18.0
Move ad2s90 resolver driver out of staging to the main tree.
Signed-off-by: Matheus Tavares <[email protected]>
Signed-off-by: Victor Colombo <[email protected]>
---
drivers/iio/resolver/Kconfig | 10 ++++++++++
drivers/iio/resolver/Makefile | 1 +
drivers/{staging => }/iio/resolver/ad2s90.c | 0
drivers/staging/iio/resolver/Kconfig | 10 ----------
drivers/staging/iio/resolver/Makefile | 1 -
5 files changed, 11 insertions(+), 11 deletions(-)
rename drivers/{staging => }/iio/resolver/ad2s90.c (100%)
diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
index 2ced9f22aa70..786801be54f6 100644
--- a/drivers/iio/resolver/Kconfig
+++ b/drivers/iio/resolver/Kconfig
@@ -3,6 +3,16 @@
#
menu "Resolver to digital converters"
+config AD2S90
+ tristate "Analog Devices ad2s90 driver"
+ depends on SPI
+ help
+ Say yes here to build support for Analog Devices spi resolver
+ to digital converters, ad2s90, provides direct access via sysfs.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ad2s90.
+
config AD2S1200
tristate "Analog Devices ad2s1200/ad2s1205 driver"
depends on SPI
diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
index 4e1dccae07e7..398d82d50028 100644
--- a/drivers/iio/resolver/Makefile
+++ b/drivers/iio/resolver/Makefile
@@ -2,4 +2,5 @@
# Makefile for Resolver/Synchro drivers
#
+obj-$(CONFIG_AD2S90) += ad2s90.o
obj-$(CONFIG_AD2S1200) += ad2s1200.o
diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/iio/resolver/ad2s90.c
similarity index 100%
rename from drivers/staging/iio/resolver/ad2s90.c
rename to drivers/iio/resolver/ad2s90.c
diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
index 6a469ee6101f..4a727c17bb8f 100644
--- a/drivers/staging/iio/resolver/Kconfig
+++ b/drivers/staging/iio/resolver/Kconfig
@@ -3,16 +3,6 @@
#
menu "Resolver to digital converters"
-config AD2S90
- tristate "Analog Devices ad2s90 driver"
- depends on SPI
- help
- Say yes here to build support for Analog Devices spi resolver
- to digital converters, ad2s90, provides direct access via sysfs.
-
- To compile this driver as a module, choose M here: the
- module will be called ad2s90.
-
config AD2S1210
tristate "Analog Devices ad2s1210 driver"
depends on SPI
diff --git a/drivers/staging/iio/resolver/Makefile b/drivers/staging/iio/resolver/Makefile
index 8d901dc7500b..b2049f2ce36e 100644
--- a/drivers/staging/iio/resolver/Makefile
+++ b/drivers/staging/iio/resolver/Makefile
@@ -2,5 +2,4 @@
# Makefile for Resolver/Synchro drivers
#
-obj-$(CONFIG_AD2S90) += ad2s90.o
obj-$(CONFIG_AD2S1210) += ad2s1210.o
--
2.18.0
Hi Matheus,
On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
<[email protected]> wrote:
>
> This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c,
> which solves the checkpatch.pl warning:
> "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1".
>
> Signed-off-by: Matheus Tavares <[email protected]>
> ---
> drivers/staging/iio/resolver/ad2s90.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 949ff55ac6b0..f439da721df8 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
This should be:
// SPDX-License-Identifier: GPL-2.0
On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam <[email protected]> wrote:
>
> Hi Matheus,
>
Hi, Fabio
> On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
> <[email protected]> wrote:
> >
> > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c,
> > which solves the checkpatch.pl warning:
> > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1".
> >
> > Signed-off-by: Matheus Tavares <[email protected]>
> > ---
> > drivers/staging/iio/resolver/ad2s90.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> > index 949ff55ac6b0..f439da721df8 100644
> > --- a/drivers/staging/iio/resolver/ad2s90.c
> > +++ b/drivers/staging/iio/resolver/ad2s90.c
> > @@ -1,3 +1,4 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
>
> This should be:
> // SPDX-License-Identifier: GPL-2.0
Hm, but it seems that the identifier "GPL-2.0" is deprecated, look:
https://spdx.org/licenses/GPL-2.0.html. It has been updated to
"GPL-2.0-only" in license list v3
(https://spdx.org/licenses/GPL-2.0-only.html). Is there some other
reason to use the deprecated "GPL-2.0" that I'm not aware of?
Thanks,
Matheus
On Fri, Nov 09, 2018 at 09:19:52PM -0200, Matheus Tavares Bernardino wrote:
> On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam <[email protected]> wrote:
> >
> > Hi Matheus,
> >
>
> Hi, Fabio
>
> > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
> > <[email protected]> wrote:
> > >
> > > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c,
> > > which solves the checkpatch.pl warning:
> > > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1".
> > >
> > > Signed-off-by: Matheus Tavares <[email protected]>
> > > ---
> > > drivers/staging/iio/resolver/ad2s90.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> > > index 949ff55ac6b0..f439da721df8 100644
> > > --- a/drivers/staging/iio/resolver/ad2s90.c
> > > +++ b/drivers/staging/iio/resolver/ad2s90.c
> > > @@ -1,3 +1,4 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> >
> > This should be:
> > // SPDX-License-Identifier: GPL-2.0
>
> Hm, but it seems that the identifier "GPL-2.0" is deprecated, look:
> https://spdx.org/licenses/GPL-2.0.html. It has been updated to
> "GPL-2.0-only" in license list v3
> (https://spdx.org/licenses/GPL-2.0-only.html). Is there some other
> reason to use the deprecated "GPL-2.0" that I'm not aware of?
Yes, please read the in-kernel documentation for all of this at:
Documentation/process/license-rules.rst
Long story short, we started the adding of these tags to the kernel
before the crazyness of the "-only" markings for GPL in spdx. Let's
keep it this way for now, if we ever get the whole kernel finished, then
we can revisit the markings and maybe do a wholesale conversion, if it's
really needed.
thanks,
greg k-h
On Fri, Nov 9, 2018 at 10:20 PM Greg Kroah-Hartman
<[email protected]> wrote:
>
> On Fri, Nov 09, 2018 at 09:19:52PM -0200, Matheus Tavares Bernardino wrote:
> > On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam <[email protected]> wrote:
> > >
> > > Hi Matheus,
> > >
> >
> > Hi, Fabio
> >
> > > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
> > > <[email protected]> wrote:
> > > >
> > > > This patch adds the SPDX GPL-2.0-only license identifier to ad2s90.c,
> > > > which solves the checkpatch.pl warning:
> > > > "WARNING: Missing or malformed SPDX-License-Identifier tag in line 1".
> > > >
> > > > Signed-off-by: Matheus Tavares <[email protected]>
> > > > ---
> > > > drivers/staging/iio/resolver/ad2s90.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> > > > index 949ff55ac6b0..f439da721df8 100644
> > > > --- a/drivers/staging/iio/resolver/ad2s90.c
> > > > +++ b/drivers/staging/iio/resolver/ad2s90.c
> > > > @@ -1,3 +1,4 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > >
> > > This should be:
> > > // SPDX-License-Identifier: GPL-2.0
> >
> > Hm, but it seems that the identifier "GPL-2.0" is deprecated, look:
> > https://spdx.org/licenses/GPL-2.0.html. It has been updated to
> > "GPL-2.0-only" in license list v3
> > (https://spdx.org/licenses/GPL-2.0-only.html). Is there some other
> > reason to use the deprecated "GPL-2.0" that I'm not aware of?
>
> Yes, please read the in-kernel documentation for all of this at:
> Documentation/process/license-rules.rst
>
> Long story short, we started the adding of these tags to the kernel
> before the crazyness of the "-only" markings for GPL in spdx. Let's
> keep it this way for now, if we ever get the whole kernel finished, then
> we can revisit the markings and maybe do a wholesale conversion, if it's
> really needed.
>
Got it, thanks for the explanation! I'll correct this in v2.
Thanks,
Matheus
> thanks,
>
> greg k-h
Hi Matheus,
On Fri, Nov 9, 2018 at 10:27 PM Matheus Tavares Bernardino
<[email protected]> wrote:
> Got it, thanks for the explanation! I'll correct this in v2.
One more suggestion: in v2 you could also consider to remove the legal
text that says GPL v2, as you are adding the SPDX tag.
On Sat, Nov 10, 2018 at 11:23 AM Fabio Estevam <[email protected]> wrote:>
> Hi Matheus,
>
> On Fri, Nov 9, 2018 at 10:27 PM Matheus Tavares Bernardino
> <[email protected]> wrote:
>
> > Got it, thanks for the explanation! I'll correct this in v2.
>
> One more suggestion: in v2 you could also consider to remove the legal
> text that says GPL v2, as you are adding the SPDX tag.
Okay, I'll do it! Thanks again for the review and suggestions!
Matheus
On Fri, 9 Nov 2018 20:00:44 -0200
Matheus Tavares <[email protected]> wrote:
> Move ad2s90 resolver driver out of staging to the main tree.
>
> Signed-off-by: Matheus Tavares <[email protected]>
> Signed-off-by: Victor Colombo <[email protected]>
For a move out of staging patch, please disable move detection.
It let's us see the whole driver and perform a thorough review on list.
Note this is the only case I'm aware of where move detection should
be disabled. I'm not sure if others have the same feeling for
such patches, but in IIO I always want to see what we are actually
moving!
Normally we then review it as if it were a new incoming driver.
That can pick up on stuff that has previously been missed.
Thanks,
Jonathan
> ---
> drivers/iio/resolver/Kconfig | 10 ++++++++++
> drivers/iio/resolver/Makefile | 1 +
> drivers/{staging => }/iio/resolver/ad2s90.c | 0
> drivers/staging/iio/resolver/Kconfig | 10 ----------
> drivers/staging/iio/resolver/Makefile | 1 -
> 5 files changed, 11 insertions(+), 11 deletions(-)
> rename drivers/{staging => }/iio/resolver/ad2s90.c (100%)
>
> diff --git a/drivers/iio/resolver/Kconfig b/drivers/iio/resolver/Kconfig
> index 2ced9f22aa70..786801be54f6 100644
> --- a/drivers/iio/resolver/Kconfig
> +++ b/drivers/iio/resolver/Kconfig
> @@ -3,6 +3,16 @@
> #
> menu "Resolver to digital converters"
>
> +config AD2S90
> + tristate "Analog Devices ad2s90 driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices spi resolver
> + to digital converters, ad2s90, provides direct access via sysfs.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad2s90.
> +
> config AD2S1200
> tristate "Analog Devices ad2s1200/ad2s1205 driver"
> depends on SPI
> diff --git a/drivers/iio/resolver/Makefile b/drivers/iio/resolver/Makefile
> index 4e1dccae07e7..398d82d50028 100644
> --- a/drivers/iio/resolver/Makefile
> +++ b/drivers/iio/resolver/Makefile
> @@ -2,4 +2,5 @@
> # Makefile for Resolver/Synchro drivers
> #
>
> +obj-$(CONFIG_AD2S90) += ad2s90.o
> obj-$(CONFIG_AD2S1200) += ad2s1200.o
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/iio/resolver/ad2s90.c
> similarity index 100%
> rename from drivers/staging/iio/resolver/ad2s90.c
> rename to drivers/iio/resolver/ad2s90.c
> diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig
> index 6a469ee6101f..4a727c17bb8f 100644
> --- a/drivers/staging/iio/resolver/Kconfig
> +++ b/drivers/staging/iio/resolver/Kconfig
> @@ -3,16 +3,6 @@
> #
> menu "Resolver to digital converters"
>
> -config AD2S90
> - tristate "Analog Devices ad2s90 driver"
> - depends on SPI
> - help
> - Say yes here to build support for Analog Devices spi resolver
> - to digital converters, ad2s90, provides direct access via sysfs.
> -
> - To compile this driver as a module, choose M here: the
> - module will be called ad2s90.
> -
> config AD2S1210
> tristate "Analog Devices ad2s1210 driver"
> depends on SPI
> diff --git a/drivers/staging/iio/resolver/Makefile b/drivers/staging/iio/resolver/Makefile
> index 8d901dc7500b..b2049f2ce36e 100644
> --- a/drivers/staging/iio/resolver/Makefile
> +++ b/drivers/staging/iio/resolver/Makefile
> @@ -2,5 +2,4 @@
> # Makefile for Resolver/Synchro drivers
> #
>
> -obj-$(CONFIG_AD2S90) += ad2s90.o
> obj-$(CONFIG_AD2S1210) += ad2s1210.o
On Fri, 9 Nov 2018 20:00:38 -0200
Matheus Tavares <[email protected]> wrote:
> This patch set adds device tree support to ad2s90, with standard
> device tree id table, adds the respective dt-binding documentation,
> solves a codestyle warning and move the driver out of staging.
>
> This patch set completes all the remaining itens listed to be done
> before moving the driver out of staging, enumerated in this mail thread:
> https://marc.info/?l=linux-iio&m=154028966111330&w=2, except by one
> codestyle problem: "CHECK: struct mutex definition without comment". It
> seems to be a commonly ignored check for mutexes of device states. If I
> am wrong, please, let me know and I will be happy to send a patch to
> tackle it.
It should be commented. Device state is not actually all that
well defined and means different things in different drivers.
Here it is very straight forward as it's role is to protect the
buffer. There is no other state maintained.
Jonathan
>
> Matheus Tavares (6):
> staging:iio:ad2s90: Add device tree support
> staging:iio:ad2s90: Remove spi setup that should be done via dt
> staging:iio:ad2s90: Add max frequency check at probe
> dt-bindings:iio:resolver: Add docs for ad2s90
> staging:iio:ad2s90: Add SPDX license identifier
> staging:iio:ad2s90: Move out of staging
>
> .../bindings/iio/resolver/ad2s90.txt | 26 ++++++++++++++++
> drivers/iio/resolver/Kconfig | 10 ++++++
> drivers/iio/resolver/Makefile | 1 +
> drivers/{staging => }/iio/resolver/ad2s90.c | 31 ++++++++++++-------
> drivers/staging/iio/resolver/Kconfig | 10 ------
> drivers/staging/iio/resolver/Makefile | 1 -
> 6 files changed, 57 insertions(+), 22 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> rename drivers/{staging => }/iio/resolver/ad2s90.c (81%)
>
On Fri, 9 Nov 2018 20:00:40 -0200
Matheus Tavares <[email protected]> wrote:
> The ad2s90 driver currently sets some spi settings (max_speed_hz and
> mode) at ad2s90_probe. This should, instead, be handled via device tree.
> This patch removes these configurations from the probe function.
>
> Note: The way in which the mentioned spi settings need to be specified
> on the ad2s90's node of a device tree will be documented in the future
> patch "dt-bindings:iio:resolver: Add docs for ad2s90".
>
> Signed-off-by: Matheus Tavares <[email protected]>
I'd actually like to get Rob and Mark's views on this one. Previously
I would just have applied it without thinking on the basis these can
be easily specified from devicetree.
Recent discussions with Rob have suggested that the settings in devicetree
should perhaps be concerned with specifying constraints about the device
that are not visible to the driver. The driver itself should apply
the device constraints, but there are others such as wiring that
might reduce the maximum frequency for example...
So should a driver be clamping an over specified value from DT for
example? Or given that is optional in DT, should it be making sure
that a controller max frequency isn't too high for the sensor?
It seems to be unusual to do this, but to my mind it would make
sense and might be worth pushing out into more drivers.
Jonathan
> ---
> drivers/staging/iio/resolver/ad2s90.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index ff32ca76ca00..95c118c48400 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -77,7 +77,6 @@ static int ad2s90_probe(struct spi_device *spi)
> {
> struct iio_dev *indio_dev;
> struct ad2s90_state *st;
> - int ret;
>
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> @@ -94,16 +93,6 @@ static int ad2s90_probe(struct spi_device *spi)
> indio_dev->num_channels = 1;
> indio_dev->name = spi_get_device_id(spi)->name;
>
> - /* need 600ns between CS and the first falling edge of SCLK */
> - spi->max_speed_hz = 830000;
> - spi->mode = SPI_MODE_3;
> - ret = spi_setup(spi);
> -
> - if (ret < 0) {
> - dev_err(&spi->dev, "spi_setup failed!\n");
> - return ret;
> - }
> -
> return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> }
>
On Fri, 9 Nov 2018 20:00:41 -0200
Matheus Tavares <[email protected]> wrote:
> This patch adds a max frequency check at the beginning of ad2s90_probe
> function so that when it is set to a value above 0.83Mhz, dev_err is
> called with an appropriate message and -EINVAL is returned.
>
> The defined limit is 0.83Mhz instead of 2Mhz, which is the chip's max
> frequency as specified in the datasheet, because, as also specified in
> the datasheet, a 600ns delay is expected between the application of a
> logic LO to CS and the application of SCLK. Since the delay is not
> implemented in the spi code, to satisfy it, SCLK's period should be at
> most 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which
> gives roughly 830000Hz.
>
> Signed-off-by: Matheus Tavares <[email protected]>
> Signed-off-by: Alexandru Ardelean <[email protected]>
> ---
> Alex's S-o-B was added because he gave the code suggestion for this
> patch.
If he gave the actual code then he should be the credited author
git commit --amend --author=...
If it was just a suggestion, then an informal tag such as
Suggested-by is usually used.
Signed-off-by has formal legal meaning to do with the developer
certificate of origin, it's not valid as a way of crediting someone
with a contribution to the patch.
>
> drivers/staging/iio/resolver/ad2s90.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> index 95c118c48400..949ff55ac6b0 100644
> --- a/drivers/staging/iio/resolver/ad2s90.c
> +++ b/drivers/staging/iio/resolver/ad2s90.c
> @@ -19,6 +19,12 @@
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
>
> +/*
> + * Although chip's max frequency is 2Mhz, it needs 600ns between CS and the
> + * first falling edge of SCLK, so frequency should be at most 1 / (2 * 6e-7)
> + */
> +#define AD2S90_MAX_SPI_FREQ_HZ 830000
> +
> struct ad2s90_state {
> struct mutex lock;
> struct spi_device *sdev;
> @@ -78,6 +84,12 @@ static int ad2s90_probe(struct spi_device *spi)
> struct iio_dev *indio_dev;
> struct ad2s90_state *st;
>
> + if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
> + dev_err(&spi->dev, "SPI CLK, %d Hz exceeds %d Hz\n",
> + spi->max_speed_hz, AD2S90_MAX_SPI_FREQ_HZ);
> + return -EINVAL;
> + }
Now this is interesting as a follow up to the previous and may actually
answer the question I raised. Let's see what Mark and Rob come
back with. If possible I'd like us to resolve this once and for all
with a thread to point people back to!
> +
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> return -ENOMEM;
On Fri, 9 Nov 2018 20:00:42 -0200
Matheus Tavares <[email protected]> wrote:
> This patch adds the device tree binding documentation for the ad2s90
> resolver-to-digital converter.
>
> Signed-off-by: Matheus Tavares <[email protected]>
> ---
> .../bindings/iio/resolver/ad2s90.txt | 26 +++++++++++++++++++
> 1 file changed, 26 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> new file mode 100644
> index 000000000000..b42cc7752ffd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> @@ -0,0 +1,26 @@
> +Analog Devices AD2S90 Resolver-to-Digital Converter
> +
> +https://www.analog.com/en/products/ad2s90.html
> +
> +Required properties:
> + - compatible : should be "adi,ad2s90"
> + - reg : SPI chip select number for the device
> + - spi-max-frequency : set maximum clock frequency, must be 830000
> + - spi-cpol and spi-cpha : must be defined to enable SPI mode 3
As the part only works in mode 3, my gut feeling is that this belongs
in the driver, not here. Rob, what do you think?
> +
> +Note about max frequency:
> + Chip's max frequency, as specified in its datasheet, is 2Mhz. But a 600ns
> + delay is expected between the application of a logic LO to CS and the
> + application of SCLK, as also specified. And since the delay is not
> + implemented in the spi code, to satisfy it, SCLK's period should be at most
> + 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which gives
> + roughly 830000Hz.
> +
> +Example:
> +resolver@0 {
> + compatible = "adi,ad2s90";
> + reg = <0>;
> + spi-max-frequency = <830000>;
> + spi-cpol;
> + spi-cpha;
> +};
On Sun, Nov 11, 2018 at 9:42 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 9 Nov 2018 20:00:40 -0200
> Matheus Tavares <[email protected]> wrote:
>
> > The ad2s90 driver currently sets some spi settings (max_speed_hz and
> > mode) at ad2s90_probe. This should, instead, be handled via device tree.
> > This patch removes these configurations from the probe function.
> >
> > Note: The way in which the mentioned spi settings need to be specified
> > on the ad2s90's node of a device tree will be documented in the future
> > patch "dt-bindings:iio:resolver: Add docs for ad2s90".
> >
> > Signed-off-by: Matheus Tavares <[email protected]>
> I'd actually like to get Rob and Mark's views on this one. Previously
> I would just have applied it without thinking on the basis these can
> be easily specified from devicetree.
>
> Recent discussions with Rob have suggested that the settings in devicetree
> should perhaps be concerned with specifying constraints about the device
> that are not visible to the driver. The driver itself should apply
> the device constraints, but there are others such as wiring that
> might reduce the maximum frequency for example...
>
> So should a driver be clamping an over specified value from DT for
> example? Or given that is optional in DT, should it be making sure
> that a controller max frequency isn't too high for the sensor?
>
First of all, thanks for the review and comments.
By what you've said here and in the reviews for patches 3 and 4 of
this patch-set, it seems to me that the most reasonable thing would be
to keep the SPI mode 3 settings at the driver but the max frequency
setting at DT and check if it exceeds the maximum at the driver (as
patch 3 does). This makes sense to me, based on what you've said,
because mode 3 is a device constraint visible to the driver (as it
won't change) but max frequency is not (because of things such as
wiring, as you said).
What do you think, Jonathan, Rob, and Mark?
Matheus
> It seems to be unusual to do this, but to my mind it would make
> sense and might be worth pushing out into more drivers.
>
> Jonathan
> > ---
> > drivers/staging/iio/resolver/ad2s90.c | 11 -----------
> > 1 file changed, 11 deletions(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> > index ff32ca76ca00..95c118c48400 100644
> > --- a/drivers/staging/iio/resolver/ad2s90.c
> > +++ b/drivers/staging/iio/resolver/ad2s90.c
> > @@ -77,7 +77,6 @@ static int ad2s90_probe(struct spi_device *spi)
> > {
> > struct iio_dev *indio_dev;
> > struct ad2s90_state *st;
> > - int ret;
> >
> > indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > if (!indio_dev)
> > @@ -94,16 +93,6 @@ static int ad2s90_probe(struct spi_device *spi)
> > indio_dev->num_channels = 1;
> > indio_dev->name = spi_get_device_id(spi)->name;
> >
> > - /* need 600ns between CS and the first falling edge of SCLK */
> > - spi->max_speed_hz = 830000;
> > - spi->mode = SPI_MODE_3;
> > - ret = spi_setup(spi);
> > -
> > - if (ret < 0) {
> > - dev_err(&spi->dev, "spi_setup failed!\n");
> > - return ret;
> > - }
> > -
> > return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > }
> >
>
On Thu, 15 Nov 2018 12:44:39 -0200
Matheus Tavares Bernardino <[email protected]> wrote:
> On Sun, Nov 11, 2018 at 9:42 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Fri, 9 Nov 2018 20:00:40 -0200
> > Matheus Tavares <[email protected]> wrote:
> >
> > > The ad2s90 driver currently sets some spi settings (max_speed_hz and
> > > mode) at ad2s90_probe. This should, instead, be handled via device tree.
> > > This patch removes these configurations from the probe function.
> > >
> > > Note: The way in which the mentioned spi settings need to be specified
> > > on the ad2s90's node of a device tree will be documented in the future
> > > patch "dt-bindings:iio:resolver: Add docs for ad2s90".
> > >
> > > Signed-off-by: Matheus Tavares <[email protected]>
> > I'd actually like to get Rob and Mark's views on this one. Previously
> > I would just have applied it without thinking on the basis these can
> > be easily specified from devicetree.
> >
> > Recent discussions with Rob have suggested that the settings in devicetree
> > should perhaps be concerned with specifying constraints about the device
> > that are not visible to the driver. The driver itself should apply
> > the device constraints, but there are others such as wiring that
> > might reduce the maximum frequency for example...
> >
> > So should a driver be clamping an over specified value from DT for
> > example? Or given that is optional in DT, should it be making sure
> > that a controller max frequency isn't too high for the sensor?
> >
>
> First of all, thanks for the review and comments.
>
> By what you've said here and in the reviews for patches 3 and 4 of
> this patch-set, it seems to me that the most reasonable thing would be
> to keep the SPI mode 3 settings at the driver but the max frequency
> setting at DT and check if it exceeds the maximum at the driver (as
> patch 3 does). This makes sense to me, based on what you've said,
> because mode 3 is a device constraint visible to the driver (as it
> won't change) but max frequency is not (because of things such as
> wiring, as you said).
>
> What do you think, Jonathan, Rob, and Mark?
Sounds good to me. I just checked the DT bindings for spi-bus
and max-frequency is indeed a required binding element for slave
devices, hence has to be there. Best to confirm it is sane in
the driver however as you suggest. I think we'll standardise
on that bit of paranoia in IIO unless Rob or Mark shouts otherwise.
Jonathan
>
> Matheus
>
> > It seems to be unusual to do this, but to my mind it would make
> > sense and might be worth pushing out into more drivers.
> >
> > Jonathan
> > > ---
> > > drivers/staging/iio/resolver/ad2s90.c | 11 -----------
> > > 1 file changed, 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/resolver/ad2s90.c b/drivers/staging/iio/resolver/ad2s90.c
> > > index ff32ca76ca00..95c118c48400 100644
> > > --- a/drivers/staging/iio/resolver/ad2s90.c
> > > +++ b/drivers/staging/iio/resolver/ad2s90.c
> > > @@ -77,7 +77,6 @@ static int ad2s90_probe(struct spi_device *spi)
> > > {
> > > struct iio_dev *indio_dev;
> > > struct ad2s90_state *st;
> > > - int ret;
> > >
> > > indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > if (!indio_dev)
> > > @@ -94,16 +93,6 @@ static int ad2s90_probe(struct spi_device *spi)
> > > indio_dev->num_channels = 1;
> > > indio_dev->name = spi_get_device_id(spi)->name;
> > >
> > > - /* need 600ns between CS and the first falling edge of SCLK */
> > > - spi->max_speed_hz = 830000;
> > > - spi->mode = SPI_MODE_3;
> > > - ret = spi_setup(spi);
> > > -
> > > - if (ret < 0) {
> > > - dev_err(&spi->dev, "spi_setup failed!\n");
> > > - return ret;
> > > - }
> > > -
> > > return devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> > > }
> > >
> >
On Sun, Nov 11, 2018 at 9:48 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 9 Nov 2018 20:00:42 -0200
> Matheus Tavares <[email protected]> wrote:
>
> > This patch adds the device tree binding documentation for the ad2s90
> > resolver-to-digital converter.
> >
> > Signed-off-by: Matheus Tavares <[email protected]>
> > ---
> > .../bindings/iio/resolver/ad2s90.txt | 26 +++++++++++++++++++
> > 1 file changed, 26 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> >
> > diff --git a/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> > new file mode 100644
> > index 000000000000..b42cc7752ffd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/resolver/ad2s90.txt
> > @@ -0,0 +1,26 @@
> > +Analog Devices AD2S90 Resolver-to-Digital Converter
> > +
> > +https://www.analog.com/en/products/ad2s90.html
> > +
> > +Required properties:
> > + - compatible : should be "adi,ad2s90"
> > + - reg : SPI chip select number for the device
> > + - spi-max-frequency : set maximum clock frequency, must be 830000
> > + - spi-cpol and spi-cpha : must be defined to enable SPI mode 3
>
> As the part only works in mode 3, my gut feeling is that this belongs
> in the driver, not here. Rob, what do you think?
>
For this patch, I assumed the part only worked in mode 3 based on the
driver's code that set this at probe. But today I carefully looked for
it at the datasheet and now I'm unsure. It is never said, explicitly,
which SPI mode ad2s90 works with. But looking at the diagram that
shows the expected pins signals at each communication moment, it seems
to me that this chip can either work in mode 0 (CPOL=0, CPHA=0) or
mode 3 (CPOL=1, CPHA=1). Could someone help me to confirm this? And if
that is the case, them the SPI mode setting should be left in DT, as
adc/mcp320x and dac/ti-dac082s085 do, right?
Also, when I thought that ad2s90 only worked in mode 3, I wrote this
patch based on the dt-binding docs for the adxl345 accelerometer,
which only works in mode 3 but lets this setting to DT not in the
driver. Do you think, perhaps, it is wrong in adxl345, them?
Thanks,
Matheus.
> > +
> > +Note about max frequency:
> > + Chip's max frequency, as specified in its datasheet, is 2Mhz. But a 600ns
> > + delay is expected between the application of a logic LO to CS and the
> > + application of SCLK, as also specified. And since the delay is not
> > + implemented in the spi code, to satisfy it, SCLK's period should be at most
> > + 2 * 600ns, so the max frequency should be 1 / (2 * 6e-7), which gives
> > + roughly 830000Hz.
> > +
> > +Example:
> > +resolver@0 {
> > + compatible = "adi,ad2s90";
> > + reg = <0>;
> > + spi-max-frequency = <830000>;
> > + spi-cpol;
> > + spi-cpha;
> > +};
>