Re: [PATCH v7 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder

From: jacopo mondi <jacopo@jmondi.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Jacopo Mondi <jacopo+renesas@jmondi.org>,
	architt@codeaurora.org, a.hajda@samsung.com, airlied@linux.ie,
	vladimir_zapolskiy@mentor.com, horms@verge.net.au,
	magnus.damm@gmail.com, geert@linux-m68k.org,
	niklas.soderlund@ragnatech.se,
	sergei.shtylyov@cogentembedded.com, robh+dt@kernel.org,
	mark.rutland@arm.com, dri-devel@lists.freedesktop.org,
	linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH v7 1/2] dt-bindings: display: bridge: Document THC63LVD1024 LVDS decoder
Date: Sat, 7 Apr 2018 11:45:56 +0200	[thread overview]
Message-ID: <20180407094556.GQ20945@w540> (raw)
In-Reply-To: <2441543.5xTHaLXzZ0@avalon>

[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]

Hi Laurent

On Fri, Apr 06, 2018 at 06:40:14PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> (CC'ing Mark Brown)

Hi Mark

[snip]
> >
> > Anyway, we spent enough time on naming issues, starting from my first
> > stupid 'pdwn' permutations then on this semi-standard names.
> >
> > I'll send next version with 'powerdown-gpios' and 'oe-gpios'
> > properties hoping that would be finally accepted by everyone.
>
> I certainly won't complain (as long as you write pwdn instead of pdwn in the
> driver :-)).
>

Oh, so you won't complain if I address your comments? Thank you! :D
By the way, the dumb pdwn name comes, again, from the chip name. I can
change it to a saner name for sure...

> > Same on the mandatory/optional VCC supply thing. Let's try to make
> > next version the final one. If the optional property with the dummy
> > regulator doesn't satisfy you and it is preferred to have a fixed-regulator
> > anyhow in DT I'll do in next version, othewise let's try not to change
> > it again. I'll just remark here that in the current Eagle design vcc is
> > connected to a power rail with no regulator at all :)
>
> I don't like the dummy regulator much, as it generates a dev_warn(), which
> makes me believe that it's a hack rather than a proper solution. You might
> want to ask Mark Brown for his opinion.
>

Sure: Hi Mark, a bit of context here to save you a long(er) reading.

Unsurprisingly, the chip for which I'm writing this small driver needs
a power supply to be properly functional :) In the development board
it is installed on, the power supply is connected to a power rail,
with no regulator. At the same time, some other designs may instead
include a regulator. I assume that's a very common situation. I
started by describing the regulator as optional in DT bindings, and
use regulator_get_optional(). If that function returns PTR_ERR, the
regulator is then ignored in driver's power management routines.

In this last version I thought I was acting smart and copied what happens
in other DRM bridges like adv7511, which use 'regulator_get()' and work
with a dummy if no regulator is provided in DT. Laurent has the above
doubts on using a dummy, and I actually share some of his concerns
(that dev_warn() is how I found out adv7511 was using a dummy, actually).

To sum up: when a regulator is described as optional in DT, do you
suggest to work with a dummy if it's not there, or use the _optional()
version of regulator_get() and manually set it to NULL and ignore it
in the enable/disable driver's routines?

Bonus question: Laurent likes to have the regulator described as required,
and thus require it to be described in DT also in cases where it is not
actually there using a 'fixed-regulator' and refuse to probe if the regulator
is not available. Do you encourage this approach, or prefer to have it
optional and handle it in the driver in one of the above described
ways?

Thank you
    j

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]