[PATCH v3 1/3] portman2x4 - whitespace fixes

[PATCH v3 1/3] portman2x4 - whitespace fixes

[Sudip Mukherjee]

To improve the readability and grep-ability of the code trailing
whitespace, multiple blank lines and space before tabs have been
removed. This patch takes care of these issues related to whitespace
changes and there was no other change.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v3: changed commit message
v2: combined 3 patches of v1 series into this patch.

 sound/drivers/portman2x4.c | 77 ++++++++++++++++++++++------------------------
 1 file changed, 37 insertions(+), 40 deletions(-)

diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
index 464385a..65b0650 100644
--- a/sound/drivers/portman2x4.c
+++ b/sound/drivers/portman2x4.c
@@ -57,7 +57,7 @@ static int index[SNDRV_CARDS]  = SNDRV_DEFAULT_IDX;
 static char *id[SNDRV_CARDS]   = SNDRV_DEFAULT_STR;
 static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;
 
-static struct platform_device *platform_devices[SNDRV_CARDS]; 
+static struct platform_device *platform_devices[SNDRV_CARDS];
 static int device_count;
 
 module_param_array(index, int, NULL, S_IRUGO);
@@ -105,7 +105,7 @@ static int portman_create(struct snd_card *card,
 	*rchip = NULL;
 
 	pm = kzalloc(sizeof(struct portman), GFP_KERNEL);
-	if (pm == NULL) 
+	if (pm == NULL)
 		return -ENOMEM;
 
 	/* Init chip specific data */
@@ -123,24 +123,24 @@ static int portman_create(struct snd_card *card,
  *********************************************************************/
 
 /* Standard PC parallel port status register equates. */
-#define	PP_STAT_BSY   	0x80	/* Busy status.  Inverted. */
-#define	PP_STAT_ACK   	0x40	/* Acknowledge.  Non-Inverted. */
-#define	PP_STAT_POUT  	0x20	/* Paper Out.    Non-Inverted. */
-#define	PP_STAT_SEL   	0x10	/* Select.       Non-Inverted. */
-#define	PP_STAT_ERR   	0x08	/* Error.        Non-Inverted. */
+#define	PP_STAT_BSY	0x80	/* Busy status.  Inverted. */
+#define	PP_STAT_ACK	0x40	/* Acknowledge.  Non-Inverted. */
+#define	PP_STAT_POUT	0x20	/* Paper Out.    Non-Inverted. */
+#define	PP_STAT_SEL	0x10	/* Select.       Non-Inverted. */
+#define	PP_STAT_ERR	0x08	/* Error.        Non-Inverted. */
 
 /* Standard PC parallel port command register equates. */
-#define	PP_CMD_IEN  	0x10	/* IRQ Enable.   Non-Inverted. */
-#define	PP_CMD_SELI 	0x08	/* Select Input. Inverted. */
-#define	PP_CMD_INIT 	0x04	/* Init Printer. Non-Inverted. */
-#define	PP_CMD_FEED 	0x02	/* Auto Feed.    Inverted. */
+#define	PP_CMD_IEN	0x10	/* IRQ Enable.   Non-Inverted. */
+#define	PP_CMD_SELI	0x08	/* Select Input. Inverted. */
+#define	PP_CMD_INIT	0x04	/* Init Printer. Non-Inverted. */
+#define	PP_CMD_FEED	0x02	/* Auto Feed.    Inverted. */
 #define	PP_CMD_STB      0x01	/* Strobe.       Inverted. */
 
 /* Parallel Port Command Register as implemented by PCP2x4. */
-#define	INT_EN	 	PP_CMD_IEN	/* Interrupt enable. */
+#define	INT_EN		PP_CMD_IEN	/* Interrupt enable. */
 #define	STROBE	        PP_CMD_STB	/* Command strobe. */
 
-/* The parallel port command register field (b1..b3) selects the 
+/* The parallel port command register field (b1..b3) selects the
  * various "registers" within the PC/P 2x4.  These are the internal
  * address of these "registers" that must be written to the parallel
  * port command register.
@@ -148,7 +148,7 @@ static int portman_create(struct snd_card *card,
 #define	RXDATA0		(0 << 1)	/* PCP RxData channel 0. */
 #define	RXDATA1		(1 << 1)	/* PCP RxData channel 1. */
 #define	GEN_CTL		(2 << 1)	/* PCP General Control Register. */
-#define	SYNC_CTL 	(3 << 1)	/* PCP Sync Control Register. */
+#define	SYNC_CTL	(3 << 1)	/* PCP Sync Control Register. */
 #define	TXDATA0		(4 << 1)	/* PCP TxData channel 0. */
 #define	TXDATA1		(5 << 1)	/* PCP TxData channel 1. */
 #define	TXDATA2		(6 << 1)	/* PCP TxData channel 2. */
@@ -218,7 +218,7 @@ static inline void portman_write_data(struct portman *pm, u8 value)
 	parport_write_data(pm->pardev->port, value);
 }
 
-static void portman_write_midi(struct portman *pm, 
+static void portman_write_midi(struct portman *pm,
 			       int port, u8 mididata)
 {
 	int command = ((port + 4) << 1);
@@ -234,20 +234,20 @@ static void portman_write_midi(struct portman *pm,
 	 */
 	command |= INT_EN;
 
-	/* Disable interrupts so that the process is not interrupted, then 
-	 * write the address associated with the current Tx channel to the 
+	/* Disable interrupts so that the process is not interrupted, then
+	 * write the address associated with the current Tx channel to the
 	 * PP Command Reg.  Do not set the Strobe signal yet.
 	 */
 
 	do {
 		portman_write_command(pm, command);
 
-		/* While the address lines settle, write parallel output data to 
+		/* While the address lines settle, write parallel output data to
 		 * PP Data Reg.  This has no effect until Strobe signal is asserted.
 		 */
 
 		portman_write_data(pm, mididata);
-		
+
 		/* If PCP channel's TxEmpty is set (TxEmpty is read through the PP
 		 * Status Register), then go write data.  Else go back and wait.
 		 */
@@ -284,7 +284,6 @@ static void portman_write_midi(struct portman *pm,
 	/* Data sent. */
 }
 
-
 /*
  *  Read MIDI byte from port
  *  Attempt to read input byte from specified hardware input port (0..).
@@ -357,7 +356,6 @@ static int portman_read_midi(struct portman *pm, int port)
 	portman_write_data(pm, 1);	/* Cause rising edge, which shifts data. */
 	portman_write_data(pm, 0);	/* Return data clock low. */
 
-
 	/* De-assert Strobe and return data. */
 	portman_write_command(pm, cmdout);	/* Output saved address+IE. */
 
@@ -370,11 +368,12 @@ static int portman_read_midi(struct portman *pm, int port)
 
 /*
  *  Checks if any input data on the given channel is available
- *  Checks RxAvail 
+ *  Checks RxAvail
  */
 static int portman_data_avail(struct portman *pm, int channel)
 {
 	int command = INT_EN;
+
 	switch (channel) {
 	case 0:
 		command |= RXDATA0;
@@ -393,7 +392,6 @@ static int portman_data_avail(struct portman *pm, int channel)
 	return 0;
 }
 
-
 /*
  *  Flushes any input
  */
@@ -456,7 +454,7 @@ static int portman_probe(struct parport *p)
 	 * hardware handshake lines to midi box:
 	 *
 	 *                                  Strobe = 0
-	 *                                  Interrupt Enable = 0            
+	 *                                  Interrupt Enable = 0
 	 */
 	/* 2 */
 	parport_write_control(p, 0);
@@ -481,7 +479,7 @@ static int portman_probe(struct parport *p)
 	/* 7 */
 	parport_write_control(p, 0);	/* Reset Strobe=0. */
 
-	/* Check if Tx circuitry is functioning properly.  If initialized 
+	/* Check if Tx circuitry is functioning properly.  If initialized
 	 * unit TxEmpty is false, send out char and see if if goes true.
 	 */
 	/* 8 */
@@ -567,12 +565,12 @@ static int snd_portman_rawmidi_create(struct snd_card *card)
 	struct snd_rawmidi *rmidi;
 	struct snd_rawmidi_substream *substream;
 	int err;
-	
-	err = snd_rawmidi_new(card, CARD_NAME, 0, 
-			      PORTMAN_NUM_OUTPUT_PORTS, 
-			      PORTMAN_NUM_INPUT_PORTS, 
+
+	err = snd_rawmidi_new(card, CARD_NAME, 0,
+			      PORTMAN_NUM_OUTPUT_PORTS,
+			      PORTMAN_NUM_INPUT_PORTS,
 			      &rmidi);
-	if (err < 0) 
+	if (err < 0)
 		return err;
 
 	rmidi->private_data = pm;
@@ -584,9 +582,9 @@ static int snd_portman_rawmidi_create(struct snd_card *card)
 	pm->rmidi = rmidi;
 
 	/* register rawmidi ops */
-	snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_OUTPUT, 
+	snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_OUTPUT,
 			    &snd_portman_midi_output);
-	snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_INPUT, 
+	snd_rawmidi_set_ops(rmidi, SNDRV_RAWMIDI_STREAM_INPUT,
 			    &snd_portman_midi_input);
 
 	/* name substreams */
@@ -621,7 +619,7 @@ static void snd_portman_interrupt(void *userdata)
 
 	/* While any input data is waiting */
 	while ((portman_read_status(pm) & INT_REQ) == INT_REQ) {
-		/* If data available on channel 0, 
+		/* If data available on channel 0,
 		   read it and stuff it into the queue. */
 		if (portman_data_avail(pm, 0)) {
 			/* Read Midi */
@@ -632,7 +630,7 @@ static void snd_portman_interrupt(void *userdata)
 						    &midivalue, 1);
 
 		}
-		/* If data available on channel 1, 
+		/* If data available on channel 1,
 		   read it and stuff it into the queue. */
 		if (portman_data_avail(pm, 1)) {
 			/* Read Midi */
@@ -658,7 +656,7 @@ static int snd_portman_probe_port(struct parport *p)
 					 0, NULL);
 	if (!pardev)
 		return -EIO;
-	
+
 	if (parport_claim(pardev)) {
 		parport_unregister_device(pardev);
 		return -EIO;
@@ -742,7 +740,7 @@ static int snd_portman_probe(struct platform_device *pdev)
 
 	if (dev >= SNDRV_CARDS)
 		return -ENODEV;
-	if (!enable[dev]) 
+	if (!enable[dev])
 		return -ENOENT;
 
 	if ((err = snd_portman_probe_port(p)) < 0)
@@ -756,7 +754,7 @@ static int snd_portman_probe(struct platform_device *pdev)
 	}
 	strcpy(card->driver, DRIVER_NAME);
 	strcpy(card->shortname, CARD_NAME);
-	sprintf(card->longname,  "%s at 0x%lx, irq %i", 
+	sprintf(card->longname,  "%s at 0x%lx, irq %i",
 		card->shortname, p->base, p->irq);
 
 	pardev = parport_register_device(p,                     /* port */
@@ -779,7 +777,7 @@ static int snd_portman_probe(struct platform_device *pdev)
 	}
 	card->private_data = pm;
 	card->private_free = snd_portman_card_private_free;
-	
+
 	if ((err = snd_portman_rawmidi_create(card)) < 0) {
 		snd_printd("Creating Rawmidi component failed\n");
 		goto __err;
@@ -823,7 +821,6 @@ static int snd_portman_remove(struct platform_device *pdev)
 	return 0;
 }
 
-
 static struct platform_driver snd_portman_driver = {
 	.probe  = snd_portman_probe,
 	.remove = snd_portman_remove,
@@ -844,7 +841,7 @@ static void snd_portman_unregister_all(void)
 			platform_device_unregister(platform_devices[i]);
 			platform_devices[i] = NULL;
 		}
-	}		
+	}
 	platform_driver_unregister(&snd_portman_driver);
 	parport_unregister_driver(&portman_parport_driver);
 }
-- 
1.9.1

[PATCH v3 2/3] portman2x4 - assignment in if

[Sudip Mukherjee]

It is not general practice to assign some values inside if statement.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v3: no change
v2: no change

 sound/drivers/portman2x4.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
index 65b0650..172685d 100644
--- a/sound/drivers/portman2x4.c
+++ b/sound/drivers/portman2x4.c
@@ -743,7 +743,8 @@ static int snd_portman_probe(struct platform_device *pdev)
 	if (!enable[dev])
 		return -ENOENT;
 
-	if ((err = snd_portman_probe_port(p)) < 0)
+	err = snd_portman_probe_port(p);
+	if (err < 0)
 		return err;
 
 	err = snd_card_new(&pdev->dev, index[dev], id[dev], THIS_MODULE,
@@ -770,7 +771,8 @@ static int snd_portman_probe(struct platform_device *pdev)
 		goto __err;
 	}
 
-	if ((err = portman_create(card, pardev, &pm)) < 0) {
+	err = portman_create(card, pardev, &pm);
+	if (err < 0) {
 		snd_printd("Cannot create main component\n");
 		parport_unregister_device(pardev);
 		goto __err;
@@ -778,7 +780,8 @@ static int snd_portman_probe(struct platform_device *pdev)
 	card->private_data = pm;
 	card->private_free = snd_portman_card_private_free;
 
-	if ((err = snd_portman_rawmidi_create(card)) < 0) {
+	err = snd_portman_rawmidi_create(card);
+	if (err < 0) {
 		snd_printd("Creating Rawmidi component failed\n");
 		goto __err;
 	}
@@ -792,13 +795,15 @@ static int snd_portman_probe(struct platform_device *pdev)
 	pm->pardev_claimed = 1;
 
 	/* init device */
-	if ((err = portman_device_init(pm)) < 0)
+	err = portman_device_init(pm);
+	if (err < 0)
 		goto __err;
 
 	platform_set_drvdata(pdev, card);
 
 	/* At this point card will be usable */
-	if ((err = snd_card_register(card)) < 0) {
+	err = snd_card_register(card);
+	if (err < 0) {
 		snd_printd("Cannot register card\n");
 		goto __err;
 	}
@@ -850,7 +855,8 @@ static int __init snd_portman_module_init(void)
 {
 	int err;
 
-	if ((err = platform_driver_register(&snd_portman_driver)) < 0)
+	err = platform_driver_register(&snd_portman_driver);
+	if (err < 0)
 		return err;
 
 	if (parport_register_driver(&portman_parport_driver) != 0) {
-- 
1.9.1

[PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

Modify portman driver to use the new parallel port device model.
The advantage of using the device model is that the device gets binded
to the hardware, we get the feature of hotplug, we can bind/unbind
the driver at runtime.
The only change is in the way the driver gets registered with the
parallel port subsystem and so as a result there is no user visible
change or any chance of regression.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

v3: changed commit message
v2:
 1. pardev_cb is initialized while declaring, thus removing the use of
memset.
 2. used pdev->id.
 3. v1 did not have the parport probe callback, but
we will need the probe callback for binding as the name of the driver
and the name of the device is different.
 4. in v1 I missed modifying snd_portman_probe_port().

 sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 14 deletions(-)

diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
index 172685d..a22f56c 100644
--- a/sound/drivers/portman2x4.c
+++ b/sound/drivers/portman2x4.c
@@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
 {
 	struct pardevice *pardev;
 	int res;
-
-	pardev = parport_register_device(p, DRIVER_NAME,
-					 NULL, NULL, NULL,
-					 0, NULL);
+	struct pardev_cb pdev_cb = {
+		.preempt = NULL,
+		.wakeup = NULL,
+		.private = NULL,
+		.irq_func = NULL,
+		.flags = 0,
+	};
+
+	/*
+	 * Specify the device number as SNDRV_CARDS + 1 so that the
+	 * device id alloted to this temporary device will never clash
+	 * with an actual device already registered.
+	 */
+	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
+					    SNDRV_CARDS + 1);
 	if (!pardev)
 		return -EIO;
 
@@ -703,10 +714,20 @@ static void snd_portman_detach(struct parport *p)
 	/* nothing to do here */
 }
 
+static int snd_portman_dev_probe(struct pardevice *pardev)
+{
+	if (strcmp(pardev->name, DRIVER_NAME))
+		return -ENODEV;
+
+	return 0;
+}
+
 static struct parport_driver portman_parport_driver = {
-	.name   = "portman2x4",
-	.attach = snd_portman_attach,
-	.detach = snd_portman_detach
+	.name		= "portman2x4",
+	.probe		= snd_portman_dev_probe,
+	.match_port	= snd_portman_attach,
+	.detach		= snd_portman_detach,
+	.devmodel	= true,
 };
 
 /*********************************************************************
@@ -734,6 +755,12 @@ static int snd_portman_probe(struct platform_device *pdev)
 	struct snd_card *card = NULL;
 	struct portman *pm = NULL;
 	int err;
+	struct pardev_cb portman_cb = {
+		.preempt = NULL,
+		.wakeup = NULL,
+		.irq_func = snd_portman_interrupt,	/* ISR */
+		.flags = PARPORT_DEV_EXCL,		/* flags */
+	};
 
 	p = platform_get_drvdata(pdev);
 	platform_set_drvdata(pdev, NULL);
@@ -758,13 +785,11 @@ static int snd_portman_probe(struct platform_device *pdev)
 	sprintf(card->longname,  "%s at 0x%lx, irq %i",
 		card->shortname, p->base, p->irq);
 
-	pardev = parport_register_device(p,                     /* port */
-					 DRIVER_NAME,           /* name */
-					 NULL,                  /* preempt */
-					 NULL,                  /* wakeup */
-					 snd_portman_interrupt, /* ISR */
-					 PARPORT_DEV_EXCL,      /* flags */
-					 (void *)card);         /* private */
+	portman_cb.private = card;			   /* private */
+	pardev = parport_register_dev_model(p,		   /* port */
+					    DRIVER_NAME,   /* name */
+					    &portman_cb,   /* callbacks */
+					    pdev->id);	   /* device number */
 	if (pardev == NULL) {
 		snd_printd("Cannot register pardevice\n");
 		err = -EIO;
-- 
1.9.1

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Thu, 04 Feb 2016 17:38:23 +0100,
Sudip Mukherjee wrote:
> 
> Modify portman driver to use the new parallel port device model.
> The advantage of using the device model is that the device gets binded
> to the hardware, we get the feature of hotplug, we can bind/unbind
> the driver at runtime.
> The only change is in the way the driver gets registered with the
> parallel port subsystem and so as a result there is no user visible
> change or any chance of regression.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> v3: changed commit message
> v2:
>  1. pardev_cb is initialized while declaring, thus removing the use of
> memset.
>  2. used pdev->id.
>  3. v1 did not have the parport probe callback, but
> we will need the probe callback for binding as the name of the driver
> and the name of the device is different.
>  4. in v1 I missed modifying snd_portman_probe_port().
> 
>  sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> index 172685d..a22f56c 100644
> --- a/sound/drivers/portman2x4.c
> +++ b/sound/drivers/portman2x4.c
> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>  {
>  	struct pardevice *pardev;
>  	int res;
> -
> -	pardev = parport_register_device(p, DRIVER_NAME,
> -					 NULL, NULL, NULL,
> -					 0, NULL);
> +	struct pardev_cb pdev_cb = {
> +		.preempt = NULL,
> +		.wakeup = NULL,
> +		.private = NULL,
> +		.irq_func = NULL,
> +		.flags = 0,
> +	};
> +
> +	/*
> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> +	 * device id alloted to this temporary device will never clash
> +	 * with an actual device already registered.
> +	 */
> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> +					    SNDRV_CARDS + 1);

Hmm, doesn't this result in a device name like "xxx.33" ?
Also, what if multiple portman devices are registered?

I suppose rather it should be passing dev variable from
snd_portman_probe()?


thanks,

Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Thu, 04 Feb 2016 17:38:23 +0100,
Sudip Mukherjee wrote:
> 
> Modify portman driver to use the new parallel port device model.
> The advantage of using the device model is that the device gets binded
> to the hardware, we get the feature of hotplug, we can bind/unbind
> the driver at runtime.
> The only change is in the way the driver gets registered with the
> parallel port subsystem and so as a result there is no user visible
> change or any chance of regression.
> 
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
> 
> v3: changed commit message
> v2:
>  1. pardev_cb is initialized while declaring, thus removing the use of
> memset.
>  2. used pdev->id.
>  3. v1 did not have the parport probe callback, but
> we will need the probe callback for binding as the name of the driver
> and the name of the device is different.
>  4. in v1 I missed modifying snd_portman_probe_port().
> 
>  sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> index 172685d..a22f56c 100644
> --- a/sound/drivers/portman2x4.c
> +++ b/sound/drivers/portman2x4.c
> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>  {
>  	struct pardevice *pardev;
>  	int res;
> -
> -	pardev = parport_register_device(p, DRIVER_NAME,
> -					 NULL, NULL, NULL,
> -					 0, NULL);
> +	struct pardev_cb pdev_cb = {
> +		.preempt = NULL,
> +		.wakeup = NULL,
> +		.private = NULL,
> +		.irq_func = NULL,
> +		.flags = 0,
> +	};
> +
> +	/*
> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> +	 * device id alloted to this temporary device will never clash
> +	 * with an actual device already registered.
> +	 */
> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> +					    SNDRV_CARDS + 1);

Hmm, doesn't this result in a device name like "xxx.33" ?
Also, what if multiple portman devices are registered?

I suppose rather it should be passing dev variable from
snd_portman_probe()?


thanks,

Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> On Thu, 04 Feb 2016 17:38:23 +0100,
> Sudip Mukherjee wrote:
> > 
> > Modify portman driver to use the new parallel port device model.
> > The advantage of using the device model is that the device gets binded
> > to the hardware, we get the feature of hotplug, we can bind/unbind
> > the driver at runtime.
> > The only change is in the way the driver gets registered with the
> > parallel port subsystem and so as a result there is no user visible
> > change or any chance of regression.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> > v3: changed commit message
> > v2:
> >  1. pardev_cb is initialized while declaring, thus removing the use of
> > memset.
> >  2. used pdev->id.
> >  3. v1 did not have the parport probe callback, but
> > we will need the probe callback for binding as the name of the driver
> > and the name of the device is different.
> >  4. in v1 I missed modifying snd_portman_probe_port().
> > 
> >  sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 39 insertions(+), 14 deletions(-)
> > 
> > diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> > index 172685d..a22f56c 100644
> > --- a/sound/drivers/portman2x4.c
> > +++ b/sound/drivers/portman2x4.c
> > @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> >  {
> >  	struct pardevice *pardev;
> >  	int res;
> > -
> > -	pardev = parport_register_device(p, DRIVER_NAME,
> > -					 NULL, NULL, NULL,
> > -					 0, NULL);
> > +	struct pardev_cb pdev_cb = {
> > +		.preempt = NULL,
> > +		.wakeup = NULL,
> > +		.private = NULL,
> > +		.irq_func = NULL,
> > +		.flags = 0,
> > +	};
> > +
> > +	/*
> > +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> > +	 * device id alloted to this temporary device will never clash
> > +	 * with an actual device already registered.
> > +	 */
> > +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> > +					    SNDRV_CARDS + 1);
> 
> Hmm, doesn't this result in a device name like "xxx.33" ?

yes, it will. But this is a temoporary device just to check if the
sound card is connected to that particular parallel port or not. After
checking this device is immediately unregistered. My idea here was to
have a device number which will never clash with another device number.
And we can never have a device like "xxx.33", so no conflict. :)

> Also, what if multiple portman devices are registered?

Even if we have multiple device, this temporary device will be
unregistered immediately after checking for the soundcard. So we should
not have any problem.

> 
> I suppose rather it should be passing dev variable from
> snd_portman_probe()?

Yes, we can. Instead of dev we can just pass the pdev->id for the device
number. Then we can have the same device number for the temporary
device used for probing and also the same number will be used in the
actual device if probing succeeds. Whichever way you want. Shall I send
the patch to use the same device number or using 33 as the device number
will do?

regards
sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> On Thu, 04 Feb 2016 17:38:23 +0100,
> Sudip Mukherjee wrote:
> > 
> > Modify portman driver to use the new parallel port device model.
> > The advantage of using the device model is that the device gets binded
> > to the hardware, we get the feature of hotplug, we can bind/unbind
> > the driver at runtime.
> > The only change is in the way the driver gets registered with the
> > parallel port subsystem and so as a result there is no user visible
> > change or any chance of regression.
> > 
> > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > ---
> > 
> > v3: changed commit message
> > v2:
> >  1. pardev_cb is initialized while declaring, thus removing the use of
> > memset.
> >  2. used pdev->id.
> >  3. v1 did not have the parport probe callback, but
> > we will need the probe callback for binding as the name of the driver
> > and the name of the device is different.
> >  4. in v1 I missed modifying snd_portman_probe_port().
> > 
> >  sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 39 insertions(+), 14 deletions(-)
> > 
> > diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> > index 172685d..a22f56c 100644
> > --- a/sound/drivers/portman2x4.c
> > +++ b/sound/drivers/portman2x4.c
> > @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> >  {
> >  	struct pardevice *pardev;
> >  	int res;
> > -
> > -	pardev = parport_register_device(p, DRIVER_NAME,
> > -					 NULL, NULL, NULL,
> > -					 0, NULL);
> > +	struct pardev_cb pdev_cb = {
> > +		.preempt = NULL,
> > +		.wakeup = NULL,
> > +		.private = NULL,
> > +		.irq_func = NULL,
> > +		.flags = 0,
> > +	};
> > +
> > +	/*
> > +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> > +	 * device id alloted to this temporary device will never clash
> > +	 * with an actual device already registered.
> > +	 */
> > +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> > +					    SNDRV_CARDS + 1);
> 
> Hmm, doesn't this result in a device name like "xxx.33" ?

yes, it will. But this is a temoporary device just to check if the
sound card is connected to that particular parallel port or not. After
checking this device is immediately unregistered. My idea here was to
have a device number which will never clash with another device number.
And we can never have a device like "xxx.33", so no conflict. :)

> Also, what if multiple portman devices are registered?

Even if we have multiple device, this temporary device will be
unregistered immediately after checking for the soundcard. So we should
not have any problem.

> 
> I suppose rather it should be passing dev variable from
> snd_portman_probe()?

Yes, we can. Instead of dev we can just pass the pdev->id for the device
number. Then we can have the same device number for the temporary
device used for probing and also the same number will be used in the
actual device if probing succeeds. Whichever way you want. Shall I send
the patch to use the same device number or using 33 as the device number
will do?

regards
sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Fri, 05 Feb 2016 07:17:06 +0100,
Sudip Mukherjee wrote:
> 
> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> > On Thu, 04 Feb 2016 17:38:23 +0100,
> > Sudip Mukherjee wrote:
> > > 
> > > Modify portman driver to use the new parallel port device model.
> > > The advantage of using the device model is that the device gets binded
> > > to the hardware, we get the feature of hotplug, we can bind/unbind
> > > the driver at runtime.
> > > The only change is in the way the driver gets registered with the
> > > parallel port subsystem and so as a result there is no user visible
> > > change or any chance of regression.
> > > 
> > > Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > > ---
> > > 
> > > v3: changed commit message
> > > v2:
> > >  1. pardev_cb is initialized while declaring, thus removing the use of
> > > memset.
> > >  2. used pdev->id.
> > >  3. v1 did not have the parport probe callback, but
> > > we will need the probe callback for binding as the name of the driver
> > > and the name of the device is different.
> > >  4. in v1 I missed modifying snd_portman_probe_port().
> > > 
> > >  sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> > >  1 file changed, 39 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> > > index 172685d..a22f56c 100644
> > > --- a/sound/drivers/portman2x4.c
> > > +++ b/sound/drivers/portman2x4.c
> > > @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> > >  {
> > >  	struct pardevice *pardev;
> > >  	int res;
> > > -
> > > -	pardev = parport_register_device(p, DRIVER_NAME,
> > > -					 NULL, NULL, NULL,
> > > -					 0, NULL);
> > > +	struct pardev_cb pdev_cb = {
> > > +		.preempt = NULL,
> > > +		.wakeup = NULL,
> > > +		.private = NULL,
> > > +		.irq_func = NULL,
> > > +		.flags = 0,
> > > +	};
> > > +
> > > +	/*
> > > +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> > > +	 * device id alloted to this temporary device will never clash
> > > +	 * with an actual device already registered.
> > > +	 */
> > > +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> > > +					    SNDRV_CARDS + 1);
> > 
> > Hmm, doesn't this result in a device name like "xxx.33" ?
> 
> yes, it will. But this is a temoporary device just to check if the
> sound card is connected to that particular parallel port or not. After
> checking this device is immediately unregistered. My idea here was to
> have a device number which will never clash with another device number.
> And we can never have a device like "xxx.33", so no conflict. :)

Ah, this is the temporary one.  If so, does it make sense to convert
this to dev_model one?  This means that the device will be notified to
udev even though this is a temporary one to be removed immediately.
It's what we'd want to avoid.  The function serves just as probing the
availability of the given port, not really registering anything
there.

That is, we need to change the registration flow itself if we really
want to move dev_model for the whole.


thanks,

Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> On Fri, 05 Feb 2016 07:17:06 +0100,
> Sudip Mukherjee wrote:
>>
>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
>>> On Thu, 04 Feb 2016 17:38:23 +0100,
>>> Sudip Mukherjee wrote:
>>>>
>>>> Modify portman driver to use the new parallel port device model.
>>>> The advantage of using the device model is that the device gets binded
>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
>>>> the driver at runtime.
>>>> The only change is in the way the driver gets registered with the
>>>> parallel port subsystem and so as a result there is no user visible
>>>> change or any chance of regression.
>>>>
>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
>>>> ---
>>>>
>>>> v3: changed commit message
>>>> v2:
>>>>   1. pardev_cb is initialized while declaring, thus removing the use of
>>>> memset.
>>>>   2. used pdev->id.
>>>>   3. v1 did not have the parport probe callback, but
>>>> we will need the probe callback for binding as the name of the driver
>>>> and the name of the device is different.
>>>>   4. in v1 I missed modifying snd_portman_probe_port().
>>>>
>>>>   sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>>>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
>>>> index 172685d..a22f56c 100644
>>>> --- a/sound/drivers/portman2x4.c
>>>> +++ b/sound/drivers/portman2x4.c
>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>>>>   {
>>>>   	struct pardevice *pardev;
>>>>   	int res;
>>>> -
>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
>>>> -					 NULL, NULL, NULL,
>>>> -					 0, NULL);
>>>> +	struct pardev_cb pdev_cb = {
>>>> +		.preempt = NULL,
>>>> +		.wakeup = NULL,
>>>> +		.private = NULL,
>>>> +		.irq_func = NULL,
>>>> +		.flags = 0,
>>>> +	};
>>>> +
>>>> +	/*
>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
>>>> +	 * device id alloted to this temporary device will never clash
>>>> +	 * with an actual device already registered.
>>>> +	 */
>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
>>>> +					    SNDRV_CARDS + 1);
>>>
>>> Hmm, doesn't this result in a device name like "xxx.33" ?
>>
>> yes, it will. But this is a temoporary device just to check if the
>> sound card is connected to that particular parallel port or not. After
>> checking this device is immediately unregistered. My idea here was to
>> have a device number which will never clash with another device number.
>> And we can never have a device like "xxx.33", so no conflict. :)
>
> Ah, this is the temporary one.  If so, does it make sense to convert
> this to dev_model one?  This means that the device will be notified to
> udev even though this is a temporary one to be removed immediately.

But since we are registering a device it should ideally follow the 
dev_model.

> It's what we'd want to avoid.  The function serves just as probing the
> availability of the given port, not really registering anything
> there.

To my understanding, it is probing for the availability of the port and 
it is also calling portman_probe() which is initializing hardware 
handshake lines to midi box and checking if the portman card is 
connected to that parallel port or not.

>
> That is, we need to change the registration flow itself if we really
> want to move dev_model for the whole.

Any hint, how to register then?
Without probing (reading and writing to that port) I can not know if 
that port is having the card and to use the port I need to register a 
device with that port.

Regards
Sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Fri, 05 Feb 2016 17:50:51 +0100,
Sudip Mukherjee wrote:
> 
> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> > On Fri, 05 Feb 2016 07:17:06 +0100,
> > Sudip Mukherjee wrote:
> >>
> >> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> >>> On Thu, 04 Feb 2016 17:38:23 +0100,
> >>> Sudip Mukherjee wrote:
> >>>>
> >>>> Modify portman driver to use the new parallel port device model.
> >>>> The advantage of using the device model is that the device gets binded
> >>>> to the hardware, we get the feature of hotplug, we can bind/unbind
> >>>> the driver at runtime.
> >>>> The only change is in the way the driver gets registered with the
> >>>> parallel port subsystem and so as a result there is no user visible
> >>>> change or any chance of regression.
> >>>>
> >>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> >>>> ---
> >>>>
> >>>> v3: changed commit message
> >>>> v2:
> >>>>   1. pardev_cb is initialized while declaring, thus removing the use of
> >>>> memset.
> >>>>   2. used pdev->id.
> >>>>   3. v1 did not have the parport probe callback, but
> >>>> we will need the probe callback for binding as the name of the driver
> >>>> and the name of the device is different.
> >>>>   4. in v1 I missed modifying snd_portman_probe_port().
> >>>>
> >>>>   sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> >>>>   1 file changed, 39 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> >>>> index 172685d..a22f56c 100644
> >>>> --- a/sound/drivers/portman2x4.c
> >>>> +++ b/sound/drivers/portman2x4.c
> >>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> >>>>   {
> >>>>   	struct pardevice *pardev;
> >>>>   	int res;
> >>>> -
> >>>> -	pardev = parport_register_device(p, DRIVER_NAME,
> >>>> -					 NULL, NULL, NULL,
> >>>> -					 0, NULL);
> >>>> +	struct pardev_cb pdev_cb = {
> >>>> +		.preempt = NULL,
> >>>> +		.wakeup = NULL,
> >>>> +		.private = NULL,
> >>>> +		.irq_func = NULL,
> >>>> +		.flags = 0,
> >>>> +	};
> >>>> +
> >>>> +	/*
> >>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> >>>> +	 * device id alloted to this temporary device will never clash
> >>>> +	 * with an actual device already registered.
> >>>> +	 */
> >>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> >>>> +					    SNDRV_CARDS + 1);
> >>>
> >>> Hmm, doesn't this result in a device name like "xxx.33" ?
> >>
> >> yes, it will. But this is a temoporary device just to check if the
> >> sound card is connected to that particular parallel port or not. After
> >> checking this device is immediately unregistered. My idea here was to
> >> have a device number which will never clash with another device number.
> >> And we can never have a device like "xxx.33", so no conflict. :)
> >
> > Ah, this is the temporary one.  If so, does it make sense to convert
> > this to dev_model one?  This means that the device will be notified to
> > udev even though this is a temporary one to be removed immediately.
> 
> But since we are registering a device it should ideally follow the 
> dev_model.

We shouldn't advertise the device that shouldn't be handled by the
user-space.  The device you're trying to register there is the one
that lives only shortly just for probing the address.


> > It's what we'd want to avoid.  The function serves just as probing the
> > availability of the given port, not really registering anything
> > there.
> 
> To my understanding, it is probing for the availability of the port and 
> it is also calling portman_probe() which is initializing hardware 
> handshake lines to midi box and checking if the portman card is 
> connected to that parallel port or not.
> 
> >
> > That is, we need to change the registration flow itself if we really
> > want to move dev_model for the whole.
> 
> Any hint, how to register then?
> Without probing (reading and writing to that port) I can not know if 
> that port is having the card and to use the port I need to register a 
> device with that port.

Just returning the error at probe of the parport device itself instead
of doing the probe twice?  The current way is racy in anyway.


Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Friday 05 February 2016 10:31 PM, Takashi Iwai wrote:
> On Fri, 05 Feb 2016 17:50:51 +0100,
> Sudip Mukherjee wrote:
>>
>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
>>> On Fri, 05 Feb 2016 07:17:06 +0100,
>>> Sudip Mukherjee wrote:
>>>>
>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
>>>>> Sudip Mukherjee wrote:
>>>>>>
>>>>>> Modify portman driver to use the new parallel port device model.
>>>>>> The advantage of using the device model is that the device gets binded
>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
>>>>>> the driver at runtime.
>>>>>> The only change is in the way the driver gets registered with the
>>>>>> parallel port subsystem and so as a result there is no user visible
>>>>>> change or any chance of regression.
>>>>>>
>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
>>>>>> ---
>>>>>>
>>>>>> v3: changed commit message
>>>>>> v2:
>>>>>>    1. pardev_cb is initialized while declaring, thus removing the use of
>>>>>> memset.
>>>>>>    2. used pdev->id.
>>>>>>    3. v1 did not have the parport probe callback, but
>>>>>> we will need the probe callback for binding as the name of the driver
>>>>>> and the name of the device is different.
>>>>>>    4. in v1 I missed modifying snd_portman_probe_port().
>>>>>>
>>>>>>    sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>>>>>>    1 file changed, 39 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
>>>>>> index 172685d..a22f56c 100644
>>>>>> --- a/sound/drivers/portman2x4.c
>>>>>> +++ b/sound/drivers/portman2x4.c
>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>>>>>>    {
>>>>>>    	struct pardevice *pardev;
>>>>>>    	int res;
>>>>>> -
>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
>>>>>> -					 NULL, NULL, NULL,
>>>>>> -					 0, NULL);
>>>>>> +	struct pardev_cb pdev_cb = {
>>>>>> +		.preempt = NULL,
>>>>>> +		.wakeup = NULL,
>>>>>> +		.private = NULL,
>>>>>> +		.irq_func = NULL,
>>>>>> +		.flags = 0,
>>>>>> +	};
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
>>>>>> +	 * device id alloted to this temporary device will never clash
>>>>>> +	 * with an actual device already registered.
>>>>>> +	 */
>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
>>>>>> +					    SNDRV_CARDS + 1);
>>>>>
>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
>>>>
>>>> yes, it will. But this is a temoporary device just to check if the
>>>> sound card is connected to that particular parallel port or not. After
>>>> checking this device is immediately unregistered. My idea here was to
>>>> have a device number which will never clash with another device number.
>>>> And we can never have a device like "xxx.33", so no conflict. :)
>>>
>>> Ah, this is the temporary one.  If so, does it make sense to convert
>>> this to dev_model one?  This means that the device will be notified to
>>> udev even though this is a temporary one to be removed immediately.
>>
>> But since we are registering a device it should ideally follow the
>> dev_model.
>
> We shouldn't advertise the device that shouldn't be handled by the
> user-space.  The device you're trying to register there is the one
> that lives only shortly just for probing the address.
>
>
>>> It's what we'd want to avoid.  The function serves just as probing the
>>> availability of the given port, not really registering anything
>>> there.
>>
>> To my understanding, it is probing for the availability of the port and
>> it is also calling portman_probe() which is initializing hardware
>> handshake lines to midi box and checking if the portman card is
>> connected to that parallel port or not.
>>
>>>
>>> That is, we need to change the registration flow itself if we really
>>> want to move dev_model for the whole.
>>
>> Any hint, how to register then?
>> Without probing (reading and writing to that port) I can not know if
>> that port is having the card and to use the port I need to register a
>> device with that port.
>
> Just returning the error at probe of the parport device itself instead
> of doing the probe twice?  The current way is racy in anyway.

Ohhhk.. so we only register once and if we find the card - we continue, 
we donot find the card then we unregister the device and return error code.
Great. I will send you a patch for your review.

Regards
Sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Fri, 05 Feb 2016 18:01:16 +0100,
Takashi Iwai wrote:
> 
> On Fri, 05 Feb 2016 17:50:51 +0100,
> Sudip Mukherjee wrote:
> > 
> > On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> > > On Fri, 05 Feb 2016 07:17:06 +0100,
> > > Sudip Mukherjee wrote:
> > >>
> > >> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> > >>> On Thu, 04 Feb 2016 17:38:23 +0100,
> > >>> Sudip Mukherjee wrote:
> > >>>>
> > >>>> Modify portman driver to use the new parallel port device model.
> > >>>> The advantage of using the device model is that the device gets binded
> > >>>> to the hardware, we get the feature of hotplug, we can bind/unbind
> > >>>> the driver at runtime.
> > >>>> The only change is in the way the driver gets registered with the
> > >>>> parallel port subsystem and so as a result there is no user visible
> > >>>> change or any chance of regression.
> > >>>>
> > >>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > >>>> ---
> > >>>>
> > >>>> v3: changed commit message
> > >>>> v2:
> > >>>>   1. pardev_cb is initialized while declaring, thus removing the use of
> > >>>> memset.
> > >>>>   2. used pdev->id.
> > >>>>   3. v1 did not have the parport probe callback, but
> > >>>> we will need the probe callback for binding as the name of the driver
> > >>>> and the name of the device is different.
> > >>>>   4. in v1 I missed modifying snd_portman_probe_port().
> > >>>>
> > >>>>   sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> > >>>>   1 file changed, 39 insertions(+), 14 deletions(-)
> > >>>>
> > >>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> > >>>> index 172685d..a22f56c 100644
> > >>>> --- a/sound/drivers/portman2x4.c
> > >>>> +++ b/sound/drivers/portman2x4.c
> > >>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> > >>>>   {
> > >>>>   	struct pardevice *pardev;
> > >>>>   	int res;
> > >>>> -
> > >>>> -	pardev = parport_register_device(p, DRIVER_NAME,
> > >>>> -					 NULL, NULL, NULL,
> > >>>> -					 0, NULL);
> > >>>> +	struct pardev_cb pdev_cb = {
> > >>>> +		.preempt = NULL,
> > >>>> +		.wakeup = NULL,
> > >>>> +		.private = NULL,
> > >>>> +		.irq_func = NULL,
> > >>>> +		.flags = 0,
> > >>>> +	};
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> > >>>> +	 * device id alloted to this temporary device will never clash
> > >>>> +	 * with an actual device already registered.
> > >>>> +	 */
> > >>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> > >>>> +					    SNDRV_CARDS + 1);
> > >>>
> > >>> Hmm, doesn't this result in a device name like "xxx.33" ?
> > >>
> > >> yes, it will. But this is a temoporary device just to check if the
> > >> sound card is connected to that particular parallel port or not. After
> > >> checking this device is immediately unregistered. My idea here was to
> > >> have a device number which will never clash with another device number.
> > >> And we can never have a device like "xxx.33", so no conflict. :)
> > >
> > > Ah, this is the temporary one.  If so, does it make sense to convert
> > > this to dev_model one?  This means that the device will be notified to
> > > udev even though this is a temporary one to be removed immediately.
> > 
> > But since we are registering a device it should ideally follow the 
> > dev_model.
> 
> We shouldn't advertise the device that shouldn't be handled by the
> user-space.  The device you're trying to register there is the one
> that lives only shortly just for probing the address.
> 
> 
> > > It's what we'd want to avoid.  The function serves just as probing the
> > > availability of the given port, not really registering anything
> > > there.
> > 
> > To my understanding, it is probing for the availability of the port and 
> > it is also calling portman_probe() which is initializing hardware 
> > handshake lines to midi box and checking if the portman card is 
> > connected to that parallel port or not.
> > 
> > >
> > > That is, we need to change the registration flow itself if we really
> > > want to move dev_model for the whole.
> > 
> > Any hint, how to register then?
> > Without probing (reading and writing to that port) I can not know if 
> > that port is having the card and to use the port I need to register a 
> > device with that port.
> 
> Just returning the error at probe of the parport device itself instead
> of doing the probe twice?  The current way is racy in anyway.

... and the problem with that is, there is no way to check whether
your upcoming change works correctly without the hardware.  It would
be no longer a "cleanup", and it's risky to do that blindly.

I appreciate your work, but it doesn't look worthy enough.  If we're
trying to eliminate the all old-style parport code from the kernel
code, OK, it's an ambitious project and we may consider taking a risk
of breakage.  Is that the case?


Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
> On Fri, 05 Feb 2016 18:01:16 +0100,
> Takashi Iwai wrote:
>>
>> On Fri, 05 Feb 2016 17:50:51 +0100,
>> Sudip Mukherjee wrote:
>>>
>>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
>>>> On Fri, 05 Feb 2016 07:17:06 +0100,
>>>> Sudip Mukherjee wrote:
>>>>>
>>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
>>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
>>>>>> Sudip Mukherjee wrote:
>>>>>>>
>>>>>>> Modify portman driver to use the new parallel port device model.
>>>>>>> The advantage of using the device model is that the device gets binded
>>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
>>>>>>> the driver at runtime.
>>>>>>> The only change is in the way the driver gets registered with the
>>>>>>> parallel port subsystem and so as a result there is no user visible
>>>>>>> change or any chance of regression.
>>>>>>>
>>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
>>>>>>> ---
>>>>>>>
>>>>>>> v3: changed commit message
>>>>>>> v2:
>>>>>>>    1. pardev_cb is initialized while declaring, thus removing the use of
>>>>>>> memset.
>>>>>>>    2. used pdev->id.
>>>>>>>    3. v1 did not have the parport probe callback, but
>>>>>>> we will need the probe callback for binding as the name of the driver
>>>>>>> and the name of the device is different.
>>>>>>>    4. in v1 I missed modifying snd_portman_probe_port().
>>>>>>>
>>>>>>>    sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>>>>>>>    1 file changed, 39 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
>>>>>>> index 172685d..a22f56c 100644
>>>>>>> --- a/sound/drivers/portman2x4.c
>>>>>>> +++ b/sound/drivers/portman2x4.c
>>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>>>>>>>    {
>>>>>>>    	struct pardevice *pardev;
>>>>>>>    	int res;
>>>>>>> -
>>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
>>>>>>> -					 NULL, NULL, NULL,
>>>>>>> -					 0, NULL);
>>>>>>> +	struct pardev_cb pdev_cb = {
>>>>>>> +		.preempt = NULL,
>>>>>>> +		.wakeup = NULL,
>>>>>>> +		.private = NULL,
>>>>>>> +		.irq_func = NULL,
>>>>>>> +		.flags = 0,
>>>>>>> +	};
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
>>>>>>> +	 * device id alloted to this temporary device will never clash
>>>>>>> +	 * with an actual device already registered.
>>>>>>> +	 */
>>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
>>>>>>> +					    SNDRV_CARDS + 1);
>>>>>>
>>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
>>>>>
>>>>> yes, it will. But this is a temoporary device just to check if the
>>>>> sound card is connected to that particular parallel port or not. After
>>>>> checking this device is immediately unregistered. My idea here was to
>>>>> have a device number which will never clash with another device number.
>>>>> And we can never have a device like "xxx.33", so no conflict. :)
>>>>
>>>> Ah, this is the temporary one.  If so, does it make sense to convert
>>>> this to dev_model one?  This means that the device will be notified to
>>>> udev even though this is a temporary one to be removed immediately.
>>>
>>> But since we are registering a device it should ideally follow the
>>> dev_model.
>>
>> We shouldn't advertise the device that shouldn't be handled by the
>> user-space.  The device you're trying to register there is the one
>> that lives only shortly just for probing the address.
>>
>>
>>>> It's what we'd want to avoid.  The function serves just as probing the
>>>> availability of the given port, not really registering anything
>>>> there.
>>>
>>> To my understanding, it is probing for the availability of the port and
>>> it is also calling portman_probe() which is initializing hardware
>>> handshake lines to midi box and checking if the portman card is
>>> connected to that parallel port or not.
>>>
>>>>
>>>> That is, we need to change the registration flow itself if we really
>>>> want to move dev_model for the whole.
>>>
>>> Any hint, how to register then?
>>> Without probing (reading and writing to that port) I can not know if
>>> that port is having the card and to use the port I need to register a
>>> device with that port.
>>
>> Just returning the error at probe of the parport device itself instead
>> of doing the probe twice?  The current way is racy in anyway.
>
> ... and the problem with that is, there is no way to check whether
> your upcoming change works correctly without the hardware.  It would
> be no longer a "cleanup", and it's risky to do that blindly.

Yes. That is why I try to change the driver with the minimum possible 
change.

>
> I appreciate your work, but it doesn't look worthy enough.  If we're
> trying to eliminate the all old-style parport code from the kernel
> code, OK, it's an ambitious project and we may consider taking a risk
> of breakage.  Is that the case?

Yes, the old api is supposed to be removed and we should only have the 
device model api. I was expecting to remove the old API by 4.7.
Is there any way to get the hardware?

Regards
Sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Fri, 05 Feb 2016 18:21:46 +0100,
Sudip Mukherjee wrote:
> 
> On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
> > On Fri, 05 Feb 2016 18:01:16 +0100,
> > Takashi Iwai wrote:
> >>
> >> On Fri, 05 Feb 2016 17:50:51 +0100,
> >> Sudip Mukherjee wrote:
> >>>
> >>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> >>>> On Fri, 05 Feb 2016 07:17:06 +0100,
> >>>> Sudip Mukherjee wrote:
> >>>>>
> >>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> >>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
> >>>>>> Sudip Mukherjee wrote:
> >>>>>>>
> >>>>>>> Modify portman driver to use the new parallel port device model.
> >>>>>>> The advantage of using the device model is that the device gets binded
> >>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
> >>>>>>> the driver at runtime.
> >>>>>>> The only change is in the way the driver gets registered with the
> >>>>>>> parallel port subsystem and so as a result there is no user visible
> >>>>>>> change or any chance of regression.
> >>>>>>>
> >>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> >>>>>>> ---
> >>>>>>>
> >>>>>>> v3: changed commit message
> >>>>>>> v2:
> >>>>>>>    1. pardev_cb is initialized while declaring, thus removing the use of
> >>>>>>> memset.
> >>>>>>>    2. used pdev->id.
> >>>>>>>    3. v1 did not have the parport probe callback, but
> >>>>>>> we will need the probe callback for binding as the name of the driver
> >>>>>>> and the name of the device is different.
> >>>>>>>    4. in v1 I missed modifying snd_portman_probe_port().
> >>>>>>>
> >>>>>>>    sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> >>>>>>>    1 file changed, 39 insertions(+), 14 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> >>>>>>> index 172685d..a22f56c 100644
> >>>>>>> --- a/sound/drivers/portman2x4.c
> >>>>>>> +++ b/sound/drivers/portman2x4.c
> >>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> >>>>>>>    {
> >>>>>>>    	struct pardevice *pardev;
> >>>>>>>    	int res;
> >>>>>>> -
> >>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
> >>>>>>> -					 NULL, NULL, NULL,
> >>>>>>> -					 0, NULL);
> >>>>>>> +	struct pardev_cb pdev_cb = {
> >>>>>>> +		.preempt = NULL,
> >>>>>>> +		.wakeup = NULL,
> >>>>>>> +		.private = NULL,
> >>>>>>> +		.irq_func = NULL,
> >>>>>>> +		.flags = 0,
> >>>>>>> +	};
> >>>>>>> +
> >>>>>>> +	/*
> >>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> >>>>>>> +	 * device id alloted to this temporary device will never clash
> >>>>>>> +	 * with an actual device already registered.
> >>>>>>> +	 */
> >>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> >>>>>>> +					    SNDRV_CARDS + 1);
> >>>>>>
> >>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
> >>>>>
> >>>>> yes, it will. But this is a temoporary device just to check if the
> >>>>> sound card is connected to that particular parallel port or not. After
> >>>>> checking this device is immediately unregistered. My idea here was to
> >>>>> have a device number which will never clash with another device number.
> >>>>> And we can never have a device like "xxx.33", so no conflict. :)
> >>>>
> >>>> Ah, this is the temporary one.  If so, does it make sense to convert
> >>>> this to dev_model one?  This means that the device will be notified to
> >>>> udev even though this is a temporary one to be removed immediately.
> >>>
> >>> But since we are registering a device it should ideally follow the
> >>> dev_model.
> >>
> >> We shouldn't advertise the device that shouldn't be handled by the
> >> user-space.  The device you're trying to register there is the one
> >> that lives only shortly just for probing the address.
> >>
> >>
> >>>> It's what we'd want to avoid.  The function serves just as probing the
> >>>> availability of the given port, not really registering anything
> >>>> there.
> >>>
> >>> To my understanding, it is probing for the availability of the port and
> >>> it is also calling portman_probe() which is initializing hardware
> >>> handshake lines to midi box and checking if the portman card is
> >>> connected to that parallel port or not.
> >>>
> >>>>
> >>>> That is, we need to change the registration flow itself if we really
> >>>> want to move dev_model for the whole.
> >>>
> >>> Any hint, how to register then?
> >>> Without probing (reading and writing to that port) I can not know if
> >>> that port is having the card and to use the port I need to register a
> >>> device with that port.
> >>
> >> Just returning the error at probe of the parport device itself instead
> >> of doing the probe twice?  The current way is racy in anyway.
> >
> > ... and the problem with that is, there is no way to check whether
> > your upcoming change works correctly without the hardware.  It would
> > be no longer a "cleanup", and it's risky to do that blindly.
> 
> Yes. That is why I try to change the driver with the minimum possible 
> change.

