Re: [PATCH v7 5/7] media: i2c: add DS90UB960 driver

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

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