Re: [PATCH V4 3/4] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

[Krzysztof Kozlowski]

On 30/10/2024 19:58, Jishnu Prakash wrote:
> +
> +static int adc5_gen3_read(struct adc5_device_data *adc, unsigned int sdam_index,
> +			  u16 offset, u8 *data, int len)
> +{
> +	return regmap_bulk_read(adc->regmap, adc->base[sdam_index].base_addr + offset, data, len);
> +}
> +
> +static int adc5_gen3_write(struct adc5_device_data *adc, unsigned int sdam_index,
> +			   u16 offset, u8 *data, int len)
> +{
> +	return regmap_bulk_write(adc->regmap, adc->base[sdam_index].base_addr + offset, data, len);
> +}
> +
> +/*
> + * Worst case delay from PBS in readying handshake bit
> + * can be up to 15ms, when PBS is busy running other
> + * simultaneous transactions, while in the best case, it is
> + * already ready at this point. Assigning polling delay and
> + * retry count accordingly.
> + */
> +
> +#define ADC5_GEN3_HS_DELAY_MIN_US		100
> +#define ADC5_GEN3_HS_DELAY_MAX_US		110
> +#define ADC5_GEN3_HS_RETRY_COUNT		150
> +
> +static int adc5_gen3_poll_wait_hs(struct adc5_device_data *adc,
> +				  unsigned int sdam_index)
> +{
> +	u8 conv_req = ADC5_GEN3_CONV_REQ_REQ;
> +	int ret, count;
> +	u8 status = 0;
> +
> +	for (count = 0; count < ADC5_GEN3_HS_RETRY_COUNT; count++) {
> +		ret = adc5_gen3_read(adc, sdam_index, ADC5_GEN3_HS, &status, 1);
> +		if (ret)
> +			return ret;
> +
> +		if (status == ADC5_GEN3_HS_READY) {
> +			ret = adc5_gen3_read(adc, sdam_index, ADC5_GEN3_CONV_REQ,
> +					     &conv_req, 1);
> +			if (ret)
> +				return ret;
> +
> +			if (!conv_req)
> +				return 0;
> +		}
> +
> +		usleep_range(ADC5_GEN3_HS_DELAY_MIN_US, ADC5_GEN3_HS_DELAY_MAX_US);
> +	}
> +
> +	pr_err("Setting HS ready bit timed out, sdam_index:%d, status:%#x\n", sdam_index, status);
> +	return -ETIMEDOUT;
> +}
> +
> +static void adc5_gen3_update_dig_param(struct adc5_channel_common_prop *prop, u8 *data)
> +{
> +	/* Update calibration select and decimation ratio select */
> +	*data &= ~(ADC5_GEN3_DIG_PARAM_CAL_SEL_MASK | ADC5_GEN3_DIG_PARAM_DEC_RATIO_SEL_MASK);
> +	*data |= FIELD_PREP(ADC5_GEN3_DIG_PARAM_CAL_SEL_MASK, prop->cal_method);
> +	*data |= FIELD_PREP(ADC5_GEN3_DIG_PARAM_DEC_RATIO_SEL_MASK, prop->decimation);
> +}
> +
> +static int adc5_gen3_status_clear(struct adc5_device_data *adc,
> +				  int sdam_index, u16 offset, u8 *val, int len)
> +{

Wait, what? Why are you defining functions in header causing multiple
copies of them? And even if: why this is not inline? But regardless:
this is a strong NAK from me.

Best regards,
Krzysztof