Re: [PATCH RFC 1/5] drm/msm/hdmi: deprecate non standard gpio properties.

Re: [PATCH RFC 1/5] drm/msm/hdmi: deprecate non standard gpio properties.

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2776 bytes --]

On Mon, Aug 10, 2015 at 12:59:22PM +0100, Srinivas Kandagatla wrote:
> This patch updates the bindings to discourage the usage of non standard
> gpio properites, this will help in projects focused on upstreaming.

That last part is an odd comment to make in the commit message of a
patch submitted upstream...

> These deprecated properties are still supported but will be remove over
> the time.

You can't ever remove them because you can't ever be sure that people
won't be using an old DTB.

> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  Documentation/devicetree/bindings/drm/msm/hdmi.txt | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> index c43aa53..acba581 100644
> --- a/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
> @@ -11,15 +11,27 @@ Required properties:
>  - interrupts: The interrupt signal from the hdmi block.
>  - clocks: device clocks
>    See ../clocks/clock-bindings.txt for details.
> -- qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin
> -- qcom,hdmi-tx-ddc-data-gpio: ddc data pin
> -- qcom,hdmi-tx-hpd-gpio: hpd pin
> +- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin
> +- qcom,hdmi-tx-ddc-data-gpios: ddc data pin
> +- qcom,hdmi-tx-hpd-gpios: hpd pin
>  - core-vdda-supply: phandle to supply regulator
>  - hdmi-mux-supply: phandle to mux regulator
>  
> +- qcom,hdmi-tx-ddc-clk-gpio: (deprecated) use
> +	"qcom,hdmi-tx-ddc-clk-gpios" instead
> +- qcom,hdmi-tx-ddc-data-gpio: (deprecated) use
> +	"qcom,hdmi-tx-ddc-data-gpios" instead
> +- qcom,hdmi-tx-hpd-gpio: (deprecated) use
> +	"qcom,hdmi-tx-hpd-gpios" instead
> +
>  Optional properties:
> -- qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin
> -- qcom,hdmi-tx-mux-sel-gpio: hdmi mux select pin
> +- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin
> +- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin
> +
> +- qcom,hdmi-tx-mux-en-gpio: (deprecated) use "qcom,hdmi-tx-mux-en-gpios"
> +	instead
> +- qcom,hdmi-tx-mux-sel-gpio: (deprecated) use "qcom,hdmi-tx-mux-sel-gpio"
> +	instead
>  - pinctrl-names: the pin control state names; should contain "default"
>  - pinctrl-0: the default pinctrl state (active)
>  - pinctrl-1: the "sleep" pinctrl state

I don't see much use in listing that these properties are deprecated. We
already have code to catch the deprecated names, so having them in the
binding will at best be distracting.

Anyway, I don't know if there's been any advice on this from the device
tree bindings maintainers, so adding devicetree@vger.kernel.org for
visibility.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 1/5] drm/msm/hdmi: deprecate non standard gpio properties.

[Rob Herring]

On Mon, Aug 10, 2015 at 7:45 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Aug 10, 2015 at 12:59:22PM +0100, Srinivas Kandagatla wrote:
>> This patch updates the bindings to discourage the usage of non standard
>> gpio properites, this will help in projects focused on upstreaming.
>
> That last part is an odd comment to make in the commit message of a
> patch submitted upstream...
>
>> These deprecated properties are still supported but will be remove over
>> the time.
>
> You can't ever remove them because you can't ever be sure that people
> won't be using an old DTB.

It is only an ABI if anyone notices...

>
>>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/drm/msm/hdmi.txt | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> index c43aa53..acba581 100644
>> --- a/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
>> @@ -11,15 +11,27 @@ Required properties:
>>  - interrupts: The interrupt signal from the hdmi block.
>>  - clocks: device clocks
>>    See ../clocks/clock-bindings.txt for details.
>> -- qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin
>> -- qcom,hdmi-tx-ddc-data-gpio: ddc data pin
>> -- qcom,hdmi-tx-hpd-gpio: hpd pin
>> +- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin
>> +- qcom,hdmi-tx-ddc-data-gpios: ddc data pin

The driver doesn't appear to do anything other than set these gpios
high. Presumably you would ultimately do a bit-bang i2c bus for these?
At that point, aren't you going to want to use the i2c-gpio binding?

I think this binding has bigger issues like why is it not using the
hdmi-connector binding which has a standard "hpd-gpios" property
already. I can't really single out this binding though. There's a lot
of crap when it comes to DRM related bindings.

>> +- qcom,hdmi-tx-hpd-gpios: hpd pin
>>  - core-vdda-supply: phandle to supply regulator
>>  - hdmi-mux-supply: phandle to mux regulator
>>
>> +- qcom,hdmi-tx-ddc-clk-gpio: (deprecated) use
>> +     "qcom,hdmi-tx-ddc-clk-gpios" instead
>> +- qcom,hdmi-tx-ddc-data-gpio: (deprecated) use
>> +     "qcom,hdmi-tx-ddc-data-gpios" instead
>> +- qcom,hdmi-tx-hpd-gpio: (deprecated) use
>> +     "qcom,hdmi-tx-hpd-gpios" instead
>> +
>>  Optional properties:
>> -- qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin
>> -- qcom,hdmi-tx-mux-sel-gpio: hdmi mux select pin
>> +- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin
>> +- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin
>> +
>> +- qcom,hdmi-tx-mux-en-gpio: (deprecated) use "qcom,hdmi-tx-mux-en-gpios"
>> +     instead
>> +- qcom,hdmi-tx-mux-sel-gpio: (deprecated) use "qcom,hdmi-tx-mux-sel-gpio"
>> +     instead
>>  - pinctrl-names: the pin control state names; should contain "default"
>>  - pinctrl-0: the default pinctrl state (active)
>>  - pinctrl-1: the "sleep" pinctrl state
>
> I don't see much use in listing that these properties are deprecated. We
> already have code to catch the deprecated names, so having them in the
> binding will at best be distracting.
>
> Anyway, I don't know if there's been any advice on this from the device
> tree bindings maintainers, so adding devicetree@vger.kernel.org for
> visibility.

If they need to be maintained, then marking them deprecated is
perfectly fine IMO. Also, if the maintainers of platforms using this
are okay with it, you can just switch it. Given there are no in tree
dts files using this, it can be argued that there is no ABI.

Rob

Re: [PATCH RFC 1/5] drm/msm/hdmi: deprecate non standard gpio properties.

[Bjorn Andersson]

On Mon 10 Aug 15:07 PDT 2015, Rob Herring wrote:

[..]
> >> -- qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin
> >> -- qcom,hdmi-tx-ddc-data-gpio: ddc data pin
> >> -- qcom,hdmi-tx-hpd-gpio: hpd pin
> >> +- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin
> >> +- qcom,hdmi-tx-ddc-data-gpios: ddc data pin
> 
> The driver doesn't appear to do anything other than set these gpios
> high. Presumably you would ultimately do a bit-bang i2c bus for these?
> At that point, aren't you going to want to use the i2c-gpio binding?
> 
> I think this binding has bigger issues like why is it not using the
> hdmi-connector binding which has a standard "hpd-gpios" property
> already. I can't really single out this binding though. There's a lot
> of crap when it comes to DRM related bindings.
> 

Shouldn't these pins just be muxed to the hdmi block in pinctl?

I know Rob had something wrt the detect pin (hdp), but the others should
never be gpios(?) Isn't this just a reminder of the codeaurora tree
gpio_requesting all pins "just to be safe"?

> >> +- qcom,hdmi-tx-hpd-gpios: hpd pin
> >>  - core-vdda-supply: phandle to supply regulator
> >>  - hdmi-mux-supply: phandle to mux regulator
> >>
> >> +- qcom,hdmi-tx-ddc-clk-gpio: (deprecated) use
> >> +     "qcom,hdmi-tx-ddc-clk-gpios" instead
> >> +- qcom,hdmi-tx-ddc-data-gpio: (deprecated) use
> >> +     "qcom,hdmi-tx-ddc-data-gpios" instead
> >> +- qcom,hdmi-tx-hpd-gpio: (deprecated) use
> >> +     "qcom,hdmi-tx-hpd-gpios" instead
> >> +
> >>  Optional properties:
> >> -- qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin
> >> -- qcom,hdmi-tx-mux-sel-gpio: hdmi mux select pin
> >> +- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin
> >> +- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin
> >> +
> >> +- qcom,hdmi-tx-mux-en-gpio: (deprecated) use "qcom,hdmi-tx-mux-en-gpios"
> >> +     instead
> >> +- qcom,hdmi-tx-mux-sel-gpio: (deprecated) use "qcom,hdmi-tx-mux-sel-gpio"
> >> +     instead
> >>  - pinctrl-names: the pin control state names; should contain "default"
> >>  - pinctrl-0: the default pinctrl state (active)
> >>  - pinctrl-1: the "sleep" pinctrl state
> >
> > I don't see much use in listing that these properties are deprecated. We
> > already have code to catch the deprecated names, so having them in the
> > binding will at best be distracting.
> >
> > Anyway, I don't know if there's been any advice on this from the device
> > tree bindings maintainers, so adding devicetree@vger.kernel.org for
> > visibility.
> 
> If they need to be maintained, then marking them deprecated is
> perfectly fine IMO. Also, if the maintainers of platforms using this
> are okay with it, you can just switch it. Given there are no in tree
> dts files using this, it can be argued that there is no ABI.
> 

Part of some dev trees floating around no-one should have used these. So
I think we should just document the proper ones and have the drivers
support Rob's old properties until he's comfortable dropping them.

That way we have a documented way forward and Rob can run his backports
of this code without forking it.

Regards,
Bjorn

Re: [PATCH RFC 1/5] drm/msm/hdmi: deprecate non standard gpio properties.

[Rob Clark]

On Mon, Aug 10, 2015 at 6:27 PM, Bjorn Andersson
<bjorn.andersson@sonymobile.com> wrote:
> On Mon 10 Aug 15:07 PDT 2015, Rob Herring wrote:
>
> [..]
>> >> -- qcom,hdmi-tx-ddc-clk-gpio: ddc clk pin
>> >> -- qcom,hdmi-tx-ddc-data-gpio: ddc data pin
>> >> -- qcom,hdmi-tx-hpd-gpio: hpd pin
>> >> +- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin
>> >> +- qcom,hdmi-tx-ddc-data-gpios: ddc data pin
>>
>> The driver doesn't appear to do anything other than set these gpios
>> high. Presumably you would ultimately do a bit-bang i2c bus for these?
>> At that point, aren't you going to want to use the i2c-gpio binding?
>>
>> I think this binding has bigger issues like why is it not using the
>> hdmi-connector binding which has a standard "hpd-gpios" property
>> already. I can't really single out this binding though. There's a lot
>> of crap when it comes to DRM related bindings.
>>
>
> Shouldn't these pins just be muxed to the hdmi block in pinctl?
>
> I know Rob had something wrt the detect pin (hdp), but the others should
> never be gpios(?) Isn't this just a reminder of the codeaurora tree
> gpio_requesting all pins "just to be safe"?

yeah, other than hdp pin (since the debounce logic in hdmi block
doesn't seem to be bulletproof), others we only request.. rest is
echo's of codeaurora..

some of it is probably not needed upstream.. and I'm ok w/ just not
documenting that in the bindings doc, or documenting as "unsupported
and going to go away so don't use", or whatever makes the most sense..
but would like to keep the driver code alive for the time being, since
it is easier for me to maintain a bit of cruft or dead code in the
upstream driver, than dealing with it on the backport (and I'm
inevitably the one who has to do both)

(Good news is that srini mentioned he was working on audio for
8064/ifc6410 upstream, so it at least seems like there is an end in
sight for backports to 3.4 device kernel..  3.10 stuff, I think we'll
have to live with a bit longer..)

BR,
-R


>> >> +- qcom,hdmi-tx-hpd-gpios: hpd pin
>> >>  - core-vdda-supply: phandle to supply regulator
>> >>  - hdmi-mux-supply: phandle to mux regulator
>> >>
>> >> +- qcom,hdmi-tx-ddc-clk-gpio: (deprecated) use
>> >> +     "qcom,hdmi-tx-ddc-clk-gpios" instead
>> >> +- qcom,hdmi-tx-ddc-data-gpio: (deprecated) use
>> >> +     "qcom,hdmi-tx-ddc-data-gpios" instead
>> >> +- qcom,hdmi-tx-hpd-gpio: (deprecated) use
>> >> +     "qcom,hdmi-tx-hpd-gpios" instead
>> >> +
>> >>  Optional properties:
>> >> -- qcom,hdmi-tx-mux-en-gpio: hdmi mux enable pin
>> >> -- qcom,hdmi-tx-mux-sel-gpio: hdmi mux select pin
>> >> +- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin
>> >> +- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin
>> >> +
>> >> +- qcom,hdmi-tx-mux-en-gpio: (deprecated) use "qcom,hdmi-tx-mux-en-gpios"
>> >> +     instead
>> >> +- qcom,hdmi-tx-mux-sel-gpio: (deprecated) use "qcom,hdmi-tx-mux-sel-gpio"
>> >> +     instead
>> >>  - pinctrl-names: the pin control state names; should contain "default"
>> >>  - pinctrl-0: the default pinctrl state (active)
>> >>  - pinctrl-1: the "sleep" pinctrl state
>> >
>> > I don't see much use in listing that these properties are deprecated. We
>> > already have code to catch the deprecated names, so having them in the
>> > binding will at best be distracting.
>> >
>> > Anyway, I don't know if there's been any advice on this from the device
>> > tree bindings maintainers, so adding devicetree@vger.kernel.org for
>> > visibility.
>>
>> If they need to be maintained, then marking them deprecated is
>> perfectly fine IMO. Also, if the maintainers of platforms using this
>> are okay with it, you can just switch it. Given there are no in tree
>> dts files using this, it can be argued that there is no ABI.
>>
>
> Part of some dev trees floating around no-one should have used these. So
> I think we should just document the proper ones and have the drivers
> support Rob's old properties until he's comfortable dropping them.
>
> That way we have a documented way forward and Rob can run his backports
> of this code without forking it.
>
> Regards,
> Bjorn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel