Hi Rodolfo,
Some comments on the MFD related parts:
On Fri, Sep 18, 2009 at 02:09:23PM +0200, Rodolfo Giometti wrote:
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2d50166..bc7058f 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"
> + depends on 83627HF
Use select here, as the hwmon W83627HF current users are not really supposed
to know that they have to go an manually select an obscure MFD core for their
driver to build.
Moreover, the symbol is MFD_W83627HF, not 83627HF.
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 491ac0f..784a892 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -263,6 +263,20 @@ 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"
You must depend on MFD_CORE here.
> + 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..39b6190
> --- /dev/null
> +++ b/drivers/mfd/w83627hf-core.c
> @@ -0,0 +1,236 @@
> +/*
> + * 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 u16 force_hwmon_addr;
> +module_param(force_hwmon_addr, ushort, 0);
> +MODULE_PARM_DESC(force_hwmon_addr,
> + "Initialize the base address of the hwmon sensors");
> +
> +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[] = {
> + "w83627hf",
> + "w83627thf",
> + "w83697hf",
> + "w83637hf",
> + "w83687thf",
> +};
> +
> +static struct resource hwmon_res = {
> + /* .start = ? - NOTE: set at runtime */
> + /* .end = ? - NOTE: set at runtime */
> + .name = DRVNAME "_hwmon",
> + .flags = IORESOURCE_IO,
> +};
> +
> +static struct mfd_cell cells[] = {
> + {
> + .name = DRVNAME "_hwmon",
> + .num_resources = 1,
> + .resources = &hwmon_res,
> + },
> +};
> +
> +/*
> + * Local functions
> + */
Local functions ?
> +
> +#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.
> +/* 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.
> +#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.
> + exit:
> + superio_exit(sio_data);
> + return err;
> +}
> +
> +static int __init w83627hf_device_add(unsigned short address,
> + const struct w83627hf_sio_data *sio_data)
> +{
> + int err;
> +
> + hwmon_res.start = address + WINB_REGION_OFFSET;
> + hwmon_res.end = address + WINB_REGION_OFFSET + WINB_REGION_SIZE - 1;
> +
> + pdev = platform_device_alloc(DRVNAME, address);
> + 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),
> + (struct resource *) (address & 0xffff), -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)
> +{
> + unsigned short address;
> + struct w83627hf_sio_data sio_data;
> +
> + mutex_init(&sio_data.lock);
> +
> + if (w83627hf_find(0x2e, &address, &sio_data)
> + && w83627hf_find(0x4e, &address, &sio_data))
> + return -ENODEV;
> +
> + /* Sets global pdev as a side effect */
> + return w83627hf_device_add(address, &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..c9f537d
> --- /dev/null
> +++ b/include/linux/mfd/w83627hf.h
> @@ -0,0 +1,112 @@
> +/*
> + * 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);
I'm not really happy with this one. The unbalanced locking here makes me feel
unconfortable.
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.
> +}
> +
> +static inline void superio_select(struct w83627hf_sio_data *sio, int ld)
> +{
> + outb(0x07, sio->sioaddr);
> + outb(ld, sio->sioaddr + 1);
> +}
> +
> +static inline void superio_outb(struct w83627hf_sio_data *sio, int reg, int val)
> +{
> + outb(reg, sio->sioaddr);
> + outb(val, sio->sioaddr + 1);
> +}
> +
> +static inline int superio_inb(struct w83627hf_sio_data *sio, int reg)
> +{
> + outb(reg, sio->sioaddr);
> + return inb(sio->sioaddr + 1);
> +}
> +
> +static inline void superio_exit(struct w83627hf_sio_data *sio)
> +{
> + outb(0xAA, sio->sioaddr);
> +
> + mutex_unlock(&sio->lock);
> +}
>
>
> --
>
> GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
> Linux Device Driver giometti@xxxxxxxx
> Embedded Systems phone: +39 349 2432127
> UNIX programming skype: rodolfo.giometti
> Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
--
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
