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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <c424cedb-5b45-43c0-897b-dec83918d658@baylibre.com>
Date: Mon, 20 Jan 2025 11:37:12 -0600
From: David Lechner <dlechner@...libre.com>
To: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>,
 "jic23@...nel.org" <jic23@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
 "conor+dt@...nel.org" <conor+dt@...nel.org>,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>
Subject: Re: [PATCH v10 8/8] iio: adc: ad4851: add ad485x driver

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;