On Mon, Aug 1, 2022 at 3:11 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> 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.
Noted for the future.
>
> ...
>
> > - depends on GPIOLIB
> > + select GPIOLIB
>
> I'm not sure why.
Changed to select from 'depends on' to avoid this circular dependency
SYNC include/config/auto.conf.cmd
drivers/gpio/Kconfig:14:error: recursive dependency detected!
drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by I2C_MUX_LTC4306
drivers/i2c/muxes/Kconfig:47: symbol I2C_MUX_LTC4306 depends on I2C_MUX
drivers/i2c/Kconfig:62: symbol I2C_MUX is selected by MPU3050_I2C
drivers/iio/gyro/Kconfig:127: symbol MPU3050_I2C depends on IIO
drivers/iio/Kconfig:6: symbol IIO is implied by HID_MCP2221
drivers/hid/Kconfig:1298: symbol HID_MCP2221 depends on GPIOLIB
For a resolution refer to Documentation/kbuild/kconfig-language.rst
>
> ...
>
> > #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()
sizeof(mcp->adc_values) would work here if needed.
>
> > +#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() ?
txbuf isn't 12 bytes long but 64 since that is the full max size a HID
transaction could
have. So sizeof() won't work in these cases..
>
> ...
>
> > + ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);
>
> Ditto,
See above.
>
> > + hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
>
> Even in an error case?
hid_hw_power was set to PM_HINT_FULLON before the check, and in error case it
should reset back to normal hint.
>
> > + if (ret) {
> > + mutex_unlock(&mcp->lock);
> > + return -EINVAL;
> > + }
>
> ...
>
> > +#if IS_REACHABLE(CONFIG_IIO)
> > + if (mcp->indio_dev)
>
> Do you need this check?
Yes basically if no ADC or DAC channel is enabled then no iio_device
get allocated or registered.
>
> > + iio_device_unregister(mcp->indio_dev);
> > +#endif
>
> ...
>
> Overall what I really do not like is that ugly ifdeffery. Can we avoid
> adding it?
Could make CONFIG_IIO required for building but not sure we really
want to add as an additional dependency.
Which is way the imply is set for CONFIG_IIO
- Matt
>
> --
> With Best Regards,
> Andy Shevchenko
