Date:   Mon, 12 Sep 2016 19:32:30 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     lgirdwood@...il.com, Douglas Anderson <dianders@...omium.org>,
        briannorris@...omium.org, javier@...hile0.org, robh+dt@...nel.org,
        mark.rutland@....com, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v4 1/4] regulator: Add set_voltage_time op

On Tue, Sep 06, 2016 at 12:03:15PM -0700, Matthias Kaehlcke wrote:

Whatever you're using to send these is not threading things as expected
and is adding a random HTML segment to the end of the e-mails - you
probably want to look at this.  If you're trying to use gmail via the
web interface you want to avoid that, it's got a tendency to mangle
things.

> -	/* Call set_voltage_time_sel if successfully obtained old_selector */
> -	if (ret == 0 && !rdev->constraints->ramp_disable && old_selector >= 0
> -		&& old_selector != selector) {
> +	if (ret != 0 || rdev->constraints->ramp_disable)
> +		goto no_delay;

You probably want to do the refactoring for splitting out decisions
about old_selector separately, it'll make the diff clearer.

> -		delay = rdev->desc->ops->set_voltage_time_sel(rdev,
> -						old_selector, selector);

> +		delay = rdev->desc->ops->set_voltage_time_sel(
> +			rdev, old_selector, selector);

Coding style - there's no need to put the rdev on the new line and the
arguments should be more indented.  Look at how the original was
indented...

> +	/* Insert any necessary delays */
> +	if (delay >= 1000) {
> +		mdelay(delay / 1000);
> +		udelay(delay % 1000);
> +	} else if (delay) {
> +		udelay(delay);
> +	}
> +
> +no_delay:

Why were the gotos there?

> @@ -2993,54 +3005,58 @@ int regulator_set_voltage_time(struct regulator *regulator,
>  {
>  	struct regulator_dev *rdev = regulator->rdev;
>  	const struct regulator_ops *ops = rdev->desc->ops;
> -	int old_sel = -1;
> -	int new_sel = -1;
> -	int voltage;
> -	int i;
>  
> -	/* Currently requires operations to do this */
> -	if (!ops->list_voltage || !ops->set_voltage_time_sel
> -	    || !rdev->desc->n_voltages)
> -		return -EINVAL;
> +	if (ops->set_voltage_time) {
> +		return ops->set_voltage_time(rdev, old_uV, new_uV);
> +	} else if (ops->set_voltage_time_sel) {
> +		int old_sel = -1;
> +		int new_sel = -1;
> +		int voltage;
> +		int i;
>  
> -	for (i = 0; i < rdev->desc->n_voltages; i++) {
> -		/* We only look for exact voltage matches here */
> -		voltage = regulator_list_voltage(regulator, i);

The diff and I expect the resulting code would be a lot clearer if we
just left most of the function indented as it is and simply directly
returned set_voltage_time().  Which is what we do anyway so no need to
reindent the rest of the code.

> +int regulator_set_voltage_time_op(struct regulator_dev *rdev,
> +				int old_uV, int new_uV)
>  {
>  	unsigned int ramp_delay = 0;
> -	int old_volt, new_volt;
>  
>  	if (rdev->constraints->ramp_delay)
>  		ramp_delay = rdev->constraints->ramp_delay;
> @@ -3052,6 +3068,28 @@ int regulator_set_voltage_time_sel(struct regulator_dev *rdev,
>  		return 0;
>  	}
>  
> +	return DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay);
> +}
> +EXPORT_SYMBOL_GPL(regulator_set_voltage_time_op);

This is getting very messy.  Having the handling of the ramp delay DT
properties in the operation that the driver in theory implements is
making things harder than they should be (and we seem to have some
broken drivers that don't do one or both of the ramp delays).  I'm not
sure this series is the place to fully fix that but it'd be good to
start that refactoring by not requiring the op be set and instead just
doing this implementation by default.  That way there's less stuff to
clean up later on.

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

Powered by blists - more mailing lists