From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dma-buf: add optional get_tph() callback Date: Wed, 27 May 2026 14:35:49 +1000 Message-ID: In-Reply-To: <20260526144401.1485788-3-zhipingz@meta.com> References: <20260526144401.1485788-1-zhipingz@meta.com> <20260526144401.1485788-3-zhipingz@meta.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review Adds an optional callback to `struct dma_buf_ops`. **Documentation nit:** The kerneldoc comment mixes `@member` doc style with= `@param` style: ```c /** * @get_tph: * @dmabuf: DMA buffer for which to retrieve TPH metadata * @steering_tag: Returns the raw TPH steering tag for @st_width * @ph: Returns the TPH processing hint (2-bit value) * @st_width: Consumer's supported steering tag width in bits (8 or 16) ``` The `@get_tph:` is the ops member name, but the subsequent `@dmabuf`, `@ste= ering_tag`, etc. are function parameters. Standard kerneldoc for ops struct= callbacks typically documents the callback signature under the `@member:` = block without using `@` for the individual callback parameters (since they = aren't struct members). This could confuse kernel-doc tooling. **Interface design question:** `ph` is documented as a 2-bit value and is s= hared between 8-bit and 16-bit ST namespaces =E2=80=94 i.e., there's only o= ne PH regardless of ST width. This matches the PCIe spec where PH is per-TL= P, not per-ST-entry. Good. **No wrapper function:** The callback is invoked directly via `dmabuf->ops-= >get_tph()` in the mlx5 consumer (patch 4) with a NULL check. There's no `d= ma_buf_get_tph()` wrapper in `dma-buf.c`. This is acceptable for an optiona= l internal callback, but a thin wrapper would be more consistent with other= dma-buf APIs (`dma_buf_pin`, `dma_buf_unpin`, etc.) and would centralize t= he NULL check. Worth considering. --- Generated by Claude Code Patch Reviewer