Fri, May 17, 2024 at 02:58:04PM +0200, Alvin Šipraga kirjoitti: > From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx> > > This driver adds GPIO function support for AD24xx A2B transceiver chips. "Add GPIO..." > When a GPIO is requested, the relevant pin is automatically muxed to > GPIO mode. The device tree property gpio-reserved-ranges can be used to > protect certain pins which are reserved for other functionality such as > I2S/TDM data. Why this doesn't use gpio-regmap? ... > +config GPIO_AD24XX > + tristate "Analog Devies Inc. AD24xx GPIO support" > + depends on A2B_AD24XX_NODE > + help > + Say Y here to enable GPIO support for AD24xx A2B transceivers. checkpatch probably complain about too short help text. You may extend it by explaining how module will be called. ... > +#include <linux/a2b/a2b.h> > +#include <linux/a2b/ad24xx.h> This seems to me not so generic as below... + bits.h + device.h + err.h > +#include <linux/gpio/driver.h> > +#include <linux/interrupt.h> + mod_devicetable.h > +#include <linux/module.h> + mutex.h > +#include <linux/of_irq.h> Please, can we avoid OF in a new code? > +#include <linux/regmap.h> ...hence move that group here and put a blank line before. ... > +struct ad24xx_gpio { > + struct device *dev; > + struct a2b_func *func; > + struct a2b_node *node; > + struct regmap *regmap; > + int irqs[AD24XX_MAX_GPIOS]; > + struct gpio_chip gpio_chip; If you move this to be the first member, you might get less code being generated at compile time. > + struct irq_chip irq_chip; Should not be here, but static. > + struct mutex mutex; > + unsigned int irq_invert : AD24XX_MAX_GPIOS; > + unsigned int irq_enable : AD24XX_MAX_GPIOS; > +}; ... > + if (ret) > + dev_err(adg->dev, > + "failed to update interrupt configuration: %d\n", ret); Why and how is this useful? ... > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); First of all it uses a wrong API (custom to IRQ core), second why do you need this? ... > + struct device_node *np; > + np = of_irq_find_parent(dev->of_node); > + if (!np) > + return -ENOENT; > + > + parent_domain = irq_find_host(np); > + of_node_put(np); > + if (!parent_domain) > + return -ENOENT; Why is this magic needed? ... > + ret = devm_gpiochip_add_data(dev, gpio_chip, adg); > + if (ret) > + return ret; > + > + return 0; return devm_gpiochip_add_data(...); -- With Best Regards, Andy Shevchenko