Hi Samuel,
On Fri, 18 Sep 2009 16:42:49 +0200, Samuel Ortiz wrote:
> On Fri, Sep 18, 2009 at 02:09:23PM +0200, Rodolfo Giometti wrote:
> > +#define W627_DEVID 0x52
> > +#define W627THF_DEVID 0x82
> > +#define W697_DEVID 0x60
> > +#define W637_DEVID 0x70
> > +#define W687THF_DEVID 0x85
> > +#define WINB_ACT_REG 0x30
> > +#define WINB_BASE_REG 0x60
> Are those 2 last ones HWMON specific. If that's so, please be more specific
> about it.
No, these are standard registers common to all logical devices.
> > +/* Constants specified below */
> > +
> > +/* Offset & size of I/O region we are interested in */
> This seems to be the hwmon region, so please be more specific about it in the
> comments and in the naming below.
Correct.
> > +#define WINB_REGION_OFFSET 5
> > +#define WINB_REGION_SIZE 2
> > +
> > +static int __init w83627hf_find(int sioaddr, unsigned short *addr,
> > + struct w83627hf_sio_data *sio_data)
> > +{
> > + int err = -ENODEV;
> > + u16 val;
> > +
> > + sio_data->sioaddr = sioaddr;
> > +
> > + superio_enter(sio_data);
> > + val = force_id ? force_id : superio_inb(sio_data, 0x20);
> > + switch (val) {
> > + case W627_DEVID:
> > + sio_data->type = w83627hf;
> > + break;
> > + case W627THF_DEVID:
> > + sio_data->type = w83627thf;
> > + break;
> > + case W697_DEVID:
> > + sio_data->type = w83697hf;
> > + break;
> > + case W637_DEVID:
> > + sio_data->type = w83637hf;
> > + break;
> > + case W687THF_DEVID:
> > + sio_data->type = w83687thf;
> > + break;
> > + case 0xff: /* No device at all */
> > + goto exit;
> > + default:
> > + pr_debug(DRVNAME ": Unsupported chip (DEVID=0x%02x)\n", val);
> > + goto exit;
> > + }
> > +
> > + sio_data->name = names[sio_data->type];
> > +
> > + superio_select(sio_data, W83627HF_LD_HWM);
> > + force_hwmon_addr &= (~7);
> > + if (force_hwmon_addr) {
> > + printk(KERN_WARNING DRVNAME ": Forcing address 0x%x\n",
> > + force_hwmon_addr);
> > + superio_outb(sio_data, WINB_BASE_REG, force_hwmon_addr >> 8);
> > + superio_outb(sio_data, WINB_BASE_REG + 1,
> > + force_hwmon_addr & 0xff);
> > + }
> > + val = (superio_inb(sio_data, WINB_BASE_REG) << 8) |
> > + superio_inb(sio_data, WINB_BASE_REG + 1);
> > + *addr = val & (~7);
> > + if (*addr == 0) {
> > + printk(KERN_WARNING DRVNAME ": Base address not set, "
> > + "skipping\n");
> > + goto exit;
> > + }
> > +
> > + val = superio_inb(sio_data, WINB_ACT_REG);
> > + if (!(val & 0x01)) {
> > + printk(KERN_WARNING DRVNAME ": Enabling HWM logical device\n");
> > + superio_outb(sio_data, WINB_ACT_REG, val | 0x01);
> > + }
> > +
> > + err = 0;
> > + pr_info(DRVNAME ": Found %s chip at %#x\n",
> > + names[sio_data->type], *addr);
>
> I'm not familiar with this chip, but although the function is called
> w83627hf_find(), it seems to me that what we're doing here is enabling the
> hwmon subdevice. Am I correct ? If that's so, could we split this function,
> and maybe even move the hwmon specific part into the hwmon driver.
The enablement of the hwmon logical device is only a side effect. The
main goal of the function is to figure out whether a supported chip is
present, and which one. The original reason to put everything is the
same function is so that we don't have to enter and exit the super-io
config mode twice.
But I wouldn't object to it being split into two separate functions,
each one doing just one thing. That would be cleaner, and speed isn't
critical.
> > + exit:
> > + superio_exit(sio_data);
> > + return err;
> > +}
> > (...)
> > +static inline void superio_enter(struct w83627hf_sio_data *sio)
> > +{
> > + mutex_lock(&sio->lock);
> > +
> > + outb(0x87, sio->sioaddr);
> > + outb(0x87, sio->sioaddr);
>
> I'm not really happy with this one. The unbalanced locking here makes me feel
> unconfortable.
Me, I don't have any objection. Callers will have to balance
superio_enter and superio_exit anyway. So hiding the mutex in there
seems reasonable.
> Something like that:
>
> void superio_outb(struct w83627hf_sio_data *sio, int ld, int reg, int val)
> {
> mutex_lock(&sio->lock);
>
> /* Enter */
> outb(0x87, sio->sioaddr)
> outb(0x87, sio->sioaddr)
>
> /* Select module */
> outb(0x07, sio->sioaddr);
> outb(ld, sio->sioaddr + 1);
>
> /* Write */
> outb(reg, sio->sioaddr);
> outb(val, sio->sioaddr + 1);
>
> /* Exit */
> outb(0xAA, sio->sioaddr);
>
> mutex_unlock(&sio->lock);
> }
>
> would look saner to me. I know we're wasting many IO ops here, but it seems to
> me that the subdevices get to call the superio API only at init time.
> I think it's worth it as your subdevices wouldnt have to care about following
> these error prone steps, and you wouldnt have any more unbalanced locking
> risks.
The above has limitations. Think of the hwmon logical device enablement
function which we discussed above. In this function we must read a
register, and depending on the value, maybe write to it, but altering
only one bit (so based on the original value.)
There is no way to achieve this in a race-free manner with your
proposed superio_outb() function and the equivalent superio_inb()
implementation. Even a superio_alterb() function wouldn't do, because
we want to print a message.
At least with Rodolfo's proposal, this can be done. With your approach,
we'd need dedicated functions for every non-trivial register access.
As far as speed is concerned, I agree it's not critical, at least for
the use cases I know of.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
