linux-kernel - Re: [PATCH v2 2/2] regulator: richtek,rt4801: use existing ena

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] [day] [month] [year] [list]

Date:   Mon, 25 Apr 2022 08:40:18 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     ChiYuan Huang <u0084500@...il.com>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        ChiYuan Huang <cy_huang@...htek.com>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] regulator: richtek,rt4801: use existing ena_gpiod
 feature

On 25/04/2022 07:49, ChiYuan Huang wrote:
>> +static int rt4801_of_parse_cb(struct device_node *np,
>> +			      const struct regulator_desc *desc,
>> +			      struct regulator_config *config)
>> +{
>> +	struct rt4801_priv *priv = config->driver_data;
>> +
>> +	if (priv->enable_gpios) {
>> +		dev_warn(priv->dev, "duplicated enable-gpios property\n");
>> +		return 0;
>> +	}
>> +	config->ena_gpiod = fwnode_gpiod_get_index(of_fwnode_handle(np),
>> +						   "enable-gpios",
> 'enable' only, gpiod API will automatically concat it to 'enable-gpios'.

Right.

>> +						   0,
>> +						   GPIOD_OUT_HIGH |
>> +						   GPIOD_FLAGS_BIT_NONEXCLUSIVE,
>> +						   "rt4801");

(...)

>>  static const struct regulator_ops rt4801_regulator_ops = {
>>  	.list_voltage = regulator_list_voltage_linear,
>>  	.set_voltage_sel = rt4801_set_voltage_sel,
>> @@ -122,6 +168,7 @@ static const struct regulator_desc rt4801_regulator_descs[] = {
>>  		.name = "DSVP",
>>  		.ops = &rt4801_regulator_ops,
>>  		.of_match = of_match_ptr("DSVP"),
>> +		.of_parse_cb = rt4801_of_parse_cb,
>>  		.type = REGULATOR_VOLTAGE,
>>  		.id = DSV_OUT_POS,
>>  		.min_uV = MIN_UV,
>> @@ -135,6 +182,7 @@ static const struct regulator_desc rt4801_regulator_descs[] = {
>>  		.name = "DSVN",
>>  		.ops = &rt4801_regulator_ops,
>>  		.of_match = of_match_ptr("DSVN"),
>> +		.of_parse_cb = rt4801_of_parse_cb,
>>  		.type = REGULATOR_VOLTAGE,
>>  		.id = DSV_OUT_NEG,
>>  		.min_uV = MIN_UV,
> 
> There's one problem.
> If 'ena_gpiod' is specified, it cannot be conexisted with ops
> 'enable/disable/is_enabled' by regulator framework.
> It will cause no one to recover the voltage back.
> You can check the original 'enable' ops.

You mean that regulator voltage is being reset after disabling it?

> 
> How about to only parse gpio in 'of_parse_cb' and put it all into the
> driver data, not to use regulator framework 'ena_gpiod'?

In such case that's the only option. Thanks for the review.


Best regards,
Krzysztof