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

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

[Matt Ranostay]

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.

Cc: Rishi Gupta <gupt21@gmail.com>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/hid/Kconfig       |   3 +-
 drivers/hid/hid-mcp2221.c | 207 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 209 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 6ce92830b5d1..eb4f4bb05226 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1298,7 +1298,8 @@ config HID_ALPS
 config HID_MCP2221
 	tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
 	depends on USB_HID && I2C
-	depends on GPIOLIB
+	select GPIOLIB
+	imply IIO
 	help
 	Provides I2C and SMBUS host adapter functionality over USB-HID
 	through MCP2221 device.
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index de52e9f7bb8c..ab8ca2a65592 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -16,6 +16,8 @@
 #include <linux/hidraw.h>
 #include <linux/i2c.h>
 #include <linux/gpio/driver.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
 #include "hid-ids.h"
 
 /* Commands codes in a raw output report */
@@ -30,6 +32,8 @@ enum {
 	MCP2221_I2C_CANCEL = 0x10,
 	MCP2221_GPIO_SET = 0x50,
 	MCP2221_GPIO_GET = 0x51,
+	MCP2221_SET_SRAM_SETTINGS = 0x60,
+	MCP2221_GET_SRAM_SETTINGS = 0x61,
 };
 
 /* Response codes in a raw input report */
