Re: [PATCH v8 3/3] media: i2c: Introduce a driver for the Techwell TW9900 decoder

[Paul Kocialkowski]

Hi Mehdi,

This is starting to look good! See more comments below.

On Wed 08 Nov 23, 14:27, Mehdi Djait wrote:
> The Techwell video decoder supports PAL, NTSC standards and
> has a parallel BT.656 output interface.
> 
> This commit adds support for this device, with basic support
> for NTSC and PAL, along with brightness and contrast controls.
> 
> The TW9900 is capable of automatic standard detection. This
> driver is implemented with support for PAL and NTSC
> autodetection.
> 
> Signed-off-by: Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> ---
> V7->V8: 
> - added a mutex to Serialize access to hardware and current mode configuration
> - added back pm_runtime
> - split get_fmt and set_fmt callbacks 
> - removed the tw9900_cancel_autodetect()
> 
> V6->V7: 
> - added #include <linux/bitfield.h> to fix a Warning from the kernel
>   robot
> - removed a dev_info and replaced a dev_err by dev_err_probe
> 
> V5->V6: 
> - dropped .skip_top and .field in the supported_modes
> - added error handling for the i2c writes/reads
> - added the colorimetry information to fill_fmt
> - removed pm_runtime
> - added the g_input_status callback
> - dropped SECAM
> - dropped the non-standard PAL/NTSC variants
> 
> V4->V5: 
> - Added .querystd() and .g_tvnorms(), dropped the .open() and
>   unified the g_fmt() / s_fmt().
> 
> V3->V4: 
> - Fix a warning about an uninitialized ret variable in probe()
> 
> 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.
> 
> V1->V2: 
> - Set the media entity type to decoder, and implement the
>   s_std/g_std ops
> 
>  MAINTAINERS                |   6 +
>  drivers/media/i2c/Kconfig  |  12 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/tw9900.c | 771 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 790 insertions(+)
>  create mode 100644 drivers/media/i2c/tw9900.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3b47e0b56859..c2e69b642609 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21142,6 +21142,12 @@ L:	linux-media@xxxxxxxxxxxxxxx
>  S:	Maintained
>  F:	drivers/media/rc/ttusbir.c
>  
> +TECHWELL TW9900 VIDEO DECODER
> +M:	Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> +L:	linux-media@xxxxxxxxxxxxxxx
> +S:	Maintained
> +F:	drivers/media/i2c/tw9900.c
> +
>  TECHWELL TW9910 VIDEO DECODER
>  L:	linux-media@xxxxxxxxxxxxxxx
>  S:	Orphan
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 59ee0ca2c978..a9667428936e 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -1186,6 +1186,18 @@ 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_DEV && I2C

You should also depend on PM and probably on GPIOLIB too.

> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API

Also select V4L2_ASYNC.

> +	help
> +	  Support for the Techwell tw9900 multi-standard video decoder.

Uppercase "TW9900" here.

> +	  It supports NTSC, PAL standards with auto-detection features.
> +
> +	  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_DEV && I2C
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index f5010f80a21f..a17ee899a859 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -136,6 +136,7 @@ obj-$(CONFIG_VIDEO_TVP514X) += tvp514x.o
>  obj-$(CONFIG_VIDEO_TVP5150) += tvp5150.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..6aa585da0864
> --- /dev/null
> +++ b/drivers/media/i2c/tw9900.c
> @@ -0,0 +1,771 @@
> +// 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@xxxxxxxxxxx>
> + * Copyright (C) 2023 Mehdi Djait <mehdi.djait@xxxxxxxxxxx>
> + */
> +
> +#include <linux/bitfield.h>
> +#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>

Not sure timer and sysfs are needed here.

> +#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_VDLOSS	BIT(7)
> +#define TW9900_REG_CHIP_STATUS_HLOCK	BIT(6)
> +#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

This register covers both selection and detection so it would be better called
TW9900_REG_STD. The docs call it "SDT" but that's a really bad decision.

> +#define TW9900_REG_STD_SEL_AUTODETECT_IN_PROGRESS BIT(7)

And this one is a bit long without a good reason. You could call it:
TW9900_REG_STD_AUTO_PROGRESS

And please tab-align all the defines to the longest one (or don't tab-align
at all, but keep it consistent).

