Re: [PATCH 1/6] thermal/intel: Introduce Intel TCC library

Rafael J. Wysocki
2022-12-12T12:12:04Z

On Sun, Dec 11, 2022 at 8:24 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
>
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + *tjmax = (eax >> 16) & 0xff;
> > >
> > > This means that the tjmax value cannot be negative.
> > >
> > > > +
> > > > + return *tjmax ? 0 : -EINVAL;
> > >
> > > So the return value of this function could be tjmax if positive or
> > > a
> > > negative error code otherwise. No return pointers needed.
> >
> > I tried both. And I think I chose this solution just because it makes
> > the following cleanup patches in this series looks prettier.
> >
> > I will try your suggestion, and if there is any other reason I wrote
> > it
> > in this way, I will find it out again. :p
>
> I see.
> Say, we have to use
>
> intel_tcc_get_temp(int cpu, int *temp)

Do we?

> so I keep the same format for all the intel_tcc_get_XXX APIs
>
> intel_tcc_get_tjmax(int cpu, int *tjmax)
> intel_tcc_get_offset(int cpu, int *offset)
>
> so that the return value is decoded in the same way. This helps avoid
> return value check mistakes for the callers.
>
> surely I can remove the return pointer if you still prefer that. :p

Using a function value directly is very much preferred unless there
really are two values to return.

In these particular cases a negative return value can be clearly
interpreted as an error condition, so using the return value directly
is sufficient.

> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_NS_GPL(intel_tcc_get_offset, INTEL_TCC);
> > > > +
> > > > +/**
> > > > + * intel_tcc_set_offset() - set the TCC offset value to Tjmax
> > > > + * @cpu: cpu that the MSR should be run on.
> > > > + * @offset: TCC offset value in degree C
> > >
> > > I think that this cannot be negative, so maybe say "in K" instead
> > > of
> > > "in degree C"?
> >
> > degree C is the unit used in the Intel SDM, so better to keep this
> > comment aligned.
> >
> > > And maybe it's better to pass u8 here?
> >
> > sounds good, will do in next version.
> >
> to keep consistent, we can use either
> int intel_tcc_get_offset(int cpu)
> int intel_tcc_set_offset(int cpu, int offset)

What about intel_tcc_set_offset(int cpu, u8 offset)?

> or
> int intel_tcc_get_offset(int cpu, u8 *offset)
> int intel_tcc_set_offset(int cpu, u8 offset)
>
> or else callers need to declare 'offset' but with different type, when
> getting and setting it, which looks strange.

Well, one can surely pass an int as a u8 argument and assign a u8
return value to an int variable.