Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1661742391-11378-1-git-send-email-u0084500@gmail.com>
 <1661742391-11378-3-git-send-email-u0084500@gmail.com> <20220912112652.5zz2bscbtcpiwrjd@mercury.elektranox.org>
In-Reply-To: <20220912112652.5zz2bscbtcpiwrjd@mercury.elektranox.org>
From:   ChiYuan Huang <u0084500@gmail.com>
Date:   Tue, 13 Sep 2022 11:08:22 +0800
Message-ID: <CADiBU3-uCLCDQ_2MvTPgJaLH7jeXJCwJc3AWF4JL1oFGXXpjsg@mail.gmail.com>
Subject: Re: [PATCH v4 2/3] power: supply: rt9471: Add Richtek RT9471 charger driver
To:     Sebastian Reichel <sebastian.reichel@collabora.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Matti Vaittinen <mazziesaccount@gmail.com>,
        =?UTF-8?B?5ri45a2Q6aao?= <alina_yu@richtek.com>,
        cy_huang <cy_huang@richtek.com>, alinayu829@gmail.com,
        Linux PM <linux-pm@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>, lkml <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Sebastian Reichel <sebastian.reichel@collabora.com> =E6=96=BC 2022=E5=B9=B4=
9=E6=9C=8812=E6=97=A5 =E9=80=B1=E4=B8=80 =E6=99=9A=E4=B8=8A7:26=E5=AF=AB=E9=
=81=93=EF=BC=9A
>
> Hi,
>
> On Mon, Aug 29, 2022 at 11:06:30AM +0800, cy_huang wrote:
> > From: ChiYuan Huang <cy_huang@richtek.com>
> >
> > Add support for the RT9471 3A 1-Cell Li+ battery charger.
> >
> > The RT9471 is a highly-integrated 3A switch mode battery charger with
> > low impedance power path to better optimize the charging efficiency.
> >
> > Co-developed-by: Alina Yu <alina_yu@richtek.com>
> > Signed-off-by: Alina Yu <alina_yu@richtek.com>
> > Signed-off-by: ChiYuan Huang <cy_huang@richtek.com>
> > ---
> > Since v4:
> > - Remove the line for the owner field in driver.
> >
> > Since v2:
> > - Fix checkpatch error about 'foo * bar' to 'foo *bar' in psy_device_to=
_chip function.
> > - Specify the member name directly for the use of linear range.
> >
> > ---
>
> Thanks, driver looks mostly good.
>
> >  drivers/power/supply/Kconfig  |  16 +
> >  drivers/power/supply/Makefile |   1 +
> >  drivers/power/supply/rt9471.c | 952 ++++++++++++++++++++++++++++++++++=
++++++++
> >  drivers/power/supply/rt9471.h |  76 ++++
> >  4 files changed, 1045 insertions(+)
> >  create mode 100644 drivers/power/supply/rt9471.c
> >  create mode 100644 drivers/power/supply/rt9471.h
> >
> > [...]
> > +static inline int rt9471_set_hiz(struct rt9471_chip *chip, int enable)
> > +{
> > +     return regmap_field_write(chip->rm_fields[F_HZ], enable);
> > +}
> > +
> > +static inline int rt9471_set_ichg(struct rt9471_chip *chip, int microa=
mp)
> > +{
> > +     return rt9471_set_value_by_field_range(chip, F_ICHG_REG,
> > +                                            RT9471_RANGE_ICHG, microam=
p);
> > +}
> > +
> > +static inline int rt9471_get_ichg(struct rt9471_chip *chip, int *micro=
amp)
> > +{
> > +     return rt9471_get_value_by_field_range(chip, F_ICHG_REG,
> > +                                            RT9471_RANGE_ICHG, microam=
p);
> > +}
> > +
> > +static inline int rt9471_set_cv(struct rt9471_chip *chip, int microvol=
t)
> > +{
> > +     return rt9471_set_value_by_field_range(chip, F_VBAT_REG,
> > +                                            RT9471_RANGE_VCHG, microvo=
lt);
> > +}
> > +
> > +static inline int rt9471_get_cv(struct rt9471_chip *chip, int *microam=
p)
> > +{
> > +     return rt9471_get_value_by_field_range(chip, F_VBAT_REG,
> > +                                            RT9471_RANGE_VCHG, microam=
p);
> > +}
> > +
> > +static inline int rt9471_set_mivr(struct rt9471_chip *chip, int microv=
olt)
> > +{
> > +     return rt9471_set_value_by_field_range(chip, F_MIVR,
> > +                                            RT9471_RANGE_MIVR, microvo=
lt);
> > +}
> > +
> > +static inline int rt9471_get_mivr(struct rt9471_chip *chip, int *micro=
volt)
> > +{
> > +     return rt9471_get_value_by_field_range(chip, F_MIVR,
> > +                                            RT9471_RANGE_MIVR, microvo=
lt);
> > +}
> > +
> > +static inline int rt9471_set_aicr(struct rt9471_chip *chip, int microa=
mp)
> > +{
> > +     return rt9471_set_value_by_field_range(chip, F_AICR, RT9471_RANGE=
_AICR,
> > +                                            microamp);
> > +}
> > +
> > +static inline int rt9471_get_aicr(struct rt9471_chip *chip, int *micro=
amp)
> > +{
> > +     return rt9471_get_value_by_field_range(chip, F_AICR, RT9471_RANGE=
_AICR,
> > +                                            microamp);
> > +}
> > +
> > +static inline int rt9471_set_iprechg(struct rt9471_chip *chip, int mic=
roamp)
> > +{
> > +     return rt9471_set_value_by_field_range(chip, F_IPRE_CHG,
> > +                                            RT9471_RANGE_IPRE, microam=
p);
> > +}
> > +
> > +static inline int rt9471_get_iprechg(struct rt9471_chip *chip, int *mi=
croamp)
> > +{
> > +     return rt9471_get_value_by_field_range(chip, F_IPRE_CHG,
> > +                                            RT9471_RANGE_IPRE, microam=
p);
> > +}
> > +
> > +static inline int rt9471_set_ieoc(struct rt9471_chip *chip, int microa=
mp)
> > +{
> > +     return rt9471_set_value_by_field_range(chip, F_IEOC_CHG,
> > +                                            RT9471_RANGE_IEOC, microam=
p);
> > +}
> > +
> > +static inline int rt9471_get_ieoc(struct rt9471_chip *chip, int *micro=
amp)
> > +{
> > +     return rt9471_get_value_by_field_range(chip, F_IEOC_CHG,
> > +                                            RT9471_RANGE_IEOC, microam=
p);
> > +}
> > +
> > +static inline int rt9471_set_chg_enable(struct rt9471_chip *chip, int =
enable)
> > +{
> > +     return regmap_field_write(chip->rm_fields[F_CHG_EN], !!enable);
> > +}
>
> Please drop these one line wrappers.
Will only keep set_ieoc/get_ieoc function, remove 'inline'
declaration, and integrate CHG_TE switch state.
>
> > [...]
> > diff --git a/drivers/power/supply/rt9471.h b/drivers/power/supply/rt947=
1.h
> > new file mode 100644
> > index 00000000..f3d8e23
> > --- /dev/null
> > +++ b/drivers/power/supply/rt9471.h
> > @@ -0,0 +1,76 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/* Copyright (C) 2022 Richtek Technology Corp. */
> > +
> > +#ifndef __RT9471_CHARGER_H
> > +#define __RT9471_CHARGER_H
> > +
> > +#define RT9471_IRQ_BC12_DONE 0
> > +#define RT9471_IRQ_DETACH    1
> > +#define RT9471_IRQ_RECHG     2
> > +#define RT9471_IRQ_CHG_DONE  3
> > +#define RT9471_IRQ_BG_CHG    4
> > +#define RT9471_IRQ_IE0C              5
> > +#define RT9471_IRQ_CHG_RDY   6
> > +#define RT9471_IRQ_VBUS_GD   7
> > +#define RT9471_IRQ_CHG_BATOV 9
> > +#define RT9471_IRQ_CHG_SYSOV 10
> > +#define RT9471_IRQ_CHG_TOUT  11
> > +#define RT9471_IRQ_CHG_BUSUV 12
> > +#define RT9471_IRQ_CHG_THREG 13
> > +#define RT9471_IRQ_CHG_AICR  14
> > +#define RT9471_IRQ_CHG_MIVR  15
> > +#define RT9471_IRQ_SYS_SHORT 16
> > +#define RT9471_IRQ_SYS_MIN   17
> > +#define RT9471_IRQ_AICC_DONE 18
> > +#define RT9471_IRQ_PE_DONE   19
> > +#define RT9471_IRQ_JEITA_COLD        20
> > +#define RT9471_IRQ_JEITA_COOL        21
> > +#define RT9471_IRQ_JEITA_WARM        22
> > +#define RT9471_IRQ_JEITA_HOT 23
> > +#define RT9471_IRQ_OTG_FAULT 24
> > +#define RT9471_IRQ_OTG_LBP   25
> > +#define RT9471_IRQ_OTG_CC    26
> > +#define RT9471_IRQ_WDT               29
> > +#define RT9471_IRQ_VAC_OV    30
> > +#define RT9471_IRQ_OTP               31
> > +
> > +#define RT9471_REG_OTGCFG    0x00
> > +#define RT9471_REG_TOP               0x01
> > +#define RT9471_REG_FUNC              0x02
> > +#define RT9471_REG_IBUS              0x03
> > +#define RT9471_REG_VBUS              0x04
> > +#define RT9471_REG_PRECHG    0x05
> > +#define RT9471_REG_VCHG              0x07
> > +#define RT9471_REG_ICHG              0x08
> > +#define RT9471_REG_CHGTMR    0x09
> > +#define RT9471_REG_EOC               0x0A
> > +#define RT9471_REG_INFO              0x0B
> > +#define RT9471_REG_JEITA     0x0C
> > +#define RT9471_REG_PUMP_EXP  0x0D
> > +#define      RT9471_REG_DPDMDET      0x0E
> > +#define RT9471_REG_ICSTAT    0x0F
> > +#define      RT9471_REG_STAT0        0x10
> > +#define RT9471_REG_STAT1     0x11
> > +#define RT9471_REG_STAT2     0x12
> > +#define RT9471_REG_IRQ0              0x20
> > +#define RT9471_REG_MASK0     0x30
> > +
> > +#define RT9471_OTGCV_MASK    GENMASK(7, 6)
> > +#define RT9471_OTGCC_MASK    BIT(0)
> > +#define RT9471_OTGEN_MASK    BIT(1)
> > +#define RT9471_CHGFAULT_MASK GENMASK(4, 1)
> > +
> > +/* Device ID */
> > +#define RT9470_DEVID         0x09
> > +#define RT9470D_DEVID                0x0A
> > +#define RT9471_DEVID         0x0D
> > +#define RT9471D_DEVID                0x0E
> > +
> > +#define RT9471_NUM_IRQ_REGS  4
> > +#define RT9471_OTGCV_MINUV   4850000
> > +#define RT9471_OTGCV_STEPUV  150000
> > +#define RT9471_NUM_VOTG              4
> > +#define RT9471_VCHG_MAXUV    4700000
> > +#define RT9471_ICHG_MAXUA    3150000
> > +
> > +#endif /* __RT9471_CHARGER_H */
>
> Please merge this into rt9471.c
>
> > --
> > 2.7.4
> >