mbox series

[v4,00/13] Add support for IIO devices in ASoC

Message ID 20230614074904.29085-1-herve.codina@bootlin.com
Headers show
Series Add support for IIO devices in ASoC | expand

Message

Herve Codina June 14, 2023, 7:48 a.m. UTC
Several weeks ago, I sent a series [1] for adding a potentiometer as an
auxiliary device in ASoC. The feedback was that the potentiometer should
be directly handled in IIO (as other potentiometers) and something more
generic should be present in ASoC in order to have a binding to import
some IIO devices into sound cards.

The series related to the IIO potentiometer device is already applied.

This series introduces audio-iio-aux. Its goal is to offer the binding
between IIO and ASoC.
It exposes attached IIO devices as ASoC auxiliary devices and allows to
control them through mixer controls.

On my system, the IIO device is a potentiometer and it is present in an
amplifier design present in the audio path.

Compare to the previous iteration
  https://lore.kernel.org/linux-kernel/20230612122926.107333-1-herve.codina@bootlin.com/
This v4 series mainly:
 - Fixes headers inclusion
 - Removes unneeded variable initialization
 - Adds a missing error check
 - Fixes typos

Best regards,
Hervé

[1] https://lore.kernel.org/linux-kernel/20230203111422.142479-1-herve.codina@bootlin.com/
[2] https://lore.kernel.org/linux-kernel/20230421085245.302169-1-herve.codina@bootlin.com/

Changes v3 -> v4
  - Patches 1, 2
    No changes.

  - Patches 3, 4, 5
    Add 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>'.

  - Patch 6 (new in v4)
    Fix headers inclusion order.

  - Patch 7 (patch 6 in v3)
    Add a comment related to __must_be_array()
    Use __array[0] of *__array

  - Patch 8 (patch 7 in v3)
    Fix minmax.h inclusion order.
    Add 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>'.

  - Patch 9 (patch 8 in v3)
    Add 'Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>'.
    Add 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>'.

  - Patch 10 (patch 9 in v3)
    Add 'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>'.

  - Patch 11 (patch 10 in v3)
    Fix a typo.
    Add	'Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>'.
    Add	'Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>'.

  - Patch 12 (patch 11 in v3)
    Fix typos in the commit log.
    Fix headers inclusion order.
    Removed unneeded variable initialization.
    Replace {0} by {}.
    Use struct device *dev in probe().
    Check an error on the snd-control-invert-range property read.

  - Patch 13 (patch12 in v3)
    No changes.

Changes v2 -> v3
  - Patches 1, 2
    No changes.

  - Patch 3, 4
    Add 'Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>'.

  - Patch 5 (new in v3)
    Removed the 'unused' variable and check the null pointer when used.

  - Patch 6 (new in v3)
    Introduce {min,max}_array().

  - Patch 7 (new in v3)
    Use max_array() in iio_channel_read_max().

  - Patch 8 (new in v3)
    Replace a FIXME comment by a TODO one.

  - Patch 9 (patch 5 in v2)
    Removed the 'unused' variable and check the null pointer when used.
    Use min_array().
    Remplace a FIXME comment by a TODO one.

  - Patch 10 (patch 6 in v2)
    Convert existing macros to return a compound litteral instead of
    adding a new helper.

  - Patch 11 (patch 7 in v2)
    Remove the file name from the C file header.
    Use directly converted DAPM macros.
    Replace <linux/module.h> by <linux/mod_devicetable.h>.
    Add <linux/platform_device.h>.
    Be sure that min <= max. Swap values if it is not the case.
    Move the bool structure member after the int ones.
    Remove unneeded assignements.
    Use dev_err_probe() when relevant.
    Use str_on_off().
    Use static_assert() instead of BUILD_BUG_ON().
    Remove unneeded comma and blank line.
    Use device_property_*() instead of the OF API.

  - patch 8 available in v2 removed as already applied

  - Patch 12 (patch 9 in v2)
    Use devm_add_action_or_reset().
    Call simple_populate_aux() from simple_parse_of().

Changes v1 -> v2
  - Patch 1
    Rename simple-iio-aux to audio-iio-aux
    Rename invert to snd-control-invert-range
    Remove the /schemas/iio/iio-consumer.yaml reference
    Remove the unneeded '|' after description

  - Patch 2 (new in v2)
    Introduce the simple-audio-card additional-devs subnode

  - Patch 3 (new in v2)
    Check err before switch() in iio_channel_read_max()

  - Patch 4 (new in v2)
    Fix raw reads and raw writes documentation

  - Patch 5 (patch 2 in v1)
    Check err before switch() in iio_channel_read_min()
    Fix documentation

  - Patch 6 (path 3 in v1)
    No changes

  - Patch 7 (patch 4 in v1)
    Rename simple-iio-aux to audio-iio-aux
    Rename invert to snd-control-invert-range
    Remove the mask usage from audio_iio_aux_{get,put}_volsw helpers
    Use directly PTR_ERR() in dev_err_probe() parameter
    Remove the '!!' construction
    Remove of_match_ptr()

  - Patch 8 (new in v2)
    Add a missing of_node_put() in the simple-card driver

  - Patch 9 (new in v2)
    Handle additional-devs in the simple-card driver

Herve Codina (13):
  ASoC: dt-bindings: Add audio-iio-aux
  ASoC: dt-bindings: simple-card: Add additional-devs subnode
  iio: inkern: Check error explicitly in iio_channel_read_max()
  iio: consumer.h: Fix raw values documentation notes
  iio: inkern: Remove the 'unused' variable usage in
    iio_channel_read_max()
  iio: inkern: Fix headers inclusion order
  minmax: Introduce {min,max}_array()
  iio: inkern: Use max_array() to get the maximum value from an array
  iio: inkern: Replace a FIXME comment by a TODO one
  iio: inkern: Add a helper to query an available minimum raw value
  ASoC: soc-dapm.h: Convert macros to return a compound literal
  ASoC: codecs: Add support for the generic IIO auxiliary devices
  ASoC: simple-card: Handle additional devices

 .../bindings/sound/audio-iio-aux.yaml         |  64 ++++
 .../bindings/sound/simple-card.yaml           |  53 +++
 drivers/iio/inkern.c                          |  86 ++++-
 include/linux/iio/consumer.h                  |  37 +-
 include/linux/minmax.h                        |  36 ++
 include/sound/soc-dapm.h                      | 138 ++++---
 sound/soc/codecs/Kconfig                      |  12 +
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/audio-iio-aux.c              | 338 ++++++++++++++++++
 sound/soc/generic/simple-card.c               |  46 ++-
 10 files changed, 741 insertions(+), 71 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/audio-iio-aux.yaml
 create mode 100644 sound/soc/codecs/audio-iio-aux.c

Comments

Andy Shevchenko June 14, 2023, 8:58 a.m. UTC | #1
On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Sort headers inclusion in alphabetic order.

More precisely fix mutex.h inclusion order as it seems to be the only
one misplaced.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/iio/inkern.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index ce537b4ca6ca..71d0424383b6 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -5,9 +5,9 @@
>   */
>  #include <linux/err.h>
>  #include <linux/export.h>
> +#include <linux/mutex.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> -#include <linux/mutex.h>
>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/iio-opaque.h>
> --
> 2.40.1
>
Andy Shevchenko June 14, 2023, 9:02 a.m. UTC | #2
On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Introduce min_array() (resp max_array()) in order to get the
> minimal (resp maximum) of values present in an array.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
See a remark below.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  include/linux/minmax.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> index 396df1121bff..2cd0d34ce921 100644
> --- a/include/linux/minmax.h
> +++ b/include/linux/minmax.h
> @@ -133,6 +133,42 @@
>   */
>  #define max_t(type, x, y)      __careful_cmp((type)(x), (type)(y), >)
>
> +/*
> + * 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< ---
> + */
> +#define __minmax_array(op, array, len) ({                      \
> +       typeof(array) __array = (array);                        \
> +       typeof(len) __len = (len);                              \
> +       typeof(__array[0] + 0) __element = __array[--__len];    \

Do we need the ' + 0' part?

> +       while (__len--)                                         \
> +               __element = op(__element, __array[__len]);      \
> +       __element; })
> +
> +/**
> + * min_array - return minimum of values present in an array
> + * @array: array
> + * @len: array length
> + *
> + * Note that @len must not be zero (empty array).
> + */
> +#define min_array(array, len) __minmax_array(min, array, len)
> +
> +/**
> + * max_array - return maximum of values present in an array
> + * @array: array
> + * @len: array length
> + *
> + * Note that @len must not be zero (empty array).
> + */
> +#define max_array(array, len) __minmax_array(max, array, len)
> +
>  /**
>   * clamp_t - return a value clamped to a given range using a given type
>   * @type: the type of variable to use
> --
> 2.40.1
>
Andy Shevchenko June 14, 2023, 9:03 a.m. UTC | #3
On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> Use max_array() to get the maximum value from an array instead of a
> custom local loop.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  drivers/iio/inkern.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 71d0424383b6..8bfd91f74101 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -5,6 +5,7 @@
>   */
>  #include <linux/err.h>
>  #include <linux/export.h>
> +#include <linux/minmax.h>
>  #include <linux/mutex.h>
>  #include <linux/property.h>
>  #include <linux/slab.h>
> @@ -875,11 +876,7 @@ static int iio_channel_read_max(struct iio_channel *chan,
>                         return -EINVAL;
>                 switch (*type) {
>                 case IIO_VAL_INT:
> -                       *val = vals[--length];
> -                       while (length) {
> -                               if (vals[--length] > *val)
> -                                       *val = vals[length];
> -                       }
> +                       *val = max_array(vals, length);
>                         break;
>                 default:
>                         /* FIXME: learn about max for other iio values */
> --
> 2.40.1
>
Herve Codina June 14, 2023, 9:42 a.m. UTC | #4
Hi Andy,

On Wed, 14 Jun 2023 12:02:57 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > Introduce min_array() (resp max_array()) in order to get the
> > minimal (resp maximum) of values present in an array.  
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> See a remark below.
> 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  include/linux/minmax.h | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/include/linux/minmax.h b/include/linux/minmax.h
> > index 396df1121bff..2cd0d34ce921 100644
> > --- a/include/linux/minmax.h
> > +++ b/include/linux/minmax.h
> > @@ -133,6 +133,42 @@
> >   */
> >  #define max_t(type, x, y)      __careful_cmp((type)(x), (type)(y), >)
> >
> > +/*
> > + * 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< ---
> > + */
> > +#define __minmax_array(op, array, len) ({                      \
> > +       typeof(array) __array = (array);                        \
> > +       typeof(len) __len = (len);                              \
> > +       typeof(__array[0] + 0) __element = __array[--__len];    \  
> 
> Do we need the ' + 0' part?

Yes.

__array can be an array of const items and it is legitimate to get the
minimum value from const items.

typeof(__array[0]) keeps the const qualifier but we need to assign __element
in the loop.
One way to drop the const qualifier is to get the type from a rvalue computed
from __array[0]. This rvalue has to have the exact same type with only the const
dropped.
'__array[0] + 0' was a perfect canditate.

Regards,
Hervé

> 
> > +       while (__len--)                                         \
> > +               __element = op(__element, __array[__len]);      \
> > +       __element; })
> > +
> > +/**
> > + * min_array - return minimum of values present in an array
> > + * @array: array
> > + * @len: array length
> > + *
> > + * Note that @len must not be zero (empty array).
> > + */
> > +#define min_array(array, len) __minmax_array(min, array, len)
> > +
> > +/**
> > + * max_array - return maximum of values present in an array
> > + * @array: array
> > + * @len: array length
> > + *
> > + * Note that @len must not be zero (empty array).
> > + */
> > +#define max_array(array, len) __minmax_array(max, array, len)
> > +
> >  /**
> >   * clamp_t - return a value clamped to a given range using a given type
> >   * @type: the type of variable to use
> > --
> > 2.40.1
> >  
> 
>
Andy Shevchenko June 14, 2023, 11:51 a.m. UTC | #5
On Wed, Jun 14, 2023 at 12:42 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Wed, 14 Jun 2023 12:02:57 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:

...

> > > +       typeof(__array[0] + 0) __element = __array[--__len];    \
> >
> > Do we need the ' + 0' part?
>
> Yes.
>
> __array can be an array of const items and it is legitimate to get the
> minimum value from const items.
>
> typeof(__array[0]) keeps the const qualifier but we need to assign __element
> in the loop.
> One way to drop the const qualifier is to get the type from a rvalue computed
> from __array[0]. This rvalue has to have the exact same type with only the const
> dropped.
> '__array[0] + 0' was a perfect canditate.

Seems like this also deserves a comment. But if the series is accepted
as is, it may be done as a follow up.
Andy Shevchenko June 14, 2023, 10:05 p.m. UTC | #6
On Wed, Jun 14, 2023 at 11:34 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Wed, 14 Jun 2023 14:51:43 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Jun 14, 2023 at 12:42 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > > On Wed, 14 Jun 2023 12:02:57 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > On Wed, Jun 14, 2023 at 10:49 AM Herve Codina <herve.codina@bootlin.com> wrote:

...

> > > > > +       typeof(__array[0] + 0) __element = __array[--__len];    \
> > > >
> > > > Do we need the ' + 0' part?
> > >
> > > Yes.
> > >
> > > __array can be an array of const items and it is legitimate to get the
> > > minimum value from const items.
> > >
> > > typeof(__array[0]) keeps the const qualifier but we need to assign __element
> > > in the loop.
> > > One way to drop the const qualifier is to get the type from a rvalue computed
> > > from __array[0]. This rvalue has to have the exact same type with only the const
> > > dropped.
> > > '__array[0] + 0' was a perfect canditate.
> >
> > Seems like this also deserves a comment. But if the series is accepted
> > as is, it may be done as a follow up.
> >
>
> Finally not so simple ...
> I did some deeper tests and the macros need to be fixed.
>
> I hope this one (with comments added) is correct:
> --- 8 ---
> /*
>  * 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< ---
>  *
>  * The first typeof(&(array)[0]) is needed in order to support arrays of both
>  * 'int *buff' and 'int buf[N]' types.
>  *
>  * typeof(__array[0] + 0) used for __element is needed as the array can be an
>  * array of const items.
>  * In order to discard the const qualifier use an arithmetic operation (rvalue).


>  * This arithmetic operation discard the const but also can lead to an integer

discards

>  * promotion. For instance, a const s8 __array[0] lead to an int __element due

leads

>  * to the promotion.
>  * In this case, simple min() or max() operation fails (type mismatch).
>  * Use min_t() or max_t() (op_t parameter) enforcing the type in order to avoid
>  * the min() or max() failure.

This part perhaps can be avoided. See below.

>  */
> #define __minmax_array(op_t, array, len) ({                     \
>         typeof(&(array)[0]) __array = (array);                  \
>         typeof(len) __len = (len);                              \
>         typeof(__array[0] + 0) __element = __array[--__len];    \
>         while (__len--)                                         \
>                 __element = op_t(typeof(__array[0]), __element, __array[__len]); \

But can't we instead have typeof(+(array[0])) in the definition of __element?
There are also other possible solutions: a) _Generic() with listed
const types to move them to non-const, and b) __auto_type (which is
supported by GCC 4.9 and clang, but not in the C11 standard).

>         __element; })

...

> Can you give me your feedback on this last version ?

Sure!

> If you are ok, it will be present in the next iteration of the series.
> Otherwise, as a last ressort, I will drop the {min,max}_array() and switch
> back to the specific min/max computation in IIO inkern.c
>
> Sorry for this back and forth and this last minute detected bug.

It's good that we have some tests (perhaps you can add something to
unit test these)? Perhaps move your code to lib/test_minmax.c as KUnit
module?
Andy Shevchenko June 15, 2023, 1:51 p.m. UTC | #7
On Thu, Jun 15, 2023 at 12:35 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Thu, 15 Jun 2023 01:05:40 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> Did the job using _Generic().

Cool! Keep my tag for that version and thank you for pursuing the
implementation that works for everybody.

> This lead to:
> --- 8< ---
> /*
>  * 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_type_cases(type)              \

__unconst_integer_type_cases() ?

>         unsigned type:  (unsigned type)0,       \
>         signed type:    (signed type)0
>
>

Single blank line is enough.

> #define __unconst_typeof(x) typeof(                     \

__unconst_integer_typeof() ?

>         _Generic((x),                                   \
>                 char: (char)0,                          \
>                 __unconst_type_cases(char),             \
>                 __unconst_type_cases(short),            \
>                 __unconst_type_cases(int),              \
>                 __unconst_type_cases(long),             \
>                 __unconst_type_cases(long long),        \
>                 default: (x)))
>
> /*
>  * 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< ---
>  *
>  * 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_typeof(__array[0]) __element = __array[--__len];      \
>         while (__len--)                                                 \
>                 __element = op(__element, __array[__len]);              \
>         __element; })
>
> /**
>  * min_array - return minimum of values present in an array
>  * @array: array
>  * @len: array length
>  *
>  * Note that @len must not be zero (empty array).
>  */
> #define min_array(array, len) __minmax_array(min, array, len)
>
> /**
>  * max_array - return maximum of values present in an array
>  * @array: array
>  * @len: array length
>  *
>  * Note that @len must not be zero (empty array).
>  */
> #define max_array(array, len) __minmax_array(max, array, len)
> --- 8< ---
>
> Do you think it looks good ?