> +#define TW9900_STDNOW_MASK	GENMASK(6, 4)
> +#define TW9900_REG_STDR		0x1D
> +#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
> +
> +#define TW9900_STD_NTSC_M	0
> +#define TW9900_STD_PAL_BDGHI	1
> +#define TW9900_STD_AUTO		7
> +
> +#define TW9900_VIDEO_POLL_TRIES 20

Same comment about tab-alignment.

> +
> +struct regval {
> +	u8 addr;
> +	u8 val;
> +};
> +
> +struct tw9900_mode {
> +	u32 width;
> +	u32 height;
> +	u32 std;
> +	const struct regval *reg_list;
> +	int n_regs;
> +};
> +
> +struct tw9900 {
> +	struct i2c_client *client;
> +	struct gpio_desc *reset_gpio;
> +	struct regulator *regulator;
> +
> +	bool streaming;

This one should also be covered by the mutex. It's global state that needs
to be kept consistent across multiple users.

> +
> +	struct v4l2_subdev subdev;
> +	struct v4l2_ctrl_handler hdl;
> +	struct media_pad pad;
> +
> +	/* Serialize access to hardware and current mode configuration. */

You can mention "global state" instead of "current mode configuration".

> +	struct mutex mutex;
> +
> +	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,	0x00 },
> +	{ 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 = 480,
> +		.std = V4L2_STD_NTSC,
> +		.reg_list = tw9900_ntsc_regs,
> +		.n_regs = ARRAY_SIZE(tw9900_ntsc_regs),
> +	},
> +	{
> +		.width = 720,
> +		.height = 576,
> +		.std = V4L2_STD_PAL,
> +		.reg_list = tw9900_pal_regs,
> +		.n_regs = ARRAY_SIZE(tw9900_pal_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; i < n_regs; i++) {
> +		ret = tw9900_write_reg(client, regs[i].addr, regs[i].val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tw9900_read_reg(struct i2c_client *client, u8 reg)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(client, reg);
> +	if (ret < 0)
> +		dev_err(&client->dev, "read reg error: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +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 = V4L2_FIELD_NONE;
> +	fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> +	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(V4L2_COLORSPACE_SMPTE170M);
> +}
> +
> +static int tw9900_get_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> +
> +	mutex_lock(&tw9900->mutex);
> +	tw9900_fill_fmt(tw9900->cur_mode, mbus_fmt);
> +	mutex_unlock(&tw9900->mutex);
> +
> +	return 0;
> +}
> +
> +static int tw9900_set_fmt(struct v4l2_subdev *sd,
> +			  struct v4l2_subdev_state *sd_state,
> +			  struct v4l2_subdev_format *fmt)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +
> +	if (tw9900->streaming)
> +		return -EBUSY;

You need this check to be mutex-protected (so you should probably call
tw9900_fill_fmt locked directly here).

> +
> +	return tw9900_get_fmt(sd, sd_state, fmt);
> +}
> +
> +static int tw9900_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_state *sd_state,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index > 0)
> +		return -EINVAL;
> +
> +	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +
> +	return 0;
> +}
> +
> +static int tw9900_power_on(struct tw9900 *tw9900)

Move this to the resume rpm handler directly. No need to have a separate
function.

