Re: [PATCH v5 02/11] mfd: Add max7360 support

[Andy Shevchenko]

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