* [PATCH v2 0/2] iio: light: ltr501: cleanup locking and headers @ 2026-04-21 2:09 rafasales 2026-04-21 2:10 ` [PATCH v2 1/2] iio: light: ltr501: update header inclusions rafasales 2026-04-21 2:10 ` [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks rafasales 0 siblings, 2 replies; 7+ messages in thread From: rafasales @ 2026-04-21 2:09 UTC (permalink / raw) To: andy, dlechner, jic23, nuno.sa; +Cc: Rafael B. Sales, linux-iio From: "Rafael B. Sales" <rafasales@usp.br> This series cleans up the iio/light/ltr501.c driver by sorting header inclusions according to the IWYU principles and simplifying locking using automatic cleanup. Changes in v2: - Added patch to sort header inclusions - Fixed naming scheme for functions in commit message - Reverted original lock boundaries - Simplified returns - Used scoped_guard() in switch cases Rafael B. Sales (2): iio: light: ltr501: update header inclusions iio: light: ltr501: use automatic cleanup of locks drivers/iio/light/ltr501.c | 130 ++++++++++++++++++------------------- 1 file changed, 65 insertions(+), 65 deletions(-) -- 2.53.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] iio: light: ltr501: update header inclusions 2026-04-21 2:09 [PATCH v2 0/2] iio: light: ltr501: cleanup locking and headers rafasales @ 2026-04-21 2:10 ` rafasales 2026-04-21 10:19 ` Jonathan Cameron 2026-04-21 2:10 ` [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks rafasales 1 sibling, 1 reply; 7+ messages in thread From: rafasales @ 2026-04-21 2:10 UTC (permalink / raw) To: andy, dlechner, jic23, nuno.sa Cc: Rafael B. Sales, Gustavo C. Arakaki, linux-iio From: "Rafael B. Sales" <rafasales@usp.br> Update header inclusions to follow IWYU (Include What You Use) principle Signed-off-by: Rafael B. Sales <rafasales@usp.br> Co-developed-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> Signed-off-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> --- drivers/iio/light/ltr501.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c index 4d99ae336f61..75a49fd9bce0 100644 --- a/drivers/iio/light/ltr501.c +++ b/drivers/iio/light/ltr501.c @@ -9,20 +9,38 @@ * TODO: IR LED characteristics */ -#include <linux/module.h> -#include <linux/mod_devicetable.h> -#include <linux/i2c.h> -#include <linux/err.h> +#include <asm/page.h> +#include <linux/array_size.h> +#include <linux/bitops.h> +#include <linux/bits.h> +#include <linux/byteorder/generic.h> +#include <linux/cleanup.h> #include <linux/delay.h> -#include <linux/regmap.h> -#include <linux/regulator/consumer.h> - -#include <linux/iio/iio.h> +#include <linux/dev_printk.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/i2c.h> +#include <linux/iio/buffer.h> #include <linux/iio/events.h> +#include <linux/iio/iio.h> #include <linux/iio/sysfs.h> #include <linux/iio/trigger_consumer.h> -#include <linux/iio/buffer.h> #include <linux/iio/triggered_buffer.h> +#include <linux/iio/types.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> +#include <linux/math.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/pm.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/sprintf.h> +#include <linux/sysfs.h> +#include <linux/types.h> #define LTR501_ALS_CONTR 0x80 /* ALS operation mode, SW reset */ #define LTR501_PS_CONTR 0x81 /* PS operation mode */ -- 2.53.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] iio: light: ltr501: update header inclusions 2026-04-21 2:10 ` [PATCH v2 1/2] iio: light: ltr501: update header inclusions rafasales @ 2026-04-21 10:19 ` Jonathan Cameron 2026-04-21 12:49 ` Andy Shevchenko 0 siblings, 1 reply; 7+ messages in thread From: Jonathan Cameron @ 2026-04-21 10:19 UTC (permalink / raw) To: rafasales; +Cc: andy, dlechner, nuno.sa, Gustavo C. Arakaki, linux-iio On Mon, 20 Apr 2026 23:10:00 -0300 rafasales@usp.br wrote: > From: "Rafael B. Sales" <rafasales@usp.br> > > Update header inclusions to follow IWYU (Include What You Use) > principle > > Signed-off-by: Rafael B. Sales <rafasales@usp.br> > Co-developed-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> > Signed-off-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> Hi, One of the issues with the kernel view of IWYU is it's a bit fuzzy (and not well documented yet). That means there are some headers we assume will always include certain others. So if you need the 'outer' header the inner one is not normally included as well. So the comments below are very much my opinion on what should and should not be there. Others may have different views and it is always a little flexible for some of them. > --- > drivers/iio/light/ltr501.c | 36 +++++++++++++++++++++++++++--------- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c > index 4d99ae336f61..75a49fd9bce0 100644 > --- a/drivers/iio/light/ltr501.c > +++ b/drivers/iio/light/ltr501.c > @@ -9,20 +9,38 @@ > * TODO: IR LED characteristics > */ > > -#include <linux/module.h> > -#include <linux/mod_devicetable.h> > -#include <linux/i2c.h> > -#include <linux/err.h> > +#include <asm/page.h> asm headers if they are needed are always in a separate block after all the linux/ ones However, it is very rare to see asm/page.h in a driver. Why do you need it here? I'm going to guess it is for PAGE_SIZE? That bit of code needs a rework to use sysfs_emit_at() instead. If you have time please could you make that change as well as a precursor to this patch and then we won't need this one. Alternative would be to convert to using read_avail(). That would be even better but is a more complex change. > +#include <linux/array_size.h> > +#include <linux/bitops.h> I think it is always safe to assume bitops.h will include bits.h > +#include <linux/bits.h> > +#include <linux/byteorder/generic.h> > +#include <linux/cleanup.h> Is this used at this point? I think this might want to be in the following patch. > #include <linux/delay.h> > -#include <linux/regmap.h> > -#include <linux/regulator/consumer.h> > - > -#include <linux/iio/iio.h> > +#include <linux/dev_printk.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/errno.h> It's a bit indirect but I think we can always assume err.h will include errno.h > +#include <linux/i2c.h> > +#include <linux/iio/buffer.h> > #include <linux/iio/events.h> > +#include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > #include <linux/iio/trigger_consumer.h> > -#include <linux/iio/buffer.h> > #include <linux/iio/triggered_buffer.h> > +#include <linux/iio/types.h> Common practice (as this driver was previously doing) is to keep the iio headers in a separate block at the end. I don't tend to enforce this but given the driver was already doing so, I think keeping it that way makes sense. > +#include <linux/interrupt.h> > +#include <linux/irqreturn.h> interrupt.h will always include irqreturn.h so don't need irqreturn.h > +#include <linux/math.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/pm.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/sprintf.h> > +#include <linux/sysfs.h> > +#include <linux/types.h> > > #define LTR501_ALS_CONTR 0x80 /* ALS operation mode, SW reset */ > #define LTR501_PS_CONTR 0x81 /* PS operation mode */ ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] iio: light: ltr501: update header inclusions 2026-04-21 10:19 ` Jonathan Cameron @ 2026-04-21 12:49 ` Andy Shevchenko 2026-04-21 12:52 ` Andy Shevchenko 0 siblings, 1 reply; 7+ messages in thread From: Andy Shevchenko @ 2026-04-21 12:49 UTC (permalink / raw) To: Jonathan Cameron Cc: rafasales, andy, dlechner, nuno.sa, Gustavo C. Arakaki, linux-iio On Tue, Apr 21, 2026 at 1:19 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 20 Apr 2026 23:10:00 -0300 > rafasales@usp.br wrote: > > Update header inclusions to follow IWYU (Include What You Use) > > principle > One of the issues with the kernel view of IWYU is it's a bit fuzzy > (and not well documented yet). > That means there are some headers we assume will always include > certain others. So if you need the 'outer' header the inner one > is not normally included as well. > > So the comments below are very much my opinion on what should > and should not be there. Others may have different views and > it is always a little flexible for some of them. It seems we are on the same page here. Just a couple (I hope insightful for you) comments below. ... > > +#include <asm/page.h> > asm headers if they are needed are always in a separate block > after all the linux/ ones > > However, it is very rare to see asm/page.h in a driver. Why do > you need it here? Yeah, the most we should see in the driver(s) as of today are div64.h and/or byteorder.h. And the former is still rare as usually we want math.h. > I'm going to guess it is for PAGE_SIZE? > > That bit of code needs a rework to use sysfs_emit_at() > instead. If you have time please could you make that change as well > as a precursor to this patch and then we won't need this one. +1. > Alternative would be to convert to using read_avail(). > That would be even better but is a more complex change. Even more — +2! ... > > +#include <linux/bitops.h> > I think it is always safe to assume bitops.h will include bits.h > > > +#include <linux/bits.h> Yes. The chain is bits.h <— bitops.h <— bitmap.h If the header more to the right is included the left ones are guaranteed to be provided (or rather the APIs they provide are guaranteed to be in the rightmost). ... > > +#include <linux/byteorder/generic.h> Note, this has not to be included. Yeah, a bit of a voodoo, but asm/byteorder.h is the correct one. ... > > +#include <linux/err.h> > > +#include <linux/errno.h> > > It's a bit indirect but I think we can always assume err.h will include > errno.h Yes and no. err.h includes asm/errno.h, but linux/err.h. Strictly speaking the use of -EPROBE_DEFER along with IS_ERR(), for example, need to include both. But I personally don't go that far (I do not require people for this particular case) and do not prevent them from doing such (when there is the case as per above example). ... > > +#include <linux/interrupt.h> > > +#include <linux/irqreturn.h> > > interrupt.h will always include irqreturn.h so don't need > irqreturn.h Agree on this implied dependency. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] iio: light: ltr501: update header inclusions 2026-04-21 12:49 ` Andy Shevchenko @ 2026-04-21 12:52 ` Andy Shevchenko 0 siblings, 0 replies; 7+ messages in thread From: Andy Shevchenko @ 2026-04-21 12:52 UTC (permalink / raw) To: Jonathan Cameron Cc: rafasales, andy, dlechner, nuno.sa, Gustavo C. Arakaki, linux-iio On Tue, Apr 21, 2026 at 3:49 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Apr 21, 2026 at 1:19 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 20 Apr 2026 23:10:00 -0300 > > rafasales@usp.br wrote: ... > > > +#include <linux/err.h> > > > +#include <linux/errno.h> > > > > It's a bit indirect but I think we can always assume err.h will include > > errno.h > > Yes and no. err.h includes asm/errno.h, but linux/err.h. Strictly I meant "but linux/errno.h" (that includes Linux specific codes >= 512. > speaking the use of -EPROBE_DEFER along with IS_ERR(), for example, > need to include both. But I personally don't go that far (I do not > require people for this particular case) and do not prevent them from > doing such (when there is the case as per above example). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks 2026-04-21 2:09 [PATCH v2 0/2] iio: light: ltr501: cleanup locking and headers rafasales 2026-04-21 2:10 ` [PATCH v2 1/2] iio: light: ltr501: update header inclusions rafasales @ 2026-04-21 2:10 ` rafasales 2026-04-21 10:29 ` Jonathan Cameron 1 sibling, 1 reply; 7+ messages in thread From: rafasales @ 2026-04-21 2:10 UTC (permalink / raw) To: andy, dlechner, jic23, nuno.sa Cc: Rafael B. Sales, Gustavo C. Arakaki, linux-iio From: "Rafael B. Sales" <rafasales@usp.br> Replace `mutex_lock()` and `mutex_unlock()` calls with guards to reduce boilerplate and allow for simpler code blocks. Signed-off-by: Rafael B. Sales <rafasales@usp.br> Co-developed-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> Signed-off-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> --- drivers/iio/light/ltr501.c | 94 +++++++++++++++----------------------- 1 file changed, 38 insertions(+), 56 deletions(-) diff --git a/drivers/iio/light/ltr501.c b/drivers/iio/light/ltr501.c index 75a49fd9bce0..d84b8f23c5a2 100644 --- a/drivers/iio/light/ltr501.c +++ b/drivers/iio/light/ltr501.c @@ -251,7 +251,7 @@ static int ltr501_ps_read_samp_freq(const struct ltr501_data *data, static int ltr501_als_write_samp_freq(struct ltr501_data *data, int val, int val2) { - int i, ret; + int i; i = ltr501_match_samp_freq(ltr501_als_samp_table, ARRAY_SIZE(ltr501_als_samp_table), @@ -260,17 +260,14 @@ static int ltr501_als_write_samp_freq(struct ltr501_data *data, if (i < 0) return i; - mutex_lock(&data->lock_als); - ret = regmap_field_write(data->reg_als_rate, i); - mutex_unlock(&data->lock_als); - - return ret; + guard(mutex)(&data->lock_als); + return regmap_field_write(data->reg_als_rate, i); } static int ltr501_ps_write_samp_freq(struct ltr501_data *data, int val, int val2) { - int i, ret; + int i; i = ltr501_match_samp_freq(ltr501_ps_samp_table, ARRAY_SIZE(ltr501_ps_samp_table), @@ -279,11 +276,8 @@ static int ltr501_ps_write_samp_freq(struct ltr501_data *data, if (i < 0) return i; - mutex_lock(&data->lock_ps); - ret = regmap_field_write(data->reg_ps_rate, i); - mutex_unlock(&data->lock_ps); - - return ret; + guard(mutex)(&data->lock_ps); + return regmap_field_write(data->reg_ps_rate, i); } static int ltr501_als_read_samp_period(const struct ltr501_data *data, int *val) @@ -505,9 +499,8 @@ static int ltr501_write_intr_prst(struct ltr501_data *data, if (new_val < 0 || new_val > 0x0f) return -EINVAL; - mutex_lock(&data->lock_als); - ret = regmap_field_write(data->reg_als_prst, new_val); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = regmap_field_write(data->reg_als_prst, new_val); if (ret >= 0) data->als_period = period; @@ -525,9 +518,8 @@ static int ltr501_write_intr_prst(struct ltr501_data *data, if (new_val < 0 || new_val > 0x0f) return -EINVAL; - mutex_lock(&data->lock_ps); - ret = regmap_field_write(data->reg_ps_prst, new_val); - mutex_unlock(&data->lock_ps); + scoped_guard(mutex, &data->lock_ps) + ret = regmap_field_write(data->reg_ps_prst, new_val); if (ret >= 0) data->ps_period = period; @@ -669,18 +661,16 @@ static int ltr501_read_info_raw(struct ltr501_data *data, switch (chan->type) { case IIO_INTENSITY: - mutex_lock(&data->lock_als); - ret = ltr501_read_als(data, buf); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = ltr501_read_als(data, buf); if (ret < 0) return ret; *val = le16_to_cpu(chan->address == LTR501_ALS_DATA1 ? buf[0] : buf[1]); return IIO_VAL_INT; case IIO_PROXIMITY: - mutex_lock(&data->lock_ps); - ret = ltr501_read_ps(data); - mutex_unlock(&data->lock_ps); + scoped_guard(mutex, &data->lock_ps) + ret = ltr501_read_ps(data); if (ret < 0) return ret; *val = ret & LTR501_PS_DATA_MASK; @@ -705,9 +695,8 @@ static int ltr501_read_raw(struct iio_dev *indio_dev, if (!iio_device_claim_direct(indio_dev)) return -EBUSY; - mutex_lock(&data->lock_als); - ret = ltr501_read_als(data, buf); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = ltr501_read_als(data, buf); iio_device_release_direct(indio_dev); if (ret < 0) return ret; @@ -820,9 +809,8 @@ static int __ltr501_write_raw(struct iio_dev *indio_dev, if (val != 0) return -EINVAL; - mutex_lock(&data->lock_als); - ret = ltr501_set_it_time(data, val2); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = ltr501_set_it_time(data, val2); return ret; default: return -EINVAL; @@ -971,18 +959,16 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev, return -EINVAL; switch (dir) { case IIO_EV_DIR_RISING: - mutex_lock(&data->lock_als); - ret = regmap_bulk_write(data->regmap, - LTR501_ALS_THRESH_UP, - &val, 2); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = regmap_bulk_write(data->regmap, + LTR501_ALS_THRESH_UP, + &val, 2); return ret; case IIO_EV_DIR_FALLING: - mutex_lock(&data->lock_als); - ret = regmap_bulk_write(data->regmap, - LTR501_ALS_THRESH_LOW, - &val, 2); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = regmap_bulk_write(data->regmap, + LTR501_ALS_THRESH_LOW, + &val, 2); return ret; default: return -EINVAL; @@ -992,18 +978,16 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev, return -EINVAL; switch (dir) { case IIO_EV_DIR_RISING: - mutex_lock(&data->lock_ps); - ret = regmap_bulk_write(data->regmap, - LTR501_PS_THRESH_UP, - &val, 2); - mutex_unlock(&data->lock_ps); + scoped_guard(mutex, &data->lock_ps) + ret = regmap_bulk_write(data->regmap, + LTR501_PS_THRESH_UP, + &val, 2); return ret; case IIO_EV_DIR_FALLING: - mutex_lock(&data->lock_ps); - ret = regmap_bulk_write(data->regmap, - LTR501_PS_THRESH_LOW, - &val, 2); - mutex_unlock(&data->lock_ps); + scoped_guard(mutex, &data->lock_ps) + ret = regmap_bulk_write(data->regmap, + LTR501_PS_THRESH_LOW, + &val, 2); return ret; default: return -EINVAL; @@ -1100,14 +1084,12 @@ static int ltr501_write_event_config(struct iio_dev *indio_dev, switch (chan->type) { case IIO_INTENSITY: - mutex_lock(&data->lock_als); - ret = regmap_field_write(data->reg_als_intr, state); - mutex_unlock(&data->lock_als); + scoped_guard(mutex, &data->lock_als) + ret = regmap_field_write(data->reg_als_intr, state); return ret; case IIO_PROXIMITY: - mutex_lock(&data->lock_ps); - ret = regmap_field_write(data->reg_ps_intr, state); - mutex_unlock(&data->lock_ps); + scoped_guard(mutex, &data->lock_ps) + ret = regmap_field_write(data->reg_ps_intr, state); return ret; default: return -EINVAL; -- 2.53.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks 2026-04-21 2:10 ` [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks rafasales @ 2026-04-21 10:29 ` Jonathan Cameron 0 siblings, 0 replies; 7+ messages in thread From: Jonathan Cameron @ 2026-04-21 10:29 UTC (permalink / raw) To: rafasales; +Cc: andy, dlechner, nuno.sa, Gustavo C. Arakaki, linux-iio On Mon, 20 Apr 2026 23:10:01 -0300 rafasales@usp.br wrote: > From: "Rafael B. Sales" <rafasales@usp.br> > > Replace `mutex_lock()` and `mutex_unlock()` calls with guards > to reduce boilerplate and allow for simpler code blocks. > > Signed-off-by: Rafael B. Sales <rafasales@usp.br> > Co-developed-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> > Signed-off-by: Gustavo C. Arakaki <gustavo.arakaki@usp.br> I think a few of these can be done in a cleaner fashion - see below. Jonathan > > static int ltr501_als_read_samp_period(const struct ltr501_data *data, int *val) > @@ -505,9 +499,8 @@ static int ltr501_write_intr_prst(struct ltr501_data *data, > if (new_val < 0 || new_val > 0x0f) > return -EINVAL; > > - mutex_lock(&data->lock_als); > - ret = regmap_field_write(data->reg_als_prst, new_val); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = regmap_field_write(data->reg_als_prst, new_val); > if (ret >= 0) > data->als_period = period; > > @@ -525,9 +518,8 @@ static int ltr501_write_intr_prst(struct ltr501_data *data, > if (new_val < 0 || new_val > 0x0f) > return -EINVAL; > > - mutex_lock(&data->lock_ps); > - ret = regmap_field_write(data->reg_ps_prst, new_val); > - mutex_unlock(&data->lock_ps); > + scoped_guard(mutex, &data->lock_ps) > + ret = regmap_field_write(data->reg_ps_prst, new_val); > if (ret >= 0) > data->ps_period = period; This and the one above are more complex as we'd not want to imply protection of ps_period by including it in the lock if that doesn't make sense. > > @@ -669,18 +661,16 @@ static int ltr501_read_info_raw(struct ltr501_data *data, > > switch (chan->type) { > case IIO_INTENSITY: > - mutex_lock(&data->lock_als); > - ret = ltr501_read_als(data, buf); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = ltr501_read_als(data, buf); > if (ret < 0) > return ret; > *val = le16_to_cpu(chan->address == LTR501_ALS_DATA1 ? > buf[0] : buf[1]); This is cheap and local stuff only. For these two cases i'd use {} + guard() as below and not worry about doing this under the lock. > return IIO_VAL_INT; > case IIO_PROXIMITY: > - mutex_lock(&data->lock_ps); > - ret = ltr501_read_ps(data); > - mutex_unlock(&data->lock_ps); > + scoped_guard(mutex, &data->lock_ps) > + ret = ltr501_read_ps(data); > if (ret < 0) > return ret; > *val = ret & LTR501_PS_DATA_MASK; > @@ -705,9 +695,8 @@ static int ltr501_read_raw(struct iio_dev *indio_dev, > if (!iio_device_claim_direct(indio_dev)) > return -EBUSY; > > - mutex_lock(&data->lock_als); > - ret = ltr501_read_als(data, buf); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = ltr501_read_als(data, buf); This use of scoped_guard() does make sense as there is non trivial work to do afterwards. > iio_device_release_direct(indio_dev); > if (ret < 0) > return ret; > @@ -820,9 +809,8 @@ static int __ltr501_write_raw(struct iio_dev *indio_dev, > if (val != 0) > return -EINVAL; I'd use a guard here (see below) for same reasons. It'll just be after this initial sanity check. > > - mutex_lock(&data->lock_als); > - ret = ltr501_set_it_time(data, val2); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = ltr501_set_it_time(data, val2); > return ret; > default: > return -EINVAL; > @@ -971,18 +959,16 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev, > return -EINVAL; > switch (dir) { > case IIO_EV_DIR_RISING: > - mutex_lock(&data->lock_als); > - ret = regmap_bulk_write(data->regmap, > - LTR501_ALS_THRESH_UP, > - &val, 2); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = regmap_bulk_write(data->regmap, > + LTR501_ALS_THRESH_UP, > + &val, 2); See below. > return ret; > case IIO_EV_DIR_FALLING: > - mutex_lock(&data->lock_als); > - ret = regmap_bulk_write(data->regmap, > - LTR501_ALS_THRESH_LOW, > - &val, 2); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = regmap_bulk_write(data->regmap, > + LTR501_ALS_THRESH_LOW, > + &val, 2); See below. > return ret; > default: > return -EINVAL; > @@ -992,18 +978,16 @@ static int ltr501_write_thresh(struct iio_dev *indio_dev, > return -EINVAL; > switch (dir) { > case IIO_EV_DIR_RISING: > - mutex_lock(&data->lock_ps); > - ret = regmap_bulk_write(data->regmap, > - LTR501_PS_THRESH_UP, > - &val, 2); > - mutex_unlock(&data->lock_ps); > + scoped_guard(mutex, &data->lock_ps) > + ret = regmap_bulk_write(data->regmap, > + LTR501_PS_THRESH_UP, > + &val, 2); See below. > return ret; > case IIO_EV_DIR_FALLING: > - mutex_lock(&data->lock_ps); > - ret = regmap_bulk_write(data->regmap, > - LTR501_PS_THRESH_LOW, > - &val, 2); > - mutex_unlock(&data->lock_ps); > + scoped_guard(mutex, &data->lock_ps) > + ret = regmap_bulk_write(data->regmap, > + LTR501_PS_THRESH_LOW, > + &val, 2); See below. > return ret; > default: > return -EINVAL; > @@ -1100,14 +1084,12 @@ static int ltr501_write_event_config(struct iio_dev *indio_dev, > > switch (chan->type) { > case IIO_INTENSITY: > - mutex_lock(&data->lock_als); > - ret = regmap_field_write(data->reg_als_intr, state); > - mutex_unlock(&data->lock_als); > + scoped_guard(mutex, &data->lock_als) > + ret = regmap_field_write(data->reg_als_intr, state); > return ret; for these I'd rather see the pattern case IIO_INTENSITY: { guard(mutex)(&data->lock_als); return regmap_field_write(); } The scoped_guard() has some annoying issues wrt to compilers and their ability to see that the loop (it's a for loop underneath) is always entered and so you can't move the return in there. The bare guard() form doesn't have that problem. > case IIO_PROXIMITY: > - mutex_lock(&data->lock_ps); > - ret = regmap_field_write(data->reg_ps_intr, state); > - mutex_unlock(&data->lock_ps); > + scoped_guard(mutex, &data->lock_ps) > + ret = regmap_field_write(data->reg_ps_intr, state); As above. > return ret; > default: > return -EINVAL; ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2026-04-21 12:53 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-21 2:09 [PATCH v2 0/2] iio: light: ltr501: cleanup locking and headers rafasales 2026-04-21 2:10 ` [PATCH v2 1/2] iio: light: ltr501: update header inclusions rafasales 2026-04-21 10:19 ` Jonathan Cameron 2026-04-21 12:49 ` Andy Shevchenko 2026-04-21 12:52 ` Andy Shevchenko 2026-04-21 2:10 ` [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks rafasales 2026-04-21 10:29 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox