linux-kernel - Re: [PATCH v2 1/2] regulator: act8865: support output voltage by VSET2[] bits

Open Source and information security mailing list archives

Message-ID: <20150930132107.GH31199@odux.rfo.atmel.com>
Date:	Wed, 30 Sep 2015 15:21:07 +0200
From:	Ludovic Desroches <ludovic.desroches@...el.com>
To:	Wenyou Yang <wenyou.yang@...el.com>
CC:	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	<devicetree@...r.kernel.org>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 1/2] regulator: act8865: support output voltage by
 VSET2[] bits

Hi Wenyou,

I see nothing wrong in it.

On Wed, Sep 30, 2015 at 03:25:49PM +0800, Wenyou Yang wrote:
> For the step-down DC/DC regulators, the output voltage is
> selectable by setting VSEL pin that when VSEL is low, output
> voltage is programmed by VSET1[] bits, and when VSEL is high,
> output voltage is programmed by VSET2[] bits.
> 
> The DT property "active-semi,vsel-high" is used to specify
> the VSEL pin at high on the board.
> 
> Signed-off-by: Wenyou Yang <wenyou.yang@...el.com>

Minor comment below, it's only my point of view.

Anyway, Reviewed-by: Ludovic Desroches <ludovic.desroches@...el.com>

> ---
> 
> Changes in v2:
>  - fix missed argument of macro "ACT88xx_REG" for act8865_alt_regulators
>    structure variable.
>  - set variable "voltage_select" initial value to avoid compile warning.
> 
>  drivers/regulator/act8865-regulator.c |   24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/act8865-regulator.c b/drivers/regulator/act8865-regulator.c
> index 896db16..f8d4cd3 100644
> --- a/drivers/regulator/act8865-regulator.c
> +++ b/drivers/regulator/act8865-regulator.c
> @@ -261,6 +261,16 @@ static const struct regulator_desc act8865_regulators[] = {
>  	ACT88xx_REG("LDO_REG4", ACT8865, LDO4, VSET, "inl67"),
>  };
>  
> +static const struct regulator_desc act8865_alt_regulators[] = {
> +	ACT88xx_REG("DCDC_REG1", ACT8865, DCDC1, VSET2, "vp1"),
> +	ACT88xx_REG("DCDC_REG2", ACT8865, DCDC2, VSET2, "vp2"),
> +	ACT88xx_REG("DCDC_REG3", ACT8865, DCDC3, VSET2, "vp3"),
> +	ACT88xx_REG("LDO_REG1", ACT8865, LDO1, VSET, "inl45"),
> +	ACT88xx_REG("LDO_REG2", ACT8865, LDO2, VSET, "inl45"),
> +	ACT88xx_REG("LDO_REG3", ACT8865, LDO3, VSET, "inl67"),
> +	ACT88xx_REG("LDO_REG4", ACT8865, LDO4, VSET, "inl67"),
> +};
> +
>  #ifdef CONFIG_OF
>  static const struct of_device_id act8865_dt_ids[] = {
>  	{ .compatible = "active-semi,act8600", .data = (void *)ACT8600 },
> @@ -413,6 +423,7 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  	struct act8865 *act8865;
>  	unsigned long type;
>  	int off_reg, off_mask;
> +	int voltage_select = 0;

I think adding this variable is useless...

>  
>  	pdata = dev_get_platdata(dev);
>  
> @@ -424,6 +435,10 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  			return -ENODEV;
>  
>  		type = (unsigned long) id->data;
> +
> +		voltage_select = !!of_get_property(dev->of_node,
> +						   "active-semi,vsel-high",
> +						   NULL);
>  	} else {
>  		type = i2c_id->driver_data;
>  	}
> @@ -442,8 +457,13 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  		off_mask = ACT8846_OFF_SYSMASK;
>  		break;
>  	case ACT8865:
> -		regulators = act8865_regulators;
> -		num_regulators = ARRAY_SIZE(act8865_regulators);
> +		if (voltage_select) {

I would directly do

if (of_get_property(dev->of_node, "active-semi,vsel-high", NULL) {

> +			regulators = act8865_alt_regulators;
> +			num_regulators = ARRAY_SIZE(act8865_alt_regulators);
> +		} else {
> +			regulators = act8865_regulators;
> +			num_regulators = ARRAY_SIZE(act8865_regulators);
> +		}
>  		off_reg = ACT8865_SYS_CTRL;
>  		off_mask = ACT8865_MSTROFF;
>  		break;
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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