* [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging
@ 2018-11-09 22:00 Matheus Tavares
2018-11-09 22:00 ` [PATCH 1/6] staging:iio:ad2s90: Add device tree support Matheus Tavares
` (6 more replies)
0 siblings, 7 replies; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/6] staging:iio:ad2s90: Add device tree support
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
@ 2018-11-09 22:00 ` Matheus Tavares
2018-11-09 22:00 ` [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt Matheus Tavares
` (5 subsequent siblings)
6 siblings, 0 replies; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
This patch adds device tree support to ad2s90 with standard
device tree id table.
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
2018-11-09 22:00 ` [PATCH 1/6] staging:iio:ad2s90: Add device tree support Matheus Tavares
@ 2018-11-09 22:00 ` Matheus Tavares
2018-11-11 11:42 ` Jonathan Cameron
2018-11-09 22:00 ` [PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe Matheus Tavares
` (4 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
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 <matheus.bernardino@usp.br>
---
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
2018-11-09 22:00 ` [PATCH 1/6] staging:iio:ad2s90: Add device tree support Matheus Tavares
2018-11-09 22:00 ` [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt Matheus Tavares
@ 2018-11-09 22:00 ` Matheus Tavares
2018-11-11 11:46 ` Jonathan Cameron
2018-11-09 22:00 ` [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90 Matheus Tavares
` (3 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
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 <matheus.bernardino@usp.br>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
` (2 preceding siblings ...)
2018-11-09 22:00 ` [PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe Matheus Tavares
@ 2018-11-09 22:00 ` Matheus Tavares
2018-11-11 11:48 ` Jonathan Cameron
2018-11-09 22:00 ` [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier Matheus Tavares
` (2 subsequent siblings)
6 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
This patch adds the device tree binding documentation for the ad2s90
resolver-to-digital converter.
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
.../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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
` (3 preceding siblings ...)
2018-11-09 22:00 ` [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90 Matheus Tavares
@ 2018-11-09 22:00 ` Matheus Tavares
2018-11-09 22:13 ` Fabio Estevam
2018-11-09 22:00 ` [PATCH 6/6] staging:iio:ad2s90: Move out of staging Matheus Tavares
2018-11-11 11:34 ` [PATCH 0/6] staging:iio:ad2s90: Add dt support and move " Jonathan Cameron
6 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
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 <matheus.bernardino@usp.br>
---
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 6/6] staging:iio:ad2s90: Move out of staging
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
` (4 preceding siblings ...)
2018-11-09 22:00 ` [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier Matheus Tavares
@ 2018-11-09 22:00 ` Matheus Tavares
2018-11-11 11:31 ` Jonathan Cameron
2018-11-11 11:34 ` [PATCH 0/6] staging:iio:ad2s90: Add dt support and move " Jonathan Cameron
6 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares @ 2018-11-09 22:00 UTC (permalink / raw)
To: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland
Cc: linux-iio, devel, devicetree, linux-kernel, Alexandru Ardelean,
kernel-usp, victorcolombo
Move ad2s90 resolver driver out of staging to the main tree.
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
Signed-off-by: Victor Colombo <victorcolombo@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-09 22:00 ` [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier Matheus Tavares
@ 2018-11-09 22:13 ` Fabio Estevam
2018-11-09 23:19 ` Matheus Tavares Bernardino
0 siblings, 1 reply; 21+ messages in thread
From: Fabio Estevam @ 2018-11-09 22:13 UTC (permalink / raw)
To: matheus.bernardino
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, devel,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-iio, linux-kernel, kernel-usp, alexandru.ardelean,
victorcolombo
Hi Matheus,
On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
<matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> ---
> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-09 22:13 ` Fabio Estevam
@ 2018-11-09 23:19 ` Matheus Tavares Bernardino
2018-11-10 0:20 ` Greg Kroah-Hartman
0 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares Bernardino @ 2018-11-09 23:19 UTC (permalink / raw)
To: festevam
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
Rob Herring, Mark Rutland, devel, devicetree, linux-iio,
linux-kernel, kernel-usp, Alexandru Ardelean, Victor Colombo
On Fri, Nov 9, 2018 at 8:13 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Matheus,
>
Hi, Fabio
> On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
> <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> > ---
> > 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-09 23:19 ` Matheus Tavares Bernardino
@ 2018-11-10 0:20 ` Greg Kroah-Hartman
2018-11-10 0:27 ` Matheus Tavares Bernardino
0 siblings, 1 reply; 21+ messages in thread
From: Greg Kroah-Hartman @ 2018-11-10 0:20 UTC (permalink / raw)
To: Matheus Tavares Bernardino
Cc: festevam, Mark Rutland, devel, Lars-Peter Clausen,
Michael Hennerich, devicetree, linux-iio, linux-kernel,
kernel-usp, Rob Herring, Peter Meerwald-Stadler, Hartmut Knaack,
Alexandru Ardelean, Victor Colombo, Jonathan Cameron
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 <festevam@gmail.com> wrote:
> >
> > Hi Matheus,
> >
>
> Hi, Fabio
>
> > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
> > <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> > > ---
> > > 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-10 0:20 ` Greg Kroah-Hartman
@ 2018-11-10 0:27 ` Matheus Tavares Bernardino
2018-11-10 13:22 ` Fabio Estevam
0 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares Bernardino @ 2018-11-10 0:27 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: festevam, Mark Rutland, devel, Lars-Peter Clausen,
Michael Hennerich, devicetree, linux-iio, linux-kernel,
kernel-usp, Rob Herring, Peter Meerwald-Stadler, Hartmut Knaack,
Alexandru Ardelean, Victor Colombo, Jonathan Cameron
On Fri, Nov 9, 2018 at 10:20 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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 <festevam@gmail.com> wrote:
> > >
> > > Hi Matheus,
> > >
> >
> > Hi, Fabio
> >
> > > On Fri, Nov 9, 2018 at 8:01 PM Matheus Tavares
> > > <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> > > > ---
> > > > 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-10 0:27 ` Matheus Tavares Bernardino
@ 2018-11-10 13:22 ` Fabio Estevam
2018-11-10 18:23 ` Matheus Tavares Bernardino
0 siblings, 1 reply; 21+ messages in thread
From: Fabio Estevam @ 2018-11-10 13:22 UTC (permalink / raw)
To: Matheus Tavares Bernardino
Cc: Greg Kroah-Hartman, Mark Rutland, devel, Lars-Peter Clausen,
Michael Hennerich,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-iio, linux-kernel, kernel-usp, Rob Herring, Peter Meerwald,
Hartmut Knaack, alexandru.ardelean, Victor Colombo,
Jonathan Cameron
Hi Matheus,
On Fri, Nov 9, 2018 at 10:27 PM Matheus Tavares Bernardino
<matheus.bernardino@usp.br> 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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier
2018-11-10 13:22 ` Fabio Estevam
@ 2018-11-10 18:23 ` Matheus Tavares Bernardino
0 siblings, 0 replies; 21+ messages in thread
From: Matheus Tavares Bernardino @ 2018-11-10 18:23 UTC (permalink / raw)
To: Fabio Estevam
Cc: Greg Kroah-Hartman, Mark Rutland, devel, Lars-Peter Clausen,
Michael Hennerich, devicetree, linux-iio, linux-kernel,
kernel-usp, Rob Herring, Peter Meerwald-Stadler, Hartmut Knaack,
Alexandru Ardelean, Victor Colombo, Jonathan Cameron
On Sat, Nov 10, 2018 at 11:23 AM Fabio Estevam <festevam@gmail.com> wrote:>
> Hi Matheus,
>
> On Fri, Nov 9, 2018 at 10:27 PM Matheus Tavares Bernardino
> <matheus.bernardino@usp.br> 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 6/6] staging:iio:ad2s90: Move out of staging
2018-11-09 22:00 ` [PATCH 6/6] staging:iio:ad2s90: Move out of staging Matheus Tavares
@ 2018-11-11 11:31 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2018-11-11 11:31 UTC (permalink / raw)
To: Matheus Tavares
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, victorcolombo
On Fri, 9 Nov 2018 20:00:44 -0200
Matheus Tavares <matheus.bernardino@usp.br> wrote:
> Move ad2s90 resolver driver out of staging to the main tree.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> Signed-off-by: Victor Colombo <victorcolombo@gmail.com>
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
` (5 preceding siblings ...)
2018-11-09 22:00 ` [PATCH 6/6] staging:iio:ad2s90: Move out of staging Matheus Tavares
@ 2018-11-11 11:34 ` Jonathan Cameron
6 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2018-11-11 11:34 UTC (permalink / raw)
To: Matheus Tavares
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, victorcolombo
On Fri, 9 Nov 2018 20:00:38 -0200
Matheus Tavares <matheus.bernardino@usp.br> 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%)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt
2018-11-09 22:00 ` [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt Matheus Tavares
@ 2018-11-11 11:42 ` Jonathan Cameron
2018-11-15 14:44 ` Matheus Tavares Bernardino
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2018-11-11 11:42 UTC (permalink / raw)
To: Matheus Tavares
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, victorcolombo, Mark Brown
On Fri, 9 Nov 2018 20:00:40 -0200
Matheus Tavares <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
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);
> }
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe
2018-11-09 22:00 ` [PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe Matheus Tavares
@ 2018-11-11 11:46 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2018-11-11 11:46 UTC (permalink / raw)
To: Matheus Tavares
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, victorcolombo
On Fri, 9 Nov 2018 20:00:41 -0200
Matheus Tavares <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> 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;
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90
2018-11-09 22:00 ` [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90 Matheus Tavares
@ 2018-11-11 11:48 ` Jonathan Cameron
2018-11-17 3:29 ` Matheus Tavares Bernardino
0 siblings, 1 reply; 21+ messages in thread
From: Jonathan Cameron @ 2018-11-11 11:48 UTC (permalink / raw)
To: Matheus Tavares
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, victorcolombo, broonie
On Fri, 9 Nov 2018 20:00:42 -0200
Matheus Tavares <matheus.bernardino@usp.br> wrote:
> This patch adds the device tree binding documentation for the ad2s90
> resolver-to-digital converter.
>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
> .../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;
> +};
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt
2018-11-11 11:42 ` Jonathan Cameron
@ 2018-11-15 14:44 ` Matheus Tavares Bernardino
2018-11-16 18:37 ` Jonathan Cameron
0 siblings, 1 reply; 21+ messages in thread
From: Matheus Tavares Bernardino @ 2018-11-15 14:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, Victor Colombo, broonie
On Sun, Nov 11, 2018 at 9:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 9 Nov 2018 20:00:40 -0200
> Matheus Tavares <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> 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);
> > }
> >
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt
2018-11-15 14:44 ` Matheus Tavares Bernardino
@ 2018-11-16 18:37 ` Jonathan Cameron
0 siblings, 0 replies; 21+ messages in thread
From: Jonathan Cameron @ 2018-11-16 18:37 UTC (permalink / raw)
To: Matheus Tavares Bernardino
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, Victor Colombo, broonie
On Thu, 15 Nov 2018 12:44:39 -0200
Matheus Tavares Bernardino <matheus.bernardino@usp.br> wrote:
> On Sun, Nov 11, 2018 at 9:42 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 9 Nov 2018 20:00:40 -0200
> > Matheus Tavares <matheus.bernardino@usp.br> 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 <matheus.bernardino@usp.br>
> > 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);
> > > }
> > >
> >
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90
2018-11-11 11:48 ` Jonathan Cameron
@ 2018-11-17 3:29 ` Matheus Tavares Bernardino
0 siblings, 0 replies; 21+ messages in thread
From: Matheus Tavares Bernardino @ 2018-11-17 3:29 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Michael Hennerich, Hartmut Knaack,
Peter Meerwald-Stadler, Greg Kroah-Hartman, Rob Herring,
Mark Rutland, linux-iio, devel, devicetree, linux-kernel,
Alexandru Ardelean, kernel-usp, Victor Colombo, broonie
On Sun, Nov 11, 2018 at 9:48 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 9 Nov 2018 20:00:42 -0200
> Matheus Tavares <matheus.bernardino@usp.br> wrote:
>
> > This patch adds the device tree binding documentation for the ad2s90
> > resolver-to-digital converter.
> >
> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> > ---
> > .../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;
> > +};
>
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2018-11-17 3:29 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-09 22:00 [PATCH 0/6] staging:iio:ad2s90: Add dt support and move out of staging Matheus Tavares
2018-11-09 22:00 ` [PATCH 1/6] staging:iio:ad2s90: Add device tree support Matheus Tavares
2018-11-09 22:00 ` [PATCH 2/6] staging:iio:ad2s90: Remove spi setup that should be done via dt Matheus Tavares
2018-11-11 11:42 ` Jonathan Cameron
2018-11-15 14:44 ` Matheus Tavares Bernardino
2018-11-16 18:37 ` Jonathan Cameron
2018-11-09 22:00 ` [PATCH 3/6] staging:iio:ad2s90: Add max frequency check at probe Matheus Tavares
2018-11-11 11:46 ` Jonathan Cameron
2018-11-09 22:00 ` [PATCH 4/6] dt-bindings:iio:resolver: Add docs for ad2s90 Matheus Tavares
2018-11-11 11:48 ` Jonathan Cameron
2018-11-17 3:29 ` Matheus Tavares Bernardino
2018-11-09 22:00 ` [PATCH 5/6] staging:iio:ad2s90: Add SPDX license identifier Matheus Tavares
2018-11-09 22:13 ` Fabio Estevam
2018-11-09 23:19 ` Matheus Tavares Bernardino
2018-11-10 0:20 ` Greg Kroah-Hartman
2018-11-10 0:27 ` Matheus Tavares Bernardino
2018-11-10 13:22 ` Fabio Estevam
2018-11-10 18:23 ` Matheus Tavares Bernardino
2018-11-09 22:00 ` [PATCH 6/6] staging:iio:ad2s90: Move out of staging Matheus Tavares
2018-11-11 11:31 ` Jonathan Cameron
2018-11-11 11:34 ` [PATCH 0/6] staging:iio:ad2s90: Add dt support and move " Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).