Re: [PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Ivan Mikhaylov <i.mikhaylov@yadro.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	linux-iio <linux-iio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Rob Herring <robh+dt@kernel.org>
Subject: Re: [PATCH 2/2] iio: proximity: Add driver support for vcnl3020 proximity sensor
Date: Tue, 24 Mar 2020 14:03:04 +0200	[thread overview]
Message-ID: <CAHp75VeHRxgKFfiqO3V+=jMB-Pqe9-1YM+0oFuuo=cjJfJa+bA@mail.gmail.com> (raw)
In-Reply-To: <321453af40ca49839bc7b9d1c65b828841492f72.camel@yadro.com>

On Tue, Mar 24, 2020 at 1:52 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:
> On Mon, 2020-03-23 at 14:10 +0200, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2020 at 12:41 PM Ivan Mikhaylov <i.mikhaylov@yadro.com> wrote:

...

> > > +static int32_t vcnl3020_init(struct vcnl3020_data *data)
> >
> > int32_t...
> >
> > > +{
> > > +       s32 rc;
> >
> > ...s32?!
> >
> > Applies to entire code.
>
> checkpatch.pl --strict doesn't show anything bad in it but I can change from
> int32_t/s32 into int easily, it's not a problem for me.

The problem is consistency. You code should be self-consistent.
And yes, s32 is quite unusual for (generic) returning variables.

...

> > > +       dev_info(&client->dev, "Proximity sensor, Rev: %02x\n",
> > > +                data->rev);
> >
> > Noise.
>
> Doesn't it help to determine the presence of driver to a common user?

You have a lot of knobs to detect this. Starting from sysfs to 'initcall_debug'.

...

> > > +               goto out;
> > > +
> > > +       return rc;
> > > +out:
> > > +       devm_iio_device_free(&client->dev, indio_dev);
> > > +       return rc;
> >
> > Managed resources are exactly for this not to be appeared in the code.
>
> I can do something like this:
> return devm_iio_device_register(&client->dev, indio_dev);
>
> Would it suffice?

I think so, but you may double check with documentation.

-- 
With Best Regards,
Andy Shevchenko