lkml.org 
[lkml]   [2010]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/


\
 
 \ /
  Last update: 2010-06-28 01:57    [from the cache]
©2003-2011 Jasper Spaans