Re: [PATCH v2] Fix loading of module radeonfb on PowerMac

2016-11-15 Thread Tomi Valkeinen 
Hi,

On 14/11/16 21:59, Mathieu Malaterre wrote:
> When the linux kernel is build with (typical kernel ship with Debian
> installer):
> 
> CONFIG_FB_OF=y
> CONFIG_VT_HW_CONSOLE_BINDING=y
> CONFIG_FB_RADEON=m
> 
> The offb driver takes precedence over module radeonfb. It is then
> impossible to load the module, error reported is:
> 
> [   96.551486] radeonfb :00:10.0: enabling device (0006 -> 0007)
> [   96.551526] radeonfb :00:10.0: BAR 0: can't reserve [mem 
> 0x9800-0x9fff pref]
> [   96.551531] radeonfb (:00:10.0): cannot request region 0.
> [   96.551545] radeonfb: probe of :00:10.0 failed with error -16
> 
> This patch reproduce the behavior of the module radeon, so as to make it
> possible to load radeonfb when offb is first loaded.
> 
> It should be noticed that `offb_destroy` is never called which explain the
> need to skip error detection on the radeon side.
> 
> Signed-off-by: Mathieu Malaterre 
> Link: https://bugs.debian.org/826629#57
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=119741
> Suggested-by: Lennart Sorensen 
> ---

Please always have a changelog in patch new revisions. For individual
patches, you can insert the changes here.

>  drivers/video/fbdev/aty/radeon_base.c | 26 ++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/video/fbdev/aty/radeon_base.c 
> b/drivers/video/fbdev/aty/radeon_base.c
> index 218339a..84d634b 100644
> --- a/drivers/video/fbdev/aty/radeon_base.c
> +++ b/drivers/video/fbdev/aty/radeon_base.c
> @@ -2259,6 +2259,22 @@ static struct bin_attribute edid2_attr = {
>   .read   = radeon_show_edid2,
>  };
>  
> +static int radeon_kick_out_firmware_fb(struct pci_dev *pdev)
> +{
> + struct apertures_struct *ap;
> +
> + ap = alloc_apertures(1);
> + if (!ap)
> + return -ENOMEM;
> +
> + ap->ranges[0].base = pci_resource_start(pdev, 0);
> + ap->ranges[0].size = pci_resource_len(pdev, 0);
> +
> + remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false);
> + kfree(ap);
> +
> + return 0;
> +}
>  
>  static int radeonfb_pci_register(struct pci_dev *pdev,
>const struct pci_device_id *ent)
> @@ -2314,19 +2330,27 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>   rinfo->fb_base_phys = pci_resource_start (pdev, 0);
>   rinfo->mmio_base_phys = pci_resource_start (pdev, 2);
>  
> + ret = radeon_kick_out_firmware_fb(pdev);
> + if (ret)
> + return ret;
> +
>   /* request the mem regions */
>   ret = pci_request_region(pdev, 0, "radeonfb framebuffer");
>   if (ret < 0) {
>   printk( KERN_ERR "radeonfb (%s): cannot request region 0.\n",
>   pci_name(rinfo->pdev));
> +#ifndef CONFIG_PPC
>   goto err_release_fb;
> +#endif

If this is not a problem on PPC, the kernel shouldn't print an error either.

>   }
>  
>   ret = pci_request_region(pdev, 2, "radeonfb mmio");
>   if (ret < 0) {
>   printk( KERN_ERR "radeonfb (%s): cannot request region 2.\n",
>   pci_name(rinfo->pdev));
> +#ifndef CONFIG_PPC
>   goto err_release_pci0;
> +#endif

Same here.

>   }
>  
>   /* map the regions */
> @@ -2511,10 +2535,12 @@ static int radeonfb_pci_register(struct pci_dev *pdev,
>   iounmap(rinfo->mmio_base);
>  err_release_pci2:
>   pci_release_region(pdev, 2);
> +#ifndef CONFIG_PPC
>  err_release_pci0:
>   pci_release_region(pdev, 0);
>  err_release_fb:
>  framebuffer_release(info);
> +#endif
>  err_disable:
>  err_out:
>   return ret;
> 

So I don't quite follow what's going on here. Why the CONFIG_PPC
conditionals? Is this problem only with OFFB and only with PPC?

And I think the code itself should have comments on this rather strange
behavior: the driver fails to get HW resources, but decides to ignore
the failure on PPC.

 Tomi



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Fix loading of module radeonfb on PowerMac

2016-11-14 Thread Tomi Valkeinen 
On 08/10/16 15:09, Mathieu Malaterre wrote:
> When the linux kernel is build with (typical kernel ship with Debian
> installer):
> 
> CONFIG_FB_OF=y
> CONFIG_VT_HW_CONSOLE_BINDING=y
> CONFIG_FB_RADEON=m
> 
> The offb driver takes precedence over module radeonfb. It is then
> impossible to load the module, error reported is:
> 
> [   96.551486] radeonfb :00:10.0: enabling device (0006 -> 0007)
> [   96.551526] radeonfb :00:10.0: BAR 0: can't reserve [mem 
> 0x9800-0x9fff pref]
> [   96.551531] radeonfb (:00:10.0): cannot request region 0.
> [   96.551545] radeonfb: probe of :00:10.0 failed with error -16
> 
> This patch reproduce the behavior of the module radeon, so as to make it
> possible to load radeonfb when offb is first loaded.
> 
> It should be noticed that `offb_destroy` is never called which explain the
> need to skip error detection on the radeon side.
> 
> Signed-off-by: Mathieu Malaterre 
> Link: https://bugs.debian.org/826629#57
> Suggested-by: Lennart Sorensen 
> ---
>  drivers/video/fbdev/aty/radeon_base.c | 22 --
>  1 file changed, 20 insertions(+), 2 deletions(-)

Thanks, queued for 4.9 fixes.

 Tomi



signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Fix loading of module radeonfb on PowerMac

2016-11-02 Thread Tomi Valkeinen 
On 08/10/16 15:09, Mathieu Malaterre wrote:
> When the linux kernel is build with (typical kernel ship with Debian
> installer):
> 
> CONFIG_FB_OF=y
> CONFIG_VT_HW_CONSOLE_BINDING=y
> CONFIG_FB_RADEON=m
> 
> The offb driver takes precedence over module radeonfb. It is then
> impossible to load the module, error reported is:
> 
> [   96.551486] radeonfb :00:10.0: enabling device (0006 -> 0007)
> [   96.551526] radeonfb :00:10.0: BAR 0: can't reserve [mem 
> 0x9800-0x9fff pref]
> [   96.551531] radeonfb (:00:10.0): cannot request region 0.
> [   96.551545] radeonfb: probe of :00:10.0 failed with error -16
> 
> This patch reproduce the behavior of the module radeon, so as to make it
> possible to load radeonfb when offb is first loaded.
> 
> It should be noticed that `offb_destroy` is never called which explain the
> need to skip error detection on the radeon side.

Why is that? It sounds rather bad if two drivers claim the same resources.

 Tomi



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2] video: fbdev: fsl: Fix the sleep function for FSL DIU module

2015-09-24 Thread Tomi Valkeinen 


On 24/09/15 13:02, Wang Dongsheng wrote:
> Hi Tomi,
> 
> Could you apply this patch?
> 
>>> For deep sleep, the diu module will power off, when wake up from the
>>> deep sleep, the registers need to be reinitialized.
>>>
>>> Signed-off-by: Jason Jin
>>> Signed-off-by: Wang Dongsheng
>>
>> Acked-by: Timur Tabi 

Thanks, queued for 4.3 fixes.

 Tomi



signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 03/15] fbdev: aty128fb: replace PPC_OF with PPC

2015-03-11 Thread Tomi Valkeinen 
On 11/03/15 06:38, Kevin Hao wrote:
 On Tue, Mar 10, 2015 at 02:23:12PM +0200, Tomi Valkeinen wrote:
 I just sent out a v2 [1] a few hours earlier with some minor updates. We 
 plan
 to merge this patch series via the powerpc tree in 4.1 cycle if I can 
 collect
 all the acks from the corresponding driver maintainers.

 Fbdev changes look ok to me.
 
 Hi Tomi,
 
 I assume that I can add a Acked-by: Tomi Valkeinen tomi.valkei...@ti.com
 for all the following patches, right?
   http://patchwork.ozlabs.org/patch/443890/
   http://patchwork.ozlabs.org/patch/443891/
   http://patchwork.ozlabs.org/patch/443892/
   http://patchwork.ozlabs.org/patch/443893/
   http://patchwork.ozlabs.org/patch/443894/
   http://patchwork.ozlabs.org/patch/443895/
   http://patchwork.ozlabs.org/patch/443897/

Yes, that's right. Sorry for not being clear on the ack.

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 03/15] fbdev: aty128fb: replace PPC_OF with PPC

2015-03-10 Thread Tomi Valkeinen 
On 27/02/15 03:05, Kevin Hao wrote:
 On Fri, Feb 27, 2015 at 11:11:15AM +1100, Benjamin Herrenschmidt wrote:
 On Sat, 2015-01-31 at 21:47 +0800, Kevin Hao wrote:
 The PPC_OF is a ppc specific option which is used to mean that the
 firmware device tree access functions are available. Since all the
 ppc platforms have a device tree, it is aways set to 'y' for ppc.
 So it makes no sense to keep a such option in the current kernel.
 Replace it with PPC.

 Signed-off-by: Kevin Hao haoke...@gmail.com

 For this and generally the whole series,

 Acked-by: Benjamin Herrenschmidt b...@kernel.crashing.org
 
 Thanks Ben.
 

 Which tree do we expect this to go through ?
 
 I just sent out a v2 [1] a few hours earlier with some minor updates. We plan
 to merge this patch series via the powerpc tree in 4.1 cycle if I can collect
 all the acks from the corresponding driver maintainers.

Fbdev changes look ok to me.

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

2014-10-27 Thread Tomi Valkeinen 
On 27/10/14 13:59, Jani Nikula wrote:

 While doing 'depends on' instead of 'select' is an easy fix for this,
 I do dislike it quite a bit. It's a major pain to go around the kernel
 config, trying to find all the dependencies that a particular driver
 wants. If I need fb-foobar, I should just be able to enable it, instead
 of first searching and selecting its minor dependencies individually.
 
 Agreed, but I don't think that's specific to this patch.

Well, no, the generic problem is not specific to this patch, but we can
avoid the issue with proper use of 'select' (at least in some cases),
which is specific to this patch.

 So, not a NACK, but a isn't there an another way to fix this?.
 
 I think the real answer would be to fix kconfig to also show menu items
 whose dependencies are not met, and then recursively enabling the
 dependencies when the item is enabled. Beyond my scope.
 
 Looking at backlight... BACKLIGHT_LCD_SUPPORT seems to be a meta
 option, it only enables a Kconfig submenu.

 So I think we could just remove the whole BACKLIGHT_LCD_SUPPORT option.
 But if we do that, all the items in drivers/video/backlight/Kconfig with
 default 'y' or 'm' would get enabled by default, so I think we should
 remove the 'default's from that file. That makes sense in any case, as I
 don't see why HP Jornada 700 series LCD Driver should be default y.

 BACKLIGHT_CLASS_DEVICE doesn't depend on anything except
 BACKLIGHT_LCD_SUPPORT, so after removing BACKLIGHT_LCD_SUPPORT it should
 be safe to 'select' BACKLIGHT_CLASS_DEVICE.

 BACKLIGHT_CLASS_DEVICE could be made a hidden option, and the drivers in
 drivers/video/backlight/Kconfig which are under BACKLIGHT_CLASS_DEVICE
 could be made to select BACKLIGHT_CLASS_DEVICE instead.
 
 I think it should be possible to choose between y and m when it's

If I'm not mistaken, if CONFIG_FOO is 'm', and it 'select's CONFIG_BAR,
and CONFIG_BAR is tristate, then CONFIG_BAR will be set to 'm'.

 selected, and it should be possible to enable it when it's not selected
 by any drivers. I'm not sure a hidden option is good for that.

Why would you want to enable it if no one uses it? Does
BACKLIGHT_CLASS_DEVICE enable something even if no driver uses it?

 That doesn't exactly fix anything, but I think it makes sense as
 BACKLIGHT_CLASS_DEVICE is something that's selected from all around the
 kernel, so it should be a selectable library instead of a Kconfig menu
 option.
 
 At least for drm/i915 BACKLIGHT_CLASS_DEVICE is an option. We use it
 if it's enabled, but we are just fine if it's not. I've learned the way
 to express that is
 
   depends on BACKLIGHT_CLASS_DEVICE || BACKLIGHT_CLASS_DEVICE=n
 
 but I don't think there's a way to express that in terms of select, is
 there? The dependency above guarantees there's no DRM_I915=y and
 BACKLIGHT_CLASS_DEVICE=m combo which would fail. And this, btw, is where
 this whole patch got started, as select didn't handle that properly.

If backlight support is considered an option for drm/i915, then I think
there should be a Kconfig option for i915 to enable backlight support,
which in turn selects BACKLIGHT_CLASS_DEVICE. And that select will force
BACKLIGHT_CLASS_DEVICE to be built-in if drm/i915 is built-in.

Oh, but it doesn't work optimally with modules. The new option needed
for that would be boolean, so BACKLIGHT_CLASS_DEVICE would always be
either y or n. Sigh...

 I didn't look at the ACPI_VIDEO side, so no idea how messy that is.
 
 Basically it's another dependency on BACKLIGHT_CLASS_DEVICE. I can only
 imagine trying to solve this problem with select is going to end up in
 recursive dependencies that spread out and need changing about as wide
 as this patch.

If ACPI_VIDEO uses select to enable BACKLIGHT_CLASS_DEVICE, then, I
think, selecting ACPI_VIDEO will also select BACKLIGHT_CLASS_DEVICE. So
I don't right away see any recursive dependencies. Or what did you have
in mind?

 In the end, I agree with the problem you have with this patch, but yet I
 think it's the right thing to do in terms of expressing the
 dependencies.

Well, dri/i915 doesn't exactly depend on backlight, if I understood you
correctly. Instead, backlight is an option for dri/i915, and you kind of
hack it to be implemented with that 'depends on BACKLIGHT_CLASS_DEVICE
|| BACKLIGHT_CLASS_DEVICE=n'.

I guess it's debatable whether drivers should automatically use features
in the kernel if they happen to be enabled in the Kconfig, or should
they be individually enabled for that driver. I personally like the
latter option, as it allows more precise control, but it probably also
depends on the feature in question.

I also think the 'depends on BACKLIGHT_CLASS_DEVICE ||
BACKLIGHT_CLASS_DEVICE=n' pattern is quite... interesting (i.e. sounds
like a hack to me =).

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org

Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

2014-10-23 Thread Tomi Valkeinen 
On 23/10/14 11:10, Daniel Vetter wrote:

 If we want to make BACKLIGHT_CLASS_DEVICE into a library thing then I
 guess we could do that, but we must then also drag it out of all the other
 meta options to make sure it's always available. No need I think to ditch

BACKLIGHT_CLASS_DEVICE only depends on HAS_IOMEM and
BACKLIGHT_LCD_SUPPORT so there are no other meta options to avoid.
HAS_IOMEM comes from drivers/video/Kconfig's Graphics support, and I
guess we can ignore it.

 the entire BACKLIGHT_LCD_SUPPORT meta option. And then everyone could
 select it.

I don't quite understand what purpose does BACKLIGHT_LCD_SUPPORT serve.
It doesn't enable any code, it just opens up new Kconfig options. Why
can't the Kconfig options be always available? It's just another option
to 'select', without any reason I can see.

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] drivers: depend on instead of select BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO

2014-10-22 Thread Tomi Valkeinen 
On 18/10/14 00:13, Jani Nikula wrote:
 Documentation/kbuild/kconfig-language.txt warns to use select with care,
 and in general use select only for non-visible symbols and for symbols
 with no dependencies, because select will force a symbol to a value
 without visiting the dependencies.
 
 Select has become particularly problematic, interdependently, with the
 BACKLIGHT_CLASS_DEVICE and ACPI_VIDEO configs. For example:
 
 scripts/kconfig/conf --randconfig Kconfig
 KCONFIG_SEED=0x48312B00
 warning: (DRM_RADEON  DRM_NOUVEAU  DRM_I915  DRM_GMA500 
 DRM_SHMOBILE  DRM_TILCDC  FB_BACKLIGHT  FB_MX3  USB_APPLEDISPLAY
  FB_OLPC_DCON  ASUS_LAPTOP  SONY_LAPTOP  THINKPAD_ACPI 
 EEEPC_LAPTOP  ACPI_CMPC  SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE
 which has unmet direct dependencies (HAS_IOMEM  BACKLIGHT_LCD_SUPPORT)
 warning: (DRM_RADEON  DRM_NOUVEAU  DRM_I915  DRM_GMA500 
 DRM_SHMOBILE  DRM_TILCDC  FB_BACKLIGHT  FB_MX3  USB_APPLEDISPLAY
  FB_OLPC_DCON  ASUS_LAPTOP  SONY_LAPTOP  THINKPAD_ACPI 
 EEEPC_LAPTOP  ACPI_CMPC  SAMSUNG_Q10) selects BACKLIGHT_CLASS_DEVICE
 which has unmet direct dependencies (HAS_IOMEM  BACKLIGHT_LCD_SUPPORT)
 
 With tristates it's possible to end up selecting FOO=y depending on
 BAR=m in the config, which gets discovered at build time, not config
 time, like reported in the thread referenced below.
 
 Do the following to fix the dependencies:
 
 * Depend on instead of select BACKLIGHT_CLASS_DEVICE everywhere. Drop
   select BACKLIGHT_LCD_SUPPORT in such cases, as it's a dependency of
   BACKLIGHT_CLASS_DEVICE.
 
 * Remove config FB_BACKLIGHT altogether, and replace it with a
   dependency on BACKLIGHT_CLASS_DEVICE. All configs selecting
   FB_BACKLIGHT select or depend on FB anyway, so we can simplify.
 
 * Depend on (ACPI  ACPI_VIDEO) || ACPI=n in several places instead of
   selecting ACPI_VIDEO and a number of its dependencies if ACPI is
   enabled. This is tied to backlight, as ACPI_VIDEO depends on
   BACKLIGHT_CLASS_DEVICE.
 
 * Replace a couple of select INPUT/VT with depends as it seemed to be
   necessary.

While doing 'depends on' instead of 'select' is an easy fix for this,
I do dislike it quite a bit. It's a major pain to go around the kernel
config, trying to find all the dependencies that a particular driver
wants. If I need fb-foobar, I should just be able to enable it, instead
of first searching and selecting its minor dependencies individually.

So, not a NACK, but a isn't there an another way to fix this?.

Looking at backlight... BACKLIGHT_LCD_SUPPORT seems to be a meta
option, it only enables a Kconfig submenu.

So I think we could just remove the whole BACKLIGHT_LCD_SUPPORT option.
But if we do that, all the items in drivers/video/backlight/Kconfig with
default 'y' or 'm' would get enabled by default, so I think we should
remove the 'default's from that file. That makes sense in any case, as I
don't see why HP Jornada 700 series LCD Driver should be default y.

BACKLIGHT_CLASS_DEVICE doesn't depend on anything except
BACKLIGHT_LCD_SUPPORT, so after removing BACKLIGHT_LCD_SUPPORT it should
be safe to 'select' BACKLIGHT_CLASS_DEVICE.

BACKLIGHT_CLASS_DEVICE could be made a hidden option, and the drivers in
drivers/video/backlight/Kconfig which are under BACKLIGHT_CLASS_DEVICE
could be made to select BACKLIGHT_CLASS_DEVICE instead.

That doesn't exactly fix anything, but I think it makes sense as
BACKLIGHT_CLASS_DEVICE is something that's selected from all around the
kernel, so it should be a selectable library instead of a Kconfig menu
option.

I didn't look at the ACPI_VIDEO side, so no idea how messy that is.

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] video: valkyriefb: Fix unused variable warning in set_valkyrie_clock()

2014-09-30 Thread Tomi Valkeinen 
On 14/09/14 12:48, Geert Uytterhoeven wrote:
 If CONFIG_ADB_CUDA=n:
 
 drivers/video/fbdev/valkyriefb.c: In function ‘set_valkyrie_clock’:
 drivers/video/fbdev/valkyriefb.c:267: warning: unused variable ‘i’
 drivers/video/fbdev/valkyriefb.c:266: warning: unused variable ‘req’
 
 Move the variable declarations inside the existing #ifdef section to fix
 this.
 
 Signed-off-by: Geert Uytterhoeven ge...@linux-m68k.org
 ---
  drivers/video/fbdev/valkyriefb.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/drivers/video/fbdev/valkyriefb.c 
 b/drivers/video/fbdev/valkyriefb.c
 index 97cb9bd1d1dd..b6ed09f16355 100644
 --- a/drivers/video/fbdev/valkyriefb.c
 +++ b/drivers/video/fbdev/valkyriefb.c
 @@ -263,10 +263,10 @@ static inline int valkyrie_vram_reqd(int video_mode, 
 int color_mode)
  
  static void set_valkyrie_clock(unsigned char *params)
  {
 +#ifdef CONFIG_ADB_CUDA
   struct adb_request req;
   int i;
  
 -#ifdef CONFIG_ADB_CUDA
   for (i = 0; i  3; ++i) {
   cuda_request(req, NULL, 5, CUDA_PACKET, CUDA_GET_SET_IIC,
0x50, i + 1, params[i]);
 

Thanks, queued for 3.18.

 Tomi




signature.asc
Description: OpenPGP digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev