[PATCH v1 0/4] MediaTek MT6357/8/9 PMIC Auxiliary ADC support

[PATCH v1 0/4] MediaTek MT6357/8/9 PMIC Auxiliary ADC support

[AngeloGioacchino Del Regno]

This series adds support for the Auxiliary ADC IP found on the MediaTek
MT635(x) PMICs, including MT6357, 6358 and 6359, exposing various ADC
channels for accessory detection (usually, on 3.5mm jack), battery
voltage, PMIC temperature, PMIC-integrated voltage regulators, and
others, depending on the PMIC model.

AngeloGioacchino Del Regno (4):
  dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC
  iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC
  mfd: mt6397-core: Add support for AUXADCs on MT6357/58/59 PMICs
  arm64: dts: mediatek: Add ADC node on MT6357, MT6358, MT6359 PMICs

 .../iio/adc/mediatek,mt6359-auxadc.yaml       |  43 ++
 arch/arm64/boot/dts/mediatek/mt6357.dtsi      |   5 +
 arch/arm64/boot/dts/mediatek/mt6358.dtsi      |   5 +
 arch/arm64/boot/dts/mediatek/mt6359.dtsi      |   5 +
 drivers/iio/adc/Kconfig                       |  12 +
 drivers/iio/adc/Makefile                      |   1 +
 drivers/iio/adc/mt6359-auxadc.c               | 598 ++++++++++++++++++
 drivers/mfd/mt6397-core.c                     |  10 +
 .../iio/adc/mediatek,mt6357-auxadc.h          |  21 +
 .../iio/adc/mediatek,mt6358-auxadc.h          |  22 +
 .../iio/adc/mediatek,mt6359-auxadc.h          |  22 +
 11 files changed, 744 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
 create mode 100644 drivers/iio/adc/mt6359-auxadc.c
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h

-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[AngeloGioacchino Del Regno]

Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
providing various ADC channels for both internal temperatures and
voltages, audio accessory detection (hp/mic/hp+mic and buttons,
usually on a 3.5mm jack) other than some basic battery statistics
on boards where the battery is managed by this PMIC.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
 .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
 .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
 .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
 4 files changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
 create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h

diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
new file mode 100644
index 000000000000..dd6c331905cf
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT6350 series PMIC AUXADC
+
+maintainers:
+  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
+
+description:
+  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
+  in some MediaTek PMICs, performing various PMIC related measurements
+  such as battery and PMIC internal voltage regulators temperatures,
+  accessory detection resistance (usually, for a 3.5mm audio jack)
+  other than voltages for various PMIC internal components.
+
+properties:
+  compatible:
+    enum:
+      - mediatek,mt6357-auxadc
+      - mediatek,mt6358-auxadc
+      - mediatek,mt6359-auxadc
+
+  "#io-channel-cells":
+    const: 1
+
+additionalProperties: false
+
+required:
+  - compatible
+  - "#io-channel-cells"
+
+examples:
+  - |
+    pmic {
+        pmic_adc: adc {
+            compatible = "mediatek,mt6359-auxadc";
+            #io-channel-cells = <1>;
+        };
+    };
+...
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
new file mode 100644
index 000000000000..03ebb1d23953
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
+
+/* ADC Channel Index */
+#define MT6357_AUXADC_BATADC		0
+#define MT6357_AUXADC_ISENSE		1
+#define MT6357_AUXADC_VCDT		2
+#define MT6357_AUXADC_BAT_TEMP		3
+#define MT6357_AUXADC_CHIP_TEMP		4
+#define MT6357_AUXADC_ACCDET		5
+#define MT6357_AUXADC_VDCXO		6
+#define MT6357_AUXADC_TSX_TEMP		7
+#define MT6357_AUXADC_HPOFS_CAL		8
+#define MT6357_AUXADC_DCXO_TEMP		9
+#define MT6357_AUXADC_VCORE_TEMP	10
+#define MT6357_AUXADC_VPROC_TEMP	11
+#define MT6357_AUXADC_VBAT		12
+
+#endif
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
new file mode 100644
index 000000000000..efa08398fafd
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
+
+/* ADC Channel Index */
+#define MT6358_AUXADC_BATADC		0
+#define MT6358_AUXADC_VCDT		1
+#define MT6358_AUXADC_BAT_TEMP		2
+#define MT6358_AUXADC_CHIP_TEMP		3
+#define MT6358_AUXADC_ACCDET		4
+#define MT6358_AUXADC_VDCXO		5
+#define MT6358_AUXADC_TSX_TEMP		6
+#define MT6358_AUXADC_HPOFS_CAL		7
+#define MT6358_AUXADC_DCXO_TEMP		8
+#define MT6358_AUXADC_VBIF		9
+#define MT6358_AUXADC_VCORE_TEMP	10
+#define MT6358_AUXADC_VPROC_TEMP	11
+#define MT6358_AUXADC_VGPU_TEMP		12
+#define MT6358_AUXADC_VBAT		13
+
+#endif
diff --git a/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
new file mode 100644
index 000000000000..59826393ee7e
--- /dev/null
+++ b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+
+#ifndef _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
+#define _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
+
+/* ADC Channel Index */
+#define MT6359_AUXADC_BATADC		0
+#define MT6359_AUXADC_BAT_TEMP		1
+#define MT6359_AUXADC_CHIP_TEMP		2
+#define MT6359_AUXADC_ACCDET		3
+#define MT6359_AUXADC_VDCXO		4
+#define MT6359_AUXADC_TSX_TEMP		5
+#define MT6359_AUXADC_HPOFS_CAL		6
+#define MT6359_AUXADC_DCXO_TEMP		7
+#define MT6359_AUXADC_VBIF		8
+#define MT6359_AUXADC_VCORE_TEMP	9
+#define MT6359_AUXADC_VPROC_TEMP	10
+#define MT6359_AUXADC_VGPU_TEMP		11
+#define MT6359_AUXADC_VBAT		12
+#define MT6359_AUXADC_IBAT		13
+
+#endif
-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[Krzysztof Kozlowski]

On 30/05/2024 11:34, AngeloGioacchino Del Regno wrote:
> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
> providing various ADC channels for both internal temperatures and
> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
> usually on a 3.5mm jack) other than some basic battery statistics
> on boards where the battery is managed by this PMIC.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
>  .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
>  .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
>  .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
>  4 files changed, 108 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> new file mode 100644
> index 000000000000..dd6c331905cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT6350 series PMIC AUXADC
> +
> +maintainers:
> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> +
> +description:
> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
> +  in some MediaTek PMICs, performing various PMIC related measurements
> +  such as battery and PMIC internal voltage regulators temperatures,
> +  accessory detection resistance (usually, for a 3.5mm audio jack)
> +  other than voltages for various PMIC internal components.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt6357-auxadc
> +      - mediatek,mt6358-auxadc
> +      - mediatek,mt6359-auxadc
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +additionalProperties: false

If there is going to be a re-spin, please move this below required: block.

> +
> +required:
> +  - compatible
> +  - "#io-channel-cells"
> +
> +examples:
> +  - |
> +    pmic {
> +        pmic_adc: adc {
> +            compatible = "mediatek,mt6359-auxadc";
> +            #io-channel-cells = <1>;
> +        };

This suggests that you should grow (make complete) the parent PMIC example.

Actually having this as a separate node is not really needed. Why it
cannot be just part of the MFD/parent node?


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[AngeloGioacchino Del Regno]

Il 01/06/24 17:32, Krzysztof Kozlowski ha scritto:
> On 30/05/2024 11:34, AngeloGioacchino Del Regno wrote:
>> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
>> providing various ADC channels for both internal temperatures and
>> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
>> usually on a 3.5mm jack) other than some basic battery statistics
>> on boards where the battery is managed by this PMIC.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
>>   .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
>>   .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
>>   .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
>>   4 files changed, 108 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>> new file mode 100644
>> index 000000000000..dd6c331905cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>> @@ -0,0 +1,43 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek MT6350 series PMIC AUXADC
>> +
>> +maintainers:
>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> +
>> +description:
>> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
>> +  in some MediaTek PMICs, performing various PMIC related measurements
>> +  such as battery and PMIC internal voltage regulators temperatures,
>> +  accessory detection resistance (usually, for a 3.5mm audio jack)
>> +  other than voltages for various PMIC internal components.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - mediatek,mt6357-auxadc
>> +      - mediatek,mt6358-auxadc
>> +      - mediatek,mt6359-auxadc
>> +
>> +  "#io-channel-cells":
>> +    const: 1
>> +
>> +additionalProperties: false
> 
> If there is going to be a re-spin, please move this below required: block.
> 

Yep, will do. Fixed up for v2.

>> +
>> +required:
>> +  - compatible
>> +  - "#io-channel-cells"
>> +
>> +examples:
>> +  - |
>> +    pmic {
>> +        pmic_adc: adc {
>> +            compatible = "mediatek,mt6359-auxadc";
>> +            #io-channel-cells = <1>;
>> +        };
> 
> This suggests that you should grow (make complete) the parent PMIC example.

Uhm, should I instead add that to bindings/mfd/mediatek,mt6357.yaml and avoid
growing the parent example?

   adc:
     type: object
     $ref: /schemas/iio/adc/mediatek,mt6359-auxadc.yaml
     unevaluatedProperties: false

> 
> Actually having this as a separate node is not really needed. Why it
> cannot be just part of the MFD/parent node?
> 

(glossary: PWRAP = PMIC [SPI] WRAPper)

The top node is the PWRAP, an IP that is (mostly) used to dispatch commands to
the PMIC, but the AUXADC is not integrated into the PWRAP, but into the PMIC.

Declaring the AUXADC as a PWRAP child would therefore be an incorrect description
of the hardware.

P.S.: not necessary, but to "complete the circle" ... note that the PWRAP can be
       skipped on some SoCs/firmwares/configurations, even though afaik that's only
       mostly for debugging purposes, it's not granted that you have pwrap between
       SoC and PMIC on all SoCs/fws/confs, even though, all of the boards that are
       supported upstream do have it and do require it.

Cheers,
Angelo

> 
> Best regards,
> Krzysztof
> 




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[Krzysztof Kozlowski]

On 04/06/2024 10:55, AngeloGioacchino Del Regno wrote:
> Il 01/06/24 17:32, Krzysztof Kozlowski ha scritto:
>> On 30/05/2024 11:34, AngeloGioacchino Del Regno wrote:
>>> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
>>> providing various ADC channels for both internal temperatures and
>>> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
>>> usually on a 3.5mm jack) other than some basic battery statistics
>>> on boards where the battery is managed by this PMIC.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> ---
>>>   .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
>>>   .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
>>>   .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
>>>   .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
>>>   4 files changed, 108 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>> new file mode 100644
>>> index 000000000000..dd6c331905cf
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>> @@ -0,0 +1,43 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MediaTek MT6350 series PMIC AUXADC
>>> +
>>> +maintainers:
>>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> +
>>> +description:
>>> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
>>> +  in some MediaTek PMICs, performing various PMIC related measurements
>>> +  such as battery and PMIC internal voltage regulators temperatures,
>>> +  accessory detection resistance (usually, for a 3.5mm audio jack)
>>> +  other than voltages for various PMIC internal components.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - mediatek,mt6357-auxadc
>>> +      - mediatek,mt6358-auxadc
>>> +      - mediatek,mt6359-auxadc
>>> +
>>> +  "#io-channel-cells":
>>> +    const: 1
>>> +
>>> +additionalProperties: false
>>
>> If there is going to be a re-spin, please move this below required: block.
>>
> 
> Yep, will do. Fixed up for v2.
> 
>>> +
>>> +required:
>>> +  - compatible
>>> +  - "#io-channel-cells"
>>> +
>>> +examples:
>>> +  - |
>>> +    pmic {
>>> +        pmic_adc: adc {
>>> +            compatible = "mediatek,mt6359-auxadc";
>>> +            #io-channel-cells = <1>;
>>> +        };
>>
>> This suggests that you should grow (make complete) the parent PMIC example.
> 
> Uhm, should I instead add that to bindings/mfd/mediatek,mt6357.yaml and avoid
> growing the parent example?

No. You should avoid this example and grow the parent example. Not avoid
parent example...

> 
>    adc:
>      type: object
>      $ref: /schemas/iio/adc/mediatek,mt6359-auxadc.yaml
>      unevaluatedProperties: false
> 
>>
>> Actually having this as a separate node is not really needed. Why it
>> cannot be just part of the MFD/parent node?
>>
> 
> (glossary: PWRAP = PMIC [SPI] WRAPper)
> 
> The top node is the PWRAP, an IP that is (mostly) used to dispatch commands to
> the PMIC, but the AUXADC is not integrated into the PWRAP, but into the PMIC.

Eh? mediatek,mt6357.yaml says it is a PMIC...

> 
> Declaring the AUXADC as a PWRAP child would therefore be an incorrect description
> of the hardware.
> 
> P.S.: not necessary, but to "complete the circle" ... note that the PWRAP can be
>        skipped on some SoCs/firmwares/configurations, even though afaik that's only
>        mostly for debugging purposes, it's not granted that you have pwrap between
>        SoC and PMIC on all SoCs/fws/confs, even though, all of the boards that are
>        supported upstream do have it and do require it.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[AngeloGioacchino Del Regno]

Il 04/06/24 11:38, Krzysztof Kozlowski ha scritto:
> On 04/06/2024 10:55, AngeloGioacchino Del Regno wrote:
>> Il 01/06/24 17:32, Krzysztof Kozlowski ha scritto:
>>> On 30/05/2024 11:34, AngeloGioacchino Del Regno wrote:
>>>> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
>>>> providing various ADC channels for both internal temperatures and
>>>> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
>>>> usually on a 3.5mm jack) other than some basic battery statistics
>>>> on boards where the battery is managed by this PMIC.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
>>>>    .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
>>>>    .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
>>>>    .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
>>>>    4 files changed, 108 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>>>    create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>>>>    create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>>>>    create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>>> new file mode 100644
>>>> index 000000000000..dd6c331905cf
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>>> @@ -0,0 +1,43 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: MediaTek MT6350 series PMIC AUXADC
>>>> +
>>>> +maintainers:
>>>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> +
>>>> +description:
>>>> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
>>>> +  in some MediaTek PMICs, performing various PMIC related measurements
>>>> +  such as battery and PMIC internal voltage regulators temperatures,
>>>> +  accessory detection resistance (usually, for a 3.5mm audio jack)
>>>> +  other than voltages for various PMIC internal components.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - mediatek,mt6357-auxadc
>>>> +      - mediatek,mt6358-auxadc
>>>> +      - mediatek,mt6359-auxadc
>>>> +
>>>> +  "#io-channel-cells":
>>>> +    const: 1
>>>> +
>>>> +additionalProperties: false
>>>
>>> If there is going to be a re-spin, please move this below required: block.
>>>
>>
>> Yep, will do. Fixed up for v2.
>>
>>>> +
>>>> +required:
>>>> +  - compatible
>>>> +  - "#io-channel-cells"
>>>> +
>>>> +examples:
>>>> +  - |
>>>> +    pmic {
>>>> +        pmic_adc: adc {
>>>> +            compatible = "mediatek,mt6359-auxadc";
>>>> +            #io-channel-cells = <1>;
>>>> +        };
>>>
>>> This suggests that you should grow (make complete) the parent PMIC example.
>>
>> Uhm, should I instead add that to bindings/mfd/mediatek,mt6357.yaml and avoid
>> growing the parent example?
> 
> No. You should avoid this example and grow the parent example. Not avoid
> parent example...
> 

I will remove examples from here and move the example that I wrote in here
to mediatek,mt6357.yaml.

>>
>>     adc:
>>       type: object
>>       $ref: /schemas/iio/adc/mediatek,mt6359-auxadc.yaml
>>       unevaluatedProperties: false
>>
>>>
>>> Actually having this as a separate node is not really needed. Why it
>>> cannot be just part of the MFD/parent node?
>>>
>>
>> (glossary: PWRAP = PMIC [SPI] WRAPper)
>>
>> The top node is the PWRAP, an IP that is (mostly) used to dispatch commands to
>> the PMIC, but the AUXADC is not integrated into the PWRAP, but into the PMIC.
> 
> Eh? mediatek,mt6357.yaml says it is a PMIC...
> 

Eh, apparently my brain cells are somewhere else today.
I was thinking about the HW, and some synapse got disconnected in the process, lol.

Just pretend I never wrote this useless wall of text, sorry. Ugh :-)

Cheers,
Angelo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[Jonathan Cameron]

On Thu, 30 May 2024 11:34:07 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
> providing various ADC channels for both internal temperatures and
> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
> usually on a 3.5mm jack) other than some basic battery statistics
> on boards where the battery is managed by this PMIC.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Hi,

What are all the headers for given the binding doc doesn't use anything from
them?

Jonathan

> ---
>  .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
>  .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
>  .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
>  .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
>  4 files changed, 108 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>  create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> new file mode 100644
> index 000000000000..dd6c331905cf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> @@ -0,0 +1,43 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT6350 series PMIC AUXADC
> +
> +maintainers:
> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> +
> +description:
> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
> +  in some MediaTek PMICs, performing various PMIC related measurements
> +  such as battery and PMIC internal voltage regulators temperatures,
> +  accessory detection resistance (usually, for a 3.5mm audio jack)
> +  other than voltages for various PMIC internal components.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - mediatek,mt6357-auxadc
> +      - mediatek,mt6358-auxadc
> +      - mediatek,mt6359-auxadc
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - "#io-channel-cells"
> +
> +examples:
> +  - |
> +    pmic {
> +        pmic_adc: adc {
> +            compatible = "mediatek,mt6359-auxadc";
> +            #io-channel-cells = <1>;
> +        };
> +    };
> +...
> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
> new file mode 100644
> index 000000000000..03ebb1d23953
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +
> +#ifndef _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
> +#define _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
> +
> +/* ADC Channel Index */
> +#define MT6357_AUXADC_BATADC		0
> +#define MT6357_AUXADC_ISENSE		1
> +#define MT6357_AUXADC_VCDT		2
> +#define MT6357_AUXADC_BAT_TEMP		3
> +#define MT6357_AUXADC_CHIP_TEMP		4
> +#define MT6357_AUXADC_ACCDET		5
> +#define MT6357_AUXADC_VDCXO		6
> +#define MT6357_AUXADC_TSX_TEMP		7
> +#define MT6357_AUXADC_HPOFS_CAL		8
> +#define MT6357_AUXADC_DCXO_TEMP		9
> +#define MT6357_AUXADC_VCORE_TEMP	10
> +#define MT6357_AUXADC_VPROC_TEMP	11
> +#define MT6357_AUXADC_VBAT		12
> +
> +#endif
> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
> new file mode 100644
> index 000000000000..efa08398fafd
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +
> +#ifndef _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
> +#define _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
> +
> +/* ADC Channel Index */
> +#define MT6358_AUXADC_BATADC		0
> +#define MT6358_AUXADC_VCDT		1
> +#define MT6358_AUXADC_BAT_TEMP		2
> +#define MT6358_AUXADC_CHIP_TEMP		3
> +#define MT6358_AUXADC_ACCDET		4
> +#define MT6358_AUXADC_VDCXO		5
> +#define MT6358_AUXADC_TSX_TEMP		6
> +#define MT6358_AUXADC_HPOFS_CAL		7
> +#define MT6358_AUXADC_DCXO_TEMP		8
> +#define MT6358_AUXADC_VBIF		9
> +#define MT6358_AUXADC_VCORE_TEMP	10
> +#define MT6358_AUXADC_VPROC_TEMP	11
> +#define MT6358_AUXADC_VGPU_TEMP		12
> +#define MT6358_AUXADC_VBAT		13
> +
> +#endif
> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> new file mode 100644
> index 000000000000..59826393ee7e
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +
> +#ifndef _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
> +#define _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
> +
> +/* ADC Channel Index */
> +#define MT6359_AUXADC_BATADC		0
> +#define MT6359_AUXADC_BAT_TEMP		1
> +#define MT6359_AUXADC_CHIP_TEMP		2
> +#define MT6359_AUXADC_ACCDET		3
> +#define MT6359_AUXADC_VDCXO		4
> +#define MT6359_AUXADC_TSX_TEMP		5
> +#define MT6359_AUXADC_HPOFS_CAL		6
> +#define MT6359_AUXADC_DCXO_TEMP		7
> +#define MT6359_AUXADC_VBIF		8
> +#define MT6359_AUXADC_VCORE_TEMP	9
> +#define MT6359_AUXADC_VPROC_TEMP	10
> +#define MT6359_AUXADC_VGPU_TEMP		11
> +#define MT6359_AUXADC_VBAT		12
> +#define MT6359_AUXADC_IBAT		13
> +
> +#endif


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[AngeloGioacchino Del Regno]

Il 02/06/24 11:40, Jonathan Cameron ha scritto:
> On Thu, 30 May 2024 11:34:07 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
>> providing various ADC channels for both internal temperatures and
>> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
>> usually on a 3.5mm jack) other than some basic battery statistics
>> on boards where the battery is managed by this PMIC.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Hi,
> 
> What are all the headers for given the binding doc doesn't use anything from
> them?
> 

The binding document doesn't use them, yes, but those are part of the devicetree
binding itself, as those can be used by devicetrees to reference an ADC channel
like <&pmic_adc MT6359_AUXADC_ACCDET>.

I think growing the binding example to add an "external" node using one of those
channels is irrelevant, as that is "outside" of the description of the AUXADC HW.

Cheers,
Angelo

> Jonathan
> 
>> ---
>>   .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
>>   .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
>>   .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
>>   .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
>>   4 files changed, 108 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>> new file mode 100644
>> index 000000000000..dd6c331905cf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
>> @@ -0,0 +1,43 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek MT6350 series PMIC AUXADC
>> +
>> +maintainers:
>> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> +
>> +description:
>> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
>> +  in some MediaTek PMICs, performing various PMIC related measurements
>> +  such as battery and PMIC internal voltage regulators temperatures,
>> +  accessory detection resistance (usually, for a 3.5mm audio jack)
>> +  other than voltages for various PMIC internal components.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - mediatek,mt6357-auxadc
>> +      - mediatek,mt6358-auxadc
>> +      - mediatek,mt6359-auxadc
>> +
>> +  "#io-channel-cells":
>> +    const: 1
>> +
>> +additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - "#io-channel-cells"
>> +
>> +examples:
>> +  - |
>> +    pmic {
>> +        pmic_adc: adc {
>> +            compatible = "mediatek,mt6359-auxadc";
>> +            #io-channel-cells = <1>;
>> +        };
>> +    };
>> +...
>> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>> new file mode 100644
>> index 000000000000..03ebb1d23953
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +
>> +#ifndef _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
>> +#define _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
>> +
>> +/* ADC Channel Index */
>> +#define MT6357_AUXADC_BATADC		0
>> +#define MT6357_AUXADC_ISENSE		1
>> +#define MT6357_AUXADC_VCDT		2
>> +#define MT6357_AUXADC_BAT_TEMP		3
>> +#define MT6357_AUXADC_CHIP_TEMP		4
>> +#define MT6357_AUXADC_ACCDET		5
>> +#define MT6357_AUXADC_VDCXO		6
>> +#define MT6357_AUXADC_TSX_TEMP		7
>> +#define MT6357_AUXADC_HPOFS_CAL		8
>> +#define MT6357_AUXADC_DCXO_TEMP		9
>> +#define MT6357_AUXADC_VCORE_TEMP	10
>> +#define MT6357_AUXADC_VPROC_TEMP	11
>> +#define MT6357_AUXADC_VBAT		12
>> +
>> +#endif
>> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>> new file mode 100644
>> index 000000000000..efa08398fafd
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +
>> +#ifndef _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
>> +#define _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
>> +
>> +/* ADC Channel Index */
>> +#define MT6358_AUXADC_BATADC		0
>> +#define MT6358_AUXADC_VCDT		1
>> +#define MT6358_AUXADC_BAT_TEMP		2
>> +#define MT6358_AUXADC_CHIP_TEMP		3
>> +#define MT6358_AUXADC_ACCDET		4
>> +#define MT6358_AUXADC_VDCXO		5
>> +#define MT6358_AUXADC_TSX_TEMP		6
>> +#define MT6358_AUXADC_HPOFS_CAL		7
>> +#define MT6358_AUXADC_DCXO_TEMP		8
>> +#define MT6358_AUXADC_VBIF		9
>> +#define MT6358_AUXADC_VCORE_TEMP	10
>> +#define MT6358_AUXADC_VPROC_TEMP	11
>> +#define MT6358_AUXADC_VGPU_TEMP		12
>> +#define MT6358_AUXADC_VBAT		13
>> +
>> +#endif
>> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
>> new file mode 100644
>> index 000000000000..59826393ee7e
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +
>> +#ifndef _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
>> +#define _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
>> +
>> +/* ADC Channel Index */
>> +#define MT6359_AUXADC_BATADC		0
>> +#define MT6359_AUXADC_BAT_TEMP		1
>> +#define MT6359_AUXADC_CHIP_TEMP		2
>> +#define MT6359_AUXADC_ACCDET		3
>> +#define MT6359_AUXADC_VDCXO		4
>> +#define MT6359_AUXADC_TSX_TEMP		5
>> +#define MT6359_AUXADC_HPOFS_CAL		6
>> +#define MT6359_AUXADC_DCXO_TEMP		7
>> +#define MT6359_AUXADC_VBIF		8
>> +#define MT6359_AUXADC_VCORE_TEMP	9
>> +#define MT6359_AUXADC_VPROC_TEMP	10
>> +#define MT6359_AUXADC_VGPU_TEMP		11
>> +#define MT6359_AUXADC_VBAT		12
>> +#define MT6359_AUXADC_IBAT		13
>> +
>> +#endif
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] dt-bindings: iio: adc: Add MediaTek MT6359 PMIC AUXADC

[Jonathan Cameron]

On Tue, 4 Jun 2024 11:04:25 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 02/06/24 11:40, Jonathan Cameron ha scritto:
> > On Thu, 30 May 2024 11:34:07 +0200
> > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> >   
> >> Add a new binding for the MT6350 Series (MT6357/8/9) PMIC AUXADC,
> >> providing various ADC channels for both internal temperatures and
> >> voltages, audio accessory detection (hp/mic/hp+mic and buttons,
> >> usually on a 3.5mm jack) other than some basic battery statistics
> >> on boards where the battery is managed by this PMIC.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>  
> > Hi,
> > 
> > What are all the headers for given the binding doc doesn't use anything from
> > them?
> >   
> 
> The binding document doesn't use them, yes, but those are part of the devicetree
> binding itself, as those can be used by devicetrees to reference an ADC channel
> like <&pmic_adc MT6359_AUXADC_ACCDET>.
> 
> I think growing the binding example to add an "external" node using one of those
> channels is irrelevant, as that is "outside" of the description of the AUXADC HW.

Add a note on that to the patch description and that is fine.

J
> 
> Cheers,
> Angelo
> 
> > Jonathan
> >   
> >> ---
> >>   .../iio/adc/mediatek,mt6359-auxadc.yaml       | 43 +++++++++++++++++++
> >>   .../iio/adc/mediatek,mt6357-auxadc.h          | 21 +++++++++
> >>   .../iio/adc/mediatek,mt6358-auxadc.h          | 22 ++++++++++
> >>   .../iio/adc/mediatek,mt6359-auxadc.h          | 22 ++++++++++
> >>   4 files changed, 108 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> >>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
> >>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
> >>   create mode 100644 include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> >> new file mode 100644
> >> index 000000000000..dd6c331905cf
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/mediatek,mt6359-auxadc.yaml
> >> @@ -0,0 +1,43 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/iio/adc/mediatek,mt6359-auxadc.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: MediaTek MT6350 series PMIC AUXADC
> >> +
> >> +maintainers:
> >> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >> +
> >> +description:
> >> +  The Auxiliary Analog/Digital Converter (AUXADC) is an ADC found
> >> +  in some MediaTek PMICs, performing various PMIC related measurements
> >> +  such as battery and PMIC internal voltage regulators temperatures,
> >> +  accessory detection resistance (usually, for a 3.5mm audio jack)
> >> +  other than voltages for various PMIC internal components.
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - mediatek,mt6357-auxadc
> >> +      - mediatek,mt6358-auxadc
> >> +      - mediatek,mt6359-auxadc
> >> +
> >> +  "#io-channel-cells":
> >> +    const: 1
> >> +
> >> +additionalProperties: false
> >> +
> >> +required:
> >> +  - compatible
> >> +  - "#io-channel-cells"
> >> +
> >> +examples:
> >> +  - |
> >> +    pmic {
> >> +        pmic_adc: adc {
> >> +            compatible = "mediatek,mt6359-auxadc";
> >> +            #io-channel-cells = <1>;
> >> +        };
> >> +    };
> >> +...
> >> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
> >> new file mode 100644
> >> index 000000000000..03ebb1d23953
> >> --- /dev/null
> >> +++ b/include/dt-bindings/iio/adc/mediatek,mt6357-auxadc.h
> >> @@ -0,0 +1,21 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> >> +
> >> +#ifndef _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
> >> +#define _DT_BINDINGS_MEDIATEK_MT6357_AUXADC_H
> >> +
> >> +/* ADC Channel Index */
> >> +#define MT6357_AUXADC_BATADC		0
> >> +#define MT6357_AUXADC_ISENSE		1
> >> +#define MT6357_AUXADC_VCDT		2
> >> +#define MT6357_AUXADC_BAT_TEMP		3
> >> +#define MT6357_AUXADC_CHIP_TEMP		4
> >> +#define MT6357_AUXADC_ACCDET		5
> >> +#define MT6357_AUXADC_VDCXO		6
> >> +#define MT6357_AUXADC_TSX_TEMP		7
> >> +#define MT6357_AUXADC_HPOFS_CAL		8
> >> +#define MT6357_AUXADC_DCXO_TEMP		9
> >> +#define MT6357_AUXADC_VCORE_TEMP	10
> >> +#define MT6357_AUXADC_VPROC_TEMP	11
> >> +#define MT6357_AUXADC_VBAT		12
> >> +
> >> +#endif
> >> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
> >> new file mode 100644
> >> index 000000000000..efa08398fafd
> >> --- /dev/null
> >> +++ b/include/dt-bindings/iio/adc/mediatek,mt6358-auxadc.h
> >> @@ -0,0 +1,22 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> >> +
> >> +#ifndef _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
> >> +#define _DT_BINDINGS_MEDIATEK_MT6358_AUXADC_H
> >> +
> >> +/* ADC Channel Index */
> >> +#define MT6358_AUXADC_BATADC		0
> >> +#define MT6358_AUXADC_VCDT		1
> >> +#define MT6358_AUXADC_BAT_TEMP		2
> >> +#define MT6358_AUXADC_CHIP_TEMP		3
> >> +#define MT6358_AUXADC_ACCDET		4
> >> +#define MT6358_AUXADC_VDCXO		5
> >> +#define MT6358_AUXADC_TSX_TEMP		6
> >> +#define MT6358_AUXADC_HPOFS_CAL		7
> >> +#define MT6358_AUXADC_DCXO_TEMP		8
> >> +#define MT6358_AUXADC_VBIF		9
> >> +#define MT6358_AUXADC_VCORE_TEMP	10
> >> +#define MT6358_AUXADC_VPROC_TEMP	11
> >> +#define MT6358_AUXADC_VGPU_TEMP		12
> >> +#define MT6358_AUXADC_VBAT		13
> >> +
> >> +#endif
> >> diff --git a/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> >> new file mode 100644
> >> index 000000000000..59826393ee7e
> >> --- /dev/null
> >> +++ b/include/dt-bindings/iio/adc/mediatek,mt6359-auxadc.h
> >> @@ -0,0 +1,22 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> >> +
> >> +#ifndef _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
> >> +#define _DT_BINDINGS_MEDIATEK_MT6359_AUXADC_H
> >> +
> >> +/* ADC Channel Index */
> >> +#define MT6359_AUXADC_BATADC		0
> >> +#define MT6359_AUXADC_BAT_TEMP		1
> >> +#define MT6359_AUXADC_CHIP_TEMP		2
> >> +#define MT6359_AUXADC_ACCDET		3
> >> +#define MT6359_AUXADC_VDCXO		4
> >> +#define MT6359_AUXADC_TSX_TEMP		5
> >> +#define MT6359_AUXADC_HPOFS_CAL		6
> >> +#define MT6359_AUXADC_DCXO_TEMP		7
> >> +#define MT6359_AUXADC_VBIF		8
> >> +#define MT6359_AUXADC_VCORE_TEMP	9
> >> +#define MT6359_AUXADC_VPROC_TEMP	10
> >> +#define MT6359_AUXADC_VGPU_TEMP		11
> >> +#define MT6359_AUXADC_VBAT		12
> >> +#define MT6359_AUXADC_IBAT		13
> >> +
> >> +#endif  
> >   
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[AngeloGioacchino Del Regno]

