Re: [PATCH v2 07/14] regulator: s2mps11: Copy supported regulators from initconst

Yadwinder Singh Brar
2014-02-13T12:21:37Z

Hi,

On Thu, Feb 13, 2014 at 2:44 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> Add __initconst to 'regulator_desc' array with supported regulators.
> During probe choose how many and which regulators will be supported
> according to device ID. Then copy the 'regulator_desc' array to
> allocated memory so the regulator core can use it.
>
> Additionally allocate array of of_regulator_match() dynamically (based
> on number of regulators) instead of allocation on the stack.
>
> This is needed for supporting different devices in s2mps11
> driver and actually prepares the regulator driver for supporting the
> S2MPS14 device.
>
> Code for supporting the S2MPS14 device will add its own array of
> 'regulator_desc' (also marked as __initconst). This way memory footprint
> of the driver will be reduced (approximately 'regulators_desc' array for
> S2MPS11 occupies 5 kB on 32-bit ARM, for S2MPS14 will occupy 3 kB).
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Yadwinder Singh Brar <yadi.brar01@gmail.com>
> ---

Just observed one trivial thing that in this patch, spacing is not
provided before and after multiplication binary operator ( _ * _ ),
which is recommended by linux spacing convention.

Other than that it looks fine to me, pls feel free to add

Reviewed-by: Yadwinder Singh Brar <yadi.brar@samsung.com>

Regards,
Yadwinder

> drivers/regulator/s2mps11.c | 74 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 64 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
> index d44bd5b3fe8e..246b25d58c2b 100644
> --- a/drivers/regulator/s2mps11.c
> +++ b/drivers/regulator/s2mps11.c
> @@ -25,10 +25,9 @@
> #include <linux/mfd/samsung/core.h>
> #include <linux/mfd/samsung/s2mps11.h>
>
> -#define S2MPS11_REGULATOR_CNT ARRAY_SIZE(regulators)
> -
> struct s2mps11_info {
> - struct regulator_dev *rdev[S2MPS11_REGULATOR_MAX];
> + struct regulator_dev **rdev;
> + unsigned int rdev_num;
>
> int ramp_delay2;
> int ramp_delay34;
> @@ -345,7 +344,7 @@ static struct regulator_ops s2mps11_buck_ops = {
> .enable_mask = S2MPS11_ENABLE_MASK \
> }
>
> -static const struct regulator_desc regulators[] = {
> +static const struct regulator_desc s2mps11_regulators[] __initconst = {
> regulator_desc_ldo2(1),
> regulator_desc_ldo1(2),
> regulator_desc_ldo1(3),
> @@ -396,21 +395,62 @@ static const struct regulator_desc regulators[] = {
> regulator_desc_buck10,
> };
>
> +/*
> + * Allocates memory under 'regulators' pointer and copies there array
> + * of regulator_desc for given device.
> + *
> + * Returns number of regulators or negative ERRNO on error.
> + */
> +static int __init
> +s2mps11_pmic_init_regulators_desc(struct platform_device *pdev,
> + struct regulator_desc **regulators)
> +{
> + const struct regulator_desc *regulators_init;
> + enum sec_device_type dev_type;
> + int rdev_num;
> +
> + dev_type = platform_get_device_id(pdev)->driver_data;
> + switch (dev_type) {
> + case S2MPS11X:
> + rdev_num = ARRAY_SIZE(s2mps11_regulators);
> + regulators_init = s2mps11_regulators;
> + break;
> + default:
> + dev_err(&pdev->dev, "Invalid device type: %u\n", dev_type);
> + return -EINVAL;
> + };
> +
> + *regulators = devm_kzalloc(&pdev->dev, sizeof(**regulators)*rdev_num,
> + GFP_KERNEL);
> + if (!*regulators)
> + return -ENOMEM;
> +
> + memcpy(*regulators, regulators_init, sizeof(**regulators)*rdev_num);
> +
> + return rdev_num;
> +}
> +
> static int s2mps11_pmic_probe(struct platform_device *pdev)
> {
> struct sec_pmic_dev *iodev = dev_get_drvdata(pdev->dev.parent);
> - struct sec_platform_data *pdata = dev_get_platdata(iodev->dev);
> - struct of_regulator_match rdata[S2MPS11_REGULATOR_MAX];
> + struct sec_platform_data *pdata = iodev->pdata;
> + struct of_regulator_match *rdata = NULL;
> struct device_node *reg_np = NULL;
> struct regulator_config config = { };
> struct s2mps11_info *s2mps11;
> int i, ret;
> + struct regulator_desc *regulators = NULL;
> + unsigned int rdev_num;
>
> s2mps11 = devm_kzalloc(&pdev->dev, sizeof(struct s2mps11_info),
> GFP_KERNEL);
> if (!s2mps11)
> return -ENOMEM;
>
> + rdev_num = s2mps11_pmic_init_regulators_desc(pdev, &regulators);
> + if (rdev_num < 0)
> + return rdev_num;
> +
> if (!iodev->dev->of_node) {
> if (pdata) {
> goto common_reg;
> @@ -421,7 +461,16 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
> }
> }
>
> - for (i = 0; i < S2MPS11_REGULATOR_CNT; i++)
> + s2mps11->rdev = devm_kzalloc(&pdev->dev,
> + sizeof(*s2mps11->rdev)*rdev_num, GFP_KERNEL);
> + if (!s2mps11->rdev)
> + return -ENOMEM;
> +
> + rdata = devm_kzalloc(&pdev->dev, sizeof(*rdata)*rdev_num, GFP_KERNEL);
> + if (!rdata)
> + return -ENOMEM;
> +
> + for (i = 0; i < rdev_num; i++)
> rdata[i].name = regulators[i].name;
>
> reg_np = of_find_node_by_name(iodev->dev->of_node, "regulators");
> @@ -430,15 +479,16 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> - of_regulator_match(&pdev->dev, reg_np, rdata, S2MPS11_REGULATOR_MAX);
> + of_regulator_match(&pdev->dev, reg_np, rdata, rdev_num);
>
> common_reg:
> platform_set_drvdata(pdev, s2mps11);
> + s2mps11->rdev_num = rdev_num;
>
> config.dev = &pdev->dev;
> config.regmap = iodev->regmap_pmic;
> config.driver_data = s2mps11;
> - for (i = 0; i < S2MPS11_REGULATOR_MAX; i++) {
> + for (i = 0; i < rdev_num; i++) {
> if (!reg_np) {
> config.init_data = pdata->regulators[i].initdata;
> config.of_node = pdata->regulators[i].reg_node;
> @@ -457,11 +507,15 @@ common_reg:
> }
> }
>
> + /* rdata was needed only for of_regulator_match() during probe */
> + if (rdata)
> + devm_kfree(&pdev->dev, rdata);
> +
> return 0;
> }
>
> static const struct platform_device_id s2mps11_pmic_id[] = {
> - { "s2mps11-pmic", 0},
> + { "s2mps11-pmic", S2MPS11X},
> { },
> };
> MODULE_DEVICE_TABLE(platform, s2mps11_pmic_id);
> --
> 1.7.9.5
>