Re: [PATCH v2] i3c: dw-i3c-master: Fix IBI count register selection for versalnet

[Jeremy Kerr]

Hi Shubhrajyoti,

> > How are you binding the driver to this device? Are you using a unique OF
> > compatible string, or something ACPI-based?
> > 
> > ... and if that can be specific to this hardware instance, would that be an
> > effective mechanism to select the IBI read method instead?
> Hi Jeremy,
> 
> VersalNet currently uses the generic "snps,dw-i3c-master-1.00a"
> compatible string — there is no unique compatible string for this
> hardware instance. The DTS    entry looks like:
> 
>   compatible = "snps,dw-i3c-master-1.00a";

You should *always* have a unique compatible string for each device
instance, if there can be any variation in behaviours from that generic
one (which you certainly do have here).

You can still fall-back on a generic one, but using that as your only
compatible value means you can't do device-specific behaviours in your
driver, as you have just found.

>   We could introduce a VersalNet-specific compatible with a generic fallback:
> 
>   compatible = "xlnx,versalnet-dw-i3c-master", "snps,dw-i3c-master-1.00a";

Yes, you want that anyway.

>   and pass a quirk flag via of_device_id.data to select the IBI read method.

Exactly, and there is already the struct dw_i3c_drvdata to help with
this.

> However, the probe-time detection avoids having to enumerate all affected
> variants — the IC_HAS_IBI_DATA=0 configuration is a synthesizable
> option in the IP and may appear in other SoCs using the same core.
> 
> Do you have a preference? If the DTS change is acceptable, I can go
> with the compatible + match-data approach

That would certainly be my recommendation.

If it's a synthesisable option, it may even be worth adding as a flag to
the binding definition (along with the same for other options that might
be present). This would mean you don't need a-priori knowledge of the
mapping of compatible strings to their synthesis options.

I don't have any access to documentation on those options though, so
can't be of much help with doing that.

> However I thought that detecting it will be helpful for backward compatible.

Given it doesn't work at the moment, there shouldn't be any backwards
compat concerns, I think?

Cheers,


Jeremy