Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   David Laight <David.Laight@ACULAB.COM>
To:     'Herve Codina' <herve.codina@bootlin.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        "Kuninori Morimoto" <kuninori.morimoto.gx@renesas.com>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
CC:     "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: RE: [PATCH v5 07/13] minmax: Introduce {min,max}_array()
Thread-Topic: [PATCH v5 07/13] minmax: Introduce {min,max}_array()
Thread-Index: AQHZn535xpz9i2gsDE6nNaGlDsvHDa+TlXFw
Date:   Tue, 20 Jun 2023 11:45:01 +0000
Message-ID: <70697b976107473b8779eea7d6c8a189@AcuMS.aculab.com>
References: <20230615152631.224529-1-herve.codina@bootlin.com>
 <20230615152631.224529-8-herve.codina@bootlin.com>
In-Reply-To: <20230615152631.224529-8-herve.codina@bootlin.com>
Accept-Language: en-GB, en-US
MIME-Version: 1.0
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Precedence: bulk

From: Herve Codina
> Sent: 15 June 2023 16:26
> 
> Introduce min_array() (resp max_array()) in order to get the
> minimal (resp maximum) of values present in an array.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  include/linux/minmax.h | 64 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 396df1121bff..1672985b02a3 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -133,6 +133,70 @@
>   */
>  #define max_t(type, x, y)	__careful_cmp((type)(x), (type)(y), >)
> 
> +/*
> + * Remove a const qualifier from integer types
> + * _Generic(foo, type-name: association, ..., default: association) performs a
> + * comparison against the foo type (not the qualified type).
> + * Do not use the const keyword in the type-name as it will not match the
> + * unqualified type of foo.
> + */
> +#define __unconst_integer_type_cases(type)	\
> +	unsigned type:  (unsigned type)0,	\
> +	signed type:    (signed type)0
> +
> +#define __unconst_integer_typeof(x) typeof(			\
> +	_Generic((x),						\
> +		char: (char)0,					\
> +		__unconst_integer_type_cases(char),		\
> +		__unconst_integer_type_cases(short),		\
> +		__unconst_integer_type_cases(int),		\
> +		__unconst_integer_type_cases(long),		\
> +		__unconst_integer_type_cases(long long),	\
> +		default: (x)))

Those are probably more generally useful and belong elsewhere.

> +
> +/*
> + * Do not check the array parameter using __must_be_array().
> + * In the following legit use-case where the "array" passed is a simple pointer,
> + * __must_be_array() will return a failure.
> + * --- 8< ---
> + * int *buff
> + * ...
> + * min = min_array(buff, nb_items);
> + * --- 8< ---

Is that needed in the .h file?

> + *
> + * The first typeof(&(array)[0]) is needed in order to support arrays of both
> + * 'int *buff' and 'int buf[N]' types.
> + *
> + * The array can be an array of const items.
> + * typeof() keeps the const qualifier. Use __unconst_typeof() in order to
> + * discard the const qualifier for the __element variable.
> + */
> +#define __minmax_array(op, array, len) ({				\
> +	typeof(&(array)[0]) __array = (array);				\
> +	typeof(len) __len = (len);					\
> +	__unconst_integer_typeof(__array[0]) __element = __array[--__len]; \

s/__element/__bound/

> +	while (__len--)							\
> +		__element = op(__element, __array[__len]);		\
> +	__element; })

I'm not all sure that all the shenanigans required to use min()
is really needed here.

It would also be generally better to process the array forwards.
So something like:
	typeof (&array[0]) __ptr = array, __limit = array + len;
	typeof (array[0] + 0) __element, __bound = *__ptr++;
	while (ptr < __limit) {
		__element = *__ptr++;
		if (__element > __bound)
			__bound = __element;
	}
	(typeof (array[0]))__bound; })
seems fine to me.
The final cast is there to convert 'int' back to un/signed char|short.
Not really needed and might generate worse code.

But if you insist on using min/max ignore this bit.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)