Re: [PATCH 4/6] regulator: Initial commit of sy7636a

[Mark Brown <broonie@kernel.org>]

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

On Sat, Jan 16, 2021 at 08:25:37PM -0800, Alistair Francis wrote:

> --- /dev/null
> +++ b/drivers/regulator/sy7636a-regulator.c
> @@ -0,0 +1,233 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Functions to access SY3686A power management chip voltages
> + *

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

> + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/
> + *
> + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@remarkable.com>

This probably needs an update.

> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

This boilerplate is redundant and should be removed.

> +static int get_vcom_voltage_op(struct regulator_dev *rdev)
> +{
> +	int ret = get_vcom_voltage_mv(rdev->regmap);
> +

Why is this get_vcom_voltage_mv() function not in the regulator driver,
and why is it not just inline here?  It also needs namespacing.

> +static int disable_regulator(struct regulator_dev *rdev)
> +{
> +	struct sy7636a *sy7636a = dev_get_drvdata(rdev->dev.parent);
> +	int ret = 0;
> +
> +	mutex_lock(&sy7636a->reglock);
> +	ret = regulator_disable_regmap(rdev);
> +	usleep_range(30000, 35000);
> +	mutex_unlock(&sy7636a->reglock);

Why do you need this delay here, and what purpose is this lock intended
to serve?  I can't understand what it's intended to protect.

> +	mutex_lock(&sy7636a->reglock);
> +	ret = regulator_is_enabled_regmap(rdev);
> +	mutex_unlock(&sy7636a->reglock);

This lock usage in particular looks confused.

> +	ret = regulator_enable_regmap(rdev);
> +	if (ret)
> +		goto finish;

> +	if (!pwr_good) {
> +		dev_err(&rdev->dev, "Power good signal timeout after %u ms\n",
> +				jiffies_to_msecs(t1 - t0));
> +		ret = -ETIME;
> +		goto finish;
> +	}

This doesn't undo the underlying enable, leaving the regulator in a
partially enabled state.

> +static const struct regulator_ops sy7636a_vcom_volt_ops = {
> +	.get_voltage = get_vcom_voltage_op,
> +	.enable = enable_regulator_pgood,
> +	.disable = disable_regulator,
> +	.is_enabled = sy7636a_regulator_is_enabled,
> +};

The namespacing for functions is very random and prone to clashes.
Given the power good signal I'd also expect a get_status() operation.

> +static int sy7636a_regulator_suspend(struct device *dev)
> +{
> +	int ret;
> +	struct sy7636a *sy7636a = dev_get_drvdata(dev->parent);
> +
> +	ret = get_vcom_voltage_mv(sy7636a->regmap);
> +
> +	if (ret > 0)
> +		sy7636a->vcom = (unsigned int)ret;
> +
> +	return 0;
> +}

What's going on here, and if you are going to store this value over
suspend why not store it in a variable of the correct type?  In general
it's surprising to need a suspend operation for a regulator.

> +	sy7636a->pgood_gpio = gdp;
> +	dev_info(sy7636a->dev,
> +		"Power good GPIO registered (gpio# %d)\n",
> +		desc_to_gpio(sy7636a->pgood_gpio));

This print is just adding noise to the boot process.

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