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

On 10/08/15 13:38, Thierry Reding wrote:

On Mon, Aug 10, 2015 at 12:59:34PM +0100, Srinivas Kandagatla wrote:

...

This patch modifies the driver to support standard gpio properties along with deprecated properties. This will help us upstream and cleanup the non-standard properties over the time.

Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org

drivers/gpu/drm/msm/hdmi/hdmi.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index 8145362..e918889 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -339,19 +339,34 @@ static const struct of_device_id dt_match[] = { };

#ifdef CONFIG_OF +/* This code will be removed once we move to gpiod based calls */

Why don't you do this now instead of duplicating what is essentially already implemented in gpiolib?

One of the thing that Rob asked in his comments (http://www.spinics.net/lists/arm-kernel/msg437675.html) was to retain the support for old devices, moving to gpiod ATM would break such devices as they are still using legacy gpiolib and its naming.

If Rob is ok to drop gpio properties which does not have "-gpio" or "-gpios" suffix then we can get rid of this function all together.

--srini

Thierry

On 10/08/15 14:26, Thierry Reding wrote:

On Mon, Aug 10, 2015 at 02:15:18PM +0100, Srinivas Kandagatla wrote:

...

On 10/08/15 13:38, Thierry Reding wrote:

...

On Mon, Aug 10, 2015 at 12:59:34PM +0100, Srinivas Kandagatla wrote:

...

This patch modifies the driver to support standard gpio properties along with deprecated properties. This will help us upstream and cleanup the non-standard properties over the time.

Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org

drivers/gpu/drm/msm/hdmi/hdmi.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index 8145362..e918889 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -339,19 +339,34 @@ static const struct of_device_id dt_match[] = { };

#ifdef CONFIG_OF +/* This code will be removed once we move to gpiod based calls */

Why don't you do this now instead of duplicating what is essentially already implemented in gpiolib?

One of the thing that Rob asked in his comments (http://www.spinics.net/lists/arm-kernel/msg437675.html) was to retain the support for old devices, moving to gpiod ATM would break such devices as they are still using legacy gpiolib and its naming.

If Rob is ok to drop gpio properties which does not have "-gpio" or "-gpios" suffix then we can get rid of this function all together.

If you make the switch to gpiod_*() APIs you'll get this for free. There's really no need for having a duplicate of what gpiod_get() already does for you.

Yes, you are right, my memory was over written on friday by looking at of_find_gpio() for long :-)

AFAIK, gpiod_get would also request the gpios so we might want to remove the additional gpio requests in the hdmi_connector.c too.

I will give that a try once again..

--srini

Thierry

On 10/08/15 13:49, Thierry Reding wrote:

On Mon, Aug 10, 2015 at 12:59:49PM +0100, Srinivas Kandagatla wrote:

...

This patch updates the bindings to discourage the usage of non standard clock names, this will help in projects focused on upstreaming.

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

Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org

Documentation/devicetree/bindings/drm/msm/hdmi.txt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt index 6dc202e..6fbfdd8 100644 --- a/Documentation/devicetree/bindings/drm/msm/hdmi.txt +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt @@ -12,16 +12,16 @@ Required properties:

• clocks: device clocks
• clock-names: Corresponding name for each entry in the clocks property.

for "qcom,hdmi-tx-8960" compatible names should be

• "core_clk"
• "master_iface_clk"
• "slave_iface_clk"

• "core_clk" is deprecated, use "core" instead

• "master_iface_clk" is deprecated, use "master_iface" instead

• "slave_iface_clk" is deprecated, use "slave_iface" instead

  for "qcom,hdmi-tx-8084" and "qcom,hdmi-tx-8074" compatible names should be

• "extp_clk"
• "alt_iface_clk"
• "iface_clk"
• "core_clk"
• "mdp_core_clk"

• "extp_clk" is deprecated, use "extp" instead
• "alt_iface_clk" is deprecated, use "alt_iface" intstead
• "iface_clk" is deprecated, use "iface" instead
• "core_clk" is deprecated, use "core" instead
• "mdp_core_clk" is deprecated, use "mdp_core" instead

Shouldn't there be a driver counterpart of this to accept the new names?

Driver changes are in this same series "[PATCH RFC 5/5] drm/msm/hdmi: remove _clk suffix from clock names"(https://lkml.org/lkml/2015/8/10/453)

--srini

Otherwise people could be switching the DTS to the new value, but the driver won't find the clocks it's looking for.

Thierry

On 10/08/15 14:33, Thierry Reding wrote:

On Mon, Aug 10, 2015 at 02:18:15PM +0100, Srinivas Kandagatla wrote:

...

On 10/08/15 13:49, Thierry Reding wrote:

...

On Mon, Aug 10, 2015 at 12:59:49PM +0100, Srinivas Kandagatla wrote:

...

This patch updates the bindings to discourage the usage of non standard clock names, this will help in projects focused on upstreaming.

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

Signed-off-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org

Documentation/devicetree/bindings/drm/msm/hdmi.txt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt index 6dc202e..6fbfdd8 100644 --- a/Documentation/devicetree/bindings/drm/msm/hdmi.txt +++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt @@ -12,16 +12,16 @@ Required properties:

• clocks: device clocks
• clock-names: Corresponding name for each entry in the clocks property.

for "qcom,hdmi-tx-8960" compatible names should be

• "core_clk"
• "master_iface_clk"
• "slave_iface_clk"

• "core_clk" is deprecated, use "core" instead

• "master_iface_clk" is deprecated, use "master_iface" instead

• "slave_iface_clk" is deprecated, use "slave_iface" instead

  for "qcom,hdmi-tx-8084" and "qcom,hdmi-tx-8074" compatible names should be

• "extp_clk"
• "alt_iface_clk"
• "iface_clk"
• "core_clk"
• "mdp_core_clk"

• "extp_clk" is deprecated, use "extp" instead
• "alt_iface_clk" is deprecated, use "alt_iface" intstead
• "iface_clk" is deprecated, use "iface" instead
• "core_clk" is deprecated, use "core" instead
• "mdp_core_clk" is deprecated, use "mdp_core" instead

Shouldn't there be a driver counterpart of this to accept the new names?

Driver changes are in this same series "[PATCH RFC 5/5] drm/msm/hdmi: remove _clk suffix from clock names"(https://lkml.org/lkml/2015/8/10/453)

I don't have that patch in my inbox. It looks to be doing things backwards (look up the deprecated name first). I think it should be:

clk = devm_clk_get(dev, id); if (IS_ERR(clk)) {

If the clock controller is not ready yet, it would return EPROBE DEFER, which gets dropped here, as a result the driver would not be probed again.

Probably both of the error codes needs be checked before returning.

char clk_name[32];

snprintf(clk_name, sizeof(clk_name), "%s_clk", id);
clk = devm_clk_get(dev, clk_name);
if (IS_ERR(clk))
	return clk;

}

Also note how I've dropped the ERR_CAST(), that's not useful here because you aren't actually casting, but simply returning clk.

Thierry