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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux