mbox series

[RFC,00/10] thermal/drivers/tsens: specify nvmem cells in DT rather than parsing them manually

Message ID 20220910124701.4060321-1-dmitry.baryshkov@linaro.org
Headers show
Series thermal/drivers/tsens: specify nvmem cells in DT rather than parsing them manually | expand

Message

Dmitry Baryshkov Sept. 10, 2022, 12:46 p.m. UTC
Historically the tsens driver fetches the calibration data as a blob and
then parses the blob on its own. This results in semi-duplicated code
spreading over the platform-specific functions.

This patch series changes tsens calibration code to use pre-parsed nvmem
cells rather than parsing the blob in the driver. For backwards
compatibility the old code is left in place for msm8916 and qcs404, two
platforms which have in-tree DT files. For msm8974 the original function
is left intact, since it differs significantly (and I can not test the
code on msm8974). For all other affected platforms the old parsing code
has been dropped as a part of this RFC.

The code was tested on msm8916 and qcs404 only, thus it is being sent as
an RFC.

Dmitry Baryshkov (10):
  dt-bindings: thermal: tsens: support per-sensor calibration cells
  thermal/drivers/tsens: Support using nvmem cells for calibration data
  thermal/drivers/tsens: drop single-cell code for msm8939
  thermal/drivers/tsens: drop single-cell code for mdm9607
  thermal/drivers/tsens: drop msm8976-specific defines
  thermal/drivers/tsens: use generic calibration routine for msm8976
  thermal/drivers/tsens: use tsens_calibrate_nvmem for msm8976
    calibration
  thermal/drivers/tsens: drop single-cell code for msm8976
  arm64: dts: qcom: msm8916: specify per-sensor calibration cells
  arm64: dts: qcom: qcs404: specify per-sensor calibration cells

 .../bindings/thermal/qcom-tsens.yaml          |  64 +++++-
 arch/arm64/boot/dts/qcom/msm8916.dtsi         |  70 +++++-
 arch/arm64/boot/dts/qcom/qcs404.dtsi          | 120 +++++++++-
 drivers/thermal/qcom/tsens-v0_1.c             | 208 +-----------------
 drivers/thermal/qcom/tsens-v1.c               | 154 +------------
 drivers/thermal/qcom/tsens.c                  |  62 ++++++
 drivers/thermal/qcom/tsens.h                  |   4 +
 7 files changed, 314 insertions(+), 368 deletions(-)

Comments

Krzysztof Kozlowski Sept. 11, 2022, 1:37 p.m. UTC | #1
On 10/09/2022 14:46, Dmitry Baryshkov wrote:
> Allow specifing the exact calibration mode and calibration data as nvmem
> cells, rather than specifying just a single calibration data blob.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../bindings/thermal/qcom-tsens.yaml          | 64 ++++++++++++++++---
>  1 file changed, 54 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 038d81338fcf..b813f6f19c1d 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -77,18 +77,62 @@ properties:
>        - const: critical
>  
>    nvmem-cells:
> -    minItems: 1
> -    maxItems: 2
> -    description:
> -      Reference to an nvmem node for the calibration data
> +    oneOf:
> +      - minItems: 1
> +        maxItems: 2
> +        description:
> +          Reference to an nvmem node for the calibration data
> +      - minItems: 5
> +        maxItems: 35
> +        description: |
> +          Reference to an nvmem cells for the calibration mode, two calibration

s/an nvmem/nvmem/

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof
Amit Kucheria Sept. 13, 2022, 7:19 p.m. UTC | #2
On Sat, Sep 10, 2022 at 6:17 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> Allow specifing the exact calibration mode and calibration data as nvmem

typo: specifying

> cells, rather than specifying just a single calibration data blob.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Amit Kucheria <amitk@kernel.org>


> ---
>  .../bindings/thermal/qcom-tsens.yaml          | 64 ++++++++++++++++---
>  1 file changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> index 038d81338fcf..b813f6f19c1d 100644
> --- a/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> +++ b/Documentation/devicetree/bindings/thermal/qcom-tsens.yaml
> @@ -77,18 +77,62 @@ properties:
>        - const: critical
>
>    nvmem-cells:
> -    minItems: 1
> -    maxItems: 2
> -    description:
> -      Reference to an nvmem node for the calibration data
> +    oneOf:
> +      - minItems: 1
> +        maxItems: 2
> +        description:
> +          Reference to an nvmem node for the calibration data
> +      - minItems: 5
> +        maxItems: 35
> +        description: |
> +          Reference to an nvmem cells for the calibration mode, two calibration
> +          bases and two cells per each sensor
>
>    nvmem-cell-names:
> -    minItems: 1
> -    items:
> -      - const: calib
> -      - enum:
> -          - calib_backup
> -          - calib_sel
> +    oneOf:
> +      - minItems: 1
> +        items:
> +          - const: calib
> +          - enum:
> +              - calib_backup
> +              - calib_sel
> +      - minItems: 5
> +        items:
> +          - const: mode
> +          - const: base1
> +          - const: base2
> +          - const: s0_p1
> +          - const: s0_p2
> +          - const: s1_p1
> +          - const: s1_p2
> +          - const: s2_p1
> +          - const: s2_p2
> +          - const: s3_p1
> +          - const: s3_p2
> +          - const: s4_p1
> +          - const: s4_p2
> +          - const: s5_p1
> +          - const: s5_p2
> +          - const: s6_p1
> +          - const: s6_p2
> +          - const: s7_p1
> +          - const: s7_p2
> +          - const: s8_p1
> +          - const: s8_p2
> +          - const: s9_p1
> +          - const: s9_p2
> +          - const: s10_p1
> +          - const: s10_p2
> +          - const: s11_p1
> +          - const: s11_p2
> +          - const: s12_p1
> +          - const: s12_p2
> +          - const: s13_p1
> +          - const: s13_p2
> +          - const: s14_p1
> +          - const: s14_p2
> +          - const: s15_p1
> +          - const: s15_p2
>
>    "#qcom,sensors":
>      description:
> --
> 2.35.1
>
Stephan Gerhold Sept. 22, 2022, 5:23 p.m. UTC | #3
Hi Dmitry,

On Sat, Sep 10, 2022 at 03:46:51PM +0300, Dmitry Baryshkov wrote:
> Historically the tsens driver fetches the calibration data as a blob and
> then parses the blob on its own. This results in semi-duplicated code
> spreading over the platform-specific functions.
> 
> This patch series changes tsens calibration code to use pre-parsed nvmem
> cells rather than parsing the blob in the driver. For backwards
> compatibility the old code is left in place for msm8916 and qcs404, two
> platforms which have in-tree DT files. For msm8974 the original function
> is left intact, since it differs significantly (and I can not test the
> code on msm8974). For all other affected platforms the old parsing code
> has been dropped as a part of this RFC.
> 
> The code was tested on msm8916 and qcs404 only, thus it is being sent as
> an RFC.
> 

Thanks a lot for working on this!

After thinking about this for a while I wonder if we can go even a step
further: Can we drop SoC-specific code entirely for 8939 and 9607 and
match the generic compatible (qcom,tsens-v0_1)? This would allow most
v0.1 plaforms to use generic code like for qcom,tsens-v2.

AFAICT with your changes only the following remains SoC-specific:

  - hw_ids (actually only needed for 8939 since 9607 has standard IDs)

While two other things are already handled:

  - num_sensors (the driver supports "#qcom,sensors" in DT already)
  - tsens_calibrate_nvmem() shift (AFAICT in downstream msm-tsens.c
    everything except 8916 uses shift = 2. 8916 needs special handling
    anyway for the backwards compatibility)

Having the generic compatible would allow me to add MSM8909 without any
code changes at all (just DT schema addition).

For 8939 we could read the hw_ids from the DT with something like:

	qcom,sensors = <0 1 2 3 5 6 7 8 9 10>;

And actually there are two revisions of 8939, the older one has one
sensor less (msm-3.10: msm8939-common.dtsi vs msm8939-v3.0.dtsi).
This could also be easily handled from the DT without any code changes:

	qcom,sensors = <0 1 2 3 5 6 7 8 9>;

The diff could be something like the following (I did not test it yet).

What do you think?

Thanks,
Stephan

 drivers/thermal/qcom/tsens-v0_1.c | 32 +++++---------------------------
 drivers/thermal/qcom/tsens.c      | 27 ++++++++++++++++++---------
 drivers/thermal/qcom/tsens.h      |  2 +-
 3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c
index c3613b7ccc19..8bfa85b31146 100644
--- a/drivers/thermal/qcom/tsens-v0_1.c
+++ b/drivers/thermal/qcom/tsens-v0_1.c
@@ -194,11 +194,6 @@ static int calibrate_8916(struct tsens_priv *priv)
 	return 0;
 }
 
-static int calibrate_8939(struct tsens_priv *priv)
-{
-	return tsens_calibrate_nvmem(priv, 2);
-}
-
 static int calibrate_8974(struct tsens_priv *priv)
 {
 	int base1 = 0, base2 = 0, i;
@@ -335,7 +330,7 @@ static int calibrate_8974(struct tsens_priv *priv)
 	return 0;
 }
 
-static int calibrate_9607(struct tsens_priv *priv)
+static int calibrate_dt(struct tsens_priv *priv)
 {
 	return tsens_calibrate_nvmem(priv, 2);
 }
@@ -401,21 +396,6 @@ struct tsens_plat_data data_8916 = {
 	.fields	= tsens_v0_1_regfields,
 };
 
-static const struct tsens_ops ops_8939 = {
-	.init		= init_common,
-	.calibrate	= calibrate_8939,
-	.get_temp	= get_temp_common,
-};
-
-struct tsens_plat_data data_8939 = {
-	.num_sensors	= 10,
-	.ops		= &ops_8939,
-	.hw_ids		= (unsigned int []){ 0, 1, 2, 4, 5, 6, 7, 8, 9, 10 },
-
-	.feat		= &tsens_v0_1_feat,
-	.fields	= tsens_v0_1_regfields,
-};
-
 static const struct tsens_ops ops_8974 = {
 	.init		= init_common,
 	.calibrate	= calibrate_8974,
@@ -429,16 +409,14 @@ struct tsens_plat_data data_8974 = {
 	.fields	= tsens_v0_1_regfields,
 };
 
-static const struct tsens_ops ops_9607 = {
+static const struct tsens_ops ops_generic_v0_1 = {
 	.init		= init_common,
-	.calibrate	= calibrate_9607,
+	.calibrate	= calibrate_dt,
 	.get_temp	= get_temp_common,
 };
 
-struct tsens_plat_data data_9607 = {
-	.num_sensors	= 5,
-	.ops		= &ops_9607,
-	.hw_ids		= (unsigned int []){ 0, 1, 2, 3, 4 },
+struct tsens_plat_data data_tsens_v0_1 = {
+	.ops		= &ops_generic_v0_1
 	.feat		= &tsens_v0_1_feat,
 	.fields	= tsens_v0_1_regfields,
 };
diff --git a/drivers/thermal/qcom/tsens.c b/drivers/thermal/qcom/tsens.c
index e69134a6419a..c4772962fb94 100644
--- a/drivers/thermal/qcom/tsens.c
+++ b/drivers/thermal/qcom/tsens.c
@@ -1021,15 +1021,9 @@ static const struct of_device_id tsens_table[] = {
 	{
 		.compatible = "qcom,ipq8064-tsens",
 		.data = &data_8960,
-	}, {
-		.compatible = "qcom,mdm9607-tsens",
-		.data = &data_9607,
 	}, {
 		.compatible = "qcom,msm8916-tsens",
 		.data = &data_8916,
-	}, {
-		.compatible = "qcom,msm8939-tsens",
-		.data = &data_8939,
 	}, {
 		.compatible = "qcom,msm8960-tsens",
 		.data = &data_8960,
@@ -1042,6 +1036,9 @@ static const struct of_device_id tsens_table[] = {
 	}, {
 		.compatible = "qcom,msm8996-tsens",
 		.data = &data_8996,
+	}, {
+		.compatible = "qcom,tsens-v0_1",
+		.data = &data_tsens_v0_1,
 	}, {
 		.compatible = "qcom,tsens-v1",
 		.data = &data_tsens_v1,
@@ -1152,6 +1149,8 @@ static int tsens_probe(struct platform_device *pdev)
 	struct tsens_priv *priv;
 	const struct tsens_plat_data *data;
 	const struct of_device_id *id;
+	unsigned int *hw_ids;
+	unsigned int dt_hw_ids[MAX_SENSORS];
 	u32 num_sensors;
 
 	if (pdev->dev.of_node)
@@ -1168,10 +1167,20 @@ static int tsens_probe(struct platform_device *pdev)
 		data = &data_8960;
 
 	num_sensors = data->num_sensors;
+	hw_ids = data->hw_ids;
 
-	if (np)
+	if (np) {
 		of_property_read_u32(np, "#qcom,sensors", &num_sensors);
 
+		ret = of_property_read_variable_u32_array(np, "qcom,sensors",
+							  dt_hw_ids,
+							  1, MAX_SENSORS);
+		if (ret > 0) {
+			hw_ids = dt_hw_ids;
+			num_sensors = ret;
+		}
+	}
+
 	if (num_sensors <= 0) {
 		dev_err(dev, "%s: invalid number of sensors\n", __func__);
 		return -EINVAL;
@@ -1187,8 +1196,8 @@ static int tsens_probe(struct platform_device *pdev)
 	priv->num_sensors = num_sensors;
 	priv->ops = data->ops;
 	for (i = 0;  i < priv->num_sensors; i++) {
-		if (data->hw_ids)
-			priv->sensor[i].hw_id = data->hw_ids[i];
+		if (hw_ids)
+			priv->sensor[i].hw_id = hw_ids[i];
 		else
 			priv->sensor[i].hw_id = i;
 	}
diff --git a/drivers/thermal/qcom/tsens.h b/drivers/thermal/qcom/tsens.h
index 504ed3394a79..439f356f0177 100644
--- a/drivers/thermal/qcom/tsens.h
+++ b/drivers/thermal/qcom/tsens.h
@@ -589,7 +589,7 @@ int get_temp_common(const struct tsens_sensor *s, int *temp);
 extern struct tsens_plat_data data_8960;
 
 /* TSENS v0.1 targets */
-extern struct tsens_plat_data data_8916, data_8939, data_8974, data_9607;
+extern struct tsens_plat_data data_8916, data_8974, data_tsens_v0_1;
 
 /* TSENS v1 targets */
 extern struct tsens_plat_data data_tsens_v1, data_8976;
Dmitry Baryshkov Sept. 24, 2022, 6:58 p.m. UTC | #4
Hi,

On 22/09/2022 20:23, Stephan Gerhold wrote:
> Hi Dmitry,
> 
> On Sat, Sep 10, 2022 at 03:46:51PM +0300, Dmitry Baryshkov wrote:
>> Historically the tsens driver fetches the calibration data as a blob and
>> then parses the blob on its own. This results in semi-duplicated code
>> spreading over the platform-specific functions.
>>
>> This patch series changes tsens calibration code to use pre-parsed nvmem
>> cells rather than parsing the blob in the driver. For backwards
>> compatibility the old code is left in place for msm8916 and qcs404, two
>> platforms which have in-tree DT files. For msm8974 the original function
>> is left intact, since it differs significantly (and I can not test the
>> code on msm8974). For all other affected platforms the old parsing code
>> has been dropped as a part of this RFC.
>>
>> The code was tested on msm8916 and qcs404 only, thus it is being sent as
>> an RFC.
>>
> 
> Thanks a lot for working on this!
> 
> After thinking about this for a while I wonder if we can go even a step
> further: Can we drop SoC-specific code entirely for 8939 and 9607 and
> match the generic compatible (qcom,tsens-v0_1)? This would allow most
> v0.1 plaforms to use generic code like for qcom,tsens-v2.

While this idea looks appealing, I think it's a bit against our custom 
to put hardware details into the driver rather than putting them into 
the DT. So, I think, the 8939 will have to stay as is, while for the 
9607 and maybe several other devices it should be possible to create a 
fallback entry.

> 
> AFAICT with your changes only the following remains SoC-specific:
> 
>    - hw_ids (actually only needed for 8939 since 9607 has standard IDs)

As I wrote, I wouldn't put this into DT.

> 
> While two other things are already handled:
> 
>    - num_sensors (the driver supports "#qcom,sensors" in DT already)
>    - tsens_calibrate_nvmem() shift (AFAICT in downstream msm-tsens.c
>      everything except 8916 uses shift = 2. 8916 needs special handling
>      anyway for the backwards compatibility)
> 
> Having the generic compatible would allow me to add MSM8909 without any
> code changes at all (just DT schema addition).
> 
> For 8939 we could read the hw_ids from the DT with something like:
> 
> 	qcom,sensors = <0 1 2 3 5 6 7 8 9 10>;
> 
> And actually there are two revisions of 8939, the older one has one
> sensor less (msm-3.10: msm8939-common.dtsi vs msm8939-v3.0.dtsi).
> This could also be easily handled from the DT without any code changes:
> 
> 	qcom,sensors = <0 1 2 3 5 6 7 8 9>;

Usually we only care about the latest revision of the chip, earlier 
revisions typically correspond to engineering samples, never hitting the 
actual consumer devices.

> 
> The diff could be something like the following (I did not test it yet).
> 
> What do you think?
I'd like to sort the calibration data for 8976 first. At this moment I'm 
waiting for the 8976 data to be tested. Also it would be nice to be able 
to cleanup the 8976 calibration code.
Stephan Gerhold Sept. 25, 2022, 10:20 a.m. UTC | #5
On Sat, Sep 24, 2022 at 09:58:56PM +0300, Dmitry Baryshkov wrote:
> On 22/09/2022 20:23, Stephan Gerhold wrote:
> > On Sat, Sep 10, 2022 at 03:46:51PM +0300, Dmitry Baryshkov wrote:
> > > Historically the tsens driver fetches the calibration data as a blob and
> > > then parses the blob on its own. This results in semi-duplicated code
> > > spreading over the platform-specific functions.
> > > 
> > > This patch series changes tsens calibration code to use pre-parsed nvmem
> > > cells rather than parsing the blob in the driver. For backwards
> > > compatibility the old code is left in place for msm8916 and qcs404, two
> > > platforms which have in-tree DT files. For msm8974 the original function
> > > is left intact, since it differs significantly (and I can not test the
> > > code on msm8974). For all other affected platforms the old parsing code
> > > has been dropped as a part of this RFC.
> > > 
> > > The code was tested on msm8916 and qcs404 only, thus it is being sent as
> > > an RFC.
> > > 
> > 
> > Thanks a lot for working on this!
> > 
> > After thinking about this for a while I wonder if we can go even a step
> > further: Can we drop SoC-specific code entirely for 8939 and 9607 and
> > match the generic compatible (qcom,tsens-v0_1)? This would allow most
> > v0.1 plaforms to use generic code like for qcom,tsens-v2.
> 
> While this idea looks appealing, I think it's a bit against our custom to
> put hardware details into the driver rather than putting them into the DT.
> So, I think, the 8939 will have to stay as is, while for the 9607 and maybe
> several other devices it should be possible to create a fallback entry.
> 

IMHO the existing tsens-v2 support is a good example that it's sometimes
better to have some minor hardware details in the DT so the driver does
not have to be changed for every single platform. Extending from
specifying the number of sensors in the DT to the exact set of sensors
is not a very big step.

Also, aren't you also going against the custom here by moving the fuse
hardware details to the DT? :)

> [...]
> > And actually there are two revisions of 8939, the older one has one
> > sensor less (msm-3.10: msm8939-common.dtsi vs msm8939-v3.0.dtsi).
> > This could also be easily handled from the DT without any code changes:
> > 
> > 	qcom,sensors = <0 1 2 3 5 6 7 8 9>;
> 
> Usually we only care about the latest revision of the chip, earlier
> revisions typically correspond to engineering samples, never hitting the
> actual consumer devices.
> 

I'm afraid we might have to care about both revisions here - I recently
checked a couple of MSM8939 devices in postmarketOS and there are
definitely two different revisions used in production - they are easily
identifiable since they have different CPU revisions in the "lscpu"
output (Cortex-A53 r0p1 vs r0p4).

Thanks,
Stephan
Dmitry Baryshkov Sept. 25, 2022, 11:21 a.m. UTC | #6
On 25 September 2022 13:20:09 GMT+03:00, Stephan Gerhold <stephan@gerhold.net> wrote:
>On Sat, Sep 24, 2022 at 09:58:56PM +0300, Dmitry Baryshkov wrote:
>> On 22/09/2022 20:23, Stephan Gerhold wrote:
>> > On Sat, Sep 10, 2022 at 03:46:51PM +0300, Dmitry Baryshkov wrote:
>> > > Historically the tsens driver fetches the calibration data as a blob and
>> > > then parses the blob on its own. This results in semi-duplicated code
>> > > spreading over the platform-specific functions.
>> > > 
>> > > This patch series changes tsens calibration code to use pre-parsed nvmem
>> > > cells rather than parsing the blob in the driver. For backwards
>> > > compatibility the old code is left in place for msm8916 and qcs404, two
>> > > platforms which have in-tree DT files. For msm8974 the original function
>> > > is left intact, since it differs significantly (and I can not test the
>> > > code on msm8974). For all other affected platforms the old parsing code
>> > > has been dropped as a part of this RFC.
>> > > 
>> > > The code was tested on msm8916 and qcs404 only, thus it is being sent as
>> > > an RFC.
>> > > 
>> > 
>> > Thanks a lot for working on this!
>> > 
>> > After thinking about this for a while I wonder if we can go even a step
>> > further: Can we drop SoC-specific code entirely for 8939 and 9607 and
>> > match the generic compatible (qcom,tsens-v0_1)? This would allow most
>> > v0.1 plaforms to use generic code like for qcom,tsens-v2.
>> 
>> While this idea looks appealing, I think it's a bit against our custom to
>> put hardware details into the driver rather than putting them into the DT.
>> So, I think, the 8939 will have to stay as is, while for the 9607 and maybe
>> several other devices it should be possible to create a fallback entry.
>> 
>
>IMHO the existing tsens-v2 support is a good example that it's sometimes
>better to have some minor hardware details in the DT so the driver does
>not have to be changed for every single platform. Extending from
>specifying the number of sensors in the DT to the exact set of sensors
>is not a very big step.

Fine, I will take a look.

>
>Also, aren't you also going against the custom here by moving the fuse
>hardware details to the DT? :)

Not quite. Fuses are completely software thing. 

>
>> [...]
>> > And actually there are two revisions of 8939, the older one has one
>> > sensor less (msm-3.10: msm8939-common.dtsi vs msm8939-v3.0.dtsi).
>> > This could also be easily handled from the DT without any code changes:
>> > 
>> > 	qcom,sensors = <0 1 2 3 5 6 7 8 9>;
>> 
>> Usually we only care about the latest revision of the chip, earlier
>> revisions typically correspond to engineering samples, never hitting the
>> actual consumer devices.
>> 
>
>I'm afraid we might have to care about both revisions here - I recently
>checked a couple of MSM8939 devices in postmarketOS and there are
>definitely two different revisions used in production - they are easily
>identifiable since they have different CPU revisions in the "lscpu"
>output (Cortex-A53 r0p1 vs r0p4).

Ugh. 

>
>Thanks,
>Stephan
Stephan Gerhold Sept. 25, 2022, 12:28 p.m. UTC | #7
On Sun, Sep 25, 2022 at 02:21:11PM +0300, Dmitry Baryshkov wrote:
> On 25 September 2022 13:20:09 GMT+03:00, Stephan Gerhold <stephan@gerhold.net> wrote:
> >On Sat, Sep 24, 2022 at 09:58:56PM +0300, Dmitry Baryshkov wrote:
> >> On 22/09/2022 20:23, Stephan Gerhold wrote:
> >> > On Sat, Sep 10, 2022 at 03:46:51PM +0300, Dmitry Baryshkov wrote:
> >> > > Historically the tsens driver fetches the calibration data as a blob and
> >> > > then parses the blob on its own. This results in semi-duplicated code
> >> > > spreading over the platform-specific functions.
> >> > > 
> >> > > This patch series changes tsens calibration code to use pre-parsed nvmem
> >> > > cells rather than parsing the blob in the driver. For backwards
> >> > > compatibility the old code is left in place for msm8916 and qcs404, two
> >> > > platforms which have in-tree DT files. For msm8974 the original function
> >> > > is left intact, since it differs significantly (and I can not test the
> >> > > code on msm8974). For all other affected platforms the old parsing code
> >> > > has been dropped as a part of this RFC.
> >> > > 
> >> > > The code was tested on msm8916 and qcs404 only, thus it is being sent as
> >> > > an RFC.
> >> > > 
> >> > 
> >> > Thanks a lot for working on this!
> >> > 
> >> > After thinking about this for a while I wonder if we can go even a step
> >> > further: Can we drop SoC-specific code entirely for 8939 and 9607 and
> >> > match the generic compatible (qcom,tsens-v0_1)? This would allow most
> >> > v0.1 plaforms to use generic code like for qcom,tsens-v2.
> >> 
> >> While this idea looks appealing, I think it's a bit against our custom to
> >> put hardware details into the driver rather than putting them into the DT.
> >> So, I think, the 8939 will have to stay as is, while for the 9607 and maybe
> >> several other devices it should be possible to create a fallback entry.
> >> 
> >
> >IMHO the existing tsens-v2 support is a good example that it's sometimes
> >better to have some minor hardware details in the DT so the driver does
> >not have to be changed for every single platform. Extending from
> >specifying the number of sensors in the DT to the exact set of sensors
> >is not a very big step.
> 
> Fine, I will take a look.
> 

Thanks!

> >
> >Also, aren't you also going against the custom here by moving the fuse
> >hardware details to the DT? :)
> 
> Not quite. Fuses are completely software thing. 
> 

Good point, but this depends on the point of view: Since those fuses are
likely "burned in" at the factory I would consider them to be part of
"hardware" like everything else. Even if I wanted to I probably couldn't
change anything about the layout, at least as a user (and probably even
as a customer). :-)

> >
> >> [...]
> >> > And actually there are two revisions of 8939, the older one has one
> >> > sensor less (msm-3.10: msm8939-common.dtsi vs msm8939-v3.0.dtsi).
> >> > This could also be easily handled from the DT without any code changes:
> >> > 
> >> > 	qcom,sensors = <0 1 2 3 5 6 7 8 9>;
> >> 
> >> Usually we only care about the latest revision of the chip, earlier
> >> revisions typically correspond to engineering samples, never hitting the
> >> actual consumer devices.
> >> 
> >
> >I'm afraid we might have to care about both revisions here - I recently
> >checked a couple of MSM8939 devices in postmarketOS and there are
> >definitely two different revisions used in production - they are easily
> >identifiable since they have different CPU revisions in the "lscpu"
> >output (Cortex-A53 r0p1 vs r0p4).
> 
> Ugh. 
> 

Indeed.

Thanks,
Stephan