[v1] iio: light: ltr390: Add debugfs register access support

[PATCH] iio: light: ltr390: Add debugfs register access support

Posted by Akshay Jindal 6 months, 2 weeks ago

Add support for debugfs_reg_access through the driver's iio_info structure
to enable low-level register read/write access for debugging.

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---
Testing details:
================
-> Tested on Raspberrypi 4B. Follow for more details.

akshayajpi@raspberrypi:~ $ uname -r
6.12.35-v8+
akshayajpi@raspberrypi:~ $ uname -a
Linux raspberrypi 6.12.35-v8+ #5 SMP PREEMPT Tue Jul 15 17:38:06 IST 2025 aarch64 GNU/Linux

-> Sensor Detection, overlaying of device tree and Driver loading
akshayajpi@raspberrypi:~ $ i2cdetect -y 1
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                         -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- 53 -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

akshayajpi@raspberrypi:~ $ sudo dtoverlay i2c-sensor ltr390
akshayajpi@raspberrypi:~ $ lsmod|grep ltr390
ltr390                 16384  0
industrialio          110592  1 ltr390
regmap_i2c             12288  1 ltr390


1. Disable sensor via debugfs, verify from i2cget and debugfs.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x0 | sudo tee direct_reg_access
0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x2
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x0 0x0 | sudo tee direct_reg_access
0x0 0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ i2cget -f -y 1 0x53 0x0
0x00

2. Disable sensor via debugfs and read data status via debugfs.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
715
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
715
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
715
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ i2cget -f -y 1 0x53 0x7
0x28
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ i2cget -f -y 1 0x53 0x7
0x00
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x7 | sudo tee direct_reg_access
0x7
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x0

3. Re-enable sensor via debugfs and read data status via debugfs.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x0 0x2 | sudo tee direct_reg_access
0x0 0x2
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x2
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
715
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
718
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
727
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x7 | sudo tee direct_reg_access
0x7
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x8

4. Enable interrupts via sysfs and verify via debugfs.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 1 | sudo tee /sys/bus/iio/devices/iio\:device0/events/in_illuminance_thresh_either_en
1
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ i2cget -f -y 1 0x53 0x19
0x14
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x19 | sudo tee direct_reg_access 
0x19
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x14

5. Write falling threshold via debugfs, verify the threshold written via sysfs.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x24 0x32 | sudo tee direct_reg_access 
0x24 0x32
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x32
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x25 0x0 | sudo tee direct_reg_access 
0x25 0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x26 0x0 | sudo tee direct_reg_access 
0x26 0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
0x0
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/events/in_illuminance_thresh_falling_value 
50
final value = 0x0 << 16 | 0x0 << 8 | 0x32 = 50

6. Block light and verify interrupts getting generated.
-> Before blocking light
cat /proc/interrupts|grep ltr390
 58:         0          0          0          0  pinctrl-bcm2835   4 Edge      ltr390_thresh_event

->After blocking light
58:         92          0          0          0  pinctrl-bcm2835   4 Edge      ltr390_thresh_event

7. write value to a non-writeable reg via debugfs.
-> LTR390_ALS_DATA_0|1|2 are non-writeable registers. Writing to them gives I/O error as expected.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0xd 0x1 | sudo tee direct_reg_access
0xd 0x1
tee: direct_reg_access: Input/output error
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0xe 0x1 | sudo tee direct_reg_access
0xe 0x1
tee: direct_reg_access: Input/output error
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0xf 0x1 | sudo tee direct_reg_access
0xf 0x1
tee: direct_reg_access: Input/output error

8. read value from a non-readable reg via debugfs.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x2 |sudo tee direct_reg_access 
0x2
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access
cat: direct_reg_access: Input/output error


9. do simple raw reads from debugfs.
-> reading raw value via sysfs:
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
705
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
695
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat /sys/bus/iio/devices/iio\:device0/in_illuminance_raw
711

-> reading via debugfs (should be in the same ballpark of sysfs)
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0xd | sudo tee direct_reg_access
0xd
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access 
0xC7
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0xe | sudo tee direct_reg_access
0xe
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access 
0x2
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0xf | sudo tee direct_reg_access
0xf
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access 
0x0
final value = 0x0 << 16 | 0x2 << 8 | 0xc7 = 711

