diff mbox series

[v3,5/5] HID: mcp2221: add ADC/DAC support via iio subsystem

Message ID 20220912173202.16723-6-matt.ranostay@konsulko.com
State Superseded
Headers show
Series HID: mcp2221: iio support and device resource management | expand

Commit Message

Matt Ranostay Sept. 12, 2022, 5:32 p.m. UTC 
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 | 187 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+)

Comments

Jonathan Cameron Sept. 18, 2022, 3:49 p.m. UTC | #1 
On Mon, 12 Sep 2022 10:32:02 -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>

Hi Matt,

Can we not provide a _scale?
Whilst not technically required in all IIO Drivers, a bare _raw interface
is rarely that much use and here the ADC and DAC clearly have very different
scales.

Otherwise, as seen below, I'd like a comment on why the registration
is kicked off in a delayed work item. Right now that looks like a hack
to ensure something else has happened first.  That's fine, but there
doesn't seem to be rescheduling if whatever that 'thing' is hasn't happened yet.
To use this sort of delayed trick, I'd definitely expect a backoff again
to be implemented...

> ---
>  drivers/hid/Kconfig       |   1 +
>  drivers/hid/hid-mcp2221.c | 187 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 188 insertions(+)


...

>  static int mcp2221_probe(struct hid_device *hdev,
>  					const struct hid_device_id *id)
>  {
> @@ -902,6 +1084,11 @@ static int mcp2221_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_i2c;
>  
> +#if IS_REACHABLE(CONFIG_IIO)
> +	INIT_DELAYED_WORK(&mcp->init_work, mcp_init_work);
> +	schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(500));

Good to have a comment here to say why you are kicking the registration of the
IIO device onto a delayed work path.

> +#endif
> +
>  	return 0;
>  
>  err_i2c:
Matt Ranostay Sept. 18, 2022, 8:12 p.m. UTC | #2 
On Sun, Sep 18, 2022 at 8:49 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 12 Sep 2022 10:32:02 -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>
>
> Hi Matt,
>
> Can we not provide a _scale?
> Whilst not technically required in all IIO Drivers, a bare _raw interface
> is rarely that much use and here the ADC and DAC clearly have very different
> scales.

I will check into that. The sampling voltage range is configurable
though and will need
to see

>
> Otherwise, as seen below, I'd like a comment on why the registration
> is kicked off in a delayed work item. Right now that looks like a hack
> to ensure something else has happened first.  That's fine, but there
> doesn't seem to be rescheduling if whatever that 'thing' is hasn't happened yet.
> To use this sort of delayed trick, I'd definitely expect a backoff again
> to be implemented...

Ok a retry here maybe would make sense to be sure the SRAM
configuration is read.
This hack is just because we have to be sure the MCP2221 is up and
running before
attempting to read the SRAM via a USB message, and is less ugly/wrong to pop a
msleep in a probe function.

Also in this case we can back down the half second delay since that is
the worst case.

- Matt

>
> > ---
> >  drivers/hid/Kconfig       |   1 +
> >  drivers/hid/hid-mcp2221.c | 187 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 188 insertions(+)
>
>
> ...
>
> >  static int mcp2221_probe(struct hid_device *hdev,
> >                                       const struct hid_device_id *id)
> >  {
> > @@ -902,6 +1084,11 @@ static int mcp2221_probe(struct hid_device *hdev,
> >       if (ret)
> >               goto err_i2c;
> >
> > +#if IS_REACHABLE(CONFIG_IIO)
> > +     INIT_DELAYED_WORK(&mcp->init_work, mcp_init_work);
> > +     schedule_delayed_work(&mcp->init_work, msecs_to_jiffies(500));
>
> Good to have a comment here to say why you are kicking the registration of the
> IIO device onto a delayed work path.
>
> > +#endif
> > +
> >       return 0;
> >
> >  err_i2c:
>
diff mbox series

Patch

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 29e69576c3d4..923b41eb76b3 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,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 +92,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 +101,16 @@  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 dac_value;
+#endif
+};
+
+struct mcp2221_iio {
+	struct mcp2221 *mcp;
 };
 
 /*
@@ -745,6 +759,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 +833,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 +867,145 @@  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;
+		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->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);
+	mutex_unlock(&mcp->lock);
+	hid_hw_power(mcp->hdev, PM_HINT_NORMAL);
+
+	if (ret)
+		return;
+
+	num_channels = mcp_iio_channels(mcp);
+	if (!num_channels)
+		return;
+
+	indio_dev = devm_iio_device_alloc(&mcp->hdev->dev, sizeof(*data));
+	if (!indio_dev)
+		return;
+
+	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);
+}
+#endif
+
 static int mcp2221_probe(struct hid_device *hdev,
 					const struct hid_device_id *id)
 {
@@ -902,6 +1084,11 @@  static int mcp2221_probe(struct hid_device *hdev,
 	if (ret)
 		goto err_i2c;
 
+#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_i2c: