| Message ID | cover.1696608809.git.mehdi.djait@bootlin.com |
|---|---|
| Headers | show |
| Series | media: i2c: Introduce driver for the TW9900 video decoder | expand |
On 06/10/2023 18:25, Mehdi Djait wrote: > The Techwell video decoder supports PAL, NTSC 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. > > The TW9900 is capable of doing automatic standard detection, this is > implemented with support for PAL and NTSC autodetection. > > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com> ... > + > +static int tw9900_check_id(struct tw9900 *tw9900, > + struct i2c_client *client) > +{ > + struct device *dev = &tw9900->client->dev; > + int ret; > + > + ret = tw9900_read_reg(client, TW9900_CHIP_ID); > + if (ret < 0) > + return ret; > + > + if (ret != TW9900_CHIP_ID) { > + dev_err(dev, "Unexpected decoder id(0x%x)\n", ret); > + return -EINVAL; > + } > + > + dev_info(dev, "Detected TW9900 (0x%x) decoder\n", TW9900_CHIP_ID); dev_dbg Do not spam log with simple success messages. Why do you always print 0x0 (TW9900_CHIP_ID) here? It does not make sense, drop. > + > + 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(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 dev_err_probe() Best regards, Krzysztof
On Fri, Oct 06, 2023 at 06:25:27PM +0200, Mehdi Djait wrote: > Hello everyone, > > This series is based on the fifth iteration of the series introducing the > tw9900 driver: sent 29 Dec 2020 [1] > > This is the version 6 of the series adding support for the Techwell > TW9900 multi standard decoder. It's a pretty simple decoder compared to > the TW9910, since it doesn't have a built-in scaler/crop engine. > > Changes 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 It's not very nice to keep the chip powered up all the time :-( > - added the g_input_status callback > - dropped SECAM > - dropped the non-standard PAL/NTSC variants > > Any feedback is appreciated, > > Mehdi Djait > > media_tree, base-commit: 2c1bae27df787c9535e48cc27bbd11c3c3e0a235 > > [1] https://lore.kernel.org/linux-media/20210401070802.1685823-1-maxime.chevallier@bootlin.com/ > > Mehdi Djait (3): > dt-bindings: vendor-prefixes: Add techwell vendor prefix > media: dt-bindings: media: i2c: Add bindings for TW9900 > media: i2c: Introduce a driver for the Techwell TW9900 decoder > > .../bindings/media/i2c/techwell,tw9900.yaml | 61 ++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > MAINTAINERS | 6 + > drivers/media/i2c/Kconfig | 12 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/tw9900.c | 651 ++++++++++++++++++ > 6 files changed, 733 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/techwell,tw9900.yaml > create mode 100644 drivers/media/i2c/tw9900.c
Hi Laurent, On Mon, Oct 09, 2023 at 05:21:22AM +0300, Laurent Pinchart wrote: > On Fri, Oct 06, 2023 at 06:25:27PM +0200, Mehdi Djait wrote: > > Hello everyone, > > > > This series is based on the fifth iteration of the series introducing the > > tw9900 driver: sent 29 Dec 2020 [1] > > > > This is the version 6 of the series adding support for the Techwell > > TW9900 multi standard decoder. It's a pretty simple decoder compared to > > the TW9910, since it doesn't have a built-in scaler/crop engine. > > > > Changes 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 > > It's not very nice to keep the chip powered up all the time :-( > I agree 100% I tried to make it work with pm_runtime but I faced many issues. I don't know if this is due to my lack of experience but here is the situation when I enable pm_runtime: I get most of the time wrong values when calling g_input_status to check if I have a signal or not. To do that I read the 0x01 – Chip Status Register I (STATUS1) and check the BIT(6): HLOCK: - 1 = Horizontal sync PLL is locked to the incoming video source. - 0 = Horizontal sync PLL is not locked. To make the g_input_status work with pm_runtime I had to add a 300 msleep after power ON! Which is a huge delay. I also face issues with the standard detection... So I decided to drop it for this first version of the driver. -- Kind Regards Mehdi Djait