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
