On Fri, 12 Jan 2024 17:40:21 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> From: Nuno Sa <nuno.sa@xxxxxxxxxx>
>
> Convert the driver to use the new IIO backend framework. The device
> functionality is expected to be the same (meaning no added or removed
> features).
>
> Also note this patch effectively breaks ABI and that's needed so we can
> properly support this device and add needed features making use of the
> new IIO framework.
>
> Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
Hi Nuno,
Some trivial stuff in here (not I reviewed in reverse so might make more
sense read that way). Mostly little changes that will reduce what
appears to be going on in this patch to the minimum possible
>
> -static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
> +static int ad9467_get_scale(struct ad9467_state *st, int *val, int *val2)
> {
> - const struct adi_axi_adc_chip_info *info = conv->chip_info;
Keep an info variable around in here as well.
> - const struct ad9467_chip_info *info1 = to_ad9467_chip_info(info);
> - struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> unsigned int i, vref_val;
> int ret;
>
> @@ -282,25 +276,23 @@ static int ad9467_get_scale(struct adi_axi_adc_conv *conv, int *val, int *val2)
> if (ret < 0)
> return ret;
>
> - vref_val = ret & info1->vref_mask;
> + vref_val = ret & st->info->vref_mask;
>
> - for (i = 0; i < info->num_scales; i++) {
> - if (vref_val == info->scale_table[i][1])
> + for (i = 0; i < st->info->num_scales; i++) {
> + if (vref_val == st->info->scale_table[i][1])
> break;
> }
>
> - if (i == info->num_scales)
> + if (i == st->info->num_scales)
> return -ERANGE;
>
> - __ad9467_get_scale(conv, i, val, val2);
> + __ad9467_get_scale(st, i, val, val2);
>
> return IIO_VAL_INT_PLUS_MICRO;
> }
>
> -static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
> +static int ad9467_set_scale(struct ad9467_state *st, int val, int val2)
> {
> - const struct adi_axi_adc_chip_info *info = conv->chip_info;
> - struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
Definitely good to keep local info variable here.
> unsigned int scale_val[2];
> unsigned int i;
> int ret;
> @@ -308,14 +300,14 @@ static int ad9467_set_scale(struct adi_axi_adc_conv *conv, int val, int val2)
> if (val != 0)
> return -EINVAL;
>
> - for (i = 0; i < info->num_scales; i++) {
> - __ad9467_get_scale(conv, i, &scale_val[0], &scale_val[1]);
> + for (i = 0; i < st->info->num_scales; i++) {
> + __ad9467_get_scale(st, i, &scale_val[0], &scale_val[1]);
> if (scale_val[0] != val || scale_val[1] != val2)
> continue;
>
> guard(mutex)(&st->lock);
> ret = ad9467_spi_write(st->spi, AN877_ADC_REG_VREF,
> - info->scale_table[i][1]);
> + st->info->scale_table[i][1]);
> if (ret < 0)
> return ret;
>
> }
>
> -static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
> +static int ad9467_write_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int val, int val2, long mask)
> {
> - const struct adi_axi_adc_chip_info *info = conv->chip_info;
> - struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> + struct ad9467_state *st = iio_priv(indio_dev);
Even here I'd keep a local info variable as it's harmless and
reduces what is changed in this patch.
> long r_clk;
>
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
> - return ad9467_set_scale(conv, val, val2);
> + return ad9467_set_scale(st, val, val2);
> case IIO_CHAN_INFO_SAMP_FREQ:
> r_clk = clk_round_rate(st->clk, val);
> - if (r_clk < 0 || r_clk > info->max_rate) {
> + if (r_clk < 0 || r_clk > st->info->max_rate) {
> dev_warn(&st->spi->dev,
> "Error setting ADC sample rate %ld", r_clk);
> return -EINVAL;
> @@ -369,26 +360,53 @@ static int ad9467_write_raw(struct adi_axi_adc_conv *conv,
> }
> }
>
> -static int ad9467_read_avail(struct adi_axi_adc_conv *conv,
> +static int ad9467_read_avail(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> const int **vals, int *type, int *length,
> long mask)
> {
> - const struct adi_axi_adc_chip_info *info = conv->chip_info;
> - struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> + struct ad9467_state *st = iio_priv(indio_dev);
As below, I'd keep the local info variable to reduce scope of changes
to the minimum.
>
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
> *vals = (const int *)st->scales;
> *type = IIO_VAL_INT_PLUS_MICRO;
> /* Values are stored in a 2D matrix */
> - *length = info->num_scales * 2;
> + *length = st->info->num_scales * 2;
> return IIO_AVAIL_LIST;
> default:
> return -EINVAL;
> }
> }
>
> +
> static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
> {
> int ret;
> @@ -401,19 +419,17 @@ static int ad9467_outputmode_set(struct spi_device *spi, unsigned int mode)
> AN877_ADC_TRANSFER_SYNC);
> }
>
> -static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
> +static int ad9467_scale_fill(struct ad9467_state *st)
> {
> - const struct adi_axi_adc_chip_info *info = conv->chip_info;
> - struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> unsigned int i, val1, val2;
const struct adi_axi_adc_chip_info *info = st->info;
I think...
Makes this patch more minimal which is nice and it's not a bad change
in it's own right..
Same with some other cases changed by this patch.
>
> - st->scales = devm_kmalloc_array(&st->spi->dev, info->num_scales,
> + st->scales = devm_kmalloc_array(&st->spi->dev, st->info->num_scales,
> sizeof(*st->scales), GFP_KERNEL);
> if (!st->scales)
> return -ENOMEM;
>
> - for (i = 0; i < info->num_scales; i++) {
> - __ad9467_get_scale(conv, i, &val1, &val2);
> + for (i = 0; i < st->info->num_scales; i++) {
> + __ad9467_get_scale(st, i, &val1, &val2);
> st->scales[i][0] = val1;
> st->scales[i][1] = val2;
> }
> @@ -421,11 +437,27 @@ static int ad9467_scale_fill(struct adi_axi_adc_conv *conv)
> return 0;
> }
>
> -static int ad9467_preenable_setup(struct adi_axi_adc_conv *conv)
> +static int ad9467_setup(struct ad9467_state *st)
> {
> - struct ad9467_state *st = adi_axi_adc_conv_priv(conv);
> + struct iio_backend_data_fmt data = {
> + .sign_extend = true,
> + .enable = true,
> + };
> + unsigned int c, mode;
> + int ret;
> +
> + mode = st->info->default_output_mode | AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
Bit of a long line. Perhaps break it after the |
> + ret = ad9467_outputmode_set(st->spi, mode);
> + if (ret)
> + return ret;
>
> - return ad9467_outputmode_set(st->spi, st->output_mode);
> + for (c = 0; c < st->info->num_channels; c++) {
> + ret = iio_backend_data_format_set(st->back, c, &data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> }
>
> static int ad9467_reset(struct device *dev)
> @@ -443,25 +475,64 @@ static int ad9467_reset(struct device *dev)
> return 0;
> }
>
> +static int ad9467_iio_backend_get(struct ad9467_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + struct device_node *__back;
> +
> + st->back = devm_iio_backend_get(&st->spi->dev, NULL);
> + /* If not found, don't error out as we might have legacy DT property */
> + if (IS_ERR(st->back) && PTR_ERR(st->back) != -ENOENT)
> + return PTR_ERR(st->back);
> + if (!IS_ERR(st->back))
> + return 0;
Why not do this one first? I know I normally moan about having error handlers
out of line, but in this case the good is out of line whatever.
if (!IS_ERR(st->back)
return 0;
if (PTR_ERR(st->back) != ENOENT)
return PTR_ERR(st->back);
...
> + /*
> + * if we don't get the backend using the normal API's, use the legacy
> + * 'adi,adc-dev' property. So we get all nodes with that property, and
> + * look for the one pointing at us. Then we directly lookup that fwnode
> + * on the backend list of registered devices. This is done so we don't
> + * make io-backends mandatory which would break DT ABI.
> + */
> + for_each_node_with_property(__back, "adi,adc-dev") {
> + struct device_node *__me;
> +
> + __me = of_parse_phandle(__back, "adi,adc-dev", 0);
> + if (!__me)
> + continue;
> +
> + if (!device_match_of_node(dev, __me)) {
> + of_node_put(__me);
> + continue;
> + }
> +
> + of_node_put(__me);
> + st->back = __devm_iio_backend_get_from_fwnode_lookup(dev,
> + of_fwnode_handle(__back));
> + of_node_put(__back);
> + return PTR_ERR_OR_ZERO(st->back);
> + }
> +
> + return -ENODEV;
> +}
> +
> static int ad9467_probe(struct spi_device *spi)
> {
>
>
> - st->output_mode = info->default_output_mode |
> - AN877_ADC_OUTPUT_MODE_TWOS_COMPLEMENT;
> + ret = ad9467_iio_backend_get(st);
> + if (ret)
> + return ret;
>
> - return 0;
> + ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_enable(st->back);
> + if (ret)
> + return ret;
I'm curious there is no iio_backend_disable() to be done in the exit path?
> +
> + ret = ad9467_setup(st);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> }
>
> static const struct of_device_id ad9467_of_match[] = {
> @@ -529,4 +610,4 @@ module_spi_driver(ad9467_driver);
> MODULE_AUTHOR("Michael Hennerich <michael.hennerich@xxxxxxxxxx>");
> MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver");
> MODULE_LICENSE("GPL v2");
> -MODULE_IMPORT_NS(IIO_ADI_AXI);
> +MODULE_IMPORT_NS(IIO_BACKEND);
>
