Re: [PATCH v10 8/8] iio: adc: ad4851: add ad485x driver

["Nuno Sá" <noname.nuno@gmail.com>]

On Mon, 2025-01-20 at 11:37 -0600, David Lechner wrote:
> On 1/20/25 6:37 AM, Miclaus, Antoniu wrote:
> > > > +		}
> > > > +		channels++;
> > > > +
> > > > +		st->bipolar_ch[reg] = fwnode_property_read_bool(child,
> > > "bipolar");
> > > > +
> > > > +		if (st->bipolar_ch[reg]) {
> > > > +			channels->scan_type.sign = 's';
> > > > +		} else {
> > > > +			ret = regmap_write(st->regmap,
> > > AD4851_REG_CHX_SOFTSPAN(reg),
> > > > +					   AD4851_SOFTSPAN_0V_40V);
> > > > +			if (ret)
> > > > +				return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	*ad4851_channels = channels;
> > > 
> > > At this point, channels is pointing to memory we didn't allocate (because
> > > of
> > > channels++). As in the previous review, I suggest we just get rid of the
> > > output
> > > parameter since indio_dev->channels already has the correct pointer.
> > > 
> > > It's less chance for mistakes like this and avoids needing to provide an
> > > unused
> > > arg in ad4857_parse_channels().
> > 
> > Hmm, how can I then do the assignments in `ad4858_parse_channels` ?
> > 
> > drivers/iio/adc/ad4851.c:1055:42: error: assignment of member
> > ‘has_ext_scan_type’ in read-only object
> >  1055 |   indio_dev->channels->has_ext_scan_type = 1;
> >       |                                          ^
> > drivers/iio/adc/ad4851.c:1057:39: error: assignment of member
> > ‘ext_scan_type’ in read-only object
> >  1057 |    indio_dev->channels->ext_scan_type = ad4851_scan_type_20_b;
> >       |                                       ^
> > drivers/iio/adc/ad4851.c:1058:43: error: assignment of member
> > ‘num_ext_scan_type’ in read-only object
> >  1058 |    indio_dev->channels->num_ext_scan_type =
> > ARRAY_SIZE(ad4851_scan_type_20_b);
> >       |                                           ^
> > drivers/iio/adc/ad4851.c:1061:39: error: assignment of member
> > ‘ext_scan_type’ in read-only object
> >  1061 |    indio_dev->channels->ext_scan_type = ad4851_scan_type_20_u;
> >       |                                       ^
> > drivers/iio/adc/ad4851.c:1062:43: error: assignment of member
> > ‘num_ext_scan_type’ in read-only object
> >  1062 |    indio_dev->channels->num_ext_scan_type =
> > ARRAY_SIZE(ad4851_scan_type_20_u);
> >       |                                           ^
> 
> I would be tempted to just not have a second loop of
> 
> 	device_for_each_child_node_scoped(dev, child)
> 
> in ad4858_parse_channels() and instead do everything in
> ad4851_parse_channels()
> and just pass a boolean parameter to conditionally handle the difference
> between the two types of chips.
> 
> Or you use a cast to remove the const qualifier.
> 
> 	ad4851_channels = (struct iio_chan_spec *)indio_dev->channels;

Hmm a bit nasty IMO :).

But up to you both anyways...

- Nuno Sá