| Message ID | 20210219081514.1592033-1-maxime.chevallier@bootlin.com |
|---|---|
| Headers | show |
| Series | media: i2c: Introduce driver for the TW9900 decoder | expand |
Hi Maxime, Some more code review comments: On 19/02/2021 09:15, 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, with basic support for NTSC > and PAL, along with brightness and contrast controls. > > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com> > --- > v1 -> v2: Set the media entity type to decoder, and implement the > s_std/g_std ops > > V2 ->V3 : Fix coding-style issues, and remove the use of the bulk API > for regulators. Make the driver select the media-controller and > V4L2-subdev APIs. > > V3->V4: Fix a warning about an uninitialized ret variable in probe() > > MAINTAINERS | 6 + > drivers/media/i2c/Kconfig | 11 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/tw9900.c | 617 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 635 insertions(+) > create mode 100644 drivers/media/i2c/tw9900.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index bfc1b86e3e73..dde9f7bdd720 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17425,6 +17425,12 @@ L: linux-media@vger.kernel.org > S: Maintained > F: drivers/media/rc/ttusbir.c > > +TECHWELL TW9900 VIDEO DECODER > +M: Maxime Chevallier <maxime.chevallier@bootlin.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +F: drivers/media/i2c/tw9900.c > + > TECHWELL TW9910 VIDEO DECODER > L: linux-media@vger.kernel.org > S: Orphan > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig > index 2b9d81e4794a..5f902557c8e5 100644 > --- a/drivers/media/i2c/Kconfig > +++ b/drivers/media/i2c/Kconfig > @@ -426,6 +426,17 @@ config VIDEO_TW2804 > To compile this driver as a module, choose M here: the > module will be called tw2804. > > +config VIDEO_TW9900 > + tristate "Techwell TW9900 video decoder" > + depends on VIDEO_V4L2 && I2C > + select MEDIA_CONTROLLER > + select VIDEO_V4L2_SUBDEV_API > + help > + Support for the Techwell tw9900 multi-standard video decoder. > + > + To compile this driver as a module, choose M here: the > + module will be called tw9900. > + > config VIDEO_TW9903 > tristate "Techwell TW9903 video decoder" > depends on VIDEO_V4L2 && I2C > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile > index a3149dce21bb..9e7df99201c6 100644 > --- a/drivers/media/i2c/Makefile > +++ b/drivers/media/i2c/Makefile > @@ -49,6 +49,7 @@ obj-$(CONFIG_VIDEO_TVP5150) += tvp5150.o > obj-$(CONFIG_VIDEO_TVP514X) += tvp514x.o > obj-$(CONFIG_VIDEO_TVP7002) += tvp7002.o > obj-$(CONFIG_VIDEO_TW2804) += tw2804.o > +obj-$(CONFIG_VIDEO_TW9900) += tw9900.o > obj-$(CONFIG_VIDEO_TW9903) += tw9903.o > obj-$(CONFIG_VIDEO_TW9906) += tw9906.o > obj-$(CONFIG_VIDEO_TW9910) += tw9910.o > diff --git a/drivers/media/i2c/tw9900.c b/drivers/media/i2c/tw9900.c > new file mode 100644 > index 000000000000..635268211982 > --- /dev/null > +++ b/drivers/media/i2c/tw9900.c > @@ -0,0 +1,617 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for the Techwell TW9900 multi-standard video decoder. > + * > + * Copyright (C) 2018 Fuzhou Rockchip Electronics Co., Ltd. > + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com> > + */ > + > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > +#include <linux/sysfs.h> > +#include <linux/timer.h> > +#include <linux/delay.h> > +#include <media/media-entity.h> > +#include <media/v4l2-async.h> > +#include <media/v4l2-ctrls.h> > +#include <media/v4l2-event.h> > +#include <media/v4l2-subdev.h> > + > +#define TW9900_REG_CHIP_ID 0x00 > +#define TW9900_REG_CHIP_STATUS 0x01 > +#define TW9900_REG_CHIP_STATUS_VLOCK 0x08 > +#define TW9900_REG_CHIP_STATUS_VDLOSS 0x80 > +#define TW9900_REG_OUT_FMT_CTL 0x03 > +#define TW9900_REG_OUT_FMT_CTL_STANDBY 0xA7 > +#define TW9900_REG_OUT_FMT_CTL_STREAMING 0xA0 > +#define TW9900_REG_CKHY_HSDLY 0x04 > +#define TW9900_REG_OUT_CTRL_I 0x05 > +#define TW9900_REG_ANALOG_CTL 0x06 > +#define TW9900_REG_CROP_HI 0x07 > +#define TW9900_REG_VDELAY_LO 0x08 > +#define TW9900_REG_VACTIVE_LO 0x09 > +#define TW9900_REG_HACTIVE_LO 0x0B > +#define TW9900_REG_CNTRL1 0x0C > +#define TW9900_REG_BRIGHT_CTL 0x10 > +#define TW9900_REG_CONTRAST_CTL 0x11 > +#define TW9900_REG_VBI_CNTL 0x19 > +#define TW9900_REG_ANAL_CTL_II 0x1A > +#define TW9900_REG_OUT_CTRL_II 0x1B > +#define TW9900_REG_STD_SEL 0x1C > +#define TW9900_REG_MISSCNT 0x26 > +#define TW9900_REG_MISC_CTL_II 0x2F > +#define TW9900_REG_VVBI 0x55 > + > +#define TW9900_CHIP_ID 0x00 > + > +#define VSYNC_POLL_INTERVAL_MS 20 > +#define VSYNC_WAIT_MAX_POLLS 50 > + > +struct regval { > + u8 addr; > + u8 val; > +}; > + > +struct tw9900_mode { > + u32 width; > + u32 height; > + u32 skip_top; > + u32 std; > + u32 field; > + const struct regval *reg_list; > + int n_regs; > +}; > + > +struct tw9900 { > + struct i2c_client *client; > + struct gpio_desc *reset_gpio; > + struct regulator *regulator; > + > + bool streaming; > + > + struct v4l2_subdev subdev; > + struct v4l2_ctrl_handler hdl; > + struct media_pad pad; > + > + struct timer_list timer; > + struct work_struct work_i2c_poll; > + > + const struct tw9900_mode *cur_mode; > +}; > + > +#define to_tw9900(sd) container_of(sd, struct tw9900, subdev) > + > +static const struct regval tw9900_init_regs[] = { > + { TW9900_REG_MISC_CTL_II, 0xE6 }, > + { TW9900_REG_MISSCNT, 0x24 }, > + { TW9900_REG_OUT_FMT_CTL, 0xA7 }, > + { TW9900_REG_ANAL_CTL_II, 0x0A }, > + { TW9900_REG_VDELAY_LO, 0x19 }, > + { TW9900_REG_STD_SEL, 0x00 }, > + { TW9900_REG_VACTIVE_LO, 0xF0 }, > + { TW9900_REG_STD_SEL, 0x07 }, > + { TW9900_REG_CKHY_HSDLY, 0x40 }, > + { TW9900_REG_ANALOG_CTL, 0x80 }, > + { TW9900_REG_CNTRL1, 0xDC }, > + { TW9900_REG_OUT_CTRL_I, 0x98 }, > +}; > + > +static const struct regval tw9900_pal_regs[] = { > + { TW9900_REG_STD_SEL, 0x01 }, > +}; > + > +static const struct regval tw9900_ntsc_regs[] = { > + { TW9900_REG_OUT_FMT_CTL, 0xA4 }, > + { TW9900_REG_VDELAY_LO, 0x12 }, > + { TW9900_REG_VACTIVE_LO, 0xF0 }, > + { TW9900_REG_CROP_HI, 0x02 }, > + { TW9900_REG_HACTIVE_LO, 0xD0 }, > + { TW9900_REG_VBI_CNTL, 0x01 }, > + { TW9900_REG_STD_SEL, 0x00 }, > +}; > + > +static const struct tw9900_mode supported_modes[] = { > + { > + .width = 720, > + .height = 576, > + .skip_top = 0, > + .std = V4L2_STD_PAL, > + .field = V4L2_FIELD_NONE, > + .reg_list = tw9900_pal_regs, > + .n_regs = ARRAY_SIZE(tw9900_pal_regs), > + }, > + { > + .width = 720, > + .height = 480, > + .skip_top = 0, > + .std = V4L2_STD_NTSC, > + .field = V4L2_FIELD_NONE, > + .reg_list = tw9900_ntsc_regs, > + .n_regs = ARRAY_SIZE(tw9900_ntsc_regs), > + }, > +}; > + > +static int tw9900_write_reg(struct i2c_client *client, u8 reg, u8 val) > +{ > + int ret; > + > + ret = i2c_smbus_write_byte_data(client, reg, val); > + > + if (ret < 0) > + dev_err(&client->dev, "write reg error: %d\n", ret); > + > + return ret; > +} > + > +static int tw9900_write_array(struct i2c_client *client, > + const struct regval *regs, int n_regs) > +{ > + int i, ret = 0; > + > + for (i = 0; ret == 0 && i <= n_regs; i++) > + ret = tw9900_write_reg(client, regs[i].addr, regs[i].val); > + > + return ret; > +} > + > +static inline u8 tw9900_read_reg(struct i2c_client *client, u8 reg) > +{ > + return i2c_smbus_read_byte_data(client, reg); > +} > + > +static void tw9900_fill_fmt(const struct tw9900_mode *mode, > + struct v4l2_mbus_framefmt *fmt) > +{ > + fmt->code = MEDIA_BUS_FMT_UYVY8_2X8; > + fmt->width = mode->width; > + fmt->height = mode->height; > + fmt->field = mode->field; > + fmt->colorspace = V4L2_COLORSPACE_SMPTE170M; > +} > + > +static int tw9900_set_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct tw9900 *tw9900 = to_tw9900(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt); > + > + mbus_fmt->width = tw9900->cur_mode->width; > + mbus_fmt->height = tw9900->cur_mode->height; These two lines are already done in tw9900_fill_fmt. > + > + return 0; > +} > + > +static int tw9900_get_fmt(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_format *fmt) > +{ > + struct tw9900 *tw9900 = to_tw9900(sd); > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; > + > + tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt); > + > + return 0; > +} In fact, tw9900_set_fmt is identical to tw9900_get_fmt. I would just drop tw9900_set_fmt and point both .get_fmt and .set_fmt to the same function. > + > +static int tw9900_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_mbus_code_enum *code) > +{ > + if (code->index >= 1) > + return -EINVAL; > + > + code->code = MEDIA_BUS_FMT_UYVY8_2X8; > + > + return 0; > +} > + > +static int tw9900_enum_frame_sizes(struct v4l2_subdev *sd, > + struct v4l2_subdev_pad_config *cfg, > + struct v4l2_subdev_frame_size_enum *fse) > +{ > + u32 index = fse->index; > + > + if (index >= 1) > + return -EINVAL; > + > + fse->code = MEDIA_BUS_FMT_UYVY8_2X8; > + > + fse->min_width = supported_modes[index].width; > + fse->max_width = supported_modes[index].width; > + fse->max_height = supported_modes[index].height; > + fse->min_height = supported_modes[index].height; > + > + return 0; > +} This function is not typically used by video receivers since the framesize is fixed depending on the standard. So there is nothing to enumerate. It is wrong in any case since it reports just supported_modes[0] (i.e. PAL) even if the current mode is NTSC. But it can just be dropped for video receivers. > + > +static int tw9900_power_on(struct tw9900 *tw9900) > +{ > + int ret; > + struct device *dev = &tw9900->client->dev; > + > + if (tw9900->reset_gpio) > + gpiod_set_value_cansleep(tw9900->reset_gpio, 1); > + > + ret = regulator_enable(tw9900->regulator); > + if (ret < 0) > + goto error; > + > + usleep_range(50000, 52000); > + > + if (tw9900->reset_gpio) > + gpiod_set_value_cansleep(tw9900->reset_gpio, 0); > + > + usleep_range(1000, 2000); > + > + ret = tw9900_write_array(tw9900->client, tw9900_init_regs, > + ARRAY_SIZE(tw9900_init_regs)); > + if (ret) { > + dev_err(dev, "Failed to init tw9900\n"); > + goto error; > + } > + > + return 0; > + > +error: > + > + return ret; > +} > + > +static void tw9900_power_off(struct tw9900 *tw9900) > +{ > + if (tw9900->reset_gpio) > + gpiod_set_value_cansleep(tw9900->reset_gpio, 1); > + > + regulator_disable(tw9900->regulator); > +} > + > +static int tw9900_s_ctrl(struct v4l2_ctrl *ctrl) > +{ > + struct tw9900 *tw9900 = container_of(ctrl->handler, struct tw9900, hdl); > + > + if (pm_runtime_suspended(&tw9900->client->dev)) > + return 0; > + > + switch (ctrl->id) { > + case V4L2_CID_BRIGHTNESS: > + tw9900_write_reg(tw9900->client, 0x10, (u8)ctrl->val); > + break; > + case V4L2_CID_CONTRAST: > + tw9900_write_reg(tw9900->client, 0x11, (u8)ctrl->val); > + break; > + default: > + return -EINVAL; > + } > + return 0; > +} > + > +static int tw9900_s_stream(struct v4l2_subdev *sd, int on) > +{ > + struct tw9900 *tw9900 = to_tw9900(sd); > + struct i2c_client *client = tw9900->client; > + int i, ret = 0; > + > + on = !!on; > + if (on == tw9900->streaming) > + return 0; > + > + if (on) { > + ret = pm_runtime_get_sync(&tw9900->client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > + return ret; > + } > + > + ret = v4l2_ctrl_handler_setup(sd->ctrl_handler); > + if (ret) > + goto put_and_return; > + > + ret = tw9900_write_array(tw9900->client, > + tw9900->cur_mode->reg_list, > + tw9900->cur_mode->n_regs); > + if (ret) > + goto put_and_return; > + > + /* Wait for VSync lock */ > + for (i = 0; i < VSYNC_WAIT_MAX_POLLS; i++) { > + u8 status = tw9900_read_reg(tw9900->client, > + TW9900_REG_CHIP_STATUS); > + if (!(status & TW9900_REG_CHIP_STATUS_VDLOSS) && > + (status & TW9900_REG_CHIP_STATUS_VLOCK)) > + break; > + > + msleep(VSYNC_POLL_INTERVAL_MS); > + } Why do you have to wait for a vsync lock? Most drivers just start regardless of lock. If there is no lock, then there is either no data being streamed (so the DMA of the video bridge will be idle as well) or it is just transmitting noise (typical for SDTV receivers). At least until a valid signal appears eventually. > + > + ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL, > + TW9900_REG_OUT_FMT_CTL_STREAMING); > + if (ret) > + goto put_and_return; > + > + } else { > + tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL, > + TW9900_REG_OUT_FMT_CTL_STANDBY); > + pm_runtime_put(&client->dev); > + } > + > + tw9900->streaming = on; > + > + return ret; > + > +put_and_return: > + pm_runtime_put(&client->dev); > + > + return ret; > +} > + > +static int tw9900_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) > +{ > + struct tw9900 *tw9900 = to_tw9900(sd); > + struct v4l2_mbus_framefmt *try_fmt; > + > + try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0); > + > + /* Initialize try_fmt */ > + tw9900_fill_fmt(tw9900->cur_mode, try_fmt); > + > + return 0; > +} Since the format is fixed based on the current standard, there is no point in initializing try_fmt as it won't be used. So just drop tw9900_open altogether. > + > +static int tw9900_runtime_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct tw9900 *tw9900 = to_tw9900(sd); > + > + return tw9900_power_on(tw9900); > +} > + > +static int tw9900_runtime_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct tw9900 *tw9900 = to_tw9900(sd); > + > + tw9900_power_off(tw9900); > + > + return 0; > +} > + > +static int tw9900_subscribe_event(struct v4l2_subdev *sd, > + struct v4l2_fh *fh, > + struct v4l2_event_subscription *sub) > +{ > + switch (sub->type) { > + case V4L2_EVENT_SOURCE_CHANGE: > + return v4l2_src_change_event_subdev_subscribe(sd, fh, sub); > + case V4L2_EVENT_CTRL: > + return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub); > + default: > + return -EINVAL; > + } > +} > + > +static const struct tw9900_mode *tw9900_get_mode_from_std(v4l2_std_id std) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(supported_modes); i++) > + if (supported_modes[i].std == std) > + return &supported_modes[i]; > + > + return NULL; > +} > + > +static int tw9900_s_std(struct v4l2_subdev *sd, v4l2_std_id norm) > +{ > + struct tw9900 *tw9900 = to_tw9900(sd); > + const struct tw9900_mode *mode; > + int ret; > + > + if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL))) > + return -EINVAL; > + > + mode = tw9900_get_mode_from_std(norm); > + if (!mode) > + return -EINVAL; > + > + ret = tw9900_write_array(tw9900->client, mode->reg_list, mode->n_regs); > + if (ret) > + return ret; > + > + tw9900->cur_mode = mode; > + > + return 0; > +} > + > +static int tw9900_g_std(struct v4l2_subdev *sd, v4l2_std_id *norm) > +{ > + struct tw9900 *tw9900 = to_tw9900(sd); > + > + *norm = tw9900->cur_mode->std; > + > + return 0; > +} > + > +static const struct dev_pm_ops tw9900_pm_ops = { > + SET_RUNTIME_PM_OPS(tw9900_runtime_suspend, > + tw9900_runtime_resume, NULL) > +}; > + > +static const struct v4l2_subdev_core_ops tw9900_core_ops = { > + .subscribe_event = tw9900_subscribe_event, > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > +}; > + > +static const struct v4l2_subdev_video_ops tw9900_video_ops = { > + .s_std = tw9900_s_std, > + .g_std = tw9900_g_std, Can the tw9900 detect the standard? (I.e. PAL, SECAM, NTSC) If so, you should implement querystd. I see that none of the other tw*.c drivers support this, so I suspect there is no hardware support for this. You also must implement g_tvnorms to report the TV standards that the hardware can understand. > + .s_stream = tw9900_s_stream, > +}; > + > +static const struct v4l2_subdev_pad_ops tw9900_pad_ops = { > + .enum_mbus_code = tw9900_enum_mbus_code, > + .enum_frame_size = tw9900_enum_frame_sizes, > + .get_fmt = tw9900_get_fmt, > + .set_fmt = tw9900_set_fmt, > +}; > + > +static const struct v4l2_subdev_ops tw9900_subdev_ops = { > + .core = &tw9900_core_ops, > + .video = &tw9900_video_ops, > + .pad = &tw9900_pad_ops, > +}; > + > +static const struct v4l2_ctrl_ops tw9900_ctrl_ops = { > + .s_ctrl = tw9900_s_ctrl, > +}; > + > +static const struct v4l2_subdev_internal_ops tw9900_internal_ops = { > + .open = tw9900_open, > +}; > + > +static int tw9900_check_id(struct tw9900 *tw9900, > + 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, "Unexpected decoder id(%04x)\n", id); > + return -EINVAL; > + } > + > + dev_info(dev, "Detected TW9900 (%04x) decoder\n", TW9900_CHIP_ID); > + > + return 0; > +} > + > +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 = 0; > + > + 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; > + > + tw9900->regulator = devm_regulator_get(&tw9900->client->dev, "vdd"); > + if (IS_ERR(tw9900->regulator)) { > + dev_err(dev, "Failed to get power regulator\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_id(tw9900, client); > + if (ret) > + goto err_power_off; > + > + tw9900->subdev.internal_ops = &tw9900_internal_ops; > + tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; This is a duplicate of a similar '|=' above. > + tw9900->pad.flags = MEDIA_PAD_FL_SOURCE; > + tw9900->subdev.entity.function = MEDIA_ENT_F_DV_DECODER; > + > + ret = media_entity_pads_init(&tw9900->subdev.entity, 1, &tw9900->pad); > + if (ret < 0) > + goto err_power_off; > + > + ret = v4l2_async_register_subdev(&tw9900->subdev); > + if (ret) { > + dev_err(dev, "v4l2 async register subdev failed\n"); > + goto err_clean_entity; > + } > + > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + pm_runtime_idle(dev); > + > + return 0; > + > +err_clean_entity: > + media_entity_cleanup(&tw9900->subdev.entity); > +err_power_off: > + tw9900_power_off(tw9900); > + > + return ret; > +} > + > +static int tw9900_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct tw9900 *tw9900 = to_tw9900(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + > + pm_runtime_disable(&client->dev); > + if (!pm_runtime_status_suspended(&client->dev)) > + tw9900_power_off(tw9900); > + pm_runtime_set_suspended(&client->dev); > + > + return 0; > +} > + > +static const struct of_device_id tw9900_of_match[] = { > + { .compatible = "techwell,tw9900" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, tw9900_of_match); > + > +static struct i2c_driver tw9900_i2c_driver = { > + .driver = { > + .name = "tw9900", > + .pm = &tw9900_pm_ops, > + .of_match_table = tw9900_of_match > + }, > + .probe = tw9900_probe, > + .remove = tw9900_remove, > +}; > + > +module_i2c_driver(tw9900_i2c_driver); > + > +MODULE_DESCRIPTION("tw9900 decoder driver"); > +MODULE_LICENSE("GPL v2"); > Regards, Hans
Hi Hans, and thanks for the review. On Thu, 4 Mar 2021 16:37:53 +0100 Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: >Hi Maxime, > >Some more code review comments: > >> +static int tw9900_set_fmt(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_format *fmt) >> +{ >> + struct tw9900 *tw9900 = to_tw9900(sd); >> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; >> + >> + tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt); >> + >> + mbus_fmt->width = tw9900->cur_mode->width; >> + mbus_fmt->height = tw9900->cur_mode->height; > >These two lines are already done in tw9900_fill_fmt. Yes right, I'll remove that [...] >> + >> + return 0; >> +} >> + >> +static int tw9900_get_fmt(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_format *fmt) >> +{ >> + struct tw9900 *tw9900 = to_tw9900(sd); >> + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format; >> + >> + tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt); >> + >> + return 0; >> +} > >In fact, tw9900_set_fmt is identical to tw9900_get_fmt. I would just drop >tw9900_set_fmt and point both .get_fmt and .set_fmt to the same function. OK, that will be cleaner indeed [...] >> + >> +static int tw9900_enum_mbus_code(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_mbus_code_enum *code) >> +{ >> + if (code->index >= 1) >> + return -EINVAL; >> + >> + code->code = MEDIA_BUS_FMT_UYVY8_2X8; >> + >> + return 0; >> +} >> + >> +static int tw9900_enum_frame_sizes(struct v4l2_subdev *sd, >> + struct v4l2_subdev_pad_config *cfg, >> + struct v4l2_subdev_frame_size_enum *fse) >> +{ >> + u32 index = fse->index; >> + >> + if (index >= 1) >> + return -EINVAL; >> + >> + fse->code = MEDIA_BUS_FMT_UYVY8_2X8; >> + >> + fse->min_width = supported_modes[index].width; >> + fse->max_width = supported_modes[index].width; >> + fse->max_height = supported_modes[index].height; >> + fse->min_height = supported_modes[index].height; >> + >> + return 0; >> +} > >This function is not typically used by video receivers since the framesize is >fixed depending on the standard. So there is nothing to enumerate. > >It is wrong in any case since it reports just supported_modes[0] (i.e. PAL) >even if the current mode is NTSC. But it can just be dropped for video receivers. Ok thanks, I'll drop that then. [...] >> + /* Wait for VSync lock */ >> + for (i = 0; i < VSYNC_WAIT_MAX_POLLS; i++) { >> + u8 status = tw9900_read_reg(tw9900->client, >> + TW9900_REG_CHIP_STATUS); >> + if (!(status & TW9900_REG_CHIP_STATUS_VDLOSS) && >> + (status & TW9900_REG_CHIP_STATUS_VLOCK)) >> + break; >> + >> + msleep(VSYNC_POLL_INTERVAL_MS); >> + } > >Why do you have to wait for a vsync lock? > >Most drivers just start regardless of lock. If there is no lock, then there >is either no data being streamed (so the DMA of the video bridge will be idle >as well) or it is just transmitting noise (typical for SDTV receivers). At least >until a valid signal appears eventually. In this case, it will transmit noise. As stated a bit below, this was implemented because this decoder actually supports automatic standard detection, but the reported standard can only be read once the vsync lock is acquired. So this is a remainder of what I implemented to try to get the standard detection work, but I can drop that for now. [...] >> +static int tw9900_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) >> +{ >> + struct tw9900 *tw9900 = to_tw9900(sd); >> + struct v4l2_mbus_framefmt *try_fmt; >> + >> + try_fmt = v4l2_subdev_get_try_format(sd, fh->pad, 0); >> + >> + /* Initialize try_fmt */ >> + tw9900_fill_fmt(tw9900->cur_mode, try_fmt); >> + >> + return 0; >> +} > >Since the format is fixed based on the current standard, there is no point >in initializing try_fmt as it won't be used. So just drop tw9900_open altogether. Ok I'll drop that :) [...] >> +static const struct v4l2_subdev_video_ops tw9900_video_ops = { >> + .s_std = tw9900_s_std, >> + .g_std = tw9900_g_std, > >Can the tw9900 detect the standard? (I.e. PAL, SECAM, NTSC) > >If so, you should implement querystd. I see that none of the other tw*.c drivers >support this, so I suspect there is no hardware support for this. So, there's hardware support for this, and I've been trying to get this to work for a while. I've come to a point where the standard detection "almost" works, but detects the wrong standard about once every 10 occurences. I don't know if this is due to the hardware I'm testing this on, my setup, or the decoder itself. This is in a setup where the standard can change on the fly (I have 2 cameras, one that streams PAL, one that streams NTSC, that are connected to the TW9900 through a switch, and I have to make so that we can detect a standard change (due to switching a camera) on the fly while the stream is started. The standard detection is also a process that is quite long and that has to be manually started, and then checked regularly to see if the decoder successfully identified a standard. I do have a followup question, which is when querystd() would be called in a "normal" scenarion (I feel that my usecase seems a bit off-track compared to classic usecases). Is it when the stream is started, or stopped ? >You also must implement g_tvnorms to report the TV standards that the hardware >can understand. Ok I didn't know about that, I'll implement that then. [...] >> + >> + tw9900->subdev.internal_ops = &tw9900_internal_ops; >> + tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > >This is a duplicate of a similar '|=' above. My bad, I'll remove that line. > >Regards, > > Hans Thanks, Maxime