Hi,
>Who reviewed the ethernet driver?
>Who reviewed the IDE driver?
>Has Bart seen this new driver?
>Why was this committed without first being run by the subsystem
maintainers?
I am very happy if people outside Axis can find time to review patches
that only affects the CRIS architecture (note that all files are under
arch/cris and there is no way you can get these drivers unless you run
CRIS). I have assumed that people like you and Bart have too much on
your hands to do this. The few times I have tried to patch CRIS stuff on
LKML the response is usally none.
>In ethernet, the MII phy probe is wrong (don't need at id 0 first),
Why is that? Phy address == 0 is the most common value for CRIS based
hardwares.
>it should be using linux/mii.h bits rather than inventing its own,
Yes.
>del_timer versus del_timer_sync is questionable,
del_timer_sync is defined to del_timer for !SMP and there is no SMP
support in CRISv10.
>the newly-added full duplex handling seems to be incorrect
>(ref drivers/net/mii.c and drivers that use mii_if->full_duplex),
Great! I can definitely use the code in mii.c. Thanks for the pointer.
>and cosmetic issues I won't bother to mention.
Yes, I agree that there are lots of legacy non-Linux formatting. I'll fix
that up.
>In IDE, it uses virt_to_page() when building the scatter/gather list --
>something that IMO should not have been allowed in -mm much less
>mainline -- and other yuckiness.
This code is copy-pasted from ide-dma.c (ide_raw_build_sglist).
Why is it ok there and not in our driver?
>In the same function, it's questionable whether or not it breaks
>with lba48.
Could be. The 2.6 driver is a direct port of the 2.4 driver and
that works with lba48 but the 2.6 driver has not been verified
to work with it.
>etrax_dma_intr doesn't appear to check for some DMA engine
>conditions that code comments elsewhere in the driver indicate
>_do_ occur.
etrax_dma_intr checks the result of ide_dma_end but e100_dma_end
always returns 0 and there is a comment there that this could
be improved. I can add some checks of the DMA status there to
make sure.
>The ATAPI-specific e100_start_dma code doesn't look like it will
>work with all current ATAPI drivers (ide-{cdrom,floppy,tape,scsi,...}.c).
In e100_start_dma there are a if (!atapi) in a few places to fix
LBA48 but there is no ATAPI specific code. The driver has only
been tested with a disk and a CD so I agree that it may fail for
some devices.
>I'm happy that the CRIS people resurfaced, too, but that's no reason to
>just shove an unreviewed patch into mainline.
We are continously working on the CRIS port. Recently we have put most
of our work on support for a new CRIS sub-architecture in 2.6. 2.6 is
not really used in any CRIS-based products yet. While 2.5 was moving
fast I was reluctant to submit patches because most people don't care
if CRIS is changing things back and forth to follow the bleeding edge.
Next time I'll send the patches to the subsystem maintainers and LKML.
I would be very happy if someone could find some time to review our
patches.
At least the following patches are expected:
Fixes for the things you've pointed out and similar stuff
USB driver
New subarchitechture (later this year)
Thanks!
/Mikael (CRIS Maintainer)
On Fri, Jun 04, 2004 at 07:46:21AM +0200, Mikael Starvik wrote:
> Hi,
>
> >Who reviewed the ethernet driver?
> >Who reviewed the IDE driver?
> >Has Bart seen this new driver?
> >Why was this committed without first being run by the subsystem
> maintainers?
>
> I am very happy if people outside Axis can find time to review patches
> that only affects the CRIS architecture (note that all files are under
> arch/cris and there is no way you can get these drivers unless you run
> CRIS). I have assumed that people like you and Bart have too much on
> your hands to do this. The few times I have tried to patch CRIS stuff on
> LKML the response is usally none.
The general rule is to locate drivers under drivers/, even the arch
specific ones. This allows for easier grepping after users of a
given API etc.
s390 has their own directory.
arm has their own directory under driver/net/ etc.
With respect to patches, a very good approach is to follow same style
as is done for s390.
Smaller logical splitted patches, being sent out after each kernel release.
This allows LKML readers to do peer review of changes to the IDE driver,
without having to step over a lot of unrelated code.
General rule is to make easy for the reader.
Sam
On Friday 04 of June 2004 07:46, Mikael Starvik wrote:
> Hi,
Hi,
> >Who reviewed the ethernet driver?
> >Who reviewed the IDE driver?
> >Has Bart seen this new driver?
> >Why was this committed without first being run by the subsystem
>
> maintainers?
>
> I am very happy if people outside Axis can find time to review patches
> that only affects the CRIS architecture (note that all files are under
> arch/cris and there is no way you can get these drivers unless you run
> CRIS). I have assumed that people like you and Bart have too much on
> your hands to do this. The few times I have tried to patch CRIS stuff on
> LKML the response is usally none.
Feel free to spam me and [email protected] on IDE related stuff.
arch specific IDE drivers should be in drivers/ide/<arch>/
> >and cosmetic issues I won't bother to mention.
>
> Yes, I agree that there are lots of legacy non-Linux formatting. I'll fix
> that up.
>
> >In IDE, it uses virt_to_page() when building the scatter/gather list --
> >something that IMO should not have been allowed in -mm much less
> >mainline -- and other yuckiness.
>
> This code is copy-pasted from ide-dma.c (ide_raw_build_sglist).
> Why is it ok there and not in our driver?
It is OK.
> >In the same function, it's questionable whether or not it breaks
> >with lba48.
>
> Could be. The 2.6 driver is a direct port of the 2.4 driver and
> that works with lba48 but the 2.6 driver has not been verified
> to work with it.
AFAICS it should work with lba48.
> >etrax_dma_intr doesn't appear to check for some DMA engine
> >conditions that code comments elsewhere in the driver indicate
> >_do_ occur.
>
> etrax_dma_intr checks the result of ide_dma_end but e100_dma_end
> always returns 0 and there is a comment there that this could
> be improved. I can add some checks of the DMA status there to
> make sure.
>
> >The ATAPI-specific e100_start_dma code doesn't look like it will
> >work with all current ATAPI drivers (ide-{cdrom,floppy,tape,scsi,...}.c).
>
> In e100_start_dma there are a if (!atapi) in a few places to fix
> LBA48 but there is no ATAPI specific code. The driver has only
> been tested with a disk and a CD so I agree that it may fail for
> some devices.
It will probably fail on devices needing write access because
e100_dma_begin() always passes e100_read_command as
argument to e100_start_dma().
> >I'm happy that the CRIS people resurfaced, too, but that's no reason to
> >just shove an unreviewed patch into mainline.
>
> We are continously working on the CRIS port. Recently we have put most
> of our work on support for a new CRIS sub-architecture in 2.6. 2.6 is
> not really used in any CRIS-based products yet. While 2.5 was moving
> fast I was reluctant to submit patches because most people don't care
> if CRIS is changing things back and forth to follow the bleeding edge.
>
> Next time I'll send the patches to the subsystem maintainers and LKML.
> I would be very happy if someone could find some time to review our
> patches.
Good!
> At least the following patches are expected:
> Fixes for the things you've pointed out and similar stuff
> USB driver
> New subarchitechture (later this year)
>
> Thanks!
> /Mikael (CRIS Maintainer)
Thanks,
Bartlomiej