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

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

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