Hi Daniel, thank you for the review. I'll work in all your other comments, there's just one I'm not sure what to do about: On Wed, 2020-12-09 at 17:05 +0100, Daniel Vetter wrote: [...] > > +void *__drmm_encoder_alloc(struct drm_device *dev, size_t size, size_t offset, > > + const struct drm_encoder_funcs *funcs, > > + int encoder_type, const char *name, ...) > > +{ > > + void *container; > > + struct drm_encoder *encoder; > > + va_list ap; > > + int ret; > > + > > + if (WARN_ON(funcs && funcs->destroy)) > > + return ERR_PTR(-EINVAL); > > + > > + container = drmm_kzalloc(dev, size, GFP_KERNEL); > > + if (!container) > > + return ERR_PTR(-EINVAL); > > + > > + encoder = container + offset; > > + > > + va_start(ap, name); > > + ret = __drm_encoder_init(dev, encoder, funcs, encoder_type, name, ap); > > + va_end(ap); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + ret = drmm_add_action_or_reset(dev, drmm_encoder_alloc_release, encoder); > > + if (ret) > > + return ERR_PTR(ret); > > + > > + return container; > > +} > > +EXPORT_SYMBOL(__drmm_encoder_alloc); > > + [...] > > + * @encoder_type: user visible type of the encoder > > + * @name: printf style format string for the encoder name, or NULL for default name > > + * > > + * Allocates and initializes an encoder. Encoder should be subclassed as part of > > + * driver encoder objects. Cleanup is automatically handled through registering > > + * drm_encoder_cleanup() with drmm_add_action(). > > + * > > + * The @drm_encoder_funcs.destroy hook must be NULL. > > + * > > + * Returns: > > + * Pointer to new encoder, or ERR_PTR on failure. > > + */ > > +#define drmm_encoder_alloc(dev, type, member, funcs, encoder_type, name, ...) \ > > + ((type *)__drmm_encoder_alloc(dev, sizeof(type), \ > > Need to upcast with container_of or this breaks if the base class is in > the wrong spot. This is modeled after devm_drm_dev_alloc(). Like __devm_drm_dev_alloc(), __drmm_encoder_alloc() returns a pointer to the allocated container structure, not a pointer to the embedded struct drm_encoder. I think this direct cast is correct, unless you suggest that __drmm_encoder_alloc should return encoder instead of container? regards Philipp _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel