linux-kernel - Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759 DCDC converter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 25 Mar 2022 22:59:29 +0800
From:   ChiYuan Huang <u0084500@...il.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     Mark Brown <broonie@...nel.org>, Rob Herring <robh+dt@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        cy_huang <cy_huang@...htek.com>, gene_chen@...htek.com,
        lkml <linux-kernel@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH 2/2] regulator: rt5759: Add support for Richtek RT5759
 DCDC converter

Krzysztof Kozlowski <krzk@...nel.org> 於 2022年3月25日 週五 下午10:47寫道：
>
> On 25/03/2022 15:10, ChiYuan Huang wrote:
> > Krzysztof Kozlowski <krzk@...nel.org> 於 2022年3月25日 週五 下午8:17寫道：
> >>
> >> On 25/03/2022 02:06, cy_huang wrote:
> >>> From: ChiYuan Huang <cy_huang@...htek.com>
> >>>
> >>> Add support for Richtek RT5759 high-performance DCDC converter.
> >>>
> >>> Signed-off-by: ChiYuan Huang <cy_huang@...htek.com>
> >>> ---
> >>>  drivers/regulator/Kconfig            |  10 +
> >>>  drivers/regulator/Makefile           |   1 +
> >>>  drivers/regulator/rt5759-regulator.c | 372 +++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 383 insertions(+)
> >>>  create mode 100644 drivers/regulator/rt5759-regulator.c
> >>>
> >>
> >> (...)
> >>
> >>> +static int rt5759_init_device_property(struct rt5759_priv *priv)
> >>> +{
> >>> +     unsigned int val = 0;
> >>> +     bool wdt_enable;
> >>> +
> >>> +     /*
> >>> +      * Only RT5759A support external watchdog input
> >>> +      */
> >>> +     if (priv->chip_type != CHIP_TYPE_RT5759A)
> >>> +             return 0;
> >>> +
> >>> +     wdt_enable = device_property_read_bool(priv->dev,
> >>> +                                            "richtek,watchdog-enable");
> >>> +     if (wdt_enable)
> >>
> >> No need for separate wdt_enable variable.
> >>
> > Ack in next.
> >>> +             val = RT5759A_WDTEN_MASK;
> >>> +
> >>> +     return regmap_update_bits(priv->regmap, RT5759A_REG_WDTEN,
> >>> +                               RT5759A_WDTEN_MASK, val);
> >>> +}
> >>> +
> >>> +static int rt5759_manufacturer_check(struct rt5759_priv *priv)
> >>> +{
> >>> +     unsigned int vendor;
> >>> +     int ret;
> >>> +
> >>> +     ret = regmap_read(priv->regmap, RT5759_REG_VENDORINFO, &vendor);
> >>> +     if (ret)
> >>> +             return ret;
> >>> +
> >>> +     if (vendor != RT5759_MANUFACTURER_ID) {
> >>> +             dev_err(priv->dev, "vendor info not correct (%d)\n", vendor);
> >>> +             return -EINVAL;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static bool rt5759_is_accessible_reg(struct device *dev, unsigned int reg)
> >>> +{
> >>> +     struct rt5759_priv *priv = dev_get_drvdata(dev);
> >>> +
> >>> +     if (reg <= RT5759_REG_DCDCSET)
> >>> +             return true;
> >>> +
> >>> +     if (priv->chip_type == CHIP_TYPE_RT5759A && reg == RT5759A_REG_WDTEN)
> >>> +             return true;
> >>> +
> >>> +     return false;
> >>> +}
> >>> +
> >>> +static const struct regmap_config rt5759_regmap_config = {
> >>> +     .reg_bits = 8,
> >>> +     .val_bits = 8,
> >>> +     .max_register = RT5759A_REG_WDTEN,
> >>> +     .readable_reg = rt5759_is_accessible_reg,
> >>> +     .writeable_reg = rt5759_is_accessible_reg,
> >>> +};
> >>> +
> >>> +static int rt5759_probe(struct i2c_client *i2c)
> >>> +{
> >>> +     struct rt5759_priv *priv;
> >>> +     int ret;
> >>> +
> >>> +     priv = devm_kzalloc(&i2c->dev, sizeof(*priv), GFP_KERNEL);
> >>> +     if (!priv)
> >>> +             return -ENOMEM;
> >>> +
> >>> +     priv->dev = &i2c->dev;
> >>> +     priv->chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);
> >>> +     i2c_set_clientdata(i2c, priv);
> >>> +
> >>> +     priv->regmap = devm_regmap_init_i2c(i2c, &rt5759_regmap_config);
> >>> +     if (IS_ERR(priv->regmap)) {
> >>> +             ret = PTR_ERR(priv->regmap);
> >>> +             dev_err(&i2c->dev, "Failed to allocate regmap (%d)\n", ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = rt5759_manufacturer_check(priv);
> >>> +     if (ret) {
> >>> +             dev_err(&i2c->dev, "Failed to check device (%d)\n", ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     ret = rt5759_init_device_property(priv);
> >>> +     if (ret) {
> >>> +             dev_err(&i2c->dev, "Failed to init device (%d)\n", ret);
> >>> +             return ret;
> >>> +     }
> >>> +
> >>> +     return rt5759_regulator_register(priv);
> >>> +}
> >>> +
> >>> +static const struct of_device_id __maybe_unused rt5759_device_table[] = {
> >>
> >> I don't think this can be __maybe_unused. It is always referenced via
> >> of_match_table, isn't it?
> >>
> > I think it can declared as '__maybe_unused'.
> > If 'of_device_id' is unused, then in probe stage,
> > 'of_device_get_match_data' will return NULL.
>
> But your of_device_id cannot be unused. It is always referenced.
>
I'm not sure, but your assumption is based on 'CONFIG_OF', right?
Only if 'CONFIG_OF' is not defined, then it'll be really unused.

> > priv->chip_type will get zero as the return value. And it will be
> > treated as rt5759, not rt5759a.
> > The difference between these two are only watchdog function supported or not.
>
>
> Best regards,
> Krzysztof