[alsa-devel] [PATCH 1/6] [ALSA] portman2x4 - fix trailing whitespace
Trailing whitespace is not encouraged by Kernel coding style.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org --- sound/drivers/portman2x4.c | 50 +++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 464385a..ba0860d 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 */ @@ -140,7 +140,7 @@ static int portman_create(struct snd_card *card, #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. @@ -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. */ @@ -370,7 +370,7 @@ 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) { @@ -456,7 +456,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 +481,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 +567,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 +584,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 +621,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 +632,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 +658,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 +742,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 +756,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 +779,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; @@ -844,7 +844,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); }
checkpatch complains about multiple blank lines and missing blank line after declaration.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org --- sound/drivers/portman2x4.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index ba0860d..4f9717b 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -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. */
@@ -375,6 +373,7 @@ static int portman_read_midi(struct portman *pm, int port) 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 */ @@ -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,
checkpatch complains about space before tab.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org --- sound/drivers/portman2x4.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 4f9717b..65b0650 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -123,21 +123,21 @@ 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 @@ -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. */
Instead of checking var == NULL, the kernel coding style recommends writing it as !var.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org --- sound/drivers/portman2x4.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 65b0650..985cf67 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -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) return -ENOMEM;
/* Init chip specific data */ @@ -764,7 +764,7 @@ static int snd_portman_probe(struct platform_device *pdev) snd_portman_interrupt, /* ISR */ PARPORT_DEV_EXCL, /* flags */ (void *)card); /* private */ - if (pardev == NULL) { + if (!pardev) { snd_printd("Cannot register pardevice\n"); err = -EIO; goto __err;
It is not general practice to assign some values inside if statement.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org --- 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 985cf67..5fcde7d 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) {
Modify portman driver to use the new parallel port device model.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org --- sound/drivers/portman2x4.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 5fcde7d..88b25ca 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -704,9 +704,10 @@ static void snd_portman_detach(struct parport *p) }
static struct parport_driver portman_parport_driver = { - .name = "portman2x4", - .attach = snd_portman_attach, - .detach = snd_portman_detach + .name = "portman2x4", + .match_port = snd_portman_attach, + .detach = snd_portman_detach, + .devmodel = true, };
/********************************************************************* @@ -734,6 +735,7 @@ 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;
p = platform_get_drvdata(pdev); platform_set_drvdata(pdev, NULL); @@ -758,13 +760,15 @@ 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 */ + memset(&portman_cb, 0, sizeof(portman_cb)); + portman_cb.private = card; /* private */ + portman_cb.irq_func = snd_portman_interrupt; /* ISR */ + portman_cb.flags = PARPORT_DEV_EXCL; /* flags */ + + pardev = parport_register_dev_model(p, /* port */ + DRIVER_NAME, /* name */ + &portman_cb, /* callbacks */ + device_count); /* device number */ if (!pardev) { snd_printd("Cannot register pardevice\n"); err = -EIO;
On Thu, 07 Jan 2016 08:15:51 +0100, Sudip Mukherjee wrote:
Modify portman driver to use the new parallel port device model.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org
Did you actually test this?
Also about the changes:
sound/drivers/portman2x4.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 5fcde7d..88b25ca 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -704,9 +704,10 @@ static void snd_portman_detach(struct parport *p) }
static struct parport_driver portman_parport_driver = {
- .name = "portman2x4",
- .attach = snd_portman_attach,
- .detach = snd_portman_detach
- .name = "portman2x4",
- .match_port = snd_portman_attach,
- .detach = snd_portman_detach,
- .devmodel = true,
};
/********************************************************************* @@ -734,6 +735,7 @@ 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;
p = platform_get_drvdata(pdev); platform_set_drvdata(pdev, NULL);
@@ -758,13 +760,15 @@ 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 */
- memset(&portman_cb, 0, sizeof(portman_cb));
- portman_cb.private = card; /* private */
- portman_cb.irq_func = snd_portman_interrupt; /* ISR */
- portman_cb.flags = PARPORT_DEV_EXCL; /* flags */
You can put them initializers except for private. Then the explicit memset can be omitted.
- pardev = parport_register_dev_model(p, /* port */
DRIVER_NAME, /* name */&portman_cb, /* callbacks */device_count); /* device number */
Does device_count really work similarly for parport_register_dev_model()? I supposed the argument being the device id number while you're passing the number of devices to create.
Takashi
if (!pardev) { snd_printd("Cannot register pardevice\n"); err = -EIO; -- 1.9.1
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Jan 07, 2016 at 11:26:44AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 08:15:51 +0100, Sudip Mukherjee wrote:
Modify portman driver to use the new parallel port device model.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org
Did you actually test this?
No. :( I donot have the hardware. But since the only change is in the way it registers with the parport so it should not break. I was preparing v2 for this and the other one. I missed seeing some more points.
Also about the changes:
sound/drivers/portman2x4.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 5fcde7d..88b25ca 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -704,9 +704,10 @@ static void snd_portman_detach(struct parport *p) }
static struct parport_driver portman_parport_driver = {
- .name = "portman2x4",
- .attach = snd_portman_attach,
- .detach = snd_portman_detach
- .name = "portman2x4",
- .match_port = snd_portman_attach,
- .detach = snd_portman_detach,
- .devmodel = true,
};
/********************************************************************* @@ -734,6 +735,7 @@ 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;
p = platform_get_drvdata(pdev); platform_set_drvdata(pdev, NULL);
@@ -758,13 +760,15 @@ 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 */
- memset(&portman_cb, 0, sizeof(portman_cb));
- portman_cb.private = card; /* private */
- portman_cb.irq_func = snd_portman_interrupt; /* ISR */
- portman_cb.flags = PARPORT_DEV_EXCL; /* flags */
You can put them initializers except for private. Then the explicit memset can be omitted.
- pardev = parport_register_dev_model(p, /* port */
DRIVER_NAME, /* name */&portman_cb, /* callbacks */device_count); /* device number */Does device_count really work similarly for parport_register_dev_model()? I supposed the argument being the device id number while you're passing the number of devices to create.
This device_count is actually used for the device name in /sys/bus/parport/devices. Something like DRIVER_NAME.device_count.
Regards Sudip
On Thu, 07 Jan 2016 11:44:34 +0100, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 11:26:44AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 08:15:51 +0100, Sudip Mukherjee wrote:
Modify portman driver to use the new parallel port device model.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org
Did you actually test this?
No. :( I donot have the hardware. But since the only change is in the way it registers with the parport so it should not break. I was preparing v2 for this and the other one. I missed seeing some more points.
Also about the changes:
sound/drivers/portman2x4.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 5fcde7d..88b25ca 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -704,9 +704,10 @@ static void snd_portman_detach(struct parport *p) }
static struct parport_driver portman_parport_driver = {
- .name = "portman2x4",
- .attach = snd_portman_attach,
- .detach = snd_portman_detach
- .name = "portman2x4",
- .match_port = snd_portman_attach,
- .detach = snd_portman_detach,
- .devmodel = true,
};
/********************************************************************* @@ -734,6 +735,7 @@ 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;
p = platform_get_drvdata(pdev); platform_set_drvdata(pdev, NULL);
@@ -758,13 +760,15 @@ 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 */
- memset(&portman_cb, 0, sizeof(portman_cb));
- portman_cb.private = card; /* private */
- portman_cb.irq_func = snd_portman_interrupt; /* ISR */
- portman_cb.flags = PARPORT_DEV_EXCL; /* flags */
You can put them initializers except for private. Then the explicit memset can be omitted.
- pardev = parport_register_dev_model(p, /* port */
DRIVER_NAME, /* name */&portman_cb, /* callbacks */device_count); /* device number */Does device_count really work similarly for parport_register_dev_model()? I supposed the argument being the device id number while you're passing the number of devices to create.
This device_count is actually used for the device name in /sys/bus/parport/devices. Something like DRIVER_NAME.device_count.
Well, but device_count is incremented in snd_portman_attach(). The management of device_count should be moved around the caller side, if we use this as the id (and use the assigned id instead of device_count in snd_portman_attach()).
Takashi
On Thu, Jan 07, 2016 at 11:50:15AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 11:44:34 +0100, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 11:26:44AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 08:15:51 +0100, Sudip Mukherjee wrote:
Modify portman driver to use the new parallel port device model.
Signed-off-by: Sudip Mukherjee sudip@vectorindia.org
Did you actually test this?
No. :( I donot have the hardware. But since the only change is in the way it registers with the parport so it should not break. I was preparing v2 for this and the other one. I missed seeing some more points.
Also about the changes:
sound/drivers/portman2x4.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 5fcde7d..88b25ca 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -704,9 +704,10 @@ static void snd_portman_detach(struct parport *p) }
static struct parport_driver portman_parport_driver = {
- .name = "portman2x4",
- .attach = snd_portman_attach,
- .detach = snd_portman_detach
- .name = "portman2x4",
- .match_port = snd_portman_attach,
- .detach = snd_portman_detach,
- .devmodel = true,
};
/********************************************************************* @@ -734,6 +735,7 @@ 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;
p = platform_get_drvdata(pdev); platform_set_drvdata(pdev, NULL);
@@ -758,13 +760,15 @@ 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 */
- memset(&portman_cb, 0, sizeof(portman_cb));
- portman_cb.private = card; /* private */
- portman_cb.irq_func = snd_portman_interrupt; /* ISR */
- portman_cb.flags = PARPORT_DEV_EXCL; /* flags */
You can put them initializers except for private. Then the explicit memset can be omitted.
- pardev = parport_register_dev_model(p, /* port */
DRIVER_NAME, /* name */&portman_cb, /* callbacks */device_count); /* device number */Does device_count really work similarly for parport_register_dev_model()? I supposed the argument being the device id number while you're passing the number of devices to create.
This device_count is actually used for the device name in /sys/bus/parport/devices. Something like DRIVER_NAME.device_count.
Well, but device_count is incremented in snd_portman_attach(). The management of device_count should be moved around the caller side, if we use this as the id (and use the assigned id instead of device_count in snd_portman_attach()).
But, snd_portman_attach() finally decides if the probe/attach was a success or not. And it will save the device in platform_devices[device_count] and then it will increment device_count to prepare it for the next device. Ofcourse, we can do it in snd_portman_probe() but isn't snd_portman_attach() the caller here?
If you want I can move the count to snd_portman_probe() but since I do not have the hardware I tried to have the minimum possible change.
regards sudip
On Thu, Jan 07, 2016 at 04:31:27PM +0530, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 11:50:15AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 11:44:34 +0100, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 11:26:44AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 08:15:51 +0100, Sudip Mukherjee wrote:
<snip>
- pardev = parport_register_dev_model(p, /* port */
DRIVER_NAME, /* name */&portman_cb, /* callbacks */device_count); /* device number */Does device_count really work similarly for parport_register_dev_model()? I supposed the argument being the device id number while you're passing the number of devices to create.
This device_count is actually used for the device name in /sys/bus/parport/devices. Something like DRIVER_NAME.device_count.
Well, but device_count is incremented in snd_portman_attach(). The management of device_count should be moved around the caller side, if we use this as the id (and use the assigned id instead of device_count in snd_portman_attach()).
did you mean something like this: (on top of my patch)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 88b25ca..d749786 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -688,14 +688,8 @@ static void snd_portman_attach(struct parport *p)
/* Since we dont get the return value of probe * We need to check if device probing succeeded or not */ - if (!platform_get_drvdata(device)) { + if (!platform_get_drvdata(device)) platform_device_unregister(device); - return; - } - - /* register device in global table */ - platform_devices[device_count] = device; - device_count++; }
static void snd_portman_detach(struct parport *p) @@ -768,7 +762,7 @@ static int snd_portman_probe(struct platform_device *pdev) pardev = parport_register_dev_model(p, /* port */ DRIVER_NAME, /* name */ &portman_cb, /* callbacks */ - device_count); /* device number */ + pdev->id); /* device number */ if (!pardev) { snd_printd("Cannot register pardevice\n"); err = -EIO; @@ -812,6 +806,10 @@ static int snd_portman_probe(struct platform_device *pdev) goto __err; }
+ /* register device in global table */ + platform_devices[device_count] = device; + device_count++; + snd_printk(KERN_INFO "Portman 2x4 on 0x%lx\n", p->base); return 0;
regards sudip
On Thu, 07 Jan 2016 12:19:36 +0100, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 04:31:27PM +0530, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 11:50:15AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 11:44:34 +0100, Sudip Mukherjee wrote:
On Thu, Jan 07, 2016 at 11:26:44AM +0100, Takashi Iwai wrote:
On Thu, 07 Jan 2016 08:15:51 +0100, Sudip Mukherjee wrote:
<snip> > > > > > > > > > + > > > > > + pardev = parport_register_dev_model(p, /* port */ > > > > > + DRIVER_NAME, /* name */ > > > > > + &portman_cb, /* callbacks */ > > > > > + device_count); /* device number */ > > > > > > > > Does device_count really work similarly for > > > > parport_register_dev_model()? I supposed the argument being the > > > > device id number while you're passing the number of devices to > > > > create. > > > > > > This device_count is actually used for the device name in > > > /sys/bus/parport/devices. Something like DRIVER_NAME.device_count. > > > > Well, but device_count is incremented in snd_portman_attach(). The > > management of device_count should be moved around the caller side, if > > we use this as the id (and use the assigned id instead of device_count > > in snd_portman_attach()).
did you mean something like this: (on top of my patch)
diff --git a/sound/drivers/portman2x4.c b/sound/drivers/portman2x4.c index 88b25ca..d749786 100644 --- a/sound/drivers/portman2x4.c +++ b/sound/drivers/portman2x4.c @@ -688,14 +688,8 @@ static void snd_portman_attach(struct parport *p)
/* Since we dont get the return value of probe * We need to check if device probing succeeded or not */
- if (!platform_get_drvdata(device)) {
- if (!platform_get_drvdata(device)) platform_device_unregister(device);
return;- }
- /* register device in global table */
- platform_devices[device_count] = device;
- device_count++;
}
static void snd_portman_detach(struct parport *p) @@ -768,7 +762,7 @@ static int snd_portman_probe(struct platform_device *pdev) pardev = parport_register_dev_model(p, /* port */ DRIVER_NAME, /* name */ &portman_cb, /* callbacks */
device_count); /* device number */
pdev->id); /* device number */@@ -812,6 +806,10 @@ static int snd_portman_probe(struct platform_device *pdev) goto __err; }
- /* register device in global table */
- platform_devices[device_count] = device;
- device_count++;
- snd_printk(KERN_INFO "Portman 2x4 on 0x%lx\n", p->base); return 0;
Hmm, this doesn't look better either. I think the necessary change is just the access of device_count in partport_register_dev_model(). This can be replaced with pdev->id, so that you don't touch device_count at all there.
In anyway, these patch series (also for mst) are too late for 4.5. These are neither serious bug fix nor function improvement, and the drivers you're modifying are for the very minor devices. So, please resubmit after the next merge window is closed.
While we're at it, some comments about other patches in the series: I see no big merit to split several whitespace and blank line patches. These are basically all whitespace fixes, so smash as a single patch. The most important point is that this won't change any code context at all but just a matter of white spaces. (And write it clearly in the change log -- so that reader can ignore this e.g. while bisecting.)
The NULL check is a matter of taste and isn't worth to fix at all.
The assignment in if is good to fix in general, so this can be a separate patch indeed.
thanks,
Takashi
participants (2)
-
Sudip Mukherjee -
Takashi Iwai