Messages in this thread | | | Date | Sat, 4 Dec 2021 21:51:13 +0200 | Subject | Re: [PATCH v9 3/3] iio: addac: add AD74413R driver | From | Cosmin Tanislav <> |
| |
On 12/4/21 19:41, Jonathan Cameron wrote: > On Fri, 3 Dec 2021 01:25:07 +0200 > Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> The AD74412R and AD74413R are quad-channel, software configurable, >> input/output solutions for building and process control applications. >> >> They contain functionality for analog output, analog input, digital input, >> resistance temperature detector, and thermocouple measurements integrated >> into a single chip solution with an SPI interface. >> >> The devices feature a 16-bit ADC and four configurable 13-bit DACs to >> provide four configurable input/output channels and a suite of diagnostic >> functions. >> >> The AD74413R differentiates itself from the AD74412R by being >> HART-compatible. >> >> When configured with channel 0 as voltage output, channel 1 as current >> output, channel 2 as voltage input and channel 3 as current input, the >> following structure is created under the corresponding IIO device. >> >> . >> ├── in_current0_offset >> ├── in_current0_raw >> ├── in_current0_sampling_frequency >> ├── in_current0_sampling_frequency_available >> ├── in_current0_scale >> ├── in_voltage1_offset >> ├── in_voltage1_raw >> ├── in_voltage1_sampling_frequency >> ├── in_voltage1_sampling_frequency_available >> ├── in_voltage1_scale >> ├── in_voltage2_offset >> ├── in_voltage2_raw >> ├── in_voltage2_sampling_frequency >> ├── in_voltage2_sampling_frequency_available >> ├── in_voltage2_scale >> ├── in_current3_offset >> ├── in_current3_raw >> ├── in_current3_sampling_frequency >> ├── in_current3_sampling_frequency_available >> ├── in_current3_scale >> ├── out_voltage0_raw >> ├── out_voltage0_scale >> ├── out_current1_raw >> ├── out_current1_scale >> ├── name >> ├── buffer >> │ ├── data_available >> │ ├── enable >> │ ├── length >> │ └── watermark >> └── scan_elements >> ├── in_current0_en >> ├── in_current0_index >> ├── in_current0_type >> ├── in_voltage1_en >> ├── in_voltage1_index >> ├── in_voltage1_type >> ├── in_voltage2_en >> ├── in_voltage2_index >> ├── in_voltage2_type >> ├── in_current3_en >> ├── in_current3_index >> └── in_current3_type >> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> > You haven't picked up Linus' tag from v7. I'm assuming that wasn't deliberate > so please add it for v10. I think there is just enough stuff in here that it makes > more sense for you to spin a new version than for me to fix it whilst applying...
Yes, it wasn't deliberate.
> All minor stuff so shouldn't take long hopefully + we still need a dt review > which might take a while depending on how busy Rob is. > > thanks, > > Jonathan > >> --- >> --- /dev/null >> +++ b/drivers/iio/addac/ad74413r.c >> @@ -0,0 +1,1476 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2021 Analog Devices, Inc. >> + * Author: Cosmin Tanislav <cosmin.tanislav@analog.com> >> + */ >> + > > ... > >> +struct ad74413r_state { >> + struct ad74413r_channel_config channel_configs[AD74413R_CHANNEL_MAX]; >> + unsigned int gpo_gpio_offsets[AD74413R_CHANNEL_MAX]; >> + unsigned int comp_gpio_offsets[AD74413R_CHANNEL_MAX]; >> + struct gpio_chip gpo_gpiochip; >> + struct gpio_chip comp_gpiochip; >> + struct completion adc_data_completion; >> + unsigned int num_gpo_gpios; >> + unsigned int num_comparator_gpios; >> + u32 sense_resistor_ohms; >> + >> + /* >> + * Synchronize consecutive operations when doing a one-shot >> + * conversion and when updating the ADC samples SPI message. >> + */ >> + struct mutex lock; >> + >> + const struct ad74413r_chip_info *chip_info; >> + struct spi_device *spi; >> + struct regulator *refin_reg; >> + struct regmap *regmap; >> + struct device *dev; >> + struct iio_trigger *trig; >> + >> + size_t adc_active_channels; >> + struct spi_message adc_samples_msg; >> + struct spi_transfer adc_samples_xfer[AD74413R_CHANNEL_MAX + 1]; >> + >> + /* >> + * DMA (thus cache coherency maintenance) requires the >> + * transfer buffers to live in their own cache lines. >> + */ >> + struct { >> + u8 rx_buf[AD74413R_FRAME_SIZE * AD74413R_CHANNEL_MAX]; >> + s64 timestamp; >> + } adc_samples_buf ____cacheline_aligned; >> + >> + u8 adc_samples_tx_buf[AD74413R_FRAME_SIZE * AD74413R_CHANNEL_MAX] >> + ____cacheline_aligned; > What was your thinking for needing to have both adc_samples_buf and adc_samples_tx_buf > at force alignment? I would have expected only adc_samples_buf would be necessary, > unless there are paths in which you are writing into one whilst DMA is going on into > the other.. If there is a reason add a comment. > Sorry, this is just something I missed when removing the other extra ____cacheline_aligned.
>> + u8 reg_tx_buf[AD74413R_FRAME_SIZE]; >> + u8 reg_rx_buf[AD74413R_FRAME_SIZE]; >> +}; >> + >> + >> +static int ad74413r_probe(struct spi_device *spi) >> +{ >> + struct ad74413r_state *st; >> + struct iio_dev *indio_dev; >> + int ret; >> + >> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + st = iio_priv(indio_dev); >> + >> + st->spi = spi; >> + st->dev = &spi->dev; >> + st->chip_info = device_get_match_data(&spi->dev); >> + mutex_init(&st->lock); >> + init_completion(&st->adc_data_completion); >> + >> + st->regmap = devm_regmap_init(st->dev, NULL, st, >> + &ad74413r_regmap_config); >> + if (IS_ERR(st->regmap)) >> + return PTR_ERR(st->regmap); >> + >> + st->refin_reg = devm_regulator_get(st->dev, "refin"); >> + if (IS_ERR(st->refin_reg)) >> + return dev_err_probe(st->dev, PTR_ERR(st->refin_reg), >> + "Failed to get refin regulator\n"); >> + >> + ret = regulator_enable(st->refin_reg); >> + if (ret) >> + return ret; >> + >> + ret = devm_add_action_or_reset(st->dev, ad74413r_regulator_disable, >> + st->refin_reg); >> + if (ret) >> + return ret; >> + >> + st->sense_resistor_ohms = 100000000; >> + device_property_read_u32(st->dev, "shunt-resistor-micro-ohms", >> + &st->sense_resistor_ohms); >> + st->sense_resistor_ohms /= 1000000; >> + >> + st->trig = devm_iio_trigger_alloc(st->dev, "%s-dev%d", >> + st->chip_info->name, iio_device_id(indio_dev)); >> + if (!st->trig) >> + return -ENOMEM; >> + >> + st->trig->ops = &ad74413r_trigger_ops; >> + iio_trigger_set_drvdata(st->trig, st); >> + >> + ret = devm_iio_trigger_register(st->dev, st->trig); >> + if (ret) >> + return ret; >> + >> + indio_dev->name = st->chip_info->name; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->info = &ad74413r_info; >> + indio_dev->trig = iio_trigger_get(st->trig); >> + >> + ret = ad74413r_reset(st); >> + if (ret) >> + return ret; >> + >> + ret = ad74413r_parse_channel_configs(indio_dev); >> + if (ret) >> + return ret; >> + >> + ret = ad74413r_setup_channels(indio_dev); >> + if (ret) >> + return ret; >> + >> + ret = ad74413r_setup_gpios(st); >> + if (ret) >> + return ret; >> + >> + if (st->num_gpo_gpios) { >> + st->gpo_gpiochip.owner = THIS_MODULE; >> + st->gpo_gpiochip.label = st->chip_info->name; >> + st->gpo_gpiochip.base = -1; >> + st->gpo_gpiochip.ngpio = st->num_gpo_gpios; >> + st->gpo_gpiochip.parent = st->dev; >> + st->gpo_gpiochip.can_sleep = true; >> + st->gpo_gpiochip.set = ad74413r_gpio_set; >> + st->gpo_gpiochip.set_multiple = ad74413r_gpio_set_multiple; >> + st->gpo_gpiochip.set_config = ad74413r_gpio_set_gpo_config; >> + st->gpo_gpiochip.get_direction = >> + ad74413r_gpio_get_gpo_direction; >> + >> + ret = devm_gpiochip_add_data(st->dev, &st->gpo_gpiochip, st); >> + if (ret) >> + return ret; >> + } >> + >> + if (st->num_comparator_gpios) { >> + st->comp_gpiochip.owner = THIS_MODULE; >> + st->comp_gpiochip.label = st->chip_info->name; >> + st->comp_gpiochip.base = -1; >> + st->comp_gpiochip.ngpio = st->num_comparator_gpios; >> + st->comp_gpiochip.parent = st->dev; >> + st->comp_gpiochip.can_sleep = true; >> + st->comp_gpiochip.get = ad74413r_gpio_get; >> + st->comp_gpiochip.get_multiple = ad74413r_gpio_get_multiple; >> + st->comp_gpiochip.set_config = ad74413r_gpio_set_comp_config; >> + st->comp_gpiochip.get_direction = >> + ad74413r_gpio_get_comp_direction; >> + >> + ret = devm_gpiochip_add_data(st->dev, &st->comp_gpiochip, st); >> + if (ret) >> + return ret; >> + } >> + >> + ret = ad74413r_set_adc_conv_seq(st, AD74413R_CONV_SEQ_OFF); >> + if (ret) >> + return ret; >> + >> + ret = devm_request_irq(st->dev, spi->irq, ad74413r_adc_data_interrupt, >> + 0, st->chip_info->name, indio_dev); >> + if (ret) > Minor, but would be good to have a dev_err_probe() here as > this is something that can result in a deferred response and it would > be good to log the debugging info.
Sure thing, will change it for the next revision.
>> + return ret; >> + >> + ret = devm_iio_triggered_buffer_setup(st->dev, indio_dev, >> + &iio_pollfunc_store_time, >> + &ad74413r_trigger_handler, >> + &ad74413r_buffer_ops); >> + if (ret) >> + return ret; >> + >> + return devm_iio_device_register(st->dev, indio_dev); >> +}
(sorry for sending the mail again, I was using Gmail before but now I tried using Thunderbird so I can disable HTML formatting.
| |