Re: [PATCH v6 2/2] Driver for ON Semi AR0521 camera sensor

["Krzysztof Hałasa" <khalasa@piap.pl>]

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