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

[Tomi Valkeinen]

On 25/01/2023 16:49, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 03:33:35PM +0200, Tomi Valkeinen wrote:
> > On 25/01/2023 14:10, Andy Shevchenko wrote:
> > > On Wed, Jan 25, 2023 at 01:15:34PM +0200, Tomi Valkeinen wrote:
> > > > On 20/01/2023 18:47, Andy Shevchenko wrote:
>
> ...
>
> > > > > > > > +	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?
> >
> > Well, probably nothing will explode if we just ignore them. But... Why would
> > we 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.
> >
> > Perhaps, but I don't have a default value here.
>
> It's impossible. You have one. 0 is also can be default.

No, I either have the value ("eq-level"), or it's undefined, not used, 
doesn't exist. There's no default eq-level.

> > In any case, I'm not quite sure what you are arguing here. Is it just that
> > you don't think the error check is necessary and should be dropped?
>
> Yes, I do not see the value of these complex error checking.
> Dropping that makes it KISS. I.o.w. why do we care about errors
> if the property is optional? Make it mandatory otherwise.

If the call fails, there's an error somewhere. Maybe the user tried to 
define eq-level, but something is wrong. Isn't it better to catch that 
error, rather than ignoring it, leaving the user wonder why things don't 
work as he expects?

> > > > > > > > +	} 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.
> >
> > The atr pointer is needed so that the serializers (ub913, ub953) can add
> > their i2c adapter to the deserializer's i2c-atr. The port is also needed for
> > that.
> >
> > The bc rate (back-channel rate) is the FPD-Link back-channel rate which the
> > serializers use for various functionalities. At the moment only the ub953
> > uses it for calculating an output clock rate.
> >
> > The bc-rate could be implemented using the clock framework, even if it's not
> > quite a plain clock. I had that code at some point, but it felt a bit off
> > and as we needed the pdata for the ATR, I added the bc-rate there.
>
> And I don't see why it is not a property of the device.

It with a "property of the device" you mean a hardware property, it's 
not because we don't know it, it can be changed at runtime. It's not 
supposed to change after probing the serializer, but up to that point it 
can change.

> > > > 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.
> >
> > Do you have any pointers to guide me into the right direction? I couldn't
> > find anything with some grepping and googling.
> >
> > If you mean "device properties" as in ACPI, and so similar to DT properties,
> > aren't those hardware properties? Only the port here is about the hardware.
>
> About hardware, or PCB, or as quirks for missing DT/ACPI/any FW properties,
> like clock rates.
>
> The Linux kernel layer for that is called software nodes. The rough
> approximation to see where and how it's being used can be achieved
> by grepping for specific macros:
>
> 	git grep -lw PROPERTY_ENTRY_.*
>
> E.g. arch/arm/mach-tegra/board-paz00.c tegra_paz00_wifikill_init()
> implementation.

Thanks, I'll have a look. But I presume we can only pass "plain" values, 
so it won't work for the ATR pointer anyway.

 Tomi