[alsa-devel] [PATCH 1/3] ASoC: rt5645: fix add missing widget
"IF1 DAC0" and "IF1 DAC3" are used in rt5645_dapm_routes but missing in rt5645_dapm_widgets.
Signed-off-by: Oder Chiou oder_chiou@realtek.com Signed-off-by: Bard Liao bardliao@realtek.com --- sound/soc/codecs/rt5645.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index 7996c9c..a72d989 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -1697,8 +1697,10 @@ static const struct snd_soc_dapm_widget rt5645_dapm_widgets[] = { /* Digital Interface */ SND_SOC_DAPM_SUPPLY("I2S1", RT5645_PWR_DIG1, RT5645_PWR_I2S1_BIT, 0, NULL, 0), + SND_SOC_DAPM_PGA("IF1 DAC0", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_PGA("IF1 DAC1", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_PGA("IF1 DAC2", SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_PGA("IF1 DAC3", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_MUX("RT5645 IF1 DAC1 L Mux", SND_SOC_NOPM, 0, 0, &rt5645_if1_dac0_tdm_sel_mux), SND_SOC_DAPM_MUX("RT5645 IF1 DAC1 R Mux", SND_SOC_NOPM, 0, 0,
We can know if dmic is used by reading the value of dmic1_data_pin and dmic2_data_pin. Also IRQ must be used if codec JD or button detection function is used. So, dmic_en and en_jd_func can be remove from platform data.
Signed-off-by: Bard Liao bardliao@realtek.com --- include/sound/rt5645.h | 3 -- sound/soc/codecs/rt5645.c | 124 ++++++++++++++++++++++------------------------ sound/soc/codecs/rt5645.h | 2 + 3 files changed, 61 insertions(+), 68 deletions(-)
diff --git a/include/sound/rt5645.h b/include/sound/rt5645.h index 120d961..652cb9e 100644 --- a/include/sound/rt5645.h +++ b/include/sound/rt5645.h @@ -15,7 +15,6 @@ struct rt5645_platform_data { /* IN2 can optionally be differential */ bool in2_diff;
- bool dmic_en; unsigned int dmic1_data_pin; /* 0 = IN2N; 1 = GPIO5; 2 = GPIO11 */ unsigned int dmic2_data_pin; @@ -24,8 +23,6 @@ struct rt5645_platform_data { unsigned int hp_det_gpio; bool gpio_hp_det_active_high;
- /* true if codec's jd function is used */ - bool en_jd_func; unsigned int jd_mode; };
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index a72d989..e435680 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -2968,7 +2968,7 @@ static int rt5645_probe(struct snd_soc_codec *codec) snd_soc_update_bits(codec, RT5645_CHARGE_PUMP, 0x0300, 0x0200);
/* for JD function */ - if (rt5645->pdata.en_jd_func) { + if (rt5645->pdata.jd_mode) { snd_soc_dapm_force_enable_pin(&codec->dapm, "JD Power"); snd_soc_dapm_force_enable_pin(&codec->dapm, "LDO2"); snd_soc_dapm_sync(&codec->dapm); @@ -3111,10 +3111,8 @@ MODULE_DEVICE_TABLE(acpi, rt5645_acpi_match); static struct rt5645_platform_data *rt5645_pdata;
static struct rt5645_platform_data strago_platform_data = { - .dmic_en = true, - .dmic1_data_pin = -1, + .dmic1_data_pin = RT5645_DMIC1_DISABLE, .dmic2_data_pin = RT5645_DMIC_DATA_IN2P, - .en_jd_func = true, .jd_mode = 3, };
@@ -3214,83 +3212,79 @@ static int rt5645_i2c_probe(struct i2c_client *i2c, regmap_update_bits(rt5645->regmap, RT5645_IN2_CTRL, RT5645_IN_DF2, RT5645_IN_DF2);
- if (rt5645->pdata.dmic_en) { + if (rt5645->pdata.dmic1_data_pin || rt5645->pdata.dmic2_data_pin) { regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, RT5645_GP2_PIN_MASK, RT5645_GP2_PIN_DMIC1_SCL); + } + switch (rt5645->pdata.dmic1_data_pin) { + case RT5645_DMIC_DATA_IN2N: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_IN2N); + break;
- switch (rt5645->pdata.dmic1_data_pin) { - case RT5645_DMIC_DATA_IN2N: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_IN2N); - break; - - case RT5645_DMIC_DATA_GPIO5: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO5); - regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, - RT5645_GP5_PIN_MASK, RT5645_GP5_PIN_DMIC1_SDA); - break; - - case RT5645_DMIC_DATA_GPIO11: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO11); - regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, - RT5645_GP11_PIN_MASK, - RT5645_GP11_PIN_DMIC1_SDA); - break; + case RT5645_DMIC_DATA_GPIO5: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO5); + regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, + RT5645_GP5_PIN_MASK, RT5645_GP5_PIN_DMIC1_SDA); + break;
- default: - break; - } + case RT5645_DMIC_DATA_GPIO11: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_1_DP_MASK, RT5645_DMIC_1_DP_GPIO11); + regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, + RT5645_GP11_PIN_MASK, + RT5645_GP11_PIN_DMIC1_SDA); + break;
- switch (rt5645->pdata.dmic2_data_pin) { - case RT5645_DMIC_DATA_IN2P: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_IN2P); - break; + default: + break; + }
- case RT5645_DMIC_DATA_GPIO6: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO6); - regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, - RT5645_GP6_PIN_MASK, RT5645_GP6_PIN_DMIC2_SDA); - break; + switch (rt5645->pdata.dmic2_data_pin) { + case RT5645_DMIC_DATA_IN2P: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_IN2P); + break;
- case RT5645_DMIC_DATA_GPIO10: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO10); - regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, - RT5645_GP10_PIN_MASK, - RT5645_GP10_PIN_DMIC2_SDA); - break; + case RT5645_DMIC_DATA_GPIO6: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO6); + regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, + RT5645_GP6_PIN_MASK, RT5645_GP6_PIN_DMIC2_SDA); + break;
- case RT5645_DMIC_DATA_GPIO12: - regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, - RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO12); - regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, - RT5645_GP12_PIN_MASK, - RT5645_GP12_PIN_DMIC2_SDA); - break; + case RT5645_DMIC_DATA_GPIO10: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO10); + regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, + RT5645_GP10_PIN_MASK, + RT5645_GP10_PIN_DMIC2_SDA); + break;
- default: - break; - } + case RT5645_DMIC_DATA_GPIO12: + regmap_update_bits(rt5645->regmap, RT5645_DMIC_CTRL1, + RT5645_DMIC_2_DP_MASK, RT5645_DMIC_2_DP_GPIO12); + regmap_update_bits(rt5645->regmap, RT5645_GPIO_CTRL1, + RT5645_GP12_PIN_MASK, + RT5645_GP12_PIN_DMIC2_SDA); + break;
+ default: + break; }
- if (rt5645->pdata.en_jd_func) { + if (rt5645->pdata.jd_mode) { regmap_update_bits(rt5645->regmap, RT5645_GEN_CTRL3, - RT5645_IRQ_CLK_GATE_CTRL, RT5645_IRQ_CLK_GATE_CTRL); + RT5645_IRQ_CLK_GATE_CTRL, + RT5645_IRQ_CLK_GATE_CTRL); regmap_update_bits(rt5645->regmap, RT5645_IN1_CTRL1, - RT5645_CBJ_BST1_EN, RT5645_CBJ_BST1_EN); + RT5645_CBJ_BST1_EN, RT5645_CBJ_BST1_EN); regmap_update_bits(rt5645->regmap, RT5645_JD_CTRL3, - RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL, - RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL); + RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL, + RT5645_JD_CBJ_EN | RT5645_JD_CBJ_POL); regmap_update_bits(rt5645->regmap, RT5645_MICBIAS, - RT5645_IRQ_CLK_INT, RT5645_IRQ_CLK_INT); - } - - if (rt5645->pdata.jd_mode) { + RT5645_IRQ_CLK_INT, RT5645_IRQ_CLK_INT); regmap_update_bits(rt5645->regmap, RT5645_IRQ_CTRL2, RT5645_IRQ_JD_1_1_EN, RT5645_IRQ_JD_1_1_EN); regmap_update_bits(rt5645->regmap, RT5645_GEN_CTRL3, diff --git a/sound/soc/codecs/rt5645.h b/sound/soc/codecs/rt5645.h index c204861..9ec4e89 100644 --- a/sound/soc/codecs/rt5645.h +++ b/sound/soc/codecs/rt5645.h @@ -2145,6 +2145,7 @@ enum { };
enum { + RT5645_DMIC1_DISABLE, RT5645_DMIC_DATA_IN2P, RT5645_DMIC_DATA_GPIO6, RT5645_DMIC_DATA_GPIO10, @@ -2152,6 +2153,7 @@ enum { };
enum { + RT5645_DMIC2_DISABLE, RT5645_DMIC_DATA_IN2N, RT5645_DMIC_DATA_GPIO5, RT5645_DMIC_DATA_GPIO11,
On Tue, May 05, 2015 at 09:42:01PM +0800, Bard Liao wrote:
We can know if dmic is used by reading the value of dmic1_data_pin and dmic2_data_pin. Also IRQ must be used if codec JD or button detection function is used. So, dmic_en and en_jd_func can be remove from platform data.
Applied, thanks.
Modify the RT5645 driver to parse platform data from device tree. Write a DT binding document to describe those properties.
Signed-off-by: Bard Liao bardliao@realtek.com --- Documentation/devicetree/bindings/sound/rt5645.txt | 72 ++++++++++++++++++++++ sound/soc/codecs/rt5645.c | 35 +++++++++++ 2 files changed, 107 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/rt5645.txt
diff --git a/Documentation/devicetree/bindings/sound/rt5645.txt b/Documentation/devicetree/bindings/sound/rt5645.txt new file mode 100644 index 0000000..f676c22 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/rt5645.txt @@ -0,0 +1,72 @@ +RT5650/RT5645 audio CODEC + +This device supports I2C only. + +Required properties: + +- compatible : One of "realtek,rt5645" or "realtek,rt5650". + +- reg : The I2C address of the device. + +- interrupts : The CODEC's interrupt output. + +Optional properties: + +- realtek,in2-differential + Boolean. Indicate MIC2 input are differential, rather than single-ended. + +- realtek,dmic1-data-pin + 0: dmic1 is not used + 1: using IN2P pin as dmic1 data pin + 2: using GPIO6 pin as dmic1 data pin + 3: using GPIO10 pin as dmic1 data pin + 4: using GPIO12 pin as dmic1 data pin + +- realtek,dmic2-data-pin + 0: dmic2 is not used + 1: using IN2N pin as dmic2 data pin + 2: using GPIO5 pin as dmic2 data pin + 3: using GPIO11 pin as dmic2 data pin + +- realtek,hp-det-gpio : The GPIO that indicate a JD event is triggered. + +- realtek,gpio-hp_det-active_high + Boolean. Indicate if hp-det-gpio is active high or not. + +-- realtek,jd-mode : The JD mode of rt5645/rt5650 + 0 : rt5645/rt5650 JD function is not used + 1 : Mode-0 (VDD=3.3V), two port jack detection + 2 : Mode-1 (VDD=3.3V), one port jack detection + 3 : Mode-2 (VDD=1.8V), one port jack detection + +Pins on the device (for linking into audio routes) for RT5645/RT5650: + + * DMIC L1 + * DMIC R1 + * DMIC L2 + * DMIC R2 + * IN1P + * IN1N + * IN2P + * IN2N + * Haptic Generator + * HPOL + * HPOR + * LOUTL + * LOUTR + * PDM1L + * PDM1R + * SPOL + * SPOR + +Example: + +codec: rt5650@1a { + compatible = "realtek,rt5650"; + reg = <0x1a>; + interrupt-parent = <&gpio>; + interrupts = <7 IRQ_TYPE_EDGE_FALLING>; + realtek,dmic-en = "true"; + realtek,en-jd-func = "true"; + realtek,jd-mode = <3>; +}; diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index e435680..36e22b6 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3134,6 +3134,37 @@ static struct dmi_system_id dmi_platform_intel_braswell[] = { { } };
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device_node *np) +{ + rt5645->pdata.in2_diff = of_property_read_bool(np, + "realtek,in2-differential"); + of_property_read_u32(np, + "realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin); + of_property_read_u32(np, + "realtek,dmic2-data-pin", &rt5645->pdata.dmic2_data_pin); + rt5645->pdata.hp_det_gpio = of_get_named_gpio(np, + "realtek,hp-det-gpio", 0); + rt5645->pdata.gpio_hp_det_active_high = of_property_read_bool(np, + "realtek,gpio-hp_det-active_high"); + of_property_read_u32(np, + "realtek,jd-mode", &rt5645->pdata.jd_mode); + /* + * HP_DET_GPIO is optional (it may be statically tied on the board). + * -ENOENT means that the property doesn't exist, i.e. there is no + * GPIO, so is not an error. Any other error code means the property + * exists, but could not be parsed. + */ + + if (!rt5645->pdata.hp_det_gpio) + rt5645->pdata.hp_det_gpio = -EINVAL; + + if (!gpio_is_valid(rt5645->pdata.hp_det_gpio) && + (rt5645->pdata.hp_det_gpio != -ENOENT)) + return rt5645->pdata.hp_det_gpio; + + return 0; +} + static int rt5645_i2c_probe(struct i2c_client *i2c, const struct i2c_device_id *id) { @@ -3153,6 +3184,10 @@ static int rt5645_i2c_probe(struct i2c_client *i2c,
if (pdata) { rt5645->pdata = *pdata; + } else if (i2c->dev.of_node) { + ret = rt5645_parse_dt(rt5645, i2c->dev.of_node); + if (ret) + return ret; } else { if (dmi_check_system(dmi_platform_intel_braswell)) { rt5645->pdata = *rt5645_pdata;
On Tue, 2015-05-05 at 21:42 +0800, Bard Liao wrote:
Modify the RT5645 driver to parse platform data from device tree. Write a DT binding document to describe those properties.
Signed-off-by: Bard Liao bardliao@realtek.com
Documentation/devicetree/bindings/sound/rt5645.txt | 72 ++++++++++++++++++++++ sound/soc/codecs/rt5645.c | 35 +++++++++++ 2 files changed, 107 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/rt5645.txt
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index e435680..36e22b6 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3134,6 +3134,37 @@ static struct dmi_system_id dmi_platform_intel_braswell[] = { { } };
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct device_node *np) +{
- rt5645->pdata.in2_diff = of_property_read_bool(np,
"realtek,in2-differential");- of_property_read_u32(np,
"realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin);
We should really be using device_property_() instead of of_property_() APIs since we will have to support both DT and ACPI properties.
Liam
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Tuesday, May 05, 2015 10:46 PM To: Bard Liao Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; alsa-devel@alsa-project.org; lars@metafoo.de; zhengxing@rock-chips.com; yang.a.fang@intel.com; koro.chen@mediatek.com; John Lin; Flove Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: rt5645: add device tree support
On Tue, 2015-05-05 at 21:42 +0800, Bard Liao wrote:
Modify the RT5645 driver to parse platform data from device tree. Write a DT binding document to describe those properties.
Signed-off-by: Bard Liao bardliao@realtek.com
Documentation/devicetree/bindings/sound/rt5645.txt | 72
++++++++++++++++++++++
sound/soc/codecs/rt5645.c | 35
+++++++++++
2 files changed, 107 insertions(+) create mode 100644
Documentation/devicetree/bindings/sound/rt5645.txt
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index e435680..36e22b6 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3134,6 +3134,37 @@ static struct dmi_system_id
dmi_platform_intel_braswell[] = {
{ } };
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct +device_node *np) {
- rt5645->pdata.in2_diff = of_property_read_bool(np,
"realtek,in2-differential");- of_property_read_u32(np,
"realtek,dmic1-data-pin", &rt5645->pdata.dmic1_data_pin);We should really be using device_property_() instead of of_property_() APIs since we will have to support both DT and ACPI properties.
We will modify it and send a new version patch for it.
Thanks.
Liam
------Please consider the environment before printing this e-mail.
-----Original Message----- From: Bard Liao Sent: Wednesday, May 06, 2015 2:43 PM To: 'Liam Girdwood' Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; alsa-devel@alsa-project.org; lars@metafoo.de; zhengxing@rock-chips.com; yang.a.fang@intel.com; koro.chen@mediatek.com; John Lin; Flove Subject: RE: [alsa-devel] [PATCH 3/3] ASoC: rt5645: add device tree support
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Tuesday, May 05, 2015 10:46 PM To: Bard Liao Cc: broonie@kernel.org; lgirdwood@gmail.com; Oder Chiou; alsa-devel@alsa-project.org; lars@metafoo.de; zhengxing@rock-chips.com; yang.a.fang@intel.com; koro.chen@mediatek.com; John Lin; Flove Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: rt5645: add device tree support
On Tue, 2015-05-05 at 21:42 +0800, Bard Liao wrote:
Modify the RT5645 driver to parse platform data from device tree. Write a DT binding document to describe those properties.
Signed-off-by: Bard Liao bardliao@realtek.com
Documentation/devicetree/bindings/sound/rt5645.txt | 72
++++++++++++++++++++++
sound/soc/codecs/rt5645.c | 35
+++++++++++
2 files changed, 107 insertions(+) create mode 100644
Documentation/devicetree/bindings/sound/rt5645.txt
diff --git a/sound/soc/codecs/rt5645.c b/sound/soc/codecs/rt5645.c index e435680..36e22b6 100644 --- a/sound/soc/codecs/rt5645.c +++ b/sound/soc/codecs/rt5645.c @@ -3134,6 +3134,37 @@ static struct dmi_system_id
dmi_platform_intel_braswell[] = {
{ } };
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct +device_node *np) {
- rt5645->pdata.in2_diff = of_property_read_bool(np,
"realtek,in2-differential");- of_property_read_u32(np,
"realtek,dmic1-data-pin",&rt5645->pdata.dmic1_data_pin);
We should really be using device_property_() instead of of_property_() APIs since we will have to support both DT and ACPI properties.
Unfortunately, I can't find a way to test it. Is that ok we just replace all of_property_ with device_property_? Also, is there any corresponding API for of_get_named_gpio? Or we can replace it with device_property_read_u32? I tried the change above, and it can build. However I don't know if it can work.
We will modify it and send a new version patch for it.
Thanks.
Liam
------Please consider the environment before printing this e-mail.
On Thu, 2015-05-07 at 05:34 +0000, Bard Liao wrote:
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct +device_node *np) {
- rt5645->pdata.in2_diff = of_property_read_bool(np,
"realtek,in2-differential");- of_property_read_u32(np,
"realtek,dmic1-data-pin",&rt5645->pdata.dmic1_data_pin);
We should really be using device_property_() instead of
of_property_()
APIs since we will have to support both DT and ACPI properties.
Unfortunately, I can't find a way to test it.
device_property() API calls abstract the device tree calls and ACPI calls. So testing with your DT based HW will work.
Is that ok we just replace all of_property_ with device_property_?
Yes, that's the intention.
Also, is there any corresponding API for of_get_named_gpio? Or we can replace it with device_property_read_u32? I tried the change above, and it can build. However I don't know if it can work.
Oh, I think that's a question for Rafael. I think the intention is to have a 1:1 mapping between the APIs so that there are no gaps ?
Liam
On 5/7/2015 1:26 PM, Liam Girdwood wrote:
On Thu, 2015-05-07 at 05:34 +0000, Bard Liao wrote:
+static int rt5645_parse_dt(struct rt5645_priv *rt5645, struct +device_node *np) {
- rt5645->pdata.in2_diff = of_property_read_bool(np,
"realtek,in2-differential");- of_property_read_u32(np,
"realtek,dmic1-data-pin",&rt5645->pdata.dmic1_data_pin);
We should really be using device_property_() instead of
of_property_()
APIs since we will have to support both DT and ACPI properties.
Unfortunately, I can't find a way to test it.
device_property() API calls abstract the device tree calls and ACPI calls. So testing with your DT based HW will work.
Is that ok we just replace all of_property_ with device_property_?
Yes, that's the intention.
Also, is there any corresponding API for of_get_named_gpio? Or we can replace it with device_property_read_u32? I tried the change above, and it can build. However I don't know if it can work.
Oh, I think that's a question for Rafael. I think the intention is to have a 1:1 mapping between the APIs so that there are no gaps ?
Yes, that's the idea, but then the GPIO maintainers would like to clean up the API too.
Mika is more familiar with the GPIO area than I am.
Mika, can you please help us here?
Rafael
On Thu, May 07, 2015 at 05:34:27AM +0000, Bard Liao wrote:
We should really be using device_property_() instead of of_property_() APIs since we will have to support both DT and ACPI properties.
Unfortunately, I can't find a way to test it.
Why can't you test it - weren't you testing your original version of the code?
Is that ok we just replace all of_property_ with device_property_? Also, is there any corresponding API for of_get_named_gpio? Or we can replace it with device_property_read_u32? I tried the change above, and it can build. However I don't know if it can work.
You shouldn't be using of_get_named_gpio() for DT stuff either, use gpiod_get() which follows the usual pattern of taking a string which is used to do the lookup with whatever firmware is in use.
On 5/7/15 7:51 AM, Mark Brown wrote:
On Thu, May 07, 2015 at 05:34:27AM +0000, Bard Liao wrote:
We should really be using device_property_() instead of of_property_() APIs since we will have to support both DT and ACPI properties.
Unfortunately, I can't find a way to test it.
Why can't you test it - weren't you testing your original version of the code?
You can test it on an platform using device tree, the device_property_ code will fetch the same information as before. If you want to test on an ACPI platform, I can help with some examples (off-list) showing how to override the DSDT table and to add those properties in an ASL device entry. You may not be able to test all the configurations but at least you can add basic checks that show the data is passed from ACPI to your driver. I used all this stuff for a proof of concept on AsusT100 where the audio routing in the machine driver is passed from ACPI.
Is that ok we just replace all of_property_ with device_property_? Also, is there any corresponding API for of_get_named_gpio? Or we can replace it with device_property_read_u32? I tried the change above, and it can build. However I don't know if it can work.
You shouldn't be using of_get_named_gpio() for DT stuff either, use gpiod_get() which follows the usual pattern of taking a string which is used to do the lookup with whatever firmware is in use.
But to Bard's credit the use of_get_named_gpio() is pretty common - i see 18 occurrences in soc/codecs alone, others will have the same problem... we talked internally (RafaelW, Darren Hart, LiamG and me) about reaching out to gpio and audio maintainers and aligning some sort of coordinated change to the gpiod framework w/ guidance to developers, did that thread start?
On Thu, May 07, 2015 at 01:28:39PM -0500, Pierre-Louis Bossart wrote:
On 5/7/15 7:51 AM, Mark Brown wrote:
You shouldn't be using of_get_named_gpio() for DT stuff either, use gpiod_get() which follows the usual pattern of taking a string which is used to do the lookup with whatever firmware is in use.
But to Bard's credit the use of_get_named_gpio() is pretty common - i see 18 occurrences in soc/codecs alone, others will have the same problem... we talked internally (RafaelW, Darren Hart, LiamG and me) about reaching out to gpio and audio maintainers and aligning some sort of coordinated change to the gpiod framework w/ guidance to developers, did that thread start?
I'm not sure there's any particular need for coordination here, the APIs are in place already - the gpiod_ APIs will already transparently look up both ACPI and DT (see __gpiod_get_index() for the implementation) and new drivers should really be using gpiod_ anyway regardless of trying to do both ACPI and DT.
On 5/7/15 1:36 PM, Mark Brown wrote:
On Thu, May 07, 2015 at 01:28:39PM -0500, Pierre-Louis Bossart wrote:
On 5/7/15 7:51 AM, Mark Brown wrote:
You shouldn't be using of_get_named_gpio() for DT stuff either, use gpiod_get() which follows the usual pattern of taking a string which is used to do the lookup with whatever firmware is in use.
But to Bard's credit the use of_get_named_gpio() is pretty common - i see 18 occurrences in soc/codecs alone, others will have the same problem... we talked internally (RafaelW, Darren Hart, LiamG and me) about reaching out to gpio and audio maintainers and aligning some sort of coordinated change to the gpiod framework w/ guidance to developers, did that thread start?
I'm not sure there's any particular need for coordination here, the APIs are in place already - the gpiod_ APIs will already transparently look up both ACPI and DT (see __gpiod_get_index() for the implementation) and new drivers should really be using gpiod_ anyway regardless of trying to do both ACPI and DT.
Agree, but there wasn't a clear message provided to the readers of this mailing list. A 'should' is a recommendation that provides no real incentive to move to the new gpiod framework. The message would be clearer if maintainers stated that new contributions using of_get_named_gpio() will no longer be merged in the mainline (maybe after a specific milestone). That would set the direction for everyone.
On Fri, May 08, 2015 at 08:30:52AM -0500, Pierre-Louis Bossart wrote:
Agree, but there wasn't a clear message provided to the readers of this mailing list. A 'should' is a recommendation that provides no real incentive to move to the new gpiod framework. The message would be clearer if maintainers stated that new contributions using of_get_named_gpio() will no longer be merged in the mainline (maybe after a specific milestone). That would set the direction for everyone.
That's what's been happening...
On 5/8/15 11:46 AM, Mark Brown wrote:
On Fri, May 08, 2015 at 08:30:52AM -0500, Pierre-Louis Bossart wrote:
Agree, but there wasn't a clear message provided to the readers of this mailing list. A 'should' is a recommendation that provides no real incentive to move to the new gpiod framework. The message would be clearer if maintainers stated that new contributions using of_get_named_gpio() will no longer be merged in the mainline (maybe after a specific milestone). That would set the direction for everyone.
That's what's been happening...
We are in agreement. I wasn't trying to argue but make sure everyone noticed this change. I saw a commit with of_get_named_gpio in sound/soc as recent as December 30, 2014 and we talked to 3rd parties unaware of this change. Now they are :-)
participants (5)
-
Bard Liao -
Liam Girdwood -
Mark Brown -
Pierre-Louis Bossart -
Rafael J. Wysocki