Hi Jacopo, Thanks for your input. Jacopo Mondi <jacopo@xxxxxxxxxx> 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
