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@xxxxxxxxxx>
> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx, Zeynep.Arslanbenzer@xxxxxxxxxx");
> +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
