Re: [PATCH v3 2/8] drm/atomic: Add support for mouse hotspots

[Javier Martinez Canillas <javierm@redhat.com>]

Pekka Paalanen <ppaalanen@gmail.com> writes:

> On Tue, 27 Jun 2023 10:56:39 +0200
> Javier Martinez Canillas <javierm@redhat.com> wrote:
>

[...]

>> > Hi Zack,
>> >
>> > where is the UAPI documentation for these new properties? I mean
>> > something ending up in the HTML docs like what other properties have in
>> > e.g. https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#plane-composition-properties
>> >
>> > Otherwise looks fine to me. Could drm_plane_create_hotspot_properties()
>> > perhaps assert that the plane type is CURSOR?
>> >  
>> 
>> I thought the same when reviewing but then I noticed this function is only
>> called from __drm_universal_plane_init() if type is DRM_PLANE_TYPE_CURSOR.
>
> Right, so why bother checking for DRIVER_CURSOR_HOTSPOT either?
> Shouldn't the function be 'static' too, not exported, and not added to
> a header?
>

Agreed. It should either be a static helper function in drm_plane.c and
not an exported symbol (in which case the checks are superflous as you
said) or the function should not make assumptions about what was checked
by the callers.

I believe that the former would be better and only make it accessible to
drivers if that is found to be needed later.

>
> Thanks,
> pq

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat