[PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[koba.ko]

From: Koba Ko <koba.ko@canonical.com>

Error messge is issued,
"platform::micmute: Setting an LED's brightness failed (-19)",
Even the device isn't presented.

Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
If one of two tokens doesn't exist, don't register platform::micmute.

Signed-off-by: Koba Ko <koba.ko@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 1e46022fb2c5..afc1ded83e56 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2208,10 +2208,13 @@ static int __init dell_init(void)
 
 	dell_laptop_register_notifier(&dell_laptop_notifier);
 
-	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
-	if (ret < 0)
-		goto fail_led;
+	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
+	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
+		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+		if (ret < 0)
+			goto fail_led;
+	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
-- 
2.17.1

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Andy Shevchenko]

+Cc: Mario

On Thu, May 7, 2020 at 12:42 PM <koba.ko@canonical.com> wrote:
>
> From: Koba Ko <koba.ko@canonical.com>
>
> Error messge is issued,
> "platform::micmute: Setting an LED's brightness failed (-19)",
> Even the device isn't presented.
>
> Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> If one of two tokens doesn't exist, don't register platform::micmute.
>

What the exact platform you are experiencing that on?

> Signed-off-by: Koba Ko <koba.ko@canonical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 1e46022fb2c5..afc1ded83e56 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2208,10 +2208,13 @@ static int __init dell_init(void)
>
>         dell_laptop_register_notifier(&dell_laptop_notifier);
>
> -       micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -       ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> -       if (ret < 0)
> -               goto fail_led;
> +       if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> +           dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> +               micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +               ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> +               if (ret < 0)
> +                       goto fail_led;
> +       }
>
>         if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>                 return 0;
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Mario.Limonciello]

> hi Andy,
> it's dell g3 3590.

OK, so this is released hardware in Inspiron.

I think this patch generally makes good sense for that case.

RE: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Mario.Limonciello]

> hi All, 
> As per advertisement, Update the patch.
> Please help to review.

Koba,

1) Please resend patch as inline text to mailing list, not attachment.
2) Also you have a typographical error in your commit message, you should fix (messge misspelled)
3) You have a grammatical error in your commit message("isn't presented")
4) As you rev the patch, you can update in the title from [PATCH] to [PATCH v2] and so on.

Thanks,

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Pali Rohár]

On Thursday 07 May 2020 17:42:42 koba.ko@canonical.com wrote:
> From: Koba Ko <koba.ko@canonical.com>
> 
> Error messge is issued,
> "platform::micmute: Setting an LED's brightness failed (-19)",
> Even the device isn't presented.
> 
> Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> If one of two tokens doesn't exist, don't register platform::micmute.
> 
> Signed-off-by: Koba Ko <koba.ko@canonical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> index 1e46022fb2c5..afc1ded83e56 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2208,10 +2208,13 @@ static int __init dell_init(void)
>  
>  	dell_laptop_register_notifier(&dell_laptop_notifier);
>  
> -	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> -	if (ret < 0)
> -		goto fail_led;
> +	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> +	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> +		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> +		if (ret < 0)
> +			goto fail_led;
> +	}

Hello! I think that this is correct approach. Changing micmute LED is
done via those GLOBAL_MIC_MUTE_DISABLE and GLOBAL_MIC_MUTE_ENABLE
tokens. And if these tokens are not supported by hardware then linux
kernel should not register micmute LED device. There are lot of Dell
machines without led diode for microphone and these machines obviously
would not support those tokens.

But this change is incomplete as registration of led class dev would be
optional. So deregistration also needs to be optional.

And I think there is missing better description / explanation of this
change to make it clear what really happens.

>  
>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>  		return 0;
> -- 
> 2.17.1
> 

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Pali Rohár]

On Thursday 07 May 2020 19:27:47 Koba Ko wrote:
> Hi Pali,
> don't understand "registration and deregistration would be optional',
> could you explain more!?

After your patch led_classdev_register() function is not always called.
And led_classdev_unregister() should not be called when there is no
device registered.

> I will modify the comment of patch.
> 
> On Thu, May 7, 2020 at 7:13 PM Pali Rohár <pali@kernel.org> wrote:
> 
> > On Thursday 07 May 2020 17:42:42 koba.ko@canonical.com wrote:
> > > From: Koba Ko <koba.ko@canonical.com>
> > >
> > > Error messge is issued,
> > > "platform::micmute: Setting an LED's brightness failed (-19)",
> > > Even the device isn't presented.
> > >
> > > Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> > > If one of two tokens doesn't exist, don't register platform::micmute.
> > >
> > > Signed-off-by: Koba Ko <koba.ko@canonical.com>
> > > ---
> > >  drivers/platform/x86/dell-laptop.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/dell-laptop.c
> > b/drivers/platform/x86/dell-laptop.c
> > > index 1e46022fb2c5..afc1ded83e56 100644
> > > --- a/drivers/platform/x86/dell-laptop.c
> > > +++ b/drivers/platform/x86/dell-laptop.c
> > > @@ -2208,10 +2208,13 @@ static int __init dell_init(void)
> > >
> > >       dell_laptop_register_notifier(&dell_laptop_notifier);
> > >
> > > -     micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > > -     ret = led_classdev_register(&platform_device->dev,
> > &micmute_led_cdev);
> > > -     if (ret < 0)
> > > -             goto fail_led;
> > > +     if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> > > +         dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> > > +             micmute_led_cdev.brightness =
> > ledtrig_audio_get(LED_AUDIO_MICMUTE);
> > > +             ret = led_classdev_register(&platform_device->dev,
> > &micmute_led_cdev);
> > > +             if (ret < 0)
> > > +                     goto fail_led;
> > > +     }
> >
> > Hello! I think that this is correct approach. Changing micmute LED is
> > done via those GLOBAL_MIC_MUTE_DISABLE and GLOBAL_MIC_MUTE_ENABLE
> > tokens. And if these tokens are not supported by hardware then linux
> > kernel should not register micmute LED device. There are lot of Dell
> > machines without led diode for microphone and these machines obviously
> > would not support those tokens.
> >
> > But this change is incomplete as registration of led class dev would be
> > optional. So deregistration also needs to be optional.
> >
> > And I think there is missing better description / explanation of this
> > change to make it clear what really happens.
> >
> > >
> > >       if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
> > >               return 0;
> > > --
> > > 2.17.1
> > >
> >

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Andy Shevchenko]

On Thu, May 7, 2020 at 2:45 PM Pali Rohár <pali@kernel.org> wrote:
> On Thursday 07 May 2020 19:27:47 Koba Ko wrote:

> > don't understand "registration and deregistration would be optional',
> > could you explain more!?
>
> After your patch led_classdev_register() function is not always called.
> And led_classdev_unregister() should not be called when there is no
> device registered.

I think it's not a strong requirement after the commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=1dbb9fb4082ce2a2f1cf9596881ddece062d15d0

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Pali Rohár]

On Thursday 07 May 2020 15:54:06 Andy Shevchenko wrote:
> On Thu, May 7, 2020 at 2:45 PM Pali Rohár <pali@kernel.org> wrote:
> > On Thursday 07 May 2020 19:27:47 Koba Ko wrote:
> 
> > > don't understand "registration and deregistration would be optional',
> > > could you explain more!?
> >
> > After your patch led_classdev_register() function is not always called.
> > And led_classdev_unregister() should not be called when there is no
> > device registered.
> 
> I think it's not a strong requirement after the commit
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=1dbb9fb4082ce2a2f1cf9596881ddece062d15d0

Thank you for update. I did know about this change.

[PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[koba.ko]

From: Koba Ko <koba.ko@canonical.com>

On dell G3-3590, error message is issued during boot up,
"platform::micmute: Setting an LED's brightness failed (-19)",
but there's no micmute led on the machine.

Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
If one of two tokens doesn't exist,
don't call led_classdev_register() for platform::micmute.
After that, you wouldn't see the platform::micmute in /sys/class/leds/,
and the error message wouldn't see in dmesg.

Signed-off-by: Koba Ko <koba.ko@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 1e46022fb2c5..afc1ded83e56 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2208,10 +2208,13 @@ static int __init dell_init(void)
 
 	dell_laptop_register_notifier(&dell_laptop_notifier);
 
-	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
-	if (ret < 0)
-		goto fail_led;
+	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
+	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
+		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+		if (ret < 0)
+			goto fail_led;
+	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
-- 
2.17.1

[PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[koba.ko]

From: Koba Ko <koba.ko@canonical.com>

During boot up, Error messge is issued,
"platform::micmute: Setting an LED's brightness failed (-19)",
but the device isn't presented.

Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
If one of two tokens doesn't exist,
don't call led_classdev_register() for platform::micmute.
After that, you wouldn't see the platform::micmute in /sys/class/leds/,
and the error message wouldn't see in dmesg.

Signed-off-by: Koba Ko <koba.ko@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 74e988f839e8..e315185dbdd6 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2164,7 +2164,7 @@ static struct led_classdev micmute_led_cdev = {
 static int __init dell_init(void)
 {
 	struct calling_interface_token *token;
-	int max_intensity = 0;
+	int max_intensity = 0, is_micmute_exist = 0;
 	int ret;
 
 	if (!dmi_check_system(dell_device_table))
@@ -2204,10 +2204,14 @@ static int __init dell_init(void)
 
 	dell_laptop_register_notifier(&dell_laptop_notifier);
 
-	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
-	if (ret < 0)
-		goto fail_led;
+	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
+	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
+		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+		if (ret < 0)
+			goto fail_led;
+		is_micmute_exist = 1;
+	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
@@ -2254,7 +2258,8 @@ static int __init dell_init(void)
 fail_get_brightness:
 	backlight_device_unregister(dell_backlight_device);
 fail_backlight:
-	led_classdev_unregister(&micmute_led_cdev);
+	if (is_micmute_exist)
+		led_classdev_unregister(&micmute_led_cdev);
 fail_led:
 	dell_cleanup_rfkill();
 fail_rfkill:
-- 
2.25.1

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Andy Shevchenko]

On Thu, May 7, 2020 at 3:36 PM Koba Ko <koba.ko@canonical.com> wrote:
>
> Hi All,
> I enhanced the patch and refine the commet.
> please check it again.

Thank you.
What you should or shouldn't have done:
- you need to use version in the patch (-v<n>, where <n> is a version)
- you need to write a changelog after cutter line '---' instead of
separate messages or so
- you need to avoid top postings like this very email you send
- you need to wait for a while (usually one day or so) to give a
change for people to look at it

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Mario.Limonciello]

> -----Original Message-----
> From: koba.ko@canonical.com <koba.ko@canonical.com>
> Sent: Thursday, May 7, 2020 7:27 AM
> To: Matthew Garrett; Pali Rohár; Darren Hart; Andy Shevchenko; platform-driver-
> x86@vger.kernel.org; linux-kernel@vger.kernel.org; Limonciello, Mario
> Subject: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if
> the related tokens don't exist.
> 
> 
> [EXTERNAL EMAIL]
> 
> From: Koba Ko <koba.ko@canonical.com>
> 
> During boot up, Error messge is issued,
> "platform::micmute: Setting an LED's brightness failed (-19)",
> but the device isn't presented.
> 
> Get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> If one of two tokens doesn't exist,
> don't call led_classdev_register() for platform::micmute.
> After that, you wouldn't see the platform::micmute in /sys/class/leds/,
> and the error message wouldn't see in dmesg.
> 

Unless it's a pre-release platform, I think it's worth mentioning in commit
message what the hardware is.

If it's pre-release, then perhaps mention "pre-release vostro platform" here.
This is useful information in case in the future something in this driver must
change for a full class/line of systems.

> Signed-off-by: Koba Ko <koba.ko@canonical.com>
> ---
>  drivers/platform/x86/dell-laptop.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-
> laptop.c
> index 74e988f839e8..e315185dbdd6 100644
> --- a/drivers/platform/x86/dell-laptop.c
> +++ b/drivers/platform/x86/dell-laptop.c
> @@ -2164,7 +2164,7 @@ static struct led_classdev micmute_led_cdev = {
>  static int __init dell_init(void)
>  {
>  	struct calling_interface_token *token;
> -	int max_intensity = 0;
> +	int max_intensity = 0, is_micmute_exist = 0;

As a nit, please declare variables on their own line and I don't like is_micmute_exist
as this is bad grammar.

Perhaps "micmute_exists" instead.

Also this variable can be a boolean type instead with it's usage.

>  	int ret;
> 
>  	if (!dmi_check_system(dell_device_table))
> @@ -2204,10 +2204,14 @@ static int __init dell_init(void)
> 
>  	dell_laptop_register_notifier(&dell_laptop_notifier);
> 
> -	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> -	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
> -	if (ret < 0)
> -		goto fail_led;
> +	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
> +	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
> +		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
> +		ret = led_classdev_register(&platform_device->dev,
> &micmute_led_cdev);
> +		if (ret < 0)
> +			goto fail_led;
> +		is_micmute_exist = 1;
> +	}
> 
>  	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
>  		return 0;
> @@ -2254,7 +2258,8 @@ static int __init dell_init(void)
>  fail_get_brightness:
>  	backlight_device_unregister(dell_backlight_device);
>  fail_backlight:
> -	led_classdev_unregister(&micmute_led_cdev);
> +	if (is_micmute_exist)
> +		led_classdev_unregister(&micmute_led_cdev);
>  fail_led:
>  	dell_cleanup_rfkill();
>  fail_rfkill:
> --
> 2.25.1

[PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[koba.ko]

From: Koba Ko <koba.ko@canonical.com>

First, get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
If one of two tokens doesn't exist, don't register platform::micmute.

Signed-off-by: Koba Ko <koba.ko@canonical.com>
---
 drivers/platform/x86/dell-laptop.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index 1e46022fb2c5..afc1ded83e56 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -2208,10 +2208,13 @@ static int __init dell_init(void)
 
 	dell_laptop_register_notifier(&dell_laptop_notifier);
 
-	micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
-	ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
-	if (ret < 0)
-		goto fail_led;
+	if (dell_smbios_find_token(GLOBAL_MIC_MUTE_DISABLE) &&
+	    dell_smbios_find_token(GLOBAL_MIC_MUTE_ENABLE)) {
+		micmute_led_cdev.brightness = ledtrig_audio_get(LED_AUDIO_MICMUTE);
+		ret = led_classdev_register(&platform_device->dev, &micmute_led_cdev);
+		if (ret < 0)
+			goto fail_led;
+	}
 
 	if (acpi_video_get_backlight_type() != acpi_backlight_vendor)
 		return 0;
-- 
2.17.1

Re: [PATCH] platform/x86: dell-laptop: don't register platform::micmute if the related tokens don't exist.

[Andy Shevchenko]

On Thu, May 7, 2020 at 11:49 AM <koba.ko@canonical.com> wrote:
>
> From: Koba Ko <koba.ko@canonical.com>
>
> First, get the related tokens of SMBIOS, GLOBAL_MIC_MUTE_DISABLE/ENABLE.
> If one of two tokens doesn't exist, don't register platform::micmute.

This doesn't explain why.

-- 
With Best Regards,
Andy Shevchenko