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

[Mark Brown]

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.

Attachment: signature.asc
Description: Digital signature