[PATCH v6 0/3] HID: mcp2221: iio support and device resource management

[PATCH v6 0/3] HID: mcp2221: iio support and device resource management

[Matt Ranostay]

This patchset is primarily to enable iio support for the MCP2221 HID driver,
but requires several Kconfig changes and device resource management.

First attempt of this patchset is referenced here:

Link: https://lore.kernel.org/all/20220729154723.99947-1-matt.ranostay@xxxxxxxxxxxx/

Changes from v1:
* Fixing various Kconfig recursive dependencies that appear with 'imply IIO'
* Switch hid-mcp2221 driver to device managed resources for i2c support
* Reworking patchset per advice on lore.kernel.org link above

Changes from v2:
* add linux-iio list to CC

Changes from v3:
* replace .remove() tasks with devm_add_action_or_reset() in .probe()
* reschedule SRAM configuration read on failures
* add IIO_CHAN_INFO_SCALE values for ADC + DAC based on reference voltage

Changes from v4:
* add .remove function with no operation to avoid hid_hw_stop() being called
  twice due to new devm functions
* add retries limit to five for reading SRAM configuration data
* update 'io: adc: stx104: fix future recursive dependencies' to new Kconfig
  location for STX104 driver on linux-next pending-fixes branch

Changes from v5:
* dropped all Kconfig changes not related to mcp2221 driver
* changed 'select GPIOLIB' to imply for mcp2221 along with IS_REACHABLE check

Matt Ranostay (3):
  HID: mcp2221: switch i2c registration to devm functions
  HID: mcp2221: change 'select GPIOLIB' to imply
  HID: mcp2221: add ADC/DAC support via iio subsystem

 drivers/hid/Kconfig       |   3 +-
 drivers/hid/hid-mcp2221.c | 310 ++++++++++++++++++++++++++++++++++----
 2 files changed, 286 insertions(+), 27 deletions(-)

-- 
2.37.2

[PATCH v6 1/3] HID: mcp2221: switch i2c registration to devm functions

[Matt Ranostay]

Switch from i2c_add_adapter() to resource managed devm_i2c_add_adapter()
for matching rest of driver initialization, and more concise code.

Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/hid/hid-mcp2221.c | 50 +++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index de52e9f7bb8c..4d10a24e3e13 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -824,6 +824,20 @@ static int mcp2221_raw_event(struct hid_device *hdev,
 	return 1;
 }
 
+/* Device resource managed function for HID unregistration */
+static void mcp2221_hid_unregister(void *ptr)
+{
+	struct hid_device *hdev = ptr;
+
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+/* This is needed to be sure hid_hw_stop() isn't called twice by the subsystem */
+static void mcp2221_remove(struct hid_device *hdev)
+{
+}
+
 static int mcp2221_probe(struct hid_device *hdev,
 					const struct hid_device_id *id)
 {
@@ -849,7 +863,8 @@ static int mcp2221_probe(struct hid_device *hdev,
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "can't open device\n");
-		goto err_hstop;
+		hid_hw_stop(hdev);
+		return ret;
 	}
 
 	mutex_init(&mcp->lock);
@@ -857,6 +872,10 @@ static int mcp2221_probe(struct hid_device *hdev,
 	hid_set_drvdata(hdev, mcp);
 	mcp->hdev = hdev;
 
+	ret = devm_add_action_or_reset(&hdev->dev, mcp2221_hid_unregister, hdev);
+	if (ret)
+		return ret;
+
 	/* Set I2C bus clock diviser */
 	if (i2c_clk_freq > 400)
 		i2c_clk_freq = 400;
@@ -873,19 +892,17 @@ static int mcp2221_probe(struct hid_device *hdev,
 			"MCP2221 usb-i2c bridge on hidraw%d",
 			((struct hidraw *)hdev->hidraw)->minor);
 
-	ret = i2c_add_adapter(&mcp->adapter);
+	ret = devm_i2c_add_adapter(&hdev->dev, &mcp->adapter);
 	if (ret) {
 		hid_err(hdev, "can't add usb-i2c adapter: %d\n", ret);
-		goto err_i2c;
+		return ret;
 	}
 	i2c_set_adapdata(&mcp->adapter, mcp);
 
 	/* Setup GPIO chip */
 	mcp->gc = devm_kzalloc(&hdev->dev, sizeof(*mcp->gc), GFP_KERNEL);
-	if (!mcp->gc) {
-		ret = -ENOMEM;
-		goto err_gc;
-	}
+	if (!mcp->gc)
+		return -ENOMEM;
 
 	mcp->gc->label = "mcp2221_gpio";
 	mcp->gc->direction_input = mcp_gpio_direction_input;
@@ -900,26 +917,9 @@ static int mcp2221_probe(struct hid_device *hdev,
 
 	ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
 	if (ret)
-		goto err_gc;
+		return ret;
 
 	return 0;
-
-err_gc:
-	i2c_del_adapter(&mcp->adapter);
-err_i2c:
-	hid_hw_close(mcp->hdev);
-err_hstop:
-	hid_hw_stop(mcp->hdev);
-	return ret;
-}
-
-static void mcp2221_remove(struct hid_device *hdev)
-{
-	struct mcp2221 *mcp = hid_get_drvdata(hdev);
-
-	i2c_del_adapter(&mcp->adapter);
-	hid_hw_close(mcp->hdev);
-	hid_hw_stop(mcp->hdev);
 }
 
 static const struct hid_device_id mcp2221_devices[] = {
-- 
2.37.2

[PATCH v6 2/3] HID: mcp2221: change 'select GPIOLIB' to imply

[Matt Ranostay]

To avoid recursive dependencies on GPIOLIB when 'imply IIO' is requested
with other drivers we should switch GPIOLIB to an imply.

This isn't the most ideal solution but avoids modifiying the Kconfig for
other drivers, and only requires a singular IS_REACHABLE(CONFIG_GPIOLIB)
check.

Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/hid/Kconfig       | 2 +-
 drivers/hid/hid-mcp2221.c | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 185a077d59cd..745fc38794ad 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1252,7 +1252,7 @@ config HID_ALPS
 config HID_MCP2221
 	tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
 	depends on USB_HID && I2C
-	depends on GPIOLIB
+	imply GPIOLIB
 	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 4d10a24e3e13..fb54f1c6fd9c 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -915,9 +915,11 @@ static int mcp2221_probe(struct hid_device *hdev,
 	mcp->gc->can_sleep = 1;
 	mcp->gc->parent = &hdev->dev;
 
+#if IS_REACHABLE(CONFIG_GPIOLIB)
 	ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
 	if (ret)
 		return ret;
+#endif
 
 	return 0;
 }
-- 
2.37.2

[PATCH v6 3/3] 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.

Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
 drivers/hid/Kconfig       |   1 +
 drivers/hid/hid-mcp2221.c | 258 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 258 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 745fc38794ad..17cce4c50e8d 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1253,6 +1253,7 @@ config HID_MCP2221
 	tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
 	depends on USB_HID && I2C
 	imply 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 fb54f1c6fd9c..2b3c3a483300 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -10,12 +10,14 @@
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/bitfield.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
 #include <linux/hid.h>
 #include <linux/hidraw.h>
 #include <linux/i2c.h>
 #include <linux/gpio/driver.h>
+#include <linux/iio/iio.h>
 #include "hid-ids.h"
 
 /* Commands codes in a raw output report */
@@ -30,6 +32,9 @@ enum {
 	MCP2221_I2C_CANCEL = 0x10,
 	MCP2221_GPIO_SET = 0x50,
 	MCP2221_GPIO_GET = 0x51,
+	MCP2221_SET_SRAM_SETTINGS = 0x60,
+	MCP2221_GET_SRAM_SETTINGS = 0x61,
+	MCP2221_READ_FLASH_DATA = 0xb0,
 };
 
 /* Response codes in a raw input report */
@@ -89,6 +94,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 +103,18 @@ 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];
+	u16 adc_values[3];
+	u8 adc_scale;
+	u8 dac_value;
+	u16 dac_scale;
+#endif
+};
+
+struct mcp2221_iio {
+	struct mcp2221 *mcp;
 };
 
 /*
@@ -713,7 +731,7 @@ static int mcp_get_i2c_eng_state(struct mcp2221 *mcp,
 static int mcp2221_raw_event(struct hid_device *hdev,
 				struct hid_report *report, u8 *data, int size)
 {
-	u8 *buf;
+	u8 *buf, tmp;
 	struct mcp2221 *mcp = hid_get_drvdata(hdev);
 
 	switch (data[0]) {
@@ -745,6 +763,9 @@ 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)
+			memcpy(&mcp->adc_values, &data[50], sizeof(mcp->adc_values));
+#endif
 			break;
 		default:
 			mcp->status = -EIO;
@@ -816,6 +837,66 @@ 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;
+
+	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;
+			}
+
+#if IS_REACHABLE(CONFIG_IIO)
+			/* DAC scale value */
+			tmp = FIELD_GET(GENMASK(7, 6), data[6]);
+			if ((data[6] & BIT(5)) && tmp)
+				mcp->dac_scale = tmp + 4;
+			else
+				mcp->dac_scale = 5;
+
+			/* ADC scale value */
+			tmp = FIELD_GET(GENMASK(4, 3), data[7]);
+			if ((data[7] & BIT(2)) && tmp)
+				mcp->adc_scale = tmp - 1;
+			else
+				mcp->adc_scale = 0;
+#endif
+
+			break;
+		default:
+			mcp->status = -EAGAIN;
+		}
+		complete(&mcp->wait_in_report);
+		break;
+
 	default:
 		mcp->status = -EIO;
 		complete(&mcp->wait_in_report);