10. Testing reads on registers beyond max_register.
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x27 | sudo tee direct_reg_access 
0x27
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access 
cat: direct_reg_access: Input/output error
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ echo 0x28 | sudo tee direct_reg_access 
0x28
akshayajpi@raspberrypi:/sys/kernel/debug/iio/iio:device0 $ cat direct_reg_access 
cat: direct_reg_access: Input/output error

 drivers/iio/light/ltr390.c | 99 ++++++++++++++++++++++++++++++++++----
 1 file changed, 89 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index ee59bbb8aa09..1f6ee0fd6d19 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -38,12 +38,20 @@
 #define LTR390_ALS_UVS_GAIN		0x05
 #define LTR390_PART_ID			0x06
 #define LTR390_MAIN_STATUS		0x07
-#define LTR390_ALS_DATA			0x0D
-#define LTR390_UVS_DATA			0x10
+#define LTR390_ALS_DATA_0		0x0D
+#define LTR390_ALS_DATA_1		0x0E
+#define LTR390_ALS_DATA_2		0x0F
+#define LTR390_UVS_DATA_0		0x10
+#define LTR390_UVS_DATA_1		0x11
+#define LTR390_UVS_DATA_2		0x12
 #define LTR390_INT_CFG			0x19
 #define LTR390_INT_PST			0x1A
-#define LTR390_THRESH_UP		0x21
-#define LTR390_THRESH_LOW		0x24
+#define LTR390_THRESH_UP_0		0x21
+#define LTR390_THRESH_UP_1		0x22
+#define LTR390_THRESH_UP_2		0x23
+#define LTR390_THRESH_LOW_0		0x24
+#define LTR390_THRESH_LOW_1		0x25
+#define LTR390_THRESH_LOW_2		0x26
 
 #define LTR390_PART_NUMBER_ID		0xb
 #define LTR390_ALS_UVS_GAIN_MASK	GENMASK(2, 0)
@@ -98,11 +106,62 @@ struct ltr390_data {
 	int int_time_us;
 };
 
+static bool ltr390_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTR390_MAIN_CTRL:
+	case LTR390_ALS_UVS_MEAS_RATE:
+	case LTR390_ALS_UVS_GAIN:
+	case LTR390_PART_ID:
+	case LTR390_MAIN_STATUS:
+	case LTR390_ALS_DATA_0:
+	case LTR390_ALS_DATA_1:
+	case LTR390_ALS_DATA_2:
+	case LTR390_UVS_DATA_0:
+	case LTR390_UVS_DATA_1:
+	case LTR390_UVS_DATA_2:
+	case LTR390_INT_CFG:
+	case LTR390_INT_PST:
+	case LTR390_THRESH_UP_0:
+	case LTR390_THRESH_UP_1:
+	case LTR390_THRESH_UP_2:
+	case LTR390_THRESH_LOW_0:
+	case LTR390_THRESH_LOW_1:
+	case LTR390_THRESH_LOW_2:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltr390_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTR390_MAIN_CTRL:
+	case LTR390_ALS_UVS_MEAS_RATE:
+	case LTR390_ALS_UVS_GAIN:
+	case LTR390_INT_CFG:
+	case LTR390_INT_PST:
+	case LTR390_THRESH_UP_0:
+	case LTR390_THRESH_UP_1:
+	case LTR390_THRESH_UP_2:
+	case LTR390_THRESH_LOW_0:
+	case LTR390_THRESH_LOW_1:
+	case LTR390_THRESH_LOW_2:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static const struct regmap_config ltr390_regmap_config = {
 	.name = "ltr390",
 	.reg_bits = 8,
 	.reg_stride = 1,
 	.val_bits = 8,
+	.max_register = LTR390_THRESH_LOW_2,
+	.readable_reg = ltr390_is_readable_reg,
+	.writeable_reg = ltr390_is_writeable_reg,
 };
 
 /* Sampling frequency is in mili Hz and mili Seconds */
@@ -194,7 +253,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 			if (ret < 0)
 				return ret;
 
-			ret = ltr390_register_read(data, LTR390_UVS_DATA);
+			ret = ltr390_register_read(data, LTR390_UVS_DATA_0);
 			if (ret < 0)
 				return ret;
 			break;
@@ -204,7 +263,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
 			if (ret < 0)
 				return ret;
 
-			ret = ltr390_register_read(data, LTR390_ALS_DATA);
+			ret = ltr390_register_read(data, LTR390_ALS_DATA_0);
 			if (ret < 0)
 				return ret;
 			break;
@@ -454,14 +513,14 @@ static int ltr390_read_threshold(struct iio_dev *indio_dev,
 
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
-		ret = ltr390_register_read(data, LTR390_THRESH_UP);
+		ret = ltr390_register_read(data, LTR390_THRESH_UP_0);
 		if (ret < 0)
 			return ret;
 		*val = ret;
 		return IIO_VAL_INT;
 
 	case IIO_EV_DIR_FALLING:
-		ret = ltr390_register_read(data, LTR390_THRESH_LOW);
+		ret = ltr390_register_read(data, LTR390_THRESH_LOW_0);
 		if (ret < 0)
 			return ret;
 		*val = ret;
@@ -480,10 +539,10 @@ static int ltr390_write_threshold(struct iio_dev *indio_dev,
 	guard(mutex)(&data->lock);
 	switch (dir) {
 	case IIO_EV_DIR_RISING:
-		return regmap_bulk_write(data->regmap, LTR390_THRESH_UP, &val, 3);
+		return regmap_bulk_write(data->regmap, LTR390_THRESH_UP_0, &val, 3);
 
 	case IIO_EV_DIR_FALLING:
-		return regmap_bulk_write(data->regmap, LTR390_THRESH_LOW, &val, 3);
+		return regmap_bulk_write(data->regmap, LTR390_THRESH_LOW_0, &val, 3);
 
 	default:
 		return -EINVAL;
@@ -586,6 +645,25 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
 	}
 }
 
+static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
+						unsigned int reg, unsigned int writeval,
+						unsigned int *readval)
+{
+	int ret;
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	guard(mutex)(&data->lock);
+
+	if (!readval)
+		return regmap_write(data->regmap, reg, writeval);
+
+	ret = regmap_read(data->regmap, reg, readval);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static const struct iio_info ltr390_info = {
 	.read_raw = ltr390_read_raw,
 	.write_raw = ltr390_write_raw,
@@ -594,6 +672,7 @@ static const struct iio_info ltr390_info = {
 	.read_event_config = ltr390_read_event_config,
 	.write_event_value = ltr390_write_event_value,
 	.write_event_config = ltr390_write_event_config,
+	.debugfs_reg_access = ltr390_debugfs_reg_access,
 };
 
 static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
-- 
2.43.0

Re: [PATCH] iio: light: ltr390: Add debugfs register access support

Posted by David Lechner 6 months, 2 weeks ago

On 7/23/25 6:46 AM, Akshay Jindal wrote:
> Add support for debugfs_reg_access through the driver's iio_info structure
> to enable low-level register read/write access for debugging.
> 
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
> Testing details:
> ================
> -> Tested on Raspberrypi 4B. Follow for more details.
> 

...

>  drivers/iio/light/ltr390.c | 99 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 89 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> index ee59bbb8aa09..1f6ee0fd6d19 100644
> --- a/drivers/iio/light/ltr390.c
> +++ b/drivers/iio/light/ltr390.c
> @@ -38,12 +38,20 @@
>  #define LTR390_ALS_UVS_GAIN		0x05
>  #define LTR390_PART_ID			0x06
>  #define LTR390_MAIN_STATUS		0x07
> -#define LTR390_ALS_DATA			0x0D
> -#define LTR390_UVS_DATA			0x10
> +#define LTR390_ALS_DATA_0		0x0D
> +#define LTR390_ALS_DATA_1		0x0E
> +#define LTR390_ALS_DATA_2		0x0F
> +#define LTR390_UVS_DATA_0		0x10
> +#define LTR390_UVS_DATA_1		0x11
> +#define LTR390_UVS_DATA_2		0x12
>  #define LTR390_INT_CFG			0x19
>  #define LTR390_INT_PST			0x1A
> -#define LTR390_THRESH_UP		0x21
> -#define LTR390_THRESH_LOW		0x24
> +#define LTR390_THRESH_UP_0		0x21
> +#define LTR390_THRESH_UP_1		0x22
> +#define LTR390_THRESH_UP_2		0x23
> +#define LTR390_THRESH_LOW_0		0x24
> +#define LTR390_THRESH_LOW_1		0x25
> +#define LTR390_THRESH_LOW_2		0x26
>  
>  #define LTR390_PART_NUMBER_ID		0xb
>  #define LTR390_ALS_UVS_GAIN_MASK	GENMASK(2, 0)
> @@ -98,11 +106,62 @@ struct ltr390_data {
>  	int int_time_us;
>  };
>  
> +static bool ltr390_is_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LTR390_MAIN_CTRL:
> +	case LTR390_ALS_UVS_MEAS_RATE:
> +	case LTR390_ALS_UVS_GAIN:
> +	case LTR390_PART_ID:
> +	case LTR390_MAIN_STATUS:
> +	case LTR390_ALS_DATA_0:
> +	case LTR390_ALS_DATA_1:
> +	case LTR390_ALS_DATA_2:
> +	case LTR390_UVS_DATA_0:
> +	case LTR390_UVS_DATA_1:
> +	case LTR390_UVS_DATA_2:
> +	case LTR390_INT_CFG:
> +	case LTR390_INT_PST:
> +	case LTR390_THRESH_UP_0:
> +	case LTR390_THRESH_UP_1:
> +	case LTR390_THRESH_UP_2:
> +	case LTR390_THRESH_LOW_0:
> +	case LTR390_THRESH_LOW_1:
> +	case LTR390_THRESH_LOW_2:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool ltr390_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case LTR390_MAIN_CTRL:
> +	case LTR390_ALS_UVS_MEAS_RATE:
> +	case LTR390_ALS_UVS_GAIN:
> +	case LTR390_INT_CFG:
> +	case LTR390_INT_PST:
> +	case LTR390_THRESH_UP_0:
> +	case LTR390_THRESH_UP_1:
> +	case LTR390_THRESH_UP_2:
> +	case LTR390_THRESH_LOW_0:
> +	case LTR390_THRESH_LOW_1:
> +	case LTR390_THRESH_LOW_2:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}

Using rd_table and wr_table is a bit more compact than readable_reg
writeable_reg.

> +
>  static const struct regmap_config ltr390_regmap_config = {
>  	.name = "ltr390",
>  	.reg_bits = 8,
>  	.reg_stride = 1,
>  	.val_bits = 8,
> +	.max_register = LTR390_THRESH_LOW_2,
> +	.readable_reg = ltr390_is_readable_reg,
> +	.writeable_reg = ltr390_is_writeable_reg,
>  };
>  
>  /* Sampling frequency is in mili Hz and mili Seconds */
> @@ -194,7 +253,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  			if (ret < 0)
>  				return ret;
>  
> -			ret = ltr390_register_read(data, LTR390_UVS_DATA);
> +			ret = ltr390_register_read(data, LTR390_UVS_DATA_0);
>  			if (ret < 0)
>  				return ret;
>  			break;
> @@ -204,7 +263,7 @@ static int ltr390_read_raw(struct iio_dev *iio_device,
>  			if (ret < 0)
>  				return ret;
>  
> -			ret = ltr390_register_read(data, LTR390_ALS_DATA);
> +			ret = ltr390_register_read(data, LTR390_ALS_DATA_0);
>  			if (ret < 0)
>  				return ret;
>  			break;
> @@ -454,14 +513,14 @@ static int ltr390_read_threshold(struct iio_dev *indio_dev,
>  
>  	switch (dir) {
>  	case IIO_EV_DIR_RISING:
> -		ret = ltr390_register_read(data, LTR390_THRESH_UP);
> +		ret = ltr390_register_read(data, LTR390_THRESH_UP_0);
>  		if (ret < 0)
>  			return ret;
>  		*val = ret;
>  		return IIO_VAL_INT;
>  
>  	case IIO_EV_DIR_FALLING:
> -		ret = ltr390_register_read(data, LTR390_THRESH_LOW);
> +		ret = ltr390_register_read(data, LTR390_THRESH_LOW_0);
>  		if (ret < 0)
>  			return ret;
>  		*val = ret;
> @@ -480,10 +539,10 @@ static int ltr390_write_threshold(struct iio_dev *indio_dev,
>  	guard(mutex)(&data->lock);
>  	switch (dir) {
>  	case IIO_EV_DIR_RISING:
> -		return regmap_bulk_write(data->regmap, LTR390_THRESH_UP, &val, 3);
> +		return regmap_bulk_write(data->regmap, LTR390_THRESH_UP_0, &val, 3);
>  
>  	case IIO_EV_DIR_FALLING:
> -		return regmap_bulk_write(data->regmap, LTR390_THRESH_LOW, &val, 3);
> +		return regmap_bulk_write(data->regmap, LTR390_THRESH_LOW_0, &val, 3);
>  
>  	default:
>  		return -EINVAL;
> @@ -586,6 +645,25 @@ static int ltr390_write_event_config(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int ltr390_debugfs_reg_access(struct iio_dev *indio_dev,
> +						unsigned int reg, unsigned int writeval,
> +						unsigned int *readval)
> +{
> +	int ret;
> +	struct ltr390_data *data = iio_priv(indio_dev);
> +
> +	guard(mutex)(&data->lock);
> +
> +	if (!readval)
> +		return regmap_write(data->regmap, reg, writeval);

nit: reverse the if logic to avoid !

> +
> +	ret = regmap_read(data->regmap, reg, readval);

Just return directly here as well.

> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static const struct iio_info ltr390_info = {
>  	.read_raw = ltr390_read_raw,
>  	.write_raw = ltr390_write_raw,
> @@ -594,6 +672,7 @@ static const struct iio_info ltr390_info = {
>  	.read_event_config = ltr390_read_event_config,
>  	.write_event_value = ltr390_write_event_value,
>  	.write_event_config = ltr390_write_event_config,
> +	.debugfs_reg_access = ltr390_debugfs_reg_access,
>  };
>  
>  static irqreturn_t ltr390_interrupt_handler(int irq, void *private)

Re: [PATCH] iio: light: ltr390: Add debugfs register access support

Posted by Jonathan Cameron 6 months, 2 weeks ago

On Wed, 23 Jul 2025 09:13:09 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 7/23/25 6:46 AM, Akshay Jindal wrote:
> > Add support for debugfs_reg_access through the driver's iio_info structure
> > to enable low-level register read/write access for debugging.
> > 
> > Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> > ---
> > Testing details:
> > ================  
> > -> Tested on Raspberrypi 4B. Follow for more details.  
> >   
> 
> ...
> 
> >  drivers/iio/light/ltr390.c | 99 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 89 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> > index ee59bbb8aa09..1f6ee0fd6d19 100644
> > --- a/drivers/iio/light/ltr390.c
> > +++ b/drivers/iio/light/ltr390.c
> > @@ -38,12 +38,20 @@
> >  #define LTR390_ALS_UVS_GAIN		0x05
> >  #define LTR390_PART_ID			0x06
> >  #define LTR390_MAIN_STATUS		0x07
> > -#define LTR390_ALS_DATA			0x0D
> > -#define LTR390_UVS_DATA			0x10
> > +#define LTR390_ALS_DATA_0		0x0D
> > +#define LTR390_ALS_DATA_1		0x0E
> > +#define LTR390_ALS_DATA_2		0x0F
> > +#define LTR390_UVS_DATA_0		0x10
> > +#define LTR390_UVS_DATA_1		0x11
> > +#define LTR390_UVS_DATA_2		0x12
> >  #define LTR390_INT_CFG			0x19
> >  #define LTR390_INT_PST			0x1A
> > -#define LTR390_THRESH_UP		0x21
> > -#define LTR390_THRESH_LOW		0x24
> > +#define LTR390_THRESH_UP_0		0x21
> > +#define LTR390_THRESH_UP_1		0x22
> > +#define LTR390_THRESH_UP_2		0x23
> > +#define LTR390_THRESH_LOW_0		0x24
> > +#define LTR390_THRESH_LOW_1		0x25
> > +#define LTR390_THRESH_LOW_2		0x26
> >  
> >  #define LTR390_PART_NUMBER_ID		0xb
> >  #define LTR390_ALS_UVS_GAIN_MASK	GENMASK(2, 0)
> > @@ -98,11 +106,62 @@ struct ltr390_data {
> >  	int int_time_us;
> >  };
> >  
> > +static bool ltr390_is_readable_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LTR390_MAIN_CTRL:
> > +	case LTR390_ALS_UVS_MEAS_RATE:
> > +	case LTR390_ALS_UVS_GAIN:
> > +	case LTR390_PART_ID:
> > +	case LTR390_MAIN_STATUS:
> > +	case LTR390_ALS_DATA_0:
> > +	case LTR390_ALS_DATA_1:
> > +	case LTR390_ALS_DATA_2:
> > +	case LTR390_UVS_DATA_0:
> > +	case LTR390_UVS_DATA_1:
> > +	case LTR390_UVS_DATA_2:
> > +	case LTR390_INT_CFG:
> > +	case LTR390_INT_PST:
> > +	case LTR390_THRESH_UP_0:
> > +	case LTR390_THRESH_UP_1:
> > +	case LTR390_THRESH_UP_2:
> > +	case LTR390_THRESH_LOW_0:
> > +	case LTR390_THRESH_LOW_1:
> > +	case LTR390_THRESH_LOW_2:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static bool ltr390_is_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case LTR390_MAIN_CTRL:
> > +	case LTR390_ALS_UVS_MEAS_RATE:
> > +	case LTR390_ALS_UVS_GAIN:
> > +	case LTR390_INT_CFG:
> > +	case LTR390_INT_PST:
> > +	case LTR390_THRESH_UP_0:
> > +	case LTR390_THRESH_UP_1:
> > +	case LTR390_THRESH_UP_2:
> > +	case LTR390_THRESH_LOW_0:
> > +	case LTR390_THRESH_LOW_1:
> > +	case LTR390_THRESH_LOW_2:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}  
> 
> Using rd_table and wr_table is a bit more compact than readable_reg
> writeable_reg.

Or given a lot of these are ranges of related registers you could use
ranges
	switch (reg) {
	case LTR390_MAIN_CTRL:
	case LTR390_ALS_UVS_MEAS_RATE:
	case LTR390_ALS_UVS_GAIN:
	case LTR390_INT_CFG:
	case LTR390_INT_PST:
	case LTR390_THRESH_UP_0 ... LTR390_THRESH_UP_2:
	case LTR390_THRESH_LOW_0 ... LTR390_THRESH_LOW_2:
		return true;

(with macros as Andy suggested)
	}

Re: [PATCH] iio: light: ltr390: Add debugfs register access support

Posted by Akshay Jindal 6 months, 2 weeks ago

Hello Reviewers/Maintainers,

Thank you for your valuable feedback.
I truly appreciate the time and effort you took to review the patch
and share your insights. All the feedback cut the overall changes by
approx 30 lines.

I have sent a V2 patch with the following changes, which is in line
with all the feedback received.

Changes since v1:
- Replaced _[0|1|2] macros with a respective common parameterized macro.
- Retained base macros to avoid churn.
- Swapped regmap_write with regmap_read to avoid negate operator.
- Simplified debugfs function by directly returning return value of
  regmap_[read|write].
- Replaced [readable|writeable]_reg with regmap ranges by using
  [rd|wr]_table property of regmap_config.

Jonathan,
Usage of ranges in switch/case was really amusing and a new learning
for me (never used it), but I felt using rd_table/wr_table
is a much cleaner way to use register ranges while checking read/write
register availability, which is fundamentally similar to
what you suggested. Hence implemented [rd|wr]_table.

Kindly provide feedback on the updated patch as per your convenience.

Thanks,
Akshay

Re: [PATCH] iio: light: ltr390: Add debugfs register access support

Posted by Andy Shevchenko 6 months, 2 weeks ago

On Wed, Jul 23, 2025 at 05:16:38PM +0530, Akshay Jindal wrote:
> Add support for debugfs_reg_access through the driver's iio_info structure
> to enable low-level register read/write access for debugging.

...

> -#define LTR390_ALS_DATA			0x0D

To avoid churn, I prefer this to stay.

> +#define LTR390_ALS_DATA_0		0x0D
> +#define LTR390_ALS_DATA_1		0x0E
> +#define LTR390_ALS_DATA_2		0x0F

Instead you can add a macro

#define LTR390_ALS_DATAx(x)		(0x0D + (x))

Same for other cases like this.

...

> +	ret = regmap_read(data->regmap, reg, readval);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

	return regmap_read(...);

-- 
With Best Regards,
Andy Shevchenko