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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

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

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