From: linux@arm.linux.org.uk (Russell King - ARM Linux)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus.
Date: Fri, 20 Jan 2012 17:11:02 +0000 [thread overview]
Message-ID: <20120120171102.GA11845@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <20120118152149.GI1068@n2100.arm.linux.org.uk>
On Wed, Jan 18, 2012 at 03:21:49PM +0000, Russell King - ARM Linux wrote:
> On Sun, Nov 27, 2011 at 10:00:54PM +0100, Jochen Friedrich wrote:
> > + if (mid && mid->driver_data) {
> > + if (id != mid->driver_data) {
> > + printk(KERN_WARNING "%s wrong ID %04x found: %04x\n",
> > + mid->name, (unsigned int) mid->driver_data, id);
> > + goto err_disable;
> > + }
> > + } else {
> > + mid = &ucb1x00_id[1];
> > + while (mid->driver_data) {
> > + if (id == mid->driver_data)
> > + break;
> > + mid++;
> > + }
> > + printk(KERN_WARNING "%s ID not found: %04x\n",
> > + ucb1x00_id[0].name, id);
>
> Why do we need this complexity when we can detect the device from its
> ID in hardware? This, to me, is just code for the sake of code.
>
> I'm far from impressed by this patch, and had it not already gone in,
> I'd be NAKing it.
Right, I'm even more convinced that the above is over the top. What
it's doing is requiring either:
(a) the autodetect option to be given, in which case we will match
using the ID from the device
(b) use a specific option, in which case the ID must match
which is absolutely silly and completely unnecessary. If we require the
ID to be correct from the hardware, there's absolutely no point what so
ever passing some kind of identifier in to the driver. It just creates
an additional unnecessary reason for things to fail - in other words,
the only thing it adds is _fragility_.
And to prove the point that it's fragile, on the Assabet I get this:
ucb1x00 ID not found: 1005
and it fails to initialize - 0x1005 is a valid id for a UCB1300 device.
So clearly this code hasn't even been tested either.
Samuel, please revert this change. It's broken and does more than what
it says in the change log.
prev parent reply other threads:[~2012-01-20 17:11 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-11-27 21:00 [PATCH 1/2] ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus Jochen Friedrich
2011-11-27 21:00 ` [PATCH 2/2] ARM: sa1100: Refactor mcp-sa11x0 to use platform resources Jochen Friedrich
2012-01-14 9:21 ` Russell King - ARM Linux
2012-01-17 9:41 ` Jochen Friedrich
2012-01-17 20:12 ` Russell King - ARM Linux
2012-01-18 11:30 ` Jochen Friedrich
2012-01-18 11:48 ` Russell King - ARM Linux
2011-12-12 17:44 ` [PATCH 1/2] ARM: sa11x0: Implement autoloading of codec and codec pdata for mcp bus Samuel Ortiz
2012-01-18 15:21 ` Russell King - ARM Linux
2012-01-20 17:11 ` Russell King - ARM Linux [this message]
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=20120120171102.GA11845@n2100.arm.linux.org.uk \
--to=linux@arm.linux.org.uk \
--cc=linux-arm-kernel@lists.infradead.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).