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: <1660225318-4063-1-git-send-email-u0084500@gmail.com> <1660225318-4063-3-git-send-email-u0084500@gmail.com>
In-Reply-To: <1660225318-4063-3-git-send-email-u0084500@gmail.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Date:   Mon, 15 Aug 2022 08:53:28 +0300
Message-ID: <CANhJrGMnjSu3MMQ2PgvSWztNgG1r-PdyLBdsT3JVqZZSfqky-w@mail.gmail.com>
Subject: Re: [PATCH 2/2] power: supply: rt9471: Add Richtek RT9471 charger driver
To:     cy_huang <u0084500@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        krzysztof.kozlowski+dt@linaro.org,
        Sebastian Reichel <sre@kernel.org>, alina_yu@richtek.com,
        ChiYuan Huang <cy_huang@richtek.com>, alinayu829@gmail.com,
        Linux PM list <linux-pm@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi ChiYuan,

Thanks for the patch :)

to 11. elok. 2022 klo 16.43 cy_huang (u0084500@gmail.com) kirjoitti:
>
> 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>
> ---

> +
> +static const struct linear_range rt9471_chg_ranges[RT9471_MAX_RANGES] = {
> +       [RT9471_RANGE_AICR] = { 50000, 1, 63, 50000 },
> +       [RT9471_RANGE_MIVR] = { 3900000, 0, 15, 100000 },
> +       [RT9471_RANGE_IPRE] = { 50000, 0, 15, 50000 },
> +       [RT9471_RANGE_VCHG] = { 3900000, 0, 80, 10000 },
> +       [RT9471_RANGE_ICHG] = { 0, 0, 63, 50000 },
> +       [RT9471_RANGE_IEOC] = { 50000, 0, 15, 50000 },
> +};

I just jumped in to ask if that could you please use the field names? Eg.
 { .min = 50000, .min_sel = 1, .max_sel = 63, .step = 50000 },

This would make it less error prone in case someone changes the
members in struct linear_range.

> +
> +static int rt9471_set_value_by_field_range(struct rt9471_chip *chip,
> +                                          enum rt9471_fields field,
> +                                          enum rt9471_ranges range, int val)
> +{
> +       unsigned int sel;
> +
> +       if (val < 0)
> +               return -EINVAL;
> +
> +       linear_range_get_selector_within(rt9471_chg_ranges + range, val, &sel);
> +
> +       return regmap_field_write(chip->rm_fields[field], sel);
> +}
> +
> +
> +static int rt9471_get_value_by_field_range(struct rt9471_chip *chip,
> +                                          enum rt9471_fields field,
> +                                          enum rt9471_ranges range, int *val)
> +{
> +       unsigned int sel, rvalue;
> +       int ret;
> +
> +       ret = regmap_field_read(chip->rm_fields[field], &sel);
> +       if (ret)
> +               return ret;
> +
> +       ret = linear_range_get_value(rt9471_chg_ranges + range, sel, &rvalue);
> +       if (ret)
> +               return ret;
> +
> +       *val = rvalue;
> +       return 0;
> +}
> +
> +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 microamp)
> +{
> +       return rt9471_set_value_by_field_range(chip, F_ICHG_REG,
> +                                              RT9471_RANGE_ICHG, microamp);
> +}
> +
> +static inline int rt9471_get_ichg(struct rt9471_chip *chip, int *microamp)
> +{
> +       return rt9471_get_value_by_field_range(chip, F_ICHG_REG,
> +                                              RT9471_RANGE_ICHG, microamp);
> +}
> +
> +static inline int rt9471_set_cv(struct rt9471_chip *chip, int microvolt)
> +{
> +       return rt9471_set_value_by_field_range(chip, F_VBAT_REG,
> +                                              RT9471_RANGE_VCHG, microvolt);
> +}
> +
> +static inline int rt9471_get_cv(struct rt9471_chip *chip, int *microamp)
> +{
> +       return rt9471_get_value_by_field_range(chip, F_VBAT_REG,
> +                                              RT9471_RANGE_VCHG, microamp);
> +}
> +
> +static inline int rt9471_set_mivr(struct rt9471_chip *chip, int microvolt)
> +{
> +       return rt9471_set_value_by_field_range(chip, F_MIVR,
> +                                              RT9471_RANGE_MIVR, microvolt);
> +}
> +
> +static inline int rt9471_get_mivr(struct rt9471_chip *chip, int *microvolt)
> +{
> +       return rt9471_get_value_by_field_range(chip, F_MIVR,
> +                                              RT9471_RANGE_MIVR, microvolt);
> +}
> +
> +static inline int rt9471_set_aicr(struct rt9471_chip *chip, int microamp)
> +{
> +       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 *microamp)
> +{
> +       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 microamp)
> +{
> +       return rt9471_set_value_by_field_range(chip, F_IPRE_CHG,
> +                                              RT9471_RANGE_IPRE, microamp);
> +}
> +
> +static inline int rt9471_get_iprechg(struct rt9471_chip *chip, int *microamp)
> +{
> +       return rt9471_get_value_by_field_range(chip, F_IPRE_CHG,
> +                                              RT9471_RANGE_IPRE, microamp);
> +}
> +
> +static inline int rt9471_set_ieoc(struct rt9471_chip *chip, int microamp)
> +{
> +       return rt9471_set_value_by_field_range(chip, F_IEOC_CHG,
> +                                              RT9471_RANGE_IEOC, microamp);
> +}
> +
> +static inline int rt9471_get_ieoc(struct rt9471_chip *chip, int *microamp)
> +{
> +       return rt9471_get_value_by_field_range(chip, F_IEOC_CHG,
> +                                              RT9471_RANGE_IEOC, microamp);
> +}
> +
> +static inline int rt9471_set_chg_enable(struct rt9471_chip *chip, int enable)
> +{
> +       return regmap_field_write(chip->rm_fields[F_CHG_EN], !!enable);
> +}
> +

//snip

> +
> +static inline struct rt9471_chip * psy_device_to_chip(struct device *dev)
> +{
> +       return power_supply_get_drvdata(to_power_supply(dev));
> +}

While skimming through the rest of the patch... This may just be my
personal preference but wrapper functions with just one line are
rarely beneficial. In the worst case they just add more lines AND hide
the details of what actually is done without any clear benefits. Well,
this is just my view so please ignore this comment if wrappers like
this are a "subsystem standard"

Other than that the patch looks good to me.

--

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));