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

Sudip Mukherjee
2016-02-05T17:04:49Z

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