@@ -838,6 +919,176 @@ static void mcp2221_remove(struct hid_device *hdev)
 {
 }
 
+#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;
+
+	if (mask == IIO_CHAN_INFO_SCALE) {
+		if (channel->output)
+			*val = 1 << mcp->dac_scale;
+		else
+			*val = 1 << mcp->adc_scale;
+
+		return IIO_VAL_INT;
+	}
+
+	mutex_lock(&mcp->lock);
+
+	if (channel->output) {
+		*val = mcp->dac_value;
+		ret = IIO_VAL_INT;
+	} else {
+		/* Read ADC values */
+		ret = mcp_chk_last_cmd_status(mcp);
+
+		if (!ret) {
+			*val = le16_to_cpu(mcp->adc_values[channel->address]);
+			if (*val >= BIT(10))
+				ret =  -EINVAL;
+			else
+				ret = IIO_VAL_INT;
+		}
+	}
+
+	mutex_unlock(&mcp->lock);
+
+	return ret;
+}
+
+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 >= BIT(5))
+		return -EINVAL;
+
+	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);
+	if (!ret)
+		mcp->dac_value = val;
+
+	mutex_unlock(&mcp->lock);
+
+	return ret;
+}
+
+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->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+		chan->scan_index = -1;
+	}
+
+	return cnt;
+}
+
+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;
+	static int retries = 5;
+	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);
+
+	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);
+
+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);
+
+	if (!retries--)
+		return;
+
+	/* Device is not ready to read SRAM or FLASH data, try again */
+	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(100));
+}
+#endif
+
 static int mcp2221_probe(struct hid_device *hdev,
 					const struct hid_device_id *id)
 {
@@ -921,6 +1172,11 @@ static int mcp2221_probe(struct hid_device *hdev,
 		return ret;
 #endif
 
+#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;
 }
 
-- 
2.37.2

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

[Jonathan Cameron]

On Fri, 30 Sep 2022 17:52:08 -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>

I'm never particularly keen on drivers from elsewhere in the tree gaining
IIO support - but that's just because it can make a bit of a mess of
changes to the IIO subsystem itself.  Having said that, this code looks fine to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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

[Jiri Kosina]

On Sun, 2 Oct 2022, Jonathan Cameron 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>
> 
> I'm never particularly keen on drivers from elsewhere in the tree gaining
> IIO support - but that's just because it can make a bit of a mess of
> changes to the IIO subsystem itself.  Having said that, this code looks fine to me.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks. Applied for 6.2.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v6 2/3] HID: mcp2221: change 'select GPIOLIB' to imply

[Benjamin Tissoires]

On Sat, Oct 1, 2022 at 2:52 AM Matt Ranostay <matt.ranostay@konsulko.com> wrote:
>
> To avoid recursive dependencies on GPIOLIB when 'imply IIO' is requested
> with other drivers we should switch GPIOLIB to an imply.
>
> This isn't the most ideal solution but avoids modifiying the Kconfig for
> other drivers, and only requires a singular IS_REACHABLE(CONFIG_GPIOLIB)
> check.
>
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
>  drivers/hid/Kconfig       | 2 +-
>  drivers/hid/hid-mcp2221.c | 2 ++
>  2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index 185a077d59cd..745fc38794ad 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -1252,7 +1252,7 @@ config HID_ALPS
>  config HID_MCP2221
>         tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
>         depends on USB_HID && I2C
> -       depends on GPIOLIB
> +       imply GPIOLIB
>         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 4d10a24e3e13..fb54f1c6fd9c 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -915,9 +915,11 @@ static int mcp2221_probe(struct hid_device *hdev,
>         mcp->gc->can_sleep = 1;
>         mcp->gc->parent = &hdev->dev;
>
> +#if IS_REACHABLE(CONFIG_GPIOLIB)
>         ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
>         if (ret)
>                 return ret;
> +#endif

Hi Matt,

This patch actually breaks my CI because devm_gpiochip_add_data() is
not the only one function that should be protected against
CONFIG_GPIOLIB.

I am getting:
---
ERROR: modpost: "gpiochip_get_data" [drivers/hid/hid-mcp2221.ko] undefined!
---

Can you also protect gpiochip_get_data() and make sure that the driver
is not completely buggy after? I assume a simple #if around all of the
calls will be worse than the current non compiling situation.

Cheers,
Benjamin

>
>         return 0;
>  }
> --
> 2.37.2
>

Re: [PATCH v6 2/3] HID: mcp2221: change 'select GPIOLIB' to imply

[Jiri Kosina]

