Changeset View
Standalone View
sys/dev/imcsmb/imcsmb.c
Property | Old Value | New Value |
---|---|---|
svn:eol-style | null | native \ No newline at end of property |
svn:keywords | null | FreeBSD=%H \ No newline at end of property |
svn:mime-type | null | text/plain \ No newline at end of property |
/*- | |||||
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD | |||||
* | |||||
* Authors: Joe Kloss; Ravi Pokala (rpokala@freebsd.org) | |||||
* | |||||
* Copyright (c) 2017-2018 Panasas | |||||
* All rights reserved. | |||||
* | |||||
* Redistribution and use in source and binary forms, with or without | |||||
* modification, are permitted provided that the following conditions | |||||
* are met: | |||||
* 1. Redistributions of source code must retain the above copyright | |||||
* notice, this list of conditions and the following disclaimer. | |||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||
* notice, this list of conditions and the following disclaimer in the | |||||
* documentation and/or other materials provided with the distribution. | |||||
* | |||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||
* SUCH DAMAGE. | |||||
*/ | |||||
/* A detailed description of this device is present in imcsmb_pci.c */ | |||||
#include <sys/param.h> | |||||
#include <sys/systm.h> | |||||
#include <sys/kernel.h> | |||||
#include <sys/module.h> | |||||
#include <sys/endian.h> | |||||
#include <sys/errno.h> | |||||
#include <sys/lock.h> | |||||
#include <sys/mutex.h> | |||||
#include <sys/syslog.h> | |||||
#include <sys/bus.h> | |||||
#include <machine/bus.h> | |||||
#include <machine/atomic.h> | |||||
#include <dev/pci/pcivar.h> | |||||
#include <dev/pci/pcireg.h> | |||||
#include <dev/smbus/smbconf.h> | |||||
#include "imcsmb_reg.h" | |||||
#include "imcsmb_var.h" | |||||
/* Device methods */ | |||||
static int imcsmb_attach(device_t dev); | |||||
static int imcsmb_detach(device_t dev); | |||||
avg: Looks like these declarations of static functions are not really required. | |||||
Done Inline ActionsI was taught to always declare functions, even static ones, near the top of the file. It might not strictly be required, but it's not prohibited either, and I find it useful. rpokala: I was taught to always declare functions, even static ones, near the top of the file. It might… | |||||
Done Inline ActionsI am certainly not asking for any change here. avg: I am certainly not asking for any change here.
Just sharing my line of reasoning.
These… | |||||
Done Inline ActionsWhen skimming through files, it's handy to see up front what's in the file. rpokala: When skimming through files, it's handy to see up front what's in the file. | |||||
static int imcsmb_probe(device_t dev); | |||||
/* SMBus methods */ | |||||
static int imcsmb_callback(device_t dev, int index, void *data); | |||||
static int imcsmb_readb(device_t dev, u_char slave, char cmd, char *byte); | |||||
static int imcsmb_readw(device_t dev, u_char slave, char cmd, short *word); | |||||
static int imcsmb_writeb(device_t dev, u_char slave, char cmd, char byte); | |||||
static int imcsmb_writew(device_t dev, u_char slave, char cmd, short word); | |||||
/* All the read/write methods wrap around this. */ | |||||
static int imcsmb_transfer(device_t dev, u_char slave, char cmd, void *data, | |||||
int word_op, int write_op); | |||||
/** | |||||
* device_attach() method. Set up the softc, including getting the set of the | |||||
* parent imcsmb_pci's registers that we will use. Create the smbus(4) device, | |||||
* which any SMBus slave device drivers will connect to. Probe and attach | |||||
* anything which might be downstream. | |||||
* | |||||
* @author rpokala | |||||
* | |||||
* @param[in,out] dev | |||||
* Device being attached. | |||||
*/ | |||||
static int | |||||
imcsmb_attach(device_t dev) | |||||
{ | |||||
struct imcsmb_softc *sc; | |||||
int rc; | |||||
/* Initialize private state */ | |||||
sc = device_get_softc(dev); | |||||
bzero(sc, sizeof(*sc)); | |||||
sc->dev = dev; | |||||
sc->imcsmb_pci = device_get_parent(dev); | |||||
sc->regs = device_get_ivars(dev); | |||||
/* Create the smbus child */ | |||||
sc->smbus = device_add_child(dev, "smbus", -1); | |||||
if (sc->smbus == NULL) { | |||||
/* Nothing has been allocated, so there's no cleanup. */ | |||||
device_printf(dev, "no \"%s\" child found\n", "smbus"); | |||||
rc = ENXIO; | |||||
goto out; | |||||
} | |||||
/* Allow other drivers to find things we don't know about */ | |||||
(void) bus_generic_probe(dev); | |||||
Done Inline ActionsWhat might those things be?.. It appears that imcsmb explicitly adds a single child and does not provide for any other child devices. avg: What might those things be?..
It appears that imcsmb explicitly adds a single child and does… | |||||
Done Inline ActionsI admit that the bus_generic_probe() / bus_generic_attach() calls are cargo-culted from somewhere. I'll change that to device_probe_and_attach(). rpokala: I admit that the `bus_generic_probe()` / `bus_generic_attach()` calls are cargo-culted from… | |||||
Done Inline ActionsNow that I think about it, imcsmb can only directly have a single smbus child, which is what slave devices like jedec_dimm(4) actually connect to. Similarly, imcsmb_pci can only directly have a pair of imcsmb children. So I'm getting rid of all the generic probe and attach stuff for both. rpokala: Now that I think about it, `imcsmb` can only directly have a single `smbus` child, which is… | |||||
Done Inline ActionsHave you tested this? avg: Have you tested this?
I think that `device_probe_and_attach` is still required.
I think that… | |||||
Done Inline ActionsHmmm... I did test, and it did work for me, but that might have been because smbus(4) was already loaded and had already parsed the hints. I'll double-check. rpokala: Hmmm... I did test, and it did work for me, but that might have been because `smbus(4)` was… | |||||
Done Inline ActionsDiscussed with @avg and @jhb in email; bus_generic_attach() is needed for the child's probe() and attach() to be called when the module is preloaded via loader.conf. When the driver is loaded as a module, the module event handler (or the generic one, if one isn't provided to DRIVER_MODULE) ends up causing the children to be probed and attached. rpokala: Discussed with @avg and @jhb in email; `bus_generic_attach()` is needed for the child's `probe… | |||||
/* Find drivers for identified devices */ | |||||
if ((rc = bus_generic_attach(dev)) != 0) { | |||||
device_printf(dev, "failed to attach child: %d\n", rc); | |||||
goto out; | |||||
} | |||||
out: | |||||
return (rc); | |||||
} | |||||
/** | |||||
* device_detach() method. attach() didn't do any allocations, so all that's | |||||
* needed here is to free up any downstream drivers and children. | |||||
* | |||||
* @author Joe Kloss | |||||
* | |||||
* @param[in] dev | |||||
* Device being detached. | |||||
*/ | |||||
Done Inline ActionsGiven you use bus_generic_detach() here (which is fine), I would just use 'bus_generic_attach()' at the end of attach above. jhb: Given you use bus_generic_detach() here (which is fine), I would just use 'bus_generic_attach… | |||||
static int | |||||
imcsmb_detach(device_t dev) | |||||
{ | |||||
int rc; | |||||
/* Detach any attached drivers */ | |||||
rc = bus_generic_detach(dev); | |||||
if (rc) { | |||||
goto out; | |||||
Done Inline Actions
avg: - redundant braces
- better to use `rc != 0`
- no need for `goto` in so simple code | |||||
Done Inline ActionsThe braces are again a matter of personal style, and are allowed by style(9). I find it much safer to include them, because I've seen so many cases where their lack has lead to mistakes. Good point about (rc != 0); that's my usual idiom, so I'll shamelessly blame my (departed) co-author for this one. ;-) As far as the goto, it's another one of my rules-of-thumb: functions have one entry-point and one exit-point, and do any necessary cleanup on the way out. But you're right, it's a bit overkill in this case. I'll reverse the overall logic to test for (rc == 0) and only call device_delete_children() in that case. rpokala: The braces are again a matter of personal style, and are allowed by style(9). I find it much… | |||||
Done Inline ActionsRegarding style(9), well, I know. It is a very recent change. I am certainly not insisting on any change, just sharing my reasoning. In this case the function was very simple, the condition was trivial and the conditional code was trivial as well. In general, wherever the prescribed style leaves room for different flavours I always ask myself which variant would have more benefit rather than following a single dogmatic rule. avg: Regarding style(9), well, I know. It is a very recent change. I am certainly not insisting on… | |||||
Done Inline ActionsSure. In my experience, having the braces is more beneficial than being a few lines shorter. :-) rpokala: Sure. In my experience, having the braces is more beneficial than being a few lines shorter. :-) | |||||
} | |||||
/* Remove all children */ | |||||
rc = device_delete_children(dev); | |||||
out: | |||||
return (rc); | |||||
} | |||||
/** | |||||
Not Done Inline ActionsNote that in most drivers, 'probe' is listed in the source before attach and detach and (also in the DEVMETHODS list) so that the code order matches the device life cycle. jhb: Note that in most drivers, 'probe' is listed in the source before attach and detach and (also… | |||||
Not Done Inline ActionsI'd prefer to keep it alphabetical, again for ease of skimming and searching. rpokala: I'd prefer to keep it alphabetical, again for ease of skimming and searching. | |||||
* device_probe() method. All the actual probing was done by the imcsmb_pci | |||||
* parent, so just report success. | |||||
* | |||||
* @author Joe Kloss | |||||
* | |||||
* @param[in,out] dev | |||||
* Device being probed. | |||||
*/ | |||||
static int | |||||
imcsmb_probe(device_t dev) | |||||
{ | |||||
device_set_desc(dev, "iMC SMBus controller"); | |||||
return (BUS_PROBE_DEFAULT); | |||||
Done Inline Actionslet's save a byte and a nit of vertical space here avg: let's save a byte and a nit of vertical space here | |||||
} | |||||
/** | |||||
* smbus_callback() method. Call the parent imcsmb_pci's request or release | |||||
* function to quiesce / restart firmware tasks which might use the SMBus. | |||||
* | |||||
* @author rpokala | |||||
* | |||||
* @param[in] dev | |||||
* Device being requested or released. | |||||
* | |||||
* @param[in] index | |||||
* Either SMB_REQUEST_BUS or SMB_RELEASE_BUS. | |||||
* | |||||
* @param[in] data | |||||
* Tell's the rest of the SMBus subsystem to allow or disallow waiting; | |||||
* this driver only works with SMB_DONTWAIT. | |||||
*/ | |||||
static int | |||||
imcsmb_callback(device_t dev, int index, void *data) | |||||
{ | |||||
struct imcsmb_softc *sc; | |||||
int *how; | |||||
int rc; | |||||
sc = device_get_softc(dev); | |||||
how = (int *) data; | |||||
switch (index) { | |||||
case SMB_REQUEST_BUS: { | |||||
if (*how != SMB_DONTWAIT) { | |||||
rc = EINVAL; | |||||
Done Inline ActionsWhy this requirement? avg: Why this requirement?
It does not appear to be justified. | |||||
Done Inline ActionsThis part definitely comes from my co-author. The issue is that we sometimes need to use this driver while panicking, and mutexes aren't allowed in that state. Since waiting requires a sleepable lock, he felt it better to just restrict this driver to not allowing waiting. rpokala: This part definitely comes from my co-author. The issue is that we sometimes need to use this… | |||||
Done Inline ActionsWell, you can make a special case for panicstr != NULL && (how & SMB_WAIT) != 0. avg: Well, you can make a special case for `panicstr != NULL && (how & SMB_WAIT) != 0`.
BTW, all… | |||||
Done Inline ActionsThe more I think about this, the less I want to change it at this juncture; it would require a whole bunch of re-testing with our (proprietary <sigh>) NVDIMM driver. Would you be okay with me leaving this as-is for now, with a promise to take a closer look at this idea in the near future? rpokala: The more I think about this, the less I want to change it at this juncture; it would require a… | |||||
Done Inline ActionsSure, no problem. avg: Sure, no problem. | |||||
goto out; | |||||
} | |||||
rc = imcsmb_pci_request_bus(sc->imcsmb_pci); | |||||
break; | |||||
} | |||||
case SMB_RELEASE_BUS: | |||||
imcsmb_pci_release_bus(sc->imcsmb_pci); | |||||
rc = 0; | |||||
break; | |||||
default: | |||||
rc = EINVAL; | |||||
break; | |||||
} | |||||
out: | |||||
return (rc); | |||||
} | |||||
/** | |||||
* smbus_readb() method. Thin wrapper around imcsmb_transfer(). | |||||
* | |||||
* @author Joe Kloss | |||||
* | |||||
* @param[in] dev | |||||
* | |||||
* @param[in] slave | |||||
* The SMBus address of the target device. | |||||
* | |||||
Done Inline Actionsstyle(9) would put ()'s around the call to icmsmb_transfer() and would also put a blank line before the return (same in the other one liners below). jhb: style(9) would put ()'s around the call to icmsmb_transfer() and would also put a blank line… | |||||
* @param[in] cmd | |||||
* The SMBus command for the target device; this is the offset for SPDs, | |||||
* or the register number for TSODs. | |||||
* | |||||
* @param[out] byte | |||||
* The byte which was read. | |||||
*/ | |||||
static int | |||||
imcsmb_readb(device_t dev, u_char slave, char cmd, char *byte) | |||||
{ | |||||
return imcsmb_transfer(dev, slave, cmd, byte, FALSE, FALSE); | |||||
} | |||||
/** | |||||
* smbus_readw() method. Thin wrapper around imcsmb_transfer(). | |||||
* | |||||
* @author Joe Kloss | |||||
* | |||||
* @param[in] dev | |||||
* | |||||
* @param[in] slave | |||||
* The SMBus address of the target device. | |||||
* | |||||
* @param[in] cmd | |||||
* The SMBus command for the target device; this is the offset for SPDs, | |||||
* or the register number for TSODs. | |||||
* | |||||
* @param[out] word | |||||
* The word which was read. | |||||
*/ | |||||
static int | |||||
imcsmb_readw(device_t dev, u_char slave, char cmd, short *word) | |||||
{ | |||||
return imcsmb_transfer(dev, slave, cmd, word, TRUE, FALSE); | |||||
} | |||||
/** | |||||
* smbus_writeb() method. Thin wrapper around imcsmb_transfer(). | |||||
* | |||||
* @author Joe Kloss | |||||
* | |||||
* @param[in] dev | |||||
* | |||||
* @param[in] slave | |||||
* The SMBus address of the target device. | |||||
* | |||||
* @param[in] cmd | |||||
* The SMBus command for the target device; this is the offset for SPDs, | |||||
* or the register number for TSODs. | |||||
* | |||||
* @param[in] byte | |||||
* The byte to write. | |||||
*/ | |||||
static int | |||||
imcsmb_writeb(device_t dev, u_char slave, char cmd, char byte) | |||||
{ | |||||
return imcsmb_transfer(dev, slave, cmd, &byte, FALSE, TRUE); | |||||
} | |||||
/** | |||||
* smbus_writew() method. Thin wrapper around imcsmb_transfer(). | |||||
* | |||||
* @author Joe Kloss | |||||
* | |||||
* @param[in] dev | |||||
* | |||||
* @param[in] slave | |||||
* The SMBus address of the target device. | |||||
* | |||||
* @param[in] cmd | |||||
* The SMBus command for the target device; this is the offset for SPDs, | |||||
* or the register number for TSODs. | |||||
* | |||||
* @param[in] word | |||||
* The word to write. | |||||
*/ | |||||
static int | |||||
imcsmb_writew(device_t dev, u_char slave, char cmd, short word) | |||||
{ | |||||
return imcsmb_transfer(dev, slave, cmd, &word, TRUE, TRUE); | |||||
} | |||||
/** | |||||
* Manipulate the PCI control registers to read data from or write data to the | |||||
* SMBus controller. | |||||
* | |||||
* @author Joe Kloss, rpokala | |||||
* | |||||
* @param[in] dev | |||||
* | |||||
* @param[in] slave | |||||
* The SMBus address of the target device. | |||||
* | |||||
* @param[in] cmd | |||||
* The SMBus command for the target device; this is the offset for SPDs, | |||||
* or the register number for TSODs. | |||||
* | |||||
* @param[in,out] data | |||||
* Pointer to either the value to be written, or where to place the value | |||||
* which was read. | |||||
* | |||||
* @param[in] word_op | |||||
* Bool: is this a word operation? | |||||
* | |||||
* @param[in] write_op | |||||
* Bool: is this a write operation? | |||||
*/ | |||||
static int | |||||
imcsmb_transfer(device_t dev, u_char slave, char cmd, void *data, int word_op, | |||||
int write_op) | |||||
{ | |||||
struct imcsmb_softc *sc; | |||||
int i; | |||||
int rc; | |||||
Done Inline ActionsYou don't need casts when casting a void * pointer in C btw. jhb: You don't need casts when casting a void * pointer in C btw. | |||||
uint32_t cmd_val; | |||||
uint32_t cntl_val; | |||||
uint32_t orig_cntl_val; | |||||
uint32_t stat_val; | |||||
uint16_t *word; | |||||
uint16_t lword; | |||||
uint8_t *byte; | |||||
uint8_t lbyte; | |||||
sc = device_get_softc(dev); | |||||
byte = (uint8_t *) data; | |||||
word = (uint16_t *) data; | |||||
lbyte = *byte; | |||||
lword = *word; | |||||
/* We modify the value of the control register; save the original, so | |||||
* we can restore it later | |||||
*/ | |||||
orig_cntl_val = pci_read_config(sc->imcsmb_pci, | |||||
sc->regs->smb_cntl, 4); | |||||
cntl_val = orig_cntl_val; | |||||
/* | |||||
* Set up the SMBCNTL register | |||||
*/ | |||||
/* [31:28] Clear the existing value of the DTI bits, then set them to | |||||
* the four high bits of the slave address. | |||||
*/ | |||||
cntl_val &= ~IMCSMB_CNTL_DTI_MASK; | |||||
cntl_val |= ((uint32_t) slave & 0xf0) << 24; | |||||
/* [27:27] Set the CLK_OVERRIDE bit, to enable normal operation */ | |||||
cntl_val |= IMCSMB_CNTL_CLK_OVERRIDE; | |||||
/* [26:26] Clear the WRITE_DISABLE bit; the datasheet says this isn't | |||||
* necessary, but empirically, it is. | |||||
*/ | |||||
cntl_val &= ~IMCSMB_CNTL_WRITE_DISABLE_BIT; | |||||
/* [9:9] Clear the POLL_EN bit, to stop the hardware TSOD polling. */ | |||||
cntl_val &= ~IMCSMB_CNTL_POLL_EN; | |||||
/* | |||||
* Set up the SMBCMD register | |||||
*/ | |||||
/* [31:31] Set the TRIGGER bit; when this gets written, the controller | |||||
* will issue the command. | |||||
*/ | |||||
cmd_val = IMCSMB_CMD_TRIGGER_BIT; | |||||
/* [29:29] For word operations, set the WORD_ACCESS bit. */ | |||||
if (word_op) { | |||||
cmd_val |= IMCSMB_CMD_WORD_ACCESS; | |||||
} | |||||
/* [27:27] For write operations, set the WRITE bit. */ | |||||
if (write_op) { | |||||
cmd_val |= IMCSMB_CMD_WRITE_BIT; | |||||
} | |||||
/* [26:24] The three non-DTI, non-R/W bits of the slave address. */ | |||||
cmd_val |= (uint32_t) ((slave & 0xe) << 23); | |||||
/* [23:16] The command (offset in the case of an EEPROM, or register in | |||||
* the case of TSOD or NVDIMM controller). | |||||
*/ | |||||
cmd_val |= (uint32_t) ((uint8_t) cmd << 16); | |||||
/* [15:0] The data to be written for a write operation. */ | |||||
if (write_op) { | |||||
if (word_op) { | |||||
/* The datasheet says the controller uses different | |||||
* endianness for word operations on I2C vs SMBus! | |||||
* I2C: [15:8] = MSB; [7:0] = LSB | |||||
* SMB: [15:8] = LSB; [7:0] = MSB | |||||
* As a practical matter, this controller is very | |||||
* specifically for use with DIMMs, the SPD (and | |||||
* NVDIMM controllers) are only accessed as bytes, | |||||
* the temperature sensor is only accessed as words, and | |||||
* the temperature sensors are I2C. Thus, byte-swap the | |||||
* word. | |||||
*/ | |||||
lword = htobe16(lword); | |||||
} else { | |||||
/* For byte operations, the data goes in the LSB, and | |||||
* the MSB is a don't care. | |||||
*/ | |||||
lword = (uint16_t) (lbyte & 0xff); | |||||
} | |||||
cmd_val |= lword; | |||||
} | |||||
/* Write the updated value to the control register first, to disable | |||||
* the hardware TSOD polling. | |||||
*/ | |||||
pci_write_config(sc->imcsmb_pci, sc->regs->smb_cntl, cntl_val, 4); | |||||
/* Poll on the BUSY bit in the status register until clear, or timeout. | |||||
* We just cleared the auto-poll bit, so we need to make sure the device | |||||
* is idle before issuing a command. We can safely timeout after 35 ms, | |||||
* as this is the maximum time the SMBus spec allows for a transaction. | |||||
*/ | |||||
for (i = 4; i != 0; i--) { | |||||
stat_val = pci_read_config(sc->imcsmb_pci, sc->regs->smb_stat, | |||||
4); | |||||
if (! (stat_val & IMCSMB_STATUS_BUSY_BIT)) { | |||||
break; | |||||
} | |||||
pause("imcsmb", 10 * hz / 1000); | |||||
} | |||||
if (i == 0) { | |||||
device_printf(sc->dev, | |||||
"transfer: timeout waiting for device to settle\n"); | |||||
} | |||||
/* Now that polling has stopped, we can write the command register. This | |||||
* starts the SMBus command. | |||||
*/ | |||||
pci_write_config(sc->imcsmb_pci, sc->regs->smb_cmd, cmd_val, 4); | |||||
/* Wait for WRITE_DATA_DONE/READ_DATA_VALID to be set, or timeout and | |||||
* fail. We wait up to 35ms. | |||||
*/ | |||||
for (i = 3500; i != 0; i -= 10) | |||||
{ | |||||
Done Inline ActionsYou either need 35000 as a starting value or to decrement by one to get 35ms. avg: You either need 35000 as a starting value or to decrement by one to get 35ms.
Right now it's 3. | |||||
Done Inline ActionsWhoops! Good catch! rpokala: Whoops! Good catch! | |||||
DELAY(10); | |||||
stat_val = pci_read_config(sc->imcsmb_pci, sc->regs->smb_stat, | |||||
4); | |||||
/* For a write, the bits holding the data contain the data being | |||||
* written. You'd think that would cause the READ_DATA_VALID bit | |||||
* to be cleared, because the data bits no longer contain valid | |||||
* data from the most recent read operation. While that would be | |||||
* logical, that's not the case here: READ_DATA_VALID is only | |||||
* cleared when starting a read operation, and WRITE_DATA_DONE | |||||
* is only cleared when starting a write operation. | |||||
*/ | |||||
if (write_op) { | |||||
if (stat_val & IMCSMB_STATUS_WRITE_DATA_DONE) { | |||||
break; | |||||
} | |||||
} else { | |||||
if (stat_val & IMCSMB_STATUS_READ_DATA_VALID) { | |||||
break; | |||||
} | |||||
} | |||||
} | |||||
if (i == 0) { | |||||
rc = SMB_ETIMEOUT; | |||||
device_printf(dev, "transfer timeout\n"); | |||||
goto out; | |||||
} | |||||
/* It is generally the case that this bit indicates non-ACK, but it | |||||
* could also indicate other bus errors. There's no way to tell the | |||||
* difference. | |||||
*/ | |||||
if (stat_val & IMCSMB_STATUS_BUS_ERROR_BIT) { | |||||
/* While it is not documented, empirically, SPD page-change | |||||
Done Inline Actionsstyle calls for (x & y) != 0 avg: style calls for `(x & y) != 0` | |||||
Done Inline ActionsIndeed it does. rpokala: Indeed it does. | |||||
* commands (writes with DTI = 0x60) always complete with the | |||||
* error bit set. So, ignore it in those cases. | |||||
*/ | |||||
if ((slave & 0xf0) != 0x60) { | |||||
rc = SMB_ENOACK; | |||||
goto out; | |||||
} | |||||
} | |||||
/* For a read operation, copy the data out */ | |||||
if (! write_op) { | |||||
if (word_op) { | |||||
Done Inline Actionsno space after ! operator avg: no space after `!` operator | |||||
Done Inline ActionsYeah, I think I did that a few places. rpokala: Yeah, I think I did that a few places. | |||||
Done Inline ActionsChanged to (write_op == 0). rpokala: Changed to `(write_op == 0)`. | |||||
/* The data is returned in bits [15:0]; as discussed | |||||
* above, byte-swap. | |||||
*/ | |||||
lword = (uint16_t) (stat_val & 0xffff); | |||||
lword = htobe16(lword); | |||||
*word = lword; | |||||
} else { | |||||
/* The data is returned in bits [7:0] */ | |||||
lbyte = (uint8_t) (stat_val & 0xff); | |||||
*byte = lbyte; | |||||
} | |||||
} | |||||
/* A lack of an error is, de facto, success. */ | |||||
rc = SMB_ENOERR; | |||||
out: | |||||
/* Restore the original value of the control register. */ | |||||
pci_write_config(sc->imcsmb_pci, sc->regs->smb_cntl, orig_cntl_val, 4); | |||||
return (rc); | |||||
Not Done Inline ActionsI would put probe first here as mentioned earlier. jhb: I would put probe first here as mentioned earlier. | |||||
} | |||||
/* Our device class */ | |||||
Done Inline ActionsYou don't need this if you aren't going to call bus_generic_probe() (which you don't need for this driver) jhb: You don't need this if you aren't going to call bus_generic_probe() (which you don't need for… | |||||
static devclass_t imcsmb_devclass; | |||||
Done Inline ActionsYou don't need the bus_driver_added and bus_new_pass lines here. The default handlers for these methods are the 'bus_generic_*' ones already from sys/kern/bus_if.m. jhb: You don't need the bus_driver_added and bus_new_pass lines here. The default handlers for… | |||||
Done Inline ActionsI could have sworn that someone told me they were required. I must have misunderstood. rpokala: I could have sworn that someone told me they were required. I must have misunderstood. | |||||
/* Device methods */ | |||||
static device_method_t imcsmb_methods[] = { | |||||
/* Device interface */ | |||||
DEVMETHOD(device_attach, imcsmb_attach), | |||||
DEVMETHOD(device_detach, imcsmb_detach), | |||||
DEVMETHOD(device_probe, imcsmb_probe), | |||||
/* Bus interface */ | |||||
DEVMETHOD(bus_add_child, bus_generic_add_child), | |||||
DEVMETHOD(bus_driver_added, bus_generic_driver_added), | |||||
DEVMETHOD(bus_new_pass, bus_generic_new_pass), | |||||
/* smbus methods */ | |||||
DEVMETHOD(smbus_callback, imcsmb_callback), | |||||
DEVMETHOD(smbus_readb, imcsmb_readb), | |||||
DEVMETHOD(smbus_readw, imcsmb_readw), | |||||
DEVMETHOD(smbus_writeb, imcsmb_writeb), | |||||
DEVMETHOD(smbus_writew, imcsmb_writew), | |||||
DEVMETHOD_END | |||||
}; | |||||
static driver_t imcsmb_driver = { | |||||
.name = "imcsmb", | |||||
.methods = imcsmb_methods, | |||||
.size = sizeof(struct imcsmb_softc), | |||||
}; | |||||
DRIVER_MODULE(imcsmb, imcsmb_pci, imcsmb_driver, imcsmb_devclass, 0, 0); | |||||
MODULE_DEPEND(imcsmb, smbus, SMBUS_MINVER, SMBUS_PREFVER, SMBUS_MAXVER); | |||||
MODULE_VERSION(imcsmb, 1); | |||||
DRIVER_MODULE(smbus, imcsmb, smbus_driver, smbus_devclass, 0, 0); | |||||
/* vi: set ts=8 sw=4 sts=8 noet: */ |
Looks like these declarations of static functions are not really required.