[PATCH v9 1/2] mrst: export get_gpio_by_name() function

[PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Lu Guanqun]

Export get_gpio_by_name() function and make its name more focused.  We are
doing this because currently get_gpio_by_name() is only used by the devices
exported from SFI DEVS table, but it is also useful for other PCI devices which
also use the GPIO lines, and have their gpio infos in the SFI GPIO table.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
Reviewed-by: Tang Feng <feng.tang@intel.com>
Acked-by: Wu Fengguang <fengguang.wu@intel.com>
---
 arch/x86/include/asm/mrst.h   |    1 +
 arch/x86/platform/mrst/mrst.c |   35 ++++++++++++++++++-----------------
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/arch/x86/include/asm/mrst.h b/arch/x86/include/asm/mrst.h
index dd8a072..d6cf6d4 100644
--- a/arch/x86/include/asm/mrst.h
+++ b/arch/x86/include/asm/mrst.h
@@ -18,6 +18,7 @@ extern int pci_mrst_init(void);
 extern int __init sfi_parse_mrtc(struct sfi_table_header *table);
 extern int sfi_mrtc_num;
 extern struct sfi_rtc_table_entry sfi_mrtc_array[];
+extern int mid_lookup_gpio(const char *name);
 
 /*
  * Medfield is the follow-up of Moorestown, it combines two chip solution into
diff --git a/arch/x86/platform/mrst/mrst.c b/arch/x86/platform/mrst/mrst.c
index bc5993f..909b2cc 100644
--- a/arch/x86/platform/mrst/mrst.c
+++ b/arch/x86/platform/mrst/mrst.c
@@ -389,7 +389,7 @@ static int __init sfi_parse_gpio(struct sfi_table_header *table)
 	return 0;
 }
 
-static int get_gpio_by_name(const char *name)
+int mid_lookup_gpio(const char *name)
 {
 	struct sfi_gpio_table_entry *pentry = gpio_table;
 	int i;
@@ -402,6 +402,7 @@ static int get_gpio_by_name(const char *name)
 	}
 	return -1;
 }
+EXPORT_SYMBOL_GPL(mid_lookup_gpio);
 
 /*
  * Here defines the array of devices platform data that IAFW would export
@@ -433,7 +434,7 @@ static void __init install_irq_resource(struct platform_device *pdev,
 static void __init *pmic_gpio_platform_data(void *info)
 {
 	static struct intel_pmic_gpio_platform_data pmic_gpio_pdata;
-	int gpio_base = get_gpio_by_name("pmic_gpio_base");
+	int gpio_base = mid_lookup_gpio("pmic_gpio_base");
 
 	if (gpio_base == -1)
 		gpio_base = 64;
@@ -453,7 +454,7 @@ static struct dw_spi_chip max3111_uart = {
 static void __init *max3111_platform_data(void *info)
 {
 	struct spi_board_info *spi_info = info;
-	int intr = get_gpio_by_name("max3111_int");
+	int intr = mid_lookup_gpio("max3111_int");
 
 	spi_info->mode = SPI_MODE_0;
 	spi_info->controller_data = &max3111_uart;
@@ -492,8 +493,8 @@ static void __init *max7315_platform_data(void *info)
 		strcpy(intr_pin_name, "max7315_int");
 	}
 
-	gpio_base = get_gpio_by_name(base_pin_name);
-	intr = get_gpio_by_name(intr_pin_name);
+	gpio_base = mid_lookup_gpio(base_pin_name);
+	intr = mid_lookup_gpio(intr_pin_name);
 
 	if (gpio_base == -1)
 		return NULL;
@@ -520,8 +521,8 @@ static void *tca6416_platform_data(void *info)
 	strcpy(base_pin_name, "tca6416_base");
 	strcpy(intr_pin_name, "tca6416_int");
 
-	gpio_base = get_gpio_by_name(base_pin_name);
-	intr = get_gpio_by_name(intr_pin_name);
+	gpio_base = mid_lookup_gpio(base_pin_name);
+	intr = mid_lookup_gpio(intr_pin_name);
 
 	if (gpio_base == -1)
 		return NULL;
@@ -539,7 +540,7 @@ static void *tca6416_platform_data(void *info)
 static void *mpu3050_platform_data(void *info)
 {
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("mpu3050_int");
+	int intr = mid_lookup_gpio("mpu3050_int");
 
 	if (intr == -1)
 		return NULL;
@@ -558,7 +559,7 @@ static void *ektf2136_spi_platform_data(void *info)
 {
 	struct spi_board_info *spi_info = info;
 	static struct ektf2136_platform_data ektf2136_spi_pdata;
-	int gpio = get_gpio_by_name("ts_int");
+	int gpio = mid_lookup_gpio("ts_int");
 
 	if (gpio == -1)
 		return NULL;
@@ -574,8 +575,8 @@ static void __init *emc1403_platform_data(void *info)
 {
 	static short intr2nd_pdata;
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("thermal_int");
-	int intr2nd = get_gpio_by_name("thermal_alert");
+	int intr = mid_lookup_gpio("thermal_int");
+	int intr2nd = mid_lookup_gpio("thermal_alert");
 
 	if (intr == -1 || intr2nd == -1)
 		return NULL;
@@ -592,7 +593,7 @@ static void __init *msic_ocd_platform_data(void *info)
 	int gpio_base;
 	struct platform_device *pdev = (struct platform_device *)info;
 
-	gpio_base = get_gpio_by_name("msic_ocd");
+	gpio_base = mid_lookup_gpio("msic_ocd");
 	if (gpio_base == -1)
 		return NULL;
 
@@ -604,8 +605,8 @@ static void __init *lis331dl_platform_data(void *info)
 {
 	static short intr2nd_pdata;
 	struct i2c_board_info *i2c_info = info;
-	int intr = get_gpio_by_name("accel_int");
-	int intr2nd = get_gpio_by_name("accel_2");
+	int intr = mid_lookup_gpio("accel_int");
+	int intr2nd = mid_lookup_gpio("accel_2");
 
 	if (intr == -1 || intr2nd == -1)
 		return NULL;
@@ -763,8 +764,8 @@ static void *tc35894xbg_platform_data(struct i2c_board_info *i2c_info,
 			      struct tc35894xbg_platform_data* tc_data)
 
 {
-	tc_data->gpio_irq = get_gpio_by_name("keypad-intr");
-	tc_data->gpio_reset = get_gpio_by_name("keypad-reset");
+	tc_data->gpio_irq = mid_lookup_gpio("keypad-intr");
+	tc_data->gpio_reset = mid_lookup_gpio("keypad-reset");
 
 	printk(KERN_INFO "Nokia: keypad-intr on %d, keypad-reset on %d\n",
 	       tc_data->gpio_irq, tc_data->gpio_reset);
@@ -1117,7 +1118,7 @@ static int __init pb_keys_init(void)
 
 	num = sizeof(gpio_button) / sizeof(struct gpio_keys_button);
 	for (i = 0; i < num; i++) {
-		gb[i].gpio = get_gpio_by_name(gb[i].desc);
+		gb[i].gpio = mid_lookup_gpio(gb[i].desc);
 		if (gb[i].gpio == -1)
 			continue;
 
-- 
1.7.2.3

[PATCH v9 2/2] sst: internal speaker needs setting a GPIO line

[Lu Guanqun]

This patch originates from Jeff Cheng's patch to enable the internal speaker.

On Moorestown platform, internal speaker's power line is connected to a GPIO
line, this information is got from SFI GPIO table, so we need set it to 1 to
enable the internal speaker, or set it to 0 to disable it.

When we set the output device, we power on or off the internal speaker on demand.

CC: "Koul, Vinod" <vinod.koul@intel.com>
CC: "Kp, Jeeja" <jeeja.kp@intel.com>
Reviewed-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Jeff Cheng <jeff_cheng@wistron.com>
Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
Signed-off-by: Wang Xingchao <xingchao.wang@intel.com>
---
 drivers/staging/intel_sst/Kconfig               |    2 +-
 drivers/staging/intel_sst/intel_sst.h           |    2 +
 drivers/staging/intel_sst/intelmid.c            |    4 ++
 drivers/staging/intel_sst/intelmid_v2_control.c |   35 ++++++++++++++++++++++-
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/intel_sst/Kconfig b/drivers/staging/intel_sst/Kconfig
index b46bd9d..76d4b39 100644
--- a/drivers/staging/intel_sst/Kconfig
+++ b/drivers/staging/intel_sst/Kconfig
@@ -11,7 +11,7 @@ config SND_INTELMID
 	select SND_PCM
 	select SND_SEQUENCER
 	select SND_JACK
-	depends on SND_INTEL_SST
+	depends on SND_INTEL_SST && X86_MRST
 	default n
 	help
 	  Say Y here to include support for the Intel(R) MID sound card driver
diff --git a/drivers/staging/intel_sst/intel_sst.h b/drivers/staging/intel_sst/intel_sst.h
index 986a3df..a68080c 100644
--- a/drivers/staging/intel_sst/intel_sst.h
+++ b/drivers/staging/intel_sst/intel_sst.h
@@ -120,6 +120,8 @@ struct snd_pmic_ops {
 	unsigned int hw_dmic_map[MFLD_MAX_HW_CH];
 	unsigned int available_dmics;
 	int (*set_hw_dmic_route) (u8 index);
+
+	unsigned gpio_speaker_power;
 };
 
 extern void sst_mad_send_jack_report(struct snd_jack *jack,
diff --git a/drivers/staging/intel_sst/intelmid.c b/drivers/staging/intel_sst/intelmid.c
index 3071904..7927523 100644
--- a/drivers/staging/intel_sst/intelmid.c
+++ b/drivers/staging/intel_sst/intelmid.c
@@ -26,6 +26,7 @@
  */
 #include <linux/slab.h>
 #include <linux/io.h>
+#include <linux/gpio.h>
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/sched.h>
@@ -945,6 +946,9 @@ static int snd_intelmad_remove(struct platform_device *pdev)
 	struct snd_intelmad *intelmaddata = platform_get_drvdata(pdev);
 
 	if (intelmaddata) {
+		if (intelmaddata->sstdrv_ops->scard_ops->gpio_speaker_power)
+			gpio_free(intelmaddata->sstdrv_ops->scard_ops->
+					gpio_speaker_power);
 		free_irq(intelmaddata->irq, intelmaddata);
 		snd_card_free(intelmaddata->card);
 	}
diff --git a/drivers/staging/intel_sst/intelmid_v2_control.c b/drivers/staging/intel_sst/intelmid_v2_control.c
index 9cfcb52..243dcad 100644
--- a/drivers/staging/intel_sst/intelmid_v2_control.c
+++ b/drivers/staging/intel_sst/intelmid_v2_control.c
@@ -26,8 +26,10 @@
  *  This file contains the control operations of vendor 3
  */
 
+#include <linux/gpio.h>
 #include <linux/pci.h>
 #include <linux/file.h>
+#include <asm/mrst.h>
 #include <sound/control.h>
 #include "intel_sst.h"
 #include "intelmid_snd_control.h"
@@ -84,6 +86,12 @@ enum reg_v3 {
 	AUXDBNC = 0x12f,
 };
 
