On Friday 31 August 2018 02:56 PM, Heiko Stübner wrote:
> Am Dienstag, 14. August 2018, 14:52:00 CEST schrieb Heiko Stuebner:
>> Hi Kishon,
>>
>> thanks for your review.
>>
>> Am Freitag, 3. August 2018, 11:24:06 CEST schrieb Kishon Vijay Abraham I:
>>> On Tuesday 10 July 2018 07:19 PM, Heiko Stuebner wrote:
>>>> +config PHY_ROCKCHIP_INNO_HDMI
>>>> + tristate "Rockchip INNO HDMI PHY Driver"
>>>> + depends on (ARCH_ROCKCHIP || COMPILE_TEST) && OF
>>>
>>> depends on COMMON_CLK since the phy registers a clock provider?
>>
>> ok
>>
>>>> +static const struct phy_config rk3228_phy_cfg[] = {
>>>> + { 165000000, {
>>>> + 0xaa, 0x00, 0x44, 0x44, 0x00, 0x00, 0x00, 0x00, 0x00,
>>>> + 0x00, 0x00, 0x00, 0x00, 0x00,
>>>
>>> If these register configurations are not a tuning value or dividers (which
>>> can have absolute values), then we should have macros for each of these
>>> configurations.
>> That might get difficult.
>>
>> That phy register set is completely undocumented even in the vendor TRMs
>> of the 2 socs. I pieced together most existing macros from code comments
>> and such from the vendor kernel. And Innosilicon is not known to willingly
>> share much of their register documentation it seems.
>>
>>>> +static unsigned long inno_hdmi_phy_get_tmdsclk(struct inno_hdmi_phy
>>>> *inno,
>>>> + unsigned long rate)
>>>> +{
>>>> + int bus_width = phy_get_bus_width(inno->phy);
>>>
>>> hmm, the bus_width could be a member of struct inno_hdmi_phy. PHY API's
>>> don't have to be used for getting data within the PHY driver itself.
>>> Looking at the phy_get_bus_width() implementation, we should have
>>> protected
>>> bus-width set and get with mutex. With that there might be a deadlock
>>> here.
>>
>> ok, so just read phy->attrs.bus_width directly here?
>>
>> I don't see how this can be part of struct inno_hdmi_phy, as the bus-width
>> depends on the color-depth used on the hdmi-controller side, so
>> depending on that it will want to adapt the phy's bus_width.
>>
>> Right now our dw_hdmi in mainline only supports one output format
>> requiring a bus_width of 8, but the vendor kernel shows [0] how this
>> wants to be used with multiple output formats.
Ignore my comment. You can use phy_get_bus_width. I'll see if there's a better
way to handle this.
>>
>> [0]
>> https://github.com/rockchip-linux/kernel/blob/release-4.4/drivers/gpu/drm/r
>> ockchip/dw_hdmi-rockchip.c#L817
>>>> +static irqreturn_t inno_hdmi_phy_rk3328_irq(int irq, void *dev_id)
>>>> +{
>>>> + struct inno_hdmi_phy *inno = dev_id;
>>>> +
>>>> + inno_update_bits(inno, 0x02, RK3328_PDATA_EN, 0);
>>>> + usleep_range(9, 10);
>>>
>>> This range looks very narrow. 10 to 20 should be okay?
>>
>> ok
>>
>>>> +static void inno_hdmi_phy_action(void *data)
>>>> +{
>>>> + struct inno_hdmi_phy *inno = data;
>>>> +
>>>> + clk_disable_unprepare(inno->refpclk);
>>>> + clk_disable_unprepare(inno->sysclk);
>>>> +}
>>>> +
>>>> +static int inno_hdmi_phy_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct inno_hdmi_phy *inno;
>>>> + const struct of_device_id *match;
>>>> + struct phy_provider *phy_provider;
>>>> + struct resource *res;
>>>> + void __iomem *regs;
>>>> + int ret;
>>>> +
>>
>> [...]
>>
>>>> + /*
>>>> + * Refpclk needs to be on, on at least the rk3328 for still
>>>> + * unknown reasons.
>>>> + */
>>>> + ret = clk_prepare_enable(inno->refpclk);
>>>> + if (ret) {
>>>> + dev_err(inno->dev, "failed to enable refpclk\n");
>>>> + clk_disable_unprepare(inno->sysclk);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = devm_add_action_or_reset(inno->dev, inno_hdmi_phy_action,
>>>> + inno);
>>>> + if (ret) {
>>>> + clk_disable_unprepare(inno->refpclk);
>>>> + clk_disable_unprepare(inno->sysclk);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + inno->regmap = devm_regmap_init_mmio(inno->dev, regs,
>>>> + &inno_hdmi_phy_regmap_config);
>>>> + if (IS_ERR(inno->regmap))
>>>
>>> here too clk_disable_unprepare and all error handling below?
>>> It's better if we just handle error handling at the bottom of the
>>> function.
>>
>> Nope ;-) ... I.e. the goal of registering the devm_action above is to have
>> the clocks get disabled in the correct order when devm undoes the other
>> things in sequence.
>>
>> Manual error handling for the clocks would also break devm, as then
>> the clocks would get disabled before the devm cleanup kicks in.
>
> but I just realized, that I don't need the initial disable_unprepare calls
> anymore as well, as devm_add_action_or_reset will call that itself
> in the error case.
All right! I'll merge once you send a patch fixing this.
Thanks
Kishon
