| Message ID | 1430833322-13531-3-git-send-email-bardliao@realtek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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/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 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 Fri, May 08, 2015 at 11:13:39AM +0300, Mika Westerberg wrote: > Looks like some of the context is lost but if I decode the above > right, you want to know if there is anything that corresponds to > of_get_named_gpio() in the generic property API? > You can use fwnode_get_named_gpiod() if you need to extract a GPIO > directly from firmware node. Otherwise you should be using > devm_gpiod_get() and friends. Right, almost certainly devm_gpiod_get() in this case as I've said several times now :(
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 :-)
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;
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