RE: [PATCH] Add Silicom Platform Driver

[Huibin Shi]

-----Original Message-----
From: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx> 
Sent: Monday, August 21, 2023 6:40 AM
To: Huibin Shi <henrys@xxxxxxxxxxxxxxx>
Cc: Henry Shi <henryshi2018@xxxxxxxxx>; hbshi69@xxxxxxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; bp@xxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; hpa@xxxxxxxxx; hdegoede@xxxxxxxxxx; markgross@xxxxxxxxxx; jdelvare@xxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; platform-driver-x86@xxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; hb_shi2003@xxxxxxxxx; Wen Wang <wenw@xxxxxxxxxxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>
Subject: Re: [PATCH] Add Silicom Platform Driver

Caution: This is an external email. Please take care when clicking links or opening attachments.


On Sat, 19 Aug 2023, Guenter Roeck wrote:

> On Sat, Aug 19, 2023 at 02:20:32PM +0000, Huibin Shi wrote:
> > Hi Guenter,
> >
> > Thanks for your comments. Probably, I should not resubmit patch too rushed. I will add version number to subject and change log in cover letter for next resubmission.
> >
> > See my comments below. Please let me know whether you accept my explanation.
> >
> > Henry
> > -----Original Message-----
> [ ... ]
>
> > > +
> > > +static u32 temp_get(void)
> > > +{
> > > +     u32 reg;
> > > +
> > > +     mutex_lock(&mec_io_mutex);
> > > +     /* Select memory region */
> > > +     outb(IO_REG_BANK, EC_ADDR_MSB);
> > > +     outb(0xc, EC_ADDR_LSB);
> > > +     /* Get current data from the address */
> > > +     reg = inl(MEC_DATA(DEFAULT_CHAN_LO));
> > > +     mutex_unlock(&mec_io_mutex);
> > > +
> > > +     return (reg >> 16) / 10;
> >
> > The hwmon ABI expects temperatures to be reported in milli-degrees C.
> > The above sets the maximum temperature to 65,535 / 10 = 6,553 milli-degrees or 6.553 degrees C. It is very unlikely that this is correct.
> >
> > Again, I commented on this before.
> >
> > Henry: this is due to an internal implementation of MIcor-controller firmware, instead of putting real temperature to the register, it put (real temperature * 10 ) to the register. So, in order to report correct temperature to user space application, the read value is divided by 10, then report to user space.
> >
> > Please let me know if you accept this. If not, I can change the code, but let user space application to do adjustment.
>
> No, I do not accept this. I do not believe that the maximum 
> temperature reported by the microcontroller is 6.553 degrees C. I 
> suspect it reports 10th of degrees C. In that case, the number 
> reported should be multiplied by 100 to make it milli-degrees C as expected by the ABI.

...And for that multiplication, please use the constant defined in include/linux/units.h instead of a literal.

Henry: will add constant define.


And please do version the patches properly, it's getting messy to track which version is which (if something is new or not). If you use git format-patch, it has an argument that can be used to add the version, no need to add it by hand.

Henry: OK I will do patch version and change log for next submission. I did not realize this before. 

--
 i.