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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
