Re: [PATCH 1/2] media: i2c: Add driver for Sony IMX728

[Krzysztof Kozlowski <krzk@kernel.org>]

On 26/06/2024 23:15, Spencer Hill wrote:
> Add a driver for the Sony IMX728 image sensor.
> 
> Signed-off-by: Spencer Hill <shill@d3engineering.com>
> ---


...

> +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