Message-ID: <CACRpkda833CiwA+ihMLm6zzTFsoCMFoesbAVrW7EMA0-vGFTFQ@mail.gmail.com>
Date: Tue, 14 Oct 2025 12:23:07 +0200
From: Linus Walleij <linus.walleij@...aro.org>
To: 1647395606@...com
Cc: sre@...nel.org, brgl@...ev.pl, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-gpio@...r.kernel.org, 
	wangwenqiang <wenqiang.wang@...ot.com>
Subject: Re: [PATCH] power: supply: Add SC8541 charger drivers

Hi Wang,

thanks for your patch!

On Mon, Oct 13, 2025 at 10:54 AM <1647395606@...com> wrote:

> From: wangwenqiang <wenqiang.wang@...ot.com>
>
> The SC8541 is a charger pump from South Chip.
> By adjusting the voltage difference between the input and output terminals,
> it can achieve a maximum charging current of 8A.
> It has been verified that this driver can operate normally on the Qualcomm QCS615 platform.
>
> Signed-off-by: wangwenqiang <wenqiang.wang@...ot.com>

(...)
> +#include <linux/gpio.h>

Don't use this legacy API, include <linux/gpio/consumer.h> if you need
to use GPIO lines and use GPIO descriptors.

> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/kthread.h>

Are you using sched.h and thread.h really?
You are using threaded interrupt handlers but that is
not coming from this file but interrupt.h.

> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>

Don't use this legacy API either, use <linux/gpio/consumer.h>

> +#include <linux/err.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/regulator/machine.h>

I don't think you're defining regulator machines either.

> +#include <linux/debugfs.h>
> +#include <linux/bitops.h>
> +#include <linux/math64.h>
> +#include <linux/regmap.h>

Please go over the includes and make sure you only include the ones
you really need.

> +#define SC8541_DRV_VERSION              "1.0.0_G"

I would just drop this, we don't use these kind of strings much these days.

> +struct flag_bit {
> +    int notify;
> +    int mask;
> +    char *name;
> +};
> +
> +struct intr_flag {
> +    int reg;
> +    int len;
> +    struct flag_bit bit[8];
> +};

Are these things such as notify, mask, reg, len really int?
Aren't they better as u32?

> +struct reg_range {
> +    u32 min;
> +    u32 max;
> +    u32 step;
> +    u32 offset;
> +    const u32 *table;
> +    u16 num_table;
> +    bool round_up;
> +};

Here is some nice use of proper types!

> +static const struct regmap_config sc8541_regmap_config = {
> +    .reg_bits = 8,
> +    .val_bits = 8,
> +
> +    .max_register = SC8541_REGMAX,
> +};

Nice use of regmap.

