linux-kernel - Re: [PATCH] Bitbanging i2c bus driver using the GPIO API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 9 Mar 2007 11:30:12 -0800
From:	David Brownell <david-b@...bell.net>
To:	Haavard Skinnemoen <hskinnemoen@...el.com>
Cc:	Jean Delvare <khali@...ux-fr.org>, Bryan Wu <bryan.wu@...log.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Deepak Saxena <dsaxena@...xity.net>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Bitbanging i2c bus driver using the GPIO API

On Friday 09 March 2007 10:48 am, Haavard Skinnemoen wrote:
> This is a very simple bitbanging i2c bus driver utilizing the new
> arch-neutral GPIO API. Useful for chips that don't have a built-in
> i2c controller, additional i2c busses, or testing purposes.

That's the right idea!  But remember that not all GPIOs support
reading back the actual value on SCL (it's an OUT pin, so lacking
multidrive capability the values "should" be what you wrote), so
getscl() support should depend on a flag in platform data.  In
the same vein, if SCL is an output-only pin, you won't be able
to change its direction ... but then, I'm not sure why you were
changing its direction in setscl() rather than just its value.

I2C has another interesting special case.  at91_set_multi_drive()
would be appropriate (yes?) for ARCH_AT91 to use on SCL, to best
support both clock stretching and multi-master configurations.

> +	gpio_direction_input(pdata->sda_pin);
> +	gpio_direction_input(pdata->scl_pin);
> +	gpio_set_value(pdata->sda_pin, 0);
> +	gpio_set_value(pdata->scl_pin, 0);

Surely you mean "output" in both cases.  So you can set the
value.  Setting the value on an input pin is undefined.  ;)

> +	printk(KERN_INFO "i2c-gpio: using pins 0x%x (sda) 0x%x (scl)\n",
> +	       pdata->sda_pin, pdata->scl_pin);

Please, no hex there.  I think dev_info() would be better; and it
might be nice to report whether clock stretching is supported.

> --- a/include/linux/i2c-id.h
> +++ b/include/linux/i2c-id.h
> @@ -194,6 +194,7 @@
>  #define I2C_HW_B_EM28XX		0x01001f /* em28xx video capture cards */
>  #define I2C_HW_B_CX2341X	0x010020 /* Conexant CX2341X MPEG encoder cards */
>  #define I2C_HW_B_INTELFB	0x010021 /* intel framebuffer driver */
> +#define I2C_HW_B_GPIO		0x010022 /* Generic GPIO-based driver */

It'd be nice to completely abolish those IDs, starting by not
adding new ones.  Especially, not adding unused ones!

- Dave
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/