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

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

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