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

[Spencer Hill <shill@d3engineering.com>]

On Thu, Jun 27, 2024 at 08:57:59AM +0200, Krzysztof Kozlowski wrote:
> 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
>
> ...
>

Will fix.

> > +
> > +       dev_info(imx728->dev, "imx728 probed\n");
>
> No, drop. It's useless and there are existing interfaces telling you this.
>

Okay

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

I'll remove that.

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

I will be moving the code from the header into the c file in response to
other feedback, does this solve this problem?

>
>
> Best regards,
> Krzysztof
>

Thanks,
Spencer
Please be aware that this email includes email addresses outside of the organization.