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