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
