Re: [PATCH 1/2] tegra20: add pinctrl driver

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, May 06, 2013 at 05:27:25PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > +
> > +static struct pinctrl_ops pinctrl_tegra20_ops = {
> > +	.set_state = pinctrl_tegra20_set_state,
> > +};
> > +
> > +static int pinctrl_tegra20_probe(struct device_d *dev)
> > +{
> > +	struct pinctrl_tegra20 *ctrl;
> > +	int i, ret = 0;
> no need the init of ret
> > +
> > +	ctrl = xzalloc(sizeof(*ctrl));
> > +
> > +	/*
> > +	 * Tegra pincontrol is split out into four independent memory ranges:
> > +	 * tristate control, function mux, pullup/down control, pad control
> > +	 * (from lowest to highest hardware address).
> > +	 * We are only interested in the first three for now.
> > +	 */
> > +	for (i = 0; i <= 2; i++) {
> > +		ctrl->regs[i] = dev_request_mem_region(dev, i);

The return values of dev_request_mem_region should be checked for new
drivers. I know 98% of the code currently does not do this and it was a
mistake. The above may fail if regions overlap or there are other bugs
in the devicetree. Having this silently fail means that the actual
register accesses do a NULL pointer deref later.

> > +	}
> drop {}
> 
> but use a specific name will simplify debug

I don't think this should be mandatory.

> 
> and make the code easier
> > +
> > +	ctrl->pinctrl.dev = dev;
> > +	ctrl->pinctrl.ops = &pinctrl_tegra20_ops;
> > +
> > +	ret = pinctrl_register(&ctrl->pinctrl);
> > +	if (ret)
> > +		free(ctrl);
> report an error message

The core will answer with an error message here. There is no need to
litter the code with error messages which only add to the binary size.
The above may fail for a developer but really should not fail for a user.
A developer has enough information with the probe functions error code.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox