[03/15] vga_switcheroo: Set active attribute to false for audio clients
diff mbox

Message ID a5c2f125f6eaa47c4aa6d8801de31d1301c8a542.1442497843.git.lukas@wunner.de
State New
Headers show

Commit Message

Lukas Wunner Aug. 27, 2015, 2:43 p.m. UTC
The active attribute in struct vga_switcheroo_client denotes whether
the outputs are currently switched to this client. The attribute is
only meaningful for vga clients. It is never used for audio clients.

The function vga_switcheroo_register_audio_client() misuses this
attribute to store whether the audio device is fully initialized.
Most likely there was a misunderstanding about the meaning of
"active" when this was added.

Set the active attribute to false for audio clients. Remove the
active parameter from vga_switcheroo_register_audio_client() and
its sole caller, hda_intel.c:register_vga_switcheroo().

vga_switcheroo_register_audio_client() was introduced by 3e9e63dbd374
("vga_switcheroo: Add the support for audio clients"). Its use in
hda_intel.c was introduced by a82d51ed24bb ("ALSA: hda - Support
VGA-switcheroo").

Cc: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/vga/vga_switcheroo.c | 5 ++---
 include/linux/vga_switcheroo.h   | 4 ++--
 sound/pci/hda/hda_intel.c        | 3 +--
 3 files changed, 5 insertions(+), 7 deletions(-)

Comments

Lukas Wunner Sept. 17, 2015, 5:15 p.m. UTC | #1
After sending the below patch away I realized that this change allows
us to simplify the code a bit by dropping the client_is_vga() call
from find_active_client().

I force-pushed an amended version of the commit to my vga_switcheroo_docs
branch on GitHub:
https://github.com/l1k/linux/commit/85dc1f299dec5a52266d82e2f4c02adfb04f77e6


On Thu, Aug 27, 2015 at 04:43:43PM +0200, Lukas Wunner wrote:
> The active attribute in struct vga_switcheroo_client denotes whether
> the outputs are currently switched to this client. The attribute is
> only meaningful for vga clients. It is never used for audio clients.
> 
> The function vga_switcheroo_register_audio_client() misuses this
> attribute to store whether the audio device is fully initialized.
> Most likely there was a misunderstanding about the meaning of
> "active" when this was added.
> 
> Set the active attribute to false for audio clients. Remove the
> active parameter from vga_switcheroo_register_audio_client() and
> its sole caller, hda_intel.c:register_vga_switcheroo().
> 
> vga_switcheroo_register_audio_client() was introduced by 3e9e63dbd374
> ("vga_switcheroo: Add the support for audio clients"). Its use in
> hda_intel.c was introduced by a82d51ed24bb ("ALSA: hda - Support
> VGA-switcheroo").
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/vga/vga_switcheroo.c | 5 ++---
>  include/linux/vga_switcheroo.h   | 4 ++--
>  sound/pci/hda/hda_intel.c        | 3 +--
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index b19a72f..fe32536 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -290,7 +290,6 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>   * @pdev: client pci device
>   * @ops: client callbacks
>   * @id: client identifier, see enum vga_switcheroo_client_id
> - * @active: whether the audio device is fully initialized
>   *
>   * Register audio client (audio device on a GPU). The power state of the
>   * client is assumed to be ON.
> @@ -299,9 +298,9 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>   */
>  int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  					 const struct vga_switcheroo_client_ops *ops,
> -					 int id, bool active)
> +					 int id)
>  {
> -	return register_client(pdev, ops, id | ID_BIT_AUDIO, active, false);
> +	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, false);
>  }
>  EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
>  
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index fe90bfc..3764991 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -128,7 +128,7 @@ int vga_switcheroo_register_client(struct pci_dev *dev,
>  				   bool driver_power_control);
>  int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  					 const struct vga_switcheroo_client_ops *ops,
> -					 int id, bool active);
> +					 int id);
>  
>  void vga_switcheroo_client_fb_set(struct pci_dev *dev,
>  				  struct fb_info *info);
> @@ -154,7 +154,7 @@ static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_i
>  static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
>  static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	const struct vga_switcheroo_client_ops *ops,
> -	int id, bool active) { return 0; }
> +	int id) { return 0; }
>  static inline void vga_switcheroo_unregister_handler(void) {}
>  static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
>  static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c38c68f..e819013 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1143,8 +1143,7 @@ static int register_vga_switcheroo(struct azx *chip)
>  	 * is there any machine with two switchable HDMI audio controllers?
>  	 */
>  	err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
> -						    VGA_SWITCHEROO_DIS,
> -						    hda->probe_continued);
> +						   VGA_SWITCHEROO_DIS);
>  	if (err < 0)
>  		return err;
>  	hda->vga_switcheroo_registered = 1;
> -- 
> 1.8.5.2 (Apple Git-48)
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Takashi Iwai Sept. 19, 2015, 4:44 p.m. UTC | #2
On Thu, 27 Aug 2015 16:43:43 +0200,
Lukas Wunner wrote:
> 
> The active attribute in struct vga_switcheroo_client denotes whether
> the outputs are currently switched to this client. The attribute is
> only meaningful for vga clients. It is never used for audio clients.
> 
> The function vga_switcheroo_register_audio_client() misuses this
> attribute to store whether the audio device is fully initialized.
> Most likely there was a misunderstanding about the meaning of
> "active" when this was added.
> 
> Set the active attribute to false for audio clients. Remove the
> active parameter from vga_switcheroo_register_audio_client() and
> its sole caller, hda_intel.c:register_vga_switcheroo().
> 
> vga_switcheroo_register_audio_client() was introduced by 3e9e63dbd374
> ("vga_switcheroo: Add the support for audio clients"). Its use in
> hda_intel.c was introduced by a82d51ed24bb ("ALSA: hda - Support
> VGA-switcheroo").
> 
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Without the cover letter I can't judge why this change is needed, so
at the next time, please put relevant people to Cc of the cover letter
(or the whole series).

In anyway, I'm traveling now and can't review in details, so I'll
have a closer look in the next week after back from vacation.


thanks,

Takashi


> ---
>  drivers/gpu/vga/vga_switcheroo.c | 5 ++---
>  include/linux/vga_switcheroo.h   | 4 ++--
>  sound/pci/hda/hda_intel.c        | 3 +--
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index b19a72f..fe32536 100644
> --- a/drivers/gpu/vga/vga_switcheroo.c
> +++ b/drivers/gpu/vga/vga_switcheroo.c
> @@ -290,7 +290,6 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>   * @pdev: client pci device
>   * @ops: client callbacks
>   * @id: client identifier, see enum vga_switcheroo_client_id
> - * @active: whether the audio device is fully initialized
>   *
>   * Register audio client (audio device on a GPU). The power state of the
>   * client is assumed to be ON.
> @@ -299,9 +298,9 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
>   */
>  int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  					 const struct vga_switcheroo_client_ops *ops,
> -					 int id, bool active)
> +					 int id)
>  {
> -	return register_client(pdev, ops, id | ID_BIT_AUDIO, active, false);
> +	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, false);
>  }
>  EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
>  
> diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> index fe90bfc..3764991 100644
> --- a/include/linux/vga_switcheroo.h
> +++ b/include/linux/vga_switcheroo.h
> @@ -128,7 +128,7 @@ int vga_switcheroo_register_client(struct pci_dev *dev,
>  				   bool driver_power_control);
>  int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  					 const struct vga_switcheroo_client_ops *ops,
> -					 int id, bool active);
> +					 int id);
>  
>  void vga_switcheroo_client_fb_set(struct pci_dev *dev,
>  				  struct fb_info *info);
> @@ -154,7 +154,7 @@ static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_i
>  static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
>  static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
>  	const struct vga_switcheroo_client_ops *ops,
> -	int id, bool active) { return 0; }
> +	int id) { return 0; }
>  static inline void vga_switcheroo_unregister_handler(void) {}
>  static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
>  static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index c38c68f..e819013 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1143,8 +1143,7 @@ static int register_vga_switcheroo(struct azx *chip)
>  	 * is there any machine with two switchable HDMI audio controllers?
>  	 */
>  	err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
> -						    VGA_SWITCHEROO_DIS,
> -						    hda->probe_continued);
> +						   VGA_SWITCHEROO_DIS);
>  	if (err < 0)
>  		return err;
>  	hda->vga_switcheroo_registered = 1;
> -- 
> 1.8.5.2 (Apple Git-48)
>
Lukas Wunner Sept. 19, 2015, 5:50 p.m. UTC | #3
Hi Takashi,

On Sat, Sep 19, 2015 at 06:44:58PM +0200, Takashi Iwai wrote:
> Without the cover letter I can't judge why this change is needed, so
> at the next time, please put relevant people to Cc of the cover letter
> (or the whole series).

There was no cover letter, aside from this reply to Daniel Vetter:
http://lists.freedesktop.org/archives/dri-devel/2015-September/090510.html

Basically he had asked for vga_switcheroo docs, which I've now written,
and in course of doing that I fixed some bugs and cosmetic issues.
I cc'ed you on the docs patch and all the patches which touch ALSA code
so that you get a chance to raise objections (or ack the patches should
you have none). Full patch set is here:
https://github.com/l1k/linux/commits/vga_switcheroo_docs

The patch you've replied to (03/15 "vga_switcheroo: Set active attribute
to false for audio clients") fixes a misuse of the "active" attribute in
struct vga_switcheroo_client by audio clients. The attribute is never
used for audio clients, so there are no behavioral changes with this patch,
it only removes superfluous/confusing code.

Thanks,

Lukas
Daniel Vetter Sept. 22, 2015, 9:17 a.m. UTC | #4
On Thu, Sep 17, 2015 at 07:15:59PM +0200, Lukas Wunner wrote:
> After sending the below patch away I realized that this change allows
> us to simplify the code a bit by dropping the client_is_vga() call
> from find_active_client().
> 
> I force-pushed an amended version of the commit to my vga_switcheroo_docs
> branch on GitHub:
> https://github.com/l1k/linux/commit/85dc1f299dec5a52266d82e2f4c02adfb04f77e6

Please resubmit just the changed patche in-reply the previous version,
with an updated commit message.

Thanks, Daniel

> 
> 
> On Thu, Aug 27, 2015 at 04:43:43PM +0200, Lukas Wunner wrote:
> > The active attribute in struct vga_switcheroo_client denotes whether
> > the outputs are currently switched to this client. The attribute is
> > only meaningful for vga clients. It is never used for audio clients.
> > 
> > The function vga_switcheroo_register_audio_client() misuses this
> > attribute to store whether the audio device is fully initialized.
> > Most likely there was a misunderstanding about the meaning of
> > "active" when this was added.
> > 
> > Set the active attribute to false for audio clients. Remove the
> > active parameter from vga_switcheroo_register_audio_client() and
> > its sole caller, hda_intel.c:register_vga_switcheroo().
> > 
> > vga_switcheroo_register_audio_client() was introduced by 3e9e63dbd374
> > ("vga_switcheroo: Add the support for audio clients"). Its use in
> > hda_intel.c was introduced by a82d51ed24bb ("ALSA: hda - Support
> > VGA-switcheroo").
> > 
> > Cc: Takashi Iwai <tiwai@suse.de>
> > Signed-off-by: Lukas Wunner <lukas@wunner.de>
> > ---
> >  drivers/gpu/vga/vga_switcheroo.c | 5 ++---
> >  include/linux/vga_switcheroo.h   | 4 ++--
> >  sound/pci/hda/hda_intel.c        | 3 +--
> >  3 files changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> > index b19a72f..fe32536 100644
> > --- a/drivers/gpu/vga/vga_switcheroo.c
> > +++ b/drivers/gpu/vga/vga_switcheroo.c
> > @@ -290,7 +290,6 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
> >   * @pdev: client pci device
> >   * @ops: client callbacks
> >   * @id: client identifier, see enum vga_switcheroo_client_id
> > - * @active: whether the audio device is fully initialized
> >   *
> >   * Register audio client (audio device on a GPU). The power state of the
> >   * client is assumed to be ON.
> > @@ -299,9 +298,9 @@ EXPORT_SYMBOL(vga_switcheroo_register_client);
> >   */
> >  int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >  					 const struct vga_switcheroo_client_ops *ops,
> > -					 int id, bool active)
> > +					 int id)
> >  {
> > -	return register_client(pdev, ops, id | ID_BIT_AUDIO, active, false);
> > +	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, false);
> >  }
> >  EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
> >  
> > diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
> > index fe90bfc..3764991 100644
> > --- a/include/linux/vga_switcheroo.h
> > +++ b/include/linux/vga_switcheroo.h
> > @@ -128,7 +128,7 @@ int vga_switcheroo_register_client(struct pci_dev *dev,
> >  				   bool driver_power_control);
> >  int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >  					 const struct vga_switcheroo_client_ops *ops,
> > -					 int id, bool active);
> > +					 int id);
> >  
> >  void vga_switcheroo_client_fb_set(struct pci_dev *dev,
> >  				  struct fb_info *info);
> > @@ -154,7 +154,7 @@ static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_i
> >  static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
> >  static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
> >  	const struct vga_switcheroo_client_ops *ops,
> > -	int id, bool active) { return 0; }
> > +	int id) { return 0; }
> >  static inline void vga_switcheroo_unregister_handler(void) {}
> >  static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
> >  static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index c38c68f..e819013 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -1143,8 +1143,7 @@ static int register_vga_switcheroo(struct azx *chip)
> >  	 * is there any machine with two switchable HDMI audio controllers?
> >  	 */
> >  	err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
> > -						    VGA_SWITCHEROO_DIS,
> > -						    hda->probe_continued);
> > +						   VGA_SWITCHEROO_DIS);
> >  	if (err < 0)
> >  		return err;
> >  	hda->vga_switcheroo_registered = 1;
> > -- 
> > 1.8.5.2 (Apple Git-48)
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Takashi Iwai Sept. 24, 2015, 9:39 a.m. UTC | #5
On Sat, 19 Sep 2015 19:50:12 +0200,
Lukas Wunner wrote:
> 
> Hi Takashi,
> 
> On Sat, Sep 19, 2015 at 06:44:58PM +0200, Takashi Iwai wrote:
> > Without the cover letter I can't judge why this change is needed, so
> > at the next time, please put relevant people to Cc of the cover letter
> > (or the whole series).
> 
> There was no cover letter, aside from this reply to Daniel Vetter:
> http://lists.freedesktop.org/archives/dri-devel/2015-September/090510.html

Then you should have had it :)  A big patch series like this should
always have a cover letter.  Otherwise stray sheep like me gets lost
easily when Cc'ed out of sudden.


Takashi

Patch
diff mbox

diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index b19a72f..fe32536 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -290,7 +290,6 @@  EXPORT_SYMBOL(vga_switcheroo_register_client);
  * @pdev: client pci device
  * @ops: client callbacks
  * @id: client identifier, see enum vga_switcheroo_client_id
- * @active: whether the audio device is fully initialized
  *
  * Register audio client (audio device on a GPU). The power state of the
  * client is assumed to be ON.
@@ -299,9 +298,9 @@  EXPORT_SYMBOL(vga_switcheroo_register_client);
  */
 int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 					 const struct vga_switcheroo_client_ops *ops,
-					 int id, bool active)
+					 int id)
 {
-	return register_client(pdev, ops, id | ID_BIT_AUDIO, active, false);
+	return register_client(pdev, ops, id | ID_BIT_AUDIO, false, false);
 }
 EXPORT_SYMBOL(vga_switcheroo_register_audio_client);
 
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index fe90bfc..3764991 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -128,7 +128,7 @@  int vga_switcheroo_register_client(struct pci_dev *dev,
 				   bool driver_power_control);
 int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 					 const struct vga_switcheroo_client_ops *ops,
-					 int id, bool active);
+					 int id);
 
 void vga_switcheroo_client_fb_set(struct pci_dev *dev,
 				  struct fb_info *info);
@@ -154,7 +154,7 @@  static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_i
 static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
-	int id, bool active) { return 0; }
+	int id) { return 0; }
 static inline void vga_switcheroo_unregister_handler(void) {}
 static inline int vga_switcheroo_process_delayed_switch(void) { return 0; }
 static inline int vga_switcheroo_get_client_state(struct pci_dev *dev) { return VGA_SWITCHEROO_ON; }
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index c38c68f..e819013 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1143,8 +1143,7 @@  static int register_vga_switcheroo(struct azx *chip)
 	 * is there any machine with two switchable HDMI audio controllers?
 	 */
 	err = vga_switcheroo_register_audio_client(chip->pci, &azx_vs_ops,
-						    VGA_SWITCHEROO_DIS,
-						    hda->probe_continued);
+						   VGA_SWITCHEROO_DIS);
 	if (err < 0)
 		return err;
 	hda->vga_switcheroo_registered = 1;