Hi Wolfram,
On Mon, Nov 28, 2016 at 10:09 PM, Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> +/* Structure for thermal temperature calculation */
> +struct equation_coefs {
> + long a1;
> + long b1;
> + long a2;
> + long b2;
Why long? Long has a different size for 32-bit and 64-bit platforms.
I know this is a driver for arm64, but if you need 64-bits, you want to make
this clear using s64, or long long.
> +static inline u32 rcar_gen3_thermal_read(struct rcar_gen3_thermal_tsc *tsc, u32 reg)
What a long function name...
> +{
> + return ioread32(tsc->base + reg);
> +}
> +
> +static inline void rcar_gen3_thermal_write(struct rcar_gen3_thermal_tsc *tsc,
> + u32 reg, u32 data)
> +{
> + iowrite32(data, tsc->base + reg);
> +}
... so using the "convenience" wrappers is more (typing) work than using
io{read,write}32() directly?
> +static void _linear_coefficient_calculation(struct rcar_gen3_thermal_tsc *tsc,
> + int *ptat, int *thcode)
> +{
> + int tj_2;
> + long a1, b1;
> + long a2, b2;
> + long a1_num, a1_den;
> + long a2_num, a2_den;
s64?
> + tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> + / (ptat[0] - ptat[2])) - CODETSD(41);
Isn't "* 1000" easier to read then CODETSD()?
> + /* calculate coefficients for linear equation */
> + a1_num = CODETSD(thcode[1] - thcode[2]);
> + a1_den = tj_2 - TJ_3;
> + a1 = (10000 * a1_num) / a1_den;
> + b1 = (10000 * thcode[2]) - ((a1 * TJ_3) / 1000);
Rounding needed for / 1000?
> + a2_num = CODETSD(thcode[1] - thcode[0]);
> + a2_den = tj_2 - TJ_1;
> + a2 = (10000 * a2_num) / a2_den;
> + b2 = (10000 * thcode[0]) - ((a2 * TJ_1) / 1000);
Idem.
> +static int rcar_gen3_thermal_probe(struct platform_device *pdev)
> +{
> + struct rcar_gen3_thermal_priv *priv;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct thermal_zone_device *zone;
> + int ret, i;
unsigned int i;
> + const struct rcar_gen3_thermal_data *match_data = of_device_get_match_data(dev);
> +
> + /* default values if FUSEs are missing */
> + int ptat[3] = { 2351, 1509, 435 };
unsigned?
> + int thcode[TSC_MAX_NUM][3] = {
unsigned?
> + { 3248, 2800, 2221 },
> + { 3245, 2795, 2216 },
> + { 3250, 2805, 2237 },
> + };
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, priv);
> +
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
> +
> + for (i = 0; i < TSC_MAX_NUM; i++) {
> + struct rcar_gen3_thermal_tsc *tsc;
> +
> + tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> + if (!tsc)
> + return -ENOMEM;
Missing pm_runtime_put() etc.?
ret = -ENOMEM;
goto error_unregister;
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
