Re: [PATCH v2 5/7] drm/bridge: Add Analogix anx6345 support

[Torsten Duwe]

On Wed, Jun 12, 2019 at 11:13:10AM +0200, Andrzej Hajda wrote:
> On 04.06.2019 14:23, Torsten Duwe wrote:
> > +
> > +static void anx6345_poweron(struct anx6345 *anx6345)
> > +{
> > +	int err;
> > +
> > +	/* Ensure reset is asserted before starting power on sequence */
> > +	gpiod_set_value_cansleep(anx6345->gpiod_reset, 1);
> 
> With fixed devm_gpiod_get below this line can be removed.

In any case, reset must be asserted for this procedure to succeed...

> > +static enum drm_mode_status
> > +anx6345_bridge_mode_valid(struct drm_bridge *bridge,
> > +			  const struct drm_display_mode *mode)
> > +{
> > +	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
> > +		return MODE_NO_INTERLACE;
> > +
> > +	/* Max 1200p at 5.4 Ghz, one lane */
> > +	if (mode->clock > 154000)
> > +		return MODE_CLOCK_HIGH;
> 
> I wonder if you shouldn't take into account training results here, ie.
> training perfrormed before validation.

Sure, but this is verbatim from the anx78xx.c sibling, code provided
by analogix. Lacking in-depth datasheets, this is probably the best effort.

> > +
> > +	/* 2.5V digital core power regulator  */
> > +	anx6345->dvdd25 = devm_regulator_get(dev, "dvdd25-supply");
> > +	if (IS_ERR(anx6345->dvdd25)) {
> > +		DRM_ERROR("dvdd25-supply not found\n");
> > +		return PTR_ERR(anx6345->dvdd25);
> > +	}
> > +
> > +	/* GPIO for chip reset */
> > +	anx6345->gpiod_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> 
> Shouldn't be set to GPIOD_OUT_HIGH?

It used to be in the original submission, and confused even more people ;-)
Fact is, the reset for this chip _is_ low active; I'm following
Documentation/devicetree/bindings/gpio/gpio.txt, "1.1) GPIO specifier
best practices", which I find rather comprehensive.

Any suggestions on how to phrase this even better are appreciated.

> > +};
> > +module_i2c_driver(anx6345_driver);
> > +
> > +MODULE_DESCRIPTION("ANX6345 eDP Transmitter driver");
> > +MODULE_AUTHOR("Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>");
> 
> Submitter, patch author, and module author are different, this can be
> correct, but maybe somebody forgot to update some of these fields.

As mentioned in the v2 cover letter, I had a closer look on which code got
actually introduced and which lines were simply copied around, and made the
copyright and authorship stick to where they belong. *I* believe this is
correct now; specific improvements welcome.

	Torsten