lkml.org 
[lkml]   [2021]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5] Driver for ON Semi AR0521 camera sensor
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

\
 
 \ /
  Last update: 2021-10-11 08:32    [W:2.202 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site