| Message ID | 1397285583-15187-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 12 Apr 2014 10:53:02 +0400 Alexander Shiyan <shc_work@mail.ru> wrote: Ping. > This adds support for the framebuffer available in the Cirrus > Logic CLPS711X CPUs. > FB features: > - 1-2-4 bits per pixel. > - Programmable panel size to a maximum of 1024x256 at 4 bps. > - Relocatible Frame Buffer (SRAM or SDRAM). > - Programmable refresh rates. > - 16 gray scale values. > This new driver supports usage with devicetree and as a general > change it removes last user of <mach/hardware.h> for CLPS711X targets, > so this subarch will fully prepared to switch to multiplatform. > The driver have been tested with custom board equipped Cirrus Logic > EP7312 in DT and non-DT mode. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> ...
Hi, On 12/04/14 09:53, Alexander Shiyan wrote: > This adds support for the framebuffer available in the Cirrus > Logic CLPS711X CPUs. > FB features: > - 1-2-4 bits per pixel. > - Programmable panel size to a maximum of 1024x256 at 4 bps. > - Relocatible Frame Buffer (SRAM or SDRAM). > - Programmable refresh rates. > - 16 gray scale values. > This new driver supports usage with devicetree and as a general > change it removes last user of <mach/hardware.h> for CLPS711X targets, > so this subarch will fully prepared to switch to multiplatform. > The driver have been tested with custom board equipped Cirrus Logic > EP7312 in DT and non-DT mode. My original comment about this is still unanswered: why a totally new driver? The proper way would be to gradually change the old driver with a patch series. Then it's possible to review the patches and see what is actually changed. Tomi
Hello. Thu, 24 Apr 2014 10:57:59 +0300 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: > Hi, > > On 12/04/14 09:53, Alexander Shiyan wrote: > > This adds support for the framebuffer available in the Cirrus > > Logic CLPS711X CPUs. > > FB features: > > - 1-2-4 bits per pixel. > > - Programmable panel size to a maximum of 1024x256 at 4 bps. > > - Relocatible Frame Buffer (SRAM or SDRAM). > > - Programmable refresh rates. > > - 16 gray scale values. > > This new driver supports usage with devicetree and as a general > > change it removes last user of <mach/hardware.h> for CLPS711X targets, > > so this subarch will fully prepared to switch to multiplatform. > > The driver have been tested with custom board equipped Cirrus Logic > > EP7312 in DT and non-DT mode. > > My original comment about this is still unanswered: why a totally new > driver? The proper way would be to gradually change the old driver with > a patch series. Then it's possible to review the patches and see what is > actually changed. I have tried to answer here: http://www.spinics.net/lists/linux-fbdev/msg14218.html ---
On 24/04/14 11:10, Alexander Shiyan wrote: > Hello. > > Thu, 24 Apr 2014 10:57:59 +0300 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: >> Hi, >> >> On 12/04/14 09:53, Alexander Shiyan wrote: >>> This adds support for the framebuffer available in the Cirrus >>> Logic CLPS711X CPUs. >>> FB features: >>> - 1-2-4 bits per pixel. >>> - Programmable panel size to a maximum of 1024x256 at 4 bps. >>> - Relocatible Frame Buffer (SRAM or SDRAM). >>> - Programmable refresh rates. >>> - 16 gray scale values. >>> This new driver supports usage with devicetree and as a general >>> change it removes last user of <mach/hardware.h> for CLPS711X targets, >>> so this subarch will fully prepared to switch to multiplatform. >>> The driver have been tested with custom board equipped Cirrus Logic >>> EP7312 in DT and non-DT mode. >> >> My original comment about this is still unanswered: why a totally new >> driver? The proper way would be to gradually change the old driver with >> a patch series. Then it's possible to review the patches and see what is >> actually changed. > > I have tried to answer here: > http://www.spinics.net/lists/linux-fbdev/msg14218.html If your answer means "it will be very difficult to see the changes if all the changes are in one patch, which change the old driver in one go", then yes, very true. But that's wrong way to do it. The right way to do it is, as I wrote above, by gradually changing the old driver with a patch series. And my question is, why not do it that way? Then it would be possible to review the patches one by one, seeing what has changed. Tomi
On 24/04/14 19:35, Alexander Shiyan wrote: >> The right way to do it is, as I wrote above, by gradually changing the >> old driver with a patch series. And my question is, why not do it that >> way? Then it would be possible to review the patches one by one, seeing >> what has changed. > > "gradually changing"... > I repeat that this is not an old modified driver, but written new. Yes, I understand that. Again, my question is, why didn't you modify the old driver? That's how things should normally be done. Instead, you made a totally new one, making proper review against the old driver impossible. > if you imagine a new file as a diff to the old, this can be clearly seen. > > There is no reason to waste time on a series of changes since I > can not even check these changes on real hardware, but only in the > last stage when the driver will be the current version. Hmm what? So is the old driver totally broken, and cannot be used at the moment? Or why you can't test on real hardware? Note that I don't know anything about the fb hardware in question, nor the driver. Maybe there's a valid reason to write a new driver from scratch. But there very rarely is. And "because I already wrote a new driver, and it's a waste of time for me to throw away my work and patch the old one", is not a very good reason. Tomi
On Wed, Apr 30, 2014 at 04:36:29PM +0400, Alexander Shiyan wrote: > Wed, 30 Apr 2014 14:14:36 +0300 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: > > On 24/04/14 19:35, Alexander Shiyan wrote: > > Hello. > > I'm a reorder quotes a bit for convenience. > > > >> The right way to do it is, as I wrote above, by gradually changing the > > >> old driver with a patch series. And my question is, why not do it that > > >> way? Then it would be possible to review the patches one by one, seeing > > >> what has changed. > > > > > > "gradually changing"... > > > I repeat that this is not an old modified driver, but written new. > > > > Yes, I understand that. Again, my question is, why didn't you modify the > > old driver? That's how things should normally be done. Instead, you made > > a totally new one, making proper review against the old driver impossible. > ... > > Hmm what? So is the old driver totally broken, and cannot be used at the > > moment? Or why you can't test on real hardware? > > Firstly, the driver uses a fixed values for xres, yres, pixclock and specific > variable ac_precale. > Secondly, the driver uses a fixed value for the physical address of the buffer. > Totally, it does not give me the ability to use the driver in the current state. > Unlikely that this will look good if I make these two significant changes in > a single patch... > > At this time the driver has three user. > Only one of them should theoretically work. > clps711x-autcpu12 should not work in the absence of memblock_reserve(). > clps711x-p720t should not work due to physical address limitation as i > noticed before. Board means to use SRAM instead of SDRAM. > Only clps711x-edb7211 should work fine (in theory). > Is this a good reason to replace the driver? I think yes. > > > Note that I don't know anything about the fb hardware in question, nor > > the driver. Maybe there's a valid reason to write a new driver from > > scratch. But there very rarely is. > > > > And "because I already wrote a new driver, and it's a waste of time for > > me to throw away my work and patch the old one", is not a very good reason. > > > > if you imagine a new file as a diff to the old, this can be clearly seen. > > > > > > There is no reason to waste time on a series of changes since I > > > can not even check these changes on real hardware, but only in the > > > last stage when the driver will be the current version. > > Summing up, I want to ask why the driver can not be reviewed as a > new and not compared to the old? > And yes, the time can be spent on more productive things to do than > to create a series, leading eventually to the same result. > As far as I know, guide to creating kernel patches allows such cases. We've definitely had cases like that in the past. Sometimes it's easier to first post a patch that removes the old driver, then one that submits the new one as a new piece of work. That, of course, assumes that the driver indeed was a rewrite and not just a bunch of incremental changes all squashed into one. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 30/04/14 15:36, Alexander Shiyan wrote: >> Hmm what? So is the old driver totally broken, and cannot be used at the >> moment? Or why you can't test on real hardware? > > Firstly, the driver uses a fixed values for xres, yres, pixclock and specific > variable ac_precale. > Secondly, the driver uses a fixed value for the physical address of the buffer. > Totally, it does not give me the ability to use the driver in the current state. > Unlikely that this will look good if I make these two significant changes in > a single patch... > > At this time the driver has three user. > Only one of them should theoretically work. > clps711x-autcpu12 should not work in the absence of memblock_reserve(). > clps711x-p720t should not work due to physical address limitation as i > noticed before. Board means to use SRAM instead of SDRAM. > Only clps711x-edb7211 should work fine (in theory). > Is this a good reason to replace the driver? I think yes. Ok, if the situation is that bad, maybe we can just switch to the new driver. Have you verified that those boards do not work from anyone? Or asked someone to test the new driver with those boards? I'm still not really happy about it, and I'd much rather see the current driver fixed. But if no one having those boards is up to the task (probably not if they have not been working at all), maybe just ditching the old driver and adding a new is the only way forward. One change that I think would be good is to change the series to first remove the old driver, and then add the new one, with the same file name as the old one. That way git log will show the history for both the new and the old driver. >> Note that I don't know anything about the fb hardware in question, nor >> the driver. Maybe there's a valid reason to write a new driver from >> scratch. But there very rarely is. >> >> And "because I already wrote a new driver, and it's a waste of time for >> me to throw away my work and patch the old one", is not a very good reason. > >>> if you imagine a new file as a diff to the old, this can be clearly seen. >>> >>> There is no reason to waste time on a series of changes since I >>> can not even check these changes on real hardware, but only in the >>> last stage when the driver will be the current version. > > Summing up, I want to ask why the driver can not be reviewed as a > new and not compared to the old? It can, of course. But we normally should not. Fixing the old driver will keep all the fixes and tweaks that have been debugged and solved with the old driver. Creating a new driver easily makes those old fixes disappear. Fixing the old driver also keeps the history, making it possible to see where an issue was introduced etc. > And yes, the time can be spent on more productive things to do than > to create a series, leading eventually to the same result. Yes, your time. But if, say, the new driver introduces bugs that already were fixed in the old driver, causing problems to other people, and to me as the maintainer, I'm sure the other people and me are not going to say "well this is ok, as this saved Alexander's time" =). Tomi
On 08/05/14 11:27, Alexander Shiyan wrote: >>> At this time the driver has three user. >>> Only one of them should theoretically work. >>> clps711x-autcpu12 should not work in the absence of memblock_reserve(). >>> clps711x-p720t should not work due to physical address limitation as i >>> noticed before. Board means to use SRAM instead of SDRAM. >>> Only clps711x-edb7211 should work fine (in theory). >>> Is this a good reason to replace the driver? I think yes. >> >> Ok, if the situation is that bad, maybe we can just switch to the new >> driver. Have you verified that those boards do not work from anyone? Or >> asked someone to test the new driver with those boards? > > I'm not familiar with other users of this platform . > I am do not have these boards, all that I have written before that it's just a theory. > Firm in which I work, uses its own board with CLPS711X CPU , this board is the > only way to check for changes on real hardware . Ok. That makes me a bit nervous... You're removing a driver, which may (or may not) have been working for other users. And adding a new one, which may not (or may) work for the other users. >> I'm still not really happy about it, and I'd much rather see the current >> driver fixed. But if no one having those boards is up to the task >> (probably not if they have not been working at all), maybe just ditching >> the old driver and adding a new is the only way forward. >> >> One change that I think would be good is to change the series to first >> remove the old driver, and then add the new one, with the same file name >> as the old one. That way git log will show the history for both the new >> and the old driver. > > In this case git-bisect will be broken. Is this OK? I think this is such a big change for the users of the driver that it's not an issue. Of course, kernel should still build at all steps, but I think it's fine if the fb driver in question will be out for a commit or two. Tomi
On Thu, May 08, 2014 at 01:14:40PM +0300, Tomi Valkeinen wrote: > On 08/05/14 11:27, Alexander Shiyan wrote: > > >>> At this time the driver has three user. > >>> Only one of them should theoretically work. > >>> clps711x-autcpu12 should not work in the absence of memblock_reserve(). > >>> clps711x-p720t should not work due to physical address limitation as i > >>> noticed before. Board means to use SRAM instead of SDRAM. > >>> Only clps711x-edb7211 should work fine (in theory). > >>> Is this a good reason to replace the driver? I think yes. > >> > >> Ok, if the situation is that bad, maybe we can just switch to the new > >> driver. Have you verified that those boards do not work from anyone? Or > >> asked someone to test the new driver with those boards? > > > > I'm not familiar with other users of this platform . > > I am do not have these boards, all that I have written before that it's just a theory. > > Firm in which I work, uses its own board with CLPS711X CPU , this board is the > > only way to check for changes on real hardware . > > Ok. That makes me a bit nervous... You're removing a driver, which may > (or may not) have been working for other users. And adding a new one, > which may not (or may) work for the other users. Keep the old one around under another Kconfig name, mark it BROKEN, and if nobody speaks up in a couple of releases, remove it? > >> I'm still not really happy about it, and I'd much rather see the current > >> driver fixed. But if no one having those boards is up to the task > >> (probably not if they have not been working at all), maybe just ditching > >> the old driver and adding a new is the only way forward. > >> > >> One change that I think would be good is to change the series to first > >> remove the old driver, and then add the new one, with the same file name > >> as the old one. That way git log will show the history for both the new > >> and the old driver. > > > > In this case git-bisect will be broken. Is this OK? > > I think this is such a big change for the users of the driver that it's > not an issue. Of course, kernel should still build at all steps, but I > think it's fine if the fb driver in question will be out for a commit or > two. Agreed. -Olof -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fri, 16 May 2014 15:56:26 -0700 ?? Olof Johansson <olof@lixom.net>: > On Thu, May 08, 2014 at 01:14:40PM +0300, Tomi Valkeinen wrote: > > On 08/05/14 11:27, Alexander Shiyan wrote: > > > > >>> At this time the driver has three user. > > >>> Only one of them should theoretically work. > > >>> clps711x-autcpu12 should not work in the absence of memblock_reserve(). > > >>> clps711x-p720t should not work due to physical address limitation as i > > >>> noticed before. Board means to use SRAM instead of SDRAM. > > >>> Only clps711x-edb7211 should work fine (in theory). > > >>> Is this a good reason to replace the driver? I think yes. > > >> > > >> Ok, if the situation is that bad, maybe we can just switch to the new > > >> driver. Have you verified that those boards do not work from anyone? Or > > >> asked someone to test the new driver with those boards? > > > > > > I'm not familiar with other users of this platform . > > > I am do not have these boards, all that I have written before that it's just a theory. > > > Firm in which I work, uses its own board with CLPS711X CPU , this board is the > > > only way to check for changes on real hardware . > > > > Ok. That makes me a bit nervous... You're removing a driver, which may > > (or may not) have been working for other users. And adding a new one, > > which may not (or may) work for the other users. > > Keep the old one around under another Kconfig name, mark it BROKEN, > and if nobody speaks up in a couple of releases, remove it? I like this variant, Tomi are you agree with this? ---
On 12/04/14 09:53, Alexander Shiyan wrote: > This adds support for the framebuffer available in the Cirrus > Logic CLPS711X CPUs. > FB features: > - 1-2-4 bits per pixel. > - Programmable panel size to a maximum of 1024x256 at 4 bps. > - Relocatible Frame Buffer (SRAM or SDRAM). > - Programmable refresh rates. > - 16 gray scale values. > This new driver supports usage with devicetree and as a general > change it removes last user of <mach/hardware.h> for CLPS711X targets, > so this subarch will fully prepared to switch to multiplatform. > The driver have been tested with custom board equipped Cirrus Logic > EP7312 in DT and non-DT mode. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > drivers/video/fbdev/clps711x-fb.c | 456 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 456 insertions(+) > create mode 100644 drivers/video/fbdev/clps711x-fb.c <snip> > + > +static int clps711x_fb_get_mode_dt(struct platform_device *pdev) > +{ > + struct device_node *disp, *np = pdev->dev.of_node; > + struct fb_info *info = platform_get_drvdata(pdev); > + struct clps711x_fb_info *cfb = info->par; > + int ret; > + > + cfb->syscon = > + syscon_regmap_lookup_by_compatible("cirrus,clps711x-syscon1"); > + if (IS_ERR(cfb->syscon)) > + return PTR_ERR(cfb->syscon); Hmm, what's the syscon stuff about? Looks like it's required, but the DT documentation patch doesn't mention it at all. Tomi
On 18/05/14 15:01, Alexander Shiyan wrote: > Fri, 16 May 2014 15:56:26 -0700 ?? Olof Johansson <olof@lixom.net>: >> On Thu, May 08, 2014 at 01:14:40PM +0300, Tomi Valkeinen wrote: >>> On 08/05/14 11:27, Alexander Shiyan wrote: >>> >>>>>> At this time the driver has three user. >>>>>> Only one of them should theoretically work. >>>>>> clps711x-autcpu12 should not work in the absence of memblock_reserve(). >>>>>> clps711x-p720t should not work due to physical address limitation as i >>>>>> noticed before. Board means to use SRAM instead of SDRAM. >>>>>> Only clps711x-edb7211 should work fine (in theory). >>>>>> Is this a good reason to replace the driver? I think yes. >>>>> >>>>> Ok, if the situation is that bad, maybe we can just switch to the new >>>>> driver. Have you verified that those boards do not work from anyone? Or >>>>> asked someone to test the new driver with those boards? >>>> >>>> I'm not familiar with other users of this platform . >>>> I am do not have these boards, all that I have written before that it's just a theory. >>>> Firm in which I work, uses its own board with CLPS711X CPU , this board is the >>>> only way to check for changes on real hardware . >>> >>> Ok. That makes me a bit nervous... You're removing a driver, which may >>> (or may not) have been working for other users. And adding a new one, >>> which may not (or may) work for the other users. >> >> Keep the old one around under another Kconfig name, mark it BROKEN, >> and if nobody speaks up in a couple of releases, remove it? > > I like this variant, Tomi are you agree with this? I don't know. All options sound somewhat bad to me, except if it's clear nobody uses the old driver. Anyway, it's rather late for 3.16. I'd like the removal of the old driver to sit in the linux-next for a while. Would it be possible to add the new driver along the old driver, and use the new driver only for the boards you have, and for boards for which it's clear the the old driver is not working? This could be merged for 3.16. It's not so nice to have two drivers for the same hardware, but in this case maybe that's not an issue. The old driver doesn't support DT, so no conflicts there, and for non-DT the driver names seem to be different. For 3.17, we could remove the old driver, and push that change to linux-next right after the 3.16 merge window has closed. Or, if you're fine with it, we can also delay adding the new driver to 3.17, and do both right after the 3.16 merge window has closed. I personally like this most, so that we have both removal of the old and adding the new sitting in the linux-next longer, but you might possibly want the new driver in sooner. Maybe I'm being overly cautious here, but as I don't have any idea about the driver and its users, I rather be too cautious than not. Tomi
Fri, 23 May 2014 15:43:40 +0300 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: > On 18/05/14 15:01, Alexander Shiyan wrote: > > Fri, 16 May 2014 15:56:26 -0700 ?? Olof Johansson <olof@lixom.net>: > >> On Thu, May 08, 2014 at 01:14:40PM +0300, Tomi Valkeinen wrote: > >>> On 08/05/14 11:27, Alexander Shiyan wrote: > >>> > >>>>>> At this time the driver has three user. > >>>>>> Only one of them should theoretically work. > >>>>>> clps711x-autcpu12 should not work in the absence of memblock_reserve(). > >>>>>> clps711x-p720t should not work due to physical address limitation as i > >>>>>> noticed before. Board means to use SRAM instead of SDRAM. > >>>>>> Only clps711x-edb7211 should work fine (in theory). > >>>>>> Is this a good reason to replace the driver? I think yes. > >>>>> > >>>>> Ok, if the situation is that bad, maybe we can just switch to the new > >>>>> driver. Have you verified that those boards do not work from anyone? Or > >>>>> asked someone to test the new driver with those boards? > >>>> > >>>> I'm not familiar with other users of this platform . > >>>> I am do not have these boards, all that I have written before that it's just a theory. > >>>> Firm in which I work, uses its own board with CLPS711X CPU , this board is the > >>>> only way to check for changes on real hardware . > >>> > >>> Ok. That makes me a bit nervous... You're removing a driver, which may > >>> (or may not) have been working for other users. And adding a new one, > >>> which may not (or may) work for the other users. > >> > >> Keep the old one around under another Kconfig name, mark it BROKEN, > >> and if nobody speaks up in a couple of releases, remove it? > > > > I like this variant, Tomi are you agree with this? > > I don't know. All options sound somewhat bad to me, except if it's clear > nobody uses the old driver. > > Anyway, it's rather late for 3.16. I'd like the removal of the old > driver to sit in the linux-next for a while. > > Would it be possible to add the new driver along the old driver, and use > the new driver only for the boards you have, and for boards for which > it's clear the the old driver is not working? This could be merged for 3.16. At this time yes, we can. But since I plan to add multiplatform support for this SOC, this seems not possible. I can try to make multiplatform support optional, then it could be done... > It's not so nice to have two drivers for the same hardware, but in this > case maybe that's not an issue. The old driver doesn't support DT, so no > conflicts there, and for non-DT the driver names seem to be different. > > For 3.17, we could remove the old driver, and push that change to > linux-next right after the 3.16 merge window has closed. > > Or, if you're fine with it, we can also delay adding the new driver to > 3.17, and do both right after the 3.16 merge window has closed. I > personally like this most, so that we have both removal of the old and > adding the new sitting in the linux-next longer, but you might possibly > want the new driver in sooner. If there will be two drivers, I will do the following: remove the non-DT support (for new driver) and will create a patch for 3.16 (this patch will no affect to arm-soc). After that, I will do optional multiplatform support for this CPU and move the boards, which do not use FB. After this architecture will be ready to add DT support, and after all boards will be converted, I'll remove the old version of the driver. OK? ---
Fri, 23 May 2014 15:31:44 +0300 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: > On 12/04/14 09:53, Alexander Shiyan wrote: > > This adds support for the framebuffer available in the Cirrus > > Logic CLPS711X CPUs. > > FB features: > > - 1-2-4 bits per pixel. > > - Programmable panel size to a maximum of 1024x256 at 4 bps. > > - Relocatible Frame Buffer (SRAM or SDRAM). > > - Programmable refresh rates. > > - 16 gray scale values. > > This new driver supports usage with devicetree and as a general > > change it removes last user of <mach/hardware.h> for CLPS711X targets, > > so this subarch will fully prepared to switch to multiplatform. > > The driver have been tested with custom board equipped Cirrus Logic > > EP7312 in DT and non-DT mode. > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > drivers/video/fbdev/clps711x-fb.c | 456 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 456 insertions(+) > > create mode 100644 drivers/video/fbdev/clps711x-fb.c > > <snip> > > > + > > +static int clps711x_fb_get_mode_dt(struct platform_device *pdev) > > +{ > > + struct device_node *disp, *np = pdev->dev.of_node; > > + struct fb_info *info = platform_get_drvdata(pdev); > > + struct clps711x_fb_info *cfb = info->par; > > + int ret; > > + > > + cfb->syscon = > > + syscon_regmap_lookup_by_compatible("cirrus,clps711x-syscon1"); > > + if (IS_ERR(cfb->syscon)) > > + return PTR_ERR(cfb->syscon); > > Hmm, what's the syscon stuff about? Looks like it's required, but the DT > documentation patch doesn't mention it at all. This does not require any separate property for FB. The syscon registers is global to platform. ---
On Friday 23 May 2014 17:13:58 Alexander Shiyan wrote: > If there will be two drivers, I will do the following: remove the non-DT > support (for new driver) and will create a patch for 3.16 (this patch will > no affect to arm-soc). > After that, I will do optional multiplatform support for this CPU and move > the boards, which do not use FB. > After this architecture will be ready to add DT support, and after all boards > will be converted, I'll remove the old version of the driver. > > OK? > Sounds good to me. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/05/14 16:13, Alexander Shiyan wrote: >> Would it be possible to add the new driver along the old driver, and use >> the new driver only for the boards you have, and for boards for which >> it's clear the the old driver is not working? This could be merged for 3.16. > > At this time yes, we can. But since I plan to add multiplatform support > for this SOC, this seems not possible. > I can try to make multiplatform support optional, then it could be done... Hmm, why is that not possible with multiplatform support? What do you mean with multiplatform support here? While the drivers would handle the same device, if they have different names then they are different device drivers from Linux's perspective. Why can't one board use the old driver, and an other board use the new driver? > If there will be two drivers, I will do the following: remove the non-DT > support (for new driver) and will create a patch for 3.16 (this patch will > no affect to arm-soc). There would be no one using the driver in 3.16, then, right? > After that, I will do optional multiplatform support for this CPU and move > the boards, which do not use FB. > After this architecture will be ready to add DT support, and after all boards > will be converted, I'll remove the old version of the driver. > > OK? I'm a bit unclear what the multiplatform stuff means here, but yes, generally sounds ok. But I don't want to make this more difficult for you than it needs to. As I said, I'm fine with the current patches, if we skip 3.16 and get them to linux-next right after the merge window. If nobody would use the new driver in 3.16 anyway (in your proposal above), would this be the easiest way? Tomi
On Friday 23 May 2014 17:10:35 Tomi Valkeinen wrote: > On 23/05/14 16:13, Alexander Shiyan wrote: > > >> Would it be possible to add the new driver along the old driver, and use > >> the new driver only for the boards you have, and for boards for which > >> it's clear the the old driver is not working? This could be merged for 3.16. > > > > At this time yes, we can. But since I plan to add multiplatform support > > for this SOC, this seems not possible. > > I can try to make multiplatform support optional, then it could be done... > > Hmm, why is that not possible with multiplatform support? What do you > mean with multiplatform support here? We are migrating all ARM platforms to allow building them into a single kernel. However, that means that device drivers cannot access platform specific header files any more and have to get hardware information from DT or through platform data. The existing driver however uses mach/hardware.h. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fri, 23 May 2014 15:48:43 +0200 ?? Arnd Bergmann <arnd@arndb.de>: > On Friday 23 May 2014 17:13:58 Alexander Shiyan wrote: > > If there will be two drivers, I will do the following: remove the non-DT > > support (for new driver) and will create a patch for 3.16 (this patch will > > no affect to arm-soc). > > After that, I will do optional multiplatform support for this CPU and move > > the boards, which do not use FB. > > After this architecture will be ready to add DT support, and after all boards > > will be converted, I'll remove the old version of the driver. > > > > OK? > > > > Sounds good to me. I am glad that a small blow-up code for this (temporary) period is not critical. Of course it's a bit more complicated than what I had planned, but I think it is possible to do. ---
On 23/05/14 17:14, Arnd Bergmann wrote: > On Friday 23 May 2014 17:10:35 Tomi Valkeinen wrote: >> On 23/05/14 16:13, Alexander Shiyan wrote: >> >>>> Would it be possible to add the new driver along the old driver, and use >>>> the new driver only for the boards you have, and for boards for which >>>> it's clear the the old driver is not working? This could be merged for 3.16. >>> >>> At this time yes, we can. But since I plan to add multiplatform support >>> for this SOC, this seems not possible. >>> I can try to make multiplatform support optional, then it could be done... >> >> Hmm, why is that not possible with multiplatform support? What do you >> mean with multiplatform support here? > > We are migrating all ARM platforms to allow building them into a single > kernel. However, that means that device drivers cannot access platform > specific header files any more and have to get hardware information from > DT or through platform data. The existing driver however uses mach/hardware.h. Ah, I see. So the problem is not with two different drivers for the same hardware device as such, but that in this case the older driver just doesn't play well with multiplatform. Tomi
On 23/05/14 17:27, Alexander Shiyan wrote: >> But I don't want to make this more difficult for you than it needs to. >> As I said, I'm fine with the current patches, if we skip 3.16 and get >> them to linux-next right after the merge window. If nobody would use the >> new driver in 3.16 anyway (in your proposal above), would this be the >> easiest way? > > This is the only way to announce initial multiplatform support (in case we will > use two differrent drivers). > Another (easiest) way is to use the current patchset... If the old driver doesn't even play well with multiplatform, and is thus blocking moving the SoC to multiplatform, I'd just get done with it in one go. So I would recommend pushing the new driver and the removal of the old one for 3.17, and having those changes in linux-next right after the 3.16 merge window. Is that ok? Tomi
Fri, 23 May 2014 17:32:08 +0300 ?? Tomi Valkeinen <tomi.valkeinen@ti.com>: > On 23/05/14 17:27, Alexander Shiyan wrote: > > >> But I don't want to make this more difficult for you than it needs to. > >> As I said, I'm fine with the current patches, if we skip 3.16 and get > >> them to linux-next right after the merge window. If nobody would use the > >> new driver in 3.16 anyway (in your proposal above), would this be the > >> easiest way? > > > > This is the only way to announce initial multiplatform support (in case we will > > use two differrent drivers). > > Another (easiest) way is to use the current patchset... > > If the old driver doesn't even play well with multiplatform, and is thus > blocking moving the SoC to multiplatform, I'd just get done with it in > one go. > > So I would recommend pushing the new driver and the removal of the old > one for 3.17, and having those changes in linux-next right after the > 3.16 merge window. > > Is that ok? For me it is OK, of course. Do I understand correctly that in this case I need to update patchset which only adds new driver and new Kconfig symbol? Probably I am add "depend on BROKEN if !ARCH_MULTIPLATFORM" to the old driver... ---
diff --git a/drivers/video/fbdev/clps711x-fb.c b/drivers/video/fbdev/clps711x-fb.c new file mode 100644 index 0000000..87fd42d --- /dev/null +++ b/drivers/video/fbdev/clps711x-fb.c @@ -0,0 +1,456 @@ +/* + * Cirrus Logic CLPS711X FB driver + * + * Copyright (C) 2014 Alexander Shiyan <shc_work@mail.ru> + * Based on driver by Russell King <rmk@arm.linux.org.uk> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/fb.h> +#include <linux/io.h> +#include <linux/lcd.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> +#include <linux/mfd/syscon/clps711x.h> +#include <linux/regulator/consumer.h> +#include <video/of_display_timing.h> + +#define CLPS711X_FB_NAME "clps711x-fb" +#define CLPS711X_FB_BPP_MAX (4) + +/* Registers relative to LCDCON */ +#define CLPS711X_LCDCON (0x0000) +# define LCDCON_GSEN BIT(30) +# define LCDCON_GSMD BIT(31) +#define CLPS711X_PALLSW (0x0280) +#define CLPS711X_PALMSW (0x02c0) +#define CLPS711X_FBADDR (0x0d40) + +struct clps711x_fb_info { + struct clk *clk; + void __iomem *base; + struct regmap *syscon; + resource_size_t buffsize; + struct fb_videomode mode; + struct regulator *lcd_pwr; + u32 ac_prescale; + bool cmap_invert; +}; + +static int clps711x_fb_setcolreg(u_int regno, u_int red, u_int green, + u_int blue, u_int transp, struct fb_info *info) +{ + struct clps711x_fb_info *cfb = info->par; + u32 level, mask, shift; + + if (regno >= BIT(info->var.bits_per_pixel)) + return -EINVAL; + + shift = 4 * (regno & 7); + mask = 0xf << shift; + /* gray = 0.30*R + 0.58*G + 0.11*B */ + level = (((red * 77 + green * 151 + blue * 28) >> 20) << shift) & mask; + if (cfb->cmap_invert) + level = 0xf - level; + + regno = (regno < 8) ? CLPS711X_PALLSW : CLPS711X_PALMSW; + + writel((readl(cfb->base + regno) & ~mask) | level, cfb->base + regno); + + return 0; +} + +static int clps711x_fb_check_var(struct fb_var_screeninfo *var, + struct fb_info *info) +{ + u32 val; + + if (var->bits_per_pixel < 1 || + var->bits_per_pixel > CLPS711X_FB_BPP_MAX) + return -EINVAL; + + if (!var->pixclock) + return -EINVAL; + + val = DIV_ROUND_UP(var->xres, 16) - 1; + if (val < 0x01 || val > 0x3f) + return -EINVAL; + + val = DIV_ROUND_UP(var->yres * var->xres * var->bits_per_pixel, 128); + val--; + if (val < 0x001 || val > 0x1fff) + return -EINVAL; + + var->transp.msb_right = 0; + var->transp.offset = 0; + var->transp.length = 0; + var->red.msb_right = 0; + var->red.offset = 0; + var->red.length = var->bits_per_pixel; + var->green = var->red; + var->blue = var->red; + var->grayscale = var->bits_per_pixel > 1; + + return 0; +} + +static int clps711x_fb_set_par(struct fb_info *info) +{ + struct clps711x_fb_info *cfb = info->par; + resource_size_t size; + u32 lcdcon, pps; + + size = (info->var.xres * info->var.yres * info->var.bits_per_pixel) / 8; + if (size > cfb->buffsize) + return -EINVAL; + + switch (info->var.bits_per_pixel) { + case 1: + info->fix.visual = FB_VISUAL_MONO01; + break; + case 2: + case 4: + info->fix.visual = FB_VISUAL_PSEUDOCOLOR; + break; + default: + return -EINVAL; + } + + info->fix.line_length = info->var.xres * info->var.bits_per_pixel / 8; + info->fix.smem_len = size; + + lcdcon = (info->var.xres * info->var.yres * + info->var.bits_per_pixel) / 128 - 1; + lcdcon |= ((info->var.xres / 16) - 1) << 13; + lcdcon |= (cfb->ac_prescale & 0x1f) << 25; + + pps = clk_get_rate(cfb->clk) / (PICOS2KHZ(info->var.pixclock) * 1000); + if (pps) + pps--; + lcdcon |= (pps & 0x3f) << 19; + + if (info->var.bits_per_pixel == 4) + lcdcon |= LCDCON_GSMD; + if (info->var.bits_per_pixel >= 2) + lcdcon |= LCDCON_GSEN; + + /* LCDCON must only be changed while the LCD is disabled */ + regmap_update_bits(cfb->syscon, SYSCON_OFFSET, SYSCON1_LCDEN, 0); + writel(lcdcon, cfb->base + CLPS711X_LCDCON); + regmap_update_bits(cfb->syscon, SYSCON_OFFSET, + SYSCON1_LCDEN, SYSCON1_LCDEN); + + return 0; +} + +static int clps711x_fb_blank(int blank, struct fb_info *info) +{ + /* Return happy */ + return 0; +} + +static struct fb_ops clps711x_fb_ops = { + .owner = THIS_MODULE, + .fb_setcolreg = clps711x_fb_setcolreg, + .fb_check_var = clps711x_fb_check_var, + .fb_set_par = clps711x_fb_set_par, + .fb_blank = clps711x_fb_blank, + .fb_fillrect = sys_fillrect, + .fb_copyarea = sys_copyarea, + .fb_imageblit = sys_imageblit, +}; + +static int clps711x_lcd_check_fb(struct lcd_device *lcddev, struct fb_info *fi) +{ + struct clps711x_fb_info *cfb = dev_get_drvdata(&lcddev->dev); + + return (!fi || fi->par == cfb) ? 1 : 0; +} + +static int clps711x_lcd_get_power(struct lcd_device *lcddev) +{ + struct clps711x_fb_info *cfb = dev_get_drvdata(&lcddev->dev); + + if (!IS_ERR_OR_NULL(cfb->lcd_pwr)) + return regulator_is_enabled(cfb->lcd_pwr); + + return 1; +} + +static int clps711x_lcd_set_power(struct lcd_device *lcddev, int power) +{ + struct clps711x_fb_info *cfb = dev_get_drvdata(&lcddev->dev); + + if (!IS_ERR_OR_NULL(cfb->lcd_pwr)) { + if (power) + return regulator_enable(cfb->lcd_pwr); + else + return regulator_disable(cfb->lcd_pwr); + } + + return 0; +} + +static struct lcd_ops clps711x_lcd_ops = { + .check_fb = clps711x_lcd_check_fb, + .get_power = clps711x_lcd_get_power, + .set_power = clps711x_lcd_set_power, +}; + +static int clps711x_fb_get_mode(struct platform_device *pdev) +{ + struct fb_info *info = platform_get_drvdata(pdev); + bool *pinvert = dev_get_platdata(&pdev->dev); + struct clps711x_fb_info *cfb = info->par; + unsigned long pixclk; + unsigned int val; + int ret; + + cfb->syscon = syscon_regmap_lookup_by_pdevname("syscon.1"); + if (IS_ERR(cfb->syscon)) + return PTR_ERR(cfb->syscon); + + ret = regmap_read(cfb->syscon, SYSCON_OFFSET, &val); + if (ret) + return ret; + + if (pinvert) + cfb->cmap_invert = *pinvert; + + /* Get mode from LCD if active */ + if (val & SYSCON1_LCDEN) { + val = readl(cfb->base + CLPS711X_LCDCON); + + switch (val & (LCDCON_GSMD | LCDCON_GSEN)) { + case LCDCON_GSMD | LCDCON_GSEN: + info->var.bits_per_pixel = 4; + break; + case LCDCON_GSEN: + info->var.bits_per_pixel = 2; + break; + default: + info->var.bits_per_pixel = 1; + break; + } + + cfb->mode.xres = (((val >> 13) & 0x3f) + 1) * 16; + cfb->mode.yres = (((val & 0x1fff) + 1) * 128) / + (cfb->mode.xres * info->var.bits_per_pixel); + cfb->ac_prescale = (val >> 25) & 0x1f; + pixclk = clk_get_rate(cfb->clk) / (((val >> 19) & 0x3f) + 1); + } else { + /* Fixed setup for existing users */ + info->var.bits_per_pixel = 4; + cfb->mode.xres = 640; + cfb->mode.yres = 240; + cfb->ac_prescale = 13; + pixclk = 10000000; + } + + cfb->mode.refresh = pixclk / (cfb->mode.xres * cfb->mode.yres); + cfb->mode.pixclock = KHZ2PICOS(pixclk / 1000); + + return 0; +} + +static int clps711x_fb_get_mode_dt(struct platform_device *pdev) +{ + struct device_node *disp, *np = pdev->dev.of_node; + struct fb_info *info = platform_get_drvdata(pdev); + struct clps711x_fb_info *cfb = info->par; + int ret; + + cfb->syscon = + syscon_regmap_lookup_by_compatible("cirrus,clps711x-syscon1"); + if (IS_ERR(cfb->syscon)) + return PTR_ERR(cfb->syscon); + + disp = of_parse_phandle(np, "display", 0); + if (!disp) { + dev_err(&pdev->dev, "No display defined\n"); + return -ENODATA; + } + + ret = of_get_fb_videomode(disp, &cfb->mode, OF_USE_NATIVE_MODE); + if (ret) + return ret; + + of_property_read_u32(disp, "ac-prescale", &cfb->ac_prescale); + cfb->cmap_invert = of_property_read_bool(disp, "cmap-invert"); + + return of_property_read_u32(disp, "bits-per-pixel", + &info->var.bits_per_pixel); +} + +static int clps711x_fb_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct clps711x_fb_info *cfb; + struct lcd_device *lcd; + struct fb_info *info; + struct resource *res; + int ret = -ENOENT; + u32 val; + + if (fb_get_options(CLPS711X_FB_NAME, NULL)) + return -ENODEV; + + info = framebuffer_alloc(sizeof(*cfb), dev); + if (!info) + return -ENOMEM; + + cfb = info->par; + platform_set_drvdata(pdev, info); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + goto out_fb_release; + cfb->base = devm_ioremap(dev, res->start, resource_size(res)); + if (!cfb->base) { + ret = -ENOMEM; + goto out_fb_release; + } + + info->fix.mmio_start = res->start; + info->fix.mmio_len = resource_size(res); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + info->screen_base = devm_ioremap_resource(dev, res); + if (IS_ERR(info->screen_base)) { + ret = PTR_ERR(info->screen_base); + goto out_fb_release; + } + + /* Physical address should be aligned to 256 MiB */ + if (res->start & 0x0fffffff) { + ret = -EINVAL; + goto out_fb_release; + } + + info->apertures = alloc_apertures(1); + if (!info->apertures) { + ret = -ENOMEM; + goto out_fb_release; + } + + cfb->buffsize = resource_size(res); + info->fix.smem_start = res->start; + info->apertures->ranges[0].base = info->fix.smem_start; + info->apertures->ranges[0].size = cfb->buffsize; + + cfb->clk = devm_clk_get(dev, NULL); + if (IS_ERR(cfb->clk)) { + ret = PTR_ERR(cfb->clk); + goto out_fb_release; + } + + ret = np ? clps711x_fb_get_mode_dt(pdev) : clps711x_fb_get_mode(pdev); + if (ret) + goto out_fb_release; + + /* Force disable LCD on any mismatch */ + if (info->fix.smem_start != (readb(cfb->base + CLPS711X_FBADDR) << 28)) + regmap_update_bits(cfb->syscon, SYSCON_OFFSET, + SYSCON1_LCDEN, 0); + + ret = regmap_read(cfb->syscon, SYSCON_OFFSET, &val); + if (ret) + goto out_fb_release; + + if (!(val & SYSCON1_LCDEN)) { + /* Setup start FB address */ + writeb(info->fix.smem_start >> 28, cfb->base + CLPS711X_FBADDR); + /* Clean FB memory */ + memset(info->screen_base, 0, cfb->buffsize); + } + + cfb->lcd_pwr = devm_regulator_get(dev, "lcd"); + if (PTR_ERR(cfb->lcd_pwr) == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; + goto out_fb_release; + } + + info->fbops = &clps711x_fb_ops; + info->flags = FBINFO_DEFAULT; + info->var.activate = FB_ACTIVATE_FORCE | FB_ACTIVATE_NOW; + info->var.height = -1; + info->var.width = -1; + info->var.vmode = FB_VMODE_NONINTERLACED; + info->fix.type = FB_TYPE_PACKED_PIXELS; + info->fix.accel = FB_ACCEL_NONE; + strlcpy(info->fix.id, CLPS711X_FB_NAME, sizeof(info->fix.id)); + fb_videomode_to_var(&info->var, &cfb->mode); + + ret = fb_alloc_cmap(&info->cmap, BIT(CLPS711X_FB_BPP_MAX), 0); + if (ret) + goto out_fb_release; + + ret = fb_set_var(info, &info->var); + if (ret) + goto out_fb_dealloc_cmap; + + ret = register_framebuffer(info); + if (ret) + goto out_fb_dealloc_cmap; + + lcd = devm_lcd_device_register(dev, "clps711x-lcd", dev, cfb, + &clps711x_lcd_ops); + if (!IS_ERR(lcd)) + return 0; + + ret = PTR_ERR(lcd); + unregister_framebuffer(info); + +out_fb_dealloc_cmap: + regmap_update_bits(cfb->syscon, SYSCON_OFFSET, SYSCON1_LCDEN, 0); + fb_dealloc_cmap(&info->cmap); + +out_fb_release: + framebuffer_release(info); + + return ret; +} + +static int clps711x_fb_remove(struct platform_device *pdev) +{ + struct fb_info *info = platform_get_drvdata(pdev); + struct clps711x_fb_info *cfb = info->par; + + regmap_update_bits(cfb->syscon, SYSCON_OFFSET, SYSCON1_LCDEN, 0); + + unregister_framebuffer(info); + fb_dealloc_cmap(&info->cmap); + framebuffer_release(info); + + return 0; +} + +static const struct of_device_id __maybe_unused clps711x_fb_dt_ids[] = { + { .compatible = "cirrus,clps711x-fb", }, + { } +}; +MODULE_DEVICE_TABLE(of, clps711x_fb_dt_ids); + +static struct platform_driver clps711x_fb_driver = { + .driver = { + .name = CLPS711X_FB_NAME, + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(clps711x_fb_dt_ids), + }, + .probe = clps711x_fb_probe, + .remove = clps711x_fb_remove, +}; +module_platform_driver(clps711x_fb_driver); + +MODULE_AUTHOR("Alexander Shiyan <shc_work@mail.ru>"); +MODULE_DESCRIPTION("Cirrus Logic CLPS711X FB driver"); +MODULE_LICENSE("GPL");
This adds support for the framebuffer available in the Cirrus Logic CLPS711X CPUs. FB features: - 1-2-4 bits per pixel. - Programmable panel size to a maximum of 1024x256 at 4 bps. - Relocatible Frame Buffer (SRAM or SDRAM). - Programmable refresh rates. - 16 gray scale values. This new driver supports usage with devicetree and as a general change it removes last user of <mach/hardware.h> for CLPS711X targets, so this subarch will fully prepared to switch to multiplatform. The driver have been tested with custom board equipped Cirrus Logic EP7312 in DT and non-DT mode. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- drivers/video/fbdev/clps711x-fb.c | 456 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 456 insertions(+) create mode 100644 drivers/video/fbdev/clps711x-fb.c