> +struct sc8541_chip {
> +    struct device *dev;
> +    struct i2c_client *client;
> +    struct regmap *regmap;
> +    struct regmap_field *rmap_fields[F_MAX_FIELDS];
> +
> +    struct sc8541_cfg_e cfg;
> +    int irq_gpio;
> +    int irq;
> +
> +    int mode;
> +
> +    bool charge_enabled;
> +    int usb_present;
> +    int vbus_volt;
> +    int ibus_curr;
> +    int vbat_volt;
> +    int ibat_curr;
> +    int die_temp;

Can all of these really be negative? Otherwise use unsigned int.
I guess the die_temp should be int because it could be negative.

> +/********************COMMON API***********************/

Drop these headers, we can see it is a common API anyway.

> +__maybe_unused static u8 val2reg(enum sc8541_reg_range id, u32 val)
> +{
> +    int i;
> +    u8 reg;
> +    const struct reg_range *range = &sc8541_reg_range[id];
> +
> +    if (!range)
> +       return val;
> +
> +    if (range->table) {
> +       if (val <= range->table[0])
> +           return 0;
> +       for (i = 1; i < range->num_table - 1; i++) {
> +           if (val == range->table[i])
> +               return i;
> +           if (val > range->table[i] &&
> +               val < range->table[i + 1])
> +               return range->round_up ? i + 1 : i;
> +       }
> +       return range->num_table - 1;
> +    }
> +    if (val <= range->min)
> +       reg = 0;
> +    else if (val >= range->max)
> +       reg = (range->max - range->offset) / range->step;
> +    else if (range->round_up)
> +       reg = (val - range->offset) / range->step + 1;
> +    else
> +       reg = (val - range->offset) / range->step;
> +    return reg;
> +}

Add some description of what the function is doing, it's hard
to understand, kerneldoc please.

> +__maybe_unused static u32 reg2val(enum sc8541_reg_range id, u8 reg)
> +{
> +    const struct reg_range *range = &sc8541_reg_range[id];
> +
> +    if (!range)
> +       return reg;
> +    return range->table ? range->table[reg] :
> +                 range->offset + range->step * reg;
> +}

Same here.

> +static int sc8541_field_read(struct sc8541_chip *sc,
> +               enum sc8541_fields field_id, int *val)
> +{
> +    int ret;
> +
> +    ret = regmap_field_read(sc->rmap_fields[field_id], val);
> +    if (ret < 0) {
> +       dev_err(sc->dev, "sc8541 read field %d fail: %d\n", field_id, ret);
> +    }
> +
> +    return ret;
> +}
> +
> +static int sc8541_field_write(struct sc8541_chip *sc,
> +               enum sc8541_fields field_id, int val)
> +{
> +    int ret;
> +
> +    ret = regmap_field_write(sc->rmap_fields[field_id], val);
> +    if (ret < 0) {
> +       dev_err(sc->dev, "sc8541 read field %d fail: %d\n", field_id, ret);
> +    }
> +
> +    return ret;
> +}
> +
> +static int sc8541_read_block(struct sc8541_chip *sc,
> +               int reg, uint8_t *val, int len)
> +{
> +    int ret;
> +
> +    ret = regmap_bulk_read(sc->regmap, reg, val, len);
> +    if (ret < 0) {
> +       dev_err(sc->dev, "sc8541 read %02x block failed %d\n", reg, ret);
> +    }
> +
> +    return ret;
> +}

Does these three indirections really but you something? You just need to
handle the returned ret once more in the code. Isn't it better to just use
regmap_* field etc directly and report errors in the code.

> +__maybe_unused static int sc8541_reg_reset(struct sc8541_chip *sc)
> +{
> +    return sc8541_field_write(sc, REG_RST, 1);
> +}
> +
> +__maybe_unused static int sc8541_dump_reg(struct sc8541_chip *sc)
> +{
> +    int ret;
> +    int i;
> +    int val;
> +
> +    for (i = 0; i <= SC8541_REGMAX; i++) {
> +       ret = regmap_read(sc->regmap, i, &val);
> +       dev_err(sc->dev, "%s reg[0x%02x] = 0x%02x\n",
> +               __func__, i, val);
> +    }
> +
> +    return ret;
> +}
> +
> +__maybe_unused static int sc8541_enable_charge(struct sc8541_chip *sc, bool en)
> +{
> +    int ret;
> +
> +    dev_info(sc->dev, "%s:%d", __func__, en);
> +
> +    ret = sc8541_field_write(sc, CHG_EN, !!en);
> +
> +    return ret;
> +}
> +
> +
> +__maybe_unused static int sc8541_check_charge_enabled(struct sc8541_chip *sc, bool *enabled)
> +{
> +    int ret, val;
> +
> +    ret = sc8541_field_read(sc, CP_SWITCHING_STAT, &val);
> +
> +    *enabled = (bool)val;
> +
> +    dev_info(sc->dev, "%s:%d", __func__, val);
> +
> +    return ret;
> +}
> +
> +__maybe_unused static int sc8541_get_status(struct sc8541_chip *sc, uint32_t *status)
> +{
> +    int ret, val;
> +    *status = 0;
> +
> +    ret = sc8541_field_read(sc, VBUS_ERRORHI_STAT, &val);
> +    if (ret < 0) {
> +       dev_err(sc->dev, "%s fail to read VBUS_ERRORHI_STAT(%d)\n", __func__, ret);
> +       return ret;
> +    }
> +    if (val != 0)
> +       *status |= BIT(ERROR_VBUS_HIGH);
> +
> +    ret = sc8541_field_read(sc, VBUS_ERRORLO_STAT, &val);
> +    if (ret < 0) {
> +       dev_err(sc->dev, "%s fail to read VBUS_ERRORLO_STAT(%d)\n", __func__, ret);
> +       return ret;
> +    }
> +    if (val != 0)
> +       *status |= BIT(ERROR_VBUS_LOW);
> +
> +
> +    return ret;
> +
> +}
> +
> +__maybe_unused static int sc8541_enable_adc(struct sc8541_chip *sc, bool en)
> +{
> +    dev_info(sc->dev, "%s:%d", __func__, en);
> +    return sc8541_field_write(sc, ADC_EN, !!en);
> +}
> +
> +__maybe_unused static int sc8541_set_adc_scanrate(struct sc8541_chip *sc, bool oneshot)
> +{
> +    dev_info(sc->dev, "%s:%d", __func__, oneshot);
> +    return sc8541_field_write(sc, ADC_RATE, !!oneshot);
> +}

This overuse of __maybe_unused means you are potentially
compiling in a lot of crap that will not be used.

Consider using static inline in a .h file instead, then they will
just not be compiled if not used.

> +__maybe_unused static int sc8541_disable_vbusovp_alarm(struct sc8541_chip *sc, bool en)
> +{
> +    int ret;
> +
> +    dev_info(sc->dev, "%s:%d", __func__, en);

Convert all these to dev_debug() to not litter the log.

> +
> +    ret = sc8541_field_write(sc, VBUS_OVP_ALM_DIS, !!en);
> +
> +    return ret;
> +}

This kind of functions also look like unnecessary indirection to me
but at least rewrite them like this:

return sc8541_field_write(sc, VBUS_OVP_ALM_DIS, !!en);

and it saves you 4-5 lines of code in each functions ince you don't
need to declare ret and return it on a separate line.

> +static int mtk_sc8541_set_vbusovp(struct charger_device *chg_dev, u32 uV)
> +{
> +    struct sc8541_chip *sc = charger_get_data(chg_dev);
> +    int mv;
> +
> +    mv = uV / 1000;

If this is coming from an IIO ADC you can use the existing prescaler
in IIO instead.

ret = iio_read_channel_processed_scale(adc_main_charger_v,
                                                       &vch, 1000);

Otherwise, maybe your ADC  *should* be an IIO device, hm?

> +static int mtk_sc8541_get_adc(struct charger_device *chg_dev, enum adc_channel chan,
> +                         int *min, int *max)
> +{
> +    struct sc8541_chip *sc = charger_get_data(chg_dev);
> +
> +    sc8541_get_adc_data(sc, to_sc8541_adc(chan), max);
> +
> +    if (chan != ADC_CHANNEL_TEMP_JC)
> +       *max = *max * 1000;
> +
> +    if (min != max)
> +               *min = *max;
> +
> +    return 0;
> +}
> +
> +static int mtk_sc8541_get_adc_accuracy(struct charger_device *chg_dev,
> +                                  enum adc_channel chan, int *min, int *max)
> +{
> +    *min = *max = sc8541_adc_accuracy_tbl[to_sc8541_adc(chan)];
> +    return 0;
> +}

Yeah this looks like this part of the driver should be in
drivers/iio/adc/*

> +static int sc8541_register_interrupt(struct sc8541_chip *sc)
> +{
> +    int ret;
> +
> +    if (gpio_is_valid(sc->irq_gpio)) {
> +       ret = gpio_request_one(sc->irq_gpio, GPIOF_IN, "sc8541_irq");
> +       if (ret) {
> +            dev_err(sc->dev, "failed to request sc8541_irq\n");
> +           return -EINVAL;
> +       }
> +       sc->irq = gpio_to_irq(sc->irq_gpio);
> +       if (sc->irq < 0) {
> +            dev_err(sc->dev, "failed to gpio_to_irq\n");
> +           return -EINVAL;
> +       }

NACK do not use these old GPIO APIs.

Use devm_gpiod_get() instead.

Just grep in the kernel for many good examples of how to
do this:

struct gpio_desc *gd = devm_gpiod_get(dev, "irq", GPIOD_IN);
int irq = gpiod_to_irq(gd);
(etc)

> +    if (sc->irq) {
> +       ret = devm_request_threaded_irq(&sc->client->dev, sc->irq,
> +               NULL, sc8541_irq_handler,
> +               IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +               sc8541_irq_name[sc->mode], sc);
> +
> +       if (ret < 0) {
> +            dev_err(sc->dev, "request irq for irq=%d failed, ret =%d\n",
> +                           sc->irq, ret);
> +           return ret;
> +       }
> +       enable_irq_wake(sc->irq);

It's nice that you use threaded IRQs with oneshot!

> +static int sc8541_set_work_mode(struct sc8541_chip *sc, int mode)
> +{
> +    sc->mode = mode;
> +
> +    dev_err(sc->dev, "work mode is %s\n", sc->mode == SC8541_STANDALONE
> +       ? "standalone" : (sc->mode == SC8541_MASTER ? "master" : "slave"));

Why is this dev_err()?

It's not an error.

> +#ifdef CONFIG_PM_SLEEP
> +static int sc8541_suspend(struct device *dev)
> +{
> +    struct sc8541_chip *sc = dev_get_drvdata(dev);
> +
> +    dev_info(sc->dev, "Suspend successfully!");
> +    if (device_may_wakeup(dev))
> +       enable_irq_wake(sc->irq);
> +    disable_irq(sc->irq);
> +
> +    return 0;
> +}
> +static int sc8541_resume(struct device *dev)
> +{
> +    struct sc8541_chip *sc = dev_get_drvdata(dev);
> +
> +    dev_info(sc->dev, "Resume successfully!");
> +    if (device_may_wakeup(dev))
> +       disable_irq_wake(sc->irq);
> +    enable_irq(sc->irq);
> +
> +    return 0;
> +}
> +
> +static const struct dev_pm_ops sc8541_pm = {
> +    SET_SYSTEM_SLEEP_PM_OPS(sc8541_suspend, sc8541_resume)
> +};
> +#endif

IIRC there is a new API for this, look through recent changes to the
kernel concerning PM ops. I don't think you need the #ifdefs anymore.

> +MODULE_DESCRIPTION("SC SC8541 Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("South Chip <Aiden-yu@...thchip.com>");

I think this is supposed to be a real person, not a function, can you
put in your own name and mail instead?

Yours,
Linus Walleij

Powered by blists - more mailing lists