* [PATCH v4 0/5] HID: mcp2221: iio support and device resource management
@ 2022-09-21 6:30 Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 1/5] i2c: muxes: ltc4306: fix future recursive dependencies Matt Ranostay
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 6:30 UTC (permalink / raw)
To: gupt21, jic23; +Cc: linux-i2c, linux-iio, linux-input, 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
Matt Ranostay (5):
i2c: muxes: ltc4306: fix future recursive dependencies
iio: addac: stx104: fix future recursive dependencies
iio: dac: fix future recursive dependencies
HID: mcp2221: switch i2c registration to devm functions
HID: mcp2221: add ADC/DAC support via iio subsystem
drivers/hid/Kconfig | 1 +
drivers/hid/hid-mcp2221.c | 297 ++++++++++++++++++++++++++++++++++----
drivers/i2c/muxes/Kconfig | 2 +-
drivers/iio/addac/Kconfig | 3 +-
drivers/iio/dac/Kconfig | 6 +-
5 files changed, 275 insertions(+), 34 deletions(-)
--
2.37.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/5] i2c: muxes: ltc4306: fix future recursive dependencies
2022-09-21 6:30 [PATCH v4 0/5] HID: mcp2221: iio support and device resource management Matt Ranostay
@ 2022-09-21 6:30 ` Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 2/5] iio: addac: stx104: " Matt Ranostay
` (3 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 6:30 UTC (permalink / raw)
To: gupt21, jic23; +Cc: linux-i2c, linux-iio, linux-input, Matt Ranostay
When using 'imply IIO' for other configurations which have 'select GPIOLIB'
the following recursive dependency is detected for I2C_MUX_LTC4306
Switch from 'select GPIOLIB' to 'depends on GPIOLIB' to avoid this per
recommendation in kconfig-language.rst
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:1227: symbol HID_MCP2221 depends on GPIOLIB
Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
drivers/i2c/muxes/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/muxes/Kconfig b/drivers/i2c/muxes/Kconfig
index ea838dbae32e..7b6a68df4a39 100644
--- a/drivers/i2c/muxes/Kconfig
+++ b/drivers/i2c/muxes/Kconfig
@@ -46,7 +46,7 @@ config I2C_MUX_GPMUX
config I2C_MUX_LTC4306
tristate "LTC LTC4306/5 I2C multiplexer"
- select GPIOLIB
+ depends on GPIOLIB
select REGMAP_I2C
help
If you say yes here you get support for the Analog Devices
--
2.37.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 2/5] iio: addac: stx104: fix future recursive dependencies
2022-09-21 6:30 [PATCH v4 0/5] HID: mcp2221: iio support and device resource management Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 1/5] i2c: muxes: ltc4306: fix future recursive dependencies Matt Ranostay
@ 2022-09-21 6:30 ` Matt Ranostay
2022-09-24 16:05 ` Jonathan Cameron
2022-09-21 6:30 ` [PATCH v4 3/5] iio: dac: " Matt Ranostay
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 6:30 UTC (permalink / raw)
To: gupt21, jic23; +Cc: linux-i2c, linux-iio, linux-input, Matt Ranostay
When using 'imply IIO' for other configurations which have 'select GPIOLIB'
the following recursive dependency is detected for STX1040
Switch from 'select GPIOLIB' to 'depends on GPIOLIB' to avoid this per
recommendation in kconfig-language.rst
drivers/gpio/Kconfig:14:error: recursive dependency detected!
drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by STX104
drivers/iio/addac/Kconfig:20: symbol STX104 depends on IIO
drivers/iio/Kconfig:6: symbol IIO is implied by HID_MCP2221
drivers/hid/Kconfig:1227: symbol HID_MCP2221 depends on GPIOLIB
Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
drivers/iio/addac/Kconfig | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/iio/addac/Kconfig b/drivers/iio/addac/Kconfig
index fcf6d2269bfc..494790816ac7 100644
--- a/drivers/iio/addac/Kconfig
+++ b/drivers/iio/addac/Kconfig
@@ -19,9 +19,8 @@ config AD74413R
config STX104
tristate "Apex Embedded Systems STX104 driver"
- depends on PC104 && X86
+ depends on PC104 && X86 && GPIOLIB
select ISA_BUS_API
- select GPIOLIB
help
Say yes here to build support for the Apex Embedded Systems STX104
integrated analog PC/104 card.
--
2.37.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 3/5] iio: dac: fix future recursive dependencies
2022-09-21 6:30 [PATCH v4 0/5] HID: mcp2221: iio support and device resource management Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 1/5] i2c: muxes: ltc4306: fix future recursive dependencies Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 2/5] iio: addac: stx104: " Matt Ranostay
@ 2022-09-21 6:30 ` Matt Ranostay
2022-09-24 16:06 ` Jonathan Cameron
2022-09-21 6:30 ` [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem Matt Ranostay
4 siblings, 1 reply; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 6:30 UTC (permalink / raw)
To: gupt21, jic23; +Cc: linux-i2c, linux-iio, linux-input, Matt Ranostay
When using 'imply IIO' for other configurations which have 'select GPIOLIB'
the following recursive dependency is detected for AD5592R/AD5593R
Switch from 'select GPIOLIB' to 'depends on GPIOLIB' to avoid this per
recommendation in kconfig-language.rst
drivers/gpio/Kconfig:14:error: recursive dependency detected!
drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by AD5592R
drivers/iio/dac/Kconfig:93: symbol AD5592R depends on IIO
drivers/iio/Kconfig:6: symbol IIO is implied by HID_MCP2221
drivers/hid/Kconfig:1227: symbol HID_MCP2221 depends on GPIOLIB
Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
---
drivers/iio/dac/Kconfig | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 80521bd28d0f..b93003e80b70 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -92,8 +92,7 @@ config AD5592R_BASE
config AD5592R
tristate "Analog Devices AD5592R ADC/DAC driver"
- depends on SPI_MASTER
- select GPIOLIB
+ depends on SPI_MASTER && GPIOLIB
select AD5592R_BASE
help
Say yes here to build support for Analog Devices AD5592R
@@ -104,8 +103,7 @@ config AD5592R
config AD5593R
tristate "Analog Devices AD5593R ADC/DAC driver"
- depends on I2C
- select GPIOLIB
+ depends on I2C && GPIOLIB
select AD5592R_BASE
help
Say yes here to build support for Analog Devices AD5593R
--
2.37.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-21 6:30 [PATCH v4 0/5] HID: mcp2221: iio support and device resource management Matt Ranostay
` (2 preceding siblings ...)
2022-09-21 6:30 ` [PATCH v4 3/5] iio: dac: " Matt Ranostay
@ 2022-09-21 6:30 ` Matt Ranostay
2022-09-21 8:04 ` Benjamin Tissoires
2022-09-21 6:30 ` [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem Matt Ranostay
4 siblings, 1 reply; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 6:30 UTC (permalink / raw)
To: gupt21, jic23; +Cc: linux-i2c, linux-iio, linux-input, 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 | 45 +++++++++++++++++----------------------
1 file changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
index de52e9f7bb8c..7ba63bcd66de 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
return 1;
}
+static void mcp2221_hid_remove(void *ptr)
+{
+ struct hid_device *hdev = ptr;
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
+
static int mcp2221_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
@@ -849,7 +857,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 +866,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_remove, hdev);
+ if (ret)
+ return ret;
+
/* Set I2C bus clock diviser */
if (i2c_clk_freq > 400)
i2c_clk_freq = 400;
@@ -873,19 +886,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 +911,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[] = {
@@ -932,7 +926,6 @@ static struct hid_driver mcp2221_driver = {
.name = "mcp2221",
.id_table = mcp2221_devices,
.probe = mcp2221_probe,
- .remove = mcp2221_remove,
.raw_event = mcp2221_raw_event,
};
--
2.37.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem
2022-09-21 6:30 [PATCH v4 0/5] HID: mcp2221: iio support and device resource management Matt Ranostay
` (3 preceding siblings ...)
2022-09-21 6:30 ` [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions Matt Ranostay
@ 2022-09-21 6:30 ` Matt Ranostay
2022-09-22 10:23 ` kernel test robot
2022-09-24 16:25 ` Jonathan Cameron
4 siblings, 2 replies; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 6:30 UTC (permalink / raw)
To: gupt21, jic23; +Cc: linux-i2c, linux-iio, linux-input, 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 | 252 +++++++++++++++++++++++++++++++++++++-
2 files changed, 252 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 863d1f96ea57..cdae312f4795 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -1228,6 +1228,7 @@ config HID_MCP2221
tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support"
depends on USB_HID && I2C
depends on 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 7ba63bcd66de..2a7783b607af 100644
--- a/drivers/hid/hid-mcp2221.c
+++ b/drivers/hid/hid-mcp2221.c
@@ -16,6 +16,7 @@
#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 +31,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 +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,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 +730,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 +762,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 +836,64 @@ 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;
+ }
+
+ /* DAC scale value */
+ tmp = (data[6] >> 6) & 0x3;
+ 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;
+
default:
mcp->status = -EIO;
complete(&mcp->wait_in_report);
@@ -832,6 +910,173 @@ static void mcp2221_hid_remove(void *ptr)
hid_hw_stop(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;
+ 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);
+
+ /* 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)
{
@@ -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;
}
--
2.37.2
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-21 6:30 ` [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions Matt Ranostay
@ 2022-09-21 8:04 ` Benjamin Tissoires
2022-09-21 17:57 ` Matt Ranostay
0 siblings, 1 reply; 16+ messages in thread
From: Benjamin Tissoires @ 2022-09-21 8:04 UTC (permalink / raw)
To: Matt Ranostay
Cc: gupt21, jic23, linux-i2c, linux-iio, linux-input,
benjamin.tissoires, Jiri Kosina
[foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
the series, as you will need ack from us and we don't necessarily monitor
every single message on linux-input]
On Sep 20 2022, Matt Ranostay wrote:
> 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 | 45 +++++++++++++++++----------------------
> 1 file changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> index de52e9f7bb8c..7ba63bcd66de 100644
> --- a/drivers/hid/hid-mcp2221.c
> +++ b/drivers/hid/hid-mcp2221.c
> @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> return 1;
> }
>
> +static void mcp2221_hid_remove(void *ptr)
> +{
> + struct hid_device *hdev = ptr;
> +
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
By default, if you remove the .remove() callback, hid_hw_stop() will get
automatically called by hid-core.c. So we are now calling it twice,
which, in a way is not a big deal but it might be an issue in the long
run.
Generally speaking, in the HID subsystem, that situation doesn't happen
a lot because hid_hw_start() is usually the last command of probe, and
we don't need to open the device in the driver itself.
Here, I guess as soon as you add the i2c adapter, you might want to have
the communication channels ready, and thus you need to have it open
*before* i2c_add_adapter.
I would suggest the following if you want to keep the devm release of
stop and close: please put a big fat warning before mcp2221_hid_remove()
explaining that this is called in devm management, *and* add a function
that would just return 0 as the .remove() callback with another big fat
warning explaining that we don't want hid-core.c to call hid_hw_stop()
because we are doing it ourself through devres.
Last, in the HID subsystem, we often interleave non devres with devres
for resource allocation, given that .remove() will be called before any
devres release. But that is assuming this ordering is OK, which doesn't
seem to be the case here. We first need to unregister the i2c adapter
and then close/stop the HID device.
> +}
> +
> static int mcp2221_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> {
> @@ -849,7 +857,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 +866,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_remove, hdev);
> + if (ret)
> + return ret;
> +
> /* Set I2C bus clock diviser */
> if (i2c_clk_freq > 400)
> i2c_clk_freq = 400;
> @@ -873,19 +886,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 +911,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[] = {
> @@ -932,7 +926,6 @@ static struct hid_driver mcp2221_driver = {
> .name = "mcp2221",
> .id_table = mcp2221_devices,
> .probe = mcp2221_probe,
> - .remove = mcp2221_remove,
> .raw_event = mcp2221_raw_event,
> };
>
> --
> 2.37.2
>
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-21 8:04 ` Benjamin Tissoires
@ 2022-09-21 17:57 ` Matt Ranostay
2022-09-22 23:44 ` Matt Ranostay
0 siblings, 1 reply; 16+ messages in thread
From: Matt Ranostay @ 2022-09-21 17:57 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: gupt21, jic23, linux-i2c, linux-iio, linux-input, Jiri Kosina
On Wed, Sep 21, 2022 at 1:05 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> [foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
> the series, as you will need ack from us and we don't necessarily monitor
> every single message on linux-input]
>
> On Sep 20 2022, Matt Ranostay wrote:
> > 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 | 45 +++++++++++++++++----------------------
> > 1 file changed, 19 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > index de52e9f7bb8c..7ba63bcd66de 100644
> > --- a/drivers/hid/hid-mcp2221.c
> > +++ b/drivers/hid/hid-mcp2221.c
> > @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> > return 1;
> > }
> >
> > +static void mcp2221_hid_remove(void *ptr)
> > +{
> > + struct hid_device *hdev = ptr;
> > +
> > + hid_hw_close(hdev);
> > + hid_hw_stop(hdev);
>
> By default, if you remove the .remove() callback, hid_hw_stop() will get
> automatically called by hid-core.c. So we are now calling it twice,
> which, in a way is not a big deal but it might be an issue in the long
> run.
>
> Generally speaking, in the HID subsystem, that situation doesn't happen
> a lot because hid_hw_start() is usually the last command of probe, and
> we don't need to open the device in the driver itself.
>
> Here, I guess as soon as you add the i2c adapter, you might want to have
> the communication channels ready, and thus you need to have it open
> *before* i2c_add_adapter.
>
> I would suggest the following if you want to keep the devm release of
> stop and close: please put a big fat warning before mcp2221_hid_remove()
> explaining that this is called in devm management, *and* add a function
> that would just return 0 as the .remove() callback with another big fat
> warning explaining that we don't want hid-core.c to call hid_hw_stop()
> because we are doing it ourself through devres.
>
Yeah maybe best to keep the non-devres if it isn't going to affect how the last
change in this series is trying to implement with iio.
I'll wait for Jonathan to chime in on this thread.
> Last, in the HID subsystem, we often interleave non devres with devres
> for resource allocation, given that .remove() will be called before any
> devres release. But that is assuming this ordering is OK, which doesn't
> seem to be the case here. We first need to unregister the i2c adapter
> and then close/stop the HID device.
Noted.
- Matt
>
> > +}
> > +
> > static int mcp2221_probe(struct hid_device *hdev,
> > const struct hid_device_id *id)
> > {
> > @@ -849,7 +857,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 +866,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_remove, hdev);
> > + if (ret)
> > + return ret;
> > +
> > /* Set I2C bus clock diviser */
> > if (i2c_clk_freq > 400)
> > i2c_clk_freq = 400;
> > @@ -873,19 +886,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 +911,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[] = {
> > @@ -932,7 +926,6 @@ static struct hid_driver mcp2221_driver = {
> > .name = "mcp2221",
> > .id_table = mcp2221_devices,
> > .probe = mcp2221_probe,
> > - .remove = mcp2221_remove,
> > .raw_event = mcp2221_raw_event,
> > };
> >
> > --
> > 2.37.2
> >
>
> Cheers,
> Benjamin
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem
2022-09-21 6:30 ` [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem Matt Ranostay
@ 2022-09-22 10:23 ` kernel test robot
2022-09-24 16:25 ` Jonathan Cameron
1 sibling, 0 replies; 16+ messages in thread
From: kernel test robot @ 2022-09-22 10:23 UTC (permalink / raw)
To: Matt Ranostay, gupt21, jic23
Cc: llvm, kbuild-all, linux-i2c, linux-iio, linux-input, Matt Ranostay
Hi Matt,
I love your patch! Yet something to improve:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on next-20220921]
[cannot apply to hid/for-next wsa/i2c/for-next linus/master v6.0-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Matt-Ranostay/HID-mcp2221-iio-support-and-device-resource-management/20220921-143207
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: hexagon-randconfig-r023-20220921 (https://download.01.org/0day-ci/archive/20220922/202209221851.IOfsmA0z-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/9576b88476cb586e6d9f8ef77969f1acd5e4a241
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Matt-Ranostay/HID-mcp2221-iio-support-and-device-resource-management/20220921-143207
git checkout 9576b88476cb586e6d9f8ef77969f1acd5e4a241
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/hid/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/hid/hid-mcp2221.c:879:10: error: no member named 'dac_scale' in 'struct mcp2221'
mcp->dac_scale = tmp + 4;
~~~ ^
drivers/hid/hid-mcp2221.c:881:10: error: no member named 'dac_scale' in 'struct mcp2221'
mcp->dac_scale = 5;
~~~ ^
>> drivers/hid/hid-mcp2221.c:886:10: error: no member named 'adc_scale' in 'struct mcp2221'
mcp->adc_scale = tmp - 1;
~~~ ^
drivers/hid/hid-mcp2221.c:888:10: error: no member named 'adc_scale' in 'struct mcp2221'
mcp->adc_scale = 0;
~~~ ^
4 errors generated.
vim +879 drivers/hid/hid-mcp2221.c
720
721 /*
722 * MCP2221 uses interrupt endpoint for input reports. This function
723 * is called by HID layer when it receives i/p report from mcp2221,
724 * which is actually a response to the previously sent command.
725 *
726 * MCP2221A firmware specific return codes are parsed and 0 or
727 * appropriate negative error code is returned. Delayed response
728 * results in timeout error and stray reponses results in -EIO.
729 */
730 static int mcp2221_raw_event(struct hid_device *hdev,
731 struct hid_report *report, u8 *data, int size)
732 {
733 u8 *buf, tmp;
734 struct mcp2221 *mcp = hid_get_drvdata(hdev);
735
736 switch (data[0]) {
737
738 case MCP2221_I2C_WR_DATA:
739 case MCP2221_I2C_WR_NO_STOP:
740 case MCP2221_I2C_RD_DATA:
741 case MCP2221_I2C_RD_RPT_START:
742 switch (data[1]) {
743 case MCP2221_SUCCESS:
744 mcp->status = 0;
745 break;
746 default:
747 mcp->status = mcp_get_i2c_eng_state(mcp, data, 2);
748 }
749 complete(&mcp->wait_in_report);
750 break;
751
752 case MCP2221_I2C_PARAM_OR_STATUS:
753 switch (data[1]) {
754 case MCP2221_SUCCESS:
755 if ((mcp->txbuf[3] == MCP2221_I2C_SET_SPEED) &&
756 (data[3] != MCP2221_I2C_SET_SPEED)) {
757 mcp->status = -EAGAIN;
758 break;
759 }
760 if (data[20] & MCP2221_I2C_MASK_ADDR_NACK) {
761 mcp->status = -ENXIO;
762 break;
763 }
764 mcp->status = mcp_get_i2c_eng_state(mcp, data, 8);
765 #if IS_REACHABLE(CONFIG_IIO)
766 memcpy(&mcp->adc_values, &data[50], sizeof(mcp->adc_values));
767 #endif
768 break;
769 default:
770 mcp->status = -EIO;
771 }
772 complete(&mcp->wait_in_report);
773 break;
774
775 case MCP2221_I2C_GET_DATA:
776 switch (data[1]) {
777 case MCP2221_SUCCESS:
778 if (data[2] == MCP2221_I2C_ADDR_NACK) {
779 mcp->status = -ENXIO;
780 break;
781 }
782 if (!mcp_get_i2c_eng_state(mcp, data, 2)
783 && (data[3] == 0)) {
784 mcp->status = 0;
785 break;
786 }
787 if (data[3] == 127) {
788 mcp->status = -EIO;
789 break;
790 }
791 if (data[2] == MCP2221_I2C_READ_COMPL) {
792 buf = mcp->rxbuf;
793 memcpy(&buf[mcp->rxbuf_idx], &data[4], data[3]);
794 mcp->rxbuf_idx = mcp->rxbuf_idx + data[3];
795 mcp->status = 0;
796 break;
797 }
798 mcp->status = -EIO;
799 break;
800 default:
801 mcp->status = -EIO;
802 }
803 complete(&mcp->wait_in_report);
804 break;
805
806 case MCP2221_GPIO_GET:
807 switch (data[1]) {
808 case MCP2221_SUCCESS:
809 if ((data[mcp->gp_idx] == MCP2221_ALT_F_NOT_GPIOV) ||
810 (data[mcp->gp_idx + 1] == MCP2221_ALT_F_NOT_GPIOD)) {
811 mcp->status = -ENOENT;
812 } else {
813 mcp->status = !!data[mcp->gp_idx];
814 mcp->gpio_dir = data[mcp->gp_idx + 1];
815 }
816 break;
817 default:
818 mcp->status = -EAGAIN;
819 }
820 complete(&mcp->wait_in_report);
821 break;
822
823 case MCP2221_GPIO_SET:
824 switch (data[1]) {
825 case MCP2221_SUCCESS:
826 if ((data[mcp->gp_idx] == MCP2221_ALT_F_NOT_GPIOV) ||
827 (data[mcp->gp_idx - 1] == MCP2221_ALT_F_NOT_GPIOV)) {
828 mcp->status = -ENOENT;
829 } else {
830 mcp->status = 0;
831 }
832 break;
833 default:
834 mcp->status = -EAGAIN;
835 }
836 complete(&mcp->wait_in_report);
837 break;
838
839 case MCP2221_SET_SRAM_SETTINGS:
840 switch (data[1]) {
841 case MCP2221_SUCCESS:
842 mcp->status = 0;
843 break;
844 default:
845 mcp->status = -EAGAIN;
846 }
847 complete(&mcp->wait_in_report);
848 break;
849
850 case MCP2221_GET_SRAM_SETTINGS:
851 switch (data[1]) {
852 case MCP2221_SUCCESS:
853 memcpy(&mcp->mode, &data[22], 4);
854 #if IS_REACHABLE(CONFIG_IIO)
855 mcp->dac_value = data[6] & GENMASK(4, 0);
856 #endif
857 mcp->status = 0;
858 break;
859 default:
860 mcp->status = -EAGAIN;
861 }
862 complete(&mcp->wait_in_report);
863 break;
864
865 case MCP2221_READ_FLASH_DATA:
866 switch (data[1]) {
867 case MCP2221_SUCCESS:
868 mcp->status = 0;
869
870 /* Only handles CHIP SETTINGS subpage currently */
871 if (mcp->txbuf[1] != 0) {
872 mcp->status = -EIO;
873 break;
874 }
875
876 /* DAC scale value */
877 tmp = (data[6] >> 6) & 0x3;
878 if ((data[6] & BIT(5)) && tmp)
> 879 mcp->dac_scale = tmp + 4;
880 else
881 mcp->dac_scale = 5;
882
883 /* ADC scale value */
884 tmp = (data[7] >> 3) & 0x3;
885 if ((data[7] & BIT(2)) && tmp)
> 886 mcp->adc_scale = tmp - 1;
887 else
888 mcp->adc_scale = 0;
889
890 break;
891 default:
892 mcp->status = -EAGAIN;
893 }
894 complete(&mcp->wait_in_report);
895 break;
896
897 default:
898 mcp->status = -EIO;
899 complete(&mcp->wait_in_report);
900 }
901
902 return 1;
903 }
904
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-21 17:57 ` Matt Ranostay
@ 2022-09-22 23:44 ` Matt Ranostay
2022-09-23 7:03 ` Benjamin Tissoires
0 siblings, 1 reply; 16+ messages in thread
From: Matt Ranostay @ 2022-09-22 23:44 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: gupt21, jic23, linux-i2c, linux-iio, linux-input, Jiri Kosina
On Wed, Sep 21, 2022 at 10:57 AM Matt Ranostay
<matt.ranostay@konsulko.com> wrote:
>
> On Wed, Sep 21, 2022 at 1:05 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > [foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
> > the series, as you will need ack from us and we don't necessarily monitor
> > every single message on linux-input]
> >
> > On Sep 20 2022, Matt Ranostay wrote:
> > > 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 | 45 +++++++++++++++++----------------------
> > > 1 file changed, 19 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > > index de52e9f7bb8c..7ba63bcd66de 100644
> > > --- a/drivers/hid/hid-mcp2221.c
> > > +++ b/drivers/hid/hid-mcp2221.c
> > > @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> > > return 1;
> > > }
> > >
> > > +static void mcp2221_hid_remove(void *ptr)
> > > +{
> > > + struct hid_device *hdev = ptr;
> > > +
> > > + hid_hw_close(hdev);
> > > + hid_hw_stop(hdev);
> >
> > By default, if you remove the .remove() callback, hid_hw_stop() will get
> > automatically called by hid-core.c. So we are now calling it twice,
> > which, in a way is not a big deal but it might be an issue in the long
> > run.
> >
> > Generally speaking, in the HID subsystem, that situation doesn't happen
> > a lot because hid_hw_start() is usually the last command of probe, and
> > we don't need to open the device in the driver itself.
> >
> > Here, I guess as soon as you add the i2c adapter, you might want to have
> > the communication channels ready, and thus you need to have it open
> > *before* i2c_add_adapter.
> >
> > I would suggest the following if you want to keep the devm release of
> > stop and close: please put a big fat warning before mcp2221_hid_remove()
> > explaining that this is called in devm management, *and* add a function
> > that would just return 0 as the .remove() callback with another big fat
> > warning explaining that we don't want hid-core.c to call hid_hw_stop()
> > because we are doing it ourself through devres.
> >
>
> Yeah maybe best to keep the non-devres if it isn't going to affect how the last
> change in this series is trying to implement with iio.
>
> I'll wait for Jonathan to chime in on this thread.
>
> > Last, in the HID subsystem, we often interleave non devres with devres
> > for resource allocation, given that .remove() will be called before any
> > devres release. But that is assuming this ordering is OK, which doesn't
> > seem to be the case here. We first need to unregister the i2c adapter
> > and then close/stop the HID device.
On second thought I2C will be unregistered before the HID calls, since
unless I'm totally
incorrect device resource management unwinds backwards in the order actions are
registered.
- Matt
>
> Noted.
>
> - Matt
>
> >
> > > +}
> > > +
> > > static int mcp2221_probe(struct hid_device *hdev,
> > > const struct hid_device_id *id)
> > > {
> > > @@ -849,7 +857,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 +866,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_remove, hdev);
> > > + if (ret)
> > > + return ret;
> > > +
> > > /* Set I2C bus clock diviser */
> > > if (i2c_clk_freq > 400)
> > > i2c_clk_freq = 400;
> > > @@ -873,19 +886,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 +911,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[] = {
> > > @@ -932,7 +926,6 @@ static struct hid_driver mcp2221_driver = {
> > > .name = "mcp2221",
> > > .id_table = mcp2221_devices,
> > > .probe = mcp2221_probe,
> > > - .remove = mcp2221_remove,
> > > .raw_event = mcp2221_raw_event,
> > > };
> > >
> > > --
> > > 2.37.2
> > >
> >
> > Cheers,
> > Benjamin
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-22 23:44 ` Matt Ranostay
@ 2022-09-23 7:03 ` Benjamin Tissoires
2022-09-23 21:22 ` Matt Ranostay
0 siblings, 1 reply; 16+ messages in thread
From: Benjamin Tissoires @ 2022-09-23 7:03 UTC (permalink / raw)
To: Matt Ranostay
Cc: Rishi Gupta, Jonathan Cameron, Linux I2C, linux-iio,
open list:HID CORE LAYER, Jiri Kosina
On Fri, Sep 23, 2022 at 1:45 AM Matt Ranostay
<matt.ranostay@konsulko.com> wrote:
>
> On Wed, Sep 21, 2022 at 10:57 AM Matt Ranostay
> <matt.ranostay@konsulko.com> wrote:
> >
> > On Wed, Sep 21, 2022 at 1:05 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > [foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
> > > the series, as you will need ack from us and we don't necessarily monitor
> > > every single message on linux-input]
> > >
> > > On Sep 20 2022, Matt Ranostay wrote:
> > > > 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 | 45 +++++++++++++++++----------------------
> > > > 1 file changed, 19 insertions(+), 26 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > > > index de52e9f7bb8c..7ba63bcd66de 100644
> > > > --- a/drivers/hid/hid-mcp2221.c
> > > > +++ b/drivers/hid/hid-mcp2221.c
> > > > @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> > > > return 1;
> > > > }
> > > >
> > > > +static void mcp2221_hid_remove(void *ptr)
> > > > +{
> > > > + struct hid_device *hdev = ptr;
> > > > +
> > > > + hid_hw_close(hdev);
> > > > + hid_hw_stop(hdev);
> > >
> > > By default, if you remove the .remove() callback, hid_hw_stop() will get
> > > automatically called by hid-core.c. So we are now calling it twice,
> > > which, in a way is not a big deal but it might be an issue in the long
> > > run.
> > >
> > > Generally speaking, in the HID subsystem, that situation doesn't happen
> > > a lot because hid_hw_start() is usually the last command of probe, and
> > > we don't need to open the device in the driver itself.
> > >
> > > Here, I guess as soon as you add the i2c adapter, you might want to have
> > > the communication channels ready, and thus you need to have it open
> > > *before* i2c_add_adapter.
> > >
> > > I would suggest the following if you want to keep the devm release of
> > > stop and close: please put a big fat warning before mcp2221_hid_remove()
> > > explaining that this is called in devm management, *and* add a function
> > > that would just return 0 as the .remove() callback with another big fat
> > > warning explaining that we don't want hid-core.c to call hid_hw_stop()
> > > because we are doing it ourself through devres.
> > >
> >
> > Yeah maybe best to keep the non-devres if it isn't going to affect how the last
> > change in this series is trying to implement with iio.
> >
> > I'll wait for Jonathan to chime in on this thread.
> >
> > > Last, in the HID subsystem, we often interleave non devres with devres
> > > for resource allocation, given that .remove() will be called before any
> > > devres release. But that is assuming this ordering is OK, which doesn't
> > > seem to be the case here. We first need to unregister the i2c adapter
> > > and then close/stop the HID device.
>
> On second thought I2C will be unregistered before the HID calls, since
> unless I'm totally
> incorrect device resource management unwinds backwards in the order actions are
> registered.
Yeah, sorry if it was not clear:
- .remove() is called *before* any devres action takes place
- devres action are LIFO, so unwinded backwards as you say
In the general case, a driver does:
int probe() {
void *pointer = devm_alloc(...)
some_more_devm_action(...)
hid_hw_start()
return 0;
}
and so the HID start action is the last one, meaning that .remove will
first call stop and then devres unwind will get called.
But here, in your case, you need hid_hw_start to be called *before*
devm_i2c_add_adapter(), meaning that the implicit .remove() will mess
up with the device, so you are forced to do something about it.
You can either keep a non devm variant, or you can override the
.remove() of HID to not do anything and do the stop/close in a
specific devm task, which you did here. You are just missing the
"let's override .remove() to ensure we keep the device open and
started while we need it".
Cheers,
Benjamin
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-23 7:03 ` Benjamin Tissoires
@ 2022-09-23 21:22 ` Matt Ranostay
2022-09-24 16:16 ` Jonathan Cameron
0 siblings, 1 reply; 16+ messages in thread
From: Matt Ranostay @ 2022-09-23 21:22 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Rishi Gupta, Jonathan Cameron, Linux I2C, linux-iio,
open list:HID CORE LAYER, Jiri Kosina
On Fri, Sep 23, 2022 at 12:03 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Fri, Sep 23, 2022 at 1:45 AM Matt Ranostay
> <matt.ranostay@konsulko.com> wrote:
> >
> > On Wed, Sep 21, 2022 at 10:57 AM Matt Ranostay
> > <matt.ranostay@konsulko.com> wrote:
> > >
> > > On Wed, Sep 21, 2022 at 1:05 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > [foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
> > > > the series, as you will need ack from us and we don't necessarily monitor
> > > > every single message on linux-input]
> > > >
> > > > On Sep 20 2022, Matt Ranostay wrote:
> > > > > 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 | 45 +++++++++++++++++----------------------
> > > > > 1 file changed, 19 insertions(+), 26 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > > > > index de52e9f7bb8c..7ba63bcd66de 100644
> > > > > --- a/drivers/hid/hid-mcp2221.c
> > > > > +++ b/drivers/hid/hid-mcp2221.c
> > > > > @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> > > > > return 1;
> > > > > }
> > > > >
> > > > > +static void mcp2221_hid_remove(void *ptr)
> > > > > +{
> > > > > + struct hid_device *hdev = ptr;
> > > > > +
> > > > > + hid_hw_close(hdev);
> > > > > + hid_hw_stop(hdev);
> > > >
> > > > By default, if you remove the .remove() callback, hid_hw_stop() will get
> > > > automatically called by hid-core.c. So we are now calling it twice,
> > > > which, in a way is not a big deal but it might be an issue in the long
> > > > run.
> > > >
> > > > Generally speaking, in the HID subsystem, that situation doesn't happen
> > > > a lot because hid_hw_start() is usually the last command of probe, and
> > > > we don't need to open the device in the driver itself.
> > > >
> > > > Here, I guess as soon as you add the i2c adapter, you might want to have
> > > > the communication channels ready, and thus you need to have it open
> > > > *before* i2c_add_adapter.
> > > >
> > > > I would suggest the following if you want to keep the devm release of
> > > > stop and close: please put a big fat warning before mcp2221_hid_remove()
> > > > explaining that this is called in devm management, *and* add a function
> > > > that would just return 0 as the .remove() callback with another big fat
> > > > warning explaining that we don't want hid-core.c to call hid_hw_stop()
> > > > because we are doing it ourself through devres.
> > > >
> > >
> > > Yeah maybe best to keep the non-devres if it isn't going to affect how the last
> > > change in this series is trying to implement with iio.
> > >
> > > I'll wait for Jonathan to chime in on this thread.
> > >
> > > > Last, in the HID subsystem, we often interleave non devres with devres
> > > > for resource allocation, given that .remove() will be called before any
> > > > devres release. But that is assuming this ordering is OK, which doesn't
> > > > seem to be the case here. We first need to unregister the i2c adapter
> > > > and then close/stop the HID device.
> >
> > On second thought I2C will be unregistered before the HID calls, since
> > unless I'm totally
> > incorrect device resource management unwinds backwards in the order actions are
> > registered.
>
> Yeah, sorry if it was not clear:
> - .remove() is called *before* any devres action takes place
> - devres action are LIFO, so unwinded backwards as you say
>
> In the general case, a driver does:
> int probe() {
> void *pointer = devm_alloc(...)
> some_more_devm_action(...)
> hid_hw_start()
> return 0;
> }
>
> and so the HID start action is the last one, meaning that .remove will
> first call stop and then devres unwind will get called.
>
> But here, in your case, you need hid_hw_start to be called *before*
> devm_i2c_add_adapter(), meaning that the implicit .remove() will mess
> up with the device, so you are forced to do something about it.
>
> You can either keep a non devm variant, or you can override the
> .remove() of HID to not do anything and do the stop/close in a
> specific devm task, which you did here. You are just missing the
> "let's override .remove() to ensure we keep the device open and
> started while we need it".
Ok, now I understand!
Thanks,
Matt
>
> Cheers,
> Benjamin
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 2/5] iio: addac: stx104: fix future recursive dependencies
2022-09-21 6:30 ` [PATCH v4 2/5] iio: addac: stx104: " Matt Ranostay
@ 2022-09-24 16:05 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-24 16:05 UTC (permalink / raw)
To: Matt Ranostay; +Cc: gupt21, linux-i2c, linux-iio, linux-input
On Tue, 20 Sep 2022 23:30:23 -0700
Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> When using 'imply IIO' for other configurations which have 'select GPIOLIB'
> the following recursive dependency is detected for STX1040
>
> Switch from 'select GPIOLIB' to 'depends on GPIOLIB' to avoid this per
> recommendation in kconfig-language.rst
>
> drivers/gpio/Kconfig:14:error: recursive dependency detected!
> drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by STX104
> drivers/iio/addac/Kconfig:20: symbol STX104 depends on IIO
> drivers/iio/Kconfig:6: symbol IIO is implied by HID_MCP2221
> drivers/hid/Kconfig:1227: symbol HID_MCP2221 depends on GPIOLIB
>
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
Given this wants to go in with the patch that causes the problem (and definitely
not after it!)- I'll assume this will go via the HID tree.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/iio/addac/Kconfig | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/iio/addac/Kconfig b/drivers/iio/addac/Kconfig
> index fcf6d2269bfc..494790816ac7 100644
> --- a/drivers/iio/addac/Kconfig
> +++ b/drivers/iio/addac/Kconfig
> @@ -19,9 +19,8 @@ config AD74413R
>
> config STX104
> tristate "Apex Embedded Systems STX104 driver"
> - depends on PC104 && X86
> + depends on PC104 && X86 && GPIOLIB
> select ISA_BUS_API
> - select GPIOLIB
> help
> Say yes here to build support for the Apex Embedded Systems STX104
> integrated analog PC/104 card.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 3/5] iio: dac: fix future recursive dependencies
2022-09-21 6:30 ` [PATCH v4 3/5] iio: dac: " Matt Ranostay
@ 2022-09-24 16:06 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-24 16:06 UTC (permalink / raw)
To: Matt Ranostay; +Cc: gupt21, linux-i2c, linux-iio, linux-input
On Tue, 20 Sep 2022 23:30:24 -0700
Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> When using 'imply IIO' for other configurations which have 'select GPIOLIB'
> the following recursive dependency is detected for AD5592R/AD5593R
>
> Switch from 'select GPIOLIB' to 'depends on GPIOLIB' to avoid this per
> recommendation in kconfig-language.rst
>
> drivers/gpio/Kconfig:14:error: recursive dependency detected!
> drivers/gpio/Kconfig:14: symbol GPIOLIB is selected by AD5592R
> drivers/iio/dac/Kconfig:93: symbol AD5592R depends on IIO
> drivers/iio/Kconfig:6: symbol IIO is implied by HID_MCP2221
> drivers/hid/Kconfig:1227: symbol HID_MCP2221 depends on GPIOLIB
>
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/iio/dac/Kconfig | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 80521bd28d0f..b93003e80b70 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -92,8 +92,7 @@ config AD5592R_BASE
>
> config AD5592R
> tristate "Analog Devices AD5592R ADC/DAC driver"
> - depends on SPI_MASTER
> - select GPIOLIB
> + depends on SPI_MASTER && GPIOLIB
> select AD5592R_BASE
> help
> Say yes here to build support for Analog Devices AD5592R
> @@ -104,8 +103,7 @@ config AD5592R
>
> config AD5593R
> tristate "Analog Devices AD5593R ADC/DAC driver"
> - depends on I2C
> - select GPIOLIB
> + depends on I2C && GPIOLIB
> select AD5592R_BASE
> help
> Say yes here to build support for Analog Devices AD5593R
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions
2022-09-23 21:22 ` Matt Ranostay
@ 2022-09-24 16:16 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-24 16:16 UTC (permalink / raw)
To: Matt Ranostay
Cc: Benjamin Tissoires, Rishi Gupta, Linux I2C, linux-iio,
open list:HID CORE LAYER, Jiri Kosina
On Fri, 23 Sep 2022 14:22:18 -0700
Matt Ranostay <matt.ranostay@konsulko.com> wrote:
> On Fri, Sep 23, 2022 at 12:03 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Fri, Sep 23, 2022 at 1:45 AM Matt Ranostay
> > <matt.ranostay@konsulko.com> wrote:
> > >
> > > On Wed, Sep 21, 2022 at 10:57 AM Matt Ranostay
> > > <matt.ranostay@konsulko.com> wrote:
> > > >
> > > > On Wed, Sep 21, 2022 at 1:05 AM Benjamin Tissoires
> > > > <benjamin.tissoires@redhat.com> wrote:
> > > > >
> > > > > [foreword: please keep Jiri and myself (the HID maintainers) CC-ed to
> > > > > the series, as you will need ack from us and we don't necessarily monitor
> > > > > every single message on linux-input]
> > > > >
> > > > > On Sep 20 2022, Matt Ranostay wrote:
> > > > > > 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 | 45 +++++++++++++++++----------------------
> > > > > > 1 file changed, 19 insertions(+), 26 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hid/hid-mcp2221.c b/drivers/hid/hid-mcp2221.c
> > > > > > index de52e9f7bb8c..7ba63bcd66de 100644
> > > > > > --- a/drivers/hid/hid-mcp2221.c
> > > > > > +++ b/drivers/hid/hid-mcp2221.c
> > > > > > @@ -824,6 +824,14 @@ static int mcp2221_raw_event(struct hid_device *hdev,
> > > > > > return 1;
> > > > > > }
> > > > > >
> > > > > > +static void mcp2221_hid_remove(void *ptr)
> > > > > > +{
> > > > > > + struct hid_device *hdev = ptr;
> > > > > > +
> > > > > > + hid_hw_close(hdev);
> > > > > > + hid_hw_stop(hdev);
> > > > >
> > > > > By default, if you remove the .remove() callback, hid_hw_stop() will get
> > > > > automatically called by hid-core.c. So we are now calling it twice,
> > > > > which, in a way is not a big deal but it might be an issue in the long
> > > > > run.
> > > > >
> > > > > Generally speaking, in the HID subsystem, that situation doesn't happen
> > > > > a lot because hid_hw_start() is usually the last command of probe, and
> > > > > we don't need to open the device in the driver itself.
> > > > >
> > > > > Here, I guess as soon as you add the i2c adapter, you might want to have
> > > > > the communication channels ready, and thus you need to have it open
> > > > > *before* i2c_add_adapter.
> > > > >
> > > > > I would suggest the following if you want to keep the devm release of
> > > > > stop and close: please put a big fat warning before mcp2221_hid_remove()
> > > > > explaining that this is called in devm management, *and* add a function
> > > > > that would just return 0 as the .remove() callback with another big fat
> > > > > warning explaining that we don't want hid-core.c to call hid_hw_stop()
> > > > > because we are doing it ourself through devres.
> > > > >
> > > >
> > > > Yeah maybe best to keep the non-devres if it isn't going to affect how the last
> > > > change in this series is trying to implement with iio.
> > > >
> > > > I'll wait for Jonathan to chime in on this thread.
Not my subsystem, so I'm happy if others have to take the headaches that
mixing and matching causes :) Personally I'd rather not!
Whilst devm_ brings it's own issues (the plumbers session on this was as
ever fun) the particular fun set of bugs that turn up because of mixing
it in probe() with manual removal in remove() was one where I've never
heard a good argument against using devm_ until the first thing in probe() where
you decide not to then not using devm_ calls after that. I have seen
a handful of cases where a different order was needed, but far more bugs
and / or difficult to reason out flows as a result of mixing them up.
Obviously straight forward allocations are fine as freeing them late doesn't
matter. Registration / consumer requests not so much.
Jonathan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem
2022-09-21 6:30 ` [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem Matt Ranostay
2022-09-22 10:23 ` kernel test robot
@ 2022-09-24 16:25 ` Jonathan Cameron
1 sibling, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2022-09-24 16:25 UTC (permalink / raw)
To: Matt Ranostay; +Cc: gupt21, linux-i2c, linux-iio, linux-input
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;
> }
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-09-24 16:25 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-21 6:30 [PATCH v4 0/5] HID: mcp2221: iio support and device resource management Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 1/5] i2c: muxes: ltc4306: fix future recursive dependencies Matt Ranostay
2022-09-21 6:30 ` [PATCH v4 2/5] iio: addac: stx104: " Matt Ranostay
2022-09-24 16:05 ` Jonathan Cameron
2022-09-21 6:30 ` [PATCH v4 3/5] iio: dac: " Matt Ranostay
2022-09-24 16:06 ` Jonathan Cameron
2022-09-21 6:30 ` [PATCH v4 4/5] HID: mcp2221: switch i2c registration to devm functions Matt Ranostay
2022-09-21 8:04 ` Benjamin Tissoires
2022-09-21 17:57 ` Matt Ranostay
2022-09-22 23:44 ` Matt Ranostay
2022-09-23 7:03 ` Benjamin Tissoires
2022-09-23 21:22 ` Matt Ranostay
2022-09-24 16:16 ` Jonathan Cameron
2022-09-21 6:30 ` [PATCH v4 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem Matt Ranostay
2022-09-22 10:23 ` kernel test robot
2022-09-24 16:25 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).