| Message ID | 1407217970-20126-1-git-send-email-bardliao@realtek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
> -----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.
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.
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
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!
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
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.
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__ */