+static void nc_set_amp_power(int power)
+{
+	if (snd_pmic_ops_nc.gpio_speaker_power)
+		gpio_set_value(snd_pmic_ops_nc.gpio_speaker_power, power);
+}
+
 /****
  * nc_init_card - initilize the sound card
  *
@@ -91,6 +99,7 @@ enum reg_v3 {
  */
 static int nc_init_card(void)
 {
+	int pin;
 	struct sc_reg_access sc_access[] = {
 		{VAUDIOCNT, 0x25, 0},
 		{VOICEPORT1, 0x00, 0},
@@ -124,6 +133,13 @@ static int nc_init_card(void)
 	snd_pmic_ops_nc.master_mute = UNMUTE;
 	snd_pmic_ops_nc.mute_status = UNMUTE;
 	sst_sc_reg_access(sc_access, PMIC_WRITE, 27);
+
+	pin = mid_lookup_gpio("amp_shutdown");
+	if (pin < 0 ||
+	    gpio_request_one(pin, GPIOF_OUT_INIT_LOW, "speaker power"))
+		pin = 0;
+	snd_pmic_ops_nc.gpio_speaker_power = pin;
+
 	pr_debug("sst: init complete!!\n");
 	return 0;
 }
@@ -210,6 +226,16 @@ static int nc_power_up_pb(unsigned int port)
 
 	msleep(30);
 
+	/*
+	 * There is a mismatch between Playback Sources and the enumerated
+	 * values of output sources.  This mismatch causes ALSA upper to send
+	 * Item 1 for Internal Speaker, but the expected enumeration is 2!  For
+	 * now, treat MONO_EARPIECE and INTERNAL_SPKR identically and power up
+	 * the needed resources
+	 */
+	if (snd_pmic_ops_nc.output_dev_id == MONO_EARPIECE ||
+	    snd_pmic_ops_nc.output_dev_id == INTERNAL_SPKR)
+		nc_set_amp_power(1);
 	return nc_enable_audiodac(UNMUTE);
 
 }
@@ -271,7 +297,6 @@ static int nc_power_down(void)
 	int retval = 0;
 	struct sc_reg_access sc_access[5];
 
-
 	if (snd_pmic_ops_nc.card_status == SND_CARD_UN_INIT)
 		retval = nc_init_card();
 	if (retval)
@@ -281,6 +306,11 @@ static int nc_power_down(void)
 
 	pr_debug("sst: powering dn nc_power_down ....\n");
 
+	if (snd_pmic_ops_nc.output_dev_id == MONO_EARPIECE ||
+	    snd_pmic_ops_nc.output_dev_id == INTERNAL_SPKR) {
+		msleep(30);
+		nc_set_amp_power(0);
+	}
 	msleep(30);
 
 	sc_access[0].reg_addr = DRVPOWERCTRL;
@@ -516,9 +546,12 @@ static int nc_set_selected_output_dev(u8 value)
 	switch (value) {
 	case STEREO_HEADPHONE:
 		retval = sst_sc_reg_access(sc_access_HP, PMIC_WRITE, 2);
+		nc_set_amp_power(0);
 		break;
+	case MONO_EARPIECE:
 	case INTERNAL_SPKR:
 		retval = sst_sc_reg_access(sc_access_IS, PMIC_WRITE, 2);
+		nc_set_amp_power(1);
 		break;
 	default:
 		pr_err("sst: rcvd illegal request: %d\n", value);
-- 
1.7.2.3

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Arjan van de Ven]

On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> Export get_gpio_by_name() function and make its name more focused.  We are
> doing this because currently get_gpio_by_name() is only used by the devices
> exported from SFI DEVS table, but it is also useful for other PCI devices which
> also use the GPIO lines, and have their gpio infos in the SFI GPIO table.
>

I can't say that I like the new name... at all.

what's "mid" ?
what is wrong with the original name????

Re: [PATCH v9 2/2] sst: internal speaker needs setting a GPIO line

[Arjan van de Ven]

On 3/27/2011 7:29 PM, Lu Guanqun wrote:
>
>   	pr_debug("sst: powering dn nc_power_down ....\n");
>
> +	if (snd_pmic_ops_nc.output_dev_id == MONO_EARPIECE ||
> +	    snd_pmic_ops_nc.output_dev_id == INTERNAL_SPKR) {
> +		msleep(30);
> +		nc_set_amp_power(0);
> +	}
>   	msleep(30);

this double msleep sounds rather harsh... why do we need two of these ?
(heck... why do we need ONE ?)

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Feng Tang]

On Mon, 28 Mar 2011 10:34:22 +0800
Arjan van de Ven <arjan@linux.intel.com> wrote:

> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> > Export get_gpio_by_name() function and make its name more focused.
> > We are doing this because currently get_gpio_by_name() is only used
> > by the devices exported from SFI DEVS table, but it is also useful
> > for other PCI devices which also use the GPIO lines, and have their
> > gpio infos in the SFI GPIO table.
> >
> 
> I can't say that I like the new name... at all.
> 
> what's "mid" ?
> what is wrong with the original name????
> 
Hi Arjan,

The original get_gpio_by_name() is static and only used in mrst.c as it's
mrst/mfld specific. So if we call it from a PCI driver, that name sounds
like a platform independent general API while it's actually mrst/mfld bound.
So we would show this dependency from the function name.

Thanks,
Feng

 

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Arjan van de Ven]

On 3/27/2011 7:47 PM, Feng Tang wrote:
> On Mon, 28 Mar 2011 10:34:22 +0800
> Arjan van de Ven<arjan@linux.intel.com>  wrote:
>
>> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
>>> Export get_gpio_by_name() function and make its name more focused.
>>> We are doing this because currently get_gpio_by_name() is only used
>>> by the devices exported from SFI DEVS table, but it is also useful
>>> for other PCI devices which also use the GPIO lines, and have their
>>> gpio infos in the SFI GPIO table.
>>>
>> I can't say that I like the new name... at all.
>>
>> what's "mid" ?
>> what is wrong with the original name????
>>
> Hi Arjan,
>
> The original get_gpio_by_name() is static and only used in mrst.c as it's
> mrst/mfld specific. So if we call it from a PCI driver, that name sounds

get_gpio_by_name() would be a very good generic name for a function 
provided by the gpio layer as well.

> like a platform independent general API while it's actually mrst/mfld bound.
> So we would show this dependency from the function name.
that makes no sense.
and again, what on earth is a "mid"??????????

Re: [PATCH v9 2/2] sst: internal speaker needs setting a GPIO line

[Lu Guanqun]

On Mon, Mar 28, 2011 at 10:35:36AM +0800, Arjan van de Ven wrote:
> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> >
> >   	pr_debug("sst: powering dn nc_power_down ....\n");
> >
> > +	if (snd_pmic_ops_nc.output_dev_id == MONO_EARPIECE ||
> > +	    snd_pmic_ops_nc.output_dev_id == INTERNAL_SPKR) {
> > +		msleep(30);
> > +		nc_set_amp_power(0);
> > +	}
> >   	msleep(30);
> 
> this double msleep sounds rather harsh... why do we need two of these ?
> (heck... why do we need ONE ?)
> 

This is legacy code from Jeff Cheng's patch.
Jeff, could you provide some information on this line?

-- 
guanqun

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Lu Guanqun]

On Mon, Mar 28, 2011 at 10:47:23AM +0800, Arjan van de Ven wrote:
> On 3/27/2011 7:47 PM, Feng Tang wrote:
> > On Mon, 28 Mar 2011 10:34:22 +0800
> > Arjan van de Ven<arjan@linux.intel.com>  wrote:
> >
> >> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> >>> Export get_gpio_by_name() function and make its name more focused.
> >>> We are doing this because currently get_gpio_by_name() is only used
> >>> by the devices exported from SFI DEVS table, but it is also useful
> >>> for other PCI devices which also use the GPIO lines, and have their
> >>> gpio infos in the SFI GPIO table.
> >>>
> >> I can't say that I like the new name... at all.
> >>
> >> what's "mid" ?
> >> what is wrong with the original name????
> >>
> > Hi Arjan,
> >
> > The original get_gpio_by_name() is static and only used in mrst.c as it's
> > mrst/mfld specific. So if we call it from a PCI driver, that name sounds
> 
> get_gpio_by_name() would be a very good generic name for a function 
> provided by the gpio layer as well.
> 
> > like a platform independent general API while it's actually mrst/mfld bound.
> > So we would show this dependency from the function name.
> that makes no sense.
> and again, what on earth is a "mid"??????????
> 

MID stands for mobile internet device.
	http://en.wikipedia.org/wiki/Mobile_Internet_device

-- 
guanqun

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Feng Tang]

Hi Arjan,

On Mon, 28 Mar 2011 10:47:23 +0800
Arjan van de Ven <arjan@linux.intel.com> wrote:

> On 3/27/2011 7:47 PM, Feng Tang wrote:
> > On Mon, 28 Mar 2011 10:34:22 +0800
> > Arjan van de Ven<arjan@linux.intel.com>  wrote:
> >
> >> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> >>> Export get_gpio_by_name() function and make its name more focused.
> >>> We are doing this because currently get_gpio_by_name() is only
> >>> used by the devices exported from SFI DEVS table, but it is also
> >>> useful for other PCI devices which also use the GPIO lines, and
> >>> have their gpio infos in the SFI GPIO table.
> >>>
> >> I can't say that I like the new name... at all.
> >>
> >> what's "mid" ?
> >> what is wrong with the original name????
> >>
> > Hi Arjan,
> >
> > The original get_gpio_by_name() is static and only used in mrst.c
> > as it's mrst/mfld specific. So if we call it from a PCI driver,
> > that name sounds
> 
> get_gpio_by_name() would be a very good generic name for a function 
> provided by the gpio layer as well.

Yes, the "get_gpio_by_name" name itself is very generic, but it's
implementation is not in the GPIO core, but inside the mrst.c and
bound to Moorestown/Medfield platforms.

> 
> > like a platform independent general API while it's actually
> > mrst/mfld bound. So we would show this dependency from the function
> > name.
> that makes no sense.
> and again, what on earth is a "mid"??????????

Sorry for missed it in last email, "mid" stands for Mobile Internet Device,
Moorestown/Medfield platform are defined as MID, and this name is already
used by some driver files.

Thanks,
Feng

Re: [Meego-kernel] [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Arjan van de Ven]

On 3/27/2011 8:00 PM, Feng Tang wrote:
> Hi Arjan,
>
> On Mon, 28 Mar 2011 10:47:23 +0800
> Arjan van de Ven<arjan@linux.intel.com>  wrote:
>
>> On 3/27/2011 7:47 PM, Feng Tang wrote:
>>> On Mon, 28 Mar 2011 10:34:22 +0800
>>> Arjan van de Ven<arjan@linux.intel.com>   wrote:
>>>
>>>> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
>>>>> Export get_gpio_by_name() function and make its name more focused.
>>>>> We are doing this because currently get_gpio_by_name() is only
>>>>> used by the devices exported from SFI DEVS table, but it is also
>>>>> useful for other PCI devices which also use the GPIO lines, and
>>>>> have their gpio infos in the SFI GPIO table.
>>>>>
>>>> I can't say that I like the new name... at all.
>>>>
>>>> what's "mid" ?
>>>> what is wrong with the original name????
>>>>
>>> Hi Arjan,
>>>
>>> The original get_gpio_by_name() is static and only used in mrst.c
>>> as it's mrst/mfld specific. So if we call it from a PCI driver,
>>> that name sounds
>> get_gpio_by_name() would be a very good generic name for a function
>> provided by the gpio layer as well.
> Yes, the "get_gpio_by_name" name itself is very generic, but it's
> implementation is not in the GPIO core, but inside the mrst.c and
> bound to Moorestown/Medfield platforms.

so how about adding it?

>>> like a platform independent general API while it's actually
>>> mrst/mfld bound. So we would show this dependency from the function
>>> name.
>> that makes no sense.
>> and again, what on earth is a "mid"??????????
> Sorry for missed it in last email, "mid" stands for Mobile Internet Device,
> Moorestown/Medfield platform are defined as MID, and this name is already
> used by some driver files.

Medfield and MRST are targeting phones. MIDs don't exist... not in this 
decade.


if it was renamed to sfi_get_gpio_by_name() it might make SOME sense, 
but even then it's a
gratuitous rename for a function that's already in the upstream kernel.




> Thanks,
> Feng
> _______________________________________________
> MeeGo-kernel mailing list
> MeeGo-kernel@lists.meego.com
> http://lists.meego.com/listinfo/meego-kernel

Re: [Meego-kernel] [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Feng Tang]

Hi Arjan,

On Mon, 28 Mar 2011 11:00:53 +0800
Arjan van de Ven <arjan@linux.intel.com> wrote:

> On 3/27/2011 8:00 PM, Feng Tang wrote:
> > Hi Arjan,
> >
> > On Mon, 28 Mar 2011 10:47:23 +0800
> > Arjan van de Ven<arjan@linux.intel.com>  wrote:
> >
> >> On 3/27/2011 7:47 PM, Feng Tang wrote:
> >>> On Mon, 28 Mar 2011 10:34:22 +0800
> >>> Arjan van de Ven<arjan@linux.intel.com>   wrote:
> >>>
> >>>> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> >>>>> Export get_gpio_by_name() function and make its name more
> >>>>> focused. We are doing this because currently get_gpio_by_name()
> >>>>> is only used by the devices exported from SFI DEVS table, but
> >>>>> it is also useful for other PCI devices which also use the GPIO
> >>>>> lines, and have their gpio infos in the SFI GPIO table.
> >>>>>
> >>>> I can't say that I like the new name... at all.
> >>>>
> >>>> what's "mid" ?
> >>>> what is wrong with the original name????
> >>>>
> >>> Hi Arjan,
> >>>
> >>> The original get_gpio_by_name() is static and only used in mrst.c
> >>> as it's mrst/mfld specific. So if we call it from a PCI driver,
> >>> that name sounds
> >> get_gpio_by_name() would be a very good generic name for a function
> >> provided by the gpio layer as well.
> > Yes, the "get_gpio_by_name" name itself is very generic, but it's
> > implementation is not in the GPIO core, but inside the mrst.c and
> > bound to Moorestown/Medfield platforms.
> 
> so how about adding it?

Will check gpio core code, thanks,

> 
> >>> like a platform independent general API while it's actually
> >>> mrst/mfld bound. So we would show this dependency from the
> >>> function name.
> >> that makes no sense.
> >> and again, what on earth is a "mid"??????????
> > Sorry for missed it in last email, "mid" stands for Mobile Internet
> > Device, Moorestown/Medfield platform are defined as MID, and this
> > name is already used by some driver files.
> 
> Medfield and MRST are targeting phones. MIDs don't exist... not in
> this decade.
> 
> 
> if it was renamed to sfi_get_gpio_by_name() it might make SOME sense, 
> but even then it's a
> gratuitous rename for a function that's already in the upstream
> kernel.

Thanks for reminding, we'll try to avoid doing so later.

- Feng

RE: [PATCH v9 2/2] sst: internal speaker needs setting a GPIO line

[Jeff_Cheng]

Hi Guanqun
The msleep is to try disable pop noise. 
If you have better way to disable pop noise, please fix it.
Thanks.

-----Original Message-----
From: Lu Guanqun [mailto:guanqun.lu@intel.com] 
Sent: Monday, March 28, 2011 10:53 AM
To: Arjan van de Ven; Jeff Cheng/WHQ/Wistron
Cc: meego-kernel@lists.meego.com; alan@linux.intel.com; Wu, Fengguang;
Tang, Feng; Fu, Michael; Wang, Xingchao; Koul, Vinod; Kp, Jeeja; Greg
Kroah-Hartman; Babu, Ramesh; R, Dharageswari;
devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 2/2] sst: internal speaker needs setting a GPIO
line

On Mon, Mar 28, 2011 at 10:35:36AM +0800, Arjan van de Ven wrote:
> On 3/27/2011 7:29 PM, Lu Guanqun wrote:
> >
> >   	pr_debug("sst: powering dn nc_power_down ....\n");
> >
> > +	if (snd_pmic_ops_nc.output_dev_id == MONO_EARPIECE ||
> > +	    snd_pmic_ops_nc.output_dev_id == INTERNAL_SPKR) {
> > +		msleep(30);
> > +		nc_set_amp_power(0);
> > +	}
> >   	msleep(30);
> 
> this double msleep sounds rather harsh... why do we need two of these
?
> (heck... why do we need ONE ?)
> 

This is legacy code from Jeff Cheng's patch.
Jeff, could you provide some information on this line?

-- 
guanqun

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Greg KH]

On Mon, Mar 28, 2011 at 10:29:19AM +0800, Lu Guanqun wrote:
> Export get_gpio_by_name() function and make its name more focused.  We are
> doing this because currently get_gpio_by_name() is only used by the devices
> exported from SFI DEVS table, but it is also useful for other PCI devices which
> also use the GPIO lines, and have their gpio infos in the SFI GPIO table.

As others pointed out, don't change the name of this function.

Also, this is only needed by the one staging driver, right?

thanks,

greg k-h

Re: [PATCH v9 2/2] sst: internal speaker needs setting a GPIO line

[Greg KH]

On Mon, Mar 28, 2011 at 10:29:20AM +0800, Lu Guanqun wrote:
> This patch originates from Jeff Cheng's patch to enable the internal speaker.

I don't understand this, why is it in the changelog entry?

> On Moorestown platform, internal speaker's power line is connected to a GPIO
> line, this information is got from SFI GPIO table, so we need set it to 1 to
> enable the internal speaker, or set it to 0 to disable it.
> 
> When we set the output device, we power on or off the internal speaker on demand.

What happens without this patch?  Is this a feature you are adding to
the driver, or a bugfix?

thanks,

greg k-h

Re: [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Feng Tang]

Hi Greg,

On Mon, 28 Mar 2011 13:50:24 +0800
Greg KH <greg@kroah.com> wrote:

> On Mon, Mar 28, 2011 at 10:29:19AM +0800, Lu Guanqun wrote:
> > Export get_gpio_by_name() function and make its name more focused.
> > We are doing this because currently get_gpio_by_name() is only used
> > by the devices exported from SFI DEVS table, but it is also useful
> > for other PCI devices which also use the GPIO lines, and have their
> > gpio infos in the SFI GPIO table.
> 
> As others pointed out, don't change the name of this function.
Yes, will keep it in mind

> 
> Also, this is only needed by the one staging driver, right?
Yes, currently it is only needed by the intel_sst driver in stating.

Thanks,
Feng

Re: [Meego-kernel] [PATCH v9 1/2] mrst: export get_gpio_by_name() function

[Alan Cox]

> Medfield and MRST are targeting phones. MIDs don't exist... not in this 
> decade.

Well you can take that up with everyone else somewhere offlist, but the
standard naming being used for these devices as a group is mid_xxx, and
has been in the kernel several releases.

> if it was renamed to sfi_get_gpio_by_name() it might make SOME sense, 
> but even then it's a
> gratuitous rename for a function that's already in the upstream kernel.

1. It needs a name that won't clash with anything else or confuse people

2. It shouldn't be exported in the first place but appearing in the PCI
data for the device.

If it must be exported then it definitely needs a rename.

Re: [PATCH v9 2/2] sst: internal speaker needs setting a GPIO line

[Alan Cox]

On Mon, 28 Mar 2011 10:29:20 +0800
Lu Guanqun <guanqun.lu@intel.com> wrote:

> This patch originates from Jeff Cheng's patch to enable the internal
> speaker.
> 
> On Moorestown platform, internal speaker's power line is connected to
> a GPIO line, this information is got from SFI GPIO table, so we need
> set it to 1 to enable the internal speaker, or set it to 0 to disable
> it.
> 
> When we set the output device, we power on or off the internal
> speaker on demand.

This one looks fine to me - just need to figure out the right way to
get the GPIO pin. Really for a PCI device it ought to be coming from
the PCI space. I will talk to Arjan and co about it.

Alan