@@ -89,6 +93,7 @@ struct mcp2221 {
 	struct i2c_adapter adapter;
 	struct mutex lock;
 	struct completion wait_in_report;
+	struct delayed_work init_work;
 	u8 *rxbuf;
 	u8 txbuf[64];
 	int rxbuf_idx;
@@ -97,6 +102,17 @@ struct mcp2221 {
 	struct gpio_chip *gc;
 	u8 gp_idx;
 	u8 gpio_dir;
+	u8 mode[4];
+#if IS_REACHABLE(CONFIG_IIO)
+	struct iio_chan_spec iio_channels[3];
+	struct iio_dev *indio_dev;
+	u16 adc_values[3];
+	u8 dac_value;
+#endif
+};
+
+struct mcp2221_iio {
+	struct mcp2221 *mcp;
 };
 
 /*
@@ -745,6 +761,10 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 				break;
 			}
 			mcp->status = mcp_get_i2c_eng_state(mcp, data, 8);
+#if IS_REACHABLE(CONFIG_IIO)
+			if (mcp->indio_dev)
+				memcpy(&mcp->adc_values, &data[50], 6);
+#endif
 			break;
 		default:
 			mcp->status = -EIO;
@@ -816,6 +836,32 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 		complete(&mcp->wait_in_report);
 		break;
 
+	case MCP2221_SET_SRAM_SETTINGS:
+		switch (data[1]) {
+		case MCP2221_SUCCESS:
+			mcp->status = 0;
+			break;
+		default:
+			mcp->status = -EAGAIN;
+		}
+		complete(&mcp->wait_in_report);
+		break;
+
+	case MCP2221_GET_SRAM_SETTINGS:
+		switch (data[1]) {
+		case MCP2221_SUCCESS:
+			memcpy(&mcp->mode, &data[22], 4);
+#if IS_REACHABLE(CONFIG_IIO)
+			mcp->dac_value = data[6] & GENMASK(4, 0);
+#endif
+			mcp->status = 0;
+			break;
+		default:
+			mcp->status = -EAGAIN;
+		}
+		complete(&mcp->wait_in_report);
+		break;
+
 	default:
 		mcp->status = -EIO;
 		complete(&mcp->wait_in_report);
@@ -824,6 +870,158 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 	return 1;
 }
 
+#if IS_REACHABLE(CONFIG_IIO)
+static int mcp2221_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *channel, int *val,
+			    int *val2, long mask)
+{
+
+	struct mcp2221_iio *priv = iio_priv(indio_dev);
+	struct mcp2221 *mcp = priv->mcp;
+	int ret;
+
+	mutex_lock(&mcp->lock);
+
+	if (channel->output) {
+		*val = mcp->dac_value;
+
+		mutex_unlock(&mcp->lock);
+	} else {
+		// Read ADC values
+		ret = mcp_chk_last_cmd_status(mcp);
+		if (ret < 0) {
+			mutex_unlock(&mcp->lock);
+			return ret;
+		}
+
+		*val = le16_to_cpu(mcp->adc_values[channel->address]);
+
+		mutex_unlock(&mcp->lock);
+
+		// Confirm value is within 10-bit range
+		if (*val > GENMASK(9, 0))
+			return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int mcp2221_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct mcp2221_iio *priv = iio_priv(indio_dev);
+	struct mcp2221 *mcp = priv->mcp;
+	int ret;
+
+	if (val < 0 || val > GENMASK(4, 0))
+		return -EINVAL;
+
+
+	hid_hw_power(mcp->hdev, PM_HINT_FULLON);
+
+	mutex_lock(&mcp->lock);
+
+	memset(mcp->txbuf, 0, 12);
+	mcp->txbuf[0] = MCP2221_SET_SRAM_SETTINGS;
+	mcp->txbuf[4] = BIT(7) | val;
+
+	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);
+
+	hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
+
+	if (ret) {
+		mutex_unlock(&mcp->lock);
+		return -EINVAL;
+	}
+
+	mcp->dac_value = val;
+
+	mutex_unlock(&mcp->lock);
+
+	return 0;
+}
+
+static const struct iio_info mcp2221_info = {
+	.read_raw = &mcp2221_read_raw,
+	.write_raw = &mcp2221_write_raw,
+};
+
+static int mcp_iio_channels(struct mcp2221 *mcp)
+{
+	int idx, cnt = 0;
+	bool dac_created = false;
+
+	// GP0 doesn't have ADC/DAC alternative function
+	for (idx = 1; idx < MCP_NGPIO; idx++) {
+		struct iio_chan_spec *chan = &mcp->iio_channels[cnt];
+
+		switch (mcp->mode[idx]) {
+		case 2:
+			chan->address = idx - 1;
+			chan->channel = cnt++;
+			break;
+		case 3:
+			// GP1 doesn't have DAC alternative function
+			if (idx == 1 || dac_created)
+				continue;
+			// DAC1 and DAC2 outputs are connected to the same DAC
+			dac_created = true;
+			chan->output = 1;
+			cnt++;
+			break;
+		default:
+			continue;
+		};
+
+		chan->type = IIO_VOLTAGE;
+		chan->indexed = 1;
+		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+		chan->scan_index = -1;
+	}
+
+	return cnt;
+}
+
+static void mcp_init_work(struct work_struct *work)
+{
+	struct mcp2221 *mcp = container_of(work, struct mcp2221, init_work.work);
+	struct mcp2221_iio *iio;
+	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);
+
+	hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
+	mutex_unlock(&mcp->lock);
+
+	if (ret)
+		return;
+
+	num_channels = mcp_iio_channels(mcp);
+	if (!num_channels)
+		return;
+
+	mcp->indio_dev = devm_iio_device_alloc(&mcp->hdev->dev, sizeof(*iio));
+
+	iio = iio_priv(mcp->indio_dev);
+	iio->mcp = mcp;
+
+	mcp->indio_dev->name = "mcp2221";
+	mcp->indio_dev->modes = INDIO_DIRECT_MODE;
+	mcp->indio_dev->info = &mcp2221_info;
+	mcp->indio_dev->channels = mcp->iio_channels;
+	mcp->indio_dev->num_channels = num_channels;
+
+	iio_device_register(mcp->indio_dev);
+}
+#endif
+
 static int mcp2221_probe(struct hid_device *hdev,
 					const struct hid_device_id *id)
 {
@@ -902,6 +1100,11 @@ static int mcp2221_probe(struct hid_device *hdev,
 	if (ret)
 		goto err_gc;
 
+#if IS_REACHABLE(CONFIG_IIO)
+	INIT_DELAYED_WORK(&mcp->init_work, mcp_init_work);
+	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(500));
+#endif
+
 	return 0;
 
 err_gc:
@@ -920,6 +1123,10 @@ static void mcp2221_remove(struct hid_device *hdev)
 	i2c_del_adapter(&mcp->adapter);
 	hid_hw_close(mcp->hdev);
 	hid_hw_stop(mcp->hdev);
+#if IS_REACHABLE(CONFIG_IIO)
+	if (mcp->indio_dev)
+		iio_device_unregister(mcp->indio_dev);
+#endif
 }
 
 static const struct hid_device_id mcp2221_devices[] = {
-- 
2.36.1

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

[Jonathan Cameron]

On Fri, 29 Jul 2022 23:47:23 +0800
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.

Seems ok, but I've not seen this done before, so will rely on others
to feedback on that element.

Otherwise, various comments inline.

> 
> Additionally the iio device only gets registered if at least one channel
> is enabled in the power-on configuration read from SRAM.
> 
> Cc: Rishi Gupta <gupt21@gmail.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/hid/Kconfig       |   3 +-
>  drivers/hid/hid-mcp2221.c | 207 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 6ce92830b5d1..eb4f4bb05226 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1298,7 +1298,8 @@ config HID_ALPS
>  config HID_MCP2221
>  	tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
>  	depends on USB_HID && I2C
> -	depends on GPIOLIB
> +	select GPIOLIB
> +	imply IIO
>  	help
>  	Provides I2C and SMBUS host adapter functionality over USB-HID
>  	through MCP2221 device.
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index de52e9f7bb8c..ab8ca2a65592 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -16,6 +16,8 @@
>  #include <linux/hidraw.h>
>  #include <linux/i2c.h>
>  #include <linux/gpio/driver.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

I can't immediately see why you need iio/sysfs.h
That's normally only relevant if non standard ABI is in use.

>  #include "hid-ids.h"
>  
>  /* Commands codes in a raw output report */
> @@ -30,6 +32,8 @@ enum {
>  	MCP2221_I2C_CANCEL = 0x10,
>  	MCP2221_GPIO_SET = 0x50,
>  	MCP2221_GPIO_GET = 0x51,
> +	MCP2221_SET_SRAM_SETTINGS = 0x60,
> +	MCP2221_GET_SRAM_SETTINGS = 0x61,
>  };
>  
>  /* Response codes in a raw input report */
> @@ -89,6 +93,7 @@ struct mcp2221 {
>  	struct i2c_adapter adapter;
>  	struct mutex lock;
>  	struct completion wait_in_report;
> +	struct delayed_work init_work;
>  	u8 *rxbuf;
>  	u8 txbuf[64];
>  	int rxbuf_idx;
> @@ -97,6 +102,17 @@ struct mcp2221 {
>  	struct gpio_chip *gc;
>  	u8 gp_idx;
>  	u8 gpio_dir;
> +	u8 mode[4];
> +#if IS_REACHABLE(CONFIG_IIO)
> +	struct iio_chan_spec iio_channels[3];
> +	struct iio_dev *indio_dev;
> +	u16 adc_values[3];
> +	u8 dac_value;
> +#endif
> +};
> +
> +struct mcp2221_iio {
> +	struct mcp2221 *mcp;
>  };
>  
>  /*
> @@ -745,6 +761,10 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  				break;
>  			}
>  			mcp->status = mcp_get_i2c_eng_state(mcp, data, 8);
> +#if IS_REACHABLE(CONFIG_IIO)
> +			if (mcp->indio_dev)
> +				memcpy(&mcp->adc_values, &data[50], 6);
> +#endif
>  			break;
>  		default:
>  			mcp->status = -EIO;
> @@ -816,6 +836,32 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  		complete(&mcp->wait_in_report);
>  		break;
>  
> +	case MCP2221_SET_SRAM_SETTINGS:
> +		switch (data[1]) {
> +		case MCP2221_SUCCESS:
> +			mcp->status = 0;
> +			break;
> +		default:
> +			mcp->status = -EAGAIN;
> +		}
> +		complete(&mcp->wait_in_report);
> +		break;
> +
> +	case MCP2221_GET_SRAM_SETTINGS:
> +		switch (data[1]) {
> +		case MCP2221_SUCCESS:
> +			memcpy(&mcp->mode, &data[22], 4);
> +#if IS_REACHABLE(CONFIG_IIO)
> +			mcp->dac_value = data[6] & GENMASK(4, 0);
Might be worth converting to more readable mask define and
FIELD_GET()

> +#endif
> +			mcp->status = 0;
> +			break;
> +		default:
> +			mcp->status = -EAGAIN;
> +		}
> +		complete(&mcp->wait_in_report);
> +		break;
> +
>  	default:
>  		mcp->status = -EIO;
>  		complete(&mcp->wait_in_report);
> @@ -824,6 +870,158 @@ static int mcp2221_raw_event(struct hid_device *hdev,
>  	return 1;
>  }
>  
> +#if IS_REACHABLE(CONFIG_IIO)
> +static int mcp2221_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +
No blank line here
> +	struct mcp2221_iio *priv = iio_priv(indio_dev);
> +	struct mcp2221 *mcp = priv->mcp;
> +	int ret;
> +
> +	mutex_lock(&mcp->lock);
For readability I'd prefer this duplicated in each of the
branches so clearly matched against the unlocks.

> +
> +	if (channel->output) {
> +		*val = mcp->dac_value;
> +
> +		mutex_unlock(&mcp->lock);
> +	} else {
> +		// Read ADC values
As below.

> +		ret = mcp_chk_last_cmd_status(mcp);
> +		if (ret < 0) {
> +			mutex_unlock(&mcp->lock);
> +			return ret;
> +		}
> +
> +		*val = le16_to_cpu(mcp->adc_values[channel->address]);
> +
> +		mutex_unlock(&mcp->lock);
> +
> +		// Confirm value is within 10-bit range
> +		if (*val > GENMASK(9, 0))
> +			return -EINVAL;
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int mcp2221_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct mcp2221_iio *priv = iio_priv(indio_dev);
> +	struct mcp2221 *mcp = priv->mcp;
> +	int ret;
> +
> +	if (val < 0 || val > GENMASK(4, 0))
This is a bit wierd.  I'd either expect comparison with a number
rather than a mask, or FIELD_FIT()


> +		return -EINVAL;
> +
Single blank line is enough.
> +
> +	hid_hw_power(mcp->hdev, PM_HINT_FULLON);
> +
> +	mutex_lock(&mcp->lock);
> +
> +	memset(mcp->txbuf, 0, 12);
> +	mcp->txbuf[0] = MCP2221_SET_SRAM_SETTINGS;
> +	mcp->txbuf[4] = BIT(7) | val;

Given GENMASK usage above, FIELD_PREP() would make this
more 'self documenting' both for the val and BIT(7)

> +
> +	ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);
> +
> +	hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
> +
> +	if (ret) {
> +		mutex_unlock(&mcp->lock);
> +		return -EINVAL;
> +	}
> +
> +	mcp->dac_value = val;
> +
> +	mutex_unlock(&mcp->lock);
> +
> +	return 0;
> +}
> +
> +static const struct iio_info mcp2221_info = {
> +	.read_raw = &mcp2221_read_raw,
> +	.write_raw = &mcp2221_write_raw,
> +};
> +
> +static int mcp_iio_channels(struct mcp2221 *mcp)
> +{
> +	int idx, cnt = 0;
> +	bool dac_created = false;
> +
> +	// GP0 doesn't have ADC/DAC alternative function

Not consistent with comment style in this driver. /* ... */

> +	for (idx = 1; idx < MCP_NGPIO; idx++) {
> +		struct iio_chan_spec *chan = &mcp->iio_channels[cnt];
> +
> +		switch (mcp->mode[idx]) {
> +		case 2:
> +			chan->address = idx - 1;
> +			chan->channel = cnt++;
> +			break;
> +		case 3:
> +			// GP1 doesn't have DAC alternative function

As above.

> +			if (idx == 1 || dac_created)
> +				continue;
> +			// DAC1 and DAC2 outputs are connected to the same DAC
> +			dac_created = true;
> +			chan->output = 1;
> +			cnt++;
> +			break;
> +		default:
> +			continue;
> +		};
> +
> +		chan->type = IIO_VOLTAGE;
> +		chan->indexed = 1;
> +		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> +		chan->scan_index = -1;
> +	}
> +
> +	return cnt;
> +}
> +
> +static void mcp_init_work(struct work_struct *work)
> +{
> +	struct mcp2221 *mcp = container_of(work, struct mcp2221, init_work.work);
> +	struct mcp2221_iio *iio;
> +	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);
> +
> +	hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
> +	mutex_unlock(&mcp->lock);
> +
> +	if (ret)
> +		return;
> +
> +	num_channels = mcp_iio_channels(mcp);
> +	if (!num_channels)
> +		return;
> +
> +	mcp->indio_dev = devm_iio_device_alloc(&mcp->hdev->dev, sizeof(*iio));
This can fail.
> +
> +	iio = iio_priv(mcp->indio_dev);
> +	iio->mcp = mcp;
> +
> +	mcp->indio_dev->name = "mcp2221";
> +	mcp->indio_dev->modes = INDIO_DIRECT_MODE;
> +	mcp->indio_dev->info = &mcp2221_info;
> +	mcp->indio_dev->channels = mcp->iio_channels;
> +	mcp->indio_dev->num_channels = num_channels;
> +
> +	iio_device_register(mcp->indio_dev);
As can this.  You need to check both.

> +}
> +#endif
> +
>  static int mcp2221_probe(struct hid_device *hdev,
>  					const struct hid_device_id *id)
>  {
> @@ -902,6 +1100,11 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_gc;
>  
> +#if IS_REACHABLE(CONFIG_IIO)
> +	INIT_DELAYED_WORK(&mcp->init_work, mcp_init_work);
> +	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(500));
> +#endif
> +
>  	return 0;
>  
>  err_gc:
> @@ -920,6 +1123,10 @@ static void mcp2221_remove(struct hid_device *hdev)
>  	i2c_del_adapter(&mcp->adapter);
>  	hid_hw_close(mcp->hdev);
>  	hid_hw_stop(mcp->hdev);
> +#if IS_REACHABLE(CONFIG_IIO)
> +	if (mcp->indio_dev)
> +		iio_device_unregister(mcp->indio_dev);
> +#endif
I'd expect remove to be reverse order of probe. Mind you this driver has a fun
mix of devm and non devm which makes it very hard to reason about correctness
and potential race conditions.  I would personally advocate preceding this
patch with a cleanup of that side of things (probably mass usage of devm_add_action_or_reset()
and appropriate callbacks).

