[PATCH 1/3] lx6464es: remove useles pci_set_drvdata(pdev, NULL)

[PATCH 1/3] lx6464es: remove useles pci_set_drvdata(pdev, NULL)

[Andy Shevchenko]

The driver core handles this.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/pci/lx6464es/lx6464es.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
index 298bc9b..3230e57 100644
--- a/sound/pci/lx6464es/lx6464es.c
+++ b/sound/pci/lx6464es/lx6464es.c
@@ -1139,7 +1139,6 @@ out_free:
 static void snd_lx6464es_remove(struct pci_dev *pci)
 {
 	snd_card_free(pci_get_drvdata(pci));
-	pci_set_drvdata(pci, NULL);
 }
 
 
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 2/3] lx6464es: dump MAC in standard form

[Andy Shevchenko]

Let's use %pM to print MAC wherever we need it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/pci/lx6464es/lx6464es.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
index 3230e57..33eb301 100644
--- a/sound/pci/lx6464es/lx6464es.c
+++ b/sound/pci/lx6464es/lx6464es.c
@@ -798,9 +798,7 @@ static int lx_init_dsp(struct lx6464es *chip)
 
 mac_ready:
 	snd_printd(LXP "mac address ready read after: %dms\n", i);
-	snd_printk(LXP "mac address: %02X.%02X.%02X.%02X.%02X.%02X\n",
-		   chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
-		   chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
+	snd_printk(LXP "mac address: %pM\n", chip->mac_address);
 
 	err = lx_init_get_version_features(chip);
 	if (err)
@@ -1113,9 +1111,7 @@ static int snd_lx6464es_probe(struct pci_dev *pci,
 	sprintf(card->id, "LX6464ES_%02X%02X%02X",
 		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
 
-	sprintf(card->shortname, "LX6464ES %02X.%02X.%02X.%02X.%02X.%02X",
-		chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
-		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
+	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);
 
 	sprintf(card->longname, "%s at 0x%lx, 0x%p, irq %i",
 		card->shortname, chip->port_plx,
-- 
1.8.2.rc0.22.gb3600c3

[PATCH 3/3] lx6464es: use %*phN to print small buffer in hex form

[Andy Shevchenko]

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 sound/pci/lx6464es/lx6464es.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
index 33eb301..7ad98f8 100644
--- a/sound/pci/lx6464es/lx6464es.c
+++ b/sound/pci/lx6464es/lx6464es.c
@@ -1108,8 +1108,7 @@ static int snd_lx6464es_probe(struct pci_dev *pci,
 	}
 
 	strcpy(card->driver, "LX6464ES");
-	sprintf(card->id, "LX6464ES_%02X%02X%02X",
-		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
+	sprintf(card->id, "LX6464ES_%3phN", chip->mac_address + 3);
 
 	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);
 
-- 
1.8.2.rc0.22.gb3600c3

Re: [PATCH 2/3] lx6464es: dump MAC in standard form

[Takashi Iwai]

At Wed, 29 May 2013 13:03:21 +0300,
Andy Shevchenko wrote:
> 
> Let's use %pM to print MAC wherever we need it.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  sound/pci/lx6464es/lx6464es.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
> index 3230e57..33eb301 100644
> --- a/sound/pci/lx6464es/lx6464es.c
> +++ b/sound/pci/lx6464es/lx6464es.c
> @@ -798,9 +798,7 @@ static int lx_init_dsp(struct lx6464es *chip)
>  
>  mac_ready:
>  	snd_printd(LXP "mac address ready read after: %dms\n", i);
> -	snd_printk(LXP "mac address: %02X.%02X.%02X.%02X.%02X.%02X\n",
> -		   chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> -		   chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> +	snd_printk(LXP "mac address: %pM\n", chip->mac_address);
>  
>  	err = lx_init_get_version_features(chip);
>  	if (err)
> @@ -1113,9 +1111,7 @@ static int snd_lx6464es_probe(struct pci_dev *pci,
>  	sprintf(card->id, "LX6464ES_%02X%02X%02X",
>  		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
>  
> -	sprintf(card->shortname, "LX6464ES %02X.%02X.%02X.%02X.%02X.%02X",
> -		chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> +	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);

This will change from the upper to the lower case, thus it'll lead to
an incompatible name string, unfortunately.


Takashi

Re: [PATCH 3/3] lx6464es: use %*phN to print small buffer in hex form

[Takashi Iwai]

At Wed, 29 May 2013 13:03:22 +0300,
Andy Shevchenko wrote:
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  sound/pci/lx6464es/lx6464es.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
> index 33eb301..7ad98f8 100644
> --- a/sound/pci/lx6464es/lx6464es.c
> +++ b/sound/pci/lx6464es/lx6464es.c
> @@ -1108,8 +1108,7 @@ static int snd_lx6464es_probe(struct pci_dev *pci,
>  	}
>  
>  	strcpy(card->driver, "LX6464ES");
> -	sprintf(card->id, "LX6464ES_%02X%02X%02X",
> -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> +	sprintf(card->id, "LX6464ES_%3phN", chip->mac_address + 3);

The same problem as my previous comment.  The case incompatible.


thanks,

Takashi

Re: [PATCH 1/3] lx6464es: remove useles pci_set_drvdata(pdev, NULL)

[Takashi Iwai]

At Wed, 29 May 2013 13:03:20 +0300,
Andy Shevchenko wrote:
> 
> The driver core handles this.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks, but I already queued a clean-up patch to remove the whole
pci_set_drvdata() with NULL for all sound/* pci drivers.


Takashi

> ---
>  sound/pci/lx6464es/lx6464es.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
> index 298bc9b..3230e57 100644
> --- a/sound/pci/lx6464es/lx6464es.c
> +++ b/sound/pci/lx6464es/lx6464es.c
> @@ -1139,7 +1139,6 @@ out_free:
>  static void snd_lx6464es_remove(struct pci_dev *pci)
>  {
>  	snd_card_free(pci_get_drvdata(pci));
> -	pci_set_drvdata(pci, NULL);
>  }
>  
>  
> -- 
> 1.8.2.rc0.22.gb3600c3
> 

Re: [PATCH 2/3] lx6464es: dump MAC in standard form

[Andy Shevchenko]

On Wed, 2013-05-29 at 12:15 +0200, Takashi Iwai wrote: 
> At Wed, 29 May 2013 13:03:21 +0300,
> Andy Shevchenko wrote:
> > 
> > Let's use %pM to print MAC wherever we need it.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  sound/pci/lx6464es/lx6464es.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
> > index 3230e57..33eb301 100644
> > --- a/sound/pci/lx6464es/lx6464es.c
> > +++ b/sound/pci/lx6464es/lx6464es.c
> > @@ -798,9 +798,7 @@ static int lx_init_dsp(struct lx6464es *chip)
> >  
> >  mac_ready:
> >  	snd_printd(LXP "mac address ready read after: %dms\n", i);
> > -	snd_printk(LXP "mac address: %02X.%02X.%02X.%02X.%02X.%02X\n",
> > -		   chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> > -		   chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > +	snd_printk(LXP "mac address: %pM\n", chip->mac_address);
> >  
> >  	err = lx_init_get_version_features(chip);
> >  	if (err)
> > @@ -1113,9 +1111,7 @@ static int snd_lx6464es_probe(struct pci_dev *pci,
> >  	sprintf(card->id, "LX6464ES_%02X%02X%02X",
> >  		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> >  
> > -	sprintf(card->shortname, "LX6464ES %02X.%02X.%02X.%02X.%02X.%02X",
> > -		chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> > -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > +	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);
> 
> This will change from the upper to the lower case, thus it'll lead to
> an incompatible name string, unfortunately.

Who is the user of this string?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] lx6464es: dump MAC in standard form

[Takashi Iwai]

At Wed, 29 May 2013 14:00:57 +0300,
Andy Shevchenko wrote:
> 
> On Wed, 2013-05-29 at 12:15 +0200, Takashi Iwai wrote: 
> > At Wed, 29 May 2013 13:03:21 +0300,
> > Andy Shevchenko wrote:
> > > 
> > > Let's use %pM to print MAC wherever we need it.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  sound/pci/lx6464es/lx6464es.c | 8 ++------
> > >  1 file changed, 2 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/sound/pci/lx6464es/lx6464es.c b/sound/pci/lx6464es/lx6464es.c
> > > index 3230e57..33eb301 100644
> > > --- a/sound/pci/lx6464es/lx6464es.c
> > > +++ b/sound/pci/lx6464es/lx6464es.c
> > > @@ -798,9 +798,7 @@ static int lx_init_dsp(struct lx6464es *chip)
> > >  
> > >  mac_ready:
> > >  	snd_printd(LXP "mac address ready read after: %dms\n", i);
> > > -	snd_printk(LXP "mac address: %02X.%02X.%02X.%02X.%02X.%02X\n",
> > > -		   chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> > > -		   chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > > +	snd_printk(LXP "mac address: %pM\n", chip->mac_address);
> > >  
> > >  	err = lx_init_get_version_features(chip);
> > >  	if (err)
> > > @@ -1113,9 +1111,7 @@ static int snd_lx6464es_probe(struct pci_dev *pci,
> > >  	sprintf(card->id, "LX6464ES_%02X%02X%02X",
> > >  		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > >  
> > > -	sprintf(card->shortname, "LX6464ES %02X.%02X.%02X.%02X.%02X.%02X",
> > > -		chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> > > -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > > +	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);
> > 
> > This will change from the upper to the lower case, thus it'll lead to
> > an incompatible name string, unfortunately.
> 
> Who is the user of this string?

It's exposed to the user-space via ioctl and it can be used as an id
string.  So this will break user-space compatibility.


Takashi

Re: [PATCH 3/3] lx6464es: use %*phN to print small buffer in hex form

[Andy Shevchenko]

On Wed, 2013-05-29 at 12:16 +0200, Takashi Iwai wrote: 
> >  	strcpy(card->driver, "LX6464ES");
> > -	sprintf(card->id, "LX6464ES_%02X%02X%02X",
> > -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > +	sprintf(card->id, "LX6464ES_%3phN", chip->mac_address + 3);
> 
> The same problem as my previous comment.  The case incompatible.

Here is card->id, which might be used by userspace in defined form.
I rather agree with your objection here.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] lx6464es: dump MAC in standard form

[Andy Shevchenko]

On Wed, 2013-05-29 at 13:03 +0200, Takashi Iwai wrote:

> > > > -	sprintf(card->shortname, "LX6464ES %02X.%02X.%02X.%02X.%02X.%02X",
> > > > -		chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> > > > -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > > > +	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);
> > > 
> > > This will change from the upper to the lower case, thus it'll lead to
> > > an incompatible name string, unfortunately.
> > 
> > Who is the user of this string?
> 
> It's exposed to the user-space via ioctl and it can be used as an id
> string.  So this will break user-space compatibility.

MAC is theoretically set of arbitrary 6 bytes.
Only what I can see here is the difference between '.' and ':' as a
delimiter. But the MAC address form is defined in IEEE 802 standards.

Anyway, I am okay if you reject this one.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/3] lx6464es: dump MAC in standard form

[Takashi Iwai]

At Wed, 29 May 2013 14:31:07 +0300,
Andy Shevchenko wrote:
> 
> On Wed, 2013-05-29 at 13:03 +0200, Takashi Iwai wrote:
> 
> > > > > -	sprintf(card->shortname, "LX6464ES %02X.%02X.%02X.%02X.%02X.%02X",
> > > > > -		chip->mac_address[0], chip->mac_address[1], chip->mac_address[2],
> > > > > -		chip->mac_address[3], chip->mac_address[4], chip->mac_address[5]);
> > > > > +	sprintf(card->shortname, "LX6464ES %pM", chip->mac_address);
> > > > 
> > > > This will change from the upper to the lower case, thus it'll lead to
> > > > an incompatible name string, unfortunately.
> > > 
> > > Who is the user of this string?
> > 
> > It's exposed to the user-space via ioctl and it can be used as an id
> > string.  So this will break user-space compatibility.
> 
> MAC is theoretically set of arbitrary 6 bytes.
> Only what I can see here is the difference between '.' and ':' as a
> delimiter. But the MAC address form is defined in IEEE 802 standards.

It doesn't matter whether it's a valid MAC representation or not.
The problem is only that this patch will change the string
representation, thus it becomes incompatible with older kernels.
That's the only point.

> Anyway, I am okay if you reject this one.

Good :)


thanks,

Takashi

> -- 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
>