[PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[Sergio Paracuellos]

On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter <dan.carpenter at oracle.com> wrote:
>
> On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
> > Depending of chip revision reset lines are inverted. It is also
> > necessary to read PCIE_FTS_NUM register before enabling the phy.
> > Hence update the code to achieve this.
> >
> > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
> > function"
> > Reported-by: NeilBrown <neil at brown.name>
> >
> > Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
> > ---
> >  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
> > index ba81b34dc1b7..1b63706e129b 100644
> > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
> > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
> > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
> >       set_phy_for_ssc(port);
> >  }
> >
> > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
> > +{
> > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > +
> > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> > +             reset_control_assert(port->pcie_rst);
> > +     else
> > +             reset_control_deassert(port->pcie_rst);
> > +}
> > +
> > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> > +{
> > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
> > +
> > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
> > +             reset_control_deassert(port->pcie_rst);
> > +     else
> > +             reset_control_assert(port->pcie_rst);
> > +}
>
> The commit message is very good that on some chips assert and deassert
> mean the opposite but I feel like this should be commented in the code
> as well or people reading this code will be very confused.
>

Ok, Dan. Agreed. I will add some comment in next series.

> Also it would be better if we could change this from a white list to a
> black list.  In other words, if they were to come out with new revs
> of the hardware, we should assume that assert means assert and deassert
> means deassert.

I understand what you are saying but I don't know how to handle those
white and black lists... Is there some kind of example where I can
take a look to handle this in a proper way?

>
> regards,
> dan carpenter

Best regards,
    Sergio Paracuellos