Hi Michael,
On Thu, Jun 17, 2021 at 02:44:28PM +0200, Michael Tretter wrote:
> Hi Sakari,
>
> On Thu, 17 Jun 2021 12:56:18 +0300, Sakari Ailus wrote:
> > Hi Michael,
> >
> > Thanks for the patch.
> >
> > On Thu, Jun 17, 2021 at 11:25:38AM +0200, Michael Tretter wrote:
...
> > > +/* Menu items for LINK_FREQ V4L2 control */
> > > +static const s64 link_freq_menu_items[] = {
> > > + /* 1 channel, 1 lane or 2 channels, 2 lanes */
> > > + 108000000,
> > > + /* 2 channels, 1 lane or 4 channels, 2 lanes */
> > > + 216000000,
> > > + /* 4 channels, 1 lane */
> > > + 432000000,
> >
> > This should come from DT, or at least you should check the information in
> > DT matches with what the driver can do.
> >
>
> I'm not sure if I understand correctly.
>
> The clock is generated by the ISL7998x based on the number of MIPI CSI-2 lanes
> and input channels. The lanes and channels are already described in the device
> tree and the clock is selected accordingly by the driver. I don't think the
> actual frequencies belong into the device tree.
Right. If the clock frequency indeed is determined by the number of lanes
and the number of lanes alone, then there's no need to put that to DT.
...
> > > +static int isl7998x_s_stream(struct v4l2_subdev *sd, int enable)
> > > +{
> > > + struct isl7998x *isl7998x = sd_to_isl7998x(sd);
> > > + struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > + struct device *dev = &client->dev;
> > > + int ret = 0;
> > > + u32 reg;
> > > +
> > > + dev_dbg(dev, "stream %s\n", enable ? "ON" : "OFF");
> > > +
> >
> > I think you need to resume the device here when streaming is started, and
> > stop it when it's stopped (at the end).
>
> I am using the driver on an i.MX6. The i.MX6 MIPI CSI expects that the MIPI
> CSI-2 device enters LP-11 before it calls s_stream to start streaming.
> Therefore, the ISL7998x has to be powered on before s_stream is called. This
> is also the reason, why I am still using the s_power callback.
>
> Is there some other possibility to handle this or am I missing something? If I
> can handle this differently, I will happily drop the s_power callback.
Good question.
There's been a plan to add new pre- and post-streaming callbacks. We could
do that now. I've cc'd you in another set I wrote.
>
> >
> > > + if (enable) {
> > > + ret = isl7998x_set_test_pattern(isl7998x);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + regmap_read(isl7998x->regmap,
> > > + ISL7998x_REG_P5_LI_ENGINE_CTL, ®);
> > > + if (enable)
> > > + reg &= ~BIT(7);
> > > + else
> > > + reg |= BIT(7);
> > > + ret = regmap_write(isl7998x->regmap,
> > > + ISL7998x_REG_P5_LI_ENGINE_CTL, reg);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int isl7998x_enum_mbus_code(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + struct v4l2_subdev_mbus_code_enum *code)
> > > +{
> > > + if (code->index >= ARRAY_SIZE(isl7998x_colour_fmts))
> > > + return -EINVAL;
> > > +
> > > + code->code = isl7998x_colour_fmts[code->index].code;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int isl7998x_enum_frame_size(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + struct v4l2_subdev_frame_size_enum *fse)
> > > +{
> > > + if (fse->index >= ARRAY_SIZE(supported_modes))
> > > + return -EINVAL;
> > > +
> > > + if (fse->code != isl7998x_colour_fmts[0].code)
> > > + return -EINVAL;
> > > +
> > > + fse->min_width = supported_modes[fse->index].width;
> > > + fse->max_width = fse->min_width;
> > > + fse->min_height = supported_modes[fse->index].height;
> > > + fse->max_height = fse->min_height;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int isl7998x_get_fmt(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + struct v4l2_subdev_format *format)
> > > +{
> > > + struct isl7998x *isl7998x = sd_to_isl7998x(sd);
> > > + struct v4l2_mbus_framefmt *mf = &format->format;
> > > + const struct isl7998x_mode *mode = isl7998x_get_mode(isl7998x);
> > > +
> > > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > + format->format = *v4l2_subdev_get_try_format(sd, cfg, format->pad);
> >
> > There seem to be a number of lines over 80 that also seem be better wrapped
> > on the next one. Could you do that?
>
> checkpatch.pl increased the limit for the line length to 100. If you want, I
> can limit the lines to 80 and wrap them accordingly.
Please. There have been no changes in coding style. checkpatch.pl simply
assumes that if you exceed the limit you know what you're doing.
...
> > > +static int __maybe_unused isl7998x_runtime_resume(struct device *dev)
> > > +{
> > > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > + struct isl7998x *isl7998x = sd_to_isl7998x(sd);
> > > +
> > > + return isl7998x_power_on(isl7998x);
> > > +}
> > > +
> > > +static int __maybe_unused isl7998x_runtime_suspend(struct device *dev)
> > > +{
> > > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > > + struct isl7998x *isl7998x = sd_to_isl7998x(sd);
> > > +
> > > + isl7998x_power_off(isl7998x);
> >
> > Please merge the two functions; same for runtime_resume callback above.
>
> Ack.
>
> Thanks for the review!
You're welcome!
--
Kind regards,
Sakari Ailus
