I reply in this mail both to max663x quetions and lm92 driver.
On Tue, 2004-04-13 at 14:17, Jean Delvare wrote:
> > > I don't say that you should skip the test. Just don't make it an
> > > error, i.e. exit in silence.
> > > (...)
> >
> > Ok, I have removed this error path, only warning.
>
> No! Read what I just said. Do not ignore the fact that functionalities
> are missing, since it'll break later then. But do not return an error
> code and message either, because it isn't necessarily an error (due to
> the fact that each chip driver is possibly called for each bus on the
> system, and a given bus does not have to support functionalities that
> are used by chips on the other busses). So the correct code would be:
>
> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> I2C_FUNC_SMBUS_WORD_DATA))
> {
> printk(KERN_DEBUG "max6635.o: Adapter doesn't support needed
> functionalities.\n");
> return 0;
> }
Point.
I've seen the i2c core source and understand your point, thank you.
> BTW it's a common habit to prefix error/debug messages with the module
> name, as I did above. I just noticed that you didn't follow it in your
> driver.
>
> > > Will you be able to run sensors-detect to test my detection
> > > function on your samples?
> >
> > Nope. It doesn't have any kind of scripting languages.
>
> OK, I guess I won't add support to sensors-detect at all then.
>
> BTW, what is your system after all? I'm quite surprised to see a
> MAX6635
> in a modern system, it looks like a rather "old" chip (designed 3 years
> ago). The level of support we want to add to our userspace tools
> (sensors-detect, libsensors, sensors) mainly depends on how likely it
> is that other users will have such a device on a system them just
> bought. So far the LM76 was listed as "unlikely to be found in a PC".
It is board based on ppc405gp for duplex/simplex 8/16 audio channel
processing. I don't think it is widely used ex?ept special cases...
> > 00: 0004 0005 0002 0003 <- registers
> >
> > 00: 000f 0014 000a 0005
> > 08: 0000 0000 0000 0000
> > 10: 000f 0014 000a 0005
> > 18: 0000 0000 0000 0000
> > 20: 000f 0014 000a 0005
> > 28: 0000 0000 0000 0000
> > 30: 000f 0014 000a 0005
> > 38: 0000 0000 0000 0000
> > 40: 000f 0014 000a 0005
> > 48: 0000 0000 0000 0000
> > 50: 000f 0014 000a 0005
> > 58: 0000 0000 0000 0000
> > 60: 000f 0014 000a 0005
> > 68: 0000 0000 0000 0000
> > 70: 000f 0014 000a 0005
> > 78: 0000 0000 0000 0000
> > 80: 0000 0000 0000 0000
> > 88: 0000 0000 0000 0000
> > 90: 0000 0000 0000 0000
> > 98: 0000 0000 0000 0000
> > a0: 0000 0000 0000 0000
> > a8: 0000 0000 0000 0000
> > b0: 0000 0000 0000 0000
> > b8: 0000 0000 0000 0000
> > c0: 0000 0000 0000 0000
> > c8: 0000 0000 0000 0000
> > d0: 0000 0000 0000 0000
> > d8: 0000 0000 0000 0000
> > e0: 0000 0000 0000 0000
> > e8: 0000 0000 0000 0000
> > f0: 0000 0000 0000 0000
> > f8: 0000 0000 0000 0000
>
> Which would tend to prove that the pointer register has actually 4 used
> bits. Or even 5 if we consider the second half of the dump. That's a
> bit surprising, I admit. Still this is enough to improve the chip's
> detection function.
Done.
> > > You don't need swap_bytes here, if would be faster without it,
> > > don't you think?
> >
> > It is called only once in load time, and main idea was to store this
> > values as cache if they are valid. I think it should be implemented
> > in this way, or not?
>
> We usually do not, although I admit we could. Still, it only makes sense
> if you read *all* the values so that caching is possible. About this, I
> noticed that you don't properly protect your update function. See in
> other chip drivers how this is done. You are supposed to cache the read
> data for a given amount of time, and there's also a flag that says
> whether the values are OK or not (basically clear at driver load time,
> and set as soon as the update was called once).
Yep. I've created caching.
I lock any data update, so data in max6635 private structure always
correct no matter who is reading it or only part of it.
I undestand your point of view, but it was intended to not locking
reading.
But now I think it is better to lock any access.
> If you want to fill the cache as soon as you start the driver, then you
> should simply call the update function and use the "returned" values
> for detection. Else you're basically coding the same function twice.
>
> Oh BTW, you don't seem to use the 4-bit address cycle trick I mentioned
> before in your detection function yet. Please do.
All is done.
> And two more comments:
>
> You can use the kernel function swab16() instead of your own swap_bytes
> function. I'm going to fix the othe drivers in the same way.
>
> > -MODULE_DESCRIPTION("MAX663x temerature sensor");
> > +MODULE_DESCRIPTION("MAX663{5,4,3} temerature sensor");
>
> This is actually the only place where 'x' is accepted, I forgot to tell
> you ;)
:)
> Looks like the LM76 is 100% compatible with the LM92, and we have a
> driver for the LM92 (lm92.c). Since the MAX6635 is obviously a clone of
> the LM76, it should fit well in the lm92 driver as well.
>
> I admit that the lm92 driver isn't our best driver, there are several
> places where it does the things in a very different way when compared
> with the other drivers. Still it would be much better to extend and fix
> this existing driver that to duplicate the exact same functionality in
> a different driver.
>
> Since the LM92 has a manufacturer ID register, you can use that to
> distinguish it from Maxim's clones. Also, I noticed that the five chips
Reading "manufacturer" register from MAX663x here shows "0x0014".
I don't know if this value may change.
> (LM76, LM92, MAX6633, MAX6634 and MAX6635) have different address
> ranges. That's something you can use to distinguish between the four of
It doesn't help very much - all of them may have 0x48 address for example.
But it is something...
> them that do not have a manufacturer's ID register (although it will of
> course not be perfect). You would also add the 4-bit cycle trick as
> mentioned in an earlier, and the unused bits in the temperature limits
> registers. The lm92.c driver has a very simple detection function at
> the moment because it could rely on the manufacturer's ID. Now that you
> can't, you have to use all the tricks we've discussed before.
I can just copy max663x detection path into lm92.c and turn it on when
manufacturer id doesn't correspond needed one for lm92.
> You are welcome to clean whatever you fell like cleaning in the lm92.c
> driver, I don't like it much (although it's definitely better than no
> driver at all, I don't intend to denigrate Abraham's work, it's just
> that it is different from the other drivers, which makes it harder to
> maintain).
--
Evgeniy Polaykov ( s0mbre )
Crash is better than data corruption. -- Art Grabowski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: max6635.c.diff.1
Type: text/x-troff-man
Size: 5661 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040413/23e92c66/attachment.bin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20040413/23e92c66/attachment-0001.bin
