Hi Sam,
thanks for your review!
I fixed most issues. Just some open small questions:
On Tue, Oct 9, 2018 at 9:43 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
> > + dev_err(s6->dev, "failed to turn display off\n");
>
> General comment:
> dev_err => DRM_DEV_ERROR?
> Should error message include the 'ret' value to make
> it easier to track what went wrong?
OK I did those changes.
I feel a bit annoyed by DRM defining their own macros to
wrap all generic dev_* stuff from the kernel, like it is extra
special or something, but the subsystem can have it
the way it wants, I thin networking does the same.
> > + /* Assert RESET */
> > + gpiod_set_value_cansleep(s6->reset_gpio, 1);
>
> I mentioned in comment to binding doc that it should not
> specify that reset is active high, but I can this this is
> hardcoded in the driver, so maye the binding is justified.
What I am trying to achieve is that all drivers just state 1
for "asserted" whether that means active low or not.
Sadly the kernel and device tree bindings are full of bad
examples where people encode the physical high/low inside
the driver and I am only slowly cleaning this up.
I try to set a good example...
> > + s6->supply = devm_regulator_get(dev, "vdd1");
> > + if (IS_ERR(s6->supply))
> > + return PTR_ERR(s6->supply);
>
> This cannot be -EPROBE_DEFER?
It can and then it bails out here and retries later.
I don't quite get the question, sorry.
> > + if (IS_ERR(s6->reset_gpio)) {
> > + ret = PTR_ERR(s6->reset_gpio);
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(dev, "failed to request GPIO: %d\n", ret);
> > + return ret;
> > + }
>
> Like this..
-EPROBE_DEFER is only tested to not produce error prints
unnecessarily. In the regulator case, the regulator core already
displays an error.
Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
