Hi David,
On Sat, 3 May 2008 08:20:41 -0700, David Brownell wrote:
> On Saturday 03 May 2008, Jean Delvare wrote:
> > > +enum lm75_type { /* keep sorted in alphabetical order */
> > > + ds1775,
> > > + ds75,
> > > + LM75, /* UPPERCASE prevents INSMOD_1 conflict(!) */
> > > + lm75a,
> > > + max6625,
> > > + max6626,
> > > + mcp980x,
> > > + stds75,
> > > + tcn75,
> > > + tmp100,
> > > + tmp101,
> > > + tmp175,
> > > + tmp275,
> > > + tmp75,
> > > +};
> >
> > Do you really need that many different types? I though that some of
> > those parts were 100% compatible, at least as far as this driver is
> > concerned. The more different types, the more tests you'll have to do
> > at initialization time or even at run time.
>
> ISTR most of them handle 12-bit encodings. And it turns
> out that the probe() code has most need to know about the
> differences: it's what will set up the state used by the rest
> of the driver. (Albeit not in this particular patch...)
>
> Re compatibility, I think explicit listings inside driver
> code lead to a lot less confusion and time wasted on writing
> needless drivers; and it also avoids wondering "is it *really*
> compatible?". Plus it's easier to trust board init code that
> matches the schematics (and other board docs).
I didn't mean to object to having that many device names, only that many
different enum values. When 2 parts are 100% compatible, I think it
makes sense to have the two names map to the same enum value in
lm75_ids, so that later code can only check for one enum value instead
of 2.
But well it's up to you, that was only a suggestion, I don't care that
much.
> > > - u16 temp[3]; /* Register values,
> > > + s16 temp[3]; /* Register values,
> >
> > This too doesn't seem to belong to this patch any longer.
>
> First time you commented on that, and that bugfix has been
> in this patch for ages. :)
Up to the previous version, the patch included min/max stuff and
wrappers for the conversions, so it made some sense to include this
change along. Now it no longer does.
> > > + { "tmp75", tmp75, },
> > > + { /* LIST END */ },
> >
> > Trailing comma not needed - you can't grow the list beyond its end.
>
> Doesn't hurt either.
Well, I think it does. A trailing comma is an invitation to add new
lines after it. The big fat comment you inserted somewhat mitigates
this, but still.
> I think adding new chip support to legacy driver side
> should be avoided. ;)
So do I, but as long as the new-style drivers do not offer a way to
forcibly instantiate a device from user-space, we might have to do that.
--
Jean Delvare
