ASoC: rt286: Add API to set supported jack type
diff mbox

Message ID 1407217970-20126-1-git-send-email-bardliao@realtek.com
State New, archived
Headers show

Commit Message

Bard Liao Aug. 5, 2014, 5:52 a.m. UTC
From: Bard Liao <bardliao@realtek.com>

RT286 need to configure the jack type (combo or non-combo) according to the
HW design. This function is given a easy way to set the correct jacy type
for those without platform data supported.

Signed-off-by: Bard Liao <bardliao@realtek.com>
---
 sound/soc/codecs/rt286.c | 33 +++++++++++++++++++++++++++++++++
 sound/soc/codecs/rt286.h |  7 +++++++
 2 files changed, 40 insertions(+)

Comments

Mark Brown Aug. 6, 2014, 8:54 p.m. UTC | #1
On Tue, Aug 05, 2014 at 01:52:50PM +0800, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> RT286 need to configure the jack type (combo or non-combo) according to the
> HW design. This function is given a easy way to set the correct jacy type
> for those without platform data supported.

If this is coming from the hardware design I'd expect this to be
configured via platform data or firmware data rather than with an API.
Why do it this way?
Bard Liao Aug. 7, 2014, 1:43 a.m. UTC | #2
> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Thursday, August 07, 2014 4:54 AM
> To: Bard Liao
> Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; lars@metafoo.de;
> Flove; Oder Chiou
> Subject: Re: [PATCH] ASoC: rt286: Add API to set supported jack type
> 
> On Tue, Aug 05, 2014 at 01:52:50PM +0800, bardliao@realtek.com wrote:
> > From: Bard Liao <bardliao@realtek.com>
> >
> > RT286 need to configure the jack type (combo or non-combo) according
> > to the HW design. This function is given a easy way to set the correct
> > jacy type for those without platform data supported.
> 
> If this is coming from the hardware design I'd expect this to be configured
> via platform data or firmware data rather than with an API.
> Why do it this way?

Actually, this patch is for Intel platform which doesn't support platform data
yet. That's why we create an API so they can call the API in machine driver.

> 
> ------Please consider the environment before printing this e-mail.
Mark Brown Aug. 7, 2014, 5:31 p.m. UTC | #3
On Thu, Aug 07, 2014 at 01:43:24AM +0000, Bard Liao wrote:

> > If this is coming from the hardware design I'd expect this to be configured
> > via platform data or firmware data rather than with an API.
> > Why do it this way?

> Actually, this patch is for Intel platform which doesn't support platform data
> yet. That's why we create an API so they can call the API in machine driver.

So, this is something that the people wanting to use ACPI in platforms
with devices like this are going to have to deal with - we need a way of
getting configuration to devices.  Traditionally small tweaks have been
done based on DMI information but that's not at all scalable.  I believe
newer ACPI specs are trying to accomodate this due to the expanding
range of systems supporting ACPI.  Punting to the machine driver just
moves the problem around, it doesn't really address it.

I've CCed in Graeme Gregory who's been writing some guidelines for this
sort of thing in the context of ARMv8 serviers - the most recent patch
is:

   http://thread.gmane.org/gmane.linux.acpi.devel/69757

The advice there looks good to me - see the "Device Enumeration"
section.
Liam Girdwood Aug. 11, 2014, 9:59 a.m. UTC | #4
On Thu, 2014-08-07 at 18:31 +0100, Mark Brown wrote:
> On Thu, Aug 07, 2014 at 01:43:24AM +0000, Bard Liao wrote:
> 
> > > If this is coming from the hardware design I'd expect this to be configured
> > > via platform data or firmware data rather than with an API.
> > > Why do it this way?
> 
> > Actually, this patch is for Intel platform which doesn't support platform data
> > yet. That's why we create an API so they can call the API in machine driver.
> 
> So, this is something that the people wanting to use ACPI in platforms
> with devices like this are going to have to deal with - we need a way of
> getting configuration to devices.  Traditionally small tweaks have been
> done based on DMI information but that's not at all scalable.  I believe
> newer ACPI specs are trying to accomodate this due to the expanding
> range of systems supporting ACPI.  Punting to the machine driver just
> moves the problem around, it doesn't really address it.
> 

Unfortunately this will be needed for some Broadwell based BIOSes since
the timing of the ACPI spec/implementation has meant that some Broadwell
BIOSes will contain the data whilst most others wont atm :( 

> I've CCed in Graeme Gregory who's been writing some guidelines for this
> sort of thing in the context of ARMv8 serviers - the most recent patch
> is:
> 
>    http://thread.gmane.org/gmane.linux.acpi.devel/69757
> 
> The advice there looks good to me - see the "Device Enumeration"
> section.

This is indeed good, just too late for the initial Broadwell BIOSes that
wont have this _DSD field for the codec (it will be hard coded for
Windows anyway).

Liam
Mark Brown Aug. 11, 2014, 12:24 p.m. UTC | #5
On Mon, Aug 11, 2014 at 10:59:45AM +0100, Liam Girdwood wrote:
> On Thu, 2014-08-07 at 18:31 +0100, Mark Brown wrote:

> > I've CCed in Graeme Gregory who's been writing some guidelines for this
> > sort of thing in the context of ARMv8 serviers - the most recent patch
> > is:

> >    http://thread.gmane.org/gmane.linux.acpi.devel/69757

> > The advice there looks good to me - see the "Device Enumeration"
> > section.

> This is indeed good, just too late for the initial Broadwell BIOSes that
> wont have this _DSD field for the codec (it will be hard coded for
> Windows anyway).

Sounds like DMI is going to be needed then - whatever ends up being done
I'd expect it should just be a case of putting the checks done to work
out the machine configuration into the CODEC driver instead of the
machine driver.  If it looks ugly this can be used to explain to people
why we need something more sensible!
Liam Girdwood Aug. 14, 2014, 8:29 a.m. UTC | #6
On Mon, 2014-08-11 at 13:24 +0100, Mark Brown wrote:
> On Mon, Aug 11, 2014 at 10:59:45AM +0100, Liam Girdwood wrote:
> > On Thu, 2014-08-07 at 18:31 +0100, Mark Brown wrote:
> 
> > > I've CCed in Graeme Gregory who's been writing some guidelines for this
> > > sort of thing in the context of ARMv8 serviers - the most recent patch
> > > is:
> 
> > >    http://thread.gmane.org/gmane.linux.acpi.devel/69757
> 
> > > The advice there looks good to me - see the "Device Enumeration"
> > > section.
> 
> > This is indeed good, just too late for the initial Broadwell BIOSes that
> > wont have this _DSD field for the codec (it will be hard coded for
> > Windows anyway).
> 
> Sounds like DMI is going to be needed then - whatever ends up being done
> I'd expect it should just be a case of putting the checks done to work
> out the machine configuration into the CODEC driver instead of the
> machine driver.  If it looks ugly this can be used to explain to people
> why we need something more sensible!

It sure will be ugly ;) This would involve the codec driver checking for
the existence of the Broadwell ADSP ACPI ID and then configuring jack
mode depending on whether it finds this ID or not.

We have the opposite implementation atm where the Broadwell machine
driver sets the jack mode at machine driver init time.

I'm not sure that showing ugly code here would help justify anything
either as we will kind of be at the mercy of whether the Windows driver
needs this information or not.

Liam
Mark Brown Aug. 14, 2014, 7:09 p.m. UTC | #7
On Thu, Aug 14, 2014 at 09:29:48AM +0100, Liam Girdwood wrote:
> On Mon, 2014-08-11 at 13:24 +0100, Mark Brown wrote:

> > Sounds like DMI is going to be needed then - whatever ends up being done
> > I'd expect it should just be a case of putting the checks done to work
> > out the machine configuration into the CODEC driver instead of the
> > machine driver.  If it looks ugly this can be used to explain to people
> > why we need something more sensible!

> It sure will be ugly ;) This would involve the codec driver checking for
> the existence of the Broadwell ADSP ACPI ID and then configuring jack
> mode depending on whether it finds this ID or not.

> We have the opposite implementation atm where the Broadwell machine
> driver sets the jack mode at machine driver init time.

> I'm not sure that showing ugly code here would help justify anything
> either as we will kind of be at the mercy of whether the Windows driver
> needs this information or not.

It's going to be ugly either way, it's just a question of how the code
get partitioned - I'd expect that whatever does this is going to end up
having to cope with working out which configuration to apply anyway.

What it does buy us is consistency in where these things get handled -
it's easier to have a consistent rule that the device is responsible for
platform data (wherever that actually comes from) than to have different
rules for different firmware styles and it'll help with back pressure on
other people who do care more about Linux even if it doesn't help with
the immediate use cases.

Patch
diff mbox

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index e4f6102..dd6b454 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -414,6 +414,39 @@  int rt286_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack)
 }
 EXPORT_SYMBOL_GPL(rt286_mic_detect);
 
+/**
+ * rt286_set_jack_type - Configure the HW supported jack type
+ *
+ * @codec:   RT286 codec
+ * @type:    supported jack type
+ *
+ * RT286 need to configure the jack type (combo or non-combo) according to the
+ * HW design. This function is given a easy way to set the correct jacy type
+ * for those without platform data supported.
+ */
+void rt286_set_jack_type(struct snd_soc_codec *codec,
+				enum rt286_jack_type type)
+{
+	struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
+
+	if (RT286_TYPE_COMBO == type)
+		rt286->pdata.cbj_en = true;
+	else
+		rt286->pdata.cbj_en = false;
+
+	if (!rt286->pdata.cbj_en) {
+		regmap_write(rt286->regmap, RT286_CBJ_CTRL2, 0x0000);
+		regmap_write(rt286->regmap, RT286_MIC1_DET_CTRL, 0x0816);
+		regmap_write(rt286->regmap, RT286_MISC_CTRL1, 0x0000);
+		regmap_update_bits(rt286->regmap,
+					RT286_CBJ_CTRL1, 0xf000, 0xb000);
+	} else {
+		regmap_update_bits(rt286->regmap,
+					RT286_CBJ_CTRL1, 0xf000, 0x5000);
+	}
+}
+EXPORT_SYMBOL_GPL(rt286_set_jack_type);
+
 static const DECLARE_TLV_DB_SCALE(out_vol_tlv, -6350, 50, 0);
 static const DECLARE_TLV_DB_SCALE(mic_vol_tlv, 0, 1000, 0);
 
diff --git a/sound/soc/codecs/rt286.h b/sound/soc/codecs/rt286.h
index b539b73..2b1cbe6 100644
--- a/sound/soc/codecs/rt286.h
+++ b/sound/soc/codecs/rt286.h
@@ -192,7 +192,14 @@  enum {
 	RT286_AIFS,
 };
 
+enum rt286_jack_type {
+	RT286_TYPE_COMBO,
+	RT286_TYPE_NONCOMBO,
+};
+
 int rt286_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack);
+void rt286_set_jack_type(struct snd_soc_codec *codec,
+				enum rt286_jack_type type);
 
 #endif /* __RT286_H__ */