On Fri, Nov 14, 2014 at 10:56:47PM -0800, Ben Zhang 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
This actually does two things - it adds the ACPI device probing and it adds the ACPI platform data. That last bit is a bit fun, I've added several of the people working on ACPI here since this is another variant on the whole ACPI platform data thing which probably needs addressing in the best practice dissemination which ought to be going on now.
+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)) {
So, here were defining what's essentially an ACPI property read API which uses something other than _DSD which is the way all the ACPI platform data specification for devices is apparently supposed to be going in order to reuse work between DT and ACPI.
+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");
Here we read a bunch of properties named with a most definitely non-DT idiom. The names here don't look great in general, in particular all the properties for differential outputs are just boolean flags for the output name which I'd expect to be flags saying if the output is in use or not rather than saying if it's in differential mode or single ended mode. Things like foo_DIFF would seem better.
My understanding is that best practice for ACPI is to use the new device_property_read_ APIs which idiomatically take DT style property names. However if there's BIOSs out there we need to support perhaps we just have to deal with this but judging from your e-mail address it seems this may be for a system intended to run Linux natively so perhaps that's not an issue.