> From: Jean Delvare [mailto:jdelvare@xxxxxxx] ... > Sorry, this happened while I was on vacation and I missed it. > > The change looks sane, so: > > Reviewed-by: Jean Delvare <jdelvare@xxxxxxx> > > And I approve backporting to older kernels if needed. > > That being said... Looking at the code, I have to say I'm surprised that the whole > value of configuration byte 0xe1 is used as true/false. > I'd expect a single bit (bit 0) in this configuration file to decide if the P2SB device > is hidden or visible. So it seems to me that we are trashing 7 bits in the process. > These bits may be unused at the moment, but could be used in future devices, > and then unexpected behavior will appear. > > I'm still approving this patch because the same problem was present before, but > I think this should be checked, and fixed if I'm right. > I'll be happy to do it myself if someone at Intel points me to the Denverton > datasheet - if it is public. Hi Jean, Yes, bit[0] is to decide the visibility of P2SB and bits[1-7] are reserved. The internal datasheet (I didn't find the public spec) says that hardware will not allocate the reserved bits[1-7] for any purpose in the future. And it can always write to bit[0] without to remember any other field values. But I agree you that using a single bit[0] would be better :-) Thanks! - Qiuxu
