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

[Andy Shevchenko]

On Fri, Feb 17, 2023 at 02:57:02PM +0200, Tomi Valkeinen wrote:
> On 17/02/2023 13:24, Andy Shevchenko wrote:
> > 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.
> 
> Having both defined or only of_node defined works for me.

But of_node is _legacy_ stuff. We should not really consider this option in the
new code.

> Perhaps the issue is that these drivers only add of_match_table, and thus
> having only .fwnode above is not enough.

No, the code should work with fwnode that carrying DT node or another.
The matching table shouldn't affect this either.

> Looking at i2c_device_match(), i2c_of_match_device() only uses of_node, so
> perhaps I would need CONFIG_ACPI for acpi_driver_match_device to do matching
> with of_node? Although I don't see the acpi code using fwnode, just of_node.
> Well, I have to say I have no idea without spending more time on this.

Again, there is a bug and that bug seems nasty one as it would allow to
work the device in one environment and not in another.

Since it's about I²C board files, I believe that an issue is in I²C core.

> > > > >    		.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.
> 
> But your suggestion accesses desc.entry[0] even if there are no entries,
> accessing possibly uninitialized memory. In that case it doesn't use it for
> anything, but at least I find that kind of code worrying.

Yes you probably will need a 0 case to be handled separately. I was and
is not objecting this.

> > > > > +			if (vc == cur_vc)
> > > > > +				continue;
> > > > > +
> > > > > +			dev_err(&priv->client->dev,
> > > > > +				"rx%u: source with multiple virtual-channels is not supported\n",
> > > > > +				nport);
> > > > > +			return -ENODEV;
> > > > > +		}

...

> > 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.
> 
> Yes, if you have a default. I don't.

It can't be true. If you have an optional property you always have a default
even if you are not using it (let's call it special case).

	foo_present = property_present();
	property_read(&foo_val);

	...

	if (foo_present) {
		// do something with foo_val
	}

The boolean variable is needed when the range of the foo_val takes all possible
values of the type (u32?). Otherwise you always can define a magic that will
tell you "okay, this is not in use". Of course having boolean always is also
fine.

> I could add a new magic number for the
> eq_level which means not-defined and use it as a default, but I don't
> usually like default values which are not 0. Here I have the manual_eq
> boolean to tell if we're using manual EQ or not.

Oh, this is similar that I described above.

But as I said, you can keep your initial version, it's up to you and
maintainers to cope with that (uglification).

-- 
With Best Regards,
Andy Shevchenko