Re: [PATCH v2 4/8] media: platform: amd: Add isp4 fw and hw interface

[Du, Bin]

Hi Sakari Ailus,

On 8/12/2025 4:20 PM, Sakari Ailus wrote:
> Hi Laurent, Bin,
>
> On Tue, Aug 12, 2025 at 10:34:32AM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 12, 2025 at 11:36:23AM +0800, Du, Bin wrote:
> > > Many thanks Laurent Pinchart for the comments.
> > >
> > > On 8/11/2025 8:31 PM, Laurent Pinchart wrote:
> > > > On Mon, Aug 11, 2025 at 11:46:27AM +0000, Sakari Ailus wrote:
> > > > > On Tue, Jul 29, 2025 at 05:12:03PM +0800, Du, Bin wrote:
> > > > > > On 7/28/2025 3:23 PM, Sakari Ailus wrote:
> > > > > > > On Wed, Jun 18, 2025 at 05:19:55PM +0800, Bin Du wrote:
> > > > > > > > ISP firmware controls ISP HW pipeline using dedicated embedded processor
> > > > > > > > called ccpu.
> > > > > > > > The communication between ISP FW and driver is using commands and
> > > > > > > > response messages sent through the ring buffer. Command buffers support
> > > > > > > > either global setting that is not specific to the steam and support stream
> > > > > > > > specific parameters. Response buffers contains ISP FW notification
> > > > > > > > information such as frame buffer done and command done. IRQ is used for
> > > > > > > > receiving response buffer from ISP firmware, which is handled in the main
> > > > > > > > isp4 media device. ISP ccpu is booted up through the firmware loading
> > > > > > > > helper function prior to stream start.
> > > > > > > > Memory used for command buffer and response buffer needs to be allocated
> > > > > > > > from amdgpu buffer manager because isp4 is a child device of amdgpu.
> > > > > > >
> > > > > > > Please rewrap this, some lines above are quite short.
> > > > > > Thanks, the line after the short line is supposed to be a new paragraph?
> > > > > > Should we put all the description in one paragraph?
> > > > >
> > > > > One or more paragraphs work fine, but a new paragraph is separated from the
> > > > > previous one by another newline.
> > > > >
> > > > > ...
> > > >
> > > > Paragraphs are defined as a block of text that convey one idea. They
> > > > should be visually separated by a space. As we can't have fractional
> > > > line spacing in plain text, paragraphs need to be separated by a blank
> > > > line. This is a typography rule that maximizes readability. There should
> > > > be no line break between sentences in a single paragraph.
> > > >
> > > > Whether you write commit messages, formal documentation or comments in
> > > > code, typography is important to give the best experience to readers.
> > > > After all, a block of text that wouldn't focus on the readers would have
> > > > no reason to exist.
> > > >
> > > >
> > > > Now compare the above with
> > > >
> > > >
> > > > Paragraphs are defined as a block of text that convey one idea. They
> > > > should be visually separated by a space.
> > > > As we can't have fractional line spacing in plain text, paragraphs need
> > > > to be separated by a blank line.
> > > > This is a typography rule that maximizes readability. There should be no
> > > > line break between sentences in a single paragraph. Whether you write
> > > > commit messages, formal documentation or comments in code, typography is
> > > > important to give the best experience to readers.
> > > > After all, a block of text that wouldn't focus on the readers would have
> > > > no reason to exist.
> > >
> > > Really appreciate the detailed guide, will follow it. May I summarize
> > > like this? 1 Separate paragraphs by a blank line. 2 Don't add line break
> > > between sentences in a single paragraph, an exception to this is commit
> > > message, because of the 75-character patch check limit, line break can
> > > be added, but it should at the 75-character limit boundary
> >
> > When I wrote "line break", I meant breaking the line after a sentence,
> > before the 75 columns limits. Text blocks should always be wrapped (at
> > 75 columns in commit messages, or 80 in kernel code). What you should
> > avoid is line breaks not related to the columns limit.
> >
> > This is fine:
> >
> > This paragraph has a long sentence that does not hold on a single line
> > of 72 characters and therefore needs to be wrapped. There is no line
> > break otherwise, for instance between the first and second sentence, or
> > within a sentence.
> >
> > This is not right:
> >
> > This paragraph has a long sentence that does not hold on a single line
> > of 72 characters and therefore needs to be wrapped.
> > There is a line break between the first and second sentence,
> > and also a line break in the second sentence, which are not fine.
>
> I wonder if this should make it to kernel documentation. I've come across
> many new developers recently who would definitely benefit from this.

Yes, totally agree, furthermore, if checkpatch script can be 
strengthened to add this kind of check, developers can even find and fix 
them before submit the patches

> Also, most editors can rewrap paragraphs of text (e.g. Emacs M-q or Joe C-k
> C-j).

--
Regards,
Bin