New GCC warns about inappropriate use of strncpy():
drivers/staging/fbtft/fbtft-core.c: In function ‘fbtft_framebuffer_alloc’: drivers/staging/fbtft/fbtft-core.c:665:2: warning: ‘strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] 665 | strncpy(info->fix.id, dev->driver->name, 16); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Later on the copy is being used with the assumption to be NULL terminated. Make sure string is NULL terminated by switching to snprintf().
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/fbtft-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index a0a67aa517f0..61f0286fb157 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -666,7 +666,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display, fbdefio->deferred_io = fbtft_deferred_io; fb_deferred_io_init(info);
- strncpy(info->fix.id, dev->driver->name, 16); + snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name); info->fix.type = FB_TYPE_PACKED_PIXELS; info->fix.visual = FB_VISUAL_TRUECOLOR; info->fix.xpanstep = 0;
Kernel documentation script complains that some of the function parameters are not described:
drivers/staging/fbtft/fbtft-core.c:543: warning: Function parameter or member 'pdata' not described in 'fbtft_framebuffer_alloc'
Describe function parameters where it's appropriate.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/fbtft-core.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index 61f0286fb157..2122c4407bdb 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -529,6 +529,7 @@ static void fbtft_merge_fbtftops(struct fbtft_ops *dst, struct fbtft_ops *src) * * @display: pointer to structure describing the display * @dev: pointer to the device for this fb, this can be NULL + * @pdata: platform data for the display in use * * Creates a new frame buffer info structure. *
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 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
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.
Make use of device property API in this driver so that both OF based system and ACPI based system can use this driver.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/fbtft-core.c | 105 ++++++++++++++++------------- 1 file changed, 58 insertions(+), 47 deletions(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index ff8cb6670ea1..e87d839d86ac 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -22,8 +22,9 @@ #include <linux/uaccess.h> #include <linux/backlight.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/spinlock.h> -#include <linux/of.h> + #include <video/mipi_display.h>
#include "fbtft.h" @@ -894,44 +895,53 @@ int fbtft_unregister_framebuffer(struct fb_info *fb_info) EXPORT_SYMBOL(fbtft_unregister_framebuffer);
/** - * fbtft_init_display_dt() - Device Tree init_display() function + * fbtft_init_display_from_property() - Device Tree init_display() function * @par: Driver data * * Return: 0 if successful, negative if error */ -static int fbtft_init_display_dt(struct fbtft_par *par) +static int fbtft_init_display_from_property(struct fbtft_par *par) { - struct device_node *node = par->info->device->of_node; - struct property *prop; - const __be32 *p; + struct device *dev = par->info->device; + int buf[64], count, index, i, j, ret; + u32 *values; u32 val; - int buf[64], i, j;
- if (!node) + count = device_property_count_u32(dev, "init"); + if (count < 0) + return count; + if (count == 0) return -EINVAL;
- prop = of_find_property(node, "init", NULL); - p = of_prop_next_u32(prop, NULL, &val); - if (!p) - return -EINVAL; + values = kmalloc_array(count, sizeof(*values), GFP_KERNEL); + if (!values) + return -ENOMEM; + + ret = device_property_read_u32_array(dev, "init", values, count); + if (ret) + goto out_free;
par->fbtftops.reset(par); if (par->gpio.cs) gpiod_set_value(par->gpio.cs, 0); /* Activate chip */
- while (p) { + index = -1; + while (index < count) { + val = values[++index]; + if (val & FBTFT_OF_INIT_CMD) { val &= 0xFFFF; i = 0; - while (p && !(val & 0xFFFF0000)) { + while ((index < count) && !(val & 0xFFFF0000)) { if (i > 63) { - dev_err(par->info->device, + dev_err(dev, "%s: Maximum register values exceeded\n", __func__); - return -EINVAL; + ret = -EINVAL; + goto out_free; } buf[i++] = val; - p = of_prop_next_u32(prop, p, &val); + val = values[++index]; } /* make debug message */ fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, @@ -961,15 +971,17 @@ static int fbtft_init_display_dt(struct fbtft_par *par) fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "init: msleep(%u)\n", val & 0xFFFF); msleep(val & 0xFFFF); - p = of_prop_next_u32(prop, p, &val); + val = values[++index]; } else { - dev_err(par->info->device, "illegal init value 0x%X\n", - val); - return -EINVAL; + dev_err(dev, "illegal init value 0x%X\n", val); + ret = -EINVAL; + goto out_free; } }
- return 0; +out_free: + kfree(values); + return ret; }
/** @@ -1132,25 +1144,24 @@ static int fbtft_verify_gpios(struct fbtft_par *par) }
/* returns 0 if the property is not present */ -static u32 fbtft_of_value(struct device_node *node, const char *propname) +static u32 fbtft_property_value(struct device *dev, const char *propname) { int ret; u32 val = 0;
- ret = of_property_read_u32(node, propname, &val); + ret = device_property_read_u32(dev, propname, &val); if (ret == 0) - pr_info("%s: %s = %u\n", __func__, propname, val); + dev_info(dev, "%s: %s = %u\n", __func__, propname, val);
return val; }
-static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) +static struct fbtft_platform_data *fbtft_properties_read(struct device *dev) { - struct device_node *node = dev->of_node; struct fbtft_platform_data *pdata;
- if (!node) { - dev_err(dev, "Missing platform data or DT\n"); + if (!dev_fwnode(dev)) { + dev_err(dev, "Missing platform data or properties\n"); return ERR_PTR(-EINVAL); }
@@ -1158,24 +1169,24 @@ static struct fbtft_platform_data *fbtft_probe_dt(struct device *dev) if (!pdata) return ERR_PTR(-ENOMEM);
- pdata->display.width = fbtft_of_value(node, "width"); - pdata->display.height = fbtft_of_value(node, "height"); - pdata->display.regwidth = fbtft_of_value(node, "regwidth"); - pdata->display.buswidth = fbtft_of_value(node, "buswidth"); - pdata->display.backlight = fbtft_of_value(node, "backlight"); - pdata->display.bpp = fbtft_of_value(node, "bpp"); - pdata->display.debug = fbtft_of_value(node, "debug"); - pdata->rotate = fbtft_of_value(node, "rotate"); - pdata->bgr = of_property_read_bool(node, "bgr"); - pdata->fps = fbtft_of_value(node, "fps"); - pdata->txbuflen = fbtft_of_value(node, "txbuflen"); - pdata->startbyte = fbtft_of_value(node, "startbyte"); - of_property_read_string(node, "gamma", (const char **)&pdata->gamma); - - if (of_find_property(node, "led-gpios", NULL)) + pdata->display.width = fbtft_property_value(dev, "width"); + pdata->display.height = fbtft_property_value(dev, "height"); + pdata->display.regwidth = fbtft_property_value(dev, "regwidth"); + pdata->display.buswidth = fbtft_property_value(dev, "buswidth"); + pdata->display.backlight = fbtft_property_value(dev, "backlight"); + pdata->display.bpp = fbtft_property_value(dev, "bpp"); + pdata->display.debug = fbtft_property_value(dev, "debug"); + pdata->rotate = fbtft_property_value(dev, "rotate"); + pdata->bgr = device_property_read_bool(dev, "bgr"); + pdata->fps = fbtft_property_value(dev, "fps"); + pdata->txbuflen = fbtft_property_value(dev, "txbuflen"); + pdata->startbyte = fbtft_property_value(dev, "startbyte"); + device_property_read_string(dev, "gamma", (const char **)&pdata->gamma); + + if (device_property_present(dev, "led-gpios")) pdata->display.backlight = 1; - if (of_find_property(node, "init", NULL)) - pdata->display.fbtftops.init_display = fbtft_init_display_dt; + if (device_property_present(dev, "init")) + pdata->display.fbtftops.init_display = fbtft_init_display_from_property; pdata->display.fbtftops.request_gpios = fbtft_request_gpios;
return pdata; @@ -1213,7 +1224,7 @@ int fbtft_probe_common(struct fbtft_display *display,
pdata = dev->platform_data; if (!pdata) { - pdata = fbtft_probe_dt(dev); + pdata = fbtft_properties_read(dev); if (IS_ERR(pdata)) return PTR_ERR(pdata); }
Now, since driver became OF independent, no need to keep OF dependency.
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/staging/fbtft/Kconfig | 2 +- drivers/staging/fbtft/fbtft.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig index cb61c2a772bd..54751d9fc0ff 100644 --- a/drivers/staging/fbtft/Kconfig +++ b/drivers/staging/fbtft/Kconfig @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 menuconfig FB_TFT tristate "Support for small TFT LCD display modules" - depends on FB && SPI && OF + depends on FB && SPI depends on GPIOLIB || COMPILE_TEST select FB_SYS_FILLRECT select FB_SYS_COPYAREA diff --git a/drivers/staging/fbtft/fbtft.h b/drivers/staging/fbtft/fbtft.h index 9b6bdb62093d..5f782da51959 100644 --- a/drivers/staging/fbtft/fbtft.h +++ b/drivers/staging/fbtft/fbtft.h @@ -309,7 +309,7 @@ MODULE_DEVICE_TABLE(of, dt_ids); \ static struct spi_driver fbtft_driver_spi_driver = { \ .driver = { \ .name = _name, \ - .of_match_table = of_match_ptr(dt_ids), \ + .of_match_table = dt_ids, \ }, \ .probe = fbtft_driver_probe_spi, \ .remove = fbtft_driver_remove_spi, \ @@ -319,7 +319,7 @@ static struct platform_driver fbtft_driver_platform_driver = { \ .driver = { \ .name = _name, \ .owner = THIS_MODULE, \ - .of_match_table = of_match_ptr(dt_ids), \ + .of_match_table = dt_ids, \ }, \ .probe = fbtft_driver_probe_pdev, \ .remove = fbtft_driver_remove_pdev, \
On 20/11/2019 09:57, Andy Shevchenko wrote:
New GCC warns about inappropriate use of strncpy():
drivers/staging/fbtft/fbtft-core.c: In function ‘fbtft_framebuffer_alloc’: drivers/staging/fbtft/fbtft-core.c:665:2: warning: ‘strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] 665 | strncpy(info->fix.id, dev->driver->name, 16); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Later on the copy is being used with the assumption to be NULL terminated. Make sure string is NULL terminated by switching to snprintf().
Even better would be strscpy():
strscpy(info->fix.id, dev->driver->name, sizeof(info->fix.id));
Steve
Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
drivers/staging/fbtft/fbtft-core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/fbtft/fbtft-core.c b/drivers/staging/fbtft/fbtft-core.c index a0a67aa517f0..61f0286fb157 100644 --- a/drivers/staging/fbtft/fbtft-core.c +++ b/drivers/staging/fbtft/fbtft-core.c @@ -666,7 +666,7 @@ struct fb_info *fbtft_framebuffer_alloc(struct fbtft_display *display, fbdefio->deferred_io = fbtft_deferred_io; fb_deferred_io_init(info);
- strncpy(info->fix.id, dev->driver->name, 16);
- snprintf(info->fix.id, sizeof(info->fix.id), "%s", dev->driver->name); info->fix.type = FB_TYPE_PACKED_PIXELS; info->fix.visual = FB_VISUAL_TRUECOLOR; info->fix.xpanstep = 0;
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
Noralf Trønnes -
Steven Price