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

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

On 21:59 Mon 06 May     , Sascha Hauer wrote:
> 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.
why not simply put a BUG in the request_mem
> 
> > > +	}
> > drop {}
> > 
> > but use a specific name will simplify debug
> 
> I don't think this should be mandatory.
the issue is that a number can be easly endup to a hiden typo (no compile
error) where a ttypo in the name will no compile.

> 
> > 
> > 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.

pinctrl_register print no error message

it's good to known why we failled no just that the probe return -Exxx

and this will not impact the binary size if we put a dev_warn or dev_debug

as we can drop them with the printk level

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