Re: [PATCH v1 1/2] usb: dwc3: core: add support for remapping global register start address

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Mon, Apr 10, 2023, Stanley Chang[昌育德] wrote:
> 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > > 476b63618511..771b35449376 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -1785,6 +1785,23 @@ static int dwc3_probe(struct platform_device
> > *pdev)
> > >       dwc_res = *res;
> > >       dwc_res.start += DWC3_GLOBALS_REGS_START;
> > >
> > > +     /* For some dwc3 controller, the dwc3 global register start address is
> > > +      * not at DWC3_GLOBALS_REGS_START (0xc100).
> > > +      */
> > 
> > Please use this comment style block:
> >  /*
> >   * a b c
> >   * d e f
> >   */
> > 
> 
> I will follow this rule.
> 
> > > +     if (dev) {
> > 
> > Why do we need this if (dev) check? When would this not be the case?
> 
> I want the variable "fixed_dwc3_globals_regs_start" to be a local variable. 
> So I added an if statement.
> I can modify it to "if (dev->of_node)" which looks more make sense.

Why? If it's within this function, it's a local variable. Don't create
arbitrary condition just to limit the scope of the variable.

Thanks,
Thinh

> 
> > 
> > > +             int fixed_dwc3_globals_regs_start;
> > 
> > Need to initialize this in case you get bogus value when the property is not
> > defined.
> 
> Thanks. I will add the initial value.
> 
> > 
> > > +
> > > +             device_property_read_u32(dev,
> > > + "snps,fixed_dwc3_globals_regs_start",
> > 
> > Property name should be using "-" instead of "_". Also can we rename it to
> > "snps,global-regs-starting-offset"
> > 
> > Thanks,
> > Thinh
> > 
> 
> I will rename as "snps,global-regs-starting-offset" in next version.
> 
> Thanks a lot of.
> Stanley