Message-ID: <CAHp75VeFdSWOxgKGEdyeRWqK2JSZ_Nw+AkaXXPnGFU-kuoazPQ@mail.gmail.com>
Date:	Fri, 6 Nov 2015 23:29:40 +0200
From:	Andy Shevchenko <andy.shevchenko@...il.com>
To:	Chen Feng <puck.chen@...ilicon.com>
Cc:	w.f@...wei.com, Samuel Ortiz <sameo@...ux.intel.com>,
	Lee Jones <lee.jones@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	"ijc+devicetree" <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, joro@...tes.org,
	iommu@...ts.linux-foundation.org, haojian.zhuang@...aro.org,
	devicetree <devicetree@...r.kernel.org>, xuwei5@...ilicon.com,
	xuyiping@...ilicon.com, kong.kongxinwei@...ilicon.com,
	z.liuxinliang@...ilicon.com, yudongbin@...ilicon.com,
	weidong2@...ilicon.com, saberlily.xia@...ilicon.com,
	haojian.zhuang@...look.com, leo.yan@...aro.org,
	linuxarm@...wei.com, dan.zhao@...ilicon.com,
	peter.panshilin@...ilicon.com, qijiwen@...ilicon.com
Subject: Re: [PATCH 6/7] regulator: hisilicon: Add hi655x pmic voltage
 regulator driver

On Thu, Nov 5, 2015 at 3:34 PM, Chen Feng <puck.chen@...ilicon.com> wrote:
> Add driver support for HiSilicon Hi655x voltage regulators.

> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/delay.h>
> +#include <linux/time.h>
> +#include <linux/regulator/hi655x-regulator.h>
> +#include <linux/mfd/hi655x-pmic.h>
> +#include <linux/regmap.h>
> +#include <linux/bitops.h>
> +
> +static int hi655x_is_enabled(struct regulator_dev *rdev)
> +{
> +       unsigned int value = 0;
> +
> +       struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +       struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;
> +
> +       regmap_read(rdev->regmap, ctrl_regs->status_reg, &value);
> +       return (value & BIT(regulator->ctrl_mask));
> +}
> +
> +static int hi655x_enable(struct regulator_dev *rdev)
> +{
> +       int ret = 0;
> +       struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +       struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;
> +
> +       ret = regmap_update_bits(rdev->regmap, ctrl_regs->enable_reg,
> +                                regulator->ctrl_mask, regulator->ctrl_mask);
> +       return ret;
> +}
> +
> +static int hi655x_disable(struct regulator_dev *rdev)
> +{
> +       int ret = 0;
> +       struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +       if (!regulator) {
> +               pr_err("get driver data error!\n");
> +               return -ENODEV;
> +       }
> +       struct hi655x_regulator_ctrl_regs *ctrl_regs = &regulator->ctrl_regs;
> +
> +       ret = regmap_update_bits(rdev->regmap, ctrl_regs->disable_reg,
> +                                regulator->ctrl_mask, regulator->ctrl_mask);
> +       return ret;
> +}
> +
> +static int hi655x_get_voltage(struct regulator_dev *rdev)
> +{
> +       unsigned int value = 0;
> +       struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +       if (!regulator) {
> +               pr_err("get driver data error!\n");
> +               return -ENODEV;
> +       }
> +       struct hi655x_regulator_vset_regs *vset_regs = &regulator->vset_regs;
> +
> +       regmap_read(rdev->regmap, vset_regs->vset_reg, &value);
> +
> +       return regulator->vset_table[value];
> +}
> +
> +static int hi655x_set_voltage(struct regulator_dev *rdev,
> +                             int min_uV, int max_uV, unsigned *selector)
> +{
> +       int i = 0;
> +       int ret = 0;
> +       int vol = 0;
> +       struct hi655x_regulator *regulator = rdev_get_drvdata(rdev);
> +
> +       if (!regulator) {
> +               pr_err("get driver data error!\n");
> +               return -ENODEV;
> +       }
> +
> +       struct hi655x_regulator_vset_regs *vset_regs = &regulator->vset_regs;
> +
> +       /**
> +        * search the matched vol and get its index
> +        */
> +       for (i = 0; i < regulator->vol_numb; i++) {
> +               vol = regulator->vset_table[i];
> +               if ((vol >= min_uV) && (vol <= max_uV))
> +                       break;
> +       }
> +
> +       if (i == regulator->vol_numb)
> +               return -1;
> +
> +       regmap_update_bits(rdev->regmap, vset_regs->vset_reg,
> +                          regulator->vset_mask, i);
> +       *selector = i;
> +
> +       return ret;
> +}
> +
> +static unsigned int hi655x_map_mode(unsigned int mode)
> +{
> +       /* hi655x pmic on hi6220 SoC only support normal mode */
> +       if (mode == REGULATOR_MODE_NORMAL)
> +               return REGULATOR_MODE_NORMAL;
> +       else
> +               return -EINVAL;
> +}
> +
> +static int hi655x_set_mode(struct regulator_dev *rdev,
> +                          unsigned int mode)
> +
> +{
> +       if (mode == REGULATOR_MODE_NORMAL)
> +               return 0;
> +       else
> +               return -EINVAL;
> +}
> +
> +static struct regulator_ops hi655x_regulator_ops = {
> +       .is_enabled = hi655x_is_enabled,
> +       .enable = hi655x_enable,
> +       .disable = hi655x_disable,
> +       .list_voltage = regulator_list_voltage_table,
> +       .get_voltage = hi655x_get_voltage,
> +       .set_voltage = hi655x_set_voltage,
> +       .set_mode = hi655x_set_mode,
> +};
> +
> +static const struct of_device_id of_hi655x_regulator_match_tbl[] = {
> +       {
> +               .compatible = "hisilicon,hi655x-regulator-pmic",
> +       },
> +};
> +MODULE_DEVICE_TABLE(of, of_hi655x_regulator_match_tbl);
> +
> +/**
> + * get the hi655x specific data from dt node.
> + */
> +static void of_get_hi655x_ctr(struct hi655x_regulator *regulator,
> +                             struct device *dev, struct device_node *np)
> +{
> +       unsigned int *vset_table = NULL;
> +
> +       of_property_read_u32_array(np, "regulator-ctrl-regs",

device property API?

> +                                  (u32 *)&regulator->ctrl_regs, 0x3);
> +       of_property_read_u32(np, "regulator-ctrl-mask", &regulator->ctrl_mask);
> +       of_property_read_u32(np, "regulator-vset-regs",
> +                            (u32 *)&regulator->vset_regs);
> +       of_property_read_u32(np, "regulator-vset-mask", &regulator->vset_mask);
> +       of_property_read_u32(np, "regulator-n-vol", &regulator->vol_numb);
> +       of_property_read_u32(np, "regulator-off-on-delay",
> +                            &regulator->rdesc.off_on_delay);
> +
> +       vset_table = devm_kzalloc(dev, regulator->vol_numb * sizeof(int),
> +                                 GFP_KERNEL);
> +
> +       of_property_read_u32_array(np, "regulator-vset-table",
> +                                  vset_table,
> +                                  regulator->vol_numb);
> +       regulator->vset_table = vset_table;
> +       regulator->rdesc.volt_table = vset_table;
> +       regulator->rdesc.n_voltages = regulator->vol_numb;
> +}
> +
> +static int hi655x_regulator_probe(struct platform_device *pdev)
> +{
> +       int ret = 0;

Is it used?

make W=1

> +       struct hi655x_regulator *regulator;
> +       struct hi655x_pmic *pmic;
> +       struct regulator_init_data *init_data;
> +       struct regulator_config config = { };
> +       struct device_node *np = pdev->dev.of_node;
> +
> +       pmic = dev_get_drvdata(pdev->dev.parent);
> +       if (!pmic) {
> +               pr_err("no pmic in the regulator parent node\n");

pr_err -> dev_err. Here and in other places.

> +               return -ENODEV;
> +       }
> +
> +       regulator = devm_kzalloc(&pdev->dev, sizeof(*regulator), GFP_KERNEL);
> +       if (!regulator)
> +               return -ENOMEM;
> +       of_get_hi655x_ctr(regulator, &pdev->dev, np);
> +
> +       regulator->rdesc.name = dev_name(&pdev->dev);
> +       regulator->rdesc.type = REGULATOR_VOLTAGE;
> +       regulator->rdesc.owner = THIS_MODULE;
> +       regulator->rdesc.of_map_mode = hi655x_map_mode;
> +       regulator->rdesc.ops = &hi655x_regulator_ops;
> +       init_data = of_get_regulator_init_data(&pdev->dev, pdev->dev.of_node,
> +                                              &regulator->rdesc);
> +       if (!init_data) {
> +               pr_err("get init data from dts error!\n");
> +               return -EINVAL;
> +       }
> +       config.dev = &pdev->dev;
> +       config.init_data = init_data;
> +       config.driver_data = regulator;
> +       config.regmap = pmic->regmap;
> +
> +       regulator->regdev = devm_regulator_register(&pdev->dev,
> +                                                   &regulator->rdesc,
> +                                                   &config);
> +       if (IS_ERR(regulator->regdev)) {
> +               pr_err("register regulator to system error!\n");
> +               return PTR_ERR(regulator->regdev);
> +       }
> +
> +       platform_set_drvdata(pdev, regulator);
> +       return 0;
> +}
> +
> +static struct platform_driver hi655x_regulator_driver = {
> +       .driver = {
> +               .name   = "hi655x_regulator",
> +               .of_match_table = of_hi655x_regulator_match_tbl,
> +       },
> +       .probe  = hi655x_regulator_probe,
> +};
> +module_platform_driver(hi655x_regulator_driver);
> +
> +MODULE_AUTHOR("Chen Feng <puck.chen@...ilicon.com>");
> +MODULE_DESCRIPTION("Hisi hi655x regulator driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/regulator/hi655x-regulator.h b/include/linux/regulator/hi655x-regulator.h
> new file mode 100644
> index 0000000..4a32095
> --- /dev/null
> +++ b/include/linux/regulator/hi655x-regulator.h
> @@ -0,0 +1,63 @@
> +/*
> + * Device driver for regulators in HI6553 IC
> + *
> + * Copyright (c) 2015 Hisilicon.
> + *
> + * Fei Wang  <w.f@...wei.com>
> + * Chen Feng <puck.chen@...ilicon.com>
> + *
> + * this regulator's probe function will be called lots of times,,
> + * because of there are lots of regulator nodes in dtb.
> + * so,that's say, the driver must be inited before the regulator nodes
> + * registor to system.

Something with spelling is broken here.

> + *
> + * Makefile have proved my guess, please refor to the makefile.
> + * when the code is rebuild i hope we can build pmu sub_system.
> + * init order can not base on compile

And here.




-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists