Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

[Randy Dunlap]

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