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