Messages in this thread | | | Date | Tue, 19 Apr 2022 19:23:22 +0200 | From | Daniel Lezcano <> | Subject | Re: [PATCH v5 4/4] thermal: k3_j72xx_bandgap: Add the bandgap driver support |
| |
Hi Kheerty,
On 12/04/2022 12:14, Keerthy wrote:
[ ... ]
> +/** > + * struct err_values - structure containing error/reference values > + * @refs: reference error values for -40C, 30C, 125C & 150C > + * @errs: Actual error values for -40C, 30C, 125C & 150C read from the efuse > + */ > +struct err_values { > + int refs[4]; > + int errs[4]; > +}; > + > +static void create_table_segments(struct err_values *err_vals, int seg, > + int *ref_table) > +{ > + int m = 0, c, num, den, i, err, idx1, idx2, err1, err2, ref1, ref2; > + > + if (seg == 0) > + idx1 = 0; > + else > + idx1 = err_vals->refs[seg]; > + > + idx2 = err_vals->refs[seg + 1]; > + err1 = err_vals->errs[seg]; > + err2 = err_vals->errs[seg + 1]; > + ref1 = err_vals->refs[seg]; > + ref2 = err_vals->refs[seg + 1]; > + > + /* > + * Calculate the slope with adc values read from the register > + * as the y-axis param and err in adc value as x-axis param > + */ > + num = ref2 - ref1; > + den = err2 - err1; > + if (den) > + m = num / den; > + c = ref2 - m * err2; > + > + /* > + * Take care of divide by zero error if error values are same > + * Or when the slope is 0 > + */ > + if (den != 0 && m != 0) { > + for (i = idx1; i <= idx2; i++) { > + err = (i - c) / m; > + if (((i + err1) < 0) || ((i + err1) > 1023)) > + continue;
Should the test being against 'err' not 'err1' ?
> + derived_table[i] = ref_table[i + err]; > + } > + } else { /* Constant error take care of divide by zero */ > + for (i = idx1; i <= idx2; i++) { > + if (((i + err1) < 0) || ((i + err1) > 1023)) > + continue; > + derived_table[i] = ref_table[i + err1]; > + } > + } > +} > + > +static int prep_lookup_table(struct err_values *err_vals, int *ref_table) > +{ > + int inc, i, seg; > + > + /* > + * Fill up the lookup table under 3 segments > + * region -40C to +30C > + * region +30C to +125C > + * region +125C to +150C > + */ > + for (seg = 0; seg < 3; seg++) > + create_table_segments(err_vals, seg, ref_table); > + > + /* Get to the first valid temperature */ > + i = 0; > + while (!derived_table[i]) > + i++; > + > + /* > + * Get to the last zero index and back fill the temperature for > + * sake of continuity > + */ > + if (i) { > + /* 300 milli celsius steps */ > + while (i--) > + derived_table[i] = derived_table[i + 1] - 300; > + /* case 0 */ > + derived_table[i] = derived_table[i + 1] - 300; > + }
memset ?
> + > + /* > + * Fill the last trailing 0s which are unfilled with increments of > + * 100 milli celsius till 1023 code > + */ > + i = 1023; > + while (!derived_table[i]) > + i--;
All those loop can be replaced by addresses variant IMO. Up to you if you want to use mem* variants.
eg. void *flz = memrchr(derived_table, 0, size); /* find last zero */
> + > + i++; > + inc = 1; > + while (i < 1024) { > + derived_table[i] = derived_table[i - 1] + inc * 100; > + i++; > + } > + > + return 0; > +}
Please replace all the 1024-ish literals everywhere by a macro
> +struct k3_thermal_data; > + > +struct k3_j72xx_bandgap { > + struct device *dev; > + void __iomem *base; > + void __iomem *cfg2_base; > + void __iomem *fuse_base; > + struct k3_thermal_data *ts_data[K3_VTM_MAX_NUM_TS]; > +}; > + > +/* common data structures */ > +struct k3_thermal_data { > + struct k3_j72xx_bandgap *bgp; > + u32 ctrl_offset; > + u32 stat_offset; > +}; > + > +static int two_cmp(int tmp, int mask) > +{ > + tmp = ~(tmp); > + tmp &= mask; > + tmp += 1; > + > + /* Return negative value */ > + return (0 - tmp); > +} > + > +static unsigned int vtm_get_best_value(unsigned int s0, unsigned int s1, > + unsigned int s2) > +{ > + int d01 = abs(s0 - s1); > + int d02 = abs(s0 - s2); > + int d12 = abs(s1 - s2); > + > + if (d01 <= d02 && d01 <= d12) > + return (s0 + s1) / 2; > + > + if (d02 <= d01 && d02 <= d12) > + return (s0 + s2) / 2; > + > + return (s1 + s2) / 2; > +} > + > +static inline int k3_bgp_read_temp(struct k3_thermal_data *devdata, > + int *temp) > +{ > + struct k3_j72xx_bandgap *bgp; > + unsigned int dtemp, s0, s1, s2; > + > + bgp = devdata->bgp; > + /* > + * Errata is applicable for am654 pg 1.0 silicon/J7ES. There > + * is a variation of the order for certain degree centigrade on AM654. > + * Work around that by getting the average of two closest > + * readings out of three readings everytime we want to > + * report temperatures. > + * > + * Errata workaround. > + */ > + s0 = readl(bgp->base + devdata->stat_offset) & > + K3_VTM_TS_STAT_DTEMP_MASK; > + s1 = readl(bgp->base + devdata->stat_offset) & > + K3_VTM_TS_STAT_DTEMP_MASK; > + s2 = readl(bgp->base + devdata->stat_offset) & > + K3_VTM_TS_STAT_DTEMP_MASK; > + dtemp = vtm_get_best_value(s0, s1, s2); > + > + if (dtemp < 0 || dtemp > 1023) > + return -EINVAL; > + > + *temp = derived_table[dtemp]; > + > + return 0; > +} > + > +/* Get temperature callback function for thermal zone */ > +static int k3_thermal_get_temp(void *devdata, int *temp) > +{ > + struct k3_thermal_data *data = devdata; > + int ret = 0; > + > + ret = k3_bgp_read_temp(data, temp); > + if (ret) > + return ret; > + > + return ret; > +} > + > +static const struct thermal_zone_of_device_ops k3_of_thermal_ops = { > + .get_temp = k3_thermal_get_temp, > +}; > + > +static int k3_j72xx_bandgap_temp_to_adc_code(int temp) > +{ > + int low = 0, high = 1023, mid; > + > + if (temp > 160000 || temp < -50000) > + return -EINVAL; > + > + /* Binary search to find the adc code */ > + while (low < (high - 1)) { > + mid = (low + high) / 2; > + if (temp <= derived_table[mid]) > + high = mid; > + else > + low = mid; > + } > + > + return mid; > +} > + > +static void get_efuse_values(int id, struct k3_thermal_data *data, int *err, > + struct k3_j72xx_bandgap *bgp) > +{ > + int i, tmp, pow; > + int ct_offsets[K3_VTM_CORRECTION_TEMP_CNT]; > + int ct_bm[K3_VTM_CORRECTION_TEMP_CNT]; > + > + /* Populate efuse reg offsets & Bit masks for -40C, 30C, 125C */ > + switch (id) { > + case 0: > + ct_offsets[0] = 0x0; > + ct_offsets[1] = 0x8; > + ct_offsets[2] = 0x4; > + ct_bm[0] = 0x3f; > + ct_bm[1] = 0x1fe000; > + ct_bm[2] = 0x1ff; > + break; > + > + case 1: > + ct_offsets[0] = 0x0; > + ct_offsets[1] = 0x8; > + ct_offsets[2] = 0x4; > + ct_bm[0] = 0xfc0; > + ct_bm[1] = 0x1fe00000; > + ct_bm[2] = 0x3fe00; > + break; > + > + case 2: > + ct_offsets[0] = 0x0; > + ct_offsets[1] = -1; > + ct_offsets[2] = 0x4; > + ct_bm[0] = 0x3f000; > + ct_bm[1] = 0x7f800000; > + ct_bm[2] = 0x7fc0000; > + break; > + > + case 3: > + ct_offsets[0] = 0x0; > + ct_offsets[1] = 0xC; > + ct_offsets[2] = -1; /* Spread across 2 registers */ > + ct_bm[0] = 0xfc0000; > + ct_bm[1] = 0x1fe0; > + ct_bm[2] = 0x1f800000; > + break; > + > + case 4: > + ct_offsets[0] = 0x0; > + ct_offsets[1] = 0xc; > + ct_offsets[2] = 0x8; > + ct_bm[0] = 0x3f000000; > + ct_bm[1] = 0x1fe000; > + ct_bm[2] = 0x1ff0; > + break; > + }
The way this is initialized looks strange to me
Why not:
int ct_offsets[][] = { { 0x0, 0x8, 0x4 }, { 0x0, 0x8, 0x4 }, { 0x0, -1, 0x4 }, { 0x0, 0xC, -1 } { 0x0, 0xc, 0x8 } };
int ct_bm[][] = { { 0x3f, 0x1fe000, 0x1ff }, { 0xfc0, 0x1fe000, 0x3fe00 }, { 0x3f000, 0x7f800000, 0x7fc0000 }, { 0xfc0000, 0x1fe0, 0x1f800000 }, { 0x3f000000, 0x1fe000, 0x1ff0 } };
And then : ct_offsets[id][i] below ?
> + for (i = 0; i < 3; i++) {
ARRAY_SIZE on ct_offsets ?
> + /* Extract the offset value using bit-mask */ > + if (ct_offsets[i] == -1 && i == 1) { > + /* 25C offset Case of Sensor 2 split between 2 regs */ > + tmp = (readl(bgp->fuse_base + 0x8) & 0xE0000000) >> (29); > + tmp |= ((readl(bgp->fuse_base + 0xC) & 0x1F) << 3); > + pow = tmp & 0x80; > + } else if (ct_offsets[i] == -1 && i == 2) { > + /* 125C Case of Sensor 3 split between 2 regs */ > + tmp = (readl(bgp->fuse_base + 0x4) & 0xF8000000) >> (27); > + tmp |= ((readl(bgp->fuse_base + 0x8) & 0xF) << 5); > + pow = tmp & 0x100; > + } else { > + tmp = readl(bgp->fuse_base + ct_offsets[i]); > + tmp &= ct_bm[i]; > + tmp = tmp >> __ffs(ct_bm[i]); > + > + /* Obtain the sign bit pow*/ > + pow = ct_bm[i] >> __ffs(ct_bm[i]); > + pow += 1; > + pow /= 2; > + } > + > + /* Check for negative value */ > + if (tmp & pow) { > + /* 2's complement value */ > + tmp = two_cmp(tmp, ct_bm[i] >> __ffs(ct_bm[i])); > + } > + err[i] = tmp; > + } > + > + /* Err value for 150C is set to 0 */ > + err[i] = 0; > +} > + > +static void print_look_up_table(struct device *dev, int *ref_table) > +{ > + int i; > + > + dev_dbg(dev, "The contents of derived array\n"); > + dev_dbg(dev, "Code Temperaturei\n");
Typo: "Temperaturei"
> + for (i = 0; i <= 1023; i++) > + dev_dbg(dev, "%d %d %d\n", i, derived_table[i], ref_table[i]); > +} > + > +static const struct of_device_id of_k3_j72xx_bandgap_match[];
Pointless forward declaration
> + > +struct k3_j72xx_bandgap_data { > + unsigned int has_errata_i2128; > +}; > + > +static int k3_j72xx_bandgap_probe(struct platform_device *pdev) > +{ > + int ret = 0, cnt, val, id, table_size = 1024;
table_size to be replaced by the macro mentioned above
> + int high_max, low_temp; > + struct resource *res; > + struct device *dev = &pdev->dev; > + struct k3_j72xx_bandgap *bgp; > + struct k3_thermal_data *data; > + int workaround_needed = 0; > + const struct k3_j72xx_bandgap_data *driver_data; > + struct thermal_zone_device *ti_thermal; > + int *ref_table; > + struct err_values err_vals;
[ ... ]
-- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
| |