[PATCH v2 0/3] Add support for attaching a regulator to w1: ds2482

[PATCH v2 0/3] Add support for attaching a regulator to w1: ds2482

[Kryštof Černý via B4 Relay]

Sending a new version based on our discussion.

Signed-off-by: Kryštof Černý <cleverline1mc@gmail.com>
---
Changes in v2:
- Removed property description
- Changed commit message of binding commit
- Link to v1: https://lore.kernel.org/r/20241115-ds2482-add-reg-v1-0-cc84b9aba126@gmail.com

---
Kryštof Černý (3):
      w1: ds2482: Add regulator support
      w1: ds2482: Fix datasheet URL
      dt-bindings: w1: ds2482: Add vcc-supply property

 .../devicetree/bindings/w1/maxim,ds2482.yaml       |  2 ++
 drivers/w1/masters/ds2482.c                        | 23 +++++++++++++++++++++-
 2 files changed, 24 insertions(+), 1 deletion(-)
---
base-commit: 6d59cab07b8d74d0f0422b750038123334f6ecc2
change-id: 20241111-ds2482-add-reg-fe13200ad7d6

Best regards,
-- 
Kryštof Černý <cleverline1mc@gmail.com>

[PATCH v2 1/3] w1: ds2482: Add regulator support

[Kryštof Černý via B4 Relay]

From: Kryštof Černý <cleverline1mc@gmail.com>

Adds a support for attaching a supply regulator.

Signed-off-by: Kryštof Černý <cleverline1mc@gmail.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 */
+	struct regulator *vcc_reg;
 };
 
 
@@ -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);
+	if (ret) {
+		dev_err(&client->dev, "Fail to enable regulator\n");
+		return ret;
+	}
+
 	data->client = client;
 	i2c_set_clientdata(client, data);
 
@@ -517,6 +534,7 @@ static int ds2482_probe(struct i2c_client *client)
 			w1_remove_master_device(&data->w1_ch[idx].w1_bm);
 	}
 exit_free:
+	regulator_disable(data->vcc_reg);
 	kfree(data);
 exit:
 	return err;
@@ -533,6 +551,9 @@ static void ds2482_remove(struct i2c_client *client)
 			w1_remove_master_device(&data->w1_ch[idx].w1_bm);
 	}
 
+	/* Disable the vcc regulator */
+	regulator_disable(data->vcc_reg);
+
 	/* Free the memory */
 	kfree(data);
 }

-- 
2.39.5

[PATCH v2 2/3] w1: ds2482: Fix datasheet URL

[Kryštof Černý via B4 Relay]

From: Kryštof Černý <cleverline1mc@gmail.com>

Current link does redirect to wrong place.

Signed-off-by: Kryštof Černý <cleverline1mc@gmail.com>
---
 drivers/w1/masters/ds2482.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/w1/masters/ds2482.c b/drivers/w1/masters/ds2482.c
index 3fb35e92fc1587dc4e609c0061fa5057e0027a80..ba86b5953b8d3ed1fe40d40a1a2b018c57fb91b8 100644
--- a/drivers/w1/masters/ds2482.c
+++ b/drivers/w1/masters/ds2482.c
@@ -7,7 +7,7 @@
  * It is a I2C to 1-wire bridge.
  * There are two variations: -100 and -800, which have 1 or 8 1-wire ports.
  * The complete datasheet can be obtained from MAXIM's website at:
- *   http://www.maxim-ic.com/quick_view2.cfm/qv_pk/4382
+ *   https://www.analog.com/en/products/ds2482-100.html
  */
 
 #include <linux/module.h>

-- 
2.39.5

[PATCH v2 3/3] dt-bindings: w1: ds2482: Add vcc-supply property

[Kryštof Černý via B4 Relay]

From: Kryštof Černý <cleverline1mc@gmail.com>

ds2482 has a VCC pin, accepting 2.9-5.5 V.

Signed-off-by: Kryštof Černý <cleverline1mc@gmail.com>
---
 Documentation/devicetree/bindings/w1/maxim,ds2482.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/w1/maxim,ds2482.yaml b/Documentation/devicetree/bindings/w1/maxim,ds2482.yaml
index 422becc6e1fa8d58665c5586ebdc611cd0b2c760..fe6b3f9a3f8bbd00366c3e36aad3ffa72ec1a31f 100644
--- a/Documentation/devicetree/bindings/w1/maxim,ds2482.yaml
+++ b/Documentation/devicetree/bindings/w1/maxim,ds2482.yaml
@@ -25,6 +25,8 @@ properties:
   reg:
     maxItems: 1
 
+  vcc-supply: true
+
 required:
   - compatible
   - reg

-- 
2.39.5

Re: [PATCH v2 1/3] w1: ds2482: Add regulator support

[Krzysztof Kozlowski]

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@gmail.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.

> +	struct regulator *vcc_reg;

Missing indentation after type - see earlier lines.

>  };
>  
>  
> @@ -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().


Best regards,
Krzysztof

Re: [PATCH v2 1/3] w1: ds2482: Add regulator support

[Kryštof Černý]

> 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@gmail.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ý

Re: [PATCH v2 1/3] w1: ds2482: Add regulator support

[Krzysztof Kozlowski]

On 24/11/2024 11:22, Kryštof Černý wrote:
>>> +	/* 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?
> 


Sure, fine.

Best regards,
Krzysztof