[PATCH v3] iio: sx9324: avoid copying property strings

[PATCH v3] iio: sx9324: avoid copying property strings

[Justin Stitt]

We're doing some needless string copies when trying to assign the proper
`prop` string. We can make `prop` a const char* and simply assign to
string literals.

For the case where a format string is used, let's extract the parsing
logic out into sx9324_parse_phase_prop(). We no longer need to create
copies or allocate new memory.

sx9324_parse_phase_prop() will simply return the default def value if it
fails.

This also cleans up some deprecated strncpy() uses [1].

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
Link: https://github.com/KSPP/linux/issues/90
Cc: linux-hardening@vger.kernel.org
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
Changes in v3:
- extract logic into sx9324_parse_phase_prop() and use string literals
  (thanks Stephen)
- rebase onto mainline bee0e7762ad2c602
- Link to v2: https://lore.kernel.org/r/20231026-strncpy-drivers-iio-proximity-sx9324-c-v2-1-cee6e5db700c@google.com

Changes in v2:
- make prop a const char* and do simple assignments (thanks Jonathan)
- rebase onto 3a568e3a961ba330
- Link to v1: https://lore.kernel.org/r/20230921-strncpy-drivers-iio-proximity-sx9324-c-v1-1-4e8d28fd1e7c@google.com
---
---
 drivers/iio/proximity/sx9324.c | 69 ++++++++++++++++++++++++++----------------
 1 file changed, 43 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index 438f9c9aba6e..e3bc30b57b19 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -873,6 +873,32 @@ static int sx9324_init_compensation(struct iio_dev *indio_dev)
 					20000, 2000000);
 }
 
+static u32 sx9324_parse_phase_prop(struct device *dev,
+				   struct sx_common_reg_default *reg_def,
+				   int idx, const char *prop)
+{
+	unsigned int pin_defs[SX9324_NUM_PINS];
+	int count, ret, pin;
+	u8 default_def;
+	u32 raw = 0;
+
+	default_def = sx9324_default_regs[idx].def;
+
+	count = device_property_count_u32(dev, prop);
+	if (count != ARRAY_SIZE(pin_defs))
+		return default_def;
+	ret = device_property_read_u32_array(dev, prop, pin_defs,
+					     ARRAY_SIZE(pin_defs));
+	if (ret)
+		return default_def;
+
+	for (pin = 0; pin < SX9324_NUM_PINS; pin++)
+		raw |= (pin_defs[pin] << (2 * pin)) &
+		       SX9324_REG_AFE_PH0_PIN_MASK(pin);
+
+	return raw;
+}
+
 static const struct sx_common_reg_default *
 sx9324_get_default_reg(struct device *dev, int idx,
 		       struct sx_common_reg_default *reg_def)
@@ -884,35 +910,30 @@ sx9324_get_default_reg(struct device *dev, int idx,
 #define SX9324_PIN_DEF "semtech,ph0-pin"
 #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
 #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
-	unsigned int pin_defs[SX9324_NUM_PINS];
-	char prop[] = SX9324_PROXRAW_DEF;
+	const char *prop = SX9324_PROXRAW_DEF;
 	u32 start = 0, raw = 0, pos = 0;
-	int ret, count, ph, pin;
 	const char *res;
+	int ret;
 
 	memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
 
 	sx_common_get_raw_register_config(dev, reg_def);
 	switch (reg_def->reg) {
 	case SX9324_REG_AFE_PH0:
+		reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+						       "semtech,ph0-pin");
+		break;
 	case SX9324_REG_AFE_PH1:
+		reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+						       "semtech,ph1-pin");
+		break;
 	case SX9324_REG_AFE_PH2:
+		reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+						       "semtech,ph2-pin");
+		break;
 	case SX9324_REG_AFE_PH3:
