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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210118123158.GE4455@sirena.org.uk>
Date:   Mon, 18 Jan 2021 12:31:58 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Alistair Francis <alistair@...stair23.me>
Cc:     lee.jones@...aro.org, robh+dt@...nel.org, lgirdwood@...il.com,
        linux-imx@....com, kernel@...gutronix.de,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        alistair23@...il.com
Subject: Re: [PATCH 4/6] regulator: Initial commit of sy7636a

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@...arkable.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.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)