On Tue, Mar 24, 2020 at 1:52 PM Ivan Mikhaylov <i.mikhaylov@xxxxxxxxx> 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@xxxxxxxxx> 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
