From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756430AbcBIMGF (ORCPT ); Tue, 9 Feb 2016 07:06:05 -0500 Received: from mail-pa0-f48.google.com ([209.85.220.48]:33286 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751841AbcBIMGB (ORCPT ); Tue, 9 Feb 2016 07:06:01 -0500 Date: Tue, 9 Feb 2016 17:35:46 +0530 From: Sudip Mukherjee To: Takashi Iwai 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 Message-ID: <20160209120546.GA12375@sudip-pc> References: <20160205061706.GA6374@sudip-pc> <56B4D2EB.4060006@gmail.com> <56B4DA2A.3050805@gmail.com> <56B7597E.6090407@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 09, 2016 at 12:32:55PM +0100, Takashi Iwai wrote: > 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 > > >>>>>>>>> --- > > >>>>>>>>> > > >>>>>>>>> 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... I think you mean parport_register_device()... > > > 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. :( If I get the chance to rewrite I will remove the platform device. > > > > >>> 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"). But since all of it depends on old hardware so I am approaching very slowly. Only 2 - 3 drivers converterd in one release and I wait for any news about some regression. Fortunately till now no regression, and some of the other drivers (panel, joystick, paride, i2c-parport etc.) were also tested after the change. The plan is to convert all drivers using parport_register_device() to parport_register_dev_model() first. Infact i have systems with parallel port but no hardware to check the daisy chain so I can not touch the old API related to that. I will send you the modified patch after removing that temp device. regards sudip