On Tue, Feb 08, 2022 at 02:44:25AM +0200, Laurent Pinchart wrote:
Hello,
On Mon, Feb 07, 2022 at 02:32:45PM -0800, Doug Anderson wrote:
On Sun, Feb 6, 2022 at 7:44 AM Sam Ravnborg sam@ravnborg.org wrote:
There is a number of bridge drivers that supports a single media bus format for DSI. Add a helper to avoid duplicating the code.
Signed-off-by: Sam Ravnborg sam@ravnborg.org
drivers/gpu/drm/drm_atomic_helper.c | 41 +++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 +++++ 2 files changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a7a05e1e26bb..94f313dc196f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3549,3 +3549,44 @@ drm_atomic_helper_bridge_propagate_bus_fmt(struct drm_bridge *bridge, return input_fmts; } EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
+/**
- drm_atomic_helper_bridge_dsi_input_bus_fmt - Define one DSI output format
Is the description right? It's called "input" format but it defines an output format?
- @bridge: bridge control structure
- @bridge_state: new bridge state
- @crtc_state: new CRTC state
- @conn_state: new connector state
- @output_fmt: tested output bus format
- @num_input_fmts: will contain the size of the returned array
Maybe indicate that it's always 1 in the comments?
- This helper is an implementation of the
- &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that supports
- a single DSI media bus format MEDIA_BUS_FMT_RGB888_1X24.
- RETURNS
kernel-doc can't parse this return syntax and warns:
warning: No description found for return value of 'drm_atomic_helper_bridge_dsi_input_bus_fmt'
- A format array with one entry containing MEDIA_BUS_FMT_RGB888_1X24,
- or NULL if the allocation failed
- */
+u32 * +drm_atomic_helper_bridge_dsi_input_bus_fmt(struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state,u32 output_fmt,unsigned int *num_input_fmts)+{
u32 *input_fmts;*num_input_fmts = 0;input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts), GFP_KERNEL);I probably wouldn't have bothered with `kcalloc` for something that's always just one value and you're setting it. Why not just
input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
...also MAX_INPUT_SEL_FORMATS isn't defined. I guess that's why you said it didn't compile?
Also: if it was common for others to want to provide fixed formats, I wonder about adding a helper function that did most of the work here? Dunno what it would be named since it's already a bit a of handful, but I'd expect to call it like:
static const u32 formats[] = { MEDIA_BUS_FMT_RGB888_1X24 }; return my_neat_helper(formats, ARRAY_SIZE(formats), num_output_formats)
Then my_neat_helper() could do kmemdup() on the array passed and fill in "num_output_formats" to be either the array size of 0 (if the kmemdup failed).
I quite like that approach. We could even have a wrapper macro that adds the ARRAY_SIZE() argument automatically.
Agree, will try to give that a spin. And will then also process all the nice feedback from Douglas.
Sam