| Message ID | 20240905-wip-bl-ad3552r-axi-v0-iio-testing-v2-0-87d669674c00@baylibre.com |
|---|---|
| Headers | show |
| Series | iio: add support for the ad3552r AXI DAC IP | expand |
On 9/5/24 10:17 AM, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Some DACs as ad3552r need a synchronous mode setting, adding > this parameter for ad3552r and for future use on other DACs, > if needed. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > Documentation/ABI/testing/sysfs-bus-iio-dac | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac b/Documentation/ABI/testing/sysfs-bus-iio-dac > index 810eaac5533c..2f4960c79385 100644 > --- a/Documentation/ABI/testing/sysfs-bus-iio-dac > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac > @@ -59,3 +59,19 @@ Description: > multiple predefined symbols. Each symbol corresponds to a different > output, denoted as out_voltageY_rawN, where N is the integer value > of the symbol. Writing an integer value N will select out_voltageY_rawN. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_voltage_synchronous_mode > +KernelVersion: 6.13 > +Contact: linux-iio@vger.kernel.org > +Description: > + Arm or disarm a wait-for-synchronization flag. Arming this flag > + means the DAC will wait for a synchronizatiopn signal on a > + specific internal or external wired connection. I.e., there are > + cases where multiple DACs IP are built in the same chip or fpga > + design, and they need to start the data stream synchronized. > + > +What: /sys/bus/iio/devices/iio:deviceX/out_voltage_synchronous_mode_available > +KernelVersion: 6.13 > +Contact: linux-iio@vger.kernel.org > +Description: > + List of available values for synchronous_mode. > Since this depends on how things are wired, it seems like this should be something specified in the devicetree, not through sysfs attributes.
On 9/5/24 10:17 AM, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Change to obtain the fdt use case as reported in the > adi,ad3552r.yaml file in this patchset, with the DAC device that > is actually using the backend set as a child node of the backend. > > To obtain this, registering all the child fdt nodes as platform > devices. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > Co-developed-by: David Lechner <dlechner@baylibre.com> > Co-developed-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > index cc31e1dcd1df..e883cd557b6a 100644 > --- a/drivers/iio/dac/adi-axi-dac.c > +++ b/drivers/iio/dac/adi-axi-dac.c > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev) > { > struct axi_dac_state *st; > const struct axi_dac_info *info; > + struct platform_device *child_pdev; > void __iomem *base; > unsigned int ver; > struct clk *clk; > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev) > return dev_err_probe(&pdev->dev, ret, > "failed to register iio backend\n"); > > + device_for_each_child_node_scoped(&pdev->dev, child) { This should use fwnode_for_each_available_child_node() so that it skips nodes with status != "okay". Would be nice to introduce a scoped version of this function too. Also, if we are allowing multiple devices on the bus, the DT bindings need to have a reg property that is unique for each child. > + struct platform_device_info pi; > + > + memset(&pi, 0, sizeof(pi)); struct platform_device_info pi = { }; avoids the need for memset(). > + > + pi.name = fwnode_get_name(child); > + pi.id = PLATFORM_DEVID_AUTO; > + pi.fwnode = child; Need to have pi.parent = &pdev->dev; It could also make sense to have all of the primary bus functions (reg read/write, ddr enable/disable, etc.) passed here as platform data instead of having everything go through the IIO backend. > + > + child_pdev = platform_device_register_full(&pi); > + if (IS_ERR(child_pdev)) > + return PTR_ERR(child_pdev); These devices need to be unregistered on any error return and when the parent device is removed. > + } > + > dev_info(&pdev->dev, "AXI DAC IP core (%d.%.2d.%c) probed\n", > ADI_AXI_PCORE_VER_MAJOR(ver), > ADI_AXI_PCORE_VER_MINOR(ver), >
On 9/5/24 10:17 AM, Angelo Dureghello wrote: > The serie comes from the previously discussed RFC, that i > converted to a normal patch from this v2. > > Purpose is to add ad3552r AXI DAC (fpga-based) support. > > The fpga DAC IP has been created to reach the maximum speed > (33MUPS) supported from the ad3552r. To obtain the maximum > transfer rate, the custom module has been implemented using > the QSPI lines in DDR mode, using a dma buffer. > > The design is actually using the DAC backend since the register > map is the same of the generic DAC IP, except for some customized > bitfields. For this reason, a new "compatible" has been added > in adi-axi-dac.c. > > Also, backend has been extended with all the needed functions > needed for this use case, keeping the names gneric. > > Note: the following patch is actually for linux-iio/testing > --- > Changes in v2: > - use unsigned int on bus_reg_read/write > - add a compatible in axi-dac backend for the ad3552r DAC IP > - minor code alignment fixes > - fix a return value not checked > - change devicetree structure setting ad3552r-axi as a backend > subnode > - add synchronous_mode_available in the ABI doc > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > Co-developed-by: David Lechner <dlechner@baylibre.com> > Co-developed-by: Nuno Sá <nuno.sa@analog.com> > We didn't actually write any of the code, so I don't think Co-developed-by: is the right way to give us credit. But we can give our Reviewed-by: tags when this is ready to be merged.
On 9/5/24 10:17 AM, Angelo Dureghello wrote: ... > + > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int err, ch = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: { > + int clk_rate; > + > + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, > + IIO_CHAN_INFO_FREQUENCY); This seems odd to me. How does the backend know what frequency we want? It would make more sense to me if this somehow indicated that we were getting the SPI SCLK rate. > + if (err != IIO_VAL_INT) Would be better to call the variable ret instead of err if it can hold something besides an error code. > + return err; > + > + /* > + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). > + * Samplerate has sense in DDR only. We should also mention that this assumes QSPI in addtion to DDR enabled. > + */ > + if (st->single_channel) > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > + else > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > + Having the sample rate depend on how many channels are enabled in the buffer seems a bit odd. Sampling frequency is not strictly defined in IIO, so I think it would be fine to always return the same value no matter how many channels are enabled. We will just need to document that the sampling frequency is the rate per sample, not per channel. So if two channels are enabled, the effective sampling rate per channel is 1/2 of the sampling rate reported by the sysfs attribute. > + *val = clk_rate; > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_RAW: Do we need iio_device_claim_direct_scoped() here to prevent attempting to do register access while a buffered write is in progress? > + err = iio_backend_bus_reg_read(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(ch), > + val, 2); > + if (err) > + return err; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_axi_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int ch = chan->channel; > + > + return iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(ch), val, 2); > + } > + unreachable(); > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + struct iio_backend_data_fmt fmt = { > + .type = IIO_BACKEND_DATA_UNSIGNED > + }; > + int loop_len, val, err; > + > + /* Inform DAC chip to switch into DDR mode */ > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > + if (err) > + return err; > + > + /* Inform DAC IP to go for DDR mode from now on */ > + err = iio_backend_ddr_enable(st->back); > + if (err) > + goto exit_err; Might want a comment or dev_warn() here. If we put the DAC in DDR mode but can't put the backend in DDR mode, then things are probably going to be a bit broken if we can't get the DAC back out of DDR mode. Not likely to ever get an error here, but it will be helpful to let readers of the code know why the unwinding isn't exactly balanced. > +> + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: > + st->single_channel = false; > + loop_len = AD3552R_STREAM_4BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + default: > + return -EINVAL; Move the switch statement before changing to DDR mode or goto exit_err here. > + } > + > + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + loop_len, 1); > + if (err) > + goto exit_err; > + > + err = iio_backend_data_transfer_addr(st->back, val); > + if (err) > + goto exit_err; > + > + /* > + * The EXT_SYNC is mandatory in the CN0585 project where 2 instances > + * of the IP are in the design and they need to generate the signals > + * synchronized. > + * > + * Note: in first IP implementations CONFIG EXT_SYNC (RO) can be 0, > + * but EXT_SYMC (ext synch ability) is enabled anyway. EXT_SYNC > + */ > + if (st->synced_transfer == AD3552R_EXT_SYNC_ARM) > + err = iio_backend_ext_sync_enable(st->back); > + else > + err = iio_backend_ext_sync_disable(st->back); > + if (err) > + goto exit_err_sync; goto exit_err; If enabling failed, no need to disable. > + > + err = iio_backend_data_format_set(st->back, 0, &fmt); > + if (err) > + goto exit_err_sync; > + > + err = iio_backend_buffer_enable(st->back); > + if (err) > + goto exit_err_sync; > + > + return 0; > + > +exit_err_sync: > + iio_backend_ext_sync_disable(st->back); > + > +exit_err: > + axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + > + iio_backend_ddr_disable(st->back); > + > + return err; > +} > + > +static int ad3552r_axi_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int err; > + > + err = iio_backend_buffer_disable(st->back); > + if (err) > + return err; Looks like iio_backend_ext_sync_disable(st->back); should be called here. > + > + /* Inform DAC to set in SDR mode */ > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + if (err) > + return err; > + > + return iio_backend_ddr_disable(st->back); > +} > +
On 9/5/24 10:17 AM, Angelo Dureghello wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep > is removed. Variables (arrays) that was used to call ad3552r_field_prep > are removed too. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- Can we also remove enum ad3552r_dev_attributes and enum ad3552r_ch_attributes? It looks like they aren't used any more after these changes.
On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Change to obtain the fdt use case as reported in the > > adi,ad3552r.yaml file in this patchset, with the DAC device that > > is actually using the backend set as a child node of the backend. > > > > To obtain this, registering all the child fdt nodes as platform > > devices. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > Co-developed-by: David Lechner <dlechner@baylibre.com> > > Co-developed-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > > index cc31e1dcd1df..e883cd557b6a 100644 > > --- a/drivers/iio/dac/adi-axi-dac.c > > +++ b/drivers/iio/dac/adi-axi-dac.c > > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev) > > { > > struct axi_dac_state *st; > > const struct axi_dac_info *info; > > + struct platform_device *child_pdev; > > void __iomem *base; > > unsigned int ver; > > struct clk *clk; > > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev) > > return dev_err_probe(&pdev->dev, ret, > > "failed to register iio backend\n"); > > > > + device_for_each_child_node_scoped(&pdev->dev, child) { > > This should use fwnode_for_each_available_child_node() so that it skips > nodes with status != "okay". > device_for_each_child_node() already only looks at available nodes IIRC - Nuno Sá
On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Change to obtain the fdt use case as reported in the > > adi,ad3552r.yaml file in this patchset, with the DAC device that > > is actually using the backend set as a child node of the backend. > > > > To obtain this, registering all the child fdt nodes as platform > > devices. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > Co-developed-by: David Lechner <dlechner@baylibre.com> > > Co-developed-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > > index cc31e1dcd1df..e883cd557b6a 100644 > > --- a/drivers/iio/dac/adi-axi-dac.c > > +++ b/drivers/iio/dac/adi-axi-dac.c > > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev) > > { > > struct axi_dac_state *st; > > const struct axi_dac_info *info; > > + struct platform_device *child_pdev; > > void __iomem *base; > > unsigned int ver; > > struct clk *clk; > > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev) > > return dev_err_probe(&pdev->dev, ret, > > "failed to register iio backend\n"); > > > > + device_for_each_child_node_scoped(&pdev->dev, child) { > > This should use fwnode_for_each_available_child_node() so that it skips > nodes with status != "okay". > > Would be nice to introduce a scoped version of this function too. > > Also, if we are allowing multiple devices on the bus, the DT bindings > need to have a reg property that is unique for each child. > > > + struct platform_device_info pi; > > + > > + memset(&pi, 0, sizeof(pi)); > > struct platform_device_info pi = { }; > > avoids the need for memset(). > > > + > > + pi.name = fwnode_get_name(child); > > + pi.id = PLATFORM_DEVID_AUTO; > > + pi.fwnode = child; > > Need to have pi.parent = &pdev->dev; > > It could also make sense to have all of the primary bus functions > (reg read/write, ddr enable/disable, etc.) passed here as platform > data instead of having everything go through the IIO backend. Note that ddr enable/disable is something that makes sense to be in the backend anyways as it is something that exists in LVDS/CMOS interfaces that are only running the dataplane. Bus operations like read/write could make sense but that would mean an interface directly between the axi-dac and the child devices (bypassing the backend or any other middle layer - maybe we could create a tiny adi-axi-bus layer on the IIO topdir or any other place in IIO) which I'm not so sure (and is a bit odd). OTOH, this bus stuff goes a bit out of scope of the backend main idea/goal so yeah... Well, let's see what others have to say about it but I don't dislike the idea. > > > + > > + child_pdev = platform_device_register_full(&pi); > > + if (IS_ERR(child_pdev)) > > + return PTR_ERR(child_pdev); > > These devices need to be unregistered on any error return and when > the parent device is removed. > Definitely this needs to be tested by manually unbinding the axi-dac device for example. I'm not really sure how this will look like and if there's any problem in removing twice the same device (likely there is). The thing is that when we connect a frontend with it's backend, a devlink is created (that guarantees that the frontend is removed before the backend). So, I'm fairly confident that if we add a devm action in here to unregister the child devices, by the time we unregister the child, it should be already gone (unless driver core somehow handles this). All of the above needs careful testing but one way out it (and since in here we have the parent - child relationship), we could add a boolean flag 'skip_devlink' to 'struct iio_backend_info' so that devlinks are skipped on these arrangements. Or we could automatically detect that the frontend is a child of the backend and skip the link (though an explicit flag might be better). - Nuno Sá >
On Thu, Sep 05, 2024 at 05:17:30PM +0200, Angelo Dureghello wrote: > The serie comes from the previously discussed RFC, that i > converted to a normal patch from this v2. > > Purpose is to add ad3552r AXI DAC (fpga-based) support. > > The fpga DAC IP has been created to reach the maximum speed > (33MUPS) supported from the ad3552r. To obtain the maximum > transfer rate, the custom module has been implemented using > the QSPI lines in DDR mode, using a dma buffer. > > The design is actually using the DAC backend since the register > map is the same of the generic DAC IP, except for some customized > bitfields. For this reason, a new "compatible" has been added > in adi-axi-dac.c. > > Also, backend has been extended with all the needed functions > needed for this use case, keeping the names gneric. > > Note: the following patch is actually for linux-iio/testing > --- > Changes in v2: > - use unsigned int on bus_reg_read/write > - add a compatible in axi-dac backend for the ad3552r DAC IP > - minor code alignment fixes > - fix a return value not checked > - change devicetree structure setting ad3552r-axi as a backend > subnode > - add synchronous_mode_available in the ABI doc Please give reviewers a chance to response to in-progress discussion on a version before sending a new one. I've left a couple of responses to v1 that I only had a chance to reply to today due to travel.
Hi Conor, On 06/09/24 11:07 AM, Conor Dooley wrote: > On Thu, Sep 05, 2024 at 05:17:30PM +0200, Angelo Dureghello wrote: >> The serie comes from the previously discussed RFC, that i >> converted to a normal patch from this v2. >> >> Purpose is to add ad3552r AXI DAC (fpga-based) support. >> >> The fpga DAC IP has been created to reach the maximum speed >> (33MUPS) supported from the ad3552r. To obtain the maximum >> transfer rate, the custom module has been implemented using >> the QSPI lines in DDR mode, using a dma buffer. >> >> The design is actually using the DAC backend since the register >> map is the same of the generic DAC IP, except for some customized >> bitfields. For this reason, a new "compatible" has been added >> in adi-axi-dac.c. >> >> Also, backend has been extended with all the needed functions >> needed for this use case, keeping the names gneric. >> >> Note: the following patch is actually for linux-iio/testing >> --- >> Changes in v2: >> - use unsigned int on bus_reg_read/write >> - add a compatible in axi-dac backend for the ad3552r DAC IP >> - minor code alignment fixes >> - fix a return value not checked >> - change devicetree structure setting ad3552r-axi as a backend >> subnode >> - add synchronous_mode_available in the ABI doc > Please give reviewers a chance to response to in-progress discussion on > a version before sending a new one. I've left a couple of responses to > v1 that I only had a chance to reply to today due to travel. sure, will wait some more days next time. Regards,
On 9/6/24 12:42 AM, Nuno Sá wrote: > On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote: >> On 9/5/24 10:17 AM, Angelo Dureghello wrote: >>> From: Angelo Dureghello <adureghello@baylibre.com> >>> >>> Change to obtain the fdt use case as reported in the >>> adi,ad3552r.yaml file in this patchset, with the DAC device that >>> is actually using the backend set as a child node of the backend. >>> >>> To obtain this, registering all the child fdt nodes as platform >>> devices. >>> >>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >>> Co-developed-by: David Lechner <dlechner@baylibre.com> >>> Co-developed-by: Nuno Sá <nuno.sa@analog.com> >>> --- >>> drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c >>> index cc31e1dcd1df..e883cd557b6a 100644 >>> --- a/drivers/iio/dac/adi-axi-dac.c >>> +++ b/drivers/iio/dac/adi-axi-dac.c >>> @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev) >>> { >>> struct axi_dac_state *st; >>> const struct axi_dac_info *info; >>> + struct platform_device *child_pdev; >>> void __iomem *base; >>> unsigned int ver; >>> struct clk *clk; >>> @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev) >>> return dev_err_probe(&pdev->dev, ret, >>> "failed to register iio backend\n"); >>> >>> + device_for_each_child_node_scoped(&pdev->dev, child) { >> >> This should use fwnode_for_each_available_child_node() so that it skips >> nodes with status != "okay". >> > > device_for_each_child_node() already only looks at available nodes IIRC > > - Nuno Sá > Ah, you are right, I did not dig deep enough.
On Thu, 5 Sep 2024 14:14:37 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Some DACs as ad3552r need a synchronous mode setting, adding > > this parameter for ad3552r and for future use on other DACs, > > if needed. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > --- > > Documentation/ABI/testing/sysfs-bus-iio-dac | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-dac b/Documentation/ABI/testing/sysfs-bus-iio-dac > > index 810eaac5533c..2f4960c79385 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-iio-dac > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-dac > > @@ -59,3 +59,19 @@ Description: > > multiple predefined symbols. Each symbol corresponds to a different > > output, denoted as out_voltageY_rawN, where N is the integer value > > of the symbol. Writing an integer value N will select out_voltageY_rawN. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/out_voltage_synchronous_mode > > +KernelVersion: 6.13 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Arm or disarm a wait-for-synchronization flag. Arming this flag > > + means the DAC will wait for a synchronizatiopn signal on a > > + specific internal or external wired connection. I.e., there are > > + cases where multiple DACs IP are built in the same chip or fpga > > + design, and they need to start the data stream synchronized. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/out_voltage_synchronous_mode_available > > +KernelVersion: 6.13 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + List of available values for synchronous_mode. > > > > Since this depends on how things are wired, it seems like this should be > something specified in the devicetree, not through sysfs attributes. > Agreed. Smells like a wiring thing given the description. Is there a case where it works either way and it is usecase dependent which choice makes sense? Superficially it seems likely if a board has this wired, there is little disadvantage in using it always. Jonathan >
On Fri, 06 Sep 2024 09:08:59 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote: > > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > Change to obtain the fdt use case as reported in the > > > adi,ad3552r.yaml file in this patchset, with the DAC device that > > > is actually using the backend set as a child node of the backend. > > > > > > To obtain this, registering all the child fdt nodes as platform > > > devices. > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > Co-developed-by: David Lechner <dlechner@baylibre.com> > > > Co-developed-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++ > > > 1 file changed, 15 insertions(+) > > > > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > > > index cc31e1dcd1df..e883cd557b6a 100644 > > > --- a/drivers/iio/dac/adi-axi-dac.c > > > +++ b/drivers/iio/dac/adi-axi-dac.c > > > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev) > > > { > > > struct axi_dac_state *st; > > > const struct axi_dac_info *info; > > > + struct platform_device *child_pdev; > > > void __iomem *base; > > > unsigned int ver; > > > struct clk *clk; > > > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev) > > > return dev_err_probe(&pdev->dev, ret, > > > "failed to register iio backend\n"); > > > > > > + device_for_each_child_node_scoped(&pdev->dev, child) { > > > > This should use fwnode_for_each_available_child_node() so that it skips > > nodes with status != "okay". Ah. That oddity strikes again... > > > > Would be nice to introduce a scoped version of this function too. > > > > Also, if we are allowing multiple devices on the bus, the DT bindings > > need to have a reg property that is unique for each child. > > > > > + struct platform_device_info pi; > > > + > > > + memset(&pi, 0, sizeof(pi)); > > > > struct platform_device_info pi = { }; > > > > avoids the need for memset(). > > > > > + > > > + pi.name = fwnode_get_name(child); > > > + pi.id = PLATFORM_DEVID_AUTO; > > > + pi.fwnode = child; > > > > Need to have pi.parent = &pdev->dev; > > > > It could also make sense to have all of the primary bus functions > > (reg read/write, ddr enable/disable, etc.) passed here as platform > > data instead of having everything go through the IIO backend. > > Note that ddr enable/disable is something that makes sense to be in the backend > anyways as it is something that exists in LVDS/CMOS interfaces that are only running > the dataplane. Bus operations like read/write could make sense but that would mean an > interface directly between the axi-dac and the child devices (bypassing the backend > or any other middle layer - maybe we could create a tiny adi-axi-bus layer on the IIO > topdir or any other place in IIO) which I'm not so sure (and is a bit odd). OTOH, > this bus stuff goes a bit out of scope of the backend main idea/goal so yeah... Well, > let's see what others have to say about it but I don't dislike the idea. For the read/write using platform data does seem reasonable to me. Agreed that DDR is dataplane (at least sometimes) so backend ops probably appropriate. > > > > > > + > > > + child_pdev = platform_device_register_full(&pi); > > > + if (IS_ERR(child_pdev)) > > > + return PTR_ERR(child_pdev); > > > > These devices need to be unregistered on any error return and when > > the parent device is removed. > > > > Definitely this needs to be tested by manually unbinding the axi-dac device for > example. I'm not really sure how this will look like and if there's any problem in > removing twice the same device (likely there is). The thing is that when we connect a > frontend with it's backend, a devlink is created (that guarantees that the frontend > is removed before the backend). So, I'm fairly confident that if we add a devm action > in here to unregister the child devices, by the time we unregister the child, it > should be already gone (unless driver core somehow handles this). > > All of the above needs careful testing but one way out it (and since in here we have > the parent - child relationship), we could add a boolean flag 'skip_devlink' to > 'struct iio_backend_info' so that devlinks are skipped on these arrangements. Or we > could automatically detect that the frontend is a child of the backend and skip the > link (though an explicit flag might be better). Agreed it needs testing but I'm not sure why it would already have gone. The driver would have unbound, but the platform device /child would still be there I think at time of removal. Can probably get away with devm to tear it down when the backend device then goes away. Maybe I'm missing some subtlety though. Jonathan > > - Nuno Sá > > >
On Thu, 05 Sep 2024 17:17:32 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Extend backend features with new calls needed later on this > patchset from axi version of ad3552r. > > A bus type property has been added to the devicetree to > inform the backend about the type of bus (interface) in use > bu the IP. > > The follwoing calls are added: > > iio_backend_ext_sync_enable > enable synchronize channels on external trigger > iio_backend_ext_sync_disable > disable synchronize channels on external trigger > iio_backend_ddr_enable > enable ddr bus transfer > iio_backend_ddr_disable > disable ddr bus transfer > iio_backend_set_bus_mode > select the type of bus, so that specific read / write > operations are performed accordingly > iio_backend_buffer_enable > enable buffer > iio_backend_buffer_disable > disable buffer > iio_backend_data_transfer_addr > define the target register address where the DAC sample > will be written. > iio_backend_bus_reg_read > generic bus read, bus-type dependent > iio_backend_bus_read_write > generic bus write, bus-type dependent The RAMP_16 definition doesn't seem immediately connected to the rest. + I'm not sure what it is from the name. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > drivers/iio/industrialio-backend.c | 157 +++++++++++++++++++++++++++++++++++++ > include/linux/iio/backend.h | 33 ++++++++ > 2 files changed, 190 insertions(+) > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c > index 20b3b5212da7..231bef4b560e 100644 > --- a/drivers/iio/industrialio-backend.c > +++ b/drivers/iio/industrialio-backend.c > @@ -718,6 +718,163 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) > return 0; > } > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h > index 37d56914d485..eb8c5bb74bb5 100644 > --- a/include/linux/iio/backend.h > +++ b/include/linux/iio/backend.h > > enum iio_backend_data_source { > IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE, > IIO_BACKEND_EXTERNAL, > + IIO_BACKEND_INTERNAL_RAMP_16, Not obvious what this is so maybe this enum needs docs in general. > IIO_BACKEND_DATA_SOURCE_MAX > };
On Thu, 05 Sep 2024 17:17:33 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Extend DAC backend with new features required for the AXI driver > version for the ad3552r DAC. Mainly, a new compatible string has > been added to support a DAC IP very similar to the generic DAC IP > but with some customizations to work with the ad3552r. > > Then, a serie of generic functions has been added to match with > ad3552r needs. Function names has been kept generic as much as > possible, to allow re-utilization from other frontend drivers. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > Co-developed-by: David Lechner <dlechner@baylibre.com> > Co-developed-by: Nuno Sá <nuno.sa@analog.com> A few comments below. > --- > drivers/iio/dac/adi-axi-dac.c | 267 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 257 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c > index 0cb00f3bec04..cc31e1dcd1df 100644 > --- a/drivers/iio/dac/adi-axi-dac.c > +++ b/drivers/iio/dac/adi-axi-dac.c > @@ -44,11 +44,34 @@ > #define AXI_DAC_RSTN_MMCM_RSTN BIT(1) > #define AXI_DAC_RSTN_RSTN BIT(0) > #define AXI_DAC_REG_CNTRL_1 0x0044 > +#define AXI_DAC_EXT_SYNC_ARM BIT(1) > +#define AXI_DAC_EXT_SYNC_DISARM BIT(2) > #define AXI_DAC_SYNC BIT(0) > #define AXI_DAC_REG_CNTRL_2 0x0048 > -#define ADI_DAC_R1_MODE BIT(4) > +#define AXI_DAC_SDR_DDR_N BIT(16) > +#define AXI_DAC_SYMB_8B BIT(14) > +#define ADI_DAC_R1_MODE BIT(5) Bug? Either was wrong before or after this change. I've no idea which. > + > +static int axi_dac_bus_reg_write(struct iio_backend *back, u32 reg, > + unsigned int val, size_t data_size) > +{ > + struct axi_dac_state *st = iio_backend_get_priv(back); > + > + switch (st->info->bus_type) { > + case AXI_DAC_BUS_TYPE_DDR_QSPI: { ... > + > + return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, > + AXI_DAC_TRANSFER_DATA); > + } > + break; Can't get here so drop the break; > + default: > + return -EOPNOTSUPP; > + } > +} > + > +static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg, > + unsigned int *val, size_t data_size) > +{ > + struct axi_dac_state *st = iio_backend_get_priv(back); > + > + switch (st->info->bus_type) { > + case AXI_DAC_BUS_TYPE_DDR_QSPI: { > + int ret; > + u32 bval; > + > + bval = 0; > + ret = axi_dac_bus_reg_write(back, AXI_DAC_RD_ADDR(reg), 0, > + data_size); > + if (ret) > + return ret; > + > + ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS, > + bval, bval != AXI_DAC_BUSY, > + 10, 100); > + if (ret) > + return ret; > + > + return regmap_read(st->regmap, AXI_DAC_CNTRL_DATA_RD, val); > + } > + break; Can't get here. > + default: > + return -EOPNOTSUPP; > + } > +}
On Thu, 05 Sep 2024 17:17:36 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep > is removed. Variables (arrays) that was used to call ad3552r_field_prep > are removed too. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> If it isn't too hard I would drag this to the start of the patch set because I'll happily pick it up as a cleanup before any discussion of later patches is complete. That will reduce what people need to look at for future versions. Jonathan
On Sun, 8 Sep 2024 16:14:20 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Thu, 05 Sep 2024 17:17:36 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > Changes to use FIELD_PREP, so that driver-specific ad3552r_field_prep > > is removed. Variables (arrays) that was used to call ad3552r_field_prep > > are removed too. > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > If it isn't too hard I would drag this to the start of the patch set > because I'll happily pick it up as a cleanup before any discussion of later > patches is complete. That will reduce what people need to look at for > future versions. > > Jonathan I'm being dopey. None of the earlier patches touched this file anyway so I can do that once you've addressed David's comment.
Le 05/09/2024 à 17:17, Angelo Dureghello a écrit : > From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > Extend DAC backend with new features required for the AXI driver > version for the ad3552r DAC. Mainly, a new compatible string has > been added to support a DAC IP very similar to the generic DAC IP > but with some customizations to work with the ad3552r. > > Then, a serie of generic functions has been added to match with > ad3552r needs. Function names has been kept generic as much as > possible, to allow re-utilization from other frontend drivers. Hi, ... > +static int axi_dac_read_raw(struct iio_backend *back, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct axi_dac_state *st = iio_backend_get_priv(back); > + > + switch (mask) { > + case IIO_CHAN_INFO_FREQUENCY: > + *val = clk_get_rate(devm_clk_get(st->dev, 0)); Having a devm_clk_get() in such a place is really unusual. Is it correct? This look like a memory leak to me. > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} ... > + /* > + * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know > + * the data size. So keeping data size control here only, > + * since data size is mandatory for to the current transfer. "... for to ..." sounds strange to my *non*-native English ears. > + * DDR state handled separately by specific backend calls, > + * generally all raw register writes are SDR. > + */ > + if (data_size == 1) > + ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2, > + AXI_DAC_SYMB_8B); > + else > + ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2, > + AXI_DAC_SYMB_8B); > + if (ret) > + return ret; ... > @@ -556,10 +792,12 @@ static int axi_dac_probe(struct platform_device *pdev) > if (!st) > return -ENOMEM; > > - expected_ver = device_get_match_data(&pdev->dev); > - if (!expected_ver) > + info = device_get_match_data(&pdev->dev); > + if (!info) writing: st->info = device_get_match_data(&pdev->dev); if (!st->info) would save the 'info' variable and a few lines of code without loosing (IMHO) readability. CJ > return -ENODEV; > > + st->info = info; > + > clk = devm_clk_get_enabled(&pdev->dev, NULL); > if (IS_ERR(clk)) > return dev_err_probe(&pdev->dev, PTR_ERR(clk), > @@ -588,12 +826,13 @@ static int axi_dac_probe(struct platform_device *pdev) > if (ret) > return ret; > > - if (ADI_AXI_PCORE_VER_MAJOR(ver) != ADI_AXI_PCORE_VER_MAJOR(*expected_ver)) { > + if (ADI_AXI_PCORE_VER_MAJOR(ver) != > + ADI_AXI_PCORE_VER_MAJOR(st->info->version)) { > dev_err(&pdev->dev, > "Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n", > - 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(st->info->version), > + ADI_AXI_PCORE_VER_MINOR(st->info->version), > + ADI_AXI_PCORE_VER_PATCH(st->info->version), > ADI_AXI_PCORE_VER_MAJOR(ver), > ADI_AXI_PCORE_VER_MINOR(ver), > ADI_AXI_PCORE_VER_PATCH(ver)); > @@ -631,10 +870,18 @@ static int axi_dac_probe(struct platform_device *pdev) > return 0; > } ...
On Thu, 05 Sep 2024 17:17:37 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Extracting common code, to share common code to be used later > by the AXI driver version (ad3552r-axi.c). > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> Looks like a few arrays are now duplicated that probably shouldn't be. Otherwise, just trivial comments inline. Jonathan > --- > drivers/iio/dac/Makefile | 2 +- > drivers/iio/dac/ad3552r-common.c | 163 +++++++++++++++++++++++ > drivers/iio/dac/ad3552r.c | 276 ++++----------------------------------- > drivers/iio/dac/ad3552r.h | 199 ++++++++++++++++++++++++++++ > 4 files changed, 389 insertions(+), 251 deletions(-) > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 2cf148f16306..56a125f56284 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -4,7 +4,7 @@ > # > > # When adding new entries keep the list in alphabetical order > -obj-$(CONFIG_AD3552R) += ad3552r.o > +obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o > obj-$(CONFIG_AD5360) += ad5360.o > obj-$(CONFIG_AD5380) += ad5380.o > obj-$(CONFIG_AD5421) += ad5421.o > diff --git a/drivers/iio/dac/ad3552r-common.c b/drivers/iio/dac/ad3552r-common.c > new file mode 100644 > index 000000000000..c8ccfbe2e95e > --- /dev/null > +++ b/drivers/iio/dac/ad3552r-common.c > @@ -0,0 +1,163 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +// > +// Copyright (c) 2010-2024 Analog Devices Inc. > +// Copyright (c) 2024 Baylibre, SAS > + > +#include <linux/device.h> > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/regulator/consumer.h> > + > +#include "ad3552r.h" > + > +static const s32 ad3552r_ch_ranges[][2] = { > + [AD3552R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, Please add spaces after { and before } given the code is moving anyway and that's my preferred style (I'm getting fussier about it over time :) > + [AD3552R_CH_OUTPUT_RANGE_0__5V] = {0, 5000}, > + [AD3552R_CH_OUTPUT_RANGE_0__10V] = {0, 10000}, > + [AD3552R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000}, > + [AD3552R_CH_OUTPUT_RANGE_NEG_10__10V] = {-10000, 10000} > +}; > + > +static const s32 ad3542r_ch_ranges[][2] = { > + [AD3542R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > + [AD3542R_CH_OUTPUT_RANGE_0__3V] = {0, 3000}, > + [AD3542R_CH_OUTPUT_RANGE_0__5V] = {0, 5000}, > + [AD3542R_CH_OUTPUT_RANGE_0__10V] = {0, 10000}, > + [AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V] = {-2500, 7500}, > + [AD3542R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000} > +}; > + > +void ad3552r_calc_custom_gain(u8 p, u8 n, s16 goffs, u16 *reg) return a u16 instead of passing it in as a pointer. Fits better with how it is used I think. > +{ > + *reg = FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, p); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, n); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, abs((s32)goffs) >> 8); > + *reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)goffs < 0); > +} > + > +int ad3552r_get_ref_voltage(struct device *dev, u32 *val) > +{ > + int voltage, delta = 100000; Trivial: Don't mix declarations with assignment and ones with out on same line. It's slightly trickier to read. This driver currently does this a lot but lets not introduce more cases. > + > + voltage = devm_regulator_get_enable_read_voltage(dev, "vref"); > + if (voltage < 0 && voltage != -ENODEV) > + return dev_err_probe(dev, voltage, > + "Error getting vref voltage\n"); > + > + if (voltage == -ENODEV) { > + if (device_property_read_bool(dev, "adi,vref-out-en")) > + *val = AD3552R_INTERNAL_VREF_PIN_2P5V; If val fits nicely into 31 bits and hence the positive values, I'd return it as then these all become return AD... > + else > + *val = AD3552R_INTERNAL_VREF_PIN_FLOATING; > + } else { and this else is then unnecessary. > + if (voltage > 2500000 + delta || voltage < 2500000 - delta) { > + dev_warn(dev, "vref-supply must be 2.5V"); > + return -EINVAL; > + } > + *val = AD3552R_EXTERNAL_VREF_PIN_INPUT; > + } > + > + return 0; > +} > + > +int ad3552r_get_drive_strength(struct device *dev, u32 *val) > +{ > + int err; > + > + err = device_property_read_u32(dev, "adi,sdo-drive-strength", val); > + if (!err && *val > 3) { > + dev_err(dev, > + "adi,sdo-drive-strength must be less than 4\n"); > + return -EINVAL; if (err) return err; if (*val > 3) { dev_err(dev, "adi,sdo-drive-strength must be less than 4\n"); return -EINVAL; } return 0; is a bit longer but keeps the error handling well separated from the good path. > + } > + > + return err; > +} > + > +int ad3552r_get_output_range(struct device *dev, enum ad3552r_id chip_id, > + struct fwnode_handle *child, u32 *val) > +{ > + int ret; > + s32 vals[2]; > + > + if (!fwnode_property_present(child, "adi,output-range-microvolt")) > + return -ENOENT; Maybe add a comment that this property is optional, so we want to distinguish this particular reason for failure. Took me a few mins to figure out why we needed this check. > + > + ret = fwnode_property_read_u32_array(child, > + "adi,output-range-microvolt", > + vals, 2); > + if (ret) > + return dev_err_probe(dev, ret, > + "invalid adi,output-range-microvolt\n"); > + > + ret = ad3552r_find_range(chip_id, vals); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "invalid adi,output-range-microvolt value\n"); > + > + *val = ret; > + > + return 0; > +} > diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c > index d867de7c90d1..c149be9c8c7d 100644 > --- a/drivers/iio/dac/ad3552r.c > +++ b/drivers/iio/dac/ad3552r.c > @@ -11,153 +11,9 @@ > -enum ad3552r_ch_output_range { > - /* Range from 0 V to 2.5 V. Requires Rfb1x connection */ > - AD3552R_CH_OUTPUT_RANGE_0__2P5V, > - /* Range from 0 V to 5 V. Requires Rfb1x connection */ > - AD3552R_CH_OUTPUT_RANGE_0__5V, > - /* Range from 0 V to 10 V. Requires Rfb2x connection */ > - AD3552R_CH_OUTPUT_RANGE_0__10V, > - /* Range from -5 V to 5 V. Requires Rfb2x connection */ > - AD3552R_CH_OUTPUT_RANGE_NEG_5__5V, > - /* Range from -10 V to 10 V. Requires Rfb4x connection */ > - AD3552R_CH_OUTPUT_RANGE_NEG_10__10V, > -}; > +#include "ad3552r.h" > > static const s32 ad3552r_ch_ranges[][2] = { This exists now in ad3552-common.c as well as here which seems unlikely to be what you want. > [AD3552R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > @@ -167,21 +23,6 @@ static const s32 ad3552r_ch_ranges[][2] = { > [AD3552R_CH_OUTPUT_RANGE_NEG_10__10V] = {-10000, 10000} > }; > > -enum ad3542r_ch_output_range { > - /* Range from 0 V to 2.5 V. Requires Rfb1x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__2P5V, > - /* Range from 0 V to 3 V. Requires Rfb1x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__3V, > - /* Range from 0 V to 5 V. Requires Rfb1x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__5V, > - /* Range from 0 V to 10 V. Requires Rfb2x connection */ > - AD3542R_CH_OUTPUT_RANGE_0__10V, > - /* Range from -2.5 V to 7.5 V. Requires Rfb2x connection */ > - AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V, > - /* Range from -5 V to 5 V. Requires Rfb2x connection */ > - AD3542R_CH_OUTPUT_RANGE_NEG_5__5V, > -}; > - > static const s32 ad3542r_ch_ranges[][2] = { Isn't this in the new location above? > [AD3542R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > [AD3542R_CH_OUTPUT_RANGE_0__3V] = {0, 3000}, > @@ -733,72 +574,32 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch) > dac->ch_data[ch].offset_dec = div_s64(tmp, span); > } > > static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac, > struct fwnode_handle *child, > u32 ch) > { > struct device *dev = &dac->spi->dev; > - u32 val; > int err; > u8 addr; > - u16 reg = 0, offset; > - > - struct fwnode_handle *gain_child __free(fwnode_handle) > - = fwnode_get_named_child_node(child, > - "custom-output-range-config"); > - if (!gain_child) > - return dev_err_probe(dev, -EINVAL, > - "mandatory custom-output-range-config property missing\n"); > - > - dac->ch_data[ch].range_override = 1; > - reg |= FIELD_PREP(AD3552R_MASK_CH_RANGE_OVERRIDE, 1); > - > - err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val); > - if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,gain-scaling-p property missing\n"); > - reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_P, val); > - dac->ch_data[ch].p = val; > + u16 reg = 0; > > - err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val); > + err = ad3552r_get_custom_gain(dev, child, > + &dac->ch_data[ch].p, > + &dac->ch_data[ch].n, > + &dac->ch_data[ch].rfb, > + &dac->ch_data[ch].gain_offset); > if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,gain-scaling-n property missing\n"); > - reg |= FIELD_PREP(AD3552R_MASK_CH_GAIN_SCALING_N, val); > - dac->ch_data[ch].n = val; > - > - err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val); > - if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,rfb-ohms property missing\n"); > - dac->ch_data[ch].rfb = val; > + return err; > > - err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val); > - if (err) > - return dev_err_probe(dev, err, > - "mandatory adi,gain-offset property missing\n"); > - dac->ch_data[ch].gain_offset = val; > + dac->ch_data[ch].range_override = 1; > > - offset = abs((s32)val); > - reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_BIT_8, (offset >> 8)); > + ad3552r_calc_custom_gain(dac->ch_data[ch].p, dac->ch_data[ch].n, > + dac->ch_data[ch].gain_offset, ®); > > - reg |= FIELD_PREP(AD3552R_MASK_CH_OFFSET_POLARITY, (s32)val < 0); > addr = AD3552R_REG_ADDR_CH_GAIN(ch); > err = ad3552r_write_reg(dac, addr, Given you now have the calculation of reg nice and compact, I'd move that after this call. It's odd to calculate a value first then not use it whilst doing some other stuff. > - offset & AD3552R_MASK_CH_OFFSET_BITS_0_7); > + abs((s32)dac->ch_data[ch].gain_offset) & > + AD3552R_MASK_CH_OFFSET_BITS_0_7); > if (err) > return dev_err_probe(dev, err, "Error writing register\n"); > > @@ -812,30 +613,17 @@ static int ad3552r_configure_custom_gain(struct ad3552r_desc *dac, > static int ad3552r_configure_device(struct ad3552r_desc *dac) > { > struct device *dev = &dac->spi->dev; > - int err, cnt = 0, voltage, delta = 100000; > - u32 vals[2], val, ch; > + int err, cnt = 0; > + u32 val, ch; > > dac->gpio_ldac = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_HIGH); > if (IS_ERR(dac->gpio_ldac)) > return dev_err_probe(dev, PTR_ERR(dac->gpio_ldac), > "Error getting gpio ldac"); > > - voltage = devm_regulator_get_enable_read_voltage(dev, "vref"); > - if (voltage < 0 && voltage != -ENODEV) > - return dev_err_probe(dev, voltage, "Error getting vref voltage\n"); > - > - if (voltage == -ENODEV) { > - if (device_property_read_bool(dev, "adi,vref-out-en")) > - val = AD3552R_INTERNAL_VREF_PIN_2P5V; > - else > - val = AD3552R_INTERNAL_VREF_PIN_FLOATING; > - } else { > - if (voltage > 2500000 + delta || voltage < 2500000 - delta) { > - dev_warn(dev, "vref-supply must be 2.5V"); > - return -EINVAL; > - } > - val = AD3552R_EXTERNAL_VREF_PIN_INPUT; > - } > + err = ad3552r_get_ref_voltage(dev, &val); > + if (err) > + return err; With suggestion above, err = ad3552_get_ref_voltage(dev); if (err) return err; val = err; > > err = ad3552r_update_reg_field(dac, > AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, ... > diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h > new file mode 100644 > index 000000000000..cada1f12f000 > --- /dev/null > +++ b/drivers/iio/dac/ad3552r.h > @@ -0,0 +1,199 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * AD3552R Digital <-> Analog converters common header > + * > + * Copyright 2024 Analog Devices Inc. > + * Author: Angelo Dureghello <adureghello@baylibre.com> It's all code lifted from elsewhere, so keep the original 2021 date as well. 2021-2024 would be fine. > + */ ...
On Thu, 5 Sep 2024 15:40:11 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > ... One reply to a comment David made. Jonathan > > > + */ > > + if (st->single_channel) > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > > + else > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > > + > > Having the sample rate depend on how many channels are enabled in > the buffer seems a bit odd. Sampling frequency is not strictly > defined in IIO, so I think it would be fine to always return the > same value no matter how many channels are enabled. > > We will just need to document that the sampling frequency is the > rate per sample, not per channel. So if two channels are enabled, > the effective sampling rate per channel is 1/2 of the sampling > rate reported by the sysfs attribute. There is an oddity around this that we've never cleared up fully. In my head at least if there is a single sampling_frequency it applies to 'scans', not individual channel reads (so would change with the number of channels enabled). If there is a per channel attribute we do have documentation: What: /sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency What: /sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency What: /sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency KernelVersion: 5.20 Contact: linux-iio@vger.kernel.org Description: Some devices have separate controls of sampling frequency for individual channels. If multiple channels are enabled in a scan, then the sampling_frequency of the scan may be computed from the per channel sampling frequencies. For many devices the sampling frequency isn't down to each sample taking N microsecs and them running back to back, it is instead a function of a periodic sampling start for samples that take the same time whatever the sampling frequency. Also for simultaneous sampling ADCs it is never channel dependent. So I think if you want to avoid the confusion, make your device fall into the description above and provide a per channel attribute rather than shared_by_all. Or keep it as things stand and have it halve when you double the channels. > > > + *val = clk_rate; > > + > > + return IIO_VAL_INT; > > + } ...
Le 05/09/2024 à 17:17, Angelo Dureghello a écrit : > From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > Extracting common code, to share common code to be used later > by the AXI driver version (ad3552r-axi.c). ... > +#include "ad3552r.h" > + > +static const s32 ad3552r_ch_ranges[][2] = { > + [AD3552R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, Maybe add some spaces around { }? > + [AD3552R_CH_OUTPUT_RANGE_0__5V] = {0, 5000}, > + [AD3552R_CH_OUTPUT_RANGE_0__10V] = {0, 10000}, > + [AD3552R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000}, > + [AD3552R_CH_OUTPUT_RANGE_NEG_10__10V] = {-10000, 10000} > +}; > + > +static const s32 ad3542r_ch_ranges[][2] = { > + [AD3542R_CH_OUTPUT_RANGE_0__2P5V] = {0, 2500}, > + [AD3542R_CH_OUTPUT_RANGE_0__3V] = {0, 3000}, > + [AD3542R_CH_OUTPUT_RANGE_0__5V] = {0, 5000}, > + [AD3542R_CH_OUTPUT_RANGE_0__10V] = {0, 10000}, > + [AD3542R_CH_OUTPUT_RANGE_NEG_2P5__7P5V] = {-2500, 7500}, > + [AD3542R_CH_OUTPUT_RANGE_NEG_5__5V] = {-5000, 5000} > +}; ... > +int ad3552r_get_custom_gain(struct device *dev, struct fwnode_handle *child, > + u8 *gs_p, u8 *gs_n, u16 *rfb, s16 *goffs) > +{ > + int err; > + u32 val; > + struct fwnode_handle *gain_child __free(fwnode_handle) > + = fwnode_get_named_child_node(child, > + "custom-output-range-config"); > + > + if (!gain_child) > + return dev_err_probe(dev, -EINVAL, > + "custom-output-range-config mandatory\n"); > + > + err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-p", &val); > + if (err) > + return dev_err_probe(dev, err, > + "adi,gain-scaling-p mandatory\n"); > + *gs_p = val; > + > + err = fwnode_property_read_u32(gain_child, "adi,gain-scaling-n", &val); > + if (err) > + return dev_err_probe(dev, err, > + "adi,gain-scaling-n property mandatory\n"); > + *gs_n = val; > + > + err = fwnode_property_read_u32(gain_child, "adi,rfb-ohms", &val); > + if (err) > + return dev_err_probe(dev, err, > + "adi,rfb-ohms mandatoryn"); Missing \ in the ending \n. CJ > + *rfb = val; > + > + err = fwnode_property_read_u32(gain_child, "adi,gain-offset", &val); > + if (err) > + return dev_err_probe(dev, err, > + "adi,gain-offset mandatory\n"); > + *goffs = val; > + > + return 0; > +} ...
On Thu, 05 Sep 2024 17:17:38 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Add support for ad3552r-axi, where ad3552r has to be controlled > by the custom (fpga-based) ad3552r AXI DAC IP. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > Co-developed-by: David Lechner <dlechner@baylibre.com> > Co-developed-by: Nuno Sá <nuno.sa@analog.com> Comments inline. > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 56a125f56284..cc2af3aa3f52 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -5,6 +5,7 @@ > > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o > +obj-$(CONFIG_AD3552R_AXI) += ad3552r-axi.o ad3552r-common.o > obj-$(CONFIG_AD5360) += ad5360.o > obj-$(CONFIG_AD5380) += ad5380.o > obj-$(CONFIG_AD5421) += ad5421.o > diff --git a/drivers/iio/dac/ad3552r-axi.c b/drivers/iio/dac/ad3552r-axi.c > new file mode 100644 > index 000000000000..a9311f29f45d > --- /dev/null > +++ b/drivers/iio/dac/ad3552r-axi.c > @@ -0,0 +1,567 @@ > + > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int err, ch = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: { > + int clk_rate; > + > + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, > + IIO_CHAN_INFO_FREQUENCY); > + if (err != IIO_VAL_INT) > + return err; If it is another postive value I think you want to return -EINVAL If it's negative return err as here. > + > + /* > + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). > + * Samplerate has sense in DDR only. > + */ > + if (st->single_channel) > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > + else > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > + > + *val = clk_rate; > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_RAW: > + err = iio_backend_bus_reg_read(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(ch), > + val, 2); > + if (err) > + return err; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_axi_set_output_range(struct ad3552r_axi_state *st, > + unsigned int mode) > +{ > + int range_ch_0 = FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode); > + int range_ch_1 = FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode); > + > + return axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, > + AD3552R_MASK_CH_OUTPUT_RANGE, > + range_ch_0 | range_ch_1, 1); return axi3552r_qspi_update_reg_bits(st->back, AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, AD3552R_MASK_CH_OUTPUT_RANGE, FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode) | FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode), 1); looks fine to me from readability point of view and it's shorter. > +} > + > +static int ad3552r_axi_setup(struct ad3552r_axi_state *st) > +{ > + struct fwnode_handle *child __free(fwnode_handle) = NULL; > + u8 gs_p, gs_n; > + s16 goffs; > + u16 id, rfb; > + u16 reg = 0, offset = 0; > + u32 val, range; > + int err; > + > + err = ad3552r_axi_reset(st); > + if (err) > + return err; > + > + err = iio_backend_ddr_disable(st->back); > + if (err) > + return err; > + > + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + AD3552R_SCRATCH_PAD_TEST_VAL1, 1); > + if (err) > + return err; > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + &val, 1); > + if (err) > + return err; > + > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL1) { > + dev_err(st->dev, > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_SCRATCH_PAD_TEST_VAL1, val); > + return -EIO; > + } > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_SCRATCH_PAD, > + AD3552R_SCRATCH_PAD_TEST_VAL2, 1); > + if (err) > + return err; > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + &val, 1); > + if (err) > + return err; > + > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL2) { > + dev_err(st->dev, > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_SCRATCH_PAD_TEST_VAL2, val); > + return -EIO; > + } This scratch pad test is a separate enough 'thing' maybe pull it out as another function called from here? > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_L, > + &val, 1); > + if (err) > + return err; > + > + id = val; > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_H, > + &val, 1); > + if (err) > + return err; > + > + id |= val << 8; > + if (id != st->model_data->chip_id) { > + dev_err(st->dev, "Chip ID mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_ID, id); > + } no {} needed here. Also dev_info() to make it slightly less ominous :) > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + AD3552R_REF_INIT, 1); Hmm. This was snuck in during previous patch. Pull new definitions out of that and put them in this patch. I only noticed because I wondered what value it had and was surprised to find it doesn't exist in current driver. I'm not sure a define for write it all to 0 is worth while. Maybe just put a zero here. > + if (err) > + return err; > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_TRANSFER_INIT, 1); Another define that sneaked in during previous patch. Given it's not 'general' and only used here. I'd drop that define and use the various parts that make it up here. Mind you those defines should be introduced in this patch not the previous one. > + if (err) > + return err; > + > + err = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > + if (err) > + return err; > + > + err = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL); > + if (err) > + return err; > + > + err = ad3552r_get_ref_voltage(st->dev, &val); > + if (err) > + return err; > + > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL, > + val, 1); > + if (err) > + return err; > + > + err = ad3552r_get_drive_strength(st->dev, &val); > + if (!err) { > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SDO_DRIVE_STRENGTH, > + val, 1); > + if (err) > + return err; > + } > + > + child = device_get_named_child_node(st->dev, "channel"); > + if (!child) > + return -EINVAL; > + > + err = ad3552r_get_output_range(st->dev, st->model_data->chip_id, > + child, &range); > + if (!err) > + return ad3552r_axi_set_output_range(st, range); > + > + if (err != -ENOENT) > + return err; > + > + /* Try to get custom range */ > + err = ad3552r_get_custom_gain(st->dev, child, > + &gs_p, &gs_n, &rfb, &goffs); > + if (err) > + return err; > + > + ad3552r_calc_custom_gain(gs_p, gs_n, goffs, ®); I'd return regs > + > + offset = abs((s32)goffs); Hmm. abs(goffs) should use a short I think which will work without the cast and ultimately rely on sign extension of the result. > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_OFFSET(0), > + offset, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_OFFSET(1), > + offset, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_GAIN(0), > + reg, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_GAIN(1), > + reg, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + return 0; > +} ... > + > +static const struct iio_chan_spec_ext_info ad3552r_axi_ext_info[] = { > + IIO_ENUM("synchronous_mode", IIO_SHARED_BY_TYPE, > + &ad3552r_synchronous_mode_enum), > + IIO_ENUM_AVAILABLE("synchronous_mode", IIO_SHARED_BY_TYPE, > + &ad3552r_synchronous_mode_enum), > + {} { } Also see discussion in next patch on this. I'm not sure it makes sense to expose this to userspace but maybe I just don't yet understand the use case. > +}; > + > +#define AD3552R_CHANNEL(ch) { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = (ch), \ > + .scan_index = (ch), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + .ext_info = ad3552r_axi_ext_info, \ > +} > + > +MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>"); > +MODULE_AUTHOR("Angelo Dureghello <adueghello@baylibre.com>"); > +MODULE_DESCRIPTION("AD3552R Driver - AXI IP version"); Maybe relax that description to include all potential backends. As long as they keep to the bindings and interfaces, someone else's completely different backend implementation should work with your front end driver. Mind you I can't immediately think of a better name and module descriptions aren't ABI anyway, so maybe we don't care. > +MODULE_LICENSE("GPL"); >
Le 05/09/2024 à 17:17, Angelo Dureghello a écrit : > From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > Add support for ad3552r-axi, where ad3552r has to be controlled > by the custom (fpga-based) ad3552r AXI DAC IP. ... > +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + struct iio_backend_data_fmt fmt = { > + .type = IIO_BACKEND_DATA_UNSIGNED > + }; > + int loop_len, val, err; > + > + /* Inform DAC chip to switch into DDR mode */ > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > + if (err) > + return err; > + > + /* Inform DAC IP to go for DDR mode from now on */ > + err = iio_backend_ddr_enable(st->back); > + if (err) > + goto exit_err; I don't know if it can be an issue, but iio_backend_ddr_disable() is called if iio_backend_ddr_enable() fails. > + > + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: > + st->single_channel = false; > + loop_len = AD3552R_STREAM_4BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + default: > + return -EINVAL; > + } > + > + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + loop_len, 1); > + if (err) > + goto exit_err; > + > + err = iio_backend_data_transfer_addr(st->back, val); > + if (err) > + goto exit_err; > + > + /* > + * The EXT_SYNC is mandatory in the CN0585 project where 2 instances > + * of the IP are in the design and they need to generate the signals > + * synchronized. > + * > + * Note: in first IP implementations CONFIG EXT_SYNC (RO) can be 0, > + * but EXT_SYMC (ext synch ability) is enabled anyway. > + */ > + if (st->synced_transfer == AD3552R_EXT_SYNC_ARM) > + err = iio_backend_ext_sync_enable(st->back); > + else > + err = iio_backend_ext_sync_disable(st->back); > + if (err) > + goto exit_err_sync; > + > + err = iio_backend_data_format_set(st->back, 0, &fmt); > + if (err) > + goto exit_err_sync; > + > + err = iio_backend_buffer_enable(st->back); > + if (err) > + goto exit_err_sync; > + > + return 0; > + > +exit_err_sync: > + iio_backend_ext_sync_disable(st->back); > + > +exit_err: > + axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + > + iio_backend_ddr_disable(st->back); > + > + return err; > +} ... > +#define AD3552R_CHANNEL(ch) { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = (ch), \ > + .scan_index = (ch), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + .ext_info = ad3552r_axi_ext_info, \ > +} > + > +static struct iio_chan_spec ad3552r_axi_channels[] = { I think (but I've not checked :)) that it could be const. CJ > + AD3552R_CHANNEL(0), > + AD3552R_CHANNEL(1), > +}; ...
On Sun, 2024-09-08 at 13:36 +0100, Jonathan Cameron wrote: > On Fri, 06 Sep 2024 09:08:59 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote: > > > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > Change to obtain the fdt use case as reported in the > > > > adi,ad3552r.yaml file in this patchset, with the DAC device that > > > > is actually using the backend set as a child node of the backend. > > > > > > > > To obtain this, registering all the child fdt nodes as platform > > > > devices. > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > Co-developed-by: David Lechner <dlechner@baylibre.com> > > > > Co-developed-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++ > > > > 1 file changed, 15 insertions(+) > > > > > > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi- > > > > dac.c > > > > index cc31e1dcd1df..e883cd557b6a 100644 > > > > --- a/drivers/iio/dac/adi-axi-dac.c > > > > +++ b/drivers/iio/dac/adi-axi-dac.c > > > > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device > > > > *pdev) > > > > { > > > > struct axi_dac_state *st; > > > > const struct axi_dac_info *info; > > > > + struct platform_device *child_pdev; > > > > void __iomem *base; > > > > unsigned int ver; > > > > struct clk *clk; > > > > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device > > > > *pdev) > > > > return dev_err_probe(&pdev->dev, ret, > > > > "failed to register iio > > > > backend\n"); > > > > > > > > + device_for_each_child_node_scoped(&pdev->dev, child) { > > > > > > This should use fwnode_for_each_available_child_node() so that it skips > > > nodes with status != "okay". > > Ah. That oddity strikes again... > > > > > > > Would be nice to introduce a scoped version of this function too. > > > > > > Also, if we are allowing multiple devices on the bus, the DT bindings > > > need to have a reg property that is unique for each child. > > > > > > > + struct platform_device_info pi; > > > > + > > > > + memset(&pi, 0, sizeof(pi)); > > > > > > struct platform_device_info pi = { }; > > > > > > avoids the need for memset(). > > > > > > > + > > > > + pi.name = fwnode_get_name(child); > > > > + pi.id = PLATFORM_DEVID_AUTO; > > > > + pi.fwnode = child; > > > > > > Need to have pi.parent = &pdev->dev; > > > > > > It could also make sense to have all of the primary bus functions > > > (reg read/write, ddr enable/disable, etc.) passed here as platform > > > data instead of having everything go through the IIO backend. > > > > Note that ddr enable/disable is something that makes sense to be in the > > backend > > anyways as it is something that exists in LVDS/CMOS interfaces that are only > > running > > the dataplane. Bus operations like read/write could make sense but that > > would mean an > > interface directly between the axi-dac and the child devices (bypassing the > > backend > > or any other middle layer - maybe we could create a tiny adi-axi-bus layer > > on the IIO > > topdir or any other place in IIO) which I'm not so sure (and is a bit odd). > > OTOH, > > this bus stuff goes a bit out of scope of the backend main idea/goal so > > yeah... Well, > > let's see what others have to say about it but I don't dislike the idea. > > For the read/write using platform data does seem reasonable to me. > Agreed that DDR is dataplane (at least sometimes) so backend ops probably > appropriate. > > > > > > > > > > + > > > > + child_pdev = platform_device_register_full(&pi); > > > > + if (IS_ERR(child_pdev)) > > > > + return PTR_ERR(child_pdev); > > > > > > These devices need to be unregistered on any error return and when > > > the parent device is removed. > > > > > > > Definitely this needs to be tested by manually unbinding the axi-dac device > > for > > example. I'm not really sure how this will look like and if there's any > > problem in > > removing twice the same device (likely there is). The thing is that when we > > connect a > > frontend with it's backend, a devlink is created (that guarantees that the > > frontend > > is removed before the backend). So, I'm fairly confident that if we add a > > devm action > > in here to unregister the child devices, by the time we unregister the > > child, it > > should be already gone (unless driver core somehow handles this). > > > > All of the above needs careful testing but one way out it (and since in here > > we have > > the parent - child relationship), we could add a boolean flag 'skip_devlink' > > to > > 'struct iio_backend_info' so that devlinks are skipped on these > > arrangements. Or we > > could automatically detect that the frontend is a child of the backend and > > skip the > > link (though an explicit flag might be better). > > Agreed it needs testing but I'm not sure why it would already have gone. > The driver would have unbound, but the platform device /child would still be > there > I think at time of removal. Can probably get away with devm to tear > it down when the backend device then goes away. > > Maybe I'm missing some subtlety though. > Hmm, I guess there was some confusion of me not explaining myself. What I wanted to say is that we will be unregistering the same device twice. But yeah, maybe it ends up just being a NOP in the driver core. But yeah, we definitely need to test this and make sure that the device is only gone by the time the parent device calls platform_device_unregister() on it. - Nuno Sá
Hi all, Some comments on top of what David already said... On Thu, 2024-09-05 at 15:40 -0500, David Lechner wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > ... > > > + > > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > > + int err, ch = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_SAMP_FREQ: { > > + int clk_rate; > > + > > + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, > > + IIO_CHAN_INFO_FREQUENCY); > > This seems odd to me. How does the backend know what frequency we want? > It would make more sense to me if this somehow indicated that we were > getting the SPI SCLK rate. > Yes, this sampling frequency bit seems very wrong atm. And the thing is, we're not even getting SCLK. According to [1], the /4 and /8 is for clk_in which is not the same as SCLK (unless I'm missing something). OTOH, if in the backend patch, that clk_get() is somehow getting sclk, that's wrong because sclk is an output clk of the IP. So we need to get clk_in which should be (typically) 133MHz. > > + if (err != IIO_VAL_INT) > > Would be better to call the variable ret instead of err if it can hold > something besides an error code. > > > + return err; > > + > > + /* > > + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). > > + * Samplerate has sense in DDR only. > > We should also mention that this assumes QSPI in addtion to DDR enabled. > I understand the QSPI bit but why the DDR part? I just don't understand the comment "Samplerate has sense in DDR only.". It needs way more explanation if that is true... > > + */ > > + if (st->single_channel) > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > > + else > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > > + > This division also looks to be very backend dependent. So it's far from ideal being in here... To me, the way we need to get this done is for the backend to effectively report back SCLK (in a correct way). Then, depending on the number of bits per clk (4 for QSPI), the word size and DDR vs SDR we get the device sample rate. With it, we then choose one of Jonathan's suggestion (a per channel attr might be less confusing). All the above said, I probably need to catch up on the above. It might happen that David and Angelo already got some more info from the hdl guys while I was on vacation. [1]: https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html - Nuno Sá
On 08/09/24 2:38 PM, Jonathan Cameron wrote: > On Thu, 05 Sep 2024 17:17:32 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > >> From: Angelo Dureghello <adureghello@baylibre.com> >> >> Extend backend features with new calls needed later on this >> patchset from axi version of ad3552r. >> >> A bus type property has been added to the devicetree to >> inform the backend about the type of bus (interface) in use >> bu the IP. >> >> The follwoing calls are added: >> >> iio_backend_ext_sync_enable >> enable synchronize channels on external trigger >> iio_backend_ext_sync_disable >> disable synchronize channels on external trigger >> iio_backend_ddr_enable >> enable ddr bus transfer >> iio_backend_ddr_disable >> disable ddr bus transfer >> iio_backend_set_bus_mode >> select the type of bus, so that specific read / write >> operations are performed accordingly >> iio_backend_buffer_enable >> enable buffer >> iio_backend_buffer_disable >> disable buffer >> iio_backend_data_transfer_addr >> define the target register address where the DAC sample >> will be written. >> iio_backend_bus_reg_read >> generic bus read, bus-type dependent >> iio_backend_bus_read_write >> generic bus write, bus-type dependent > The RAMP_16 definition doesn't seem immediately connected to the rest. > + I'm not sure what it is from the name. It's a 16bit ramp that the backend is able to generate internally, can be used for testing. I changed the name to IIO_BACKEND_INTERNAL_RAMP_16BIT > >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >> --- >> drivers/iio/industrialio-backend.c | 157 +++++++++++++++++++++++++++++++++++++ >> include/linux/iio/backend.h | 33 ++++++++ >> 2 files changed, 190 insertions(+) >> >> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c >> index 20b3b5212da7..231bef4b560e 100644 >> --- a/drivers/iio/industrialio-backend.c >> +++ b/drivers/iio/industrialio-backend.c >> @@ -718,6 +718,163 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) >> return 0; >> } >> diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h >> index 37d56914d485..eb8c5bb74bb5 100644 >> --- a/include/linux/iio/backend.h >> +++ b/include/linux/iio/backend.h >> >> enum iio_backend_data_source { >> IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE, >> IIO_BACKEND_EXTERNAL, >> + IIO_BACKEND_INTERNAL_RAMP_16, > Not obvious what this is so maybe this enum needs docs in general. > >> IIO_BACKEND_DATA_SOURCE_MAX >> };
On Sun, 2024-09-08 at 18:28 +0200, Christophe JAILLET wrote: > Le 05/09/2024 à 17:17, Angelo Dureghello a écrit : > > From: Angelo Dureghello > > <adureghello-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > > > Add support for ad3552r-axi, where ad3552r has to be controlled > > by the custom (fpga-based) ad3552r AXI DAC IP. > > ... > > > +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) > > +{ > > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > > + struct iio_backend_data_fmt fmt = { > > + .type = IIO_BACKEND_DATA_UNSIGNED > > + }; > > + int loop_len, val, err; > > + > > + /* Inform DAC chip to switch into DDR mode */ > > + err = axi3552r_qspi_update_reg_bits(st->back, > > + > > AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > 1); > > + if (err) > > + return err; > > + > > + /* Inform DAC IP to go for DDR mode from now on */ > > + err = iio_backend_ddr_enable(st->back); > > + if (err) > > + goto exit_err; > > I don't know if it can be an issue, but iio_backend_ddr_disable() is > called if iio_backend_ddr_enable() fails. > > I don't think it would be an issue but conceptually it does not really make sense. Yeah, it should be fixed... - Nuno Sá
The serie comes from the previously discussed RFC, that i converted to a normal patch from this v2. Purpose is to add ad3552r AXI DAC (fpga-based) support. The fpga DAC IP has been created to reach the maximum speed (33MUPS) supported from the ad3552r. To obtain the maximum transfer rate, the custom module has been implemented using the QSPI lines in DDR mode, using a dma buffer. The design is actually using the DAC backend since the register map is the same of the generic DAC IP, except for some customized bitfields. For this reason, a new "compatible" has been added in adi-axi-dac.c. Also, backend has been extended with all the needed functions needed for this use case, keeping the names gneric. Note: the following patch is actually for linux-iio/testing --- Changes in v2: - use unsigned int on bus_reg_read/write - add a compatible in axi-dac backend for the ad3552r DAC IP - minor code alignment fixes - fix a return value not checked - change devicetree structure setting ad3552r-axi as a backend subnode - add synchronous_mode_available in the ABI doc Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> Co-developed-by: David Lechner <dlechner@baylibre.com> Co-developed-by: Nuno Sá <nuno.sa@analog.com> --- Angelo Dureghello (9): dt-bindings: iio: dac: ad3552r: add io-backend property iio: backend: extend features iio: backend adi-axi-dac: extend features iio: backend adi-axi-dac: add registering of child fdt node dt-bindings: iio: dac: add ad3552r axi-dac compatible iio: dac: ad3552r: changes to use FIELD_PREP iio: dac: ad3552r: extract common code (no changes in behavior intended) iio: dac: ad3552r: add axi platform driver iio: ABI: add DAC sysfs synchronous_mode parameter Documentation/ABI/testing/sysfs-bus-iio-dac | 16 + .../devicetree/bindings/iio/dac/adi,ad3552r.yaml | 42 +- .../devicetree/bindings/iio/dac/adi,axi-dac.yaml | 1 + drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 3 +- drivers/iio/dac/ad3552r-axi.c | 567 +++++++++++++++++++++ drivers/iio/dac/ad3552r-common.c | 163 ++++++ drivers/iio/dac/ad3552r.c | 394 +++----------- drivers/iio/dac/ad3552r.h | 199 ++++++++ drivers/iio/dac/adi-axi-dac.c | 282 +++++++++- drivers/iio/industrialio-backend.c | 157 ++++++ include/linux/iio/backend.h | 33 ++ 12 files changed, 1532 insertions(+), 336 deletions(-) --- base-commit: 8b6f7caca90addc22eb4ae958639048001096003 change-id: 20240905-wip-bl-ad3552r-axi-v0-iio-testing-1ef516b10ef0 Best regards,