-
Notifications
You must be signed in to change notification settings - Fork 841
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ad5791 offload #2616
base: main
Are you sure you want to change the base?
ad5791 offload #2616
Conversation
802e9b1
to
3976d47
Compare
03d271e
to
65e5a18
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it goes my review. Note that I'm expecting some stuff to be sent upstream (like gpio reset)... Also it seems this driver could use a proper refactor if you are willing to do that 😄
st->gpio_ldac = devm_gpiod_get_optional(&spi->dev, "ldac", | ||
GPIOD_OUT_LOW); | ||
if (IS_ERR(st->gpio_ldac)) | ||
return PTR_ERR(st->gpio_ldac); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the intent of the above two gpio's? I would say, if we're not using them now, don't bother in adding them. Still ok to leave them in the bindings though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are actually on the eval board and if i dont drive them they are set as input. we need to make sure they are on the right state: CLR high and LDAC low (datasheet table9 p21) for the output to be set according to the register value when we write on spi xfer. I think we need to keep them.
st->gpio_reset = devm_gpiod_get_optional(&spi->dev, "reset", | ||
GPIOD_OUT_LOW); | ||
if (IS_ERR(st->gpio_reset)) | ||
return PTR_ERR(st->gpio_reset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above makes sense to have already. But do a reset in case we have it. Typical pattern is:
gpio_reset = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(gpio_reset))
return PTR_ERR(gpio_reset);
if (gpio_reset) {
// typically there's some time we need to wait for the reset to happen
fseep(x);
// get it out of reset
gpiod_set_value_cansleep(gpio_reset, 0);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
drivers/iio/dac/ad5791.c
Outdated
st->pwr_down = pwr_down; | ||
|
||
ret = ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl); | ||
ret = __ad5791_dac_powerdown(indio_dev, pwr_down); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated change. Not disagreeing with it but should be in a separate patch. If you want to take it one step further, it even seems that some proper locking is missing... Moreover, we should set st->pwr_down
after we actually do ad5791_spi_write(st, AD5791_ADDR_CTRL, st->ctrl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will drop this change and only check the power status in pre-enable. as you suggested below.
drivers/iio/dac/ad5791.c
Outdated
|
||
if (spi_engine_ex_offload_supported(spi)) { | ||
indio_dev->channels = | ||
&ad5791_channels[spi_get_device_id(spi)->driver_data]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above could also be an opportunity for a nice cleanup. Adding a chip_info structure and actual OF support. Thus getting rid of the id enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
drivers/iio/dac/ad5791.c
Outdated
struct ad5791_state *st = iio_priv(indio_dev); | ||
int ret; | ||
|
||
ret = __ad5791_dac_powerdown(indio_dev, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now it's related but see below...
drivers/iio/dac/ad5791.c
Outdated
pwm_disable(st->cnv_trigger); | ||
spi_bus_unlock(st->spi->master); | ||
|
||
return __ad5791_dac_powerdown(indio_dev, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to assume we want to powerdown the DAC? Since it's a user control I would rather check it and return error or something in ad5791_buffer_preenable() in case it's in a powerdown state. This would also mean that we can't power it down while in buffer mode. Thus, using iio_device_claim_direct_mode()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
drivers/iio/dac/ad5791.c
Outdated
int ret; | ||
|
||
if (!st->cnv_trigger) | ||
return -ENODEV; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, this is only available for offload support and cnv_trigger is mandatory (otherwise we fail to probe). So, seems like a redundant check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple more suggestions for upstream "cleanup":
- Use
devm_regulator_get_enable_read_voltage()
to simplify the regulator code. - Drop the driver
remove
callback by using moredevm_
.
@@ -31,6 +31,17 @@ properties: | |||
gain of two configuration. | |||
type: boolean | |||
|
|||
reset-gpios: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jonathan also likes to have all of the power supplies in the bindings, so we can add vcc-suppy
, vrefp-supply
, vrefn-supply
and iovcc-supply
when we submit this upstream.
65e5a18
to
5be52c8
Compare
changes in V2: implement new suggested driver simplifications:
Fixes related to comments:
|
drivers/iio/dac/ad5791.c
Outdated
@@ -430,30 +412,9 @@ static int ad5791_probe(struct spi_device *spi) | |||
indio_dev->name = st->chip_info->name; | |||
ret = iio_device_register(indio_dev); | |||
if (ret) | |||
goto error_disable_reg_neg; | |||
return dev_err_probe(&spi->dev, ret, "unable to register iio device\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most drivers will just return devm_iio_device_register()
directly, so don't usually have the dev_err_probe()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
0c0d193
to
9c53b59
Compare
changes in V3:
|
Depending on board layout, the ad57xx may need control of reset, clear, and ldac pins by the host driver. Add optional bindings for these gpios. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Include a chip info struct in device SPI and device OF match tables to provide channel definitions for each particular ADC model and eliminate device enum. Suggested-by: Nuno Sa <nuno.sa@analog.com> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
The ad7591 has reset, clr and ldac gpios. For the DAC to output data writen to the data register the state of these gpios has to be: RESET=1, CLR=1, LDAC=0 Add these gpios to the driver making them optional in case they are fixed on the pcb. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Simplify probe by using of the devm_regulator_get_enable_read_voltage. Suggested-by: David Lechner <dlechner@baylibre.com> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Use devm_iio_device_register to automatically free resources. Suggested-by: David Lechner <dlechner@baylibre.com> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
This property is not defined on the binidngs, and is not used by the driver. Remove it Suggested-by: David Lechner <dlechner@baylibre.com> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
This adds the bi-directional gpio block to the d10 nano Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Add devicetree for ad57xx on de10nano. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
To allow offload to stream a "tx" type of xfer we need the write flag set but no messages on the SDO memory of the offload engine. but the current code relies on the tx_buf pointer for both of these operations, so there is no way to separate the two. The offload patches currently on development for upstream add flags on core spi structures to flag an offload xfer as a streaming xfer. While those patches get review and are ready to backport to the adi tree, Workaround this limitation by checking if the tx_buf address is -1 to flag it as "streaming type". Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
Add spi offload support to stream tx buffers using dma. This allows loading samples to the dac with a rate of 1 MSPS. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
9c53b59
to
e9af1e7
Compare
Changes V4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think it would be best to upstream the first 5 patches first before merging this in case there are additional changes there. And I still think it would be a good idea to also add the power supplies to the DT bindings since we are touching this anyway.
adc_trigger: pwm@0x00050000 { | ||
compatible = "adi,axi-pwmgen-2.00.a"; | ||
reg = <0x00050000 0x1000>; | ||
#pwm-cells = <2>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using #pwm-cells = <3>
would be nice, see https://lore.kernel.org/linux-pwm/20241024102554.711689-2-u.kleine-koenig@baylibre.com
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
|
||
dmas = <&tx_dma 0>; | ||
dma-names = "tx"; | ||
pwms = <&adc_trigger 0 0>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How unusual to have the default period 0. I'd rather expect to see a period that matches the max freq for that chip here (or something bigger).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. just for the record, i could not clearly make out a max frequency clearly mentioned on the datasheet.
only the spi speed of 35Mbps is mentioned. if we calculate 24 bits, i think a sample rate of 1Mbps is "reasonable"
static int ad5791_get_sampling_freq(struct ad5791_state *st) | ||
{ | ||
return DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, | ||
pwm_get_period(st->cnv_trigger)); | ||
} | ||
|
||
static int __ad5791_set_sampling_freq(struct ad5791_state *st, int freq) | ||
{ | ||
struct pwm_state cnv_state; | ||
u32 rem; | ||
|
||
pwm_init_state(st->cnv_trigger, &cnv_state); | ||
cnv_state.period = div_u64_rem((u64)DIV_ROUND_UP(st->ref_clk_rate, freq) * NSEC_PER_SEC, | ||
st->ref_clk_rate, &rem); | ||
if (rem) | ||
cnv_state.period += 1; | ||
|
||
cnv_state.duty_cycle = DIV_ROUND_UP(NSEC_PER_SEC, st->ref_clk_rate); | ||
|
||
return pwm_apply_state(st->cnv_trigger, &cnv_state); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's strange to use up-rounding when calculating the period from the frequency, but round-closest for the other direction.
The calculation freq -> period isn't very intuitive, a comment would be nice to explain the details. (I hope you didn't copy that formula from a driver where I introduced that without a comment :-))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. i copied were you had the comment. i just did not copy the comment itself. setting pwm in the driver should go away when the offload stuff is backpacked to mainline, thats why i did not bother. but ill add it.
return PTR_ERR(st->cnv_trigger); | ||
|
||
ret = devm_add_action_or_reset(&spi->dev, ad5791_pwm_diasble, | ||
st->cnv_trigger); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: s/diasble/disable/
Registering that devm action before enabling the PWM is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
spi offload support for ad57xx dac
This adds offload support for the ad57xx dac to be able to send samples up to 1MSPS.
PR Type
PR Checklist