| Message ID | 1586942266-21480-1-git-send-email-jprakash@codeaurora.org |
|---|---|
| Headers | show |
| Series | iio: adc: Add support for QCOM SPMI PMIC7 ADC | expand |
On Wed, 15 Apr 2020 14:47:44 +0530, Jishnu Prakash wrote: > Convert the adc bindings from .txt to .yaml format. > > Signed-off-by: Jishnu Prakash <jprakash@codeaurora.org> > --- > .../devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 173 ------------- > .../bindings/iio/adc/qcom,spmi-vadc.yaml | 288 +++++++++++++++++++++ > 2 files changed, 288 insertions(+), 173 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.example.dt.yaml: adc@3100: 'adc-chan@0x39', 'adc-chan@0x9', 'adc-chan@0xa', 'adc-chan@0xe', 'adc-chan@0xf' do not match any of the regexes: '.*-names$', '.*-supply$', '^#.*-cells$', '^#[a-zA-Z0-9,+\\-._]{0,63}$', '^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}$', '^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}@[0-9a-fA-F]+(,[0-9a-fA-F]+)*$', '^__.*__$', 'pinctrl-[0-9]+' See https://patchwork.ozlabs.org/patch/1271025 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
On Thu, Apr 16, 2020 at 1:48 AM Jishnu Prakash <jprakash@codeaurora.org> wrote: > > The ADC architecture on PMIC7 is changed as compared to PMIC5. The > major change from PMIC5 is that all SW communication to ADC goes through > PMK8350, which communicates with other PMICs through PBS when the ADC > on PMK8350 works in master mode. The SID register is used to identify the > PMICs with which the PBS needs to communicate. Add support for the same. Please, split pr_*() -> dev_*() to separate patch. Also think about other logical pieces you may split out. ... > +static const struct adc5_data adc7_data_pmic; Global variable? Hmm... ... > + int ret; > + u8 conv_req = 0, buf[4]; > + > + ret = adc5_masked_write(adc, ADC_APP_SID, ADC_APP_SID_MASK, prop->sid); > + if (ret) > + return ret; > + > + ret = adc5_read(adc, ADC5_USR_DIG_PARAM, buf, sizeof(buf)); > + if (ret < 0) Does > 0 have a meaning? > + return ret; > + > + /* Digital param selection */ > + adc5_update_dig_param(adc, prop, &buf[0]); > + > + /* Update fast average sample value */ > + buf[1] &= 0xff & ~ADC5_USR_FAST_AVG_CTL_SAMPLES_MASK; What the point of 0xff & part? > + buf[1] |= prop->avg_samples; > + > + /* Select ADC channel */ > + buf[2] = prop->channel; > + > + /* Select HW settle delay for channel */ > + buf[3] &= 0xff & ~ADC5_USR_HW_SETTLE_DELAY_MASK; Ditto. > + buf[3] |= prop->hw_settle_time; ... > +static int adc7_do_conversion(struct adc5_chip *adc, > + struct adc5_channel_prop *prop, > + struct iio_chan_spec const *chan, > + u16 *data_volt, u16 *data_cur) > +{ > + int ret; > + u8 status = 0; Redundant assignment. > + mutex_lock(&adc->lock); > + > + ret = adc7_configure(adc, prop); > + if (ret) { > + dev_err(adc->dev, "ADC configure failed with %d\n", ret); > + goto unlock; > + } > + > + /* No support for polling mode at present*/ Missed space > + wait_for_completion_timeout(&adc->complete, ADC7_CONV_TIMEOUT); > + > + ret = adc5_read(adc, ADC5_USR_STATUS1, &status, 1); > + if (ret < 0) Remove all ' < 0' where it is not needed. > + goto unlock; > + > + if (status & ADC5_USR_STATUS1_CONV_FAULT) { > + dev_err(adc->dev, "Unexpected conversion fault\n"); > + ret = -EIO; > + goto unlock; > + } > + > + ret = adc5_read_voltage_data(adc, data_volt); > + > +unlock: > + mutex_unlock(&adc->lock); ... > + for (i = 0; i < adc->nchannels; i++) { > + v_channel = (adc->chan_props[i].sid << ADC_CHANNEL_OFFSET | > + adc->chan_props[i].channel); Too many parentheses or they are in a wrong position. I don't remember operator precedence by heart. > + if (v_channel == iiospec->args[0]) > + return i; > + } ... > + /* > + * Value read from "reg" is virtual channel number > + * virtual channel number = (sid << 8 | channel number). Too many parentheses. And perhaps formulas better to have on a separate line. > + */ ... > +static const struct vadc_map_pt adcmap7_100k[] = { > + { 4250657, -40960 }, > + { 3962085, -39936 }, > + { 419448, -3072 }, > + { 396851, -2048 }, > + { 375597, -1024 }, > + { 355598, 0 }, > + { 336775, 1024 }, > + { 319052, 2048 }, > + { 302359, 3072 }, > + { 2560, 128000 }, > + { 2489, 129024 }, > + { 2420, 130048 } > +}; I'm wondering why you have second column here? Can't you derive it from index? Seems to me pretty easy calculus. ... > + int ret, result = 0; Redundant assignment. > + if (adc_code >= RATIO_MAX_ADC7) > + return -EINVAL; > + > + /* (ADC code * R_PULLUP (100Kohm)) / (full_scale_code - ADC code)*/ > + resistance *= R_PU_100K; > + resistance = div64_s64(resistance, RATIO_MAX_ADC7 - adc_code); > + > + ret = qcom_vadc_map_voltage_temp(adcmap7_100k, > + ARRAY_SIZE(adcmap7_100k), > + resistance, &result); > + if (ret) > + return ret; > + > + *result_mdec = result; ... > + for (i = 0; i < ARRAY_SIZE(adcmap7_die_temp); i++) > + if (adcmap7_die_temp[i].x > voltage) > + break; > + > + if (i == 0) { > + *result_mdec = DIE_TEMP_ADC7_SCALE_1; > + } else if (i == ARRAY_SIZE(adcmap7_die_temp)) { > + *result_mdec = DIE_TEMP_ADC7_MAX; I think you can done these checks before loop, and return immediately. > + } else { > + vtemp0 = adcmap7_die_temp[i - 1].x; > + voltage = voltage - vtemp0; > + temp = div64_s64(voltage * DIE_TEMP_ADC7_SCALE_FACTOR, > + adcmap7_die_temp[i - 1].y); > + temp += DIE_TEMP_ADC7_SCALE_1 + (DIE_TEMP_ADC7_SCALE_2 * (i - 1)); > + *result_mdec = temp; > + }
On Wed, 15 Apr 2020 14:47:44 +0530 Jishnu Prakash <jprakash@codeaurora.org> wrote: > Convert the adc bindings from .txt to .yaml format. > I read patch 2 before this one for some reason but same question applies here Given we are now enforcing a lot of the values explicitly are we better off dropping the text description of that. It looks to me like a potential place to get out of sync given the information is a bit further down. > Signed-off-by: Jishnu Prakash <jprakash@codeaurora.org> > --- > .../devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 173 ------------- > .../bindings/iio/adc/qcom,spmi-vadc.yaml | 288 +++++++++++++++++++++ > 2 files changed, 288 insertions(+), 173 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > deleted file mode 100644 > index c878768..0000000 > --- a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt > +++ /dev/null > @@ -1,173 +0,0 @@ > -Qualcomm's SPMI PMIC ADC > - > -- SPMI PMIC voltage ADC (VADC) provides interface to clients to read > - voltage. The VADC is a 15-bit sigma-delta ADC. > -- SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read > - voltage. The VADC is a 16-bit sigma-delta ADC. > - > -VADC node: > - > -- compatible: > - Usage: required > - Value type: <string> > - Definition: Should contain "qcom,spmi-vadc". > - Should contain "qcom,spmi-adc5" for PMIC5 ADC driver. > - Should contain "qcom,spmi-adc-rev2" for PMIC rev2 ADC driver. > - Should contain "qcom,pms405-adc" for PMS405 PMIC > - > -- reg: > - Usage: required > - Value type: <prop-encoded-array> > - Definition: VADC base address in the SPMI PMIC register map. > - > -- #address-cells: > - Usage: required > - Value type: <u32> > - Definition: Must be one. Child node 'reg' property should define ADC > - channel number. > - > -- #size-cells: > - Usage: required > - Value type: <u32> > - Definition: Must be zero. > - > -- #io-channel-cells: > - Usage: required > - Value type: <u32> > - Definition: Must be one. For details about IIO bindings see: > - Documentation/devicetree/bindings/iio/iio-bindings.txt > - > -- interrupts: > - Usage: optional > - Value type: <prop-encoded-array> > - Definition: End of conversion interrupt. > - > -Channel node properties: > - > -- reg: > - Usage: required > - Value type: <u32> > - Definition: ADC channel number. > - See include/dt-bindings/iio/qcom,spmi-vadc.h > - > -- label: > - Usage: required for "qcom,spmi-adc5" and "qcom,spmi-adc-rev2" > - Value type: <empty> > - Definition: ADC input of the platform as seen in the schematics. > - For thermistor inputs connected to generic AMUX or GPIO inputs > - these can vary across platform for the same pins. Hence select > - the platform schematics name for this channel. > - > -- qcom,decimation: > - Usage: optional > - Value type: <u32> > - Definition: This parameter is used to decrease ADC sampling rate. > - Quicker measurements can be made by reducing decimation ratio. > - - For compatible property "qcom,spmi-vadc", valid values are > - 512, 1024, 2048, 4096. If property is not found, default value > - of 512 will be used. > - - For compatible property "qcom,spmi-adc5", valid values are 250, 420 > - and 840. If property is not found, default value of 840 is used. > - - For compatible property "qcom,spmi-adc-rev2", valid values are 256, > - 512 and 1024. If property is not present, default value is 1024. > - > -- qcom,pre-scaling: > - Usage: optional > - Value type: <u32 array> > - Definition: Used for scaling the channel input signal before the signal is > - fed to VADC. The configuration for this node is to know the > - pre-determined ratio and use it for post scaling. Select one from > - the following options. > - <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10> > - If property is not found default value depending on chip will be used. > - > -- qcom,ratiometric: > - Usage: optional > - Value type: <empty> > - Definition: Channel calibration type. > - - For compatible property "qcom,spmi-vadc", if this property is > - specified VADC will use the VDD reference (1.8V) and GND for > - channel calibration. If property is not found, channel will be > - calibrated with 0.625V and 1.25V reference channels, also > - known as absolute calibration. > - - For compatible property "qcom,spmi-adc5" and "qcom,spmi-adc-rev2", > - if this property is specified VADC will use the VDD reference > - (1.875V) and GND for channel calibration. If property is not found, > - channel will be calibrated with 0V and 1.25V reference channels, > - also known as absolute calibration. > - > -- qcom,hw-settle-time: > - Usage: optional > - Value type: <u32> > - Definition: Time between AMUX getting configured and the ADC starting > - conversion. The 'hw_settle_time' is an index used from valid values > - and programmed in hardware to achieve the hardware settling delay. > - - For compatible property "qcom,spmi-vadc" and "qcom,spmi-adc-rev2", > - Delay = 100us * (hw_settle_time) for hw_settle_time < 11, > - and 2ms * (hw_settle_time - 10) otherwise. > - Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800, > - 900 us and 1, 2, 4, 6, 8, 10 ms. > - If property is not found, channel will use 0us. > - - For compatible property "qcom,spmi-adc5", delay = 15us for > - value 0, 100us * (value) for values < 11, > - and 2ms * (value - 10) otherwise. > - Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800, > - 900 us and 1, 2, 4, 6, 8, 10 ms > - Certain controller digital versions have valid values of > - 15, 100, 200, 300, 400, 500, 600, 700, 1, 2, 4, 8, 16, 32, 64, 128 ms > - If property is not found, channel will use 15us. > - > -- qcom,avg-samples: > - Usage: optional > - Value type: <u32> > - Definition: Number of samples to be used for measurement. > - Averaging provides the option to obtain a single measurement > - from the ADC that is an average of multiple samples. The value > - selected is 2^(value). > - - For compatible property "qcom,spmi-vadc", valid values > - are: 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 > - If property is not found, 1 sample will be used. > - - For compatible property "qcom,spmi-adc5" and "qcom,spmi-adc-rev2", > - valid values are: 1, 2, 4, 8, 16 > - If property is not found, 1 sample will be used. > - > -NOTE: > - > -For compatible property "qcom,spmi-vadc" following channels, also known as > -reference point channels, are used for result calibration and their channel > -configuration nodes should be defined: > -VADC_REF_625MV and/or VADC_SPARE1(based on PMIC version) VADC_REF_1250MV, > -VADC_GND_REF and VADC_VDD_VADC. > - > -Example: > - > -#include <dt-bindings/iio/qcom,spmi-vadc.h> > -#include <linux/irq.h> > -/* ... */ > - > - /* VADC node */ > - pmic_vadc: vadc@3100 { > - compatible = "qcom,spmi-vadc"; > - reg = <0x3100>; > - interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>; > - #address-cells = <1>; > - #size-cells = <0>; > - #io-channel-cells = <1>; > - io-channel-ranges; > - > - /* Channel node */ > - adc-chan@VADC_LR_MUX10_USB_ID { > - reg = <VADC_LR_MUX10_USB_ID>; > - qcom,decimation = <512>; > - qcom,ratiometric; > - qcom,hw-settle-time = <200>; > - qcom,avg-samples = <1>; > - qcom,pre-scaling = <1 3>; > - }; > - }; > - > - /* IIO client node */ > - usb { > - io-channels = <&pmic_vadc VADC_LR_MUX10_USB_ID>; > - io-channel-names = "vadc"; > - }; > diff --git a/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > new file mode 100644 > index 0000000..8273981 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml > @@ -0,0 +1,288 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/qcom,spmi-vadc.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm's SPMI PMIC ADC > + > +maintainers: > + - Andy Gross <agross@kernel.org> > + - Bjorn Andersson <bjorn.andersson@linaro.org> > + > +description: | > + SPMI PMIC voltage ADC (VADC) provides interface to clients to read > + voltage. The VADC is a 15-bit sigma-delta ADC. > + SPMI PMIC5 voltage ADC (ADC) provides interface to clients to read > + voltage. The VADC is a 16-bit sigma-delta ADC. > + > +properties: > + compatible: > + oneOf: > + - items: > + - const: qcom,pms405-adc > + - const: qcom,spmi-adc-rev2 > + > + - items: > + - enum: > + - qcom,spmi-vadc > + - qcom,spmi-adc5 > + - qcom,spmi-adc-rev2 > + > + reg: > + description: VADC base address in the SPMI PMIC register map > + maxItems: 1 > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + '#io-channel-cells': > + const: 1 > + > + interrupts: > + maxItems: 1 > + description: > + End of conversion interrupt. > + > +required: > + - compatible > + - reg > + - '#address-cells' > + - '#size-cells' > + - '#io-channel-cells' > + > +patternProperties: > + "^.*@[0-9a-fx]+$": > + type: object > + description: | > + Represents the external channels which are connected to the ADC. > + For compatible property "qcom,spmi-vadc" following channels, also known as > + reference point channels, are used for result calibration and their channel > + configuration nodes should be defined: > + VADC_REF_625MV and/or VADC_SPARE1(based on PMIC version) VADC_REF_1250MV, > + VADC_GND_REF and VADC_VDD_VADC. > + > + properties: > + reg: > + description: | > + ADC channel number. > + See include/dt-bindings/iio/qcom,spmi-vadc.h > + > + label: > + $ref: /schemas/types.yaml#/definitions/string > + description: | > + ADC input of the platform as seen in the schematics. > + For thermistor inputs connected to generic AMUX or GPIO inputs > + these can vary across platform for the same pins. Hence select > + the platform schematics name for this channel. > + > + qcom,decimation: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + This parameter is used to decrease ADC sampling rate. > + Quicker measurements can be made by reducing decimation ratio. > + - For compatible property "qcom,spmi-vadc", valid values are > + 512, 1024, 2048, 4096. If property is not found, default value > + of 512 will be used. > + - For compatible property "qcom,spmi-adc5", valid values are 250, 420 > + and 840. If property is not found, default value of 840 is used. > + - For compatible property "qcom,spmi-adc-rev2", valid values are 256, > + 512 and 1024. If property is not present, default value is 1024. > + > + qcom,pre-scaling: > + description: | > + Used for scaling the channel input signal before the signal is > + fed to VADC. The configuration for this node is to know the > + pre-determined ratio and use it for post scaling. It is a pair of > + integers, denoting the numerator and denominator of the fraction by which > + input signal is multiplied. For example, <1 3> indicates the signal is scaled > + down to 1/3 of its value before ADC measurement. Select one from > + the following options. > + <1 1>, <1 3>, <1 4>, <1 6>, <1 20>, <1 8>, <10 81>, <1 10> > + If property is not found default value depending on chip will be used. > + allOf: > + - $ref: /schemas/types.yaml#/definitions/uint32-array > + oneOf: > + - items: > + - const: 1 > + - enum: [ 1, 3, 4, 6, 20, 8, 10 ] > + > + - items: > + - const: 10 > + - const: 81 > + > + qcom,ratiometric: > + description: | > + Channel calibration type. > + - For compatible property "qcom,spmi-vadc", if this property is > + specified VADC will use the VDD reference (1.8V) and GND for > + channel calibration. If property is not found, channel will be > + calibrated with 0.625V and 1.25V reference channels, also > + known as absolute calibration. > + - For compatible property "qcom,spmi-adc5" and "qcom,spmi-adc-rev2", > + if this property is specified VADC will use the VDD reference (1.875V) > + and GND for channel calibration. If property is not found, channel > + will be calibrated with 0V and 1.25V reference channels, also known > + as absolute calibration. > + type: boolean > + > + qcom,hw-settle-time: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + Time between AMUX getting configured and the ADC starting > + conversion. The 'hw_settle_time' is an index used from valid values > + and programmed in hardware to achieve the hardware settling delay. > + - For compatible property "qcom,spmi-vadc" and "qcom,spmi-adc-rev2", > + Delay = 100us * (hw_settle_time) for hw_settle_time < 11, > + and 2ms * (hw_settle_time - 10) otherwise. > + Valid values are: 0, 100, 200, 300, 400, 500, 600, 700, 800, > + 900 us and 1, 2, 4, 6, 8, 10 ms. > + If property is not found, channel will use 0us. > + - For compatible property "qcom,spmi-adc5", delay = 15us for > + value 0, 100us * (value) for values < 11, > + and 2ms * (value - 10) otherwise. > + Valid values are: 15, 100, 200, 300, 400, 500, 600, 700, 800, > + 900 us and 1, 2, 4, 6, 8, 10 ms > + Certain controller digital versions have valid values of > + 15, 100, 200, 300, 400, 500, 600, 700, 1, 2, 4, 8, 16, 32, 64, 128 ms > + If property is not found, channel will use 15us. > + > + qcom,avg-samples: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + Number of samples to be used for measurement. > + Averaging provides the option to obtain a single measurement > + from the ADC that is an average of multiple samples. The value > + selected is 2^(value). > + - For compatible property "qcom,spmi-vadc", valid values > + are: 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 > + If property is not found, 1 sample will be used. > + > + required: > + - reg > + > +allOf: > + - if: > + properties: > + compatible: > + contains: > + const: qcom,spmi-vadc > + > + then: > + patternProperties: > + "^.*@[0-9a-fx]+$": > + minItems: 4 > + properties: > + qcom,decimation: > + items: > + enum: [ 512, 1024, 2048, 4096 ] > + default: 512 > + > + qcom,hw-settle-time: > + items: > + enum: [ 0, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1, 2, > + 4, 6, 8, 10 ] > + default: 0 > + > + qcom,avg-samples: > + items: > + enum: [ 1, 2, 4, 8, 16, 32, 64, 128, 256, 512 ] > + default: 1 > + > + - if: > + properties: > + compatible: > + contains: > + const: qcom,spmi-adc-rev2 > + > + then: > + patternProperties: > + "^.*@[0-9a-fx]+$": > + properties: > + qcom,decimation: > + items: > + enum: [ 256, 512, 1024 ] > + default: 1024 > + > + qcom,hw-settle-time: > + items: > + enum: [ 0, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1, 2, > + 4, 6, 8, 10 ] > + default: 0 > + > + qcom,avg-samples: > + items: > + enum: [ 1, 2, 4, 8, 16 ] > + default: 1 > + > + - if: > + properties: > + compatible: > + contains: > + const: qcom,spmi-adc5 > + > + then: > + patternProperties: > + "^.*@[0-9a-fx]+$": > + properties: > + qcom,decimation: > + items: > + enum: [ 250, 420, 840 ] > + default: 840 > + > + qcom,hw-settle-time: > + items: > + enum: [ 15, 100, 200, 300, 400, 500, 600, 700, 800, 900, 1, 2, > + 4, 6, 8, 10, 16, 32, 64, 128 ] > + default: 15 > + > + qcom,avg-samples: > + items: > + enum: [ 1, 2, 4, 8, 16 ] > + default: 1 > + > +examples: > + - | > + spmi_bus { > + #address-cells = <1>; > + #size-cells = <0>; > + /* VADC node */ > + pmic_vadc: adc@3100 { > + compatible = "qcom,spmi-vadc"; > + reg = <0x3100>; > + interrupts = <0x0 0x31 0x0 0x1>; > + #address-cells = <1>; > + #size-cells = <0>; > + #io-channel-cells = <1>; > + io-channel-ranges; > + > + /* Channel node */ > + adc-chan@0x39 { > + reg = <0x39>; > + qcom,decimation = <512>; > + qcom,ratiometric; > + qcom,hw-settle-time = <200>; > + qcom,avg-samples = <1>; > + qcom,pre-scaling = <1 3>; > + }; > + > + adc-chan@0x9 { > + reg = <0x9>; > + }; > + > + adc-chan@0xa { > + reg = <0xa>; > + }; > + > + adc-chan@0xe { > + reg = <0xe>; > + }; > + > + adc-chan@0xf { > + reg = <0xf>; > + }; > + }; > + };
Hi Rob I can see the first error: "....chosen node must be at root node" from 'make dt_binding_check' even without my patch applied, so it does not seem related. I will fix the second error in the next post. On 4/17/2020 2:12 AM, Rob Herring wrote: > On Wed, 15 Apr 2020 14:47:44 +0530, Jishnu Prakash wrote: >> Convert the adc bindings from .txt to .yaml format. >> >> Signed-off-by: Jishnu Prakash <jprakash@codeaurora.org> >> --- >> .../devicetree/bindings/iio/adc/qcom,spmi-vadc.txt | 173 ------------- >> .../bindings/iio/adc/qcom,spmi-vadc.yaml | 288 +++++++++++++++++++++ >> 2 files changed, 288 insertions(+), 173 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.txt >> create mode 100644 Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.yaml >> > My bot found errors running 'make dt_binding_check' on your patch: > > Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/adc/qcom,spmi-vadc.example.dt.yaml: adc@3100: 'adc-chan@0x39', 'adc-chan@0x9', 'adc-chan@0xa', 'adc-chan@0xe', 'adc-chan@0xf' do not match any of the regexes: '.*-names$', '.*-supply$', '^#.*-cells$', '^#[a-zA-Z0-9,+\\-._]{0,63}$', '^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}$', '^[a-zA-Z][a-zA-Z0-9,+\\-._]{0,63}@[0-9a-fA-F]+(,[0-9a-fA-F]+)*$', '^__.*__$', 'pinctrl-[0-9]+' > > See https://patchwork.ozlabs.org/patch/1271025 > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure dt-schema is up to date: > > pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade > > Please check and re-submit.
Hi Andy, On 4/27/2020 6:58 PM, Andy Shevchenko wrote: > On Mon, Apr 27, 2020 at 3:56 PM Jishnu Prakash <jprakash@codeaurora.org> wrote: >> On 4/17/2020 3:51 PM, Andy Shevchenko wrote: >> On Thu, Apr 16, 2020 at 1:48 AM Jishnu Prakash <jprakash@codeaurora.org> wrote: > Stop using HTML. It breaks badly the reply and discussion. > > ... > >> +static const struct adc5_data adc7_data_pmic; >> >> Global variable? Hmm... >> >> adc7_data_pmic is referenced twice before its actual definition (which was added along with corresponding adc5_data struct for PMIC5 ADC), so I have given the initial declaration here. > Maybe you can realize how to avoid global variable at all? There is a way to remove this, I'll make this change with some other changes in the fifth patch of my latest post. > > ... > >> + buf[1] &= 0xff & ~ADC5_USR_FAST_AVG_CTL_SAMPLES_MASK; >> >> What the point of 0xff & part? >> >> This was something you suggested in my first post: >> >>> + buf[1] &= (u8) ~ADC5_USR_FAST_AVG_CTL_SAMPLES_MASK; >> Use '0xFF ^ _MASK' instead of casting. >> >> ... >> >>> + buf[3] &= (u8) ~ADC5_USR_HW_SETTLE_DELAY_MASK; >> Ditto. >> >> I think "0xff &" works as intended here in place of casting to (u8)... > Does it work without casting? (Note, I suggested slightly different expression) > I.o.w. what the problem casting solves? I checked this part again. It looks like casting is not strictly required here, I'll remove it in my latest post. > >> + buf[1] |= prop->avg_samples; >> + >> + /* Select ADC channel */ >> + buf[2] = prop->channel; >> + >> + /* Select HW settle delay for channel */ >> + buf[3] &= 0xff & ~ADC5_USR_HW_SETTLE_DELAY_MASK; >> >> Ditto. >> >> + buf[3] |= prop->hw_settle_time; >