Re: [patch/rfc 2.6.20-git] parport reports physical devices

On Monday 19 February 2007 6:18 am, Jean Delvare wrote:
Hi David,

On Sun, 18 Feb 2007 21:08:07 -0800, David Brownell wrote:
Currently a parport_driver can't get a handle on the device node for the
underlying parport (PNPACPI, PCI, etc). That prevents correct placement
of sysfs child nodes, which can affect things like power management.

This patch resolves that issue for non-legacy configurations:

* "struct parport" now has a field pointing to that device node,
and non-legacy port drivers now initialize that device pointer:
- parport_mfc3 (can't test or build; no Amiga + Zorro here)
- parport_pc (and stop using only pci_device internally)

Only in the PCI and PNP cases. Super-I/O and legacy cases still don't
have a valid device pointer to pass. This annoys me because the laptop
I'm using for my daily work has such a legacy parallel port,

And SuperIO precludes PNP support? I guess that's one of the Mysteries.

Well, as I said, "non-legacy" configs. One must start somewhere, and
I have no (working) legacy hardware to even try!


- parport_serial
- parport_sunbpp (can't test or build, no SPARC + SBUS here)

* pnp now initializes device dma masks (24bits), preventing oopses
when generic dma calls are made using pnp device nodes

The patch is quite large and I fail to see the relation between the dma
mask bug and the original purpose of the patch.

Preventing that oopsing, while letting me have a single patch to work with.


Can you possibly split
the dma fixes to a separate patch? So we can get this part in the
kernel faster.

Certainly could; that's one of the comments I expected.

But someone who knows PNP better than I do should confirm that part is
correct; I'm not sure a 24bit mask is correct for *all* PNP devices.
(And while it shouldn't hurt for devices that don't support DMA, it might
still be better not to set DMA masks for such devices...)


* some of the layered parport_driver code now uses that pointer:
- i2c-parport (parent of i2c_adapter)
- spi_butterfly (parent of spi_master, allowing cruft removal)

Yes, the cleanups in the spi_butterfly driver are quite nice :) They
didn't apply cleanly on my tree though, I guess you have some local
changes.

Yes, removal of class_device and friends from the SPI stack. That's a
case where a "struct class" adds no value.


- lp (creating class_device)
- ppdev (parent of parportN device)
- tipar (creating class_device)

Sanity tested on a PC, where PNPACPI provides the device to parport_pc,
using spi_butterfly. But I've got to wonder about parport DMA...

Tested on my other machine with has a PNP parallel port too, and it
worked fine. Great :)

Good! Did you happen to try parport printing, with DMA?


@@ -2180,9 +2181,10 @@ struct parport *parport_pc_probe_port (u
priv->fifo_depth = 0;
priv->dma_buf = NULL;
priv->dma_handle = 0;
- priv->dev = dev;
INIT_LIST_HEAD(&priv->list);
priv->port = p;
+
+ p->dev = dev;

This might be the right place to add some warning if dev == NULL, so
that the remaining drivers can be spotted and ported over time? Or even
lower in the stack, in parport_announce_port()?

Lower would be better, if there's going to be such a warning; the issue
isn't specific to parport_pc. This version doesn't add such a warning.


--- g26.orig/drivers/i2c/busses/i2c-parport.c 2006-12-12 19:25:43.000000000 -0800
+++ g26/drivers/i2c/busses/i2c-parport.c 2007-02-18 09:13:34.000000000 -0800
@@ -143,7 +143,7 @@ static struct i2c_algo_bit_data parport_

/* ----- I2c and parallel port call-back functions and structures --------- */

-static struct i2c_adapter parport_adapter = {
+static const struct i2c_adapter parport_adapter = {
.owner = THIS_MODULE,
.class = I2C_CLASS_HWMON,
.id = I2C_HW_B_LP,

This change doesn't belong to this patch at all, although it is
correct. I'll be happy to apply it if you send it to me separately.

(Or it might be even better to get rid of this static structure
altogether and intialize the fields of the dynamically allocated
structure individually instead. This would make the driver smaller.)

Smaller is better. ;)


@@ -175,6 +175,7 @@ static void i2c_parport_attach (struct p
adapter->algo_data.getscl = NULL;
adapter->algo_data.data = port;
adapter->adapter.algo_data = &adapter->algo_data;
+ adapter->adapter.dev.parent = port->physport->dev;

if (parport_claim_or_block(adapter->pdev) < 0) {
printk(KERN_ERR "i2c-parport: Could not claim parallel port\n");

Maybe we should even fail if the physical device is missing, as you did
in spi_butterfly?

I didn't want to make that call for the I2C stack, certainly not as
part of this patch! The goal here wasn't "fix everything"; rather
to start the fixing, by providing the API and making the most common
cases behave.


As you know, some i2c subsystem changes are not
possible until all i2c-adapter drivers have a valid physical parent
device. I don't want to wait that all parport drivers have been
updating before we can clean up i2c-core itself.

Which means that I will have to fix the legacy parport_pc case right
now, otherwise I will no longer be able to use i2c-parport and this
isn't an option for me.

I admire your enthusiasm. :)


What do you think would be the right way to do
it? A platform driver I guess, and we create a platform device for
every successful call to parport_pc_probe_port() with a NULL dev
pointer? That would be a fake driver, as the probe() and remove()
methods would do nothing as far as I can see, but that's all I can
think of at the moment.

Yes, a platform_driver ... and ideally, moving all that hardware probing
and scanning code into a separate file. Probe/scan steps shouldn't really
be part of *any* driver.

There are probably good reasons (== deep hardware braindamage on older
systems that are now hard to find) for the strange init sequencing in
that code, but I can't see why they should prevent splitting out

(a) device discovery via probing, PNP, or whatever; from

(b) the driver itself, getting handed a device node that's
pre-configured with the resources reported by discovery.

Maybe the maintainers of the parport stack will have comments. Though
the info in MAINTAINERS seems dated, if not obsolete.

- Dave

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/