Re: [PATCH] HID: mcp2221: add ADC/DAC support via iio subsystem

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, Jul 29, 2022 at 5:49 PM Matt Ranostay
<matt.ranostay@konsulko.com> wrote:
>
> Add support for 3x 10-bit ADC and 1x DAC channels registered via
> the iio subsystem.
>
> To prevent breakage and unexpected dependencies this support only is
> only built if CONFIG_IIO is enabled, and is only weakly referenced by
> 'imply IIO' within the respective Kconfig.
>
> Additionally the iio device only gets registered if at least one channel
> is enabled in the power-on configuration read from SRAM.

I tried to leave the comments not clashed with Jonathan's ones below.

...

> Cc: Rishi Gupta <gupt21@gmail.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Use --cc in the parameters to `git format-patch` or move them after
the cutter '---' line below, so they won't pollute the Git commit
message.

...

> -       depends on GPIOLIB
> +       select GPIOLIB

I'm not sure why.

...

>  #include <linux/hidraw.h>
>  #include <linux/i2c.h>
>  #include <linux/gpio/driver.h>

+ blank line.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

+ blank line.

>  #include "hid-ids.h"

> +#if IS_REACHABLE(CONFIG_IIO)
> +       struct iio_chan_spec iio_channels[3];
> +       struct iio_dev *indio_dev;
> +       u16 adc_values[3];
> +       u8 dac_value;
> +#endif
> +};

...

> +#if IS_REACHABLE(CONFIG_IIO)
> +                       if (mcp->indio_dev)
> +                               memcpy(&mcp->adc_values, &data[50], 6);

sizeof()

> +#endif

...

> +               // Confirm value is within 10-bit range
> +               if (*val > GENMASK(9, 0))

  if (*val >= BIT(10))

 will make comment useless

> +                       return -EINVAL;
> +       }

...

> +       if (val < 0 || val > GENMASK(4, 0))

In a similar way, val >= BIT(5).

> +               return -EINVAL;

...

> +       memset(mcp->txbuf, 0, 12);

sizeof() ?

...

> +       ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);

Ditto,

> +       hid_hw_power(mcp->hdev, PM_HINT_NORMAL);

Even in an error case?

> +       if (ret) {
> +               mutex_unlock(&mcp->lock);
> +               return -EINVAL;
> +       }

...

> +#if IS_REACHABLE(CONFIG_IIO)
> +       if (mcp->indio_dev)

Do you need this check?

> +               iio_device_unregister(mcp->indio_dev);
> +#endif

...

Overall what I really do not like is that ugly ifdeffery. Can we avoid
adding it?

-- 
With Best Regards,
Andy Shevchenko