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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 1 Aug 2022 11:08:39 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Aug 1, 2022 at 6:19 AM Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> > On Mon, Aug 1, 2022 at 3:11 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:  
> > > On Fri, Jul 29, 2022 at 5:49 PM Matt Ranostay
> > > <matt.ranostay@konsulko.com> 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.

ah. Now the other patch makes more sense.   Make sure to pull this
driver an that one together in a series if we are taking that forwards.

I agree for allocator APIs but not so much for registration APIs.
I checked a few similar ones.

input_unregister_device() doesn't
hwmon_device_unregister() doesn't.

Got bored at that point :)

Would be relatively easy to take this driver fully devm_ I think
then you can just use devm_iio_device_register() if you have a device
to register.


> 
> ...
> 
> > > 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.
> 
Agreed.  HID maintainer call to make I think. 

One alternative is the standard approach of spin another file with wrappers around
the IIO registration calls, then stub that out in the header if CONFIG_IIO
not built appropriately.

Jonathan