Even if it goes to HEAD it will break older binaries compiled against 9.x, etc. if they are run on an 11+ system. That said, I'm not sure this is a widely used API either. I would perhaps post this specific question on arch@ to ask if it's ok to break the ABI without providing a wrapper in this case. At least since you have changed the structure size it is relatively easy to add wrappers in the future if we find that we need them.

Very nice. I think the if addr check is fine for location_str.

I would probably go ahead and use a switch here to match other bus drivers (even though there is only 1 ivar currently). Perhaps something like:

addr = device_get_ivars(child);
if (addr == NULL)
    return (ENOENT);
switch (which) {
...

OTOH, you might also consider returning -1 for the addr for devices without a valid addr. (and make the ACCESSOR return an int.) Drivers could then do this in probe methods:

if (smbus_get_addr(dev) < 0)
    /* Not an enumerated device. */
    return (ENXIO);

Similarly, smbus.c's probe should probably be changed to do something like:

if (smbus_get_addr(dev) >= 0)
   /* Ignore enumerated devices. */
   return (ENXIO);

(Of course, I'd argue that /dev/smbX should be something that the smbus driver provides directly instead of having a fake device_t child to do it. /dev/pci isn't provided by a fake PCI device, it's handled by the bus driver directly. Then you would always have a valid smbus_get_addr()).

I think SMBUS_IVAR_ADDR is probably more consistent with other drivers?

Also, you should possibly add a __BUS_ACCESSOR() so drivers can use 'smbus_get_addr()'.

I did what you suggested and made smbus_get_addr return -1 (using a slightly different code structure). While I agree that it would be nicer if smbus could provide smb itself, I think that it's a bit too much for this patch.

SMBUS_IVAR_ADDR was already defined in smbconf.h, so I removed the enum from smbus.h and turned the define in smbconf.h into an enum. I also added __BUS_ACCESSOR for addr.

Be sure to update this just before you commit. At least try to remember. People notice when this isn't within a couple of days and may send you email :)

So is the limit 32 or 1024? Or is 32 the limit before you need an external buffer?

Not sure this comment adds much value.

mixing hex and decimal like this makes my brain hurt. Please use hex for both.

Don't you need an error != 0 here too?

ditto?

ditto?

why not error = EINVAL; ?

If you are ditching compat you can kill this. If not, then you need a lot more stuff like this...

Why int for a count? Also, I'd use the SMB_MAXBLOCKSIZE. However, here it says 255 is the max, while below it tests for 1024. Which is it? ditto below for rcount.

Note: All the IOCTLs change number when you change the size of smbcmd like you have. That's likely OK, but I wanted to make sure this isn't an interface we need to have a compat shim for...

I'll try :)

It's 32 byte for SMB 2.0, but 1024 for I2C transactions.

I removed it.

It does.

That's been ported as is. I have to agree though, so I'll add:

if (error)
    break;

to match the style used in smb.c (instead of "if (!error && ...))

ditto.

ditto.

We already discussed that in this code review.
https://reviews.freebsd.org/D1955?id=4055#inline-11733

good point.

See above for 32 vs 1024 byte.
We could also make it unsigned, but I wouldn't do that without a compelling reason.

Not sure about this comment, I blindly ported that one. I'll remove the comment.

This should change to not blindly attach (this was one of the things I mentioned earlier to remove the need for the custom smbus_add_child()). There are a few options you can use.

1. You can fail attach if smbus_get_addr(dev) is not -1.

2. You can return BUS_PROBE_NOWILDCARD so that it will not attach to devices unless they have hardcoded the "smb" name.

I would probably do both, so:

smb_probe(device_t dev)
{

    if (smbus_get_addr(dev) != -1)
        return (ENXIO);

    device_set_desc(dev, "SMBus generic I/O");
    return (BUS_PROBE_NOWILDCARD);
}

The other part of this change is that any other drivers like isl or cyapa should _not_ have an identify routine (just like PCI drivers don't create devices, they probe to devices created by the bus). They should also require a valid address by failing to probe a device if smbus_get_addr() is -1, so:

cyapa_probe(dev)
{
    ...
    if (smbus_get_addr(dev) == -1)
        return (ENXIO);
    ...
    /* Current probe routine */
}

Yes, this is definitely correct. I had thought this was done in an identify routine in the 'smb' driver instead.

Odd, smb has an identify routine as well. This just gets weirder the more I look at this. :)

Can you try replacing this explicit 'device_add_child()' with a call to 'bus_generic_probe()'? That will actually call smb_identify() which is currently unused.

Hmm, thinking some more, it may be that the identify routine gets used when you kldload smb after smbus. Using bus_generic_probe() is more correct then as it makes things more consistent so that the device is always added by the same bit of code. If that works, please use that instead of the explicit device_add_child().

Please start new sentences on a new line in manpages. This makes it easier for translators. You might also want to add Warren Block on this to review the manpage changes for you.

You can probably drop this blank line, but that's a very minor nit.

Perhaps move this under bootverbose?

If you want to list the found devices that don't have an attached driver during boot, then you might consider having a no_match method to do this. You can look at the PCI bus driver in sys/dev/pci/pci.c for an example implementation.

Replace "i/o" with "I/O".

"In order to" is usually better replaced with just "To".

"to talk to" is ugly. Is it needed?

This can be rearranged:

command does not transfer any data.
It just issues the device address with write intent to the bus.

As above:

command does not transfer any data.
It just issues the device address with read intent to the bus.

"The SendByte command" is redundant and can be just

.Em SendByte
sends the byte provided in the

These can be done with the other commands also. The only time to point out whether it is a command or field or something is when that is not clear from the context.

"the cmd field" is somewhat redundant, could be just

...provided in
.Fa cmd
to the device.

As above:

be returned in
.Fa cmd .

"The" is not needed. Also, use active rather than passive: will be -> is.

Returned data is stored in

"and" is not needed.

As above:

Returned data is stored in

No comma after "device".

Does "available" mean it can be read from that constant, or that is the constant to modify if needed?

No "and".

"roll-up" should probably be written as two words. Also, break the line for a new sentence.

The comma goes at the end of the macro on the previous line, or the formatting might not be handled correctly. Note that there is a space before it. Also, "and" is not needed:

.Fa wbuf ,
then reads

Shouldn't SMBUS_ADDR_MAX be 0xf0 if we are using FreeBSD's conventional 8-bit slave addresses?

This looks like it does an equivalent of smbus_readb(dev, slave, 0x01, ...), but only for drivers that implement smbus_trans.
I wonder why readb and not, say, recvb.
Also, why 0x01? 0x00 is even more magic...

The slave address is incremented by 1 in this loop.
So, are we using 7-bit formatted slave addresses here?
Is smbus_trans expected to take such addresses?
Because I thought that the FreeBSD convention is to use 8-bit formatted addresses (with the lsb always being zero).
Yes, smb(4) says this:

The slave address is specified in the seven most
significant bits (i.e., “left-justified”). The least significant bit of
the slave address must be zero.