Yes!

> For, the KUnit tests, I agree, it would be nice to have something.
> I need some more substantial work to implement and run the test in KUnit
> and the first task will be learning the KUnit test system.
> I will do that but out of this series.

Thank you, it's fine with me.
David Laight June 16, 2023, 9:08 a.m. UTC | #8
From: Herve Codina
> Sent: 15 June 2023 10:35
> > ...
> >
> > > > > > > +       typeof(__array[0] + 0) __element = __array[--__len];    \
> > > > > >
> > > > > > Do we need the ' + 0' part?
> > > > >
> > > > > Yes.
> > > > >
> > > > > __array can be an array of const items and it is legitimate to get the
> > > > > minimum value from const items.
> > > > >
> > > > > typeof(__array[0]) keeps the const qualifier but we need to assign __element
> > > > > in the loop.
> > > > > One way to drop the const qualifier is to get the type from a rvalue computed
> > > > > from __array[0]. This rvalue has to have the exact same type with only the const
> > > > > dropped.
> > > > > '__array[0] + 0' was a perfect canditate.
> > > >
> > > > Seems like this also deserves a comment. But if the series is accepted
> > > > as is, it may be done as a follow up.
> > > >
> > >
> > > Finally not so simple ...
> > > I did some deeper tests and the macros need to be fixed.
> > >
> > > I hope this one (with comments added) is correct:
> > > --- 8 ---
> > > /*
> > >  * 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< ---
> > >  *
> > >  * The first typeof(&(array)[0]) is needed in order to support arrays of both
> > >  * 'int *buff' and 'int buf[N]' types.
> > >  *
> > >  * typeof(__array[0] + 0) used for __element is needed as the array can be an
> > >  * array of const items.
> > >  * In order to discard the const qualifier use an arithmetic operation (rvalue).
> >
> >
> > >  * This arithmetic operation discard the const but also can lead to an integer
> >
> > discards
> >
> > >  * promotion. For instance, a const s8 __array[0] lead to an int __element due
> >
> > leads
> >
> > >  * to the promotion.
> > >  * In this case, simple min() or max() operation fails (type mismatch).
> > >  * Use min_t() or max_t() (op_t parameter) enforcing the type in order to avoid
> > >  * the min() or max() failure.
> >
> > This part perhaps can be avoided. See below.
> >
> > >  */
> > > #define __minmax_array(op_t, array, len) ({                     \
> > >         typeof(&(array)[0]) __array = (array);                  \
> > >         typeof(len) __len = (len);                              \
> > >         typeof(__array[0] + 0) __element = __array[--__len];    \
> > >         while (__len--)                                         \
> > >                 __element = op_t(typeof(__array[0]), __element, __array[__len]); \
> >
> > But can't we instead have typeof(+(array[0])) in the definition of __element?
> > There are also other possible solutions: a) _Generic() with listed
> > const types to move them to non-const, and b) __auto_type (which is
> > supported by GCC 4.9 and clang, but not in the C11 standard).
> 
> typeof(+(array[0])) keeps the promotion.
> 
> __auto_type works with my gcc-12 but not with a gcc-5.5. Depending on the
> compiler version, it discards or keeps the const qualifier. For this reason
> I would prefer to not use it.

