lkml.org 
[lkml]   [2019]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 5/7] drm/bridge: Add Analogix anx6345 support
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@collabora.com>");
>
> 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


\
 
 \ /
  Last update: 2019-07-18 18:42    [W:0.065 / U:5.272 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site