Re: [PATCH] i2c: Floppy controller bus driver

[Herbert Poetzl <herbert-dBHVzrDq9nF4Lj/PQRBjDg@public.gmane.org>]

On Sun, Aug 17, 2008 at 10:45:11PM +0200, Jean Delvare wrote:
> Hi Herbert,
> 
> On Fri, 15 Aug 2008 22:50:12 +0200, Herbert Poetzl wrote:
> > 
> > Greetings!
> > 
> > I recently got the idea to (re)use the (at least for me)
> > very often unused floppy connector to attach some hardware
> > to monitor system temperature and control some fans
> > 
> > the basic idea is to connect one of the Motor Control
> > lines with the Disk Changed input to form the SDA line,
> > and the other Motor Control line to control the SCL
> > 
> > this works surprisingly well, as the Motor Control lines
> > are open drain, and the Disk Changed input is CMOS/TTL
> > (depending on the controller)
> > 
> > here is the tested patch for 2.6.27-rc3 with configurable
> > floppy base address, but hardcoded Motor signals
> 
> That's fine with me, as long as the default address is 
> somewhat standard.

yep, see below ...

> > please consider for (mainline) inclusion!
> > 
> > TIA,
> > Herbert
> > 
> > 
> > diff -NurpP --minimal linux-2.6.27-rc3/drivers/i2c/busses/i2c-floppy.c linux-2.6.27-rc3-fi2c-v0.1/drivers/i2c/busses/i2c-floppy.c
> > --- linux-2.6.27-rc3/drivers/i2c/busses/i2c-floppy.c	1970-01-01 01:00:00.000000000 +0100
> > +++ linux-2.6.27-rc3-fi2c-v0.1/drivers/i2c/busses/i2c-floppy.c	2008-08-15 21:49:26.000000000 +0200
> > @@ -0,0 +1,271 @@
> > +/* ------------------------------------------------------------------------ *
> > + * i2c-floppy.c I2C bus over floppy controller                              *
> > + * ------------------------------------------------------------------------ *
> > +   Copyright (C) 2008 Herbert Poetzl <herbert-dBHVzrDq9nF4Lj/PQRBjDg@public.gmane.org>
> > +
> > +   Somewhat based on i2c-parport-light.c driver
> > +   Copyright (C) 2003-2007 Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/ioport.h>
> > +#include <linux/i2c.h>
> > +#include <linux/i2c-algo-bit.h>
> > +#include <asm/io.h>
> > +
> > +
> > +#ifndef I2C_HW_B_FLOPPY
> > +#define I2C_HW_B_FLOPPY         0x010001 /* Floppy controller */
> > +#endif
> 
> As discussed on IRC, these driver IDs are optional and will be removed
> soon. So please don't define a new one.
> 
> > +
> > +static struct platform_device *pdev;
> > +static unsigned char dor = 0;
> > +
> > +static u16 base = 0;
> 
> Do not initialize static globals to 0 - the compiler does it for you.
> BTW, please run scripts/checkpatch.pl on your patch and fix the errors
> and warnings.

okay, will do, thanks for the hint

> > +module_param(base, ushort, 0);
> > +MODULE_PARM_DESC(base, "Base I/O address");
> > +
> > +
> > +
> > +#define DEFAULT_BASE	0x3F0
> 
> Is this address any standard?

yep, it is the default address for PC floppy controllers

http://www.pcguide.com/ref/fdd/confUsage-c.html

> > +#define DRVNAME		"i2c-floppy"
> > +
> > +
> > +#define FOFF_DOR	0x02
> > +#define FOFF_DIR	0x07
> > +
> > +#define FDOR_MOTEA	0x10
> > +#define FDOR_MOTEB	0x20
> > +
> > +#define FDIR_DCHNG	0x80
> > +
> > +#define SCL		FDOR_MOTEA
> > +#define SDA		FDOR_MOTEB
> > +#define SDA_IN		FDIR_DCHNG
> > +
> > +#define LO_INV		(SDA|SCL)
> > +#define LI_INV		(SDA_IN)
> > +
> > +
> > +
> > +

> Fewer blank lines wouldn't hurt.

k, no problem ...

> > +/* ----- Device list ------------------------------------------------------ */
> > +
> > +struct i2c_floppy_data {
> > +	unsigned char dor;
> > +};
> 
> That's a bit confusing. You define a structure for private data (which
> is good) but apparently the driver code instead accesses a global
> variable?

yep, sorry, is a leftover from the previous version
which was more like the i2c-parport driver than the
i2c-parport-light, will remove it for now ...

> > +
> > +struct i2c_floppy {
> > +	struct i2c_adapter adapter;
> > +	struct i2c_algo_bit_data algo_data;
> > +	struct i2c_floppy_data data;
> > +	struct i2c_floppy *next;
> 
> Your driver doesn't actually support multiple devices, so this last
> pointer is useless.

correct, another leftover ...

> Hmm, looking at the rest of the driver code, you don't actually use
> this structure anywhere? This definitely needs some cleanup.
> 
> > +};
> > +
> > +
> > +/* ----- Low-level floppy access ------------------------------------------ */
> > +
> > +static void port_dor_out(unsigned char d)
> > +{
> > +	outb(d ^ LO_INV, base + FOFF_DOR);
> > +}
> > +
> > +static unsigned char port_dir_in(void)
> > +{
> > +	return inb(base + FOFF_DIR) ^ LI_INV;
> > +}
> 
> These two functions are certainly worth inlining.

okay, static inline fine?

> > +
> > +
> > +/* ----- I2C algorithm call-back functions and structures ----------------- */
> > +
> > +static void floppy_setscl(void *data, int state)
> > +{
> > +	if (state)
> > +		dor |= SCL;
> > +	else
> > +		dor &= ~SCL;
> > +
> > +	port_dor_out(dor);
> > +}
> > +
> > +static void floppy_setsda(void *data, int state)
> > +{
> > +	if (state)
> > +		dor |= SDA;
> > +	else
> > +		dor &= ~SDA;
> > +
> > +	port_dor_out(dor);
> > +}
> > +
> > +static int floppy_getsda(void *data)
> > +{
> > +	return port_dir_in() & SDA_IN;
> > +}
> > +
> > +
> > +/* Encapsulate the functions above in the correct structure
> > +   Note that getscl will be set to NULL by the attaching code for adapters
> > +   that cannot read SCL back */
> 
> You must have copied this comment from another driver... but it doesn't
> apply to yours.

yep .. will clean that up

> > +static struct i2c_algo_bit_data floppy_algo_data = {
> > +	.setsda		= floppy_setsda,
> > +	.setscl		= floppy_setscl,
> > +	.getsda		= floppy_getsda,
> > +	.getscl		= NULL,
> 
> No need to initialize to NULL explicitly, the compiler does it for you.

yes, I know, I just wanted to keep it there to remind me
that scl cannot be read back atm

> > +	.udelay		= 50,
> > +	.timeout	= HZ,
> > +};
> > +
> > +
> > +
> > +/* ----- Driver registration ---------------------------------------------- */
> > +
> > +static struct i2c_adapter floppy_adapter = {
> > +	.owner		= THIS_MODULE,
> > +	.class		= I2C_CLASS_HWMON,
> > +	.id		= I2C_HW_B_FLOPPY,
> 
> Drop.

k, will do

> > +	.algo_data	= &floppy_algo_data,
> > +	.name		= "Floppy controller adapter",
> > +};
> > +
> > +static int __devinit i2c_floppy_probe(struct platform_device *pdev)
> > +{
> > +	int err;
> > +	struct resource *res;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > +	if (!request_region(res->start, res->end - res->start + 1, DRVNAME))
> > +		return -EBUSY;
> > +
> > +	/* Reset hardware to a sane state (SCL and SDA high) */
> > +	floppy_setsda(NULL, 1);
> > +	floppy_setscl(NULL, 1);
> > +
> > +	floppy_adapter.dev.parent = &pdev->dev;
> > +	err = i2c_bit_add_bus(&floppy_adapter);
> > +	if (err) {
> > +		dev_err(&pdev->dev, "Unable to register with I2C\n");
> > +		goto exit_region;
> > +	}
> > +	return 0;
> > +
> > +exit_region:
> > +	release_region(res->start, res->end - res->start + 1);
> > +	return err;
> > +}
> > +
> > +static int __devexit i2c_floppy_remove(struct platform_device *pdev)
> > +{
> > +	struct resource *res;
> > +
> > +	i2c_del_adapter(&floppy_adapter);
> 
> In general, "remove" functions should avoid accessing globals. You
> should always be able to get access to everything you need based on the
> device that is passed into parameter. Just use platform_get_drvdata
> (and use platform_set_drvdata in the "probe" function.)

hmm, is that something you plan to change in the
i2c-parport-light driver too, if so, please point
me to the patch, I'll gladly adjust it to match that

> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
> > +	release_region(res->start, res->end - res->start + 1);
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver i2c_floppy_driver = {
> > +	.driver = {
> > +		.owner	= THIS_MODULE,
> > +		.name	= DRVNAME,
> > +	},
> > +	.probe		= i2c_floppy_probe,
> > +	.remove		= __devexit_p(i2c_floppy_remove),
> > +};
> > +
> > +static int __init i2c_floppy_device_add(u16 address)
> > +{
> > +	struct resource res = {
> > +		.start	= address,
> > +		.end	= address + 7,
> > +		.name	= DRVNAME,
> > +		.flags	= IORESOURCE_IO,
> > +	};
> > +	int err;
> > +
> > +	pdev = platform_device_alloc(DRVNAME, -1);
> > +	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(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 i2c_floppy_init(void)
> > +{
> > +	int err;
> > +
> > +	if (base == 0) {
> > +		pr_info(DRVNAME ": using default base 0x%x\n", DEFAULT_BASE);
> > +		base = DEFAULT_BASE;
> > +	}
> > +
> > +	/* Sets global pdev as a side effect */
> > +	err = i2c_floppy_device_add(base);
> > +	if (err)
> > +		goto exit;
> > +
> > +	err = platform_driver_register(&i2c_floppy_driver);
> > +	if (err)
> > +		goto exit_device;
> > +
> > +	return 0;
> > +
> > +exit_device:
> > +	platform_device_unregister(pdev);
> > +exit:
> > +	return err;
> > +}
> > +
> > +static void __exit i2c_floppy_exit(void)
> > +{
> > +	platform_driver_unregister(&i2c_floppy_driver);
> > +	platform_device_unregister(pdev);
> > +}
> > +
> > +
> > +MODULE_AUTHOR("Herbert Poetzl <herbert-dBHVzrDq9nF4Lj/PQRBjDg@public.gmane.org>");
> > +MODULE_DESCRIPTION("I2C bus over floppy controller");
> > +MODULE_LICENSE("GPL");
> > +
> > +module_init(i2c_floppy_init);
> > +module_exit(i2c_floppy_exit);
> > diff -NurpP --minimal linux-2.6.27-rc3/drivers/i2c/busses/Kconfig linux-2.6.27-rc3-fi2c-v0.1/drivers/i2c/busses/Kconfig
> > --- linux-2.6.27-rc3/drivers/i2c/busses/Kconfig	2008-08-15 21:19:24.000000000 +0200
> > +++ linux-2.6.27-rc3-fi2c-v0.1/drivers/i2c/busses/Kconfig	2008-08-15 21:51:33.000000000 +0200
> > @@ -564,6 +564,17 @@ config I2C_TINY_USB
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called i2c-tiny-usb.
> >  
> > +config I2C_FLOPPY
> 
> Right subsection, but please follow the alphabetical order.

okay, will do

> > +	tristate "Floppy controller adapter"
> > +	select I2C_ALGOBIT
> 
> You probably want to add "default n", as most users will never need
> this driver.

yep, that's correct ... for now :)

> > +	help
> > +	  This supports floppy controller I2C adapters using the motor
> > +	  control lines for SDA and SCL, and the drive change for SDA
> > +	  readback.
> > +
> > +	  This support is also available as a module.  If so, the module
> > +	  will be called i2c-floppy.
> 
> This lacks a warning that this driver is meant for do-it-yourself
> hardware and most people can safely say no.
> 
> Out of curiosity, what would happen if someone was to run this driver
> with actual floppy disk drives connected?

I'd say the drive(s) will spin up a little, but
that's about all that can happen, the driver doesn't
do anything 'bad' with the floppy controller

the other way round is probably more a problem, as
the floppy driver will not handle the changing input
line that gracefully, but that is a problem users
of this kind of hardware should be able to deal with

> > +
> >  comment "Graphics adapter I2C/DDC channel drivers"
> >  	depends on PCI
> >  
> > diff -NurpP --minimal linux-2.6.27-rc3/drivers/i2c/busses/Makefile linux-2.6.27-rc3-fi2c-v0.1/drivers/i2c/busses/Makefile
> > --- linux-2.6.27-rc3/drivers/i2c/busses/Makefile	2008-08-15 21:19:24.000000000 +0200
> > +++ linux-2.6.27-rc3-fi2c-v0.1/drivers/i2c/busses/Makefile	2008-08-15 21:43:03.000000000 +0200
> > @@ -52,6 +52,7 @@ obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport
> >  obj-$(CONFIG_I2C_PARPORT_LIGHT)	+= i2c-parport-light.o
> >  obj-$(CONFIG_I2C_TAOS_EVM)	+= i2c-taos-evm.o
> >  obj-$(CONFIG_I2C_TINY_USB)	+= i2c-tiny-usb.o
> > +obj-$(CONFIG_I2C_FLOPPY)	+= i2c-floppy.o
> 
> Alphabetical order please.

k, consider it done

> >  
> >  # Graphics adapter I2C/DDC channel drivers
> >  obj-$(CONFIG_I2C_VOODOO3)	+= i2c-voodoo3.o
> > diff -NurpP --minimal linux-2.6.27-rc3/include/linux/i2c-id.h linux-2.6.27-rc3-fi2c-v0.1/include/linux/i2c-id.h
> > --- linux-2.6.27-rc3/include/linux/i2c-id.h	2008-08-15 21:20:04.000000000 +0200
> > +++ linux-2.6.27-rc3-fi2c-v0.1/include/linux/i2c-id.h	2008-08-15 21:42:14.000000000 +0200
> > @@ -91,6 +91,7 @@
> >  
> >  /* --- Bit algorithm adapters						*/
> >  #define I2C_HW_B_LP		0x010000 /* Parallel port Philips style */
> > +#define I2C_HW_B_FLOPPY		0x010001 /* Floppy controller */
> >  #define I2C_HW_B_BT848		0x010005 /* BT848 video boards */
> >  #define I2C_HW_B_VIA		0x010007 /* Via vt82c586b */
> >  #define I2C_HW_B_HYDRA		0x010008 /* Apple Hydra Mac I/O */
> 
> Your patch misses Documentation/i2c/busses/i2c-floppy explaining 
> how to build the hardware, things to pay attention to, etc.

okay, something like the i2c-parport-light is fine?
(including the pinout description of course)

thanks for the feedback,
Herbert

PS: expect an updated version soon ...

> -- 
> Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c