Just define two variables typeof(__array[0] + 0) one for an element
and one for the limit.
The just test (eg):
	if (limit > item) limit = item;
finally cast the limit back to the original type.
The promotions of char/short to signed int won't matter.
There is no need for all the type-checking in min/max.

Indeed, if min_t(type, a, b) is in anyway sane it should
expand to:
	type _a = a, _b = b;
	_a < _b ? _a : _b
without any of the checks that min() does.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Herve Codina June 16, 2023, 11:48 a.m. UTC | #9
Hi David,

On Fri, 16 Jun 2023 09:08:22 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

...

> 
> Just define two variables typeof(__array[0] + 0) one for an element
> and one for the limit.
> The just test (eg):
> 	if (limit > item) limit = item;
> finally cast the limit back to the original type.
> The promotions of char/short to signed int won't matter.
> There is no need for all the type-checking in min/max.
> 
> Indeed, if min_t(type, a, b) is in anyway sane it should
> expand to:
> 	type _a = a, _b = b;
> 	_a < _b ? _a : _b
> without any of the checks that min() does.

I finally move to use _Generic() in order to "unconstify" and avoid the
integer promotion. With this done, no extra cast is needed and min()/max()
are usable.

The patch is available in the v5 series.
  https://lore.kernel.org/linux-kernel/20230615152631.224529-8-herve.codina@bootlin.com/

Do you think the code present in the v5 series should be changed ?
If so, can you give me your feedback on the v5 series ?

Thanks for your review,
Hervé
David Laight June 16, 2023, 12:42 p.m. UTC | #10
From: Herve Codina <herve.codina@bootlin.com>
> Sent: 16 June 2023 12:49
> 
> Hi David,
> 
> On Fri, 16 Jun 2023 09:08:22 +0000
> David Laight <David.Laight@ACULAB.COM> wrote:
> 
> ...
> 
> >
> > Just define two variables typeof(__array[0] + 0) one for an element
> > and one for the limit.
> > The just test (eg):
> > 	if (limit > item) limit = item;
> > finally cast the limit back to the original type.
> > The promotions of char/short to signed int won't matter.
> > There is no need for all the type-checking in min/max.
> >
> > Indeed, if min_t(type, a, b) is in anyway sane it should
> > expand to:
> > 	type _a = a, _b = b;
> > 	_a < _b ? _a : _b
> > without any of the checks that min() does.
> 
> I finally move to use _Generic() in order to "unconstify" and avoid the
> integer promotion. With this done, no extra cast is needed and min()/max()
> are usable.
> 
> The patch is available in the v5 series.
>   https://lore.kernel.org/linux-kernel/20230615152631.224529-8-herve.codina@bootlin.com/
> 
> Do you think the code present in the v5 series should be changed ?
> If so, can you give me your feedback on the v5 series ?

It seems horribly over-complicated just to get around the perverse
over-strong type checking that min/max do just to avoid sign
extension issues.

Maybe I ought to try getting a patch accepted that just checks
  is_signed_type(typeof(x)) == is_signed_type(typeof(y))
instead of
  typeof(x) == typeof(y)
Then worry about the valid signed v unsigned cases.

Indeed, since the array index can be assumed not to have side
effects you could use __cmp(x, y, op) directly.

No one has pointed out that __element should be __bound.

	David

	

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