| Message ID | 20191120095716.26628-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/5] fbtft: Make sure string is NULL terminated | expand |
Den 20.11.2019 10.57, skrev Andy Shevchenko: > First of all there is no need to guard GPIO request by CONFIG_OF. > It works for everybody independently on resource provider. While here, > rename the function to reflect the above. > > Moreover, since we have a global dependency to OF, the rest of > conditional compilation is no-op, i.e. it's always be true. > > Due to above drop useless #ifdef CONFIG_OF and therefore dead code. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/staging/fbtft/fbtft-core.c | 19 ++----------------- > 1 file changed, 2 insertions(+), 17 deletions(-) > > diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c <snip> > @@ -1184,17 +1176,10 @@ static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) > pdata->display.backlight = 1; > if (of_find_property(node, "init", NULL)) > pdata->display.fbtftops.init_display = fbtft_init_display_dt; > - pdata->display.fbtftops.request_gpios = fbtft_request_gpios_dt; > + pdata->display.fbtftops.request_gpios = fbtft_request_gpios; You can ditch the .request_gpios callback and call fbtft_request_gpios() directly in fbtft_register_framebuffer(). That will make it safe to drop the OF dependency, otherwise .request_gpios will be NULL in the non-DT case. This is one of the bugs that follwed the gpio refactoring. You can also ditch the .request_gpios_match callback if you want, it isn't called anymore (it is set in fb_agm1264k-fl). Noralf. > > return pdata; > } > -#else > -static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) > -{ > - dev_err(dev, "Missing platform data\n"); > - return ERR_PTR(-EINVAL); > -} > -#endif > > /** > * fbtft_probe_common() - Generic device probe() helper function >
Den 20.11.2019 15.43, skrev Noralf Trønnes: > > > Den 20.11.2019 10.57, skrev Andy Shevchenko: >> First of all there is no need to guard GPIO request by CONFIG_OF. >> It works for everybody independently on resource provider. While here, >> rename the function to reflect the above. >> >> Moreover, since we have a global dependency to OF, the rest of >> conditional compilation is no-op, i.e. it's always be true. >> >> Due to above drop useless #ifdef CONFIG_OF and therefore dead code. >> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/staging/fbtft/fbtft-core.c | 19 ++----------------- >> 1 file changed, 2 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c > > <snip> > >> @@ -1184,17 +1176,10 @@ static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) >> pdata->display.backlight = 1; >> if (of_find_property(node, "init", NULL)) >> pdata->display.fbtftops.init_display = fbtft_init_display_dt; >> - pdata->display.fbtftops.request_gpios = fbtft_request_gpios_dt; >> + pdata->display.fbtftops.request_gpios = fbtft_request_gpios; > > You can ditch the .request_gpios callback and call fbtft_request_gpios() > directly in fbtft_register_framebuffer(). That will make it safe to drop > the OF dependency, otherwise .request_gpios will be NULL in the non-DT > case. This is one of the bugs that follwed the gpio refactoring. Really difficult to read this fbtft code (that I wrote...). The NULL deref can only happen when dev->platform_data is set. That can't happen, in mainline at least, now that fbtft_device is gone. > > You can also ditch the .request_gpios_match callback if you want, it > isn't called anymore (it is set in fb_agm1264k-fl). > > Noralf. > >> >> return pdata; >> } >> -#else >> -static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) >> -{ >> - dev_err(dev, "Missing platform data\n"); >> - return ERR_PTR(-EINVAL); >> -} >> -#endif >> >> /** >> * fbtft_probe_common() - Generic device probe() helper function >> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Wed, Nov 20, 2019 at 04:04:17PM +0100, Noralf Trønnes wrote: > Den 20.11.2019 15.43, skrev Noralf Trønnes: > > Den 20.11.2019 10.57, skrev Andy Shevchenko: > >> First of all there is no need to guard GPIO request by CONFIG_OF. > >> It works for everybody independently on resource provider. While here, > >> rename the function to reflect the above. > >> > >> Moreover, since we have a global dependency to OF, the rest of > >> conditional compilation is no-op, i.e. it's always be true. > >> > >> Due to above drop useless #ifdef CONFIG_OF and therefore dead code. > >> > >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > >> --- > >> drivers/staging/fbtft/fbtft-core.c | 19 ++----------------- > >> 1 file changed, 2 insertions(+), 17 deletions(-) > >> > >> diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c > > > > <snip> > > > >> @@ -1184,17 +1176,10 @@ static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) > >> pdata->display.backlight = 1; > >> if (of_find_property(node, "init", NULL)) > >> pdata->display.fbtftops.init_display = fbtft_init_display_dt; > >> - pdata->display.fbtftops.request_gpios = fbtft_request_gpios_dt; > >> + pdata->display.fbtftops.request_gpios = fbtft_request_gpios; > > > > You can ditch the .request_gpios callback and call fbtft_request_gpios() > > directly in fbtft_register_framebuffer(). That will make it safe to drop > > the OF dependency, otherwise .request_gpios will be NULL in the non-DT > > case. This is one of the bugs that follwed the gpio refactoring. > > Really difficult to read this fbtft code (that I wrote...). > The NULL deref can only happen when dev->platform_data is set. That > can't happen, in mainline at least, now that fbtft_device is gone. Hmm... If I read code correctly this patch doesn't change this logic. We have non-NULL ->request_gpios() in case of pdata != NULL if and only if supplier gives it to us. The above assignment happens only for DT case (fbtft_properties_read() is guarded against non-DT, okay non-fwnode, cases). > > You can also ditch the .request_gpios_match callback if you want, it > > isn't called anymore (it is set in fb_agm1264k-fl). I guess both improvements can be done later since they are not affecting the logic in this series.
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 2122c4407bdb..ff8cb6670ea1 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -70,7 +70,6 @@ void fbtft_dbg_hex(const struct device *dev, int groupsize, } EXPORT_SYMBOL(fbtft_dbg_hex); -#ifdef CONFIG_OF static int fbtft_request_one_gpio(struct fbtft_par *par, const char *name, int index, struct gpio_desc **gpiop) @@ -92,14 +91,11 @@ static int fbtft_request_one_gpio(struct fbtft_par *par, return ret; } -static int fbtft_request_gpios_dt(struct fbtft_par *par) +static int fbtft_request_gpios(struct fbtft_par *par) { int i; int ret; - if (!par->info->device->of_node) - return -EINVAL; - ret = fbtft_request_one_gpio(par, "reset", 0, &par->gpio.reset); if (ret) return ret; @@ -135,7 +131,6 @@ static int fbtft_request_gpios_dt(struct fbtft_par *par) return 0; } -#endif #ifdef CONFIG_FB_BACKLIGHT static int fbtft_backlight_update_status(struct backlight_device *bd) @@ -898,7 +893,6 @@ int fbtft_unregister_framebuffer(struct fb_info *fb_info) } EXPORT_SYMBOL(fbtft_unregister_framebuffer); -#ifdef CONFIG_OF /** * fbtft_init_display_dt() - Device Tree init_display() function * @par: Driver data @@ -977,7 +971,6 @@ static int fbtft_init_display_dt(struct fbtft_par *par) return 0; } -#endif /** * fbtft_init_display() - Generic init_display() function @@ -1138,7 +1131,6 @@ static int fbtft_verify_gpios(struct fbtft_par *par) return 0; } -#ifdef CONFIG_OF /* returns 0 if the property is not present */ static u32 fbtft_of_value(struct device_node *node, const char *propname) { @@ -1184,17 +1176,10 @@ static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) pdata->display.backlight = 1; if (of_find_property(node, "init", NULL)) pdata->display.fbtftops.init_display = fbtft_init_display_dt; - pdata->display.fbtftops.request_gpios = fbtft_request_gpios_dt; + pdata->display.fbtftops.request_gpios = fbtft_request_gpios; return pdata; } -#else -static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) -{ - dev_err(dev, "Missing platform data\n"); - return ERR_PTR(-EINVAL); -} -#endif /** * fbtft_probe_common() - Generic device probe() helper function
First of all there is no need to guard GPIO request by CONFIG_OF. It works for everybody independently on resource provider. While here, rename the function to reflect the above. Moreover, since we have a global dependency to OF, the rest of conditional compilation is no-op, i.e. it's always be true. Due to above drop useless #ifdef CONFIG_OF and therefore dead code. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/staging/fbtft/fbtft-core.c | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-)