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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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.

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

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

...

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

So it needs to be fixed then :-)
Glad that you agreed on this.

-- 
With Best Regards,
Andy Shevchenko