Re: [PATCH v2 06/10] media: Replace void * with video_device_state * in all driver ioctl implementations

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sat, Mar 07, 2026 at 09:49:52PM +0200, David Dull wrote:
> Hi Jai,
> 
> This patch is too large to be reasonably reviewable in its current form.

This review is too generated by LLMs to be actionable in its current
form.

> The stated change is conceptually simple: replace the opaque `void *priv`
> argument with `struct video_device_state *state` in V4L2 ioctl
> implementations. However, this single patch touches hundreds of files
> across drivers, helpers, framework code, staging code, and public
> headers. That makes it extremely difficult to validate correctness,
> spot exceptions, and reason about regressions from mailing list review
> alone.
> 
> A few issues stand out:
> 
> 1. Patch granularity
> 
>    This should not be one monolithic patch. At minimum it should be
>    split into:
> 
>    - core/framework changes
>    - helper conversions
>    - driver conversions by subsystem or directory class
>    - staging/test-driver conversions separately
> 
>    Right now the size alone makes meaningful review and bisection much
>    worse than it needs to be.
> 
> 2. Mechanical conversion claim vs. manual exceptions
> 
>    The changelog says most changes were automated with Coccinelle,
>    while function signature updates in headers and edge cases were
>    handled manually. That is exactly why this needs splitting.
>    Mechanical treewide conversions are one thing; manual edge-case
>    handling is where subtle semantic mistakes tend to hide.
> 
> 3. API conversion proof is missing
> 
>    If this is primarily a scripted transformation, the review should
>    center on the semantic patch and on proving there are no remaining
>    mismatches.
> 
>    Please include:
> 
>    - the Coccinelle script as a separate patch or in the cover letter
>    - a summary of what could not be converted automatically
>    - a treewide grep result showing there are no remaining ioctl
>      prototypes using `void *priv` where
>      `struct video_device_state *state` is now required
> 
> 4. Conversion consistency
> 
>    Several call sites now rename the parameter to `state` but continue
>    to use it only as a positional placeholder, which is fine
>    mechanically, but the patch should avoid mixing semantic conversion
>    with opportunistic cleanup. Formatting-only churn and spacing
>    adjustments should be kept to the minimum necessary for the
>    signature change.
> 
> 5. Risk concentration
> 
>    This patch touches both framework headers and many driver
>    implementations in the same changeset. That amplifies the blast
>    radius of any mistake and makes it harder to tell whether a
>    reported regression belongs to the API change itself or to one of
>    the driver-side edits.
> 
> 6. Reviewability for maintainers
> 
>    The CC list spans a large number of maintainers and mailing lists.
>    That is appropriate for notification, but not a substitute for
>    reviewable patch structure. Individual maintainers should not have
>    to sift through a large treewide refactor to find the parts that
>    affect their drivers.
> 
> Please respin this as a structured series with the mechanical
> transformation isolated from the framework changes and with a clear
> accounting of manual fixups and exceptions.
> 
> As it stands, I do not think this patch is reviewable in one piece.
> 
> Acked By : David Dull

-- 
Regards,

Laurent Pinchart