linux-kernel - RE: [PATCH v3 6/6] 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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <CY4PR03MB33996900AAB90A050375CBB39B4F2@CY4PR03MB3399.namprd03.prod.outlook.com>
Date: Fri, 25 Oct 2024 11:35:06 +0000
From: "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
To: David Lechner <dlechner@...libre.com>
CC: Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
        Conor
 Dooley <conor+dt@...nel.org>, "Sa, Nuno" <Nuno.Sa@...log.com>,
        "Bogdan,
 Dragos" <Dragos.Bogdan@...log.com>,
        "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 v3 6/6] iio: adc: ad4851: add ad485x driver

> 
> > +static int ad4851_set_oversampling_ratio(struct ad4851_state *st,
> > +					 const struct iio_chan_spec *chan,
> > +					 unsigned int osr)
> > +{
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	if (osr == 1) {
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > +					 AD4851_OS_EN_MSK, 0);
> > +		if (ret)
> > +			return ret;
> > +	} else {
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > +					 AD4851_OS_EN_MSK,
> AD4851_OS_EN_MSK);
> > +		if (ret)
> > +			return ret;
> 
> regmap_clear_bits() and regmap_set_bits() would make this a bit
> less verbose and consistent with the effort started in [1].
> 
> [1]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20240617-review-v3-0-
> 88d1338c4cca@...libre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VY
> nCyeikpTumyjO0qDTn7eF7Fd-
> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rv3yGMDWs$
>
Will do in v5.
> 
> > +
> > +		val = ad4851_osr_to_regval(osr);
> > +		if (val < 0)
> > +			return -EINVAL;
> > +
> > +		ret = regmap_update_bits(st->regmap,
> AD4851_REG_OVERSAMPLE,
> > +					 AD4851_OS_RATIO_MSK, val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	switch (chan->scan_type.realbits) {
> > +	case 20:
> > +		switch (osr) {
> > +		case 1:
> > +			val = 20;
> > +			break;
> > +		default:
> > +			val = 24;
> > +			break;
> > +		}
> > +		break;
> > +	case 16:
> > +		val = 16;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = iio_backend_data_size_set(st->back, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(st->regmap, AD4851_REG_PACKET,
> > +				  AD4851_PACKET_FORMAT_MASK, (osr == 1)
> ? 0 : 1);
> > +}
> > +
> > +static int ad4851_get_oversampling_ratio(struct ad4851_state *st,
> unsigned int *val)
> > +{
> > +	unsigned int osr;
> > +	int ret;
> > +
> > +	ret = regmap_read(st->regmap, AD4851_REG_OVERSAMPLE, &osr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!FIELD_GET(AD4851_OS_EN_MSK, osr))
> > +		*val = 1;
> > +	else
> > +		*val =
> ad4851_oversampling_ratios[FIELD_GET(AD4851_OS_RATIO_MSK, osr)];
> 
> Why is 1 not in the table?
Because there is no 1 in the OS_RATIO table from datasheet. 1 means you disable the OS via OS_EN bit.
> 
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int ad4851_setup(struct ad4851_state *st)
> > +{
> > +	unsigned int product_id;
> > +	int ret;
> > +
> 
> Would be nice to do a hard reset here if possible using st->pd_gpio
> (datasheet says to cycle this twice and then wait 1 ms).

Sure, will do in v5.

> > +	ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(st->regmap,
> AD4851_REG_INTERFACE_CONFIG_A,
> > +			   AD4851_SW_RESET);
> > +	if (ret)
> > +		return ret;
> > +
...
> 
> > +	.scan_index = index,						\
> > +	.scan_type = {							\
> > +		.sign = 's',						\
> > +		.realbits = real,					\
> > +		.storagebits = storage,					\
> 
> Since enabling oversampling can change realbits, this driver will likely
> need to implement scan_type_ext so that userspace is aware of the
> difference when oversampling is enabled. (Adding support for oversampling
> could always be a followup patch instead of trying to do everything
> all at once.)

Will do in v5.

> 
> See the ad7380 driver as an example of how to impelemt this. [2]
> 
> [2]: https://urldefense.com/v3/__https://lore.kernel.org/linux-
> iio/20240530-iio-add-support-for-multiple-scan-types-v3-5-
> cbc4acea2cfa@...libre.com/__;!!A3Ni8CS0y2Y!4LS7UI11XqIHRgT3ckx76VYn
> CyeikpTumyjO0qDTn7eF7Fd-
> jFFL8yqpYcMAxP_u3VC09bfIAB7gW_rvGoM_sEA$
> 
> Also, I would expect the .sign value to depend on how the
> input is being used. If it is differential or single-ended
> bipolar, then it is signed, but if it is signle-ended unipoloar
> then it is unsiged.
> 
> Typically, this is coming from the devicetree because it
> depends on what is wired up to the input.

This topic is mentioned in the cover letter, maybe not argued enough there.
Yes, the go-to approach is to specify the unipolar/bipolar configuration in the devicetree.
But this is a request from the actual users of the driver: to have the softspan fully
controlled from userspace. That's why the offset and scale implementations were added.
Both these attributes are influencing the softspan.

> > +	},								\
> > +}
> 
> ...