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.
>
> [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.
