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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