Add a driver to support reading the Auxiliary ADC IP found in the
MediaTek MT6357, MT6358 and MT6359 Power Management ICs.

This driver provides multiple ADC channels for system monitoring,
such as battery voltage, PMIC temperature, PMIC-internal voltage
regulators temperature, and others.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iio/adc/Kconfig         |  12 +
 drivers/iio/adc/Makefile        |   1 +
 drivers/iio/adc/mt6359-auxadc.c | 598 ++++++++++++++++++++++++++++++++
 3 files changed, 611 insertions(+)
 create mode 100644 drivers/iio/adc/mt6359-auxadc.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..1c3df21beaf3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -870,6 +870,18 @@ config MCP3911
 	  This driver can also be built as a module. If so, the module will be
 	  called mcp3911.
 
+config MEDIATEK_MT6359_AUXADC
+	tristate "MediaTek MT6359 PMIC AUXADC driver"
+	depends on MFD_MT6397
+	help
+	  Say yes here to enable support for MediaTek MT6357, MT6358 and
+	  MT6359 PMICs Auxiliary ADC.
+	  This driver provides multiple channels for system monitoring,
+	  such as battery voltage, PMIC temperature, and others.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called mt6359-auxadc.
+
 config MEDIATEK_MT6360_ADC
 	tristate "Mediatek MT6360 ADC driver"
 	depends on MFD_MT6360
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index edb32ce2af02..da7d4452b1e0 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
 obj-$(CONFIG_MCP3422) += mcp3422.o
 obj-$(CONFIG_MCP3564) += mcp3564.o
 obj-$(CONFIG_MCP3911) += mcp3911.o
+obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
 obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
 obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
 obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
new file mode 100644
index 000000000000..0481bd3f0144
--- /dev/null
+++ b/drivers/iio/adc/mt6359-auxadc.c
@@ -0,0 +1,598 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MediaTek MT6359 PMIC AUXADC IIO driver
+ *
+ * Copyright (c) 2021 MediaTek Inc.
+ * Copyright (c) 2024 Collabora Ltd
+ * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/mfd/mt6397/core.h>
+
+#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
+#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
+#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>
+
+#define AUXADC_AVG_TIME_US		10
+#define AUXADC_POLL_DELAY_US		100
+#define AUXADC_TIMEOUT_US		32000
+#define AUXADC_VOLT_FULL		1800
+#define IMP_STOP_DELAY_US		150
+#define IMP_POLL_DELAY_US		1000
+
+#define PMIC_RG_RESET_VAL		(BIT(0) | BIT(3))
+#define PMIC_AUXADC_RDY_BIT		BIT(15)
+#define PMIC_AUXADC_ADCx(x)		((x) << 1)
+#define MT6357_IMP_ADC_NUM		30
+#define MT6358_IMP_ADC_NUM		28
+
+#define MT6358_DCM_CK_SW_EN		GENMASK(1, 0)
+#define MT6358_IMP0_CLEAR		(BIT(14) | BIT(7))
+#define MT6358_IMP0_IRQ_RDY		BIT(8)
+#define MT6358_IMP1_AUTOREPEAT_EN	BIT(15)
+
+#define MT6359_IMP0_CONV_EN		BIT(0)
+#define MT6359_IMP1_IRQ_RDY		BIT(15)
+
+enum mtk_pmic_auxadc_regs {
+	PMIC_AUXADC_ADC0,
+	PMIC_AUXADC_DCM_CON,
+	PMIC_AUXADC_IMP0,
+	PMIC_AUXADC_IMP1,
+	PMIC_AUXADC_IMP3,
+	PMIC_AUXADC_RQST0,
+	PMIC_AUXADC_RQST1,
+	PMIC_HK_TOP_WKEY,
+	PMIC_HK_TOP_RST_CON0,
+	PMIC_FGADC_R_CON0,
+	PMIC_AUXADC_REGS_MAX
+};
+
+enum mtk_pmic_auxadc_channels {
+	PMIC_AUXADC_CHAN_BATADC,
+	PMIC_AUXADC_CHAN_ISENSE,
+	PMIC_AUXADC_CHAN_VCDT,
+	PMIC_AUXADC_CHAN_BAT_TEMP,
+	PMIC_AUXADC_CHAN_BATID,
+	PMIC_AUXADC_CHAN_CHIP_TEMP,
+	PMIC_AUXADC_CHAN_VCORE_TEMP,
+	PMIC_AUXADC_CHAN_VPROC_TEMP,
+	PMIC_AUXADC_CHAN_VGPU_TEMP,
+	PMIC_AUXADC_CHAN_ACCDET,
+	PMIC_AUXADC_CHAN_VDCXO,
+	PMIC_AUXADC_CHAN_TSX_TEMP,
+	PMIC_AUXADC_CHAN_HPOFS_CAL,
+	PMIC_AUXADC_CHAN_DCXO_TEMP,
+	PMIC_AUXADC_CHAN_VBIF,
+	PMIC_AUXADC_CHAN_IBAT,
+	PMIC_AUXADC_CHAN_VBAT,
+	PMIC_AUXADC_CHAN_MAX
+};
+
+/**
+ * struct mt6359_auxadc - Main driver structure
+ * @dev:           Device pointer
+ * @regmap:        Regmap from SoC PMIC Wrapper
+ * @pdata:         PMIC specific platform data
+ * @lock:          Mutex lock for AUXADC reads
+ * @timed_out:     Signals whether the last read timed out
+ */
+struct mt6359_auxadc {
+	struct device *dev;
+	struct regmap *regmap;
+	const struct mtk_pmic_auxadc_pdata *pdata;
+	struct mutex lock;
+	bool timed_out;
+};
+
+/**
+ * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
+ * @req_idx:       Request register number
+ * @req_mask:      Bitmask to activate a channel
+ * @num_samples:   Number of AUXADC samples for averaging
+ * @r_numerator:   Resistance ratio numerator
+ * @r_denominator: Resistance ratio denominator
+ */
+struct mtk_pmic_auxadc_chan {
+	u8 req_idx;
+	u16 req_mask;
+	u16 num_samples;
+	u8 r_numerator;
+	u8 r_denominator;
+};
+
+/**
+ * struct mtk_pmic_auxadc_pdata - PMIC specific platform data
+ * @channels:       IIO specification of ADC channels
+ * @num_channels:   Number of ADC channels
+ * @desc:           PMIC AUXADC channel data
+ * @regs:           List of PMIC specific registers
+ * @sec_unlock_key: Security unlock key for HK_TOP writes
+ * @imp_adc_num:    ADC channel for IMP readings
+ * @read_imp:       Callback to read PMIC IMP channels
+ */
+struct mtk_pmic_auxadc_pdata {
+	const struct iio_chan_spec *channels;
+	int num_channels;
+	const struct mtk_pmic_auxadc_chan *desc;
+	const u16 *regs;
+	u16 sec_unlock_key;
+	u8 imp_adc_num;
+	int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
+};
+
+#define MTK_PMIC_ADC_CHAN(_ch_idx, _req_idx, _req_bit, _samples, _rnum, _rdiv)	\
+	[PMIC_AUXADC_CHAN_##_ch_idx] = {					\
+		.req_idx = _req_idx,						\
+		.req_mask = BIT(_req_bit),					\
+		.num_samples = _samples,					\
+		.r_numerator = _rnum,						\
+		.r_denominator = _rdiv,						\
+	}
+
+#define MTK_PMIC_IIO_CHAN(_model, _name, _ch_idx, _adc_idx, _nbits, _ch_type)	\
+{										\
+	.type = _ch_type,							\
+	.channel = _model##_AUXADC_##_ch_idx,					\
+	.address = _adc_idx,							\
+	.scan_index = PMIC_AUXADC_CHAN_##_ch_idx,				\
+	.datasheet_name = __stringify(_name),					\
+	.scan_type =  {								\
+		.sign = 'u',							\
+		.realbits = _nbits,						\
+		.storagebits = 16,						\
+		.endianness = IIO_CPU						\
+	},									\
+	.indexed = 1,								\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)	\
+}
+
+static const struct iio_chan_spec mt6357_auxadc_channels[] = {
+	MTK_PMIC_IIO_CHAN(MT6357, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6357, isense, ISENSE, 1, 12, IIO_CURRENT),
+	MTK_PMIC_IIO_CHAN(MT6357, cdt_v, VCDT, 2, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6357, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6357, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6357, acc_det, ACCDET, 5, 12, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6357, dcxo_v, VDCXO, 6, 12, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6357, tsx_temp, TSX_TEMP, 7, 15, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6357, hp_ofs_cal, HPOFS_CAL, 9, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6357, dcxo_temp, DCXO_TEMP, 36, 15, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6357, vcore_temp, VCORE_TEMP, 40, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6357, vproc_temp, VPROC_TEMP, 41, 12, IIO_TEMP),
+
+	/* IMP channels */
+	MTK_PMIC_IIO_CHAN(MT6357, batt_v, VBAT, 0, 15, IIO_VOLTAGE),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6357_auxadc_ch_desc[] = {
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(ISENSE, PMIC_AUXADC_RQST0, 0, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 0, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
+	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
+	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
+	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 5, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 6, 8, 1, 1),
+
+	/* IMP channels */
+	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 3, 1),
+};
+
+static const u16 mt6357_auxadc_regs[] = {
+	[PMIC_HK_TOP_RST_CON0]	= 0xf90,
+	[PMIC_AUXADC_DCM_CON]	= 0x122e,
+	[PMIC_AUXADC_ADC0]	= 0x1088,
+	[PMIC_AUXADC_IMP0]	= 0x119c,
+	[PMIC_AUXADC_IMP1]	= 0x119e,
+	[PMIC_AUXADC_RQST0]	= 0x110e,
+	[PMIC_AUXADC_RQST1]	= 0x1114,
+};
+
+static const struct iio_chan_spec mt6358_auxadc_channels[] = {
+	MTK_PMIC_IIO_CHAN(MT6358, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6358, cdt_v, VCDT, 2, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, acc_det, ACCDET, 5, 12, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6358, dcxo_v, VDCXO, 6, 12, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6358, tsx_temp, TSX_TEMP, 7, 15, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, hp_ofs_cal, HPOFS_CAL, 9, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6358, dcxo_temp, DCXO_TEMP, 10, 15, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, bif_v, VBIF, 11, 12, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6358, vcore_temp, VCORE_TEMP, 38, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, vproc_temp, VPROC_TEMP, 39, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6358, vgpu_temp, VGPU_TEMP, 40, 12, IIO_TEMP),
+
+	/* IMP channels */
+	MTK_PMIC_IIO_CHAN(MT6358, batt_v, VBAT, 0, 15, IIO_VOLTAGE),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6358_auxadc_ch_desc[] = {
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 3, 1),
+	MTK_PMIC_ADC_CHAN(VCDT, PMIC_AUXADC_RQST0, 0, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 2, 1),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
+	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
+	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
+	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
+	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 2, 1),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
+
+	/* IMP channels */
+	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
+};
+
+static const u16 mt6358_auxadc_regs[] = {
+	[PMIC_HK_TOP_RST_CON0]	= 0xf90,
+	[PMIC_AUXADC_DCM_CON]	= 0x1260,
+	[PMIC_AUXADC_ADC0]	= 0x1088,
+	[PMIC_AUXADC_IMP0]	= 0x1208,
+	[PMIC_AUXADC_IMP1]	= 0x120a,
+	[PMIC_AUXADC_RQST0]	= 0x1108,
+	[PMIC_AUXADC_RQST1]	= 0x110a,
+};
+
+static const struct iio_chan_spec mt6359_auxadc_channels[] = {
+	MTK_PMIC_IIO_CHAN(MT6359, bat_adc, BATADC, 0, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6359, batt_temp, BAT_TEMP, 3, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6359, chip_temp, CHIP_TEMP, 4, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6359, acc_det, ACCDET, 5, 12, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6359, dcxo_v, VDCXO, 6, 12, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6359, tsx_temp, TSX_TEMP, 7, 15, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6359, hp_ofs_cal, HPOFS_CAL, 9, 15, IIO_RESISTANCE),
+	MTK_PMIC_IIO_CHAN(MT6359, dcxo_temp, DCXO_TEMP, 10, 15, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6359, bif_v, VBIF, 11, 12, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6359, vcore_temp, VCORE_TEMP, 30, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6359, vproc_temp, VPROC_TEMP, 31, 12, IIO_TEMP),
+	MTK_PMIC_IIO_CHAN(MT6359, vgpu_temp, VGPU_TEMP, 32, 12, IIO_TEMP),
+
+	/* IMP channels */
+	MTK_PMIC_IIO_CHAN(MT6359, batt_v, VBAT, 0, 15, IIO_VOLTAGE),
+	MTK_PMIC_IIO_CHAN(MT6359, batt_i, IBAT, 0, 15, IIO_CURRENT),
+};
+
+static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
+	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
+	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
+	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
+	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
+	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
+	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
+	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
+	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
+	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
+
+	/* IMP channels */
+	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
+	MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
+};
+
+static const u16 mt6359_auxadc_regs[] = {
+	[PMIC_FGADC_R_CON0]	= 0xd88,
+	[PMIC_HK_TOP_WKEY]	= 0xfb4,
+	[PMIC_HK_TOP_RST_CON0]	= 0xf90,
+	[PMIC_AUXADC_RQST0]	= 0x1108,
+	[PMIC_AUXADC_RQST1]	= 0x110a,
+	[PMIC_AUXADC_ADC0]	= 0x1088,
+	[PMIC_AUXADC_IMP0]	= 0x1208,
+	[PMIC_AUXADC_IMP1]	= 0x120a,
+	[PMIC_AUXADC_IMP3]	= 0x120e,
+};
+
+static void mt6358_stop_imp_conv(struct mt6359_auxadc *adc_dev)
+{
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	struct regmap *regmap = adc_dev->regmap;
+
+	regmap_set_bits(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6358_IMP0_CLEAR);
+	regmap_clear_bits(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6358_IMP0_CLEAR);
+	regmap_clear_bits(regmap, pdata->regs[PMIC_AUXADC_IMP1], MT6358_IMP1_AUTOREPEAT_EN);
+	regmap_clear_bits(regmap, pdata->regs[PMIC_AUXADC_DCM_CON], MT6358_DCM_CK_SW_EN);
+}
+
+static int mt6358_start_imp_conv(struct mt6359_auxadc *adc_dev)
+{
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	struct regmap *regmap = adc_dev->regmap;
+	u32 val;
+	int ret;
+
+	regmap_set_bits(regmap, pdata->regs[PMIC_AUXADC_DCM_CON], MT6358_DCM_CK_SW_EN);
+	regmap_set_bits(regmap, pdata->regs[PMIC_AUXADC_IMP1], MT6358_IMP1_AUTOREPEAT_EN);
+
+	ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
+				       val, !!(val & MT6358_IMP0_IRQ_RDY),
+				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
+	if (ret) {
+		mt6358_stop_imp_conv(adc_dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mt6358_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
+{
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	struct regmap *regmap = adc_dev->regmap;
+	u16 reg_adc0 = pdata->regs[PMIC_AUXADC_ADC0];
+	int val_v, ret;
+
+	ret = mt6358_start_imp_conv(adc_dev);
+	if (ret)
+		return ret;
+
+	/* Read the params before stopping */
+	regmap_read(regmap, reg_adc0 + PMIC_AUXADC_ADCx(pdata->imp_adc_num), &val_v);
+
+	mt6358_stop_imp_conv(adc_dev);
+
+	*vbat = val_v;
+	*ibat = 0;
+
+	return 0;
+}
+
+static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
+{
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	struct regmap *regmap = adc_dev->regmap;
+	int val_v, val_i, ret;
+	u32 val;
+
+	/* Start conversion */
+	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
+	ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
+				       val, !!(val & MT6359_IMP1_IRQ_RDY),
+				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
+
+	/* Stop conversion regardless of the result */
+	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
+	if (ret)
+		return ret;
+
+	/* If it succeeded, wait for the registers to be populated */
+	usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
+
+	ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
+	if (ret)
+		return ret;
+
+	*vbat = val_v;
+	*ibat = val_i;
+
+	return 0;
+}
+
+static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
+	.channels = mt6357_auxadc_channels,
+	.num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
+	.desc = mt6357_auxadc_ch_desc,
+	.regs = mt6357_auxadc_regs,
+	.imp_adc_num = MT6357_IMP_ADC_NUM,
+	.read_imp = mt6358_read_imp,
+};
+
+static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
+	.channels = mt6358_auxadc_channels,
+	.num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
+	.desc = mt6358_auxadc_ch_desc,
+	.regs = mt6358_auxadc_regs,
+	.imp_adc_num = MT6358_IMP_ADC_NUM,
+	.read_imp = mt6358_read_imp,
+};
+
+static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
+	.channels = mt6359_auxadc_channels,
+	.num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
+	.desc = mt6359_auxadc_ch_desc,
+	.regs = mt6359_auxadc_regs,
+	.sec_unlock_key = 0x6359,
+	.read_imp = mt6359_read_imp,
+};
+
+static void mt6359_auxadc_reset(struct mt6359_auxadc *adc_dev)
+{
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	struct regmap *regmap = adc_dev->regmap;
+
+	/* Unlock HK_TOP writes */
+	if (pdata->sec_unlock_key)
+		regmap_write(regmap, pdata->regs[PMIC_HK_TOP_WKEY], pdata->sec_unlock_key);
+
+	/* Assert ADC reset */
+	regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
+
+	/* De-assert ADC reset */
+	regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
+
+	/* Lock HK_TOP writes again */
+	if (pdata->sec_unlock_key)
+		regmap_write(regmap, pdata->regs[PMIC_HK_TOP_WKEY], 0);
+}
+
+static int mt6359_auxadc_read_adc(struct mt6359_auxadc *adc_dev,
+				  const struct iio_chan_spec *chan, int *out)
+{
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
+	struct regmap *regmap = adc_dev->regmap;
+	u32 val;
+	int ret;
+
+	/* Request to start sampling for ADC channel */
+	ret = regmap_write(regmap, pdata->regs[desc->req_idx], desc->req_mask);
+	if (ret)
+		return ret;
+
+	/* Wait until all samples are averaged */
+	usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
+		     (desc->num_samples + 1) * AUXADC_AVG_TIME_US);
+
+	ret = regmap_read_poll_timeout(regmap,
+				       (pdata->regs[PMIC_AUXADC_ADC0] +
+					PMIC_AUXADC_ADCx(chan->address)),
+				       val, (val & PMIC_AUXADC_RDY_BIT),
+				       AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/* Stop sampling */
+	regmap_write(regmap, pdata->regs[desc->req_idx], 0);
+
+	*out = val & GENMASK(chan->scan_type.realbits - 1, 0);
+	return 0;
+}
+
+static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan, char *label)
+{
+	return sysfs_emit(label, "%s\n", chan->datasheet_name);
+}
+
+static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  int *val, int *val2, long mask)
+{
+	struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
+	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
+	const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
+	int ret;
+
+	if (mask == IIO_CHAN_INFO_SCALE) {
+		*val = desc->r_numerator * AUXADC_VOLT_FULL;
+
+		if (desc->r_denominator > 1) {
+			*val2 = desc->r_denominator;
+			return IIO_VAL_FRACTIONAL;
+		}
+
+		return IIO_VAL_INT;
+	}
+
+	mutex_lock(&adc_dev->lock);
+
+	switch (chan->scan_index) {
+	case PMIC_AUXADC_CHAN_IBAT:
+		ret = adc_dev->pdata->read_imp(adc_dev, val2, val);
+		break;
+	case PMIC_AUXADC_CHAN_VBAT:
+		ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
+		break;
+	default:
+		ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
+		break;
+	}
+
+	mutex_unlock(&adc_dev->lock);
+
+	if (ret) {
+		/*
+		 * If we get more than one timeout, it's possible that the
+		 * AUXADC is stuck: perform a full reset to recover it.
+		 */
+		if (ret == -ETIMEDOUT) {
+			if (adc_dev->timed_out) {
+				dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
+				mt6359_auxadc_reset(adc_dev);
+			}
+			adc_dev->timed_out = true;
+		}
+		return ret;
+	}
+	adc_dev->timed_out = false;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info mt6359_auxadc_info = {
+	.read_label = mt6359_auxadc_read_label,
+	.read_raw = mt6359_auxadc_read_raw,
+};
+
+static int mt6359_auxadc_probe(struct platform_device *pdev)
+{
+	struct device *mt6397_mfd_dev = pdev->dev.parent;
+	struct mt6359_auxadc *adc_dev;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int ret;
+
+	/* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
+	regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
+	if (!regmap)
+		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	adc_dev = iio_priv(indio_dev);
+	adc_dev->regmap = regmap;
+	adc_dev->dev = &pdev->dev;
+
+	adc_dev->pdata = device_get_match_data(&pdev->dev);
+	if (!adc_dev->pdata)
+		return -EINVAL;
+
+	mutex_init(&adc_dev->lock);
+
+	mt6359_auxadc_reset(adc_dev);
+
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->info = &mt6359_auxadc_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = adc_dev->pdata->channels;
+	indio_dev->num_channels = adc_dev->pdata->num_channels;
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
+
+	return 0;
+}
+
+static const struct of_device_id mt6359_auxadc_of_match[] = {
+	{ .compatible = "mediatek,mt6357-auxadc", .data = &mt6357_pdata },
+	{ .compatible = "mediatek,mt6358-auxadc", .data = &mt6358_pdata },
+	{ .compatible = "mediatek,mt6359-auxadc", .data = &mt6359_pdata },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mt6359_auxadc_of_match);
+
+static struct platform_driver mt6359_auxadc_driver = {
+	.driver = {
+		.name = "mt6359-auxadc",
+		.of_match_table = mt6359_auxadc_of_match,
+	},
+	.probe	= mt6359_auxadc_probe,
+};
+module_platform_driver(mt6359_auxadc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>");
+MODULE_DESCRIPTION("MediaTek MT6359 PMIC AUXADC Driver");
-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[Andy Shevchenko]

On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Add a driver to support reading the Auxiliary ADC IP found in the
> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>
> This driver provides multiple ADC channels for system monitoring,
> such as battery voltage, PMIC temperature, PMIC-internal voltage
> regulators temperature, and others.

> ---

Here is no explanation on how this is differ to any of the three
existing drivers? I.o.w. why do we need a brand new one?

...

+ bits.h

> +#include <linux/delay.h>

> +#include <linux/kernel.h>

Why?

+ mod_devicetable.h
> +#include <linux/module.h>

> +#include <linux/of.h>

Why?

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

+ types.h

+ blank line

> +#include <linux/iio/iio.h>

+ Blank line

> +#include <linux/mfd/mt6397/core.h>

...

> +#define PMIC_RG_RESET_VAL              (BIT(0) | BIT(3))

In this form it requires a comment explaining each mentioned bit.

> +#define PMIC_AUXADC_ADCx(x)            ((x) << 1)

Seems like a useless macro, it occupies much more space than in-place shift.

...

> +#define MT6358_IMP0_CLEAR              (BIT(14) | BIT(7))

As per above.

...

> +/**
> + * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
> + * @req_idx:       Request register number
> + * @req_mask:      Bitmask to activate a channel
> + * @num_samples:   Number of AUXADC samples for averaging
> + * @r_numerator:   Resistance ratio numerator
> + * @r_denominator: Resistance ratio denominator
> + */
> +struct mtk_pmic_auxadc_chan {
> +       u8 req_idx;
> +       u16 req_mask;
> +       u16 num_samples;

> +       u8 r_numerator;
> +       u8 r_denominator;

Can you add struct u8_fract to the math.h and use it? I will Ack/R the
respective patch.

> +};

...

> +struct mtk_pmic_auxadc_pdata {
> +       const struct iio_chan_spec *channels;
> +       int num_channels;

Why signed?

> +       const struct mtk_pmic_auxadc_chan *desc;
> +       const u16 *regs;
> +       u16 sec_unlock_key;
> +       u8 imp_adc_num;
> +       int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> +};

...

> +static const u16 mt6357_auxadc_regs[] = {
> +       [PMIC_HK_TOP_RST_CON0]  = 0xf90,

Can we use the fixed-width values so all of them will look nice and
easy to parse?

> +       [PMIC_AUXADC_DCM_CON]   = 0x122e,
> +       [PMIC_AUXADC_ADC0]      = 0x1088,
> +       [PMIC_AUXADC_IMP0]      = 0x119c,
> +       [PMIC_AUXADC_IMP1]      = 0x119e,
> +       [PMIC_AUXADC_RQST0]     = 0x110e,
> +       [PMIC_AUXADC_RQST1]     = 0x1114,
> +};

...

> +static const u16 mt6358_auxadc_regs[] = {

Ditto.

> +       [PMIC_HK_TOP_RST_CON0]  = 0xf90,
> +       [PMIC_AUXADC_DCM_CON]   = 0x1260,
> +       [PMIC_AUXADC_ADC0]      = 0x1088,
> +       [PMIC_AUXADC_IMP0]      = 0x1208,
> +       [PMIC_AUXADC_IMP1]      = 0x120a,
> +       [PMIC_AUXADC_RQST0]     = 0x1108,
> +       [PMIC_AUXADC_RQST1]     = 0x110a,
> +};

...

> +static const u16 mt6359_auxadc_regs[] = {

Ditto.

> +       [PMIC_FGADC_R_CON0]     = 0xd88,
> +       [PMIC_HK_TOP_WKEY]      = 0xfb4,
> +       [PMIC_HK_TOP_RST_CON0]  = 0xf90,
> +       [PMIC_AUXADC_RQST0]     = 0x1108,
> +       [PMIC_AUXADC_RQST1]     = 0x110a,
> +       [PMIC_AUXADC_ADC0]      = 0x1088,
> +       [PMIC_AUXADC_IMP0]      = 0x1208,
> +       [PMIC_AUXADC_IMP1]      = 0x120a,
> +       [PMIC_AUXADC_IMP3]      = 0x120e,
> +};

...

> +       ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
> +                                      val, !!(val & MT6358_IMP0_IRQ_RDY),
> +                                      IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> +       if (ret) {
> +               mt6358_stop_imp_conv(adc_dev);

> +               return ret;
> +       }
> +
> +       return 0;

  if (ret)
    foo()

  return ret;


...

> +       int val_v, ret;

Why is val_v signed?

...

> +       int val_v, val_i, ret;

Ditto for all val_*.

...

> +       /* If it succeeded, wait for the registers to be populated */
> +       usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);

fsleep() ?

...

> +       /* Assert ADC reset */
> +       regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);

No required delay in between?

> +       /* De-assert ADC reset */
> +       regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);

...

> +       /* Wait until all samples are averaged */
> +       usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
> +                    (desc->num_samples + 1) * AUXADC_AVG_TIME_US);

fsleep()

...

> +       ret = regmap_read_poll_timeout(regmap,
> +                                      (pdata->regs[PMIC_AUXADC_ADC0] +
> +                                       PMIC_AUXADC_ADCx(chan->address)),

Drop unneeded parentheses and possibly make it one line.

> +                                      val, (val & PMIC_AUXADC_RDY_BIT),

Unneeded parentheses.

> +                                      AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> +       if (ret)
> +               return ret;

...

> +       mutex_lock(&adc_dev->lock);

Why not use cleanup.h?

...

> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> +{

  struct device *dev = &pdev->dev;

> +       struct device *mt6397_mfd_dev = pdev->dev.parent;

  ... = dev->parent;

> +       struct mt6359_auxadc *adc_dev;
> +       struct iio_dev *indio_dev;
> +       struct regmap *regmap;
> +       int ret;
> +
> +       /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> +       regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> +       if (!regmap)
> +               return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> +
> +       indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       adc_dev = iio_priv(indio_dev);
> +       adc_dev->regmap = regmap;
> +       adc_dev->dev = &pdev->dev;
> +
> +       adc_dev->pdata = device_get_match_data(&pdev->dev);
> +       if (!adc_dev->pdata)
> +               return -EINVAL;
> +
> +       mutex_init(&adc_dev->lock);
> +
> +       mt6359_auxadc_reset(adc_dev);
> +
> +       indio_dev->dev.parent = &pdev->dev;
> +       indio_dev->name = dev_name(&pdev->dev);
> +       indio_dev->info = &mt6359_auxadc_info;
> +       indio_dev->modes = INDIO_DIRECT_MODE;
> +       indio_dev->channels = adc_dev->pdata->channels;
> +       indio_dev->num_channels = adc_dev->pdata->num_channels;
> +
> +       ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +       if (ret < 0)

Why  ' < 0' ?

> +               return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
> +
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[AngeloGioacchino Del Regno]

Il 30/05/24 15:34, Andy Shevchenko ha scritto:
> On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Add a driver to support reading the Auxiliary ADC IP found in the
>> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>>
>> This driver provides multiple ADC channels for system monitoring,
>> such as battery voltage, PMIC temperature, PMIC-internal voltage
>> regulators temperature, and others.
> 
>> ---
> 
> Here is no explanation on how this is differ to any of the three
> existing drivers? I.o.w. why do we need a brand new one?
> 

Not a SoC AUXADC but a PMIC AUXADC, different register layout and different
handling for configuration, reset, and reading.

So okay I'm adding a nicer text of what I just wrote to the commit description.

> ...
> 
> + bits.h
> 
>> +#include <linux/delay.h>
> 
>> +#include <linux/kernel.h>
> 
> Why?
> 

Because I forgot to cleanup the headers :-\

> + mod_devicetable.h
>> +#include <linux/module.h>
> 
>> +#include <linux/of.h>
> 
> Why?
> 

Same reason :')

...And yes that should be linux/property.h instead, for device_get_match_data().

>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
> 
> + types.h
> 
> + blank line
> 
>> +#include <linux/iio/iio.h>
> 
> + Blank line
> 
>> +#include <linux/mfd/mt6397/core.h>
> 
> ...
> 
>> +#define PMIC_RG_RESET_VAL              (BIT(0) | BIT(3))
> 
> In this form it requires a comment explaining each mentioned bit.
> 

I don't have an explanation for this, I know it's two different bits from some
reveng, but the downstream driver declares that simply as 0x9.

Should I just "mask" this as 0x9 instead?

>> +#define PMIC_AUXADC_ADCx(x)            ((x) << 1)
> 
> Seems like a useless macro, it occupies much more space than in-place shift.
> 

Well that was done to enhance human readability, but okay I will just use an
in-place shift.

> ...
> 
>> +#define MT6358_IMP0_CLEAR              (BIT(14) | BIT(7))
> 
> As per above.
> 

Same, I don't have any explanation for that.

If you prefer, I can define this as 0x4080, but honestly I prefer keeping
it as-is since I am sure it's not a magic number but really two bits to flip
in a register.

> ...
> 
>> +/**
>> + * struct mtk_pmic_auxadc_chan - PMIC AUXADC channel data
>> + * @req_idx:       Request register number
>> + * @req_mask:      Bitmask to activate a channel
>> + * @num_samples:   Number of AUXADC samples for averaging
>> + * @r_numerator:   Resistance ratio numerator
>> + * @r_denominator: Resistance ratio denominator
>> + */
>> +struct mtk_pmic_auxadc_chan {
>> +       u8 req_idx;
>> +       u16 req_mask;
>> +       u16 num_samples;
> 
>> +       u8 r_numerator;
>> +       u8 r_denominator;
> 
> Can you add struct u8_fract to the math.h and use it? I will Ack/R the
> respective patch.
> 

Yeah, I did that exactly because u8_fract wasn't there and I didn't want
to waste more bits, but since you just asked for it... well, I'm happier :-)

>> +};
> 
> ...
> 
>> +struct mtk_pmic_auxadc_pdata {
>> +       const struct iio_chan_spec *channels;
>> +       int num_channels;
> 
> Why signed?
> 

Yeah, that doesn't make any sense, that's going to be u8.

>> +       const struct mtk_pmic_auxadc_chan *desc;
>> +       const u16 *regs;
>> +       u16 sec_unlock_key;
>> +       u8 imp_adc_num;
>> +       int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
>> +};
> 
> ...
> 
>> +static const u16 mt6357_auxadc_regs[] = {
>> +       [PMIC_HK_TOP_RST_CON0]  = 0xf90,
> 
> Can we use the fixed-width values so all of them will look nice and
> easy to parse?
> 

We obviously can, let's do that.

>> +       [PMIC_AUXADC_DCM_CON]   = 0x122e,
>> +       [PMIC_AUXADC_ADC0]      = 0x1088,
>> +       [PMIC_AUXADC_IMP0]      = 0x119c,
>> +       [PMIC_AUXADC_IMP1]      = 0x119e,
>> +       [PMIC_AUXADC_RQST0]     = 0x110e,
>> +       [PMIC_AUXADC_RQST1]     = 0x1114,
>> +};
> 
> ...
> 
>> +static const u16 mt6358_auxadc_regs[] = {
> 
> Ditto.
> 
>> +       [PMIC_HK_TOP_RST_CON0]  = 0xf90,
>> +       [PMIC_AUXADC_DCM_CON]   = 0x1260,
>> +       [PMIC_AUXADC_ADC0]      = 0x1088,
>> +       [PMIC_AUXADC_IMP0]      = 0x1208,
>> +       [PMIC_AUXADC_IMP1]      = 0x120a,
>> +       [PMIC_AUXADC_RQST0]     = 0x1108,
>> +       [PMIC_AUXADC_RQST1]     = 0x110a,
>> +};
> 
> ...
> 
>> +static const u16 mt6359_auxadc_regs[] = {
> 
> Ditto.
> 
>> +       [PMIC_FGADC_R_CON0]     = 0xd88,
>> +       [PMIC_HK_TOP_WKEY]      = 0xfb4,
>> +       [PMIC_HK_TOP_RST_CON0]  = 0xf90,
>> +       [PMIC_AUXADC_RQST0]     = 0x1108,
>> +       [PMIC_AUXADC_RQST1]     = 0x110a,
>> +       [PMIC_AUXADC_ADC0]      = 0x1088,
>> +       [PMIC_AUXADC_IMP0]      = 0x1208,
>> +       [PMIC_AUXADC_IMP1]      = 0x120a,
>> +       [PMIC_AUXADC_IMP3]      = 0x120e,
>> +};
> 
> ...
> 
>> +       ret = regmap_read_poll_timeout(adc_dev->regmap, pdata->regs[PMIC_AUXADC_IMP0],
>> +                                      val, !!(val & MT6358_IMP0_IRQ_RDY),
>> +                                      IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> +       if (ret) {
>> +               mt6358_stop_imp_conv(adc_dev);
> 
>> +               return ret;
>> +       }
>> +
>> +       return 0;
> 
>    if (ret)
>      foo()
> 
>    return ret;
> 
> 
> ...
> 
>> +       int val_v, ret;
> 
> Why is val_v signed?
> 
> ...
> 
>> +       int val_v, val_i, ret;
> 
> Ditto for all val_*.
> 

Yup, changed to u32.

> ...
> 
>> +       /* If it succeeded, wait for the registers to be populated */
>> +       usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
> 
> fsleep() ?
> 

Okay

> ...
> 
>> +       /* Assert ADC reset */
>> +       regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
> 
> No required delay in between?
> 

No, as strange as it may look, there is no delay required in between: this is
because the register R/W is behind the PMIC Wrapper as much as all of the other
MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes.

>> +       /* De-assert ADC reset */
>> +       regmap_clear_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
> 
> ...
> 
>> +       /* Wait until all samples are averaged */
>> +       usleep_range(desc->num_samples * AUXADC_AVG_TIME_US,
>> +                    (desc->num_samples + 1) * AUXADC_AVG_TIME_US);
> 
> fsleep()
> 
> ...
> 
>> +       ret = regmap_read_poll_timeout(regmap,
>> +                                      (pdata->regs[PMIC_AUXADC_ADC0] +
>> +                                       PMIC_AUXADC_ADCx(chan->address)),
> 
> Drop unneeded parentheses and possibly make it one line.
> 
>> +                                      val, (val & PMIC_AUXADC_RDY_BIT),
> 
> Unneeded parentheses.
> 

Yeah, forgot those there during cleanup, whoops!

>> +                                      AUXADC_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> +       if (ret)
>> +               return ret;
> 
> ...
> 
>> +       mutex_lock(&adc_dev->lock);
> 
> Why not use cleanup.h?
> 

I want to unlock the mutex immediately right after executing read_imp() or
mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex
is being held, as that makes no sense for this driver.

Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they
require better documentation as, for example, I don't understand when the
guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed
to release it.


> ...
> 
>> +static int mt6359_auxadc_probe(struct platform_device *pdev)
>> +{
> 
>    struct device *dev = &pdev->dev;

Yeah without that it simply looked more "beautiful" to read, but okay, added.

> 
>> +       struct device *mt6397_mfd_dev = pdev->dev.parent;
> 
>    ... = dev->parent;
> 
>> +       struct mt6359_auxadc *adc_dev;
>> +       struct iio_dev *indio_dev;
>> +       struct regmap *regmap;
>> +       int ret;
>> +
>> +       /* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
>> +       regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
>> +       if (!regmap)
>> +               return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
>> +
>> +       indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
>> +       if (!indio_dev)
>> +               return -ENOMEM;
>> +
>> +       adc_dev = iio_priv(indio_dev);
>> +       adc_dev->regmap = regmap;
>> +       adc_dev->dev = &pdev->dev;
>> +
>> +       adc_dev->pdata = device_get_match_data(&pdev->dev);
>> +       if (!adc_dev->pdata)
>> +               return -EINVAL;
>> +
>> +       mutex_init(&adc_dev->lock);
>> +
>> +       mt6359_auxadc_reset(adc_dev);
>> +
>> +       indio_dev->dev.parent = &pdev->dev;
>> +       indio_dev->name = dev_name(&pdev->dev);
>> +       indio_dev->info = &mt6359_auxadc_info;
>> +       indio_dev->modes = INDIO_DIRECT_MODE;
>> +       indio_dev->channels = adc_dev->pdata->channels;
>> +       indio_dev->num_channels = adc_dev->pdata->num_channels;
>> +
>> +       ret = devm_iio_device_register(&pdev->dev, indio_dev);
>> +       if (ret < 0)
> 
> Why  ' < 0' ?
> 

Uuuuuh.. well... I have no idea why this "< 0" is in there, I must have gremlins
living inside my keyboard, secretly inserting those checks when I'm not looking.

Must definitely be that.

Jokes apart, thanks for the review!
...a v2 will come asap, along with the math.h change :-)

Cheers!
Angelo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[Andy Shevchenko]

On Tue, Jun 4, 2024 at 1:38 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
> Il 30/05/24 15:34, Andy Shevchenko ha scritto:
> > On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:

...

> >> +#define PMIC_RG_RESET_VAL              (BIT(0) | BIT(3))
> >
> > In this form it requires a comment explaining each mentioned bit.
>
> I don't have an explanation for this, I know it's two different bits from some
> reveng, but the downstream driver declares that simply as 0x9.
>
> Should I just "mask" this as 0x9 instead?

In this case for all of the questionable forms, please add a oneline
comment suggesting that "these are different bits without known
purpose of each." or something like that.

...

> >> +#define MT6358_IMP0_CLEAR              (BIT(14) | BIT(7))
> >
> > As per above.
> >
>
> Same, I don't have any explanation for that.
>
> If you prefer, I can define this as 0x4080, but honestly I prefer keeping
> it as-is since I am sure it's not a magic number but really two bits to flip
> in a register.

As per above.

...

> >> +       u8 r_numerator;
> >> +       u8 r_denominator;
> >
> > Can you add struct u8_fract to the math.h and use it? I will Ack/R the
> > respective patch.
>
> Yeah, I did that exactly because u8_fract wasn't there and I didn't want
> to waste more bits, but since you just asked for it... well, I'm happier :-)

Note, it's enough to have my Rb tag and route that change via IIO
tree. We have done similar way for other changes in math.h (or aline)
in the past.

...

> >> +       /* Assert ADC reset */
> >> +       regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
> >
> > No required delay in between?
>
> No, as strange as it may look, there is no delay required in between: this is
> because the register R/W is behind the PMIC Wrapper as much as all of the other
> MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes.

Maybe a comment?

...

> >> +       mutex_lock(&adc_dev->lock);
> >
> > Why not use cleanup.h?
>
> I want to unlock the mutex immediately right after executing read_imp() or
> mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex
> is being held, as that makes no sense for this driver.

That's why we have scoped_guard(). Exactly for such cases.

> Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they
> require better documentation as, for example, I don't understand when the
> guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed
> to release it.

They are cryptic due to limitations in C language. But for the end
user it doesn't matter. The behaviour is well understandable and makes
code cleaner and less prone for errors such as missing unlocks. So,
please use cleanup.h.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[AngeloGioacchino Del Regno]

Il 04/06/24 13:05, Andy Shevchenko ha scritto:
> On Tue, Jun 4, 2024 at 1:38 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>> Il 30/05/24 15:34, Andy Shevchenko ha scritto:
>>> On Thu, May 30, 2024 at 12:34 PM AngeloGioacchino Del Regno
>>> <angelogioacchino.delregno@collabora.com> wrote:
> 
> ...
> 
>>>> +#define PMIC_RG_RESET_VAL              (BIT(0) | BIT(3))
>>>
>>> In this form it requires a comment explaining each mentioned bit.
>>
>> I don't have an explanation for this, I know it's two different bits from some
>> reveng, but the downstream driver declares that simply as 0x9.
>>
>> Should I just "mask" this as 0x9 instead?
> 
> In this case for all of the questionable forms, please add a oneline
> comment suggesting that "these are different bits without known
> purpose of each." or something like that.
> 

Perfect. Comment added.

> ...
> 
>>>> +#define MT6358_IMP0_CLEAR              (BIT(14) | BIT(7))
>>>
>>> As per above.
>>>
>>
>> Same, I don't have any explanation for that.
>>
>> If you prefer, I can define this as 0x4080, but honestly I prefer keeping
>> it as-is since I am sure it's not a magic number but really two bits to flip
>> in a register.
> 
> As per above.
> 
> ...
> 
>>>> +       u8 r_numerator;
>>>> +       u8 r_denominator;
>>>
>>> Can you add struct u8_fract to the math.h and use it? I will Ack/R the
>>> respective patch.
>>
>> Yeah, I did that exactly because u8_fract wasn't there and I didn't want
>> to waste more bits, but since you just asked for it... well, I'm happier :-)
> 
> Note, it's enough to have my Rb tag and route that change via IIO
> tree. We have done similar way for other changes in math.h (or aline)
> in the past.
> 

Sure.

> ...
> 
>>>> +       /* Assert ADC reset */
>>>> +       regmap_set_bits(regmap, pdata->regs[PMIC_HK_TOP_RST_CON0], PMIC_RG_RESET_VAL);
>>>
>>> No required delay in between?
>>
>> No, as strange as it may look, there is no delay required in between: this is
>> because the register R/W is behind the PMIC Wrapper as much as all of the other
>> MediaTek PMIC (sub)devices, so, missing delays was intentional here, yes.
> 
> Maybe a comment?
> 

Done :-)

/* De-assert ADC reset. No wait required, as pwrap takes care of that for us. */

> ...
> 
>>>> +       mutex_lock(&adc_dev->lock);
>>>
>>> Why not use cleanup.h?
>>
>> I want to unlock the mutex immediately right after executing read_imp() or
>> mt6359_auxadc_read_adc(), and I don't want the reset to be done while a mutex
>> is being held, as that makes no sense for this driver.
> 
> That's why we have scoped_guard(). Exactly for such cases.
> 

Thanks for the hint, looking at other usages that was straightforward.

>> Besides, I find the macros in cleanup.h to be cryptic - in my opinion, they
>> require better documentation as, for example, I don't understand when the
>> guard(mutex)(my_mutex) is supposed to acquire the lock and when it's supposed
>> to release it.
> 
> They are cryptic due to limitations in C language. But for the end
> user it doesn't matter. The behaviour is well understandable and makes
> code cleaner and less prone for errors such as missing unlocks. So,
> please use cleanup.h.
> 

Indeed, but my point was that the documentation can (and probably should)
be improved.


Cheers,
Angelo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[Jonathan Cameron]

On Thu, 30 May 2024 11:34:08 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Add a driver to support reading the Auxiliary ADC IP found in the
> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
> 
> This driver provides multiple ADC channels for system monitoring,
> such as battery voltage, PMIC temperature, PMIC-internal voltage
> regulators temperature, and others.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

I'll leave you to answer the 'why a new driver' in response to Andy's review
and just assume it makes sense whilst reviewing this.

What are IMP channels?

A few additional comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index edb32ce2af02..da7d4452b1e0 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MCP3564) += mcp3564.o
>  obj-$(CONFIG_MCP3911) += mcp3911.o
> +obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
>  obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
>  obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
> new file mode 100644
> index 000000000000..0481bd3f0144
> --- /dev/null
> +++ b/drivers/iio/adc/mt6359-auxadc.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MediaTek MT6359 PMIC AUXADC IIO driver
> + *
> + * Copyright (c) 2021 MediaTek Inc.
> + * Copyright (c) 2024 Collabora Ltd
> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/mt6397/core.h>
> +
> +#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
> +#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
> +#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>

So I 'guess' these headers are dt-bindings because you want
to consume them from other drivers?  That's fine, but if so please
add info on that to the DT binding patch.

> +/**
> + * struct mt6359_auxadc - Main driver structure
> + * @dev:           Device pointer
> + * @regmap:        Regmap from SoC PMIC Wrapper
> + * @pdata:         PMIC specific platform data
> + * @lock:          Mutex lock for AUXADC reads

Expand on this mutex comment.  What is it protecting?
I think it's about ensuring they are serialized to ensure configuration
is not changed during the read sequence.

> + * @timed_out:     Signals whether the last read timed out
> + */
> +struct mt6359_auxadc {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	const struct mtk_pmic_auxadc_pdata *pdata;
> +	struct mutex lock;
> +	bool timed_out;
> +};
> +

> +/**
> + * struct mtk_pmic_auxadc_pdata - PMIC specific platform data

I'm not sure this is conventionally what we think of as platform
data.  This is chip specific stuff. Platform data tends to be
more about how things are wired up etc.  A common term for this
stuff is chip_info.

> + * @channels:       IIO specification of ADC channels
> + * @num_channels:   Number of ADC channels
> + * @desc:           PMIC AUXADC channel data
> + * @regs:           List of PMIC specific registers
> + * @sec_unlock_key: Security unlock key for HK_TOP writes
> + * @imp_adc_num:    ADC channel for IMP readings
> + * @read_imp:       Callback to read PMIC IMP channels
> + */
> +struct mtk_pmic_auxadc_pdata {
> +	const struct iio_chan_spec *channels;
> +	int num_channels;
> +	const struct mtk_pmic_auxadc_chan *desc;
> +	const u16 *regs;
> +	u16 sec_unlock_key;
> +	u8 imp_adc_num;
> +	int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> +};
> +

> +
> +static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
> +	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
> +	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
> +	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
> +	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
> +	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
> +	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
> +	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
> +	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
> +	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
> +	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
> +	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
> +	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
> +
> +	/* IMP channels */
What are these? Expand IMP perhaps!

> +	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
> +	MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
> +};


> +static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
> +{
> +	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> +	struct regmap *regmap = adc_dev->regmap;
> +	int val_v, val_i, ret;
> +	u32 val;
> +
> +	/* Start conversion */
> +	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
> +	ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
> +				       val, !!(val & MT6359_IMP1_IRQ_RDY),

The condition is just as true or false without the !! so drop those.

> +				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> +
> +	/* Stop conversion regardless of the result */
> +	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
> +	if (ret)
> +		return ret;
> +
> +	/* If it succeeded, wait for the registers to be populated */
> +	usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
> +
> +	ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
> +	if (ret)
> +		return ret;

Why read them both if only one is wanted?  Do you need to read the data
for some reason - i.e. to allow for fresh reads or clear some status bit
or similar?  If so add a comment.  Otherwise easy to do

	if (vbat) {
		int val_v;
		ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
		if (ret)
			return ret;
		*vbat = val_v;
	}
etc


> +
> +	*vbat = val_v;
> +	*ibat = val_i;
> +
> +	return 0;
> +}
> +
> +static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
> +	.channels = mt6357_auxadc_channels,
> +	.num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
> +	.desc = mt6357_auxadc_ch_desc,
> +	.regs = mt6357_auxadc_regs,
> +	.imp_adc_num = MT6357_IMP_ADC_NUM,
> +	.read_imp = mt6358_read_imp,
> +};
> +
> +static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
> +	.channels = mt6358_auxadc_channels,
> +	.num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
> +	.desc = mt6358_auxadc_ch_desc,
> +	.regs = mt6358_auxadc_regs,
> +	.imp_adc_num = MT6358_IMP_ADC_NUM,
> +	.read_imp = mt6358_read_imp,
> +};
> +
> +static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
> +	.channels = mt6359_auxadc_channels,
> +	.num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
> +	.desc = mt6359_auxadc_ch_desc,
> +	.regs = mt6359_auxadc_regs,
> +	.sec_unlock_key = 0x6359,
> +	.read_imp = mt6359_read_imp,
> +};
>

> +
> +static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan, char *label)
> +{
> +	return sysfs_emit(label, "%s\n", chan->datasheet_name);
> +}
> +
> +static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  int *val, int *val2, long mask)
> +{
> +	struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
> +	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> +	const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
> +	int ret;
> +
> +	if (mask == IIO_CHAN_INFO_SCALE) {
> +		*val = desc->r_numerator * AUXADC_VOLT_FULL;
> +
> +		if (desc->r_denominator > 1) {
> +			*val2 = desc->r_denominator;
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	mutex_lock(&adc_dev->lock);
> +
> +	switch (chan->scan_index) {
> +	case PMIC_AUXADC_CHAN_IBAT:
> +		ret = adc_dev->pdata->read_imp(adc_dev, val2, val);

This is very odd. Why reverse the parameters between the IBAT and VBAT
channels?  I suspect you want to throw away a parameter. That's fine but
don't use val2 for it. Either make that function handle NULL pointers
or add a local int temp or similar for this purpose.

> +		break;
> +	case PMIC_AUXADC_CHAN_VBAT:
> +		ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
> +		break;
> +	default:
> +		ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
> +		break;
> +	}
> +
> +	mutex_unlock(&adc_dev->lock);
> +
> +	if (ret) {
> +		/*
> +		 * If we get more than one timeout, it's possible that the
> +		 * AUXADC is stuck: perform a full reset to recover it.
> +		 */
> +		if (ret == -ETIMEDOUT) {
> +			if (adc_dev->timed_out) {
> +				dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
> +				mt6359_auxadc_reset(adc_dev);
> +			}
> +			adc_dev->timed_out = true;
> +		}
> +		return ret;
> +	}
> +	adc_dev->timed_out = false;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info mt6359_auxadc_info = {
> +	.read_label = mt6359_auxadc_read_label,
> +	.read_raw = mt6359_auxadc_read_raw,
> +};
> +
> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> +{
> +	struct device *mt6397_mfd_dev = pdev->dev.parent;
> +	struct mt6359_auxadc *adc_dev;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	/* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> +	regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> +	if (!regmap)
> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc_dev = iio_priv(indio_dev);
> +	adc_dev->regmap = regmap;
> +	adc_dev->dev = &pdev->dev;
> +
> +	adc_dev->pdata = device_get_match_data(&pdev->dev);
> +	if (!adc_dev->pdata)
> +		return -EINVAL;
> +
> +	mutex_init(&adc_dev->lock);
> +
> +	mt6359_auxadc_reset(adc_dev);
> +
> +	indio_dev->dev.parent = &pdev->dev;
No need to set that, the IIO core does it for you in
devm_iio_device_alloc()
> +	indio_dev->name = dev_name(&pdev->dev);

This tends to be fragile at best.  The name should be the part number, best
way to reliably get that is either to query a whoami type register if there
is one or put it in your pdata.

> +	indio_dev->info = &mt6359_auxadc_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = adc_dev->pdata->channels;
> +	indio_dev->num_channels = adc_dev->pdata->num_channels;
> +
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "failed to register iio device\n");
> +
> +	return 0;
> +}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[AngeloGioacchino Del Regno]

Il 02/06/24 12:11, Jonathan Cameron ha scritto:
> On Thu, 30 May 2024 11:34:08 +0200
> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> Add a driver to support reading the Auxiliary ADC IP found in the
>> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
>>
>> This driver provides multiple ADC channels for system monitoring,
>> such as battery voltage, PMIC temperature, PMIC-internal voltage
>> regulators temperature, and others.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> 
> I'll leave you to answer the 'why a new driver' in response to Andy's review
> and just assume it makes sense whilst reviewing this.
> 

Simply, the Auxiliary ADC IP in the MT635x PMIC series is different, and handled
differently from the others that are supported by other drivers, other than having
a different register layout.

Adding this to any other driver, such as mt6577_auxadc (a SoC - not PMIC - auxadc
IP driver!), would be like having two different drivers in one and wouldn't make
any sense :-)

> What are IMP channels?
> 

Honestly? Well, it's called like that. I don't have any clear description of that
and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.

What I know is what I wrote in the driver, and this is:
* IMP has IBAT and VBAT ADCs
* It needs different handling from the other ADCs, as shown.

...and nothing else :-(

> A few additional comments inline.
> 
> Thanks,
> 
> Jonathan
> 
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index edb32ce2af02..da7d4452b1e0 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
>>   obj-$(CONFIG_MCP3422) += mcp3422.o
>>   obj-$(CONFIG_MCP3564) += mcp3564.o
>>   obj-$(CONFIG_MCP3911) += mcp3911.o
>> +obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
>>   obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
>>   obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
>>   obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
>> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
>> new file mode 100644
>> index 000000000000..0481bd3f0144
>> --- /dev/null
>> +++ b/drivers/iio/adc/mt6359-auxadc.c
>> @@ -0,0 +1,598 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * MediaTek MT6359 PMIC AUXADC IIO driver
>> + *
>> + * Copyright (c) 2021 MediaTek Inc.
>> + * Copyright (c) 2024 Collabora Ltd
>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/mfd/mt6397/core.h>
>> +
>> +#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
>> +#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
>> +#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>
> 
> So I 'guess' these headers are dt-bindings because you want
> to consume them from other drivers?  That's fine, but if so please
> add info on that to the DT binding patch.
> 

...consume from devicetrees. And the purpose of including those headers is to
index the channels as per the bindings, of course.
That was a *very* light explanation, but I really don't think that I have to
expand at all...

>> +/**
>> + * struct mt6359_auxadc - Main driver structure
>> + * @dev:           Device pointer
>> + * @regmap:        Regmap from SoC PMIC Wrapper
>> + * @pdata:         PMIC specific platform data
>> + * @lock:          Mutex lock for AUXADC reads
> 
> Expand on this mutex comment.  What is it protecting?
> I think it's about ensuring they are serialized to ensure configuration
> is not changed during the read sequence.
> 

Yes, exactly that.

"Mutex to serialize AUXADC reading vs configuration"
Looks good? :-)

>> + * @timed_out:     Signals whether the last read timed out
>> + */
>> +struct mt6359_auxadc {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	const struct mtk_pmic_auxadc_pdata *pdata;
>> +	struct mutex lock;
>> +	bool timed_out;
>> +};
>> +
> 
>> +/**
>> + * struct mtk_pmic_auxadc_pdata - PMIC specific platform data
> 
> I'm not sure this is conventionally what we think of as platform
> data.  This is chip specific stuff. Platform data tends to be
> more about how things are wired up etc.  A common term for this
> stuff is chip_info.
> 

Alright, I can do...

/**
  * struct mtk_pmic_auxadc_info - PMIC specific chip info
......
struct mtk_pmic_auxadc_info {
	members;
}


struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;

"cinfo" because otherwise it's going to impact code readability later on

>> + * @channels:       IIO specification of ADC channels
>> + * @num_channels:   Number of ADC channels
>> + * @desc:           PMIC AUXADC channel data
>> + * @regs:           List of PMIC specific registers
>> + * @sec_unlock_key: Security unlock key for HK_TOP writes
>> + * @imp_adc_num:    ADC channel for IMP readings
>> + * @read_imp:       Callback to read PMIC IMP channels
>> + */
>> +struct mtk_pmic_auxadc_pdata {
>> +	const struct iio_chan_spec *channels;
>> +	int num_channels;
>> +	const struct mtk_pmic_auxadc_chan *desc;
>> +	const u16 *regs;
>> +	u16 sec_unlock_key;
>> +	u8 imp_adc_num;
>> +	int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
>> +};
>> +
> 
>> +
>> +static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
>> +	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
>> +	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
>> +	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
>> +	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
>> +	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
>> +	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
>> +
>> +	/* IMP channels */
> What are these? Expand IMP perhaps!
> 

Yeah, well.. it's... "imp"... can't do anything about that, sorry. :-(

>> +	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
>> +	MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
>> +};
> 
> 
>> +static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
>> +{
>> +	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
>> +	struct regmap *regmap = adc_dev->regmap;
>> +	int val_v, val_i, ret;
>> +	u32 val;
>> +
>> +	/* Start conversion */
>> +	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
>> +	ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
>> +				       val, !!(val & MT6359_IMP1_IRQ_RDY),
> 
> The condition is just as true or false without the !! so drop those.
> 

Heh yes, I forgot to clean that up before sending, thanks for catching that!

>> +				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
>> +
>> +	/* Stop conversion regardless of the result */
>> +	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* If it succeeded, wait for the registers to be populated */
>> +	usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
>> +
>> +	ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
>> +	if (ret)
>> +		return ret;
> 
> Why read them both if only one is wanted?  Do you need to read the data
> for some reason - i.e. to allow for fresh reads or clear some status bit
> or similar?  If so add a comment.  Otherwise easy to do
> 

Yeah, the state machine inside of the AUXADC IP starts the conversion, takes as
many samples as you allow it to take, then stores an average of those in those
registers that I'm reading here.

Reading the registers resets the state machine, so the next time you start it
you won't start averaging current reads versus old ones...

> 	if (vbat) {
> 		int val_v;
> 		ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> 		if (ret)
> 			return ret;
> 		*vbat = val_v;
> 	}
> etc
> 
> 
>> +
>> +	*vbat = val_v;
>> +	*ibat = val_i;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
>> +	.channels = mt6357_auxadc_channels,
>> +	.num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
>> +	.desc = mt6357_auxadc_ch_desc,
>> +	.regs = mt6357_auxadc_regs,
>> +	.imp_adc_num = MT6357_IMP_ADC_NUM,
>> +	.read_imp = mt6358_read_imp,
>> +};
>> +
>> +static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
>> +	.channels = mt6358_auxadc_channels,
>> +	.num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
>> +	.desc = mt6358_auxadc_ch_desc,
>> +	.regs = mt6358_auxadc_regs,
>> +	.imp_adc_num = MT6358_IMP_ADC_NUM,
>> +	.read_imp = mt6358_read_imp,
>> +};
>> +
>> +static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
>> +	.channels = mt6359_auxadc_channels,
>> +	.num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
>> +	.desc = mt6359_auxadc_ch_desc,
>> +	.regs = mt6359_auxadc_regs,
>> +	.sec_unlock_key = 0x6359,
>> +	.read_imp = mt6359_read_imp,
>> +};
>>
> 
>> +
>> +static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
>> +				    const struct iio_chan_spec *chan, char *label)
>> +{
>> +	return sysfs_emit(label, "%s\n", chan->datasheet_name);
>> +}
>> +
>> +static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
>> +				  const struct iio_chan_spec *chan,
>> +				  int *val, int *val2, long mask)
>> +{
>> +	struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
>> +	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
>> +	const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
>> +	int ret;
>> +
>> +	if (mask == IIO_CHAN_INFO_SCALE) {
>> +		*val = desc->r_numerator * AUXADC_VOLT_FULL;
>> +
>> +		if (desc->r_denominator > 1) {
>> +			*val2 = desc->r_denominator;
>> +			return IIO_VAL_FRACTIONAL;
>> +		}
>> +
>> +		return IIO_VAL_INT;
>> +	}
>> +
>> +	mutex_lock(&adc_dev->lock);
>> +
>> +	switch (chan->scan_index) {
>> +	case PMIC_AUXADC_CHAN_IBAT:
>> +		ret = adc_dev->pdata->read_imp(adc_dev, val2, val);
> 
> This is very odd. Why reverse the parameters between the IBAT and VBAT
> channels?  I suspect you want to throw away a parameter. That's fine but
> don't use val2 for it. Either make that function handle NULL pointers
> or add a local int temp or similar for this purpose.
> 

I honestly prefer to have a simple variable swap instead of adding handling
for NULL pointers, even though it's just a "if NULL don't use me".

But okay, I also understand why you don't want me to use val2 as a "waste bin"...

I will add NULL pointer handling in the read_imp callback, as that's the shortest
and lightest option that I have here.

	case PMIC_AUXADC_CHAN_IBAT:
		ret = adc_dev->pdata->read_imp(adc_dev, NULL, val);
		break;
	case PMIC_AUXADC_CHAN_VBAT:
		ret = adc_dev->pdata->read_imp(adc_dev, val, NULL);
		break;

read_imp
{
	....

	if (vbat)
		*vbat = val_v;
	if (ibat)
		*ibat = val_i;

	return 0;
}

>> +		break;
>> +	case PMIC_AUXADC_CHAN_VBAT:
>> +		ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
>> +		break;
>> +	default:
>> +		ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
>> +		break;
>> +	}
>> +
>> +	mutex_unlock(&adc_dev->lock);
>> +
>> +	if (ret) {
>> +		/*
>> +		 * If we get more than one timeout, it's possible that the
>> +		 * AUXADC is stuck: perform a full reset to recover it.
>> +		 */
>> +		if (ret == -ETIMEDOUT) {
>> +			if (adc_dev->timed_out) {
>> +				dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
>> +				mt6359_auxadc_reset(adc_dev);
>> +			}
>> +			adc_dev->timed_out = true;
>> +		}
>> +		return ret;
>> +	}
>> +	adc_dev->timed_out = false;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static const struct iio_info mt6359_auxadc_info = {
>> +	.read_label = mt6359_auxadc_read_label,
>> +	.read_raw = mt6359_auxadc_read_raw,
>> +};
>> +
>> +static int mt6359_auxadc_probe(struct platform_device *pdev)
>> +{
>> +	struct device *mt6397_mfd_dev = pdev->dev.parent;
>> +	struct mt6359_auxadc *adc_dev;
>> +	struct iio_dev *indio_dev;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	/* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
>> +	regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
>> +	if (!regmap)
>> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	adc_dev = iio_priv(indio_dev);
>> +	adc_dev->regmap = regmap;
>> +	adc_dev->dev = &pdev->dev;
>> +
>> +	adc_dev->pdata = device_get_match_data(&pdev->dev);
>> +	if (!adc_dev->pdata)
>> +		return -EINVAL;
>> +
>> +	mutex_init(&adc_dev->lock);
>> +
>> +	mt6359_auxadc_reset(adc_dev);
>> +
>> +	indio_dev->dev.parent = &pdev->dev;
> No need to set that, the IIO core does it for you in
> devm_iio_device_alloc()

Ah, cool, thank you!

>> +	indio_dev->name = dev_name(&pdev->dev);
> 
> This tends to be fragile at best.  The name should be the part number, best
> way to reliably get that is either to query a whoami type register if there
> is one or put it in your pdata.
> 

Okay. I don't want to deal with unknown revision IDs and such, as the auxadc
IP is always the same between all of them and I don't have a clear list of
numbers here, so I'll just throw in the MT6357 MT6358 MT6359 names in the
chip_info and call it a day :-)

Thanks for the review btw!

Cheers,
Angelo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[Andy Shevchenko]

On Tue, Jun 4, 2024 at 12:42 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
> Il 02/06/24 12:11, Jonathan Cameron ha scritto:
> > On Thu, 30 May 2024 11:34:08 +0200
> > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

...

> > What are IMP channels?
>
> Honestly? Well, it's called like that. I don't have any clear description of that
> and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.
>
> What I know is what I wrote in the driver, and this is:
> * IMP has IBAT and VBAT ADCs
> * It needs different handling from the other ADCs, as shown.
>
> ...and nothing else :-(

I could speculate with confidence that this means IMPedance (since it's ADC).

From MTK6329  datasheet:
"The hardware also includes necessary modes to allow for simultaneous
current and voltage measurement
which can be utilized to estimate the battery impedance."
And googling in vendor trees also suggests the same.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[Andy Shevchenko]

On Tue, Jun 4, 2024 at 1:55 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Jun 4, 2024 at 12:42 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> > Il 02/06/24 12:11, Jonathan Cameron ha scritto:
> > > On Thu, 30 May 2024 11:34:08 +0200
> > > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

...

> > > What are IMP channels?
> >
> > Honestly? Well, it's called like that. I don't have any clear description of that
> > and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.
> >
> > What I know is what I wrote in the driver, and this is:
> > * IMP has IBAT and VBAT ADCs
> > * It needs different handling from the other ADCs, as shown.
> >
> > ...and nothing else :-(
>
> I could speculate with confidence that this means IMPedance (since it's ADC).
>
> From MTK6329  datasheet:

The datasheet for MTK6359 I found on
https://ebin.pub/qdownload/mt6359-pmic-datasheet-15nbsped.html
also has the same wording in "3.2.7 Fuel Gauge".

> "The hardware also includes necessary modes to allow for simultaneous
> current and voltage measurement
> which can be utilized to estimate the battery impedance."
> And googling vendor trees also suggests the same.

-- 
With Best Regards,
Andy Shevchenko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[AngeloGioacchino Del Regno]

Il 04/06/24 12:55, Andy Shevchenko ha scritto:
> On Tue, Jun 4, 2024 at 12:42 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>> Il 02/06/24 12:11, Jonathan Cameron ha scritto:
>>> On Thu, 30 May 2024 11:34:08 +0200
>>> AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
> ...
> 
>>> What are IMP channels?
>>
>> Honestly? Well, it's called like that. I don't have any clear description of that
>> and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.
>>
>> What I know is what I wrote in the driver, and this is:
>> * IMP has IBAT and VBAT ADCs
>> * It needs different handling from the other ADCs, as shown.
>>
>> ...and nothing else :-(
> 
> I could speculate with confidence that this means IMPedance (since it's ADC).
> 
>  From MTK6329  datasheet:
> "The hardware also includes necessary modes to allow for simultaneous
> current and voltage measurement
> which can be utilized to estimate the battery impedance."
> And googling in vendor trees also suggests the same.
> 

That does make a lot of sense. Thanks for that, I don't have the 6329 datasheet.

Great, one more piece added to the puzzle!

Cheers,
Angelo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] iio: adc: Add support for MediaTek MT6357/8/9 Auxiliary ADC

[Jonathan Cameron]

On Tue, 4 Jun 2024 11:42:05 +0200
AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 02/06/24 12:11, Jonathan Cameron ha scritto:
> > On Thu, 30 May 2024 11:34:08 +0200
> > AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> >   
> >> Add a driver to support reading the Auxiliary ADC IP found in the
> >> MediaTek MT6357, MT6358 and MT6359 Power Management ICs.
> >>
> >> This driver provides multiple ADC channels for system monitoring,
> >> such as battery voltage, PMIC temperature, PMIC-internal voltage
> >> regulators temperature, and others.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>  
> > 
> > I'll leave you to answer the 'why a new driver' in response to Andy's review
> > and just assume it makes sense whilst reviewing this.
> >   
> 
> Simply, the Auxiliary ADC IP in the MT635x PMIC series is different, and handled
> differently from the others that are supported by other drivers, other than having
> a different register layout.
> 
> Adding this to any other driver, such as mt6577_auxadc (a SoC - not PMIC - auxadc
> IP driver!), would be like having two different drivers in one and wouldn't make
> any sense :-)

That's fine. I missed the PMIC bit!

> 
> > What are IMP channels?
> >   
> 
> Honestly? Well, it's called like that. I don't have any clear description of that
> and not even datasheets are unrolling the meaning of "IMP". So.. I don't know.
> 
> What I know is what I wrote in the driver, and this is:
> * IMP has IBAT and VBAT ADCs
> * It needs different handling from the other ADCs, as shown.
> 
> ...and nothing else :-(

This is where kernel developers are rescued by sarcasm.

/* IMP - A small type of devil maybe? */
Let your creativity run wild ;)
Joking really. Just add a note to say that we have no idea what it means.

> 
> > A few additional comments inline.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> >> index edb32ce2af02..da7d4452b1e0 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -79,6 +79,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o
> >>   obj-$(CONFIG_MCP3422) += mcp3422.o
> >>   obj-$(CONFIG_MCP3564) += mcp3564.o
> >>   obj-$(CONFIG_MCP3911) += mcp3911.o
> >> +obj-$(CONFIG_MEDIATEK_MT6359_AUXADC) += mt6359-auxadc.o
> >>   obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> >>   obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
> >>   obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> >> diff --git a/drivers/iio/adc/mt6359-auxadc.c b/drivers/iio/adc/mt6359-auxadc.c
> >> new file mode 100644
> >> index 000000000000..0481bd3f0144
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/mt6359-auxadc.c
> >> @@ -0,0 +1,598 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * MediaTek MT6359 PMIC AUXADC IIO driver
> >> + *
> >> + * Copyright (c) 2021 MediaTek Inc.
> >> + * Copyright (c) 2024 Collabora Ltd
> >> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >> + */
> >> +
> >> +#include <linux/delay.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/iio/iio.h>
> >> +#include <linux/mfd/mt6397/core.h>
> >> +
> >> +#include <dt-bindings/iio/adc/mediatek,mt6357-auxadc.h>
> >> +#include <dt-bindings/iio/adc/mediatek,mt6358-auxadc.h>
> >> +#include <dt-bindings/iio/adc/mediatek,mt6359-auxadc.h>  
> > 
> > So I 'guess' these headers are dt-bindings because you want
> > to consume them from other drivers?  That's fine, but if so please
> > add info on that to the DT binding patch.
> >   
> 
> ...consume from devicetrees. And the purpose of including those headers is to
> index the channels as per the bindings, of course.
> That was a *very* light explanation, but I really don't think that I have to
> expand at all...

That's fine. Stick it in the patch description for patch 1.
> 
> >> +/**
> >> + * struct mt6359_auxadc - Main driver structure
> >> + * @dev:           Device pointer
> >> + * @regmap:        Regmap from SoC PMIC Wrapper
> >> + * @pdata:         PMIC specific platform data
> >> + * @lock:          Mutex lock for AUXADC reads  
> > 
> > Expand on this mutex comment.  What is it protecting?
> > I think it's about ensuring they are serialized to ensure configuration
> > is not changed during the read sequence.
> >   
> 
> Yes, exactly that.
> 
> "Mutex to serialize AUXADC reading vs configuration"
> Looks good? :-)
Mutex is obvious - so perhaps.

Serialize multi access AUXADC reading against configuration changes.

> 
> >> + * @timed_out:     Signals whether the last read timed out
> >> + */
> >> +struct mt6359_auxadc {
> >> +	struct device *dev;
> >> +	struct regmap *regmap;
> >> +	const struct mtk_pmic_auxadc_pdata *pdata;
> >> +	struct mutex lock;
> >> +	bool timed_out;
> >> +};
> >> +  
> >   
> >> +/**
> >> + * struct mtk_pmic_auxadc_pdata - PMIC specific platform data  
> > 
> > I'm not sure this is conventionally what we think of as platform
> > data.  This is chip specific stuff. Platform data tends to be
> > more about how things are wired up etc.  A common term for this
> > stuff is chip_info.
> >   
> 
> Alright, I can do...
> 
> /**
>   * struct mtk_pmic_auxadc_info - PMIC specific chip info
> ......
> struct mtk_pmic_auxadc_info {
> 	members;
> }
> 
> 
> struct mtk_pmic_auxadc_info *cinfo = adc_dev->chip_info;
> 
> "cinfo" because otherwise it's going to impact code readability later on
> 
> >> + * @channels:       IIO specification of ADC channels
> >> + * @num_channels:   Number of ADC channels
> >> + * @desc:           PMIC AUXADC channel data
> >> + * @regs:           List of PMIC specific registers
> >> + * @sec_unlock_key: Security unlock key for HK_TOP writes
> >> + * @imp_adc_num:    ADC channel for IMP readings
> >> + * @read_imp:       Callback to read PMIC IMP channels
> >> + */
> >> +struct mtk_pmic_auxadc_pdata {
> >> +	const struct iio_chan_spec *channels;
> >> +	int num_channels;
> >> +	const struct mtk_pmic_auxadc_chan *desc;
> >> +	const u16 *regs;
> >> +	u16 sec_unlock_key;
> >> +	u8 imp_adc_num;
> >> +	int (*read_imp)(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat);
> >> +};
> >> +  
> >   
> >> +
> >> +static const struct mtk_pmic_auxadc_chan mt6359_auxadc_ch_desc[] = {
> >> +	MTK_PMIC_ADC_CHAN(BATADC, PMIC_AUXADC_RQST0, 0, 128, 7, 2),
> >> +	MTK_PMIC_ADC_CHAN(BAT_TEMP, PMIC_AUXADC_RQST0, 3, 8, 5, 2),
> >> +	MTK_PMIC_ADC_CHAN(CHIP_TEMP, PMIC_AUXADC_RQST0, 4, 8, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(ACCDET, PMIC_AUXADC_RQST0, 5, 8, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(VDCXO, PMIC_AUXADC_RQST0, 6, 8, 3, 2),
> >> +	MTK_PMIC_ADC_CHAN(TSX_TEMP, PMIC_AUXADC_RQST0, 7, 128, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(HPOFS_CAL, PMIC_AUXADC_RQST0, 9, 256, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(DCXO_TEMP, PMIC_AUXADC_RQST0, 10, 16, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(VBIF, PMIC_AUXADC_RQST0, 11, 8, 5, 2),
> >> +	MTK_PMIC_ADC_CHAN(VCORE_TEMP, PMIC_AUXADC_RQST1, 8, 8, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(VPROC_TEMP, PMIC_AUXADC_RQST1, 9, 8, 1, 1),
> >> +	MTK_PMIC_ADC_CHAN(VGPU_TEMP, PMIC_AUXADC_RQST1, 10, 8, 1, 1),
> >> +
> >> +	/* IMP channels */  
> > What are these? Expand IMP perhaps!
> >   
> 
> Yeah, well.. it's... "imp"... can't do anything about that, sorry. :-(

It's growing horns and it's definitely a red one...

> 
> >> +	MTK_PMIC_ADC_CHAN(VBAT, 0, 0, 128, 7, 2),
> >> +	MTK_PMIC_ADC_CHAN(IBAT, 0, 0, 128, 7, 2),
> >> +};  
> > 
> >   
> >> +static int mt6359_read_imp(struct mt6359_auxadc *adc_dev, int *vbat, int *ibat)
> >> +{
> >> +	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> >> +	struct regmap *regmap = adc_dev->regmap;
> >> +	int val_v, val_i, ret;
> >> +	u32 val;
> >> +
> >> +	/* Start conversion */
> >> +	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], MT6359_IMP0_CONV_EN);
> >> +	ret = regmap_read_poll_timeout(regmap, pdata->regs[PMIC_AUXADC_IMP1],
> >> +				       val, !!(val & MT6359_IMP1_IRQ_RDY),  
> > 
> > The condition is just as true or false without the !! so drop those.
> >   
> 
> Heh yes, I forgot to clean that up before sending, thanks for catching that!
> 
> >> +				       IMP_POLL_DELAY_US, AUXADC_TIMEOUT_US);
> >> +
> >> +	/* Stop conversion regardless of the result */
> >> +	regmap_write(regmap, pdata->regs[PMIC_AUXADC_IMP0], 0);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/* If it succeeded, wait for the registers to be populated */
> >> +	usleep_range(IMP_STOP_DELAY_US, IMP_STOP_DELAY_US + 50);
> >> +
> >> +	ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	ret = regmap_read(regmap, pdata->regs[PMIC_FGADC_R_CON0], &val_i);
> >> +	if (ret)
> >> +		return ret;  
> > 
> > Why read them both if only one is wanted?  Do you need to read the data
> > for some reason - i.e. to allow for fresh reads or clear some status bit
> > or similar?  If so add a comment.  Otherwise easy to do
> >   
> 
> Yeah, the state machine inside of the AUXADC IP starts the conversion, takes as
> many samples as you allow it to take, then stores an average of those in those
> registers that I'm reading here.
> 
> Reading the registers resets the state machine, so the next time you start it
> you won't start averaging current reads versus old ones...

Ah ok. Comment please on need to read to reset the register.
Everyone loves read to clear registers... *Sigh*

> 
> > 	if (vbat) {
> > 		int val_v;
> > 		ret = regmap_read(regmap, pdata->regs[PMIC_AUXADC_IMP3], &val_v);
> > 		if (ret)
> > 			return ret;
> > 		*vbat = val_v;
> > 	}
> > etc
> > 
> >   
> >> +
> >> +	*vbat = val_v;
> >> +	*ibat = val_i;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct mtk_pmic_auxadc_pdata mt6357_pdata = {
> >> +	.channels = mt6357_auxadc_channels,
> >> +	.num_channels = ARRAY_SIZE(mt6357_auxadc_channels),
> >> +	.desc = mt6357_auxadc_ch_desc,
> >> +	.regs = mt6357_auxadc_regs,
> >> +	.imp_adc_num = MT6357_IMP_ADC_NUM,
> >> +	.read_imp = mt6358_read_imp,
> >> +};
> >> +
> >> +static const struct mtk_pmic_auxadc_pdata mt6358_pdata = {
> >> +	.channels = mt6358_auxadc_channels,
> >> +	.num_channels = ARRAY_SIZE(mt6358_auxadc_channels),
> >> +	.desc = mt6358_auxadc_ch_desc,
> >> +	.regs = mt6358_auxadc_regs,
> >> +	.imp_adc_num = MT6358_IMP_ADC_NUM,
> >> +	.read_imp = mt6358_read_imp,
> >> +};
> >> +
> >> +static const struct mtk_pmic_auxadc_pdata mt6359_pdata = {
> >> +	.channels = mt6359_auxadc_channels,
> >> +	.num_channels = ARRAY_SIZE(mt6359_auxadc_channels),
> >> +	.desc = mt6359_auxadc_ch_desc,
> >> +	.regs = mt6359_auxadc_regs,
> >> +	.sec_unlock_key = 0x6359,
> >> +	.read_imp = mt6359_read_imp,
> >> +};
> >>  
> >   
> >> +
> >> +static int mt6359_auxadc_read_label(struct iio_dev *indio_dev,
> >> +				    const struct iio_chan_spec *chan, char *label)
> >> +{
> >> +	return sysfs_emit(label, "%s\n", chan->datasheet_name);
> >> +}
> >> +
> >> +static int mt6359_auxadc_read_raw(struct iio_dev *indio_dev,
> >> +				  const struct iio_chan_spec *chan,
> >> +				  int *val, int *val2, long mask)
> >> +{
> >> +	struct mt6359_auxadc *adc_dev = iio_priv(indio_dev);
> >> +	const struct mtk_pmic_auxadc_pdata *pdata = adc_dev->pdata;
> >> +	const struct mtk_pmic_auxadc_chan *desc = &pdata->desc[chan->scan_index];
> >> +	int ret;
> >> +
> >> +	if (mask == IIO_CHAN_INFO_SCALE) {
> >> +		*val = desc->r_numerator * AUXADC_VOLT_FULL;
> >> +
> >> +		if (desc->r_denominator > 1) {
> >> +			*val2 = desc->r_denominator;
> >> +			return IIO_VAL_FRACTIONAL;
> >> +		}
> >> +
> >> +		return IIO_VAL_INT;
> >> +	}
> >> +
> >> +	mutex_lock(&adc_dev->lock);
> >> +
> >> +	switch (chan->scan_index) {
> >> +	case PMIC_AUXADC_CHAN_IBAT:
> >> +		ret = adc_dev->pdata->read_imp(adc_dev, val2, val);  
> > 
> > This is very odd. Why reverse the parameters between the IBAT and VBAT
> > channels?  I suspect you want to throw away a parameter. That's fine but
> > don't use val2 for it. Either make that function handle NULL pointers
> > or add a local int temp or similar for this purpose.
> >   
> 
> I honestly prefer to have a simple variable swap instead of adding handling
> for NULL pointers, even though it's just a "if NULL don't use me".
> 
> But okay, I also understand why you don't want me to use val2 as a "waste bin"...
Indeed not!

> 
> I will add NULL pointer handling in the read_imp callback, as that's the shortest
> and lightest option that I have here.
> 
> 	case PMIC_AUXADC_CHAN_IBAT:
> 		ret = adc_dev->pdata->read_imp(adc_dev, NULL, val);
> 		break;
> 	case PMIC_AUXADC_CHAN_VBAT:
> 		ret = adc_dev->pdata->read_imp(adc_dev, val, NULL);

that works, or just use a locally defined variable to dump the value you don't
want in if  you prefer.  int garbage; or similar.

> 		break;
> 
> read_imp
> {
> 	....
> 
> 	if (vbat)
> 		*vbat = val_v;
> 	if (ibat)
> 		*ibat = val_i;
> 
> 	return 0;
> }
> 
> >> +		break;
> >> +	case PMIC_AUXADC_CHAN_VBAT:
> >> +		ret = adc_dev->pdata->read_imp(adc_dev, val, val2);
> >> +		break;
> >> +	default:
> >> +		ret = mt6359_auxadc_read_adc(adc_dev, chan, val);
> >> +		break;
> >> +	}
> >> +
> >> +	mutex_unlock(&adc_dev->lock);
> >> +
> >> +	if (ret) {
> >> +		/*
> >> +		 * If we get more than one timeout, it's possible that the
> >> +		 * AUXADC is stuck: perform a full reset to recover it.
> >> +		 */
> >> +		if (ret == -ETIMEDOUT) {
> >> +			if (adc_dev->timed_out) {
> >> +				dev_warn(adc_dev->dev, "Resetting stuck ADC!\r\n");
> >> +				mt6359_auxadc_reset(adc_dev);
> >> +			}
> >> +			adc_dev->timed_out = true;
> >> +		}
> >> +		return ret;
> >> +	}
> >> +	adc_dev->timed_out = false;
> >> +
> >> +	return IIO_VAL_INT;
> >> +}
> >> +
> >> +static const struct iio_info mt6359_auxadc_info = {
> >> +	.read_label = mt6359_auxadc_read_label,
> >> +	.read_raw = mt6359_auxadc_read_raw,
> >> +};
> >> +
> >> +static int mt6359_auxadc_probe(struct platform_device *pdev)
> >> +{
> >> +	struct device *mt6397_mfd_dev = pdev->dev.parent;
> >> +	struct mt6359_auxadc *adc_dev;
> >> +	struct iio_dev *indio_dev;
> >> +	struct regmap *regmap;
> >> +	int ret;
> >> +
> >> +	/* Regmap is from SoC PMIC Wrapper, parent of the mt6397 MFD */
> >> +	regmap = dev_get_regmap(mt6397_mfd_dev->parent, NULL);
> >> +	if (!regmap)
> >> +		return dev_err_probe(&pdev->dev, -ENODEV, "Failed to get regmap\n");
> >> +
> >> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
> >> +	if (!indio_dev)
> >> +		return -ENOMEM;
> >> +
> >> +	adc_dev = iio_priv(indio_dev);
> >> +	adc_dev->regmap = regmap;
> >> +	adc_dev->dev = &pdev->dev;
> >> +
> >> +	adc_dev->pdata = device_get_match_data(&pdev->dev);
> >> +	if (!adc_dev->pdata)
> >> +		return -EINVAL;
> >> +
> >> +	mutex_init(&adc_dev->lock);
> >> +
> >> +	mt6359_auxadc_reset(adc_dev);
> >> +
> >> +	indio_dev->dev.parent = &pdev->dev;  
> > No need to set that, the IIO core does it for you in
> > devm_iio_device_alloc()  
> 
> Ah, cool, thank you!
> 
> >> +	indio_dev->name = dev_name(&pdev->dev);  
> > 
> > This tends to be fragile at best.  The name should be the part number, best
> > way to reliably get that is either to query a whoami type register if there
> > is one or put it in your pdata.
> >   
> 
> Okay. I don't want to deal with unknown revision IDs and such, as the auxadc
> IP is always the same between all of them and I don't have a clear list of
> numbers here, so I'll just throw in the MT6357 MT6358 MT6359 names in the
> chip_info and call it a day :-)

ok. I think.  Note we should allow for the mess of fallback compatibles with
at most an info print if we can tell something is different from expected.
Not sure that is relevant here though.
> 
> Thanks for the review btw!
np

Jonathan

> 
> Cheers,
> Angelo
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/4] mfd: mt6397-core: Add support for AUXADCs on MT6357/58/59 PMICs

[AngeloGioacchino Del Regno]

Add the relevant AUXADC driver entries to the MFD cells of the
MT6357, MT6358, MT6359 PMICs to support their Auxiliary ADCs.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/mfd/mt6397-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/mfd/mt6397-core.c b/drivers/mfd/mt6397-core.c
index 4fd4a2da5ad7..c2939e785818 100644
--- a/drivers/mfd/mt6397-core.c
+++ b/drivers/mfd/mt6397-core.c
@@ -135,6 +135,9 @@ static const struct mfd_cell mt6323_devs[] = {
 
 static const struct mfd_cell mt6357_devs[] = {
 	{
+		.name = "mt6359-auxadc",
+		.of_compatible = "mediatek,mt6357-auxadc"
+	}, {
 		.name = "mt6357-regulator",
 	}, {
 		.name = "mt6357-rtc",
@@ -175,6 +178,9 @@ static const struct mfd_cell mt6331_mt6332_devs[] = {
 
 static const struct mfd_cell mt6358_devs[] = {
 	{
+		.name = "mt6359-auxadc",
+		.of_compatible = "mediatek,mt6358-auxadc"
+	}, {
 		.name = "mt6358-regulator",
 		.of_compatible = "mediatek,mt6358-regulator"
 	}, {
@@ -194,6 +200,10 @@ static const struct mfd_cell mt6358_devs[] = {
 };
 
 static const struct mfd_cell mt6359_devs[] = {
+	{
+		.name = "mt6359-auxadc",
+		.of_compatible = "mediatek,mt6359-auxadc"
+	},
 	{ .name = "mt6359-regulator", },
 	{
 		.name = "mt6359-rtc",
-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v1 3/4] mfd: mt6397-core: Add support for AUXADCs on MT6357/58/59 PMICs

[Lee Jones]

On Thu, 30 May 2024 11:34:09 +0200, AngeloGioacchino Del Regno wrote:
> Add the relevant AUXADC driver entries to the MFD cells of the
> MT6357, MT6358, MT6359 PMICs to support their Auxiliary ADCs.
> 
> 

Applied, thanks!

[3/4] mfd: mt6397-core: Add support for AUXADCs on MT6357/58/59 PMICs
      commit: fa9d9f4b5588003a955193fe9ff009687541f3e7

--
Lee Jones [李琼斯]


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/4] arm64: dts: mediatek: Add ADC node on MT6357, MT6358, MT6359 PMICs

[AngeloGioacchino Del Regno]

Add support for the ADC on MT6357/8/9 and keep it default enabled
as this IP is always present on those PMICs.
Users may use different IIO channels depending on board-specific
routing.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 arch/arm64/boot/dts/mediatek/mt6357.dtsi | 5 +++++
 arch/arm64/boot/dts/mediatek/mt6358.dtsi | 5 +++++
 arch/arm64/boot/dts/mediatek/mt6359.dtsi | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
index 3330a03c2f74..5fafa842d312 100644
--- a/arch/arm64/boot/dts/mediatek/mt6357.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi
@@ -10,6 +10,11 @@ &pwrap {
 	mt6357_pmic: pmic {
 		compatible = "mediatek,mt6357";
 
+		pmic_adc: adc {
+			compatible = "mediatek,mt6357-auxadc";
+			#io-channel-cells = <1>;
+		};
+
 		regulators {
 			mt6357_vproc_reg: buck-vproc {
 				regulator-name = "vproc";
diff --git a/arch/arm64/boot/dts/mediatek/mt6358.dtsi b/arch/arm64/boot/dts/mediatek/mt6358.dtsi
index a1b96013f814..641d452fbc08 100644
--- a/arch/arm64/boot/dts/mediatek/mt6358.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6358.dtsi
@@ -10,6 +10,11 @@ pmic: pmic {
 		interrupt-controller;
 		#interrupt-cells = <2>;
 
+		pmic_adc: adc {
+			compatible = "mediatek,mt6358-auxadc";
+			#io-channel-cells = <1>;
+		};
+
 		mt6358codec: mt6358codec {
 			compatible = "mediatek,mt6358-sound";
 			mediatek,dmic-mode = <0>; /* two-wires */
diff --git a/arch/arm64/boot/dts/mediatek/mt6359.dtsi b/arch/arm64/boot/dts/mediatek/mt6359.dtsi
index df3e822232d3..8e1b8c85c6ed 100644
--- a/arch/arm64/boot/dts/mediatek/mt6359.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt6359.dtsi
@@ -9,6 +9,11 @@ pmic: pmic {
 		interrupt-controller;
 		#interrupt-cells = <2>;
 
+		pmic_adc: adc {
+			compatible = "mediatek,mt6359-auxadc";
+			#io-channel-cells = <1>;
+		};
+
 		mt6359codec: mt6359codec {
 		};
 
-- 
2.45.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel