On Mon, Sep 20, 2021 at 08:37:23PM +0100, Russell King (Oracle) wrote:
> On Mon, Sep 20, 2021 at 07:33:27PM +0100, Mark Brown wrote:
> > On Mon, Sep 20, 2021 at 11:20:29AM +0100, Russell King (Oracle) wrote:
> > Are you sure? Looking at the binding document it appears that the
> > fallback to jedec,spi-nor is mandatory in all cases - it's either one of
> > the two items: cases both of which are lists with jedec,spi-nor in them
> > or just the plain jedec,spi-nor fallback. It kind of doesn't matter
> > given that we weren't enforcing it in the past but still.
> We aren't even enforcing it today either - running the DT checker is
> entirely optional, and it's not even carried by distros, so across
> distro upgrades it breaks. I'd also suggest that almost no one bothers
> to run it either, looking at the almost 6700 lines of output it
> produces for my build - the chances of spotting anything relevant in
> that are practically zero.
Right, good - my read of the DT binding document was correct at
least. Like we're both saying it doesn't really matter what was
documented, what we were accepting is what matters.
> because, although the driver accepts devices of type "w25q32", it
> isn't listed in the DT schema. However, the driver code itself
> accepts that "w25q32" is used in DT - so the schema is out of step
> with the driver and has been for ages.
Looks like it's trying to list it but not quite managing.
Another thing to fix...
> > This then causes issues for anything trying to bind based with DT
> > aliases AIUI so it's just pushing the problems around to different
> > devices. I think ideally we should be including the fallback compat IDs
> > that could also be matched along with the OF aliases.
> I have a different view. These patches have fixed one problem by
> creating another problem - they have _changed_ the module alias
> that SPI creates for DT.
> Originally, the module alias was created via of_modalias_node()
> in of_register_spi_device():
> /* Select device driver */
> rc = of_modalias_node(nc, spi->modalias,
> sizeof(spi->modalias));
> if (rc < 0) {
> dev_err(&ctlr->dev, "cannot find modalias for %pOF\n", nc);
> goto err_out;
> }
> However, as a result of the above two commits, the modalias that
> is now given to userspace has changed from whatever
> of_modalias_node() produced to whatever of_device_modalias() and
> of_device_uevent_modalias() produces - which is something quite
> different.
> So, IMHO the change in these two patches was _wrong_ and always
> was wrong, and was always going to lead to this problem. Randomly
> deciding to have a different modalias policy is always going to
> lead to problems like this.
Hrm, right - I hadn't really registered that we were generating
compat modaliases in quite that way (and hadn't had the bandwidth
to dig into that properly today, should do tomorrow). First pass
I'd think that either SPI or probably of_device_uevent_modalias()
ought to be generating both formats (assuming we can list
multiple modaliases which I'm not sure on) but like I say I've
really not dug into this properly yet at this point.
I'm reasonably sure we should be continuing to generate the old
style modalises like a revert would, my main questions are if we
can arrange to provide both types so that anything that won't
match on the compat type can also work, and if there's any
fallout that needs fixing up if we can't and end up doing the
revert.
> > That doesn't seem particularly hard TBH, and if we're going to be
> > listing any compatibles we really ought to be including them all rather
> > than just a random one.
>
> Looking at the shere number of combinations given the regexp in
> the DT binding document, I think someone would need to script its
> generation - expanding just the first set leads to:
...
> And then you have a similar set for n25q*. Once all of that regexp
> has been expanded, one then needs to add the list for "backward
> compatibility" with all the different manufacturer prefixes that
> of_modalias_node() stripped off.
> Are you really sure this is a solution you wish to require?
It's true that the manufacturer prefixes blow stuff up for this
particular driver especially badly without wildcards which gets
messy... the interaction between generic parts like flash and
the DT aliases definitely isn't at all nice at the minute, the
compat stuff is doing a good job of sidestepping some of the
explosion in compatibles.
Attachment:
signature.asc
Description: PGP signature
