Hi Philipp,

On 2021/12/1, 5:12 PM, "Philipp Zabel" [off-list ref] wrote:

    Hi Billy,

    On Wed, 2021-12-01 at 03:30 +0000, Billy Tsai wrote:
    >   > Hi Philipp,
    >   > 
    >   > On 2021/11/30, 5:52 PM, "Philipp Zabel" [off-list ref] wrote:
    >   > 
    >   >     On Mon, 2021-11-29 at 14:43 +0800, Billy Tsai wrote:
    >   >     [...]
    >   >     >   > +	ret = clk_prepare_enable(priv->clk);
    >   >     >   > +	if (ret)
    >   >     >   > +		return dev_err_probe(dev, ret, "Couldn't enable clock\n");
    >   >     >   > +
    >   >     >   > +	ret = reset_control_deassert(priv->reset);
    >   >     >   > +	if (ret) {
    >   >     >   > +		dev_err_probe(dev, ret, "Couldn't deassert reset control\n");
    >   >     >   > +		goto err_disable_clk;
    >   >     >   > +	}
    >   > 
    >   >     >   Is there any reason to keep the clocks running and the controller out of
    >   >     >   reset while the PWM outputs are disabled?
    >   > 
    >   > Can you tell me about your concerns with this process?

    >   No particular concerns, just curiosity.

    >   > In my opinion, they are used to provide the clock and de-assert the reset of the PWM engine. If we didn't release
    >   > them in probe stage the CPU can't and shouldn't read the register of the PWM engine when call the get_state.
    >   > Assume that we want to adjust them dynamically, the driver needs to add more conditions to check and keep the status
    >   > of each PWM channel, which is not a good deal for the server platform.

    >   Thanks. I don't know the hardware, so I have no idea whether disabling
    >   the clocks would even save a measurable (let alone appreciable) amount
    >   of power.

    >   I've just seen other PWM drivers use runtime PM or enable/disable clocks
    >   dynamically, and wondered why this one doesn't.

Thanks for the heads up, I think that for the usage we don't need to implement that.

Best Regards,
Billy Tsai