[PATCH v2 0/2] iio: light: ltr501: cleanup locking and headers

[PATCH v2 0/2] iio: light: ltr501: cleanup locking and headers

[rafasales]

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

[PATCH v2 1/2] iio: light: ltr501: update header inclusions

[rafasales]

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

[PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks

[rafasales]

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

Re: [PATCH v2 1/2] iio: light: ltr501: update header inclusions

[Jonathan Cameron]

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 */

Re: [PATCH v2 2/2] iio: light: ltr501: use automatic cleanup of locks

[Jonathan Cameron]

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;

Re: [PATCH v2 1/2] iio: light: ltr501: update header inclusions

[Andy Shevchenko]

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

Re: [PATCH v2 1/2] iio: light: ltr501: update header inclusions

[Andy Shevchenko]

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