linux-kernel - Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <m3wnjnfqlx.fsf@t19.piap.pl>
Date:   Wed, 29 Dec 2021 15:11:22 +0100
From:   Krzysztof Hałasa <khalasa@...p.pl>
To:     Joe Perches <joe@...ches.com>
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>,
        Jacopo Mondi <jacopo@...ndi.org>
Subject: Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

Hello Joe,

Joe Perches <joe@...ches.com> writes:

>> +/* External clock (extclk) frequencies */
>> +#define AR0521_EXTCLK_MIN	  (10 * 1000 * 1000)
>
> Generally, adding a prefix like AR0521_ to defines that are
> locally defined in a single file unnecessarily increases
> identifier length.

Right. In general, I don't do that (for that very reason), however in
drivers/media this looks like a common practice and I didn't want to
break it.

> e.g. Using this identifier anywhere
>
>> +#define AR0521_REG_HISPI_CONTROL_STATUS_FRAMER_TEST_MODE_ENABLE 0x80

Right. However, such a name helps looking this up in the docs.
E.g. the register name in the docs is "hispi_control_status" and the
bitfield is "framer_test_mode" or something like that.
Since it's just one register (+ value) and it actually fits in 80
columns without too much problems, I'd rather like to leave it
unchanged.

> Many of the 80 column line lengths and line wrapping used in this
> file are not really nice to read.  I believe you don't have to be
> strict about 80 column lines.

Well, personally I think we could all switch to VT100's 132 columns.
Introduced in '78 :-) That's what I currently use for non-kernel tasks
(not the VT100 but just the line length). OTOH I'm using that emacs
wrapping mode so longer lines aren't a problem either.
But here, in drivers/media, I'm told 80 column is strict.

>> +#define be		cpu_to_be16
>
> It's a pity there's no way to declare an array with all members
> having a specific endianness.  Making sure all elements in these
> arrays are declared with be() is tedious.

Right. Unfortunately anything else would mean recoding.

>> +#define AR0521_NUM_SUPPLIES ARRAY_SIZE(ar0521_supply_names)
>
> It's almost always better to use ARRAY_SIZE directly and not
> use a #define for the array size.

It's another custom in drivers/media, but I guess I don't have to follow
it closely, do I? I never liked the #define.

>> +static int ar0521_set_gains(struct ar0521_dev *sensor)
>> +{
> []
>> +	dev_dbg(&sensor->i2c_client->dev, "%s()\n", __func__);
>
> ftrace works and perhaps all the similar debug logging uses aren't
> really necessary.

TBH I've never used ftrace.
It appears that it can't show the arguments, can it?
If not, I'd rather leave these dev_dbg()s in place - like other
drivers/media/* in fact.
However obviously the code without deb_dbg()s would be cleaner, so if
ftrace can show the (formatted) arguments, I'm all for it.

Thanks for looking at this,
-- 
Krzysztof "Chris" Hałasa

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