[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a45a6b73-3d46-4293-ae55-8b901087680b@linaro.org>
Date: Tue, 19 Nov 2024 10:04:02 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Jishnu Prakash <quic_jprakash@...cinc.com>, 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
On 13/11/2024 15:06, Jishnu Prakash wrote:
> 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.
This is a very odd coding style. Look around other header files: do you
see such patterns? No, because it leads to potential issues I mentioned
above..
Best regards,
Krzysztof
Powered by blists - more mailing lists