Re: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
Hi Bingbu, Arnd,
On Tue, Mar 05, 2019 at 12:25:18AM +, Cao, Bingbu wrote:
...
> > @@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > struct v4l2_rect *const bds = [IPU3_CSS_RECT_BDS];
> > struct v4l2_rect *const env = [IPU3_CSS_RECT_ENVELOPE];
> > struct v4l2_rect *const gdc = [IPU3_CSS_RECT_GDC];
> > - struct imgu_css_queue q[IPU3_CSS_QUEUES];
> > + struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
> > +imgu_css_queue), GFP_KERNEL);
>
> Could you use the devm_kcalloc()?
No, because this is not related to the device, called instead on
e.g. VIDIOC_TRY_FMT.
> > struct v4l2_pix_format_mplane *const in =
> > [IPU3_CSS_QUEUE_IN].fmt.mpix;
> > struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> > int imgu_css_fmt_try(struct imgu_css *css,
> > [IPU3_CSS_QUEUE_VF].fmt.mpix;
> > int i, s, ret;
> >
> > + if (!q) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> [Cao, Bingbu]
> The goto here is wrong, you can just report an error, and I prefer it is next
> to the alloc.
I agree, the goto is just not needed.
> > +
> > /* Adjust all formats, get statistics buffer sizes and formats */
> > for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> > if (fmts[i])
> > @@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> > dev_notice(css->dev, "can not initialize queue %s\n",
> >qnames[i]);
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > }
> > for (i = 0; i < IPU3_CSS_RECTS; i++) { @@ -1788,7 +1795,8 @@ int
> > imgu_css_fmt_try(struct imgu_css *css,
> > if (!imgu_css_queue_enabled([IPU3_CSS_QUEUE_IN]) ||
> > !imgu_css_queue_enabled([IPU3_CSS_QUEUE_OUT])) {
> > dev_warn(css->dev, "required queues are disabled\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> >
> > if (!imgu_css_queue_enabled([IPU3_CSS_QUEUE_OUT])) { @@ -1829,7
> > +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > ret = imgu_css_find_binary(css, pipe, q, r);
> > if (ret < 0) {
> > dev_err(css->dev, "failed to find suitable binary\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > css->pipes[pipe].bindex = ret;
> >
> > @@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> > dev_err(css->dev,
> > "final resolution adjustment failed\n");
> > - return -EINVAL;
> > + ret = -EINVAL;
> > + goto out;
> > }
> > *fmts[i] = q[i].fmt.mpix;
> > }
> > @@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
> > bds->width, bds->height, gdc->width, gdc->height,
> > out->width, out->height, vf->width, vf->height);
> >
> > - return 0;
> > + ret = 0;
> > +out:
> > + kfree(q);
> > + return ret;
> > }
> >
> > int imgu_css_fmt_set(struct imgu_css *css,
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[bug report] habanalabs: add virtual memory and MMU modules
Hello Omer Shpigelman,
The patch 0feaf86d4e69: "habanalabs: add virtual memory and MMU
modules" from Feb 16, 2019, leads to the following static checker
warning:
drivers/misc/habanalabs/memory.c:96 alloc_device_memory()
warn: integer overflows '(args->alloc.mem_size + (page_size - 1)) >>
page_shift'
drivers/misc/habanalabs/memory.c
53 static int alloc_device_memory(struct hl_ctx *ctx, struct hl_mem_in
*args,
54 u32 *ret_handle)
55 {
56 struct hl_device *hdev = ctx->hdev;
57 struct hl_vm *vm = >vm;
58 struct hl_vm_phys_pg_pack *phys_pg_pack;
59 u64 paddr = 0;
60 u32 total_size, num_pgs, num_curr_pgs, page_size, page_shift;
61 int handle, rc, i;
62 bool contiguous;
63
64 num_curr_pgs = 0;
65 page_size = hdev->asic_prop.dram_page_size;
66 page_shift = __ffs(page_size);
67 num_pgs = (args->alloc.mem_size + (page_size - 1)) >> page_shift;
^^
This addition can have an integer overflow. mem_size is a u64 that
comes from the user in the IOCTL. Also num_pgs is a u32 so it can't
hold mem_size / 4096.
68 total_size = num_pgs << page_shift;
^
So can this shift. total_size is u32.
69
70 contiguous = args->flags & HL_MEM_CONTIGUOUS;
71
72 if (contiguous) {
73 paddr = (u64) gen_pool_alloc(vm->dram_pg_pool, total_size);
74 if (!paddr) {
75 dev_err(hdev->dev,
76 "failed to allocate %u huge contiguous pages\n",
77 num_pgs);
78 return -ENOMEM;
79 }
80 }
81
82 phys_pg_pack = kzalloc(sizeof(*phys_pg_pack), GFP_KERNEL);
83 if (!phys_pg_pack) {
84 rc = -ENOMEM;
85 goto pages_pack_err;
86 }
87
88 phys_pg_pack->vm_type = VM_TYPE_PHYS_PACK;
89 phys_pg_pack->asid = ctx->asid;
90 phys_pg_pack->npages = num_pgs;
91 phys_pg_pack->page_size = page_size;
92 phys_pg_pack->total_size = total_size;
93 phys_pg_pack->flags = args->flags;
94 phys_pg_pack->contiguous = contiguous;
95
--> 96 phys_pg_pack->pages = kcalloc(num_pgs, sizeof(u64), GFP_KERNEL);
^^^
We allocate less memory than intended.
97 if (!phys_pg_pack->pages) {
98 rc = -ENOMEM;
99 goto pages_arr_err;
100 }
101
102 if (phys_pg_pack->contiguous) {
103 for (i = 0 ; i < num_pgs ; i++)
104 phys_pg_pack->pages[i] = paddr + i * page_size;
105 } else {
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
__
BRs,
Cao, Bingbu
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, March 5, 2019 4:28 AM
> To: Sakari Ailus ; Mauro Carvalho Chehab
> ; Greg Kroah-Hartman
> Cc: Arnd Bergmann ; Zhi, Yong ; Cao,
> Bingbu ; linux-me...@vger.kernel.org;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
>
> The imgu_css_queue structure is too large to be put on the kernel stack,
> as we can see in 32-bit builds:
>
> drivers/staging/media/ipu3/ipu3-css.c: In function 'imgu_css_fmt_try':
> drivers/staging/media/ipu3/ipu3-css.c:1863:1: error: the frame size of
> 1172 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> By dynamically allocating this array, the stack usage goes down to an
> acceptable 140 bytes for the same x86-32 configuration.
>
> Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline
> programming")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/media/ipu3/ipu3-css.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-css.c
> b/drivers/staging/media/ipu3/ipu3-css.c
> index 15ab77e4b766..664c14b7a518 100644
> --- a/drivers/staging/media/ipu3/ipu3-css.c
> +++ b/drivers/staging/media/ipu3/ipu3-css.c
> @@ -3,6 +3,7 @@
>
> #include
> #include
> +#include
>
> #include "ipu3-css.h"
> #include "ipu3-css-fw.h"
> @@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
> struct v4l2_rect *const bds = [IPU3_CSS_RECT_BDS];
> struct v4l2_rect *const env = [IPU3_CSS_RECT_ENVELOPE];
> struct v4l2_rect *const gdc = [IPU3_CSS_RECT_GDC];
> - struct imgu_css_queue q[IPU3_CSS_QUEUES];
> + struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
> +imgu_css_queue), GFP_KERNEL);
Could you use the devm_kcalloc()?
> struct v4l2_pix_format_mplane *const in =
> [IPU3_CSS_QUEUE_IN].fmt.mpix;
> struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> int imgu_css_fmt_try(struct imgu_css *css,
> [IPU3_CSS_QUEUE_VF].fmt.mpix;
> int i, s, ret;
>
> + if (!q) {
> + ret = -ENOMEM;
> + goto out;
> + }
[Cao, Bingbu]
The goto here is wrong, you can just report an error, and I prefer it is next
to the alloc.
> +
> /* Adjust all formats, get statistics buffer sizes and formats */
> for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> if (fmts[i])
> @@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> dev_notice(css->dev, "can not initialize queue %s\n",
> qnames[i]);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> }
> for (i = 0; i < IPU3_CSS_RECTS; i++) { @@ -1788,7 +1795,8 @@ int
> imgu_css_fmt_try(struct imgu_css *css,
> if (!imgu_css_queue_enabled([IPU3_CSS_QUEUE_IN]) ||
> !imgu_css_queue_enabled([IPU3_CSS_QUEUE_OUT])) {
> dev_warn(css->dev, "required queues are disabled\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> if (!imgu_css_queue_enabled([IPU3_CSS_QUEUE_OUT])) { @@ -1829,7
> +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> ret = imgu_css_find_binary(css, pipe, q, r);
> if (ret < 0) {
> dev_err(css->dev, "failed to find suitable binary\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> css->pipes[pipe].bindex = ret;
>
> @@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> dev_err(css->dev,
> "final resolution adjustment failed\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> *fmts[i] = q[i].fmt.mpix;
> }
> @@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
>bds->width, bds->height, gdc->width, gdc->height,
>out->width, out->height, vf->width, vf->height);
>
> - return 0;
> + ret = 0;
> +out:
> + kfree(q);
> + return ret;
> }
>
> int imgu_css_fmt_set(struct imgu_css *css,
> --
> 2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH] media: staging/intel-ipu3: mark PM function as __maybe_unused
Hi, Bergmann,
Thanks for your patch.
Reviewed-by: Cao, Bingbu
__
BRs,
Cao, Bingbu
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, March 5, 2019 4:29 AM
> To: Sakari Ailus ; Mauro Carvalho Chehab
> ; Greg Kroah-Hartman
> Cc: Arnd Bergmann ; Zhi, Yong ;
> Tomasz Figa ; Qiu, Tian Shu
> ; Cao, Bingbu ; linux-
> me...@vger.kernel.org; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] media: staging/intel-ipu3: mark PM function as
> __maybe_unused
>
> The imgu_rpm_dummy_cb() looks like an API misuse that is explained in
> the comment above it. Aside from that, it also causes a warning when
> power management support is disabled:
>
> drivers/staging/media/ipu3/ipu3.c:794:12: error: 'imgu_rpm_dummy_cb'
> defined but not used [-Werror=unused-function]
>
> The warning is at least easy to fix by marking the function as
> __maybe_unused.
>
> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci
> device driver")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/media/ipu3/ipu3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3.c
> b/drivers/staging/media/ipu3/ipu3.c
> index d575ac78c8f0..d00d26264c37 100644
> --- a/drivers/staging/media/ipu3/ipu3.c
> +++ b/drivers/staging/media/ipu3/ipu3.c
> @@ -791,7 +791,7 @@ static int __maybe_unused imgu_resume(struct device
> *dev)
> * PCI rpm framework checks the existence of driver rpm callbacks.
> * Place a dummy callback here to avoid rpm going into error state.
> */
> -static int imgu_rpm_dummy_cb(struct device *dev)
> +static __maybe_unused int imgu_rpm_dummy_cb(struct device *dev)
> {
> return 0;
> }
> --
> 2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/3] PCI: hv: Fix a memory leak in hv_eject_device_work()
After a device is just created in new_pcichild_device(), hpdev->refs is set
to 2 (i.e. the initial value of 1 plus the get_pcichild()).
When we hot remove the device from the host, in Linux VM we first call
hv_pci_eject_device(), which increases hpdev->refs by get_pcichild() and
then schedules a work of hv_eject_device_work(), so hpdev->refs becomes 3
(let's ignore the paired get/put_pcichild() in other places). But in
hv_eject_device_work(), currently we only call put_pcichild() twice,
meaning the 'hpdev' struct can't be freed in put_pcichild(). This patch
adds one put_pcichild() to fix the memory leak.
BTW, the device can also be removed when we run "rmmod pci-hyperv". On this
path (hv_pci_remove() -> hv_pci_bus_exit() -> hv_pci_devices_present()),
hpdev->refs is 2, and we do correctly call put_pcichild() twice in
pci_devices_present_work().
Fixes: 4daace0d8ce8 ("PCI: hv: Add paravirtual PCI front-end for Microsoft
Hyper-V VMs")
Signed-off-by: Dexuan Cui
Cc:
---
drivers/pci/controller/pci-hyperv.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index 95441a35eceb..30f16b882746 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1900,6 +1900,9 @@ static void hv_eject_device_work(struct work_struct *work)
sizeof(*ejct_pkt), (unsigned long),
VM_PKT_DATA_INBAND, 0);
+ /* For the get_pcichild() in hv_pci_eject_device() */
+ put_pcichild(hpdev);
+ /* For the two refs got in new_pcichild_device() */
put_pcichild(hpdev);
put_pcichild(hpdev);
put_hvpcibus(hpdev->hbus);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/3] pci-hyperv: fix memory leak and add pci_destroy_slot()
Patch #1 fixes a memory leak caused by incorrectly-maintained hpdev->refs. Patch #2 and #3 make sure the "slot" is removed in all the scenarios. Without them, in the quick hot-add/hot-remove test, systemd-dev may easily crash when trying to access a dangling sys file in /sys/bus/pci/slots/: "BUG: unable to handle kernel paging request". BTW, Patch #2 was posted on Feb 7, 2019, and this is the v2: the change to hv_eject_device_work() in v1 is removed, as the change is only needed when we hot-remove the device and remove the pci-hyperv driver at the same time. It looks more work is required to make this scenaro work correctly, and since removing the driver is not really a "usual" usage, we can address this scenario in the future. Please review the patchset. Dexuan Cui (3): PCI: hv: Fix a memory leak in hv_eject_device_work() PCI: hv: Add hv_pci_remove_slots() when we unload the driver PCI: hv: Add pci_destroy_slot() in pci_devices_present_work(), if necessary drivers/pci/controller/pci-hyperv.c | 23 +++ 1 file changed, 23 insertions(+) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/3] PCI: hv: Add pci_destroy_slot() in pci_devices_present_work(), if necessary
When we hot-remove a device, usually the host sends us a PCI_EJECT message,
and a PCI_BUS_RELATIONS message with bus_rel->device_count == 0. But when
we do the quick hot-add/hot-remove test, the host may not send us the
PCI_EJECT message, if the guest has not fully finished the initialization
by sending the PCI_RESOURCES_ASSIGNED* message to the host, so it's
potentially unsafe to only depend on the pci_destroy_slot() in
hv_eject_device_work(), though create_root_hv_pci_bus() ->
hv_pci_assign_slots() is not called in this case. Note: in this case, the
host still sends the guest a PCI_BUS_RELATIONS message with
bus_rel->device_count == 0.
And, in the quick hot-add/hot-remove test, we can have such a race: before
pci_devices_present_work() -> new_pcichild_device() adds the new device
into hbus->children, we may have already received the PCI_EJECT message,
and hence the taklet handler hv_pci_onchannelcallback() may fail to find
the "hpdev" by get_pcichild_wslot(hbus, dev_message->wslot.slot), so
hv_pci_eject_device() is NOT called; later create_root_hv_pci_bus() ->
hv_pci_assign_slots() creates the slot, and the PCI_BUS_RELATIONS message
with bus_rel->device_count == 0 removes the device from hbus->children, and
we end up being unable to remove the slot in hv_pci_remove() ->
hv_pci_remove_slots().
The patch removes the slot in pci_devices_present_work() when the device
is removed. This can address the above race. Note 1:
pci_devices_present_work() and hv_eject_device_work() run in the
singled-threaded hbus->wq, so there is not a double-remove issue for the
slot. Note 2: we can't offload hv_pci_eject_device() from
hv_pci_onchannelcallback() to the workqueue, because we need
hv_pci_onchannelcallback() synchronously call hv_pci_eject_device() to
poll the channel's ringbuffer to work around the
"hangs in hv_compose_msi_msg()" issue: see
commit de0aa7b2f97d ("PCI: hv: Fix 2 hang issues in hv_compose_msi_msg()")
Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot
information")
Signed-off-by: Dexuan Cui
Cc: sta...@vger.kernel.org
---
drivers/pci/controller/pci-hyperv.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index b489412e3502..82acd6155adf 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1776,6 +1776,10 @@ static void pci_devices_present_work(struct work_struct
*work)
hpdev = list_first_entry(, struct hv_pci_dev,
list_entry);
list_del(>list_entry);
+
+ if (hpdev->pci_slot)
+ pci_destroy_slot(hpdev->pci_slot);
+
put_pcichild(hpdev);
}
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/3] PCI: hv: Add hv_pci_remove_slots() when we unload the driver
When we unload pci-hyperv, the host doesn't send us a PCI_EJECT message.
In this case we also need to make sure the sysfs pci slot directory
is removed, otherwise "cat /sys/bus/pci/slots/2/address" will trigger
"BUG: unable to handle kernel paging request" (I noticed the issue when
systemd-dev crashed for me when I unloaded the driver). And, if we
unload/reload the driver several times, we'll have multiple pci slot
directories in /sys/bus/pci/slots/ like this:
root@localhost:~# ls -rtl /sys/bus/pci/slots/
total 0
drwxr-xr-x 2 root root 0 Feb 7 10:49 2
drwxr-xr-x 2 root root 0 Feb 7 10:49 2-1
drwxr-xr-x 2 root root 0 Feb 7 10:51 2-2
The patch adds the missing code.
Fixes: a15f2c08c708 ("PCI: hv: support reporting serial number as slot
information")
Signed-off-by: Dexuan Cui
Acked-by: Stephen Hemminger
Cc: sta...@vger.kernel.org
---
drivers/pci/controller/pci-hyperv.c | 16
1 file changed, 16 insertions(+)
diff --git a/drivers/pci/controller/pci-hyperv.c
b/drivers/pci/controller/pci-hyperv.c
index 30f16b882746..b489412e3502 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -1486,6 +1486,21 @@ static void hv_pci_assign_slots(struct hv_pcibus_device
*hbus)
}
}
+/*
+ * Remove entries in sysfs pci slot directory.
+ */
+static void hv_pci_remove_slots(struct hv_pcibus_device *hbus)
+{
+ struct hv_pci_dev *hpdev;
+
+ list_for_each_entry(hpdev, >children, list_entry) {
+ if (!hpdev->pci_slot)
+ continue;
+ pci_destroy_slot(hpdev->pci_slot);
+ hpdev->pci_slot = NULL;
+ }
+}
+
/**
* create_root_hv_pci_bus() - Expose a new root PCI bus
* @hbus: Root PCI bus, as understood by this driver
@@ -2680,6 +2695,7 @@ static int hv_pci_remove(struct hv_device *hdev)
pci_lock_rescan_remove();
pci_stop_root_bus(hbus->pci_bus);
pci_remove_root_bus(hbus->pci_bus);
+ hv_pci_remove_slots(hbus);
pci_unlock_rescan_remove();
hbus->state = hv_pcibus_removed;
}
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] media: staging: davinci_vpfe: disallow building with COMPILE_TEST
The driver should really call dm365_isif_setup_pinmux() through a callback,
but it runs platform specific code by itself, which never actually compiled:
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2028:2: error:
implicit declaration of function 'davinci_cfg_reg'
[-Werror,-Wimplicit-function-declaration]
davinci_cfg_reg(DM365_VIN_CAM_WEN);
^
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2028:2: error:
this function declaration is not a prototype [-Werror,-Wstrict-prototypes]
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2028:18: error:
use of undeclared identifier 'DM365_VIN_CAM_WEN'
davinci_cfg_reg(DM365_VIN_CAM_WEN);
^
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2029:18: error:
use of undeclared identifier 'DM365_VIN_CAM_VD'
davinci_cfg_reg(DM365_VIN_CAM_VD);
^
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2030:18: error:
use of undeclared identifier 'DM365_VIN_CAM_HD'
davinci_cfg_reg(DM365_VIN_CAM_HD);
^
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2031:18: error:
use of undeclared identifier 'DM365_VIN_YIN4_7_EN'
davinci_cfg_reg(DM365_VIN_YIN4_7_EN);
^
/git/arm-soc/drivers/staging/media/davinci_vpfe/dm365_isif.c:2032:18: error:
use of undeclared identifier 'DM365_VIN_YIN0_3_EN'
davinci_cfg_reg(DM365_VIN_YIN0_3_EN);
^
7 errors generated.
Fixes: 4907c73deefe ("media: staging: davinci_vpfe: allow building with
COMPILE_TEST")
Signed-off-by: Arnd Bergmann
---
drivers/staging/media/davinci_vpfe/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/davinci_vpfe/Kconfig
b/drivers/staging/media/davinci_vpfe/Kconfig
index aea449a8dbf8..84ac6b9e1767 100644
--- a/drivers/staging/media/davinci_vpfe/Kconfig
+++ b/drivers/staging/media/davinci_vpfe/Kconfig
@@ -1,7 +1,7 @@
config VIDEO_DM365_VPFE
tristate "DM365 VPFE Media Controller Capture Driver"
depends on VIDEO_V4L2
- depends on (ARCH_DAVINCI_DM365 && !VIDEO_DM365_ISIF) || COMPILE_TEST
+ depends on (ARCH_DAVINCI_DM365 && !VIDEO_DM365_ISIF)
depends on VIDEO_V4L2_SUBDEV_API
depends on VIDEO_DAVINCI_VPBE_DISPLAY
select VIDEOBUF2_DMA_CONTIG
--
2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] media: staging/intel-ipu3: mark PM function as __maybe_unused
The imgu_rpm_dummy_cb() looks like an API misuse that is explained
in the comment above it. Aside from that, it also causes a warning
when power management support is disabled:
drivers/staging/media/ipu3/ipu3.c:794:12: error: 'imgu_rpm_dummy_cb' defined
but not used [-Werror=unused-function]
The warning is at least easy to fix by marking the function as
__maybe_unused.
Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci device
driver")
Signed-off-by: Arnd Bergmann
---
drivers/staging/media/ipu3/ipu3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/ipu3/ipu3.c
b/drivers/staging/media/ipu3/ipu3.c
index d575ac78c8f0..d00d26264c37 100644
--- a/drivers/staging/media/ipu3/ipu3.c
+++ b/drivers/staging/media/ipu3/ipu3.c
@@ -791,7 +791,7 @@ static int __maybe_unused imgu_resume(struct device *dev)
* PCI rpm framework checks the existence of driver rpm callbacks.
* Place a dummy callback here to avoid rpm going into error state.
*/
-static int imgu_rpm_dummy_cb(struct device *dev)
+static __maybe_unused int imgu_rpm_dummy_cb(struct device *dev)
{
return 0;
}
--
2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] media: staging/intel-ipu3-v4l: reduce kernel stack usage
The v4l2_pix_format_mplane structure is too large to be put on the kernel
stack, as we can see in 32-bit builds:
drivers/staging/media/ipu3/ipu3-v4l2.c: In function 'imgu_fmt':
drivers/staging/media/ipu3/ipu3-v4l2.c:753:1: error: the frame size of 1028
bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
By dynamically allocating this array, the stack usage goes down to an
acceptable 272 bytes for the same x86-32 configuration.
Fixes: a0ca1627b450 ("media: staging/intel-ipu3: Add v4l2 driver based on media
framework")
Signed-off-by: Arnd Bergmann
---
drivers/staging/media/ipu3/ipu3-v4l2.c | 40 --
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c
b/drivers/staging/media/ipu3/ipu3-v4l2.c
index 9c0352b193a7..c34b433539c4 100644
--- a/drivers/staging/media/ipu3/ipu3-v4l2.c
+++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
@@ -664,12 +664,11 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned
int pipe, int node,
struct v4l2_format *f, bool try)
{
struct device *dev = >pci_dev->dev;
- struct v4l2_pix_format_mplane try_fmts[IPU3_CSS_QUEUES];
struct v4l2_pix_format_mplane *fmts[IPU3_CSS_QUEUES] = { NULL };
struct v4l2_rect *rects[IPU3_CSS_RECTS] = { NULL };
struct v4l2_mbus_framefmt pad_fmt;
unsigned int i, css_q;
- int r;
+ int ret;
struct imgu_css_pipe *css_pipe = >css.pipes[pipe];
struct imgu_media_pipe *imgu_pipe = >imgu_pipe[pipe];
struct imgu_v4l2_subdev *imgu_sd = _pipe->imgu_sd;
@@ -698,9 +697,13 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned int
pipe, int node,
continue;
if (try) {
- try_fmts[i] =
- imgu_pipe->nodes[inode].vdev_fmt.fmt.pix_mp;
- fmts[i] = _fmts[i];
+ fmts[i] =
kmemdup(_pipe->nodes[inode].vdev_fmt.fmt.pix_mp,
+ sizeof(struct v4l2_pix_format_mplane),
+ GFP_KERNEL);
+ if (!fmts[i]) {
+ ret = -ENOMEM;
+ goto out;
+ }
} else {
fmts[i] = _pipe->nodes[inode].vdev_fmt.fmt.pix_mp;
}
@@ -730,26 +733,33 @@ static int imgu_fmt(struct imgu_device *imgu, unsigned
int pipe, int node,
* before we return success from this function, so set it here.
*/
css_q = imgu_node_to_queue(node);
- if (fmts[css_q])
- *fmts[css_q] = f->fmt.pix_mp;
- else
- return -EINVAL;
+ if (!fmts[css_q]) {
+ ret = -EINVAL;
+ goto out;
+ }
+ *fmts[css_q] = f->fmt.pix_mp;
if (try)
- r = imgu_css_fmt_try(>css, fmts, rects, pipe);
+ ret = imgu_css_fmt_try(>css, fmts, rects, pipe);
else
- r = imgu_css_fmt_set(>css, fmts, rects, pipe);
+ ret = imgu_css_fmt_set(>css, fmts, rects, pipe);
- /* r is the binary number in the firmware blob */
- if (r < 0)
- return r;
+ /* ret is the binary number in the firmware blob */
+ if (ret < 0)
+ goto out;
if (try)
f->fmt.pix_mp = *fmts[css_q];
else
f->fmt = imgu_pipe->nodes[node].vdev_fmt.fmt;
- return 0;
+out:
+ if (try) {
+ for (i = 0; i < IPU3_CSS_QUEUES; i++)
+ kfree(fmts[i]);
+ }
+
+ return ret;
}
static int imgu_try_fmt(struct file *file, void *fh, struct v4l2_format *f)
--
2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] media: staging/intel-ipu3: reduce kernel stack usage
The imgu_css_queue structure is too large to be put on the kernel
stack, as we can see in 32-bit builds:
drivers/staging/media/ipu3/ipu3-css.c: In function 'imgu_css_fmt_try':
drivers/staging/media/ipu3/ipu3-css.c:1863:1: error: the frame size of 1172
bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
By dynamically allocating this array, the stack usage goes down to an
acceptable 140 bytes for the same x86-32 configuration.
Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline programming")
Signed-off-by: Arnd Bergmann
---
drivers/staging/media/ipu3/ipu3-css.c | 25 +++--
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/ipu3/ipu3-css.c
b/drivers/staging/media/ipu3/ipu3-css.c
index 15ab77e4b766..664c14b7a518 100644
--- a/drivers/staging/media/ipu3/ipu3-css.c
+++ b/drivers/staging/media/ipu3/ipu3-css.c
@@ -3,6 +3,7 @@
#include
#include
+#include
#include "ipu3-css.h"
#include "ipu3-css-fw.h"
@@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
struct v4l2_rect *const bds = [IPU3_CSS_RECT_BDS];
struct v4l2_rect *const env = [IPU3_CSS_RECT_ENVELOPE];
struct v4l2_rect *const gdc = [IPU3_CSS_RECT_GDC];
- struct imgu_css_queue q[IPU3_CSS_QUEUES];
+ struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
imgu_css_queue), GFP_KERNEL);
struct v4l2_pix_format_mplane *const in =
[IPU3_CSS_QUEUE_IN].fmt.mpix;
struct v4l2_pix_format_mplane *const out =
@@ -1753,6 +1754,11 @@ int imgu_css_fmt_try(struct imgu_css *css,
[IPU3_CSS_QUEUE_VF].fmt.mpix;
int i, s, ret;
+ if (!q) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
/* Adjust all formats, get statistics buffer sizes and formats */
for (i = 0; i < IPU3_CSS_QUEUES; i++) {
if (fmts[i])
@@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
IPU3_CSS_QUEUE_TO_FLAGS(i))) {
dev_notice(css->dev, "can not initialize queue %s\n",
qnames[i]);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
}
for (i = 0; i < IPU3_CSS_RECTS; i++) {
@@ -1788,7 +1795,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
if (!imgu_css_queue_enabled([IPU3_CSS_QUEUE_IN]) ||
!imgu_css_queue_enabled([IPU3_CSS_QUEUE_OUT])) {
dev_warn(css->dev, "required queues are disabled\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
if (!imgu_css_queue_enabled([IPU3_CSS_QUEUE_OUT])) {
@@ -1829,7 +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
ret = imgu_css_find_binary(css, pipe, q, r);
if (ret < 0) {
dev_err(css->dev, "failed to find suitable binary\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
css->pipes[pipe].bindex = ret;
@@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
IPU3_CSS_QUEUE_TO_FLAGS(i))) {
dev_err(css->dev,
"final resolution adjustment failed\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
*fmts[i] = q[i].fmt.mpix;
}
@@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
bds->width, bds->height, gdc->width, gdc->height,
out->width, out->height, vf->width, vf->height);
- return 0;
+ ret = 0;
+out:
+ kfree(q);
+ return ret;
}
int imgu_css_fmt_set(struct imgu_css *css,
--
2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: olpc_dcon_xo_1: add missing 'const' qualifier
gcc noticed a mismatch between the type qualifiers after a recent
cleanup:
drivers/staging/olpc_dcon/olpc_dcon_xo_1.c: In function 'dcon_init_xo_1':
drivers/staging/olpc_dcon/olpc_dcon_xo_1.c:48:26: error: initialization
discards 'const' qualifier from pointer target type
[-Werror=discarded-qualifiers]
Add the 'const' keyword that should have been there all along.
Fixes: 2159fb372929 ("staging: olpc_dcon: olpc_dcon_xo_1.c: Switch to the gpio
descriptor interface")
Signed-off-by: Arnd Bergmann
---
drivers/staging/olpc_dcon/olpc_dcon_xo_1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
b/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
index 80b8d4153414..a54286498a47 100644
--- a/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
+++ b/drivers/staging/olpc_dcon/olpc_dcon_xo_1.c
@@ -45,7 +45,7 @@ static int dcon_init_xo_1(struct dcon_priv *dcon)
{
unsigned char lob;
int ret, i;
- struct dcon_gpio *pin = _asis[0];
+ const struct dcon_gpio *pin = _asis[0];
for (i = 0; i < ARRAY_SIZE(gpios_asis); i++) {
gpios[i] = devm_gpiod_get(>client->dev, pin[i].name,
--
2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: axis-fifo: add CONFIG_OF dependency
When building without CONFIG_OF, the compiler loses track of the flow
control in axis_fifo_probe(), and thinks that many variables are used
without an initialization even though we actually leave the function
before the first use:
drivers/staging/axis-fifo/axis-fifo.c: In function 'axis_fifo_probe':
drivers/staging/axis-fifo/axis-fifo.c:900:5: error: 'rxd_tdata_width' may be
used uninitialized in this function [-Werror=maybe-uninitialized]
if (rxd_tdata_width != 32) {
^
drivers/staging/axis-fifo/axis-fifo.c:907:5: error: 'txd_tdata_width' may be
used uninitialized in this function [-Werror=maybe-uninitialized]
if (txd_tdata_width != 32) {
^
drivers/staging/axis-fifo/axis-fifo.c:914:5: error: 'has_tdest' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
if (has_tdest) {
^
drivers/staging/axis-fifo/axis-fifo.c:919:5: error: 'has_tid' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
When CONFIG_OF is set, this does not happen, and since the driver cannot
work without it, just add that option as a Kconfig dependency.
Signed-off-by: Arnd Bergmann
---
drivers/staging/axis-fifo/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/staging/axis-fifo/Kconfig
b/drivers/staging/axis-fifo/Kconfig
index 687537203d9c..d9725888af6f 100644
--- a/drivers/staging/axis-fifo/Kconfig
+++ b/drivers/staging/axis-fifo/Kconfig
@@ -3,6 +3,7 @@
#
config XIL_AXIS_FIFO
tristate "Xilinx AXI-Stream FIFO IP core driver"
+ depends on OF
default n
help
This adds support for the Xilinx AXI-Stream
--
2.20.0
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: greybus: bundle.c fix comparison to NULL
Fix checkpatch issue:
Comparison to NULL could be written "!bundle->state"
Signed-off-by: Dominik Adamski
---
drivers/staging/greybus/bundle.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/greybus/bundle.c b/drivers/staging/greybus/bundle.c
index e97b2b87ba47..3f702db9e098 100644
--- a/drivers/staging/greybus/bundle.c
+++ b/drivers/staging/greybus/bundle.c
@@ -32,7 +32,7 @@ static ssize_t state_show(struct device *dev, struct
device_attribute *attr,
{
struct gb_bundle *bundle = to_gb_bundle(dev);
- if (bundle->state == NULL)
+ if (!bundle->state)
return sprintf(buf, "\n");
return sprintf(buf, "%s\n", bundle->state);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: comedi: ni_mio_common: Fix divide-by-zero for DIO cmdtest
`ni_cdio_cmdtest()` validates Comedi asynchronous commands for the DIO
subdevice (subdevice 2) of supported National Instruments M-series
cards. It is called when handling the `COMEDI_CMD` and `COMEDI_CMDTEST`
ioctls for this subdevice. There are two causes for a possible
divide-by-zero error when validating that the `stop_arg` member of the
passed-in command is not too large.
The first cause for the divide-by-zero is that calls to
`comedi_bytes_per_scan()` are only valid once the command has been
copied to `s->async->cmd`, but that copy is only done for the
`COMEDI_CMD` ioctl. For the `COMEDI_CMDTEST` ioctl, it will use
whatever was left there by the previous `COMEDI_CMD` ioctl, if any.
(This is very likely, as it is usual for the application to use
`COMEDI_CMDTEST` before `COMEDI_CMD`.) If there has been no previous,
valid `COMEDI_CMD` for this subdevice, then `comedi_bytes_per_scan()`
will return 0, so the subsequent division in `ni_cdio_cmdtest()` of
`s->async->prealloc_bufsz / comedi_bytes_per_scan(s)` will be a
divide-by-zero error. To fix this error, call a new function
`comedi_bytes_per_scan_cmd(s, cmd)`, based on the existing
`comedi_bytes_per_scan(s)` but using a specified `struct comedi_cmd` for
its calculations. (Also refactor `comedi_bytes_per_scan()` to call the
new function.)
Once the first cause for the divide-by-zero has been fixed, the second
cause is that `comedi_bytes_per_scan_cmd()` can legitimately return 0 if
the `scan_end_arg` member of the `struct comedi_cmd` being tested is 0.
Fix it by only performing the division (and validating that `stop_arg`
is no more than the maximum value) if `comedi_bytes_per_scan_cmd()`
returns a non-zero value.
The problem was reported on the COMEDI mailing list here:
https://groups.google.com/forum/#!topic/comedi_list/4t9WlHzMhKM
Fixes: f164cbf98fa8 ("staging: comedi: ni_mio_common: add finite regeneration
to dio output")
Cc: # 4.6+
Cc: Spencer E. Olson
Signed-off-by: Ian Abbott
---
drivers/staging/comedi/comedidev.h| 2 ++
drivers/staging/comedi/drivers.c | 33 ---
.../staging/comedi/drivers/ni_mio_common.c| 10 --
3 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/comedi/comedidev.h
b/drivers/staging/comedi/comedidev.h
index a7d569cfca5d..0dff1ac057cd 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -1001,6 +1001,8 @@ int comedi_dio_insn_config(struct comedi_device *dev,
unsigned int mask);
unsigned int comedi_dio_update_state(struct comedi_subdevice *s,
unsigned int *data);
+unsigned int comedi_bytes_per_scan_cmd(struct comedi_subdevice *s,
+ struct comedi_cmd *cmd);
unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s);
unsigned int comedi_nscans_left(struct comedi_subdevice *s,
unsigned int nscans);
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index eefa62f42c0f..5a32b8fc000e 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -394,11 +394,13 @@ unsigned int comedi_dio_update_state(struct
comedi_subdevice *s,
EXPORT_SYMBOL_GPL(comedi_dio_update_state);
/**
- * comedi_bytes_per_scan() - Get length of asynchronous command "scan" in bytes
+ * comedi_bytes_per_scan_cmd() - Get length of asynchronous command "scan" in
+ * bytes
* @s: COMEDI subdevice.
+ * @cmd: COMEDI command.
*
* Determines the overall scan length according to the subdevice type and the
- * number of channels in the scan.
+ * number of channels in the scan for the specified command.
*
* For digital input, output or input/output subdevices, samples for
* multiple channels are assumed to be packed into one or more unsigned
@@ -408,9 +410,9 @@ EXPORT_SYMBOL_GPL(comedi_dio_update_state);
*
* Returns the overall scan length in bytes.
*/
-unsigned int comedi_bytes_per_scan(struct comedi_subdevice *s)
+unsigned int comedi_bytes_per_scan_cmd(struct comedi_subdevice *s,
+ struct comedi_cmd *cmd)
{
- struct comedi_cmd *cmd = >async->cmd;
unsigned int num_samples;
unsigned int bits_per_sample;
@@ -427,6 +429,29 @@ unsigned int comedi_bytes_per_scan(struct comedi_subdevice
*s)
}
return comedi_samples_to_bytes(s, num_samples);
}
+EXPORT_SYMBOL_GPL(comedi_bytes_per_scan_cmd);
+
+/**
+ * comedi_bytes_per_scan() - Get length of asynchronous command "scan" in bytes
+ * @s: COMEDI subdevice.
+ *
+ * Determines the overall scan length according to the subdevice type and the
+ * number of channels in the scan for the current command.
+ *
+ * For digital input, output or input/output subdevices, samples for
+ * multiple channels are assumed to be packed into one or more unsigned
+ * short or unsigned int values according to the subdevice's %SDF_LSAMPL
+ *
Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
On Sat, 2019-03-02 at 19:08 +, Jonathan Cameron wrote:
> [External]
>
>
> On Sat, 2 Mar 2019 19:07:16 +
> Jonathan Cameron wrote:
>
> > On Fri, 1 Mar 2019 07:17:04 +
> > "Ardelean, Alexandru" wrote:
> >
> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > > >
> > > >
> > > > The AD7780 driver contains status pattern bits designed for
> > > > checking
> > > > whether serial transfers have been correctly performed. Pattern
> > > > macros
> > > > were previously generated through bit fields. This patch sets good
> > > > pattern values directly and masks through GENMASK.
> > > >
> > > > Signed-off-by: Renato Lui Geh
> > > > ---
> > > > drivers/staging/iio/adc/ad7780.c | 20 +---
> > > > 1 file changed, 9 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 7a68e90ddf14..56c49e28f432 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -17,6 +17,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include
> > > > #include
> > > > @@ -28,16 +29,13 @@
> > > > #define AD7780_ID1 BIT(4)
> > > > #define AD7780_ID0 BIT(3)
> > > > #define AD7780_GAINBIT(2)
> > > > -#define AD7780_PAT1BIT(1)
> > > > -#define AD7780_PAT0BIT(0)
> > >
> > > I don't see a problem to leave the bitfields; they can be read &
> > > matched
> > > easier with the datasheet.
> > >
> > > >
> > > > -#define AD7780_PATTERN (AD7780_PAT0)
> > > > -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
> > > >
> > > > -#define AD7170_PAT2BIT(2)
> > > > +#define AD7780_PATTERN_GOOD1
> > >
> > > It was also nice before that the PAT0..PAT2 bitfields were used to
> > > define a
> > > good pattern, since it's easier to match with the datasheet.
> >
> > This one was much suggestion. Not particularly important one.
>
> Not enough sleep this week clearly :)
> This one was _my_ suggestion.
Ok.
I assumed a bit I missed a discussion somewhere.
Let's have this as-is here.
I don't mind it much either.
My [personal] feeling is that [in the context of a move-out-of-staging-of-
this-driver] this patch is a bit of noise.
If the series were more tidy-up, then I probably would not have replied.
>
> >
> > Personally if a datasheet is pointlessly confusing I tend to ignore it.
> > This is a two bit field as the bits don't have independent meaning!
> >
> > I'm not strongly tied to it though and as it's an Analog driver and
> > you all do a good job maintaining the set I'll go with your preference!
> > I do prefer the naming of PATTERN_GOOD though if nothing else!
> > >
> > >
> > > > +#define AD7780_PATTERN_MASKGENMASK(1, 0)
> > >
> > > I like the general usage of GENMASK, but I'm not sure in this case
> > > it's
> > > worth doing. Maybe I missed a discussion somewhere, about doing this
> > > change, but it is mostly a cosmetic without any functional change.
> > >
> > >
> > > >
> > > > -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > > > -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 |
> > > > AD7170_PAT2)
> > > > +#define AD7170_PATTERN_GOOD5
> > > > +#define AD7170_PATTERN_MASKGENMASK(2, 0)
> > > >
> > > > #define AD7780_GAIN_MIDPOINT 64
> > > > #define AD7780_FILTER_MIDPOINT 13350
> > > > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > > > ad7780_sigma_delta_info = {
> > > > static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > > > [ID_AD7170] = {
> > > > .channel = AD7170_CHANNEL(12, 24),
> > > > - .pattern = AD7170_PATTERN,
> > > > + .pattern = AD7170_PATTERN_GOOD,
> > > > .pattern_mask = AD7170_PATTERN_MASK,
> > > > .is_ad778x = false,
> > > > },
> > > > [ID_AD7171] = {
> > > > .channel = AD7170_CHANNEL(16, 24),
> > > > - .pattern = AD7170_PATTERN,
> > > > + .pattern = AD7170_PATTERN_GOOD,
> > > > .pattern_mask = AD7170_PATTERN_MASK,
> > > > .is_ad778x = false,
> > > > },
> > > > [ID_AD7780] = {
> > > > .channel = AD7780_CHANNEL(24, 32),
> > > > - .pattern = AD7780_PATTERN,
> > > > + .pattern = AD7780_PATTERN_GOOD,
> > > > .pattern_mask = AD7780_PATTERN_MASK,
> > > > .is_ad778x = true,
> > > > },
> > > > [ID_AD7781] = {
> > > > .channel = AD7780_CHANNEL(20, 32),
> > > > - .pattern = AD7780_PATTERN,
> > > > + .pattern = AD7780_PATTERN_GOOD,
> > > > .pattern_mask = AD7780_PATTERN_MASK,
> > > > .is_ad778x = true,
> > > > },
> > > > --
> > > > 2.21.0
> > > >
>
>
Re: [PATCH v4 4/9] staging:iio:ad7780: add chip ID values and mask
On Sun, 2019-03-03 at 14:53 +, Jonathan Cameron wrote: > [External] > > > On Sun, 3 Mar 2019 11:01:09 -0300 > Renato Lui Geh wrote: > > > On 03/01, Ardelean, Alexandru wrote: > > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote: > > > > > > > > > > > > The ad7780 supports both the ad778x and ad717x families. Each chip > > > > has > > > > a corresponding ID. This patch provides a mask for extracting ID > > > > values > > > > from the status bits and also macros for the correct values for the > > > > ad7170, ad7171, ad7780 and ad7781. > > > > > > > > Signed-off-by: Renato Lui Geh > > > > --- > > > > drivers/staging/iio/adc/ad7780.c | 8 ++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/staging/iio/adc/ad7780.c > > > > b/drivers/staging/iio/adc/ad7780.c > > > > index 56c49e28f432..ad7617a3a141 100644 > > > > --- a/drivers/staging/iio/adc/ad7780.c > > > > +++ b/drivers/staging/iio/adc/ad7780.c > > > > @@ -26,10 +26,14 @@ > > > > #define AD7780_RDY BIT(7) > > > > #define AD7780_FILTER BIT(6) > > > > #define AD7780_ERR BIT(5) > > > > -#define AD7780_ID1 BIT(4) > > > > -#define AD7780_ID0 BIT(3) > > > > #define AD7780_GAINBIT(2) > > > > > > > > +#define AD7170_ID 0 > > > > +#define AD7171_ID 1 > > > > +#define AD7780_ID 1 > > > > +#define AD7781_ID 0 > > > > + > > > > +#define AD7780_ID_MASK (BIT(3) | BIT(4)) > > > > > > This also doesn't have any functionality change. > > > The AD7170_ID, AD7171_ID, AD7780_ID & AD7781_ID IDs are also unused > > > (maybe > > > in a later patch they are ?). > > > > They aren't. I added them following a previous review suggestion. > > Should > > I remove them? > > Can we check them? It's always useful to confirm that the device is > the one you think it is. Then we can either use what is there > with a suitable warning, or if that is tricky just fault out as the > dt is giving us the wrong part number. > > J I guess `dev_warn_ratelimited()` could be used to make sure syslog isn't spammed-to-death when doing multiple conversions, and the ID isn't correct. Since these IDs are read after you get a sample, I'm a bit fearful of log- spam. I wouldn't throw an error in the ad7780_postprocess_sample() for this, but warning [with rate-limit] sounds reasonable. > > > > > > > I would also leave the AD7780_ID1 & AD7780_ID0 definitions in place, > > > since > > > they're easier matched with the datasheet. > > > > > > > > > > > #define AD7780_PATTERN_GOOD1 > > > > #define AD7780_PATTERN_MASKGENMASK(1, 0) > > > > -- > > > > 2.21.0 > > > > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
