lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <MN2PR03MB51973A5D648C695EA2B353728B9C9@MN2PR03MB5197.namprd03.prod.outlook.com>
Date:   Mon, 17 Apr 2023 09:44:18 +0000
From:   "Arslanbenzer, Zeynep" <Zeynep.Arslanbenzer@...log.com>
To:     Lee Jones <lee@...nel.org>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        "sre@...nel.org" <sre@...nel.org>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>
Subject: RE: [PATCH v2 8/8] mfd: max77658: Add ADI MAX77643/54/58/59 MFD
 Support

>On Wed, 22 Mar 2023, Zeynep Arslanbenzer wrote:
>
>> MFD driver for MAX77643/MAX77654/MAX77658/MAX77659 to enable its sub
>
>Please drop all references to 'MFD'.
>
>What are these devices, really?  I suspect they are PMICs?

Hi Lee,

Thank you for your review. Yes, all devices covered by this patch are PMIC.

>
>> devices.
>>
>> The MAX77643 is a multi-function devices. It includes regulator.
>>
>> The MAX77654 is a multi-function devices. It includes regulator and 
>> charger.
>>
>> The MAX77658 is a multi-function devices. It includes regulator, 
>> charger and battery.
>>
>> The MAX77659 is a multi-function devices. It includes regulator and 
>> charger.
>>
>> Signed-off-by: Nurettin Bolucu <Nurettin.Bolucu@...log.com>>
>> Signed-off-by: Zeynep Arslanbenzer <Zeynep.Arslanbenzer@...log.com>>
>> ---
>>  drivers/mfd/Kconfig          |  15 ++
>>  drivers/mfd/Makefile         |   1 +
>>  drivers/mfd/max77658.c       | 448 +++++++++++++++++++++++++++++++++++
>>  include/linux/mfd/max77658.h |  88 +++++++
>>  4 files changed, 552 insertions(+)
>>  create mode 100644 drivers/mfd/max77658.c  create mode 100644 
>> include/linux/mfd/max77658.h
>>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 
>> 8b93856de432..7b4be7fb8662 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -821,6 +821,21 @@ config MFD_MAX77650
>>  	  the following functionalities of the device: GPIO, regulator,
>>  	  charger, LED, onkey.
>>
>> +config MFD_MAX77658
>> +	tristate "Analog Devices MAX77643/MAX77654/MAX77658/MAX77659 PMIC Support"
>> +	depends on I2C
>> +	depends on OF
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	select REGMAP_IRQ
>> +	help
>> +	  Say Y here to add support for Analog Devices
>> +	  MAX77643/MAX77654/MAX77658/MAX77659 Power Management IC.
>
>"MAX776xx series"?

I will update the patch to be compatible with the MAX776xx.

>
>> +	  This is the core multifunction
>
>Just "core driver" is fine.
>
>Odd place to line wrap?
>
>> +	  driver for interacting with the device. Additional drivers can be
>
>"can be"?  It's probably pretty useless if you don't, no?
>
>> +	  enabled in order to use the following functionalities of the device:
>> +	  regulator, charger.
>
>"... in order to use the regular and charger functionality of the device".
>
>>  config MFD_MAX77686
>>  	tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
>>  	depends on I2C
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 
>> 7ed3ef4a698c..f52aff45878f 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -163,6 +163,7 @@ obj-$(CONFIG_MFD_DA9150)	+= da9150-core.o
>>  obj-$(CONFIG_MFD_MAX14577)	+= max14577.o
>>  obj-$(CONFIG_MFD_MAX77620)	+= max77620.o
>>  obj-$(CONFIG_MFD_MAX77650)	+= max77650.o
>> +obj-$(CONFIG_MFD_MAX77658)	+= max77658.o
>>  obj-$(CONFIG_MFD_MAX77686)	+= max77686.o
>>  obj-$(CONFIG_MFD_MAX77693)	+= max77693.o
>>  obj-$(CONFIG_MFD_MAX77714)	+= max77714.o
>> diff --git a/drivers/mfd/max77658.c b/drivers/mfd/max77658.c new file 
>> mode 100644 index 000000000000..a1c6db48eb08
>> --- /dev/null
>> +++ b/drivers/mfd/max77658.c
>> @@ -0,0 +1,448 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2023 Analog Devices, Inc.
>> + * ADI driver for the MAX77643/MAX77654/MAX77658/MAX77659
>> + */
>
>No need to list every device.
>
>"MAX776xx series"?
>
>> +#include <linux/i2c.h>>
>> +#include <linux/mfd/core.h>>
>> +#include <linux/mfd/max77658.h>>
>> +#include <linux/mod_devicetable.h>>
>> +#include <linux/of_device.h>>
>> +#include <linux/regmap.h>>
>> +
>> +#define I2C_ADDR_FUEL_GAUGE (0x6C >>>> 1)
>> +
>> +static const struct regmap_config max77658_regmap_config = {
>> +	.reg_bits   = 8,
>> +	.val_bits   = 8,
>> +};
>> +
>> +static const struct regmap_config max77658_regmap_config_fg = {
>> +	.reg_bits   = 8,
>> +	.val_bits   = 16,
>> +	.cache_type = REGCACHE_NONE,
>> +	.val_format_endian = REGMAP_ENDIAN_LITTLE, };
>> +
>> +static const struct regmap_irq max77643_glbl0_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_GPIO0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_GPIO0_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_EN_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_EN_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_TJAL1_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_TJAL2_R, },
>> +	{ .mask = MAX77643_BIT_INT_GLBL0_DOD0_R, }, };
>> +
>> +static const struct regmap_irq_chip max77643_glbl0_irq_chip = {
>> +	.name           = "max77643_glbl0",
>> +	.status_base    = MAX77658_REG_INT_GLBL0,
>> +	.mask_base      = MAX77643_REG_INTM_GLBL0,
>> +	.num_regs       = 1,
>> +	.irqs           = max77643_glbl0_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77643_glbl0_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77658_glbl0_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_GPIO0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_GPIO0_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_EN_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_EN_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_TJAL1_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_TJAL2_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_DOD1_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_DOD0_R, }, };
>> +
>> +static const struct regmap_irq_chip max77654_glbl0_irq_chip = {
>> +	.name           = "max77654_glbl0",
>> +	.status_base    = MAX77658_REG_INT_GLBL0,
>> +	.mask_base      = MAX77654_REG_INTM_GLBL0,
>> +	.num_regs       = 1,
>> +	.irqs           = max77658_glbl0_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77658_glbl0_irqs),
>> +};
>> +
>> +static const struct regmap_irq_chip max77658_glbl0_irq_chip = {
>> +	.name           = "max77658_glbl0",
>> +	.status_base    = MAX77658_REG_INT_GLBL0,
>> +	.mask_base      = MAX77658_REG_INTM_GLBL0,
>> +	.num_regs       = 1,
>> +	.irqs           = max77658_glbl0_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77658_glbl0_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77659_glbl0_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_GPIO0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_GPIO0_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_EN_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_EN_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_TJAL1_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_TJAL2_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL0_DOD0_R, }, };
>> +
>> +static const struct regmap_irq_chip max77659_glbl0_irq_chip = {
>> +	.name           = "max77659_glbl0",
>> +	.status_base    = MAX77658_REG_INT_GLBL0,
>> +	.mask_base      = MAX77654_REG_INTM_GLBL0,
>> +	.num_regs       = 1,
>> +	.irqs           = max77659_glbl0_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77659_glbl0_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77643_glbl1_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_SBB0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_SBB1_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_SBB2_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_LDO0_F, }, };
>> +
>> +static const struct regmap_irq_chip max77643_glbl1_irq_chip = {
>> +	.name           = "max77643_glbl1",
>> +	.status_base    = MAX77658_REG_INT_GLBL1,
>> +	.mask_base      = MAX77643_REG_INTM_GLBL1,
>> +	.num_regs       = 1,
>> +	.irqs           = max77643_glbl1_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77643_glbl1_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77654_glbl1_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_R, },
>> +	{ .mask = MAX77654_BIT_INT_GLBL1_GPI2_F, },
>> +	{ .mask = MAX77654_BIT_INT_GLBL1_GPI2_R, },
>> +	{ .mask = MAX77654_BIT_INT_GLBL1_SBB_TO, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_LDO0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_LDO1_F, }, };
>> +
>> +static const struct regmap_irq_chip max77654_glbl1_irq_chip = {
>> +	.name           = "max77654_glbl1",
>> +	.status_base    = MAX77658_REG_INT_GLBL1,
>> +	.mask_base      = MAX77654_REG_INTM_GLBL1,
>> +	.num_regs       = 1,
>> +	.irqs           = max77654_glbl1_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77654_glbl1_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77658_glbl1_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_R, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_SBB0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_SBB1_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_SBB2_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_LDO0_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_LDO1_F, }, };
>> +
>> +static const struct regmap_irq_chip max77658_glbl1_irq_chip = {
>> +	.name           = "max77658_glbl1",
>> +	.status_base    = MAX77658_REG_INT_GLBL1,
>> +	.mask_base      = MAX77658_REG_INTM_GLBL1,
>> +	.num_regs       = 1,
>> +	.irqs           = max77658_glbl1_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77658_glbl1_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77659_glbl1_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_F, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_GPI1_R, },
>> +	{ .mask = MAX77659_BIT_INT_GLBL1_SBB_TO, },
>> +	{ .mask = MAX77658_BIT_INT_GLBL1_LDO0_F, }, };
>> +
>> +static const struct regmap_irq_chip max77659_glbl1_irq_chip = {
>> +	.name           = "max77659_glbl1",
>> +	.status_base    = MAX77658_REG_INT_GLBL1,
>> +	.mask_base      = MAX77658_REG_INTM_GLBL1,
>> +	.num_regs       = 1,
>> +	.irqs           = max77659_glbl1_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77659_glbl1_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77658_chg_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_THM, },
>> +	{ .mask = MAX77658_BIT_INT_CHG, },
>> +	{ .mask = MAX77658_BIT_INT_CHGIN, },
>> +	{ .mask = MAX77658_BIT_INT_TJ_REG, },
>> +	{ .mask = MAX77658_BIT_INT_CHGIN_CTRL, },
>> +	{ .mask = MAX77658_BIT_INT_SYS_CTRL, },
>> +	{ .mask = MAX77658_BIT_INT_SYS_CNFG, }, };
>> +
>> +static const struct regmap_irq_chip max77654_chg_irq_chip = {
>> +	.name           = "max77654_chg",
>> +	.status_base    = MAX77658_REG_INT_CHG,
>> +	.mask_base      = MAX77658_REG_INTM_CHG,
>> +	.num_regs       = 1,
>> +	.irqs           = max77658_chg_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77658_chg_irqs),
>> +};
>> +
>> +static const struct regmap_irq_chip max77658_chg_irq_chip = {
>> +	.name           = "max77658_chg",
>> +	.status_base    = MAX77658_REG_INT_CHG,
>> +	.mask_base      = MAX77658_REG_INTM_CHG,
>> +	.num_regs       = 1,
>> +	.irqs           = max77658_chg_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77658_chg_irqs),
>> +};
>> +
>> +static const struct regmap_irq max77659_chg_irqs[] = {
>> +	{ .mask = MAX77658_BIT_INT_THM, },
>> +	{ .mask = MAX77658_BIT_INT_CHG, },
>> +	{ .mask = MAX77658_BIT_INT_CHGIN, },
>> +	{ .mask = MAX77658_BIT_INT_TJ_REG, },
>> +	{ .mask = MAX776569_BIT_INT_SYS_CTRL, }, };
>> +
>> +static const struct regmap_irq_chip max77659_chg_irq_chip = {
>> +	.name           = "max77659_chg",
>> +	.status_base    = MAX77658_REG_INT_CHG,
>> +	.mask_base      = MAX77658_REG_INTM_CHG,
>> +	.num_regs       = 1,
>> +	.irqs           = max77659_chg_irqs,
>> +	.num_irqs       = ARRAY_SIZE(max77659_chg_irqs),
>> +};
>> +
>> +static const struct mfd_cell max77643_devs[] = {
>> +	MFD_CELL_OF("max77643-regulator", NULL, NULL, 0, 0,
>> +		    "adi,max77643-regulator"),
>> +};
>> +
>> +static const struct mfd_cell max77654_devs[] = {
>> +	MFD_CELL_OF("max77654-regulator", NULL, NULL, 0, 0,
>> +		    "adi,max77654-regulator"),
>> +	MFD_CELL_OF("max77654-charger", NULL, NULL, 0, 0,
>> +		    "adi,max77654-charger"),
>> +};
>> +
>> +static const struct mfd_cell max77658_devs[] = {
>> +	MFD_CELL_OF("max77658-regulator", NULL, NULL, 0, 0,
>> +		    "adi,max77658-regulator"),
>> +	MFD_CELL_OF("max77658-charger", NULL, NULL, 0, 0,
>> +		    "adi,max77658-charger"),
>> +	MFD_CELL_OF("max77658-battery", NULL, NULL, 0, 0,
>> +		    "adi,max77658-battery"),
>> +};
>> +
>> +static const struct mfd_cell max77659_devs[] = {
>> +	MFD_CELL_OF("max77659-regulator", NULL, NULL, 0, 0,
>> +		    "adi,max77659-regulator"),
>> +	MFD_CELL_OF("max77659-charger", NULL, NULL, 0, 0,
>> +		    "adi,max77659-charger"),
>> +};
>
>You can unwrap all of these, you have 100-chars to play with.

Based on other maintainers' reviews, I limited the number of characters per line to 80. 
Here is the link for the related review -> https://lore.kernel.org/lkml/9915e079-c6b5-a8f4-734e-f0325809efd0@linaro.org/
I can continue according to the joint decision.

>
>> +static const struct chip_info chip[] = {
>> +	[ID_MAX77643] = {
>> +		.id = ID_MAX77643,
>> +		.n_devs = ARRAY_SIZE(max77643_devs),
>> +		.devs = max77643_devs,
>> +	},
>> +	[ID_MAX77654] = {
>> +		.id = ID_MAX77654,
>> +		.n_devs = ARRAY_SIZE(max77654_devs),
>> +		.devs = max77654_devs,
>> +	},
>> +	[ID_MAX77658] = {
>> +		.id = ID_MAX77658,
>> +		.n_devs = ARRAY_SIZE(max77658_devs),
>> +		.devs = max77658_devs,
>> +	},
>> +	[ID_MAX77659] = {
>> +		.id = ID_MAX77659,
>> +		.n_devs = ARRAY_SIZE(max77659_devs),
>> +		.devs = max77659_devs,
>> +	},
>> +};
>> +
>> +static int max77658_pmic_irq_init(struct device *dev) {
>> +	const struct regmap_irq_chip *glbl0_chip, *glbl1_chip, *chg_chip;
>> +	struct max77658_dev *max77658 = dev_get_drvdata(dev);
>> +	int ret, i;
>> +
>> +	switch (max77658->>chip->>id) {
>> +	case ID_MAX77643:
>> +		glbl0_chip = &max77643_glbl0_irq_chip;
>> +		glbl1_chip = &max77643_glbl1_irq_chip;
>> +		break;
>> +	case ID_MAX77654:
>> +		glbl0_chip = &max77654_glbl0_irq_chip;
>> +		glbl1_chip = &max77654_glbl1_irq_chip;
>> +		chg_chip = &max77654_chg_irq_chip;
>> +		break;
>> +	case ID_MAX77658:
>> +		glbl0_chip = &max77658_glbl0_irq_chip;
>> +		glbl1_chip = &max77658_glbl1_irq_chip;
>> +		chg_chip = &max77658_chg_irq_chip;
>> +		break;
>> +	case ID_MAX77659:
>> +		glbl0_chip = &max77659_glbl0_irq_chip;
>> +		glbl1_chip = &max77659_glbl1_irq_chip;
>> +		chg_chip = &max77659_chg_irq_chip;
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	for (i = 0; i < glbl0_chip->>num_regs; i++) {
>> +		ret = regmap_update_bits(max77658->>regmap,
>> +					 glbl0_chip->>mask_base,
>> +					 (1 << glbl0_chip->>irqs[i].reg_offset),
>> +					 1);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Unable to write Global0 Interrupt Masking register\n");
>> +	}
>> +
>> +	for (i = 0; i < glbl1_chip->>num_regs; i++) {
>> +		ret = regmap_update_bits(max77658->>regmap,
>> +					 glbl1_chip->>mask_base,
>> +					 (1 << glbl1_chip->>irqs[i].reg_offset),
>> +					 1);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Unable to write Global1 Interrupt Masking register\n");
>> +	}
>> +
>> +	if (max77658->>chip->>id != ID_MAX77643) {
>> +		for (i = 0; i < chg_chip->>num_regs; i++) {
>> +			ret = regmap_update_bits(max77658->>regmap,
>> +						 chg_chip->>mask_base,
>> +						 (1 <<
>> +						 chg_chip->>irqs[i].reg_offset),
>> +						 1);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "Unable to write Charger Interrupt Masking register\n");
>> +		}
>> +
>> +		ret = devm_regmap_add_irq_chip(dev, max77658->>regmap,
>> +					       max77658->>irq,
>> +					       IRQF_ONESHOT | IRQF_SHARED, 0,
>> +					       chg_chip, &max77658->>irqc_chg);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Failed to add charger IRQ chip\n");
>> +	}
>> +
>> +	ret = devm_regmap_add_irq_chip(dev, max77658->>regmap, max77658->>irq,
>> +				       IRQF_ONESHOT | IRQF_SHARED, 0,
>> +				       glbl0_chip, &max77658->>irqc_glbl0);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to add global0 IRQ chip\n");
>> +
>> +	return devm_regmap_add_irq_chip(dev, max77658->>regmap, max77658->>irq,
>> +					IRQF_ONESHOT | IRQF_SHARED, 0,
>> +					glbl1_chip, &max77658->>irqc_glbl1);
>
>This function is hectic.
>
>What exactly are you doing here?

I mask all interrupts of global0, global1, and charger inside the loops. 

>
>> +}
>> +
>> +static int max77658_pmic_setup(struct device *dev) {
>> +	struct max77658_dev *max77658 = dev_get_drvdata(dev);
>> +	int ret;
>> +
>> +	ret = max77658_pmic_irq_init(max77658->>dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
>> +				   max77658->>chip->>devs, max77658->>chip->>n_devs,
>> +				   NULL, 0, NULL);
>> +
>
>These are usually placed in probe.
>
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to add sub-devices\n");
>> +
>> +	ret = device_init_wakeup(dev, true);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Unable to init wakeup\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id max77658_i2c_id[];
>
>What on earth is this?  A struct forward declaration?
>
>If you need this, just move the original code block to here.
>
>> +static int max77658_i2c_probe(struct i2c_client *client) {
>> +	struct max77658_dev *max77658;
>> +	struct i2c_client *fuel;
>> +
>> +	max77658 = devm_kzalloc(&client->>dev, sizeof(*max77658), GFP_KERNEL);
>> +	if (!max77658)
>> +		return -ENOMEM;
>> +
>> +	i2c_set_clientdata(client, max77658);
>> +	max77658->>dev = &client->>dev;
>
>How do you fetch back max77658?
>
>Don't you need access to the device structs?
>
>If so, you already have a reference, no?
>
>> +	max77658->>irq = client->>irq;
>> +
>> +	if (max77658->>dev->>of_node)
>> +		max77658->>chip  = of_device_get_match_data(max77658->>dev);
>> +	else
>> +		max77658->>chip  = (struct chip_info *)
>
>Cast from void * shouldn't be required.
>
>> +					i2c_match_id(max77658_i2c_id,
>> +						     client)->>driver_data;
>
>100-chars everywhere.
>
>> +	if (!max77658->>chip)
>> +		return -EINVAL;
>> +
>> +	max77658->>regmap = devm_regmap_init_i2c(client,
>> +						&max77658_regmap_config);
>> +	if (IS_ERR(max77658->>regmap))
>> +		return dev_err_probe(max77658->>dev, PTR_ERR(max77658->>regmap),
>> +				     "Failed to allocate register map\n");
>
>s/allocate/initialise/
>
>"regmap" is fine.
>
>> +	fuel = i2c_new_dummy_device(client->>adapter, I2C_ADDR_FUEL_GAUGE);
>> +	if (IS_ERR(fuel))
>> +		return dev_err_probe(max77658->>dev, PTR_ERR(fuel),
>> +				     "failed add i2c device[0x%Xh]\n",
>
>"Failed to create I2C device"
>
>> +				      I2C_ADDR_FUEL_GAUGE);
>> +
>> +	i2c_set_clientdata(fuel, max77658);
>> +
>> +	max77658->>regmap_fg = devm_regmap_init_i2c(fuel,
>> +						   &max77658_regmap_config_fg);
>> +	if (IS_ERR(max77658->>regmap_fg))
>> +		return dev_err_probe(max77658->>dev,
>> +				     PTR_ERR(max77658->>regmap_fg),
>> +				     "failed to initialize i2c device[0x%Xh]\n",
>
>"Failed"
>
>"I2C"
>
>> +				     I2C_ADDR_FUEL_GAUGE);
>> +
>> +	return max77658_pmic_setup(max77658->>dev);
>> +}
>> +
>> +static const struct of_device_id max77658_of_id[] = {
>> +	{ .compatible = "adi,max77643", .data = &chip[ID_MAX77643] },
>> +	{ .compatible = "adi,max77654", .data = &chip[ID_MAX77654] },
>> +	{ .compatible = "adi,max77658", .data = &chip[ID_MAX77658] },
>> +	{ .compatible = "adi,max77659", .data = &chip[ID_MAX77659] },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, max77658_of_id);
>> +
>> +static const struct i2c_device_id max77658_i2c_id[] = {
>> +	{ "max77643", (kernel_ulong_t)&chip[ID_MAX77643] },
>> +	{ "max77654", (kernel_ulong_t)&chip[ID_MAX77654] },
>> +	{ "max77658", (kernel_ulong_t)&chip[ID_MAX77658] },
>> +	{ "max77659", (kernel_ulong_t)&chip[ID_MAX77659] },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, max77658_i2c_id);
>
>Please don't pass MFD data through the DT platform initialisation API.
>
>Pass the Chip ID and match in C-code.
>
>> +static struct i2c_driver max77658_driver = {
>> +	.driver = {
>> +		.name = "max77658",
>> +		.of_match_table = max77658_of_id,
>> +	},
>> +	.probe_new = max77658_i2c_probe,
>> +	.id_table = max77658_i2c_id,
>> +};
>> +module_i2c_driver(max77658_driver);
>> +
>> +MODULE_DESCRIPTION("MAX77658 MFD Driver"); 
>> +MODULE_AUTHOR("Nurettin.Bolucu@...log.com, 
>> +Zeynep.Arslanbenzer@...log.com"); MODULE_LICENSE("GPL");
>> diff --git a/include/linux/mfd/max77658.h 
>> b/include/linux/mfd/max77658.h new file mode 100644 index 
>> 000000000000..471a8474513e
>> --- /dev/null
>> +++ b/include/linux/mfd/max77658.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +
>> +#ifndef __MAX77658_MFD_H__
>> +#define __MAX77658_MFD_H__
>> +
>> +#include <linux/bits.h>>
>> +#include <linux/types.h>>
>> +
>> +#define MAX77658_REG_INT_GLBL0	0x00
>> +#define MAX77658_REG_INT_CHG	0x01
>> +#define MAX77658_REG_INT_GLBL1	0x04
>> +#define MAX77658_REG_INTM_CHG	0x07
>> +#define MAX77658_REG_INTM_GLBL0	0x08
>> +#define MAX77658_REG_INTM_GLBL1	0x09
>> +
>> +#define MAX77654_REG_INTM_GLBL1	0x08
>> +#define MAX77654_REG_INTM_GLBL0	0x09
>> +
>> +#define MAX77643_REG_INT_GLBL1	0x01
>> +#define MAX77643_REG_INTM_GLBL0	0x04
>> +#define MAX77643_REG_INTM_GLBL1	0x05
>> +
>> +#define MAX77658_BIT_INT_GLBL0_GPIO0_F	BIT(0)
>> +#define MAX77658_BIT_INT_GLBL0_GPIO0_R	BIT(1)
>> +#define MAX77658_BIT_INT_GLBL0_EN_F	BIT(2)
>> +#define MAX77658_BIT_INT_GLBL0_EN_R	BIT(3)
>> +#define MAX77658_BIT_INT_GLBL0_TJAL1_R	BIT(4)
>> +#define MAX77658_BIT_INT_GLBL0_TJAL2_R	BIT(5)
>> +#define MAX77658_BIT_INT_GLBL0_DOD1_R	BIT(6)
>> +#define MAX77658_BIT_INT_GLBL0_DOD0_R	BIT(7)
>> +
>> +#define MAX77643_BIT_INT_GLBL0_DOD0_R	BIT(6)
>> +
>> +#define MAX77658_BIT_INT_GLBL1_GPI1_F	BIT(0)
>> +#define MAX77658_BIT_INT_GLBL1_GPI1_R	BIT(1)
>> +#define MAX77658_BIT_INT_GLBL1_SBB0_F	BIT(2)
>> +#define MAX77658_BIT_INT_GLBL1_SBB1_F	BIT(3)
>> +#define MAX77658_BIT_INT_GLBL1_SBB2_F	BIT(4)
>> +#define MAX77658_BIT_INT_GLBL1_LDO0_F	BIT(5)
>> +#define MAX77658_BIT_INT_GLBL1_LDO1_F	BIT(6)
>> +
>> +#define MAX77659_BIT_INT_GLBL1_SBB_TO	BIT(4)
>> +
>> +#define MAX77654_BIT_INT_GLBL1_GPI2_F	BIT(2)
>> +#define MAX77654_BIT_INT_GLBL1_GPI2_R	BIT(3)
>> +#define MAX77654_BIT_INT_GLBL1_SBB_TO	BIT(4)
>> +
>> +#define MAX77658_BIT_INT_THM		BIT(0)
>> +#define MAX77658_BIT_INT_CHG		BIT(1)
>> +#define MAX77658_BIT_INT_CHGIN		BIT(2)
>> +#define MAX77658_BIT_INT_TJ_REG		BIT(3)
>> +#define MAX77658_BIT_INT_CHGIN_CTRL	BIT(4)
>> +#define MAX77658_BIT_INT_SYS_CTRL	BIT(5)
>> +#define MAX77658_BIT_INT_SYS_CNFG	BIT(6)
>> +
>> +#define MAX776569_BIT_INT_SYS_CTRL	BIT(4)
>> +
>> +enum max77658_ids {
>> +	ID_MAX77643,
>> +	ID_MAX77654,
>> +	ID_MAX77658,
>> +	ID_MAX77659
>> +};
>> +
>> +struct chip_info {
>> +	enum max77658_ids id;
>> +	int n_devs;
>> +	const struct mfd_cell *devs;
>> +};
>> +
>> +struct device;
>> +struct regmap;
>> +struct regmap_irq_chip_data;
>> +
>> +struct max77658_dev {
>> +	struct device *dev;
>> +	const struct chip_info *chip;
>> +
>> +	int irq;
>> +	struct regmap_irq_chip_data *irqc_glbl0;
>> +	struct regmap_irq_chip_data *irqc_glbl1;
>> +	struct regmap_irq_chip_data *irqc_chg;
>> +
>> +	struct regmap *regmap;
>> +	struct regmap *regmap_fg;
>> +};
>> +
>> +#endif /* __MAX77658_MFD_H__ */
>> --
>> 2.25.1
>>
>
>--
>Lee Jones [李琼斯]

Best regards,
Zeynep

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