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

[Mehdi Djait]

Hi Paul,

thank you for the review!

On Thu, Nov 02, 2023 at 11:03:42AM +0100, Paul Kocialkowski wrote:
> > +static int tw9900_write_reg(struct i2c_client *client, u8 reg, u8 val)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, reg, val);
> 
> Is this an SMBUS device in particular? Or is there any reason to use the SMBUS
> API instead of the general I2C API?
> 

I think I will keep using the SMBUS API here. The reason is in the
kernel documentation:

--------------------------------------------------------------------------------
If you write a driver for some I2C device, please try to use the SMBus commands 
if at all possible (if the device uses only that subset of the I2C protocol). 
This makes it possible to use the device driver on both SMBus adapters and I2C 
adapters (the SMBus command set is automatically translated to I2C on I2C 
adapters, but plain I2C commands can not be handled at all on most pure SMBus 
adapters).
--------------------------------------------------------------------------------

And the vast majority of the drivers under /media/i2c are using the
SMBUS API.

> > +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_cfg_fmt(struct v4l2_subdev *sd,
> 
> You might have to differentiate between set_fmt/get_fmt to return -EBUSY
> if streaming is on in set_fmt. However I understand it will just copy the
> current mode in both cases, but this might still be required to follow v4l2
> semantics (please double-check).
> 

This should be done in the driver calling the pad subdev_call set_fmt,
right ?

--
Kind Regards
Mehdi Djait