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

From: Matt Ranostay <matt.ranostay@konsulko.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: gupt21@gmail.com, linux-iio@vger.kernel.org,
	linux-input@vger.kernel.org, linux-i2c@vger.kernel.org
Subject: Re: [PATCH v3 5/5] HID: mcp2221: add ADC/DAC support via iio subsystem
Date: Sun, 18 Sep 2022 13:12:08 -0700	[thread overview]
Message-ID: <CAJCx=g=JPRTm-Qjy233BfOfzFiSVonM5GbS2BrRrzp8aNZA4MQ@mail.gmail.com> (raw)
In-Reply-To: <20220918164920.5bc2bc87@jic23-huawei>

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