On 7/1/19 10:43 AM, Sakari Ailus wrote:
[...]
>> + { ISL7998x_REG_P0_CLK_CTL_1, 0x1f },
>> + { ISL7998x_REG_P0_CLK_CTL_2, 0x43 },
>> + { ISL7998x_REG_P0_CLK_CTL_3, 0x4f },
>
> It'd be great to see what these magical numbers signify. How about some
> additional #defines for the bits? Same below.
That would mean mindlessly transcribing big part of the datasheet here,
but I think it would add very little value, except for another level of
obfuscation. If anyone ever needs to tweak anything here , they would
have a datasheet along with it anyway.
[...]
>> +
>> +/* Menu items for LINK_FREQ V4L2 control */
>> +static const s64 link_freq_menu_items[] = {
>> + 108000000, 216000000, 432000000
>> +};
>
> Where to these numbers come from?
The datasheet, these are the mipi csi2 link frequencies.
>> +
>> +/* Menu items for TEST_PATTERN V4L2 control */
>> +static const char * const isl7998x_test_pattern_menu[] = {
>> + "Disabled", "Enabled-PAL (720x576)", "Enabled-NTSC (720x480)"
>> +};
>> +
>> +struct isl7998x {
>> + struct v4l2_subdev subdev;
>> + struct regmap *regmap;
>> + struct gpio_desc *pd_gpio;
>> + unsigned int nr_mipi_lanes;
>> + u32 nr_inputs;
>> +
>> + unsigned int width;
>> + unsigned int height;
>> + const struct isl7998x_datafmt *fmt;
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> + struct media_pad pad;
>
> Hmm. Didn't you have four inputs in your device?
Yes, this is the output (mipi csi2) pad. There is only one.
>
>> +#endif
>> +
>> + struct v4l2_ctrl_handler ctrl_handler;
>> + struct mutex ctrl_mutex;
>> + /* V4L2 Controls */
>> + struct v4l2_ctrl *link_freq;
>> + u8 test_pattern_enabled;
>> + u8 test_pattern_bars;
>> + u8 test_pattern_chans;
>> + u8 test_pattern_color;
>> +};
>> +
>> +static struct isl7998x *sd_to_isl7998x(struct v4l2_subdev *sd)
>> +{
>> + return container_of(sd, struct isl7998x, subdev);
>> +}
>> +
>> +static struct isl7998x *i2c_to_isl7998x(const struct i2c_client *client)
>> +{
>> + return sd_to_isl7998x(i2c_get_clientdata(client));
>> +}
>> +
>> +static int isl7998x_init(struct isl7998x *isl7998x)
>> +{
>> + const unsigned int lanes = isl7998x->nr_mipi_lanes;
>> + const u32 isl7998x_video_in_chan_map[] = { 0x00, 0x11, 0x02, 0x02 };
>> + const struct reg_sequence isl7998x_init_seq_custom[] = {
>> + { ISL7998x_REG_P0_VIDEO_IN_CHAN_CTL,
>> + isl7998x_video_in_chan_map[isl7998x->nr_inputs - 1] },
>> + { ISL7998x_REG_P0_CLK_CTL_4, (lanes == 1) ? 0x40 : 0x41 },
>> + { ISL7998x_REG_P5_LI_ENGINE_CTL, (lanes == 1) ? 0x01 : 0x02 },
>> + { ISL7998x_REG_P5_LI_ENGINE_LINE_CTL,
>> + 0x20 | ((isl7998x->width >> 7) & 0x1f) },
>> + { ISL7998x_REG_P5_LI_ENGINE_PIC_WIDTH,
>> + (isl7998x->width << 1) & 0xff },
>> + };
>> + struct regmap *regmap = isl7998x->regmap;
>> + int ret;
>> +
>> + ret = regmap_register_patch(regmap, isl7998x_init_seq_1,
>> + ARRAY_SIZE(isl7998x_init_seq_1));
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_register_patch(regmap, isl7998x_init_seq_custom,
>> + ARRAY_SIZE(isl7998x_init_seq_custom));
>> + if (ret)
>> + return ret;
>> +
>> + return regmap_register_patch(regmap, isl7998x_init_seq_2,
>> + ARRAY_SIZE(isl7998x_init_seq_2));
>> +}
>> +
>> +static int isl7998x_g_mbus_config(struct v4l2_subdev *sd,
>> + struct v4l2_mbus_config *cfg)
>> +{
>> + struct isl7998x *isl7998x = sd_to_isl7998x(sd);
>> +
>> + cfg->type = V4L2_MBUS_CSI2_DPHY;
>> + cfg->flags = V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
>> + if (isl7998x->nr_mipi_lanes == 1)
>> + cfg->flags |= V4L2_MBUS_CSI2_1_LANE;
>> + else
>> + cfg->flags |= V4L2_MBUS_CSI2_2_LANE;
>
> Drivers nowadays generally get this from DT. You can omit this function
> safely. For dynamic configuration we'll need something else (Jacopo's been
> working on that with me).
So how else would I configure my MIPI CSI2 receiver ? I presume the iMX6
CSI2 driver needs to be updated somehow ?
>> +
>> + return 0;
>> +}
>> +
>> +static int isl7998x_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> + struct isl7998x *isl7998x = sd_to_isl7998x(sd);
>> +
>> + return isl7998x_init(isl7998x);
>
> Is this needed both to initialise the hardware as well as start streaming?
Yes, otherwise the csi2 link is unstable.
> Does this also enable the CSI-2 transmitter in driver's probe? This could
> be problematic for some receivers.
How so ?
>> +}
>> +
>> +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->pad || code->index >= ARRAY_SIZE(isl7998x_colour_fmts))
>> + return -EINVAL;
>> +
>> + code->code = isl7998x_colour_fmts[code->index].code;
>
> A newline would be nice here.
>
>> + return 0;
>> +}
>> +
>> +static const unsigned int isl7998x_std_res[] = {
>> + 480, 576, 576, 480, 480, 576, 480, 480
>> +};
>> +
>> +static int isl7998x_update_std(struct isl7998x *isl7998x)
>> +{
>> + unsigned int height[ISL7998x_INPUTS];
>> + u8 scanning = GENMASK(ISL7998x_INPUTS - 1, 0);
>> + unsigned int i;
>> + int ret;
>> + u32 reg;
>> +
>> + while (true) {
>> + for (i = 0; i < ISL7998x_INPUTS; i++) {
>> + ret = regmap_read(isl7998x->regmap,
>> + ISL7998x_REG_Px_DEC_SDT(i + 1), ®);
>> + if (ret)
>> + return ret;
>> +
>> + /* Detection is still in progress, restart. */
>> + if (reg & ISL7998x_REG_Px_DEC_SDT_DET) {
>> + scanning = GENMASK(ISL7998x_INPUTS - 1, 0);
>> + break;
>> + }
>> +
>> + scanning &= ~BIT(i);
>> + height[i] = isl7998x_std_res[(reg >> 4) & 0x7];
>> + }
>> +
>> + if (!scanning)
>> + break;
>> +
>> + mdelay(1);
>
> How long does this take in general? Could you use msleep() instead of
> mdelay() in order to avoid a busy loop at least seemingly of unknown
> length?
I changed it to usleep_range()
>> + }
>> +
>> + /*
>> + * According to Renesas FAE, all input cameras must have the
>> + * same standard on this chip.
>> + */
>> + for (i = 1; i < isl7998x->nr_inputs ; i++)
>> + if (height[i - 1] != height[i])
>> + return -EINVAL;
>> +
>> + isl7998x->height = height[0];
>> +
>> + 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;
>> +
>> + if (format->pad != 0)
>> + return -EINVAL;
>> +
>> + if (isl7998x->test_pattern_enabled == 1) {
>> + mf->width = 720;
>> + mf->height = 576;
>> + } else if (isl7998x->test_pattern_enabled == 2) {
>> + mf->width = 720;
>> + mf->height = 480;
>> + } else {
>> + mf->width = isl7998x->width;
>> + mf->height = isl7998x->height;
>> + }
>> +
>> + mf->code = isl7998x->fmt->code;
>> + mf->field = V4L2_FIELD_INTERLACED;
>> + mf->colorspace = 0;
>> +
>> + return 0;
>> +}
>> +
>> +static int isl7998x_set_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;
>> + int ret;
>> +
>> + if (format->pad != 0)
>> + return -EINVAL;
>
> You can omit the pad checks in pad ops; the framework does that nowadays.
OK
>> +
>> + if (format->format.width != 720 ||
>> + (format->format.height != 480 && format->format.height != 576))
>> + return -EINVAL;
>> +
>> + if (format->format.code != MEDIA_BUS_FMT_YUYV8_2X8)
>> + return -EINVAL;
>
> Instead of returning an error, you should simply pick a valid format if the
> user-supplied format (mbus code, size) isn't supported.
>
>> +
>> + mf->width = format->format.width;
>> + mf->height = format->format.height;
>> + mf->code = format->format.code;
>> + mf->field = V4L2_FIELD_INTERLACED;
>> + mf->colorspace = 0;
>
> I'd just use what the user provided.
How can that even work ? The hardware might not support that.
>> +
>> + if (format->which != V4L2_SUBDEV_FORMAT_TRY) {
>> + ret = isl7998x_update_std(isl7998x);
>> + if (ret)
>> + return ret;
>> + mf->width = isl7998x->width;
>> + mf->height = isl7998x->height;
>> + isl7998x->fmt = &isl7998x_colour_fmts[0];
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int isl7998x_set_test_pattern(struct isl7998x *isl7998x)
>> +{
>> + const struct reg_sequence isl7998x_init_seq_tpg_off[] = {
>> + { ISL7998x_REG_P5_LI_ENGINE_TP_GEN_CTL, 0 },
>> + { ISL7998x_REG_P5_LI_ENGINE_CTL_2, 0 }
>> + };
>> + const struct reg_sequence isl7998x_init_seq_tpg_on[] = {
>> + { ISL7998x_REG_P5_TP_GEN_BAR_PATTERN,
>> + isl7998x->test_pattern_bars << 6 },
>> + { ISL7998x_REG_P5_LI_ENGINE_CTL_2,
>> + isl7998x->test_pattern_enabled == 1 ? BIT(2) : 0 },
>> + { ISL7998x_REG_P5_LI_ENGINE_TP_GEN_CTL,
>> + (isl7998x->test_pattern_chans << 4) |
>> + (isl7998x->test_pattern_color << 2) }
>> + };
>> + struct regmap *regmap = isl7998x->regmap;
>> +
>> + if (isl7998x->test_pattern_enabled) {
>> + return regmap_register_patch(regmap, isl7998x_init_seq_tpg_on,
>> + ARRAY_SIZE(isl7998x_init_seq_tpg_on));
>> + } else {
>> + return regmap_register_patch(regmap, isl7998x_init_seq_tpg_off,
>> + ARRAY_SIZE(isl7998x_init_seq_tpg_off));
>> + }
>
> No need for braces.
I believe they are required by checkpatch for multiline conditionals.
>> +}
>> +
>> +static int isl7998x_set_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct isl7998x *isl7998x = container_of(ctrl->handler,
>> + struct isl7998x, ctrl_handler);
>> +
>> + switch (ctrl->id) {
>> + case V4L2_CID_TEST_PATTERN_BARS:
>> + isl7998x->test_pattern_bars = ctrl->val & 0x3;
>> + return isl7998x_set_test_pattern(isl7998x);
>> + case V4L2_CID_TEST_PATTERN_CHANNELS:
>> + isl7998x->test_pattern_chans = ctrl->val & 0xf;
>> + return isl7998x_set_test_pattern(isl7998x);
>> + case V4L2_CID_TEST_PATTERN_COLOR:
>> + isl7998x->test_pattern_color = ctrl->val & 0x3;
>> + return isl7998x_set_test_pattern(isl7998x);
>> + case V4L2_CID_TEST_PATTERN:
>> + isl7998x->test_pattern_enabled = !!ctrl->val;
>> + return isl7998x_set_test_pattern(isl7998x);
>
> It'd simplify the driver to use the control value in
> isl7998x_set_test_pattern() without storing the intermediate result in
> driver's own struct.
You need to keep track of which controls are set and how , which is why
they are stored in the drivers' private data.
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int isl7998x_get_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> + struct isl7998x *isl7998x = container_of(ctrl->handler,
>> + struct isl7998x, ctrl_handler);
>> +
>> + v4l2_info(&isl7998x->subdev, "ctrl(id:0x%x,val:0x%x) is not handled\n",
>> + ctrl->id, ctrl->val);
>
> If you have no volatile controls, you can omit this function.
OK
[...]
>> +static int isl7998x_probe(struct i2c_client *client,
>> + const struct i2c_device_id *did)
>> +{
>> + struct device *dev = &client->dev;
>> + struct v4l2_fwnode_endpoint endpoint;
>
> Please do initialise the bus_type field at least.
v4l2_fwnode_endpoint_parse() should do that, right ?
>> + struct device_node *ep;
>> + struct isl7998x *isl7998x;
>> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>> + u32 chip_id;
>> + int i, ret;
>> +
>> + ret = i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA);
>> + if (!ret) {
>> + dev_warn(&adapter->dev,
>> + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
>> + return -EIO;
>> + }
>> +
>> + isl7998x = devm_kzalloc(dev, sizeof(*isl7998x), GFP_KERNEL);
>> + if (!isl7998x)
>> + return -ENOMEM;
>> +
>> + /* Default to all four inputs active unless specified otherwise. */
>> + ret = of_property_read_u32(dev->of_node, "isil,num-inputs",
>> + &isl7998x->nr_inputs);
>> + if (ret)
>> + isl7998x->nr_inputs = 4;
>> +
>> + if (isl7998x->nr_inputs != 1 && isl7998x->nr_inputs != 2 &&
>> + isl7998x->nr_inputs != 4) {
>> + dev_err(dev, "Invalid number of inputs selected in DT\n");
>> + return -EINVAL;
>> + }
>> +
>> + isl7998x->pd_gpio = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_HIGH);
>> + if (IS_ERR(isl7998x->pd_gpio)) {
>> + dev_err(dev, "Failed to retrieve/request PD GPIO: %ld\n",
>> + PTR_ERR(isl7998x->pd_gpio));
>> + return PTR_ERR(isl7998x->pd_gpio);
>> + }
>> +
>> + isl7998x->regmap = devm_regmap_init_i2c(client, &isl7998x_regmap);
>> + if (IS_ERR(isl7998x->regmap)) {
>> + ret = PTR_ERR(isl7998x->regmap);
>> + dev_err(dev, "Failed to allocate register map: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ep = of_graph_get_next_endpoint(dev->of_node, NULL);
>> + if (!ep) {
>> + dev_err(dev, "Missing endpoint node\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &endpoint);
>> + of_node_put(ep);
>> + if (ret) {
>> + dev_err(dev, "Failed to parse endpoint\n");
>> + return ret;
>> + }
>> +
>> + if (endpoint.bus_type != V4L2_MBUS_CSI2_DPHY ||
>> + endpoint.bus.mipi_csi2.num_data_lanes == 0 ||
>> + endpoint.bus.mipi_csi2.num_data_lanes > 2) {
>> + dev_err(dev, "Invalid bus type or number of MIPI lanes\n");
>> + return -EINVAL;
>> + }
>> +
>> + isl7998x->nr_mipi_lanes = endpoint.bus.mipi_csi2.num_data_lanes;
>> +
>> + v4l2_i2c_subdev_init(&isl7998x->subdev, client, &isl7998x_subdev_ops);
>> + isl7998x->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> + isl7998x->pad.flags = MEDIA_PAD_FL_SOURCE;
>> + isl7998x->subdev.entity.ops = &isl7998x_entity_ops;
>> + isl7998x->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
> This is definitely not a camera sensor.
>
> MEDIA_ENT_F_VID_IF_BRIDGE perhaps?
>
> How does the driver work without MC?
MC ?
> E.g. how are the formats on the inputs set up?
They are auto-detected by the chip.
[...]
--
Best regards,
Marek Vasut
