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

[John Keeping]

On Tue, 16 Aug 2016 18:20:06 +0100, Mark Brown wrote:

> On Sun, Aug 14, 2016 at 12:18:22PM +0100, John Keeping wrote:
> 
> > The Chromebook Pixel 2015 uses this codec with the ACPI ID RT5677CE, so
> > add an ACPI match table and support for reading properties from ACPI.  
> 
> This would be a lot easier to review with a concrete description of what
> "support for reading properties from ACPI" means and probably also split
> out a bit so that different things were being added separately.

OK, I'll have a think about this, although I'm not sure how much it can
be split up.  We can add the ACPI device property names in a separate
patch and maybe add the match table as a final step, but the GPIO
mapping and probe changes depend on each other.

> > +/* GPIO indexes defined by ACPI */
> > +enum {
> > +	RT5677_GPIO_PLUG_DET,
> > +	RT5677_GPIO_MIC_PRESENT_L,
> > +	RT5677_GPIO_HOTWORD_DET_L,
> > +	RT5677_GPIO_DSP_INT,
> > +	RT5677_GPIO_HP_AMP_SHDN_L,
> > +};  
> 
> If these are an ABI you should explicitly assign the values so that they
> can't get remapped by future edits.  If they're not an ABI I don't
> understand the comment.

Yes, these are ABI.  I'll add explicit values.

> > +	if (ACPI_HANDLE(dev)) {
> > +		u32 val;
> > +
> > +		if (!device_property_read_u32(dev, "DCLK", &val))
> > +			rt5677->pdata.dmic2_clk_pin = val;
> > +
> > +		rt5677->pdata.in1_diff = device_property_read_bool(dev, "IN1");
> > +		rt5677->pdata.in2_diff = device_property_read_bool(dev, "IN2");  
> 
> What happens if someone makes a machine which uses the DT<->ACPI
> mappings (especially given that this is currently undocumented)?  That
> would not work which defeats the whole purpose of using the device
> property APIs.  Shouldn't we be accepting either property?

I'm not sure we want to accept undocumented properties in the DT case.
I went looking for other drivers that have different property names for
ACPI and DT and the only example I could find is:

    drivers/net/ethernet/amd/xgbe/xgbe-main.c

In that case there are separate functions for parsing DT and ACPI
properties.  Having separate functions would have made this patch a lot
clearer, so if we keep the separation between DT and ACPI I'll make that
change for the next round.

The ideal might be to have something similar to the ACPI GPIO mapping
support so that the ACPI device property code could handle the different
names, but that feels like overkill if there are only two drivers that
use different property names in DT and ACPI.