Re: [PATCH 2/5] media: i2c: imx355: Support device tree probing and resource management

[Krzysztof Kozlowski <krzk@kernel.org>]

On 01/07/2025 00:59, Richard Acayan wrote:

>  static const struct imx355_reg imx355_global_regs[] = {
> @@ -1683,6 +1698,7 @@ static struct imx355_hwcfg *imx355_get_hwcfg(struct device *dev)
>  static int imx355_probe(struct i2c_client *client)
>  {
>  	struct imx355 *imx355;
> +	size_t i;
>  	int ret;
>  
>  	imx355 = devm_kzalloc(&client->dev, sizeof(*imx355), GFP_KERNEL);
> @@ -1694,6 +1710,42 @@ static int imx355_probe(struct i2c_client *client)
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx355->sd, client, &imx355_subdev_ops);
>  
> +	for (i = 0; i < ARRAY_SIZE(imx355_supply_names); i++)
> +		imx355->supplies[i].supply = imx355_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(&client->dev,
> +				      ARRAY_SIZE(imx355->supplies),
> +				      imx355->supplies);
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "could not get regulators");

Syntax is return dev_err_probe

> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx355->supplies),
> +				    imx355->supplies);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable regulators: %d\n", ret);
> +		return ret;
> +	}
> +
> +	imx355->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_HIGH);

So you keep the device disabled all the time... this makes no sense. How
can it work if it is always disabled?

> +	if (IS_ERR(imx355->reset_gpio)) {
> +		ret = PTR_ERR(imx355->reset_gpio);
> +		dev_err_probe(&client->dev, ret, "failed to get gpios");
> +		goto error_vreg_disable;
> +	}
> +
> +	imx355->mclk = devm_clk_get(&client->dev, "mclk");
> +	if (IS_ERR(imx355->mclk)) {
> +		ret = PTR_ERR(imx355->mclk);
> +		dev_err_probe(&client->dev, ret, "failed to get mclk");

No, syntax is ret = dev_err_probe.

> +		goto error_vreg_disable;
> +	}
> +
> +	clk_prepare_enable(imx355->mclk);
> +	usleep_range(12000, 13000);
> +
>  	/* Check module identity */
>  	ret = imx355_identify_module(imx355);
>  	if (ret) {
> @@ -1756,6 +1808,10 @@ static int imx355_probe(struct i2c_client *client)
>  
>  error_probe:
>  	mutex_destroy(&imx355->mutex);
> +	clk_disable_unprepare(imx355->mclk);



And where do you clean in remove()? No, just use devm_clk_get_enabled. I
think you are trying to upstream some old code. No, don't. You repeat
several poor patterns, like these err_probe handling or above clock bug
(actual bug). Just look how newest drivers are doing, not the
downstream. Downstream is the greatest antipattern. Absolute antipattern.


Best regards,
Krzysztof