On Mon, Jan 18, 2021 at 4:32 AM Mark Brown <broonie@xxxxxxxxxx> wrote: > > 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. Fixed. > > > + * Copyright (C) 2019 reMarkable AS - http://www.remarkable.com/ > > + * > > + * Author: Lars Ivar Miljeteig <lars.ivar.miljeteig@xxxxxxxxxxxxxx> > > 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. Fixed. > > > +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. I'm not sure what you mean, can you please explain? > > > +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 The delay is to allow a power ramp up, I have added a comment. > to serve? I can't understand what it's intended to protect. Apparently the mutex is to protect enable/disable, I don't think it's required and I will remove it. > > > + 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. Thanks, fixed. > > > +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. Fixed. > Given the power good signal I'd also expect a get_status() operation. Added. > > > +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 is part of the vendor's kernel, they specifically added it to ensure vcom is set on resume. I have fixed the variable type. > 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. Removed. Alistair
