Re: [PATCH v8 1/4] drm/bridge: add support for sn65dsi86 bridge driver

[Sean Paul <seanpaul@chromium.org>]

On Tue, Jun 05, 2018 at 04:47:15PM +0530, Vinod wrote:
> On 05-06-18, 11:10, Sandeep Panda wrote:
> > Add support for TI's sn65dsi86 dsi2edp bridge chip.
> > The chip converts DSI transmitted signal to eDP signal,
> > which is fed to the connected eDP panel.
> > 
> > This chip can be controlled via either i2c interface or
> > dsi interface. Currently in driver all the control registers
> > are being accessed through i2c interface only.
> > Also as of now HPD support has not been added to bridge
> > chip driver.
> > 
> > Changes in v1:
> >  - Split the dt-bindings and the driver support into separate patches
> >    (Andrzej Hajda).
> >  - Use of gpiod APIs to parse and configure gpios instead of obsolete ones
> >    (Andrzej Hajda).
> >  - Use macros to define the register offsets (Andrzej Hajda).
> 
> This is pretty useless for changelog. This is useful for review but not
> down the line when this is applied
> 
> Since you have cover letter, you may add it there. Or after sob and ---
> tag in the patch, that way it is skipped while applying..

FWIW, in drm we prefer the changelog is included in the commit message. It gives
credit to the reviewers, sometimes explains why decisions were made, and commit
message space is cheap :-).

Sean

> 
> > +#define SN_ENABLE_VID_STREAM_BIT	BIT(3)
> > +#define SN_DSIA_NUM_LANES_BITS		(BIT(4) | BIT(3))
> > +#define SN_DP_NUM_LANES_BITS		(BIT(5) | BIT(4))
> > +#define SN_DP_DATA_RATE_BITS		(BIT(7) | BIT(6) | BIT(5))
> 
> GENMASK(7, 5)
> 
> > +static int __maybe_unused ti_sn_bridge_resume(struct device *dev)
> > +{
> > +	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> > +	int ret = 0;
> 
> superfluous initialization
> 
> > +static int __maybe_unused ti_sn_bridge_suspend(struct device *dev)
> > +{
> > +	struct ti_sn_bridge *pdata = dev_get_drvdata(dev);
> > +	int ret = 0;
> 
> here as well
> 
> > +static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> > +{
> > +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> > +	struct edid *edid;
> > +	u32 num_modes;
> > +
> > +	if (pdata->panel) {
> > +		DRM_DEBUG_KMS("get mode from connected drm_panel\n");
> > +		return drm_panel_get_modes(pdata->panel);
> > +	}
> > +
> > +	if (!pdata->ddc)
> > +		return 0;
> > +
> > +	pm_runtime_get_sync(pdata->dev);
> 
> you should check return of this
> 
> > +static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
> > +{
> > +	int i = 0;
> 
> superfluous initialization
> 
> > +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> > +{
> > +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> > +	unsigned int val = 0;
> 
> here as well
> 
> > +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> > +{
> > +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> > +
> > +	pm_runtime_get_sync(pdata->dev);
> 
> error check required
> -- 
> ~Vinod

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel