[PATCH v4 1/3] dell-led: Change dell-led.h to dell-common.h

[PATCH v4 1/3] dell-led: Change dell-led.h to dell-common.h

[Kai-Heng Feng]

This header will be used for more than just led. Change it to a more
generic name.

Cc: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v4: Change the commit message to clarify there's no more runtime pm
    warning.
    Also skip the check for thunderbolt attached devices.

v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
    of error code.
    Use DMI_DEV_TYPE_OEM_STRING to match Dell System.

v2: Mario suggested to squash the HDA part into the same series.

 drivers/platform/x86/dell-laptop.c          | 2 +-
 include/linux/{dell-led.h => dell-common.h} | 4 ++--
 sound/pci/hda/dell_wmi_helper.c             | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
 rename include/linux/{dell-led.h => dell-common.h} (61%)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index c52c6723374b..8ba820e6c3d0 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -29,7 +29,7 @@
 #include <linux/mm.h>
 #include <linux/i8042.h>
 #include <linux/debugfs.h>
-#include <linux/dell-led.h>
+#include <linux/dell-common.h>
 #include <linux/seq_file.h>
 #include <acpi/video.h>
 #include "dell-rbtn.h"
diff --git a/include/linux/dell-led.h b/include/linux/dell-common.h
similarity index 61%
rename from include/linux/dell-led.h
rename to include/linux/dell-common.h
index 92521471517f..37e4b614dd74 100644
--- a/include/linux/dell-led.h
+++ b/include/linux/dell-common.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __DELL_LED_H__
-#define __DELL_LED_H__
+#ifndef __DELL_COMMON_H__
+#define __DELL_COMMON_H__
 
 int dell_micmute_led_set(int on);
 
diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
index 1b48a8c19d28..56050cc3c0ee 100644
--- a/sound/pci/hda/dell_wmi_helper.c
+++ b/sound/pci/hda/dell_wmi_helper.c
@@ -4,7 +4,7 @@
  */
 
 #if IS_ENABLED(CONFIG_DELL_LAPTOP)
-#include <linux/dell-led.h>
+#include <linux/dell-common.h>
 
 enum {
 	MICMUTE_LED_ON,
-- 
2.17.0

[PATCH v4 2/3] platform/x86: dell-*: Add interface for switchable graphics status query

[Kai-Heng Feng]

On some Dell platforms, there's a BIOS option "Enable Switchable
Graphics". This information is useful if we want to do different things
based on this value, e.g. disable unused audio controller that comes
with the discrete graphics.

Cc: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v4: Change the commit message to clarify there's no more runtime pm
    warning.
    Also skip the check for thunderbolt attached devices.

v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
    of error code.
    Use DMI_DEV_TYPE_OEM_STRING to match Dell System.

v2: Mario suggested to squash the HDA part into the same series.

 drivers/platform/x86/dell-laptop.c      | 17 +++++++++++++++++
 drivers/platform/x86/dell-smbios-base.c |  2 ++
 drivers/platform/x86/dell-smbios.h      |  2 ++
 include/linux/dell-common.h             |  1 +
 4 files changed, 22 insertions(+)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 8ba820e6c3d0..033a27b190cc 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2116,6 +2116,23 @@ int dell_micmute_led_set(int state)
 }
 EXPORT_SYMBOL_GPL(dell_micmute_led_set);
 
+bool dell_switchable_gfx_is_enabled(void)
+{
+	struct calling_interface_buffer buffer;
+	struct calling_interface_token *token;
+
+	token = dell_smbios_find_token(SWITCHABLE_GRAPHICS_ENABLE);
+	if (!token)
+		return false;
+
+	dell_fill_request(&buffer, token->location, 0, 0, 0);
+	if (dell_send_request(&buffer, CLASS_TOKEN_READ, SELECT_TOKEN_STD))
+		return false;
+
+	return !!buffer.output[1];
+}
+EXPORT_SYMBOL_GPL(dell_switchable_gfx_is_enabled);
+
 static int __init dell_init(void)
 {
 	struct calling_interface_token *token;
diff --git a/drivers/platform/x86/dell-smbios-base.c b/drivers/platform/x86/dell-smbios-base.c
index 33fb2a20458a..881ce42f0ca7 100644
--- a/drivers/platform/x86/dell-smbios-base.c
+++ b/drivers/platform/x86/dell-smbios-base.c
@@ -86,6 +86,8 @@ struct token_range {
 static struct token_range token_whitelist[] = {
 	/* used by userspace: fwupdate */
 	{CAP_SYS_ADMIN,	CAPSULE_EN_TOKEN,	CAPSULE_DIS_TOKEN},
+	/* can indicate to userspace Switchable Graphics enable status */
+	{CAP_SYS_ADMIN,	SWITCHABLE_GRAPHICS_ENABLE,	SWITCHABLE_GRAPHICS_DISABLE},
 	/* can indicate to userspace that WMI is needed */
 	{0x0000,	WSMT_EN_TOKEN,		WSMT_DIS_TOKEN}
 };
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index d8adaf959740..7863e6a7cff8 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -37,6 +37,8 @@
 #define KBD_LED_AUTO_100_TOKEN	0x02F6
 #define GLOBAL_MIC_MUTE_ENABLE	0x0364
 #define GLOBAL_MIC_MUTE_DISABLE	0x0365
+#define SWITCHABLE_GRAPHICS_ENABLE	0x037A
+#define SWITCHABLE_GRAPHICS_DISABLE	0x037B
 
 struct notifier_block;
 
diff --git a/include/linux/dell-common.h b/include/linux/dell-common.h
index 37e4b614dd74..1a90bc9a3bea 100644
--- a/include/linux/dell-common.h
+++ b/include/linux/dell-common.h
@@ -3,5 +3,6 @@
 #define __DELL_COMMON_H__
 
 int dell_micmute_led_set(int on);
+bool dell_switchable_gfx_is_enabled(void);
 
 #endif
-- 
2.17.0

Re: [PATCH v4 2/3] platform/x86: dell-*: Add interface for switchable graphics status query

[Andy Shevchenko]

On Fri, Apr 20, 2018 at 12:44 PM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> On some Dell platforms, there's a BIOS option "Enable Switchable
> Graphics". This information is useful if we want to do different things
> based on this value, e.g. disable unused audio controller that comes
> with the discrete graphics.

>  int dell_micmute_led_set(int on);
> +bool dell_switchable_gfx_is_enabled(void);

I would rather preserve existing API, i.e.
 - prototype int func(void), where return either state or negative error code.
 - naming: dell_switchable_gfx_get()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/3] platform/x86: dell-*: Add interface for switchable graphics status query

[Andy Shevchenko]

On Fri, Apr 20, 2018 at 12:44 PM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> On some Dell platforms, there's a BIOS option "Enable Switchable
> Graphics". This information is useful if we want to do different things
> based on this value, e.g. disable unused audio controller that comes
> with the discrete graphics.

>  int dell_micmute_led_set(int on);
> +bool dell_switchable_gfx_is_enabled(void);

I would rather preserve existing API, i.e.
 - prototype int func(void), where return either state or negative error code.
 - naming: dell_switchable_gfx_get()

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai-Heng Feng]

Some Dell platforms (Preicsion 7510/7710/7520/7720) have a BIOS option
"Switchable Graphics" (SG).

When SG is enabled, we have:
00:02.0 VGA compatible controller: Intel Corporation Device 591b (rev 04)
00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31)
01:00.0 VGA compatible controller: Advanced Micro Devices, Inc.  [AMD/ATI] Ellesmere [Polaris10]
01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580]

The Intel Audio outputs all the sound, including HDMI audio. The audio
controller comes with AMD graphics doesn't get used.

When SG is disabled, we have:
00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31)
01:00.0 VGA compatible controller: Advanced Micro Devices, Inc.  [AMD/ATI] Ellesmere [Polaris10]
01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580]

Now it's a typical discrete-only system. HDMI audio comes from AMD audio
controller, others from Intel audio controller.

When SG is enabled, the unused AMD audio contoller still exposes its
sysfs, so userspace still opens the control file and stream. If
userspace tries to output sound through the stream, it hangs the system.

Since the discrete audio controller isn't useful when SG is enabled, we
should just disable the device.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
v4: Change the commit message to clarify there's no more runtime pm
    warning.
    Also skip the check for thunderbolt attached devices.

v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
    of error code.
    Use DMI_DEV_TYPE_OEM_STRING to match Dell System.

v2: Mario suggested to squash the HDA part into the same series.

 sound/pci/hda/hda_intel.c | 43 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 7a111a1b5836..3981193c8d29 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -49,6 +49,8 @@
 #include <linux/clocksource.h>
 #include <linux/time.h>
 #include <linux/completion.h>
+#include <linux/dell-common.h>
+#include <linux/dmi.h>
 
 #ifdef CONFIG_X86
 /* for snoop control */
@@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
 	}
 }
 
+#if IS_ENABLED(CONFIG_DELL_LAPTOP)
+static bool check_dell_switchable_gfx(struct pci_dev *pdev)
+{
+	bool (*dell_switchable_gfx_is_enabled_func)(void);
+	bool enabled;
+
+	/* Thunderbolt devices won't be switchable */
+	if (pci_is_thunderbolt_attached(pdev))
+		return false;
+
+	/* Only check for Dell laptops and AIOs */
+	if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) ||
+	    !(dmi_match(DMI_CHASSIS_TYPE, "10") ||
+	      dmi_match(DMI_CHASSIS_TYPE, "13")) ||
+	    !(pdev->vendor == PCI_VENDOR_ID_ATI ||
+	      pdev->vendor == PCI_VENDOR_ID_NVIDIA))
+		return false;
+
+	dell_switchable_gfx_is_enabled_func =
+		symbol_request(dell_switchable_gfx_is_enabled);
+	if (!dell_switchable_gfx_is_enabled_func)
+		return false;
+
+	enabled = dell_switchable_gfx_is_enabled_func();
+
+	symbol_put(dell_switchable_gfx_is_enabled);
+
+	return enabled;
+}
+#else
+static bool check_dell_switchable_gfx(struct pci_dev *pdev)
+{
+	return false;
+}
+#endif
+
 /* check the snoop mode availability */
 static void azx_check_snoop_available(struct azx *chip)
 {
@@ -1709,6 +1747,11 @@ static int azx_create(struct snd_card *card, struct pci_dev *pci,
 	if (err < 0)
 		return err;
 
+	if (check_dell_switchable_gfx(pci)) {
+		pci_disable_device(pci);
+		return -ENODEV;
+	}
+
 	hda = kzalloc(sizeof(*hda), GFP_KERNEL);
 	if (!hda) {
 		pci_disable_device(pci);
-- 
2.17.0

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Takashi Iwai]

On Fri, 20 Apr 2018 11:44:32 +0200,
Kai-Heng Feng wrote:
> 
> Now it's a typical discrete-only system. HDMI audio comes from AMD audio
> controller, others from Intel audio controller.
> 
> When SG is enabled, the unused AMD audio contoller still exposes its
> sysfs, so userspace still opens the control file and stream. If
> userspace tries to output sound through the stream, it hangs the system.

Hm, could you give more information about how it hangs?


> @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
>  	}
>  }
>  
> +#if IS_ENABLED(CONFIG_DELL_LAPTOP)

This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
tristate.

> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)

I'd remove "_dell" word here.  Such a check would be likely needed for
other vendors, and it's quite possible that the function will be
extended to cover a wider DMI table.


thanks,

Takashi

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai Heng Feng]

> On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>
> On Fri, 20 Apr 2018 11:44:32 +0200,
> Kai-Heng Feng wrote:
>> Now it's a typical discrete-only system. HDMI audio comes from AMD audio
>> controller, others from Intel audio controller.
>>
>> When SG is enabled, the unused AMD audio contoller still exposes its
>> sysfs, so userspace still opens the control file and stream. If
>> userspace tries to output sound through the stream, it hangs the system.
>
> Hm, could you give more information about how it hangs?

Well, I should say "it hangs the userspace process" instead.

$ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
...and it just stopped. Can't Ctrl+C to break it.

>
>
>> @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
>>  	}
>>  }
>>
>> +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
>
> This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
> tristate.

Thanks, will update in next version.

>
>> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
>
> I'd remove "_dell" word here.  Such a check would be likely needed for
> other vendors, and it's quite possible that the function will be
> extended to cover a wider DMI table.

Makes sense. Will also update this one.

Kai-Heng

>
>
> thanks,
>
> Takashi

Re: [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai Heng Feng]

> On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>
> On Fri, 20 Apr 2018 11:44:32 +0200,
> Kai-Heng Feng wrote:
>> Now it's a typical discrete-only system. HDMI audio comes from AMD audio
>> controller, others from Intel audio controller.
>>
>> When SG is enabled, the unused AMD audio contoller still exposes its
>> sysfs, so userspace still opens the control file and stream. If
>> userspace tries to output sound through the stream, it hangs the system.
>
> Hm, could you give more information about how it hangs?

Well, I should say "it hangs the userspace process" instead.

$ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
...and it just stopped. Can't Ctrl+C to break it.

>
>
>> @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
>>  	}
>>  }
>>
>> +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
>
> This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
> tristate.

Thanks, will update in next version.

>
>> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
>
> I'd remove "_dell" word here.  Such a check would be likely needed for
> other vendors, and it's quite possible that the function will be
> extended to cover a wider DMI table.

Makes sense. Will also update this one.

Kai-Heng

>
>
> thanks,
>
> Takashi

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Pali Rohár]

On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
> 
> 
> > On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Fri, 20 Apr 2018 11:44:32 +0200,
> > Kai-Heng Feng wrote:
> > > Now it's a typical discrete-only system. HDMI audio comes from AMD audio
> > > controller, others from Intel audio controller.
> > > 
> > > When SG is enabled, the unused AMD audio contoller still exposes its
> > > sysfs, so userspace still opens the control file and stream. If
> > > userspace tries to output sound through the stream, it hangs the system.
> > 
> > Hm, could you give more information about how it hangs?
> 
> Well, I should say "it hangs the userspace process" instead.
> 
> $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
> ...and it just stopped. Can't Ctrl+C to break it.

So userspace process cannot be killed at all? Then it is different bug
in kernel and disabling pci device is just a workaround. Not a real fix.

I would propose to find out what happen and why it cannot be killed
(probably it stuck somewhere in kernel) and fix it properly.

> > 
> > 
> > > @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
> > >  	}
> > >  }
> > > 
> > > +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
> > 
> > This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
> > tristate.
> 
> Thanks, will update in next version.
> 
> > 
> > > +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
> > 
> > I'd remove "_dell" word here.  Such a check would be likely needed for
> > other vendors, and it's quite possible that the function will be
> > extended to cover a wider DMI table.
> 
> Makes sense. Will also update this one.
> 
> Kai-Heng
> 
> > 
> > 
> > thanks,
> > 
> > Takashi

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Pali Rohár]

On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
> 
> 
> > On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Fri, 20 Apr 2018 11:44:32 +0200,
> > Kai-Heng Feng wrote:
> > > Now it's a typical discrete-only system. HDMI audio comes from AMD audio
> > > controller, others from Intel audio controller.
> > > 
> > > When SG is enabled, the unused AMD audio contoller still exposes its
> > > sysfs, so userspace still opens the control file and stream. If
> > > userspace tries to output sound through the stream, it hangs the system.
> > 
> > Hm, could you give more information about how it hangs?
> 
> Well, I should say "it hangs the userspace process" instead.
> 
> $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
> ...and it just stopped. Can't Ctrl+C to break it.

So userspace process cannot be killed at all? Then it is different bug
in kernel and disabling pci device is just a workaround. Not a real fix.

I would propose to find out what happen and why it cannot be killed
(probably it stuck somewhere in kernel) and fix it properly.

> > 
> > 
> > > @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
> > >  	}
> > >  }
> > > 
> > > +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
> > 
> > This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
> > tristate.
> 
> Thanks, will update in next version.
> 
> > 
> > > +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
> > 
> > I'd remove "_dell" word here.  Such a check would be likely needed for
> > other vendors, and it's quite possible that the function will be
> > extended to cover a wider DMI table.
> 
> Makes sense. Will also update this one.
> 
> Kai-Heng
> 
> > 
> > 
> > thanks,
> > 
> > Takashi

-- 
Pali Rohár
pali.rohar@gmail.com
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai Heng Feng]

> On Apr 23, 2018, at 4:08 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
>>> On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>
>>> On Fri, 20 Apr 2018 11:44:32 +0200,
>>> Kai-Heng Feng wrote:
>>>> Now it's a typical discrete-only system. HDMI audio comes from AMD audio
>>>> controller, others from Intel audio controller.
>>>>
>>>> When SG is enabled, the unused AMD audio contoller still exposes its
>>>> sysfs, so userspace still opens the control file and stream. If
>>>> userspace tries to output sound through the stream, it hangs the system.
>>>
>>> Hm, could you give more information about how it hangs?
>>
>> Well, I should say "it hangs the userspace process" instead.
>>
>> $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
>> ...and it just stopped. Can't Ctrl+C to break it.
>
> So userspace process cannot be killed at all? Then it is different bug
> in kernel and disabling pci device is just a workaround. Not a real fix.
>
> I would propose to find out what happen and why it cannot be killed
> (probably it stuck somewhere in kernel) and fix it properly.

That's because the audio device got runtime suspended by the graphics.

In this case, if we really want to use the the discrete audio, then we also  
need to wake up the graphics.
The discrete audio is totally useless when SG is enabled, so my approach is  
just to disable it.

Kai-Heng

>
>>>> @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
>>>>  	}
>>>>  }
>>>>
>>>> +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
>>>
>>> This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
>>> tristate.
>>
>> Thanks, will update in next version.
>>
>>>> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
>>>
>>> I'd remove "_dell" word here.  Such a check would be likely needed for
>>> other vendors, and it's quite possible that the function will be
>>> extended to cover a wider DMI table.
>>
>> Makes sense. Will also update this one.
>>
>> Kai-Heng
>>
>>> thanks,
>>>
>>> Takashi
>
> -- 
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai Heng Feng]

> On Apr 23, 2018, at 4:08 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
>>> On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>
>>> On Fri, 20 Apr 2018 11:44:32 +0200,
>>> Kai-Heng Feng wrote:
>>>> Now it's a typical discrete-only system. HDMI audio comes from AMD audio
>>>> controller, others from Intel audio controller.
>>>>
>>>> When SG is enabled, the unused AMD audio contoller still exposes its
>>>> sysfs, so userspace still opens the control file and stream. If
>>>> userspace tries to output sound through the stream, it hangs the system.
>>>
>>> Hm, could you give more information about how it hangs?
>>
>> Well, I should say "it hangs the userspace process" instead.
>>
>> $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
>> ...and it just stopped. Can't Ctrl+C to break it.
>
> So userspace process cannot be killed at all? Then it is different bug
> in kernel and disabling pci device is just a workaround. Not a real fix.
>
> I would propose to find out what happen and why it cannot be killed
> (probably it stuck somewhere in kernel) and fix it properly.

That's because the audio device got runtime suspended by the graphics.

In this case, if we really want to use the the discrete audio, then we also  
need to wake up the graphics.
The discrete audio is totally useless when SG is enabled, so my approach is  
just to disable it.

Kai-Heng

>
>>>> @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
>>>>  	}
>>>>  }
>>>>
>>>> +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
>>>
>>> This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
>>> tristate.
>>
>> Thanks, will update in next version.
>>
>>>> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
>>>
>>> I'd remove "_dell" word here.  Such a check would be likely needed for
>>> other vendors, and it's quite possible that the function will be
>>> extended to cover a wider DMI table.
>>
>> Makes sense. Will also update this one.
>>
>> Kai-Heng
>>
>>> thanks,
>>>
>>> Takashi
>
> -- 
> Pali Rohár
> pali.rohar@gmail.com
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Pali Rohár]

On Monday 23 April 2018 16:18:35 Kai Heng Feng wrote:
> 
> 
> > On Apr 23, 2018, at 4:08 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> > 
> > On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
> > > > On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > > > 
> > > > On Fri, 20 Apr 2018 11:44:32 +0200,
> > > > Kai-Heng Feng wrote:
> > > > > Now it's a typical discrete-only system. HDMI audio comes from AMD audio
> > > > > controller, others from Intel audio controller.
> > > > > 
> > > > > When SG is enabled, the unused AMD audio contoller still exposes its
> > > > > sysfs, so userspace still opens the control file and stream. If
> > > > > userspace tries to output sound through the stream, it hangs the system.
> > > > 
> > > > Hm, could you give more information about how it hangs?
> > > 
> > > Well, I should say "it hangs the userspace process" instead.
> > > 
> > > $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
> > > ...and it just stopped. Can't Ctrl+C to break it.
> > 
> > So userspace process cannot be killed at all? Then it is different bug
> > in kernel and disabling pci device is just a workaround. Not a real fix.
> > 
> > I would propose to find out what happen and why it cannot be killed
> > (probably it stuck somewhere in kernel) and fix it properly.
> 
> That's because the audio device got runtime suspended by the graphics.
> 
> In this case, if we really want to use the the discrete audio, then we also
> need to wake up the graphics.
> The discrete audio is totally useless when SG is enabled, so my approach is
> just to disable it.

I'm not saying that disabling PCI should not be done.

I'm saying that if userspace process which opened that audio device
cannot be killed, then you found different kernel bug and that should be
fixed.

> Kai-Heng
> 
> > 
> > > > > @@ -1627,6 +1629,42 @@ static void check_msi(struct azx *chip)
> > > > >  	}
> > > > >  }
> > > > > 
> > > > > +#if IS_ENABLED(CONFIG_DELL_LAPTOP)
> > > > 
> > > > This should be IS_REACHABLE(), as both dell-laptop and HD-audio are
> > > > tristate.
> > > 
> > > Thanks, will update in next version.
> > > 
> > > > > +static bool check_dell_switchable_gfx(struct pci_dev *pdev)
> > > > 
> > > > I'd remove "_dell" word here.  Such a check would be likely needed for
> > > > other vendors, and it's quite possible that the function will be
> > > > extended to cover a wider DMI table.
> > > 
> > > Makes sense. Will also update this one.
> > > 
> > > Kai-Heng
> > > 
> > > > thanks,
> > > > 
> > > > Takashi
> > 
> > -- 
> > Pali Rohár
> > pali.rohar@gmail.com

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Lukas Wunner]

On Mon, Apr 23, 2018 at 04:18:35PM +0800, Kai Heng Feng wrote:
> >On Apr 23, 2018, at 4:08 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
> >>>On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >>>On Fri, 20 Apr 2018 11:44:32 +0200, Kai-Heng Feng wrote:
> >>>>Now it's a typical discrete-only system. HDMI audio comes from AMD audio
> >>>>controller, others from Intel audio controller.
> >>>>
> >>>>When SG is enabled, the unused AMD audio contoller still exposes its
> >>>>sysfs, so userspace still opens the control file and stream. If
> >>>>userspace tries to output sound through the stream, it hangs the system.
> >>>
> >>>Hm, could you give more information about how it hangs?
> >>
> >>Well, I should say "it hangs the userspace process" instead.
> >>
> >>$ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
> >>...and it just stopped. Can't Ctrl+C to break it.
> >
> >So userspace process cannot be killed at all? Then it is different bug
> >in kernel and disabling pci device is just a workaround. Not a real fix.
> >
> >I would propose to find out what happen and why it cannot be killed
> >(probably it stuck somewhere in kernel) and fix it properly.
> 
> That's because the audio device got runtime suspended by the graphics.
> 
> In this case, if we really want to use the the discrete audio, then we also
> need to wake up the graphics.
> The discrete audio is totally useless when SG is enabled, so my approach is
> just to disable it.

I don't quite follow, that should be fixed by commit 07f4f97d7b4b
("vga_switcheroo: Use device link for HDA controller") which landed
in v4.17-rc1.

My understanding was that with SG enabled, the external DP/HDMI ports
are muxed to the Intel GPU, so audio can only be streamed to external
displays by the Intel HDA, not by the HDA integrated into the discrete
AMD/Nvidia GPU.  Audio streamed to the latter would essentially end up
in a blackhole.  And preventing the user from seeing such useless audio
devices was the sole purpose of this commit.  Am I missing something?

Thanks,

Lukas

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai Heng Feng]

> On Apr 25, 2018, at 5:13 AM, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, Apr 23, 2018 at 04:18:35PM +0800, Kai Heng Feng wrote:
>>> On Apr 23, 2018, at 4:08 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>>> On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
>>>>> On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>> On Fri, 20 Apr 2018 11:44:32 +0200, Kai-Heng Feng wrote:
>>>>>> Now it's a typical discrete-only system. HDMI audio comes from AMD  
>>>>>> audio
>>>>>> controller, others from Intel audio controller.
>>>>>>
>>>>>> When SG is enabled, the unused AMD audio contoller still exposes its
>>>>>> sysfs, so userspace still opens the control file and stream. If
>>>>>> userspace tries to output sound through the stream, it hangs the  
>>>>>> system.
>>>>>
>>>>> Hm, could you give more information about how it hangs?
>>>>
>>>> Well, I should say "it hangs the userspace process" instead.
>>>>
>>>> $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
>>>> ...and it just stopped. Can't Ctrl+C to break it.
>>>
>>> So userspace process cannot be killed at all? Then it is different bug
>>> in kernel and disabling pci device is just a workaround. Not a real fix.
>>>
>>> I would propose to find out what happen and why it cannot be killed
>>> (probably it stuck somewhere in kernel) and fix it properly.
>>
>> That's because the audio device got runtime suspended by the graphics.
>>
>> In this case, if we really want to use the the discrete audio, then we  
>> also
>> need to wake up the graphics.
>> The discrete audio is totally useless when SG is enabled, so my approach  
>> is
>> just to disable it.
>
> I don't quite follow, that should be fixed by commit 07f4f97d7b4b
> ("vga_switcheroo: Use device link for HDA controller") which landed
> in v4.17-rc1.

Looks like I hit a new bug: the discrete GFX and its audio controller never  
enters to D3.
The GFX can enter to D3 with my prosed patch.

>
> My understanding was that with SG enabled, the external DP/HDMI ports
> are muxed to the Intel GPU, so audio can only be streamed to external
> displays by the Intel HDA, not by the HDA integrated into the discrete
> AMD/Nvidia GPU.  Audio streamed to the latter would essentially end up
> in a blackhole.  And preventing the user from seeing such useless audio
> devices was the sole purpose of this commit.  Am I missing something?

Yes this is the intention.

Kai-Heng

>
> Thanks,
>
> Lukas

Re: [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Kai Heng Feng]

> On Apr 25, 2018, at 5:13 AM, Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, Apr 23, 2018 at 04:18:35PM +0800, Kai Heng Feng wrote:
>>> On Apr 23, 2018, at 4:08 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>>> On Monday 23 April 2018 16:04:55 Kai Heng Feng wrote:
>>>>> On Apr 20, 2018, at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>>>> On Fri, 20 Apr 2018 11:44:32 +0200, Kai-Heng Feng wrote:
>>>>>> Now it's a typical discrete-only system. HDMI audio comes from AMD  
>>>>>> audio
>>>>>> controller, others from Intel audio controller.
>>>>>>
>>>>>> When SG is enabled, the unused AMD audio contoller still exposes its
>>>>>> sysfs, so userspace still opens the control file and stream. If
>>>>>> userspace tries to output sound through the stream, it hangs the  
>>>>>> system.
>>>>>
>>>>> Hm, could you give more information about how it hangs?
>>>>
>>>> Well, I should say "it hangs the userspace process" instead.
>>>>
>>>> $ speaker-test -t wav -c 2 -D hw:CARD=HDMI,DEV=3
>>>> ...and it just stopped. Can't Ctrl+C to break it.
>>>
>>> So userspace process cannot be killed at all? Then it is different bug
>>> in kernel and disabling pci device is just a workaround. Not a real fix.
>>>
>>> I would propose to find out what happen and why it cannot be killed
>>> (probably it stuck somewhere in kernel) and fix it properly.
>>
>> That's because the audio device got runtime suspended by the graphics.
>>
>> In this case, if we really want to use the the discrete audio, then we  
>> also
>> need to wake up the graphics.
>> The discrete audio is totally useless when SG is enabled, so my approach  
>> is
>> just to disable it.
>
> I don't quite follow, that should be fixed by commit 07f4f97d7b4b
> ("vga_switcheroo: Use device link for HDA controller") which landed
> in v4.17-rc1.

Looks like I hit a new bug: the discrete GFX and its audio controller never  
enters to D3.
The GFX can enter to D3 with my prosed patch.

>
> My understanding was that with SG enabled, the external DP/HDMI ports
> are muxed to the Intel GPU, so audio can only be streamed to external
> displays by the Intel HDA, not by the HDA integrated into the discrete
> AMD/Nvidia GPU.  Audio streamed to the latter would essentially end up
> in a blackhole.  And preventing the user from seeing such useless audio
> devices was the sole purpose of this commit.  Am I missing something?

Yes this is the intention.

Kai-Heng

>
> Thanks,
>
> Lukas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [alsa-devel] [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Lukas Wunner]

On Thu, Apr 26, 2018 at 03:52:08PM +0800, Kai Heng Feng wrote:
> > On Apr 25, 2018, at 5:13 AM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Mon, Apr 23, 2018 at 04:18:35PM +0800, Kai Heng Feng wrote:
> > > That's because the audio device got runtime suspended by the graphics.
> > >
> > > In this case, if we really want to use the the discrete audio,
> > > then we also need to wake up the graphics.
> > > The discrete audio is totally useless when SG is enabled,
> > > so my approach is just to disable it.
> >
> > I don't quite follow, that should be fixed by commit 07f4f97d7b4b
> > ("vga_switcheroo: Use device link for HDA controller") which landed
> > in v4.17-rc1.
> 
> Looks like I hit a new bug: the discrete GFX and its audio controller
> never enters to D3.
> The GFX can enter to D3 with my prosed patch.

Can you find out why?  Does the HDA controller have a codec as child
device that doesn't runtime suspend, perhaps because it failed to
initialize correctly?  If a codec device fails to runtime suspend,
the HDA controller (as parent) and by extension the GPU (via the
device link) are also kept runtime active.

Do you see anything in dmesg when the AMD HDA controller probes?
If you look in /sys/bus/pci/devices/0000:01:00.1/power/, does the
HDA controller have active kids and what is its usage count?


> > My understanding was that with SG enabled, the external DP/HDMI ports
> > are muxed to the Intel GPU, so audio can only be streamed to external
> > displays by the Intel HDA, not by the HDA integrated into the discrete
> > AMD/Nvidia GPU.  Audio streamed to the latter would essentially end up
> > in a blackhole.  And preventing the user from seeing such useless audio
> > devices was the sole purpose of this commit.  Am I missing something?
> 
> Yes this is the intention.

Could you add this information to the commit message then?

Thanks,

Lukas

Re: [PATCH v4 3/3] ALSA: hda: Disabled unused audio controller for Dell platforms with Switchable Graphics

[Andy Shevchenko]

On Fri, Apr 20, 2018 at 12:44 PM, Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
> Some Dell platforms (Preicsion 7510/7710/7520/7720) have a BIOS option
> "Switchable Graphics" (SG).
>
> When SG is enabled, we have:
> 00:02.0 VGA compatible controller: Intel Corporation Device 591b (rev 04)
> 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31)
> 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc.  [AMD/ATI] Ellesmere [Polaris10]
> 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580]
>
> The Intel Audio outputs all the sound, including HDMI audio. The audio
> controller comes with AMD graphics doesn't get used.
>
> When SG is disabled, we have:
> 00:1f.3 Audio device: Intel Corporation CM238 HD Audio Controller (rev 31)
> 01:00.0 VGA compatible controller: Advanced Micro Devices, Inc.  [AMD/ATI] Ellesmere [Polaris10]
> 01:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Ellesmere [Radeon RX 580]
>
> Now it's a typical discrete-only system. HDMI audio comes from AMD audio
> controller, others from Intel audio controller.
>
> When SG is enabled, the unused AMD audio contoller still exposes its
> sysfs, so userspace still opens the control file and stream. If
> userspace tries to output sound through the stream, it hangs the system.
>
> Since the discrete audio controller isn't useful when SG is enabled, we
> should just disable the device.

> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)

Yeah, as following existing naming scheme in the driver
(check_hdmi_disabled(), for example)
I would rather call it similar:

check_switchable_gfx_enabled()

> +{
> +       bool (*dell_switchable_gfx_is_enabled_func)(void);
> +       bool enabled;
> +
> +       /* Thunderbolt devices won't be switchable */
> +       if (pci_is_thunderbolt_attached(pdev))
> +               return false;
> +
> +       /* Only check for Dell laptops and AIOs */

> +       if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) ||
> +           !(dmi_match(DMI_CHASSIS_TYPE, "10") ||
> +             dmi_match(DMI_CHASSIS_TYPE, "13")) ||

Can't we do it in more flexible way, i.e. using struct dmi_system_id
table and dmi_check_system() ?

> +           !(pdev->vendor == PCI_VENDOR_ID_ATI ||
> +             pdev->vendor == PCI_VENDOR_ID_NVIDIA))
> +               return false;

I would rather split DMI and PCI checks.

> +
> +       dell_switchable_gfx_is_enabled_func =
> +               symbol_request(dell_switchable_gfx_is_enabled);
> +       if (!dell_switchable_gfx_is_enabled_func)
> +               return false;
> +
> +       enabled = dell_switchable_gfx_is_enabled_func();
> +
> +       symbol_put(dell_switchable_gfx_is_enabled);
> +
> +       return enabled;
> +}
> +#else
> +static bool check_dell_switchable_gfx(struct pci_dev *pdev)

inline

> +{
> +       return false;
> +}
> +#endif

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/3] dell-led: Change dell-led.h to dell-common.h

[Takashi Iwai]

On Fri, 20 Apr 2018 11:44:30 +0200,
Kai-Heng Feng wrote:
> 
> This header will be used for more than just led. Change it to a more
> generic name.
> 
> Cc: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

BTW, wouldn't it be better postpone the rename?
Renaming a file in a cross-tree patchset makes harder to apply in
general.


thanks,

Takashi

> ---
> v4: Change the commit message to clarify there's no more runtime pm
>     warning.
>     Also skip the check for thunderbolt attached devices.
> 
> v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
>     of error code.
>     Use DMI_DEV_TYPE_OEM_STRING to match Dell System.
> 
> v2: Mario suggested to squash the HDA part into the same series.
> 
>  drivers/platform/x86/dell-laptop.c          | 2 +-
>  include/linux/{dell-led.h => dell-common.h} | 4 ++--
>  sound/pci/hda/dell_wmi_helper.c             | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>  rename include/linux/{dell-led.h => dell-common.h} (61%)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index c52c6723374b..8ba820e6c3d0 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -29,7 +29,7 @@
>  #include <linux/mm.h>
>  #include <linux/i8042.h>
>  #include <linux/debugfs.h>
> -#include <linux/dell-led.h>
> +#include <linux/dell-common.h>
>  #include <linux/seq_file.h>
>  #include <acpi/video.h>
>  #include "dell-rbtn.h"
> diff --git a/include/linux/dell-led.h b/include/linux/dell-common.h
> similarity index 61%
> rename from include/linux/dell-led.h
> rename to include/linux/dell-common.h
> index 92521471517f..37e4b614dd74 100644
> --- a/include/linux/dell-led.h
> +++ b/include/linux/dell-common.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef __DELL_LED_H__
> -#define __DELL_LED_H__
> +#ifndef __DELL_COMMON_H__
> +#define __DELL_COMMON_H__
>  
>  int dell_micmute_led_set(int on);
>  
> diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
> index 1b48a8c19d28..56050cc3c0ee 100644
> --- a/sound/pci/hda/dell_wmi_helper.c
> +++ b/sound/pci/hda/dell_wmi_helper.c
> @@ -4,7 +4,7 @@
>   */
>  
>  #if IS_ENABLED(CONFIG_DELL_LAPTOP)
> -#include <linux/dell-led.h>
> +#include <linux/dell-common.h>
>  
>  enum {
>  	MICMUTE_LED_ON,
> -- 
> 2.17.0
> 
> 

Re: [PATCH v4 1/3] dell-led: Change dell-led.h to dell-common.h

[Takashi Iwai]

On Fri, 20 Apr 2018 11:44:30 +0200,
Kai-Heng Feng wrote:
> 
> This header will be used for more than just led. Change it to a more
> generic name.
> 
> Cc: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>

BTW, wouldn't it be better postpone the rename?
Renaming a file in a cross-tree patchset makes harder to apply in
general.


thanks,

Takashi

> ---
> v4: Change the commit message to clarify there's no more runtime pm
>     warning.
>     Also skip the check for thunderbolt attached devices.
> 
> v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
>     of error code.
>     Use DMI_DEV_TYPE_OEM_STRING to match Dell System.
> 
> v2: Mario suggested to squash the HDA part into the same series.
> 
>  drivers/platform/x86/dell-laptop.c          | 2 +-
>  include/linux/{dell-led.h => dell-common.h} | 4 ++--
>  sound/pci/hda/dell_wmi_helper.c             | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
>  rename include/linux/{dell-led.h => dell-common.h} (61%)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index c52c6723374b..8ba820e6c3d0 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -29,7 +29,7 @@
>  #include <linux/mm.h>
>  #include <linux/i8042.h>
>  #include <linux/debugfs.h>
> -#include <linux/dell-led.h>
> +#include <linux/dell-common.h>
>  #include <linux/seq_file.h>
>  #include <acpi/video.h>
>  #include "dell-rbtn.h"
> diff --git a/include/linux/dell-led.h b/include/linux/dell-common.h
> similarity index 61%
> rename from include/linux/dell-led.h
> rename to include/linux/dell-common.h
> index 92521471517f..37e4b614dd74 100644
> --- a/include/linux/dell-led.h
> +++ b/include/linux/dell-common.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -#ifndef __DELL_LED_H__
> -#define __DELL_LED_H__
> +#ifndef __DELL_COMMON_H__
> +#define __DELL_COMMON_H__
>  
>  int dell_micmute_led_set(int on);
>  
> diff --git a/sound/pci/hda/dell_wmi_helper.c b/sound/pci/hda/dell_wmi_helper.c
> index 1b48a8c19d28..56050cc3c0ee 100644
> --- a/sound/pci/hda/dell_wmi_helper.c
> +++ b/sound/pci/hda/dell_wmi_helper.c
> @@ -4,7 +4,7 @@
>   */
>  
>  #if IS_ENABLED(CONFIG_DELL_LAPTOP)
> -#include <linux/dell-led.h>
> +#include <linux/dell-common.h>
>  
>  enum {
>  	MICMUTE_LED_ON,
> -- 
> 2.17.0
> 
> 

Re: [PATCH v4 1/3] dell-led: Change dell-led.h to dell-common.h

[Kai Heng Feng]

> On Apr 25, 2018, at 12:26 AM, Takashi Iwai <tiwai@suse.de> wrote:
>
> On Fri, 20 Apr 2018 11:44:30 +0200,
> Kai-Heng Feng wrote:
>> This header will be used for more than just led. Change it to a more
>> generic name.
>>
>> Cc: Mario Limonciello <mario.limonciello@dell.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> BTW, wouldn't it be better postpone the rename?
> Renaming a file in a cross-tree patchset makes harder to apply in
> general.
>

You are right, I'll keep the original filename in next version.

Kai-Heng

>
> thanks,
>
> Takashi
>
>> ---
>> v4: Change the commit message to clarify there's no more runtime pm
>>     warning.
>>     Also skip the check for thunderbolt attached devices.
>>
>> v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
>>     of error code.
>>     Use DMI_DEV_TYPE_OEM_STRING to match Dell System.
>>
>> v2: Mario suggested to squash the HDA part into the same series.
>>
>>  drivers/platform/x86/dell-laptop.c          | 2 +-
>>  include/linux/{dell-led.h => dell-common.h} | 4 ++--
>>  sound/pci/hda/dell_wmi_helper.c             | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>  rename include/linux/{dell-led.h => dell-common.h} (61%)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c  
>> b/drivers/platform/x86/dell-laptop.c
>> index c52c6723374b..8ba820e6c3d0 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -29,7 +29,7 @@
>>  #include <linux/mm.h>
>>  #include <linux/i8042.h>
>>  #include <linux/debugfs.h>
>> -#include <linux/dell-led.h>
>> +#include <linux/dell-common.h>
>>  #include <linux/seq_file.h>
>>  #include <acpi/video.h>
>>  #include "dell-rbtn.h"
>> diff --git a/include/linux/dell-led.h b/include/linux/dell-common.h
>> similarity index 61%
>> rename from include/linux/dell-led.h
>> rename to include/linux/dell-common.h
>> index 92521471517f..37e4b614dd74 100644
>> --- a/include/linux/dell-led.h
>> +++ b/include/linux/dell-common.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: GPL-2.0 */
>> -#ifndef __DELL_LED_H__
>> -#define __DELL_LED_H__
>> +#ifndef __DELL_COMMON_H__
>> +#define __DELL_COMMON_H__
>>
>>  int dell_micmute_led_set(int on);
>>
>> diff --git a/sound/pci/hda/dell_wmi_helper.c  
>> b/sound/pci/hda/dell_wmi_helper.c
>> index 1b48a8c19d28..56050cc3c0ee 100644
>> --- a/sound/pci/hda/dell_wmi_helper.c
>> +++ b/sound/pci/hda/dell_wmi_helper.c
>> @@ -4,7 +4,7 @@
>>   */
>>
>>  #if IS_ENABLED(CONFIG_DELL_LAPTOP)
>> -#include <linux/dell-led.h>
>> +#include <linux/dell-common.h>
>>
>>  enum {
>>  	MICMUTE_LED_ON,
>> -- 
>> 2.17.0

Re: [PATCH v4 1/3] dell-led: Change dell-led.h to dell-common.h

[Kai Heng Feng]

> On Apr 25, 2018, at 12:26 AM, Takashi Iwai <tiwai@suse.de> wrote:
>
> On Fri, 20 Apr 2018 11:44:30 +0200,
> Kai-Heng Feng wrote:
>> This header will be used for more than just led. Change it to a more
>> generic name.
>>
>> Cc: Mario Limonciello <mario.limonciello@dell.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> BTW, wouldn't it be better postpone the rename?
> Renaming a file in a cross-tree patchset makes harder to apply in
> general.
>

You are right, I'll keep the original filename in next version.

Kai-Heng

>
> thanks,
>
> Takashi
>
>> ---
>> v4: Change the commit message to clarify there's no more runtime pm
>>     warning.
>>     Also skip the check for thunderbolt attached devices.
>>
>> v3: Simplify dell_switchable_gfx_is_enabled() by returning bool instead
>>     of error code.
>>     Use DMI_DEV_TYPE_OEM_STRING to match Dell System.
>>
>> v2: Mario suggested to squash the HDA part into the same series.
>>
>>  drivers/platform/x86/dell-laptop.c          | 2 +-
>>  include/linux/{dell-led.h => dell-common.h} | 4 ++--
>>  sound/pci/hda/dell_wmi_helper.c             | 2 +-
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>  rename include/linux/{dell-led.h => dell-common.h} (61%)
>>
>> diff --git a/drivers/platform/x86/dell-laptop.c  
>> b/drivers/platform/x86/dell-laptop.c
>> index c52c6723374b..8ba820e6c3d0 100644
>> --- a/drivers/platform/x86/dell-laptop.c
>> +++ b/drivers/platform/x86/dell-laptop.c
>> @@ -29,7 +29,7 @@
>>  #include <linux/mm.h>
>>  #include <linux/i8042.h>
>>  #include <linux/debugfs.h>
>> -#include <linux/dell-led.h>
>> +#include <linux/dell-common.h>
>>  #include <linux/seq_file.h>
>>  #include <acpi/video.h>
>>  #include "dell-rbtn.h"
>> diff --git a/include/linux/dell-led.h b/include/linux/dell-common.h
>> similarity index 61%
>> rename from include/linux/dell-led.h
>> rename to include/linux/dell-common.h
>> index 92521471517f..37e4b614dd74 100644
>> --- a/include/linux/dell-led.h
>> +++ b/include/linux/dell-common.h
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: GPL-2.0 */
>> -#ifndef __DELL_LED_H__
>> -#define __DELL_LED_H__
>> +#ifndef __DELL_COMMON_H__
>> +#define __DELL_COMMON_H__
>>
>>  int dell_micmute_led_set(int on);
>>
>> diff --git a/sound/pci/hda/dell_wmi_helper.c  
>> b/sound/pci/hda/dell_wmi_helper.c
>> index 1b48a8c19d28..56050cc3c0ee 100644
>> --- a/sound/pci/hda/dell_wmi_helper.c
>> +++ b/sound/pci/hda/dell_wmi_helper.c
>> @@ -4,7 +4,7 @@
>>   */
>>
>>  #if IS_ENABLED(CONFIG_DELL_LAPTOP)
>> -#include <linux/dell-led.h>
>> +#include <linux/dell-common.h>
>>
>>  enum {
>>  	MICMUTE_LED_ON,
>> -- 
>> 2.17.0