Wip/bl/ad3552r v1 #2525

spectrum70 · 2024-06-27T18:09:31Z

Hi Nuno and all,

so, to take advantage from your review of yesterday, i added some qspi apies. You can have a look.

In the backend there is still "get_regmap" now active, until i find how to implement:

set/get scan_mode
set/get input_souorce
set/get stream_state
reset (this seems included in the backend "enable" so could be removed from the axi-ad3552r driver)
debug fs register access (this seems the most problematic, i can remove the function in case for now)

Important, in adi-linux "main" all the recent dac backend stuff is missing, so i ported needed parts just for test now. The
last 5 commits should be reviewed, same code runs on mainline linux now.

Note:
i cannot have ad3552r (cn0585_v1 branch) pyadi-iio python test working since in recent kernel "watermark" sysfs attribute is set readonly. Also some specific IOCTL are missing, so getting warning i cannot reach high speed and test fails. I tested anyway that i can write on buffer in dma_input, so the driver should work as expected.

spectrum70 · 2024-06-28T14:59:00Z

Hi Nuno,
propsing also a V2, all actually needed from the driver has been moved to the backend.

removed totally regmap exposure from the backend
moved to backend side all AXI functions needed from ad3552r

Will be back on monday 22.

spectrum70 · 2024-07-22T08:35:45Z

Hi @nunojsa, i am back on this, any comment on this job ?

nunojsa

Just did a very small review since I think main thing for now is to figure how the new interfaces should look like.

Documentation/devicetree/bindings/iio/dac/adi,ad3552r-axi.yaml

drivers/iio/industrialio-backend.c

nunojsa · 2024-07-22T14:58:51Z

drivers/iio/dac/adi-axi-dac.c

 	.set_sample_rate = axi_dac_set_sample_rate,
+	.qspi_set_stream_state = axi_dac_qspi_set_stream_state,
+	.qspi_get_stream_state = axi_dac_qspi_get_stream_state,
+	.qspi_update_chan_reg_addr = axi_dac_qspi_update_chan_reg_addr,


Not commenting much the above three interfaces for now. But why do we need them? What's the purpose? I'm asking because I want to have (or at least try) something way more generic than the above which is "attached/dedicated" to a qspi bus

@nunojsa I mainly added interfaces for all the operations that uses the axi regmap.
stream_state actually starts and stop the stream (dma transfer). Starting the stream i see the generated output by oscilloscope.

I see that i should be able to remove them. obtaining the same from the frontend, using more generic iio_backend_bus_send() and iio_backend_bus_recv()

nunojsa · 2024-07-22T15:09:37Z

drivers/iio/dac/adi-axi-dac.c

+	.qspi_update_chan_reg_addr = axi_dac_qspi_update_chan_reg_addr,
+	.qspi_read_reg = axi_dac_qspi_read_reg,
+	.qspi_write_reg = axi_dac_qspi_write_reg,
+	.qspi_update_reg_bits = axi_dac_qspi_update_reg_bits,


Again, the above is just too directed to a qspi bus. I would prefer something more generic. Like:

iio_backend_bus_send(backend, const char *buf, size_t len); iio_backend_bus_recv(backend, char *buf, size_t len);

Then, backend drivers do what they have to depending on the bus interface. I guess we may have more in the future. Hence, trying to come up with some kind of protocol may not be feasible and with the above, things are transparent for the framework as it's up to the frontend and backend to make it right. I'm also using a plain buffer instead of using a register and value variable pair as I can foresee things like CRC being needed.

Also note that .qspi_update_reg_bits is not really needed. You can very well implement that at the framework level with read()/write(). No need for a new op.

Last thing that comes to mind is that this likely needs a devicetree property indicating the backend as bus capabilities (like is being used to write/read device registers). Reason is that we don't want to expose these interfaces in all designs (just the ones that really have such an interface)

Please talk with @dlech... I discussed something like the above with him IIRC

My thinking on this particular chip is that we could:

Convert the existing upstream driver to use SPI regmap.

Add a backend function like iio_backend_get_frontend_regmap().

This would return a regmap that is implemented with the proposed axi_dac_qspi_read_reg and axi_dac_qspi_write_reg functions (replacing qspi with frontend to make the names more generic).

This would allow much of the code in the existing upstream ad3552r driver to be reused as-is because of the regmap abstraction.

Split the existing driver into a SPI module with a SPI device probe function and an IIO backend module with a platform device probe function. (Or is it possible to have two module_driver() in one module?)

The majority of the existing driver would be in a common file shared by both the SPI module and platform device module.

The only difference between the two drivers would be a little bit in the probe function and how buffered writes are implemented.

Alternately, we could add the iio_backend_bus_send() and iio_backend_bus_recv() functions to the backend instead of `iio_backend_get_frontend_regmap() and implement the regmap that calls those in the ad3552r platform driver and still get the same code sharing benefit.

What do you think of this approach @nunojsa?

Add a backend function like iio_backend_get_frontend_regmap()

Hmm creating a bi-directional link between backends and frontends may be a very good way to shoot ourselves in the foot :). Right now the expectation is that frontends cannot exist if backends are not available (which is guaranteed with device_links). If we now create an API from a backend to access a frontend, we would then need to make sure the frontend is available while we need whatever resource we get from it. So we create kind of deadlock dependency. Might be doable but will add a lot of complexity that I would prefer to avoid.

Split the existing driver into a SPI module with a SPI device probe function and an IIO backend module with a platform device probe function. (Or is it possible to have two module_driver() in one module?)

I think you can't... I did had some issues with it. It may be related to be two driver's of the same time but I don't think so. I guess it's more related with module_init() macro when CONFIG_MODULE is enabled. But you can anyways have your own explicit module_init()/module_exit() calls and register multiple drivers from your init function - not seeing a reason for that not to work.

Alternately, we could add the iio_backend_bus_send() and iio_backend_bus_recv() functions to the backend instead of `iio_backend_get_frontend_regmap() and implement the regmap that calls those in the ad3552r platform driver and still get the same code sharing benefit.

I may be biased but I think the above is my preference for now. Hope my arguments for it make sense to you @dlech

creating a bi-directional link between backends and frontends may be a very good

Maybe "frontend" is not the right word choice. I am not suggesting that the backend driver communicate with the frontend driver, only the frontend chip.

Right now the expectation is that frontends cannot exist if backends are not available

This is exactly what I am suggesting. The frontend driver can't work unless it gets the physical bus access functions from the backend.

Anyway, the main point is that we should be able to share quite a bit of code, e.g. all of the regulator stuff and any other hardware that doesn't go through the backend. And if we use the regmap abstraction, then we could share chip configuration functions for both the SPI bus and the AXI ADC backend versions of the driver.

@dlech @nunojsa

thanks for the comments / help.

Current PR keeps generic-spi (spi driver, ad3552r.c) and adds ad3552r-axi.c version (platform driver). SPI registers and bitmasks are now shared in a common header.

Common code is actually

reading product id

setting voltage ranges

voltage reference config

The axi version is actually a small driver, implemented for maximum speed, it has minimal/fixed configuration (some settings are applied the same for both channels), and uses some special commands not existing in the generic-spi.

It's ok for me the approach by implementing

iio_backend_bus_send(backend, const char *buf, size_t len);
iio_backend_bus_recv(backend, char *buf, size_t len);

What about in ad3552r_common.c to just pass a regmap * to the spi read / write common functions ?

Anyway, the main point is that we should be able to share quite a bit of code, e.g. all of the regulator stuff and any other hardware that doesn't go through the backend. And if we use the regmap abstraction, then we could share chip configuration functions for both the SPI bus and the AXI ADC backend versions of the driver.

Indeed, your naming got me confused but I guess I now understand what you mean. Basically, you're saying for the bakend to implement a custom regmap_bus and then the frontend driver would get a pointer to that regmap_bus right? Hmm, this is an interesting idea yes... I don't think this is as flexible as the send()/recv() approach but should do the trick 99% of the time and it is more practical (and we can make use of apis like update_bits()).

Alright, I like the two approaches so I'm leaving that up to you guys to decide....

What about in ad3552r_common.c to just pass a regmap * to the spi read / write common functions ?

Not sure I'm following this? But anyways, if we go the regmap direction I guess you can just regmap APIs for both the axi and the spi version. If we use the recv()/send() approach you can just pass in an ops struct with write() and read() callback and implement them from the axi and spi drivers.

spectrum70 · 2024-07-26T13:52:06Z

@NunoSa @dlech

Please let me know if i am going in the correct direction.

Add a new version (a first intermediate step for what has been discussed):