Jonathan

>  }
>  
>  static const struct hid_device_id mcp2221_devices[] = {

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

[Andy Shevchenko]

On Fri, Jul 29, 2022 at 5:49 PM 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.

I tried to leave the comments not clashed with Jonathan's ones below.

...

> Cc: Rishi Gupta <gupt21@gmail.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Use --cc in the parameters to `git format-patch` or move them after
the cutter '---' line below, so they won't pollute the Git commit
message.

...

> -       depends on GPIOLIB
> +       select GPIOLIB

I'm not sure why.

...

>  #include <linux/hidraw.h>
>  #include <linux/i2c.h>
>  #include <linux/gpio/driver.h>

+ blank line.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

+ blank line.

>  #include "hid-ids.h"

> +#if IS_REACHABLE(CONFIG_IIO)
> +       struct iio_chan_spec iio_channels[3];
> +       struct iio_dev *indio_dev;
> +       u16 adc_values[3];
> +       u8 dac_value;
> +#endif
> +};

...

> +#if IS_REACHABLE(CONFIG_IIO)
> +                       if (mcp->indio_dev)
> +                               memcpy(&mcp->adc_values, &data[50], 6);

sizeof()

> +#endif

...

> +               // Confirm value is within 10-bit range
> +               if (*val > GENMASK(9, 0))

  if (*val >= BIT(10))

 will make comment useless

