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