[PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

[PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

[Alexandru Ardelean]

This change is done in preparation of adding an `struct adis_timeout` type.
Some ADIS drivers support multiple drivers, with various combinations of
timeouts. Creating static tables for each driver is possible, but that also
creates quite a lot of duplication.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/gyro/adis16136.c | 49 ++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index f10c4f173898..129de2bd5845 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -465,24 +465,6 @@ static const char * const adis16136_status_error_msgs[] = {
 	[ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL] = "Flash checksum error",
 };
 
-static const struct adis_data adis16136_data = {
-	.diag_stat_reg = ADIS16136_REG_DIAG_STAT,
-	.glob_cmd_reg = ADIS16136_REG_GLOB_CMD,
-	.msc_ctrl_reg = ADIS16136_REG_MSC_CTRL,
-
-	.self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST,
-	.startup_delay = 80,
-
-	.read_delay = 10,
-	.write_delay = 10,
-
-	.status_error_msgs = adis16136_status_error_msgs,
-	.status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
-		BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
-		BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
-		BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL),
-};
-
 enum adis16136_id {
 	ID_ADIS16133,
 	ID_ADIS16135,
@@ -509,11 +491,36 @@ static const struct adis16136_chip_info adis16136_chip_info[] = {
 	},
 };
 
+static struct adis_data *adis16136_adis_data_alloc(struct adis16136 *st,
+						   struct device *dev)
+{
+	struct adis_data *data;
+
+	data = devm_kzalloc(dev, sizeof(struct adis_data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	data->msc_ctrl_reg = ADIS16136_REG_MSC_CTRL;
+	data->glob_cmd_reg = ADIS16136_REG_GLOB_CMD;
+	data->diag_stat_reg = ADIS16136_REG_DIAG_STAT;
+	data->self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST;
+	data->read_delay = 10;
+	data->write_delay = 10;
+	data->status_error_msgs = adis16136_status_error_msgs;
+	data->status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
+				BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
+				BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
+				BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL);
+
+	return data;
+}
+
 static int adis16136_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
 	struct adis16136 *adis16136;
 	struct iio_dev *indio_dev;
+	const struct adis_data *adis16136_data;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adis16136));
@@ -532,7 +539,11 @@ static int adis16136_probe(struct spi_device *spi)
 	indio_dev->info = &adis16136_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = adis_init(&adis16136->adis, indio_dev, spi, &adis16136_data);
+	adis16136_data = adis16136_adis_data_alloc(adis16136, &spi->dev);
+	if (IS_ERR(adis16136_data))
+		return PTR_ERR(adis16136_data);
+
+	ret = adis_init(&adis16136->adis, indio_dev, spi, adis16136_data);
 	if (ret)
 		return ret;
 
-- 
2.20.1

[PATCH 2/5] iio: imu: adis16400: construct adis data on probe vs static on driver

[Alexandru Ardelean]

This change is done in preparation of adding an `struct adis_timeout` type.
Some ADIS drivers support multiple drivers, with various combinations of
timeouts. Creating static tables for each driver is possible, but that also
creates quite a lot of duplication.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16400.c | 71 +++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 30 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 662cb5367c11..d77bd6792437 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -1077,35 +1077,6 @@ static const char * const adis16400_status_error_msgs[] = {
 	[ADIS16400_DIAG_STAT_POWER_LOW] = "Power supply below 4.75V",
 };
 
-static const struct adis_data adis16400_data = {
-	.msc_ctrl_reg = ADIS16400_MSC_CTRL,
-	.glob_cmd_reg = ADIS16400_GLOB_CMD,
-	.diag_stat_reg = ADIS16400_DIAG_STAT,
-
-	.read_delay = 50,
-	.write_delay = 50,
-
-	.self_test_mask = ADIS16400_MSC_CTRL_MEM_TEST,
-	.startup_delay = ADIS16400_STARTUP_DELAY,
-
-	.status_error_msgs = adis16400_status_error_msgs,
-	.status_error_mask = BIT(ADIS16400_DIAG_STAT_ZACCL_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_YACCL_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_XACCL_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_XGYRO_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_YGYRO_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_ZGYRO_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_ALARM2) |
-		BIT(ADIS16400_DIAG_STAT_ALARM1) |
-		BIT(ADIS16400_DIAG_STAT_FLASH_CHK) |
-		BIT(ADIS16400_DIAG_STAT_SELF_TEST) |
-		BIT(ADIS16400_DIAG_STAT_OVERFLOW) |
-		BIT(ADIS16400_DIAG_STAT_SPI_FAIL) |
-		BIT(ADIS16400_DIAG_STAT_FLASH_UPT) |
-		BIT(ADIS16400_DIAG_STAT_POWER_HIGH) |
-		BIT(ADIS16400_DIAG_STAT_POWER_LOW),
-};
-
 static void adis16400_setup_chan_mask(struct adis16400_state *st)
 {
 	const struct adis16400_chip_info *chip_info = st->variant;
@@ -1120,11 +1091,47 @@ static void adis16400_setup_chan_mask(struct adis16400_state *st)
 	}
 }
 
+static struct adis_data *adis16400_adis_data_alloc(struct adis16400_state *st,
+						   struct device *dev)
+{
+	struct adis_data *data;
+
+	data = devm_kzalloc(dev, sizeof(struct adis_data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	data->msc_ctrl_reg = ADIS16400_MSC_CTRL;
+	data->glob_cmd_reg = ADIS16400_GLOB_CMD;
+	data->diag_stat_reg = ADIS16400_DIAG_STAT;
+	data->read_delay = 50;
+	data->write_delay = 50;
+	data->self_test_mask = ADIS16400_MSC_CTRL_MEM_TEST;
+	data->status_error_msgs = adis16400_status_error_msgs;
+	data->status_error_mask = BIT(ADIS16400_DIAG_STAT_ZACCL_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_YACCL_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_XACCL_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_XGYRO_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_YGYRO_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_ZGYRO_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_ALARM2) |
+				BIT(ADIS16400_DIAG_STAT_ALARM1) |
+				BIT(ADIS16400_DIAG_STAT_FLASH_CHK) |
+				BIT(ADIS16400_DIAG_STAT_SELF_TEST) |
+				BIT(ADIS16400_DIAG_STAT_OVERFLOW) |
+				BIT(ADIS16400_DIAG_STAT_SPI_FAIL) |
+				BIT(ADIS16400_DIAG_STAT_FLASH_UPT) |
+				BIT(ADIS16400_DIAG_STAT_POWER_HIGH) |
+				BIT(ADIS16400_DIAG_STAT_POWER_LOW);
+
+	return data;
+}
+
 static int adis16400_probe(struct spi_device *spi)
 {
 	struct adis16400_state *st;
 	struct iio_dev *indio_dev;
 	int ret;
+	const struct adis_data *adis16400_data;
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (indio_dev == NULL)
@@ -1151,7 +1158,11 @@ static int adis16400_probe(struct spi_device *spi)
 			st->adis.burst->extra_len = sizeof(u16);
 	}
 
-	ret = adis_init(&st->adis, indio_dev, spi, &adis16400_data);
+	adis16400_data = adis16400_adis_data_alloc(st, &spi->dev);
+	if (IS_ERR(adis16400_data))
+		return PTR_ERR(adis16400_data);
+
+	ret = adis_init(&st->adis, indio_dev, spi, adis16400_data);
 	if (ret)
 		return ret;
 
-- 
2.20.1

[PATCH 3/5] iio: imu: adis16480: construct adis data on probe vs static on driver

[Alexandru Ardelean]

This change is done in preparation of adding an `struct adis_timeout` type.
Some ADIS drivers support multiple drivers, with various combinations of
timeouts. Creating static tables for each driver is possible, but that also
creates quite a lot of duplication.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/imu/adis16480.c | 61 ++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index f73094e8d35d..1edfed83d480 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -1022,29 +1022,6 @@ static const char * const adis16480_status_error_msgs[] = {
 	[ADIS16480_DIAG_STAT_BARO_FAIL] = "Barometer self-test failure",
 };
 
-static const struct adis_data adis16480_data = {
-	.diag_stat_reg = ADIS16480_REG_DIAG_STS,
-	.glob_cmd_reg = ADIS16480_REG_GLOB_CMD,
-	.has_paging = true,
-
-	.read_delay = 5,
-	.write_delay = 5,
-
-	.status_error_msgs = adis16480_status_error_msgs,
-	.status_error_mask = BIT(ADIS16480_DIAG_STAT_XGYRO_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_YGYRO_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_ZGYRO_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_XACCL_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_YACCL_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_ZACCL_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_XMAGN_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_YMAGN_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_ZMAGN_FAIL) |
-		BIT(ADIS16480_DIAG_STAT_BARO_FAIL),
-
-	.enable_irq = adis16480_enable_irq,
-};
-
 static int adis16480_config_irq_pin(struct device_node *of_node,
 				    struct adis16480 *st)
 {
@@ -1195,9 +1172,41 @@ static int adis16480_get_ext_clocks(struct adis16480 *st)
 	return 0;
 }
 
+static struct adis_data *adis16480_adis_data_alloc(struct adis16480 *st,
+						   struct device *dev)
+{
+	struct adis_data *data;
+
+	data = devm_kzalloc(dev, sizeof(struct adis_data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	data->glob_cmd_reg = ADIS16480_REG_GLOB_CMD;
+	data->diag_stat_reg = ADIS16480_REG_DIAG_STS;
+	data->has_paging = true;
+	data->self_test_mask = BIT(1);
+	data->read_delay = 5;
+	data->write_delay = 5;
+	data->status_error_msgs = adis16480_status_error_msgs;
+	data->status_error_mask = BIT(ADIS16480_DIAG_STAT_XGYRO_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_YGYRO_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_ZGYRO_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_XACCL_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_YACCL_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_ZACCL_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_XMAGN_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_YMAGN_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_ZMAGN_FAIL) |
+				BIT(ADIS16480_DIAG_STAT_BARO_FAIL);
+	data->enable_irq = adis16480_enable_irq;
+
+	return data;
+}
+
 static int adis16480_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
+	const struct adis_data *adis16480_data;
 	struct iio_dev *indio_dev;
 	struct adis16480 *st;
 	int ret;
@@ -1218,7 +1227,11 @@ static int adis16480_probe(struct spi_device *spi)
 	indio_dev->info = &adis16480_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = adis_init(&st->adis, indio_dev, spi, &adis16480_data);
+	adis16480_data = adis16480_adis_data_alloc(st, &spi->dev);
+	if (IS_ERR(adis16480_data))
+		return PTR_ERR(adis16480_data);
+
+	ret = adis_init(&st->adis, indio_dev, spi, adis16480_data);
 	if (ret)
 		return ret;
 
-- 
2.20.1

[PATCH 4/5] iio: adis: Introduce timeouts structure

[Alexandru Ardelean]

From: Nuno Sá <nuno.sa@analog.com>

The adis library only allows to define a `startup_delay` which for some
devices is enough. However, other devices define different timeouts with
significantly different timings which could lead to devices to not wait
enough time or to wait a lot more than necessary (which is not
efficient). This patch introduces a new timeout struct that must be
passed into `adis_init()`. There are mainly, for now, three timeouts
used. This is also an introductory patch with the goal of refactoring
`adis_initial_startup()`. New driver's (eg: adis16480, adis16460) are
replicating code for the device initial setup. With some changes (being
this the first one) we can pass this to `adis_initial_startup()`.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/accel/adis16201.c         |  7 +++++
 drivers/iio/accel/adis16209.c         |  7 +++++
 drivers/iio/gyro/adis16136.c          | 18 ++++++++++++
 drivers/iio/gyro/adis16260.c          |  7 +++++
 drivers/iio/imu/adis.c                | 18 +++++++++---
 drivers/iio/imu/adis16400.c           | 41 +++++++++++++++++++++++++++
 drivers/iio/imu/adis16460.c           |  7 +++++
 drivers/iio/imu/adis16480.c           | 36 +++++++++++++++++++++++
 drivers/staging/iio/accel/adis16203.c |  7 +++++
 drivers/staging/iio/accel/adis16240.c |  7 +++++
 include/linux/iio/imu/adis.h          | 13 +++++++++
 11 files changed, 164 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
index c4810c73b2a2..c92d22387b01 100644
--- a/drivers/iio/accel/adis16201.c
+++ b/drivers/iio/accel/adis16201.c
@@ -233,6 +233,12 @@ static const char * const adis16201_status_error_msgs[] = {
 	[ADIS16201_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.975V",
 };
 
+static const struct adis_timeout adis16201_timeouts = {
+	.reset_ms = ADIS16201_STARTUP_DELAY_MS,
+	.sw_reset_ms = ADIS16201_STARTUP_DELAY_MS,
+	.self_test_ms = ADIS16201_STARTUP_DELAY_MS,
+};
+
 static const struct adis_data adis16201_data = {
 	.read_delay = 20,
 	.msc_ctrl_reg = ADIS16201_MSC_CTRL_REG,
@@ -242,6 +248,7 @@ static const struct adis_data adis16201_data = {
 	.self_test_mask = ADIS16201_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
 	.startup_delay = ADIS16201_STARTUP_DELAY_MS,
+	.timeouts = &adis16201_timeouts,
 
 	.status_error_msgs = adis16201_status_error_msgs,
 	.status_error_mask = BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
diff --git a/drivers/iio/accel/adis16209.c b/drivers/iio/accel/adis16209.c
index 98d77af8a2b0..f5a78fc11919 100644
--- a/drivers/iio/accel/adis16209.c
+++ b/drivers/iio/accel/adis16209.c
@@ -243,6 +243,12 @@ static const char * const adis16209_status_error_msgs[] = {
 	[ADIS16209_STAT_POWER_LOW_BIT] = "Power supply below 2.975V",
 };
 
+static const struct adis_timeout adis16209_timeouts = {
+	.reset_ms = ADIS16209_STARTUP_DELAY_MS,
+	.self_test_ms = ADIS16209_STARTUP_DELAY_MS,
+	.sw_reset_ms = ADIS16209_STARTUP_DELAY_MS,
+};
+
 static const struct adis_data adis16209_data = {
 	.read_delay = 30,
 	.msc_ctrl_reg = ADIS16209_MSC_CTRL_REG,
@@ -252,6 +258,7 @@ static const struct adis_data adis16209_data = {
 	.self_test_mask = ADIS16209_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
 	.startup_delay = ADIS16209_STARTUP_DELAY_MS,
+	.timeouts = &adis16209_timeouts,
 
 	.status_error_msgs = adis16209_status_error_msgs,
 	.status_error_mask = BIT(ADIS16209_STAT_SELFTEST_FAIL_BIT) |
diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index 129de2bd5845..f928a8b42869 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -59,6 +59,7 @@
 struct adis16136_chip_info {
 	unsigned int precision;
 	unsigned int fullscale;
+	const struct adis_timeout *timeouts;
 };
 
 struct adis16136 {
@@ -472,22 +473,38 @@ enum adis16136_id {
 	ID_ADIS16137,
 };
 
+static const struct adis_timeout adis16133_timeouts = {
+	.reset_ms = 75,
+	.sw_reset_ms = 75,
+	.self_test_ms = 50,
+};
+
+static const struct adis_timeout adis16136_timeouts = {
+	.reset_ms = 128,
+	.sw_reset_ms = 75,
+	.self_test_ms = 245,
+};
+
 static const struct adis16136_chip_info adis16136_chip_info[] = {
 	[ID_ADIS16133] = {
 		.precision = IIO_DEGREE_TO_RAD(1200),
 		.fullscale = 24000,
+		.timeouts = &adis16133_timeouts,
 	},
 	[ID_ADIS16135] = {
 		.precision = IIO_DEGREE_TO_RAD(300),
 		.fullscale = 24000,
+		.timeouts = &adis16133_timeouts,
 	},
 	[ID_ADIS16136] = {
 		.precision = IIO_DEGREE_TO_RAD(450),
 		.fullscale = 24623,
+		.timeouts = &adis16136_timeouts,
 	},
 	[ID_ADIS16137] = {
 		.precision = IIO_DEGREE_TO_RAD(1000),
 		.fullscale = 24609,
+		.timeouts = &adis16136_timeouts,
 	},
 };
 
@@ -511,6 +528,7 @@ static struct adis_data *adis16136_adis_data_alloc(struct adis16136 *st,
 				BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
 				BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
 				BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL);
+	data->timeouts = st->chip_info->timeouts;
 
 	return data;
 }
diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index 726a0aa321a8..0e3a66a7726d 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -332,6 +332,12 @@ static const char * const adis1620_status_error_msgs[] = {
 	[ADIS16260_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 4.75",
 };
 
+static const struct adis_timeout adis16260_timeouts = {
+	.reset_ms = ADIS16260_STARTUP_DELAY,
+	.sw_reset_ms = ADIS16260_STARTUP_DELAY,
+	.self_test_ms = ADIS16260_STARTUP_DELAY,
+};
+
 static const struct adis_data adis16260_data = {
 	.write_delay = 30,
 	.read_delay = 30,
@@ -341,6 +347,7 @@ static const struct adis_data adis16260_data = {
 
 	.self_test_mask = ADIS16260_MSC_CTRL_MEM_TEST,
 	.startup_delay = ADIS16260_STARTUP_DELAY,
+	.timeouts = &adis16260_timeouts,
 
 	.status_error_msgs = adis1620_status_error_msgs,
 	.status_error_mask = BIT(ADIS16260_DIAG_STAT_FLASH_CHK_BIT) |
diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index c53f3ed3cb97..3e12ad4b71ba 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -317,19 +317,25 @@ EXPORT_SYMBOL_GPL(__adis_check_status);
 int __adis_reset(struct adis *adis)
 {
 	int ret;
+	const struct adis_timeout *timeouts = adis->data->timeouts;
 
 	ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
 			ADIS_GLOB_CMD_SW_RESET);
-	if (ret)
+	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to reset device: %d\n", ret);
+		return ret;
+	}
 
-	return ret;
+	msleep(timeouts->sw_reset_ms);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(__adis_reset);
 
 static int adis_self_test(struct adis *adis)
 {
 	int ret;
+	const struct adis_timeout *timeouts = adis->data->timeouts;
 
 	ret = __adis_write_reg_16(adis, adis->data->msc_ctrl_reg,
 			adis->data->self_test_mask);
@@ -339,7 +345,7 @@ static int adis_self_test(struct adis *adis)
 		return ret;
 	}
 
-	msleep(adis->data->startup_delay);
+	msleep(timeouts->self_test_ms);
 
 	ret = __adis_check_status(adis);
 
@@ -368,7 +374,6 @@ int adis_initial_startup(struct adis *adis)
 	if (ret) {
 		dev_err(&adis->spi->dev, "Self-test failed, trying reset.\n");
 		__adis_reset(adis);
-		msleep(adis->data->startup_delay);
 		ret = adis_self_test(adis);
 		if (ret) {
 			dev_err(&adis->spi->dev, "Second self-test failed, giving up.\n");
@@ -444,6 +449,11 @@ EXPORT_SYMBOL_GPL(adis_single_conversion);
 int adis_init(struct adis *adis, struct iio_dev *indio_dev,
 	struct spi_device *spi, const struct adis_data *data)
 {
+	if (!data || !data->timeouts) {
+		dev_err(&spi->dev, "No config data or timeouts not defined!\n");
+		return -EINVAL;
+	}
+
 	mutex_init(&adis->state_lock);
 	adis->spi = spi;
 	adis->data = data;
diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index d77bd6792437..29b28ebc5611 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -156,6 +156,7 @@ struct adis16400_state;
 
 struct adis16400_chip_info {
 	const struct iio_chan_spec *channels;
+	const struct adis_timeout *timeouts;
 	const int num_channels;
 	const long flags;
 	unsigned int gyro_scale_micro;
@@ -929,6 +930,36 @@ static const struct iio_chan_spec adis16334_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(ADIS16400_SCAN_TIMESTAMP),
 };
 
+static const struct adis_timeout adis16300_timeouts = {
+	.reset_ms = ADIS16400_STARTUP_DELAY,
+	.sw_reset_ms = ADIS16400_STARTUP_DELAY,
+	.self_test_ms = ADIS16400_STARTUP_DELAY,
+};
+
+static const struct adis_timeout adis16362_timeouts = {
+	.reset_ms = 130,
+	.sw_reset_ms = 130,
+	.self_test_ms = 12,
+};
+
+static const struct adis_timeout adis16400_timeouts = {
+	.reset_ms = 170,
+	.sw_reset_ms = 170,
+	.self_test_ms = 12,
+};
+
+static const struct adis_timeout adis16445_timeouts = {
+	.reset_ms = 55,
+	.sw_reset_ms = 55,
+	.self_test_ms = 16,
+};
+
+static const struct adis_timeout adis16448_timeouts = {
+	.reset_ms = 90,
+	.sw_reset_ms = 90,
+	.self_test_ms = 45,
+};
+
 static struct adis16400_chip_info adis16400_chips[] = {
 	[ADIS16300] = {
 		.channels = adis16300_channels,
@@ -941,6 +972,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 140000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16300_timeouts,
 	},
 	[ADIS16334] = {
 		.channels = adis16334_channels,
@@ -964,6 +996,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.flags = ADIS16400_NO_BURST | ADIS16400_HAS_SLOW_MODE,
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16300_timeouts,
 	},
 	[ADIS16360] = {
 		.channels = adis16350_channels,
@@ -976,6 +1009,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16300_timeouts,
 	},
 	[ADIS16362] = {
 		.channels = adis16350_channels,
@@ -988,6 +1022,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16362_timeouts,
 	},
 	[ADIS16364] = {
 		.channels = adis16350_channels,
@@ -1000,6 +1035,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16362_timeouts,
 	},
 	[ADIS16367] = {
 		.channels = adis16350_channels,
@@ -1012,6 +1048,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 136000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16300_timeouts,
 	},
 	[ADIS16400] = {
 		.channels = adis16400_channels,
@@ -1023,6 +1060,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 25000000 / 140000, /* 25 C = 0x00 */
 		.set_freq = adis16400_set_freq,
 		.get_freq = adis16400_get_freq,
+		.timeouts = &adis16400_timeouts,
 	},
 	[ADIS16445] = {
 		.channels = adis16445_channels,
@@ -1036,6 +1074,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 31000000 / 73860, /* 31 C = 0x00 */
 		.set_freq = adis16334_set_freq,
 		.get_freq = adis16334_get_freq,
+		.timeouts = &adis16445_timeouts,
 	},
 	[ADIS16448] = {
 		.channels = adis16448_channels,
@@ -1049,6 +1088,7 @@ static struct adis16400_chip_info adis16400_chips[] = {
 		.temp_offset = 31000000 / 73860, /* 31 C = 0x00 */
 		.set_freq = adis16334_set_freq,
 		.get_freq = adis16334_get_freq,
+		.timeouts = &adis16448_timeouts,
 	}
 };
 
@@ -1122,6 +1162,7 @@ static struct adis_data *adis16400_adis_data_alloc(struct adis16400_state *st,
 				BIT(ADIS16400_DIAG_STAT_FLASH_UPT) |
 				BIT(ADIS16400_DIAG_STAT_POWER_HIGH) |
 				BIT(ADIS16400_DIAG_STAT_POWER_LOW);
+	data->timeouts = st->variant->timeouts;
 
 	return data;
 }
diff --git a/drivers/iio/imu/adis16460.c b/drivers/iio/imu/adis16460.c
index b55812521537..9539cfe4a259 100644
--- a/drivers/iio/imu/adis16460.c
+++ b/drivers/iio/imu/adis16460.c
@@ -383,6 +383,12 @@ static const char * const adis16460_status_error_msgs[] = {
 	[ADIS16460_DIAG_STAT_FLASH_UPT] = "Flash update failure",
 };
 
+static const struct adis_timeout adis16460_timeouts = {
+	.reset_ms = 225,
+	.sw_reset_ms = 225,
+	.self_test_ms = 10,
+};
+
 static const struct adis_data adis16460_data = {
 	.diag_stat_reg = ADIS16460_REG_DIAG_STAT,
 	.glob_cmd_reg = ADIS16460_REG_GLOB_CMD,
@@ -398,6 +404,7 @@ static const struct adis_data adis16460_data = {
 		BIT(ADIS16460_DIAG_STAT_SPI_COMM) |
 		BIT(ADIS16460_DIAG_STAT_FLASH_UPT),
 	.enable_irq = adis16460_enable_irq,
+	.timeouts = &adis16460_timeouts,
 };
 
 static int adis16460_probe(struct spi_device *spi)
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 1edfed83d480..d6adcd4a281b 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -138,6 +138,7 @@ struct adis16480_chip_info {
 	unsigned int max_dec_rate;
 	const unsigned int *filter_freqs;
 	bool has_pps_clk_mode;
+	const struct adis_timeout *timeouts;
 };
 
 enum adis16480_int_pin {
@@ -794,6 +795,30 @@ enum adis16480_variant {
 	ADIS16497_3,
 };
 
+static const struct adis_timeout adis16485_timeouts = {
+	.reset_ms = 560,
+	.sw_reset_ms = 120,
+	.self_test_ms = 12,
+};
+
+static const struct adis_timeout adis16480_timeouts = {
+	.reset_ms = 560,
+	.sw_reset_ms = 560,
+	.self_test_ms = 12,
+};
+
+static const struct adis_timeout adis16495_timeouts = {
+	.reset_ms = 170,
+	.sw_reset_ms = 130,
+	.self_test_ms = 40,
+};
+
+static const struct adis_timeout adis16495_1_timeouts = {
+	.reset_ms = 250,
+	.sw_reset_ms = 210,
+	.self_test_ms = 20,
+};
+
 static const struct adis16480_chip_info adis16480_chip_info[] = {
 	[ADIS16375] = {
 		.channels = adis16485_channels,
@@ -812,6 +837,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
+		.timeouts = &adis16485_timeouts,
 	},
 	[ADIS16480] = {
 		.channels = adis16480_channels,
@@ -824,6 +850,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
+		.timeouts = &adis16480_timeouts,
 	},
 	[ADIS16485] = {
 		.channels = adis16485_channels,
@@ -836,6 +863,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
+		.timeouts = &adis16485_timeouts,
 	},
 	[ADIS16488] = {
 		.channels = adis16480_channels,
@@ -848,6 +876,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.int_clk = 2460000,
 		.max_dec_rate = 2048,
 		.filter_freqs = adis16480_def_filter_freqs,
+		.timeouts = &adis16485_timeouts,
 	},
 	[ADIS16495_1] = {
 		.channels = adis16485_channels,
@@ -861,6 +890,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.timeouts = &adis16495_1_timeouts,
 	},
 	[ADIS16495_2] = {
 		.channels = adis16485_channels,
@@ -874,6 +904,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.timeouts = &adis16495_1_timeouts,
 	},
 	[ADIS16495_3] = {
 		.channels = adis16485_channels,
@@ -887,6 +918,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.timeouts = &adis16495_1_timeouts,
 	},
 	[ADIS16497_1] = {
 		.channels = adis16485_channels,
@@ -900,6 +932,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.timeouts = &adis16495_1_timeouts,
 	},
 	[ADIS16497_2] = {
 		.channels = adis16485_channels,
@@ -913,6 +946,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.timeouts = &adis16495_1_timeouts,
 	},
 	[ADIS16497_3] = {
 		.channels = adis16485_channels,
@@ -926,6 +960,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
 		.max_dec_rate = 4250,
 		.filter_freqs = adis16495_def_filter_freqs,
 		.has_pps_clk_mode = true,
+		.timeouts = &adis16495_1_timeouts,
 	},
 };
 
@@ -1199,6 +1234,7 @@ static struct adis_data *adis16480_adis_data_alloc(struct adis16480 *st,
 				BIT(ADIS16480_DIAG_STAT_ZMAGN_FAIL) |
 				BIT(ADIS16480_DIAG_STAT_BARO_FAIL);
 	data->enable_irq = adis16480_enable_irq;
+	data->timeouts = st->chip_info->timeouts;
 
 	return data;
 }
diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
index 39687139a7d3..3d706ee02df0 100644
--- a/drivers/staging/iio/accel/adis16203.c
+++ b/drivers/staging/iio/accel/adis16203.c
@@ -237,6 +237,12 @@ static const char * const adis16203_status_error_msgs[] = {
 	[ADIS16203_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.975V",
 };
 
+static const struct adis_timeout adis16203_timeouts = {
+	.reset_ms = ADIS16203_STARTUP_DELAY,
+	.sw_reset_ms = ADIS16203_STARTUP_DELAY,
+	.self_test_ms = ADIS16203_STARTUP_DELAY
+};
+
 static const struct adis_data adis16203_data = {
 	.read_delay = 20,
 	.msc_ctrl_reg = ADIS16203_MSC_CTRL,
@@ -246,6 +252,7 @@ static const struct adis_data adis16203_data = {
 	.self_test_mask = ADIS16203_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
 	.startup_delay = ADIS16203_STARTUP_DELAY,
+	.timeouts = &adis16203_timeouts,
 
 	.status_error_msgs = adis16203_status_error_msgs,
 	.status_error_mask = BIT(ADIS16203_DIAG_STAT_SELFTEST_FAIL_BIT) |
diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index a480409090c0..9bba4238100f 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -359,6 +359,12 @@ static const char * const adis16240_status_error_msgs[] = {
 	[ADIS16240_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 2.225V",
 };
 
+static const struct adis_timeout adis16240_timeouts = {
+	.reset_ms = ADIS16240_STARTUP_DELAY,
+	.sw_reset_ms = ADIS16240_STARTUP_DELAY,
+	.self_test_ms = ADIS16240_STARTUP_DELAY,
+};
+
 static const struct adis_data adis16240_data = {
 	.write_delay = 35,
 	.read_delay = 35,
@@ -369,6 +375,7 @@ static const struct adis_data adis16240_data = {
 	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
 	.startup_delay = ADIS16240_STARTUP_DELAY,
+	.timeouts = &adis16240_timeouts,
 
 	.status_error_msgs = adis16240_status_error_msgs,
 	.status_error_mask = BIT(ADIS16240_DIAG_STAT_PWRON_FAIL_BIT) |
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 4b5bc0e06e69..853dc8c8365c 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -22,6 +22,17 @@
 struct adis;
 struct adis_burst;
 
+/**
+ * struct adis_timeouts - ADIS chip variant timeouts
+ * @reset_ms - Wait time after rst pin goes inactive
+ * @sw_reset_ms - Wait time after sw reset command
+ * @self_test_ms - Wait time after self test command
+ */
+struct adis_timeout {
+	u16 reset_ms;
+	u16 sw_reset_ms;
+	u16 self_test_ms;
+};
 /**
  * struct adis_data - ADIS chip variant specific data
  * @read_delay: SPI delay for read operations in us
@@ -32,6 +43,7 @@ struct adis_burst;
  * @diag_stat_reg: Register address of the DIAG_STAT register
  * @status_error_msgs: Array of error messgaes
  * @status_error_mask:
+ * @timeouts: Chip specific delays
  */
 struct adis_data {
 	unsigned int read_delay;
@@ -45,6 +57,7 @@ struct adis_data {
 	unsigned int self_test_mask;
 	bool self_test_no_autoclear;
 	unsigned int startup_delay;
+	const struct adis_timeout *timeouts;
 
 	const char * const *status_error_msgs;
 	unsigned int status_error_mask;
-- 
2.20.1

[PATCH 5/5] iio: adis: Remove startup_delay

[Alexandru Ardelean]

From: Nuno Sá <nuno.sa@analog.com>

All timeouts are now handled by a dedicated timeout struct. This
variable is no longer needed.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
(cherry picked from commit d4d0c7c2145504a1062d74be8436ce11cced61f4)
---
 drivers/iio/accel/adis16201.c         | 1 -
 drivers/iio/accel/adis16209.c         | 1 -
 drivers/iio/gyro/adis16260.c          | 1 -
 drivers/staging/iio/accel/adis16203.c | 1 -
 drivers/staging/iio/accel/adis16240.c | 1 -
 include/linux/iio/imu/adis.h          | 1 -
 6 files changed, 6 deletions(-)

diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
index c92d22387b01..0f0f27a8184e 100644
--- a/drivers/iio/accel/adis16201.c
+++ b/drivers/iio/accel/adis16201.c
@@ -247,7 +247,6 @@ static const struct adis_data adis16201_data = {
 
 	.self_test_mask = ADIS16201_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16201_STARTUP_DELAY_MS,
 	.timeouts = &adis16201_timeouts,
 
 	.status_error_msgs = adis16201_status_error_msgs,
diff --git a/drivers/iio/accel/adis16209.c b/drivers/iio/accel/adis16209.c
index f5a78fc11919..c6dbd2424e10 100644
--- a/drivers/iio/accel/adis16209.c
+++ b/drivers/iio/accel/adis16209.c
@@ -257,7 +257,6 @@ static const struct adis_data adis16209_data = {
 
 	.self_test_mask = ADIS16209_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16209_STARTUP_DELAY_MS,
 	.timeouts = &adis16209_timeouts,
 
 	.status_error_msgs = adis16209_status_error_msgs,
diff --git a/drivers/iio/gyro/adis16260.c b/drivers/iio/gyro/adis16260.c
index 0e3a66a7726d..be09b3e5910c 100644
--- a/drivers/iio/gyro/adis16260.c
+++ b/drivers/iio/gyro/adis16260.c
@@ -346,7 +346,6 @@ static const struct adis_data adis16260_data = {
 	.diag_stat_reg = ADIS16260_DIAG_STAT,
 
 	.self_test_mask = ADIS16260_MSC_CTRL_MEM_TEST,
-	.startup_delay = ADIS16260_STARTUP_DELAY,
 	.timeouts = &adis16260_timeouts,
 
 	.status_error_msgs = adis1620_status_error_msgs,
diff --git a/drivers/staging/iio/accel/adis16203.c b/drivers/staging/iio/accel/adis16203.c
index 3d706ee02df0..39dfe3f7f254 100644
--- a/drivers/staging/iio/accel/adis16203.c
+++ b/drivers/staging/iio/accel/adis16203.c
@@ -251,7 +251,6 @@ static const struct adis_data adis16203_data = {
 
 	.self_test_mask = ADIS16203_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16203_STARTUP_DELAY,
 	.timeouts = &adis16203_timeouts,
 
 	.status_error_msgs = adis16203_status_error_msgs,
diff --git a/drivers/staging/iio/accel/adis16240.c b/drivers/staging/iio/accel/adis16240.c
index 9bba4238100f..8337a94018f4 100644
--- a/drivers/staging/iio/accel/adis16240.c
+++ b/drivers/staging/iio/accel/adis16240.c
@@ -374,7 +374,6 @@ static const struct adis_data adis16240_data = {
 
 	.self_test_mask = ADIS16240_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16240_STARTUP_DELAY,
 	.timeouts = &adis16240_timeouts,
 
 	.status_error_msgs = adis16240_status_error_msgs,
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 853dc8c8365c..d2fcf45b4cef 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -56,7 +56,6 @@ struct adis_data {
 
 	unsigned int self_test_mask;
 	bool self_test_no_autoclear;
-	unsigned int startup_delay;
 	const struct adis_timeout *timeouts;
 
 	const char * const *status_error_msgs;
-- 
2.20.1

Re: [PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

[Jonathan Cameron]

On Fri, 13 Dec 2019 18:03:08 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change is done in preparation of adding an `struct adis_timeout` type.
> Some ADIS drivers support multiple drivers, with various combinations of
> timeouts. Creating static tables for each driver is possible, but that also
> creates quite a lot of duplication.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

There are considerable advantages to using constant structures,
(security - not that relevant here probably, XiP, general readability)

So to take a series like this I want to see evidence that it makes
a significant difference.  So far you just have cases where we end up
with a worse result.  More code, harder to read...

Hence it will take a lot to persuade me to take this series without
the follow up patches where I assume significant advantages are seen.

Jonathan


> ---
>  drivers/iio/gyro/adis16136.c | 49 ++++++++++++++++++++++--------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
> index f10c4f173898..129de2bd5845 100644
> --- a/drivers/iio/gyro/adis16136.c
> +++ b/drivers/iio/gyro/adis16136.c
> @@ -465,24 +465,6 @@ static const char * const adis16136_status_error_msgs[] = {
>  	[ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL] = "Flash checksum error",
>  };
>  
> -static const struct adis_data adis16136_data = {
> -	.diag_stat_reg = ADIS16136_REG_DIAG_STAT,
> -	.glob_cmd_reg = ADIS16136_REG_GLOB_CMD,
> -	.msc_ctrl_reg = ADIS16136_REG_MSC_CTRL,
> -
> -	.self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST,
> -	.startup_delay = 80,
> -
> -	.read_delay = 10,
> -	.write_delay = 10,
> -
> -	.status_error_msgs = adis16136_status_error_msgs,
> -	.status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
> -		BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
> -		BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
> -		BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL),
> -};
> -
>  enum adis16136_id {
>  	ID_ADIS16133,
>  	ID_ADIS16135,
> @@ -509,11 +491,36 @@ static const struct adis16136_chip_info adis16136_chip_info[] = {
>  	},
>  };
>  
> +static struct adis_data *adis16136_adis_data_alloc(struct adis16136 *st,
> +						   struct device *dev)
> +{
> +	struct adis_data *data;
> +
> +	data = devm_kzalloc(dev, sizeof(struct adis_data), GFP_KERNEL);
> +	if (!data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	data->msc_ctrl_reg = ADIS16136_REG_MSC_CTRL;
> +	data->glob_cmd_reg = ADIS16136_REG_GLOB_CMD;
> +	data->diag_stat_reg = ADIS16136_REG_DIAG_STAT;
> +	data->self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST;
> +	data->read_delay = 10;
> +	data->write_delay = 10;
> +	data->status_error_msgs = adis16136_status_error_msgs;
> +	data->status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
> +				BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
> +				BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
> +				BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL);
> +
> +	return data;
> +}
> +
>  static int adis16136_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
>  	struct adis16136 *adis16136;
>  	struct iio_dev *indio_dev;
> +	const struct adis_data *adis16136_data;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adis16136));
> @@ -532,7 +539,11 @@ static int adis16136_probe(struct spi_device *spi)
>  	indio_dev->info = &adis16136_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = adis_init(&adis16136->adis, indio_dev, spi, &adis16136_data);
> +	adis16136_data = adis16136_adis_data_alloc(adis16136, &spi->dev);
> +	if (IS_ERR(adis16136_data))
> +		return PTR_ERR(adis16136_data);
> +
> +	ret = adis_init(&adis16136->adis, indio_dev, spi, adis16136_data);
>  	if (ret)
>  		return ret;
>  

Re: [PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

[Ardelean, Alexandru]

On Sun, 2019-12-15 at 16:18 +0000, Jonathan Cameron wrote:
> [External]
> 
> On Fri, 13 Dec 2019 18:03:08 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > This change is done in preparation of adding an `struct adis_timeout`
> > type.
> > Some ADIS drivers support multiple drivers, with various combinations
> > of
> > timeouts. Creating static tables for each driver is possible, but that
> > also
> > creates quite a lot of duplication.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> There are considerable advantages to using constant structures,
> (security - not that relevant here probably, XiP, general readability)
> 
> So to take a series like this I want to see evidence that it makes
> a significant difference.  So far you just have cases where we end up
> with a worse result.  More code, harder to read...
> 
> Hence it will take a lot to persuade me to take this series without
> the follow up patches where I assume significant advantages are seen.
> 

Well, we've have some discussion about this, and how to do it.
There are several alternatives.

Some of the ideas were:
1. Keep the static data and clone it + populate the adis_timeout data as
needed during probe [based on each device's chip-info]
2. Rework all the chip-info data to include the adis_data types/info

2. may require more work ; 1. require fewer patches

This implementation [in this series] is 1. but without keeping the static
data and template.
I guess the idea was to reduce memory usage [by keeping the static data]. I
admit the memory usage is not that big.

I'll take a look at this again, and see if 2. can work more nicely.
It might be that 1. would be the end-result, but who knows?

Thanks
Alex


> Jonathan
> 
> 
> > ---
> >  drivers/iio/gyro/adis16136.c | 49 ++++++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/iio/gyro/adis16136.c
> > b/drivers/iio/gyro/adis16136.c
> > index f10c4f173898..129de2bd5845 100644
> > --- a/drivers/iio/gyro/adis16136.c
> > +++ b/drivers/iio/gyro/adis16136.c
> > @@ -465,24 +465,6 @@ static const char * const
> > adis16136_status_error_msgs[] = {
> >  	[ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL] = "Flash checksum error",
> >  };
> >  
> > -static const struct adis_data adis16136_data = {
> > -	.diag_stat_reg = ADIS16136_REG_DIAG_STAT,
> > -	.glob_cmd_reg = ADIS16136_REG_GLOB_CMD,
> > -	.msc_ctrl_reg = ADIS16136_REG_MSC_CTRL,
> > -
> > -	.self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST,
> > -	.startup_delay = 80,
> > -
> > -	.read_delay = 10,
> > -	.write_delay = 10,
> > -
> > -	.status_error_msgs = adis16136_status_error_msgs,
> > -	.status_error_mask = BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
> > -		BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
> > -		BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
> > -		BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL),
> > -};
> > -
> >  enum adis16136_id {
> >  	ID_ADIS16133,
> >  	ID_ADIS16135,
> > @@ -509,11 +491,36 @@ static const struct adis16136_chip_info
> > adis16136_chip_info[] = {
> >  	},
> >  };
> >  
> > +static struct adis_data *adis16136_adis_data_alloc(struct adis16136
> > *st,
> > +						   struct device *dev)
> > +{
> > +	struct adis_data *data;
> > +
> > +	data = devm_kzalloc(dev, sizeof(struct adis_data), GFP_KERNEL);
> > +	if (!data)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	data->msc_ctrl_reg = ADIS16136_REG_MSC_CTRL;
> > +	data->glob_cmd_reg = ADIS16136_REG_GLOB_CMD;
> > +	data->diag_stat_reg = ADIS16136_REG_DIAG_STAT;
> > +	data->self_test_mask = ADIS16136_MSC_CTRL_SELF_TEST;
> > +	data->read_delay = 10;
> > +	data->write_delay = 10;
> > +	data->status_error_msgs = adis16136_status_error_msgs;
> > +	data->status_error_mask =
> > BIT(ADIS16136_DIAG_STAT_FLASH_UPDATE_FAIL) |
> > +				BIT(ADIS16136_DIAG_STAT_SPI_FAIL) |
> > +				BIT(ADIS16136_DIAG_STAT_SELF_TEST_FAIL) |
> > +				BIT(ADIS16136_DIAG_STAT_FLASH_CHKSUM_FAIL);
> > +
> > +	return data;
> > +}
> > +
> >  static int adis16136_probe(struct spi_device *spi)
> >  {
> >  	const struct spi_device_id *id = spi_get_device_id(spi);
> >  	struct adis16136 *adis16136;
> >  	struct iio_dev *indio_dev;
> > +	const struct adis_data *adis16136_data;
> >  	int ret;
> >  
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adis16136));
> > @@ -532,7 +539,11 @@ static int adis16136_probe(struct spi_device *spi)
> >  	indio_dev->info = &adis16136_info;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> > -	ret = adis_init(&adis16136->adis, indio_dev, spi, &adis16136_data);
> > +	adis16136_data = adis16136_adis_data_alloc(adis16136, &spi->dev);
> > +	if (IS_ERR(adis16136_data))
> > +		return PTR_ERR(adis16136_data);
> > +
> > +	ret = adis_init(&adis16136->adis, indio_dev, spi, adis16136_data);
> >  	if (ret)
> >  		return ret;
> >  

Re: [PATCH 1/5] iio: gyro: adi16136: construct adis data on probe vs static on driver

[Sa, Nuno]

Hi all,

On Mon, 2019-12-16 at 07:49 +0000, Ardelean, Alexandru wrote:
> On Sun, 2019-12-15 at 16:18 +0000, Jonathan Cameron wrote:
> > 
> > On Fri, 13 Dec 2019 18:03:08 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> > 
> > > This change is done in preparation of adding an `struct
> > > adis_timeout`
> > > type.
> > > Some ADIS drivers support multiple drivers, with various
> > > combinations
> > > of
> > > timeouts. Creating static tables for each driver is possible, but
> > > that
> > > also
> > > creates quite a lot of duplication.
> > > 
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> > 
> > There are considerable advantages to using constant structures,
> > (security - not that relevant here probably, XiP, general
> > readability)
> > 
> > So to take a series like this I want to see evidence that it makes
> > a significant difference.  So far you just have cases where we end
> > up
> > with a worse result.  More code, harder to read...
> > 
> > Hence it will take a lot to persuade me to take this series without
> > the follow up patches where I assume significant advantages are
> > seen.
> > 
> 
> Well, we've have some discussion about this, and how to do it.
> There are several alternatives.
> 
> Some of the ideas were:
> 1. Keep the static data and clone it + populate the adis_timeout data
> as
> needed during probe [based on each device's chip-info]
> 2. Rework all the chip-info data to include the adis_data types/info
> 
> 2. may require more work ; 1. require fewer patches
> 
> This implementation [in this series] is 1. but without keeping the
> static
> data and template.
> I guess the idea was to reduce memory usage [by keeping the static
> data]. I
> admit the memory usage is not that big.
> 
> I'll take a look at this again, and see if 2. can work more nicely.
> It might be that 1. would be the end-result, but who knows?
> 
> Thanks
> Alex

I guess we also need to prepare/send the following patches to show
Jonathan why we need to dynamically allocate the data structure in some
drivers. In the end is because some devices require different timeouts
(handled by the adis core library) than the others and, in some cases
these differences are quite significative. It was even happening that
in some cases, we were not sleeping enough time (eg: after a reset
command). In the next patches, a timeout structure is included that
needs to be filled for each device.

Alex, maybe we should include more patches in this series to show the
"big picture" and then we can discuss if this is the best approach.

Nuno Sá