On Tue, Apr 28, 2015 at 12:39:04PM -0500, J. German Rivera wrote: > Change-Id: I2a986c465989c3811de19cfe9ed0b77168250cb1 > Reviewed-on: http://git.am.freescale.net:8181/33626 > Tested-by: Review Code-CDREVIEW <CDREVIEW@xxxxxxxxxxxxx> These things are totally useless to the rest of us. Don't add them. > diff --git a/drivers/staging/fsl-mc/TODO b/drivers/staging/fsl-mc/TODO > index d78288b..1b8868d 100644 > --- a/drivers/staging/fsl-mc/TODO > +++ b/drivers/staging/fsl-mc/TODO > @@ -8,6 +8,9 @@ > * Add at least one device driver for a DPAA2 object (child device of the > fsl-mc bus). > > +* Enable code blocks guarded by #ifdef GIC_ITS_MC_SUPPORT, when GIC-ITS > + support for the MC MSIs gets merged. > + When will that be? I'd really prefer to not add these ifdeffed bits at all. > + if (status & (DPRC_IRQ_EVENT_OBJ_ADDED | > + DPRC_IRQ_EVENT_OBJ_REMOVED | > + DPRC_IRQ_EVENT_CONTAINER_DESTROYED | > + DPRC_IRQ_EVENT_OBJ_DESTROYED | > + DPRC_IRQ_EVENT_OBJ_CREATED)) { > + unsigned int irq_count; > + > + error = dprc_scan_objects(mc_dev, &irq_count); > + if (error < 0) { > + dev_err(dev, "dprc_scan_objects() failed: %d\n", error); > + goto out; > + } > + > + WARN_ON((int16_t)irq_count < 0); This code is doing "WARN_ON(test_bit(15, (unsigned long *)&irq_count));". That seems like nonsense. Anyway, just delete the WARN_ON(). > + > + if ((int16_t)irq_count > > + mc_bus->resource_pools[FSL_MC_POOL_IRQ].max_count) { Why are we casting this? Also can you align it like: if (irq_count > mc_bus->resource_pools[FSL_MC_POOL_IRQ].max_count) { [tab][tab][space][space][space][space]mc_bus->resource_pools[ That way you can tell the if condition from the indented block. Plus that is standard kernel indenting style these days. [ snip ] > + irqs = devm_kzalloc(&mc_dev->dev, irq_count * sizeof(irqs[0]), > + GFP_KERNEL); > + if (!irqs) { > + error = -ENOMEM; > + dev_err(&mc_dev->dev, "No memory to allocate irqs[]\n"); > + goto error; I kind of hate One Err Style error handling, because the error labels are so general... You can never guess the point of it until you scroll down to read what "goto error;" does. The error handling here calls devm_kfree() which is not needed... devm_ functions automatically clean up after themselves. This seems a pattern throughout. Do a search for devm_free() and see which ones are really needed or not. Also the error message isn't needed here. kzalloc() has its own better error messages built-in. Adding these error messages which will never be printed is just a waste of RAM. In other words this should look like: irqs = devm_kcalloc(&mc_dev->dev, sizeof(*irqs), irq_count, GFP_KERNEL); if (!irqs) return -ENOMEM; regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
