Re: [PATCH] HID: mcp2221: add ADC support

[Jonathan Cameron <jic23@kernel.org>]

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