> +                       return -EINVAL;
> +       }

...

> +       if (val < 0 || val > GENMASK(4, 0))

In a similar way, val >= BIT(5).

> +               return -EINVAL;

...

> +       memset(mcp->txbuf, 0, 12);

sizeof() ?

...

> +       ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);

Ditto,

> +       hid_hw_power(mcp->hdev, PM_HINT_NORMAL);

Even in an error case?

> +       if (ret) {
> +               mutex_unlock(&mcp->lock);
> +               return -EINVAL;
> +       }

...

> +#if IS_REACHABLE(CONFIG_IIO)
> +       if (mcp->indio_dev)

Do you need this check?

> +               iio_device_unregister(mcp->indio_dev);
> +#endif

...

Overall what I really do not like is that ugly ifdeffery. Can we avoid
adding it?

-- 
With Best Regards,
Andy Shevchenko

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

[Matt Ranostay]

On Sun, Jul 31, 2022 at 7:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 29 Jul 2022 23:47:23 +0800
> 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.
>
> Seems ok, but I've not seen this done before, so will rely on others
> to feedback on that element.
>
> Otherwise, various comments inline.
>
> >
> > Additionally the iio device only gets registered if at least one channel
> > is enabled in the power-on configuration read from SRAM.
> >
> > Cc: Rishi Gupta <gupt21@gmail.com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/hid/Kconfig       |   3 +-
> >  drivers/hid/hid-mcp2221.c | 207 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 209 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 6ce92830b5d1..eb4f4bb05226 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -1298,7 +1298,8 @@ config HID_ALPS
> >  config HID_MCP2221
> >       tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
> >       depends on USB_HID && I2C
> > -     depends on GPIOLIB
> > +     select GPIOLIB
> > +     imply IIO
> >       help
> >       Provides I2C and SMBUS host adapter functionality over USB-HID
> >       through MCP2221 device.
> > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > index de52e9f7bb8c..ab8ca2a65592 100644
> > --- a/drivers/hid/hid-mcp2221.c
> > +++ b/drivers/hid/hid-mcp2221.c
> > @@ -16,6 +16,8 @@
> >  #include <linux/hidraw.h>
> >  #include <linux/i2c.h>
> >  #include <linux/gpio/driver.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> I can't immediately see why you need iio/sysfs.h
> That's normally only relevant if non standard ABI is in use.
>
> >  #include "hid-ids.h"
> >
> >  /* Commands codes in a raw output report */
> > @@ -30,6 +32,8 @@ enum {
> >       MCP2221_I2C_CANCEL = 0x10,
> >       MCP2221_GPIO_SET = 0x50,
> >       MCP2221_GPIO_GET = 0x51,
> > +     MCP2221_SET_SRAM_SETTINGS = 0x60,
> > +     MCP2221_GET_SRAM_SETTINGS = 0x61,
> >  };
> >
> >  /* Response codes in a raw input report */
> > @@ -89,6 +93,7 @@ struct mcp2221 {
> >       struct i2c_adapter adapter;
> >       struct mutex lock;
> >       struct completion wait_in_report;
> > +     struct delayed_work init_work;
> >       u8 *rxbuf;
> >       u8 txbuf[64];
> >       int rxbuf_idx;
> > @@ -97,6 +102,17 @@ struct mcp2221 {
> >       struct gpio_chip *gc;
> >       u8 gp_idx;
> >       u8 gpio_dir;
> > +     u8 mode[4];
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +     struct iio_chan_spec iio_channels[3];
> > +     struct iio_dev *indio_dev;
> > +     u16 adc_values[3];
> > +     u8 dac_value;
> > +#endif
> > +};
> > +
> > +struct mcp2221_iio {
> > +     struct mcp2221 *mcp;
> >  };
> >
> >  /*
> > @@ -745,6 +761,10 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> >                               break;
> >                       }
> >                       mcp->status = mcp_get_i2c_eng_state(mcp, data, 8);
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +                     if (mcp->indio_dev)
> > +                             memcpy(&mcp->adc_values, &data[50], 6);
> > +#endif
> >                       break;
> >               default:
> >                       mcp->status = -EIO;
> > @@ -816,6 +836,32 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> >               complete(&mcp->wait_in_report);
> >               break;
> >
> > +     case MCP2221_SET_SRAM_SETTINGS:
> > +             switch (data[1]) {
> > +             case MCP2221_SUCCESS:
> > +                     mcp->status = 0;
> > +                     break;
> > +             default:
> > +                     mcp->status = -EAGAIN;
> > +             }
> > +             complete(&mcp->wait_in_report);
> > +             break;
> > +
> > +     case MCP2221_GET_SRAM_SETTINGS:
> > +             switch (data[1]) {
> > +             case MCP2221_SUCCESS:
> > +                     memcpy(&mcp->mode, &data[22], 4);
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +                     mcp->dac_value = data[6] & GENMASK(4, 0);
> Might be worth converting to more readable mask define and
> FIELD_GET()
>
> > +#endif
> > +                     mcp->status = 0;
> > +                     break;
> > +             default:
> > +                     mcp->status = -EAGAIN;
> > +             }
> > +             complete(&mcp->wait_in_report);
> > +             break;
> > +
> >       default:
> >               mcp->status = -EIO;
> >               complete(&mcp->wait_in_report);
> > @@ -824,6 +870,158 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> >       return 1;
> >  }
> >
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +static int mcp2221_read_raw(struct iio_dev *indio_dev,
> > +                         struct iio_chan_spec const *channel, int *val,
> > +                         int *val2, long mask)
> > +{
> > +
> No blank line here
> > +     struct mcp2221_iio *priv = iio_priv(indio_dev);
> > +     struct mcp2221 *mcp = priv->mcp;
> > +     int ret;
> > +
> > +     mutex_lock(&mcp->lock);
> For readability I'd prefer this duplicated in each of the
> branches so clearly matched against the unlocks.
>
> > +
> > +     if (channel->output) {
> > +             *val = mcp->dac_value;
> > +
> > +             mutex_unlock(&mcp->lock);
> > +     } else {
> > +             // Read ADC values
> As below.
>
> > +             ret = mcp_chk_last_cmd_status(mcp);
> > +             if (ret < 0) {
> > +                     mutex_unlock(&mcp->lock);
> > +                     return ret;
> > +             }
> > +
> > +             *val = le16_to_cpu(mcp->adc_values[channel->address]);
> > +
> > +             mutex_unlock(&mcp->lock);
> > +
> > +             // Confirm value is within 10-bit range
> > +             if (*val > GENMASK(9, 0))
> > +                     return -EINVAL;
> > +     }
> > +
> > +     return IIO_VAL_INT;
> > +}
> > +
> > +static int mcp2221_write_raw(struct iio_dev *indio_dev,
> > +                          struct iio_chan_spec const *chan,
> > +                          int val, int val2, long mask)
> > +{
> > +     struct mcp2221_iio *priv = iio_priv(indio_dev);
> > +     struct mcp2221 *mcp = priv->mcp;
> > +     int ret;
> > +
> > +     if (val < 0 || val > GENMASK(4, 0))
> This is a bit wierd.  I'd either expect comparison with a number
> rather than a mask, or FIELD_FIT()

Personally I'm fine with that or just using >= 31 for instance

>
>
> > +             return -EINVAL;
> > +
> Single blank line is enough.
> > +
> > +     hid_hw_power(mcp->hdev, PM_HINT_FULLON);
> > +
> > +     mutex_lock(&mcp->lock);
> > +
> > +     memset(mcp->txbuf, 0, 12);
> > +     mcp->txbuf[0] = MCP2221_SET_SRAM_SETTINGS;
> > +     mcp->txbuf[4] = BIT(7) | val;
>
> Given GENMASK usage above, FIELD_PREP() would make this
> more 'self documenting' both for the val and BIT(7)

BIT(7) signals that the field is changed for the transaction so it can
update the value.

>
> > +
> > +     ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);
> > +
> > +     hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
> > +
> > +     if (ret) {
> > +             mutex_unlock(&mcp->lock);
> > +             return -EINVAL;
> > +     }
> > +
> > +     mcp->dac_value = val;
> > +
> > +     mutex_unlock(&mcp->lock);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct iio_info mcp2221_info = {
> > +     .read_raw = &mcp2221_read_raw,
> > +     .write_raw = &mcp2221_write_raw,
> > +};
> > +
> > +static int mcp_iio_channels(struct mcp2221 *mcp)
> > +{
> > +     int idx, cnt = 0;
> > +     bool dac_created = false;
> > +
> > +     // GP0 doesn't have ADC/DAC alternative function
>
> Not consistent with comment style in this driver. /* ... */
>
> > +     for (idx = 1; idx < MCP_NGPIO; idx++) {
> > +             struct iio_chan_spec *chan = &mcp->iio_channels[cnt];
> > +
> > +             switch (mcp->mode[idx]) {
> > +             case 2:
> > +                     chan->address = idx - 1;
> > +                     chan->channel = cnt++;
> > +                     break;
> > +             case 3:
> > +                     // GP1 doesn't have DAC alternative function
>
> As above.
>
> > +                     if (idx == 1 || dac_created)
> > +                             continue;
> > +                     // DAC1 and DAC2 outputs are connected to the same DAC
> > +                     dac_created = true;
> > +                     chan->output = 1;
> > +                     cnt++;
> > +                     break;
> > +             default:
> > +                     continue;
> > +             };
> > +
> > +             chan->type = IIO_VOLTAGE;
> > +             chan->indexed = 1;
> > +             chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
> > +             chan->scan_index = -1;
> > +     }
> > +
> > +     return cnt;
> > +}
> > +
> > +static void mcp_init_work(struct work_struct *work)
> > +{
> > +     struct mcp2221 *mcp = container_of(work, struct mcp2221, init_work.work);
> > +     struct mcp2221_iio *iio;
> > +     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);
> > +
> > +     hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
> > +     mutex_unlock(&mcp->lock);
> > +
> > +     if (ret)
> > +             return;
> > +
> > +     num_channels = mcp_iio_channels(mcp);
> > +     if (!num_channels)
> > +             return;
> > +
> > +     mcp->indio_dev = devm_iio_device_alloc(&mcp->hdev->dev, sizeof(*iio));
> This can fail.

