Re: [PATCH v9 0/8] i2c-atr and FPDLink

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 17/02/2023 13:24, Andy Shevchenko wrote:
On Fri, Feb 17, 2023 at 08:57:32AM +0200, Tomi Valkeinen wrote:
On 16/02/2023 17:53, Andy Shevchenko wrote:
On Thu, Feb 16, 2023 at 04:07:39PM +0200, Tomi Valkeinen wrote:

...

   	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).

This needs to be investigated. The whole fwnode approach, when we have both
fwnode and legacy of_node fields in the same data structure, is that fwnode
_OR_ of_node initialization is enough, when both are defined the fwnode
should take precedence.

If your testing is correct (and I have no doubts) it means we have a serious
bug lurking somewhere.

Having both defined or only of_node defined works for me.

Perhaps the issue is that these drivers only add of_match_table, and thus having only .fwnode above is not enough.

Looking at i2c_device_match(), i2c_of_match_device() only uses of_node, so perhaps I would need CONFIG_ACPI for acpi_driver_match_device to do matching with of_node? Although I don't see the acpi code using fwnode, just of_node. Well, I have to say I have no idea without spending more time on this.

   		.platform_data = ser_pdata,
   	};

...

		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.

You may try to compile both variants and see which one gets lets code.
I believe it will be mine or they are equivalent in case compiler is clever
enough to recognize the invariant.

But your suggestion accesses desc.entry[0] even if there are no entries, accessing possibly uninitialized memory. In that case it doesn't use it for anything, but at least I find that kind of code worrying.

+			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.

Yes.

...

...

   	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.

Up to you, but this just a good example why I do not like how optional
properties are handled in a "smart" way.

To me

	foo = DEFAULT;
	_property_read_(&foo); // no error checking

is clean, neat, small and good enough solution.

Yes, if you have a default. I don't. I could add a new magic number for the eq_level which means not-defined and use it as a default, but I don't usually like default values which are not 0. Here I have the manual_eq boolean to tell if we're using manual EQ or not.

 Tomi




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux