Re: [PATCH] media: i2c: IMX296 camera sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Mon, Nov 04, 2019 at 03:33:51PM +0200, Laurent Pinchart wrote:

...

> > > +static int imx296_read(struct imx296 *imx, u32 addr)
> > > +{
> > > +	u8 data[3] = { 0, 0, 0 };
> > > +	int ret;
> > > +
> > > +	ret = regmap_raw_read(imx->regmap, addr & IMX296_REG_ADDR_MASK, data,
> > > +			      (addr >> IMX296_REG_SIZE_SHIFT) & 3);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return (data[2] << 16) | (data[1] << 8) | data[0];
> > 
> > Hmm. Don't you need to cast this to a wider type to avoid limiting this to
> > an 8-bit number?
> 
> The C99 standard states in section 6.5.7 "Bitwise shift operators"
> 
> "The integer promotions are performed on each of the operands. The type
> of the result is that of the promoted left operand."
> 
> Section 6.3.1.1 "Boolean, characters, and integers" then states
> 
> "If an int can represent all values of the original type, the value is
> converted to an int; otherwise, it is converted to an unsigned int.
> These are called the integer promotions. All other types are unchanged
> by the integer promotions."
> 
> No cast is thus needed.

Ack. Thanks for the explanation.

...

> > > +	/*
> > > +	 * HMAX and VMAX configure horizontal and vertical blanking by
> > > +	 * specifying the total line time and frame time respectively. The line
> > > +	 * time is specified in operational clock units (which appears to be the
> > > +	 * output of an internal PLL, fixed at 74.25 MHz regardless of the
> > > +	 * exernal clock frequency), while the frame time is specified as a
> > > +	 * number of lines.
> > > +	 *
> > > +	 * In the vertical direction the sensor outputs the following:
> > > +	 *
> > > +	 * - one line for the FS packet
> > > +	 * - two lines of embedded data (DT 0x12)
> > > +	 * - six null lines (DT 0x10)
> > > +	 * - four lines of vertical effective optical black (DT 0x37)
> > > +	 * - 8 to 1088 lines of active image data (RAW10, DT 0x2b)
> > > +	 * - one line for the FE packet
> > > +	 * - 16 or more lines of vertical blanking
> > > +	 */
> > > +	imx296_write(imx, IMX296_HMAX, 1100, &ret);
> > > +	imx296_write(imx, IMX296_VMAX, format->height + imx->vblank->cur.val,
> > > +		     &ret);
> > > +
> > > +	for (i = 0; i < 4; ++i)
> > 
> > It'd be nice to have e.g. NR_IMX296_INCKSEL or something instead of 4.
> 
> How about ARRAY_SIZE(imx->clk_params->incksel) ?

Yes, please.

...

> > > +static int imx296_init_cfg(struct v4l2_subdev *sd,
> > > +			   struct v4l2_subdev_pad_config *cfg)
> > > +{
> > > +	struct v4l2_subdev_selection sel = { 0 };
> > > +	struct v4l2_subdev_format format = { 0 };
> > > +
> > > +	sel.target = V4L2_SEL_TGT_CROP;
> > > +	sel.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > > +	sel.r.width = IMX296_PIXEL_ARRAY_WIDTH;
> > > +	sel.r.height = IMX296_PIXEL_ARRAY_HEIGHT;
> > 
> > It'd be nicer to set these in variable declaration. Same below.
> 
> It's partly a matter of style, but as this is a new driver and you
> maintain sensor drivers, I'll switch to your preferred style here.

It could be argued whether this is a (plain) style issue or not. Thanks in
any case.

...

> > > +			const struct i2c_device_id *did)
> > > +{
> > > +	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> > > +	unsigned long clk_rate;
> > > +	struct imx296 *imx;
> > > +	unsigned int i;
> > > +	int ret;
> > > +
> > > +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> > > +		dev_warn(&adapter->dev,
> > > +			 "I2C-Adapter doesn't support I2C_FUNC_SMBUS_BYTE\n");
> > > +		return -EIO;
> > > +	}
> > > +
> > > +	imx = devm_kzalloc(&client->dev, sizeof(*imx), GFP_KERNEL);
> > > +	if (!imx)
> > > +		return -ENOMEM;
> > > +
> > > +	imx->dev = &client->dev;
> > 
> > You can use &v4l2_get_subdevdata(sd)->dev to get the device, so no need for
> > the dev field.
> 
> I could, but only after registering the subdev, and thus not in
> imx296_identify_model() for instance. I could pass the device pointer to
> that function, but I'd prefer accessing dev through a single method
> everywhere in the driver to make it less error-prone.

Fair enough.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com