On Fri, 21 Oct 2022, Benjamin Tissoires wrote:

> > To avoid recursive dependencies on GPIOLIB when 'imply IIO' is requested
> > with other drivers we should switch GPIOLIB to an imply.
> >
> > This isn't the most ideal solution but avoids modifiying the Kconfig for
> > other drivers, and only requires a singular IS_REACHABLE(CONFIG_GPIOLIB)
> > check.
> >
> > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > ---
> >  drivers/hid/Kconfig       | 2 +-
> >  drivers/hid/hid-mcp2221.c | 2 ++
> >  2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index 185a077d59cd..745fc38794ad 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -1252,7 +1252,7 @@ config HID_ALPS
> >  config HID_MCP2221
> >         tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
> >         depends on USB_HID && I2C
> > -       depends on GPIOLIB
> > +       imply GPIOLIB
> >         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 4d10a24e3e13..fb54f1c6fd9c 100644
> > --- a/drivers/hid/hid-mcp2221.c
> > +++ b/drivers/hid/hid-mcp2221.c
> > @@ -915,9 +915,11 @@ static int mcp2221_probe(struct hid_device *hdev,
> >         mcp->gc->can_sleep = 1;
> >         mcp->gc->parent = &hdev->dev;
> >
> > +#if IS_REACHABLE(CONFIG_GPIOLIB)
> >         ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
> >         if (ret)
> >                 return ret;
> > +#endif
> 
> Hi Matt,
> 
> This patch actually breaks my CI because devm_gpiochip_add_data() is
> not the only one function that should be protected against
> CONFIG_GPIOLIB.
> 
> I am getting:
> ---
> ERROR: modpost: "gpiochip_get_data" [drivers/hid/hid-mcp2221.ko] undefined!
> ---
> 
> Can you also protect gpiochip_get_data() and make sure that the driver
> is not completely buggy after? I assume a simple #if around all of the
> calls will be worse than the current non compiling situation.

Benjamin,

this should be fixed in hid.git via 3d74c9eca1a2bda. If you still see 
issues with that applied, please speak up :)

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v6 2/3] HID: mcp2221: change 'select GPIOLIB' to imply

[Benjamin Tissoires]

On Fri, Oct 21, 2022 at 2:13 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> On Fri, 21 Oct 2022, Benjamin Tissoires wrote:
>
> > > To avoid recursive dependencies on GPIOLIB when 'imply IIO' is requested
> > > with other drivers we should switch GPIOLIB to an imply.
> > >
> > > This isn't the most ideal solution but avoids modifiying the Kconfig for
> > > other drivers, and only requires a singular IS_REACHABLE(CONFIG_GPIOLIB)
> > > check.
> > >
> > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> > > ---
> > >  drivers/hid/Kconfig       | 2 +-
> > >  drivers/hid/hid-mcp2221.c | 2 ++
> > >  2 files changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > > index 185a077d59cd..745fc38794ad 100644
> > > --- a/drivers/hid/Kconfig
> > > +++ b/drivers/hid/Kconfig
> > > @@ -1252,7 +1252,7 @@ config HID_ALPS
> > >  config HID_MCP2221
> > >         tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
> > >         depends on USB_HID && I2C
> > > -       depends on GPIOLIB
> > > +       imply GPIOLIB
> > >         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 4d10a24e3e13..fb54f1c6fd9c 100644
> > > --- a/drivers/hid/hid-mcp2221.c
> > > +++ b/drivers/hid/hid-mcp2221.c
> > > @@ -915,9 +915,11 @@ static int mcp2221_probe(struct hid_device *hdev,
> > >         mcp->gc->can_sleep = 1;
> > >         mcp->gc->parent = &hdev->dev;
> > >
> > > +#if IS_REACHABLE(CONFIG_GPIOLIB)
> > >         ret = devm_gpiochip_add_data(&hdev->dev, mcp->gc, mcp);
> > >         if (ret)
> > >                 return ret;
> > > +#endif
> >
> > Hi Matt,
> >
> > This patch actually breaks my CI because devm_gpiochip_add_data() is
> > not the only one function that should be protected against
> > CONFIG_GPIOLIB.
> >
> > I am getting:
> > ---
> > ERROR: modpost: "gpiochip_get_data" [drivers/hid/hid-mcp2221.ko] undefined!
> > ---
> >
> > Can you also protect gpiochip_get_data() and make sure that the driver
> > is not completely buggy after? I assume a simple #if around all of the
> > calls will be worse than the current non compiling situation.
>
> Benjamin,
>
> this should be fixed in hid.git via 3d74c9eca1a2bda. If you still see
> issues with that applied, please speak up :)
>

Oh, I wasn't Cc-ed on that series, and my bot still hasn't updated my CI branch.

Thanks for the fix Matt :)

Cheers,
Benjamin