Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Wolfram,

On Mon, Nov 28, 2016 at 10:09 PM, Wolfram Sang
<wsa+renesas@sang-engineering.com> 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@linux-m68k.org

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