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

[Rodolfo Giometti <giometti@enneenne.com>]

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

On Thu, Sep 17, 2009 at 04:34:45PM +0200, Samuel Ortiz wrote:
> On Thu, Sep 17, 2009 at 03:34:50PM +0200, Jean Delvare wrote:
> 
> > > @@ -1532,37 +1381,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;
> > >  }
> > 
> > This looks wrong. The whole point of the MFD infrastructure is to
> > control who is accessing the common registers. Here you let the hwmon
> > driver access the Super-I/O configuration registers without any kind of
> > synchronisation. What if another user of w83627hf-core does the same at
> > the same moment?
> > 
> > You need a way to protect common registers. You'll need a mutex, for
> > sure. Then you can either export this mutex and count on all "children"
> > drivers' cooperation to properly request and release it. Or you can
> > move all the code accessing the common registers into w83627hf-core,
> > and stop exporting the superio_* functions. I don't know if there is a
> > recommended practice for MFD drivers. Samuel?
> The recommended practice is definitely to have your register accessing
> routines defined from the MFD core. That's what all MFD drivers do.

Looking at chip's documentation I suppose I have to add a mutex and
then count on all "children" drivers' cooperation to properly request
and release the configuration register access. I cannot define
functions to manage such registers since a children may do several
reads/writes to complish its task.

> > > +	  This driver can also be built as a module.  If so, the module
> > > +	  will be called w83627hf-core.
> > > +
> > 
> > Shouldn't this option be selected automatically by the w83627hf (hwmon)
> > driver? Currently it is possible to select only the hwmon driver, but
> > that driver will never work because the required platform driver won't
> > be created. This is a problem for current users upgrading to a new
> > kernel, as CONFIG_MFD_W83627HF defaults to N.
> > 
> > But then again I don't know if there is already a policy regarding this
> > for MFD drivers. Samuel?
> If your subdevice driver is calling some of the MFD core routines, or if
> there's a platform dependency, then your subdevice driver (w83627hf in that
> case) Kconfig should depend on MFD_W83627HF.

Ok!

Thanks for your suggestions,

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