linux-kernel - Re: [PATCH v8 2/2] On Semi AR0521 sensor driver

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220301143044.2l4vlwbnh5n3g5ng@uno.localdomain>
Date:   Tue, 1 Mar 2022 15:30:44 +0100
From:   Jacopo Mondi <jacopo@...ndi.org>
To:     Krzysztof Hałasa <khalasa@...p.pl>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v8 2/2] On Semi AR0521 sensor driver

Hi Krzysztof

On Tue, Mar 01, 2022 at 01:34:28PM +0100, Krzysztof Hałasa wrote:
> Jacopo Mondi <jacopo@...ndi.org> writes:
>
> > In subject
> >
> > "media: i2c:"
> >
> > Same for 1/2 where permutation of "media: dt-bindings: i2c:" are used
> > when adding bindings for media i2c drivers.
>
> You know, it's rather hard to know all these tiny requirements. Let
> alone get them right all the time.
>

I usually rely on what git log suggests me if in doubt.

> >> +	pm_runtime_set_active(&client->dev);
> >> +	pm_runtime_enable(&client->dev);
> >> +	pm_runtime_idle(&client->dev);
> >
> > Do you have an _idle() callback ? This seems a no-op to me, or am I
> > mistaken ? (runtime_pm is still cryptic to me sometimes)
>
> Do you mean only the pm_runtime_idle()? Sakari just requested I add it.
>

Ok then

> > Can't you just remove power_on() if it's not needed ?
>
> It wouldn't work without CONFIG_PM then, would it?
>

True that. And an #ifdef is maybe undesirable.

Although, if with idle you can power-down the chip after probe, I'm
fine with that

> > There still are a few checkpatch warnings which might be worth
> > considering  but nothing huge.
>
> Do you mean this?
>
> WARNING: Block comments use * on subsequent lines
> #411: FILE: drivers/media/i2c/ar0521.c:351:
> +               /* Reset gain, the sensor may produce all white pixels without
> +                  this */
>
> WARNING: Block comments use a trailing */ on a separate line
> #411: FILE: drivers/media/i2c/ar0521.c:351:
> +                  this */
>
> ... which are about a single comment which I had to wrap to fit in 80
> columns, hardly a block comment by my standards. Not to mention Linus
> saying...

The warning suggests to go for

        /*
         * This when a comment
         * spans on multiple lines
         */

instead of

        /* Going for this style when
           a comment is on multiple lines */


> --
> Krzysztof "Chris" Hałasa
>
> Sieć Badawcza Łukasiewicz
> Przemysłowy Instytut Automatyki i Pomiarów PIAP
> Al. Jerozolimskie 202, 02-486 Warszawa