| Message ID | 20201120030103.36138-1-matt.ranostay@konsulko.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: mcp2221: add ADC support | expand |
On Fri, Nov 20, 2020 at 8:31 AM Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > Add support for the three 10-bit ADC channels registered via > the IIO subsystem. > > Cc: linux-input@vger.kernel.org > Cc: linux-iio@vger.kernel.org > CC: Rishi Gupta <gupt21@gmail.com> > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > drivers/hid/Kconfig | 1 + > drivers/hid/hid-mcp2221.c | 65 ++++++++++++++++++++++++++++++++++++++- > 2 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > index 05315b434276..4795744d9979 100644 > --- a/drivers/hid/Kconfig > +++ b/drivers/hid/Kconfig > @@ -1157,6 +1157,7 @@ config HID_MCP2221 > tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" > depends on USB_HID && I2C > depends on GPIOLIB > + depends on IIO I am wondering what will happen on systems which do not enable IIO. This driver can not be used there. Is my understanding correct? > 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 0d27ccb55dd9..7e62f1dc54d3 100644 > --- a/drivers/hid/hid-mcp2221.c > +++ b/drivers/hid/hid-mcp2221.c > @@ -18,6 +18,9 @@ > #include <linux/gpio/driver.h> > #include "hid-ids.h" > > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > + > /* Commands codes in a raw output report */ > enum { > MCP2221_I2C_WR_DATA = 0x90, > @@ -56,6 +59,7 @@ enum { > */ > struct mcp2221 { > struct hid_device *hdev; > + struct iio_dev *indio_dev; > struct i2c_adapter adapter; > struct mutex lock; > struct completion wait_in_report; > @@ -67,6 +71,11 @@ struct mcp2221 { > struct gpio_chip *gc; > u8 gp_idx; > u8 gpio_dir; > + u16 adc_values[3]; > +}; > + > +struct mcp2221_iio { > + struct mcp2221 *mcp; > }; > > /* > @@ -712,6 +721,7 @@ static int mcp2221_raw_event(struct hid_device *hdev, > break; > } > mcp->status = mcp_get_i2c_eng_state(mcp, data, 8); > + memcpy(&mcp->adc_values, &data[50], 6); > break; > default: > mcp->status = -EIO; > @@ -791,11 +801,54 @@ static int mcp2221_raw_event(struct hid_device *hdev, > return 1; > } > > +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); > + > + /* Read ADC values */ > + ret = mcp_chk_last_cmd_status(mcp); > + if (ret < 0) > + return ret; > + > + *val = le16_to_cpu(mcp->adc_values[channel->channel]); > + > + mutex_unlock(&mcp->lock); > + > + return IIO_VAL_INT; > +} > + > +static const struct iio_info mcp2221_info = { > + .read_raw = &mcp2221_read_raw, > +}; > + > +#define MCP2221_ADC_CHANNEL(idx) \ > + { \ > + .type = IIO_VOLTAGE, \ > + .indexed = 1, \ > + .channel = idx, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .scan_index = -1, \ > + } > + > +static const struct iio_chan_spec mcp2221_channels[] = { > + MCP2221_ADC_CHANNEL(0), > + MCP2221_ADC_CHANNEL(1), > + MCP2221_ADC_CHANNEL(2), > +}; > + > static int mcp2221_probe(struct hid_device *hdev, > const struct hid_device_id *id) > { > int ret; > struct mcp2221 *mcp; > + struct mcp2221_iio *iio; > > mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL); > if (!mcp) > @@ -869,8 +922,17 @@ static int mcp2221_probe(struct hid_device *hdev, > if (ret) > goto err_gc; > > - return 0; > + mcp->indio_dev = devm_iio_device_alloc(&hdev->dev, sizeof(*iio)); > + iio = iio_priv(mcp->indio_dev); > + iio->mcp = mcp; > + > + mcp->indio_dev->name = "mcp2221_adc"; > + mcp->indio_dev->modes = INDIO_DIRECT_MODE; > + mcp->indio_dev->info = &mcp2221_info; > + mcp->indio_dev->channels = mcp2221_channels; > + mcp->indio_dev->num_channels = ARRAY_SIZE(mcp2221_channels); > > + return iio_device_register(mcp->indio_dev); > err_gc: > i2c_del_adapter(&mcp->adapter); > err_i2c: > @@ -884,6 +946,7 @@ static void mcp2221_remove(struct hid_device *hdev) > { > struct mcp2221 *mcp = hid_get_drvdata(hdev); > > + iio_device_unregister(mcp->indio_dev); > i2c_del_adapter(&mcp->adapter); > hid_hw_close(mcp->hdev); > hid_hw_stop(mcp->hdev); > -- > 2.20.1 >
On Fri, Nov 20, 2020 at 5:15 AM rishi gupta <gupt21@gmail.com> wrote: > > On Fri, Nov 20, 2020 at 8:31 AM Matt Ranostay > <matt.ranostay@konsulko.com> wrote: > > > > Add support for the three 10-bit ADC channels registered via > > the IIO subsystem. > > > > Cc: linux-input@vger.kernel.org > > Cc: linux-iio@vger.kernel.org > > CC: Rishi Gupta <gupt21@gmail.com> > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > > --- > > drivers/hid/Kconfig | 1 + > > drivers/hid/hid-mcp2221.c | 65 ++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 65 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > index 05315b434276..4795744d9979 100644 > > --- a/drivers/hid/Kconfig > > +++ b/drivers/hid/Kconfig > > @@ -1157,6 +1157,7 @@ config HID_MCP2221 > > tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" > > depends on USB_HID && I2C > > depends on GPIOLIB > > + depends on IIO > I am wondering what will happen on systems which do not enable IIO. > This driver can not be used there. > Is my understanding correct? Actually yeah this should be "select IIO" to avoid that issue. - Matt > > 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 0d27ccb55dd9..7e62f1dc54d3 100644 > > --- a/drivers/hid/hid-mcp2221.c > > +++ b/drivers/hid/hid-mcp2221.c > > @@ -18,6 +18,9 @@ > > #include <linux/gpio/driver.h> > > #include "hid-ids.h" > > > > +#include <linux/iio/iio.h> > > +#include <linux/iio/sysfs.h> > > + > > /* Commands codes in a raw output report */ > > enum { > > MCP2221_I2C_WR_DATA = 0x90, > > @@ -56,6 +59,7 @@ enum { > > */ > > struct mcp2221 { > > struct hid_device *hdev; > > + struct iio_dev *indio_dev; > > struct i2c_adapter adapter; > > struct mutex lock; > > struct completion wait_in_report; > > @@ -67,6 +71,11 @@ struct mcp2221 { > > struct gpio_chip *gc; > > u8 gp_idx; > > u8 gpio_dir; > > + u16 adc_values[3]; > > +}; > > + > > +struct mcp2221_iio { > > + struct mcp2221 *mcp; > > }; > > > > /* > > @@ -712,6 +721,7 @@ static int mcp2221_raw_event(struct hid_device *hdev, > > break; > > } > > mcp->status = mcp_get_i2c_eng_state(mcp, data, 8); > > + memcpy(&mcp->adc_values, &data[50], 6); > > break; > > default: > > mcp->status = -EIO; > > @@ -791,11 +801,54 @@ static int mcp2221_raw_event(struct hid_device *hdev, > > return 1; > > } > > > > +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); > > + > > + /* Read ADC values */ > > + ret = mcp_chk_last_cmd_status(mcp); > > + if (ret < 0) > > + return ret; > > + > > + *val = le16_to_cpu(mcp->adc_values[channel->channel]); > > + > > + mutex_unlock(&mcp->lock); > > + > > + return IIO_VAL_INT; > > +} > > + > > +static const struct iio_info mcp2221_info = { > > + .read_raw = &mcp2221_read_raw, > > +}; > > + > > +#define MCP2221_ADC_CHANNEL(idx) \ > > + { \ > > + .type = IIO_VOLTAGE, \ > > + .indexed = 1, \ > > + .channel = idx, \ > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > > + .scan_index = -1, \ > > + } > > + > > +static const struct iio_chan_spec mcp2221_channels[] = { > > + MCP2221_ADC_CHANNEL(0), > > + MCP2221_ADC_CHANNEL(1), > > + MCP2221_ADC_CHANNEL(2), > > +}; > > + > > static int mcp2221_probe(struct hid_device *hdev, > > const struct hid_device_id *id) > > { > > int ret; > > struct mcp2221 *mcp; > > + struct mcp2221_iio *iio; > > > > mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL); > > if (!mcp) > > @@ -869,8 +922,17 @@ static int mcp2221_probe(struct hid_device *hdev, > > if (ret) > > goto err_gc; > > > > - return 0; > > + mcp->indio_dev = devm_iio_device_alloc(&hdev->dev, sizeof(*iio)); > > + iio = iio_priv(mcp->indio_dev); > > + iio->mcp = mcp; > > + > > + mcp->indio_dev->name = "mcp2221_adc"; > > + mcp->indio_dev->modes = INDIO_DIRECT_MODE; > > + mcp->indio_dev->info = &mcp2221_info; > > + mcp->indio_dev->channels = mcp2221_channels; > > + mcp->indio_dev->num_channels = ARRAY_SIZE(mcp2221_channels); > > > > + return iio_device_register(mcp->indio_dev); > > err_gc: > > i2c_del_adapter(&mcp->adapter); > > err_i2c: > > @@ -884,6 +946,7 @@ static void mcp2221_remove(struct hid_device *hdev) > > { > > struct mcp2221 *mcp = hid_get_drvdata(hdev); > > > > + iio_device_unregister(mcp->indio_dev); > > i2c_del_adapter(&mcp->adapter); > > hid_hw_close(mcp->hdev); > > hid_hw_stop(mcp->hdev); > > -- > > 2.20.1 > >
On 11/20/20 8:17 PM, Matt Ranostay wrote: > On Fri, Nov 20, 2020 at 5:15 AM rishi gupta <gupt21@gmail.com> wrote: >> On Fri, Nov 20, 2020 at 8:31 AM Matt Ranostay >> <matt.ranostay@konsulko.com> wrote: >>> Add support for the three 10-bit ADC channels registered via >>> the IIO subsystem. >>> >>> Cc: linux-input@vger.kernel.org >>> Cc: linux-iio@vger.kernel.org >>> CC: Rishi Gupta <gupt21@gmail.com> >>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >>> --- >>> drivers/hid/Kconfig | 1 + >>> drivers/hid/hid-mcp2221.c | 65 ++++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 65 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig >>> index 05315b434276..4795744d9979 100644 >>> --- a/drivers/hid/Kconfig >>> +++ b/drivers/hid/Kconfig >>> @@ -1157,6 +1157,7 @@ config HID_MCP2221 >>> tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" >>> depends on USB_HID && I2C >>> depends on GPIOLIB >>> + depends on IIO >> I am wondering what will happen on systems which do not enable IIO. >> This driver can not be used there. >> Is my understanding correct? > Actually yeah this should be "select IIO" to avoid that issue. No, we should not have a individual driver select a framework. This will cause all kinds of issues with reverse dependencies. It might be worth splitting this driver into a MFD driver, then the MFD cells could have their own module that depend on the subsystem and if not enabled the functionality will not be provided. - Lars
On Fri, Nov 20, 2020 at 11:54 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > > On 11/20/20 8:17 PM, Matt Ranostay wrote: > > On Fri, Nov 20, 2020 at 5:15 AM rishi gupta <gupt21@gmail.com> wrote: > >> On Fri, Nov 20, 2020 at 8:31 AM Matt Ranostay > >> <matt.ranostay@konsulko.com> wrote: > >>> Add support for the three 10-bit ADC channels registered via > >>> the IIO subsystem. > >>> > >>> Cc: linux-input@vger.kernel.org > >>> Cc: linux-iio@vger.kernel.org > >>> CC: Rishi Gupta <gupt21@gmail.com> > >>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > >>> --- > >>> drivers/hid/Kconfig | 1 + > >>> drivers/hid/hid-mcp2221.c | 65 ++++++++++++++++++++++++++++++++++++++- > >>> 2 files changed, 65 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > >>> index 05315b434276..4795744d9979 100644 > >>> --- a/drivers/hid/Kconfig > >>> +++ b/drivers/hid/Kconfig > >>> @@ -1157,6 +1157,7 @@ config HID_MCP2221 > >>> tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" > >>> depends on USB_HID && I2C > >>> depends on GPIOLIB > >>> + depends on IIO > >> I am wondering what will happen on systems which do not enable IIO. > >> This driver can not be used there. > >> Is my understanding correct? > > Actually yeah this should be "select IIO" to avoid that issue. > > No, we should not have a individual driver select a framework. This will > cause all kinds of issues with reverse dependencies. > > It might be worth splitting this driver into a MFD driver, then the MFD > cells could have their own module that depend on the subsystem and if > not enabled the functionality will not be provided. Would it make sense to use IS_REACHABLE(CONFIG_IIO) for the iio blocks? Guessing the weak reference "imply IIO" would still be bad for the driver selecting a framework? Thanks, Matt > > - Lars >
On Fri, 20 Nov 2020 12:31:23 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > On Fri, Nov 20, 2020 at 11:54 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > > > > On 11/20/20 8:17 PM, Matt Ranostay wrote: > > > On Fri, Nov 20, 2020 at 5:15 AM rishi gupta <gupt21@gmail.com> wrote: > > >> On Fri, Nov 20, 2020 at 8:31 AM Matt Ranostay > > >> <matt.ranostay@konsulko.com> wrote: > > >>> Add support for the three 10-bit ADC channels registered via > > >>> the IIO subsystem. > > >>> > > >>> Cc: linux-input@vger.kernel.org > > >>> Cc: linux-iio@vger.kernel.org > > >>> CC: Rishi Gupta <gupt21@gmail.com> > > >>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > > >>> --- > > >>> drivers/hid/Kconfig | 1 + > > >>> drivers/hid/hid-mcp2221.c | 65 ++++++++++++++++++++++++++++++++++++++- > > >>> 2 files changed, 65 insertions(+), 1 deletion(-) > > >>> > > >>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig > > >>> index 05315b434276..4795744d9979 100644 > > >>> --- a/drivers/hid/Kconfig > > >>> +++ b/drivers/hid/Kconfig > > >>> @@ -1157,6 +1157,7 @@ config HID_MCP2221 > > >>> tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" > > >>> depends on USB_HID && I2C > > >>> depends on GPIOLIB > > >>> + depends on IIO > > >> I am wondering what will happen on systems which do not enable IIO. > > >> This driver can not be used there. > > >> Is my understanding correct? > > > Actually yeah this should be "select IIO" to avoid that issue. > > > > No, we should not have a individual driver select a framework. This will > > cause all kinds of issues with reverse dependencies. > > > > It might be worth splitting this driver into a MFD driver, then the MFD > > cells could have their own module that depend on the subsystem and if > > not enabled the functionality will not be provided. > > Would it make sense to use IS_REACHABLE(CONFIG_IIO) for the iio blocks? > > Guessing the weak reference "imply IIO" would still be bad for the > driver selecting a framework? A lesser option than going full MFD for this (which is probably the right design decision but is a big change) would be to just put the IIO stuff in a separate C file and use some build time magic. I agree with Lars though that this is probably better done as an MFD. It supports a bunch of things in entirely different subsystems afterall. Jonathan > > Thanks, > > Matt > > > > > - Lars > >
diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig index 05315b434276..4795744d9979 100644 --- a/drivers/hid/Kconfig +++ b/drivers/hid/Kconfig @@ -1157,6 +1157,7 @@ config HID_MCP2221 tristate "Microchip MCP2221 HID USB-to-I2C/SMbus host support" depends on USB_HID && I2C depends on GPIOLIB + depends on 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 0d27ccb55dd9..7e62f1dc54d3 100644 --- a/drivers/hid/hid-mcp2221.c +++ b/drivers/hid/hid-mcp2221.c @@ -18,6 +18,9 @@ #include <linux/gpio/driver.h> #include "hid-ids.h" +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + /* Commands codes in a raw output report */ enum { MCP2221_I2C_WR_DATA = 0x90, @@ -56,6 +59,7 @@ enum { */ struct mcp2221 { struct hid_device *hdev; + struct iio_dev *indio_dev; struct i2c_adapter adapter; struct mutex lock; struct completion wait_in_report; @@ -67,6 +71,11 @@ struct mcp2221 { struct gpio_chip *gc; u8 gp_idx; u8 gpio_dir; + u16 adc_values[3]; +}; + +struct mcp2221_iio { + struct mcp2221 *mcp; }; /* @@ -712,6 +721,7 @@ static int mcp2221_raw_event(struct hid_device *hdev, break; } mcp->status = mcp_get_i2c_eng_state(mcp, data, 8); + memcpy(&mcp->adc_values, &data[50], 6); break; default: mcp->status = -EIO; @@ -791,11 +801,54 @@ static int mcp2221_raw_event(struct hid_device *hdev, return 1; } +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); + + /* Read ADC values */ + ret = mcp_chk_last_cmd_status(mcp); + if (ret < 0) + return ret; + + *val = le16_to_cpu(mcp->adc_values[channel->channel]); + + mutex_unlock(&mcp->lock); + + return IIO_VAL_INT; +} + +static const struct iio_info mcp2221_info = { + .read_raw = &mcp2221_read_raw, +}; + +#define MCP2221_ADC_CHANNEL(idx) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = idx, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .scan_index = -1, \ + } + +static const struct iio_chan_spec mcp2221_channels[] = { + MCP2221_ADC_CHANNEL(0), + MCP2221_ADC_CHANNEL(1), + MCP2221_ADC_CHANNEL(2), +}; + static int mcp2221_probe(struct hid_device *hdev, const struct hid_device_id *id) { int ret; struct mcp2221 *mcp; + struct mcp2221_iio *iio; mcp = devm_kzalloc(&hdev->dev, sizeof(*mcp), GFP_KERNEL); if (!mcp) @@ -869,8 +922,17 @@ static int mcp2221_probe(struct hid_device *hdev, if (ret) goto err_gc; - return 0; + mcp->indio_dev = devm_iio_device_alloc(&hdev->dev, sizeof(*iio)); + iio = iio_priv(mcp->indio_dev); + iio->mcp = mcp; + + mcp->indio_dev->name = "mcp2221_adc"; + mcp->indio_dev->modes = INDIO_DIRECT_MODE; + mcp->indio_dev->info = &mcp2221_info; + mcp->indio_dev->channels = mcp2221_channels; + mcp->indio_dev->num_channels = ARRAY_SIZE(mcp2221_channels); + return iio_device_register(mcp->indio_dev); err_gc: i2c_del_adapter(&mcp->adapter); err_i2c: @@ -884,6 +946,7 @@ static void mcp2221_remove(struct hid_device *hdev) { struct mcp2221 *mcp = hid_get_drvdata(hdev); + iio_device_unregister(mcp->indio_dev); i2c_del_adapter(&mcp->adapter); hid_hw_close(mcp->hdev); hid_hw_stop(mcp->hdev);
Add support for the three 10-bit ADC channels registered via the IIO subsystem. Cc: linux-input@vger.kernel.org Cc: linux-iio@vger.kernel.org CC: Rishi Gupta <gupt21@gmail.com> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> --- drivers/hid/Kconfig | 1 + drivers/hid/hid-mcp2221.c | 65 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 1 deletion(-)