Hi Heimir, On 19-Dec-24 6:51 PM, Heimir Thor Sverrisson wrote: > Add a new driver for the Omnivision OV02C10 camera sensor. This is based > on the out of tree driver by Hao Yao <hao.yao@xxxxxxxxx> from: > https://github.com/intel/ipu6-drivers/blob/master/drivers/media/i2c/ov02c10.c > > This has been tested on a Dell XPS 9440 together with the IPU6 isys CSI > driver and the libcamera software ISP code. > > Tested-by: Heimir Thor Sverrisson <heimir.sverrisson@xxxxxxxxx> > Signed-off-by: Heimir Thor Sverrisson <heimir.sverrisson@xxxxxxxxx> First of all thank you for your work on cleaning up this driver. I have done a quick review and I noticed 2 things which need to be addressed, see comment inline below. When you've time please submit a v7 with this fixed and also add the Tested-by tags given in this v6 email-thread. > --- > drivers/media/i2c/Kconfig | 10 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/ov02c10.c | 1322 +++++++++++++++++++++++++++++++++++ > 3 files changed, 1333 insertions(+) > create mode 100644 drivers/media/i2c/ov02c10.c <snip> > diff --git a/drivers/media/i2c/ov02c10.c b/drivers/media/i2c/ov02c10.c > new file mode 100644 > index 000000000000..03646cabed15 > --- /dev/null > +++ b/drivers/media/i2c/ov02c10.c > @@ -0,0 +1,1322 @@ <snip> > +static int ov02c10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct ov02c10 *ov02c10 = to_ov02c10(sd); > + > + mutex_lock(&ov02c10->mutex); > + ov02c10_update_pad_format(&supported_modes[0], > + v4l2_subdev_state_get_format(fh->state, 0)); > + mutex_unlock(&ov02c10->mutex); > + return 0; > +} There are 2 issues with this: 1. Since it only touches the fh state (the ov02c10 pointer is only used for the mutex) locking the mutex is not necessary, so the locking can be dropped (and that is dropped the ov02c10 pointer can also be dropped). 2. This is not the place to init the fh state, the fh-state should be initialized through the v4l2_subdev_internal_ops.init_state operand / function pointer instead of through the v4l2_subdev_internal_ops.open op. See: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov2680.c#n790 <snip> > +static const struct v4l2_subdev_internal_ops ov02c10_internal_ops = { > + .open = ov02c10_open, As mentioned above this should be init_state instead. > +}; > + > +static int ov02c10_read_mipi_lanes(struct ov02c10 *ov02c10, struct device *dev) > +{ > + struct v4l2_fwnode_endpoint bus_cfg = { > + .bus_type = V4L2_MBUS_CSI2_DPHY > + }; > + struct fwnode_handle *ep; > + struct fwnode_handle *fwnode = dev_fwnode(dev); > + int ret = 0; > + > + ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > + if (!ep) > + return -ENXIO; > + > + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > + fwnode_handle_put(ep); > + if (ret) > + return ret; The calling of fwnode_graph_get_next_endpoint() + v4l2_fwnode_endpoint_alloc_parse() is already done in ov02c10_check_hwcfg(). Please modify ov02c10_check_hwcfg() to take an additonal struct ov02c10 *ov02c10 argument and then do the checking + storing of .num_data_lanes there and drop this function (as well as the calling of this function. > + > + if (bus_cfg.bus.mipi_csi2.num_data_lanes != 2 && > + bus_cfg.bus.mipi_csi2.num_data_lanes != 4) { > + dev_err(dev, "number of CSI2 data lanes %d is not supported", > + bus_cfg.bus.mipi_csi2.num_data_lanes); > + return(-EINVAL); > + } > + ov02c10->mipi_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes; > + return ret; > +} <snip> Regards, Hans
