Re: [PATCH 1/2] iio: Fix scan mask subset check logic

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 5 May 2025 11:24:36 -0300
Fabio Estevam <festevam@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sun, May 4, 2025 at 2:04 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > Ah. I was wrong for last two channels above. The scan masks for single channel for
> > differential channels also index a bunch we aren't using in this device.
> >  
> > > +       MAX1363_CHAN_B(0, 1, d0m1, 12, bits, ev_spec, num_ev_spec),     \
> > > +       MAX1363_CHAN_B(2, 3, d2m3, 13, bits, ev_spec, num_ev_spec),     \  
> > //gap here as we aren't using 4,5  6,7 8,9, 10,11
> >  
> > > +       MAX1363_CHAN_B(1, 0, d1m0, 14, bits, ev_spec, num_ev_spec),     \  
> > Should be 18
> > https://elixir.bootlin.com/linux/v6.14.5/source/drivers/iio/adc/max1363.c#L262  
> > > +       MAX1363_CHAN_B(3, 2, d3m2, 15, bits, ev_spec, num_ev_spec),     \  
> > and 19 I believe.  
> 
> This works fine for me, thanks:
> 
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -504,10 +504,10 @@ static const struct iio_event_spec max1363_events[] = {
>         MAX1363_CHAN_U(1, _s1, 1, bits, ev_spec, num_ev_spec),          \
>         MAX1363_CHAN_U(2, _s2, 2, bits, ev_spec, num_ev_spec),          \
>         MAX1363_CHAN_U(3, _s3, 3, bits, ev_spec, num_ev_spec),          \
> -       MAX1363_CHAN_B(0, 1, d0m1, 4, bits, ev_spec, num_ev_spec),      \
> -       MAX1363_CHAN_B(2, 3, d2m3, 5, bits, ev_spec, num_ev_spec),      \
> -       MAX1363_CHAN_B(1, 0, d1m0, 6, bits, ev_spec, num_ev_spec),      \
> -       MAX1363_CHAN_B(3, 2, d3m2, 7, bits, ev_spec, num_ev_spec),      \
> +       MAX1363_CHAN_B(0, 1, d0m1, 12, bits, ev_spec, num_ev_spec),     \
> +       MAX1363_CHAN_B(2, 3, d2m3, 13, bits, ev_spec, num_ev_spec),     \
> +       MAX1363_CHAN_B(1, 0, d1m0, 18, bits, ev_spec, num_ev_spec),     \
> +       MAX1363_CHAN_B(3, 2, d3m2, 19, bits, ev_spec, num_ev_spec),     \
>         IIO_CHAN_SOFT_TIMESTAMP(8)                                      \
>         }
> 
> > Maybe we can simplify this and make it less error prone. These scan indices
> > are always the same as the enum max1363_mode entries in the 2nd or 3rd parameter.
> > We can just reuse those I think.  Only the single channel ones apply here
> > but those are the first set of entries in that enum.
> >
> > Bonus points for just dropping the parameter and using the existing addr
> > parameter for the macro as si as well.
> >
> > That is:
> > #define MAX1363_CHAN_U(num, addr, bits, ev_spec, num_ev_spec)   \
> >         {                                                               \
> >                 .type = IIO_VOLTAGE,                                    \
> >                 .indexed = 1,                                           \
> >                 .channel = num,                                         \
> >                 .address = addr,                                        \
> >                 .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> >                 .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> >                 .datasheet_name = "AIN"#num,                            \
> >                 .scan_type = {                                          \
> >                         .sign = 'u',                                    \
> >                         .realbits = bits,                               \
> >                         .storagebits = (bits > 8) ? 16 : 8,             \
> >                         .endianness = IIO_BE,                           \
> >                 },                                                      \
> >                 .scan_index = (addr),                                   \
> > The above changed from si to addr  
> 
> I tried this change on top of the previous one, and it still works.
> 
> How do you think we should proceed? Can you please submit formal patches?
That would be perfect! If you can do it by modifying the macro as that is
going to be the simplest path to ensure there are no others even better.

Jonathan

> 
> You can add:
> 
> Reported-by: Fabio Estevam <festevam@denx.de>
> Tested-by: Fabio Estevam <festevam@denx.de>
> 
> Thanks