ad2s1210 fixes and updates by dbogdan · Pull Request #123 · analogdevicesinc/linux

dbogdan · 2018-06-22T06:52:24Z

These changes make the driver usable on AD-FMCMOTCON2-EBZ.

commodo · 2018-06-22T07:07:46Z

checkpatch has a few pertinent complaints

commodo · 2018-06-22T07:14:07Z

drivers/staging/iio/resolver/ad2s1210.c

 #include <linux/sysfs.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>


I'll reference this commit f86b233 since that's where the initial comments were added.
No need to duplicate them here.

commodo · 2018-06-22T07:16:17Z

drivers/staging/iio/resolver/ad2s1210.c

-	gpiod_set_value(st->pdata->a[0], ad2s1210_mode_vals[mode][0]);
-	gpiod_set_value(st->pdata->a[1], ad2s1210_mode_vals[mode][1]);
-	st->mode = mode;
+	if (st->mode != MOD_CONFIG_ONLY) {


this is a nitpick; but another way to do it here is to exit early;
it minimizes the patch size;
so, it could be done as:

if (st->mode == MOD_CONFIG_ONLY) return;

commodo · 2018-06-22T07:18:59Z

drivers/staging/iio/resolver/ad2s1210.c

-	return ad2s1210_resolution_value[
-		(gpiod_get_value(st->pdata->res[0]) << 1) |
-		gpiod_get_value(st->pdata->res[1])];
+	int res0 = 0;


this could exit early with

if (st->mode == MOD_CONFIG_ONLY) return ad2s1210_resolution_value[0];

commodo · 2018-06-22T07:21:00Z

drivers/staging/iio/resolver/ad2s1210.c

+	int index;
+
+	if (st->mode != MOD_CONFIG_ONLY) {
+		index = (st->resolution - 10) / 2;


i like the caching of the calculated value here;
so, i guess this could be left as is here [with respect to exiting early, it should be fine as is];

commodo · 2018-06-22T07:25:28Z

drivers/staging/iio/resolver/ad2s1210.c

+		if (st->mode != MOD_CONFIG_ONLY)
+			ad2s1210_set_mode(MOD_POS, st);
+		else {
+			st->rx[0] = ad2s1210_config_read(st,


you could assign a reg = AD2S1210_REG_POSITION here and reg = AD2S1210_REG_VELOCITY
and do below:

if (st->mode != MOD_CONFIG_ONLY) { ret = spi_read(st->sdev, st->rx, 2); if (ret < 0) goto error_ret; } else { st->rx[0] = ad2s1210_config_read(st, reg); st->rx[1] = ad2s1210_config_read(st, reg + 1); }

dbogdan · 2018-06-22T07:27:08Z

My fault - it seems that if 2 commits are pushed, checkpatch is run only on the second one... I'll do the fixes.

commodo · 2018-06-22T07:45:44Z

A bit related to this series; should we merge the ad2s1210.h file/struct into the ad2s1210.c file ?
Do we want to keep supporting external platform data in this tree ?

dbogdan · 2018-06-22T07:56:15Z

This is a good question. @larsclausen ?

stefpopa · 2018-06-22T08:00:46Z

drivers/staging/iio/resolver/ad2s1210.c

+	if (ret < 0)
+		return ret;
+
+	return st->rx[0];


I am thinking if this can be done with a single spi_sync_transfer()?

It can be for sure if an array of spi_transfers is used. Each xfer should be initialized in this case, so I decided to simply call spi_sync_transfer() twice.
I'm not really against any of these two options...

A advantage of a single transfer is that it will be locked. If you split it into two transfers other devices might squeeze their own transfers in between.

I'll do the update.

lclausen-adi · 2018-06-22T08:12:57Z

@dbogdan Your choice

lclausen-adi · 2018-06-22T08:14:30Z

drivers/staging/iio/resolver/ad2s1210.c

+		return NULL;
+
+	pdata->gpioin = of_property_read_bool(np,
+				"adi,gpios-input-direction-enable");


Whenever you have a DT property that ends in a verb you've lost the game. The DT is descriptive not imperative.

Think "GPIOs are wired as inputs" and not "GPIOs should be configured as inputs".

dbogdan · 2018-06-22T11:26:47Z

v2:

move GPIO decriptors from platform_data to state;
optimize ad2s1210_setup_gpios() and use gpiod_get_index();
exit earlier from the specific functions if MOD_CONFIG_ONLY is used;
optimize ad2s1210_read_raw();
rename dts attribute.

dbogdan · 2018-06-22T11:59:03Z

v3:

use an array of spi_transfers instead of calling spi_sync_transfer() twice.

commodo

LGTM
Will merge Monday or Tuesday if no objections

No functional changes. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

According to AD2S1210 datasheet, if the serial interface is used, the 8-bit address should be latched using the rising edge of the WR/FSYNC signal. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

The descriptor-based GPIO interface is a safer alternative to the legacy GPIO interface. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

The AD2S1210 can be operated entirely in configuration mode. In this mode, the only required GPIO is SAMPLE. The driver will enable MOD_CONFIG_ONLY when any of the other GPIOs is missing. AD-FMCMOTCON2-EBZ evaluation board is one example where AD2S1210 is used in this mode. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

The AD2S1210's driver will automatically enable this mode when SAMPLE is the only GPIO defined. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

CLKIN is independent of SPI SCLK. Its default value is AD2S1210_DEF_CLKIN. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

The part is installed on AD-FMCMOTCON2-EBZ evaluation board. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

dbogdan · 2018-06-24T22:19:36Z

After more testing, v4 appeared. :)
v4:

enable .cs_change for the spi_transfers;
don't set spi->mode if device tree approach is used;
enable the driver in zynq_xcomm_adv7511_defconfig.

dbogdan requested review from commodo, lclausen-adi, mhennerich and stefpopa June 22, 2018 06:52

commodo reviewed Jun 22, 2018

View reviewed changes

stefpopa reviewed Jun 22, 2018

View reviewed changes

lclausen-adi reviewed Jun 22, 2018

View reviewed changes

dbogdan force-pushed the ad2s1210 branch 2 times, most recently from d012e80 to 72bfd03 Compare June 22, 2018 10:31

dbogdan force-pushed the ad2s1210 branch from 72bfd03 to c46e223 Compare June 22, 2018 11:57

commodo approved these changes Jun 22, 2018

View reviewed changes

stefpopa approved these changes Jun 22, 2018

View reviewed changes

staging: iio: ad2s1210: Remove unused variable

182dd70

No functional changes. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

dbogdan force-pushed the ad2s1210 branch from 4ce9146 to d2eb86d Compare June 24, 2018 21:21

dbogdan added 5 commits June 25, 2018 01:00

staging: iio: ad2s1210: Fix SPI reading

bcbed57

According to AD2S1210 datasheet, if the serial interface is used, the 8-bit address should be latched using the rising edge of the WR/FSYNC signal. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

staging: iio: ad2s1210: Add device tree support

19bb17d

Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

staging: iio: ad2s1210: Switch to descriptor-based GPIO interface

0d121b4

The descriptor-based GPIO interface is a safer alternative to the legacy GPIO interface. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

dts: fmcmotcon2: Remove unused attribute

7973a1f

The AD2S1210's driver will automatically enable this mode when SAMPLE is the only GPIO defined. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

dbogdan added 2 commits June 25, 2018 01:00

staging: iio: ad2s1210: Fix clkin initial value

8495b42

CLKIN is independent of SPI SCLK. Its default value is AD2S1210_DEF_CLKIN. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

configs: zynq_xcomm_adv7511_defconfig: Enable AD2S1210 driver

cb3ea7f

The part is installed on AD-FMCMOTCON2-EBZ evaluation board. Signed-off-by: Dragos Bogdan <dragos.bogdan@analog.com>

dbogdan force-pushed the ad2s1210 branch from d2eb86d to cb3ea7f Compare June 24, 2018 22:00

commodo merged commit 2ca7300 into master Jun 25, 2018

commodo deleted the ad2s1210 branch June 25, 2018 10:55

Conversation

dbogdan commented Jun 22, 2018

Uh oh!

commodo commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbogdan commented Jun 22, 2018

Uh oh!

commodo commented Jun 22, 2018

Uh oh!

dbogdan commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lclausen-adi commented Jun 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbogdan commented Jun 22, 2018

Uh oh!

dbogdan commented Jun 22, 2018

Uh oh!

commodo left a comment

Choose a reason for hiding this comment

Uh oh!

dbogdan commented Jun 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants