> > > From: Ricky Wu <ricky_wu@xxxxxxxxxxx>
> > >
> > > 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
