On Sun, 07 Feb 2016 15:49:34 +0100,
Sudip Mukherjee wrote:
>
> 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@xxxxxxxxxxxxxxx>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> 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.
The original code uses parport_register_port() and this assigns a new
temporary number by itself. Well, the issue is cosmetic, but...
> 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.
... that's the only and biggest problem. The whole rewrite needs the
check with the actual hardware, ideally.
> >>> 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?
This really depends on the demand. As already mentioned, if your
change is about getting rid of the whole legacy
parport_register_port() and its old API, it'd be worth to take a
risk. But then you should really concentrate only on that. Just
convert it without playing too much with white space changes, etc, and
make it in a series of the whole patchset (or at least show a "big
picture").
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
