linux-kernel - Re: [PATCH v4] iio: adc: pac1921: Add ACPI support to Microchip pac1921

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <172923759060.37070.4692932928966596704@njaxe.localdomain>
Date: Fri, 18 Oct 2024 09:46:30 +0200
From: Matteo Martelli <matteomartelli3@...il.com>
To: jic23@...nel.org, lars@...afoo.de, victor.duicu@...rochip.com
Cc: marius.cristea@...rochip.com, victor.duicu@...rochip.com, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] iio: adc: pac1921: Add ACPI support to Microchip pac1921

Quoting victor.duicu@...rochip.com (2024-10-15 13:24:41)
> From: Victor Duicu <victor.duicu@...rochip.com>
> 
> This patch implements ACPI support to Microchip pac1921.
> The driver can read shunt resistor value and label from ACPI table.
> 
> Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
> ---
> 
> The patch was tested on minnowboard and sama5.
> 
> Differences related to previous versions:
> v4:
> - change name of pac1921_shunt_is_valid to pac1921_shunt_is_invalid.
> - fix coding style.
> - in pac1921_parse_of_fw change back to device_property_read_u32.
> 
> v3:
> - simplify and make inline function pac1921_shunt_is_valid. Make argument u64.
> - fix link to DSM documentation.
> - in pac1921_match_acpi_device and pac1921_parse_of_fw, the shunt value is
> read as u64.
> - in pac1921_parse_of_fw remove code for reading label value from
> devicetree.
> - in pac1921_write_shunt_resistor cast the multiply result to u64 in order
> to fix overflow.
> 
> v2:
> - remove name variable from priv. Driver reads label attribute with
> sysfs.
> - define pac1921_shunt_is_valid function.
> - move default assignments in pac1921_probe to original position.
> - roll back coding style changes.
> - add documentation for DSM(the linked document was used as reference).
> - remove acpi_match_device in pac1921_match_acpi_device.
> - remove unnecessary null assignment and comment.
> - change name of function pac1921_match_of_device to
> pac1921_parse_of_fw.
> 
> v1:
> - initial version for review.
> 
>  drivers/iio/adc/pac1921.c | 104 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 90 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/pac1921.c b/drivers/iio/adc/pac1921.c
> index a96fae546bc1..8b5127b7ee3c 100644
> --- a/drivers/iio/adc/pac1921.c
> +++ b/drivers/iio/adc/pac1921.c
> @@ -67,6 +67,10 @@ enum pac1921_mxsl {
>  #define PAC1921_DEFAULT_DI_GAIN                0 /* 2^(value): 1x gain (HW default) */
>  #define PAC1921_DEFAULT_NUM_SAMPLES    0 /* 2^(value): 1 sample (HW default) */
>  
> +#define PAC1921_ACPI_GET_UOHMS_VALS             0
> +#define PAC1921_ACPI_GET_LABEL                 1
> +#define PAC1921_DSM_UUID                        "f7bb9932-86ee-4516-a236-7a7a742e55cb"
> +
>  /*
>   * Pre-computed scale factors for BUS voltage
>   * format: IIO_VAL_INT_PLUS_NANO
> @@ -204,6 +208,11 @@ struct pac1921_priv {
>         } scan;
>  };
>  
> +static inline bool pac1921_shunt_is_invalid(u64 shunt_val)
> +{
> +       return (shunt_val == 0 || shunt_val > INT_MAX);
> +}
> +
>  /*
>   * Check if first integration after configuration update has completed.
>   *
> @@ -792,13 +801,13 @@ static ssize_t pac1921_write_shunt_resistor(struct iio_dev *indio_dev,
>         if (ret)
>                 return ret;
>  
> -       rshunt_uohm = val * MICRO + val_fract;
> -       if (rshunt_uohm == 0 || rshunt_uohm > INT_MAX)
> +       rshunt_uohm = (u64)val * MICRO + val_fract;
> +       if (pac1921_shunt_is_invalid(rshunt_uohm))
>                 return -EINVAL;
>  
>         guard(mutex)(&priv->lock);
>  
> -       priv->rshunt_uohm = rshunt_uohm;
> +       priv->rshunt_uohm = (u32)rshunt_uohm;

This explicit cast seems unnecessary (note the similar assignment in the
pac1921_match_acpi_device() that doesn't use the explicit cast).

Beside that the rest of the patch looks good to me.

Acked-by: Matteo Martelli <matteomartelli3@...il.com>

Best regards,
Matteo Martelli