Re: [lm-sensors] [PATCH] hwmon w83627hf: add mfd support.

[Rodolfo Giometti <giometti@enneenne.com>]

[-- Attachment #1.1: Type: text/plain, Size: 16183 bytes --]

On Fri, Sep 18, 2009 at 04:42:49PM +0200, Samuel Ortiz wrote:
> 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.

Fixed.

> > 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.

Fixed.

> > +	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@linux.it>
> > + *
> > + *  Based on drivers/hwmon/w83627hf.c
> > + *
> > + *  Original copyright note:
> > + *    Copyright (c) 1998 - 2003  Frodo Looijaard <frodol@dds.nl>,
> > + *    Philip Edelbrock <phil@netroedge.com>,
> > + *    and Mark Studebaker <mdsxyz123@yahoo.com>
> > + *    Ported to 2.6 by Bernhard C. Schrenk <clemy@clemy.org>
> > + *    Copyright (c) 2007  Jean Delvare <khali@linux-fr.org>
> > + *
> > + *    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 ?

Fixed.

> > +
> > +#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.

Fixed.

> > +/* 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.

Fixed.

> > +#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.

Fixed.

> > + 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@linux.it>");
> > +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@linux.it>
> > + *
> > + *  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.

This is not true for all sub-devices. GPIOs sub-device, for instance,
uses such functions to reads/writes data.

> 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.

Unluckely this not possible. Once we do superio_enter() we should be
able to do several reads and writes whose are not easily fixed in the
above function.

Clients devices must have the possibility to mix several calls of
superio_outb()/superio_inb(), so device drivers must use them properly.

> > +}
> > +
> > +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);
> > +}

I'll repropose a new patch ASAP.

Thanks,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors