Re: [PATCH 1/2] drm: Add few more wrapper functions for drm panel

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2632 bytes --]

On Wed, Mar 02, 2016 at 06:28:31PM +0530, Deepak M wrote:
> Currently there are few pair of functions which
> are called during the panel enable/disable sequence.
> To improve the granularity, adding few more wrapper
> functions so that the functions are more specific
> on what they are doing.
> 
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> ---
>  include/drm/drm_panel.h | 92 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 77 insertions(+), 15 deletions(-)

Sorry, but no. You're not giving any rationale for why you think the
granularity needs to be increased. The documentation already states that
panel drivers can use ->enable() and ->disable() to turn on and off the
backlight, why do you need the extra callbacks? The same is true for the
->prepare() and ->unprepare() callbacks, which are described to perform
what your new ->power_on() and ->power_off() callbacks would do.

Increasing the granularity isn't always a good thing. It means that
drivers can now call the functions in many more variations.

If you think you really need finer granularity, you need to do a better
job of explaining why. Also, Cc'ing me on the second patch, which I do
not have in any of my inboxes, and which I assume contains a usage
example of these new callbacks, might help me understand the need for
this.

One more comment below...

> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
[...]
>  struct drm_panel_funcs {
>  	int (*disable)(struct drm_panel *panel);
> @@ -73,6 +90,10 @@ struct drm_panel_funcs {
>  	int (*get_modes)(struct drm_panel *panel);
>  	int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
>  			   struct display_timing *timings);
> +	int (*power_on)(struct drm_panel *panel);
> +	int (*power_off)(struct drm_panel *panel);
> +	int (*backlight_on)(struct drm_panel *panel);
> +	int (*backlight_off)(struct drm_panel *panel);
>  };
[...]
> +static inline int drm_panel_get_info(struct drm_panel *panel,
> +				struct drm_connector *connector)
> +{
> +	if (connector && panel && panel->funcs && panel->funcs->get_info)
> +		return panel->funcs->get_info(panel, connector);
> +
> +	return panel ? -ENOSYS : -EINVAL;
> +}

This callback no longer exists, so this patch is going to break
compilation.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel