ADIS16550 #2510

rbolboac · 2024-06-13T11:20:54Z

PR Description

Please replace this comment with a summary of your changes, and add any context
necessary to understand them. List any dependencies required for this change.
To check the checkboxes below, insert a 'x' between square brackets (without
any space), or simply check them after publishing the PR.
If you changes include a breaking change, please specify dependent PRs in the
description and try to push all related PRs simultaneously.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

nunojsa

Some initial review from my side. Some other notes is to refactor some commit messages and titles. You should use iio: imu: ... for the title. Also, for the the patch introducing the driver we should do better than Integrate ADIS16550 with adis lib and also on the commit message.

On another topic (and note that I don't care much) I think at this point the code is mostly the original one so you should cherry-pick the patches and keep me as the author (and add your co-developed-by tag). If during review the driver changes in a very significant way then, yes, you should make reverse things (you as author and me with co-dev tag).

Also, addition to MAINTAINERS and ABI docs (for the write lock) are missing

nunojsa · 2024-06-17T12:04:08Z

drivers/iio/imu/adis.c

@@ -491,6 +494,11 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_NS_GPL(adis_single_conversion, IIO_ADISLIB);

+static struct adis_ops default_ops = {


static const... See below.

Also let's call it adis_default_ops or adis_ops or something like that. Key is it should have the adis prefix.

nunojsa · 2024-06-17T12:07:06Z

drivers/iio/imu/adis.c

+
+		if (!adis->ops->write)
+			adis->ops->write = __adis_write_reg;
+	}


Let's simplify this to:

if (!adis->ops) adis->ops = &default_ops;

I think it's fairly unlikely for us to need a custom read() but not a custom write() and vice-versa. Also that could be made to work by exporting the default functions. This way we can make default_ops const as well as struct adis_ops *ops; in struct adis which is way better.

nunojsa · 2024-06-17T12:08:23Z

drivers/iio/imu/adis.c

+		reg_size = adis->data->diag_stat_size;
+
+	ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status,
+			      reg_size);


the above call should likely be in patch 1?

in patch 1 there is already an update of __adis_read_reg_16 using the ops structure (adis->ops->write(adis, reg, val, 2);). This call was added like this ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status, reg_size); only because the reg_size is variable now.

nunojsa · 2024-06-17T12:10:42Z

Documentation/devicetree/bindings/iio/imu/adi,adis16550.yaml

+      Configures the device SYNC pin. The following modes are supported
+      0 - direct_sync
+      1 - scaled_sync
+    $ref: /schemas/types.yaml#/definitions/uint32


Since this is new, use strings in here. I think it's the preferred way these days.

nunojsa · 2024-06-17T12:12:08Z

drivers/iio/imu/adis16550.c

+#define ADIS16550_SPI_CRC(x)		FIELD_PREP(ADIS16550_SPI_CRC_MASK, x)
+#define ADIS16550_SPI_CRC_GET(x)	FIELD_GET(ADIS16550_SPI_CRC_MASK, x)
+#define ADIS16550_SPI_SV_MASK		GENMASK(7, 6)
+#define ADIS16550_SPI_SV_GET(x)		FIELD_GET(ADIS16550_SPI_SV_MASK, x)


These days I think Jonathan prefers to have the FIELD_PREP/GET() stuff inline in the code

nunojsa · 2024-06-17T12:24:36Z

drivers/iio/imu/adis16550.c

+		goto done;
+	}
+
+		for_each_set_bit(bit, indio_dev->active_scan_mask, indio_dev->masklength) {


indentations...

nunojsa · 2024-06-17T12:25:55Z

drivers/iio/imu/adis16550.c

+		kfree(adis->xfer);
+		adis->xfer = NULL;
+		return -ENOMEM;
+	}


I think most of the above code is duplicated but at this point I'm also not sure that something like an option as .prepare_scan_mode() or something like that is worth it (at this point).

nunojsa · 2024-06-17T12:26:28Z

drivers/iio/imu/adis16550.c

+
+	tx = adis->buffer + burst_length;
+	tx[0] = 0x00;
+	tx[1] = 0x00;


I guess we can omit the above two lines (should be 0 already)

nunojsa · 2024-06-17T12:26:51Z

drivers/iio/imu/adis16550.c

+
+	if (sync_mode >= st->info->num_sync) {
+		dev_err(dev, "Invalid sync mode: %u for %s\n", sync_mode,
+			st->info->name);


dev_err_probe(). Ditto for the other places

nunojsa · 2024-06-17T12:27:10Z

drivers/iio/imu/adis16550.c

+
+	ret = devm_add_action_or_reset(dev, adis16550_disable_clk, clk);
+	if (ret)
+		return ret;


devm_clk_get_enabled()

amiclaus

changes in v2:

refactor commit titles.
add maintainers
drop write_lock custom attr.
use static const for default_ops and add adis_ prefix
use strings for sync-mode
use FIELD_GET/FIELD_PREP inline
drop some const
use GENMASK where possible
implement set_freq/get_freq based on scale mode.
fix indentations
use dev_error_probe where possible
use devm_clk_get_enabled
add vdd regulator

nunojsa

Another round. Also you need to add your's and Ramona Co-developed-by tag otherwise you'll be asked about all thos sign offs

nunojsa · 2024-07-15T07:38:59Z

drivers/iio/imu/adis.c

@@ -520,6 +528,9 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,

 	adis->spi = spi;
 	adis->data = data;
+	if (!adis->ops)
+		adis->ops = &adis_default_ops;


since we are assuming we always have write() and read() we should sanity check it here (for ops given by drivers).

nunojsa · 2024-07-15T07:40:01Z

include/linux/iio/imu/adis.h

@@ -115,6 +130,7 @@ struct adis {

 	const struct adis_data	*data;
 	unsigned int		burst_extra_len;
+	const struct adis_ops		*ops;


nit: not aligned with other members

nunojsa · 2024-07-15T07:44:47Z

MAINTAINERS

@@ -1293,6 +1293,16 @@ S:	Supported
 F:	drivers/iio/imu/adis16475.c
 F:	Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml

+ANALOG DEVICES INC ADIS16550 DRIVER
+M:	Nuno Sa <[email protected]>


Since I'm here, I should likely be also in the bindings patch

nunojsa · 2024-07-15T07:45:13Z

MAINTAINERS

+L:	[email protected]
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/imu/adi,adis16550.yaml


This file is introduced in the bindings patch. Hence, the entry should be introduced in that patch...

nunojsa · 2024-07-15T07:45:49Z

drivers/iio/imu/adis16550.c

+	{ .compatible = "adi,adis16550", .data = &adis16550_chip_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, adis16550_of_match);


add a spi id table for module auto load

nunojsa · 2024-07-15T09:09:15Z

drivers/iio/imu/adis16550.c

+static void adis16550_debugfs_init(struct iio_dev *indio_dev)
+{
+}
+#endif


I think this can be done in a better way. Drop the #ifdef CONFIG_DEBUG_FS. Instead add this in the beginning of adis16550_debugfs_init():

if (IS_ENABLED(CONFIG_DEBUG_FS)) return;

And let the compiler remove all the unused API's :)

nunojsa · 2024-07-15T09:10:30Z

drivers/iio/imu/adis16550.c

+	__be32 __din[2], __dout[2];
+	u16 data = 0;
+	int ret;
+	const bool wr = readval ? false : true;


no need for const...

nunojsa · 2024-07-15T09:14:38Z

drivers/iio/imu/adis16550.c

+
+	ret = spi_sync(spi, &msg);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret, "Spi failure %d\n", ret);


drop dev_err_probe(). Not always called under probe()

nunojsa · 2024-07-15T09:14:58Z

drivers/iio/imu/adis16550.c

+		else if ((writeval >> 16) != data && reg != ADIS16550_REG_COMMAND)
+			return dev_err_probe(&spi->dev, -EIO,
+					     "Data not written: wr: 0x%04X, rcv: 0x%04X\n",
+					     writeval >> 16, data);


nunojsa · 2024-07-15T09:19:29Z

drivers/iio/imu/adis16550.c

+	if (crc_rcv != crc)
+		return dev_err_probe(&adis->spi->dev, -EIO,
+				     "Invalid crc, rcv: 0x%02x, calc: 0x%02x!\n",
+				     crc_rcv, crc);


drop `dev_err_probe(). Normal dev_err()

This patch introduces a custom ops struct letting users define custom read and write functions. Some adis devices might define a completely different spi protocol from the one used in the default implementation. Co-developed-by: Ramona Gradinariu <[email protected]> Signed-off-by: Ramona Gradinariu <[email protected]> Co-developed-by: Antoniu Miclaus <[email protected]> Signed-off-by: Antoniu Miclaus <[email protected]> Signed-off-by: Nuno Sá <[email protected]>

Some devices may have more than 16 bits of status. This patch allows the user to specify the size of the DIAG_STAT register. It defaults to 2 if not specified. This is mainly for backward compatibility. Co-developed-by: Ramona Gradinariu <[email protected]> Signed-off-by: Ramona Gradinariu <[email protected]> Co-developed-by: Antoniu Miclaus <[email protected]> Signed-off-by: Antoniu Miclaus <[email protected]> Signed-off-by: Nuno Sá <[email protected]>

amiclaus

v3:

add proper co-developed-by tags
align members in struct adis.
add MAINTAINERS per patch
add spi id table
use static const
use device_get_match_data
use device_property_match_string
return error if property not found for scaled frequency
drop const where redundant
drop extra line.
use IS_ENABLED(CONFIG_DEBUG_FS)
drop dev_err_probe where iti doesn't apply.

Document the ADIS16550 device devicetree bindings. Co-developed-by: Antoniu Miclaus <[email protected]> Signed-off-by: Antoniu Miclaus <[email protected]> Signed-off-by: Ramona Gradinariu <[email protected]>

The ADIS16550 is a complete inertial system that includes a triaxis gyroscope and a triaxis accelerometer. Each inertial sensor in the ADIS16550 combines industry leading MEMS only technology with signal conditioning that optimizes dynamic performance. The factory calibration characterizes each sensor for sensitivity, bias, and alignment. As a result, each sensor has its own dynamic com- pensation formulas that provide accurate sensor measurements Co-developed-by: Ramona Gradinariu <[email protected]> Signed-off-by: Ramona Gradinariu <[email protected]> Co-developed-by: Antoniu Miclaus <[email protected]> Signed-off-by: Antoniu Miclaus <[email protected]> Signed-off-by: Nuno Sá <[email protected]>

Add documentation for adis16550 driver which describes the driver device files and shows how the user may use the ABI for various scenarios (configuration, measurement, etc.). Co-developed-by: Antoniu Miclaus <[email protected]> Signed-off-by: Antoniu Miclaus <[email protected]> Signed-off-by: Ramona Gradinariu <[email protected]>

Add entry for the ADIS16550 driver. Signed-off-by: Ramona Gradinariu <[email protected]>

nunojsa

Some stuff that yo're still not addressing. Anyways, I think we can speed things up and send it upstream and continue from there.

nunojsa · 2024-09-13T11:40:39Z

drivers/iio/imu/adis16550.c

+		.name = "adis16550",
+		.of_match_table = adis16550_of_match,
+	},
+	.probe = adis16550_probe,


spi_device_id not being used...

nunojsa · 2024-09-13T11:51:38Z

drivers/iio/imu/adis16550.c

+	else if (!strcmp(str, "scaled_sync"))
+		sync_mode = 1;
+	else
+		return -EINVAL;


Oh my bad... the above does not look neat.

Check this one:

fwnode_property_match_property_string()

Though I would look in linux-next to see of there's any device_property_match_property_string() variant

nunojsa · 2024-09-13T11:55:24Z

drivers/iio/imu/adis16550.c

+		 * In pps scaled sync we must scale the input clock to a range
+		 * of [3000 45000].
+		 */
+		ret = device_property_read_u32(dev, "adi,scaled-output-hz",


It seems to me this is still doing things in a very old fashioned way... Look at this one:

https://elixir.bootlin.com/linux/v6.11-rc7/source/drivers/iio/imu/adis16475.c#L1824

Likely it's the same deal here?

nunojsa · 2024-09-13T11:56:05Z

drivers/iio/imu/adis16550.c

+{
+	struct adis *adis = iio_device_get_drvdata(indio_dev);
+	u8 *tx;
+	const u16 burst_length = ADIS16550_BURST_DATA_LEN;


drop const

nunojsa · 2024-09-13T12:00:27Z

drivers/iio/imu/adis16550.c

+			break;
+		}
+	}
+


Did you tried the above and for some reason it did not worked?

nunojsa · 2024-09-13T12:02:09Z

drivers/iio/imu/adis16550.c

+	return 0;
+}
+
+static int adis16550_set_gyro_filter_freq(struct adis16550 *st, int freq)


unsigned int...

nunojsa · 2024-09-13T12:02:29Z

drivers/iio/imu/adis16550.c

+
+	return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
+				  ADIS16550_GYRO_FIR_EN_MASK,
+				  (u32)FIELD_PREP(ADIS16550_GYRO_FIR_EN_MASK, en));


Why the unlocked version? My other comment still holds valid for the possible values of freq

nunojsa · 2024-09-13T12:03:52Z

drivers/iio/imu/adis16550.c

+	if (freq)
+		en = true;
+
+	return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,


needs to be locked

nunojsa · 2024-09-13T12:04:40Z

drivers/iio/imu/adis16550.c

+	return ret;
+}
+
+static int adis16550_get_accl_filter_freq(struct adis16550 *st, int *freq)


The below 4 functions look really similar. There#s high odds they can be combined in just 2 functions

nunojsa · 2024-09-13T12:05:33Z

drivers/iio/imu/adis16550.c

+static void adis16550_debugfs_init(struct iio_dev *indio_dev)
+{
+	if (!IS_ENABLED(CONFIG_DEBUG_FS))
+		return;


Odd place for this... Put it below declarations

rbolboac requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, danmois, CiprianRegus and machschmitt as code owners June 13, 2024 11:20

rbolboac self-assigned this Jun 13, 2024

rbolboac force-pushed the dev/adis16550 branch 4 times, most recently from ebdf441 to 83a8887 Compare June 14, 2024 09:47

nunojsa reviewed Jun 17, 2024

View reviewed changes

amiclaus force-pushed the dev/adis16550 branch from 83a8887 to 889b56c Compare July 11, 2024 09:16

amiclaus reviewed Jul 11, 2024

View reviewed changes

amiclaus force-pushed the dev/adis16550 branch from 889b56c to feee072 Compare July 11, 2024 09:53

nunojsa reviewed Jul 15, 2024

View reviewed changes

nunojsa added 2 commits July 26, 2024 15:28

amiclaus force-pushed the dev/adis16550 branch from feee072 to d90670a Compare July 29, 2024 10:34

amiclaus reviewed Jul 29, 2024

View reviewed changes

rbolboac and others added 4 commits August 28, 2024 14:02

dt-bindings: iio: Add adis16550 bindings

4f79001

Document the ADIS16550 device devicetree bindings. Co-developed-by: Antoniu Miclaus <[email protected]> Signed-off-by: Antoniu Miclaus <[email protected]> Signed-off-by: Ramona Gradinariu <[email protected]>

iio: Kconfig.adi: imply ADIS16550

cedd2c0

Add entry for the ADIS16550 driver. Signed-off-by: Ramona Gradinariu <[email protected]>

amiclaus force-pushed the dev/adis16550 branch from d90670a to cedd2c0 Compare August 28, 2024 11:03

nunojsa reviewed Sep 13, 2024

View reviewed changes

ADIS16550 #2510

Are you sure you want to change the base?

ADIS16550 #2510

Conversation

rbolboac commented Jun 13, 2024 • edited Loading

PR Description

PR Type

PR Checklist

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amiclaus left a comment

Choose a reason for hiding this comment

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amiclaus left a comment

Choose a reason for hiding this comment

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbolboac commented Jun 13, 2024 •

edited

Loading