lkml.org 
[lkml]   [2018]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver
    On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@st.com> wrote:

    Thanks for working on this complex expander driver.
    It is a bit daunting. Sorry if there are lots of comments and
    considerations, but it reflects the complexity of the hardware.

    > +enum mfx_block {
    > + MFX_BLOCK_GPIO = BIT(0),
    > + MFX_BLOCK_TS = BIT(1),
    > + MFX_BLOCK_IDD = BIT(2),
    > + MFX_BLOCK_ALTGPIO = BIT(3),
    > +};

    This looks suspiciously similar to this:

    enum stmpe_block {
    STMPE_BLOCK_GPIO = 1 << 0,
    STMPE_BLOCK_KEYPAD = 1 << 1,
    STMPE_BLOCK_TOUCHSCREEN = 1 << 2,
    STMPE_BLOCK_ADC = 1 << 3,
    STMPE_BLOCK_PWM = 1 << 4,
    STMPE_BLOCK_ROTATOR = 1 << 5,
    };

    Apparently the same hardware designers are involved.

    > +int mfx_reg_write(struct mfx *mfx, u8 reg, u8 data);
    > +int mfx_reg_read(struct mfx *mfx, u8 reg);
    > +int mfx_block_read(struct mfx *mfx, u8 reg, u8 length, u8 *values);
    > +int mfx_block_write(struct mfx *mfx, u8 reg, u8 length, const u8 *values);
    > +int mfx_set_bits(struct mfx *mfx, u8 reg, u8 mask, u8 val);

    Do you need this? Can't you just use regmap and pass
    around a struct regmap *map to access registers?

    You don't necessarily need to use the default I2C regmap
    (like, e.g. drivers/mfd/stw481x.c) but even if a more
    complex access pattern is used to read/write registers
    I am pretty sure you can use regmap for it.

    > +int mfx_enable(struct mfx *mfx, unsigned int blocks);
    > +int mfx_disable(struct mfx *mfx, unsigned int blocks);

    This is similar to
    extern int stmpe_enable(struct stmpe *stmpe, unsigned int blocks);
    extern int stmpe_disable(struct stmpe *stmpe, unsigned int blocks);

    So again I am suspicious about duplication of driver code.

    It even looks a bit like this driver started as a copy of
    the STMPE driver, which is not a good sign. It might be
    that it was copied from there because the hardware is
    actually very similar.

    > +/* General */
    > +#define MFX_REG_CHIP_ID 0x00 /* R */
    > +#define MFX_REG_FW_VERSION_MSB 0x01 /* R */
    > +#define MFX_REG_FW_VERSION_LSB 0x02 /* R */
    > +#define MFX_REG_SYS_CTRL 0x40 /* RW */

    The STMPE driver defines enumerated registers in
    include/linux/mfd/stmpe.h
    then assign each variant using the model specifics in
    drivers/mfd/stmpe.h

    This doesn't seem super much different.

    Even if the old STMPE driver may be a bad fit, is is better
    to improve that (e.g. migrate it to use regmap and rewrite the
    stmpe-gpio.c driver to use pin control) and use also for this
    driver, or write a new driver from scratch like this?

    I'm not so sure.

    I do know that developers not always like to take out old
    hardware and old development boards and start hacking
    away before they can get some nice new hardware going
    but I am worried that this may be one of those cases where
    a serious cleanup of the aging STMPE driver may be a
    first necessary step.

    > +/* IRQ output management */
    > +#define MFX_REG_IRQ_OUT_PIN 0x41 /* RW */
    > +#define MFX_REG_IRQ_SRC_EN 0x42 /* RW */
    > +#define MFX_REG_IRQ_PENDING 0x08 /* R */
    > +#define MFX_REG_IRQ_ACK 0x44 /* RW */

    Very similar to STMPE it seems.

    > +/* MFX_REG_SYS_CTRL bitfields */
    > +#define MFX_REG_SYS_CTRL_GPIO_EN BIT(0)
    > +#define MFX_REG_SYS_CTRL_TS_EN BIT(1)
    > +#define MFX_REG_SYS_CTRL_IDD_EN BIT(2)
    > +#define MFX_REG_SYS_CTRL_ALTGPIO_EN BIT(3)

    I guess these blocks works the same as with STMPE,
    that you can only use one of them at the time?

    What is altgpio?

    > +/* MFX_REG_IRQ_OUT_PIN bitfields */
    > +#define MFX_REG_IRQ_OUT_PIN_TYPE BIT(0) /* 0-OD 1-PP */
    > +#define MFX_REG_IRQ_OUT_PIN_POL BIT(1) /* 0-active LOW 1-active HIGH */

    I have not read the patch yet. But just for notice:
    This output IRQ type needs to be handled as well.

    Check the code in
    drivers/iio/common/st_sensors/st_sensors_trigger.c

    To see how you can detect the properties of an IRQ
    to set the right polarity, and handling of open drain
    IRQ lines.

    Yours,
    Linus Walleij

    \
     
     \ /
      Last update: 2018-02-22 14:45    [W:5.016 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site