Generally, this looks good to my eye. I think the masks are overkill, but that comes from my experience in pci land where they were over-used and under-needed.

sys/dev/jedec_ts/jedec_ts.c
57	Since the vendor mask is always 0xffff, why have it? It only seems useful for the catch-all at the end...
59	device_mask is likewise always 0xff00. Looks like the lower 8 bits are supposed to be a revision number. I see why you want to mask it, but realistically, will there ever be a time we'd have two different entries here?

Can you point to a copy (or even a recent draft?) of the TSE2004av spec for reference?

In D11730#243308, @rpokala wrote:

Can you point to a copy (or even a recent draft?) of the TSE2004av spec for reference?

It's here https://www.jedec.org/category/keywords/tse2004av
Registration is required, but it is hassle free.

rpokala added inline comments.Jul 26 2017, 5:15 PM

sys/dev/jedec_ts/jedec_ts.c
59	JEDEC Standard No. 21-C 2.24. TSE2004av Device ID / Revision Register The upper byte of the Device ID / Revision Register must be 0x22 for the TSE2004av. The lower byte holds the revision value which is vendor-specific. This register is not available on EE1004-v devices. Based on that, we should be looking at all four nybbles, which should be 0x22XX. But as pointed out in the comment, not everyone uses the correct device ID. Since it's the low-order bits which are vendor-specific, shouldn't we only look at those for identification? (But maybe the vendor datasheets indicate that the low-order bits might be different even for the same part? I haven't looked at them yet; I'll do that later today. If that's the case, then I'd say we shouldn't look at the device-id at all.)

rpokala added a reviewer: rpokala.Jul 26 2017, 5:15 PM

avg added inline comments.Jul 27 2017, 11:33 AM

sys/dev/jedec_ts/jedec_ts.c
59	The low bits are "volatile". Most vendor specifications say something to the effect that the revision is X in this version of the product and will be incremented in subsequent versions. Anyway, this is mostly moot as of all the devices out there I found only two that have 0x22 in the upper half.

the vendor and device ID masks were not that useful, in fact

Harbormaster completed remote builds in B10697: Diff 31252.Jul 27 2017, 11:39 AM

@imp I removed the masks as they were not useful indeed and added only noise.

One thing that can be considered a regression in the proposed code is that now device_set_desc() is called from the attach method, not from the probe method as before.
That's fine in general, but the messages printed just before DEVICE_ATTACH do not contain any description now.

That is, before the change I had:

jedec_ts0: <DIMM memory sensor> at addr 0x30 on smbus0

and now I have:

jedec_ts0 at addr 0x30 on smbus0

sysctl reports the description, of course:

dev.jedec_ts.0.%desc: Atmel DIMM temperature sensor

In D11730#243574, @avg wrote:

One thing that can be considered a regression in the proposed code is that now device_set_desc() is called from the attach method, not from the probe method as before.
That's fine in general, but the messages printed just before DEVICE_ATTACH do not contain any description now.

Shouldn't the matching be done in probe anyway? If there's no match, return ENXIO, and attach won't be called. That's the way man 9 DEVICE_PROBE and man 9 DEVICE_ATTACH say it should work anyway.

sys/dev/jedec_ts/jedec_ts.c
54	Add a note saying that the spec requires the upper byte of the device_id be 0x22, but that most vendors don't honor that, so that's why we're not checking for it. That makes the 0x22 in the generic case a bit less magic.

In D11730#243634, @rpokala wrote:

Shouldn't the matching be done in probe anyway? If there's no match, return ENXIO, and attach won't be called. That's the way man 9 DEVICE_PROBE and man 9 DEVICE_ATTACH say it should work anyway.

There is something special about i2c devices.
I recall @imp specifically advising me that any access to the device should be done in attach, not in probe.

sys/dev/jedec_ts/jedec_ts.c
54	Isn't this comment the comment that you are requesting?

I recall @imp specifically advising me that any access to the device should be done in attach, not in probe.

Fascinating. For my education, could you explain why, @imp?

sys/dev/jedec_ts/jedec_ts.c
54	Err, yes - partly. It mentions that few use the proper one. How about: ... devices should use (high-order byte 0x22), but very few do in practice ...

In D11730#243975, @rpokala wrote:

I recall @imp specifically advising me that any access to the device should be done in attach, not in probe.

Fascinating. For my education, could you explain why, @imp?

Because writing to the IIC bus can cause a write cycle to EEROMs.

sys/dev/jedec_ts/jedec_ts.c
59	Right, hence my comment: we should just have a blanket policy of ignoring them and have no need for it in this table.

In D11730#244159, @imp wrote:

In D11730#243975, @rpokala wrote:

I recall @imp specifically advising me that any access to the device should be done in attach, not in probe.

Fascinating. For my education, could you explain why, @imp?

Because writing to the IIC bus can cause a write cycle to EEROMs.

I know that all too well... <thousand-yard-stare as he remembers SMBus battles past and present>

But, I don't see how it would make a difference in this case - it would be the same operations as it's currently doing in ts_attach(), just in ts_probe(). And since ts_probe() would only be run against those devices specified by the admin via /boot/device.hints, I don't see why there is any additional danger doing it earlier.

What am I missing?

In D11730#244161, @rpokala wrote:

In D11730#244159, @imp wrote:

In D11730#243975, @rpokala wrote:

I recall @imp specifically advising me that any access to the device should be done in attach, not in probe.

Fascinating. For my education, could you explain why, @imp?

Because writing to the IIC bus can cause a write cycle to EEROMs.

I know that all too well... <thousand-yard-stare as he remembers SMBus battles past and present>

But, I don't see how it would make a difference in this case - it would be the same operations as it's currently doing in ts_attach(), just in ts_probe(). And since ts_probe() would only be run against those devices specified by the admin via /boot/device.hints, I don't see why there is any additional danger doing it earlier.

What am I missing?

So long as it isn't automatic, but done to a specific address by request in a config file that's not there by default, then I think you may be right. It isn't horrible to do.

In D11730#244186, @imp wrote:

So long as it isn't automatic, but done to a specific address by request in a config file that's not there by default, then I think you may be right. It isn't horrible to do.

Okay then. Since no automatic enumeration is involved, I return to my original suggestion: move type determination and reporting to ts_probe(), and just have ts_attach() create the sysctls.

As far as I know we never had an automatic enumeration for i2c and smbus devices, at least on x86.
So, not sure what changed now... :-)
BTW, here is the original comment:
https://reviews.freebsd.org/D8174?id=21137#inline-49103

sys/dev/jedec_ts/jedec_ts.c
54	I think that this comment is fine as is. There is another comment in the code where the magic device ID is actually used.
59	I removed the generic device ID from the table... I added it as a special case to `ts_match_device` though.

move hardware probing to the device probe method

Looks good to me. Thanks for the changes.

This revision is now accepted and ready to land.Sep 12 2017, 9:46 PM

Closed by commit rS323540: jedec_ts: add many more devices from various vendors (authored by avg). · Explain WhySep 13 2017, 1:03 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Path

Size

sys/

dev/

jedec_ts/

jedec_ts.c

138 lines

Diff 31252

View Options

sys/dev/jedec_ts/jedec_ts.c

jedec_ts: add many more devices from various vendorsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31252

sys/dev/jedec_ts/jedec_ts.c

jedec_ts: add many more devices from various vendors
ClosedPublic
Actions

Revision Contents
Changeset List