Message-ID: <cc54d945-9530-8918-c9fd-4c048be83bd9@linaro.org>
Date:   Tue, 20 Dec 2022 15:47:42 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Zeynep Arslanbenzer <Zeynep.Arslanbenzer@...log.com>,
        lee@...nel.org, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, sre@...nel.org,
        lgirdwood@...il.com, broonie@...nel.org
Cc:     Nurettin.Bolucu@...log.com, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 1/6] drivers: mfd: add MAX77659 PMIC support

On 20/12/2022 14:22, Zeynep Arslanbenzer wrote:
> This patch adds MFD driver for MAX77659 to enable its sub
> devices.
> 
> The MAX77659 is a multi-function devices. It includes
> charger and regulator
> 
> Signed-off-by: Nurettin Bolucu <Nurettin.Bolucu@...log.com>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@...log.com>
> ---
>  MAINTAINERS                  |   8 ++
>  drivers/mfd/Kconfig          |  14 +++
>  drivers/mfd/Makefile         |   1 +
>  drivers/mfd/max77659.c       | 188 +++++++++++++++++++++++++++++++++++
>  include/linux/mfd/max77659.h |  60 +++++++++++
>  5 files changed, 271 insertions(+)
>  create mode 100644 drivers/mfd/max77659.c
>  create mode 100644 include/linux/mfd/max77659.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a608f19da3a9..45a8a471c7c0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12640,6 +12640,14 @@ F:	drivers/power/supply/max77650-charger.c
>  F:	drivers/regulator/max77650-regulator.c
>  F:	include/linux/mfd/max77650.h


> +	ret = regmap_read(me->regmap, MAX77659_REG_INT_CHG, &val);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Unable to read Charger Interrupt Status register\n");
> +	ret = device_init_wakeup(dev, true);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Unable to init wakeup\n");
> +
> +	ret = devm_mfd_add_devices(dev, -1, max77659_devs, ARRAY_SIZE(max77659_devs),
> +				   NULL, 0, NULL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add sub-devices\n");
> +
> +	enable_irq_wake(me->irq);

You do not allow a wakeup-source in DT, do you?

> +
> +	return 0;
> +}
> +
> +static int max77659_i2c_probe(struct i2c_client *client)
> +{
> +	struct max77659_dev *me;

Do you see other MFD driver calling this "me"? Please submit code which
is consistent with Linux style, not with your own.

> +
> +	me = devm_kzalloc(&client->dev, sizeof(*me), GFP_KERNEL);
> +	if (!me)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, me);
> +	me->dev = &client->dev;
> +	me->irq = client->irq;
> +
> +	me->regmap = devm_regmap_init_i2c(client, &max77659_regmap_config);
> +
> +	if (IS_ERR(me->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(me->regmap),
> +				     "Failed to allocate register map\n");
> +
> +	return max77659_pmic_setup(me);
> +}
> +
> +static const struct of_device_id max77659_of_id[] = {
> +	{ .compatible = "adi,max77659" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, max77659_of_id);
> +
> +static const struct i2c_device_id max77659_i2c_id[] = {
> +	{ MAX77659_NAME, 0 },

Nope. Use proper string.

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max77659_i2c_id);
> +
> +static struct i2c_driver max77659_i2c_driver = {
> +	.driver = {
> +		.name = MAX77659_NAME,
> +		.of_match_table = of_match_ptr(max77659_of_id),

You will have warnings here, so the patch was not really compile tested.
Drop of_match_ptr() and check your code before sending (W=1, smatch,
sparse, coccinelle)

> +	},
> +	.probe_new = max77659_i2c_probe,
> +	.id_table = max77659_i2c_id,
> +};
> +
> +module_i2c_driver(max77659_i2c_driver);
> +
> +MODULE_DESCRIPTION("max77659 MFD Driver");
> +MODULE_AUTHOR("Nurettin.Bolucu@...log.com, Zeynep.Arslanbenzer@...log.com");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");
> diff --git a/include/linux/mfd/max77659.h b/include/linux/mfd/max77659.h
> new file mode 100644
> index 000000000000..ef781e6e54c2
> --- /dev/null
> +++ b/include/linux/mfd/max77659.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef __MAX77659_MFD_H__
> +#define __MAX77659_MFD_H__

Header guard should be more descriptive (with pieces of path)

> +
> +#include <linux/bits.h>
> +
> +#define MAX77659_NAME "max77659"

Not really for include.

> +
> +#define MAX77659_REGULATOR_NAME MAX77659_NAME "-regulator"
> +#define MAX77659_CHARGER_NAME MAX77659_NAME "-charger"

Neither these.

> +
> +#define MAX77659_REG_INT_GLBL0 0x00
> +#define MAX77659_REG_INT_CHG 0x01
> +#define MAX77659_REG_INT_GLBL1 0x04
> +#define MAX77659_REG_INT_M_CHG 0x07
> +#define MAX77659_REG_INTM_GLBL1 0x08
> +#define MAX77659_REG_INTM_GLBL0 0x09

That's absolutely unreadable code.

> +
> +#define MAX77659_INT_GLBL0_MASK 0xFF
> +#define MAX77659_INT_GLBL1_MASK 0x33
> +
> +#define MAX77659_BIT_INT_GLBL0_GPIO0_F BIT(0)
> +#define MAX77659_BIT_INT_GLBL0_GPIO0_R BIT(1)
> +#define MAX77659_BIT_INT_GLBL0_EN_F BIT(2)
> +#define MAX77659_BIT_INT_GLBL0_EN_R BIT(3)
> +#define MAX77659_BIT_INT_GLBL0_TJAL1_R BIT(4)
> +#define MAX77659_BIT_INT_GLBL0_TJAL2_R BIT(5)
> +#define MAX77659_BIT_INT_GLBL0_DOD0_R BIT(7)
> +
> +#define MAX77659_BIT_INT_GLBL1_GPI1_F BIT(0)
> +#define MAX77659_BIT_INT_GLBL1_GPI1_R BIT(1)
> +#define MAX77659_BIT_INT_GLBL1_SBB_TO BIT(4)
> +#define MAX77659_BIT_INT_GLBL1_LDO0_F BIT(5)
> +
> +#define MAX77659_BIT_INT_THM BIT(0)
> +#define MAX77659_BIT_INT_CHG BIT(1)
> +#define MAX77659_BIT_INT_CHGIN BIT(2)
> +#define MAX77659_BIT_INT_TJ_REG BIT(3)
> +#define MAX77659_BIT_INT_SYS_CTRL BIT(4)
> +
> +enum {
> +	MAX77659_DEV_PMIC,
> +	MAX77659_DEV_CHARGER,
> +	MAX77659_DEV_REGULATOR,
> +	MAX77659_NUM_DEVS,
> +};
> +
> +struct max77659_dev {
> +	struct device *dev;
> +
> +	int irq;
> +	struct regmap_irq_chip_data *irqc_glbl0;
> +	struct regmap_irq_chip_data *irqc_glbl1;
> +	struct regmap_irq_chip_data	*irqc_chg;

Keep your code consistent.

> +
> +	struct regmap *regmap;
> +};
> +
> +#endif /* __MAX77659_MFD_H__ */

Best regards,
Krzysztof

Powered by blists - more mailing lists