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 6:19 AM Matt Ranostay <matt.ranostay@xxxxxxxxxxxx> wrote:
> 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:

First of all, please, remove unneeded context when replying!
(And I believe that non-commented stuff will be addressed as suggested)

...

> > > -       depends on GPIOLIB
> > > +       select GPIOLIB
> >
> > I'm not sure why.
>
> Changed to select from 'depends on' to avoid this circular dependency

Was it before your patch? If so, it should be addressed separately as a fix.

>   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

Isn't it the real problem here?

> 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

...

> > > +                       if (mcp->indio_dev)
> > > +                               memcpy(&mcp->adc_values, &data[50], 6);
> >
> > sizeof()
>
> sizeof(mcp->adc_values) would work here if needed.

You need to write code to be more robust, using hardcoded magics when
it's easy to derive is not good practice.

...

> > > +       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..

I see, what about a specific definition with a self-explanatory name?

...

> > > +       ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);
> >
> > Ditto,
>
> See above.

See above.

...

> > > +       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);

So, we have an inconvenience in the iio_device_unregister(), i.e. it
doesn't perform the NULL-check by itself. I recommend fixing it there
and dropping this check in the caller. This is standard practice in
the Linux kernel for resource deallocator APIs.

...

> > 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

The code looks ugly with this kind of ifdeffery. But okay, I leave it
up to maintainers, just my 2cents.

-- 
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