> +
> + return 0;
> +}
> +
> +static int adis16550_reset(struct adis *adis)
> +{
> + return __adis_write_reg_16(adis, ADIS16550_REG_COMMAND, BIT(15));
> +}
> +
> +static int adis16550_config_sync(struct adis16550 *st)
> +{
> + struct device *dev = &st->adis.spi->dev;
> + const struct adis16550_sync *sync;
> + struct clk *clk;
> + int ret, i;
> + u16 mode;
> +
> + clk = devm_clk_get_optional_enabled(dev, NULL);
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> + if (!clk) {
> + st->clk_freq_hz = st->info->int_clk * 1000;
> + return 0;
> + }
> +
> + st->clk_freq_hz = clk_get_rate(clk);
> +
> + for (i = 0; i < st->info->num_sync; i++) {
> + if (st->clk_freq_hz >= st->info->sync_mode[i].min_rate &&
> + st->clk_freq_hz <= st->info->sync_mode[i].max_rate) {
> + sync = &st->info->sync_mode[i];
Odd that a thing called sync is set to an element of an array called sync_mode
but then contains sync->sync_mode. Something wants renaming here.
Maybe call the structure something like sync_mode_data so that
it incorporates sync mode and stuff about it? I don't really mind what exact
naming is as long as it ends up less confusing than currnetly.
> + break;
> + }
> + }
> +
> + if (i == st->info->num_sync)
> + return dev_err_probe(dev, -EINVAL, "Clk rate: %lu not in a valid range",
> + st->clk_freq_hz);
> +
> + if (sync->sync_mode == ADIS16550_SYNC_MODE_SCALED) {
> + u16 sync_scale;
> + /*
> + * In sps scaled sync we must scale the input clock to a range
> + * of [3000 4500].
> + */
> +
> + sync_scale = DIV_ROUND_CLOSEST(st->info->int_clk, st->clk_freq_hz);
> +
> + if (3000 > sync_scale || 4500 > sync_scale)
That looks implausible. Only actually tests in sync_scale is less than 3000
I'd guess one test is backwards. Make sure to test the values that can go
into code like this.
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid value:%u for sync_scale",
> + sync_scale);
> +
> + ret = adis_write_reg_16(&st->adis, ADIS16550_REG_SYNC_SCALE,
> + sync_scale);
> + if (ret)
> + return ret;
> +
> + st->clk_freq_hz = st->info->int_clk;
> + }
> +
> + st->clk_freq_hz *= 1000;
> +
> + mode = FIELD_PREP(ADIS16550_SYNC_MODE_MASK, sync->sync_mode) |
> + FIELD_PREP(ADIS16550_SYNC_EN_MASK, true);
> +
> + return __adis_update_bits(&st->adis, ADIS16550_REG_CONFIG,
> + ADIS16550_SYNC_MASK, mode);
> +}
> +static int adis16550_probe(struct spi_device *spi)
> +{
> + u16 burst_length = ADIS16550_BURST_DATA_LEN;
> + struct device *dev = &spi->dev;
> + struct iio_dev *indio_dev;
> + struct adis16550 *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->info = spi_get_device_match_data(spi);
> + if (!st->info)
> + return -EINVAL;
> +
> + indio_dev->name = st->info->name;
> + indio_dev->channels = st->info->channels;
> + indio_dev->num_channels = st->info->num_channels;
> + indio_dev->available_scan_masks = adis16550_channel_masks;
> + indio_dev->info = &adis16550_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->adis.ops = &adis16550_ops;
> + st->adis.xfer = NULL;
> + st->xfer = kzalloc(2 * sizeof(*st->xfer),
> + GFP_KERNEL);
Why not a devm_kzalloc or better yet devm_kcalloc()
given it is two of a particular size.
Alternatively if it is always 2 why not just embed an array
of two xfer in st and don't have an extra allocation at all?
> + if (!st->xfer)
> + return -ENOMEM;
> +
> + st->adis.buffer = NULL;
> + st->buffer = kzalloc(burst_length + sizeof(u32),
> + GFP_KERNEL);
devm_kzalloc() so it is cleaned up automatically on error or remove.
If you were going to continue not using devm for these you'd
need to free them in every error path. Use devm to avoid
that.
> + if (!st->buffer)
> + return -ENOMEM;
> +
> + st->xfer[0].tx_buf = st->buffer + burst_length;
> + st->xfer[0].len = 4;
> + st->xfer[0].cs_change = 1;
> + st->xfer[0].delay.value = 8;
> + st->xfer[0].delay.unit = SPI_DELAY_UNIT_USECS;
> + st->xfer[1].rx_buf = st->buffer;
> + st->xfer[1].len = burst_length;
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to get vdd regulator\n");
> +
> + ret = adis_init(&st->adis, indio_dev, spi, &adis16550_data);
> + if (ret)
> + return ret;
> +
> + ret = __adis_initial_startup(&st->adis);
> + if (ret)
> + return ret;
> +
> + ret = adis16550_config_sync(st);
> + if (ret)
> + return ret;
> +
> + ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> + adis16550_trigger_handler);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + adis16550_debugfs_init(indio_dev);
> +
> + return 0;
> +}
> +
> +static void adis16550_remove(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> +
> + struct iio_dev *indio_dev;
> + struct adis16550 *st;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
You are allocating in the remove path. That makes no sense.
> + st = iio_priv(indio_dev);
> +
> + kfree(st->xfer);
> + kfree(st->buffer);
As above. Embed xfer directly as an array of 2 structures in
st, and for st->buffer use devm_kzalloc()
Then no need to have a remove at all.
> +}
> +
> +static const struct spi_device_id adis16550_id[] = {
> + { "adis16550", (kernel_ulong_t)&adis16550_chip_info},
> + { }
> +};
> +MODULE_DEVICE_TABLE(spi, adis16550_id);
> +
> +static const struct of_device_id adis16550_of_match[] = {
> + { .compatible = "adi,adis16550", .data = &adis16550_chip_info },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, adis16550_of_match);
> +
> +static struct spi_driver adis16550_driver = {
> + .driver = {
> + .name = "adis16550",
> + .of_match_table = adis16550_of_match,
> + },
> + .probe = adis16550_probe,
> + .id_table = adis16550_id,
> + .remove = adis16550_remove,
> +};
> +module_spi_driver(adis16550_driver);
> +
> +MODULE_AUTHOR("Nuno Sa <nuno.sa@xxxxxxxxxx>");
> +MODULE_AUTHOR("Ramona Gradinariu <ramona.gradinariu@xxxxxxxxxx>");
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Analog Devices ADIS16550 IMU driver");
> +MODULE_IMPORT_NS("IIO_ADISLIB");
> +MODULE_LICENSE("GPL");
