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.
> Maybe a custom FPD-Link bus could do that, but that's
> then going into totally new directions.
--
With Best Regards,
Andy Shevchenko
