| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |
| Who | What | When |
|---|---|---|
| mchehab | hi all | [11:55] |
| hverkuil | hi! | [11:55] |
| mchehab | syoung: with regards to lirc.h, I'm considering to add it to Documentation, in order to document the enum and an structure that it is declared there
this would likely solve better the reported docs issue on it this works better if we re-license the RC files to GPL 2.0 or GFDL. Would that be ok for you? (as far as I checked, the copyrights/contributions for the contents of the books were written by either you or me, so relicensing should be fine) there were just a couple of patches from other developers, basically fixing typos or renaming vars (with IMO doesn't change the copyrights)... | [11:56] |
| syoung | hi | [11:59] |
| mchehab | and a patch adding sys_class_rc_rcN_wakeup_protocols documentation, which is too small to alter the copyright owners | [12:00] |
| syoung | I don't mind re-licensing to GPL-2.0 | [12:00] |
| mchehab | ok. I'll send then a patch re-licensing as:
-.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later +.. SPDX-License-Identifier: GPL-2.0 OR GFDL-1.1-no-invariants-or-later for the files under Documentation/userspace-api/media/rc/ and send to the ML in order to collect your SoB and then place a kernel-doc markup on a separate patch | [12:01] |
| syoung | thank you, I'll add my SoB | [12:02] |
| mchehab | thanks! | [12:02] |
| hverkuil | I don't have anything to report, other than that I can't quite spend as much time on code review as I want to. I am currently going through the H.264 patches from Ezequiel to make this a proper public API. It looks good, I hope to post a PR today/tomorrow. I am also doing the same for the stateless FWHT codec (as used in vicodec) since that's ready to be promoted as a public API as well.
The other patches I need to look at are gnurou's poll fixes. They also should go into 5.11, but some more testing is needed on my side. | [12:03] |
| mchehab | yeah, code review can sometimes take lots of time
I wish I had time to help you cleaning up the queue I'll try to do it before the end of the year, if I find the time (perhaps after 5.10 being released, I'll have some time) sailus: you tried to ping me earlier today... is that anything related to patch review? | [12:05] |
| sailus | mchehab: Yes. | [12:21] |
| mchehab | hmm... it will still need your patch touching lirc.h.rst.exceptions | [12:21] |
| sailus | I wrote to you on #v4l, I wonder if you noticed it. | [12:21] |
| mchehab | all I had there was:
(11:06:30) sailus: mchehab: Bom dia! (11:26:07) mchehab: sailus: moikka! (11:26:22) sailus: mchehab: How do you do? (11:45:46) mchehab: fine, and you? | [12:22] |
| sailus | Yesterday.
16:10 GMT + 2. | [12:22] |
| mchehab | (15:10:51) sailus: mchehab: I'm aware of the checkpatch.pl warnings.
(15:11:18) sailus: The question, I think, is whether or not something should be done with them. (15:11:42) sailus: The builder doesn't tell the patch number so it's a bit hard to figure out exactly where they're coming from. (15:12:27) sailus: I know a fair amount of them is coming from the rename patches --- some lines that are changed from *smiapp* to *ccs* trigger them. (15:13:05) sailus: The driver has used standard C integer types (e.g. uint32_t) all along, and I've done the same in the new patches. (15:13:46) sailus: Oh, actually the patch information is there, indeed. (15:14:32) sailus: The long lines (over 100 characters) are arrays of macro definitions. I don't think splitting them would really bring something valuable. (15:15:04) sailus: But I'll switch to the BIT macro if you prefer. found on my historical data I didn't notice it yesterday, sorry for that! | [12:23] |
| sailus | No worries.
I just wanted to discuss the matter. | [12:24] |
| mchehab | the builder says what patch produced each warning... for instance:
patches/0002-Documentation-ccs-Add-CCS-driver-documentation.patch: checkpatch.pl: $ cat patches/0002-Documentation-ccs-Add-CCS-driver-documentation.patch | formail -c | ./scripts/checkpatch.pl --terse --mailback --no-summary --strict -:16: WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? the warning was produced by patch 02 | [12:25] |
| sailus | Ah, that one, yes.
I think I forgot to check that after making the documentation changes. I'll recheck the rest. | [12:26] |
| mchehab | (this is just an example... for all patches, it says what patch produced each warning
I opted to use the --terse for the checkpatch output, in order to make a shorter e-mail | [12:26] |
| sailus | But what's your thought on the two classes, rename related ones, as well as a long list of definitions?
And uint32_t, that has been used by the code all along. | [12:27] |
| mchehab | well, a renaming patch should not touch anything... that's OK | [12:27] |
| sailus | I don't insist on uint32_t, but it should be consistent. | [12:27] |
| mchehab | (yet, as you're changing lots of lines, I would expect a patch at the end doing some checkpatch cleanup, as mass changes are good opportunities to cleanup such kind of stuff)
sailus, in the case of things like uint 32_t, you could always do something like that: sed s,uint32_t,u32, -i drivers/media/.../*.[ch] rule of thumb: we use uint32_t only on uAPI headers | [12:28] |
| sailus | Everyone thought this was fine back in 2012. ;)
Would you be fine with a patch on the top of the current (> 100 patches) set? | [12:30] |
| mchehab | yeah, I'm OK with that | [12:31] |
| sailus | Great! | [12:31] |
| mchehab | sailus: languages (and coding styles) are dynamic... they change with time | [12:31] |
| sailus | Yeah... but the code stays as-is, if no-one changes it. :) | [12:32] |
| mchehab | btw, now checkpatch only warnings with lines bigger than 100 columns by default...
still, it is recommented to keep lines with 80 columns whenever possible | [12:32] |
| sailus | I agree.
I wonder whether the change of dropping the warnings on lines over 80 was a good idea. | [12:32] |
| mchehab | (and still we accept longer lines, in the cases where breaking it make things worse, like on strings)
well, this is just a checkpatch default | [12:33] |
| sailus | Now I give lots of review comments on wrapping at 80, just because the patch author didn't pay any attention. | [12:33] |
| mchehab | you can change it via command line | [12:33] |
| sailus | Ah, that's good to know. | [12:33] |
| mchehab | but yeah, I agree with you on that matter
changing the default to 100 just makes harder for reviewers | [12:33] |
| sailus | There was a discussion on this, with Andy suggesting to changing it back.
Just a few more points: the long lines in definition lists, and BIT macro. | [12:34] |
| mchehab | my two cents on it is that whatever max limit it is there, it should be consistent with coding-style.rst... which keeps saying:
"The preferred limit on the length of a single line is 80 columns." | [12:35] |
| sailus | Yes, indeed.
And I don't think the justification for that has changed at all. | [12:35] |
| mchehab | the main reason didn't change
(and people nowadays don't use anymore text consoles with 80 columns - which was probably the original reason for that at the old days) (13:35:24) sailus: Just a few more points: the long lines in definition lists, and BIT macro. what do you mean by definition lists? I guess you meant those, right? +#define SMIAPP_REG_U16_DIGITAL_CROP_X_OFFSET (0x0408 | CCS_FL_16BIT) +#define SMIAPP_REG_U16_DIGITAL_CROP_Y_OFFSET (0x040a | CCS_FL_16BIT) +#define SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_WIDTH (0x040c | CCS_FL_16BIT) +#define SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_HEIGHT (0x040e | CCS_FL_16BIT) I'm OK on keeping it as-is just ignore checkpatch warning on this specific case (well, we might do things like: #define SMIAPP_REG_U16_DIGITAL_CROP_IMAGE_HEIGHT \ \t\t\t(0x040e | CCS_FL_16BIT) but, I don't see any strong reason why doing that there | [12:36] |
| sailus | Yes, for instance.
The right side of the macro begins on the same column. That's the same for all register definitions. At least for now. | [12:41] |
| mchehab | the way it is, works for me | [12:42] |
| sailus | Ack. | [12:42] |
| mchehab | the BIT macro, however, is something that you should use
there are some bugs that may remain unnoticed by not using it | [12:42] |
| sailus | Yes, I came to think of that as well, after posting the set.
Not here, as it's unsigned. :-) | [12:43] |
| mchehab | in particular, things like 1<<31 will produce a wrong result depending on the arch | [12:43] |
| sailus | But I'll switch.
Thanks! | [12:43] |
| mchehab | so, better to use it everywhere (even when the number is lower), as it makes things more consistent
anything else? | [12:44] |
| sailus | Not this time.
I'll try to post a new version soon. | [12:45] |
| mchehab | ok, thanks! | [12:48] |
| .... (idle for 18mn) | ||
| syoung: patch sent
patch->patches | [13:06] | |
| syoung | thanks!
I've sent my SoB | [13:11] |
| ................ (idle for 1h19mn) | ||
| mchehab | syoung: thanks! patches applied, together with your DVB&RC series | [14:33] |
| ..... (idle for 20mn) | ||
| syoung | mchehab: thanks! | [14:53] |
| ↑back Search ←Prev date Next date→ Show only urls | (Click on time to select a line by its url) |