On 26/06/2024 23:15, Spencer Hill wrote:
> Add a driver for the Sony IMX728 image sensor.
>
> Signed-off-by: Spencer Hill <shill@xxxxxxxxxxxxxxxxx>
> ---
...
> +static int imx728_probe(struct i2c_client *client)
> +{
> + struct imx728 *imx728;
> + struct v4l2_subdev *sd;
> + struct v4l2_ctrl_handler *ctrl_hdr;
> + int ret;
> +
> + imx728 = devm_kzalloc(&client->dev, sizeof(*imx728), GFP_KERNEL);
> + if (!imx728)
> + return -ENOMEM;
> +
> + imx728->dev = &client->dev;
> +
> + imx728->regmap = devm_regmap_init_i2c(client, &imx728_regmap_config);
> + if (IS_ERR(imx728->regmap))
> + return PTR_ERR(imx728->regmap);
> +
> + imx728->xclr_gpio = devm_gpiod_get_optional(imx728->dev,
> + "xclr", GPIOD_OUT_LOW);
> + if (IS_ERR(imx728->xclr_gpio))
> + return PTR_ERR(imx728->xclr_gpio);
> +
> + imx728->clk = devm_clk_get(imx728->dev, "inck");
> + if (IS_ERR(imx728->clk))
> + return PTR_ERR(imx728->clk);
> +
> + imx728->clk_rate = clk_get_rate(imx728->clk);
> + dev_info(imx728->dev, "inck rate: %lu Hz\n", imx728->clk_rate);
dev_dbg
clock rates are easy to check from sysfs
...
> +
> + dev_info(imx728->dev, "imx728 probed\n");
No, drop. It's useless and there are existing interfaces telling you this.
> + pm_runtime_mark_last_busy(imx728->dev);
> + pm_runtime_put_autosuspend(imx728->dev);
> + return 0;
> +
> +err_subdev_cleanup:
> + v4l2_subdev_cleanup(&imx728->subdev);
> +
> +err_pm_disable:
> + pm_runtime_dont_use_autosuspend(imx728->dev);
> + pm_runtime_put_noidle(imx728->dev);
> + pm_runtime_disable(imx728->dev);
> +
> +err_ctrl_free:
> + v4l2_ctrl_handler_free(ctrl_hdr);
> + mutex_destroy(&imx728->lock);
> +
> +err_media_cleanup:
> + media_entity_cleanup(&imx728->subdev.entity);
> +
> + return ret;
> +}
> +
> +MODULE_DEVICE_TABLE(of, imx728_dt_id);
> +
> +static struct i2c_driver imx728_i2c_driver = {
> + .driver = {
> + .name = "imx728",
> + .of_match_table = of_match_ptr(imx728_dt_id),
Drop of_match_ptr(), you have warnigns here.
> + .pm = &imx728_pm_ops,
> + },
> + .probe = imx728_probe,
> + .remove = imx728_remove,
> +};
> +
> +module_i2c_driver(imx728_i2c_driver);
> +struct imx728 {
> + struct device *dev;
> +
> + struct clk *clk;
> + struct i2c_client *client;
> + struct regmap *regmap;
> + struct gpio_desc *xclr_gpio;
> +
> + struct v4l2_subdev subdev;
> + struct v4l2_mbus_framefmt format;
> + struct media_pad pad;
> +
> + struct imx728_ctrl ctrl;
> +
> + unsigned long clk_rate;
> + u32 fps;
> +
> + struct mutex lock;
> + bool streaming;
> +};
> +
> +static const struct v4l2_area imx728_framesizes[] = {
> + {
> + .width = IMX728_OUT_WIDTH,
> + .height = IMX728_OUT_HEIGHT,
> + },
> +};
> +
> +static const u32 imx728_mbus_formats[] = {
No, NAK. You cannot have data allocations in header. This does not make
any sense and leeds to duplicated structures.
Best regards,
Krzysztof
