Re: [PATCH v8 3/7] media: platform: amd: Add isp4 fw and hw interface

[Hans Anda <hansa@airmail.cc>]

Hi Bin,

I have just read your patch and have a minor issues and a few style suggestions.

> 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

One last typo of steam -> stream


> +/* [...]
> + * @brief Host and Firmware command & response channel.
> + *        Two types of command/response channel.
> + *          Type Global Command has one command/response channel.
> + *          Type Stream Command has one command/response channel.
> + *-----------                                        ------------
> + *|         |       ---------------------------      |          |
> + *|         |  ---->|  Global Command         |----> |          |
> + *|         |       ---------------------------      |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|         |       ---------------------------      |          |
> + *|         |  ---->|   Stream Command        |----> |          |
> + *|         |       ---------------------------      |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|  HOST   |                                        | Firmware |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|         |       --------------------------       |          |
> + *|         |  <----|  Global Response       |<----  |          |
> + *|         |       --------------------------       |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *|         |       --------------------------       |          |
> + *|         |  <----|  Stream Response       |<----  |          |
> + *|         |       --------------------------       |          |
> + *|         |                                        |          |
> + *|         |                                        |          |
> + *-----------                                        ------------
> + */

+ *        Two types of command/response channel.
+ *          Type Global Command has one command/response channel.
+ *          Type Stream Command has one command/response channel.
+ *-----------                                        ------------
+ *|         |                                        |          |
+ *|         |       ---------------------------      |          |
+ *|         |  ---->|     Global Command      |----> |          |
+ *|         |       ---------------------------      |          |
+ *|         |       ---------------------------      |          |
+ *|         |  <----|     Global Response     |<---- |          |
+ *|         |       ---------------------------      |          |
+ *|         |                                        |          |
+ *|         |                                        |          |
+ *|         |                                        |          |
+ *|  HOST   |                                        | Firmware |
+ *|         |                                        |          |
+ *|         |                                        |          |
+ *|         |       ---------------------------      |          |
+ *|         |  ---->|     Stream Command      |----> |          |
+ *|         |       ---------------------------      |          |
+ *|         |       ---------------------------      |          |
+ *|         |  <----|     Stream Response     |<---- |          |
+ *|         |       ---------------------------      |          |
+ *|         |                                        |          |
+ *|         |                                        |          |
+ *-----------                                        ------------
+ */

This way the order of the text comment is structured the same as the visual
comment. i made some adjustments for symmetry.  It's no error, it's a style
suggestion.	


> +/*
> + * @brief command ID format
> + *        cmd_id is in the format of following type:
> + *        type: indicate command type, global/stream commands.
> + *        group: indicate the command group.
> + *        id: A unique command identification in one type and group.
> + *        |<-Bit31 ~ Bit24->|<-Bit23 ~ Bit16->|<-Bit15 ~ Bit0->|
> + *        |      type       |      group      |       id       |

+ *        id: A unique command identification in one type and group.
+ *        |<-Bit31~24->|<-Bit23~16->|<-  Bit15   ~   Bit0  ->|
+ *        |    type    |   group    |           id           |

So the width of the parts would co relate to the number of bits (8,8,16).
Again, a suggestion

+ *        id: A unique command identification in one type and group.
+ *        |<-  Bit0   ~   Bit15  ->|<-Bit16~23->|<-Bit24~31->|
+ *        |           id           |   group    |    type    |
+ */

If there is no technical reason for numbering Right to Left you could flip it.
Left to Right is simpler to read.
I guess there is a technical reason for your way.

> +	/*
> +	 * A check num for debug usage, host can set the buf_tags
> +	 * to different number

+	 * to different numbers
or
+	 * to a different number

Both versions work grammatically.


> + [...]
> +};
> +
> +/* FW cmd ring buffer configuration */

> + [...]
> +};
> +
> +/* FW resp ring buffer configuration */
> +static struct isp4if_rb_config isp4if_resp_rb_config[ISP4IF_STREAM_ID_MAX] = {
>  [...]
> +/* FW log ring buffer configuration */
> +static struct isp4if_rb_config isp4if_log_rb_config = {
> +	.name = "LOG_RB",
> 
> +
> +	rd_ptr = isp4hw_rreg(ispif->mmio, rreg);
> +	wr_ptr = isp4hw_rreg(ispif->mmio, wreg);
> +
> +	/* Read and write pointers are equal, indicating the ringbuf is empty
> */

>  [...]
> +	/*
> +	 * Ignore one byte from the bytes free to prevent rd_ptr from equaling
> +	 * wr_ptr when the ringbuf is full, because rd_ptr == wr_ptr is
> +	 * supposed to indicate that the ringbuf is empty.

ringbuf -> ringbuffer or ring buffer, it's best to stick to one.
just in case someone greps for. That is a suggestion as well.

Good Work.
I'm just getting to know c - That's why I only found grammar/ style issues.
But maybe this way i will get into programming

Greeting Hans