But it's no 100% compatible transition.  That's the first problem.

> > I appreciate your work, but it doesn't look worthy enough.  If we're
> > trying to eliminate the all old-style parport code from the kernel
> > code, OK, it's an ambitious project and we may consider taking a risk
> > of breakage.  Is that the case?
> 
> Yes, the old api is supposed to be removed and we should only have the 
> device model api. I was expecting to remove the old API by 4.7.
> Is there any way to get the hardware?

No, unfortunately.  It's an old hardware, after all.  It's difficult
to find even a decent machine with a parallel port...


Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Saturday 06 February 2016 12:41 AM, Takashi Iwai wrote:
> On Fri, 05 Feb 2016 18:21:46 +0100,
> Sudip Mukherjee wrote:
>>
>> On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
>>> On Fri, 05 Feb 2016 18:01:16 +0100,
>>> Takashi Iwai wrote:
>>>>
>>>> On Fri, 05 Feb 2016 17:50:51 +0100,
>>>> Sudip Mukherjee wrote:
>>>>>
>>>>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
>>>>>> On Fri, 05 Feb 2016 07:17:06 +0100,
>>>>>> Sudip Mukherjee wrote:
>>>>>>>
>>>>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
>>>>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
>>>>>>>> Sudip Mukherjee wrote:
>>>>>>>>>
>>>>>>>>> Modify portman driver to use the new parallel port device model.
>>>>>>>>> The advantage of using the device model is that the device gets binded
>>>>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
>>>>>>>>> the driver at runtime.
>>>>>>>>> The only change is in the way the driver gets registered with the
>>>>>>>>> parallel port subsystem and so as a result there is no user visible
>>>>>>>>> change or any chance of regression.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> v3: changed commit message
>>>>>>>>> v2:
>>>>>>>>>     1. pardev_cb is initialized while declaring, thus removing the use of
>>>>>>>>> memset.
>>>>>>>>>     2. used pdev->id.
>>>>>>>>>     3. v1 did not have the parport probe callback, but
>>>>>>>>> we will need the probe callback for binding as the name of the driver
>>>>>>>>> and the name of the device is different.
>>>>>>>>>     4. in v1 I missed modifying snd_portman_probe_port().
>>>>>>>>>
>>>>>>>>>     sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>>>>>>>>>     1 file changed, 39 insertions(+), 14 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
>>>>>>>>> index 172685d..a22f56c 100644
>>>>>>>>> --- a/sound/drivers/portman2x4.c
>>>>>>>>> +++ b/sound/drivers/portman2x4.c
>>>>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>>>>>>>>>     {
>>>>>>>>>     	struct pardevice *pardev;
>>>>>>>>>     	int res;
>>>>>>>>> -
>>>>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
>>>>>>>>> -					 NULL, NULL, NULL,
>>>>>>>>> -					 0, NULL);
>>>>>>>>> +	struct pardev_cb pdev_cb = {
>>>>>>>>> +		.preempt = NULL,
>>>>>>>>> +		.wakeup = NULL,
>>>>>>>>> +		.private = NULL,
>>>>>>>>> +		.irq_func = NULL,
>>>>>>>>> +		.flags = 0,
>>>>>>>>> +	};
>>>>>>>>> +
>>>>>>>>> +	/*
>>>>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
>>>>>>>>> +	 * device id alloted to this temporary device will never clash
>>>>>>>>> +	 * with an actual device already registered.
>>>>>>>>> +	 */
>>>>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
>>>>>>>>> +					    SNDRV_CARDS + 1);
>>>>>>>>
>>>>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
>>>>>>>
>>>>>>> yes, it will. But this is a temoporary device just to check if the
>>>>>>> sound card is connected to that particular parallel port or not. After
>>>>>>> checking this device is immediately unregistered. My idea here was to
>>>>>>> have a device number which will never clash with another device number.
>>>>>>> And we can never have a device like "xxx.33", so no conflict. :)
>>>>>>
>>>>>> Ah, this is the temporary one.  If so, does it make sense to convert
>>>>>> this to dev_model one?  This means that the device will be notified to
>>>>>> udev even though this is a temporary one to be removed immediately.
>>>>>
>>>>> But since we are registering a device it should ideally follow the
>>>>> dev_model.
>>>>
>>>> We shouldn't advertise the device that shouldn't be handled by the
>>>> user-space.  The device you're trying to register there is the one
>>>> that lives only shortly just for probing the address.
>>>>
>>>>
>>>>>> It's what we'd want to avoid.  The function serves just as probing the
>>>>>> availability of the given port, not really registering anything
>>>>>> there.
>>>>>
>>>>> To my understanding, it is probing for the availability of the port and
>>>>> it is also calling portman_probe() which is initializing hardware
>>>>> handshake lines to midi box and checking if the portman card is
>>>>> connected to that parallel port or not.
>>>>>
>>>>>>
>>>>>> That is, we need to change the registration flow itself if we really
>>>>>> want to move dev_model for the whole.
>>>>>
>>>>> Any hint, how to register then?
>>>>> Without probing (reading and writing to that port) I can not know if
>>>>> that port is having the card and to use the port I need to register a
>>>>> device with that port.
>>>>
>>>> Just returning the error at probe of the parport device itself instead
>>>> of doing the probe twice?  The current way is racy in anyway.
>>>
>>> ... and the problem with that is, there is no way to check whether
>>> your upcoming change works correctly without the hardware.  It would
>>> be no longer a "cleanup", and it's risky to do that blindly.
>>
>> Yes. That is why I try to change the driver with the minimum possible
>> change.
>
> But it's no 100% compatible transition.  That's the first problem.

Well, the first problem that i can see is using the same fixed number as 
the temporary device, so we can have a race there. Another problem might 
be that the same device number can be tried for platform device.

BTW, why do we need the platform device here? we can directly probe for 
the device and register the sound card if the device is available from 
the attach function (now match_port). And the device number can be 
automatically generated. I think that will solve many of the problems. 
But the changes without checking on hardware will be risky again.

>
>>> I appreciate your work, but it doesn't look worthy enough.  If we're
>>> trying to eliminate the all old-style parport code from the kernel
>>> code, OK, it's an ambitious project and we may consider taking a risk
>>> of breakage.  Is that the case?
>>
>> Yes, the old api is supposed to be removed and we should only have the
>> device model api. I was expecting to remove the old API by 4.7.
>> Is there any way to get the hardware?
>
> No, unfortunately.  It's an old hardware, after all.  It's difficult
> to find even a decent machine with a parallel port...

I have an i5 with an onboard parallel port, additionally one more pci 
card parallel port.
So what do you suggest? how should we approach?

regards
sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Saturday 06 February 2016 12:41 AM, Takashi Iwai wrote:
> On Fri, 05 Feb 2016 18:21:46 +0100,
> Sudip Mukherjee wrote:
>>
>> On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
>>> On Fri, 05 Feb 2016 18:01:16 +0100,
>>> Takashi Iwai wrote:
>>>>
>>>> On Fri, 05 Feb 2016 17:50:51 +0100,
>>>> Sudip Mukherjee wrote:
>>>>>
>>>>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
>>>>>> On Fri, 05 Feb 2016 07:17:06 +0100,
>>>>>> Sudip Mukherjee wrote:
>>>>>>>
>>>>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
>>>>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
>>>>>>>> Sudip Mukherjee wrote:
>>>>>>>>>
>>>>>>>>> Modify portman driver to use the new parallel port device model.
>>>>>>>>> The advantage of using the device model is that the device gets binded
>>>>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
>>>>>>>>> the driver at runtime.
>>>>>>>>> The only change is in the way the driver gets registered with the
>>>>>>>>> parallel port subsystem and so as a result there is no user visible
>>>>>>>>> change or any chance of regression.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> v3: changed commit message
>>>>>>>>> v2:
>>>>>>>>>     1. pardev_cb is initialized while declaring, thus removing the use of
>>>>>>>>> memset.
>>>>>>>>>     2. used pdev->id.
>>>>>>>>>     3. v1 did not have the parport probe callback, but
>>>>>>>>> we will need the probe callback for binding as the name of the driver
>>>>>>>>> and the name of the device is different.
>>>>>>>>>     4. in v1 I missed modifying snd_portman_probe_port().
>>>>>>>>>
>>>>>>>>>     sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
>>>>>>>>>     1 file changed, 39 insertions(+), 14 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
>>>>>>>>> index 172685d..a22f56c 100644
>>>>>>>>> --- a/sound/drivers/portman2x4.c
>>>>>>>>> +++ b/sound/drivers/portman2x4.c
>>>>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
>>>>>>>>>     {
>>>>>>>>>     	struct pardevice *pardev;
>>>>>>>>>     	int res;
>>>>>>>>> -
>>>>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
>>>>>>>>> -					 NULL, NULL, NULL,
>>>>>>>>> -					 0, NULL);
>>>>>>>>> +	struct pardev_cb pdev_cb = {
>>>>>>>>> +		.preempt = NULL,
>>>>>>>>> +		.wakeup = NULL,
>>>>>>>>> +		.private = NULL,
>>>>>>>>> +		.irq_func = NULL,
>>>>>>>>> +		.flags = 0,
>>>>>>>>> +	};
>>>>>>>>> +
>>>>>>>>> +	/*
>>>>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
>>>>>>>>> +	 * device id alloted to this temporary device will never clash
>>>>>>>>> +	 * with an actual device already registered.
>>>>>>>>> +	 */
>>>>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
>>>>>>>>> +					    SNDRV_CARDS + 1);
>>>>>>>>
>>>>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
>>>>>>>
>>>>>>> yes, it will. But this is a temoporary device just to check if the
>>>>>>> sound card is connected to that particular parallel port or not. After
>>>>>>> checking this device is immediately unregistered. My idea here was to
>>>>>>> have a device number which will never clash with another device number.
>>>>>>> And we can never have a device like "xxx.33", so no conflict. :)
>>>>>>
>>>>>> Ah, this is the temporary one.  If so, does it make sense to convert
>>>>>> this to dev_model one?  This means that the device will be notified to
>>>>>> udev even though this is a temporary one to be removed immediately.
>>>>>
>>>>> But since we are registering a device it should ideally follow the
>>>>> dev_model.
>>>>
>>>> We shouldn't advertise the device that shouldn't be handled by the
>>>> user-space.  The device you're trying to register there is the one
>>>> that lives only shortly just for probing the address.
>>>>
>>>>
>>>>>> It's what we'd want to avoid.  The function serves just as probing the
>>>>>> availability of the given port, not really registering anything
>>>>>> there.
>>>>>
>>>>> To my understanding, it is probing for the availability of the port and
>>>>> it is also calling portman_probe() which is initializing hardware
>>>>> handshake lines to midi box and checking if the portman card is
>>>>> connected to that parallel port or not.
>>>>>
>>>>>>
>>>>>> That is, we need to change the registration flow itself if we really
>>>>>> want to move dev_model for the whole.
>>>>>
>>>>> Any hint, how to register then?
>>>>> Without probing (reading and writing to that port) I can not know if
>>>>> that port is having the card and to use the port I need to register a
>>>>> device with that port.
>>>>
>>>> Just returning the error at probe of the parport device itself instead
>>>> of doing the probe twice?  The current way is racy in anyway.
>>>
>>> ... and the problem with that is, there is no way to check whether
>>> your upcoming change works correctly without the hardware.  It would
>>> be no longer a "cleanup", and it's risky to do that blindly.
>>
>> Yes. That is why I try to change the driver with the minimum possible
>> change.
>
> But it's no 100% compatible transition.  That's the first problem.

Well, the first problem that i can see is using the same fixed number as 
the temporary device, so we can have a race there. Another problem might 
be that the same device number can be tried for platform device.

BTW, why do we need the platform device here? we can directly probe for 
the device and register the sound card if the device is available from 
the attach function (now match_port). And the device number can be 
automatically generated. I think that will solve many of the problems. 
But the changes without checking on hardware will be risky again.

>
>>> I appreciate your work, but it doesn't look worthy enough.  If we're
>>> trying to eliminate the all old-style parport code from the kernel
>>> code, OK, it's an ambitious project and we may consider taking a risk
>>> of breakage.  Is that the case?
>>
>> Yes, the old api is supposed to be removed and we should only have the
>> device model api. I was expecting to remove the old API by 4.7.
>> Is there any way to get the hardware?
>
> No, unfortunately.  It's an old hardware, after all.  It's difficult
> to find even a decent machine with a parallel port...

I have an i5 with an onboard parallel port, additionally one more pci 
card parallel port.
So what do you suggest? how should we approach?

regards
sudip

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Takashi Iwai]

On Sun, 07 Feb 2016 15:49:34 +0100,
Sudip Mukherjee wrote:
> 
> On Saturday 06 February 2016 12:41 AM, Takashi Iwai wrote:
> > On Fri, 05 Feb 2016 18:21:46 +0100,
> > Sudip Mukherjee wrote:
> >>
> >> On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
> >>> On Fri, 05 Feb 2016 18:01:16 +0100,
> >>> Takashi Iwai wrote:
> >>>>
> >>>> On Fri, 05 Feb 2016 17:50:51 +0100,
> >>>> Sudip Mukherjee wrote:
> >>>>>
> >>>>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> >>>>>> On Fri, 05 Feb 2016 07:17:06 +0100,
> >>>>>> Sudip Mukherjee wrote:
> >>>>>>>
> >>>>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> >>>>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
> >>>>>>>> Sudip Mukherjee wrote:
> >>>>>>>>>
> >>>>>>>>> Modify portman driver to use the new parallel port device model.
> >>>>>>>>> The advantage of using the device model is that the device gets binded
> >>>>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
> >>>>>>>>> the driver at runtime.
> >>>>>>>>> The only change is in the way the driver gets registered with the
> >>>>>>>>> parallel port subsystem and so as a result there is no user visible
> >>>>>>>>> change or any chance of regression.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> v3: changed commit message
> >>>>>>>>> v2:
> >>>>>>>>>     1. pardev_cb is initialized while declaring, thus removing the use of
> >>>>>>>>> memset.
> >>>>>>>>>     2. used pdev->id.
> >>>>>>>>>     3. v1 did not have the parport probe callback, but
> >>>>>>>>> we will need the probe callback for binding as the name of the driver
> >>>>>>>>> and the name of the device is different.
> >>>>>>>>>     4. in v1 I missed modifying snd_portman_probe_port().
> >>>>>>>>>
> >>>>>>>>>     sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> >>>>>>>>>     1 file changed, 39 insertions(+), 14 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> >>>>>>>>> index 172685d..a22f56c 100644
> >>>>>>>>> --- a/sound/drivers/portman2x4.c
> >>>>>>>>> +++ b/sound/drivers/portman2x4.c
> >>>>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> >>>>>>>>>     {
> >>>>>>>>>     	struct pardevice *pardev;
> >>>>>>>>>     	int res;
> >>>>>>>>> -
> >>>>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
> >>>>>>>>> -					 NULL, NULL, NULL,
> >>>>>>>>> -					 0, NULL);
> >>>>>>>>> +	struct pardev_cb pdev_cb = {
> >>>>>>>>> +		.preempt = NULL,
> >>>>>>>>> +		.wakeup = NULL,
> >>>>>>>>> +		.private = NULL,
> >>>>>>>>> +		.irq_func = NULL,
> >>>>>>>>> +		.flags = 0,
> >>>>>>>>> +	};
> >>>>>>>>> +
> >>>>>>>>> +	/*
> >>>>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> >>>>>>>>> +	 * device id alloted to this temporary device will never clash
> >>>>>>>>> +	 * with an actual device already registered.
> >>>>>>>>> +	 */
> >>>>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> >>>>>>>>> +					    SNDRV_CARDS + 1);
> >>>>>>>>
> >>>>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
> >>>>>>>
> >>>>>>> yes, it will. But this is a temoporary device just to check if the
> >>>>>>> sound card is connected to that particular parallel port or not. After
> >>>>>>> checking this device is immediately unregistered. My idea here was to
> >>>>>>> have a device number which will never clash with another device number.
> >>>>>>> And we can never have a device like "xxx.33", so no conflict. :)
> >>>>>>
> >>>>>> Ah, this is the temporary one.  If so, does it make sense to convert
> >>>>>> this to dev_model one?  This means that the device will be notified to
> >>>>>> udev even though this is a temporary one to be removed immediately.
> >>>>>
> >>>>> But since we are registering a device it should ideally follow the
> >>>>> dev_model.
> >>>>
> >>>> We shouldn't advertise the device that shouldn't be handled by the
> >>>> user-space.  The device you're trying to register there is the one
> >>>> that lives only shortly just for probing the address.
> >>>>
> >>>>
> >>>>>> It's what we'd want to avoid.  The function serves just as probing the
> >>>>>> availability of the given port, not really registering anything
> >>>>>> there.
> >>>>>
> >>>>> To my understanding, it is probing for the availability of the port and
> >>>>> it is also calling portman_probe() which is initializing hardware
> >>>>> handshake lines to midi box and checking if the portman card is
> >>>>> connected to that parallel port or not.
> >>>>>
> >>>>>>
> >>>>>> That is, we need to change the registration flow itself if we really
> >>>>>> want to move dev_model for the whole.
> >>>>>
> >>>>> Any hint, how to register then?
> >>>>> Without probing (reading and writing to that port) I can not know if
> >>>>> that port is having the card and to use the port I need to register a
> >>>>> device with that port.
> >>>>
> >>>> Just returning the error at probe of the parport device itself instead
> >>>> of doing the probe twice?  The current way is racy in anyway.
> >>>
> >>> ... and the problem with that is, there is no way to check whether
> >>> your upcoming change works correctly without the hardware.  It would
> >>> be no longer a "cleanup", and it's risky to do that blindly.
> >>
> >> Yes. That is why I try to change the driver with the minimum possible
> >> change.
> >
> > But it's no 100% compatible transition.  That's the first problem.
> 
> Well, the first problem that i can see is using the same fixed number as 
> the temporary device, so we can have a race there.

The original code uses parport_register_port() and this assigns a new
temporary number by itself.  Well, the issue is cosmetic, but...

> Another problem might 
> be that the same device number can be tried for platform device.
> 
> BTW, why do we need the platform device here? we can directly probe for 
> the device and register the sound card if the device is available from 
> the attach function (now match_port). And the device number can be 
> automatically generated. I think that will solve many of the problems. 
> But the changes without checking on hardware will be risky again.

... that's the only and biggest problem.  The whole rewrite needs the
check with the actual hardware, ideally.


> >>> I appreciate your work, but it doesn't look worthy enough.  If we're
> >>> trying to eliminate the all old-style parport code from the kernel
> >>> code, OK, it's an ambitious project and we may consider taking a risk
> >>> of breakage.  Is that the case?
> >>
> >> Yes, the old api is supposed to be removed and we should only have the
> >> device model api. I was expecting to remove the old API by 4.7.
> >> Is there any way to get the hardware?
> >
> > No, unfortunately.  It's an old hardware, after all.  It's difficult
> > to find even a decent machine with a parallel port...
> 
> I have an i5 with an onboard parallel port, additionally one more pci 
> card parallel port.
> So what do you suggest? how should we approach?

This really depends on the demand.  As already mentioned, if your
change is about getting rid of the whole legacy
parport_register_port() and its old API, it'd be worth to take a
risk.  But then you should really concentrate only on that.  Just
convert it without playing too much with white space changes, etc, and
make it in a series of the whole patchset (or at least show a "big
picture").


thanks,

Takashi

Re: [PATCH v3 3/3] portman2x4 - use new parport device model

[Sudip Mukherjee]

On Tue, Feb 09, 2016 at 12:32:55PM +0100, Takashi Iwai wrote:
> On Sun, 07 Feb 2016 15:49:34 +0100,
> Sudip Mukherjee wrote:
> > 
> > On Saturday 06 February 2016 12:41 AM, Takashi Iwai wrote:
> > > On Fri, 05 Feb 2016 18:21:46 +0100,
> > > Sudip Mukherjee wrote:
> > >>
> > >> On Friday 05 February 2016 10:36 PM, Takashi Iwai wrote:
> > >>> On Fri, 05 Feb 2016 18:01:16 +0100,
> > >>> Takashi Iwai wrote:
> > >>>>
> > >>>> On Fri, 05 Feb 2016 17:50:51 +0100,
> > >>>> Sudip Mukherjee wrote:
> > >>>>>
> > >>>>> On Friday 05 February 2016 05:25 PM, Takashi Iwai wrote:
> > >>>>>> On Fri, 05 Feb 2016 07:17:06 +0100,
> > >>>>>> Sudip Mukherjee wrote:
> > >>>>>>>
> > >>>>>>> On Thu, Feb 04, 2016 at 05:51:07PM +0100, Takashi Iwai wrote:
> > >>>>>>>> On Thu, 04 Feb 2016 17:38:23 +0100,
> > >>>>>>>> Sudip Mukherjee wrote:
> > >>>>>>>>>
> > >>>>>>>>> Modify portman driver to use the new parallel port device model.
> > >>>>>>>>> The advantage of using the device model is that the device gets binded
> > >>>>>>>>> to the hardware, we get the feature of hotplug, we can bind/unbind
> > >>>>>>>>> the driver at runtime.
> > >>>>>>>>> The only change is in the way the driver gets registered with the
> > >>>>>>>>> parallel port subsystem and so as a result there is no user visible
> > >>>>>>>>> change or any chance of regression.
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> > >>>>>>>>> ---
> > >>>>>>>>>
> > >>>>>>>>> v3: changed commit message
> > >>>>>>>>> v2:
> > >>>>>>>>>     1. pardev_cb is initialized while declaring, thus removing the use of
> > >>>>>>>>> memset.
> > >>>>>>>>>     2. used pdev->id.
> > >>>>>>>>>     3. v1 did not have the parport probe callback, but
> > >>>>>>>>> we will need the probe callback for binding as the name of the driver
> > >>>>>>>>> and the name of the device is different.
> > >>>>>>>>>     4. in v1 I missed modifying snd_portman_probe_port().
> > >>>>>>>>>
> > >>>>>>>>>     sound/drivers/portman2x4.c | 53 ++++++++++++++++++++++++++++++++++------------
> > >>>>>>>>>     1 file changed, 39 insertions(+), 14 deletions(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c
> > >>>>>>>>> index 172685d..a22f56c 100644
> > >>>>>>>>> --- a/sound/drivers/portman2x4.c
> > >>>>>>>>> +++ b/sound/drivers/portman2x4.c
> > >>>>>>>>> @@ -650,10 +650,21 @@ static int snd_portman_probe_port(struct parport *p)
> > >>>>>>>>>     {
> > >>>>>>>>>     	struct pardevice *pardev;
> > >>>>>>>>>     	int res;
> > >>>>>>>>> -
> > >>>>>>>>> -	pardev = parport_register_device(p, DRIVER_NAME,
> > >>>>>>>>> -					 NULL, NULL, NULL,
> > >>>>>>>>> -					 0, NULL);
> > >>>>>>>>> +	struct pardev_cb pdev_cb = {
> > >>>>>>>>> +		.preempt = NULL,
> > >>>>>>>>> +		.wakeup = NULL,
> > >>>>>>>>> +		.private = NULL,
> > >>>>>>>>> +		.irq_func = NULL,
> > >>>>>>>>> +		.flags = 0,
> > >>>>>>>>> +	};
> > >>>>>>>>> +
> > >>>>>>>>> +	/*
> > >>>>>>>>> +	 * Specify the device number as SNDRV_CARDS + 1 so that the
> > >>>>>>>>> +	 * device id alloted to this temporary device will never clash
> > >>>>>>>>> +	 * with an actual device already registered.
> > >>>>>>>>> +	 */
> > >>>>>>>>> +	pardev = parport_register_dev_model(p, DRIVER_NAME, &pdev_cb,
> > >>>>>>>>> +					    SNDRV_CARDS + 1);
> > >>>>>>>>
> > >>>>>>>> Hmm, doesn't this result in a device name like "xxx.33" ?
> > >>>>>>>
> > >>>>>>> yes, it will. But this is a temoporary device just to check if the
> > >>>>>>> sound card is connected to that particular parallel port or not. After
> > >>>>>>> checking this device is immediately unregistered. My idea here was to
> > >>>>>>> have a device number which will never clash with another device number.
> > >>>>>>> And we can never have a device like "xxx.33", so no conflict. :)
> > >>>>>>
> > >>>>>> Ah, this is the temporary one.  If so, does it make sense to convert
> > >>>>>> this to dev_model one?  This means that the device will be notified to
> > >>>>>> udev even though this is a temporary one to be removed immediately.
> > >>>>>
> > >>>>> But since we are registering a device it should ideally follow the
> > >>>>> dev_model.
> > >>>>
> > >>>> We shouldn't advertise the device that shouldn't be handled by the
> > >>>> user-space.  The device you're trying to register there is the one
> > >>>> that lives only shortly just for probing the address.
> > >>>>
> > >>>>
> > >>>>>> It's what we'd want to avoid.  The function serves just as probing the
> > >>>>>> availability of the given port, not really registering anything
> > >>>>>> there.
> > >>>>>
> > >>>>> To my understanding, it is probing for the availability of the port and
> > >>>>> it is also calling portman_probe() which is initializing hardware
> > >>>>> handshake lines to midi box and checking if the portman card is
> > >>>>> connected to that parallel port or not.
> > >>>>>
> > >>>>>>
> > >>>>>> That is, we need to change the registration flow itself if we really
> > >>>>>> want to move dev_model for the whole.
> > >>>>>
> > >>>>> Any hint, how to register then?
> > >>>>> Without probing (reading and writing to that port) I can not know if
> > >>>>> that port is having the card and to use the port I need to register a
> > >>>>> device with that port.
> > >>>>
> > >>>> Just returning the error at probe of the parport device itself instead
> > >>>> of doing the probe twice?  The current way is racy in anyway.
> > >>>
> > >>> ... and the problem with that is, there is no way to check whether
> > >>> your upcoming change works correctly without the hardware.  It would
> > >>> be no longer a "cleanup", and it's risky to do that blindly.
> > >>
> > >> Yes. That is why I try to change the driver with the minimum possible
> > >> change.
> > >
> > > But it's no 100% compatible transition.  That's the first problem.
> > 
> > Well, the first problem that i can see is using the same fixed number as 
> > the temporary device, so we can have a race there.
> 
> The original code uses parport_register_port() and this assigns a new
> temporary number by itself.  Well, the issue is cosmetic, but...

I think you mean parport_register_device()...
> 
> > Another problem might 
> > be that the same device number can be tried for platform device.
> > 
> > BTW, why do we need the platform device here? we can directly probe for 
> > the device and register the sound card if the device is available from 
> > the attach function (now match_port). And the device number can be 
> > automatically generated. I think that will solve many of the problems. 
> > But the changes without checking on hardware will be risky again.
> 
> ... that's the only and biggest problem.  The whole rewrite needs the
> check with the actual hardware, ideally.

:(
If I get the chance to rewrite I will remove the platform device.		

> 
> 
> > >>> I appreciate your work, but it doesn't look worthy enough.  If we're
> > >>> trying to eliminate the all old-style parport code from the kernel
> > >>> code, OK, it's an ambitious project and we may consider taking a risk
> > >>> of breakage.  Is that the case?
> > >>
> > >> Yes, the old api is supposed to be removed and we should only have the
> > >> device model api. I was expecting to remove the old API by 4.7.
> > >> Is there any way to get the hardware?
> > >
> > > No, unfortunately.  It's an old hardware, after all.  It's difficult
> > > to find even a decent machine with a parallel port...
> > 
> > I have an i5 with an onboard parallel port, additionally one more pci 
> > card parallel port.
> > So what do you suggest? how should we approach?
> 
> This really depends on the demand.  As already mentioned, if your
> change is about getting rid of the whole legacy
> parport_register_port() and its old API, it'd be worth to take a
> risk.  But then you should really concentrate only on that.  Just
> convert it without playing too much with white space changes, etc, and
> make it in a series of the whole patchset (or at least show a "big
> picture").

But since all of it depends on old hardware so I am approaching very
slowly. Only 2 - 3 drivers converterd in one release and I wait for any
news about some regression. Fortunately till now no regression, and some
of the other drivers (panel, joystick, paride, i2c-parport etc.) were
also tested after the change. The plan is to convert all drivers using
parport_register_device() to parport_register_dev_model() first. Infact
i have systems with parallel port but no hardware to check the daisy
chain so I can not touch the old API related to that.
I will send you the modified patch after removing that temp device.

regards
sudip