| Message ID | 037985fe50fe79c79b9df95fa7b4e577378f9a60.1732105157.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM KX134ACR-LBZ | expand |
On Thu, 21 Nov 2024 10:20:23 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > A few functions in KX022A need to use mutex for protecting the > enabling/disabling of the measurement while configurations are being > made. Some of the functions can be slightly simplified by using the > __cleanup based scoped mutexes, which allows dropping the goto based > unlocking at error path. > > Simplify error paths using guard(mutex). > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Now we have guard(), the main reason (I think) for the combined on + lock and off + unlock paths is gone. So can we just flatten those and do the locking at caller. After this patch there are only 4 more users anyway and the ones in the switch statement can be easily done with {} and guard. The ones in probe() can be done with a scoped_guard() Jonathan > --- > > Revision history: > v1 => v2: > - patch number changed because a change was added to the series. > - rebased on iio/testing to avoid conflicts with queued fixes > --- > drivers/iio/accel/kionix-kx022a.c | 61 ++++++++++++------------------- > 1 file changed, 23 insertions(+), 38 deletions(-) > > diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c > index b6664299e0d5..98953178a580 100644 > --- a/drivers/iio/accel/kionix-kx022a.c > +++ b/drivers/iio/accel/kionix-kx022a.c > @@ -5,6 +5,7 @@ > * ROHM/KIONIX accelerometer driver > */ > > +#include <linux/cleanup.h> > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/interrupt.h> > @@ -448,7 +449,7 @@ static void kx022a_reg2scale(unsigned int val, unsigned int *val1, > *val2 = kx022a_scale_table[val][1]; > } > > -static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on) > +static int __kx022a_turn_on_off(struct kx022a_data *data, bool on) > { > int ret; > > @@ -469,7 +470,7 @@ static int kx022a_turn_off_lock(struct kx022a_data *data) > int ret; > > mutex_lock(&data->mutex); > - ret = kx022a_turn_on_off_unlocked(data, false); > + ret = __kx022a_turn_on_off(data, false); > if (ret) > mutex_unlock(&data->mutex); > > @@ -480,7 +481,7 @@ static int kx022a_turn_on_unlock(struct kx022a_data *data) > { > int ret; > > - ret = kx022a_turn_on_off_unlocked(data, true); > + ret = __kx022a_turn_on_off(data, true); > mutex_unlock(&data->mutex); > > return ret; > @@ -912,18 +913,19 @@ static int kx022a_fifo_disable(struct kx022a_data *data) > { > int ret = 0; > > - ret = kx022a_turn_off_lock(data); > + guard(mutex)(&data->mutex); > + ret = __kx022a_turn_on_off(data, false); > if (ret) > return ret; > > ret = regmap_clear_bits(data->regmap, data->ien_reg, KX022A_MASK_WMI); > if (ret) > - goto unlock_out; > + return ret; > > ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2, > KX022A_MASK_BUF_EN); > if (ret) > - goto unlock_out; > + return ret; > > data->state &= ~KX022A_STATE_FIFO; > > @@ -931,12 +933,7 @@ static int kx022a_fifo_disable(struct kx022a_data *data) > > kfree(data->fifo_buffer); > > - return kx022a_turn_on_unlock(data); > - > -unlock_out: > - mutex_unlock(&data->mutex); > - > - return ret; > + return __kx022a_turn_on_off(data, true); > } > > static int kx022a_buffer_predisable(struct iio_dev *idev) > @@ -959,33 +956,29 @@ static int kx022a_fifo_enable(struct kx022a_data *data) > if (!data->fifo_buffer) > return -ENOMEM; > > - ret = kx022a_turn_off_lock(data); > + guard(mutex)(&data->mutex); > + ret = __kx022a_turn_on_off(data, false); > if (ret) > return ret; > > /* Update watermark to HW */ > ret = kx022a_fifo_set_wmi(data); > if (ret) > - goto unlock_out; > + return ret; > > /* Enable buffer */ > ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, > KX022A_MASK_BUF_EN); > if (ret) > - goto unlock_out; > + return ret; > > data->state |= KX022A_STATE_FIFO; > ret = regmap_set_bits(data->regmap, data->ien_reg, > KX022A_MASK_WMI); > if (ret) > - goto unlock_out; > - > - return kx022a_turn_on_unlock(data); > - > -unlock_out: > - mutex_unlock(&data->mutex); > + return ret; > > - return ret; > + return __kx022a_turn_on_off(data, true); > } > > static int kx022a_buffer_postenable(struct iio_dev *idev) > @@ -1053,7 +1046,7 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) > struct kx022a_data *data = iio_priv(idev); > irqreturn_t ret = IRQ_NONE; > > - mutex_lock(&data->mutex); > + guard(mutex)(&data->mutex); > > if (data->trigger_enabled) { > iio_trigger_poll_nested(data->trig); > @@ -1068,8 +1061,6 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) > ret = IRQ_HANDLED; > } > > - mutex_unlock(&data->mutex); > - > return ret; > } > > @@ -1079,32 +1070,26 @@ static int kx022a_trigger_set_state(struct iio_trigger *trig, > struct kx022a_data *data = iio_trigger_get_drvdata(trig); > int ret = 0; > > - mutex_lock(&data->mutex); > + guard(mutex)(&data->mutex); > > if (data->trigger_enabled == state) > - goto unlock_out; > + return 0; > > if (data->state & KX022A_STATE_FIFO) { > dev_warn(data->dev, "Can't set trigger when FIFO enabled\n"); > - ret = -EBUSY; > - goto unlock_out; > + return -EBUSY; > } > > - ret = kx022a_turn_on_off_unlocked(data, false); > + ret = __kx022a_turn_on_off(data, false); > if (ret) > - goto unlock_out; > + return ret; > > data->trigger_enabled = state; > ret = kx022a_set_drdy_irq(data, state); > if (ret) > - goto unlock_out; > - > - ret = kx022a_turn_on_off_unlocked(data, true); > - > -unlock_out: > - mutex_unlock(&data->mutex); > + return ret; > > - return ret; > + return __kx022a_turn_on_off(data, true); > } > > static const struct iio_trigger_ops kx022a_trigger_ops = {
Hello Jonathan, Thanks again! On 23/11/2024 18:42, Jonathan Cameron wrote: > On Thu, 21 Nov 2024 10:20:23 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> A few functions in KX022A need to use mutex for protecting the >> enabling/disabling of the measurement while configurations are being >> made. Some of the functions can be slightly simplified by using the >> __cleanup based scoped mutexes, which allows dropping the goto based >> unlocking at error path. >> >> Simplify error paths using guard(mutex). >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > Now we have guard(), the main reason (I think) for the > combined on + lock and off + unlock paths is gone. So can > we just flatten those and do the locking at caller. I did consider this too :) Why I decided to keep it as it is, (even though we need the extra mutex_unlock() at certain error path) is because I kind of like the lock+off and unlock+on functions. This locking does not protect data, but really a sequence of operations that needs to be done while sensor is OFF state. It's almost like a doc saying that "please, ensure the sensor is OFF for the following operations" :) (Another thing is that we do claim the direct mode in write_raw, and goto is still handy for releasing it. Scoped guards won't play nicely with goto. Yes, we could probably use the __cleanup for direct mode, but I still like the lock+off, unlock+on for the reason above) Yours, -- Matti
On Mon, 25 Nov 2024 11:34:36 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > Hello Jonathan, > > Thanks again! > > On 23/11/2024 18:42, Jonathan Cameron wrote: > > On Thu, 21 Nov 2024 10:20:23 +0200 > > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > > > >> A few functions in KX022A need to use mutex for protecting the > >> enabling/disabling of the measurement while configurations are being > >> made. Some of the functions can be slightly simplified by using the > >> __cleanup based scoped mutexes, which allows dropping the goto based > >> unlocking at error path. > >> > >> Simplify error paths using guard(mutex). > >> > >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Now we have guard(), the main reason (I think) for the > > combined on + lock and off + unlock paths is gone. So can > > we just flatten those and do the locking at caller. > > I did consider this too :) > > Why I decided to keep it as it is, (even though we need the extra > mutex_unlock() at certain error path) is because I kind of like the > lock+off and unlock+on functions. This locking does not protect data, > but really a sequence of operations that needs to be done while sensor > is OFF state. It's almost like a doc saying that "please, ensure the > sensor is OFF for the following operations" :) hmm. I really don't like them because they are 'unusual' :) I'd argue they just ensure a sequence of writes go in as an atomic thing. Two of those writes happen to be turn it off and turn it on. So the data the are protecting is the device internal state data. > > (Another thing is that we do claim the direct mode in write_raw, and > goto is still handy for releasing it. Scoped guards won't play nicely > with goto. Yes, we could probably use the __cleanup for direct mode, but > I still like the lock+off, unlock+on for the reason above) There is a nice new cleanup that David did to make the direct mode handling much cleaner. if_not_cond_guard(iio_claim_direct_try, indio_dev) return -EBUSY; > > Yours, > -- Matti >
On 26/11/2024 19:55, Jonathan Cameron wrote: > On Mon, 25 Nov 2024 11:34:36 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> Hello Jonathan, >> >> Thanks again! >> >> On 23/11/2024 18:42, Jonathan Cameron wrote: >>> On Thu, 21 Nov 2024 10:20:23 +0200 >>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >>> >>>> A few functions in KX022A need to use mutex for protecting the >>>> enabling/disabling of the measurement while configurations are being >>>> made. Some of the functions can be slightly simplified by using the >>>> __cleanup based scoped mutexes, which allows dropping the goto based >>>> unlocking at error path. >>>> >>>> Simplify error paths using guard(mutex). >>>> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> >>> Now we have guard(), the main reason (I think) for the >>> combined on + lock and off + unlock paths is gone. So can >>> we just flatten those and do the locking at caller. >> >> I did consider this too :) >> >> Why I decided to keep it as it is, (even though we need the extra >> mutex_unlock() at certain error path) is because I kind of like the >> lock+off and unlock+on functions. This locking does not protect data, >> but really a sequence of operations that needs to be done while sensor >> is OFF state. It's almost like a doc saying that "please, ensure the >> sensor is OFF for the following operations" :) > > hmm. I really don't like them because they are 'unusual' :) I could argue these aren't totally unusual, perhaps unusual in IIO. I fell in love with this type of functions when Guenter suggested this approach for me in the wdg. Well, IIO is your territory so I'll mutilate this file accordingly. > I'd argue they just ensure a sequence of writes go in as an atomic thing. > Two of those writes happen to be turn it off and turn it on. Well, the data-sheet is very clear what comes to clearing the PC1 bit when the various CNTL register are touched: https://fscdn.rohm.com/kionix/en/datasheet/kx022acr-z-e.pdf (at the beginning of various CNTL register descriptions). So, the on/off thing is not something that just happens - and this is what these functions did try to underline :) > So the data the are protecting is the device internal state data. > >> >> (Another thing is that we do claim the direct mode in write_raw, and >> goto is still handy for releasing it. Scoped guards won't play nicely >> with goto. Yes, we could probably use the __cleanup for direct mode, but >> I still like the lock+off, unlock+on for the reason above) > There is a nice new cleanup that David did to make the direct mode > handling much cleaner. > > if_not_cond_guard(iio_claim_direct_try, indio_dev) > return -EBUSY; Ah. Nice. This is not yet in the iio_testing though. I'll add this 'drop the off+lock, on+unlock -functions change as an individual patch. It'll depend on the if_not_cond_guard() while the rest of the patches should have no dependencies to any "not yet in iio_testing" stuff. I do have the patches ready for sending but I don't have sensors to test this at home. I'll give this a try at the office tomorrow and send it out then. Yours, -- Matti
diff --git a/drivers/iio/accel/kionix-kx022a.c b/drivers/iio/accel/kionix-kx022a.c index b6664299e0d5..98953178a580 100644 --- a/drivers/iio/accel/kionix-kx022a.c +++ b/drivers/iio/accel/kionix-kx022a.c @@ -5,6 +5,7 @@ * ROHM/KIONIX accelerometer driver */ +#include <linux/cleanup.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/interrupt.h> @@ -448,7 +449,7 @@ static void kx022a_reg2scale(unsigned int val, unsigned int *val1, *val2 = kx022a_scale_table[val][1]; } -static int kx022a_turn_on_off_unlocked(struct kx022a_data *data, bool on) +static int __kx022a_turn_on_off(struct kx022a_data *data, bool on) { int ret; @@ -469,7 +470,7 @@ static int kx022a_turn_off_lock(struct kx022a_data *data) int ret; mutex_lock(&data->mutex); - ret = kx022a_turn_on_off_unlocked(data, false); + ret = __kx022a_turn_on_off(data, false); if (ret) mutex_unlock(&data->mutex); @@ -480,7 +481,7 @@ static int kx022a_turn_on_unlock(struct kx022a_data *data) { int ret; - ret = kx022a_turn_on_off_unlocked(data, true); + ret = __kx022a_turn_on_off(data, true); mutex_unlock(&data->mutex); return ret; @@ -912,18 +913,19 @@ static int kx022a_fifo_disable(struct kx022a_data *data) { int ret = 0; - ret = kx022a_turn_off_lock(data); + guard(mutex)(&data->mutex); + ret = __kx022a_turn_on_off(data, false); if (ret) return ret; ret = regmap_clear_bits(data->regmap, data->ien_reg, KX022A_MASK_WMI); if (ret) - goto unlock_out; + return ret; ret = regmap_clear_bits(data->regmap, data->chip_info->buf_cntl2, KX022A_MASK_BUF_EN); if (ret) - goto unlock_out; + return ret; data->state &= ~KX022A_STATE_FIFO; @@ -931,12 +933,7 @@ static int kx022a_fifo_disable(struct kx022a_data *data) kfree(data->fifo_buffer); - return kx022a_turn_on_unlock(data); - -unlock_out: - mutex_unlock(&data->mutex); - - return ret; + return __kx022a_turn_on_off(data, true); } static int kx022a_buffer_predisable(struct iio_dev *idev) @@ -959,33 +956,29 @@ static int kx022a_fifo_enable(struct kx022a_data *data) if (!data->fifo_buffer) return -ENOMEM; - ret = kx022a_turn_off_lock(data); + guard(mutex)(&data->mutex); + ret = __kx022a_turn_on_off(data, false); if (ret) return ret; /* Update watermark to HW */ ret = kx022a_fifo_set_wmi(data); if (ret) - goto unlock_out; + return ret; /* Enable buffer */ ret = regmap_set_bits(data->regmap, data->chip_info->buf_cntl2, KX022A_MASK_BUF_EN); if (ret) - goto unlock_out; + return ret; data->state |= KX022A_STATE_FIFO; ret = regmap_set_bits(data->regmap, data->ien_reg, KX022A_MASK_WMI); if (ret) - goto unlock_out; - - return kx022a_turn_on_unlock(data); - -unlock_out: - mutex_unlock(&data->mutex); + return ret; - return ret; + return __kx022a_turn_on_off(data, true); } static int kx022a_buffer_postenable(struct iio_dev *idev) @@ -1053,7 +1046,7 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) struct kx022a_data *data = iio_priv(idev); irqreturn_t ret = IRQ_NONE; - mutex_lock(&data->mutex); + guard(mutex)(&data->mutex); if (data->trigger_enabled) { iio_trigger_poll_nested(data->trig); @@ -1068,8 +1061,6 @@ static irqreturn_t kx022a_irq_thread_handler(int irq, void *private) ret = IRQ_HANDLED; } - mutex_unlock(&data->mutex); - return ret; } @@ -1079,32 +1070,26 @@ static int kx022a_trigger_set_state(struct iio_trigger *trig, struct kx022a_data *data = iio_trigger_get_drvdata(trig); int ret = 0; - mutex_lock(&data->mutex); + guard(mutex)(&data->mutex); if (data->trigger_enabled == state) - goto unlock_out; + return 0; if (data->state & KX022A_STATE_FIFO) { dev_warn(data->dev, "Can't set trigger when FIFO enabled\n"); - ret = -EBUSY; - goto unlock_out; + return -EBUSY; } - ret = kx022a_turn_on_off_unlocked(data, false); + ret = __kx022a_turn_on_off(data, false); if (ret) - goto unlock_out; + return ret; data->trigger_enabled = state; ret = kx022a_set_drdy_irq(data, state); if (ret) - goto unlock_out; - - ret = kx022a_turn_on_off_unlocked(data, true); - -unlock_out: - mutex_unlock(&data->mutex); + return ret; - return ret; + return __kx022a_turn_on_off(data, true); } static const struct iio_trigger_ops kx022a_trigger_ops = {
A few functions in KX022A need to use mutex for protecting the enabling/disabling of the measurement while configurations are being made. Some of the functions can be slightly simplified by using the __cleanup based scoped mutexes, which allows dropping the goto based unlocking at error path. Simplify error paths using guard(mutex). Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- Revision history: v1 => v2: - patch number changed because a change was added to the series. - rebased on iio/testing to avoid conflicts with queued fixes --- drivers/iio/accel/kionix-kx022a.c | 61 ++++++++++++------------------- 1 file changed, 23 insertions(+), 38 deletions(-)