Noted.

> > +
> > +     iio = iio_priv(mcp->indio_dev);
> > +     iio->mcp = mcp;
> > +
> > +     mcp->indio_dev->name = "mcp2221";
> > +     mcp->indio_dev->modes = INDIO_DIRECT_MODE;
> > +     mcp->indio_dev->info = &mcp2221_info;
> > +     mcp->indio_dev->channels = mcp->iio_channels;
> > +     mcp->indio_dev->num_channels = num_channels;
> > +
> > +     iio_device_register(mcp->indio_dev);
> As can this.  You need to check both.
>

Noted.

> > +}
> > +#endif
> > +
> >  static int mcp2221_probe(struct hid_device *hdev,
> >                                       const struct hid_device_id *id)
> >  {
> > @@ -902,6 +1100,11 @@ static int mcp2221_probe(struct hid_device *hdev,
> >       if (ret)
> >               goto err_gc;
> >
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +     INIT_DELAYED_WORK(&mcp->init_work, mcp_init_work);
> > +     schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(500));
> > +#endif
> > +
> >       return 0;
> >
> >  err_gc:
> > @@ -920,6 +1123,10 @@ static void mcp2221_remove(struct hid_device *hdev)
> >       i2c_del_adapter(&mcp->adapter);
> >       hid_hw_close(mcp->hdev);
> >       hid_hw_stop(mcp->hdev);
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +     if (mcp->indio_dev)
> > +             iio_device_unregister(mcp->indio_dev);
> > +#endif
> I'd expect remove to be reverse order of probe. Mind you this driver has a fun
> mix of devm and non devm which makes it very hard to reason about correctness
> and potential race conditions.  I would personally advocate preceding this
> patch with a cleanup of that side of things (probably mass usage of devm_add_action_or_reset()
> and appropriate callbacks).

Yeah the mix of devm and non-devm i agree is less than ideal..

 - Matt
>
> Jonathan
>
> >  }
> >
> >  static const struct hid_device_id mcp2221_devices[] = {
>

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

[Matt Ranostay]

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:
> >
> > 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.
>
> I tried to leave the comments not clashed with Jonathan's ones below.
>
> ...
>
> > Cc: Rishi Gupta <gupt21@gmail.com>
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Use --cc in the parameters to `git format-patch` or move them after
> the cutter '---' line below, so they won't pollute the Git commit
> message.

Noted for the future.
>
> ...
>
> > -       depends on GPIOLIB
> > +       select GPIOLIB
>
> I'm not sure why.

Changed to select from 'depends on' to avoid this circular dependency

  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
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
For a resolution refer to Documentation/kbuild/kconfig-language.rst

>
> ...
>
> >  #include <linux/hidraw.h>
> >  #include <linux/i2c.h>
> >  #include <linux/gpio/driver.h>
>
> + blank line.
>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
>
> + blank line.
>
> >  #include "hid-ids.h"
>
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +       struct iio_chan_spec iio_channels[3];
> > +       struct iio_dev *indio_dev;
> > +       u16 adc_values[3];
> > +       u8 dac_value;
> > +#endif
> > +};
>
> ...
>
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +                       if (mcp->indio_dev)
> > +                               memcpy(&mcp->adc_values, &data[50], 6);
>
> sizeof()

sizeof(mcp->adc_values) would work here if needed.

>
> > +#endif
>
> ...
>
> > +               // Confirm value is within 10-bit range
> > +               if (*val > GENMASK(9, 0))
>
>   if (*val >= BIT(10))
>
>  will make comment useless
>
> > +                       return -EINVAL;
> > +       }
>
> ...
>
> > +       if (val < 0 || val > GENMASK(4, 0))
>
> In a similar way, val >= BIT(5).
>
> > +               return -EINVAL;
>
> ...
>
> > +       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..

>
> ...
>
> > +       ret = mcp_send_data_req_status(mcp, mcp->txbuf, 12);
>
> Ditto,

See above.

>
> > +       hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
>
> Even in an error case?

hid_hw_power was set to PM_HINT_FULLON before the check, and in error case it
should reset back to normal hint.

>
> > +       if (ret) {
> > +               mutex_unlock(&mcp->lock);
> > +               return -EINVAL;
> > +       }
>
> ...
>
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +       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);
> > +#endif
>
> ...
>
> 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

- Matt

>
> --
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

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.

...

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

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

[Jonathan Cameron]

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

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

[Andy Shevchenko]

On Sat, Aug 6, 2022 at 6:18 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 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:

...

> > > > > +       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 :)

Good point. I think that check is simply not needed then. We should
never come to _unregister() with a NULL pointer, right?

> 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.

-- 
With Best Regards,
Andy Shevchenko