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

    > 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?

    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.
    --
    Krzysztof "Chris" Hałasa

    Sieć Badawcza Łukasiewicz
    Przemysłowy Instytut Automatyki i Pomiarów PIAP
    Al. Jerozolimskie 202, 02-486 Warszawa

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