Messages in this thread | | | Date | Mon, 28 Jun 2010 01:55:16 +0200 | From | Samuel Ortiz <> | Subject | Re: [PATCHv2 1/3] mfd: add STMPE I/O Expander support |
| |
Hi Rabin,
On Tue, Jun 22, 2010 at 07:25:27PM +0530, Rabin Vincent wrote: > add support for the stmpe family of i/o expanders from > stmicroelectronics. these devices include upto 24 gpios and a varying > selection of blocks, including pwm, keypad, and touchscreen controllers. > this patch adds the mfd core.
Thanks for re-spinning this patchset. I have some comments though:
> diff --git a/drivers/mfd/kconfig b/drivers/mfd/kconfig > index 9da0e50..63dce71 100644 > --- a/drivers/mfd/kconfig > +++ b/drivers/mfd/kconfig > @@ -177,6 +177,18 @@ config twl4030_codec > select mfd_core > default n > > +config mfd_stmpe > + bool "support stmicroelectronics stmpe" > + depends on i2c=y && generic_hardirqs > + select mfd_core > + help > + support for the stmpe family of i/o expanders from > + stmicroelectronics. Please be more verbose here in saying which chipsets are supported. Also, describing the kind of sub devices they provide won't hurt.
> diff --git a/drivers/mfd/stmpe-devices.c b/drivers/mfd/stmpe-devices.c > new file mode 100644 > index 0000000..fa6934d > --- /dev/null > +++ b/drivers/mfd/stmpe-devices.c I like the smtpe_variant design, but I'd rather see those definitions being part of smtpe.c directly.
> +/** > + * stmpe_reg_read() - read a single stmpe register > + * @stmpe: device to read from > + * @reg: register to read > + */ > +int stmpe_reg_read(struct stmpe *stmpe, u8 reg) > +{ > + int ret; > + > + ret = i2c_smbus_read_byte_data(stmpe->i2c, reg); > + if (ret < 0) > + dev_err(stmpe->dev, "failed to read reg %#x: %d\n", > + reg, ret); > + > + dev_vdbg(stmpe->dev, "rd: reg %#x => data %#x\n", reg, ret); > + > + return ret; > +} > +export_symbol(stmpe_reg_read); I think your locking is broken here. If your exporting this routine (and the next ones below), you'd better make sure you're under stmpe->lock for the stmpe register concurrent access. What I suggest is that you'd have the exported routines taking your stmpe lock, and then have an internal version (e.g. named with a __stmpe prefix) without lock taken for your core code. In your case, you could probably call the i2c I/O routines directly, that's up to you.
> +/** > + * stmpe_set_bits() - set the value of a bitfield in a stmpe register > + * @stmpe: device to write to > + * @reg: register to write > + * @mask: mask of bits to set > + * @val: value to set > + */ > +int stmpe_set_bits(struct stmpe *stmpe, u8 reg, u8 mask, u8 val) > +{ > + int ret; > + > + mutex_lock(&stmpe->lock); > + > + ret = stmpe_reg_read(stmpe, reg); That one for example would be __stmpe_read().
> +/** > + * stmpe_block_write() - write multiple stmpe registers > + * @stmpe: device to write to > + * @reg: first register > + * @length: number of registers > + * @values: values to write > + */ > +int stmpe_block_write(struct stmpe *stmpe, u8 reg, u8 length, > + const u8 *values) > +{ > + int ret; > + > + dev_vdbg(stmpe->dev, "wr: regs %#x (%d)\n", reg, length); > +#ifdef VERBOSE_DEBUG > + print_hex_dump_bytes("stmpe wr: ", dump_prefix_offset, values, length); > +#endif I don't really enjoy this part for 2 reasons: - You should use a less generic ifdef switch, prefixed with STMPE_ for example. - I'd rather see a #ifdef STMPE_VERBOSE_DEBUG #define stmpe_hex_xump() print_hex_dump_bytes() #else #define stmpe_hex_xump() #endif defined in your stmpe.h.
The rest looks fine to me.
Cheers, Samuel.
-- intel open source technology centre http://oss.intel.com/
| |