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

Sudip Mukherjee
2016-02-05T16:50:58Z

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