Linux Virtualization - Feb 2020 - [PATCH 2/6] drm: Add drm_simple_encoder_{init,create}()

> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
> +	.destroy = drm_encoder_cleanup,
> +};
> +
> +/**
> + * drm_simple_encoder_init - Init a preallocated encoder
> + * @dev: drm device
> + * @funcs: callbacks for this encoder
> + * @encoder_type: user visible type of the encoder
> + * @name: printf style format string for the encoder name, or NULL
> + *        for default name
> + *
> + * Initialises a preallocated encoder that has no further functionality.
The
> + * encoder will be released automatically.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_simple_encoder_init(struct drm_device *dev,
> +			    struct drm_encoder *encoder,
> +			    int encoder_type, const char *name, ...)
How about using

#define drm_simple_encoder_init(dev, type, name, ...) \
        drm_encoder_init(dev, drm_simple_encoder_funcs_cleanup, type, name,
__VA_ARGS__)

instead ?

cheers,
  Gerd

Hi Gerd

Am 07.02.20 um 11:33 schrieb Gerd Hoffmann:
>> +static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup
= {
>> +	.destroy = drm_encoder_cleanup,
>> +};
>> +
>> +/**
>> + * drm_simple_encoder_init - Init a preallocated encoder
>> + * @dev: drm device
>> + * @funcs: callbacks for this encoder
>> + * @encoder_type: user visible type of the encoder
>> + * @name: printf style format string for the encoder name, or NULL
>> + *        for default name
>> + *
>> + * Initialises a preallocated encoder that has no further
functionality. The
>> + * encoder will be released automatically.
>> + *
>> + * Returns:
>> + * Zero on success, error code on failure.
>> + */
>> +int drm_simple_encoder_init(struct drm_device *dev,
>> +			    struct drm_encoder *encoder,
>> +			    int encoder_type, const char *name, ...)
> 
> How about using
> 
> #define drm_simple_encoder_init(dev, type, name, ...) \
>         drm_encoder_init(dev, drm_simple_encoder_funcs_cleanup, type, name,
__VA_ARGS__)
> 
> instead ?
I'd prefer a function call for aesthetic reasons and for not having to
export the drm_simple_encoder_funcs_cleanup. drm_simple_encoder_create()
is also a function and probably cannot be turned into a macro. So having
a function for drm_simple_encoder_init seems consequent.

I guess you want to save a few lines in the implementation of
drm_simple_encoder_init() (?) If so, I'd rather try to share more
internal code among the various init and create functions.

Best regards
Thomas
> 
> cheers,
>   Gerd
> 
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 N?rnberg, Germany
(HRB 36809, AG N?rnberg)
Gesch?ftsf?hrer: Felix Imend?rffer

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20200207/c8bcb463/attachment.sig>

> > How about using
> > 
> > #define drm_simple_encoder_init(dev, type, name, ...) \
> >         drm_encoder_init(dev, drm_simple_encoder_funcs_cleanup, type,
name, __VA_ARGS__)
> > 
> > instead ?
> I guess you want to save a few lines in the implementation of
> drm_simple_encoder_init() (?) If so, I'd rather try to share more
> internal code among the various init and create functions.
Yes.  You have the namestr stuff duplicated in all functions, and with
a #define that goes away.

But maybe that can be simply be dropped?  The drivers with a simple
encoder seem to not care much about the name and just pass NULL ...

cheers,
  Gerd