On Mon, Sep 22, 2014 at 4:11 PM, Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > On Mon, Sep 22, 2014 at 02:01:38PM +0530, Ajay kumar wrote: >> Hi Thierry, >> >> On Mon, Sep 22, 2014 at 1:40 PM, Thierry Reding >> <thierry.reding@xxxxxxxxx> wrote: >> > On Thu, Sep 18, 2014 at 11:20:40AM +0530, Ajay kumar wrote: >> >> Hi Tomi, >> >> >> >> On Wed, Sep 17, 2014 at 9:52 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: >> >> > On 17/09/14 17:29, Ajay kumar wrote: >> >> >> Hi Tomi, >> >> >> >> >> >> Thanks for your comments. >> >> >> >> >> >> On Wed, Sep 17, 2014 at 5:22 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: >> >> >>> On 27/08/14 17:39, Ajay Kumar wrote: >> >> >>>> Add documentation for DT properties supported by ps8622/ps8625 >> >> >>>> eDP-LVDS converter. >> >> >>>> >> >> >>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> >> >> >>>> --- >> >> >>>> .../devicetree/bindings/video/bridge/ps8622.txt | 20 ++++++++++++++++++++ >> >> >>>> 1 file changed, 20 insertions(+) >> >> >>>> create mode 100644 Documentation/devicetree/bindings/video/bridge/ps8622.txt >> >> >>>> >> >> >>>> diff --git a/Documentation/devicetree/bindings/video/bridge/ps8622.txt b/Documentation/devicetree/bindings/video/bridge/ps8622.txt >> >> >>>> new file mode 100644 >> >> >>>> index 0000000..0ec8172 >> >> >>>> --- /dev/null >> >> >>>> +++ b/Documentation/devicetree/bindings/video/bridge/ps8622.txt >> >> >>>> @@ -0,0 +1,20 @@ >> >> >>>> +ps8622-bridge bindings >> >> >>>> + >> >> >>>> +Required properties: >> >> >>>> + - compatible: "parade,ps8622" or "parade,ps8625" >> >> >>>> + - reg: first i2c address of the bridge >> >> >>>> + - sleep-gpios: OF device-tree gpio specification for PD_ pin. >> >> >>>> + - reset-gpios: OF device-tree gpio specification for RST_ pin. >> >> >>>> + >> >> >>>> +Optional properties: >> >> >>>> + - lane-count: number of DP lanes to use >> >> >>>> + - use-external-pwm: backlight will be controlled by an external PWM >> >> >>> >> >> >>> What does this mean? That the backlight support from ps8625 is not used? >> >> >>> If so, maybe "disable-pwm" or something? >> >> >> "use-external-pwm" or "disable-bridge-pwm" would be better. >> >> > >> >> > Well, the properties are about the bridge. "use-external-pwm" means that >> >> > the bridge uses an external PWM, which, if I understood correctly, is >> >> > not what the property is about. >> >> > >> >> > "disable-bridge-pwm" is ok, but the "bridge" there is extra. The >> >> > properties are about the bridge, so it's implicit. >> >> Ok. I will use "disable-pwm". >> > >> > Why is this even necessary? According to the datasheet this device has >> > circuitry for backlight control. If so, I'd expect it to expose either a >> > backlight device or a PWM device. That way unless somebody is using the >> > backlight/PWM exposed by the bridge the bridge can simply disable PWM. >> The driver does expose a backlight device. >> And, the decision(whether to expose a backlight device or not) is made >> based on the DT flag "use-external-pwm". >> This was discussed before, and you suggested to use the boolean >> property, refer to this link: >> http://lists.freedesktop.org/archives/dri-devel/2014-July/065048.html > > I think you misunderstood what I said, or maybe I didn't explain clearly > what I meant. If the PS8622 provides a backlight there's nothing wrong > with always exposing it. The bridge itself isn't going to be using the > backlight anyway. Rather the panel itself should be using the backlight > device exposed by PS8622 or some separate backlight device. > > To illustrate by an example: > > ps8622: ... { > compatible = "parade,ps8622"; > ... > }; > > panel { > ... > backlight = <&ps8622>; > ... > }; No, if ps8622 backlight control is used, we need not specify the backlight phandle for the panel driver. Somehow, ps8622 internal circuitry keeps the bootup glitch free :) Backlight control and panel controls can be separate then. > Or: > > backlight: ... { > compatible = "pwm-backlight"; > ... > }; > > panel { > ... > backlight = <&backlight>; > ... > }; This is the way it is for peach_pit. > What you did in v6 of this series was look up a backlight device and > then not use it. That seemed unnecessary. Looking at v6 again the reason > for getting a phandle to the backlight was so that the device itself did > not expose its own backlight controlling circuitry if an external one > was being used. But since the bridge has no business controlling the > backlight, having the backlight phandle in the bridge is not correct. > > So I think what you could do in the driver instead is always expose the > backlight device. If the panel used a different backlight, the PS8622's > internal on simply wouldn't be accessed. It would still be possible to > control the backlight in sysfs, but that shouldn't be a problem (only > root can access it) That would be like simple exposing a feature which cannot be used by the user, ideally which "should not be" used by the user. > That said, I have no strong objections to the boolean property if you > feel like it's really necessary. Won't you think having a boolean property for an optional feature of the device, is better than all these? Ajay -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
