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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Krzysztof,

On Mon, Oct 11, 2021 at 08:20:32AM +0200, Krzysztof Hałasa wrote:
> Hi Jacopo,
> 
> Thanks for your input.
> 
> Jacopo Mondi <jacopo@jmondi.org> writes:
> 
> > To my understanding the C99 standard added support for the //
> > commenting style and tollerate them, but they're still from C++
> 
> Sure. Not everything coming from C++ is bad.
> 
> > and I
> > see very few places where they're used in the kernel,
> 
> It's not going to change if no one uses //.
> 
> > 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
> 
> As Randy wrote, perhaps we need to bring the coding-style up to date?
> 
> > 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.
> 
> I could. The question is "why?" IMHO the C++ style is (in places I use
> it) better than the /* */. Why should I use the worse thing?

It's also a matter of consistency, to try and unify the coding style
across similar drivers in a subsytem. In this case, the media system
frowns upon C++-style comments.

> > In my personal opinion lifting that restriction caused more pain than
> > anything, as different subsystem are now imposing different
> > requirements.
> 
> I think it was always the restriction causing more harm than good.
> It's not like the "spirit" behind it was wrong - no. The oversided lines
> SHOULD be avoided. It was the hard limit which was wrong: a) the limit
> itself (80) was definitely inadequate, and b) the hard limit should have
> never existed. 8-character tabs only made this worse (e.g. I use 4-chars
> tabs outside the kernel).
> 
> This is all about readability, right? Hard rules don't play well with
> it.
> 
> Things like:
>                                        fst_tx_dma(card,
>                                                    card->tx_dma_handle_card,
>                                                    BUF_OFFSET(txBuffer[pi]
>                                                               [port->txpos][0]),
>                                                    skb->len);
> Is this better, isn't it?
> However I do realize my opinion may be a bit distorted since I have some
> vision problems.
> 
> > 	ret = ar0521_write_regs(sensor, pixel_timing_recommended, ARRAY_SIZE(pixel_timing_recommended));
> > 	if (ret)
> > 		goto off;
> >
> >
> > should be
> >
> > 	ret = ar0521_write_regs(sensor, pixel_timing_recommended,
> >                                 ARRAY_SIZE(pixel_timing_recommended));
> > 	if (ret)
> > 		goto off;
> 
> Do you consider the second one BETTER? I definitely don't (though it
> this case the difference is small). If it's worse, why should I use it?

I find the second option much more readable, yes.

Code is written once and read often, so you should consider the coding
style in use in the subsystem.

> Also, in such cases I try to align the arguments (ARRAY_SIZE right below
> sensor). Still IMHO worse than #1.
> 
> > if you go over 100 you should ask yourself what are you doing :)
> 
> I do. Sometimes the answer is I'm doing the right thing :-)
> And sometimes I change the code. You won't see it because it's already
> changed.
> 
> > The sensor frame rate is configured by userspace by changing the
> > blankings through the V4L2_CID_[VH]BLANK.
> >
> > You are right the current definition is akward to work with, as there
> > is no way to set the 'total pixels' like you have suggested, but it's
> > rather userspace that knowing the desired total sizes has to compute
> > the blankings by subtracting the visible sizes (plus the mandatory min
> > blanking sizes).
> 
> But it can't do that, can it?
> This could be adequate for a sensor with fixed pixel clock. Here we can
> control pixel clocks at will, how is the driver going to know what pixel
> clock should it use? Also, the "extra delay" can't be set with
> V4L2_CID_[VH]BLANK, it needs interval-based timings or the "total pixel"
> or something alike.

Additional controls may be needed, I haven't studied this particular
sensor in details, but in general frame rate is controlled explicitly
through low-level parameters for raw sensors, which means controlling
h/v blank and possibly the pixel clock from userspace. The
.g_frame_interval() and .s_frame_interval() operations are deprecated
(and should never have been used) for raw sensors.

-- 
Regards,

Laurent Pinchart