Message-ID: <e4141652-53c0-fce1-dac7-5da5368e2240@ideasonboard.com>
Date:   Fri, 17 Feb 2023 08:57:32 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Andy Shevchenko <andriy.shevchenko@...el.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Shawn Tu <shawnx.tu@...el.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>,
        Satish Nagireddy <satish.nagireddy@...cruise.com>
Subject: Re: [PATCH v9 0/8] i2c-atr and FPDLink

Hi,

On 16/02/2023 17:53, Andy Shevchenko wrote:
> On Thu, Feb 16, 2023 at 04:07:39PM +0200, Tomi Valkeinen wrote:
> 
> ...
> 
>> +	if (!c2a)
> 
> I would expect here dev_warn() to let user know about "shouldn't happened, but
> have happened" situation.

Sure, I'll add.

>> +		return; /* This shouldn't happen */
> 
> ...
> 
>> -	static const struct v4l2_mbus_framefmt format = {
>> +	static const struct v4l2_mbus_framefmt informat = {
> 
> Naming a bit confusing. Is it "information" that cut or what?
> 
> in_format

Indeed, that's better.

>> +	static const struct v4l2_mbus_framefmt outformat = {
> 
> out_format
> 
> ...
> 
>> -out_unlock:
>> +out:
> 
> Why?

I think this was a mistake, I'll change it back.

> ...
> 
>> +/*
>> + * (Possible) TODOs
> 
> TODOs:

Ok...

>> + *
>> + * - PM for serializer and remote peripherals. We need to manage:
>> + *   - VPOC
>> + *     - Power domain? Regulator? Somehow any remote device should be able to
>> + *       cause the VPOC to be turned on.
>> + *   - Link between the deserializer and the serializer
>> + *     - Related to VPOC management. We probably always want to turn on the VPOC
>> + *       and then enable the link.
>> + *   - Serializer's services: i2c, gpios, power
>> + *     - The serializer needs to resume before the remote peripherals can
>> + *       e.g. use the i2c.
>> + *     - How to handle gpios? Reserving a gpio essentially keeps the provider
>> + *       (serializer) always powered on.
>> + * - Do we need a new bus for the FPD-Link? At the moment the serializers
>> + *   are children of the same i2c-adapter where the deserializer resides.
>> + * - i2c-atr could be made embeddable instead of allocatable.
>> + */
> 
> ...
> 
>>   struct atr_alias_table_entry {
>>   	u16 alias_id;	/* Alias ID from DT */
>>   
>> -	bool reserved;
>> +	bool in_use;
>>   	u8 nport;
>>   	u8 slave_id;	/* i2c client's local i2c address */
>>   	u8 port_reg_idx;
> 
> Wouldn't be wiser to move boolean at the end so if any obscure
> architecture/compiler makes it longer than a byte it won't increase the memory
> footprint. (Actually wouldn't it be aligned to u16 followed by u8 as well as
> they are different types?)

Sure, I can move it.

>>   };
> 
> ...
> 
>> +static int ub960_read16(struct ub960_data *priv, u8 reg, u16 *val)
>> +{
>> +	struct device *dev = &priv->client->dev;
>> +	unsigned int v1, v2;
>> +	int ret;
>> +
>> +	mutex_lock(&priv->reg_lock);
>> +
>> +	ret = regmap_read(priv->regmap, reg, &v1);
>> +	if (ret) {
>> +		dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n",
>> +			__func__, reg, ret);
>> +		goto out_unlock;
>> +	}
>> +
>> +	ret = regmap_read(priv->regmap, reg + 1, &v2);
>> +	if (ret) {
>> +		dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n",
>> +			__func__, reg + 1, ret);
>> +		goto out_unlock;
>> +	}
> 
> Wondering why bulk read can't be used against properly typed __be16 variable?

I'll do that.

>> +	*val = (v1 << 8) | v2;
> 
> + be16_to_cpu() here.

Yep.

>> +out_unlock:
>> +	mutex_unlock(&priv->reg_lock);
>> +
>> +	return ret;
>> +}
> 
> ...
> 
>> +static int ub960_rxport_read16(struct ub960_data *priv, u8 nport, u8 reg,
>> +			       u16 *val)
>>   {
> 
> Ditto.
> 
>> +}
> 
> ...
> 
>>   	struct i2c_board_info ser_info = {
>> -		.of_node = to_of_node(rxport->remote_fwnode),
>> -		.fwnode = rxport->remote_fwnode,
> 
>> +		.of_node = to_of_node(rxport->ser.fwnode),
>> +		.fwnode = rxport->ser.fwnode,
> 
> Why do you need to have both?!

I didn't debug it, but having only fwnode there will break the probing 
(no match).

>>   		.platform_data = ser_pdata,
>>   	};
> 
> ...
> 
>> +	for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) {
> 
> Pre-increment is non-standard in the kernel.
> 
>> +		struct ub960_rxport *rxport = priv->rxports[nport];
>> +		struct v4l2_mbus_frame_desc desc;
>> +		int ret;
>> +		u8 cur_vc;
>> +
>> +		if (!rxport)
>> +			continue;
>> +
>> +		ret = v4l2_subdev_call(rxport->source.sd, pad, get_frame_desc,
>> +				       rxport->source.pad, &desc);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>> +			continue;
> 
> 		cur_vc = desc.entry[0].bus.csi2.vc;
> 
>> +		for (i = 0; i < desc.num_entries; ++i) {
>> +			u8 vc = desc.entry[i].bus.csi2.vc;
> 
>> +			if (i == 0) {
>> +				cur_vc = vc;
>> +				continue;
>> +			}
> 
> This is an invariant to the loop, see above.

Well, the current code handles the case of num_entries == 0. I can 
change it as you suggest, and first check if num_entries == 0 and also 
start the loop from 1.

>> +			if (vc == cur_vc)
>> +				continue;
>> +
>> +			dev_err(&priv->client->dev,
>> +				"rx%u: source with multiple virtual-channels is not supported\n",
>> +				nport);
>> +			return -ENODEV;
>> +		}
>> +	}
> 
> ...
> 
>> +	for (i = 0; i < 6; ++i)
>>   		ub960_read(priv, UB960_SR_FPD3_RX_ID(i), &id[i]);
>>   	id[6] = 0;
> 
> Wondering if this magic can be defined.

