Re: [PATCH] staging: android: ion: Change dma_buf_kmap()/dma_buf_kmap_atomic() implementation
On 02/17/2018 01:54 AM, Laura Abbott wrote:
> On 02/16/2018 04:17 AM, Alexey Skidanov wrote:
>>
>>
>> On 02/16/2018 01:48 AM, Laura Abbott wrote:
>>> On 02/12/2018 02:33 PM, Alexey Skidanov wrote:
Current ion kernel mapping implementation uses vmap() to map previously
allocated buffers into kernel virtual address space.
On 32-bit platforms, vmap() might fail on large enough buffers due
to the
limited available vmalloc space. dma_buf_kmap() should guarantee that
only one page is mapped at a time.
To fix this, kmap()/kmap_atomic() is used to implement the appropriate
interfaces.
Signed-off-by: Alexey Skidanov
---
drivers/staging/android/ion/ion.c | 97
+++
drivers/staging/android/ion/ion.h | 1 -
2 files changed, 48 insertions(+), 50 deletions(-)
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 57e0d80..75830e3 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "ion.h"
@@ -119,8 +120,6 @@ static struct ion_buffer
*ion_buffer_create(struct ion_heap *heap,
void ion_buffer_destroy(struct ion_buffer *buffer)
{
- if (WARN_ON(buffer->kmap_cnt > 0))
- buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
buffer->heap->ops->free(buffer);
kfree(buffer);
}
@@ -140,34 +139,6 @@ static void _ion_buffer_destroy(struct ion_buffer
*buffer)
ion_buffer_destroy(buffer);
}
-static void *ion_buffer_kmap_get(struct ion_buffer *buffer)
-{
- void *vaddr;
-
- if (buffer->kmap_cnt) {
- buffer->kmap_cnt++;
- return buffer->vaddr;
- }
- vaddr = buffer->heap->ops->map_kernel(buffer->heap, buffer);
- if (WARN_ONCE(!vaddr,
- "heap->ops->map_kernel should return ERR_PTR on error"))
- return ERR_PTR(-EINVAL);
- if (IS_ERR(vaddr))
- return vaddr;
- buffer->vaddr = vaddr;
- buffer->kmap_cnt++;
- return vaddr;
-}
-
-static void ion_buffer_kmap_put(struct ion_buffer *buffer)
-{
- buffer->kmap_cnt--;
- if (!buffer->kmap_cnt) {
- buffer->heap->ops->unmap_kernel(buffer->heap, buffer);
- buffer->vaddr = NULL;
- }
-}
-
static struct sg_table *dup_sg_table(struct sg_table *table)
{
struct sg_table *new_table;
@@ -305,34 +276,68 @@ static void ion_dma_buf_release(struct dma_buf
*dmabuf)
_ion_buffer_destroy(buffer);
}
+static inline int sg_page_count(struct scatterlist *sg)
+{
+ return PAGE_ALIGN(sg->offset + sg->length) >> PAGE_SHIFT;
+}
+
+static struct page *ion_dma_buf_get_page(struct sg_table *sg_table,
+ unsigned long offset)
+{
+ struct page *page;
+ struct scatterlist *sg;
+ int i;
+ unsigned int nr_pages;
+
+ nr_pages = 0;
+ page = NULL;
+ /* Find the page with specified offset */
+ for_each_sg(sg_table->sgl, sg, sg_table->nents, i) {
+ if (nr_pages + sg_page_count(sg) > offset) {
+ page = nth_page(sg_page(sg), offset - nr_pages);
+ break;
+ }
+
+ nr_pages += sg_page_count(sg);
+ }
+
+ return page;
+}
static void *ion_dma_buf_kmap(struct dma_buf *dmabuf, unsigned long
offset)
{
struct ion_buffer *buffer = dmabuf->priv;
- return buffer->vaddr + offset * PAGE_SIZE;
+ return kmap(ion_dma_buf_get_page(buffer->sg_table, offset));
}
>>>
>>> This unfortunately doesn't work for uncached buffers. We need to create
>>> an uncached alias otherwise we break what little coherency that
>>> guarantees.
>>> I'm still convinced that we can't actually do the uncached buffers
>>> correctly and they should be removed...
>>>
>>> Thanks,
>>> Laura
>>>
>> I assume that your concern is possible cache coherency issue as result
>> of the memory access with the different cache attributes. Especially, if
>> these accesses are from different context.
>>
>> But dma-buf requires that the cpu access should be started with
>> dma_buf_start_cpu_access() and ended with dma_buf_end_cpu_access() (and
>> with appropriate ioctl calls from the user space) to ensure that there
>> is no IO coherency issues. That is, the appropriate cache lines are
>> invalidated before the access and are flushed after the access (in case
>> of read/write access).
>>
>> So, it seems, that in the end of each CPU access, the most update copy
>> of the buffer is in the R
[PATCH] Staging: ks7010: fix unnecessary parentheses in ks_hostif.c
This patch fixes up unncessary parentheses warning found by checkpatch.pl script
Signed-off-by: Yash Omer
---
drivers/staging/ks7010/ks_hostif.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/ks7010/ks_hostif.c
b/drivers/staging/ks7010/ks_hostif.c
index 7bd567e233d7..36055652842d 100644
--- a/drivers/staging/ks7010/ks_hostif.c
+++ b/drivers/staging/ks7010/ks_hostif.c
@@ -36,7 +36,7 @@ inline u8 get_BYTE(struct ks_wlan_private *priv)
{
u8 data;
- data = *(priv->rxp)++;
+ data = *priv->rxp++;
/* length check in advance ! */
--(priv->rx_size);
return data;
--
2.14.3
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: ks7010: Fix Unnecessary parentheses in ks_hostif.c
On Sat, Feb 17, 2018 at 12:58:46PM +0530, Yash Omer wrote: > This patch fixes up unnecessary parenthesis issue found by checkpatch.pl > > Signed-off-by: Yash Omer > --- > drivers/staging/ks7010/ks_hostif.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/ks7010/ks_hostif.c > b/drivers/staging/ks7010/ks_hostif.c > index 975dbbb3abd0..7bd567e233d7 100644 > --- a/drivers/staging/ks7010/ks_hostif.c > +++ b/drivers/staging/ks7010/ks_hostif.c > @@ -850,7 +850,7 @@ void hostif_scan_indication(struct ks_wlan_private *priv) > DPRINTK(4, " scan_ind_count=%d :: aplist.size=%d\n", > priv->scan_ind_count, priv->aplist.size); > get_ap_information(priv, (struct ap_info_t *)(priv->rxp), > -&(priv->aplist.ap[priv->scan_ind_count - > 1])); > +&(priv->aplist.ap priv->scan_ind_count - 1)); Yash, you seem like a nice person but this isn't even a vaguely reasonable change. This isn't valid C. You really need to learn C before you can make a meaningful contribution to the kernel. :( regards, dan carpenter ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
On Mon, 12 Feb 2018 17:57:31 +0300 Dan Carpenter wrote: > On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote: > > But these should be done when we have *more* instances. > > > > For eg: > > I added a tab space in function static int adis16201_read_raw() argument > > to match open parentheses in this patch. But I also added tabs while > > removing and adding suitable suffix to the macros. So, should it also be > > done in a separate patch ? > > If you're changing a line of code and you fix a white space issue on > that same line, then that's fine. If it's just in the same function, > then do it in a separate patch. In other words, adding tabs when you're > moving around macros is fine, but adding it to the arguments is > unrelated. > > This patch was honestly pretty tricky to review. > > Jonathan assumes reviewers have the datasheet in front of them and I > assume that that they don't. He's probably right... But especially > comments like this: Actually I don't. I like the code to be very clear without the datasheet. That is one of the reasons I always advocate making it very clear what is a register and what is a field. The _REG postfix is useful to my mind for that reason. What I really don't like is needing comments to tell you what a register is for when the name of the define should make it clear. Obviously there are sometimes places you can't do this because the meaning cannot be explained in a short enough name but they are fairly rare. I agree it is a trade off on whether the naming is sufficiently clear or not and your example of the power supply one is a classic. That register has a stupid name on the datasheet given how easy it would have been to make it clear in the name choice that it was measuring the power supply. The naming things _OUT on this datasheet is particularly nasty as it adds nothing other than confusion. However, the question arises on whether it makes sense to get rid of that in the driver and make it harder to read with the datasheet. > > *val2 = 22; /* 1.22 mV */ > > They seem really helpful to me. This isn't about the data sheet, it is about knowledge of IIO. That one is perhaps debatable as the base units for voltage are less than ideal (I really wish I had been a stickler for SI units throughout from the first - this came about through trying to maintain compatibility with hwmon which with hind sight was a bad idea). > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
On Tue, 13 Feb 2018 01:16:05 +0530 Himanshu Jha wrote: > On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter wrote: > > On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote: > > > But these should be done when we have *more* instances. > > > > > > For eg: > > > I added a tab space in function static int adis16201_read_raw() argument > > > to match open parentheses in this patch. But I also added tabs while > > > removing and adding suitable suffix to the macros. So, should it also be > > > done in a separate patch ? > > > > If you're changing a line of code and you fix a white space issue on > > that same line, then that's fine. If it's just in the same function, > > then do it in a separate patch. In other words, adding tabs when you're > > moving around macros is fine, but adding it to the arguments is > > unrelated. > > I will try and divide my patches next time :) > > > This patch was honestly pretty tricky to review. > > I am sorry for that. Might be easy for IIO reviewers ;) > > > Jonathan assumes reviewers have the datasheet in front of them and I > > assume that that they don't. He's probably right... But especially > > comments like this: > > > > *val2 = 22; /* 1.22 mV */ > > They are pretty obvious as you can see from the return statements > just below that which is : > > return IIO_VAL_INT_PLUS_MICRO; > > These comments are obvious because we know 'val1' will be responsible > for Integer part(1.0) and 'val2' for the Micro part(22 * 10^-6 = 0.22). > Isn't it ? > > Although I am new to IIO please correct if I'm wrong. > The oddity here is that the base units (here mV) are inconsistent due to some ancient attempts to align with other subsystems. Dan is perhaps correct in that the comment might be helpful for that reason. If so I would prefer to see it made clear what is going on. /* Voltage base units are mV hence 1.22 mV */ Also should definitely have it's own line rather than being associated with just the val2 line like it currently is. Jonathan ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
On Mon, 12 Feb 2018 16:10:01 +0300
Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> > Use sign_extend32 function instead of manually coding it. Also, adjust a
> ^
> > switch block to explicitly match channels and return -EINVAL as default
> > case which improves code readability.
>
> Greg likes to say something along the lines of "when you start your
> sentence with "Also, " that could be a clue that it should be a separate
> patch.".
>
> >
> > Signed-off-by: Himanshu Jha
> > ---
> > drivers/staging/iio/accel/adis16201.c | 13 -
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/iio/accel/adis16201.c
> > b/drivers/staging/iio/accel/adis16201.c
> > index 011d2c5..6800347 100644
> > --- a/drivers/staging/iio/accel/adis16201.c
> > +++ b/drivers/staging/iio/accel/adis16201.c
> > @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev
> > *indio_dev,
> > case IIO_CHAN_INFO_SCALE:
> > switch (chan->type) {
> > case IIO_VOLTAGE:
> > - if (chan->channel == 0) {
> > + switch (chan->channel) {
> > + case 0:
> > *val = 1;
> > *val2 = 22;
> > - } else {
> > + break;
> > + case 1:
> > *val = 0;
> > *val2 = 61;
> > + break;
> > + default:
> > + return -EINVAL;
> > }
>
> I don't think this improves readability. The -EINVAL is to handle a
> driver bug which we haven't introduced yet. Probably we would be better
> off printing a warning or something? But it feels weird to introduce so
> much code to handle a bug which would actually be pretty difficult to
> write. The original code is fine.
Hmm. My thought here was that it is not obvious from the code
that we only have channel 0 and channel 1. The if statement
kind of implies that channel 0 is special compared to 'all the other'
elements where as what we are actually doing is picking from
a set of options. So semantically it's a switch statement being
implemented as an if else pair ;)
Perhaps we can compromise on the addition of a comment on the else
case to say it only applies to channel 1?
Dan, what do you think?
It isn't particularly important either way though so feel free to
just drop this one.
>
> > return IIO_VAL_INT_PLUS_MICRO;
> > case IIO_TEMP:
> > @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> > if (ret)
> > return ret;
> >
> > - val16 &= (1 << bits) - 1;
> > - val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> > - *val = val16;
> > + *val = sign_extend32(val16, bits - 1);
>
> Yeah. This is a nice clean up.
>
> regards,
> dan carpenter
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
On Mon, 12 Feb 2018 17:45:42 +0300 Dan Carpenter wrote: > On Mon, Feb 12, 2018 at 08:11:57PM +0530, Himanshu Jha wrote: > > On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote: > > > I think -M is prefered for these types of diffs? Not sure. > > > > I wrote about that in the cover letter if you missed. :) > > > > Yeah. I seldom read cover letters. For anyone else (like me) who also doesn't often read them... This was a specific request from me to not use move detection. The reason is that it presents an opportunity for staging graduation patches for people to have the whole code in front of them allowing them to do a full review as if it were a new driver. I personally find this very helpful in this one case. For any other code move I'm as in favour of short emails as the next person ;) Jonathan > > > > > + ret = adis_init(st, indio_dev, spi, &adis16201_data); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL); > > > > + if (ret) > > > > + return ret; > > > > > > We should clean up the IRQ which we enabled in adis_init() instead of > > > returning directly. > > > > I'm not sure about how to do that. > > > > I believe in you that you can figure it out. :) > > regards, > dan carpenter > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: ks7010: Fix Unnecessary parentheses in ks_hostif.c
On Sat, Feb 17, 2018 at 12:58:46PM +0530, Yash Omer wrote: > This patch fixes up unnecessary parenthesis issue found by checkpatch.pl > > Signed-off-by: Yash Omer > --- > drivers/staging/ks7010/ks_hostif.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/ks7010/ks_hostif.c > b/drivers/staging/ks7010/ks_hostif.c > index 975dbbb3abd0..7bd567e233d7 100644 > --- a/drivers/staging/ks7010/ks_hostif.c > +++ b/drivers/staging/ks7010/ks_hostif.c > @@ -850,7 +850,7 @@ void hostif_scan_indication(struct ks_wlan_private *priv) > DPRINTK(4, " scan_ind_count=%d :: aplist.size=%d\n", > priv->scan_ind_count, priv->aplist.size); > get_ap_information(priv, (struct ap_info_t *)(priv->rxp), > -&(priv->aplist.ap[priv->scan_ind_count - > 1])); > +&(priv->aplist.ap priv->scan_ind_count - 1)); Did you actually compile this? Please, step back and go learn the C language first really really well. The change you made here is obviously not correct at all. I suggest you work on userspace programs first for a few years before working on the kernel, it is not a place to learn how to program. good luck! greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: ks7010: fix unnecessary parentheses in ks_hostif.c
On Sat, Feb 17, 2018 at 03:26:45PM +0530, Yash Omer wrote:
> This patch fixes up unncessary parentheses warning found by checkpatch.pl
> script
>
> Signed-off-by: Yash Omer
> ---
> drivers/staging/ks7010/ks_hostif.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/ks7010/ks_hostif.c
> b/drivers/staging/ks7010/ks_hostif.c
> index 7bd567e233d7..36055652842d 100644
> --- a/drivers/staging/ks7010/ks_hostif.c
> +++ b/drivers/staging/ks7010/ks_hostif.c
> @@ -36,7 +36,7 @@ inline u8 get_BYTE(struct ks_wlan_private *priv)
> {
> u8 data;
>
> - data = *(priv->rxp)++;
> + data = *priv->rxp++;
Oh that's a mess, please just leave the original () there, otherwise you
have to really remember what order comes first here. The () gives the
reader a huge hint as to what is being incremented.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: gdm724x: tty: Fix macro argument reuse that could cause side-effects.
On Fri, Feb 16, 2018 at 02:26:55PM -0800, Quytelda Kahja wrote:
> Fix a coding style warning from checkpatch.pl. Use GNU extensions to create
> references to the results of problem macro arguments when they are evaluated
> so
> that they can be used safely multiple times.
>
> Signed-off-by: Quytelda Kahja
> ---
> drivers/staging/gdm724x/gdm_tty.c | 24
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/gdm724x/gdm_tty.c
> b/drivers/staging/gdm724x/gdm_tty.c
> index fc7682c18f20..73d39fa86d10 100644
> --- a/drivers/staging/gdm724x/gdm_tty.c
> +++ b/drivers/staging/gdm724x/gdm_tty.c
> @@ -37,14 +37,22 @@
>
> #define MUX_TX_MAX_SIZE 2048
>
> -#define gdm_tty_send(n, d, l, i, c, b) (\
> - n->tty_dev->send_func(n->tty_dev->priv_dev, d, l, i, c, b))
> -#define gdm_tty_recv(n, c) (\
> - n->tty_dev->recv_func(n->tty_dev->priv_dev, c))
> -#define gdm_tty_send_control(n, r, v, d, l) (\
> - n->tty_dev->send_control(n->tty_dev->priv_dev, r, v, d, l))
> -
> -#define GDM_TTY_READY(gdm) (gdm && gdm->tty_dev && gdm->port.count)
> +#define gdm_tty_send(n, d, l, i, c, b)
> \
> + ({ typeof(n) n_ = (n); \
> + void *priv_dev = n_->tty_dev->priv_dev; \
> + n_->tty_dev->send_func(priv_dev, d, l, i, c, b); })
> +#define gdm_tty_recv(n, c) \
> + ({ typeof(n) n_ = (n); \
> + void *priv_dev = n_->tty_dev->priv_dev; \
> + n_->tty_dev->recv_func(priv_dev, c); })
> +#define gdm_tty_send_control(n, r, v, d, l) \
> + ({ typeof(n) n_ = (n); \
> + void *priv_dev = n_->tty_dev->priv_dev; \
> + n_->tty_dev->send_control(priv_dev, r, v, d, l); })
> +
> +#define GDM_TTY_READY(gdm) \
> + ({ typeof(gdm) gdm_ = gdm; \
> + gdm_ && gdm_->tty_dev && gdm_->port.count; })
Ugh, that's a mess. How about just replacing the use of these odd
macros with the "real" call instead? The fact that they are messing
around with the tty_dev call directly is really strange and should be
made a lot more obvious, as that probably needs to be fixed up.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio/meter: add name to function definition arguments
On Fri, 16 Feb 2018 11:16:58 -0200 Rodrigo Siqueira wrote: > Hi Daniel > > > Hi Rodrigo, > > > > I think this is a nice finding. One comment inline: > > > > On Vi, 2018-02-16 at 10:50 -0200, rodrigosiqueira wrote: > > > This patch fixes the checkpatch.pl warning: > > > > > > drivers/staging/iio/meter/ade7854.h:157: WARNING: function definition > > > argument 'struct device *' should also have an identifier name... > > > > > > + int (*read_reg_32)(struct device *dev, u16 reg_address, u32 *val); > > > + int (*write_reg_8)(struct device *dev, u16 reg_address, u8 value); > > > > > > Any particular reason for using val vs value? I get that one is a pointer > > and another a plain type, but I think the name should be the same. > > Before I selected the name, I figure out that read_reg_* and write_reg_* > was assigned inside the iio/meter/ade7754-(i2c|spi).c files by function > like ade7754_*_read_reg_* and ade7754_*_write_reg_* . > > I considered to use 'value' name for both functions parameters, however, > I noticed that function ade7754_*_write_reg_* adopted the name 'value' > for the last argument and ade7754_*_read_reg_* named the last argument > as 'val'. So, for consistency sake between the header file and the c > code, I decided to use the same parameter name patterns. > Hohum. It isn't even that consistent ;) ade7754_write_reg_8 uses val and ade7754_write_reg_16 uses value. I would suggest another patch to make them all val. Thanks, Jonathan > > > thanks, > > Daniel. > > > > Thanks ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/3] adis16209 driver cleanup
This patchset has been introduced for the cleanup of adis16209 driver. This patchset cleans up miscellaneous code fragments and improves the code readability. Shreeya Patel (3): Staging: iio: adis16209: Use SPDX identifier Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement drivers/staging/iio/accel/adis16209.c | 231 -- 1 file changed, 82 insertions(+), 149 deletions(-) -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
Use SPDX identifier format instead of GPLv2. Also rearrange the headers in alphabetical order. Signed-off-by: Shreeya Patel --- drivers/staging/iio/accel/adis16209.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c index 7fcef9a..e3d9f80 100644 --- a/drivers/staging/iio/accel/adis16209.c +++ b/drivers/staging/iio/accel/adis16209.c @@ -1,19 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0+ /* * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer * * Copyright 2010 Analog Devices Inc. - * - * Licensed under the GPL-2 or later. */ #include #include #include +#include +#include #include #include #include -#include -#include #include #include -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix
Remove some unnecessary comments and therefore, add _REG suffix for registers. Group the control register and register field macros together. Also, align function arguments to match open parentheses using tabs. Signed-off-by: Shreeya Patel --- drivers/staging/iio/accel/adis16209.c | 209 +++--- 1 file changed, 69 insertions(+), 140 deletions(-) diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c index e3d9f80..6101212 100644 --- a/drivers/staging/iio/accel/adis16209.c +++ b/drivers/staging/iio/accel/adis16209.c @@ -19,136 +19,67 @@ #include #include -#define ADIS16209_STARTUP_DELAY220 /* ms */ - -/* Flash memory write count */ -#define ADIS16209_FLASH_CNT 0x00 - -/* Output, power supply */ -#define ADIS16209_SUPPLY_OUT 0x02 - -/* Output, x-axis accelerometer */ -#define ADIS16209_XACCL_OUT 0x04 - -/* Output, y-axis accelerometer */ -#define ADIS16209_YACCL_OUT 0x06 - -/* Output, auxiliary ADC input */ -#define ADIS16209_AUX_ADC0x08 - -/* Output, temperature */ -#define ADIS16209_TEMP_OUT 0x0A - -/* Output, x-axis inclination */ -#define ADIS16209_XINCL_OUT 0x0C - -/* Output, y-axis inclination */ -#define ADIS16209_YINCL_OUT 0x0E - -/* Output, +/-180 vertical rotational position */ -#define ADIS16209_ROT_OUT0x10 - -/* Calibration, x-axis acceleration offset null */ -#define ADIS16209_XACCL_NULL 0x12 - -/* Calibration, y-axis acceleration offset null */ -#define ADIS16209_YACCL_NULL 0x14 - -/* Calibration, x-axis inclination offset null */ -#define ADIS16209_XINCL_NULL 0x16 - -/* Calibration, y-axis inclination offset null */ -#define ADIS16209_YINCL_NULL 0x18 - -/* Calibration, vertical rotation offset null */ -#define ADIS16209_ROT_NULL 0x1A - -/* Alarm 1 amplitude threshold */ -#define ADIS16209_ALM_MAG1 0x20 - -/* Alarm 2 amplitude threshold */ -#define ADIS16209_ALM_MAG2 0x22 - -/* Alarm 1, sample period */ -#define ADIS16209_ALM_SMPL1 0x24 - -/* Alarm 2, sample period */ -#define ADIS16209_ALM_SMPL2 0x26 - -/* Alarm control */ -#define ADIS16209_ALM_CTRL 0x28 - -/* Auxiliary DAC data */ -#define ADIS16209_AUX_DAC0x30 - -/* General-purpose digital input/output control */ -#define ADIS16209_GPIO_CTRL 0x32 - -/* Miscellaneous control */ -#define ADIS16209_MSC_CTRL 0x34 - -/* Internal sample period (rate) control */ -#define ADIS16209_SMPL_PRD 0x36 - -/* Operation, filter configuration */ -#define ADIS16209_AVG_CNT0x38 - -/* Operation, sleep mode control */ -#define ADIS16209_SLP_CNT0x3A - -/* Diagnostics, system status register */ -#define ADIS16209_DIAG_STAT 0x3C - -/* Operation, system command register */ -#define ADIS16209_GLOB_CMD 0x3E +#define ADIS16209_STARTUP_DELAY_MS 220 +#define ADIS16209_FLASH_CNT_REG0x00 + +/* Data Output Register Definitions */ +#define ADIS16209_SUPPLY_OUT_REG 0x02 +#define ADIS16209_XACCL_OUT_REG0x04 +#define ADIS16209_YACCL_OUT_REG0x06 +#define ADIS16209_AUX_ADC_REG 0x08 +#define ADIS16209_TEMP_OUT_REG 0x0A +#define ADIS16209_XINCL_OUT_REG0x0C +#define ADIS16209_YINCL_OUT_REG0x0E +#define ADIS16209_ROT_OUT_REG 0x10 + +/* Calibration Register Definitions */ +#define ADIS16209_XACCL_NULL_REG 0x12 +#define ADIS16209_YACCL_NULL_REG 0x14 +#define ADIS16209_XINCL_NULL_REG 0x16 +#define ADIS16209_YINCL_NULL_REG 0x18 +#define ADIS16209_ROT_NULL_REG 0x1A + +/* Alarm Register Definitions */ +#define ADIS16209_ALM_MAG1_REG 0x20 +#define ADIS16209_ALM_MAG2_REG 0x22 +#define ADIS16209_ALM_SMPL1_REG0x24 +#define ADIS16209_ALM_SMPL2_REG0x26 +#define ADIS16209_ALM_CTRL_REG 0x28 + +#define ADIS16209_AUX_DAC_REG 0x30 +#define ADIS16209_GPIO_CTRL_REG0x32 +#define ADIS16209_SMPL_PRD_REG 0x36 +#define ADIS16209_AVG_CNT_REG 0x38 +#define ADIS16209_SLP_CNT_REG 0x3A /* MSC_CTRL */ -/* Self-test at power-on: 1 = disabled, 0 = enabled */ +#define ADIS16209_MSC_CTRL_REG 0x34 #define ADIS16209_MSC_CTRL_PWRUP_SELF_TEST BIT(10) - -/* Self-test enable */ -#define ADIS16209_MSC_CTRL_SELF_TEST_ENBIT(8) - -/* Data-ready enable: 1 = enabled, 0 = disabled */ -#define ADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2) - -/* Data-ready polarity: 1 = active high, 0 = active low */ -#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1) - -/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */ -#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2 BIT(0) +#define ADIS16209_MSC_CTRL_SELF_TEST_ENBIT(8) +#define ADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2) +#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1) +#define ADIS16209_MSC_CTRL_DATA_RDY_DIO1 BIT(0) /* DIAG_STAT */ -/*
[PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement
Use sign_extend32 function instead of manually coding it. Also, adjust a
switch block to explicitly match channels and return -EINVAL as default
case which improves code readability.
Signed-off-by: Shreeya Patel
---
drivers/staging/iio/accel/adis16209.c | 15 ++-
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16209.c
b/drivers/staging/iio/accel/adis16209.c
index 6101212..f7c228b 100644
--- a/drivers/staging/iio/accel/adis16209.c
+++ b/drivers/staging/iio/accel/adis16209.c
@@ -150,10 +150,16 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
switch (chan->type) {
case IIO_VOLTAGE:
*val = 0;
- if (chan->channel == 0)
+ switch (chan->channel) {
+ case 0:
*val2 = 305180;
- else
+ break;
+ case 1:
*val2 = 610500;
+ break;
+ default:
+ return -EINVAL;
+ }
return IIO_VAL_INT_PLUS_MICRO;
case IIO_TEMP:
*val = -470;
@@ -187,9 +193,8 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
ret = adis_read_reg_16(st, addr, &val16);
if (ret)
return ret;
- val16 &= (1 << bits) - 1;
- val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
- *val = val16;
+
+ *val = sign_extend32(val16, bits - 1);
return IIO_VAL_INT;
}
return -EINVAL;
--
2.7.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
On Sat, Feb 17, 2018 at 12:23:53PM +, Jonathan Cameron wrote:
> On Mon, 12 Feb 2018 16:10:01 +0300
> Dan Carpenter wrote:
>
> > On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> > > Use sign_extend32 function instead of manually coding it. Also, adjust a
> > ^
> > > switch block to explicitly match channels and return -EINVAL as default
> > > case which improves code readability.
> >
> > Greg likes to say something along the lines of "when you start your
> > sentence with "Also, " that could be a clue that it should be a separate
> > patch.".
> >
> > >
> > > Signed-off-by: Himanshu Jha
> > > ---
> > > drivers/staging/iio/accel/adis16201.c | 13 -
> > > 1 file changed, 8 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/accel/adis16201.c
> > > b/drivers/staging/iio/accel/adis16201.c
> > > index 011d2c5..6800347 100644
> > > --- a/drivers/staging/iio/accel/adis16201.c
> > > +++ b/drivers/staging/iio/accel/adis16201.c
> > > @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev
> > > *indio_dev,
> > > case IIO_CHAN_INFO_SCALE:
> > > switch (chan->type) {
> > > case IIO_VOLTAGE:
> > > - if (chan->channel == 0) {
> > > + switch (chan->channel) {
> > > + case 0:
> > > *val = 1;
> > > *val2 = 22;
> > > - } else {
> > > + break;
> > > + case 1:
> > > *val = 0;
> > > *val2 = 61;
> > > + break;
> > > + default:
> > > + return -EINVAL;
> > > }
> >
> > I don't think this improves readability. The -EINVAL is to handle a
> > driver bug which we haven't introduced yet. Probably we would be better
> > off printing a warning or something? But it feels weird to introduce so
> > much code to handle a bug which would actually be pretty difficult to
> > write. The original code is fine.
>
> Hmm. My thought here was that it is not obvious from the code
> that we only have channel 0 and channel 1. The if statement
> kind of implies that channel 0 is special compared to 'all the other'
> elements where as what we are actually doing is picking from
> a set of options. So semantically it's a switch statement being
> implemented as an if else pair ;)
>
> Perhaps we can compromise on the addition of a comment on the else
> case to say it only applies to channel 1?
>
> Dan, what do you think?
>
> It isn't particularly important either way though so feel free to
> just drop this one.
>
To be honest, I dont care either way... The original and the new code
are equivalently clean to me so I have a "leave the code as-is bias" but
if someone else is invested in this code then I like to let the person
who cares the most be the one to decide.
This patch is actually fine but the patch description makes it sound
like it's doing two things. If the subject was
"cleanup adis16201_read_raw()" then that would sound like one thing.
I obviously review thousands of staging patches so some of my responses
are pretty mechanical at this point. If it's a two random things from
the same file then split it into to two patches, but if it's from the
same function that's acceptable.
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: gdm724x: mux: Check return value of register_lte_tty_driver().
Check the return value of of the register_lte_tty_driver() call in the
module initialization function.
Signed-off-by: Quytelda Kahja
---
drivers/staging/gdm724x/gdm_mux.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/gdm724x/gdm_mux.c
b/drivers/staging/gdm724x/gdm_mux.c
index 996b1f538aae..63921bad519e 100644
--- a/drivers/staging/gdm724x/gdm_mux.c
+++ b/drivers/staging/gdm724x/gdm_mux.c
@@ -657,7 +657,11 @@ static struct usb_driver gdm_mux_driver = {
static int __init gdm_usb_mux_init(void)
{
- register_lte_tty_driver();
+ int ret;
+
+ ret = register_lte_tty_driver();
+ if (ret)
+ return ret;
return usb_register(&gdm_mux_driver);
}
--
2.16.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
