lkml.org 
[lkml]   [2021]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor
Date
Hello Joe,

Joe Perches <joe@perches.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

\
 
 \ /
  Last update: 2021-12-29 15:12    [W:0.179 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site