| Message ID | 20221220024721.947147-9-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thermal/drivers/tsens: specify nvmem cells in DT rather than parsing them manually | expand |
On 20/12/2022 02:47, Dmitry Baryshkov wrote: > There is no dtsi file for msm8939 in the kernel sources. Drop the > compatibility with unofficial dtsi and remove support for handling the > single-cell calibration data on msm8939. > > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Maybe its a better idea to just - finally - get the 8939 dtsi upstream. Is there anything that needs to change in this ? If not I'll just V2 in in the next few hours. tsens: thermal-sensor@4a9000 { compatible = "qcom,msm8939-tsens", "qcom,tsens-v0_1"; reg = <0x004a9000 0x1000>, /* TM */ <0x004a8000 0x1000>; /* SROT */ nvmem-cells = <&tsens_caldata>; nvmem-cell-names = "calib"; #qcom,sensors = <10>; interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "uplow"; #thermal-sensor-cells = <1>; }; https://git.linaro.org/people/bryan.odonoghue/kernel.git/commit/?h=linux-next-22-12-15-msm8939-no-cpr&id=146087a134306dfb6e1ec48b20b19a278f336b15 --- bod
On 20.12.2022 11:08, Bryan O'Donoghue wrote: > On 20/12/2022 02:47, Dmitry Baryshkov wrote: >> There is no dtsi file for msm8939 in the kernel sources. Drop the >> compatibility with unofficial dtsi and remove support for handling the >> single-cell calibration data on msm8939. >> >> Cc: Shawn Guo <shawn.guo@linaro.org> >> Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Maybe its a better idea to just - finally - get the 8939 dtsi upstream. > > Is there anything that needs to change in this ? If not I'll just V2 in in the next few hours. The point of this patchset is to remove logic like this, as it's very repetetive and all it does is read fuses with a ton of magic offsets. Dmitry pushes that to DT with the generic nvmem API, so that instead of these magic &s and <<s, one is simply supposed to define QFPROM fuses at the correct offset and with a matching bits=<> property. This does not remove any functionality, it just changes how the fuses are accessed and makes the code more generic. Konrad > > tsens: thermal-sensor@4a9000 { > compatible = "qcom,msm8939-tsens", "qcom,tsens-v0_1"; > reg = <0x004a9000 0x1000>, /* TM */ > <0x004a8000 0x1000>; /* SROT */ > nvmem-cells = <&tsens_caldata>; > nvmem-cell-names = "calib"; > #qcom,sensors = <10>; > interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "uplow"; > #thermal-sensor-cells = <1>; > }; > > https://git.linaro.org/people/bryan.odonoghue/kernel.git/commit/?h=linux-next-22-12-15-msm8939-no-cpr&id=146087a134306dfb6e1ec48b20b19a278f336b15 > > --- > bod
On 20/12/2022 10:21, Konrad Dybcio wrote: > The point of this patchset is to remove logic like this, as it's > very repetetive and all it does is read fuses with a ton of magic > offsets. Dmitry pushes that to DT with the generic nvmem API, so > that instead of these magic &s and <<s, one is simply supposed to > define QFPROM fuses at the correct offset and with a matching bits=<> > property. This does not remove any functionality, it just changes > how the fuses are accessed and makes the code more generic. > > Konrad Hmm but then there's extra work to get this working again on 8939 right? Seems very dogmatic to drop working code for want of sending a dtsi out. Certainly my preference is to keep exiting working code and just complete landing the relevant dtsi, rather than eject working code and have to do the same work all over again. --- bod
On 20.12.2022 11:26, Bryan O'Donoghue wrote: > On 20/12/2022 10:21, Konrad Dybcio wrote: >> The point of this patchset is to remove logic like this, as it's >> very repetetive and all it does is read fuses with a ton of magic >> offsets. Dmitry pushes that to DT with the generic nvmem API, so >> that instead of these magic &s and <<s, one is simply supposed to >> define QFPROM fuses at the correct offset and with a matching bits=<> >> property. This does not remove any functionality, it just changes >> how the fuses are accessed and makes the code more generic. >> >> Konrad > > Hmm but then there's extra work to get this working again on 8939 right? > > Seems very dogmatic to drop working code for want of sending a dtsi out. > > Certainly my preference is to keep exiting working code and just complete landing the relevant dtsi, rather than eject working code and have to do the same work all over again. Well, I don't really know how to say it in a way that wouldn't make it sound mean, but trust me I don't want it to be mean.. Mainline does not and will not (for the most part) care about out of tree code, so cleanups of parts like this with no users are wholly expected if your DT hasn't landed upstream (or has been stuck in review for a long long time like it is the case with various parts of 8939).. Keeping this old iteration is blocking progress, as the other similar ones (that *do* have mainline users) are left in place just to be backwards compatible with old DTs that may have been pulled from torvalds/linux by third party projects, like U-Boot, *BSDs or something. Trimming away this now-duplicated code will shrink the driver, reducing bloat for everyone that compiles it in and doesn't use the 8939-specific path. Konrad > > --- > bod
On 20/12/2022 10:32, Konrad Dybcio wrote: > Mainline does not and will not (for the most part) care about > out of tree code, so cleanups of parts like this with no users > are wholly expected if your DT hasn't landed upstream (or has been > stuck in review for a long long time like it is the case with > various parts of 8939).. Keeping this old iteration is blocking > progress, as the other similar ones (that*do* have mainline users) > are left in place just to be backwards compatible with old DTs > that may have been pulled from torvalds/linux by third party projects, > like U-Boot, *BSDs or something. Trimming away this now-duplicated > code will shrink the driver, reducing bloat for everyone that > compiles it in and doesn't use the 8939-specific path. I entirely take your point on duration Konrad but, I think we can be just a little more pragmatic and hold off on dropping working code and land the dtsi. We went to the trouble of upstreaming the enabling code for the 8939, the right thing to do, IMO is to finish off the job. --- bod
Hello Bryan, On Tue, 20 Dec 2022 at 12:34, Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 20/12/2022 10:32, Konrad Dybcio wrote: > > Mainline does not and will not (for the most part) care about > > out of tree code, so cleanups of parts like this with no users > > are wholly expected if your DT hasn't landed upstream (or has been > > stuck in review for a long long time like it is the case with > > various parts of 8939).. Keeping this old iteration is blocking > > progress, as the other similar ones (that*do* have mainline users) > > are left in place just to be backwards compatible with old DTs > > that may have been pulled from torvalds/linux by third party projects, > > like U-Boot, *BSDs or something. Trimming away this now-duplicated > > code will shrink the driver, reducing bloat for everyone that > > compiles it in and doesn't use the 8939-specific path. > > I entirely take your point on duration Konrad but, I think we can be > just a little more pragmatic and hold off on dropping working code and > land the dtsi. > > We went to the trouble of upstreaming the enabling code for the 8939, > the right thing to do, IMO is to finish off the job. I'm pretty sorry to step on your toes here. It's up to the maintainers of the platform and of the thermal subsystem. However I'd suggest getting rid of this code. Would me doing the dtsi patch for you to test on 8939 help you in any way?
On 20/12/2022 13:29, Dmitry Baryshkov wrote:
> Would me doing the dtsi patch for you to test on 8939 help you in any way?
Yep.
I'd be happy enough with whatever re-enabling work is required to offset
the removal.
---
bod
On 20/12/2022 02:47, Dmitry Baryshkov wrote: > There is no dtsi file for msm8939 in the kernel sources. Drop the > compatibility with unofficial dtsi and remove support for handling the > single-cell calibration data on msm8939. > > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Happy to take an off-list or follow on patch from Dmitry to re-enable on 8939. I will drop the relevant node on v2 of the 8939 patches in the interregnum. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
diff --git a/drivers/thermal/qcom/tsens-v0_1.c b/drivers/thermal/qcom/tsens-v0_1.c index 7f87b403c6fa..8f1ea7075354 100644 --- a/drivers/thermal/qcom/tsens-v0_1.c +++ b/drivers/thermal/qcom/tsens-v0_1.c @@ -48,63 +48,6 @@ #define MSM8916_CAL_SEL_MASK 0xe0000000 #define MSM8916_CAL_SEL_SHIFT 29 -/* eeprom layout data for 8939 */ -#define MSM8939_BASE0_MASK 0x000000ff -#define MSM8939_BASE1_MASK 0xff000000 -#define MSM8939_BASE0_SHIFT 0 -#define MSM8939_BASE1_SHIFT 24 - -#define MSM8939_S0_P1_MASK 0x000001f8 -#define MSM8939_S1_P1_MASK 0x001f8000 -#define MSM8939_S2_P1_MASK_0_4 0xf8000000 -#define MSM8939_S2_P1_MASK_5 0x00000001 -#define MSM8939_S3_P1_MASK 0x00001f80 -#define MSM8939_S4_P1_MASK 0x01f80000 -#define MSM8939_S5_P1_MASK 0x00003f00 -#define MSM8939_S6_P1_MASK 0x03f00000 -#define MSM8939_S7_P1_MASK 0x0000003f -#define MSM8939_S8_P1_MASK 0x0003f000 -#define MSM8939_S9_P1_MASK 0x07e00000 - -#define MSM8939_S0_P2_MASK 0x00007e00 -#define MSM8939_S1_P2_MASK 0x07e00000 -#define MSM8939_S2_P2_MASK 0x0000007e -#define MSM8939_S3_P2_MASK 0x0007e000 -#define MSM8939_S4_P2_MASK 0x7e000000 -#define MSM8939_S5_P2_MASK 0x000fc000 -#define MSM8939_S6_P2_MASK 0xfc000000 -#define MSM8939_S7_P2_MASK 0x00000fc0 -#define MSM8939_S8_P2_MASK 0x00fc0000 -#define MSM8939_S9_P2_MASK_0_4 0xf8000000 -#define MSM8939_S9_P2_MASK_5 0x00002000 - -#define MSM8939_S0_P1_SHIFT 3 -#define MSM8939_S1_P1_SHIFT 15 -#define MSM8939_S2_P1_SHIFT_0_4 27 -#define MSM8939_S2_P1_SHIFT_5 0 -#define MSM8939_S3_P1_SHIFT 7 -#define MSM8939_S4_P1_SHIFT 19 -#define MSM8939_S5_P1_SHIFT 8 -#define MSM8939_S6_P1_SHIFT 20 -#define MSM8939_S7_P1_SHIFT 0 -#define MSM8939_S8_P1_SHIFT 12 -#define MSM8939_S9_P1_SHIFT 21 - -#define MSM8939_S0_P2_SHIFT 9 -#define MSM8939_S1_P2_SHIFT 21 -#define MSM8939_S2_P2_SHIFT 1 -#define MSM8939_S3_P2_SHIFT 13 -#define MSM8939_S4_P2_SHIFT 25 -#define MSM8939_S5_P2_SHIFT 14 -#define MSM8939_S6_P2_SHIFT 26 -#define MSM8939_S7_P2_SHIFT 6 -#define MSM8939_S8_P2_SHIFT 18 -#define MSM8939_S9_P2_SHIFT_0_4 27 -#define MSM8939_S9_P2_SHIFT_5 13 - -#define MSM8939_CAL_SEL_MASK 0x7 -#define MSM8939_CAL_SEL_SHIFT 0 - /* eeprom layout data for 8974 */ #define BASE1_MASK 0xff #define S0_P1_MASK 0x3f00 @@ -284,81 +227,6 @@ static int calibrate_8916(struct tsens_priv *priv) return 0; } -static int calibrate_8939(struct tsens_priv *priv) -{ - int base0 = 0, base1 = 0, i; - u32 p1[10], p2[10]; - int mode = 0; - u32 *qfprom_cdata; - u32 cdata[6]; - int ret; - - ret = tsens_calibrate_common(priv); - if (!ret) - return 0; - - qfprom_cdata = (u32 *)qfprom_read(priv->dev, "calib"); - if (IS_ERR(qfprom_cdata)) - return PTR_ERR(qfprom_cdata); - - /* Mapping between qfprom nvmem and calibration data */ - cdata[0] = qfprom_cdata[12]; - cdata[1] = qfprom_cdata[13]; - cdata[2] = qfprom_cdata[0]; - cdata[3] = qfprom_cdata[1]; - cdata[4] = qfprom_cdata[22]; - cdata[5] = qfprom_cdata[21]; - - mode = (cdata[0] & MSM8939_CAL_SEL_MASK) >> MSM8939_CAL_SEL_SHIFT; - dev_dbg(priv->dev, "calibration mode is %d\n", mode); - - switch (mode) { - case TWO_PT_CALIB: - base1 = (cdata[3] & MSM8939_BASE1_MASK) >> MSM8939_BASE1_SHIFT; - p2[0] = (cdata[0] & MSM8939_S0_P2_MASK) >> MSM8939_S0_P2_SHIFT; - p2[1] = (cdata[0] & MSM8939_S1_P2_MASK) >> MSM8939_S1_P2_SHIFT; - p2[2] = (cdata[1] & MSM8939_S2_P2_MASK) >> MSM8939_S2_P2_SHIFT; - p2[3] = (cdata[1] & MSM8939_S3_P2_MASK) >> MSM8939_S3_P2_SHIFT; - p2[4] = (cdata[1] & MSM8939_S4_P2_MASK) >> MSM8939_S4_P2_SHIFT; - p2[5] = (cdata[2] & MSM8939_S5_P2_MASK) >> MSM8939_S5_P2_SHIFT; - p2[6] = (cdata[2] & MSM8939_S6_P2_MASK) >> MSM8939_S6_P2_SHIFT; - p2[7] = (cdata[3] & MSM8939_S7_P2_MASK) >> MSM8939_S7_P2_SHIFT; - p2[8] = (cdata[3] & MSM8939_S8_P2_MASK) >> MSM8939_S8_P2_SHIFT; - p2[9] = (cdata[4] & MSM8939_S9_P2_MASK_0_4) >> MSM8939_S9_P2_SHIFT_0_4; - p2[9] |= ((cdata[5] & MSM8939_S9_P2_MASK_5) >> MSM8939_S9_P2_SHIFT_5) << 5; - for (i = 0; i < priv->num_sensors; i++) - p2[i] = (base1 + p2[i]) << 2; - fallthrough; - case ONE_PT_CALIB2: - base0 = (cdata[2] & MSM8939_BASE0_MASK) >> MSM8939_BASE0_SHIFT; - p1[0] = (cdata[0] & MSM8939_S0_P1_MASK) >> MSM8939_S0_P1_SHIFT; - p1[1] = (cdata[0] & MSM8939_S1_P1_MASK) >> MSM8939_S1_P1_SHIFT; - p1[2] = (cdata[0] & MSM8939_S2_P1_MASK_0_4) >> MSM8939_S2_P1_SHIFT_0_4; - p1[2] |= ((cdata[1] & MSM8939_S2_P1_MASK_5) >> MSM8939_S2_P1_SHIFT_5) << 5; - p1[3] = (cdata[1] & MSM8939_S3_P1_MASK) >> MSM8939_S3_P1_SHIFT; - p1[4] = (cdata[1] & MSM8939_S4_P1_MASK) >> MSM8939_S4_P1_SHIFT; - p1[5] = (cdata[2] & MSM8939_S5_P1_MASK) >> MSM8939_S5_P1_SHIFT; - p1[6] = (cdata[2] & MSM8939_S6_P1_MASK) >> MSM8939_S6_P1_SHIFT; - p1[7] = (cdata[3] & MSM8939_S7_P1_MASK) >> MSM8939_S7_P1_SHIFT; - p1[8] = (cdata[3] & MSM8939_S8_P1_MASK) >> MSM8939_S8_P1_SHIFT; - p1[9] = (cdata[4] & MSM8939_S9_P1_MASK) >> MSM8939_S9_P1_SHIFT; - for (i = 0; i < priv->num_sensors; i++) - p1[i] = ((base0) + p1[i]) << 2; - break; - default: - for (i = 0; i < priv->num_sensors; i++) { - p1[i] = 500; - p2[i] = 780; - } - break; - } - - compute_intercept_slope(priv, p1, p2, mode); - kfree(qfprom_cdata); - - return 0; -} - static int calibrate_8974(struct tsens_priv *priv) { int base1 = 0, base2 = 0, i; @@ -598,6 +466,12 @@ static const struct reg_field tsens_v0_1_regfields[MAX_REGFIELDS] = { [TRDY] = REG_FIELD(TM_TRDY_OFF, 0, 0), }; +static const struct tsens_ops ops_v0_1 = { + .init = init_common, + .calibrate = tsens_calibrate_common, + .get_temp = get_temp_common, +}; + static const struct tsens_ops ops_8916 = { .init = init_common, .calibrate = calibrate_8916, @@ -613,15 +487,9 @@ 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, + .ops = &ops_v0_1, .hw_ids = (unsigned int []){ 0, 1, 2, 3, 5, 6, 7, 8, 9, 10 }, .feat = &tsens_v0_1_feat,
There is no dtsi file for msm8939 in the kernel sources. Drop the compatibility with unofficial dtsi and remove support for handling the single-cell calibration data on msm8939. Cc: Shawn Guo <shawn.guo@linaro.org> Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/thermal/qcom/tsens-v0_1.c | 146 ++---------------------------- 1 file changed, 7 insertions(+), 139 deletions(-)