On Thu, Jul 16, 2020 at 10:27 AM Cristian Pop <cristian.pop@xxxxxxxxxx> wrote:
>
> Implementation for 1-bit ADC (comparator) and a 1-bit DAC (switch)
...
> +struct one_bit_adc_dac_state {
> + struct platform_device *pdev;
Wouldn't 'struct device *dev;' be enough?
> + struct gpio_descs *in_gpio_descs;
> + struct gpio_descs *out_gpio_descs;
> +};
...
> +{
> + struct one_bit_adc_dac_state *st = iio_priv(indio_dev);
> + int in_num_ch = 0, out_num_ch = 0;
> + int channel = chan->channel;
> + if (st->in_gpio_descs)
> + in_num_ch = st->in_gpio_descs->ndescs;
> +
> + if (st->out_gpio_descs)
> + out_num_ch = st->out_gpio_descs->ndescs;
I'm wondering if you need these conditionals. Is it even possible you
have ndescs > 0 and no descriptors?
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + if (channel < in_num_ch) {
> + *val = gpiod_get_value_cansleep(
> + st->in_gpio_descs->desc[channel]);
Strange indentation...
> + } else {
> + channel -= in_num_ch;
> + *val = gpiod_get_value_cansleep(
> + st->out_gpio_descs->desc[channel]);
Ditto.
> + }
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int one_bit_adc_dac_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long info)
> +{
Same comments as per previous function above.
> +}
...
> + ret = device_property_read_string_array(&st->pdev->dev,
> + propname,
> + gpio_names,
> + num_ch);
> + if (ret < 0)
if (ret)
> + return ret;
...
> + channels = devm_kcalloc(indio_dev->dev.parent, (in_num_ch + out_num_ch),
Extra parentheses.
> + sizeof(struct iio_chan_spec), GFP_KERNEL);
sizeof(*channels) ?
> + if (!channels)
> + return -ENOMEM;
...
> +static const struct of_device_id one_bit_adc_dac_dt_match[] = {
> + { .compatible = "adi,one-bit-adc-dac" },
> + {},
No comma here!
> +};
> +
Unnecessary blank line.
> +MODULE_DEVICE_TABLE(of, one_bit_adc_dac_dt_match);
...
> +
Ditto.
> +module_platform_driver(one_bit_adc_dac_driver);
--
With Best Regards,
Andy Shevchenko
