[lm-sensors] ADT7473 and ADT7475

[lm-sensors] ADT7473 and ADT7475

[Jean Delvare]

Hi Darrick,

Do you have a list of differences between the ADT7473 and ADT7475?
Looking briefly at the datasheets and drivers, I am under the
impression that they are essentially compatible and that a combined
driver for both chips would make a lot of sense. What do you think?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Jean Delvare]

Hi all,

On Sat, 7 Nov 2009 15:46:21 +0100, Jean Delvare wrote:
> Do you have a list of differences between the ADT7473 and ADT7475?
> Looking briefly at the datasheets and drivers, I am under the
> impression that they are essentially compatible and that a combined
> driver for both chips would make a lot of sense. What do you think?

The more I look at these datasheets, the more obvious is becomes that
we really want a single driver for both chips. It is pointless to have
35 kB of duplicated code.

Now there's the question of which driver to keep. I've gathered the
following evidences:

Difference between the ADT7473 and ADT7475 devices:

* The ADT7475 has a fixed I2C address (0x2e) while some ADT7473
  variants have a selectable address (0x2c, 0x2d or 0x2e).
* The ADT7475 has two additional configuration registers
  (Configuration 6 at 0x10 and Configuration 7 at 0x11), but the
  adt7475 driver doesn't make use of them.
* The ADT7473 has five additional registers (Temperature operating
  points at 0x33-0x35 and Dynamic Tmin control at 0x36 and 0x37)
  but the adt7473 driver doesn't make use of them.

Difference between the adt7473 and adt7475 drivers:

* The adt7473 driver is older (kernel 2.6.25, while the adt7475 driver
  was added in kernel 2.6.29) but the adt7475 driver is probably more
  popular.
* The adt7475 driver reads the extra resolution bits in registers 0x76
  and 0x77, while the adt7473 driver does not.
* The adt7475 driver has support for temperature offsets, critical
  limits and their hysteresis, and fault flag, and PWM frequency
  selection, while the adt7473 driver does not support any of these.
* The adt7473 implements the non-standard attribute
  pwm_use_point2_pwm_at_crit, the adt7475 driver doesn't support that.

I couldn't see any obvious difference between the chips for all the
registers which are used by the drivers. But maybe there are, only a
careful datasheet review would tell.

Bugs found in the drivers:

* The adt7475 driver creates pwm#_auto_channel_temp attributes instead
  of the standard pwm#_auto_channels_temp.
* The adt7475 driver refreshed limit values every 2 seconds even
  though the comment in the code says every 60 seconds. 60 seconds
  would make more sense.
* One coding style error in driver adt7473.

At the light of all this, I think I'd keep the adt7475 driver and merge
ADT7473 support therein. If anyone can think of good reasons to do it
the other way around, please speak up.

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Hans de Goede]

Hi,

On 11/08/2009 04:16 PM, Jean Delvare wrote:

<snip>

>
> At the light of all this, I think I'd keep the adt7475 driver and merge
> ADT7473 support therein. If anyone can think of good reasons to do it
> the other way around, please speak up.
>

Yes that sounds like the sensible thing to do to me.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Darrick J. Wong]

On Sun, Nov 08, 2009 at 04:16:13PM +0100, Jean Delvare wrote:

> At the light of all this, I think I'd keep the adt7475 driver and merge
> ADT7473 support therein. If anyone can think of good reasons to do it
> the other way around, please speak up.

Sounds like a good idea to me.

--D

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Jean Delvare]

Hi Darrick,

On Mon, 9 Nov 2009 10:09:00 -0800, Darrick J. Wong wrote:
> On Sun, Nov 08, 2009 at 04:16:13PM +0100, Jean Delvare wrote:
> 
> > At the light of all this, I think I'd keep the adt7475 driver and merge
> > ADT7473 support therein. If anyone can think of good reasons to do it
> > the other way around, please speak up.
> 
> Sounds like a good idea to me.

I'll do that then. When I'm done, I'll ask you to test the result, as I
presume you have access to a live ADT7473 chip.

What about attribute pwm_use_point2_pwm_at_crit? It doesn't sound
terribly useful to me, but OTOH you did implement it in the adt7473
driver, and writable at that, so maybe you had the need. Do you want me
to add this feature to the adt7475 driver?

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Darrick J. Wong]

On Fri, Nov 13, 2009 at 03:36:19PM +0100, Jean Delvare wrote:
> Hi Darrick,
> 
> On Mon, 9 Nov 2009 10:09:00 -0800, Darrick J. Wong wrote:
> > On Sun, Nov 08, 2009 at 04:16:13PM +0100, Jean Delvare wrote:
> > 
> > > At the light of all this, I think I'd keep the adt7475 driver and merge
> > > ADT7473 support therein. If anyone can think of good reasons to do it
> > > the other way around, please speak up.
> > 
> > Sounds like a good idea to me.
> 
> I'll do that then. When I'm done, I'll ask you to test the result, as I
> presume you have access to a live ADT7473 chip.
> 
> What about attribute pwm_use_point2_pwm_at_crit? It doesn't sound
> terribly useful to me, but OTOH you did implement it in the adt7473
> driver, and writable at that, so maybe you had the need. Do you want me
> to add this feature to the adt7475 driver?

Yes, please.  By the way, will there be any attempt to make
CONFIG_SENSORS_ADT7473 select CONFIG_SENSORS_ADT7475 when the transition is
done?

--D

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Jean Delvare]

On Fri, 13 Nov 2009 10:41:57 -0800, Darrick J. Wong wrote:
> On Fri, Nov 13, 2009 at 03:36:19PM +0100, Jean Delvare wrote:
> > Hi Darrick,
> > 
> > On Mon, 9 Nov 2009 10:09:00 -0800, Darrick J. Wong wrote:
> > > On Sun, Nov 08, 2009 at 04:16:13PM +0100, Jean Delvare wrote:
> > > 
> > > > At the light of all this, I think I'd keep the adt7475 driver and merge
> > > > ADT7473 support therein. If anyone can think of good reasons to do it
> > > > the other way around, please speak up.
> > > 
> > > Sounds like a good idea to me.
> > 
> > I'll do that then. When I'm done, I'll ask you to test the result, as I
> > presume you have access to a live ADT7473 chip.
> > 
> > What about attribute pwm_use_point2_pwm_at_crit? It doesn't sound
> > terribly useful to me, but OTOH you did implement it in the adt7473
> > driver, and writable at that, so maybe you had the need. Do you want me
> > to add this feature to the adt7475 driver?
> 
> Yes, please.

OK, will do.

> By the way, will there be any attempt to make
> CONFIG_SENSORS_ADT7473 select CONFIG_SENSORS_ADT7475 when the transition is
> done?

No, CONFIG_SENSORS_ADT7473 will simply be dropped.

What could be done is have CONFIG_SENSORS_ADT7473 select
CONFIG_SENSORS_ADT7475 _during_ the transition phase, if you think this
can be useful. I am a little reluctant to have Kconfig dependencies
with no technical justifications, but in this case I can't think of any
problem that could cause, so why not.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Darrick J. Wong]

On Fri, Nov 13, 2009 at 08:43:47PM +0100, Jean Delvare wrote:
> No, CONFIG_SENSORS_ADT7473 will simply be dropped.
> 
> What could be done is have CONFIG_SENSORS_ADT7473 select
> CONFIG_SENSORS_ADT7475 _during_ the transition phase, if you think this
> can be useful. I am a little reluctant to have Kconfig dependencies
> with no technical justifications, but in this case I can't think of any
> problem that could cause, so why not.

"The adt747[35] drivers were refactored into one driver" seems like a good
enough technical justification to me.

Then again since the drivers don't autoprobe and most everyone will have a
'modprobe adt7473' in some init script anyway, I suppose it won't really help.
So, it's probably not worth the extra effort.

--D

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] ADT7473 and ADT7475

[Jean Delvare]

On Mon, 16 Nov 2009 09:51:42 -0800, Darrick J. Wong wrote:
> On Fri, Nov 13, 2009 at 08:43:47PM +0100, Jean Delvare wrote:
> > No, CONFIG_SENSORS_ADT7473 will simply be dropped.
> > 
> > What could be done is have CONFIG_SENSORS_ADT7473 select
> > CONFIG_SENSORS_ADT7475 _during_ the transition phase, if you think this
> > can be useful. I am a little reluctant to have Kconfig dependencies
> > with no technical justifications, but in this case I can't think of any
> > problem that could cause, so why not.
> 
> "The adt747[35] drivers were refactored into one driver" seems like a good
> enough technical justification to me.
> 
> Then again since the drivers don't autoprobe and most everyone will have a

Actually, there will be some form of unintended auto-loading taking
place, exactly because of drivers being merged: loading the adt7473
driver on a system with a supported chip will instantiate an "adt7473"
driver. This in turn will trigger the loading of the adt7475 driver,
because this is where official adt7473 support is. In my tests, the
adt7473 driver is faster and the adt7475 driver gets loaded but isn't
used (for the ADT7473 at least). But I can imagine that in other cases
the adt7475 driver might pick the adt7473 first, so we are in the
unfortunate situation where we don't control exactly which driver picks
the device. This will stay that way until we finally delete the adt7473
driver. Hopefully both drivers will work the same so nobody will
notice ;)

This transition raises an interesting question about what should go in
initialization scripts to load i2c drivers. "modprobe $driver" is
vulnerable to consolidation as we're doing now. "modprobe i2c:$device"
would allow for smoother transitions, with the problem that 1* we would
need to teach sensors-detect about all device names and 2* a change in
the i2c alias format (which I can't exclude at this point) would break
all systems, so it's not bullet-proof either.

> 'modprobe adt7473' in some init script anyway, I suppose it won't really help.
> So, it's probably not worth the extra effort.

There's a log message that will be printed when using the deprecated
driver, and we will update sensors-detect to point to the adt7475
driver for ADT7473 support. I hope that will be good enough (and the
ADT7473 doesn't seem to be overly popular either.)

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors