Hi Rob,
Almost every time when we submit the DT changes for hdmi,
we are asked why are we using non-standard dt properties.
This patchset marks the old style dt properties as deprecated,
and let the driver support latest *-gpios and clk names properties.
This should allow us to submit the DT patches with more standard
dt properites
Old style properties are still supported and will be removed
once we see no users for it.
I have tested these changes on IFC6410.
Thanks,
srini
Srinivas Kandagatla (5):
drm/msm/hdmi: deprecate non standard gpio properties.
drm/msm/hdmi: make use of standard gpio properties.
drm/msm/hdmi: update bindings to include clock-names.
drm/msm/hdmi: deprecate non standard clock-names
drm/msm/hdmi: remove _clk suffix from clock names.
Documentation/devicetree/bindings/drm/msm/hdmi.txt | 35 ++++++++++--
drivers/gpu/drm/msm/hdmi/hdmi.c | 64 +++++++++++++++++-----
2 files changed, 79 insertions(+), 20 deletions(-)
--
1.9.1
This patch updates the bindings to discourage the usage of non standard
gpio properites, 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 <[email protected]>
---
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
--
1.9.1
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 <[email protected]>
---
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 */
static int get_gpio(struct device *dev, struct device_node *of_node, const char *name)
{
+ char name2[32];
int gpio = of_get_named_gpio(of_node, name, 0);
- if (gpio < 0) {
- char name2[32];
- snprintf(name2, sizeof(name2), "%s-gpio", name);
- gpio = of_get_named_gpio(of_node, name2, 0);
- if (gpio < 0) {
- dev_err(dev, "failed to get gpio: %s (%d)\n",
- name, gpio);
- gpio = -1;
- }
- }
+
+ if (gpio_is_valid(gpio))
+ goto deprecated;
+
+ snprintf(name2, sizeof(name2), "%s-gpio", name);
+ gpio = of_get_named_gpio(of_node, name2, 0);
+
+ if (gpio_is_valid(gpio))
+ goto deprecated;
+
+
+ snprintf(name2, sizeof(name2), "%s-gpios", name);
+ gpio = of_get_named_gpio(of_node, name2, 0);
+
+ if (gpio_is_valid(gpio))
+ return gpio;
+
+ dev_err(dev, "failed to get gpio: %s (%d)\n", name, gpio);
+
+ return -1;
+
+deprecated:
+ dev_warn(dev, "binding deprecated for %s\n", name);
return gpio;
}
#endif
--
1.9.1
This patch updates the bindings to include the clock names used in the
driver, this would make it easy to add deprecated warning once we move
to use standard clock properties.
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
Documentation/devicetree/bindings/drm/msm/hdmi.txt | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/Documentation/devicetree/bindings/drm/msm/hdmi.txt b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
index acba581..6dc202e 100644
--- a/Documentation/devicetree/bindings/drm/msm/hdmi.txt
+++ b/Documentation/devicetree/bindings/drm/msm/hdmi.txt
@@ -10,6 +10,19 @@ Required properties:
- reg-names: "core_physical"
- interrupts: The interrupt signal from the hdmi block.
- 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"
+
+ 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"
+
See ../clocks/clock-bindings.txt for details.
- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin
- qcom,hdmi-tx-ddc-data-gpios: ddc data pin
--
1.9.1
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 <[email protected]>
---
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
See ../clocks/clock-bindings.txt for details.
- qcom,hdmi-tx-ddc-clk-gpios: ddc clk pin
--
1.9.1
This patch modifies the driver to support clock names without _clk
suffix, usage of clk names with _clk suffix seems to be non-standard and
picked up everytime in DT patch review.
So lets fix this and make the other clk names deprecated till we decide
to remove them forever.
Signed-off-by: Srinivas Kandagatla <[email protected]>
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index e918889..c454d32 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -69,6 +69,24 @@ static void hdmi_destroy(struct hdmi *hdmi)
platform_set_drvdata(hdmi->pdev, NULL);
}
+static struct clk *hdmi_clk_get(struct device *dev, const char *id)
+{
+ char clk_name[32];
+ struct clk *clk;
+
+ snprintf(clk_name, sizeof(clk_name), "%s_clk", id);
+ clk = devm_clk_get(dev, clk_name);
+ if (IS_ERR(clk)) {
+ clk = devm_clk_get(dev, id);
+ if (IS_ERR(clk))
+ return ERR_CAST(clk);
+ } else {
+ dev_warn(dev, "binding deprecated for %s\n", clk_name);
+ }
+
+ return clk;
+}
+
/* construct hdmi at bind/probe time, grab all the resources. If
* we are to EPROBE_DEFER we want to do it here, rather than later
* at modeset_init() time
@@ -158,7 +176,7 @@ static struct hdmi *hdmi_init(struct platform_device *pdev)
for (i = 0; i < config->hpd_clk_cnt; i++) {
struct clk *clk;
- clk = devm_clk_get(&pdev->dev, config->hpd_clk_names[i]);
+ clk = hdmi_clk_get(&pdev->dev, config->hpd_clk_names[i]);
if (IS_ERR(clk)) {
ret = PTR_ERR(clk);
dev_err(&pdev->dev, "failed to get hpd clk: %s (%d)\n",
@@ -175,10 +193,11 @@ static struct hdmi *hdmi_init(struct platform_device *pdev)
ret = -ENOMEM;
goto fail;
}
+
for (i = 0; i < config->pwr_clk_cnt; i++) {
struct clk *clk;
- clk = devm_clk_get(&pdev->dev, config->pwr_clk_names[i]);
+ clk = hdmi_clk_get(&pdev->dev, config->pwr_clk_names[i]);
if (IS_ERR(clk)) {
ret = PTR_ERR(clk);
dev_err(&pdev->dev, "failed to get pwr clk: %s (%d)\n",
@@ -296,7 +315,7 @@ static struct hdmi_platform_config hdmi_tx_8660_config = {
};
static const char *hpd_reg_names_8960[] = {"core-vdda", "hdmi-mux"};
-static const char *hpd_clk_names_8960[] = {"core_clk", "master_iface_clk", "slave_iface_clk"};
+static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
static struct hdmi_platform_config hdmi_tx_8960_config = {
.phy_init = hdmi_phy_8960_init,
@@ -306,8 +325,8 @@ static struct hdmi_platform_config hdmi_tx_8960_config = {
static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
static const char *hpd_reg_names_8x74[] = {"hpd-gdsc", "hpd-5v"};
-static const char *pwr_clk_names_8x74[] = {"extp_clk", "alt_iface_clk"};
-static const char *hpd_clk_names_8x74[] = {"iface_clk", "core_clk", "mdp_core_clk"};
+static const char *pwr_clk_names_8x74[] = {"extp", "alt_iface"};
+static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core"};
static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0};
static struct hdmi_platform_config hdmi_tx_8074_config = {
--
1.9.1
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 <[email protected]>
> ---
> 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?
Thierry
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 <[email protected]>
> ---
> 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 [email protected] for
visibility.
Thierry
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 <[email protected]>
> ---
> 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?
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 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 <[email protected]>
>> ---
>> 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 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 <[email protected]>
>> ---
>> 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 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 <[email protected]>
> >>---
> >> 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.
Thierry
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 <[email protected]>
> >>---
> >> 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)) {
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
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 <[email protected]>
>>>> ---
>>>> 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
>
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 <[email protected]>
>>>> ---
>>>> 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 Mon, Aug 10, 2015 at 7:45 AM, Thierry Reding
<[email protected]> 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 <[email protected]>
>> ---
>> 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 [email protected] 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
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 [email protected] 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
On Mon, Aug 10, 2015 at 6:27 PM, Bjorn Andersson
<[email protected]> 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 [email protected] 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