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

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

Sep 10, 2013, 5:49 AM

Post #1 of 4 (146 views)

This patch contains some pretty basic clean-ups in probe() pertaining to
the simplification of error handling and a couple of readability adaptions.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
drivers/iio/pressure/st_pressure_core.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 3abada2..6ffd949 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -232,21 +232,23 @@ static const struct iio_trigger_ops st_press_trigger_ops = {
int st_press_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *plat_data)
{
- int err;
struct st_sensor_data *pdata = iio_priv(indio_dev);
+ int irq = pdata->get_irq_data_ready(indio_dev);
+ int err;

indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &press_info;

err = st_sensors_check_device_support(indio_dev,
- ARRAY_SIZE(st_press_sensors), st_press_sensors);
+ ARRAY_SIZE(st_press_sensors),
+ st_press_sensors);
if (err < 0)
- goto st_press_common_probe_error;
+ return err;

pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
- pdata->multiread_bit = pdata->sensor->multi_read_bit;
- indio_dev->channels = pdata->sensor->ch;
- indio_dev->num_channels = pdata->sensor->num_ch;
+ pdata->multiread_bit = pdata->sensor->multi_read_bit;
+ indio_dev->channels = pdata->sensor->ch;
+ indio_dev->num_channels = pdata->sensor->num_ch;

if (pdata->sensor->fs.addr != 0)
pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
@@ -261,32 +263,30 @@ int st_press_common_probe(struct iio_dev *indio_dev,

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

jic23 at kernel

Sep 10, 2013, 9:23 AM

Post #2 of 4 (133 views)

Permalink

Lee Jones <lee.jones@linaro.org> wrote:
>This patch contains some pretty basic clean-ups in probe() pertaining
>to
>the simplification of error handling and a couple of readability
>adaptions.
>
>Signed-off-by: Lee Jones <lee.jones@linaro.org>
>---
>drivers/iio/pressure/st_pressure_core.c | 32
>++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/iio/pressure/st_pressure_core.c
>b/drivers/iio/pressure/st_pressure_core.c
>index 3abada2..6ffd949 100644
>--- a/drivers/iio/pressure/st_pressure_core.c
>+++ b/drivers/iio/pressure/st_pressure_core.c
>@@ -232,21 +232,23 @@ static const struct iio_trigger_ops
>st_press_trigger_ops = {
> int st_press_common_probe(struct iio_dev *indio_dev,
> struct st_sensors_platform_data *plat_data)
> {
>- int err;
> struct st_sensor_data *pdata = iio_priv(indio_dev);
>+ int irq = pdata->get_irq_data_ready(indio_dev);
>+ int err;
>
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &press_info;
>
> err = st_sensors_check_device_support(indio_dev,
>- ARRAY_SIZE(st_press_sensors), st_press_sensors);
>+ ARRAY_SIZE(st_press_sensors),
>+ st_press_sensors);
> if (err < 0)
>- goto st_press_common_probe_error;
>+ return err;
>
> pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>- pdata->multiread_bit = pdata->sensor->multi_read_bit;
>- indio_dev->channels = pdata->sensor->ch;
>- indio_dev->num_channels = pdata->sensor->num_ch;
>+ pdata->multiread_bit = pdata->sensor->multi_read_bit;
>+ indio_dev->channels = pdata->sensor->ch;
>+ indio_dev->num_channels = pdata->sensor->num_ch;
>
> if (pdata->sensor->fs.addr != 0)
> pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>@@ -261,32 +263,30 @@ int st_press_common_probe(struct iio_dev
>*indio_dev,
>
> 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!
>+ 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);

--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

lee.jones at linaro

Sep 11, 2013, 12:19 AM

Post #3 of 4 (134 views)

Permalink

> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

jic23 at kernel

Sep 14, 2013, 10:20 AM

Post #4 of 4 (133 views)

Permalink

On 09/11/13 08:19, Lee Jones wrote:
>>> 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'.

>>> + 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);
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/