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: <c8f40177-1d81-4c8e-8319-e78623cded42@gmail.com>
Date: Sun, 24 Nov 2024 11:22:00 +0100
From: Kryštof Černý <cleverline1mc@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Stefan Wahren <stefan.wahren@...rgebyte.com>,
 Ben Gardner <bgardner@...tec.com>, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v2 1/3] w1: ds2482: Add regulator support

> On Fri, Nov 22, 2024 at 09:53:57AM +0100, Kryštof Černý wrote:
>> Adds a support for attaching a supply regulator.
>>
>> Signed-off-by: Kryštof Černý <cleverline1mc@...il.com>
>> ---
>>   drivers/w1/masters/ds2482.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/w1/masters/ds2482.c b/drivers/w1/masters/ds2482.c
>> index a2ecbb863c57f38bffc8e3cd463db1940e603179..3fb35e92fc1587dc4e609c0061fa5057e0027a80 100644
>> --- a/drivers/w1/masters/ds2482.c
>> +++ b/drivers/w1/masters/ds2482.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/i2c.h>
>>   #include <linux/delay.h>
>> +#include <linux/regulator/consumer.h>
>>   
>>   #include <linux/w1.h>
>>   
>> @@ -117,6 +118,9 @@ struct ds2482_data {
>>   	u8			channel;
>>   	u8			read_prt;	/* see DS2482_PTR_CODE_xxx */
>>   	u8			reg_config;
>> +
>> +	/* reference to the optional regulator */
> 
> Drop comment, obvious.

I will drop all the other comments, as they seem to have the same level
of "obviousness" as this one to me.


> 
>> +	struct regulator *vcc_reg;
> 
> Missing indentation after type - see earlier lines.

struct will be removed with switching to devm_regulator_get_enable().

> 
>>   };
>>   
>>   
>> @@ -445,6 +449,7 @@ static int ds2482_probe(struct i2c_client *client)
>>   	int err = -ENODEV;
>>   	int temp1;
>>   	int idx;
>> +	int ret;
>>   
>>   	if (!i2c_check_functionality(client->adapter,
>>   				     I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
>> @@ -457,6 +462,18 @@ static int ds2482_probe(struct i2c_client *client)
>>   		goto exit;
>>   	}
>>   
>> +	/* Get the vcc regulator */
>> +	data->vcc_reg = devm_regulator_get(&client->dev, "vcc");
>> +	if (IS_ERR(data->vcc_reg))
>> +		return PTR_ERR(data->vcc_reg);
>> +
>> +	/* Enable the vcc regulator */
>> +	ret = regulator_enable(data->vcc_reg);
> 
> You wanted devm_regulator_get_enable().
> 
> ... but your comment also suggests devm_regulator_get_enable_optional().
> 

This is a good point, my implementation is based on observation of a few 
other drivers and it's not needed in this case. This will reduce the 
amount of changes.

I think my wording was not correct. By optionally I meant that most 
hardware designs do not use a separate power supply regulator, so they 
do not need to specify one, but the device needs power to function.
My current view is that it should not be optional after all, so I would 
go with devm_regulator_get_enable(). Could you please tell me your view 
on this?

> 
> Best regards,
> Krzysztof
> 

Thank you very much for the review,
Kryštof Černý

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