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