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

On 09/11/13 08:19, Lee Jones wrote:

quoted

quoted

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

Revert the change.  Whilst not complex to follow it is non obvious to save only
a couple of lines. Adding a comment would take almost as much space as just doing
it the 'easy way'.

quoted

quoted

+	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);