| Message ID | 1422712065-9403-6-git-send-email-haokexin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Michael Ellerman |
| Headers | show |
Hi Kevin, On Sat, 31 Jan 2015 21:47:35 +0800 Kevin Hao <haokexin@gmail.com> wrote: > > The OF functionality has moved to a common place and be used by many > archs. So we don't need to depend on PPC_OF option any more. This is > a preparation for killing PPC_OF. I suspect that you want to do the PPC_OF -> PPC conversion on this file rather than just removing PPC_OF uses. > diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c > index aae10ce74f14..91a80bb8f988 100644 > --- a/drivers/video/fbdev/imsttfb.c > +++ b/drivers/video/fbdev/imsttfb.c > @@ -1470,7 +1470,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > unsigned long addr, size; > struct imstt_par *par; > struct fb_info *info; > -#ifdef CONFIG_PPC_OF > struct device_node *dp; I see no way in this file for struct device_node to be defined (especially if CONFIG_PPC is not set). of.h may be included implicitly, but that is very dependent on the architecture and CONFIG_ options. > dp = pci_device_to_OF_node(pdev); > @@ -1478,7 +1477,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > printk(KERN_INFO "%s: OF name %s\n",__func__, dp->name); > else > printk(KERN_ERR "imsttfb: no OF node for pci device\n"); > -#endif /* CONFIG_PPC_OF */ This will emit the above error if CONFIG_OF is not set whereas in the past it would not.
On Sun, Feb 01, 2015 at 01:44:33PM +1100, Stephen Rothwell wrote: > Hi Kevin, > > On Sat, 31 Jan 2015 21:47:35 +0800 Kevin Hao <haokexin@gmail.com> wrote: > > > > The OF functionality has moved to a common place and be used by many > > archs. So we don't need to depend on PPC_OF option any more. This is > > a preparation for killing PPC_OF. > > I suspect that you want to do the PPC_OF -> PPC conversion on this file > rather than just removing PPC_OF uses. That was my first thought, but the codes protected by the PPC_OF seem not ppc specific and should be safe for other archs which also support OF. So I drop the PPC_OF completely. Did I miss something? > > > diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c > > index aae10ce74f14..91a80bb8f988 100644 > > --- a/drivers/video/fbdev/imsttfb.c > > +++ b/drivers/video/fbdev/imsttfb.c > > @@ -1470,7 +1470,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > > unsigned long addr, size; > > struct imstt_par *par; > > struct fb_info *info; > > -#ifdef CONFIG_PPC_OF > > struct device_node *dp; > > I see no way in this file for struct device_node to be defined > (especially if CONFIG_PPC is not set). of.h may be included > implicitly, but that is very dependent on the architecture and CONFIG_ > options. This do pass the build test for the non-OF archs, such as x86. But your concerns sound pretty reasonable, so I will explicitly include of.h. > > > dp = pci_device_to_OF_node(pdev); > > @@ -1478,7 +1477,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > > printk(KERN_INFO "%s: OF name %s\n",__func__, dp->name); > > else > > printk(KERN_ERR "imsttfb: no OF node for pci device\n"); > > -#endif /* CONFIG_PPC_OF */ > > This will emit the above error if CONFIG_OF is not set whereas in the > past it would not. How about change it to: if (IS_ENABLED(CONFIG_OF)) printk(KERN_ERR "imsttfb: no OF node for pci device\n"); Thanks, Kevin
On Sun, Feb 01, 2015 at 01:51:50PM +0800, Kevin Hao wrote: > > > diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c > > > index aae10ce74f14..91a80bb8f988 100644 > > > --- a/drivers/video/fbdev/imsttfb.c > > > +++ b/drivers/video/fbdev/imsttfb.c > > > @@ -1470,7 +1470,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > > > unsigned long addr, size; > > > struct imstt_par *par; > > > struct fb_info *info; > > > -#ifdef CONFIG_PPC_OF > > > struct device_node *dp; > > > > I see no way in this file for struct device_node to be defined > > (especially if CONFIG_PPC is not set). of.h may be included > > implicitly, but that is very dependent on the architecture and CONFIG_ > > options. > > This do pass the build test for the non-OF archs, such as x86. But your > concerns sound pretty reasonable, so I will explicitly include of.h. I took a second look at this. It seems that there is a declaration of struct device_node in linux/device.h and there is also no access to the member of device_node in this driver. So we are safe to not include of.h here. That is also why I didn't get the build failure for the non-OF archs. :-) Thanks, Kevin
Hi Kevin, On Sun, 1 Feb 2015 13:51:50 +0800 Kevin Hao <haokexin@gmail.com> wrote: > > That was my first thought, but the codes protected by the PPC_OF seem not > ppc specific and should be safe for other archs which also support OF. So I > drop the PPC_OF completely. Did I miss something? Ah, ok. > > > dp = pci_device_to_OF_node(pdev); > > > @@ -1478,7 +1477,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > > > printk(KERN_INFO "%s: OF name %s\n",__func__, dp->name); > > > else > > > printk(KERN_ERR "imsttfb: no OF node for pci device\n"); > > > -#endif /* CONFIG_PPC_OF */ > > > > This will emit the above error if CONFIG_OF is not set whereas in the > > past it would not. > > How about change it to: > if (IS_ENABLED(CONFIG_OF)) > printk(KERN_ERR "imsttfb: no OF node for pci device\n"); Looks good.
Hi Kevin, On Tue, 3 Feb 2015 10:20:02 +0800 Kevin Hao <haokexin@gmail.com> wrote: > > I took a second look at this. It seems that there is a declaration of > struct device_node in linux/device.h and there is also no access to the > member of device_node in this driver. So we are safe to not include of.h here. > That is also why I didn't get the build failure for the non-OF archs. :-) Right. Seems ok, then.
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c index aae10ce74f14..91a80bb8f988 100644 --- a/drivers/video/fbdev/imsttfb.c +++ b/drivers/video/fbdev/imsttfb.c @@ -1470,7 +1470,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) unsigned long addr, size; struct imstt_par *par; struct fb_info *info; -#ifdef CONFIG_PPC_OF struct device_node *dp; dp = pci_device_to_OF_node(pdev); @@ -1478,7 +1477,6 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) printk(KERN_INFO "%s: OF name %s\n",__func__, dp->name); else printk(KERN_ERR "imsttfb: no OF node for pci device\n"); -#endif /* CONFIG_PPC_OF */ info = framebuffer_alloc(sizeof(struct imstt_par), &pdev->dev); @@ -1501,11 +1499,9 @@ static int imsttfb_probe(struct pci_dev *pdev, const struct pci_device_id *ent) switch (pdev->device) { case PCI_DEVICE_ID_IMS_TT128: /* IMS,tt128mbA */ par->ramdac = IBM; -#ifdef CONFIG_PPC_OF if (dp && ((strcmp(dp->name, "IMS,tt128mb8") == 0) || (strcmp(dp->name, "IMS,tt128mb8A") == 0))) par->ramdac = TVP; -#endif /* CONFIG_PPC_OF */ break; case PCI_DEVICE_ID_IMS_TT3D: /* IMS,tt3d */ par->ramdac = TVP;
The OF functionality has moved to a common place and be used by many archs. So we don't need to depend on PPC_OF option any more. This is a preparation for killing PPC_OF. Signed-off-by: Kevin Hao <haokexin@gmail.com> --- drivers/video/fbdev/imsttfb.c | 4 ---- 1 file changed, 4 deletions(-)