Re: [PATCH 2/2] power: supply: rt9471: Add Richtek RT9471 charger driver

From: ChiYuan Huang
Date: Mon Aug 15 2022 - 02:11:25 EST

Matti Vaittinen <mazziesaccount@xxxxxxxxx> 於 2022年8月15日 週一 下午1:53寫道：
>
> Hi ChiYuan,
>
> Thanks for the patch :)
>
> to 11. elok. 2022 klo 16.43 cy_huang (u0084500@xxxxxxxxx) kirjoitti:
> >
> > From: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Alina Yu <alina_yu@xxxxxxxxxxx>
> > Signed-off-by: ChiYuan Huang <cy_huang@xxxxxxxxxxx>
> > ---
>
> > +
> > +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.
>
Yes, sure, if something changed for this structure, this could be a problem.
Thanks for the comment.
> > +
> > +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"
>
I'm not sure what the 'subsystem standard' is.
I declare it as 'inline' function and the function name to tell the
user what I'm doing.
This may be silly. But from my aspect, it makes each property set/get
more clear.
> 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));