Re: [Outreachy kernel] [PATCH v2] staging: speakup: Comparison to NULL could be written
On Thu, Mar 02, 2017 at 02:13:02PM +0530, Arushi Singhal wrote:
> Fixed coding style for null comparisons in speakup driver to be more
> consistant with the rest of the kernel coding style.
>
> Signed-off-by: Arushi Singhal
> ---
> changes in v2
> - fixed coding style error and upto the coding style.
Hi Arushi,
Take another look at Joe's message about checkpatch on the patch
file. Looks like you missed that.
Here's some tips on styling your commit and log messages:
You've probably seen feedback of putting the message into
the "imperative". ie...it's as if you are directing/commanding
what is to happen. Best way I've found to get that to sink in
is to look at your predecessors...and look at more than one
because poor messages do sneak in occasionally.
For this one, I might do this:
> staging/speakup$ gitpretty *.c | grep NULL
> (my alias: alias gitpretty='git log --pretty=oneline --abbrev-commit')
>
> d3da1cb staging: speakup: (coding style) Rewrite comparisons to NULL
> a90624c Staging: speakup: kobjects.c: Remove explicit NULL comparison
> 114885e Staging: speakup: serialio.c: Remove explicit NULL comparison
> 562c4798 Staging: speakup: devsynth.c: Remove explicit NULL comparison
> ff52fc3 Staging: speakup: varhandlers.c: Remove explicit NULL comparison
>
Then, go further into one that looks like it might match your change:
> staging/speakup$ git log a90624c
> commit a90624cf253cc74e9464b42d54aa4825575edefe
> Author: Shraddha Barke
> Date: Fri Sep 11 11:32:28 2015 +0530
>
> Staging: speakup: kobjects.c: Remove explicit NULL comparison
> >
> Remove explicit NULL comparison and write it in its simpler form.
>
>
This would give me confidence in the commit message and log.
And, since I tend toward the obsessive ;), if the next day I do this
fix in another directory, I would repeat this process on that directory
and file because styles can vary by driver/subsystem. Perhaps not on
such a simple fix, but more so as you dive deeper.
alisons
>
> drivers/staging/speakup/fakekey.c | 2 +-
> drivers/staging/speakup/kobjects.c | 2 +-
> drivers/staging/speakup/main.c | 38
> +++---
> 3 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/staging/speakup/fakekey.c
> b/drivers/staging/speakup/fakekey.c
> index d76da0a1382c..294c74b47224 100644
> --- a/drivers/staging/speakup/fakekey.c
> +++ b/drivers/staging/speakup/fakekey.c
> @@ -56,7 +56,7 @@ int speakup_add_virtual_keyboard(void)
>
> void speakup_remove_virtual_keyboard(void)
> {
> - if (virt_keyboard != NULL) {
> + if (virt_keyboard) {
> input_unregister_device(virt_keyboard);
> virt_keyboard = NULL;
> }
> diff --git a/drivers/staging/speakup/kobjects.c
> b/drivers/staging/speakup/kobjects.c
> index 5d871ec3693c..2fef55569bfd 100644
> --- a/drivers/staging/speakup/kobjects.c
> +++ b/drivers/staging/speakup/kobjects.c
> @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct
> kobj_attribute *attr,
> len--;
> new_synth_name[len] = '\0';
> spk_strlwr(new_synth_name);
> - if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
> + if (synth && !strcmp(new_synth_name, synth->name)) {
> pr_warn("%s already in use\n", new_synth_name);
> } else if (synth_init(new_synth_name) != 0) {
> pr_warn("failed to init synth %s\n", new_synth_name);
> diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> index c2f70ef5b9b3..a12ec2b061fe 100644
> --- a/drivers/staging/speakup/main.c
> +++ b/drivers/staging/speakup/main.c
> @@ -299,7 +299,7 @@ static void speakup_shut_up(struct vc_data *vc)
> spk_shut_up |= 0x01;
> spk_parked &= 0xfe;
> speakup_date(vc);
> - if (synth != NULL)
> + if (synth)
> spk_do_flush();
> }
>
> @@ -441,7 +441,7 @@ static void speak_char(u_char ch)
> synth_printf("%s", spk_str_caps_stop);
> return;
> }
> - if (cp == NULL) {
> + if (!cp) {
> pr_info("speak_char: cp == NULL!\n");
> return;
> }
> @@ -1157,7 +1157,7 @@ static void do_handle_shift(struct vc_data *vc, u_char
> value, char up_flag)
> {
> unsigned long flags;
>
> - if (synth == NULL || up_flag || spk_killed)
> + if (!synth || up_flag || spk_killed)
> return;
> spin_lock_irqsave(&speakup_info.spinlock, flags);
> if (cursor_track == read_all_mode) {
> @@ -1195,7 +1195,7 @@ static void do_handle_latin(struct vc_data *vc, u_char
> value, char up_flag)
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> return;
> }
> - if (synth == NULL || spk_killed) {
> + if (!synth || spk_killed) {
> spin_unlock_irqrestore(&speakup_info.spinlock, flags);
> return;
> }
> @@ -1279,7 +1279,7 @@ void spk_reset_def
Re: [Outreachy kernel] [PATCH 1/4] staging: speakup: Placed Logical on the previous line
On Thu, Mar 02, 2017 at 09:05:57PM +0530, Arushi Singhal wrote: > Placed Logical continuations on the previous line as reported by > checkpatch.pl. > > Signed-off-by: Arushi Singhal Hi Arushi, I'm not seeing the patch cover letter for this one. That would be your [PATCH 0/4] and would come first and then 1 through 4 follow threaded as your've done. Stating in imperative might look like this: place logical continuation on previous line insert spaces around operator use tabs for indentation align open parenthesis I didn't look into the changes themselves. alisons > --- > drivers/staging/speakup/main.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c > index a12ec2b061fe..25acebb9311f 100644 > --- a/drivers/staging/speakup/main.c > +++ b/drivers/staging/speakup/main.c > @@ -2144,10 +2144,10 @@ speakup_key(struct vc_data *vc, int shift_state, int > keycode, u_short keysym, > if (up_flag || spk_killed || type == KT_SHIFT) > goto out; > spk_shut_up &= 0xfe; > - kh = (value == KVAL(K_DOWN)) > - || (value == KVAL(K_UP)) > - || (value == KVAL(K_LEFT)) > - || (value == KVAL(K_RIGHT)); > + kh = (value == KVAL(K_DOWN)) || > + (value == KVAL(K_UP)) || > + (value == KVAL(K_LEFT)) || > + (value == KVAL(K_RIGHT)); > if ((cursor_track != read_all_mode) || !kh) > if (!spk_no_intr) > spk_do_flush(); > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170302153600.13803-1-arushisinghal19971997%40gmail.com. > For more options, visit https://groups.google.com/d/optout. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH 1/6] staging: wlan-ng: Fix sparse warning of restricted __le16
On Thu, Mar 02, 2017 at 12:14:40PM +0100, Julia Lawall wrote: > > > On Thu, 2 Mar 2017, SIMRAN SINGHAL wrote: > > > On Thu, Mar 2, 2017 at 3:20 PM, Julia Lawall wrote: > > > > > > > > > On Thu, 2 Mar 2017, Julia Lawall wrote: > > > > > >> > > >> > > >> On Thu, 2 Mar 2017, simran singhal wrote: > > >> > > >> > This patch fixes the following sparse warning: > > >> > warning: cast to restricted __le16 > > >> > > >> You commit message should not say just fix X. What have you done to > > >> carry > > >> out the fix and why is this the correct approach? > > > > > > This comment applies to all of the patches in the series. > > > > > > julia > > > > > The changes and sparse warnings for them seems reasonable to me as > > after doing the > > change sparse was not showing error like cast to __le16 or something like > > this. > > Also it compiled perfectly. > > This doesn't mean that it actually works. The function calls at least may > have been put there for a reason. Maysbe you can find other patches that > relate to this call and see what they do. > > julia Hi Simran, Just following on Julia's feedback and I see you've gotten a message that this breaks little endian machines. Good for you for trying to go deeper. I'd suggest taking this one at a time and figure out what is going on with one of them. As Julia says, with this kind of change, you will have to prove why it is correct. (You might not be able to change any of these - I don't know, but I know you'll learn something!) Look here - and then pick one to trace through - https://kernelnewbies.org/EndianIssues Generally - probably best to test the waters with something of this type, with a single patch. Get that nailed, then move onto a patchset, if you have a pattern. Heads down & good luck! alisons > > > > >> > > >> julia > > >> > > >> > > > >> > Signed-off-by: simran singhal > > >> > --- > > >> > drivers/staging/wlan-ng/prism2sta.c | 51 > > >> > ++--- > > >> > 1 file changed, 25 insertions(+), 26 deletions(-) > > >> > > > >> > diff --git a/drivers/staging/wlan-ng/prism2sta.c > > >> > b/drivers/staging/wlan-ng/prism2sta.c > > >> > index 984804b..82d3a70 100644 > > >> > --- a/drivers/staging/wlan-ng/prism2sta.c > > >> > +++ b/drivers/staging/wlan-ng/prism2sta.c > > >> > @@ -372,10 +372,9 @@ static int prism2sta_mlmerequest(struct > > >> > wlandevice *wlandev, > > >> > qualmsg->noise.status = > > >> > P80211ENUM_msgitem_status_data_ok; > > >> > > > >> > - qualmsg->link.data = > > >> > le16_to_cpu(hw->qual.cq_curr_bss); > > >> > - qualmsg->level.data = > > >> > - le16_to_cpu(hw->qual.asl_curr_bss); > > >> > - qualmsg->noise.data = > > >> > le16_to_cpu(hw->qual.anl_curr_fc); > > >> > + qualmsg->link.data = hw->qual.cq_curr_bss; > > >> > + qualmsg->level.data = hw->qual.asl_curr_bss; > > >> > + qualmsg->noise.data = hw->qual.anl_curr_fc; > > >> > qualmsg->txrate.data = hw->txrate; > > >> > > > >> > break; > > >> > @@ -603,10 +602,10 @@ static int prism2sta_getcardinfo(struct > > >> > wlandevice *wlandev) > > >> > } > > >> > > > >> > /* get all the nic id fields in host byte order */ > > >> > - hw->ident_nic.id = le16_to_cpu(hw->ident_nic.id); > > >> > - hw->ident_nic.variant = le16_to_cpu(hw->ident_nic.variant); > > >> > - hw->ident_nic.major = le16_to_cpu(hw->ident_nic.major); > > >> > - hw->ident_nic.minor = le16_to_cpu(hw->ident_nic.minor); > > >> > + hw->ident_nic.id = hw->ident_nic.id; > > >> > + hw->ident_nic.variant = hw->ident_nic.variant; > > >> > + hw->ident_nic.major = hw->ident_nic.major; > > >> > + hw->ident_nic.minor = hw->ident_nic.minor; > > >> > > > >> > netdev_info(wlandev->netdev, "ident: nic h/w: id=0x%02x > > >> > %d.%d.%d\n", > > >> > hw->ident_nic.id, hw->ident_nic.major, > > >> > @@ -622,10 +621,10 @@ static int prism2sta_getcardinfo(struct > > >> > wlandevice *wlandev) > > >> > } > > >> > > > >> > /* get all the private fw id fields in host byte order */ > > >> > - hw->ident_pri_fw.id = le16_to_cpu(hw->ident_pri_fw.id); > > >> > - hw->ident_pri_fw.variant = le16_to_cpu(hw->ident_pri_fw.variant); > > >> > - hw->ident_pri_fw.major = le16_to_cpu(hw->ident_pri_fw.major); > > >> > - hw->ident_pri_fw.minor = le16_to_cpu(hw->ident_pri_fw.minor); > > >> > + hw->ident_pri_fw.id = hw->ident_pri_fw.id; > > >> > + hw->ident_pri_fw.variant = hw->ident_pri_fw.variant; > > >> > + hw->ident_pri_fw.major = hw->ident_pri_fw.major; > > >> > + hw->ident_pri_fw.minor = hw->ident_pri_fw.minor; > > >> > > > >> > netdev_info(wlandev->netdev, "ident: pri f/w: id=0x%02x > > >> > %d.%d.%d\n", > > >> > hw->ident_pri_fw.id, hw->ident_pri_fw.major, > > >> > @@ -648,10 +647,10 @@ static int prism2sta_
Re: [Outreachy kernel] [PATCH v2] staging: speakup: Comparison to NULL could be written
On Thu, Mar 02, 2017 at 08:37:21AM -0800, Alison Schofield wrote:
> On Thu, Mar 02, 2017 at 02:13:02PM +0530, Arushi Singhal wrote:
> > Fixed coding style for null comparisons in speakup driver to be more
> > consistant with the rest of the kernel coding style.
> >
> > Signed-off-by: Arushi Singhal
> > ---
> > changes in v2
> > - fixed coding style error and upto the coding style.
>
> Hi Arushi,
>
> Take another look at Joe's message about checkpatch on the patch
> file. Looks like you missed that.
Arushi,
Looking further, seems you only missed the spelling warning:
WARNING: 'consistant' may be misspelled - perhaps 'consistent'?
You would be missing this if you are running checkpatch on your
.c file, but not doing the checkpatch "hook" from the firstpatch
tutorial. That hook would catch this. And, you can just catch
it by running checkpatch on the patch file after it's built.
(The hook is a painless failsafe...recommend using it.)
alisons
>
> Here's some tips on styling your commit and log messages:
>
> You've probably seen feedback of putting the message into
> the "imperative". ie...it's as if you are directing/commanding
> what is to happen. Best way I've found to get that to sink in
> is to look at your predecessors...and look at more than one
> because poor messages do sneak in occasionally.
>
> For this one, I might do this:
>
> > staging/speakup$ gitpretty *.c | grep NULL
> > (my alias: alias gitpretty='git log --pretty=oneline --abbrev-commit')
> >
> > d3da1cb staging: speakup: (coding style) Rewrite comparisons to NULL
> > a90624c Staging: speakup: kobjects.c: Remove explicit NULL comparison
> > 114885e Staging: speakup: serialio.c: Remove explicit NULL comparison
> > 562c4798 Staging: speakup: devsynth.c: Remove explicit NULL comparison
> > ff52fc3 Staging: speakup: varhandlers.c: Remove explicit NULL comparison
> >
> Then, go further into one that looks like it might match your change:
>
> > staging/speakup$ git log a90624c
> > commit a90624cf253cc74e9464b42d54aa4825575edefe
> > Author: Shraddha Barke
> > Date: Fri Sep 11 11:32:28 2015 +0530
> >
> > Staging: speakup: kobjects.c: Remove explicit NULL comparison
> > >
> > Remove explicit NULL comparison and write it in its simpler form.
> >
> >
>
> This would give me confidence in the commit message and log.
> And, since I tend toward the obsessive ;), if the next day I do this
> fix in another directory, I would repeat this process on that directory
> and file because styles can vary by driver/subsystem. Perhaps not on
> such a simple fix, but more so as you dive deeper.
>
> alisons
>
> >
> > drivers/staging/speakup/fakekey.c | 2 +-
> > drivers/staging/speakup/kobjects.c | 2 +-
> > drivers/staging/speakup/main.c | 38
> > +++---
> > 3 files changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/staging/speakup/fakekey.c
> > b/drivers/staging/speakup/fakekey.c
> > index d76da0a1382c..294c74b47224 100644
> > --- a/drivers/staging/speakup/fakekey.c
> > +++ b/drivers/staging/speakup/fakekey.c
> > @@ -56,7 +56,7 @@ int speakup_add_virtual_keyboard(void)
> >
> > void speakup_remove_virtual_keyboard(void)
> > {
> > - if (virt_keyboard != NULL) {
> > + if (virt_keyboard) {
> > input_unregister_device(virt_keyboard);
> > virt_keyboard = NULL;
> > }
> > diff --git a/drivers/staging/speakup/kobjects.c
> > b/drivers/staging/speakup/kobjects.c
> > index 5d871ec3693c..2fef55569bfd 100644
> > --- a/drivers/staging/speakup/kobjects.c
> > +++ b/drivers/staging/speakup/kobjects.c
> > @@ -391,7 +391,7 @@ static ssize_t synth_store(struct kobject *kobj, struct
> > kobj_attribute *attr,
> > len--;
> > new_synth_name[len] = '\0';
> > spk_strlwr(new_synth_name);
> > - if ((synth != NULL) && (!strcmp(new_synth_name, synth->name))) {
> > + if (synth && !strcmp(new_synth_name, synth->name)) {
> > pr_warn("%s already in use\n", new_synth_name);
> > } else if (synth_init(new_synth_name) != 0) {
> > pr_warn("failed to init synth %s\n", new_synth_name);
> > diff --git a/drivers/staging/speakup/main.c b/drivers/staging/speakup/main.c
> > index c2f70ef5b9b3..a12ec2b061fe 100644
> > --- a/drivers/staging/speakup/main.c
> > +++ b/drivers/staging/speakup/main.c
> > @@ -299,7 +299
Re: [Outreachy kernel] Re: [PATCH v2 4/6] staging: fbtft: Fix sparse warnings of incorrect type in assignment
On Thu, Mar 02, 2017 at 02:26:37PM +0100, Noralf Trønnes wrote: > > Den 02.03.2017 14.04, skrev simran singhal: > >This patch fixes the following sparse warnings: > > > >drivers/staging/fbtft/fbtft-bus.c:166:36: warning: incorrect type in > >assignment (different base types) > >drivers/staging/fbtft/fbtft-bus.c:166:36:expected unsigned short > >[unsigned] [short] [usertype] > >drivers/staging/fbtft/fbtft-bus.c:166:36:got restricted __be16 > >[usertype] > > > >drivers/staging/fbtft/fbtft-io.c:74:29: warning: incorrect type in > >assignment (different base types) > >drivers/staging/fbtft/fbtft-io.c:74:29:expected unsigned long long > >[unsigned] [long] [long long] [usertype] > >drivers/staging/fbtft/fbtft-io.c:74:29:got restricted __be64 [usertype] > > > > > >Signed-off-by: simran singhal > >--- > > v2: > >-changed commit message > > > > drivers/staging/fbtft/fbtft-bus.c | 2 +- > > drivers/staging/fbtft/fbtft-io.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/staging/fbtft/fbtft-bus.c > >b/drivers/staging/fbtft/fbtft-bus.c > >index ec45043..df2223e 100644 > >--- a/drivers/staging/fbtft/fbtft-bus.c > >+++ b/drivers/staging/fbtft/fbtft-bus.c > >@@ -163,7 +163,7 @@ int fbtft_write_vmem16_bus8(struct fbtft_par *par, > >size_t offset, size_t len) > > to_copy, remain - to_copy); > > for (i = 0; i < to_copy; i++) > >-txbuf16[i] = cpu_to_be16(vmem16[i]); > >+txbuf16[i] = vmem16[i]; > > This change breaks functionality on little endian machines like > the Raspberry Pi. > > > Noralf. > Hi Simran, It's probably good to get back to this one while we have Noralf's attention. While the change above - in fbtft-bus.c is a problem, the change below in fbtft-io.c looks ok. So, compare what you did. You can't *not* do the endian conversion. This is a cleanup change, a cosmetic change. There is no underlying bug, so you just need to get the typing correct w/out affecting behavior. BTW: I think it's OK to pull this one out and send a v3 of this alone. That would mean abandoning the patchset and doing them one at a time. They are all in different drivers anyway. alisons > > vmem16 = vmem16 + to_copy; > > ret = par->fbtftops.write(par, par->txbuf.buf, > >diff --git a/drivers/staging/fbtft/fbtft-io.c > >b/drivers/staging/fbtft/fbtft-io.c > >index d868405..ffb9a3b 100644 > >--- a/drivers/staging/fbtft/fbtft-io.c > >+++ b/drivers/staging/fbtft/fbtft-io.c > >@@ -71,7 +71,7 @@ int fbtft_write_spi_emulate_9(struct fbtft_par *par, void > >*buf, size_t len) > > src++; > > } > > tmp |= ((*src & 0x0100) ? 1 : 0); > >-*(u64 *)dst = cpu_to_be64(tmp); > >+*(__be64 *)dst = cpu_to_be64(tmp); > > dst += 8; > > *dst++ = (u8)(*src++ & 0x00FF); > > added++; > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/fe8d6a85-3d4e-8019-937b-22389b942da3%40tronnes.org. > For more options, visit https://groups.google.com/d/optout. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v3 4/6] staging: fbtft: Fix sparse warnings of incorrect type in assignment
On Sun, Mar 05, 2017 at 10:35:33AM +0530, simran singhal wrote: > This patch fixes the following sparse warnings: > > drivers/staging/fbtft/fbtft-io.c:74:29: warning: incorrect type in assignment > (different base types) > drivers/staging/fbtft/fbtft-io.c:74:29:expected unsigned long long > [unsigned] [long] [long long] [usertype] > drivers/staging/fbtft/fbtft-io.c:74:29:got restricted __be64 [usertype] > > > Signed-off-by: simran singhal > --- > > v3: >-Change commit message >-Drop the changes did in fbtft-bus.c Hi Simran, Sorry, I didn't mean to drop the broken part of fbtft, but rather to fix it. Let's tidy up a few things: - we can't send out one patch labelled as part of a patchset without resending the entire set. That's why I was suggesting just abandoning the patchset, and beginning to rework this anew with only the fbtft driver in a single patch. - I suggest keeping the -bus and -io file changes together. - Look to correct what is wrong with fbtft-bus.c before sending a new patch. - Patch Subject would be something like: use __be64 types for endian correctness (if this is what you end up doing - hint) So, next step, 1) read up on endianess and propose how to change -bus.c. OK to check that idea/code snippet here without re-rolling the patch. https://kernelnewbies.org/EndianIssues and Vaishali also gave a good link to lwn.net article and to see some example fixes (in iio subsystem) do this: git log --pretty=oneline --abbrev-commit | grep iio | grep endian 2) build the patch. At this point I say make it a first version and start totally fresh. However, below the line --- where you usually put the version comments, you can add a comment stating that this was previously part of a patchset, but is now a single patch. alisons > > drivers/staging/fbtft/fbtft-io.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fbtft-io.c > b/drivers/staging/fbtft/fbtft-io.c > index d868405..ffb9a3b 100644 > --- a/drivers/staging/fbtft/fbtft-io.c > +++ b/drivers/staging/fbtft/fbtft-io.c > @@ -71,7 +71,7 @@ int fbtft_write_spi_emulate_9(struct fbtft_par *par, void > *buf, size_t len) > src++; > } > tmp |= ((*src & 0x0100) ? 1 : 0); > - *(u64 *)dst = cpu_to_be64(tmp); > + *(__be64 *)dst = cpu_to_be64(tmp); > dst += 8; > *dst++ = (u8)(*src++ & 0x00FF); > added++; > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170305050533.GA10169%40singhal-Inspiron-5558. > For more options, visit https://groups.google.com/d/optout. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: media: Remove unnecessary function and its call
On Sun, Mar 05, 2017 at 12:17:21PM +0530, simran singhal wrote:
> The function atomisp_set_stop_timeout on being called, simply returns
> back. The function hasn't been mentioned in the TODO and doesn't have
> FIXME code around. Hence, atomisp_set_stop_timeout and its calls have been
> removed.
>
> Signed-off-by: simran singhal
> ---
Hi Simran,
It's helpful to state right in the subject line what you removed.
ie. remove unused function atomisp_set_stop_timeout()
If you do that, scan's or grep'ing the git log pretty oneline's can
easily see this without having to dig into the log.
(gitpretty='git log --pretty=oneline --abbrev-commit')
Can you share to Outreachy group how you found this? By inspection
or otherwise??
Thanks,
alisons
alisons
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c | 1 -
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h | 1 -
> drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c | 5 -
> 3 files changed, 7 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> index e99f7b8..66299dd 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_cmd.c
> @@ -1700,7 +1700,6 @@ void atomisp_wdt_work(struct work_struct *work)
> }
> }
> #endif
> - atomisp_set_stop_timeout(ATOMISP_CSS_STOP_TIMEOUT_US);
> dev_err(isp->dev, "timeout recovery handling done\n");
> atomic_set(&isp->wdt_work_queued, 0);
>
> diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> index 5a404e4..0b9ced5 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat.h
> @@ -660,7 +660,6 @@ int atomisp_css_set_acc_parameters(struct atomisp_acc_fw
> *acc_fw);
> int atomisp_css_isr_thread(struct atomisp_device *isp,
> bool *frame_done_found,
> bool *css_pipe_done);
> -void atomisp_set_stop_timeout(unsigned int timeout);
>
> bool atomisp_css_valid_sof(struct atomisp_device *isp);
>
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> index 6697d72..cfa0ad4 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_compat_css20.c
> @@ -4699,11 +4699,6 @@ int atomisp_css_isr_thread(struct atomisp_device *isp,
> return 0;
> }
>
> -void atomisp_set_stop_timeout(unsigned int timeout)
> -{
> - return;
> -}
> -
> bool atomisp_css_valid_sof(struct atomisp_device *isp)
> {
> unsigned int i, j;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170305064721.GA22548%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: adis16060_core: Use private driver lock instead of mlock
On Sun, Mar 12, 2017 at 06:40:52PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: simran singhal
Hi Simran,
These are now IIO tasks for this round of Outreachy. Please
follow the directions on the IIO task page w.r.t. claiming,
sending, and all.
Thanks,
alisons
> ---
> drivers/staging/iio/gyro/adis16060_core.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..90a3a18 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -29,11 +29,13 @@
> * @us_r:actual spi_device to read back data
> * @buf: transmit or receive buffer
> * @buf_lock:mutex to protect tx and rx
> + * @lock:protect sensor state
> **/
> struct adis16060_state {
> struct spi_device *us_w;
> struct spi_device *us_r;
> struct mutexbuf_lock;
> + struct mutexlock;/* protect sensor state */
>
> u8 buf[3] cacheline_aligned;
> };
> @@ -87,7 +89,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = adis16060_spi_write(indio_dev, chan->address);
> if (ret < 0)
> goto out_unlock;
> @@ -96,7 +98,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out_unlock;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +114,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170312131052.GA21816%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: adis16060_core: Use private driver lock instead of mlock
On Sun, Mar 12, 2017 at 06:40:52PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: simran singhal
This does not compile.
alisons
> ---
> drivers/staging/iio/gyro/adis16060_core.c | 8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..90a3a18 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -29,11 +29,13 @@
> * @us_r:actual spi_device to read back data
> * @buf: transmit or receive buffer
> * @buf_lock:mutex to protect tx and rx
> + * @lock:protect sensor state
> **/
> struct adis16060_state {
> struct spi_device *us_w;
> struct spi_device *us_r;
> struct mutexbuf_lock;
> + struct mutexlock;/* protect sensor state */
>
> u8 buf[3] cacheline_aligned;
> };
> @@ -87,7 +89,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = adis16060_spi_write(indio_dev, chan->address);
> if (ret < 0)
> goto out_unlock;
> @@ -96,7 +98,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out_unlock;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +114,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170312131052.GA21816%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock
On Sun, Mar 12, 2017 at 07:02:50PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Fix some coding style issues related to white space also.
>
> Signed-off-by: simran singhal
Hi Simran, This looks good to me. Let's see what the
reviewers say. I think the white space stuff is ok,
since it was right where you were editing.
alisons
> ---
> drivers/staging/iio/meter/ade7753.c | 14 --
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..ca99d82 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -81,12 +81,14 @@
> * @tx: transmit buffer
> * @rx: receive buffer
> * @buf_lock: mutex to protect tx and rx
> + * @lock:protect sensor state
> **/
> struct ade7753_state {
> - struct spi_device *us;
> - struct mutexbuf_lock;
> - u8 tx[ADE7753_MAX_TX]
> cacheline_aligned;
> - u8 rx[ADE7753_MAX_RX];
> + struct spi_device *us;
> + struct mutexbuf_lock;
> + struct mutexlock; /* protect sensor state */
> + u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> + u8 rx[ADE7753_MAX_RX];
> };
>
> static int ade7753_spi_write_reg_8(struct device *dev,
> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> if (!val)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
>
> t = 27900 / val;
> if (t > 0)
> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret ? ret : len;
> }
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH] staging: iio: ade7753: replace mlock with driver private lock
On Mon, Mar 13, 2017 at 09:28:34AM +0530, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 12:03 AM, Alison Schofield
> wrote:
> > On Sun, Mar 12, 2017 at 07:02:50PM +0530, simran singhal wrote:
> >> The IIO subsystem is redefining iio_dev->mlock to be used by
> >> the IIO core only for protecting device operating mode changes.
> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> >>
> >> In this driver, mlock was being used to protect hardware state
> >> changes. Replace it with a lock in the devices global data.
> >>
> >> Fix some coding style issues related to white space also.
> >>
> >> Signed-off-by: simran singhal
> >
> > Hi Simran, This looks good to me. Let's see what the
> > reviewers say. I think the white space stuff is ok,
> > since it was right where you were editing.
> > alisons
> >
> Alison, so sending this patch here on outreachy mailing list is fine.
> Still confuse :P
You are OK. You sent it to everyone suggested in the Task Description.
This patch was sent before I posted the Task Description. I'm assuming
that since then you've found the posted Task:
https://kernelnewbies.org/IIO_tasks
Find Coding Task 2 --> "PATCHES need to be sent to all of:"
>
> >> ---
> >> drivers/staging/iio/meter/ade7753.c | 14 --
> >> 1 file changed, 8 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/meter/ade7753.c
> >> b/drivers/staging/iio/meter/ade7753.c
> >> index dfd8b71..ca99d82 100644
> >> --- a/drivers/staging/iio/meter/ade7753.c
> >> +++ b/drivers/staging/iio/meter/ade7753.c
> >> @@ -81,12 +81,14 @@
> >> * @tx: transmit buffer
> >> * @rx: receive buffer
> >> * @buf_lock: mutex to protect tx and rx
> >> + * @lock:protect sensor state
> >> **/
> >> struct ade7753_state {
> >> - struct spi_device *us;
> >> - struct mutexbuf_lock;
> >> - u8 tx[ADE7753_MAX_TX]
> >> cacheline_aligned;
> >> - u8 rx[ADE7753_MAX_RX];
> >> + struct spi_device *us;
> >> + struct mutexbuf_lock;
> >> + struct mutexlock; /* protect sensor state */
> >> + u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> >> + u8 rx[ADE7753_MAX_RX];
> >> };
> >>
> >> static int ade7753_spi_write_reg_8(struct device *dev,
> >> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> if (!val)
> >> return -EINVAL;
> >>
> >> - mutex_lock(&indio_dev->mlock);
> >> + mutex_lock(&st->lock);
> >>
> >> t = 27900 / val;
> >> if (t > 0)
> >> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
> >>
> >> out:
> >> - mutex_unlock(&indio_dev->mlock);
> >> + mutex_unlock(&st->lock);
> >>
> >> return ret ? ret : len;
> >> }
> >> --
> >> 2.7.4
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >> email to outreachy-kernel+unsubscr...@googlegroups.com.
> >> To post to this group, send email to outreachy-ker...@googlegroups.com.
> >> To view this discussion on the web visit
> >> https://groups.google.com/d/msgid/outreachy-kernel/20170312133250.GA7772%40singhal-Inspiron-5558.
> >> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v3] staging: adis16060_core: Use private driver lock instead of mlock
On Mon, Mar 13, 2017 at 11:41:30PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Signed-off-by: simran singhal
> ---
>
> v3:
>-Removed new lock to reuse the existing lock
Hi Simran,
Check Lars review comments about refactoring into a
write_then_read function protected by same buf_lock.
As it is now, it'll deadlock on buf_lock. It locks in
_read_raw and then calls _spi_write where an attempt
is made to grab the same lock.
alisons
>
> drivers/staging/iio/gyro/adis16060_core.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..602ec53 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -83,11 +83,12 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> {
> u16 tval = 0;
> int ret;
> + struct adis16060_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->buf_lock);
> ret = adis16060_spi_write(indio_dev, chan->address);
> if (ret < 0)
> goto out_unlock;
> @@ -96,7 +97,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> if (ret < 0)
> goto out_unlock;
>
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +113,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
> return ret;
> }
>
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170313181130.GA27111%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v2] staging: iio: ade7753: replace mlock with driver private lock
On Mon, Mar 13, 2017 at 10:01:07PM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
>
> Fix some coding style issues related to white space also.
>
> Signed-off-by: simran singhal
> ---
>
> v2:
>-Removed new lock to reuse the existing lock
Simran,
The good news is that you have 2 patches that have similar
challenges! I'll suggest picking one, drive it to completion,
then do the other.
This has the nested locking issue that Lars warned about.
Need to refactor to avoid. Check back on his review comments.
I suggest dropping those whitespace changes - they appear
out of place in this patch since you are no longer actually
touching those lines of code.
alisons
>
> drivers/staging/iio/meter/ade7753.c | 12 ++--
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..d88eaa3 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -83,10 +83,10 @@
> * @buf_lock: mutex to protect tx and rx
> **/
> struct ade7753_state {
> - struct spi_device *us;
> - struct mutexbuf_lock;
> - u8 tx[ADE7753_MAX_TX]
> cacheline_aligned;
> - u8 rx[ADE7753_MAX_RX];
> + struct spi_device *us;
> + struct mutexbuf_lock;
> + u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> + u8 rx[ADE7753_MAX_RX];
> };
>
> static int ade7753_spi_write_reg_8(struct device *dev,
> @@ -484,7 +484,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> if (!val)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->buf_lock);
>
> t = 27900 / val;
> if (t > 0)
> @@ -505,7 +505,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->buf_lock);
>
> return ret ? ret : len;
> }
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170313163107.GA31496%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH 0/2] IIO coding tasks
On Tue, Mar 21, 2017 at 07:00:17PM +0530, Arushi Singhal wrote: > Patchseries of IIO coding tasks This wouldn't be a patchset. Although they touch the same driver, the changes are unrelated. See more below... > > Arushi Singhal (2): > staging: ad7759: Replace mlock with driver private lock This one is already been submitted. If you have a v2 for it, then v2 the original patch. > staging: iio: ade7759: Move header content to implementation file This patch is done and applied already. See the Coding Task #1 notes in the IIO Tasks page. alisons > > drivers/staging/iio/meter/ade7759.c | 56 > +-- > drivers/staging/iio/meter/ade7759.h | 53 - > 2 files changed, 54 insertions(+), 57 deletions(-) > > -- > 2.11.0 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20170321133021.6737-1-arushisinghal19971997%40gmail.com. > For more options, visit https://groups.google.com/d/optout. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v6] staging: Use buf_lock instead of mlock and Refactor code
On Mon, Mar 20, 2017 at 01:36:21AM +0530, simran singhal wrote:
Hi Simran,
I going to ask for a v7 without looking at the code ;)
Subject line needs subsystem and driver.
Subject and log message can be improved.
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with buf_lock in the devices global data.
^^^ this was not done
>
> As buf_lock protects both the adis16060_spi_write() and
> adis16060_spi_read() functions and both are always called in
> pair. First write, then read. Thus, refactor the code to have
> one single function adis16060_spi_write_than_read() which is
> protected by the existing buf_lock.
This was done. So, you were able to obsolete the need for mlock
by creating the paired function.
>
> Removed nested locks as the function adis16060_read_raw call
> a lock on &st->buf_lock and then calls the function
> adis16060_spi_write which again tries to get hold
> of the same lock.
this was not done. Yes, you avoided nested locks through
proper coding, but we don't want to give the impression in the
log message that there was a pre-existing nested lock issue.
I did checkpatch & compile it...but looked no further yet.
alisons
>
> Signed-off-by: simran singhal
> ---
>
> v6:
>-Change commit message
>-Remove nested lock
>
> drivers/staging/iio/gyro/adis16060_core.c | 40
> ++-
> 1 file changed, 13 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..1c6de46 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -40,25 +40,17 @@ struct adis16060_state {
>
> static struct iio_dev *adis16060_iio_dev;
>
> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> + u8 conf, u16 *val)
> {
> int ret;
> struct adis16060_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->buf_lock);
> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
> ret = spi_write(st->us_w, st->buf, 3);
> - mutex_unlock(&st->buf_lock);
> -
> - return ret;
> -}
> -
> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> -{
> - int ret;
> - struct adis16060_state *st = iio_priv(indio_dev);
>
> - mutex_lock(&st->buf_lock);
> + if (ret < 0)
> + return ret;
>
> ret = spi_read(st->us_r, st->buf, 3);
>
> @@ -69,8 +61,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev,
> u16 *val)
>*/
> if (!ret)
> *val = ((st->buf[0] & 0x3) << 12) |
> - (st->buf[1] << 4) |
> - ((st->buf[2] >> 4) & 0xF);
> + (st->buf[1] << 4) |
> + ((st->buf[2] >> 4) & 0xF);
> mutex_unlock(&st->buf_lock);
>
> return ret;
> @@ -83,20 +75,18 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> {
> u16 tval = 0;
> int ret;
> + struct adis16060_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> /* Take the iio_dev status lock */
> - mutex_lock(&indio_dev->mlock);
> - ret = adis16060_spi_write(indio_dev, chan->address);
> + mutex_lock(&st->buf_lock);
> + ret = adis16060_spi_write_than_read(indio_dev,
> + chan->address, &tval);
> + mutex_unlock(&st->buf_lock);
> if (ret < 0)
> - goto out_unlock;
> + return ret;
>
> - ret = adis16060_spi_read(indio_dev, &tval);
> - if (ret < 0)
> - goto out_unlock;
> -
> - mutex_unlock(&indio_dev->mlock);
> *val = tval;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_OFFSET:
> @@ -110,10 +100,6 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
> }
>
> return -EINVAL;
> -
> -out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> - return ret;
> }
>
> static const struct iio_info adis16060_info = {
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgi
Re: [Outreachy kernel] [PATCH 0/2] IIO coding tasks
On Tue, Mar 21, 2017 at 05:39:38PM +0100, Julia Lawall wrote: > > > On Tue, 21 Mar 2017, Arushi Singhal wrote: > > > On Tue, Mar 21, 2017 at 9:33 PM, Alison Schofield > > wrote: > > On Tue, Mar 21, 2017 at 07:00:17PM +0530, Arushi Singhal wrote: > > > Patchseries of IIO coding tasks > > > > This wouldn't be a patchset. Although they touch the same > > driver, the changes are unrelated. See more below... > > > > This I have send as a Patchset because as you have mentioned below that the > > [PATCH 2/2] was already done before by someone but I think so it is not yet > > applied in the Greg's tree yet.So I have done both the changes and as they > > should be applied one after other so that's why I have send them as > > Patchset. > > For the IIO patches, it is better to work on the IIO tree, not Greg's. > Greg manages staging, not IIO. The IIO patches should appear in Greg's > tree eventually. > > julia We didn't direct applicants to create an iio tree. At this point, it seems more than is necessary. They can follow the directions in the task descriptions and avoid the collisions. Of course, they are welcome to create a tree to iio/testing. (IMHO it's more overhead/busy work and maybe not the best use of time in the home stretch of the application period.) alisons > > > > > > > Arushi Singhal (2): > > >  staging: ad7759: Replace mlock with driver private lock > > > > This one is already been submitted. If you have a v2 for it, > > then v2 > > the original patch. > > > > Is it submitted by me only before? And this is not the v2. > > I have just resed it. > > >  staging: iio: ade7759: Move header content to implementation > > file > > > > This patch is done and applied already. See the Coding Task #1 > > notes > > in the IIO Tasks page. > > > > Not at applied I think so. > > Thanks > > Arushi > > > > alisons > > > > > > > > drivers/staging/iio/meter/ade7759.c  | 56 > > +-- > > > drivers/staging/iio/meter/ade7759.h  | 53 > > - > > > 2 files changed, 54 insertions(+), 57 deletions(-) > > > > > > -- > > > 2.11.0 > > > > > > -- > > > You received this message because you are subscribed to the Google > > Groups "outreachy-kernel" group. > > > To unsubscribe from this group and stop receiving emails from it, > > send an email to outreachy-kernel+unsubscr...@googlegroups.com. > > > To post to this group, send email to > > outreachy-ker...@googlegroups.com. > > > To view this discussion on the web > > > visithttps://groups.google.com/d/msgid/outreachy-kernel/20170321133021.6737-1-ar > > ushisinghal19971997%40gmail.com. > > > For more options, visit https://groups.google.com/d/optout. > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "outreachy-kernel" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to outreachy-kernel+unsubscr...@googlegroups.com. > > To post to this group, send email to outreachy-ker...@googlegroups.com. > > To view this discussion on the web > > visithttps://groups.google.com/d/msgid/outreachy-kernel/CA%2BXqjF9dVy33Dsv0H2z8x > > taNeMOW7SQgr4qa4wLwz6xFNVTsUA%40mail.gmail.com. > > For more options, visit https://groups.google.com/d/optout. > > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH 0/2] IIO coding tasks
On Tue, Mar 21, 2017 at 10:52:46PM +0530, Arushi Singhal wrote: > On Tue, Mar 21, 2017 at 10:32 PM, Alison Schofield > wrote: > > > On Tue, Mar 21, 2017 at 05:39:38PM +0100, Julia Lawall wrote: > > > > > > > > > On Tue, 21 Mar 2017, Arushi Singhal wrote: > > > > > > > On Tue, Mar 21, 2017 at 9:33 PM, Alison Schofield < > > amsfiel...@gmail.com> > > > > wrote: > > > > On Tue, Mar 21, 2017 at 07:00:17PM +0530, Arushi Singhal wrote: > > > > > Patchseries of IIO coding tasks > > > > > > > > This wouldn't be a patchset. Although they touch the same > > > > driver, the changes are unrelated. See more below... > > > > > > > > This I have send as a Patchset because as you have mentioned below > > that the > > > > [PATCH 2/2] was already done before by someone but I think so it is > > not yet > > > > applied in the Greg's tree yet.So I have done both the changes and as > > they > > > > should be applied one after other so that's why I have send them as > > > > Patchset. > > > > > > For the IIO patches, it is better to work on the IIO tree, not Greg's. > > > Greg manages staging, not IIO. The IIO patches should appear in Greg's > > > tree eventually. > > > > > > julia > > > > We didn't direct applicants to create an iio tree. At this point, > > it seems more than is necessary. They can follow the directions in the > > task descriptions and avoid the collisions. > > > > Of course, they are welcome to create a tree to iio/testing. > > > > (IMHO it's more overhead/busy work and maybe not the best use > > of time in the home stretch of the application period.) > > > > alisons > > > > Hi Alison > As you have mentioned that my [PATCH 2/2] is already being done someone. So > how can I make the changes of [PATCH 1/2] on top of it as [PATCH 2/2] is > not yet applied on the staging tree. > Please suggest me. > Thanks > Arushi Arushi - I don't see anything needing to be done by you! Your ad7759 mlock patch is awaiting review. It's not your issue to keep up with all changes going into the file. When it gets applied will most likely merge with no merge issue at all. alisons > > > > > > > > > > > > > > Arushi Singhal (2): > > > > > staging: ad7759: Replace mlock with driver private lock > > > > > > > > This one is already been submitted. If you have a v2 for it, > > > > then v2 > > > > the original patch. > > > > > > > > Is it submitted by me only before? And this is not the v2. > > > > I have just resed it. > > > > > staging: iio: ade7759: Move header content to implementation > > > > file > > > > > > > > This patch is done and applied already. See the Coding Task #1 > > > > notes > > > > in the IIO Tasks page. > > > > > > > > Not at applied I think so. > > > > Thanks > > > > Arushi > > > > > > > > alisons > > > > > > > > > > > > > > drivers/staging/iio/meter/ade7759.c | 56 > > > > +-- > > > > > drivers/staging/iio/meter/ade7759.h | 53 > > > > - > > > > > 2 files changed, 54 insertions(+), 57 deletions(-) > > > > > > > > > > -- > > > > > 2.11.0 > > > > > > > > > > -- > > > > > You received this message because you are subscribed to the Google > > > > Groups "outreachy-kernel" group. > > > > > To unsubscribe from this group and stop receiving emails from it, > > > > send an email to outreachy-kernel+unsubscr...@googlegroups.com. > > > > > To post to this group, send email to > > > > outreachy-ker...@googlegroups.com. > > > > > To view this discussion on the web visithttps://groups.google. > > com/d/msgid/outreachy-kernel/20170321133021.6737-1-ar > > > > ushisinghal19971997%40gmail.com. > > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > > > > > -- > > > > You received this message because you are subscribed to the Google > > Groups > > > > "outreachy-kernel" group. > > > > To unsubscribe from this group and stop receiving emails from it, send > > an > > > > email to outreachy-kernel+unsubscr...@googlegroups.com. > > > > To post to this group, send email to outreachy-ker...@googlegroups.com > > . > > > > To view this discussion on the web visithttps://groups.google. > > com/d/msgid/outreachy-kernel/CA%2BXqjF9dVy33Dsv0H2z8x > > > > taNeMOW7SQgr4qa4wLwz6xFNVTsUA%40mail.gmail.com. > > > > For more options, visit https://groups.google.com/d/optout. > > > > > > > > > > > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v6] staging: Use buf_lock instead of mlock and Refactor code
On Tue, Mar 21, 2017 at 10:34:01PM +0530, SIMRAN SINGHAL wrote:
> On Tue, Mar 21, 2017 at 10:18 PM, Alison Schofield
> wrote:
> > On Mon, Mar 20, 2017 at 01:36:21AM +0530, simran singhal wrote:
> >
> > Hi Simran,
> >
> > I going to ask for a v7 without looking at the code ;)
> > Subject line needs subsystem and driver.
> > Subject and log message can be improved.
>
> Hi Alison,
> I have already sent v7 with changed subject.
Simran,
I see v7. Needs subsystem (iio) and to nitpick, driver name
is "adis16060" ;) Other comments still apply.
Please append all version histories below the --- for review.
v7:
v6:
.
.
v2:
thanks,
alisons
>
> >
> >> The IIO subsystem is redefining iio_dev->mlock to be used by
> >> the IIO core only for protecting device operating mode changes.
> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> >>
> >> In this driver, mlock was being used to protect hardware state
> >> changes. Replace it with buf_lock in the devices global data.
> >^^^ this was not done
> >>
> >> As buf_lock protects both the adis16060_spi_write() and
> >> adis16060_spi_read() functions and both are always called in
> >> pair. First write, then read. Thus, refactor the code to have
> >> one single function adis16060_spi_write_than_read() which is
> >> protected by the existing buf_lock.
> > This was done. So, you were able to obsolete the need for mlock
> > by creating the paired function.
> >
> >>
> >> Removed nested locks as the function adis16060_read_raw call
> >> a lock on &st->buf_lock and then calls the function
> >> adis16060_spi_write which again tries to get hold
> >> of the same lock.
> > this was not done. Yes, you avoided nested locks through
> > proper coding, but we don't want to give the impression in the
> > log message that there was a pre-existing nested lock issue.
> >
> > I did checkpatch & compile it...but looked no further yet.
> >
> > alisons
> >>
> >> Signed-off-by: simran singhal
> >> ---
> >>
> >> v6:
> >>-Change commit message
> >>-Remove nested lock
> >>
> >> drivers/staging/iio/gyro/adis16060_core.c | 40
> >> ++-
> >> 1 file changed, 13 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/gyro/adis16060_core.c
> >> b/drivers/staging/iio/gyro/adis16060_core.c
> >> index c9d46e7..1c6de46 100644
> >> --- a/drivers/staging/iio/gyro/adis16060_core.c
> >> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> >> @@ -40,25 +40,17 @@ struct adis16060_state {
> >>
> >> static struct iio_dev *adis16060_iio_dev;
> >>
> >> -static int adis16060_spi_write(struct iio_dev *indio_dev, u8 val)
> >> +static int adis16060_spi_write_than_read(struct iio_dev *indio_dev,
> >> + u8 conf, u16 *val)
> >> {
> >> int ret;
> >> struct adis16060_state *st = iio_priv(indio_dev);
> >>
> >> - mutex_lock(&st->buf_lock);
> >> - st->buf[2] = val; /* The last 8 bits clocked in are latched */
> >> + st->buf[2] = conf; /* The last 8 bits clocked in are latched */
> >> ret = spi_write(st->us_w, st->buf, 3);
> >> - mutex_unlock(&st->buf_lock);
> >> -
> >> - return ret;
> >> -}
> >> -
> >> -static int adis16060_spi_read(struct iio_dev *indio_dev, u16 *val)
> >> -{
> >> - int ret;
> >> - struct adis16060_state *st = iio_priv(indio_dev);
> >>
> >> - mutex_lock(&st->buf_lock);
> >> + if (ret < 0)
> >> + return ret;
> >>
> >> ret = spi_read(st->us_r, st->buf, 3);
> >>
> >> @@ -69,8 +61,8 @@ static int adis16060_spi_read(struct iio_dev *indio_dev,
> >> u16 *val)
> >>*/
> >> if (!ret)
> >> *val = ((st->buf[0] & 0x3) << 12) |
> >> - (st->buf[1] << 4) |
> >> - ((st->buf[2] >> 4) & 0xF);
> >> + (st->buf[1] << 4) |
> >> + ((st->buf[2] >> 4) & 0xF);
> >> mutex_unlock(&st->buf_lock);
> >>
> >> return ret;
> >> @@ -83,20 +75,18 @@ stat
Re: [Outreachy kernel] [PATCH v3 0/2] Replace mlock with private lock and delete whitespaces
On Tue, Mar 21, 2017 at 11:33:54PM +0530, simran singhal wrote: > The patch series replaces mlock with a private lock for driver ad9834 and > Fix coding style issues related to white spaces. Hi Simran, I'm getting lost. Patchset Subject Line needs subsystem and driver. The comment above says ad9834 but the patches below say ade7753. Can we drive adis16060 through ACK and then come back to this one? (ie. applyling lessons learned) thanks, alisons > > v3: > -Using new private "lock" instead of using "buf_lock" >as it can cause deadlock. > -Sending it as a series of two patches. > > v2: > -Using the existing buf_lock instead of lock. > > > simran singhal (2): > staging: iio: ade7753: Remove trailing whitespaces > staging: iio: ade7753: Replace mlock with driver private lock > > drivers/staging/iio/meter/ade7753.c | 14 -- > 1 file changed, 8 insertions(+), 6 deletions(-) > > -- > 2.7.4 > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To post to this group, send email to outreachy-ker...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/1490119436-20042-1-git-send-email-singhalsimran0%40gmail.com. > For more options, visit https://groups.google.com/d/optout. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock
On Fri, Mar 24, 2017 at 12:05:20AM +0530, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>
> In this driver, mlock was being used to protect hardware state
> changes. Replace it with a lock in the devices global data.
Hi Simran,
Please post all revision histories below the --- not just the most
recent.
Does this lock enforce the needed "atomicity" in the write_frequency
function? I read Jonathans comment on a previous revision about
"ensuring the spi bus frequency and sampling frequency of the device
are changed in an atomic fashion"
Is it possible for another spi bus transaction (read or write) to
occur between the read and write in write_frequency?
alisons
>
> Signed-off-by: simran singhal
> ---
>
> v4:
>-Add mutex_init
>
> drivers/staging/iio/meter/ade7753.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/meter/ade7753.c
> b/drivers/staging/iio/meter/ade7753.c
> index b71fbd3..30aebaf 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -80,11 +80,13 @@
> * @us: actual spi_device
> * @tx: transmit buffer
> * @rx: receive buffer
> + * @lock: protect sensor state
> * @buf_lock: mutex to protect tx and rx
> **/
> struct ade7753_state {
> struct spi_device *us;
> struct mutexbuf_lock;
> + struct mutexlock; /* protect sensor state */
> u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> u8 rx[ADE7753_MAX_RX];
> };
> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> if (!val)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
>
> t = 27900 / val;
> if (t > 0)
> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret ? ret : len;
> }
> @@ -581,6 +583,7 @@ static int ade7753_probe(struct spi_device *spi)
> st = iio_priv(indio_dev);
> st->us = spi;
> mutex_init(&st->buf_lock);
> + mutex_init(&st->lock);
>
> indio_dev->name = spi->dev.driver->name;
> indio_dev->dev.parent = &spi->dev;
> --
> 2.7.4
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscr...@googlegroups.com.
> To post to this group, send email to outreachy-ker...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Outreachy kernel] [PATCH v4] staging: iio: ade7753: Replace mlock with driver private lock
On Tue, Mar 28, 2017 at 10:55:17PM +0530, SIMRAN SINGHAL wrote:
> On Fri, Mar 24, 2017 at 12:51 AM, Alison Schofield
> wrote:
> > On Fri, Mar 24, 2017 at 12:05:20AM +0530, simran singhal wrote:
> >> The IIO subsystem is redefining iio_dev->mlock to be used by
> >> the IIO core only for protecting device operating mode changes.
> >> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> >>
> >> In this driver, mlock was being used to protect hardware state
> >> changes. Replace it with a lock in the devices global data.
> >
> > Hi Simran,
> >
> > Please post all revision histories below the --- not just the most
> > recent.
> >
> Sorry, will not repeat this.
>
> > Does this lock enforce the needed "atomicity" in the write_frequency
> > function? I read Jonathans comment on a previous revision about
> > "ensuring the spi bus frequency and sampling frequency of the device
> > are changed in an atomic fashion"
> >
>
> By introducing another lock I am protecting read_modify_write and
> in this way also protecting the designated register that we are about
> to write.
I see it protecting this path from being re-entered. My uncertainty
is about other paths to read/write.
>
> > Is it possible for another spi bus transaction (read or write) to
> > occur between the read and write in write_frequency?
> >
>
> Gargi has also come up with a solution.
> https://groups.google.com/forum/#!topic/outreachy-kernel/kzE9CrI5Bd8
>
> Should I do like her as her's also seem correct or go ahead with this.
My suggestion would be to wait for feedback on Gargi's patch.
(See the Outreachy log about creating similar solutions.)
We will not be able to close on this set of patches during the
Outreachy application window. You can continue to push for closure
beyond the March 30th date as your time allows :)
Thanks,
alisons
>
> > alisons
> >>
> >> Signed-off-by: simran singhal
> >> ---
> >>
> >> v4:
> >>-Add mutex_init
> >>
> >> drivers/staging/iio/meter/ade7753.c | 7 +--
> >> 1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/iio/meter/ade7753.c
> >> b/drivers/staging/iio/meter/ade7753.c
> >> index b71fbd3..30aebaf 100644
> >> --- a/drivers/staging/iio/meter/ade7753.c
> >> +++ b/drivers/staging/iio/meter/ade7753.c
> >> @@ -80,11 +80,13 @@
> >> * @us: actual spi_device
> >> * @tx: transmit buffer
> >> * @rx: receive buffer
> >> + * @lock: protect sensor state
> >> * @buf_lock: mutex to protect tx and rx
> >> **/
> >> struct ade7753_state {
> >> struct spi_device *us;
> >> struct mutexbuf_lock;
> >> + struct mutexlock; /* protect sensor state */
> >> u8 tx[ADE7753_MAX_TX] cacheline_aligned;
> >> u8 rx[ADE7753_MAX_RX];
> >> };
> >> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> if (!val)
> >> return -EINVAL;
> >>
> >> - mutex_lock(&indio_dev->mlock);
> >> + mutex_lock(&st->lock);
> >>
> >> t = 27900 / val;
> >> if (t > 0)
> >> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device
> >> *dev,
> >> ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
> >>
> >> out:
> >> - mutex_unlock(&indio_dev->mlock);
> >> + mutex_unlock(&st->lock);
> >>
> >> return ret ? ret : len;
> >> }
> >> @@ -581,6 +583,7 @@ static int ade7753_probe(struct spi_device *spi)
> >> st = iio_priv(indio_dev);
> >> st->us = spi;
> >> mutex_init(&st->buf_lock);
> >> + mutex_init(&st->lock);
> >>
> >> indio_dev->name = spi->dev.driver->name;
> >> indio_dev->dev.parent = &spi->dev;
> >> --
> >> 2.7.4
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups
> >> "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an
> >> email to outreachy-kernel+unsubscr...@googlegroups.com.
> >> To post to this group, send email to outreachy-ker...@googlegroups.com.
> >> To view this discussion on the web visit
> >> https://groups.google.com/d/msgid/outreachy-kernel/20170323183520.GA9871%40singhal-Inspiron-5558.
> >> For more options, visit https://groups.google.com/d/optout.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: iio: convert bare unsigned usage to unsigned int
Use kernel preferred unsigned int declaration style.
Patch created using:
git ls-files drivers/staging/iio | \
xargs ./scripts/checkpatch.pl -f --fix-inplace --types=unspecified_int
Hand edits restored columns in structure definitions.
Signed-off-by: Alison Schofield
---
drivers/staging/iio/adc/ad7280a.c | 40 +++
drivers/staging/iio/adc/ad7280a.h | 8 ++---
drivers/staging/iio/adc/ad7606.h | 28
drivers/staging/iio/adc/ad7606_core.c | 6 ++--
drivers/staging/iio/adc/ad7780.c | 2 +-
drivers/staging/iio/impedance-analyzer/ad5933.c | 14
drivers/staging/iio/meter/ade7758_ring.c | 4 +--
drivers/staging/iio/resolver/ad2s1210.h | 8 ++---
drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 12 +++
9 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7280a.c
b/drivers/staging/iio/adc/ad7280a.c
index 62e5eca..a06b46c 100644
--- a/drivers/staging/iio/adc/ad7280a.c
+++ b/drivers/staging/iio/adc/ad7280a.c
@@ -155,7 +155,7 @@ static void ad7280_crc8_build_table(unsigned char *crc_tab)
}
}
-static unsigned char ad7280_calc_crc8(unsigned char *crc_tab, unsigned val)
+static unsigned char ad7280_calc_crc8(unsigned char *crc_tab, unsigned int val)
{
unsigned char crc;
@@ -165,7 +165,7 @@ static unsigned char ad7280_calc_crc8(unsigned char
*crc_tab, unsigned val)
return crc ^ (val & 0xFF);
}
-static int ad7280_check_crc(struct ad7280_state *st, unsigned val)
+static int ad7280_check_crc(struct ad7280_state *st, unsigned int val)
{
unsigned char crc = ad7280_calc_crc8(st->crc_tab, val >> 10);
@@ -191,7 +191,7 @@ static void ad7280_delay(struct ad7280_state *st)
usleep_range(250, 500);
}
-static int __ad7280_read32(struct ad7280_state *st, unsigned *val)
+static int __ad7280_read32(struct ad7280_state *st, unsigned int *val)
{
int ret;
struct spi_transfer t = {
@@ -211,10 +211,10 @@ static int __ad7280_read32(struct ad7280_state *st,
unsigned *val)
return 0;
}
-static int ad7280_write(struct ad7280_state *st, unsigned devaddr,
- unsigned addr, bool all, unsigned val)
+static int ad7280_write(struct ad7280_state *st, unsigned int devaddr,
+ unsigned int addr, bool all, unsigned int val)
{
- unsigned reg = devaddr << 27 | addr << 21 |
+ unsigned int reg = devaddr << 27 | addr << 21 |
(val & 0xFF) << 13 | all << 12;
reg |= ad7280_calc_crc8(st->crc_tab, reg >> 11) << 3 | 0x2;
@@ -223,11 +223,11 @@ static int ad7280_write(struct ad7280_state *st, unsigned
devaddr,
return spi_write(st->spi, &st->buf[0], 4);
}
-static int ad7280_read(struct ad7280_state *st, unsigned devaddr,
- unsigned addr)
+static int ad7280_read(struct ad7280_state *st, unsigned int devaddr,
+ unsigned int addr)
{
int ret;
- unsigned tmp;
+ unsigned int tmp;
/* turns off the read operation on all parts */
ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_HB, 1,
@@ -261,11 +261,11 @@ static int ad7280_read(struct ad7280_state *st, unsigned
devaddr,
return (tmp >> 13) & 0xFF;
}
-static int ad7280_read_channel(struct ad7280_state *st, unsigned devaddr,
- unsigned addr)
+static int ad7280_read_channel(struct ad7280_state *st, unsigned int devaddr,
+ unsigned int addr)
{
int ret;
- unsigned tmp;
+ unsigned int tmp;
ret = ad7280_write(st, devaddr, AD7280A_READ, 0, addr << 2);
if (ret)
@@ -299,11 +299,11 @@ static int ad7280_read_channel(struct ad7280_state *st,
unsigned devaddr,
return (tmp >> 11) & 0xFFF;
}
-static int ad7280_read_all_channels(struct ad7280_state *st, unsigned cnt,
- unsigned *array)
+static int ad7280_read_all_channels(struct ad7280_state *st, unsigned int cnt,
+ unsigned int *array)
{
int i, ret;
- unsigned tmp, sum = 0;
+ unsigned int tmp, sum = 0;
ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_READ, 1,
AD7280A_CELL_VOLTAGE_1 << 2);
@@ -338,7 +338,7 @@ static int ad7280_read_all_channels(struct ad7280_state
*st, unsigned cnt,
static int ad7280_chain_setup(struct ad7280_state *st)
{
- unsigned val, n;
+ unsigned int val, n;
int ret;
ret = ad7280_write(st, AD7280A_DEVADDR_MASTER, AD7280A_CONTROL_LB, 1,
@@ -401,7 +401,7 @@ static ssize_t ad7280_store_balance_sw(struct device *dev,
struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
bool readin;
i
[PATCH] staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()
Two instances are moved to the new claim/release API:
In the first instance, the driver was using mlock followed by
iio_buffer_enabled(). Replace that code with the new API to guarantee
the device stays in direct mode. There is no change in driver behavior.
In the second instance, the driver was not using mlock to hold the
device in direct mode, but should have been. Here we introduce the
new API to guarantee direct mode. This is a change in driver behavior.
Signed-off-by: Alison Schofield
---
drivers/staging/iio/adc/ad7606_core.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7606_core.c
b/drivers/staging/iio/adc/ad7606_core.c
index 6dbc811..f914b8d 100644
--- a/drivers/staging/iio/adc/ad7606_core.c
+++ b/drivers/staging/iio/adc/ad7606_core.c
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_RAW:
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev))
- ret = -EBUSY;
- else
- ret = ad7606_scan_direct(indio_dev, chan->address);
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = ad7606_scan_direct(indio_dev, chan->address);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
@@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void *dev_id)
struct iio_dev *indio_dev = dev_id;
struct ad7606_state *st = iio_priv(indio_dev);
- if (iio_buffer_enabled(indio_dev)) {
+ if (!iio_device_claim_direct_mode(indio_dev)) {
schedule_work(&st->poll_work);
+ iio_device_release_direct_mode(indio_dev);
} else {
st->done = true;
wake_up_interruptible(&st->wq_data_avail);
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()
On Sun, Apr 03, 2016 at 10:09:13AM +0100, Jonathan Cameron wrote:
> On 01/04/16 17:53, Alison Schofield wrote:
> > Two instances are moved to the new claim/release API:
> >
> > In the first instance, the driver was using mlock followed by
> > iio_buffer_enabled(). Replace that code with the new API to guarantee
> > the device stays in direct mode. There is no change in driver behavior.
> >
> > In the second instance, the driver was not using mlock to hold the
> > device in direct mode, but should have been. Here we introduce the
> > new API to guarantee direct mode. This is a change in driver behavior.
> >
> > Signed-off-by: Alison Schofield
> > ---
> > drivers/staging/iio/adc/ad7606_core.c | 15 ---
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7606_core.c
> > b/drivers/staging/iio/adc/ad7606_core.c
> > index 6dbc811..f914b8d 100644
> > --- a/drivers/staging/iio/adc/ad7606_core.c
> > +++ b/drivers/staging/iio/adc/ad7606_core.c
> > @@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
> >
> > switch (m) {
> > case IIO_CHAN_INFO_RAW:
> > - mutex_lock(&indio_dev->mlock);
> > - if (iio_buffer_enabled(indio_dev))
> > - ret = -EBUSY;
> > - else
> > - ret = ad7606_scan_direct(indio_dev, chan->address);
> > - mutex_unlock(&indio_dev->mlock);
> > + ret = iio_device_claim_direct_mode(indio_dev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ad7606_scan_direct(indio_dev, chan->address);
> > + iio_device_release_direct_mode(indio_dev);
> >
> > if (ret < 0)
> > return ret;
> > @@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void
> > *dev_id)
> > struct iio_dev *indio_dev = dev_id;
> > struct ad7606_state *st = iio_priv(indio_dev);
> >
> > - if (iio_buffer_enabled(indio_dev)) {
> > + if (!iio_device_claim_direct_mode(indio_dev)) {
> > schedule_work(&st->poll_work);
> > + iio_device_release_direct_mode(indio_dev);
> Unfortunately this won't work. That interrupt is still in traditional non
> threaded form. This will take a mutex in a top half interrupt handler
> where a sleep cannot occur.
>
> I'm just wondering how expensive it would be to fix this by moving that over
> to a threaded handler. In the poll_work case (buffer) it would be cleaner to
> do
> so. I'm really confused what the intended interrupt handler
> is in here. I 'think' the sequence is:
>
> Trigger fires the convst pin whether in top half or the bottom half of
> a threaded interrupt, but not both - I guess this works, if it is rather
> 'unusual'.
>
> We then get a interrupt to indicate that it has finished conversion and that
> filters through to actually fill the buffer via a traditional top half /
> bottom half interrupt handler.
>
> So if we were to convert that to a threaded interrupt (with no top half / non
> threaded part), we could drop the schedule_work and just call
> ad7606_poll_bh_to_ring from the thread handler.
>
> In the direct read case I doubt we care about the delay in dropping to a
> thread prior to signalling the data is ready.
>
> Can't think why this driver is still in staging :)
>
> Lars, any interest from Analog in getting this one cleaned up? Also
> do you have any test hardware, if we mess around with this interrupt handling?
>
> Jonathan
I see the problem. Thanks for the review and the details for the
redesign. Plan to v2 this patch leaving out this piece. I'll keep
the interrupt handler rework on my radar pending Lars's comments.
alisons
>
>
> > } else {
> > st->done = true;
> > wake_up_interruptible(&st->wq_data_avail);
> >
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()
Replace the code that guarantees the device stays in direct mode with
iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by: Alison Schofield
---
Changed in v2:
- removed improper application of claim/release from intr handler
- updated changelog
drivers/staging/iio/adc/ad7606_core.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7606_core.c
b/drivers/staging/iio/adc/ad7606_core.c
index 6dbc811..f79ee61 100644
--- a/drivers/staging/iio/adc/ad7606_core.c
+++ b/drivers/staging/iio/adc/ad7606_core.c
@@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_RAW:
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev))
- ret = -EBUSY;
- else
- ret = ad7606_scan_direct(indio_dev, chan->address);
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = ad7606_scan_direct(indio_dev, chan->address);
+ iio_device_release_direct_mode(indio_dev);
if (ret < 0)
return ret;
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] staging: wilc1000: move initialization of the config values
Move the initialization of the config values so that an uninit'd
mutex is not exposed and to simplify the initialization process.
The code was allocating a structure with a lock, initializing and
taking the lock, setting some values, and then releasing the
lock. If no other thread can get it, then the lock isn't needed.
If another thread can get it, it could find it before the mutex
is initialized.
Make it safe and simple by setting the config values and initializing
their mutex before the kthread is started. No lock/unlock needed.
Signed-off-by: Alison Schofield
---
Note- not verified with hardware.
drivers/staging/wilc1000/host_interface.c | 18 --
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/wilc1000/host_interface.c
b/drivers/staging/wilc1000/host_interface.c
index 04cbff5..40f3d11 100644
--- a/drivers/staging/wilc1000/host_interface.c
+++ b/drivers/staging/wilc1000/host_interface.c
@@ -3410,6 +3410,14 @@ int wilc_init(struct net_device *dev, struct host_if_drv
**hif_drv_handler)
init_completion(&hif_drv->comp_get_rssi);
init_completion(&hif_drv->comp_inactive_time);
+ hif_drv->cfg_values.site_survey_enabled = SITE_SURVEY_OFF;
+ hif_drv->cfg_values.scan_source = DEFAULT_SCAN;
+ hif_drv->cfg_values.active_scan_time = ACTIVE_SCAN_TIME;
+ hif_drv->cfg_values.passive_scan_time = PASSIVE_SCAN_TIME;
+ hif_drv->cfg_values.curr_tx_rate = AUTORATE;
+
+ mutex_init(&hif_drv->cfg_values_lock);
+
if (clients_count == 0) {
result = wilc_mq_create(&hif_msg_q);
@@ -3435,20 +3443,10 @@ int wilc_init(struct net_device *dev, struct
host_if_drv **hif_drv_handler)
setup_timer(&hif_drv->connect_timer, TimerCB_Connect, 0);
setup_timer(&hif_drv->remain_on_ch_timer, ListenTimerCB, 0);
- mutex_init(&hif_drv->cfg_values_lock);
- mutex_lock(&hif_drv->cfg_values_lock);
-
hif_drv->hif_state = HOST_IF_IDLE;
- hif_drv->cfg_values.site_survey_enabled = SITE_SURVEY_OFF;
- hif_drv->cfg_values.scan_source = DEFAULT_SCAN;
- hif_drv->cfg_values.active_scan_time = ACTIVE_SCAN_TIME;
- hif_drv->cfg_values.passive_scan_time = PASSIVE_SCAN_TIME;
- hif_drv->cfg_values.curr_tx_rate = AUTORATE;
hif_drv->p2p_timeout = 0;
- mutex_unlock(&hif_drv->cfg_values_lock);
-
clients_count++;
return result;
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC] staging: iio: isl29018: use regmap to retrieve struct device
Remove struct device from drivers global data and use regmap
API to retrieve device info instead.
This replacement can be done for drivers that include regmap
in their global data.
Signed-off-by: Alison Schofield
---
drivers/staging/iio/light/isl29018.c | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/staging/iio/light/isl29018.c
b/drivers/staging/iio/light/isl29018.c
index 03dbfb6..76d9f74 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -100,7 +100,6 @@ static const struct isl29018_scale {
};
struct isl29018_chip {
- struct device *dev;
struct regmap *regmap;
struct mutexlock;
int type;
@@ -180,30 +179,31 @@ static int isl29018_read_sensor_input(struct
isl29018_chip *chip, int mode)
int status;
unsigned int lsb;
unsigned int msb;
+ struct device *dev = regmap_get_device(chip->regmap);
/* Set mode */
status = regmap_write(chip->regmap, ISL29018_REG_ADD_COMMAND1,
mode << COMMMAND1_OPMODE_SHIFT);
if (status) {
- dev_err(chip->dev,
+ dev_err(dev,
"Error in setting operating mode err %d\n", status);
return status;
}
msleep(CONVERSION_TIME_MS);
status = regmap_read(chip->regmap, ISL29018_REG_ADD_DATA_LSB, &lsb);
if (status < 0) {
- dev_err(chip->dev,
+ dev_err(dev,
"Error in reading LSB DATA with err %d\n", status);
return status;
}
status = regmap_read(chip->regmap, ISL29018_REG_ADD_DATA_MSB, &msb);
if (status < 0) {
- dev_err(chip->dev,
+ dev_err(dev,
"Error in reading MSB DATA with error %d\n", status);
return status;
}
- dev_vdbg(chip->dev, "MSB 0x%x and LSB 0x%x\n", msb, lsb);
+ dev_vdbg(dev, "MSB 0x%x and LSB 0x%x\n", msb, lsb);
return (msb << 8) | lsb;
}
@@ -246,13 +246,14 @@ static int isl29018_read_proximity_ir(struct
isl29018_chip *chip, int scheme,
int status;
int prox_data = -1;
int ir_data = -1;
+ struct device *dev = regmap_get_device(chip->regmap);
/* Do proximity sensing with required scheme */
status = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
COMMANDII_SCHEME_MASK,
scheme << COMMANDII_SCHEME_SHIFT);
if (status) {
- dev_err(chip->dev, "Error in setting operating mode\n");
+ dev_err(dev, "Error in setting operating mode\n");
return status;
}
@@ -525,10 +526,11 @@ static int isl29035_detect(struct isl29018_chip *chip)
{
int status;
unsigned int id;
+ struct device *dev = regmap_get_device(chip->regmap);
status = regmap_read(chip->regmap, ISL29035_REG_DEVICE_ID, &id);
if (status < 0) {
- dev_err(chip->dev,
+ dev_err(dev,
"Error reading ID register with error %d\n",
status);
return status;
@@ -553,6 +555,7 @@ enum {
static int isl29018_chip_init(struct isl29018_chip *chip)
{
int status;
+ struct device *dev = regmap_get_device(chip->regmap);
if (chip->type == isl29035) {
status = isl29035_detect(chip);
@@ -582,7 +585,7 @@ static int isl29018_chip_init(struct isl29018_chip *chip)
*/
status = regmap_write(chip->regmap, ISL29018_REG_TEST, 0x0);
if (status < 0) {
- dev_err(chip->dev, "Failed to clear isl29018 TEST reg.(%d)\n",
+ dev_err(dev, "Failed to clear isl29018 TEST reg.(%d)\n",
status);
return status;
}
@@ -593,7 +596,7 @@ static int isl29018_chip_init(struct isl29018_chip *chip)
*/
status = regmap_write(chip->regmap, ISL29018_REG_ADD_COMMAND1, 0);
if (status < 0) {
- dev_err(chip->dev, "Failed to clear isl29018 CMD1 reg.(%d)\n",
+ dev_err(dev, "Failed to clear isl29018 CMD1 reg.(%d)\n",
status);
return status;
}
@@ -604,14 +607,14 @@ static int isl29018_chip_init(struct isl29018_chip *chip)
status = isl29018_set_scale(chip, chip->scale.scale,
chip->scale.uscale);
if (status < 0) {
- dev_err(chip->dev, "Init of isl29018 fails\n");
+
[RFC PATCH 0/2] iio: introduce iio_{claim|release}_direct_mode()
This patchset introduces two helper functions to simplify driver code
requiring the device to be locked in direct mode during execution of a
code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver
is holding the lock in direct mode. Unknown cases might be those that
should, but don't, hold the lock.
Alternate implementation: Generalize to support a claim on any mode.
Do iio_claim_mode(device,mode) where if the device is in *mode*, it
is guaranteed to stay that way until release is called. I considered
and rejected this option because a) not sure other modes would ever
need to be locked, and b) the semantic improvement is less when it
is generalized.
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.html
Alison Schofield (2):
iio: core: implement iio_{claim|release}_direct_mode()
staging: iio: adc7192: use iio_{claim|release}_direct_mode()
drivers/iio/industrialio-core.c | 39 +++
drivers/staging/iio/adc/ad7192.c | 24 +---
include/linux/iio/iio.h | 2 ++
3 files changed, 50 insertions(+), 15 deletions(-)
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()
It is often the case that the driver wants to be sure a device stays
in direct mode while it is executing a task or series of tasks. To
accomplish this today, the driver performs this sequence: 1) take the
device state lock, 2)verify it is not in a buffered mode, 3) execute
some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these
steps and make it more semantically expressive.
iio_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed
to stay that way until iio_release_direct_mode() is called.
iio_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay
in direct mode.
Signed-off-by: Alison Schofield
---
drivers/iio/industrialio-core.c | 39 +++
include/linux/iio/iio.h | 2 ++
2 files changed, 41 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 70cb7eb..f6f0c89 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
#include "iio_core.h"
#include "iio_core_trigger.h"
@@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev,
struct iio_dev *indio_dev)
}
EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
+/**
+ * iio_claim_direct_mode - Keep device in direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * If the device is in direct mode it is guaranteed to
+ * stay that way until iio_release_direct_mode() is called.
+ *
+ * Use with iio_release_direct_mode()
+ *
+ * Returns: 0 on success, -EINVAL on failure
+ */
+int iio_claim_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_lock(&indio_dev->mlock);
+
+ if (iio_buffer_enabled(indio_dev)) {
+ mutex_unlock(&indio_dev->mlock);
+ return -EINVAL;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iio_claim_direct_mode);
+
+/**
+ * iio_release_direct_mode - releases claim on direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * Release the claim. Device is no longer guaranteed to stay
+ * in direct mode.
+ *
+ * Use with iio_claim_direct_mode()
+ */
+void iio_release_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_unlock(&indio_dev->mlock);
+}
+EXPORT_SYMBOL_GPL(iio_release_direct_mode);
+
subsys_initcall(iio_init);
module_exit(iio_exit);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index ce9e9c1..9efe2af 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);
int devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev);
void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev);
int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
+int iio_claim_direct_mode(struct iio_dev *indio_dev);
+void iio_release_direct_mode(struct iio_dev *indio_dev);
extern struct bus_type iio_bus_type;
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC PATCH 2/2] staging: iio: adc7192: use iio_{claim|release}_direct_mode()
Replace the code that guarantees the device stays in direct mode with
iio_{claim|release}_direct_mode() which does same.
Signed-off-by: Alison Schofield
---
drivers/staging/iio/adc/ad7192.c | 24 +---
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f843f19..401ec91 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,
if (lval == 0)
return -EINVAL;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
div = st->mclk / (lval * st->f_order * 1024);
if (div < 1 || div > 1023) {
@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,
ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
out:
- mutex_unlock(&indio_dev->mlock);
+ iio_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,
if (ret < 0)
return ret;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch ((u32)this_attr->address) {
case AD7192_REG_GPOCON:
@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
int ret, i;
unsigned int tmp;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_release_direct_mode(indio_dev);
return ret;
}
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC PATCH v2 0/2] iio: introduce iio_device_{claim|release}_direct_mode()
This patchset introduces two helper functions to simplify driver code
requiring the device to be locked in direct mode during execution of a
code path. The staging driver ad7192 is updated to demonstrate usage.
This could be applied to approximately 18 known cases where the driver
is holding the lock in direct mode. Unknown cases might be those that
should, but don't, hold the lock.
Alternate implementation: Generalize to support a claim on any mode.
Do iio_claim_mode(device,mode) where if the device is in *mode*, it
is guaranteed to stay that way until release is called. I considered
and rejected this option because a) not sure other modes would ever
need to be locked, and b) the semantic improvement is less when it
is generalized.
This patchset was inspired by a discussion on linux-iio:
http://www.spinics.net/lists/linux-iio/msg18540.html
Changes in v2:
o use iio_device prefix for new functions
o replace EINVAL with EBUSY on failure to claim direct mode
o update commit msg & changelog to reflect new prefix
Alison Schofield (2):
iio: core: implement iio_device_{claim|release}_direct_mode()
staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
drivers/iio/industrialio-core.c | 39 +++
drivers/staging/iio/adc/ad7192.c | 24 +---
include/linux/iio/iio.h | 2 ++
3 files changed, 50 insertions(+), 15 deletions(-)
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC PATCH v2 1/2] iio: core: implement iio_device_{claim|release}_direct_mode()
It is often the case that the driver wants to be sure a device stays
in direct mode while it is executing a task or series of tasks. To
accomplish this today, the driver performs this sequence: 1) take the
device state lock, 2) verify it is not in a buffered mode, 3) execute
some tasks, and 4) release that lock.
This patch introduces a pair of helper functions that simplify these
steps and make it more semantically expressive.
iio_device_claim_direct_mode()
If the device is not in any buffered mode it is guaranteed
to stay that way until iio_release_direct_mode() is called.
iio_device_release_direct_mode()
Release the claim. Device is no longer guaranteed to stay
in direct mode.
Signed-off-by: Alison Schofield
---
drivers/iio/industrialio-core.c | 39 +++
include/linux/iio/iio.h | 2 ++
2 files changed, 41 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 70cb7eb..2e768bc 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
#include "iio_core.h"
#include "iio_core_trigger.h"
@@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev,
struct iio_dev *indio_dev)
}
EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
+/**
+ * iio_device_claim_direct_mode - Keep device in direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * If the device is in direct mode it is guaranteed to stay
+ * that way until iio_device_release_direct_mode() is called.
+ *
+ * Use with iio_device_release_direct_mode()
+ *
+ * Returns: 0 on success, -EBUSY on failure
+ */
+int iio_device_claim_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_lock(&indio_dev->mlock);
+
+ if (iio_buffer_enabled(indio_dev)) {
+ mutex_unlock(&indio_dev->mlock);
+ return -EBUSY;
+ }
+ return 0;
+}
+EXPORT_SYMBOL_GPL(iio_device_claim_direct_mode);
+
+/**
+ * iio_device_release_direct_mode - releases claim on direct mode
+ * @indio_dev: the iio_dev associated with the device
+ *
+ * Release the claim. Device is no longer guaranteed to stay
+ * in direct mode.
+ *
+ * Use with iio_device_claim_direct_mode()
+ */
+void iio_device_release_direct_mode(struct iio_dev *indio_dev)
+{
+ mutex_unlock(&indio_dev->mlock);
+}
+EXPORT_SYMBOL_GPL(iio_device_release_direct_mode);
+
subsys_initcall(iio_init);
module_exit(iio_exit);
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index b2b1677..0b2773a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -527,6 +527,8 @@ void iio_device_unregister(struct iio_dev *indio_dev);
int devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev);
void devm_iio_device_unregister(struct device *dev, struct iio_dev *indio_dev);
int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
+int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
+void iio_device_release_direct_mode(struct iio_dev *indio_dev);
extern struct bus_type iio_bus_type;
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC PATCH v2 2/2] staging: iio: ad7192: use iio_device_{claim|release}_direct_mode()
Replace the code that guarantees the device stays in direct mode with
iio_device_{claim|release}_direct_mode() which does same.
Signed-off-by: Alison Schofield
---
drivers/staging/iio/adc/ad7192.c | 24 +---
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index f843f19..94a2e06 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -349,11 +349,9 @@ static ssize_t ad7192_write_frequency(struct device *dev,
if (lval == 0)
return -EINVAL;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
div = st->mclk / (lval * st->f_order * 1024);
if (div < 1 || div > 1023) {
@@ -366,7 +364,7 @@ static ssize_t ad7192_write_frequency(struct device *dev,
ad_sd_write_reg(&st->sd, AD7192_REG_MODE, 3, st->mode);
out:
- mutex_unlock(&indio_dev->mlock);
+ iio_device_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -434,11 +432,9 @@ static ssize_t ad7192_set(struct device *dev,
if (ret < 0)
return ret;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch ((u32)this_attr->address) {
case AD7192_REG_GPOCON:
@@ -461,7 +457,7 @@ static ssize_t ad7192_set(struct device *dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_device_release_direct_mode(indio_dev);
return ret ? ret : len;
}
@@ -555,11 +551,9 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
int ret, i;
unsigned int tmp;
- mutex_lock(&indio_dev->mlock);
- if (iio_buffer_enabled(indio_dev)) {
- mutex_unlock(&indio_dev->mlock);
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
return -EBUSY;
- }
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -582,7 +576,7 @@ static int ad7192_write_raw(struct iio_dev *indio_dev,
ret = -EINVAL;
}
- mutex_unlock(&indio_dev->mlock);
+ iio_device_release_direct_mode(indio_dev);
return ret;
}
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()
On Sat, Mar 05, 2016 at 06:02:36PM +, Jonathan Cameron wrote: > On 02/03/16 13:28, Lars-Peter Clausen wrote: > > On 03/01/2016 08:02 PM, Alison Schofield wrote: > >> It is often the case that the driver wants to be sure a device stays > >> in direct mode while it is executing a task or series of tasks. To > >> accomplish this today, the driver performs this sequence: 1) take the > >> device state lock, 2)verify it is not in a buffered mode, 3) execute > >> some tasks, and 4) release that lock. > >> > >> This patch introduces a pair of helper functions that simplify these > >> steps and make it more semantically expressive. > >> > >> iio_claim_direct_mode() > >> If the device is not in any buffered mode it is guaranteed > >> to stay that way until iio_release_direct_mode() is called. > >> > >> iio_release_direct_mode() > >> Release the claim. Device is no longer guaranteed to stay > >> in direct mode. > >> > >> Signed-off-by: Alison Schofield > > > > Looks basically good. > Agreed - nothing to add from me to what Lars has covered here. > Nice to 'hide' the accesses to mlock as well as will cut out the desire > to 'abuse it'. Amusingly we only just 'fixed' the docs to to say this > element of iio_dev was usable by drivers. Once we have these new functions > in use throughout the tree, we will want to flip that back again to internal > only. > > Jonathan > Thanks for the review (& Lars too) Thinking about your note about flipping the mlock field back to INTERNAL (from DRIVER), this change, even when it's applied to all relevant instances, doesn't get us all the way there. While these claim/release functions will remove direct access to mlock where a driver wants to hold direct mode, the drivers are grabbing mlock for other reasons also. (too many reasons/instances for me to quickly understand or summarize) I'm willing to look at it further and comment if that's helpful. alisons ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] iio: trigger: free trigger resource correctly
Using iio_trigger_put() to free a trigger leads to release of a resource we never held. Replace with iio_trigger_free(). Signed-off-by: Alison Schofield --- Patches to use devm_* funcs are ready to follow this for the interrupt & bfin-timer triggers. drivers/iio/trigger/iio-trig-interrupt.c | 4 ++-- drivers/iio/trigger/iio-trig-sysfs.c | 2 +- drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/iio/trigger/iio-trig-interrupt.c b/drivers/iio/trigger/iio-trig-interrupt.c index 572bc6f..b18e50d 100644 --- a/drivers/iio/trigger/iio-trig-interrupt.c +++ b/drivers/iio/trigger/iio-trig-interrupt.c @@ -84,7 +84,7 @@ static int iio_interrupt_trigger_probe(struct platform_device *pdev) error_free_trig_info: kfree(trig_info); error_put_trigger: - iio_trigger_put(trig); + iio_trigger_free(trig); error_ret: return ret; } @@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct platform_device *pdev) iio_trigger_unregister(trig); free_irq(trig_info->irq, trig); kfree(trig_info); - iio_trigger_put(trig); + iio_trigger_free(trig); return 0; } diff --git a/drivers/iio/trigger/iio-trig-sysfs.c b/drivers/iio/trigger/iio-trig-sysfs.c index 3dfab2b..202e8b8 100644 --- a/drivers/iio/trigger/iio-trig-sysfs.c +++ b/drivers/iio/trigger/iio-trig-sysfs.c @@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id) return 0; out2: - iio_trigger_put(t->trig); + iio_trigger_free(t->trig); free_t: kfree(t); out1: diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c index 9658f20..4e0b4ee 100644 --- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c +++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c @@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct platform_device *pdev) out1: iio_trigger_unregister(st->trig); out: - iio_trigger_put(st->trig); + iio_trigger_free(st->trig); return ret; } @@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct platform_device *pdev) peripheral_free(st->t->pin); free_irq(st->irq, st); iio_trigger_unregister(st->trig); - iio_trigger_put(st->trig); + iio_trigger_free(st->trig); return 0; } -- 2.1.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] iio: trigger: free trigger resource correctly
On Wed, Jan 18, 2017 at 12:56:29PM +0300, Dan Carpenter wrote: > On Tue, Jan 17, 2017 at 07:00:28PM -0800, Alison Schofield wrote: > > Using iio_trigger_put() to free a trigger leads to release of > > a resource we never held. Replace with iio_trigger_free(). > > They're basically the same except iio_trigger_put() puts the module and > the device and free only puts the device. > > I've looked at this briefly, but I can't figure out how iio_trigger_get/ > iio_trigger_put is supposed to be used. There isn't any documentation. > I'm trying to review this code, but I can't figure out where we *are* > supposed to be doing the put. > > For example, iio_device_unregister_trigger_consumer() takes a put, but > iio_device_register_trigger_consumer() doesn't do a get... It's all > very confusing. > > You seem like you know what's going on. Can we get some documentation? > Dan, Although I'm comfortable within the bounds of this fix (pair the alloc & free's and do not _put the module resource we never _get'd) beyond that, not so much. I'm just figuring it out myself. linux-iio experts...pointers, help? alisons > > > > Signed-off-by: Alison Schofield > > --- > > Patches to use devm_* funcs are ready to follow this for > > the interrupt & bfin-timer triggers. > > > > drivers/iio/trigger/iio-trig-interrupt.c | 4 ++-- > > drivers/iio/trigger/iio-trig-sysfs.c | 2 +- > > drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++-- > > 3 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/trigger/iio-trig-interrupt.c > > b/drivers/iio/trigger/iio-trig-interrupt.c > > index 572bc6f..b18e50d 100644 > > --- a/drivers/iio/trigger/iio-trig-interrupt.c > > +++ b/drivers/iio/trigger/iio-trig-interrupt.c > > @@ -84,7 +84,7 @@ static int iio_interrupt_trigger_probe(struct > > platform_device *pdev) > > error_free_trig_info: > > kfree(trig_info); > > error_put_trigger: > > - iio_trigger_put(trig); > > + iio_trigger_free(trig); > > > We could rename this label. > > regards, > dan carpenter > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] iio: trigger: free trigger resource correctly
These stand-alone trigger drivers were using iio_trigger_put()
where they should have been using iio_trigger_free(). The
iio_trigger_put() adds a module_put which is bad since they
never did a module_get.
In the sysfs driver, module_get/put's are used as triggers are
added & removed. This extra module_put() occurs on an error path
in the probe routine (probably rare).
In the bfin-timer & interrupt trigger drivers, the module resources
are not explicitly managed, so it's doing a put on something that
was never get'd. It occurs on the probe error path and on the
remove path (not so rare).
Tested with the sysfs trigger driver.
The bfin & interrupt drivers were build tested & inspected only.
Signed-off-by: Alison Schofield
---
Changes in v2:
Renamed probe jump label to say free instead of put.
Added further explanation to commit log.
Note from v1:
Patches to use devm_* funcs are ready to follow this for
the interrupt & bfin-timer triggers.
drivers/iio/trigger/iio-trig-interrupt.c | 8
drivers/iio/trigger/iio-trig-sysfs.c | 2 +-
drivers/staging/iio/trigger/iio-trig-bfin-timer.c | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/trigger/iio-trig-interrupt.c
b/drivers/iio/trigger/iio-trig-interrupt.c
index 572bc6f..e18f12b 100644
--- a/drivers/iio/trigger/iio-trig-interrupt.c
+++ b/drivers/iio/trigger/iio-trig-interrupt.c
@@ -58,7 +58,7 @@ static int iio_interrupt_trigger_probe(struct platform_device
*pdev)
trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
if (!trig_info) {
ret = -ENOMEM;
- goto error_put_trigger;
+ goto error_free_trigger;
}
iio_trigger_set_drvdata(trig, trig_info);
trig_info->irq = irq;
@@ -83,8 +83,8 @@ static int iio_interrupt_trigger_probe(struct platform_device
*pdev)
free_irq(irq, trig);
error_free_trig_info:
kfree(trig_info);
-error_put_trigger:
- iio_trigger_put(trig);
+error_free_trigger:
+ iio_trigger_free(trig);
error_ret:
return ret;
}
@@ -99,7 +99,7 @@ static int iio_interrupt_trigger_remove(struct
platform_device *pdev)
iio_trigger_unregister(trig);
free_irq(trig_info->irq, trig);
kfree(trig_info);
- iio_trigger_put(trig);
+ iio_trigger_free(trig);
return 0;
}
diff --git a/drivers/iio/trigger/iio-trig-sysfs.c
b/drivers/iio/trigger/iio-trig-sysfs.c
index 3dfab2b..202e8b8 100644
--- a/drivers/iio/trigger/iio-trig-sysfs.c
+++ b/drivers/iio/trigger/iio-trig-sysfs.c
@@ -174,7 +174,7 @@ static int iio_sysfs_trigger_probe(int id)
return 0;
out2:
- iio_trigger_put(t->trig);
+ iio_trigger_free(t->trig);
free_t:
kfree(t);
out1:
diff --git a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
index 9658f20..4e0b4ee 100644
--- a/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
+++ b/drivers/staging/iio/trigger/iio-trig-bfin-timer.c
@@ -260,7 +260,7 @@ static int iio_bfin_tmr_trigger_probe(struct
platform_device *pdev)
out1:
iio_trigger_unregister(st->trig);
out:
- iio_trigger_put(st->trig);
+ iio_trigger_free(st->trig);
return ret;
}
@@ -273,7 +273,7 @@ static int iio_bfin_tmr_trigger_remove(struct
platform_device *pdev)
peripheral_free(st->t->pin);
free_irq(st->irq, st);
iio_trigger_unregister(st->trig);
- iio_trigger_put(st->trig);
+ iio_trigger_free(st->trig);
return 0;
}
--
2.1.4
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
