From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755272AbbDXHpe (ORCPT ); Fri, 24 Apr 2015 03:45:34 -0400 Received: from mail-pd0-f178.google.com ([209.85.192.178]:35020 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755250AbbDXHp3 (ORCPT ); Fri, 24 Apr 2015 03:45:29 -0400 Date: Fri, 24 Apr 2015 13:15:22 +0530 From: Sudip Mukherjee To: Dan Carpenter Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem Message-ID: <20150424074522.GG3489@sudip-PC> References: <1429624355-9252-1-git-send-email-sudipm.mukherjee@gmail.com> <20150423151837.GY16501@mwanda> <20150424065026.GC3489@sudip-PC> <20150424070454.GA16501@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150424070454.GA16501@mwanda> 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 Fri, Apr 24, 2015 at 10:04:54AM +0300, Dan Carpenter wrote: > On Fri, Apr 24, 2015 at 12:20:26PM +0530, Sudip Mukherjee wrote: > > > What is the point of the check function really? The name isn't clear. > > yes, i am a bit blunt in thinking of new names, i hope you have noticed > > that in my naming of the labels .. :) > > > > as the name was not sufficient i mentioned it in the comments. This check > > function will receive the device details and will decide if it wants to > > connect to that device. If it wants to connect then it registers its device > > and mark the port as claimed. > > Infact, on second thought, i will return the success or error from check, > > then if the driver has found the device to connect then we can stop the > > iteration there. > > > > maybe a better name can be check_port() ? > > match() or match_port() something. > > > > > > > Since it always returns zero that means we loop through all the devices > > > and then returns NULL. It feels like a function called > > > bus_find_device() should find something. We have bus_for_each_dev() if > > > we just want to iterate. > > > > > yes, bus_for_each_dev() will be better here. thanks. > > If we're match then bus_find_device() is correct. It's just that's not > what v2 did. that was the main intention but but bus_find_device() will also do a get_device() once a match is found, then in that case I will have to do a put_device() immediately after bus_find_device() completes. > > > > > > > + > > > > +/* > > > > > > + > > > > + par_dev->name = devname; > > > > > > The existing code is buggy here as we discussed previously. Could you > > > just fix that before we do anything else? It's freaking me out. > > > > quoting from your previous mail: > > >My concern is that it gets freed before we are done using it or something > > > > here, i have modified that and we are no longer using the string passed > > as an argument. we have duplicated it using kstrdup and using that and > > it gets freed in free_pardevice(). > > or am i missing something here? > > Ah. Ok. Thanks. I missed that and I don't think the patch has hit > linux-next yet. hehe , no. Greg has to apply the patch, and in the last patch he found some points in his review. I will send in a v3 as soon as the confusion about the bus_find_device() or bus_for_each_dev() clears. regards sudip > > regards, > dan carpenter >