Re: [PATCH 03/10] iio: imu: kmx61: Enhance error handling
On 01/04/2015 12:45 PM, Jonathan Cameron wrote: 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. Thanks for doing this Jonathan. Daniel. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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 03/10] iio: imu: kmx61: Enhance error handling
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 >> --- >> 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); >> -
Re: [PATCH 03/10] iio: imu: kmx61: Enhance error handling
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. > > Signed-off-by: Daniel Baluta > --- > 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
[PATCH 03/10] iio: imu: kmx61: Enhance error handling
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. Signed-off-by: Daniel Baluta --- 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; } @@ -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; } @@ -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)