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    [W:0.028 / U:2.500 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site