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

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

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