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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 25 Mar 2019 13:17:47 +0200
From:   Sakari Ailus <sakari.ailus@...ux.intel.com>
To:     Mickael GUENE <mickael.guene@...com>
Cc:     Sakari Ailus <sakari.ailus@....fi>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Ben Kao <ben.kao@...el.com>,
        Todor Tomov <todor.tomov@...aro.org>,
        Rui Miguel Silva <rui.silva@...aro.org>,
        Akinobu Mita <akinobu.mita@...il.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Jason Chen <jasonx.z.chen@...el.com>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Tianshu Qiu <tian.shu.qiu@...el.com>,
        Bingbu Cao <bingbu.cao@...el.com>
Subject: Re: [PATCH v1 2/3] media:st-mipid02: MIPID02 CSI-2 to PARALLEL
 bridge driver

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@...ux.intel.com