set more generic names for new backend functions, to be easily reused in the future
add new read/write function to send/recv data to the bus
bus type specified by a devicetree property
brief test to work

root@analog:~# echo -10/+10V > /sys/bus/iio/devices/iio\:device1/output_range
root@analog:~# echo 65523 > /sys/bus/iio/devices/iio\:device1/out_voltage0_raw 
root@analog:~# echo 0 > /sys/bus/iio/devices/iio\:device1/out_voltage0_raw 
root@analog:~#

drivers/iio/industrialio-backend.c

drivers/iio/dac/adi-axi-dac.c

dlech · 2024-07-26T20:29:54Z

drivers/iio/industrialio-backend.c

+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int iio_backend_update_chan_reg_addr(struct iio_backend *back, u32 chan_address)


I don't really understand what this function does. But I see that it is called in ad3552r_axi_update_scan_mode(). So it makes me think we might be able to use the existing iio_backend_chan_enable() and iio_backend_chan_disable() functions instead.

We could use that. Maybe is more setting "what" channel, but i can go with enable/disable.

AD3552R_REG_ADDR_CH_DAC_16B(0);
AD3552R_REG_ADDR_CH_DAC_16B(1);
AD3552R_REG_ADDR_CH_INPUT_24B(0);
AD3552R_REG_ADDR_CH_INPUT_24B(1);

looks like iio_backend_data_format_set may suit better, since just the channel register offset is updated here.

@dlech No as of now, i cannot find any available call to be used. The value that get written is the address of the register where the samples will be written. Can be the 16 or 24 bit sample register, and the value is driver (chip) specific.

OK. I understand now.

Can we somehow map the channel address with the channel number? I definitely think the naming is too specific for this usecase but I'm still not sure what would be a better on. Maybe iio_backend_data_transfer_from() or iio_backend_update_sample_from()? And rename chan_address -> address. At this point not sure it's that meaningful to have chan_

But more importantly this clearly needs a description. Use the usecase you're familiar with but try to keep it generic.

Like "Some devices may need to inform the backend an adress/localtion where to load data from"

Not sure if the above really makes sense for this device but I guess you got the idea.

Thanks. I changed it to iio_backend_data_transfer_addr() for now, as more generic as possible, adding a meaningful description, as "Some devices may need to inform the backend about an adress/location where to read or write the data."

dlech · 2024-07-26T21:09:31Z

I will keep looking at this next week.

The documentation at http://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html is not very good for understanding what some of the AXI DAC registers do, so more comments in the code could help us understand better why certain operations are needed and what they actually do.

spectrum70 · 2024-07-29T13:25:51Z

I will keep looking at this next week.

The documentation at http://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html is not very good for understanding what some of the AXI DAC registers do, so more comments in the code could help us understand better why certain operations are needed and what they actually do.

@dlech Still not fully clear to me.
Documentation talks about "AD3552R IP core". How can AD3552R be common to other DAC's in the future ? I am assuming that other XXX IP cores will use (or try to use) same AXI bitfields of AD3552R, am i correct ?

Btw, adding some comments where possible.

spectrum70 · 2024-07-29T13:44:04Z

@dlech
Pushed new PR:

fix custom register names
split backend and dac backend commits
add comments where possible

dlech · 2024-07-29T13:58:39Z

Documentation talks about "AD3552R IP core". How can AD3552R be common to other DAC's in the future ? I am assuming that other XXX IP cores will use (or try to use) same AXI bitfields of AD3552R, am i correct ?

If the HDL project for AD3552R has extra memory mapped registers that are not present in the generic AXI DAC IP block, then it could make sense to add an extra compatible string to Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml to indicate this. Then, in the adi-axi-dac.c driver, we could add a chip info structure with feature flags similar to what we do with the MODULE_DEVICE_TABLE in the ad3552r.c driver.

But, if all of the registers are the same as the generic AXI DAC IP core, then we should not need this extra compatible string.

I didn't look at ever single register, but the register map from http://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html looks the same as the generic one from https://wiki.analog.com/resources/fpga/docs/axi_dac_ip, so maybe the are the same?

drivers/iio/dac/ad3552r-axi.c

Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml

drivers/iio/dac/ad3552r-axi.c

Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml

drivers/iio/dac/Kconfig

drivers/iio/dac/ad3552r-axi.c

dlech · 2024-07-29T16:25:41Z

drivers/iio/dac/ad3552r-axi.c

+	if (ret)
+		return ret;
+
+	st->ref_clk = devm_clk_get_prepared(&pdev->dev, NULL);


I think we should use iio_backend_set_sampling_freq() in ad3552r_axi_write_raw() instead of getting the clock here since the clocks isn't connected to the DAC chip.

@dlech not clear. QSPI has a clock, connected from xilinx (fpga) pad to the DAC chip. But should be handled fully from fpga HDL. I removed fdt clock reference and as well the clk enable, all works properly.

iio_backend_set_sampling_freq() seems not impacting on this clock.

So, this is my understanding.

DAC sample rate in direct write is dependent from the QSPI clock.

AXI DAC starts using a (probably fixed) QSPI clock of 66MHZ. In DDR we have 132/4 (to have 16bit samples) so we have 33 MUPS (as declared from the datasheet "Dual Channel, 16-Bit, 33 MUPS"

So sample rate is connected to the interface clock, and RATECNTRL may be able to change it, but there is no api in the backend still for this. Not sure if it's the case to add it.

I tried to write RATECTRL register, the only writable, clock seems always fixed at 66Mhz, whatever value i write on it.

I think the RATECTRL is only meaningful for the IP internal DDS... In theory, you can get the interface frequency with CLK_FREQ and CLK_RATIO and then use the RATECTRL to decimate the internal sampling frequency. However, on the design I tried this (which is an old one) changing the RATECTRL seemed to have no effect.
In current implementation, iio_backend_set_sampling_freq() is only meaningful you DDS is supported and you're exposing the core ALTVOLTAGE channel.

The reason why I use iio_backend_set_sampling_freq() which honestly is not something I really like is:

The value you get in the CLK_FREQ register is measured by the IP so you do have some rounding that could potential be an issue in some cases (not sure though).

More importantly, the way to actually get the sampling frequency is not the same for all designs. So I would prefer to have some more info in the IP in order to be able to do it in a generic way.

For the above reasons I went with iio_backend_set_sampling_freq() which is nothing more than notifying the backend of the interface clock (which is the same as the sampling frequency for RATECNTRL = 1) since often the interface clock comes from the converter. Hopefully this can be improved at some point.

Looks good, if I'm not missing nothing that should mean that you're measuring approx 133332824. If DDR is on that matches the 66MHz.

Not clear from where you get 133332824, could you explain ?

The docs say:

This is relative to the processor clock and in many cases is 100MHz. The number is represented as unsigned 16.16 format.

So the calculation would be something like:

unsigned long system_clk_hz = 100 * MEGA; /* TODO: get this using clk_get_rate() */ u32 clk_freq = 87381; /* TODO: actually read from CLK_FREQ register */ u32 clk_ratio = 1; /* TODO: actually read from CLK_RATIO register */ /* convert from 16.16 format to hz relative to system_clk_hz */ u16 int_val = reg_val >> 16; u16 frac_val = reg_val & 0xFFFF; u32 interface_clk_hz = (int_val * system_clk_hz + mul_u64_u32_div(frac_val, system_clk_hz, UINT16_MAX)) * clk_ratio;

Here, interface_clk_hz should be approx 133_333_333 (~133MHz).

Then the docs say:

Note that the actual sampling clock may not be the same as the interface clock- software must consider device specific implementation parameters to calculate the final sampling clock.

So this would probably be the 133MHz interface clock times 2 for DDR transferring 2 bits per clock cycle, times 4 for QSPI transferring 4 bits at a time, divided by 16 bits per sample to get the 66MHz sample rate. (Datasheet max sample rate is 33MHz, but I assume that is dividing 66MHz by 2 channels)

Not clear from where you get 133332824, could you explain ?

Pretty much what @dlech has above... But note that I already explained why I did not went with this before. Normally, getting the internal core sample rate is only meaningful if the internal DDS is being used.

include/linux/iio/backend.h

spectrum70 · 2024-07-30T07:32:20Z

Documentation talks about "AD3552R IP core". How can AD3552R be common to other DAC's in the future ? I am assuming that other XXX IP cores will use (or try to use) same AXI bitfields of AD3552R, am i correct ?

If the HDL project for AD3552R has extra memory mapped registers that are not present in the generic AXI DAC IP block, then it could make sense to add an extra compatible string to Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml to indicate this. Then, in the adi-axi-dac.c driver, we could add a chip info structure with feature flags similar to what we do with the MODULE_DEVICE_TABLE in the ad3552r.c driver.

But, if all of the registers are the same as the generic AXI DAC IP core, then we should not need this extra compatible string.

I didn't look at ever single register, but the register map from http://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html looks the same as the generic one from https://wiki.analog.com/resources/fpga/docs/axi_dac_ip, so maybe the are the same?

@nunojsa Resgister offsets and names are the same. Issue is the "CUSTOM" register, that can be used in different ways depending on the IP. What about a write_custom_reg() api ?

Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml

drivers/iio/industrialio-backend.c

drivers/iio/dac/adi-axi-dac.c

drivers/iio/dac/ad3552r-axi.c

dlech · 2024-08-13T23:24:13Z

drivers/iio/dac/ad3552r-axi.c

+			return err;
+	}
+
+	err = iio_backend_ddr_enable(st->back);


