Re: [PATCH v2 WIP 1/2] parport: add device-model to parport subsystem

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

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.
> 
> > 
> > > > +
> > > > +/*
> > <snip>	
> > > > +
> > > > +	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
>