Re: [PATCH v3 18/23] regulator: add s2dos05 regulator support

Mark Brown
2024-06-18T14:08:09Z

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Tue, Jun 18, 2024 at 04:59:52PM +0300, Dzmitry Sankouski wrote:

> index 000000000000..3c58a1bd2262
> --- /dev/null
> +++ b/drivers/regulator/s2dos05-regulator.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * s2dos05.c - Regulator driver for the Samsung s2dos05
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +static int s2m_enable(struct regulator_dev *rdev)
> +{
> + struct s2dos05_data *info = rdev_get_drvdata(rdev);
> + struct regmap *regmap = info->regmap;
> +
> + return regmap_update_bits(regmap, rdev->desc->enable_reg,
> + rdev->desc->enable_mask,
> + rdev->desc->enable_mask);
> +}

Please use the generic regmap helpers rather than open coding them.

> + reg_np = of_get_child_by_name(dev->parent->of_node, "regulators");
> + if (!reg_np) {
> + dev_err(dev, "could not find regulators sub-node\n");
> + return -EINVAL;
> + }
> +
> + err = of_regulator_match(dev, reg_np, rdata, rdev_num);
> + of_node_put(reg_np);

Use of_match for this rather than open coding.

> + s2dos05 = devm_kzalloc(dev, sizeof(struct s2dos05_data),
> + GFP_KERNEL);

> + rdata = kcalloc(rdev_num, sizeof(*rdata), GFP_KERNEL);
> + if (!rdata)
> + return -ENOMEM;

Mixing devm_ and regular allocations seems likely to go wrong, please be
consistent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]