Re: [PATCH v4 3/4] media: i2c: Add MAX9286 driver

[Kieran Bingham]

Hi Luca,

Thank you for your review,

On 13/11/2018 14:49, Luca Ceresoli wrote:
> Hi Kieran, All,
> 
> below a few minor questions, and a big one at the bottom.
> 
> On 02/11/18 16:47, Kieran Bingham wrote:
>> From: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>>
>> The MAX9286 is a 4-channel GMSL deserializer with coax or STP input and
>> CSI-2 output. The device supports multicamera streaming applications,
>> and features the ability to synchronise the attached cameras.
>>
>> CSI-2 output can be configured with 1 to 4 lanes, and a control channel
>> is supported over I2C, which implements an I2C mux to facilitate
>> communications with connected cameras across the reverse control
>> channel.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>> Signed-off-by: Niklas SÃderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> 
> [...]
> 
>> +struct max9286_device {
>> +	struct i2c_client *client;
>> +	struct v4l2_subdev sd;
>> +	struct media_pad pads[MAX9286_N_PADS];
>> +	struct regulator *regulator;
>> +	bool poc_enabled;
>> +	int streaming;
>> +
>> +	struct i2c_mux_core *mux;
>> +	unsigned int mux_channel;
>> +
>> +	struct v4l2_ctrl_handler ctrls;
>> +
>> +	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> 
> 5 pads, 4 formats. Why does the source node have no fmt?

The source pad is a CSI2 link - so a 'frame format' would be inappropriate.


>> +static int max9286_init(struct device *dev, void *data)
>> +{
>> +	struct max9286_device *max9286;
>> +	struct i2c_client *client;
>> +	struct device_node *ep;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	/* Skip non-max9286 devices. */
>> +	if (!dev->of_node || !of_match_node(max9286_dt_ids, dev->of_node))
>> +		return 0;
>> +
>> +	client = to_i2c_client(dev);
>> +	max9286 = i2c_get_clientdata(client);
>> +
>> +	/* Enable the bus power. */
>> +	ret = regulator_enable(max9286->regulator);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "Unable to turn PoC on\n");
>> +		return ret;
>> +	}
>> +
>> +	max9286->poc_enabled = true;
>> +
>> +	ret = max9286_setup(max9286);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to setup max9286\n");
>> +		goto err_regulator;
>> +	}
>> +
>> +	v4l2_i2c_subdev_init(&max9286->sd, client, &max9286_subdev_ops);
>> +	max9286->sd.internal_ops = &max9286_subdev_internal_ops;
>> +	max9286->sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>                           ^
> 
> This way you're clearing the V4L2_SUBDEV_FL_IS_I2C set by
> v4l2_i2c_subdev_init(), even though using devicetree I think this won't
> matter in the current kernel code. However I think "max9286->sd.flags |=
> ..." is more correct here, and it's also what most other drivers do.

A quick glance looks like you're right.
That looks like a good catch!

I've updated locally ready for v5.

>> +	v4l2_ctrl_handler_init(&max9286->ctrls, 1);
>> +	/*
>> +	 * FIXME: Compute the real pixel rate. The 50 MP/s value comes from the
>> +	 * hardcoded frequency in the BSP CSI-2 receiver driver.
>> +	 */
>> +	v4l2_ctrl_new_std(&max9286->ctrls, NULL, V4L2_CID_PIXEL_RATE,
>> +			  50000000, 50000000, 1, 50000000);
>> +	max9286->sd.ctrl_handler = &max9286->ctrls;
>> +	ret = max9286->ctrls.error;
>> +	if (ret)
>> +		goto err_regulator;
>> +
>> +	max9286->sd.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> 
> According to the docs MEDIA_ENT_F_VID_IF_BRIDGE appears more fitting.

Yes, I agree. We recently updated the adv748x to this too.

Also updated locally to add to v5.


>> +static int max9286_probe(struct i2c_client *client,
>> +			 const struct i2c_device_id *did)
>> +{
>> +	struct max9286_device *dev;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>> +	if (!dev)
>> +		return -ENOMEM;
>> +
>> +	dev->client = client;
>> +	i2c_set_clientdata(client, dev);
>> +
>> +	for (i = 0; i < MAX9286_N_SINKS; i++)
>> +		max9286_init_format(&dev->fmt[i]);
>> +
>> +	ret = max9286_parse_dt(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dev->regulator = regulator_get(&client->dev, "poc");
>> +	if (IS_ERR(dev->regulator)) {
>> +		if (PTR_ERR(dev->regulator) != -EPROBE_DEFER)
>> +			dev_err(&client->dev,
>> +				"Unable to get PoC regulator (%ld)\n",
>> +				PTR_ERR(dev->regulator));
>> +		ret = PTR_ERR(dev->regulator);
>> +		goto err_free;
>> +	}
>> +
>> +	/*
>> +	 * We can have multiple MAX9286 instances on the same physical I2C
>> +	 * bus, and I2C children behind ports of separate MAX9286 instances
>> +	 * having the same I2C address. As the MAX9286 starts by default with
>> +	 * all ports enabled, we need to disable all ports on all MAX9286
>> +	 * instances before proceeding to further initialize the devices and
>> +	 * instantiate children.
>> +	 *
>> +	 * Start by just disabling all channels on the current device. Then,
>> +	 * if all other MAX9286 on the parent bus have been probed, proceed
>> +	 * to initialize them all, including the current one.
>> +	 */
>> +	max9286_i2c_mux_close(dev);
>> +
>> +	/*
>> +	 * The MAX9286 initialises with auto-acknowledge enabled by default.
>> +	 * This means that if multiple MAX9286 devices are connected to an I2C
>> +	 * bus, another MAX9286 could ack I2C transfers meant for a device on
>> +	 * the other side of the GMSL links for this MAX9286 (such as a
>> +	 * MAX9271). To prevent that disable auto-acknowledge early on; it
>> +	 * will be enabled later as needed.
>> +	 */
>> +	max9286_configure_i2c(dev, false);
>> +
>> +	ret = device_for_each_child(client->dev.parent, &client->dev,
>> +				    max9286_is_bound);
>> +	if (ret)
>> +		return 0;
>> +
>> +	dev_dbg(&client->dev,
>> +		"All max9286 probed: start initialization sequence\n");
>> +	ret = device_for_each_child(client->dev.parent, NULL,
>> +				    max9286_init);
> 
> I can't manage to like this initialization sequence, sorry. If at all
> possible, each max9286 should initialize itself independently from each
> other, like any normal driver.

Yes, I think we're in agreement here, but unfortunately this section is
a workaround for the fact that our devices share a common address space.

We (currently) *must* disable both devices before we start the
initialisation process for either on our platform currently...

That said - I think this section needs to be removed from the upstream
part at least for now. I think we should probably carry this
'workaround' separately.

This part is the core issue that I talked about in my presentation at
ALS-Japan [0]

 [0] https://sched.co/EaXa

> First, it requires that each chip on the remote side can configure its
> own slave address. Not all chips do.
> 
> Second, using a static i2c address map does not scale well and limits
> hotplugging, as I discussed in my reply to patch 1/4. The problem should
> be solvable cleanly if the MAX9286 supports address translation like the
> TI chips.

I don't think we can treat GMSL as hot-pluggable currently ... But as we
discussed - I see that we should think about this for FPD-Link

Also as a further aside here, we use "device_is_bound" which is not
exported, and means that this driver won't compile successfully as a
module currently (thanks to the kbuild test robot for highlighting that)


> Thanks,
> 

-- 
Regards
--
Kieran