[PATCH 26/38] iio: pressure-core: st: Clean-up probe() function

[Lee Jones]

> > 	err = st_sensors_init_sensor(indio_dev, plat_data);
> > 	if (err < 0)
> >-		goto st_press_common_probe_error;
> >+		return err;
> > 
> >-	if (pdata->get_irq_data_ready(indio_dev) > 0) {
> >+	if (irq > 0) {
> > 		err = st_press_allocate_ring(indio_dev);
> > 		if (err < 0)
> >-			goto st_press_common_probe_error;
> >+			return err;
> > 
> > 		err = st_sensors_allocate_trigger(indio_dev,
> >-							ST_PRESS_TRIGGER_OPS);
> >+						  ST_PRESS_TRIGGER_OPS);
> > 		if (err < 0)
> > 			goto st_press_probe_trigger_error;
> > 	}
> > 
> > 	err = iio_device_register(indio_dev);
> >-	if (err)
> 
> This bit of handling is confusing. I would much rather see the if IRQ at the goto. Here the first thought is why is it not an error if there is no IRQ! 

I certainly see your point. But surely anyone would see after a second
or two that we're returning err and not 0 in this case, so the error
would still be returned, we're not ignoring it. Adding this extra
comparison saves several lines of code.

If you think it's 'too' confusing, I'll revert the change.

Or perhaps a comment:

/* Only deallocate_[trigger|ring] if they were allocated. */
or
/* Only deallocate_[trigger|ring] if we have an IRQ line. */
or
/* If no IRQ was specified, just return the error. */

> >+	if (err && irq > 0)
> > 		goto st_press_device_register_error;
> > 
> > 	return err;
> > 
> > st_press_device_register_error:
> >-	if (pdata->get_irq_data_ready(indio_dev) > 0)
> >-		st_sensors_deallocate_trigger(indio_dev);
> >+	st_sensors_deallocate_trigger(indio_dev);
> > st_press_probe_trigger_error:
> >-	if (pdata->get_irq_data_ready(indio_dev) > 0)
> >-		st_press_deallocate_ring(indio_dev);
> >-st_press_common_probe_error:
> >+	st_press_deallocate_ring(indio_dev);
> >+
> > 	return err;
> > }
> > EXPORT_SYMBOL(st_press_common_probe);
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog