Re: [PATCH 8/8] power: supply: Add driver for Qualcomm SMBCHG

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 08/08/2022 13:05, Yassine Oudjana wrote:
> 
> On Mon, Aug 8 2022 at 11:55:02 +03:00:00, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 08/08/2022 10:34, Yassine Oudjana wrote:
>>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>>  Add a driver for the switch-mode battery charger found on
>>>  PMICs such as PMI8994. This block is referred to in the vendor
>>>  kernel[1] as smbcharger or SMBCHG. It has USB and DC inputs,
>>>  and can generate VBUS for USB OTG with a boost regulator.
>>>  It supports Qualcomm Quick Charge 2.0, and can operate along
>>>  with a parallel charger (SMB1357, or SMB1351 for added Quick
>>>  Charge 3.0 support) for improved efficiency at higher currents.
>>>
>>>  At the moment, this driver supports charging off of the USB input
>>>  at 5V with input current limit up to 3A. It also includes support
>>>  for operating the OTG boost regulator as well as extcon
>>>  functionality, reporting states of USB and USB_HOST with VBUS and
>>>  charge port types.
>>>
>>>  Co-developed-by: Alejandro Tafalla <atafalla@dnyon.com>
>>>  Signed-off-by: Alejandro Tafalla <atafalla@dnyon.com>
>>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>>  [1] 
>>> https://github.com/android-linux-stable/msm-3.18/blob/kernel.lnx.3.18.r34-rel/drivers/power/qpnp-smbcharger.c
>>>  ---
>>>   MAINTAINERS                        |    2 +
>>>   drivers/power/supply/Kconfig       |   11 +
>>>   drivers/power/supply/Makefile      |    1 +
>>>   drivers/power/supply/qcom-smbchg.c | 1664 
>>> ++++++++++++++++++++++++++++
>>>   drivers/power/supply/qcom-smbchg.h |  428 +++++++
>>>   5 files changed, 2106 insertions(+)
>>>   create mode 100644 drivers/power/supply/qcom-smbchg.c
>>>   create mode 100644 drivers/power/supply/qcom-smbchg.h
>>>
>>>  diff --git a/MAINTAINERS b/MAINTAINERS
>>>  index f6cf3a27d132..9b8693050890 100644
>>>  --- a/MAINTAINERS
>>>  +++ b/MAINTAINERS
>>>  @@ -16964,6 +16964,8 @@ L:	linux-pm@vger.kernel.org
>>>   L:	linux-arm-msm@vger.kernel.org
>>>   S:	Maintained
>>>   F:	Documentation/devicetree/bindings/power/supply/qcom,smbchg.yaml
>>>  +F:	drivers/power/supply/qcom-smbchg.c
>>>  +F:	drivers/power/supply/qcom-smbchg.h
>>>
>>>   QUALCOMM TSENS THERMAL DRIVER
>>>   M:	Amit Kucheria <amitk@kernel.org>
>>>  diff --git a/drivers/power/supply/Kconfig 
>>> b/drivers/power/supply/Kconfig
>>>  index 1aa8323ad9f6..246bfc118d9f 100644
>>>  --- a/drivers/power/supply/Kconfig
>>>  +++ b/drivers/power/supply/Kconfig
>>>  @@ -633,6 +633,17 @@ config CHARGER_QCOM_SMBB
>>>   	  documentation for more detail.  The base name for this driver is
>>>   	  'pm8941_charger'.
>>>
>>>  +config CHARGER_QCOM_SMBCHG
>>>  +	tristate "Qualcomm Switch-Mode Battery Charger"
>>
>> As I mentioned in cover letter, this should be either squashed into
>> Caleb's work, merged into some common part or kept separate but with
>> clear explaining why it cannot be merged.
>>
>> Some incomplete review follows:
>>
>>>  +	depends on MFD_SPMI_PMIC || COMPILE_TEST
>>>  +	depends on OF
>>>  +	depends on EXTCON
>>>  +	depends on REGULATOR
>>>  +	select QCOM_PMIC_SEC_WRITE
>>>  +	help
>>>  +	  Say Y to include support for the Switch-Mode Battery Charger 
>>> block
>>>  +	  found in Qualcomm PMICs such as PMI8994.
>>>  +
>>>   config CHARGER_BQ2415X
>>>   	tristate "TI BQ2415x battery charger driver"
>>>   	depends on I2C
>>>  diff --git a/drivers/power/supply/Makefile 
>>> b/drivers/power/supply/Makefile
>>>  index 7f02f36aea55..7c2c037cd8b1 100644
>>>  --- a/drivers/power/supply/Makefile
>>>  +++ b/drivers/power/supply/Makefile
>>>  @@ -83,6 +83,7 @@ obj-$(CONFIG_CHARGER_MAX8998)	+= max8998_charger.o
>>>   obj-$(CONFIG_CHARGER_MP2629)	+= mp2629_charger.o
>>>   obj-$(CONFIG_CHARGER_MT6360)	+= mt6360_charger.o
>>>   obj-$(CONFIG_CHARGER_QCOM_SMBB)	+= qcom_smbb.o
>>>  +obj-$(CONFIG_CHARGER_QCOM_SMBCHG)	+= qcom-smbchg.o
>>>   obj-$(CONFIG_CHARGER_BQ2415X)	+= bq2415x_charger.o
>>>   obj-$(CONFIG_CHARGER_BQ24190)	+= bq24190_charger.o
>>>   obj-$(CONFIG_CHARGER_BQ24257)	+= bq24257_charger.o
>>>  diff --git a/drivers/power/supply/qcom-smbchg.c 
>>> b/drivers/power/supply/qcom-smbchg.c
>>>  new file mode 100644
>>>  index 000000000000..23a9667953c9
>>>  --- /dev/null
>>>  +++ b/drivers/power/supply/qcom-smbchg.c
>>>  @@ -0,0 +1,1664 @@
>>>  +// SPDX-License-Identifier: GPL-2.0-only
>>
>> Several things look like based from original sources, so please retain
>> original copyright.
> 
> Do I replace the existing copyright here with the original one, just 
> add it, or mention that this driver is based on downstream sources 
> (maybe putting a link as well) then add it?

Add original copyright and optionally mention that it is based on
downstream source. Links are not needed.

>>
>>>  +
>>>  +static int smbchg_probe(struct platform_device *pdev)
>>>  +{
>>>  +	struct smbchg_chip *chip;
>>>  +	struct regulator_config config = {};
>>>  +	struct power_supply_config supply_config = {};
>>>  +	int i, irq, ret;
>>>  +
>>>  +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>>  +	if (!chip)
>>>  +		return -ENOMEM;
>>>  +
>>>  +	chip->dev = &pdev->dev;
>>>  +
>>>  +	chip->regmap = dev_get_regmap(chip->dev->parent, NULL);
>>>  +	if (!chip->regmap) {
>>>  +		dev_err(chip->dev, "Failed to get regmap\n");
>>>  +		return -ENODEV;
>>>  +	}
>>>  +
>>>  +	ret = of_property_read_u32(chip->dev->of_node, "reg", 
>>> &chip->base);
>>
>> First: device_xxx
> 
> Okay.
> 
>> Second: what if address is bigger than u32? Shouldn't this be
>> of_read_number or something similar in device_xxx API?
> 
> The address wouldn't exceed sizeof(u16). Actually now I think I 
> should've used property_read_u16 instead. I couldn't find a device_* 
> equivalent of of_read_number (or at least not a direct one), are you 
> sure it exists?

I think u16 would be confusing as reg size is minimum u32 (with
address-cells==1). Instead of of_read_number(), maybe this should be
of_get_address() (see pm8941-pwrkey.c), but there is no device_xxx()
equivalent. Still I think it would be the most appropriate to parse
actual address.



Best regards,
Krzysztof