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.
>
> 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 <[EMAIL PROTECTED]>
> +
> + Somewhat based on i2c-parport-light.c driver
> + Copyright (C) 2003-2007 Jean Delvare <[EMAIL PROTECTED]>
> +
> + 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.
> +module_param(base, ushort, 0);
> +MODULE_PARM_DESC(base, "Base I/O address");
> +
> +
> +
> +#define DEFAULT_BASE 0x3F0
Is this address any standard?
> +#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.
> +/* ----- 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?
> +
> +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.
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.
> +
> +
> +/* ----- 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.
> +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.
> + .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.
> + .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.)
> +
> + 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 <[EMAIL PROTECTED]>");
> +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.
> + tristate "Floppy controller adapter"
> + select I2C_ALGOBIT
You probably want to add "default n", as most users will never need
this driver.
> + 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?
> +
> 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.
>
> # 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.
--
Jean Delvare
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
