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

[Wolfram Sang <wsa-dev@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2792 bytes --]

Hi Geert,

thanks for the prompt review!

> > +/* 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.

I'll check if int will do.

> > +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?

TBH I don't favor such macros, but since they are in quite some Renesas
drivers, I kept them in the first take for consistency reasons.

> > +       tj_2 = (CODETSD((ptat[1] - ptat[2]) * 137)
> > +               / (ptat[0] - ptat[2])) - CODETSD(41);
> 
> Isn't "* 1000" easier to read then CODETSD()?

Probably. I'd think there can be done even more to make this code a tad
more readable. For the first take, I'd like to keep these formulas,
though. They come from the BSP and are the only documentation about how
to calculate the temperature which we currently have. Changing them
will obsolete all the testing done so far.


> > +       /* 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?

Ditto.

> > +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;

I'll likely produce more bugs if 'i' is not an int ;)

> 
> > +       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?

Had that before, didn't work. Since the calculation involves
substraction with other ints, I prefer to have them all the same type as
the fix.

> > +               tsc = devm_kzalloc(dev, sizeof(*tsc), GFP_KERNEL);
> > +               if (!tsc)
> > +                       return -ENOMEM;
> 
> Missing pm_runtime_put() etc.?
> 
> ret = -ENOMEM;
> goto error_unregister;

Yes!

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]