Messages in this thread | | | From | andy.shevchenko@gmail ... | Date | Tue, 7 Mar 2023 01:54:07 +0200 | Subject | Re: [PATCH v1 2/2] gpio: fxl6408: add I2C GPIO expander driver |
| |
Mon, Mar 06, 2023 at 09:34:46AM +0100, Francesco Dolcini kirjoitti: > From: Emanuele Ghidoli <emanuele.ghidoli@toradex.com> > > Support Fairchild (now ON Semiconductor) fxl6408 which has 8 GPIO lines > and is controlled by I2C bus.
Is it really GPIO expander and not a (semi-)featured pin control with GPIO capability?
Can we have a Datasheet: tag here?
> Signed-off-by: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
...
> + help > + GPIO driver for Fairchild Semiconductor FXL6408 GPIO expander
Checkpatch usually complains on the help < 3 lines. You may add the module name for M choice.
...
> + * Author: Emanuele Ghidoli <emanuele.ghidoli@toradex.com>
> + *
Unneeded blank line.
...
> +#include <linux/gpio.h>
No way. This must not be in any code.
...
> +#include <linux/of_platform.h>
Why? For discrete components make sure you have not an OF-centric code.
...
> +static const struct regmap_range rd_range[] = { > + { FXL6408_REG_DEVICE_ID, FXL6408_REG_DEVICE_ID }, > + { FXL6408_REG_IO_DIR, FXL6408_REG_OUTPUT }, > + { FXL6408_REG_INPUT_STATUS, FXL6408_REG_INPUT_STATUS }
In all definitions where the entry is _not_ a terminator, leave the trailing comma in place.
> +};
...
> + };
> + gpio_config.regmap = devm_regmap_init_i2c(client, ®map);
> +
This blank line is misplaced. Should be before devm_regmap_init_i2c() call.
> + if (IS_ERR(gpio_config.regmap)) {
> + dev_err(dev, "failed to allocate register map\n"); > + return PTR_ERR(gpio_config.regmap);
return dev_err_probe();
> + }
...
> + /* Disable High-Z of outputs, so that our OUTPUT updates > + * actually take effect. > + */
/* * This is correct style for multi-line * comments. Yours needs to be fixed. */
...
> + ret = regmap_write(gpio_config.regmap, FXL6408_REG_OUTPUT_HIGH_Z, 0); > + if (ret) {
> + dev_err(dev, "failed to write 'output high Z' register\n"); > + return ret;
return dev_err_probe(...);
> + }
...
> +static const struct i2c_device_id fxl6408_id[] = { > + { "fxl6408", 0 }, > + { },
But no comma for a terminator entry. > +};
...
> +
Unneeded blank line.
> +module_i2c_driver(fxl6408_driver);
-- With Best Regards, Andy Shevchenko
| |