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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 20 Sep 2022 23:30:26 -0700
Matt Ranostay <matt.ranostay@konsulko.com> 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@konsulko.com>
> ---
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;
>  }
>