On Tue, 20 Sep 2022 23:30:26 -0700 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. > > Signed-off-by: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx> > --- A few comments inline. > + > + case MCP2221_READ_FLASH_DATA: > + switch (data[1]) { > + case MCP2221_SUCCESS: > + mcp->status = 0; > + > + /* Only handles CHIP SETTINGS subpage currently */ > + if (mcp->txbuf[1] != 0) { > + mcp->status = -EIO; > + break; > + } > + > + /* DAC scale value */ > + tmp = (data[6] >> 6) & 0x3; Perhaps use FIELD_GET() and a suitably defined mask? > + if ((data[6] & BIT(5)) && tmp) > + mcp->dac_scale = tmp + 4; > + else > + mcp->dac_scale = 5; > + > + /* ADC scale value */ > + tmp = (data[7] >> 3) & 0x3; > + if ((data[7] & BIT(2)) && tmp) > + mcp->adc_scale = tmp - 1; > + else > + mcp->adc_scale = 0; > + > + break; > + default: > + mcp->status = -EAGAIN; > + } > + complete(&mcp->wait_in_report); > + break; > + > ... > + > +static void mcp_init_work(struct work_struct *work) > +{ > + struct iio_dev *indio_dev; > + struct mcp2221 *mcp = container_of(work, struct mcp2221, init_work.work); > + struct mcp2221_iio *data; > + int ret, num_channels; > + > + hid_hw_power(mcp->hdev, PM_HINT_FULLON); > + mutex_lock(&mcp->lock); > + > + mcp->txbuf[0] = MCP2221_GET_SRAM_SETTINGS; > + ret = mcp_send_data_req_status(mcp, mcp->txbuf, 1); > + No blank line between a call and it's error handlers. Better to visually group them together. > + if (ret == -EAGAIN) > + goto reschedule_task; > + > + num_channels = mcp_iio_channels(mcp); > + if (!num_channels) > + goto unlock; > + > + mcp->txbuf[0] = MCP2221_READ_FLASH_DATA; > + mcp->txbuf[1] = 0; > + ret = mcp_send_data_req_status(mcp, mcp->txbuf, 2); > + > + if (ret == -EAGAIN) > + goto reschedule_task; > + > + indio_dev = devm_iio_device_alloc(&mcp->hdev->dev, sizeof(*data)); > + if (!indio_dev) > + goto unlock; > + > + data = iio_priv(indio_dev); > + data->mcp = mcp; > + > + indio_dev->name = "mcp2221"; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &mcp2221_info; > + indio_dev->channels = mcp->iio_channels; > + indio_dev->num_channels = num_channels; > + > + devm_iio_device_register(&mcp->hdev->dev, indio_dev); If you aren't going full devm, I'd keep this as an iio_device_alloc() and release it by hand in remove. > + > +unlock: > + mutex_unlock(&mcp->lock); > + hid_hw_power(mcp->hdev, PM_HINT_NORMAL); > + > + return; > + > +reschedule_task: > + mutex_unlock(&mcp->lock); > + hid_hw_power(mcp->hdev, PM_HINT_NORMAL); > + > + /* Device is not ready to read SRAM or FLASH data, try again */ > + schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100)); Add a count. If we end up here lots of times, probably want to give up! > +} > +#endif > + > static int mcp2221_probe(struct hid_device *hdev, > const struct hid_device_id *id) > { > @@ -913,6 +1158,11 @@ static int mcp2221_probe(struct hid_device *hdev, > if (ret) > return ret; > > +#if IS_REACHABLE(CONFIG_IIO) > + INIT_DELAYED_WORK(&mcp->init_work, mcp_init_work); > + schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100)); > +#endif > + > return 0; > } >