Re: Interface change: Now or later?

From: Mitchell Blank Jr (
Date: Wed Aug 02 2000 - 02:42:57 EST

(answering out of order for the kernel folks who aren't interested in the
details of the PHY interface)

> > (I guess I'm
> > old fashioned, but I actually care about code freezes)
> If I'd believed in Code freeze, I'd have been stuck since last august.
> That's a year now!

Well, a year ago Werner probably would have let it in. Now, I'm saying
no. Development is great fun, but Linus has said point blank that we
should only being sending him bug fixes. Calling this interface change
a bug fix would be, IMO, a bit of a reach. I'm just trying to hold to
the party line here.

> > * Don't implement the PHY as a seperate driver - just combine it with
> > your SAR driver. The entire point of having them seperate is that
> > the same PHY will often be used by several different SAR chips.
> > Since yours would currently be the only SAR supporting the 16-bit
> > interface, there is little to gain by having the drivers seperate.
> I've done this. I'm into the "cleanup" stage now, so I'd rather do it
> right, once and for all.

Well, we're just going to have to do it twice and for all :-) Seriously,
just put all the PHY code together and book-end it with a nice big block
comment saying "*** This is a driver for the AtmCo model ABC-1234 PHY ***"
Make it communicate with your driver with _{read,write}16 methods. Then
it will be a no-brainer to break the code back out into a seperate driver
in 2.5.

If it's any consulation, a driver I am working on is also hitting the
limitations of the current SAR/PHY interface, and I'm having to employ
some gross hacks to get around this. So I feel your pain.

Those not interested in the gritty details of the SAR/PHY interface can
stop reading now.

> > * The PHYs should be able to propagate link-up/link-down state for
> > physical layers that have a strong connection-oriented basis (DSL)
> My SAR, the FS50, is especially for DSL modems. I Have no idea how a
> physical layer down event would propagate from the PHY side to the
> FS50. I cold poll a PHY register maybe. Hmm. OK. that'd be part of
> the PHY driver, right?

Yep. Right now it doesn't matter too much since the ATM layer doesn't have
good support for these sorts of PHYs that have a strong "up/down" state.
I'm working around this currently in my SAR driver by calling special
"->itf_{up,down}" entries which I've tacked on the end of a private
version of phy_ops. The interface is "uped" when the first vcc is brought
up on the interface, and is "downed" with the last vc to leave. The
problem then is what do we do if the DSL connection is lost unexpectadly.
Ideally there should be some way for userspace to be notified (probably by
making the socket act similarly to a dead TCP socket) so pppd (or whatever)
can try to reestablish the connection.

> > * DSL PHYs can support variable link rates which the SAR may need to
> > change timing for.
> Right: We need separate up and downlink speeds, right?


> > struct phy_event {
> > enum phy_event_type type;
> Isn't this just like folding
> ..._phy...
> all into one?

Yes, which sounds bad, but I think it might actually be cleaner in the
long run. First, it makes it less of a hassle to add events in the
future. Second, I want to make it so simple SARs don't have to
implement the whole set of commands - instead these 'phy events' will be
dispatched through the main atm code. So a simple PCI card could have
something like:

        enum phy_event_result mysar_phy_event_handler(
                struct atm_dev *atmdev,
                struct phy_event *event)
                switch (event->type) {
                        case phyevent_read8:
                                return phy_event_result_ok;
                        case phyevent_write8:
                                return phy_event_result_ok;
                                return phy_event_result_unknown_type;

and then the core ATM code would have something like:

        enum phy_event_result do_phy_event(
                struct atm_dev *atmdev,
                struct phy_event *event)
                enum phy_event_result r;
                r = atmdev->ops->phy_event_handler(atmdev, event);
                if (r != phy_event_result_unknown_type)
                        return r;
                switch (r->type) {
                        case phyevent_read8:
                                event->type = phyevent_read16;
                                r = atmdev->ops->phy_event_handler(atmdev, event);
                                if (r != phy_event_result_unknown_type)
                                /* FALLTHROUGH */
                        case phyevent_read16:
                                event->type = phyevent_read32;
                                r = atmdev->ops->phy_event_handler(atmdev, event);
                        case phyevent_write8:
                                event->type = phyevent_write16;
                                r = atmdev->ops->phy_event_handler(atmdev, event);
                                if (r != phy_event_result_unknown_type)
                                /* FALLTHROUGH */
                        case phyevent_write16:
                                event->type = phyevent_write32;
                                r = atmdev->ops->phy_event_handler(atmdev, event);
                        case phyevent_readN:
                        case phyevent_writeN: {
                                int i;
                                struct phy_event e;
                                e.type = (event->type == phyevent_readN) ?
                                    phyevent_read8 : phyevent_write8;
                                for (i = 0; i < event->nbytes ; i++) {
                                        e.addr = event->addr + i;
                                        e.val = event->data[i];
                                        r = do_phy_event(atmdev, &e);
                                        if (r != phy_event_result_ok)
                return r;

That way a PHY designed to work for either PCI or USB SARs could use
writeN for things like firmware uploads and it would work regardless.

Like I said, I'm still mulling over the choices for a new interface.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
Please read the FAQ at

This archive was generated by hypermail 2b29 : Mon Aug 07 2000 - 21:00:07 EST