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

From: Krzysztof Kozlowski
Date: Mon Aug 08 2022 - 09:42:42 EST

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@xxxxxxxxxx> wrote:
>> On 08/08/2022 10:34, Yassine Oudjana wrote:
>>> From: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> Signed-off-by: Alejandro Tafalla <atafalla@xxxxxxxxx>
>>> Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
>>>
>>> [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@xxxxxxxxxxxxxxx
>>> L: linux-arm-msm@xxxxxxxxxxxxxxx
>>> 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@xxxxxxxxxx>
>>> 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