Re: [PATCH v7] iio: adc: pac1921: Add ACPI support to Microchip pac1921

[Andy Shevchenko]

Thu, Oct 31, 2024 at 08:52:05AM +0200, victor.duicu@xxxxxxxxxxxxx kirjoitti:
> From: Victor Duicu <victor.duicu@xxxxxxxxxxxxx>
> 
> This patch implements ACPI support to Microchip pac1921.
> The driver can read shunt resistor value and label from ACPI table.

More comments below.

...

> +#define PAC1921_ACPI_GET_UOHMS_VALS             0

uOHM ?

#define PAC1921_ACPI_GET_uOHM_VALS             0

...

> +/* The maximum accepted value of shunt_resistor in UOHMS <= INT_MAX */
> +#define PAC1921_MAX_SHUNT_VALUE_OHMS		2147

Instead of the comment do it like this:

#define PAC1921_MAX_SHUNT_VALUE_OHM		(INT_MAX / MICRO)

Need to include limits.h and units.h.

...

> +static inline bool pac1921_shunt_is_invalid(u32 shunt_val)

is_invalid is confusing name, make it rather is_valid

> +{
> +	return shunt_val == 0 || shunt_val > INT_MAX;
> +}

...

> +	/* This check is to ensure val * MICRO won't overflow */
> +	if (val < 0 || val > PAC1921_MAX_SHUNT_VALUE_OHMS)
> +		return -EINVAL;
> +
>  	rshunt_uohm = val * MICRO + val_fract;
> -	if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> +	if (pac1921_shunt_is_invalid(rshunt_uohm))
>  		return -EINVAL;

With the above check how this can't be a dead code?

...

> +	rez = acpi_evaluate_dsm(handle, &guid, 1, PAC1921_ACPI_GET_UOHMS_VALS, NULL);
> +	if (!rez)

Use status instead of rez for the variable name. This is kinda standard to keep
the return of ACPI APIs.

> +		return dev_err_probe(dev, -EINVAL,
> +				     "Could not read shunt from ACPI table\n");

...

> +	label = devm_kstrdup(dev, rez->package.elements->string.pointer, GFP_KERNEL);
> +	if (!label)
> +		return dev_err_probe(dev, -EINVAL, "Label is NULL\n");

We do not print an error for -ENOMEM, which should be here as the error code
(Jonathan already pointed out on this).

So, just

		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko