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

[Sudip Mukherjee]

On Thu, Apr 23, 2015 at 06:18:37PM +0300, Dan Carpenter wrote:
> On Tue, Apr 21, 2015 at 07:22:34PM +0530, Sudip Mukherjee wrote:
<snip>
> >  	.owner		= NULL,
> >  };
> >  
> > +struct bus_type parport_bus_type = {
> > +	.name           = "parport",
> > +};
> 
> Can you make this static?  The indenting is a bit off.

done in v3.
> 
<snip>
> > + * which it wants to register its device.
> > + */
> > +static int port_check(struct device *dev, void *drv)
> > +{
> > +	((struct parport_driver *)drv)->check(to_parport_dev(dev));
> > +	return 0;
> > +}
> 
> The cast isn't beautiful.  Do this:
> 
> static int port_check(struct device *dev, void *_drv)
> {
> 	struct parport_driver *drv = _drv;
> 
> 	drv->check(to_parport_dev(dev));
> 	return 0;
> }
> 
> 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() ? 
> 
> 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.

> > +
> > +/*
<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?

> 
> > +	par_dev->port = port;
> > +	par_dev->daisy = -1;
> > +	par_dev->preempt = par_dev_cb->preempt;
> > +	par_dev->wakeup = par_dev_cb->wakeup;
> > +	par_dev->private = par_dev_cb->private;
> > +	par_dev->flags = par_dev_cb->flags;
> > +	par_dev->irq_func = par_dev_cb->irq_func;
> > +	par_dev->waiting = 0;
> > +	par_dev->timeout = 5 * HZ;
> > +
> > +	par_dev->dev.parent = &port->ddev;
> > +	par_dev->dev.bus = &parport_bus_type;
> > +	dev_set_name(&par_dev->dev, "%s", devname);
> 
> dev_set_name() allocates a copy of "devname".  It can fail with -ENOMEM.
> It's not likely but we check all the other allocations so we may as
> well check here.
ok.
> 
> > +	par_dev->dev.release = free_pardevice;
> > +	par_dev->devmodel = true;
> > +	ret = device_register(&par_dev->dev);
> > +	if (ret)
> > +		goto err_free_all;
> 
> This is a bad name because as soon as you add a new allocation you will
> have to change the name.  Use something like err_put_dev.
you have said this in your last mail also. i missed it .. sorry .. :(
		
> 
> > +
> > -
> > +	struct device ddev;	/* to link with the bus */
> 
> What does ddev stand for?  Anyway, it's probably better to call it
> bus_dev?
ok.
I think I will wait today for some comments from Greg and then send in
the v3 tomorrow.

thanks for your time and the review.

regards
sudip

> 
> 
> regards,
> dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/