Re: [PATCH 2/6] mfd: Add ST Multi-Function eXpander core driver

[Linus Walleij]

On Thu, Feb 8, 2018 at 3:27 PM, Amelie Delaunay <amelie.delaunay@xxxxxx> 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