On Wed, Jan 25, 2023 at 01:15:34PM +0200, Tomi Valkeinen wrote:
On 20/01/2023 18:47, Andy Shevchenko wrote:
...
Esp. taking into account that some of them are using actually
post-inc. Why this difference?
Possibly a different person has written that particular piece of code, or
maybe a copy paste from somewhere.
I'm personally fine with seeing both post and pre increments in code.
I'm not :-), if it's not required by the code. Pre-increment always puzzles
me: Is here anything I have to pay an additional attention to?
That is interesting, as to me pre-increment is the simpler, more obvious
case. It's just:
v = v + 1
v
Whereas post-increment is:
temp = v
v = v + 1
temp
In any case, we're side-tracking here, I think =).
Yes, just see the statistics of use below.
...
+ for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) {
Post-inc?
I still like pre-inc =).
I see there's a mix os post and pre incs in the code. I'll align those when
I encounter them, but I don't think it's worth the effort to methodically go
through all of them to change them use the same style.
Kernel uses post-inc is an idiom for loops:
$ git grep -n -w '[_a-z0-9]\+++' | wc -l
148693
$ git grep -n -w ' ++[a-z0-9_]\+' | wc -l
8701
So, non-standard pattern needs to be explained.
+ }
...
+ 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);
+ return ret;
+ }
This seems like trying to handle special cases, if you want it to be optional,
why not ignoring all errors?
I don't follow. Why would we ignore all errors even if the property is
optional? If there's a failure in reading the property, or checking if it
exists or not, surely that's an actual error to be handled, not to be
ignored?
What the problem to ignore them?
But if you are really pedantic about it, perhaps the proper way is to add
fwnode_property_*_optional()
APIs to the set where you take default and return 0 in case default had been
used for the absent property.
+ } else if (eq_level > UB960_MAX_EQ_LEVEL) {
+ dev_err(dev, "rx%u: illegal 'ti,eq-level' value: %d\n", nport,
+ eq_level);
This part is a validation of DT again, but we discussed above this.
+ } else {
+ rxport->eq.manual_eq = true;
+ rxport->eq.manual.eq_level = eq_level;
+ }
...
+struct ds90ub9xx_platform_data {
+ u32 port;
+ struct i2c_atr *atr;
+ unsigned long bc_rate;
Not sure why we need this to be public except, probably, atr...
The port and atr are used by the serializers, for atr. The bc_rate is used
by the serializers to figure out the clocking (they may use the FPD-Link's
frequency internally).
The plain numbers can be passed as device properties. That's why the question
about platform data. Platform data in general is discouraged to be used in a
new code.
Device properties, as in, coming from DT?
From anywhere.
The port could be in the DT, but
the others are not hardware properties.
Why do we need them? For example, bc_rate.
Yes, I don't like using platform data. We need some way to pass information
between the drivers.
Device properties allow that and targeting to remove the legacy platform data
in zillions of the drivers.