From: Jonathan Cameron <jic23@kernel.org>
To: Hartmut Knaack <knaack.h@gmx.de>,
Daniel Baluta <daniel.baluta@intel.com>
Cc: lars@metafoo.de, pmeerw@pmeerw.net, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org,
srinivas.pandruvada@linux.intel.com
Subject: Re: [PATCH 03/10] iio: imu: kmx61: Enhance error handling
Date: Sun, 04 Jan 2015 10:45:41 +0000 [thread overview]
Message-ID: <54A919D5.2000408@kernel.org> (raw)
In-Reply-To: <54A54FE2.1020407@gmx.de>
On 01/01/15 13:47, Hartmut Knaack wrote:
> Daniel Baluta schrieb am 23.12.2014 um 14:22:
>> This fixes parts of kmx61 error handling to make code easier to read and to be
>> more consistent with IIO coding conventions:
>> * prefer as single point for error handling instead of duplicating code
>> for each function
>> * directly return a value from a case branch instead of breaking
>> * fix error message for writing REG_CTRL1
>>
>> Also, add separate error paths for kmx61_trigger_setup/iio_triggered_buffer_setup
>> calls.
> Some issues remain in this one, please see inline. Otherwise looking good.
>
Fixed up and applied.
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>> drivers/iio/imu/kmx61.c | 100 +++++++++++++++++++++---------------------------
>> 1 file changed, 43 insertions(+), 57 deletions(-)
>>
>> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
>> index e9cbd91..4fc4f63 100644
>> --- a/drivers/iio/imu/kmx61.c
>> +++ b/drivers/iio/imu/kmx61.c
>> @@ -656,11 +656,7 @@ static int kmx61_setup_new_data_interrupt(struct kmx61_data *data,
>> return ret;
>> }
>>
>> - ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> - if (ret)
>> - return ret;
>> -
>> - return 0;
>> + return kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> }
>>
>> static int kmx61_chip_update_thresholds(struct kmx61_data *data)
>> @@ -678,12 +674,10 @@ static int kmx61_chip_update_thresholds(struct kmx61_data *data)
>> ret = i2c_smbus_write_byte_data(data->client,
>> KMX61_REG_WUF_THRESH,
>> data->wake_thresh);
>> - if (ret < 0) {
>> + if (ret < 0)
>> dev_err(&data->client->dev, "Error writing reg_wuf_thresh\n");
>> - return ret;
>> - }
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int kmx61_setup_any_motion_interrupt(struct kmx61_data *data,
>> @@ -737,11 +731,7 @@ static int kmx61_setup_any_motion_interrupt(struct kmx61_data *data,
>> return ret;
>> }
>> mode |= KMX61_ACT_STBY_BIT;
>> - ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> - if (ret)
>> - return ret;
>> -
>> - return 0;
>> + return kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> }
>>
>> /**
>> @@ -924,10 +914,10 @@ static int kmx61_read_event(struct iio_dev *indio_dev,
>> switch (info) {
>> case IIO_EV_INFO_VALUE:
>> *val = data->wake_thresh;
>> - break;
>> + return IIO_VAL_INT;
>> case IIO_EV_INFO_PERIOD:
>> *val = data->wake_duration;
>> - break;
>> + return IIO_VAL_INT;
>> default:
>> return -EINVAL;
>> }
> Down below this line is still a return IIO_VAL_INT, which is now obsolete.
>> @@ -950,10 +940,10 @@ static int kmx61_write_event(struct iio_dev *indio_dev,
>> switch (info) {
>> case IIO_EV_INFO_VALUE:
>> data->wake_thresh = val;
>> - break;
>> + return IIO_VAL_INT;
>> case IIO_EV_INFO_PERIOD:
>> data->wake_duration = val;
>> - break;
>> + return IIO_VAL_INT;
>> default:
>> return -EINVAL;
>> }
> Same obsolete return exists below this line.
>> @@ -978,7 +968,7 @@ static int kmx61_write_event_config(struct iio_dev *indio_dev,
>> int state)
>> {
>> struct kmx61_data *data = kmx61_get_data(indio_dev);
>> - int ret;
>> + int ret = 0;
>>
>> if (state && data->ev_enable_state)
>> return 0;
>> @@ -987,27 +977,25 @@ static int kmx61_write_event_config(struct iio_dev *indio_dev,
>>
>> if (!state && data->motion_trig_on) {
>> data->ev_enable_state = 0;
>> - mutex_unlock(&data->lock);
>> - return 0;
>> + goto err_unlock;
>> }
>>
>> ret = kmx61_set_power_state(data, state, KMX61_ACC);
>> - if (ret < 0) {
>> - mutex_unlock(&data->lock);
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto err_unlock;
>>
>> ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
>> if (ret < 0) {
>> kmx61_set_power_state(data, false, KMX61_ACC);
>> - mutex_unlock(&data->lock);
>> - return ret;
>> + goto err_unlock;
>> }
>>
>> data->ev_enable_state = state;
>> +
>> +err_unlock:
>> mutex_unlock(&data->lock);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int kmx61_acc_validate_trigger(struct iio_dev *indio_dev,
>> @@ -1066,8 +1054,7 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>
>> if (!state && data->ev_enable_state && data->motion_trig_on) {
>> data->motion_trig_on = false;
>> - mutex_unlock(&data->lock);
>> - return 0;
>> + goto err_unlock;
>> }
>>
>>
>> @@ -1077,10 +1064,8 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> device = KMX61_MAG;
>>
>> ret = kmx61_set_power_state(data, state, device);
>> - if (ret < 0) {
>> - mutex_unlock(&data->lock);
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto err_unlock;
>>
>> if (data->acc_dready_trig == trig || data->mag_dready_trig == trig)
>> ret = kmx61_setup_new_data_interrupt(data, state, device);
>> @@ -1088,8 +1073,7 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> ret = kmx61_setup_any_motion_interrupt(data, state, KMX61_ACC);
>> if (ret < 0) {
>> kmx61_set_power_state(data, false, device);
>> - mutex_unlock(&data->lock);
>> - return ret;
>> + goto err_unlock;
>> }
>>
>> if (data->acc_dready_trig == trig)
>> @@ -1098,10 +1082,10 @@ static int kmx61_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> data->mag_dready_trig_on = state;
>> else
>> data->motion_trig_on = state;
>> -
>> +err_unlock:
>> mutex_unlock(&data->lock);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int kmx61_trig_try_reenable(struct iio_trigger *trig)
>> @@ -1207,7 +1191,7 @@ ack_intr:
>> ret |= KMX61_REG_CTRL1_BIT_RES;
>> ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>> if (ret < 0)
>> - dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>> + dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>>
>> ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_INL);
>> if (ret < 0)
>> @@ -1409,15 +1393,17 @@ static int kmx61_probe(struct i2c_client *client,
>> data->acc_dready_trig =
>> kmx61_trigger_setup(data, data->acc_indio_dev,
>> "dready");
>> - if (IS_ERR(data->acc_dready_trig))
>> - return PTR_ERR(data->acc_dready_trig);
>> + if (IS_ERR(data->acc_dready_trig)) {
>> + ret = PTR_ERR(data->acc_dready_trig);
> Double whitespace.
>> + goto err_chip_uninit;
>> + }
>>
>> data->mag_dready_trig =
>> kmx61_trigger_setup(data, data->mag_indio_dev,
>> "dready");
>> if (IS_ERR(data->mag_dready_trig)) {
>> ret = PTR_ERR(data->mag_dready_trig);
>> - goto err_trigger_unregister;
>> + goto err_trigger_unregister_acc_dready;
>> }
>>
>> data->motion_trig =
>> @@ -1425,7 +1411,7 @@ static int kmx61_probe(struct i2c_client *client,
>> "any-motion");
>> if (IS_ERR(data->motion_trig)) {
>> ret = PTR_ERR(data->motion_trig);
>> - goto err_trigger_unregister;
>> + goto err_trigger_unregister_mag_dready;
>> }
>>
>> ret = iio_triggered_buffer_setup(data->acc_indio_dev,
>> @@ -1435,7 +1421,7 @@ static int kmx61_probe(struct i2c_client *client,
>> if (ret < 0) {
>> dev_err(&data->client->dev,
>> "Failed to setup acc triggered buffer\n");
>> - goto err_trigger_unregister;
>> + goto err_trigger_unregister_motion;
>> }
>>
>> ret = iio_triggered_buffer_setup(data->mag_indio_dev,
>> @@ -1445,14 +1431,14 @@ static int kmx61_probe(struct i2c_client *client,
>> if (ret < 0) {
>> dev_err(&data->client->dev,
>> "Failed to setup mag triggered buffer\n");
>> - goto err_trigger_unregister;
>> + goto err_buffer_cleanup_acc;
>> }
>> }
>>
>> ret = iio_device_register(data->acc_indio_dev);
>> if (ret < 0) {
>> dev_err(&client->dev, "Failed to register acc iio device\n");
>> - goto err_buffer_cleanup;
>> + goto err_buffer_cleanup_mag;
>> }
>>
>> ret = iio_device_register(data->mag_indio_dev);
>> @@ -1475,18 +1461,18 @@ err_iio_unregister_mag:
>> iio_device_unregister(data->mag_indio_dev);
>> err_iio_unregister_acc:
>> iio_device_unregister(data->acc_indio_dev);
>> -err_buffer_cleanup:
>> - if (client->irq >= 0) {
>> - iio_triggered_buffer_cleanup(data->acc_indio_dev);
>> +err_buffer_cleanup_mag:
>> + if (client->irq >= 0)
>> iio_triggered_buffer_cleanup(data->mag_indio_dev);
>> - }
>> -err_trigger_unregister:
>> - if (data->acc_dready_trig)
>> - iio_trigger_unregister(data->acc_dready_trig);
>> - if (data->mag_dready_trig)
>> - iio_trigger_unregister(data->mag_dready_trig);
>> - if (data->motion_trig)
>> - iio_trigger_unregister(data->motion_trig);
>> +err_buffer_cleanup_acc:
>> + if (client->irq >= 0)
>> + iio_triggered_buffer_cleanup(data->acc_indio_dev);
>> +err_trigger_unregister_motion:
>> + iio_trigger_unregister(data->motion_trig);
>> +err_trigger_unregister_mag_dready:
>> + iio_trigger_unregister(data->mag_dready_trig);
>> +err_trigger_unregister_acc_dready:
>> + iio_trigger_unregister(data->acc_dready_trig);
>> err_chip_uninit:
>> kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>> return ret;
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
next prev parent reply other threads:[~2015-01-04 10:45 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-23 13:22 [PATCH 00/10] iio: KMX61 fixes Daniel Baluta
2014-12-23 13:22 ` [PATCH 01/10] iio: imu: kmx61: Save odr_bits for later use Daniel Baluta
2015-01-01 13:45 ` Hartmut Knaack
2015-01-04 10:41 ` Jonathan Cameron
2014-12-23 13:22 ` [PATCH 02/10] iio: imu: kmx61: Don't ignore kmx61_set_power_state errors Daniel Baluta
2015-01-01 13:45 ` Hartmut Knaack
2015-01-04 10:42 ` Jonathan Cameron
2014-12-23 13:22 ` [PATCH 03/10] iio: imu: kmx61: Enhance error handling Daniel Baluta
2015-01-01 13:47 ` Hartmut Knaack
2015-01-04 10:45 ` Jonathan Cameron [this message]
2015-01-05 8:57 ` Daniel Baluta
2014-12-23 13:22 ` [PATCH 04/10] iio: imu: kmx61: Fixup parameters alignment Daniel Baluta
2015-01-01 13:47 ` Hartmut Knaack
2015-01-04 10:47 ` Jonathan Cameron
2014-12-23 13:22 ` [PATCH 05/10] iio: imu: kmx61: Drop unused device parameter Daniel Baluta
2015-01-01 13:49 ` Hartmut Knaack
2014-12-23 13:22 ` [PATCH 06/10] iio: imu: kmx61: Use false instead of 0 for ev_enable_state Daniel Baluta
2015-01-01 13:50 ` Hartmut Knaack
2015-01-04 10:49 ` Jonathan Cameron
2014-12-23 13:22 ` [PATCH 07/10] iio: imu: kmx61: Fix device initialization when setting trigger state Daniel Baluta
2015-01-01 13:51 ` Hartmut Knaack
2015-01-04 10:51 ` Jonathan Cameron
2014-12-23 13:22 ` [PATCH 08/10] iio: imu: kmx61: Remove unnecessary REG_INS1 read Daniel Baluta
2015-01-01 13:51 ` Hartmut Knaack
2015-01-04 10:54 ` Jonathan Cameron
2014-12-23 13:22 ` [PATCH 09/10] iio: imu: kmx61: Drop odr_bits from kmx61_samp_freq_table Daniel Baluta
2015-01-01 13:53 ` Hartmut Knaack
2015-01-04 10:55 ` Jonathan Cameron
2015-01-04 11:19 ` Hartmut Knaack
2014-12-23 13:22 ` [PATCH 10/10] iio: imu: kmx61: Use correct base when reading data Daniel Baluta
2015-01-01 13:54 ` Hartmut Knaack
2014-12-26 9:57 ` [PATCH 00/10] iio: KMX61 fixes Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54A919D5.2000408@kernel.org \
--to=jic23@kernel.org \
--cc=daniel.baluta@intel.com \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
--cc=srinivas.pandruvada@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).