Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200716072737.9535-1-cristian.pop@analog.com>
In-Reply-To: <20200716072737.9535-1-cristian.pop@analog.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 21 Jul 2020 22:28:49 +0300
Message-ID: <CAHp75VfoykNS5kqNt=0t4nHJS_+p5R2DUYKB02H3TCMQPNOisg@mail.gmail.com>
Subject: Re: [RFC PATCH] one-bit-adc-dac: Add initial version of one bit ADC, DAC
To:     Cristian Pop <cristian.pop@analog.com>
Cc:     linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Jonathan Cameron <jic23@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jul 16, 2020 at 10:27 AM Cristian Pop <cristian.pop@analog.com> 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