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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sat, Aug 6, 2022 at 6:18 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Mon, 1 Aug 2022 11:08:39 +0200
> Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Mon, Aug 1, 2022 at 6:19 AM Matt Ranostay <matt.ranostay@xxxxxxxxxxxx> wrote:
> > > On Mon, Aug 1, 2022 at 3:11 AM Andy Shevchenko
> > > <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Fri, Jul 29, 2022 at 5:49 PM Matt Ranostay
> > > > <matt.ranostay@xxxxxxxxxxxx> wrote:

...

> > > > > +       if (mcp->indio_dev)
> > > >
> > > > Do you need this check?
> > >
> > > Yes basically if no ADC or DAC channel is enabled then no iio_device
> > > get allocated or registered.
> >
> > > > > +               iio_device_unregister(mcp->indio_dev);
> >
> > So, we have an inconvenience in the iio_device_unregister(), i.e. it
> > doesn't perform the NULL-check by itself. I recommend fixing it there
> > and dropping this check in the caller. This is standard practice in
> > the Linux kernel for resource deallocator APIs.
>
> ah. Now the other patch makes more sense.   Make sure to pull this
> driver an that one together in a series if we are taking that forwards.
>
> I agree for allocator APIs but not so much for registration APIs.
> I checked a few similar ones.
>
> input_unregister_device() doesn't
> hwmon_device_unregister() doesn't.
>
> Got bored at that point :)

Good point. I think that check is simply not needed then. We should
never come to _unregister() with a NULL pointer, right?

> Would be relatively easy to take this driver fully devm_ I think
> then you can just use devm_iio_device_register() if you have a device
> to register.

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux