Re: [PATCH v3 RESEND 07/11] pwm: imx: Provide atomic PWM support for i.MX PWMv2

[Boris Brezillon]

On Tue, 3 Jan 2017 23:46:58 +0100
Lukasz Majewski <lukma@xxxxxxx> wrote:

> > > > > >> > > > Same goes for the regression introduced in patch 2: I
> > > > > >> > > > think it's better to keep things bisectable on all
> > > > > >> > > > platforms (even if it appeared to work by chance on
> > > > > >> > > > imx7, it did work before this change).      
> > > > > >> > >
> > > > > >> > > Could you be more specific about your idea to solve this
> > > > > >> > > problem?      
> > > > > >> >
> > > > > >> > Stefan already provided a patch, I just think it should be
> > > > > >> > fixed before patch 2 to avoid breaking bisectibility.      
> > > > > >>
> > > > > >> My idea is as follows:
> > > > > >>
> > > > > >> I will drop patch v2 (prepared by Sasha) and then squash
> > > > > >> Stefan's patch [1] to patch 7/11. The "old" ipg enable code
> > > > > >> will be removed with other not needed code during
> > > > > >> conversion.      
> > > > > > 
> > > > > > How about keeping patch 2 but enabling/disabling the periph
> > > > > > clk in imx_pwm_config() instead of completely dropping the
> > > > > > enable/disable clk sequence.
> > > > > > 
> > > > > > In patch 7 you just add the logic we talked about earlier:
> > > > > > unconditionally enable the periph clk when entering the
> > > > > > imx_pwm_apply_v2() function and disable it before leaving the
> > > > > > function.
> > > > > > 
> > > > > > This way you can preserve bisectibility and still get rid of
> > > > > > the ipg clk.
> > > > > > 
> > > > > > Stefan, what's your opinion?      
> > > > > 
> > > > > We will get rid of the ipg clocks anyway in patch 8 (which
> > > > > removes those functions completely).
> > > > > 
> > > > > So I think Lukasz approach should be fine, just drop patch 2 and
> > > > > squash my patch into patch 7.    
> > > > 
> > > > Well, the end result will be same (ipg_clk will be gone after
> > > > patch 8), but then it's hard to track why this clock suddenly
> > > > disappeared. I still think it's worth adding an extra commit
> > > > explaining that enabling the per_clk before accessing IP
> > > > registers is needed on some platforms (imx7), and that IPG clk is
> > > > actually not required until we start using it as a source for the
> > > > PWM signal generation.
> > > > 
> > > > Maybe I'm the only one to think so. In this case, feel free to
> > > > drop patch 2.    
> > > 
> > > If you feel really bad about this issue, then we can drop patch 2
> > > and:
> > > 
> > > reorganize patch 7/11 to 
> > >  - keep code, which adds imx_pwm_apply_v2() function code (just
> > > moves it as is) 
> > >  - remove .apply = imx_pwm_apply_v2 entry from pwm_ops structure.
> > > 
> > > 
> > > On top of it add patch to enable/disable unconditionally the
> > > imx->clk_per clock to avoid problems on imx7 (and state them in
> > > commit message).
> > > 
> > > Then we add separate patch with 
> > > .apply = imx_pwm_apply_v2 to pwm_ops structure to enable "new"
> > > atomic approach.
> > > 
> > > And at last we apply patch 8/11, which removes the code for old (non
> > > atomic) behaviour.
> > > 
> > > All the issues are documented in this way on the cost of having
> > > "dead" (I mean not used) imx_pwm_apply_v2() for two commits.
> > >   
> > 
> > This looks even more complicated.
> > Sorry, but I don't see the problem with modifying patch 2 to enable
> > per_clk instead of ipg_clk. Can you explain what's bothering you?  
> 
> But in patch 2:
> "pwm: imx: remove ipg clock"
> 
>  we _remove_ the clk_ipg from imx_pwm_config() and imx_pwm_probe(), so
>  I'm quite puzzled with your above statement.

See my reworked version below.

> 
> > 
> > If you really want to do the change after patch 7, fine, but in this
> > case, keep the existing logic: enable/disable ipg_clk in
> > imx_pwm_apply_v2() until you drop the ipg_clk and replace the ipg_clk
> > enable/disable sequence by the equivalent enable/disable per_clk one.
> >   
> 
> Frankly, I do agree with Stefan here - we should drop patch 2, squash
> all changes (including imx7 clock issues) to patch 7 (including verbose
> commit message) and remove the non-atomic code in patch 8.

Hm, this is not like I'm asking something impossible here (see the
following patch).

--->8---