Re: [PATCH v2 2/2] drm/panel: Add driver for Samsung S6E63M0 panel

[PaweÅ Chmiel]

On piÄtek, 1 lutego 2019 22:36:07 CET Sam Ravnborg wrote:
> Hi PaweÅ
> 
> Looks good, thanks for addressing all the review feedback.
> 
> On Fri, Feb 01, 2019 at 06:28:52PM +0100, PaweÅ Chmiel wrote:
> > This patch adds Samsung S6E63M0 AMOLED LCD panel driver, connected over
> > spi. It's based on already removed, non dt s6e63m0 driver and
> > panel-samsung-ld9040. It can be found for example in some of Samsung
> > Aries based phones.
> > 
> > Signed-off-by: PaweÅ Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
> If you consider (do not change unless you think it better) the
> following nits than you can add my:
> 
> Reviewed-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
> 
> 	Sam
> 
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index 3f3537719beb..be05ed5218eb 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -158,6 +158,13 @@ config DRM_PANEL_SAMSUNG_S6E63J0X03
> >  	depends on BACKLIGHT_CLASS_DEVICE
> >  	select VIDEOMODE_HELPERS
> >  
> > +config DRM_PANEL_SAMSUNG_S6E63M0
> > +	tristate "Samsung S6E63M0 RGB/SPI panel"
> > +	depends on OF
> > +	depends on SPI
> > +	depends on BACKLIGHT_CLASS_DEVICE
> > +	select VIDEOMODE_HELPERS
> With the use of display_mode the above "select VIDEOMODE_HELPERS"
> is likely no longer required. Please check.
Confirmed that it's not needed now.
> 
> A help text would be nice.
Simple help text added.
> 
> > +
> >  config DRM_PANEL_SAMSUNG_S6E8AA0
> >  	tristate "Samsung S6E8AA0 DSI video mode panel"
> >  	depends on OF
> 
> 
> > +#include <video/of_videomode.h>
> > +#include <video/videomode.h>
> Please check if these two files are required.
Removed.
> 
> > +struct s6e63m0 {
> > +	struct device *dev;
> > +	struct drm_panel panel;
> > +	struct backlight_device *bl_dev;
> > +
> > +	struct regulator_bulk_data supplies[2];
> > +	struct gpio_desc *reset_gpio;
> > +	struct videomode vm;
> vm is no longer used - delete it.
Removed.
> 
> > +
> > +	bool prepared;
> > +	bool enabled;
> > +
> > +	/*
> > +	 * This field is tested by functions directly accessing bus before
> > +	 * transfer, transfer is skipped if it is set. In case of transfer
> > +	 * failure or unexpected response the field is set to error value.
> > +	 * Such construct allows to eliminate many checks in higher level
> > +	 * functions.
> > +	 */
> > +	int error;
> > +};
> > +
> 
> > +static int s6e63m0_get_modes(struct drm_panel *panel)
> > +{
> > +	struct drm_connector *connector = panel->connector;
> > +	struct drm_display_mode *mode;
> > +
> > +	mode = drm_mode_duplicate(panel->drm, &default_mode);
> > +	if (!mode) {
> > +		DRM_ERROR("failed to add mode %ux%ux@%u\n",
> > +			  default_mode.hdisplay, default_mode.vdisplay,
> > +			  default_mode.vrefresh);
> I recall I have seen a generic way to print the above,
> but I have failed to find it.
> Maybe it is just my memory that fools me.
> 
> The above is fine.
> 
> > +
> > +	s6e63m0_backlight_register(ctx);
> 
> Is it correct that we continue even if we fail to register backlight?
Fixed, now errors are handled and it's called before adding drm panel, 
so we won't do this if there will be any error during backlight setup.
> 
> 
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static const struct of_device_id s6e63m0_of_match[] = {
> > +	{ .compatible = "samsung,s6e63m0" },
> > +	{ }
> 
> Add /* sentinel */ comment?
Added
> 
> > +};
> 
> 
I'll be sending v3 version with all issues fixed. It would be nice to have this tested on Samsung Goni.
Thanks for all comments