Do we need to call iio_backend_ddr_disable() if any of the later functions returns an error?

I don't see where this is done before this function returns.

drivers/iio/dac/ad3552r-axi.c

nunojsa

Another round from me. Main pain point for now (unless I'm missing something) is to properly get the sampling frequency.

Also, the bus stuff is getting better but I might still push for the regmap based API 😉 . I feel it brings added value so at least a try could be worth it (to take an actual look on how it would look like)

drivers/iio/industrialio-backend.c

include/linux/iio/backend.h

drivers/iio/dac/adi-axi-dac.c

nunojsa · 2024-08-14T12:54:02Z

drivers/iio/dac/adi-axi-dac.c

+		err = axi_dac_bus_reg_write(back,
+					    AXI_DAC_RD_ADDR(reg), 0, val_size);
+		if (err)
+			return err;


It looks weird enough that maybe it deserves a comment on why we have to write the register with a value of 0.

This comes from the original driver, can try to investigate. Looks like a write of 0 and a check for busy is needed for a next correct write.

nunojsa · 2024-08-14T12:55:10Z

drivers/iio/dac/adi-axi-dac.c

+					       bval, bval != AXI_DAC_BUSY,
+					       10, 100);
+		if (err)
+			return err;


The above poll should be guaranteed by axi_dac_bus_reg_write()

sorry, not clear

drivers/iio/dac/adi-axi-dac.c

nunojsa · 2024-08-14T12:59:49Z

drivers/iio/dac/ad3552r-axi.c

+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return iio_backend_read_raw(st->back, chan, val, val2, mask);


As I commented before, what value are you getting in here? My expectation (assuming the backend gives us SCLK) for this would be something like:

ret = iio_backend_read_raw(st->back, chan, val, val2, mask); if (ret) return ret; *val = DIV_ROUND_CLOSEST(*val * (1 + ddr_en)) * n_spi_lanes, sample_size)

All of the above info should be something that we could easily know in the frontend driver. It#s just that what we get from the backend is not what I would expect (I think). But see my other comment

Let's see the code i pushed.

dlech · 2024-08-20T21:13:14Z

Documentation/ABI/testing/sysfs-bus-iio-dac

@@ -59,3 +59,11 @@ 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.11


Suggested change

KernelVersion: 6.11

KernelVersion: 6.13

Jonathan stops taking new code at rc6 and we'll want Nuno's approval on this series when he gets back, so 6.13 will likely be the version this actually lands in.

dlech · 2024-08-20T21:20:23Z

Documentation/devicetree/bindings/iio/dac/adi,axi-dac.yaml

@@ -38,6 +38,13 @@ properties:
  clocks:
    maxItems: 1

+  bus-type:


Jonathan always suggests to add a default value for when the property is not present. So we should add that. Otherwise, I think maintainers will say the same as Nuno that this should be a boolean property since it only has one value. I guess parallel makes sense as the default since that is what the existing user of the AXI DAC has?

In the code there is no handling for the parallel bus, so not totally sure adding "parallel" here now is appropriate.
Also adding parallel here means roll back again to a switch/case, considering also parallel.

What about, as i initially did, to add AXI_DAC_BUS_TYPE_NONE as 0 ? And setting default 0

From the point of view of the device tree that is describing the wiring, there is always going to be some kind of bus that is connected to the DAC chip, so I don't think "none" makes sense to put in the device tree. I suggested "parallel" as the default since the one already existing user of the AXI DAC (ad9739a) uses a parallel bus connection.

