On Fri, Jul 29, 2022 at 5:49 PM Matt Ranostay
<matt.ranostay@xxxxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
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
