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

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: <aab228cf-d209-48a8-8daf-99df75f8b460@quicinc.com>
Date: Wed, 13 Nov 2024 19:36:29 +0530
From: Jishnu Prakash <quic_jprakash@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>, <jic23@...nel.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <conor+dt@...nel.org>, <agross@...nel.org>, <andersson@...nel.org>,
        <dmitry.baryshkov@...aro.org>, <konrad.dybcio@...aro.org>,
        <daniel.lezcano@...aro.org>, <sboyd@...nel.org>,
        <quic_subbaram@...cinc.com>, <quic_collinsd@...cinc.com>,
        <quic_amelende@...cinc.com>, <quic_kamalw@...cinc.com>,
        <amitk@...nel.org>
CC: <lee@...nel.org>, <rafael@...nel.org>, <rui.zhang@...el.com>,
        <lukasz.luba@....com>, <lars@...afoo.de>, <quic_skakitap@...cinc.com>,
        <neil.armstrong@...aro.org>, <devicetree@...r.kernel.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-iio@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-pm@...r.kernel.org>,
        <cros-qcom-dts-watchers@...omium.org>
Subject: Re: [PATCH V4 3/4] iio: adc: Add support for QCOM PMIC5 Gen3 ADC

Hi Krzysztof,

On 10/31/2024 4:33 PM, Krzysztof Kozlowski wrote:
> 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.

This was meant to hold macros and some helper functions used in both main and auxiliary driver files.
I see what you mean - I'll move the function definitions into a new .c file and mark them inline.

Thanks,
Jishnu



> 
> Best regards,
> Krzysztof
>