The number of ID registers? Yes, I can add a define.

> ...
> 
>> +	priv->atr.aliases = devm_kcalloc(dev, table_size,
>> +					 sizeof(struct atr_alias_table_entry),
> 
> 	sizeof(*priv->atr.aliases) ?

Sure.

>> +					 GFP_KERNEL);
>> +	if (!priv->atr.aliases)
>>   		return -ENOMEM;
> 
> ...
> 
>>   	if (ret) {
>>   		if (ret != -EINVAL) {
>> -			dev_err(dev,
>> -				"rx%u: failed to read 'ti,strobe-pos': %d\n",
>> -				nport, ret);
>> +			dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
>> +				"ti,strobe-pos", ret);
>>   			return ret;
>>   		}
>>   	} else if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS ||
>> @@ -3512,8 +3403,8 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
>>   	ret = fwnode_property_read_u32(link_fwnode, "ti,eq-level", &eq_level);
>>   	if (ret) {
>>   		if (ret != -EINVAL) {
>> -			dev_err(dev, "rx%u: failed to read 'ti,eq-level': %d\n",
>> -				nport, ret);
>> +			dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
>> +				"ti,eq-level", ret);
>>   			return ret;
>>   		}
>>   	} else if (eq_level > UB960_MAX_EQ_LEVEL) {
> 

Hmm, I noticed this one (and the one above) was missing return -EINVAL.

> Seems like you may do (in both cases) similar to the above:
> 
> 	var = 0;
> 	ret = read_u32();
> 	if (ret && ret != -EINVAL) {
> 		// error handling
> 	}
> 	if (var > limit) {
> 		// another error handling
> 	}

That's not the same. You'd also need to do:

if (!ret) {
	// handle the retrieved value
}

which, I think, is not any clearer (perhaps more unclear).

What I could do is:

if (ret) {
	if (ret != -EINVAL) {
		dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
			"ti,eq-level", ret);
		return ret;
	}
} else {
	if (eq_level > UB960_MAX_EQ_LEVEL) {
		dev_err(dev, "rx%u: illegal 'ti,eq-level' value: %d\n",
			nport, eq_level);
		return -EINVAL;
	}

	rxport->eq.manual_eq = true;
	rxport->eq.manual.eq_level = eq_level;
}

Maybe the above style makes it clearer, as it clearly splits the "don't 
have value" and "have value" branches.

> ...
> 
>> +	static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { "vpoc0", "vpoc1",
>> +							       "vpoc2", "vpoc3" };
> 
> Wouldn't be better to format it as
> 
> 	static const char *vpoc_names[UB960_MAX_RX_NPORTS] = {
> 		"vpoc0", "vpoc1", "vpoc2", "vpoc3",
> 	};
> 
> ?

Clang-format disagrees, but I agree with you ;).

  Tomi

Powered by blists - more mailing lists