[PATCH] staging: mt7621-pci: update driver's TODO file

[Sergio Paracuellos]

On Mon, Feb 11, 2019 at 3:52 AM NeilBrown <neil at brown.name> wrote:
>
> On Sun, Feb 10 2019, Sergio Paracuellos wrote:
>
> > Some of the things included in driver's TODO file have
> > been properly achieved. Update file accordly.
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
> > ---
> > Hi Neil,
> >
> > I've been looking through the code of this driver and pci-phy
> > driver while thinking in what is missing already to get this
> > mainlined out of staging. I don't really know what cleanups
> > are missing here. The only thinkg is not clear yet is the thing
> > with the clocks defined in device-tree. Should be just remove
> > them and give this stuff a try to get feedback or what is missing?
> > Can you please point me out in the right direction?
>
> Hi Sergio,
>  thanks for persisting with this.

Hi Neil,

>
> I think the "right" thing to do with the clocks is to write a driver
> that support ralink,rt2880-clock, similar to the way
>  arch/mips/ralink/reset.c
> supports ralink,rt2880-reset

I see. I also do think this is the correct thing to do. So let's do
it. A minor doubt here,
Should I add this driver to staging in order to be checked and tested
or should it be just directly
send to ralink and mips people to try it to be mainlined directly?

>
> It would support clk_enable by setting the relevant bit in
> #define RALINK_CLKCFG1                  0x30
>
> and clk_disable by clearing it.
> The pci-mt7621.c (and pci-mt7620.c) could use this to enable/disable
> the clocks, rather than poking directly at the register.

This stuff has been moved to the phy part of the pci so we should move
also the port clocks to there in the device tree and remove them from the
pci port bindingsand handle clk_* options there.

>
> Also, pci-mt7621.c (maybe) shouldn't be poking RALINK_PCIE_RST directly.
> This is apparently another reset line the driver needs, so it should be
> described in devicetree. i.e add another reset-name "pcie".

True, I will add a new reset line to the device tree to achieve this.

>
> Also,
> #define RALINK_PCI_IO_MAP_BASE          0x1e160000
>
> duplicates info that is in devicetree:
>                 ranges = <
>                         0x02000000 0 0x00000000 0x60000000 0 0x10000000 /* pci memory */
>                         0x01000000 0 0x00000000 0x1e160000 0 0x00010000 /* io space */
>                                                 ^^HERE^^^^
>                 >;
>
> I wonder if that matter...

I knew this, the thing is that this is only used to set the register
IOBASE which is the base Address for IO Space window. I am not sure
if just writing this after request resources from device tree will
work. I'll add this change as the last patch of the series just to see
what
happen.

>
> A tiny thing:
>
>          * 3'b100                  0            x               x
>          * 3'b101                  1            x               0
>          * 3'b110                  1            0               x
>          * 3'b111                  2            1               0
>
> There are two sets of 8 spaces in there, that should be a TAB, and there
> is a space before a TAB, that should be removed (yes, I do have x-ray vision).

I will change this also. Awesome vision, BTW :-)

>
>
> I really don't know how important all this is. Maybe it would be best
> to post the patch and see what people say.

Let's change all of these and post then the PATCHes to get last feedback.

>
> Thanks,
> NeilBrown

Thanks a lot.

Best regards,
    Sergio Paracuellos
>
> >
> > Thanks in advance for your time.
> >
> > Best regards,
> >     Sergio Paracuellos
> >  drivers/staging/mt7621-pci/TODO | 8 --------
> >  1 file changed, 8 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci/TODO b/drivers/staging/mt7621-pci/TODO
> > index cf30f629b9fd..ccfd266db4ca 100644
> > --- a/drivers/staging/mt7621-pci/TODO
> > +++ b/drivers/staging/mt7621-pci/TODO
> > @@ -1,12 +1,4 @@
> >
> >  - general code review and cleanup
> > -- can this be converted to not require PCI_DRIVERS_LEGACY ??
> > -    The irq returned by pcibios_map_irq is a "hwirq" (hardware irq number)
> > -    and pci_assign_irq() assigns this directly to dev->irq, which
> > -    expects a "virq" (virtual irq number).  These numbers are different
> > -    on MIPS.  There is a gross hack to make it work on one
> > -    specific platform, so it can be tested.
> > -- Should this be merged with arch/mips/pci/pci-mt7620.c ??
> > -- ensure device-tree requirements are documented
> >
> >  Cc:  NeilBrown <neil at brown.name>
> > --
> > 2.19.1