> +{
> +	int ret;
> +
> +	if (tw9900->reset_gpio)
> +		gpiod_set_value_cansleep(tw9900->reset_gpio, 1);
> +
> +	ret = regulator_enable(tw9900->regulator);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(50000, 52000);
> +
> +	if (tw9900->reset_gpio)
> +		gpiod_set_value_cansleep(tw9900->reset_gpio, 0);
> +
> +	usleep_range(1000, 2000);
> +
> +	mutex_lock(&tw9900->mutex);

The mutex should apply to every operation done in this function (i.e. lock and
unlock should be the first and last things done) since it deals with global
hardware state.

> +	ret = tw9900_write_array(tw9900->client, tw9900_init_regs,
> +				 ARRAY_SIZE(tw9900_init_regs));
> +	mutex_unlock(&tw9900->mutex);
> +
> +	/* This sleep is needed for the Horizontal Sync PLL to lock. */
> +	msleep(300);
> +
> +	return ret;
> +}
> +
> +static void tw9900_power_off(struct tw9900 *tw9900)
> +{

Same comment about folding into rpm suspend and mutex protection.

> +	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);
> +	int ret;
> +
> +	if (pm_runtime_suspended(&tw9900->client->dev))
> +		return 0;
> +
> +	/* v4l2_ctrl_lock() locks tw9900->mutex. */
> +	switch (ctrl->id) {
> +	case V4L2_CID_BRIGHTNESS:
> +		ret = tw9900_write_reg(tw9900->client, TW9900_REG_BRIGHT_CTL,
> +				       (u8)ctrl->val);
> +		break;
> +	case V4L2_CID_CONTRAST:
> +		ret = tw9900_write_reg(tw9900->client, TW9900_REG_CONTRAST_CTL,
> +				       (u8)ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int tw9900_s_stream(struct v4l2_subdev *sd, int on)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	struct i2c_client *client = tw9900->client;
> +	int ret;
> +
> +	if (tw9900->streaming == on)
> +		return 0;

You need this check to be mutex-protected.

> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(&client->dev);

You should use pm_runtime_resume_and_get instead...

> +		if (ret < 0) {
> +			pm_runtime_put_noidle(&client->dev);

... which will gracefull handle this too.

> +			return ret;
> +		}

> +
> +		mutex_lock(&tw9900->mutex);
> +
> +		ret = __v4l2_ctrl_handler_setup(sd->ctrl_handler);
> +		if (ret)
> +			goto err_unlock;
> +
> +		ret = tw9900_write_array(tw9900->client,
> +					 tw9900->cur_mode->reg_list,
> +					 tw9900->cur_mode->n_regs);
> +		if (ret)
> +			goto err_unlock;
> +
> +		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
> +				       TW9900_REG_OUT_FMT_CTL_STREAMING);
> +		if (ret)
> +			goto err_unlock;
> +
> +	} else {
> +		mutex_lock(&tw9900->mutex);
> +
> +		ret = tw9900_write_reg(client, TW9900_REG_OUT_FMT_CTL,
> +				       TW9900_REG_OUT_FMT_CTL_STANDBY);
> +		if (ret)
> +			goto err_unlock;
> +
> +		pm_runtime_put(&client->dev);

You need to call this outside of the lock since it might call suspend (and
deadlock).

> +	}
> +
> +	tw9900->streaming = on;
> +	mutex_unlock(&tw9900->mutex);
> +
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&tw9900->mutex);
> +	pm_runtime_put(&client->dev);
> +
> +	return ret;
> +}
> +
> +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 int tw9900_s_std(struct v4l2_subdev *sd, v4l2_std_id std)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	const struct tw9900_mode *mode;
> +	int i, ret = 0;
> +
> +	if (!(std & (V4L2_STD_NTSC | V4L2_STD_PAL)))
> +		return -EINVAL;
> +
> +	mutex_lock(&tw9900->mutex);
> +
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> +		if (supported_modes[i].std & std)
> +			mode = &supported_modes[i];
> +	if (!mode) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}

You can lock here, the list of supported modes is not going to change.

> +
> +	tw9900->cur_mode = mode;
> +
> +out_unlock:
> +	mutex_unlock(&tw9900->mutex);
> +
> +	return ret;
> +}
> +
> +static int tw9900_get_stream_std(struct tw9900 *tw9900,
> +				 v4l2_std_id *std)
> +{
> +	int cur_std, ret;
> +
> +	lockdep_assert_held(&tw9900->mutex);
> +
> +	ret = tw9900_read_reg(tw9900->client, TW9900_REG_STD_SEL);
> +	if (ret < 0) {
> +		*std = V4L2_STD_UNKNOWN;
> +		return ret;
> +	}
> +
> +	cur_std = FIELD_GET(TW9900_STDNOW_MASK, ret);
> +	switch (cur_std) {
> +	case TW9900_STD_NTSC_M:
> +		*std = V4L2_STD_NTSC;
> +		break;
> +	case TW9900_STD_PAL_BDGHI:
> +		*std = V4L2_STD_PAL;
> +		break;
> +	case TW9900_STD_AUTO:
> +		*std = V4L2_STD_UNKNOWN;
> +		break;
> +	default:
> +		*std = V4L2_STD_UNKNOWN;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tw9900_g_std(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +
> +	mutex_lock(&tw9900->mutex);
> +	*std = tw9900->cur_mode->std;
> +	mutex_unlock(&tw9900->mutex);
> +
> +	return 0;
> +}
> +
> +static int tw9900_start_autodetect(struct tw9900 *tw9900)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&tw9900->mutex);
> +
> +	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STDR,
> +			       BIT(TW9900_STD_NTSC_M) |
> +			       BIT(TW9900_STD_PAL_BDGHI));
> +	if (ret)
> +		return ret;
> +
> +	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STD_SEL,
> +			       TW9900_STD_AUTO);
> +	if (ret)
> +		return ret;
> +
> +	ret = tw9900_write_reg(tw9900->client, TW9900_REG_STDR,
> +			       BIT(TW9900_STD_NTSC_M) |
> +			       BIT(TW9900_STD_PAL_BDGHI) |
> +			       BIT(TW9900_STD_AUTO));
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Autodetect takes a while to start, and during the starting sequence
> +	 * the autodetection status is reported as done.
> +	 */
> +	msleep(30);
> +
> +	return 0;
> +}
> +
> +static int tw9900_detect_done(struct tw9900 *tw9900, bool *done)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&tw9900->mutex);
> +
> +	ret = tw9900_read_reg(tw9900->client, TW9900_REG_STD_SEL);
> +	if (ret < 0)
> +		return ret;
> +
> +	*done = !(ret & TW9900_REG_STD_SEL_AUTODETECT_IN_PROGRESS);
> +
> +	return 0;
> +}
> +
> +static int tw9900_querystd(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	bool done = false;
> +	int i, ret;
> +
> +	if (tw9900->streaming)
> +		return -EBUSY;

You need this to be locked.

> +
> +	ret = pm_runtime_get_sync(&tw9900->client->dev);

pm_runtime_resume_and_get here too.

> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&tw9900->client->dev);
> +		return ret;
> +	}
> +
> +	mutex_lock(&tw9900->mutex);
> +
> +	ret = tw9900_start_autodetect(tw9900);
> +	if (ret)
> +		goto out_unlock;
> +
> +	for (i = 0; i < TW9900_VIDEO_POLL_TRIES; i++) {
> +		ret = tw9900_detect_done(tw9900, &done);
> +		if (ret)
> +			goto out_unlock;
> +
> +		if (done)
> +			break;
> +
> +		msleep(20);
> +	}
> +
> +	if (!done) {
> +		ret = -ETIMEDOUT;
> +		goto out_unlock;
> +	}
> +
> +	ret = tw9900_get_stream_std(tw9900, std);
> +
> +out_unlock:
> +	mutex_unlock(&tw9900->mutex);
> +	pm_runtime_put(&tw9900->client->dev);
> +
> +	return ret;
> +}
> +
> +static int tw9900_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *std)
> +{
> +	*std = V4L2_STD_NTSC | V4L2_STD_PAL;
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops tw9900_pm_ops = {
> +	SET_RUNTIME_PM_OPS(tw9900_runtime_suspend,
> +			   tw9900_runtime_resume, NULL)

The macro is not useful as soon as you depend on PM, so you can just directly
register callbacks for runtime_suspend/runtime_resume here.

> +};

Also you should move the definition closer to the i2c_driver definition.

> +
> +static int tw9900_g_input_status(struct v4l2_subdev *sd, u32 *status)
> +{
> +	struct tw9900 *tw9900 = to_tw9900(sd);
> +	int ret;
> +
> +	*status = V4L2_IN_ST_NO_SIGNAL;
> +
> +	ret = pm_runtime_get_sync(&tw9900->client->dev);

pm_runtime_resume_and_get here too.

> +	if (ret < 0) {
> +		pm_runtime_put_noidle(&tw9900->client->dev);
> +		return ret;
> +	}
> +
> +	mutex_lock(&tw9900->mutex);
> +	ret = tw9900_read_reg(tw9900->client, TW9900_REG_CHIP_STATUS);
> +	mutex_unlock(&tw9900->mutex);
> +

You can put the call to pm_runtime_put unconditionally here: the return code
value is not going to change because we cut power ;)

> +	if (ret < 0) {
> +		pm_runtime_put(&tw9900->client->dev);
> +		return ret;
> +	}
> +
> +	*status = ret & TW9900_REG_CHIP_STATUS_HLOCK ? 0 : V4L2_IN_ST_NO_SIGNAL;
> +
> +	pm_runtime_put(&tw9900->client->dev);
> +
> +	return 0;
> +}
> +
> +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,
> +	.querystd	= tw9900_querystd,
> +	.g_tvnorms	= tw9900_g_tvnorms,
> +	.g_input_status = tw9900_g_input_status,
> +	.s_stream	= tw9900_s_stream,
> +};
> +
> +static const struct v4l2_subdev_pad_ops tw9900_pad_ops = {
> +	.enum_mbus_code		= tw9900_enum_mbus_code,

Looks like there's one not-needed tab for alignment here.

> +	.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 int tw9900_check_id(struct tw9900 *tw9900,
> +			   struct i2c_client *client)
> +{
> +	struct device *dev = &tw9900->client->dev;
> +	int ret;
> +
> +	mutex_lock(&tw9900->mutex);
> +	ret = tw9900_read_reg(client, TW9900_CHIP_ID);
> +	mutex_unlock(&tw9900->mutex);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret != TW9900_CHIP_ID) {
> +		dev_err(dev, "Unexpected decoder id %#x\n", ret);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tw9900_probe(struct i2c_client *client)
> +{
> +	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_optional(dev, "reset",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(tw9900->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(tw9900->reset_gpio),
> +				     "Failed to get reset gpio\n");
> +
> +	tw9900->regulator = devm_regulator_get(&tw9900->client->dev, "vdd");
> +	if (IS_ERR(tw9900->regulator))
> +		return dev_err_probe(dev, PTR_ERR(tw9900->regulator),
> +				     "Failed to get power regulator\n");
> +
> +	v4l2_i2c_subdev_init(&tw9900->subdev, client, &tw9900_subdev_ops);
> +	tw9900->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> +				V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +	mutex_init(&tw9900->mutex);
> +
> +	hdl = &tw9900->hdl;
> +
> +	ret = v4l2_ctrl_handler_init(hdl, 2);
> +	if (ret)
> +		return ret;
> +
> +	hdl->lock = &tw9900->mutex;
> +
> +	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) {
> +		ret = hdl->error;
> +		goto err_free_handler;
> +	}
> +
> +	ret = tw9900_power_on(tw9900);
> +	if (ret)
> +		goto err_free_handler;
> +
> +	ret = tw9900_check_id(tw9900, client);
> +	if (ret)
> +		goto err_power_off;

I would move this check after the rpm init so you can use the regular
pm_runtime_resume_and_get. Also don't forget to put because we still don't
want to keep the device powered at all times!

> +
> +	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;
> +	}

But we still want to keep this at the end (after checking the chip id).

> +
> +	pm_runtime_set_active(dev);

Don't set it active, set it to suspended instead with pm_runtime_set_suspended.

> +	pm_runtime_enable(dev);
> +	pm_runtime_idle(dev);

No need for this.

> +
> +	return 0;
> +
> +err_clean_entity:
> +	media_entity_cleanup(&tw9900->subdev.entity);
> +err_power_off:
> +	tw9900_power_off(tw9900);
> +err_free_handler:
> +	v4l2_ctrl_handler_free(hdl);
> +	mutex_destroy(&tw9900->mutex);
> +
> +	return ret;
> +}
> +
> +static void 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);
> +	v4l2_ctrl_handler_free(sd->ctrl_handler);
> +
> +	pm_runtime_disable(&client->dev);
> +	if (!pm_runtime_status_suspended(&client->dev))
> +		tw9900_power_off(tw9900);

If all calls are balanced you should not need this. You might still want to
call pm_runtime_suspend under this condition, but you need to do it before you
call pm_runtime_disable.

> +
> +	pm_runtime_set_suspended(&client->dev);

This is not useful, you can remove it.

> +	mutex_destroy(&tw9900->mutex);
> +}
> +
> +static const struct i2c_device_id tw9900_id[] = {
> +	{ "tw9900", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tw9900_id);
> +
> +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,
> +	.id_table = tw9900_id,
> +};
> +
> +module_i2c_driver(tw9900_i2c_driver);
> +
> +MODULE_DESCRIPTION("tw9900 decoder driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.41.0
> 

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Attachment: signature.asc
Description: PGP signature