Re: [PATCH 2/2] media: i2c: ov13b10: Support tps68470 regulator and gpio

[johannes . goede]

Hi Arun,

Thank you for your patch.

On 30-Jan-26 10:24, Arun T wrote:
> Ov13b10 sensor get clock and regulator from TPS68470 PMIC.
> Added tps68470 regulator/gpio names in power_on()
> 
> Signed-off-by: Arun T <arun.t@xxxxxxxxx>
> ---
>  drivers/media/i2c/ov13b10.c | 38 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov13b10.c b/drivers/media/i2c/ov13b10.c
> index 5421874732bc..c2469c88c722 100644
> --- a/drivers/media/i2c/ov13b10.c
> +++ b/drivers/media/i2c/ov13b10.c
> @@ -709,6 +709,10 @@ struct ov13b10 {
>  
>  	struct clk *img_clk;
>  	struct regulator *avdd;
> +	struct regulator *vio;
> +	struct regulator *core;
> +	struct gpio_desc *enable;

This new enable gpio does not seem to get used in the patch
at all ?

> +
>  	struct gpio_desc *reset;
>  
>  	/* V4L2 Controls */
> @@ -1475,12 +1479,19 @@ static int ov13b10_get_pm_resources(struct ov13b10 *ov13b)
>  	unsigned long freq;
>  	int ret;
>  
> -	ov13b->reset = devm_gpiod_get_optional(ov13b->dev, "reset", GPIOD_OUT_LOW);
> +	if (strstr(dev_name(ov13b->dev), "OVTI13B1:01"))
> +		ov13b->reset = devm_gpiod_get_optional(ov13b->dev, "s_resetn", GPIOD_OUT_LOW);
> +	else
> +		ov13b->reset = devm_gpiod_get_optional(ov13b->dev, "reset", GPIOD_OUT_LOW);

Nack, you're adding a lookup in patch 1/2, simply use "reset"
there instead of "s_resetn" and then you don't need this.

More importantly board/platform specific info like this must NOT
be present in sensor drivers.

All uses of "if (strstr(dev_name(ov13b->dev), "OVTI13B1:01"))"
in this patch MUST be dropped.

>  	if (IS_ERR(ov13b->reset))
>  		return dev_err_probe(ov13b->dev, PTR_ERR(ov13b->reset),
>  				     "failed to get reset gpio\n");
>  
> -	ov13b->img_clk = devm_v4l2_sensor_clk_get(ov13b->dev, NULL);
> +	if (strstr(dev_name(ov13b->dev), "OVTI13B1:01"))
> +		ov13b->img_clk = devm_v4l2_sensor_clk_get(ov13b->dev, "tps68470-clk");
> +	else
> +		ov13b->img_clk = devm_v4l2_sensor_clk_get(ov13b->dev, NULL);
> +

Same here, the old code with the NULL name arg will work fine since
there should be only 1 clk provider.

>  	if (IS_ERR(ov13b->img_clk))
>  		return dev_err_probe(ov13b->dev, PTR_ERR(ov13b->img_clk),
>  				     "failed to get imaging clock\n");
> @@ -1490,8 +1501,11 @@ static int ov13b10_get_pm_resources(struct ov13b10 *ov13b)
>  		return dev_err_probe(ov13b->dev, -EINVAL,
>  				     "external clock %lu is not supported\n",
>  				     freq);
> +	if (strstr(dev_name(ov13b->dev), "OVTI13B1:01"))
> +		ov13b->avdd = devm_regulator_get_optional(ov13b->dev, "ana");
> +	else
> +		ov13b->avdd = devm_regulator_get_optional(ov13b->dev, "avdd");

Again you're providing the map information in patch 1/2 just call it avdd!

>  
> -	ov13b->avdd = devm_regulator_get_optional(ov13b->dev, "avdd");
>  	if (IS_ERR(ov13b->avdd)) {
>  		ret = PTR_ERR(ov13b->avdd);
>  		ov13b->avdd = NULL;
> @@ -1499,6 +1513,24 @@ static int ov13b10_get_pm_resources(struct ov13b10 *ov13b)
>  			return dev_err_probe(ov13b->dev, ret,
>  					     "failed to get avdd regulator\n");
>  	}
> +	if (strstr(dev_name(ov13b->dev), "OVTI13B1:01")){
> +		ov13b->avdd = devm_regulator_get_optional(ov13b->dev, "dovdd");
> +		if (IS_ERR(ov13b->avdd)) {
> +			ret = PTR_ERR(ov13b->avdd);
> +			ov13b->avdd = NULL;
> +			if (ret != -ENODEV)
> +				return dev_err_probe(ov13b->dev, ret,
> +			               "failed to get avdd regulator\n");
> +		}
> +		ov13b->avdd = devm_regulator_get_optional(ov13b->dev, "dvdd");
> +		if (IS_ERR(ov13b->avdd)) {
> +			ret = PTR_ERR(ov13b->avdd);
> +			ov13b->avdd = NULL;
> +			if (ret != -ENODEV)
> +				return dev_err_probe(ov13b->dev, ret,
> +					"failed to get avdd regulator\n");
> +		}
> +	}

You're using ov13b->avdd to store the other 2 regulators too that cannot
be right...

Also for adding multiple regulators you should use the bulk regulator API,
see e.g. :

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/media/i2c/hi556.c?id=375fc903e57cb3ca4d2d5408de98d6369d4c8334

And there is no need for the if (strstr(dev_name(ov13b->dev), "OVTI13B1:01")){
here, the regulator core will provide dummy regulators on boards
where there are no regulators defined for dovdd and dvdd.

Regards,

Hans