linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Brian Norris <computersforpeace@gmail.com>
To: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Mark Brown <broonie@kernel.org>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	linux-mtd@lists.infradead.org,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org
Subject: Re: spi: OF module autoloading is still broken
Date: Mon, 16 Nov 2015 12:47:02 -0800	[thread overview]
Message-ID: <20151116204702.GP8456@google.com> (raw)
In-Reply-To: <564A35EB.5080008@osg.samsung.com>

Hi,

On Mon, Nov 16, 2015 at 05:00:43PM -0300, Javier Martinez Canillas wrote:
> On 11/16/2015 04:24 PM, Brian Norris wrote:

> I also didn't think about wilcards... I wonder why there are trailing
> wildcards for a compatible string. After all a compatible string should
> define a particular IP and there could be a foo,bar and foo,barbaz that
> have different drivers and what prevents today the driver with alias
> of:N*T*Cfoo,bar* to be loaded due a MODALIAS=of:NfooT<NULL>Cfoo,barbar ?
> 
> So I think we need this regardless of my patch:
> 
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 5b96206e9aab..cd0002f4a199 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -704,7 +704,6 @@ static int do_of_entry (const char *filename, void *symval, char *alias)
>  		if (isspace (*tmp))
>  			*tmp = '_';
>  
> -	add_wildcard(alias);
>  	return 1;
>  }
>  ADD_TO_DEVTABLE("of", of_device_id, do_of_entry);

(I'm also not an expert in this stuff, but...) That looks reasonable.
You might refer to commit ac551828993e ("modpost: i2c aliases need no
trailing wildcard") for inspiration. You might also modify the "always"
in:

/* Always end in a wildcard, for future extension */
static inline void add_wildcard(char *str)
{
	...
}

And of course, you probably should CC those who are responsible for the
core device tree probing and device/driver interactions for something
like this.

> Now that I think about it, there is another issue and is that today spi:foo
> defines a namespace while changing to of: will make the namespace flat so
> a platform driver that has the same vendor and model will have the same
> modalias.
> 
> IOW, for board files will be platform:bar and i2c:bar while for OF will be
> of:NfooT<NULL>Cfoo,bar in both cases. I wonder if we should reuse the type
> for that and store the subsystem prefix there. What do you think?

I'm not sure I understand all the issues here to be able to comment
properly. But I bet someone else might.

(For me, it might help if you had a more concrete example to speak of.)

> Thanks a lot for pointing out all these issues. It is indeed harder than
> I thought.

No problem!

> > I don't think you have problems only with bad FDTs. I think you have a
> > problem with perfectly valid DTs.
> >
> 
> Agreed, I wonder if spi_uevent() shouldn't be changed to report both the
> OF and old SPI modaliases to avoid breaking a lot of drivers but at the
> same time allowing DT-only drivers to not need an SPI ID table.

That's the solution I imagined, though I haven't tested it yet. I don't
see much precedent for reporting multiple modaliases, so there could be
some kind of ABI issues around that too.

Regards,
Brian

  reply	other threads:[~2015-11-16 20:47 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <56104E88.3040807@gmail.com>
2015-11-12 18:59 ` m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading Brian Norris
2015-11-13 19:40   ` spi: OF module autoloading is still broken (was: Re: m25p80: Commit "allow arbitrary OF matching for "jedec,spi-nor"" breaks module autoloading) Brian Norris
2015-11-13 22:12     ` Mark Brown
2015-11-13 22:51       ` Brian Norris
2015-11-13 23:14         ` Mark Brown
2015-11-13 23:48           ` Brian Norris
2015-11-16 13:53             ` Mark Brown
2015-11-16 17:26               ` spi: OF module autoloading is still broken Javier Martinez Canillas
2015-11-16 17:51                 ` Mark Brown
2015-11-16 18:00                   ` Javier Martinez Canillas
2015-11-16 17:19             ` Javier Martinez Canillas
2015-11-16 17:49               ` Mark Brown
2015-11-16 17:57                 ` Javier Martinez Canillas
2015-11-16 19:24               ` Brian Norris
2015-11-16 20:00                 ` Javier Martinez Canillas
2015-11-16 20:47                   ` Brian Norris [this message]
2015-11-16 21:32                     ` Javier Martinez Canillas
2015-11-16 21:51                       ` Brian Norris
2015-11-17 13:14                         ` Javier Martinez Canillas
2015-11-17 13:19                           ` Mark Brown
2015-11-17 13:36                             ` Javier Martinez Canillas
2015-11-18 20:07                       ` Javier Martinez Canillas
2015-11-19 12:47                         ` Javier Martinez Canillas
2015-11-17 13:34                   ` Mark Brown
2015-11-17 13:38                     ` Javier Martinez Canillas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20151116204702.GP8456@google.com \
    --to=computersforpeace@gmail.com \
    --cc=broonie@kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=javier@osg.samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-spi@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).