On 10/9/21 2:07 AM, Jacopo Mondi wrote:
Hi Krzysztof,
I've been testing this driver in the last few days, thanks for your
effort in upstreaming it!
I'll separately comment on what I had to change to have it working for
my use case, but let me continue the discussion from where it was left
pending here to add my 2 cents.
On Thu, Oct 07, 2021 at 11:11:09AM +0200, Krzysztof Hałasa wrote:
Hi Sakari,
Thanks for your input.
Where's the corresponding DT binding patch? Ideally it would be part of the
same set.
Well I've sent it a moment before this one. Will make them a set next
time.
+#define AR0521_WIDTH_BLANKING_MIN 572u
+#define AR0521_HEIGHT_BLANKING_MIN 28u // must be even
Please use /* */ for comments. The SPDX tag is an exception.
As far as I know, this is no longer the case, the C99 comments are now
permitted and maybe even encouraged. Or was I dreaming?
checkpatch doesn't protest either.
To my understanding the C99 standard added support for the //
commenting style and tollerate them, but they're still from C++ and I
see very few places where they're used in the kernel, and per as far I
know they're still not allowed by the coding style
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html
Maybe we should update coding-style then.
Looking at how you used comments in the driver I think you could get
rid of most // comments easily, the register tables might be an
exception but I would really try to remove them from there as well.
Please wrap your lines at 80 or earlier, unless a sound reason exists to do
otherwise.
This limitation appears to be lifted as well, after all those years.
Is there a specific reason to still use it here? Yes, lines longer than
80 chars make the code much more readable (for my eyes, at least).
Yes, I know there is some "soft" limit, and I trim lines when it makes
them better in my opinion.
In my personal opinion lifting that restriction caused more pain than
anything, as different subsystem are now imposing different
requirements. Here everything has been so far pretty strict about
going over 80-cols, but I think there are situation where it makes
sense in example
[snip]
My suggestion is: aim to 80 cols whenever possible, if it forces you
to do things like the above shown function declaration you can go a
little over that
Yes, 80 max is still preferred. Up to 100 may be tolerable in some
cases.
As reported here
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
if you go over 100 you should ask yourself what are you doing :)
--
~Randy
