Re: [PATCH v3 1/3] phy: Add exynos-simple-phy driver

[Rahul Sharma]

On 19 May 2014 16:24, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
> On 19.05.2014 09:10, Rahul Sharma wrote:
>> On 16 May 2014 20:19, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
>>> On 16.05.2014 16:30, Rahul Sharma wrote:
>>>> On 16 May 2014 16:20, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
>>>>> On 16.05.2014 12:35, Rahul Sharma wrote:
>>>>>> On 16 May 2014 15:12, Rahul Sharma <rahul.sharma@xxxxxxxxxxx> wrote:
>>>>>>> On 16 May 2014 03:14, Tomasz Figa <tomasz.figa@xxxxxxxxx> wrote:
>>>>>>>> On 15.05.2014 06:01, Rahul Sharma wrote:
>>>>>> [snip]
>>>>>>>>>> the PHY provider.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please correct me if I got you wrong. You want somthing like this:
>>>>>>>>>
>>>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>>>          ...
>>>>>>>>>           simple_phys: simple-phys {
>>>>>>>>>                         compatible = "samsung,exynos5420-simple-phy";
>>>>>>>>>                         ...
>>>>>>>>>           };
>>>>>>>>> };
>>>>>>>>
>>>>>>>> Not exactly.
>>>>>>>>
>>>>>>>> What I meant is that the PMU node itself should be the PHY provider, e.g.
>>>>>>>>
>>>>>>>> pmu_system_controller: system-controller@10040000 {
>>>>>>>>         /* ... */
>>>>>>>>         #phy-cells = <1>;
>>>>>>>> };
>>>>>>>>
>>>>>>>> and then the PMU node should instantiate the Exynos simple PHY driver,
>>>>>>>> as this is a driver for a facility existing entirely inside of the PMU.
>>>>>>>> Moreover, the driver should be rather called Exynos PMU PHY.
>>>>>>>>
>>>>>>>> I know this isn't really possible at the moment, but with device tree we
>>>>>>>> must design things carefully, so it's better to take a bit more time and
>>>>>>>> do things properly.
>>>>>>>>
>>>>>>>> So my opinion on this is that there should be a central Exynos PMU
>>>>>>>> driver that claims the IO region and instantiates necessary subdrivers,
>>>>>>>> such as Exynos PMU PHY driver, Exynos CLKOUT driver, Exynos cpuidle
>>>>>>>> driver and more, similar to what is being done in drivers/mfd.
>>>>>>>
>>>>>>
>>>>>> Hi Tomasz,
>>>>>>
>>>>>> These PHYs are not part of PMU as such. I am not sure if it is correct to
>>>>>> probe them as phy provider for all these phys. Only relation of these phys with
>>>>>> the PMU is 'enable/disable control'.
>>>>>
>>>>> Well, in reality what is implemented by this driver is not even a PHY,
>>>>> just some kind of power controllers, which are contained entirely in the
>>>>> PMU.
>>>>>
>>>>
>>>> I agree. Actually the role of generic phy framework for these 'simple' phys is
>>>> only that much.
>>>>
>>>>>> Controlling this bit using regmap interface
>>>>>> still looks better to me.
>>>>>
>>>>> Well, when there is a choice between using regmap and not using regmap,
>>>>> I'd rather choose the latter. Why would you want to introduce additional
>>>>> abstraction layer if there is no need for such?
>>>>>
>>>>>>
>>>>>> IMHO Ideal method would be probing these PHYs independently and resolving
>>>>>> the necessary dependencies like syscon handle, clocks etc. This way we will
>>>>>> not be having any common phy provider for all these independent PHYs and it
>>>>>> would be clean to add each of these phy nodes in DT. Please see my original
>>>>>> comment below.
>>>>>>
>>>>>> http://lkml.iu.edu/hypermail/linux/kernel/1404.1/00701.html
>>>>>
>>>>> With the solution I proposed, you don't need any kind of dependencies
>>>>> for those simple power controllers. They are just single bits that don't
>>>>> need anything special to operate, except PMU clock running.
>>>>
>>>> In that case we can further trim it down and let the drivers use the regmap
>>>> interface to control this bit. Many drivers including HDMI, DP just need that
>>>> much functionality from the phy provider.
>>>
>>> Well, this is what several drivers already do, like USB PHY (dedicated
>>> IP block), watchdog (for watchdog mask), SATA PHY (dedicated IP block
>>> too) or will do, like I2C (for configuration of I2C mux on Exynos5).
>>>
>>> At least this would be consistent with them and wouldn't be an API
>>> abuse, so I'd be inclined to go this way more than introducing
>>> abstractions like this patch does.
>>
>> Ok. I had already posted a patch for this at
>> http://www.spinics.net/lists/linux-samsung-soc/msg28049.html
>> I will revive that thread.
>
> Looks good to me.
>
>>
>> @Tomasz Stanislawski, Do you have different opinion here?
>
> I'm afraid Tomasz might not be very responsive during next few days, as
> he is on a business trip. You might be able to reach him on our internal
> communicator, though.

Thanks Tomasz,

I will contact him over communicator.

Regards.

>
> Best regards,
> Tomasz
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/