Messages in this thread | | | Date | Fri, 05 Feb 2016 22:51:46 +0530 | From | Sudip Mukherjee <> | Subject | Re: [PATCH v3 3/3] portman2x4 - use new parport device model |
| |
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
| |