dlech · 2024-08-20T21:22:59Z

drivers/iio/dac/ad3552r-axi.c

+#include <linux/iio/buffer.h>
+#include <linux/iio/backend.h>
+#include <linux/iopoll.h>
+#include <linux/of.h>


Suggested change

#include <linux/of.h>

#include <linux/mod_devicetable.h>

It doesn't look like we actually need the of header.

I already checked includes one by one, all are needed. of.h actually is needed for the fwnode_handle, Tried mod_devicetable.h, it is not helping.

I tried #include <linux/fwnode.h>

Still getting errors related to __free and cleanup of fwnode_handle

Seems i need to keep linux/of.h

__free comes from linux/cleanup.h.

dlech · 2024-08-20T21:24:17Z

drivers/iio/dac/ad3552r-axi.c

+#include <linux/gpio/consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/backend.h>
+#include <linux/iopoll.h>


Suggested change

#include <linux/iopoll.h>

Doesn't look like we are using anything from this header.

as above, all includes are used, removing it i get errors on msleep, fsleep, so i can replace it with delay.h. Done

drivers/iio/dac/ad3552r-axi.c

dlech · 2024-08-20T22:41:28Z

drivers/iio/dac/adi-axi-dac.c

+	if (data->type == IIO_BACKEND_DATA_UNSIGNED)
+		/*
+		 * AXI_DAC_REG_CNTRL_2 BIT(4) selects the data format,
+		 * (on web doc page meaning is inverted):


Can we fix the docs instead of making this comment?

Ok let's see what the HDL guys say

analogdevicesinc/hdl@c09d674

It is possible the HDL has a bug, anbd not the docs ?

drivers/iio/dac/adi-axi-dac.c

spectrum70 · 2024-08-21T10:32:40Z

fixed points above where/as possible, please see my comments
samplerate returned always as per buffered mode (DDR) as 33333333 UPS
TODO: eventually remove totally ext_sync stuff

Add new ad3552r-axi devicetree. Signed-off-by: Angelo Dureghello <[email protected]>

Add nodes and example related to the DAC AXI IP version of the ad3552r driver. Considering the target DAC chip is the same, even if the controller interface is different, using the same fdt node. Signed-off-by: Angelo Dureghello <[email protected]>

spectrum70 · 2024-08-23T12:25:56Z

New push, fixed bad sine wave output due to wrong enable/disable DDR sequence.
DDR must be set on DAC and DAC chip accordingly, in the proper sequence.

Also, EXT_SYNC mechanism is mandatory, cn0585 project used 2 of this IP synchronized together.

Tested by python adi-pyiio branch cn0585.

dlech · 2024-08-23T14:52:46Z

Also, EXT_SYNC mechanism is mandatory, cn0585 project used 2 of this IP synchronized together.

If it is always required, can we remove the sysfs attribute and just always enable it for now?

dlech · 2024-08-23T15:00:07Z

drivers/iio/dac/ad3552r-axi.c

+	return 0;
+}
+
+static int ad3552r_axi_update_scan_mode(struct iio_dev *indio_dev,


This function still lacks error unwinding and things will be in a bad state if there is ever an error.

Also, some of this should be moved to the postenable callback so that it is balanced with the predisable callback. I.e. since we disable DDR mode in predisable, we should be enabling DDR mode in postenable so that everything is balanced.

It should also be fine to move everything from here to postenable and remove the scan_mode callback if needed.

The way the core code works, if postenable fails, there is nothing to unwind what was done in update_scan_mode, so update_scan_mode should not be changing any settings like DDR mode that are required to be undone later for proper operation of the chip.

About EXT_SYNC, it is used in another project, i don't think we should enable it always, since we don't have any external trigger.

DDR is actually enabled in the same place and sequence where it was in the original driver. It was functional that way and didn't touched the sequence. Ok, i can put all quite in post_enable, except the channel mask, so we still need update_scan_mode.

If enabling EXT_SYNC depends on which HDL project is being used, then this seems like something that should be coming from the compile feature register in AXI DAC or from the devicetree (e.g. different compatible string for each HDL project). It doesn't seems like this is something that usespace should be expected to enable or disable depending on which HDL project is being used.

dlech · 2024-08-23T15:10:47Z

drivers/iio/dac/ad3552r-axi.c

+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		int clk_rate;
+
+		err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, mask);


This one still seems odd to me because we are requesting the sample frequency from the backend, but the value that is returned is not the sample rate, but rather the interface clock rate. It would make more sense if the backend could just return the sample rate directly.

dlech · 2024-08-23T15:20:24Z

drivers/iio/dac/ad3552r-axi.c

+#define AD3552R_CHANNEL(ch) { \
+	.type = IIO_VOLTAGE, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_all = (((ch) == 0) ? \


Suggested change

.info_mask_shared_by_all = (((ch) == 0) ? \

.info_mask_shared_by_all = (((ch) == 0) ? \

The (ch) == 0 check is not needed since this is info_mask_shared_by_all. It doesn't hurt to have it declared more than once. The IIO core code will filter out the duplicates.

Backend cannot know ad5332r DAC IP specs (divide /4 or /8). Changed to IIO_CHAN_INFO_FREQUENCY, could be better..

drivers/iio/dac/ad3552r-axi.c

dlech · 2024-08-23T15:26:58Z

drivers/iio/dac/ad3552r-axi.c

+		return -EINVAL;
+
+	err = ad3552r_get_output_range(st->dev, st->chip_id, child, &range);
+	if (err)


Looks like this still needs to be fixed.

dlech · 2024-08-23T15:29:31Z

drivers/iio/dac/ad3552r-axi.c

+	return 0;
+}
+
+static int ad3552r_axi_setup(struct ad3552r_axi_state *st)


This function is missing some of the functionality from ad3552r_configure_device() in the existing upstream driver.

This is needed to enable regulators and configure a register based on the voltage level.

And we need to handle the "adi,sdo-drive-strength" DT property.

Hi @dlech
fixed all the above,

add custom range setup

add regulator setup

add drive strength setup

move as much setup code as possible as common code

drivers/iio/dac/ad3552r.c

dlech · 2024-08-27T14:52:05Z

drivers/iio/dac/ad3552r.h

+	AD3552R_CH_OUTPUT_RANGE_NEG_10__10V,
+};
+
+u16 ad3552r_field_prep(u16 val, u16 mask);


Are we going to replace this with FIELD_PREP?

This could be in a separate patch at the beginning of the series and Jonathan might apply it right away since that change is a good cleanup to have anyway.

I tired but i get errors in some case. As

warning: comparison of constant ‘18446744073709551615’ with boolean expression is always false [-Wbool-compare]
BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >
^
././include/linux/compiler_types.h:490:9: note: in definition of macro ‘__compiletime_assert’

drivers/iio/dac/ad3552r-axi.c

drivers/iio/dac/ad3552r-common.c

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 synchronize channels on external trigger iio_backend_ddr_enable enable 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 or 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 write, bus-type dependent iio_backend_bus_read generic bus read, bus-type dependent Signed-off-by: Angelo Dureghello <[email protected]>

Extend DAC backend with new features required for the AXI driver version for the a3552r DAC. Signed-off-by: Angelo Dureghello <[email protected]>

Add bus property. Signed-off-by: Angelo Dureghello <[email protected]>

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 removerd too. Signed-off-by: Angelo Dureghello <[email protected]>

Extracting common code, to share common code to be used later by the AXI driver version (ad3552r-axi.c). Signed-off-by: Angelo Dureghello <[email protected]>

Add support for ad3552r AXI DAC IP version. Signed-off-by: Angelo Dureghello <[email protected]>

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 <[email protected]>

spectrum70 requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners June 27, 2024 18:09

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 192d7eb to d249dcc Compare June 28, 2024 14:56

nunojsa reviewed Jul 22, 2024

View reviewed changes

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from d249dcc to 5eba63b Compare July 26, 2024 13:48

dlech changed the base branch from main to Wip/bl/ad3552r/base July 26, 2024 20:25

dlech reviewed Jul 26, 2024

View reviewed changes

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 5eba63b to 84b0393 Compare July 29, 2024 13:39

dlech reviewed Jul 29, 2024

View reviewed changes

include/linux/iio/backend.h Outdated Show resolved Hide resolved

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch 3 times, most recently from 2d045d7 to e94157b Compare July 31, 2024 21:02

dlech changed the base branch from iio-testing to Wip/bl/ad3552r/base August 13, 2024 22:07

dlech force-pushed the wip/bl/ad3552r-v1 branch from c8895be to 880e5c6 Compare August 13, 2024 22:16

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 880e5c6 to 38ac22a Compare August 13, 2024 22:41

spectrum70 changed the base branch from Wip/bl/ad3552r/base to iio-testing August 13, 2024 22:41

dlech changed the base branch from iio-testing to Wip/bl/ad3552r/base August 13, 2024 22:46

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 1efec0b to 38ac22a Compare August 13, 2024 22:46

dlech reviewed Aug 13, 2024

View reviewed changes

nunojsa reviewed Aug 14, 2024

View reviewed changes

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 38ac22a to 9952fc3 Compare August 20, 2024 21:09

dlech reviewed Aug 20, 2024

View reviewed changes

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 9952fc3 to 2e8bd11 Compare August 21, 2024 10:31

spectrum70 added 2 commits August 22, 2024 21:36

*** DO NOT MERGE *** : dts: add ad3552r-axi devicetree

baa6f78

Add new ad3552r-axi devicetree. Signed-off-by: Angelo Dureghello <[email protected]>

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch 2 times, most recently from b5c65d1 to 6959b69 Compare August 23, 2024 12:20

dlech reviewed Aug 23, 2024

View reviewed changes

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch 2 times, most recently from 0e25471 to 0c4cafa Compare August 27, 2024 13:07

dlech reviewed Aug 27, 2024

View reviewed changes

spectrum70 added 7 commits August 28, 2024 16:46

iio: backend: extend DAC backend features

43b8526

Extend DAC backend with new features required for the AXI driver version for the a3552r DAC. Signed-off-by: Angelo Dureghello <[email protected]>

dt-bindings: iio: dac: add adi axi-dac bus property

3b8383b

Add bus property. Signed-off-by: Angelo Dureghello <[email protected]>

iio: dac: ad3552r: changes to use FIELD_PREP

d8974ad

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 removerd too. Signed-off-by: Angelo Dureghello <[email protected]>

iio: dac: ad3552r: extract common code (no changes in behavior intended)

abd41ec

Extracting common code, to share common code to be used later by the AXI driver version (ad3552r-axi.c). Signed-off-by: Angelo Dureghello <[email protected]>

iio: dac: ad3552r: add axi platform driver

40099a0

Add support for ad3552r AXI DAC IP version. Signed-off-by: Angelo Dureghello <[email protected]>

iio: ABI: add DAC sysfs synchronous_mode parameter

1b1a492

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 <[email protected]>

spectrum70 force-pushed the wip/bl/ad3552r-v1 branch from 0c4cafa to 1b1a492 Compare August 28, 2024 16:29

	.info_mask_shared_by_all = (((ch) == 0) ? \
	.info_mask_shared_by_all = (((ch) == 0) ? \

Wip/bl/ad3552r v1 #2525

Are you sure you want to change the base?

Wip/bl/ad3552r v1 #2525

Conversation

spectrum70 commented Jun 27, 2024 • edited Loading

spectrum70 commented Jun 28, 2024

spectrum70 commented Jul 22, 2024

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 commented Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

spectrum70 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

spectrum70 Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

dlech commented Jul 26, 2024

spectrum70 commented Jul 29, 2024 • edited Loading

spectrum70 commented Jul 29, 2024

dlech commented Jul 29, 2024

Choose a reason for hiding this comment

spectrum70 Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

spectrum70 Jul 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlech Aug 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 commented Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 commented Aug 21, 2024

spectrum70 commented Aug 23, 2024

dlech commented Aug 23, 2024

Choose a reason for hiding this comment

spectrum70 Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spectrum70 commented Jun 27, 2024 •

edited

Loading

spectrum70 Jul 23, 2024 •

edited

Loading

spectrum70 commented Jul 26, 2024 •

edited

Loading

spectrum70 Jul 29, 2024 •

edited

Loading

spectrum70 Jul 29, 2024 •

edited

Loading

spectrum70 Aug 2, 2024 •

edited

Loading

spectrum70 commented Jul 29, 2024 •

edited

Loading

spectrum70 Jul 31, 2024 •

edited

Loading

spectrum70 Jul 31, 2024 •

edited

Loading

dlech Aug 10, 2024 •

edited

Loading

spectrum70 commented Jul 30, 2024 •

edited

Loading

spectrum70 Aug 21, 2024 •

edited

Loading

spectrum70 Aug 21, 2024 •

edited

Loading

spectrum70 Aug 23, 2024 •

edited

Loading