Date:   Tue, 19 Apr 2022 19:23:22 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Keerthy <j-keerthy@...com>, robh+dt@...nel.org,
        rui.zhang@...el.com, amitk@...nel.org, kristo@...nel.org
Cc:     linux-pm@...r.kernel.org, vigneshr@...com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
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

Powered by blists - more mailing lists