| Message ID | 20241030185854.4015348-1-quic_jprakash@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for QCOM SPMI PMIC5 Gen3 ADC | expand |
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 >
Hi Krzysztof, On 10/31/2024 1:06 PM, Krzysztof Kozlowski wrote: > On Thu, Oct 31, 2024 at 12:28:50AM +0530, Jishnu Prakash wrote: >> PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2, >> with all SW communication to ADC going through PMK8550 which >> communicates with other PMICs through PBS. The major difference is >> that the register interface used here is that of an SDAM present on >> PMK8550, rather than a dedicated ADC peripheral. There may be more than one >> SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may >> be used for either immediate reads (same functionality as previous PMIC5 and >> PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5 >> Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality >> combined into the same driver. >> >> Patch 1 is a cleanup, to move the QCOM ADC dt-bindings files from >> dt-bindings/iio to dt-bindings/iio/adc folder, as they are >> specifically for ADC devices. It also fixes all compilation errors >> with this change in driver and devicetree files and similar errors >> in documentation for dtbinding check. >> >> Patch 2 adds bindings for ADC5 Gen3 peripheral. >> >> Patch 3 adds the main driver for ADC5 Gen3. >> >> Patch 4 adds the auxiliary thermal driver which supports the ADC_TM >> functionality of ADC5 Gen3. >> >> Changes since v3: >> - Updated files affected by adc file path change in /arch/arm folder, >> which were missed earlier. > > I don't think this was tested afterwards... If you are referring to the error found by the bot in my V4 patches 1 and 2, I think the error is invalid. In both cases, this is the error: fatal error: dt-bindings/iio/adc/qcom,spmi-vadc.h: No such file or directory But this file is added in patch 1, through a renaming: rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (100%) I have replied to Rob on the patch 1 mail pointing this out, but I did not get any reply yet. I have also tried updating dtschema and running 'make dt_binding_check' again myself and I did not get this error. If this was some rare error on the bot's side, which may not always happen, I'm thinking of pushing the same patch again in the next patch series, as I think there is nothing to fix from my side. Please let me know if you have any other suggestions. Thanks, Jishnu > > Best regards, > Krzysztof >
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
PMIC5 Gen3 has a similar ADC architecture to that on PMIC5 Gen2, with all SW communication to ADC going through PMK8550 which communicates with other PMICs through PBS. The major difference is that the register interface used here is that of an SDAM present on PMK8550, rather than a dedicated ADC peripheral. There may be more than one SDAM used for ADC5 Gen3. Each ADC SDAM has eight channels, each of which may be used for either immediate reads (same functionality as previous PMIC5 and PMIC5 Gen2 ADC peripherals) or recurring measurements (same as PMIC5 and PMIC5 Gen2 ADC_TM functionality). In this case, we have VADC and ADC_TM functionality combined into the same driver. Patch 1 is a cleanup, to move the QCOM ADC dt-bindings files from dt-bindings/iio to dt-bindings/iio/adc folder, as they are specifically for ADC devices. It also fixes all compilation errors with this change in driver and devicetree files and similar errors in documentation for dtbinding check. Patch 2 adds bindings for ADC5 Gen3 peripheral. Patch 3 adds the main driver for ADC5 Gen3. Patch 4 adds the auxiliary thermal driver which supports the ADC_TM functionality of ADC5 Gen3. Changes since v3: - Updated files affected by adc file path change in /arch/arm folder, which were missed earlier. - Added ADC5 Gen3 documentation changes in existing qcom,spmi-vadc.yaml file instead of adding separate file and addressed reviewer comments for all bindings. - Addressed review comments in driver patch. Split out TM functionality into auxiliary driver in separate patch and added required changes in main driver. - Link to v3: https://lore.kernel.org/all/20231231171237.3322376-1-quic_jprakash@quicinc.com/ Changes since v2: - Reordered patches to keep cleanup change for ADC files first. - Moved ADC5 Gen3 documentation into a separate file Changes since v1: - Dropped patches 1-5 for changing 'ADC7' peripheral name to 'ADC5 Gen2'. - Addressed reviewer comments for binding and driver patches for ADC5 Gen3. - Combined patches 8-11 into a single patch as requested by reviewers to make the change clearer and made all fixes required in same patch. Jishnu Prakash (4): dt-bindings: iio/adc: Move QCOM ADC bindings to iio/adc folder dt-bindings: iio: adc: Add support for QCOM PMIC5 Gen3 ADC iio: adc: Add support for QCOM PMIC5 Gen3 ADC thermal: qcom: add support for PMIC5 Gen3 ADC thermal monitoring .../bindings/iio/adc/qcom,spmi-vadc.yaml | 226 +++++- .../bindings/mfd/qcom,spmi-pmic.yaml | 2 +- .../bindings/thermal/qcom-spmi-adc-tm-hc.yaml | 2 +- .../bindings/thermal/qcom-spmi-adc-tm5.yaml | 6 +- arch/arm/boot/dts/qcom/pm8226.dtsi | 2 +- arch/arm/boot/dts/qcom/pm8941.dtsi | 2 +- arch/arm/boot/dts/qcom/pma8084.dtsi | 2 +- arch/arm/boot/dts/qcom/pmx55.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm4125.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm6125.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm6150.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm6150l.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm660.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm660l.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm7250b.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8150.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8150b.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8150l.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8916.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8950.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8953.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8994.dtsi | 2 +- arch/arm64/boot/dts/qcom/pm8998.dtsi | 2 +- arch/arm64/boot/dts/qcom/pmi632.dtsi | 2 +- arch/arm64/boot/dts/qcom/pmi8950.dtsi | 2 +- arch/arm64/boot/dts/qcom/pmm8155au_1.dtsi | 2 +- arch/arm64/boot/dts/qcom/pmp8074.dtsi | 2 +- arch/arm64/boot/dts/qcom/pms405.dtsi | 2 +- .../boot/dts/qcom/qcm6490-fairphone-fp5.dts | 4 +- .../boot/dts/qcom/qcm6490-shift-otter.dts | 4 +- arch/arm64/boot/dts/qcom/sc7280-idp.dts | 2 +- arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 2 +- arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi | 4 +- arch/arm64/boot/dts/qcom/sc8180x-pmics.dtsi | 2 +- .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 2 +- arch/arm64/boot/dts/qcom/sc8280xp-pmics.dtsi | 6 +- .../boot/dts/qcom/sm7225-fairphone-fp4.dts | 2 +- .../boot/dts/qcom/sm7325-nothing-spacewar.dts | 6 +- arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 8 +- drivers/iio/adc/Kconfig | 25 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/qcom-spmi-adc5-gen3.c | 724 ++++++++++++++++++ drivers/iio/adc/qcom-spmi-adc5.c | 2 +- drivers/iio/adc/qcom-spmi-vadc.c | 2 +- drivers/thermal/qcom/Kconfig | 11 + drivers/thermal/qcom/Makefile | 1 + drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c | 489 ++++++++++++ .../iio/adc/qcom,spmi-adc5-gen3-pm8550.h | 46 ++ .../iio/adc/qcom,spmi-adc5-gen3-pm8550b.h | 85 ++ .../iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h | 22 + .../iio/adc/qcom,spmi-adc5-gen3-pmk8550.h | 52 ++ .../iio/{ => adc}/qcom,spmi-adc7-pm7325.h | 2 +- .../iio/{ => adc}/qcom,spmi-adc7-pm8350.h | 2 +- .../iio/{ => adc}/qcom,spmi-adc7-pm8350b.h | 2 +- .../iio/{ => adc}/qcom,spmi-adc7-pmk8350.h | 2 +- .../iio/{ => adc}/qcom,spmi-adc7-pmr735a.h | 2 +- .../iio/{ => adc}/qcom,spmi-adc7-pmr735b.h | 2 +- .../iio/{ => adc}/qcom,spmi-adc7-smb139x.h | 2 +- .../iio/{ => adc}/qcom,spmi-vadc.h | 81 ++ include/linux/iio/adc/qcom-adc5-gen3-common.h | 233 ++++++ 60 files changed, 2032 insertions(+), 82 deletions(-) create mode 100644 drivers/iio/adc/qcom-spmi-adc5-gen3.c create mode 100644 drivers/thermal/qcom/qcom-spmi-adc-tm5-gen3.c create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550.h create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550b.h create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pm8550vx.h create mode 100644 include/dt-bindings/iio/adc/qcom,spmi-adc5-gen3-pmk8550.h rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm7325.h (98%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350.h (98%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pm8350b.h (99%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmk8350.h (97%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735a.h (95%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-pmr735b.h (95%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-adc7-smb139x.h (93%) rename include/dt-bindings/iio/{ => adc}/qcom,spmi-vadc.h (77%) create mode 100644 include/linux/iio/adc/qcom-adc5-gen3-common.h base-commit: 6fb2fa9805c501d9ade047fc511961f3273cdcb5