[PATCH v4 02/19] drm: add drmm_encoder_alloc()

[Daniel Vetter]

On Thu, Dec 10, 2020 at 12:59 PM Philipp Zabel <p.zabel at pengutronix.de> wrote:
>
> 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?

Uh sorry, I misread and forgot how __devm_drm_dev_alloc works too.
Looks all correct.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch