From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756186AbcBETLP (ORCPT ); Fri, 5 Feb 2016 14:11:15 -0500 Received: from mx2.suse.de ([195.135.220.15]:43231 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752902AbcBETLK (ORCPT ); Fri, 5 Feb 2016 14:11:10 -0500 Date: Fri, 05 Feb 2016 20:11:08 +0100 Message-ID: From: Takashi Iwai To: Sudip Mukherjee Cc: Jaroslav Kysela , alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/3] portman2x4 - use new parport device model In-Reply-To: <56B4DA2A.3050805@gmail.com> References: <1454603903-15967-1-git-send-email-sudipm.mukherjee@gmail.com> <1454603903-15967-3-git-send-email-sudipm.mukherjee@gmail.com> <20160205061706.GA6374@sudip-pc> <56B4D2EB.4060006@gmail.com> <56B4DA2A.3050805@gmail.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/24.5 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > >>>>>>> --- > >>>>>>> > >>>>>>> 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. > > 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... Takashi