Re: [PATCH v1 2/3] media:st-mipid02: MIPID02 CSI-2 to PARALLEL bridge driver

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

Hi Mickael,

On Mon, Mar 18, 2019 at 09:57:44AM +0000, Mickael GUENE wrote:
> Hi Sakari,
> 
> Thanks for your review. Find my comments below.
> 
> On 3/16/19 23:14, Sakari Ailus wrote:
...
> >> +static struct v4l2_subdev *mipid02_find_sensor(struct mipid02_dev *bridge)
> >> +{
> >> +	struct media_device *mdev = bridge->sd.v4l2_dev->mdev;
> >> +	struct media_entity *entity;
> >> +
> >> +	if (!mdev)
> >> +		return NULL;
> >> +
> >> +	media_device_for_each_entity(entity, mdev)
> >> +		if (entity->function == MEDIA_ENT_F_CAM_SENSOR)
> >> +			return media_entity_to_v4l2_subdev(entity);
> > 
> > Hmm. Could you instead use the link state to determine which of the
> > receivers is active? You'll need one more pad, and then you'd had 1:1
> > mapping between ports and pads.
> > 
>  Goal here is not to detect which of the receivers is active but to find
> sensor in case there are others sub-dev in chain (for example a 
> serializer/deserializer as found in cars).

You shouldn't make assumptions on the rest of the pipeline beyond the
device that's directly connected. You might not even have a camera there.

>  For the moment the driver doesn't support second input port usage,
> this is why there is no such second sink pad yet in the driver.

Could you add the second sink pad now, so that the uAPI remains the same
when you add support for it? Nothing is connected to it but I don't think
it's an issue.

...

> >> +
> >> +	sensor = mipid02_find_sensor(bridge);
> >> +	if (!sensor)
> >> +		goto error;
> >> +
> >> +	dev_dbg(&client->dev, "use sensor '%s'", sensor->name);
> >> +	memset(&bridge->r, 0, sizeof(bridge->r));
> >> +	/* build registers content */
> >> +	code = mipid02_get_source_code(bridge, sensor);
> >> +	ret |= mipid02_configure_from_rx(bridge, code, sensor);
> >> +	ret |= mipid02_configure_from_tx(bridge);
> >> +	ret |= mipid02_configure_from_code(bridge, code);
> >> +
> >> +	/* write mipi registers */
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_CLK_LANE_REG1,
> >> +		bridge->r.clk_lane_reg1);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_CLK_LANE_REG3, CLK_MIPI_CSI);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_DATA_LANE0_REG1,
> >> +		bridge->r.data_lane0_reg1);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_DATA_LANE0_REG2,
> >> +		DATA_MIPI_CSI);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_DATA_LANE1_REG1,
> >> +		bridge->r.data_lane1_reg1);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_DATA_LANE1_REG2,
> >> +		DATA_MIPI_CSI);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_MODE_REG1,
> >> +		MODE_NO_BYPASS | bridge->r.mode_reg1);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_MODE_REG2,
> >> +		bridge->r.mode_reg2);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_DATA_ID_RREG,
> >> +		bridge->r.data_id_rreg);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_DATA_SELECTION_CTRL,
> >> +		SELECTION_MANUAL_DATA | SELECTION_MANUAL_WIDTH);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_PIX_WIDTH_CTRL,
> >> +		bridge->r.pix_width_ctrl);
> >> +	ret |= mipid02_write_reg(bridge, MIPID02_PIX_WIDTH_CTRL_EMB,
> >> +		bridge->r.pix_width_ctrl_emb);
> > 
> > Be careful with the error codes. ret will be returned by the s_stream
> > callback below.
> > 
>  I didn't understand your remark. Can you elaborate a little bit more ?

If the functions return different error codes, then ret possibly won't be a
valid error code, or at least it's not going to be what it was intended to
do. You'll need to stop when you encounter an error and then return it to
the caller.

...

> >> +static int mipid02_parse_tx_ep(struct mipid02_dev *bridge)
> >> +{
> >> +	struct i2c_client *client = bridge->i2c_client;
> >> +	struct v4l2_fwnode_endpoint ep;
> >> +	struct device_node *ep_node;
> >> +	int ret;
> >> +
> >> +	memset(&ep, 0, sizeof(ep));
> >> +	ep.bus_type = V4L2_MBUS_PARALLEL;
> > 
> > You can set the field in variable declaration, and omit memset. The same in
> > the function above.
> > 
> According to v4l2_fwnode_endpoint_parse() documentation:
>  * This function parses the V4L2 fwnode endpoint specific parameters from the
>  * firmware. The caller is responsible for assigning @vep.bus_type to a valid
>  * media bus type. The caller may also set the default configuration for the
>  * endpoint
> It seems safer to clear ep else it may select unwanted default configuration
> for the endpoint ?

By setting one of the fields in a struct in declaration, the rest will be
zeroed by the compiler. That's from the C standard.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com