On Fri, Oct 28, 2022 at 03:11:31AM +0000, Jim Lin wrote:
> On Thu, 2022-10-27 at 16:00 +0200, Greg KH wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote:
> > > Program USB2 pad PD controls during port connect/disconnect, port
> > > suspend/resume, and test mode, to reduce power consumption on
> > > disconnect or suspend.
> > >
> > > Signed-off-by: Petlozu Pravareshwar <petlozup@xxxxxxxxxx>
> > > Signed-off-by: JC Kuo <jckuo@xxxxxxxxxx>
> > > Signed-off-by: Jim Lin <jilin@xxxxxxxxxx>
> >
> > Who is the author here? These do not seem to be in the correct order
> > if
> > you are the author, right?
> > > This is an old patch. Each time went with some small modification.
>
>
> Petlozu is author for local Kernel 3.18
>
> Then JC for local Kernel 4.4
> Now my turn for Kernel 5.xx
Then you all need to figure out how to proper document this (hint, read
the documentation for how to do that...)
> > > ---
> > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124
> > > v3: No change on copyright
> > > v4: Remove hcd_to_tegra_xusb() function which is used only once.
> > > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c)
> > > Invoke xhci_hub_control() directly (xhci-tegra.c)
> > >
> > > drivers/usb/host/xhci-tegra.c | 131
> > > +++++++++++++++++++++++++++++++++-
> > > 1 file changed, 130 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-
> > > tegra.c
> > > index c8af2cd2216d..f685bb7459ba 100644
> > > --- a/drivers/usb/host/xhci-tegra.c
> > > +++ b/drivers/usb/host/xhci-tegra.c
> > > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc {
> > > } fpci;
> > > };
> > >
> > > +enum tegra_xhci_phy_type {
> > > + USB3_PHY,
> > > + USB2_PHY,
> > > + HSIC_PHY,
> > > + MAX_PHY_TYPES,
> > > +};
> > > +
> > > struct tegra_xusb_soc {
> > > const char *firmware;
> > > const char * const *supply_names;
> > > @@ -274,6 +281,7 @@ struct tegra_xusb {
> > >
> > > bool suspended;
> > > struct tegra_xusb_context context;
> > > + u32 enable_utmi_pad_after_lp0_exit;
> >
> > This is a bitfield, how do we know it will fit in a u32? What is the
> > range you are putting in here?
> >
> > thanks,
> >
> > greg k-h
> static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb
> *tegra)
> {
> unsigned int i;
>
> for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) {
> if (!is_host_mode_phy(tegra, USB2_PHY, i))
> continue;
> /* USB2 */
> if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i))
> :
> How many bits to be used is based on tegra->soc-
> >phy_types[USB2_PHY].num which is defined like
>
> static const struct tegra_xusb_phy_type tegra210_phy_types[] = {
> :
> { .name = "usb2", .num = 4, },
> :
> };
>
> static const struct tegra_xusb_phy_type tegra194_phy_types[] = {
> :
> { .name = "usb2", .num = 4, },
> };
> , so far at most 4.
>
> Therefore u8 for enable_utmi_pad_after_lp0_exit is long enough.
I am sorry, I do not understand. If you are needing to treat this as a
bitfield, then properly define it that way so it is obvious what is
happening.
As it is, this is very confusing and I do not understand what is going
on at all. What is the relationship to "num" and the bit being set?
Why set a bit at all?
thanks,
greg k-h
