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@xxxxxxxxx>
> > ---
> >   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ý