Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy

[Dilip Kota]

Hi Vinod,

On 5/5/2020 3:54 PM, Dilip Kota wrote:
> On 5/5/2020 1:21 PM, Vinod Koul wrote:
> > On 04-05-20, 17:32, Dilip Kota wrote:
> > > On 5/4/2020 5:20 PM, Vinod Koul wrote:
> > > > On 04-05-20, 16:26, Dilip Kota wrote:
> > > > > On 5/4/2020 3:29 PM, Vinod Koul wrote:
> > > > > > On 30-04-20, 15:15, Dilip Kota wrote:
> > > > > >
> > > > > > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u32 mask, u32 val)
> > > > > > > +{
> > > > > > > +ÂÂÂ u32 reg_val;
> > > > > > > +
> > > > > > > +ÂÂÂ reg_val = readl(base + reg);
> > > > > > > +ÂÂÂ reg_val &= ~mask;
> > > > > > > +ÂÂÂ reg_val |= FIELD_PREP(mask, val);
> > > > > > > +ÂÂÂ writel(reg_val, base + reg);
> > > > > > bypassing regmap here... why?
> > > > > It is not regmap address, one of the below two addresses are 
> > > > > passed to this
> > > > > function.
> > > > okay, perhaps add a comment somewhere that regmap is not used for this
> > > > base?
> > > I dont see a need of adding a comment, describing don't do regmap here.
> > Driver uses regmap except here, which seems odd hence explanation
> > required for this.
> During the driver Probe, the register phandles are stored in regmap 
> datatype variables and PHY core addresses are stored in iomem datatype.
> Since then, regmap access is performed for the regmap datatype 
> variables and readl/writel access is performed on the iomem datatype 
> variables. And nowhere in the driver iomem datatype address are 
> converted to regmap address and performed regmap access.
>
> Driver is not doing any 'regmap_init' on any physical address. Driver 
> is getting the register address phandle from the device tree node and 
> performing the regmap access.
> ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 
> 1, 0, &ref);
> [...]
> cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode));
>
> [...]
> ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 
> 1, 0, &ref);
> [...]
>
> cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode));
>
> [...]
> cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
> Â[...]
> cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core");
>
> The DT parsing logic in the driver is explaining why the PHY driver 
> should do regmap access and to whom should be done. For this reason i 
> am a bit puzzled to what more is needed to explain in the comments and 
> where to add it.
> Please let me know your view.
Gentle Reminder!
Could you please update on this.

Regards,
Dilip

> Regards,
> Dilip