Re: [PATCH v5 WIP 5/5] paride: use new parport device model

[Jean Delvare]

Hi Sudip,

On Wed,  6 May 2015 15:46:17 +0530, Sudip Mukherjee wrote:
> modify paride driver to use the new parallel port device model.

Leading capital please ;-)

> 
> Signed-off-by: Sudip Mukherjee <sudip@xxxxxxxxxxxxxxx>
> ---
> 
> in v4 of i2c-parport patch Jean mentioned to use the full name while
> in probe. That has been done in other drivers except this one.
> The higher layer drivers (pcd , pd etc.) are appending the unit number
> to the name before the name is being sent to the lower level (paride).

Mmmpf. If not all drivers can stick to the convention, this kind of
voids my original point.

Anyway... Again not a complete review, just two things which caught my
eye:

> -static int pi_register_parport(PIA * pi, int verbose)
> +static int pi_register_parport(PIA *pi, int verbose, int unit)
>  {
>  	struct parport *port;
> +	struct pardev_cb *par_cb;
>  
>  	port = parport_find_base(pi->port);
>  	if (!port)
>  		return 0;
> -
> -	pi->pardev = parport_register_device(port,
> -					     pi->device, NULL,
> -					     pi_wake_up, NULL, 0, (void *) pi);
> +	par_cb = kzalloc(sizeof(*par_cb), GFP_KERNEL);
> +	if (!par_cb) {
> +		parport_put_port(port);
> +		return 0;
> +	}
> +	par_cb->flags = 0;
> +	par_cb->wakeup = pi_wake_up;
> +	par_cb->private = (void *)pi;
> +	pi->pardev = parport_register_dev_model(port, pi->device, par_cb,
> +						unit);

If pi->device already includes the device number, and you are passing
it again as unit, won't you end up with odd names like pcd0.0, pcd1.1,
pcd2.2 etc?

>  	parport_put_port(port);
> +	kfree(par_cb);

If you don't need par_cb anywhere else then you shouldn't be allocating
it dynamically. It's small enough to fit on the stack really.

-- 
Jean Delvare
SUSE L3 Support
--
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/