Re: [PATCH v1 1/9] drm/bridge: add DRM_BRIDGE_STATE_OPS macro

[Sam Ravnborg <sam@ravnborg.org>]

Hi Laurent,

On Tue, Feb 08, 2022 at 02:30:42AM +0200, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Sun, Feb 06, 2022 at 04:43:57PM +0100, Sam Ravnborg wrote:
> > The DRM_BRIDGE_STATE_OPS can be used as shortcut for bridge drivers that
> > do not subclass drm_bridge_state to assign the default operations for
> > reset, duplicate and destroy of the state.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Douglas Anderson <dianders@chromium.org>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  include/drm/drm_bridge.h | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 061d87313fac..fc00304be643 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -935,4 +935,16 @@ static inline struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> >  }
> >  #endif
> >  
> > +/**
> > + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
> 
> I'd name the macro DRM_BRIDGE_STATE_DEFAULT_OPS.
OK, done.

> 
> > + *
> > + * Bridge driver that do not subclass &drm_bridge_state can use the helpers
> > + * for reset, duplicate, and destroy. This macro provides a shortcut for
> > + * setting the helpers in the &drm_bridge_funcs structure.
> > + */
> > +#define DRM_BRIDGE_STATE_OPS \
> > +	.atomic_reset = drm_atomic_helper_bridge_reset,				\
> > +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,	\
> > +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
> 
> I'm not a big fan of such macros for a small number of operations, as I
> find that it obfuscates the code a bit, but that could change once I get
> used to the new macro :-)
The use of the macro is a nice signal that all the relevant default
operations are specified - no need to look up if all are included.

I have on my TODO to update all relevant bridge drivers to use it.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart