On Tue, Nov 18, 2014 at 03:11:26PM +0100, Olliver Schinagl wrote: > > On 18-11-14 14:54, Maxime Ripard wrote: > >On Tue, Nov 18, 2014 at 02:47:33PM +0100, Olliver Schinagl wrote: > >>Hey Alexandre, > >> > >>On 05-11-14 16:15, Alexandre Belloni wrote: > >>>Hi, > >>> > >>>This patch series adds support for the PWM controller found on the > >>>Allwinner > >>>SoCs. > >>> > >>>The first patch adds the driver itself. > >>>The second patch adds the DT binding documentation > >>> > >>>Changes in v8: > >>> - renamed the driver sun4i as the PWM IP is different in the next sunxi > >>> SoCs > >>Why did you decide to rename it to sun4i? From your bindings document I > >>understand you still support sun4i and sun7i, what happened to sun5i? > >> > >>I went over the datasheets of sun4i, sun5i and sun7i and the disp_lcd.c from > >>the latest linux-sunxi kernels and have to agree, they are all 4 > >>inconsistent and messy, but I'm not sure what you mean with PWM IP is > >>different in next sunxi soc's. is sun5i different to sun4i? is sun7i > >>different? or is sun6i (A31), sun8i (a80) and sun9i (A23) different? > >> > >>What I get from the datasheet is, that sun4i and sun5i are exactly the same, > >>with the exception that sun5i only has 1 PWM (~exposed~). I belive that is > >>easily solved with the bindings by having allwinner-sun4i and allwinner > >>sun5i bindings if I'm not mistaken. > >> > >>As for sun7i compared to the other ones, according to disp_lcd.c sun5i and > >>sun7i should behave exactly the same. This is contradicting to the > >>datasheet, where sun4i and sun5i are the same. > >> > >>So what are the major differences that I can see between the 3? sun4i > >>defines the PWM prescaler register value 0b1111 as being undefined, and > >>sun5i and sun7i as /1? Did you verify this (I haven't I admit, i bumped into > >>this while looking for your patch ;-) )? I wouldn't be supprised if it where > >>a typo on allwinners end in the datasheet ... disp_lcd.c stops at 72000 for > >>the last entry. We should just check sun4i, sun5i and sun7i hardware to see > >>if it behaves the same with a prescaler of 0b1111, which I would not be > >>totally surprised if it did. > >> > >>The other difference I notice is that sun7i and sun5i use 16bit period > >>register where sun4i uses a 8bit register. This is probably the only reason > >>why they put a #ifdef in disp_lcd.c, calculations turn out differently. I > >>don't recognize this behavior at all in your driver however. I do think they > >>that there is a difference here, since they did split up the original driver > >>here because of this difference. > >> > >>The pre-scaler bypass bit and pwm ready bit you all ready take care of :) > >A31 and later are using a different IP, that is not supported by this > >driver. > > Ah, see that was not clear to me ;) Also a comment in the code would be > helpful how the 8bit vs 16bit period register is being handled with regards > to sun4i and sun7i. I can't seem to spot where this is taken into account, > since the disp_lcd.c code suggests there is supposedly some difference? I > can't tell from their code what is really going on, since they are using > unsigned 32 bit integers and crap that in the lower and upper half of the > 16bit register, so maybe the register has always been 16bit but the docs are > simply wrong? But then why differentiate between the two chip generations > ...
Is this a bug report? Or just some theorical issues that we might
never encounter?
> >This driver only support the controller introduced with the A10, that
> >only saw minor differences between SoCs, like you have shown.
> >
> >>Finally, though I'm sure I should have replied to it inline in the actual
> >>code, but hoping i can let it slip through here is that you define your
> >>local data as:
> >>
> >>+ static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
> >>
> >>which looks really strange to me, since there is no a20 using the sun4i
> >>architecture :) I know it's just nitpicking, it just looks really odd. Maybe
> >>the compatible naming works just as well? sun4i_pwm_data; sun7i_pwm_data
> >>(and sun5i_pwm_data if you want to take care of only pwm0, only pwm1 (if we
> >>ever encounter such a configuration, disabled pwm0, enabled pwm1) or both to
> >>be used?)
> >This driver is name sun4i_pwm, hence the prefix.
>
> Ah right, unrelated, but I guess I should change my sunxi driver name to
> sun4i as well then?
What? Why?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
signature.asc
Description: Digital signature
