On Tue, Mar 18, 2025 at 05:26:18PM +0100, mathieu.dubois-briand@xxxxxxxxxxx wrote: > From: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx> > > Add core driver to support MAX7360 i2c chip, multi function device > with keypad, GPIO, PWM, GPO and rotary encoder submodules. ... + array_size.h > +#include <linux/bits.h> > +#include <linux/delay.h> > +#include <linux/device.h> Since it won;t make v6.15-rc1 anyway the above can be better specified as device/devres.h dev_printk.h as device.h is monstrous. + err.h > +#include <linux/i2c.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/core.h> > +#include <linux/mfd/max7360.h> + mod_devicetable.h > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/types.h> ... > + ret = regmap_write(regmap, MAX7360_REG_GPIOCFG, > + MAX7360_GPIO_CFG_GPIO_RST); I would suggest to leave it as a single line. In this and similar cases when it is ~83 characters, it's still fine (and even for strict 80 there is a documented exception) ... > + return ret; > + } > + > + return 0; Just return ret; ? > + ret = max7360_mask_irqs(regmap); > + if (ret) > + return dev_err_probe(dev, ret, "Could not mask interrupts\n"); Hmm... As far as I can read this masks GPIO interrups. Does it do anything else? If it's covered by the GPIO/pin control drivers, one want probably to see that to be done there in the respective callback (init_hw_irq or alike, I don't remember the name by heart). ... > +#ifndef __LINUX_MFD_MAX7360_H > +#define __LINUX_MFD_MAX7360_H > + > +#include <linux/bits.h> > +#include <linux/types.h> Do you need types.h here? I don't see for what... > +struct device; Neither this. Perhaps it's for the following changes? Then add when required, not now. > +#endif -- With Best Regards, Andy Shevchenko
