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