-		ph = reg_def->reg - SX9324_REG_AFE_PH0;
-		snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
-
-		count = device_property_count_u32(dev, prop);
-		if (count != ARRAY_SIZE(pin_defs))
-			break;
-		ret = device_property_read_u32_array(dev, prop, pin_defs,
-						     ARRAY_SIZE(pin_defs));
-		if (ret)
-			break;
-
-		for (pin = 0; pin < SX9324_NUM_PINS; pin++)
-			raw |= (pin_defs[pin] << (2 * pin)) &
-			       SX9324_REG_AFE_PH0_PIN_MASK(pin);
-		reg_def->def = raw;
+		reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx,
+						       "semtech,ph3-pin");
 		break;
 	case SX9324_REG_AFE_CTRL0:
 		ret = device_property_read_string(dev,
@@ -937,11 +958,9 @@ sx9324_get_default_reg(struct device *dev, int idx,
 	case SX9324_REG_AFE_CTRL4:
 	case SX9324_REG_AFE_CTRL7:
 		if (reg_def->reg == SX9324_REG_AFE_CTRL4)
-			strncpy(prop, "semtech,ph01-resolution",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph01-resolution";
 		else
-			strncpy(prop, "semtech,ph23-resolution",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph23-resolution";
 
 		ret = device_property_read_u32(dev, prop, &raw);
 		if (ret)
@@ -1012,11 +1031,9 @@ sx9324_get_default_reg(struct device *dev, int idx,
 	case SX9324_REG_PROX_CTRL0:
 	case SX9324_REG_PROX_CTRL1:
 		if (reg_def->reg == SX9324_REG_PROX_CTRL0)
-			strncpy(prop, "semtech,ph01-proxraw-strength",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph01-proxraw-strength";
 		else
-			strncpy(prop, "semtech,ph23-proxraw-strength",
-				ARRAY_SIZE(prop));
+			prop = "semtech,ph23-proxraw-strength";
 		ret = device_property_read_u32(dev, prop, &raw);
 		if (ret)
 			break;

---
base-commit: bee0e7762ad2c6025b9f5245c040fcc36ef2bde8
change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039

Best regards,
--
Justin Stitt <justinstitt@google.com>

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Joe Perches]

On Tue, 2023-12-12 at 00:42 +0000, Justin Stitt wrote:
> We're doing some needless string copies when trying to assign the proper
> `prop` string. We can make `prop` a const char* and simply assign to
> string literals.

trivia:

I would have updated it like this moving the
various declarations into the case blocks
where they are used and removing a few unused
#defines

---
 drivers/iio/proximity/sx9324.c | 69 +++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index ac2ed2da21ccc..c50c1108a69cc 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -877,17 +877,8 @@ static const struct sx_common_reg_default *
 sx9324_get_default_reg(struct device *dev, int idx,
 		       struct sx_common_reg_default *reg_def)
 {
-	static const char * const sx9324_rints[] = { "lowest", "low", "high",
-		"highest" };
-	static const char * const sx9324_csidle[] = { "hi-z", "hi-z", "gnd",
-		"vdd" };
-#define SX9324_PIN_DEF "semtech,ph0-pin"
-#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
-#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
-	unsigned int pin_defs[SX9324_NUM_PINS];
-	char prop[] = SX9324_PROXRAW_DEF;
-	u32 start = 0, raw = 0, pos = 0;
-	int ret, count, ph, pin;
+	u32 raw = 0;
+	int ret;
 
 	memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
 
@@ -896,7 +887,13 @@ sx9324_get_default_reg(struct device *dev, int idx,
 	case SX9324_REG_AFE_PH0:
 	case SX9324_REG_AFE_PH1:
 	case SX9324_REG_AFE_PH2:
-	case SX9324_REG_AFE_PH3:
+	case SX9324_REG_AFE_PH3: {
+		unsigned int pin_defs[SX9324_NUM_PINS];
+		int count;
+		int pin;
+		int ph;
+		char prop[32];
+
 		ph = reg_def->reg - SX9324_REG_AFE_PH0;
 		snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
 
@@ -913,7 +910,15 @@ sx9324_get_default_reg(struct device *dev, int idx,
 			       SX9324_REG_AFE_PH0_PIN_MASK(pin);
 		reg_def->def = raw;
 		break;
-	case SX9324_REG_AFE_CTRL0:
+	}
+	case SX9324_REG_AFE_CTRL0: {
+		static const char * const sx9324_csidle[] = {
+			"hi-z", "hi-z", "gnd", "vdd"
+		};
+		static const char * const sx9324_rints[] = {
+			"lowest", "low", "high", "highest"
+		};
+
 		ret = device_property_match_property_string(dev, "semtech,cs-idle-sleep",
 							    sx9324_csidle,
 							    ARRAY_SIZE(sx9324_csidle));
@@ -930,16 +935,17 @@ sx9324_get_default_reg(struct device *dev, int idx,
 			reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
 		}
 		break;
+	}
 	case SX9324_REG_AFE_CTRL4:
-	case SX9324_REG_AFE_CTRL7:
+	case SX9324_REG_AFE_CTRL7: {
+		const char *type;
+
 		if (reg_def->reg == SX9324_REG_AFE_CTRL4)
-			strncpy(prop, "semtech,ph01-resolution",
-				ARRAY_SIZE(prop));
+			type = "semtech,ph01-resolution";
 		else
-			strncpy(prop, "semtech,ph23-resolution",
-				ARRAY_SIZE(prop));
+			type = "semtech,ph23-resolution";
 
-		ret = device_property_read_u32(dev, prop, &raw);
+		ret = device_property_read_u32(dev, type, &raw);
 		if (ret)
 			break;
 
@@ -949,6 +955,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
 		reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
 					   raw);
 		break;
+	}
 	case SX9324_REG_AFE_CTRL8:
 		ret = device_property_read_u32(dev,
 				"semtech,input-precharge-resistor-ohms",
@@ -982,17 +989,21 @@ sx9324_get_default_reg(struct device *dev, int idx,
 					   6 + raw * (raw + 3) / 2);
 		break;
 
-	case SX9324_REG_ADV_CTRL5:
+	case SX9324_REG_ADV_CTRL5: {
+		u32 start = 0;
+
 		ret = device_property_read_u32(dev, "semtech,startup-sensor",
 					       &start);
 		if (ret)
 			break;
-
 		reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
 		reg_def->def |= FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
 					   start);
 		break;
-	case SX9324_REG_PROX_CTRL4:
+	}
+	case SX9324_REG_PROX_CTRL4: {
+		u32 pos = 0;
+
 		ret = device_property_read_u32(dev, "semtech,avg-pos-strength",
 					       &pos);
 		if (ret)
@@ -1005,15 +1016,16 @@ sx9324_get_default_reg(struct device *dev, int idx,
 		reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK,
 					   raw);
 		break;
+	}
 	case SX9324_REG_PROX_CTRL0:
-	case SX9324_REG_PROX_CTRL1:
+	case SX9324_REG_PROX_CTRL1: {
+		const char *type;
+
 		if (reg_def->reg == SX9324_REG_PROX_CTRL0)
-			strncpy(prop, "semtech,ph01-proxraw-strength",
-				ARRAY_SIZE(prop));
+			type = "semtech,ph01-proxraw-strength";
 		else
-			strncpy(prop, "semtech,ph23-proxraw-strength",
-				ARRAY_SIZE(prop));
-		ret = device_property_read_u32(dev, prop, &raw);
+			type = "semtech,ph23-proxraw-strength";
+		ret = device_property_read_u32(dev, type, &raw);
 		if (ret)
 			break;
 
@@ -1022,6 +1034,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
 					   raw);
 		break;
 	}
+	}
 	return reg_def;
 }
 

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Jonathan Cameron]

On Mon, 11 Dec 2023 22:30:12 -0800
Joe Perches <joe@perches.com> wrote:

> On Tue, 2023-12-12 at 00:42 +0000, Justin Stitt wrote:
> > We're doing some needless string copies when trying to assign the proper
> > `prop` string. We can make `prop` a const char* and simply assign to
> > string literals.  
> 
> trivia:
> 
> I would have updated it like this moving the
> various declarations into the case blocks
> where they are used and removing a few unused
> #defines

I'd definitely like to see those defines gone.
Arguably an unrelated change as I guess they are left from a previous refactor
of this code.

Why prop to type renaming?  It's getting passed into calls where the parameter
is propname so I'd understand renaming to that, but type just seems a bit random
to me.  I do wonder if we are better off having some long lines and getting rid
of the property naming local variables completely by just duplicating
the device_property_read_u32() call and passing them in directly.

Moving declarations more locally is a nice to have but I'll leave that up to Justin.

Anyhow, both solutions look much better than the original so I'm fine either way
(subject to responses to Stephen's review)

> 
> ---
>  drivers/iio/proximity/sx9324.c | 69 +++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> index ac2ed2da21ccc..c50c1108a69cc 100644
> --- a/drivers/iio/proximity/sx9324.c
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -877,17 +877,8 @@ static const struct sx_common_reg_default *
>  sx9324_get_default_reg(struct device *dev, int idx,
>  		       struct sx_common_reg_default *reg_def)
>  {
> -	static const char * const sx9324_rints[] = { "lowest", "low", "high",
> -		"highest" };
> -	static const char * const sx9324_csidle[] = { "hi-z", "hi-z", "gnd",
> -		"vdd" };
> -#define SX9324_PIN_DEF "semtech,ph0-pin"
> -#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
> -#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
> -	unsigned int pin_defs[SX9324_NUM_PINS];
> -	char prop[] = SX9324_PROXRAW_DEF;
> -	u32 start = 0, raw = 0, pos = 0;
> -	int ret, count, ph, pin;
> +	u32 raw = 0;
> +	int ret;
>  
>  	memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
>  
> @@ -896,7 +887,13 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  	case SX9324_REG_AFE_PH0:
>  	case SX9324_REG_AFE_PH1:
>  	case SX9324_REG_AFE_PH2:
> -	case SX9324_REG_AFE_PH3:
> +	case SX9324_REG_AFE_PH3: {
> +		unsigned int pin_defs[SX9324_NUM_PINS];
> +		int count;
> +		int pin;
> +		int ph;
> +		char prop[32];
> +
>  		ph = reg_def->reg - SX9324_REG_AFE_PH0;
>  		snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
>  
> @@ -913,7 +910,15 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  			       SX9324_REG_AFE_PH0_PIN_MASK(pin);
>  		reg_def->def = raw;
>  		break;
> -	case SX9324_REG_AFE_CTRL0:
> +	}
> +	case SX9324_REG_AFE_CTRL0: {
> +		static const char * const sx9324_csidle[] = {
> +			"hi-z", "hi-z", "gnd", "vdd"
> +		};
> +		static const char * const sx9324_rints[] = {
> +			"lowest", "low", "high", "highest"
> +		};
> +
>  		ret = device_property_match_property_string(dev, "semtech,cs-idle-sleep",
>  							    sx9324_csidle,
>  							    ARRAY_SIZE(sx9324_csidle));
> @@ -930,16 +935,17 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  			reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
>  		}
>  		break;
> +	}
>  	case SX9324_REG_AFE_CTRL4:
> -	case SX9324_REG_AFE_CTRL7:
> +	case SX9324_REG_AFE_CTRL7: {
> +		const char *type;
> +
>  		if (reg_def->reg == SX9324_REG_AFE_CTRL4)
> -			strncpy(prop, "semtech,ph01-resolution",
> -				ARRAY_SIZE(prop));
> +			type = "semtech,ph01-resolution";
>  		else
> -			strncpy(prop, "semtech,ph23-resolution",
> -				ARRAY_SIZE(prop));
> +			type = "semtech,ph23-resolution";
>  
> -		ret = device_property_read_u32(dev, prop, &raw);
> +		ret = device_property_read_u32(dev, type, &raw);
>  		if (ret)
>  			break;
>  
> @@ -949,6 +955,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  		reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
>  					   raw);
>  		break;
> +	}
>  	case SX9324_REG_AFE_CTRL8:
>  		ret = device_property_read_u32(dev,
>  				"semtech,input-precharge-resistor-ohms",
> @@ -982,17 +989,21 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  					   6 + raw * (raw + 3) / 2);
>  		break;
>  
> -	case SX9324_REG_ADV_CTRL5:
> +	case SX9324_REG_ADV_CTRL5: {
> +		u32 start = 0;
> +
>  		ret = device_property_read_u32(dev, "semtech,startup-sensor",
>  					       &start);
>  		if (ret)
>  			break;
> -
>  		reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
>  		reg_def->def |= FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
>  					   start);
>  		break;
> -	case SX9324_REG_PROX_CTRL4:
> +	}
> +	case SX9324_REG_PROX_CTRL4: {
> +		u32 pos = 0;
> +
>  		ret = device_property_read_u32(dev, "semtech,avg-pos-strength",
>  					       &pos);
>  		if (ret)
> @@ -1005,15 +1016,16 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  		reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK,
>  					   raw);
>  		break;
> +	}
>  	case SX9324_REG_PROX_CTRL0:
> -	case SX9324_REG_PROX_CTRL1:
> +	case SX9324_REG_PROX_CTRL1: {
> +		const char *type;
> +
>  		if (reg_def->reg == SX9324_REG_PROX_CTRL0)
> -			strncpy(prop, "semtech,ph01-proxraw-strength",
> -				ARRAY_SIZE(prop));
> +			type = "semtech,ph01-proxraw-strength";
>  		else
> -			strncpy(prop, "semtech,ph23-proxraw-strength",
> -				ARRAY_SIZE(prop));
> -		ret = device_property_read_u32(dev, prop, &raw);
> +			type = "semtech,ph23-proxraw-strength";
> +		ret = device_property_read_u32(dev, type, &raw);
>  		if (ret)
>  			break;
>  
> @@ -1022,6 +1034,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  					   raw);
>  		break;
>  	}
> +	}
>  	return reg_def;
>  }
>  
> 
> 

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Joe Perches]

On Sun, 2023-12-17 at 13:24 +0000, Jonathan Cameron wrote:
> On Mon, 11 Dec 2023 22:30:12 -0800
> Joe Perches <joe@perches.com> wrote:
> 
> > On Tue, 2023-12-12 at 00:42 +0000, Justin Stitt wrote:
> > > We're doing some needless string copies when trying to assign the proper
> > > `prop` string. We can make `prop` a const char* and simply assign to
> > > string literals.  
> > 
> > trivia:
> > 
> > I would have updated it like this moving the
> > various declarations into the case blocks
> > where they are used and removing a few unused
> > #defines
> 
> I'd definitely like to see those defines gone.
> Arguably an unrelated change as I guess they are left from a previous refactor
> of this code.
> 
> Why prop to type renaming?

random, no specific need, though I prefer not reusing
identifiers with different types in separate local scopes.

>   It's getting passed into calls where the parameter
> is propname so I'd understand renaming to that, but type just seems a bit random
> to me.  I do wonder if we are better off having some long lines and getting rid
> of the property naming local variables completely by just duplicating
> the device_property_read_u32() call and passing them in directly.

maybe, give it a try and see what you think.

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Kees Cook]

On Tue, Dec 12, 2023 at 12:42:52AM +0000, Justin Stitt wrote:
> We're doing some needless string copies when trying to assign the proper
> `prop` string. We can make `prop` a const char* and simply assign to
> string literals.
> 
> For the case where a format string is used, let's extract the parsing
> logic out into sx9324_parse_phase_prop(). We no longer need to create
> copies or allocate new memory.
> 
> sx9324_parse_phase_prop() will simply return the default def value if it
> fails.
> 
> This also cleans up some deprecated strncpy() uses [1].
> 
> Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-hardening@vger.kernel.org
> Signed-off-by: Justin Stitt <justinstitt@google.com>

Yeah, I like this approach. Totally removes the string copying and
creates a helper that does the "heavy lifting". :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v3] iio: sx9324: avoid copying property strings

[Stephen Boyd]

Quoting Justin Stitt (2023-12-11 16:42:52)
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> index 438f9c9aba6e..e3bc30b57b19 100644
> --- a/drivers/iio/proximity/sx9324.c
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -873,6 +873,32 @@ static int sx9324_init_compensation(struct iio_dev *indio_dev)
>                                         20000, 2000000);
>  }
>
> +static u32 sx9324_parse_phase_prop(struct device *dev,

return u8? because that's the type of struct sx_common_reg_default::def.

> +                                  struct sx_common_reg_default *reg_def,
> +                                  int idx, const char *prop)
> +{
> +       unsigned int pin_defs[SX9324_NUM_PINS];
> +       int count, ret, pin;
> +       u8 default_def;
> +       u32 raw = 0;
> +
> +       default_def = sx9324_default_regs[idx].def;

Do we need to do this? Isn't this reg_def->def?

> +
> +       count = device_property_count_u32(dev, prop);
> +       if (count != ARRAY_SIZE(pin_defs))
> +               return default_def;

return reg_def->def?

> +       ret = device_property_read_u32_array(dev, prop, pin_defs,
> +                                            ARRAY_SIZE(pin_defs));
> +       if (ret)
> +               return default_def;

return reg_def->def?

> +
> +       for (pin = 0; pin < SX9324_NUM_PINS; pin++)
> +               raw |= (pin_defs[pin] << (2 * pin)) &
> +                      SX9324_REG_AFE_PH0_PIN_MASK(pin);
> +
> +       return raw;
> +}
> +
>  static const struct sx_common_reg_default *
>  sx9324_get_default_reg(struct device *dev, int idx,
>                        struct sx_common_reg_default *reg_def)
> @@ -884,35 +910,30 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  #define SX9324_PIN_DEF "semtech,ph0-pin"
>  #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
>  #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"

Can you send a followup to remove these defines?

> -       unsigned int pin_defs[SX9324_NUM_PINS];
> -       char prop[] = SX9324_PROXRAW_DEF;
> +       const char *prop = SX9324_PROXRAW_DEF;

This can be left unassigned now, right?

>         u32 start = 0, raw = 0, pos = 0;
> -       int ret, count, ph, pin;
>         const char *res;
> +       int ret;