Hi Jean, Rodolfo,
On Thu, Oct 01, 2009 at 03:23:51PM +0200, Jean Delvare wrote:
> Hi Rodolfo,
>
> On Thu, 24 Sep 2009 14:01:11 +0200, Rodolfo Giometti wrote:
> > The file has been splitted up into two parts:
> >
> > * drivers/mfd/w83627hf-core.c - detects the chip and define proper
> > platform devices into mfd support
> >
> > * drivers/hwmon/w83627hf.c - implements the driver for hwmon
> > functionality only
> >
> > Signed-off-by: Rodolfo Giometti <giometti@xxxxxxxx>
> > ---
> > drivers/hwmon/Kconfig | 1 +
> > drivers/hwmon/w83627hf.c | 376 ++++++++++++------------------------------
> > drivers/mfd/Kconfig | 15 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/w83627hf-core.c | 186 +++++++++++++++++++++
> > include/linux/mfd/w83627hf.h | 118 +++++++++++++
> > 6 files changed, 428 insertions(+), 269 deletions(-)
> > create mode 100644 drivers/mfd/w83627hf-core.c
> > create mode 100644 include/linux/mfd/w83627hf.h
>
> Sorry for the late review...
>
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 2d50166..f6cf2af 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -894,6 +894,7 @@ config SENSORS_W83L786NG
> >
> > config SENSORS_W83627HF
> > tristate "Winbond W83627HF, W83627THF, W83637HF, W83687THF, W83697HF"
> > + select MFD_W83627HF
> > select HWMON_VID
> > help
> > If you say yes here you get support for the Winbond W836X7 series
> > diff --git a/drivers/hwmon/w83627hf.c b/drivers/hwmon/w83627hf.c
> > index 389150b..748f77a 100644
> > --- a/drivers/hwmon/w83627hf.c
> > +++ b/drivers/hwmon/w83627hf.c
> > @@ -52,13 +52,9 @@
> > #include <linux/ioport.h>
> > #include <linux/acpi.h>
> > #include <asm/io.h>
> > +#include <linux/mfd/w83627hf.h>
> > #include "lm75.h"
> >
> > -static struct platform_device *pdev;
> > -
> > -#define DRVNAME "w83627hf"
> > -enum chips { w83627hf, w83627thf, w83697hf, w83637hf, w83687thf };
> > -
> > static u16 force_addr;
> > module_param(force_addr, ushort, 0);
> > MODULE_PARM_DESC(force_addr,
> > @@ -72,87 +68,11 @@ static int init = 1;
> > module_param(init, bool, 0);
> > MODULE_PARM_DESC(init, "Set to zero to bypass chip initialization");
> >
> > -static unsigned short force_id;
> > -module_param(force_id, ushort, 0);
> > -MODULE_PARM_DESC(force_id, "Override the detected device ID");
> > -
> > -/* modified from kernel/include/traps.c */
> > -static int REG; /* The register to read/write */
> > -#define DEV 0x07 /* Register: Logical device select */
> > -static int VAL; /* The value to read/write */
> > -
> > -/* logical device numbers for superio_select (below) */
> > -#define W83627HF_LD_FDC 0x00
> > -#define W83627HF_LD_PRT 0x01
> > -#define W83627HF_LD_UART1 0x02
> > -#define W83627HF_LD_UART2 0x03
> > -#define W83627HF_LD_KBC 0x05
> > -#define W83627HF_LD_CIR 0x06 /* w83627hf only */
> > -#define W83627HF_LD_GAME 0x07
> > -#define W83627HF_LD_MIDI 0x07
> > -#define W83627HF_LD_GPIO1 0x07
> > -#define W83627HF_LD_GPIO5 0x07 /* w83627thf only */
> > -#define W83627HF_LD_GPIO2 0x08
> > -#define W83627HF_LD_GPIO3 0x09
> > -#define W83627HF_LD_GPIO4 0x09 /* w83627thf only */
> > -#define W83627HF_LD_ACPI 0x0a
> > -#define W83627HF_LD_HWM 0x0b
> > -
> > -#define DEVID 0x20 /* Register: Device ID */
> > -
> > -#define W83627THF_GPIO5_EN 0x30 /* w83627thf only */
> > -#define W83627THF_GPIO5_IOSR 0xf3 /* w83627thf only */
> > -#define W83627THF_GPIO5_DR 0xf4 /* w83627thf only */
> > -
> > -#define W83687THF_VID_EN 0x29 /* w83687thf only */
> > -#define W83687THF_VID_CFG 0xF0 /* w83687thf only */
> > -#define W83687THF_VID_DATA 0xF1 /* w83687thf only */
> > -
> > -static inline void
> > -superio_outb(int reg, int val)
> > -{
> > - outb(reg, REG);
> > - outb(val, VAL);
> > -}
> > -
> > -static inline int
> > -superio_inb(int reg)
> > -{
> > - outb(reg, REG);
> > - return inb(VAL);
> > -}
> > -
> > -static inline void
> > -superio_select(int ld)
> > -{
> > - outb(DEV, REG);
> > - outb(ld, VAL);
> > -}
> > -
> > -static inline void
> > -superio_enter(void)
> > -{
> > - outb(0x87, REG);
> > - outb(0x87, REG);
> > -}
> > -
> > -static inline void
> > -superio_exit(void)
> > -{
> > - outb(0xAA, REG);
> > -}
> > -
> > -#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
> > /* Constants specified below */
> >
> > -/* Alignment of the base address */
> > -#define WINB_ALIGNMENT ~7
>
> Not sure why you removed that one while your code still uses it.
>
> > +#define HWMON_CR30 0x30
> > +#define ACTIVATION_CTRL (1 << 0)
> > +#define HWMON_CR60 0x60
>
> Why are you changing the name of these registers from meaningful ones
> to meaningless ones?
>
> >
> > /* Offset & size of I/O region we are interested in */
> > #define WINB_REGION_OFFSET 5
> > @@ -380,10 +300,6 @@ struct w83627hf_data {
> > u8 vrm_ovt; /* Register value, 627THF/637HF/687THF only */
> > };
> >
> > -struct w83627hf_sio_data {
> > - enum chips type;
> > -};
> > -
> >
> > static int w83627hf_probe(struct platform_device *pdev);
> > static int __devexit w83627hf_remove(struct platform_device *pdev);
> > @@ -397,7 +313,7 @@ static void w83627hf_init_device(struct platform_device *pdev);
> > static struct platform_driver w83627hf_driver = {
> > .driver = {
> > .owner = THIS_MODULE,
> > - .name = DRVNAME,
> > + .name = DRVNAME "_hwmon",
> > },
> > .probe = w83627hf_probe,
> > .remove = __devexit_p(w83627hf_remove),
> > @@ -1126,80 +1042,6 @@ show_name(struct device *dev, struct device_attribute *devattr, char *buf)
> > }
> > static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> >
> > -static int __init w83627hf_find(int sioaddr, unsigned short *addr,
> > - struct w83627hf_sio_data *sio_data)
> > -{
> > - int err = -ENODEV;
> > - u16 val;
> > -
> > - static const __initdata char *names[] = {
> > - "W83627HF",
> > - "W83627THF",
> > - "W83697HF",
> > - "W83637HF",
> > - "W83687THF",
> > - };
> > -
> > - REG = sioaddr;
> > - VAL = sioaddr + 1;
> > -
> > - superio_enter();
> > - val = force_id ? force_id : superio_inb(DEVID);
> > - 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;
> > - }
> > -
> > - superio_select(W83627HF_LD_HWM);
> > - force_addr &= WINB_ALIGNMENT;
> > - if (force_addr) {
> > - printk(KERN_WARNING DRVNAME ": Forcing address 0x%x\n",
> > - force_addr);
> > - superio_outb(WINB_BASE_REG, force_addr >> 8);
> > - superio_outb(WINB_BASE_REG + 1, force_addr & 0xff);
> > - }
> > - val = (superio_inb(WINB_BASE_REG) << 8) |
> > - superio_inb(WINB_BASE_REG + 1);
> > - *addr = val & WINB_ALIGNMENT;
> > - if (*addr == 0) {
> > - printk(KERN_WARNING DRVNAME ": Base address not set, "
> > - "skipping\n");
> > - goto exit;
> > - }
> > -
> > - val = superio_inb(WINB_ACT_REG);
> > - if (!(val & 0x01)) {
> > - printk(KERN_WARNING DRVNAME ": Enabling HWM logical device\n");
> > - superio_outb(WINB_ACT_REG, val | 0x01);
> > - }
> > -
> > - err = 0;
> > - pr_info(DRVNAME ": Found %s chip at %#x\n",
> > - names[sio_data->type], *addr);
> > -
> > - exit:
> > - superio_exit();
> > - return err;
> > -}
> > -
> > #define VIN_UNIT_ATTRS(_X_) \
> > &sensor_dev_attr_in##_X_##_input.dev_attr.attr, \
> > &sensor_dev_attr_in##_X_##_min.dev_attr.attr, \
> > @@ -1278,27 +1120,89 @@ static const struct attribute_group w83627hf_group_opt = {
> > .attrs = w83627hf_attributes_opt,
> > };
> >
> > +static int w83627hf_enable_hwmon(struct w83627hf_sio_data *sio_data)
> > +{
> > + u16 val;
> > + int ret;
> > +
> > + superio_enter(sio_data);
> > +
> > + superio_select(sio_data, W83627HF_LD_HWM);
> > +
> > + force_addr &= (~7);
> > + if (force_addr) {
> > + printk(KERN_WARNING DRVNAME ": Forcing address 0x%x\n",
> > + force_addr);
> > + superio_outb(sio_data, HWMON_CR60, force_addr >> 8);
> > + superio_outb(sio_data, HWMON_CR60 + 1, force_addr & 0xff);
> > + }
> > +
> > + val = (superio_inb(sio_data, HWMON_CR60) << 8) |
> > + superio_inb(sio_data, HWMON_CR60 + 1);
> > + ret = val & (~7);
> > + pr_info(DRVNAME ": hwmon chip %s at %#x\n", sio_data->name, ret);
> > +
> > + if (ret == 0) {
> > + printk(KERN_WARNING DRVNAME ": Base address not set, "
> > + "skipping\n");
> > + ret = -EINVAL;
> > + goto exit;
> > + }
> > +
> > + val = superio_inb(sio_data, HWMON_CR30);
> > + superio_outb(sio_data, HWMON_CR30, val | ACTIVATION_CTRL);
> > +
> > +exit:
> > + superio_exit(sio_data);
> > +
> > + return ret;
> > +}
>
> If this function was moved where w83627hf_find() was before, your patch
> would be somewhat smaller and easier to review.
>
> > +
> > +static void w83627hf_disable_hwmon(struct w83627hf_sio_data *sio_data)
> > +{
> > + u16 val;
> > +
> > + superio_enter(sio_data);
> > +
> > + superio_select(sio_data, W83627HF_LD_HWM);
> > +
> > + val = superio_inb(sio_data, HWMON_CR30);
> > + superio_outb(sio_data, HWMON_CR30, val & ~ACTIVATION_CTRL);
> > +
> > + superio_exit(sio_data);
> > +}
>
> No, we don't want to do this. Most systems boot with hardware
> monitoring enabled, and the fact of loading then unloading a driver
> should not change this. It only makes sense to disable hwmon on the few
> systems where we had to enable it ourselves when loading the driver,
> but your code doesn't check for this.
>
> > +
> > static int __devinit w83627hf_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > - struct w83627hf_sio_data *sio_data = dev->platform_data;
> > + struct w83627hf_sio_data *sio_data = dev->parent->platform_data;
> > struct w83627hf_data *data;
> > - struct resource *res;
> > int err, i;
> >
> > - static const char *names[] = {
> > - "w83627hf",
> > - "w83627thf",
> > - "w83697hf",
> > - "w83637hf",
> > - "w83687thf",
> > + struct resource res = {
> > + .start = /* address + */ WINB_REGION_OFFSET,
> > + .end = /* address + */ WINB_REGION_OFFSET +
> > + WINB_REGION_SIZE - 1,
> > + .name = DRVNAME "_hwmon",
> > + .flags = IORESOURCE_IO,
> > };
> >
> > - res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > - if (!request_region(res->start, WINB_REGION_SIZE, DRVNAME)) {
> > + err = w83627hf_enable_hwmon(sio_data);
> > + if (err < 0)
> > + return err;
> > +
> > + /* Before doing our job we should fixup ioport range */
> > + res.start += err;
> > + res.end += err;
>
> Again, I don't get the point of this relative adjustment. Just
> omit .start and .end in the original resource declaration, and set them
> when you have all the required information.
>
> But more importantly, I don't get why you are creating a new resource
> here, instead of getting it from the platform device? This is how
> things are supposed to work.
>
> Ah yes... You need to do this or you can't implement the force_addr
> module parameter. Hmpf. You know what? I think it's about time to get
> rid of this parameter. It just doesn't fit in the MFD design, and I'm
> not even sure why needs this. If some board needs to force the address,
> it should get fixed in the BIOS, or it can be done from user-space
> (using isaset) or in a platform-specific quirk. Or if we want to have a
> module parameter, that would be a generic one in the MFD driver (so
> that you can force the address of any LD, nor just the hwmon one.
>
> So I'd say, just drop the force_addr module parameter for now.
> Preferably in a separate patch. Or I can even do it myself if you want.
> That way you can really stick to the MFD design and this will be a much
> better base for the future.
>
> BTW I am sorry that this is such a long process to get your patch
> right. But this is the first Super-I/O driver we convert to the MFD
> design, so it was somewhat expected it would take some tries before we
> got everything right.
>
> > +
> > + err = acpi_check_resource_conflict(&res);
> > + if (err)
> > + goto ERROR0;
>
> I don't understand why this happens only now, rather than in
> mfd_w83627hf? If there is a resource conflict then there is no point in
> instantiating the (hwmon) w83627hf platform device at all.
>
> > +
> > + if (!request_region(res.start, WINB_REGION_SIZE, DRVNAME "_hwmon")) {
> > dev_err(dev, "Failed to request region 0x%lx-0x%lx\n",
> > - (unsigned long)res->start,
> > - (unsigned long)(res->start + WINB_REGION_SIZE - 1));
> > + (unsigned long) res.start,
> > + (unsigned long) (res.start + WINB_REGION_SIZE - 1));
> > err = -EBUSY;
> > goto ERROR0;
> > }
> > @@ -1307,9 +1211,9 @@ static int __devinit w83627hf_probe(struct platform_device *pdev)
> > err = -ENOMEM;
> > goto ERROR1;
> > }
> > - data->addr = res->start;
> > + data->addr = res.start;
> > data->type = sio_data->type;
> > - data->name = names[sio_data->type];
> > + data->name = sio_data->name;
> > mutex_init(&data->lock);
> > mutex_init(&data->update_lock);
> > platform_set_drvdata(pdev, data);
> > @@ -1442,15 +1346,20 @@ static int __devinit w83627hf_probe(struct platform_device *pdev)
> > platform_set_drvdata(pdev, NULL);
> > kfree(data);
> > ERROR1:
> > - release_region(res->start, WINB_REGION_SIZE);
> > + release_region(res.start, WINB_REGION_SIZE);
> > ERROR0:
> > + w83627hf_disable_hwmon(sio_data);
> > return err;
> > }
> >
> > static int __devexit w83627hf_remove(struct platform_device *pdev)
> > {
> > + struct device *dev = &pdev->dev;
> > + struct w83627hf_sio_data *sio_data = dev->parent->platform_data;
> > struct w83627hf_data *data = platform_get_drvdata(pdev);
> > - struct resource *res;
> > + unsigned int addr = data->addr;
> > +
> > + w83627hf_disable_hwmon(sio_data);
> >
> > hwmon_device_unregister(data->hwmon_dev);
> >
> > @@ -1459,8 +1368,7 @@ static int __devexit w83627hf_remove(struct platform_device *pdev)
> > platform_set_drvdata(pdev, NULL);
> > kfree(data);
> >
> > - res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > - release_region(res->start, WINB_REGION_SIZE);
> > + release_region(addr, WINB_REGION_SIZE);
> >
> > return 0;
> > }
> > @@ -1511,20 +1419,22 @@ static int w83627hf_read_value(struct w83627hf_data *data, u16 reg)
> >
> > static int __devinit w83627thf_read_gpio5(struct platform_device *pdev)
> > {
> > + struct device *dev = &pdev->dev;
> > + struct w83627hf_sio_data *sio_data = dev->parent->platform_data;
> > int res = 0xff, sel;
> >
> > - superio_enter();
> > - superio_select(W83627HF_LD_GPIO5);
> > + superio_enter(sio_data);
> > + superio_select(sio_data, W83627HF_LD_GPIO5);
> >
> > /* Make sure these GPIO pins are enabled */
> > - if (!(superio_inb(W83627THF_GPIO5_EN) & (1<<3))) {
> > + if (!(superio_inb(sio_data, W83627THF_GPIO5_EN) & (1<<3))) {
> > dev_dbg(&pdev->dev, "GPIO5 disabled, no VID function\n");
> > goto exit;
> > }
> >
> > /* Make sure the pins are configured for input
> > There must be at least five (VRM 9), and possibly 6 (VRM 10) */
> > - sel = superio_inb(W83627THF_GPIO5_IOSR) & 0x3f;
> > + sel = superio_inb(sio_data, W83627THF_GPIO5_IOSR) & 0x3f;
> > if ((sel & 0x1f) != 0x1f) {
> > dev_dbg(&pdev->dev, "GPIO5 not configured for VID "
> > "function\n");
> > @@ -1532,37 +1442,39 @@ static int __devinit w83627thf_read_gpio5(struct platform_device *pdev)
> > }
> >
> > dev_info(&pdev->dev, "Reading VID from GPIO5\n");
> > - res = superio_inb(W83627THF_GPIO5_DR) & sel;
> > + res = superio_inb(sio_data, W83627THF_GPIO5_DR) & sel;
> >
> > exit:
> > - superio_exit();
> > + superio_exit(sio_data);
> > return res;
> > }
> >
> > static int __devinit w83687thf_read_vid(struct platform_device *pdev)
> > {
> > + struct device *dev = &pdev->dev;
> > + struct w83627hf_sio_data *sio_data = dev->parent->platform_data;
> > int res = 0xff;
> >
> > - superio_enter();
> > - superio_select(W83627HF_LD_HWM);
> > + superio_enter(sio_data);
> > + superio_select(sio_data, W83627HF_LD_HWM);
> >
> > /* Make sure these GPIO pins are enabled */
> > - if (!(superio_inb(W83687THF_VID_EN) & (1 << 2))) {
> > + if (!(superio_inb(sio_data, W83687THF_VID_EN) & (1 << 2))) {
> > dev_dbg(&pdev->dev, "VID disabled, no VID function\n");
> > goto exit;
> > }
> >
> > /* Make sure the pins are configured for input */
> > - if (!(superio_inb(W83687THF_VID_CFG) & (1 << 4))) {
> > + if (!(superio_inb(sio_data, W83687THF_VID_CFG) & (1 << 4))) {
> > dev_dbg(&pdev->dev, "VID configured as output, "
> > "no VID function\n");
> > goto exit;
> > }
> >
> > - res = superio_inb(W83687THF_VID_DATA) & 0x3f;
> > + res = superio_inb(sio_data, W83687THF_VID_DATA) & 0x3f;
> >
> > exit:
> > - superio_exit();
> > + superio_exit(sio_data);
> > return res;
> > }
>
> BTW, all these changes would make a very nice preliminary patch, to
> make the main patch smaller and more readable.
>
> >
> > @@ -1783,94 +1695,20 @@ static struct w83627hf_data *w83627hf_update_device(struct device *dev)
> > return data;
> > }
> >
> > -static int __init w83627hf_device_add(unsigned short address,
> > - const struct w83627hf_sio_data *sio_data)
> > -{
> > - struct resource res = {
> > - .start = address + WINB_REGION_OFFSET,
> > - .end = address + WINB_REGION_OFFSET + WINB_REGION_SIZE - 1,
> > - .name = DRVNAME,
> > - .flags = IORESOURCE_IO,
> > - };
> > - int err;
> > -
> > - err = acpi_check_resource_conflict(&res);
> > - if (err)
> > - goto exit;
> > -
> > - pdev = platform_device_alloc(DRVNAME, address);
> > - if (!pdev) {
> > - err = -ENOMEM;
> > - printk(KERN_ERR DRVNAME ": Device allocation failed\n");
> > - goto exit;
> > - }
> > -
> > - err = platform_device_add_resources(pdev, &res, 1);
> > - if (err) {
> > - printk(KERN_ERR DRVNAME ": Device resource addition failed "
> > - "(%d)\n", err);
> > - goto exit_device_put;
> > - }
> > -
> > - err = platform_device_add_data(pdev, sio_data,
> > - sizeof(struct w83627hf_sio_data));
> > - if (err) {
> > - printk(KERN_ERR DRVNAME ": Platform data allocation failed\n");
> > - goto exit_device_put;
> > - }
> > -
> > - err = platform_device_add(pdev);
> > - if (err) {
> > - printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
> > - err);
> > - goto exit_device_put;
> > - }
> > -
> > - return 0;
> > -
> > -exit_device_put:
> > - platform_device_put(pdev);
> > -exit:
> > - return err;
> > -}
> > -
> > static int __init sensors_w83627hf_init(void)
> > {
> > - int err;
> > - unsigned short address;
> > - struct w83627hf_sio_data sio_data;
> > -
> > - if (w83627hf_find(0x2e, &address, &sio_data)
> > - && w83627hf_find(0x4e, &address, &sio_data))
> > - return -ENODEV;
> > -
> > - err = platform_driver_register(&w83627hf_driver);
> > - if (err)
> > - goto exit;
> > -
> > - /* Sets global pdev as a side effect */
> > - err = w83627hf_device_add(address, &sio_data);
> > - if (err)
> > - goto exit_driver;
> > -
> > - return 0;
> > -
> > -exit_driver:
> > - platform_driver_unregister(&w83627hf_driver);
> > -exit:
> > - return err;
> > + return platform_driver_register(&w83627hf_driver);
> > }
> >
> > static void __exit sensors_w83627hf_exit(void)
> > {
> > - platform_device_unregister(pdev);
> > platform_driver_unregister(&w83627hf_driver);
> > }
> >
> > MODULE_AUTHOR("Frodo Looijaard <frodol@xxxxxx>, "
> > "Philip Edelbrock <phil@xxxxxxxxxxxxx>, "
> > "and Mark Studebaker <mdsxyz123@xxxxxxxxx>");
> > -MODULE_DESCRIPTION("W83627HF driver");
> > +MODULE_DESCRIPTION("W83627HF hwmon driver");
> > MODULE_LICENSE("GPL");
> >
> > module_init(sensors_w83627hf_init);
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 491ac0f..b20d6e5 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -263,6 +263,21 @@ config EZX_PCAP
> > This enables the PCAP ASIC present on EZX Phones. This is
> > needed for MMC, TouchScreen, Sound, USB, etc..
> >
> > +config MFD_W83627HF
> > + tristate "Winbond W83627HF, W83627THF, W83637HF, W83687THF, W83697HF"
> > + depends on MFD_CORE
> > + help
> > + If you say yes here you add support for the Winbond W836X7 series
> > + of super-IO chips: the W83627HF, W83627THF, W83637HF, W83687THF and
> > + W83697HF to your platform.
> > +
> > + This is a multi functional device and this support defines a new
> > + platform device only. See other configuration submenus in order to
> > + enable the drivers of Winbond chip's functionalities.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called w83627hf-core.
> > +
> > endmenu
> >
> > menu "Multimedia Capabilities Port drivers"
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 6f8a9a1..1401ac9 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -27,6 +27,7 @@ obj-$(CONFIG_TWL4030_CORE) += twl4030-core.o twl4030-irq.o
> > obj-$(CONFIG_MFD_CORE) += mfd-core.o
> >
> > obj-$(CONFIG_EZX_PCAP) += ezx-pcap.o
> > +obj-$(CONFIG_MFD_W83627HF) += w83627hf-core.o
> >
> > obj-$(CONFIG_MCP) += mcp-core.o
> > obj-$(CONFIG_MCP_SA11X0) += mcp-sa11x0.o
> > diff --git a/drivers/mfd/w83627hf-core.c b/drivers/mfd/w83627hf-core.c
> > new file mode 100644
> > index 0000000..0fc0b6b
> > --- /dev/null
> > +++ b/drivers/mfd/w83627hf-core.c
> > @@ -0,0 +1,186 @@
> > +/*
> > + * w83627hf.c - platform device support
> > + * Copyright (c) 2009 Rodolfo Giometti <giometti@xxxxxxxx>
> > + *
> > + * Based on drivers/hwmon/w83627hf.c
> > + *
> > + * Original copyright note:
> > + * Copyright (c) 1998 - 2003 Frodo Looijaard <frodol@xxxxxx>,
> > + * Philip Edelbrock <phil@xxxxxxxxxxxxx>,
> > + * and Mark Studebaker <mdsxyz123@xxxxxxxxx>
> > + * Ported to 2.6 by Bernhard C. Schrenk <clemy@xxxxxxxxx>
> > + * Copyright (c) 2007 Jean Delvare <khali@xxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/err.h>
> > +#include <linux/mutex.h>
> > +#include <linux/ioport.h>
> > +#include <linux/acpi.h>
> > +#include <linux/io.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/w83627hf.h>
> > +
> > +static unsigned short force_id;
> > +module_param(force_id, ushort, 0);
> > +MODULE_PARM_DESC(force_id, "Override the detected device ID");
> > +
> > +/*
> > + * Devices definitions
> > + */
> > +
> > +static struct platform_device *pdev;
> > +
> > +static char *names[] = {
>
> Couldn't these be made const char *?
>
> > + "w83627hf",
> > + "w83627thf",
> > + "w83697hf",
> > + "w83637hf",
> > + "w83687thf",
> > +};
>
> Would probably be worth a note that this array must stay in sync with
> enum chips (and vice-versa.)
>
> > +
> > +static struct mfd_cell cells[] = {
> > + {
> > + .name = DRVNAME "_hwmon",
> > + },
> > +};
> > +
> > +#define W627_DEVID 0x52
> > +#define W627THF_DEVID 0x82
> > +#define W697_DEVID 0x60
> > +#define W637_DEVID 0x70
> > +#define W687THF_DEVID 0x85
> > +
> > +static int __init w83627hf_find(int sioaddr, 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;
> > + }
> > +
> > + err = 0;
> > + sio_data->name = names[sio_data->type];
> > +
> > + pr_info(DRVNAME ": Found %s chip at %#x\n", sio_data->name, sioaddr);
> > +
> > +exit:
> > + superio_exit(sio_data);
> > +
> > + return err;
> > +}
> > +
> > +static int __init w83627hf_device_add(const struct w83627hf_sio_data *sio_data)
> > +{
> > + int err;
> > +
> > + pdev = platform_device_alloc(DRVNAME, 0);
> > + if (!pdev) {
> > + err = -ENOMEM;
> > + printk(KERN_ERR DRVNAME ": Device allocation failed\n");
> > + goto exit;
> > + }
> > +
> > + err = platform_device_add_data(pdev, sio_data,
> > + sizeof(struct w83627hf_sio_data));
> > + if (err) {
> > + printk(KERN_ERR DRVNAME ": Platform data allocation failed\n");
> > + goto exit_device_put;
> > + }
> > +
> > + err = platform_device_add(pdev);
> > + if (err) {
> > + printk(KERN_ERR DRVNAME ": Device addition failed (%d)\n",
> > + err);
> > + goto exit_device_put;
> > + }
> > +
> > + err = mfd_add_devices(&pdev->dev, pdev->id, cells, ARRAY_SIZE(cells),
> > + 0, -1);
> > + if (err) {
> > + printk(KERN_ERR DRVNAME ": Cannot add sub devices (%d)\n",
> > + err);
> > + goto exit_device_unregister;
> > + }
> > +
> > + return 0;
> > +
> > +exit_device_unregister:
> > + platform_device_unregister(pdev);
> > +exit_device_put:
> > + platform_device_put(pdev);
> > +exit:
> > + return err;
> > +}
> > +
> > +static int __init w83627hf_init(void)
> > +{
> > + struct w83627hf_sio_data sio_data;
> > + int ret;
> > +
> > + mutex_init(&sio_data.lock);
> > +
> > + ret = w83627hf_find(0x2e, &sio_data);
> > + if (ret) {
> > + ret = w83627hf_find(0x4e, &sio_data);
> > + if (ret)
> > + return -ENODEV;
> > + }
> > +
> > + /* Sets global pdev as a side effect */
> > + return w83627hf_device_add(&sio_data);
> > +}
> > +
> > +static void __exit w83627hf_exit(void)
> > +{
> > + mfd_remove_devices(&pdev->dev);
> > + platform_device_unregister(pdev);
> > +}
> > +
> > +MODULE_AUTHOR("Rodolfo Giometti <giometti@xxxxxxxx>");
> > +MODULE_DESCRIPTION("W83627HF platform devices definitions");
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(w83627hf_init);
> > +module_exit(w83627hf_exit);
> > diff --git a/include/linux/mfd/w83627hf.h b/include/linux/mfd/w83627hf.h
> > new file mode 100644
> > index 0000000..5f4200b
> > --- /dev/null
> > +++ b/include/linux/mfd/w83627hf.h
> > @@ -0,0 +1,118 @@
> > +/*
> > + * w83627hf.h - platform device support, header file
> > + * Copyright (c) 2009 Rodolfo Giometti <giometti@xxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> > + */
> > +
> > +#include <linux/mutex.h>
> > +#include <linux/io.h>
> > +
> > +#define DRVNAME "w83627hf"
> > +
> > +enum chips {
> > + w83627hf,
> > + w83627thf,
> > + w83697hf,
> > + w83637hf,
> > + w83687thf
> > +};
> > +
> > +struct w83627hf_sio_data {
> > + int sioaddr;
> > + enum chips type;
> > + char *name;
> > +
> > + struct mutex lock;
> > +};
> > +
> > +/* logical device numbers for superio_select (below) */
> > +#define W83627HF_LD_FDC 0x00
> > +#define W83627HF_LD_PRT 0x01
> > +#define W83627HF_LD_UART1 0x02
> > +#define W83627HF_LD_UART2 0x03
> > +#define W83627HF_LD_KBC 0x05
> > +#define W83627HF_LD_CIR 0x06 /* w83627hf only */
> > +#define W83627HF_LD_GAME 0x07
> > +#define W83627HF_LD_MIDI 0x07
> > +#define W83627HF_LD_GPIO1 0x07
> > +#define W83627HF_LD_GPIO5 0x07 /* w83627thf only */
> > +#define W83627HF_LD_GPIO2 0x08
> > +#define W83627HF_LD_GPIO3 0x09
> > +#define W83627HF_LD_GPIO4 0x09 /* w83627thf only */
> > +#define W83627HF_LD_ACPI 0x0a
> > +#define W83627HF_LD_HWM 0x0b
> > +
> > +#define W83627THF_GPIO5_EN 0x30 /* w83627thf only */
> > +#define W83627THF_GPIO5_IOSR 0xf3 /* w83627thf only */
> > +#define W83627THF_GPIO5_DR 0xf4 /* w83627thf only */
> > +
> > +#define W83687THF_VID_EN 0x29 /* w83687thf only */
> > +#define W83687THF_VID_CFG 0xF0 /* w83687thf only */
> > +#define W83687THF_VID_DATA 0xF1 /* w83687thf only */
> > +
> > +/*
> > + * Common configuration registers access functions.
> > + *
> > + * These registers are special and they must me accessed by using a well
> > + * specified protocol. Client drivers __must__ do as follow in order to
> > + * get access correctly to these registers:
> > + *
> > + * superio_enter()
> > + *
> > + * superio_select()/superio_outb()/superio_inb()
> > + *
> > + * superio_exit();
> > + *
> > + */
> > +
> > +static inline void superio_enter(struct w83627hf_sio_data *sio)
> > +{
> > + mutex_lock(&sio->lock);
> > +
> > + outb(0x87, sio->sioaddr);
> > + outb(0x87, sio->sioaddr);
> > +}
> > +
> > +static inline void superio_select(struct w83627hf_sio_data *sio, int ld)
> > +{
> > + WARN_ON(!mutex_is_locked(&sio->lock));
> > +
> > + outb(0x07, sio->sioaddr);
> > + outb(ld, sio->sioaddr + 1);
> > +}
> > +
> > +static inline void superio_outb(struct w83627hf_sio_data *sio, int reg, int val)
> > +{
> > + WARN_ON(!mutex_is_locked(&sio->lock));
> > +
> > + outb(reg, sio->sioaddr);
> > + outb(val, sio->sioaddr + 1);
> > +}
> > +
> > +static inline int superio_inb(struct w83627hf_sio_data *sio, int reg)
> > +{
> > + WARN_ON(!mutex_is_locked(&sio->lock));
> > +
> > + outb(reg, sio->sioaddr);
> > + return inb(sio->sioaddr + 1);
> > +}
> > +
> > +static inline void superio_exit(struct w83627hf_sio_data *sio)
> > +{
You should add a WARN_ON(!mutex_is_locked(&sio->lock)); here as well.
> > + outb(0xAA, sio->sioaddr);
> > +
> > + mutex_unlock(&sio->lock);
> > +}
>
> The rest starts looking good.
Same here. As far as the MFD part is concerned, it's fine with me except for
this one missing WARN_ON().
I'll wait until we get Jean's ack for the hwmon part, and push it through my
for-next branch.
Cheers,
Samuel.
> --
> Jean Delvare
--
Intel Open Source Technology Centre
http://oss.intel.com/
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
