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

[Mark Brown]

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: signature.asc
Description: PGP signature