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

[Dilip Kota <eswara.kota@linux.intel.com>]

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