LinuxLists.cc - [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

2014-11-15 06:57:14

Subject: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

The rt5677 codec driver looks for ACPI device ID "RT5677CE",
which is specified in coreboot. This patch allows platform
data to be obtained via ACPI

Signed-off-by: Ben Zhang <[email protected]>
---
sound/soc/codecs/rt5677.c | 52 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 5d317c68..384281d 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -20,6 +20,7 @@
#include <linux/i2c.h>
#include <linux/platform_device.h>
#include <linux/spi/spi.h>
+#include <linux/acpi.h>
#include <linux/firmware.h>
#include <linux/gpio.h>
#include <sound/core.h>
@@ -4525,6 +4526,43 @@ static int rt5677_parse_dt(struct rt5677_priv *rt5677, struct device_node *np)
return 0;
}

+#ifdef CONFIG_ACPI
+
+static unsigned long long rt5677_parse_acpi_entry(struct device *dev,
+ acpi_string name)
+{
+ acpi_handle handle = ACPI_HANDLE(dev);
+ unsigned long long val;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(handle, name, NULL, &val);
+ if (ACPI_FAILURE(status)) {
+ dev_err(dev, "Failed to parse ACPI entry %s, default to 0: %d\n",
+ name, status);
+ return 0;
+ }
+ return val;
+}
+
+static void rt5677_parse_acpi(struct rt5677_priv *rt5677, struct device *dev)
+{
+ rt5677->pdata.dmic2_clk_pin = (enum rt5677_dmic2_clk)
+ rt5677_parse_acpi_entry(dev, "DCLK");
+ rt5677->pdata.in1_diff = (bool)rt5677_parse_acpi_entry(dev, "IN1");
+ rt5677->pdata.in2_diff = (bool)rt5677_parse_acpi_entry(dev, "IN2");
+ rt5677->pdata.lout1_diff = (bool)rt5677_parse_acpi_entry(dev, "OUT1");
+ rt5677->pdata.lout2_diff = (bool)rt5677_parse_acpi_entry(dev, "OUT2");
+ rt5677->pdata.lout3_diff = (bool)rt5677_parse_acpi_entry(dev, "OUT3");
+ rt5677->pdata.jd1_gpio = rt5677_parse_acpi_entry(dev, "JD1");
+ rt5677->pdata.jd2_gpio = rt5677_parse_acpi_entry(dev, "JD2");
+ rt5677->pdata.jd3_gpio = rt5677_parse_acpi_entry(dev, "JD3");
+}
+#else
+static void rt5677_parse_acpi(struct rt5677_priv *rt5677, struct device *dev)
+{
+}
+#endif
+
static struct regmap_irq rt5677_irqs[] = {
[RT5677_IRQ_JD1] = {
.reg_offset = 0,
@@ -4604,6 +4642,7 @@ static int rt5677_i2c_probe(struct i2c_client *i2c,
if (pdata)
rt5677->pdata = *pdata;

+ rt5677->pow_ldo2 = -EINVAL;
if (i2c->dev.of_node) {
ret = rt5677_parse_dt(rt5677, i2c->dev.of_node);
if (ret) {
@@ -4611,8 +4650,8 @@ static int rt5677_i2c_probe(struct i2c_client *i2c,
ret);
return ret;
}
- } else {
- rt5677->pow_ldo2 = -EINVAL;
+ } else if (ACPI_HANDLE(&i2c->dev)) {
+ rt5677_parse_acpi(rt5677, &i2c->dev);
}

if (gpio_is_valid(rt5677->pow_ldo2)) {
@@ -4708,10 +4747,19 @@ static int rt5677_i2c_remove(struct i2c_client *i2c)
return 0;
}

+#ifdef CONFIG_ACPI
+static const struct acpi_device_id rt5677_acpi_id[] = {
+ { "RT5677CE", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, rt5677_acpi_id);
+#endif
+
static struct i2c_driver rt5677_i2c_driver = {
.driver = {
.name = "rt5677",
.owner = THIS_MODULE,
+ .acpi_match_table = ACPI_PTR(rt5677_acpi_id),
},
.probe = rt5677_i2c_probe,
.remove = rt5677_i2c_remove,
--
2.1.0.rc2.206.gedb03e5

2014-11-15 06:57:21

by Ben Zhang

[permalink] [raw]

Subject: [PATCH 2/2] ASoC: rt5677: add a platform config option for PDM clock divider

The PDM output clock can use a divider of 1/2/3/4 based on the system clock

Signed-off-by: Ben Zhang <[email protected]>
---
Documentation/devicetree/bindings/sound/rt5677.txt | 5 +++++
include/sound/rt5677.h | 8 ++++++++
sound/soc/codecs/rt5677.c | 7 +++++++
3 files changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/rt5677.txt b/Documentation/devicetree/bindings/sound/rt5677.txt
index 740ff77..141f3e1 100644
--- a/Documentation/devicetree/bindings/sound/rt5677.txt
+++ b/Documentation/devicetree/bindings/sound/rt5677.txt
@@ -27,6 +27,10 @@ Optional properties:
Boolean. Indicate MIC1/2 input and LOUT1/2/3 outputs are differential,
rather than single-ended.

+- realtek,pdm_clk_div
+ Select 0/1/2/3 as PDM clock divider 1/2/4/3 respectively.
+ PDM clock = system clock / PDM clock divider
+
- realtek,gpio-config
Array of six 8bit elements that configures GPIO.
0 - floating (reset value)
@@ -71,6 +75,7 @@ rt5677 {
realtek,pow-ldo2-gpio =
<&gpio TEGRA_GPIO(V, 3) GPIO_ACTIVE_HIGH>;
realtek,in1-differential = "true";
+ realtek,pdm_clk_div = <1> /* PDM clock = system clock / 2 */
realtek,gpio-config = /bits/ 8 <0 0 0 0 0 2>; /* pull up GPIO6 */
realtek,jd2-gpio = <3>; /* Enables Jack detection for GPIO6 */
};
diff --git a/include/sound/rt5677.h b/include/sound/rt5677.h
index d9eb7d8..368aa33 100644
--- a/include/sound/rt5677.h
+++ b/include/sound/rt5677.h
@@ -17,6 +17,12 @@ enum rt5677_dmic2_clk {
RT5677_DMIC_CLK2 = 1,
};

+enum rt5677_pdm_clk_div {
+ RT5677_PDM_CLK_DIV1 = 0,
+ RT5677_PDM_CLK_DIV2 = 1,
+ RT5677_PDM_CLK_DIV4 = 2,
+ RT5677_PDM_CLK_DIV3 = 3,
+};

struct rt5677_platform_data {
/* IN1/IN2/LOUT1/LOUT2/LOUT3 can optionally be differential */
@@ -27,6 +33,8 @@ struct rt5677_platform_data {
bool lout3_diff;
/* DMIC2 clock source selection */
enum rt5677_dmic2_clk dmic2_clk_pin;
+ /* System clock to PDM filter divider */
+ enum rt5677_pdm_clk_div pdm_clk_div;

/* configures GPIO, 0 - floating, 1 - pulldown, 2 - pullup */
u8 gpio_config[6];
diff --git a/sound/soc/codecs/rt5677.c b/sound/soc/codecs/rt5677.c
index 384281d..383cb61 100644
--- a/sound/soc/codecs/rt5677.c
+++ b/sound/soc/codecs/rt5677.c
@@ -4502,6 +4502,8 @@ static int rt5677_parse_dt(struct rt5677_priv *rt5677, struct device_node *np)
"realtek,lout2-differential");
rt5677->pdata.lout3_diff = of_property_read_bool(np,
"realtek,lout3-differential");
+ of_property_read_u32(np, "realtek,pdm_clk_div",
+ &rt5677->pdata.pdm_clk_div);

rt5677->pow_ldo2 = of_get_named_gpio(np,
"realtek,pow-ldo2-gpio", 0);
@@ -4548,6 +4550,8 @@ static void rt5677_parse_acpi(struct rt5677_priv *rt5677, struct device *dev)
{
rt5677->pdata.dmic2_clk_pin = (enum rt5677_dmic2_clk)
rt5677_parse_acpi_entry(dev, "DCLK");
+ rt5677->pdata.pdm_clk_div = (enum rt5677_pdm_clk_div)
+ rt5677_parse_acpi_entry(dev, "PCLK");
rt5677->pdata.in1_diff = (bool)rt5677_parse_acpi_entry(dev, "IN1");
rt5677->pdata.in2_diff = (bool)rt5677_parse_acpi_entry(dev, "IN2");
rt5677->pdata.lout1_diff = (bool)rt5677_parse_acpi_entry(dev, "OUT1");
@@ -4730,6 +4734,9 @@ static int rt5677_i2c_probe(struct i2c_client *i2c,
RT5677_GPIO5_DIR_OUT);
}

+ regmap_update_bits(rt5677->regmap, RT5677_PDM_DATA_CTRL1,
+ RT5677_PDM_DIV_MASK, rt5677->pdata.pdm_clk_div);
+
rt5677_init_gpio(i2c);
rt5677_irq_init(i2c);

--
2.1.0.rc2.206.gedb03e5

2014-11-25 12:09:33

On Tue, Nov 25, 2014 at 08:00:12AM -0800, Darren Hart wrote:
> On 11/25/14 04:11, Grant Likely wrote:

> > Also, since this patch is targeted at v3.19 or later, the
> > device-properties API should be used. Don't create something custom.

> Right. The ACPI/UEFI forum is managing the creation of new DSD bindings
> and ensuring they are documented online. I believe this is the... 3rd so
> far? So we're still optimizing the process. But yes, please, send the
> schema itself for review and let's get this documented and migrated over
> to _DSD.

To be clear the main reason I'm querying this is that it doesn't appear
to be a _DSD based binding at all (as far as I understand it, the API
it's using is from before the dawn of time or at least the dawn of git).

Given the design of _DSD is to share with DT and we already have device
tree bindings for the device we should be using, it's not clear to me if
we want to grind them all through UEFI and I suspect they'd be unhappy
if we tried but pretty much all audio CODECs are good candidates for use
with ACPI given the new hardware designs Intel have so if we are doing
it I ought to be bouncing everyone to UEFI forum.

Attachments:

(No filename) (1.15 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-11-25 18:33:07

by Darren Hart

[permalink] [raw]

Subject: Re: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

On 11/25/14 09:21, Mark Brown wrote:
> On Tue, Nov 25, 2014 at 08:00:12AM -0800, Darren Hart wrote:
>> On 11/25/14 04:11, Grant Likely wrote:
>
>>> Also, since this patch is targeted at v3.19 or later, the
>>> device-properties API should be used. Don't create something
>>> custom.
>
>> Right. The ACPI/UEFI forum is managing the creation of new DSD
>> bindings and ensuring they are documented online. I believe this
>> is the... 3rd so far? So we're still optimizing the process. But
>> yes, please, send the schema itself for review and let's get this
>> documented and migrated over to _DSD.
>
> To be clear the main reason I'm querying this is that it doesn't
> appear to be a _DSD based binding at all (as far as I understand
> it, the API it's using is from before the dawn of time or at least
> the dawn of git).
>
> Given the design of _DSD is to share with DT and we already have
> device tree bindings for the device we should be using, it's not
> clear to me if we want to grind them all through UEFI and I suspect
> they'd be unhappy if we tried but pretty much all audio CODECs are
> good candidates for use with ACPI given the new hardware designs
> Intel have so if we are doing it I ought to be bouncing everyone to
> UEFI forum.

Right, I realized between sending and driving into the office that my
statement might be construed this way. I meant *new* _DSD bindings
should go through the ACPI/UEFI forum. Where we can reuse DT bindings,
we should absolutely do that, agreed. We should still document this
and link to the DT binding so it can be referenced and used even when
Linux is not the target OS.

--
Darren Hart
Intel Open Source Technology Center

2014-11-25 18:37:44

by Liam Girdwood

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

On Tue, 2014-11-25 at 08:01 -0800, Darren Hart wrote:
>
> On 11/25/14 06:28, Liam Girdwood wrote:
> > On Tue, 2014-11-25 at 12:11 +0000, Grant Likely wrote:
> >> On Sat, Nov 15, 2014 at 6:56 AM, Ben Zhang <[email protected]> wrote:
> >>> The rt5677 codec driver looks for ACPI device ID "RT5677CE",
> >>> which is specified in coreboot. This patch allows platform
> >>> data to be obtained via ACPI
> >>>
> >>> Signed-off-by: Ben Zhang <[email protected]>
> >>
> >> This looks like an ideal time to talk about shared DT and ACPI driver
> >> bindings. This driver /already/ has a firmware binding. It is
> >> documented in the kernel under
> >> Documentation/bindings/sound/rt5677.txt. We now have a standard method
> >> for sharing bindings between DT and ACPI in the _DSD method[1].
> >> Support for DSD is in linux-next and getting merged into v3.19. This
> >> is exactly the case that _DSD should be used for passing additional
> >> data to the driver, and it should use the existing binding.
> >>
> >> [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> >>
> >> For a long time we've had the rule on DT that new bindings must be
> >> documented before we merge a patch. That rule I think has been a good
> >> one, even if it is a little chaoitc. I think when it comes to ACPI
> >> drivers that we should be requiring the same: Document the binding,
> >> either in the kernel as a DT binding, or point to somewhere else that
> >> has the binding documented.
> >>
> >> Also, since this patch is targeted at v3.19 or later, the
> >> device-properties API should be used. Don't create something custom.
> >>
> >
> > My sentiments exactly, there would be little point having bespoke device
> > properties for every single device. Btw, we also need to align here with
> > Windows too !
> >
>
> The Windows folks definitely know about _DSD (and helped define it), so
> this is a good opportunity to work through that process. Liam, do you
> have a good contact to start that discussion?
>

I do, I've contacted them off-list to align on this with Realtek.

Liam

2014-11-25 18:45:21

On Tue, Nov 25, 2014 at 11:07:06AM -0800, Darren Hart wrote:
> On 11/25/14 10:43, Mark Brown wrote:

> > Link from where - do we want to talk to the ACPI/UEFI forum and
> > figure out some kind of fast track process for them to add an
> > "it's already covered by DT, see here" entry to their database for
> > example? We also ought to work out how to make sure ACPI IDs are
> > registered there as well, should be possible to have something
> > simple as part of that.

> This is a current topic with the ACPI working group. We have the
> following document:

> http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf

> I don't know if we want to have a list of them here, or if a separate
> document is needed. The important point is that it is independent from

Seems to me like some indirection is going to be better rather than
having one big document if there's widespread adoption, avoids lots of
document churn.

> the ACPI specification itself so that it can be updated out of band
> with the specification, and not be subject to rather plodding pace
> that would imply.

> Rafael, I've missed several of these meetings unfortunately, and I'm
> not sure if we've closed on this point. Do you know?

OK, I'll see if any of the people at work have been following as well.
For now continuing to just document everything as DT bindings in the way
we are already seems like a sensible interim approach.

Attachments:

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

On Mon, Dec 01, 2014 at 11:16:31PM +0100, Rafael J. Wysocki wrote:
> On Monday, December 01, 2014 05:51:00 PM Mark Brown wrote:

> > The dream here is that people working on building systems, people
> > working on Windows drivers and people working on Linux drivers will at
> > some point be able to collaborate. If we're going to go off and do our
> > own thing for Linux without talking to anyone that's not really addressing
> > the issue.

> Well, that's already going on in the DT land, isn't it? It has been going on
> for quite a while AFAICS.

In theory (and where it's actually relevant in practice to at least some
extent) this stuff is all OS neutral, there's a definite willingness for
it to be so.

> > There's also the option that Windows drivers start using _DSD themselves
> > which is, I understand, the goal towards which the people working on at
> > least audio are heading.

> Technically, Windows driver writers can evaluate _DSD and handle the
> information the way we do, but I'm not sure if this is really convenient for
> them.

I'm not sure how convenient it is, though I'm reasonably sure a helper
library could make it so.

> We use _DSD, because we want drivers to work with DT as well with ACPI without
> adding special DT-specific or ACPI-specific code to them. The people who work
> on Windows drivers don't have this problem, so as I said, if they care about
> Linux at all, that may be a good enough motivation for them to look at _DSD,
> but if they don't, I honestly don't see why they would do that.

They care about getting properties out, or at least they should, and in
this market many of the device vendors care about Linux at least as much
as they do Windows (sometimes even more than they care about Windows,
there's overlap with the Android market).

> > Note that all this discussion is pretty much about drivers for single
> > devices which can be wired into the system in a flexible manner, even in
> > a Windows world you won't vary the device ID. At present we're quirking
> > on DMI.

> So the answer to that in my view is: Use _DSD and allocate your own device IDs
> for Windows drivers to bind to.

Right, so we circle back to the original question about documenting
those IDs and _DSD properties. :)

> > > > We also need a way of getting the word out to people that they should be
> > > > doing this (also a problem no matter if we use PRP0001 or something UEFI
> > > > specific).

> > > What do you mean by "something UEFI specific"?

> > Sorry, I mean ACPI specific (UEFI forum).

> Do you mean a special device ID of some sort, then?

No, just a regular one.

> > It's not just the device IDs you need, it's the properties too.

> I suppose you have some specific examples in mind that I may not be familiar
> with and we may spend an arbitrary amount of time speaking past each other. :-)

Things like Documentation/devicetree/bindings/sound/wm8962.txt (at least
the optional properties), basically "how is this device wired into the
board" properties.

> That's where we are today. Do you have any suggestions on what else we can do?

I'd like to see a space where people working with a device can publish
what they've done in terms of firmware binding for it in a manner that
might work for them; at present it seems like the UEFI forum is the best
place to start doing that, there's the start of a register and process
for updating it there at least.

Attachments:

(No filename) (3.34 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-12-01 22:34:30

On Thu, Dec 04, 2014 at 11:12:34AM +0000, Grant Likely wrote:
> On Sat, 29 Nov 2014 23:27:52 +0100

> > There basically are two ways around that. The first one is to have all
> > knowledge related to device IDs in drivers (which effectively is what
> > Windows does and which implies "board files" of sorts) and the second one is
> > to make it possible to use overlays on top of the existing ACPI tables that
> > will allow people to provide the properties expected by a more generic driver
> > (this way, if the vendor didn't care to provide _DSD, for example, in the
> > original ACPI tables, the system integrator would be able to use an overlay
> > in an initramfs or boot partition to amend them).

> Either approach amounts to pretty much the same thing. The
> kernel/distribution needs to carry around device specific driver data
> which is exactly how we've supported x86 hardware. Whether the data is
> in-kernel or in-userspace is kind of an implementation detail. :-)

This isn't entirely what we've doing for x86 up until now - there's always
been an expectation that the firmware will be setting things up to work
by default (or so we can read the setup back from the hardwaer when we
take over) and we're mostly just carrying quirk data when that doesn't
pan out.

> _DSD support is a different situation though. Even if Windows doesn't
> care to directly support _DSD, drivers are free to use _DSD properties.
> This is the scenario where the shared binding repository will be
> important.

Right, this would be ideal for the devices we're looking at.

Attachments:

(No filename) (1.53 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-12-04 16:46:43

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

On Thu, Dec 04, 2014 at 10:48:19AM +0000, Grant Likely wrote:
> On Tue, 25 Nov 2014 22:40:53 +0100

> > We absolutely need to start registering the existing bindings in there, but
> > that needs to be rate limited somehow, because the process may not be very
> > efficient to start with.

> Beyond having the document point to the existing DT binding
> documentation, I think this is a non-starter. It won't be helpful for
> anyone to have two separate repositories containing the same bindings.
> They will get out of sync and we will have pain as a result.

Right, I think we have to pick one and reference it from the other.
Having the one place be inside the Linux kernel doesn't seem super
awesome.

> For existing bindings we need to have a way to share the documentation,
> and I don't think we can even talk about whether it makes sense to
> migrate the documetation to the UEFI managed repo before the UEFI process is
> fully flushed out.

We definitely don't want to start dumping everything in immediately but
but starting the conversation to both provide urgency and requirements
and and be engaged in the process seems sensible. Let's try to take
advantage of the recent changes in UEFI governance to help shape the
firmware interfaces we'll be working with.

Attachments:

(No filename) (1.24 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2014-12-04 21:31:56

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [PATCH 1/2] ASoC: rt5677: Add ACPI device probing

On Thursday, December 04, 2014 10:48:19 AM Grant Likely wrote:
> On Tue, 25 Nov 2014 22:40:53 +0100
> , "Rafael J. Wysocki" <[email protected]>
> wrote:
> > On Tuesday, November 25, 2014 08:27:22 PM Mark Brown wrote:
> > >
> > > --ReaqsoxgOBHFXBhH
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Disposition: inline
> > >
> > > On Tue, Nov 25, 2014 at 09:31:27PM +0100, Rafael J. Wysocki wrote:
> > > > On Tuesday, November 25, 2014 11:07:06 AM Darren Hart wrote:
> > >
> > > > > This is a current topic with the ACPI working group. We have the
> > > > > following document:
> > >
> > > > > http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > >
> > > > This hasn't been discussed a lot at the meetings I attended.
> > >
> > > > The bindings management process is being set up within the UEFI Forum, but I'm
> > > > not sure if/how the existing DT bindings documented in the kernel tree are
> > > > going to be covered by it ATM.
> > >
> > > Al Stone (CCed) pointed me at the following two documents:
> > >
> > > http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf
> > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf
> > >
> > > (the first one being the actual process in so far as it exists). The
> > > process appears to be to mail requests in a specific format to the ASWG
> > > chairperson (the address is apparently supposed to be [email protected]).
> > > It looks like all the properties are expected to end up in one or more
> > > PDF files like the second one.
> > >
> > > My initial thought would be to require that we send any DT properties
> > > defined for devices with ACPI identifiers registered there and hope the
> > > volume doesn't DoS them.
> >
> > We absolutely need to start registering the existing bindings in there, but
> > that needs to be rate limited somehow, because the process may not be very
> > efficient to start with.
>
> Beyond having the document point to the existing DT binding
> documentation, I think this is a non-starter. It won't be helpful for
> anyone to have two separate repositories containing the same bindings.

If they *are* the same, it won't. But at least in some cases (eg. GPIO)
they won't be the same.

Also we need a way to say "this binding has been documented elsewhere, see XYZ
and this is how to translate the format" in the UEFI documentation for the
existing bindings that will be the same.

> They will get out of sync and we will have pain as a result.
>
> For existing bindings we need to have a way to share the documentation,
> and I don't think we can even talk about whether it makes sense to
> migrate the documetation to the UEFI managed repo before the UEFI process is
> fully flushed out.

Agreed.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.