Hi Hans,
On Fri, 25 Sep 2020 14:52:25 +0200
Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>Hi Maxime,
>
>Some comments below, this driver needs to be changed:
Thanks for the review !
>On 18/09/2020 16:24, Maxime Chevallier wrote:
>> The Techwell video decoder supports PAL, NTSC and SECAM input formats,
>> and outputs a BT.656 signal.
>>
>> This commit adds support for this device, based on an implementation
>> made by Rockchip. This implemention adds basic support for NTSC and PAL,
>> and some basic brightness and contrast controls.
>>
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>
[ ... ]
>> +static const struct v4l2_subdev_ops tw9900_subdev_ops = {
>> + .core = &tw9900_core_ops,
>> + .video = &tw9900_video_ops,
>> + .pad = &tw9900_pad_ops,
>> + .sensor = &tw9900_sensor_ops,
>> +};
>
>This is wrong. This is not a sensor, so you don't set the format, instead
>you set the TV standard (s_std).
>
>drivers/media/i2c/tw9910.c is a fairly OK template to use. The tw9910 supports
>a simple scaler as well, but I don't know if the tw9900 has the same feature.
>If not, then the format resolution is fixed based on the current selected
>TV standard.
>
>There is definitely no need for g_skip_top_lines: 1) it's a sensor-only op,
>and 2) that function always returns 0, so why keep it?
Thanks for the clarification. Indeed the TW9900 is simpler and doesn't
have a scaler. I'll stick to the s_std / g_std ops then.
I'll also change the denomination from "sensor" to "decoder", you're
right.
>> +
>> +static const struct v4l2_ctrl_ops tw9900_ctrl_ops = {
>> + .s_ctrl = tw9900_s_ctrl,
>> +};
>> +
>> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
>> +static const struct v4l2_subdev_internal_ops tw9900_internal_ops = {
>> + .open = tw9900_open,
>> +};
>> +#endif
>> +
>> +static int tw9900_check_sensor_id(struct tw9900 *tw9900,
>
>*Not* a sensor :-)
>
>> + struct i2c_client *client)
>> +{
>> + struct device *dev = &tw9900->client->dev;
>> + u8 id;
>> +
>> + id = tw9900_read_reg(client, TW9900_CHIP_ID);
>> +
>> + if (id != TW9900_CHIP_ID) {
>> + dev_err(dev, "Wrong camera sensor id(%04x)\n", id);
>> + return -EINVAL;
>> + }
>> +
>> + dev_info(dev, "Detected TW9900 (%04x) sensor\n", TW9900_CHIP_ID);
>> +
>> + return 0;
>> +}
>> +
>> +static int tw9900_configure_regulators(struct tw9900 *tw9900)
>> +{
>> + u32 i;
>> +
>> + for (i = 0; i < TW9900_NUM_SUPPLIES; i++)
>> + tw9900->supplies[i].supply = tw9900_supply_names[i];
>> +
>> + return devm_regulator_bulk_get(&tw9900->client->dev,
>> + TW9900_NUM_SUPPLIES,
>> + tw9900->supplies);
>> +}
>> +
>> +static int tw9900_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct device *dev = &client->dev;
>> + struct v4l2_ctrl_handler *hdl;
>> + struct tw9900 *tw9900;
>> + int ret;
>> +
>> + tw9900 = devm_kzalloc(dev, sizeof(*tw9900), GFP_KERNEL);
>> + if (!tw9900)
>> + return -ENOMEM;
>> +
>> + tw9900->client = client;
>> + tw9900->cur_mode = &supported_modes[0];
>> +
>> + tw9900->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
>> + if (IS_ERR(tw9900->reset_gpio))
>> + tw9900->reset_gpio = NULL;
>> +
>> + ret = tw9900_configure_regulators(tw9900);
>> + if (ret) {
>> + dev_err(dev, "Failed to get power regulators\n");
>> + return ret;
>> + }
>> +
>> + v4l2_i2c_subdev_init(&tw9900->subdev, client, &tw9900_subdev_ops);
>> + tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>> +
>> + hdl = &tw9900->hdl;
>> +
>> + v4l2_ctrl_handler_init(hdl, 2);
>> +
>> + v4l2_ctrl_new_std(hdl, &tw9900_ctrl_ops, V4L2_CID_BRIGHTNESS,
>> + -128, 127, 1, 0);
>> + v4l2_ctrl_new_std(hdl, &tw9900_ctrl_ops, V4L2_CID_CONTRAST,
>> + 0, 255, 1, 0x60);
>> +
>> + tw9900->subdev.ctrl_handler = hdl;
>> + if (hdl->error) {
>> + int err = hdl->error;
>> +
>> + v4l2_ctrl_handler_free(hdl);
>> + return err;
>> + }
>> +
>> + ret = __tw9900_power_on(tw9900);
>> + if (ret)
>> + return ret;
>> +
>> + ret = tw9900_check_sensor_id(tw9900, client);
>> + if (ret)
>> + goto err_power_off;
>> +
>> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
>> + tw9900->subdev.internal_ops = &tw9900_internal_ops;
>> + tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +#endif
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> + tw9900->pad.flags = MEDIA_PAD_FL_SOURCE;
>> + tw9900->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
>Set to MEDIA_ENT_F_ATV_DECODER.
Will do ! Thanks agains,
Maxime
--
Maxime Chevallier, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
