On Mon, 3 Oct 2022 13:30:15 +0300
Cosmin Tanislav <demonsingur@xxxxxxxxx> wrote:
> From: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx>
>
> The AD74115H is a single-channel, software-configurable, input and
> output device for industrial control applications. The AD74115H
> provides a wide range of use cases, integrated on a single chip.
>
> These use cases include analog output, analog input, digital output,
> digital input, resistance temperature detector (RTD), and thermocouple
> measurement capability. The AD74115H also has an integrated HART modem.
>
> A serial peripheral interface (SPI) is used to handle all communications
> to the device, including communications with the HART modem. The digital
> input and digital outputs can be accessed via the SPI or the
> general-purpose input and output (GPIO) pins to support higher
> speed data rates.
>
> The device features a 16-bit, sigma-delta analog-to-digital converter
> (ADC) and a 14-bit digital-to-analog converter (DAC).
> The AD74115H contains a high accuracy 2.5 V on-chip reference that can
> be used as the DAC and ADC reference.
>
> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@xxxxxxxxxx>
Hi Cosmin,
Took me a touch longer than expected to get back to this.
Anyhow, a few minor comments inline. It's a huge driver so I'm not going
to looks super closely at individual register settings etc (I rarely
do unless something 'smells' off!)
Biggest additional thing in here is a suggest to change the fw parsing
code to make it clear when we are just parsing and when applying the
result.
Rest looked good to me.
Jonathan
> +static int _ad74115_get_adc_code(struct ad74115_state *st,
> + enum ad74115_adc_ch channel, int *val)
> +{
> + unsigned int uval;
> + int ret;
> +
> + reinit_completion(&st->adc_data_completion);
> +
> + ret = ad74115_set_adc_ch_en(st, channel, true);
> + if (ret)
> + return ret;
> +
> + ret = ad74115_set_adc_conv_seq(st, AD74115_ADC_CONV_SEQ_SINGLE);
> + if (ret)
> + return ret;
> +
> + ret = wait_for_completion_timeout(&st->adc_data_completion,
> + msecs_to_jiffies(1000));
> + if (!ret) {
> + ret = -ETIMEDOUT;
> + return ret;
return -ETIMEDOUT;
> + }
> +
> + ret = regmap_read(st->regmap, ad74115_adc_ch_data_regs[channel], &uval);
> + if (ret)
> + return ret;
> +
> + ret = ad74115_set_adc_conv_seq(st, AD74115_ADC_CONV_SEQ_STANDBY);
> + if (ret)
> + return ret;
> +
> + ret = ad74115_set_adc_ch_en(st, channel, false);
> + if (ret)
> + return ret;
> +
> + *val = uval;
> +
> + return IIO_VAL_INT;
> +}
...
> +static int ad74115_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long info)
> +{
> + struct ad74115_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + if (ad74115_is_diag_channel(chan->channel))
> + return ad74115_get_diag_scale(st, chan, val, val2);
> + else if (chan->output)
> + return ad74115_get_dac_scale(st, chan, val, val2);
> +
> + return ad74115_get_adc_scale(st, chan, val, val2);
> + case IIO_CHAN_INFO_OFFSET:
> + if (ad74115_is_diag_channel(chan->channel))
> + return ad74115_get_diag_offset(st, chan, val, val2);
> + else if (chan->output)
> + return ad74115_get_dac_offset(st, chan, val);
> +
> + return ad74115_get_adc_offset(st, chan, val);
> + case IIO_CHAN_INFO_RAW:
Trivial: Perhaps order these to match the enum order.
> + if (chan->output)
> + return ad74115_get_dac_code(st, chan, val);
> +
> + return ad74115_get_adc_code(indio_dev, chan, val);
> + case IIO_CHAN_INFO_PROCESSED:
> + ret = ad74115_get_adc_code(indio_dev, chan, val);
> + if (ret)
> + return ret;
> +
> + return ad74115_adc_code_to_resistance(*val, val, val2);
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (chan->output)
> + return ad74115_get_dac_rate(st, chan, val);
> +
> + return ad74115_get_adc_rate(st, chan, val);
> + default:
> + return -EINVAL;
> + }
> +}
> +static int ad74115_parse_fw_prop(struct ad74115_state *st,
> + const struct ad74115_fw_prop *prop, u32 *retval)
> +{
> + struct device *dev = &st->spi->dev;
> + u32 val;
> + int ret;
> +
> + if (prop->is_boolean) {
> + val = device_property_read_bool(dev, prop->name);
Maybe worth splitting this into two separate utility functions as there isn't
a lot of overlap for the is_boolean case with the others.
> + } else {
> + ret = device_property_read_u32(dev, prop->name, &val);
> + if (ret)
> + return 0;
> + }
> +
> + *retval = val;
> +
> + if (prop->is_boolean) {
> + if (prop->negate)
> + val = !val;
> + } else {
> + if (prop->lookup_tbl)
> + ret = _ad74115_find_tbl_index(prop->lookup_tbl,
> + prop->lookup_tbl_len,
> + val, &val);
> + else if (prop->max && val > prop->max)
> + ret = -EINVAL;
> + else
> + ret = 0;
> +
> + if (ret)
> + return dev_err_probe(dev, -EINVAL,
> + "Invalid value %u for prop %s\n",
> + val, prop->name);
> + }
> +
> + if (!prop->mask)
I'm not sure I like the fact there is a mask or not controlling if this
is directly applied. That isn't obvious at the call sight. Perhaps add
an explicit parameter to the function call - or rename it to have
ad75115_parse_fw_prop() called by ad75115_apply_fw_prop() (see below)
So at the call site it is immediately clear if a state update is an expected
side effect.
> + return 0;
> +
> + val = (val << __ffs(prop->mask)) & prop->mask;
> +
> + return regmap_update_bits(st->regmap, prop->reg, prop->mask, val);
Not obvious from function naming that you will do a register update.
Perhaps rename as
ad74115_apply_fw_prop() or something along those lines?
> +}
> +
> +static int ad74115_setup_comp_gc(struct ad74115_state *st)
> +{
> + struct device *dev = &st->spi->dev;
> + u32 val;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD74115_DIN_CONFIG1_REG, &val);
> + if (ret)
> + return ret;
> +
> + if (!(val & AD74115_DIN_COMPARATOR_EN_MASK))
> + return 0;
> +
> + st->gc.owner = THIS_MODULE;
Could do a structure copy based initialization for this.
st->gc = (struct gpio_chip) {
.owner =
.label = ...
...
};
Up to you, but generally I think it looks a little nicer.
> + st->gc.label = AD74115_NAME;
> + st->gc.base = -1;
> + st->gc.ngpio = 1;
> + st->gc.parent = dev;
> + st->gc.can_sleep = true;
> + st->gc.get_direction = ad74115_comp_gpio_get_direction;
> + st->gc.get = ad74115_comp_gpio_get;
> + st->gc.set_config = ad74115_comp_gpio_set_config;
> +
> + return devm_gpiochip_add_data(dev, &st->gc, st);
> +}
> +
> +static int ad74115_setup(struct iio_dev *indio_dev)
> +{
> + struct ad74115_state *st = iio_priv(indio_dev);
> + unsigned int i;
> + u32 val;
> + int ret;
> +
> + val = AD74115_CH_FUNC_HIGH_IMPEDANCE;
> + ret = ad74115_parse_fw_prop(st, &ad74115_ch_func_fw_prop, &val);
As mentioned above, I'd like to see it clearer on whether these
calls apply the configuration or just parse the fw.
> + if (ret)
> + return ret;
> +
> + indio_dev->num_channels += ad74115_channels_map[val].num_channels;
> + st->ch_func = val;
> +
> + val = false;
> + ret = ad74115_parse_fw_prop(st, &ad74115_dac_hart_slew_prop, &val);
> + if (ret)
> + return ret;
> +
> + st->dac_hart_slew = val;
> +
> + if (val) {
> + ret = regmap_update_bits(st->regmap, AD74115_OUTPUT_CONFIG_REG,
> + AD74115_OUTPUT_SLEW_EN_MASK,
> + FIELD_PREP(AD74115_OUTPUT_SLEW_EN_MASK,
> + AD74115_SLEW_MODE_HART));
> + if (ret)
> + return ret;
> + }
> +
> + val = AD74115_DIN_THRESHOLD_MODE_AVDD;
> + ret = ad74115_parse_fw_prop(st, &ad74115_din_threshold_mode_fw_prop, &val);
> + if (ret)
> + return ret;
> +
> + if (val == AD74115_DIN_THRESHOLD_MODE_AVDD) {
> + ret = regulator_get_voltage(st->regulators[AD74115_AVDD_REGULATOR].consumer);
> + if (ret < 0)
> + return ret;
> +
> + st->avdd_mv = ret / 1000;
> + }
> +
> + st->din_threshold_mode = val;
> +
> + val = false;
> + ret = ad74115_parse_fw_prop(st, &ad74115_dac_bipolar_fw_prop, &val);
> + if (ret)
> + return ret;
> +
> + st->dac_bipolar = val;
> +
> + val = AD74115_RTD_MODE_3_WIRE;
> + ret = ad74115_parse_fw_prop(st, &ad74115_ch_func_fw_prop, &val);
> + if (ret)
> + return ret;
> +
> + st->rtd_mode = val;
> +
> + for (i = 0; i < AD74115_GPIO_NUM; i++) {
> + val = AD74115_GPIO_MODE_LOGIC;
> +
> + ret = ad74115_parse_fw_prop(st, &ad74115_gpio_mode_fw_props[i], &val);
> + if (ret)
> + return ret;
> +
> + if (val == AD74115_GPIO_MODE_LOGIC)
> + st->gpio_valid_mask |= BIT(i);
> + }
> +
> + for (i = 0; i < AD74115_DIAG_NUM; i++) {
> + val = AD74115_DIAG_FUNC_DISABLED;
> +
> + ret = ad74115_parse_fw_prop(st, &ad74115_diag_func_fw_props[i], &val);
> + if (ret)
> + return ret;
> +
> + st->diag_func[i] = val;
> +
> + if (val != AD74115_DIAG_FUNC_DISABLED)
> + indio_dev->num_channels++;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(ad74115_fw_props); i++) {
> + const struct ad74115_fw_prop *prop = &ad74115_fw_props[i];
> +
> + ret = ad74115_parse_fw_prop(st, prop, &val);
> + if (ret)
> + return ret;
> + }
> +
> + ret = ad74115_setup_gc(st);
gc isn't enough to make it obvious to me that this is a gpiochip.
I'd use the extra characters to just spell it out. I briefly wondered what
garbage collection was doing in here ;)
> + if (ret)
> + return ret;
> +
> + ret = ad74115_setup_comp_gc(st);
> + if (ret)
> + return ret;
> +
> + return ad74115_setup_iio_channels(indio_dev);
> +}
> +
>
