Re: [PATCH v3] misc: rtsx: add power saving function and bios guide options

[Bjorn Helgaas <helgaas@kernel.org>]

> > > From: Ricky Wu <ricky_wu@realtek.com>
> > >
> > > Added rts5227 rts5249 rts5260 rts5228 power saving functions,
> > > added BIOS guide MMC funciton and U_d3_en register support and
> > > fixed rts5260 driving parameter
> > 
> > This should be split into small logical pieces.  I can't really tell
> > what those would be, but just based on the commit message, it could
> > be:
> > 
> >   1) Add rts5227 rts5249 rts5260 rts5228 power saving functions
> > 
> >   2) Add BIOS guide MMC function and U_d3_en register support
> > 
> >   3) Fix rts5260 driving parameter
> > 
> > s/funciton/function/
> > 
> > It looks like 1) *might* be just this:
> > 
> >   rts*_force_power_down()
> >   {
> >     ...
> >     rtsx_pci_write_register(pcr, pcr->reg_pm_ctrl3, D3_DELINK_MODE_EN,
> > 			    D3_DELINK_MODE_EN);
> > 
> > That should be a single patch by itself so it's obvious that it's
> > doing the same thing to several drivers.
> 
> Ok, I will have a extra patch for all xx_force_power_down()

Great, thanks!

> > Explain what "BIOS guide MMC function" means.  Mention the name of the
> > function this adds so we can connect the patch with the commit log.
> > 
> 
> "BIOS guide MMC function" means, via BIOS setting to know MMC card
> support or not 

It will be helpful if this is in a patch by itself.

As far as I know, there's no actual BIOS *call* here, so you must be
looking at some setting in the *device* itself, on the assumption that
it was done by the BIOS?

That sounds like it could become a problem if the device is ever reset
or put in a low-power state.  For resets, and possibly even a
low-power state, BIOS won't be involved, so the setting will be lost,
and the device may work differently after the reset than it did
before.  That sounds undesirable.

> > Explain what "U_d3_en" is; that doesn't even appear in the patch.
> 
> I am going to remove U_d3_en from patch description, this mean
> D3_DELINK_MODE_EN register 

OK.  Given the size of the patch, I think the commit log is too short
to describe what's going on.  More details would be helpful.

Bjorn