| Message ID | 20240112-iio-backend-v5-0-bdecad041ab4@analog.com |
|---|---|
| Headers | show |
| Series | iio: add new backend framework | expand |
On Fri, 12 Jan 2024 17:40:21 +0100 Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > From: Nuno Sa <nuno.sa@analog.com> > > 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@analog.com> 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@analog.com>"); > MODULE_DESCRIPTION("Analog Devices AD9467 ADC driver"); > MODULE_LICENSE("GPL v2"); > -MODULE_IMPORT_NS(IIO_ADI_AXI); > +MODULE_IMPORT_NS(IIO_BACKEND); >
On Fri, 12 Jan 2024 17:40:22 +0100 Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > From: Nuno Sa <nuno.sa@analog.com> > > Move to the IIO backend framework. Devices supported by adi-axi-adc now > register themselves as backend devices. > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> A few quick drive by comments whist I wait for a build to finish... > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c > index 0f21d1d98b9f..741b53c25bb1 100644 > --- a/drivers/iio/adc/adi-axi-adc.c > +++ b/drivers/iio/adc/adi-axi-adc.c > @@ -8,6 +8,7 @@ >> static int adi_axi_adc_probe(struct platform_device *pdev) > { ... > @@ -390,37 +205,23 @@ static int adi_axi_adc_probe(struct platform_device *pdev) > if (ret) > return ret; > > - if (cl->info->version > ver) { > + if (*expected_ver > ver) { > dev_err(&pdev->dev, > "IP core version is too old. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n", Format doesn't match with later. > - ADI_AXI_PCORE_VER_MAJOR(cl->info->version), > - ADI_AXI_PCORE_VER_MINOR(cl->info->version), > - ADI_AXI_PCORE_VER_PATCH(cl->info->version), > + ADI_AXI_PCORE_VER_MAJOR(*expected_ver), > + ADI_AXI_PCORE_VER_MINOR(*expected_ver), > + ADI_AXI_PCORE_VER_PATCH(*expected_ver), > ADI_AXI_PCORE_VER_MAJOR(ver), > ADI_AXI_PCORE_VER_MINOR(ver), > ADI_AXI_PCORE_VER_PATCH(ver)); > return -ENODEV; > } > > - indio_dev->info = &adi_axi_adc_info; > - indio_dev->name = "adi-axi-adc"; > - indio_dev->modes = INDIO_DIRECT_MODE; > - indio_dev->num_channels = conv->chip_info->num_channels; > - indio_dev->channels = conv->chip_info->channels; > - > - ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev); > + ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, st); > if (ret) > return ret; > > - ret = adi_axi_adc_setup_channels(&pdev->dev, st); > - if (ret) > - return ret; > - > - ret = devm_iio_device_register(&pdev->dev, indio_dev); > - if (ret) > - return ret; > - > - dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n", > + dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n", I'd rip this (I think) unrelated change out to reduce noise in here somewhat. I'm curious though as it's still %c above. > ADI_AXI_PCORE_VER_MAJOR(ver), > ADI_AXI_PCORE_VER_MINOR(ver), > ADI_AXI_PCORE_VER_PATCH(ver)); > @@ -428,6 +229,8 @@ static int adi_axi_adc_probe(struct platform_device *pdev) > return 0; > }
On Fri, 12 Jan 2024 17:40:20 +0100 Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > From: Nuno Sa <nuno.sa@analog.com> > > This is a Framework to handle complex IIO aggregate devices. > > The typical architecture is to have one device as the frontend device which > can be "linked" against one or multiple backend devices. All the IIO and > userspace interface is expected to be registers/managed by the frontend > device which will callback into the backends when needed (to get/set > some configuration that it does not directly control). > > The basic framework interface is pretty simple: > - Backends should register themselves with @devm_iio_backend_register() > - Frontend devices should get backends with @devm_iio_backend_get() > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> A few minor comments inline. ... > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c > new file mode 100644 > index 000000000000..994bc68c2679 > --- /dev/null > +++ b/drivers/iio/industrialio-backend.c > @@ -0,0 +1,411 @@ ... > + > +/* > + * Helper struct for requesting buffers. Allows for multiple buffers per > + * backend. Only seems to be used to ensure we have all the data needed to free it... So comment seems less than obviously connected to that. > + */ > +struct iio_backend_buffer_pair { > + struct iio_backend *back; > + struct iio_buffer *buffer; > +}; > + > +/** > + * iio_backend_chan_enable - Enable a backend channel. > + * @back: Backend device. > + * @chan: Channel number. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan) > +{ > + return iio_backend_op_call(back, chan_enable, chan); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_enable, IIO_BACKEND); > + > +/** > + * iio_backend_chan_disable - Disable a backend channel. > + * @back: Backend device. > + * @chan: Channel number. Would be good to be consistent on . or not. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan) > +{ > + return iio_backend_op_call(back, chan_disable, chan); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_disable, IIO_BACKEND); > + > +/** > + * iio_backend_chan_enable - Enable the backend. > + * @back: Backend device ... > +/** > + * devm_iio_backend_get_from_fwnode_lookup Not valid kernel doc + name is wrong. Make sure you run the kernel-doc script over this and fix any errors or warnings reported. > + * @dev: Device where to bind the backend lifetime. > + * @fwnode: Firmware node of the backend device. > + * > + * Search the backend list for a device matching @fwnode. > + * This API should not be used and it's only present for preventing the first > + * user of this framework to break it's DT ABI. > + * > + * RETURNS: > + * A backend pointer, negative error pointer otherwise. > + */ > +struct iio_backend * > +__devm_iio_backend_get_from_fwnode_lookup(struct device *dev, > + struct fwnode_handle *fwnode) > +{ > + struct iio_backend *back; > + int ret; > + > + guard(mutex)(&iio_back_lock); > + list_for_each_entry(back, &iio_back_list, entry) { > + if (!device_match_fwnode(back->dev, fwnode)) > + continue; > + > + ret = __devm_iio_backend_get(dev, back); > + if (ret) > + return ERR_PTR(ret); > + > + return back; > + } > + > + return ERR_PTR(-EPROBE_DEFER); > +} > +EXPORT_SYMBOL_NS_GPL(__devm_iio_backend_get_from_fwnode_lookup, IIO_BACKEND); > > +/** > + * devm_iio_backend_register - Register a new backend device > + * @dev: Backend device being registered. > + * @ops: Backend ops > + * @priv: Device private data. > + * > + * @ops and @priv are both mandatory. Not providing them results in -EINVAL. It's unusual to 'insist' on the private data. Whilst it's highly likely it will always be there from a core point of view we don't mind it being NULL. This is different from the ops as we want to be able to call those without checking they are there. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int devm_iio_backend_register(struct device *dev, > + const struct iio_backend_ops *ops, void *priv) > +{ > + struct iio_backend *back; > + > + if (!ops || !priv) { > + pr_err("%s: No backend ops or private data given\n", > + dev_name(dev)); > + return -EINVAL; > + } > + > + /* > + * Through device_links, we guarantee that a frontend device cannot be > + * bound/exist if the backend driver is not around. Hence, we can bind > + * the backend object lifetime with the device being passed since > + * removing it will torn the frontend down. "will have torn" or "will tear the" > + */ > + back = devm_kzalloc(dev, sizeof(*back), GFP_KERNEL); > + if (!back) > + return -ENOMEM; > + > + back->ops = ops; > + back->owner = dev->driver->owner; > + back->dev = dev; > + back->priv = priv; > + mutex_lock(&iio_back_lock); > + list_add(&back->entry, &iio_back_list); > + mutex_unlock(&iio_back_lock); > + > + return devm_add_action_or_reset(dev, iio_backend_unregister, back); > +} > +EXPORT_SYMBOL_NS_GPL(devm_iio_backend_register, IIO_BACKEND); > + > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>"); > +MODULE_DESCRIPTION("Framework to handle complex IIO aggregate devices"); > +MODULE_LICENSE("GPL");
On Fri, 2024-01-12 at 17:33 +0000, Jonathan Cameron wrote: > On Fri, 12 Jan 2024 17:40:21 +0100 > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > From: Nuno Sa <nuno.sa@analog.com> > > > > 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@analog.com> > > 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_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. Alright, will do it in all the places you pointed out... > > > > > - 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 | One of those cases where I think going beyond the 80 limit col helps readability. But I can break the line if that's your preference. > > > + 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); > > ... Yeah, really because I wanted to have the error condition checked first. But I agree in this particular case it makes the code simpler by not doing it that way. > > > > + /* > > + * 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? > Ehehe something I have in my mind, yes. I'm just not disabling the core because it was the same with the previous approach. My goal was to have (more or less) the same state before vs after introducing the backend. I was thinking in adding a devm_iio_backend_enable() as a follow up patch and use it in here (or actually use it for the first axi-dac/dds user as that one will be come from a "clean" state). If you prefer I can already turn iio_backend_enable() -> devm_iio_backend_enable() and use it in this patch. - Nuno Sá >
On Fri, 2024-01-12 at 17:39 +0000, Jonathan Cameron wrote: > On Fri, 12 Jan 2024 17:40:22 +0100 > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > From: Nuno Sa <nuno.sa@analog.com> > > > > Move to the IIO backend framework. Devices supported by adi-axi-adc now > > register themselves as backend devices. > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > A few quick drive by comments whist I wait for a build to finish... > > > diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c > > index 0f21d1d98b9f..741b53c25bb1 100644 > > --- a/drivers/iio/adc/adi-axi-adc.c > > +++ b/drivers/iio/adc/adi-axi-adc.c > > @@ -8,6 +8,7 @@ > > > static int adi_axi_adc_probe(struct platform_device *pdev) > > { > ... > > > @@ -390,37 +205,23 @@ static int adi_axi_adc_probe(struct platform_device > > *pdev) > > if (ret) > > return ret; > > > > - if (cl->info->version > ver) { > > + if (*expected_ver > ver) { > > dev_err(&pdev->dev, > > "IP core version is too old. Expected %d.%.2d.%c, > > Reported %d.%.2d.%c\n", > > Format doesn't match with later. > > > - ADI_AXI_PCORE_VER_MAJOR(cl->info->version), > > - ADI_AXI_PCORE_VER_MINOR(cl->info->version), > > - ADI_AXI_PCORE_VER_PATCH(cl->info->version), > > + ADI_AXI_PCORE_VER_MAJOR(*expected_ver), > > + ADI_AXI_PCORE_VER_MINOR(*expected_ver), > > + ADI_AXI_PCORE_VER_PATCH(*expected_ver), > > ADI_AXI_PCORE_VER_MAJOR(ver), > > ADI_AXI_PCORE_VER_MINOR(ver), > > ADI_AXI_PCORE_VER_PATCH(ver)); > > return -ENODEV; > > } > > > > - indio_dev->info = &adi_axi_adc_info; > > - indio_dev->name = "adi-axi-adc"; > > - indio_dev->modes = INDIO_DIRECT_MODE; > > - indio_dev->num_channels = conv->chip_info->num_channels; > > - indio_dev->channels = conv->chip_info->channels; > > - > > - ret = adi_axi_adc_config_dma_buffer(&pdev->dev, indio_dev); > > + ret = devm_iio_backend_register(&pdev->dev, &adi_axi_adc_generic, > > st); > > if (ret) > > return ret; > > > > - ret = adi_axi_adc_setup_channels(&pdev->dev, st); > > - if (ret) > > - return ret; > > - > > - ret = devm_iio_device_register(&pdev->dev, indio_dev); > > - if (ret) > > - return ret; > > - > > - dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%c) probed\n", > > + dev_info(&pdev->dev, "AXI ADC IP core (%d.%.2d.%d) probed\n", > I'd rip this (I think) unrelated change out to reduce noise in here somewhat. > I'm curious though as it's still %c above. Oh, just forgot about the above %c... Yeah, the %c format gives a crappy output. That's why I changed it :) - Nuno Sá
On Sat, 2024-01-13 at 17:22 +0000, Jonathan Cameron wrote: > On Fri, 12 Jan 2024 17:40:20 +0100 > Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org> wrote: > > > From: Nuno Sa <nuno.sa@analog.com> > > > > This is a Framework to handle complex IIO aggregate devices. > > > > The typical architecture is to have one device as the frontend device which > > can be "linked" against one or multiple backend devices. All the IIO and > > userspace interface is expected to be registers/managed by the frontend > > device which will callback into the backends when needed (to get/set > > some configuration that it does not directly control). > > > > The basic framework interface is pretty simple: > > - Backends should register themselves with @devm_iio_backend_register() > > - Frontend devices should get backends with @devm_iio_backend_get() > > > > Signed-off-by: Nuno Sa <nuno.sa@analog.com> > > A few minor comments inline. > > ... > > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio- > > backend.c > > new file mode 100644 > > index 000000000000..994bc68c2679 > > --- /dev/null > > +++ b/drivers/iio/industrialio-backend.c > > @@ -0,0 +1,411 @@ > > ... > > > + > > +/* > > + * Helper struct for requesting buffers. Allows for multiple buffers per > > + * backend. > Only seems to be used to ensure we have all the data needed to free it... > So comment seems less than obviously connected to that. I'll update the comment... > > + */ > > +struct iio_backend_buffer_pair { > > + struct iio_backend *back; > > + struct iio_buffer *buffer; > > +}; > > + > > > +/** > > + * iio_backend_chan_enable - Enable a backend channel. > > + * @back: Backend device. > > + * @chan: Channel number. > > + * > > + * RETURNS: > > + * 0 on success, negative error number on failure. > > + */ > > +int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan) > > +{ > > + return iio_backend_op_call(back, chan_enable, chan); > > +} > > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_enable, IIO_BACKEND); > > + > > +/** > > + * iio_backend_chan_disable - Disable a backend channel. > > + * @back: Backend device. > > + * @chan: Channel number. > Would be good to be consistent on . or not. Agreed. > > + * > > + * RETURNS: > > + * 0 on success, negative error number on failure. > > + */ > > +int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan) > > +{ > > + return iio_backend_op_call(back, chan_disable, chan); > > +} > > +EXPORT_SYMBOL_NS_GPL(iio_backend_chan_disable, IIO_BACKEND); > > + > > +/** > > + * iio_backend_chan_enable - Enable the backend. > > + * @back: Backend device > > > > ... > > > > +/** > > + * devm_iio_backend_get_from_fwnode_lookup > > Not valid kernel doc + name is wrong. Make sure you run > the kernel-doc script over this and fix any errors or warnings > reported. Noted. > > > + * @dev: Device where to bind the backend lifetime. > > + * @fwnode: Firmware node of the backend device. > > + * > > + * Search the backend list for a device matching @fwnode. > > + * This API should not be used and it's only present for preventing the > > first > > + * user of this framework to break it's DT ABI. > > + * > > + * RETURNS: > > + * A backend pointer, negative error pointer otherwise. > > + */ > > +struct iio_backend * > > +__devm_iio_backend_get_from_fwnode_lookup(struct device *dev, > > + struct fwnode_handle *fwnode) > > +{ > > + struct iio_backend *back; > > + int ret; > > + > > + guard(mutex)(&iio_back_lock); > > + list_for_each_entry(back, &iio_back_list, entry) { > > + if (!device_match_fwnode(back->dev, fwnode)) > > + continue; > > + > > + ret = __devm_iio_backend_get(dev, back); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return back; > > + } > > + > > + return ERR_PTR(-EPROBE_DEFER); > > +} > > +EXPORT_SYMBOL_NS_GPL(__devm_iio_backend_get_from_fwnode_lookup, > > IIO_BACKEND); > > > > > > +/** > > + * devm_iio_backend_register - Register a new backend device > > + * @dev: Backend device being registered. > > + * @ops: Backend ops > > + * @priv: Device private data. > > + * > > + * @ops and @priv are both mandatory. Not providing them results in - > > EINVAL. > > It's unusual to 'insist' on the private data. > Whilst it's highly likely it will always be there from a core point of view > we don't mind it being NULL. This is different from the ops as we want > to be able to call those without checking they are there. > Hmm, you're right. The private is for the callers to care as we don't really touch it. - Nuno Sá >
> > > + 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? > > > > Ehehe something I have in my mind, yes. I'm just not disabling the core because > it was the same with the previous approach. My goal was to have (more or less) > the same state before vs after introducing the backend. I was thinking in adding > a devm_iio_backend_enable() as a follow up patch and use it in here (or actually > use it for the first axi-dac/dds user as that one will be come from a "clean" > state). > > If you prefer I can already turn iio_backend_enable() -> > devm_iio_backend_enable() and use it in this patch. Might be cleaner to do that. Or add a big fat comment here to say it'll get cleaned up in a follow up patch. > > - Nuno Sá > >
v1: https://lore.kernel.org/linux-iio/20231204144925.4fe9922f@jic23-huawei/T/#m222f5175273b81dbfe40b7f0daffcdc67d6cb8ff v2: https://lore.kernel.org/r/20231208-dev-iio-backend-v2-0-5450951895e1@analog.com v3: https://lore.kernel.org/linux-iio/20231213-dev-iio-backend-v3-0-bb9f12a5c6dc@analog.com/ v4: https://lore.kernel.org/r/20231220-iio-backend-v4-0-998e9148b692@analog.com Changes in v5: - Patch 2 * Update commit subject and message; * Add '#io-backends-cells' property. - Patch 4 * Also support '#io-backends-cells'. - Patch 6 * Improve Kconfig help message; * Only use device_links to control backends access. With this, we can remove all the mutex + sync logic as we are now guaranteed that a frontend cannot be up and running if one of the backends is gone. * Error out if we can't create the device_link (so we can guarantee the above). * Improve docs for __devm_iio_backend_get_from_fwnode_lookup(); * Add a __ prefix to devm_iio_backend_get_from_fwnode_lookup() so it's clear that API should not be used (or used with care); * Drop devm_iio_backend_get_optional(); * remove unneeded headers. - Patch 7 * Handle the optional backend in the driver. The biggest difference is that the backend @ops pointer handling to reason about the backend existence is gone. Now, we rely on device_links as that makes sure all consumers (in the link) are unbound before the provider. Hence, the frontend is guaranteed to be unbound if any of the backends is gone. That simplifies things a lot... Keeping the block diagram so we don't have to follow links to check one of the typical setups. ------------------------------------------------------- ------------------ | ----------- ------------ ------- FPGA | | ADC |------------------------| | AXI ADC |---------| DMA CORE |------| RAM | | | (Frontend/IIO) | Serial Data (eg: LVDS) | |(backend)|---------| |------| | | | |------------------------| ----------- ------------ ------- | ------------------ ------------------------------------------------------- --- Nuno Sa (7): dt-bindings: adc: ad9467: add new io-backend property dt-bindings: adc: axi-adc: update bindings for backend framework driver: core: allow modifying device_links flags iio: buffer-dmaengine: export buffer alloc and free functions iio: add the IIO backend framework iio: adc: ad9467: convert to backend framework iio: adc: adi-axi-adc: move to backend framework Olivier Moysan (1): of: property: add device link support for io-backends .../devicetree/bindings/iio/adc/adi,ad9467.yaml | 4 + .../devicetree/bindings/iio/adc/adi,axi-adc.yaml | 7 +- MAINTAINERS | 8 + drivers/base/core.c | 14 +- drivers/iio/Kconfig | 9 + drivers/iio/Makefile | 1 + drivers/iio/adc/Kconfig | 4 +- drivers/iio/adc/ad9467.c | 283 +++++++++----- drivers/iio/adc/adi-axi-adc.c | 381 +++++-------------- drivers/iio/buffer/industrialio-buffer-dmaengine.c | 8 +- drivers/iio/industrialio-backend.c | 411 +++++++++++++++++++++ drivers/of/property.c | 2 + include/linux/iio/adc/adi-axi-adc.h | 68 ---- include/linux/iio/backend.h | 73 ++++ include/linux/iio/buffer-dmaengine.h | 3 + 15 files changed, 806 insertions(+), 470 deletions(-) --- base-commit: 3f4525f924e21d4f532517b17a20ffa5df7c0db7 change-id: 20231219-iio-backend-a3dc1a6a7a58 -- Thanks! - Nuno Sá