RE: [PATCH 2/2 v4] usb: ohci: remove ep93xx bus glue platform driver
On Saturday, October 19, 2013 2:22 PM, Greg KH wrote: > On Thu, Oct 17, 2013 at 01:29:11PM -0700, H Hartley Sweeten wrote: >> Convert ep93xx to use the OHCI platform driver and remove the >> ohci-ep93xx bus glue driver. >> >> Enable CONFIG_OHCI_HCD_PLATFORM in the ep93xx_defconfig so that USB >> is still enabled by default on the EP93xx platform. >> >> Signed-off-by: H Hartley Sweeten >> Acked-by: Alan Stern >> Cc: Ryan Mallon >> Cc: Lennert Buytenhek >> Cc: Greg Kroah-Hartman >> Cc: Olof Johansson >> Cc: Russell King >> --- >> arch/arm/configs/ep93xx_defconfig | 1 + >> arch/arm/mach-ep93xx/clock.c | 2 +- >> arch/arm/mach-ep93xx/core.c | 38 +++- >> drivers/usb/host/ohci-ep93xx.c| 184 >> -- >> drivers/usb/host/ohci-hcd.c | 18 >> 5 files changed, 36 insertions(+), 207 deletions(-) >> delete mode 100644 drivers/usb/host/ohci-ep93xx.c > > This patch doesn't apply to my usb-next branch of usb.git at all, what > happened? Did Alan's other recent changes interfere with this? With the temporary linux-next shutdown it appears that I based this on the mainline git tree. Sorry about that. > Can you refresh this against my tree and resend it so that I can apply > it? I've applied the 1/2 patch already. I'll try to pull usb-next today and rebase this. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/2 v4] usb: ohci: remove ep93xx bus glue platform driver
On Saturday, October 19, 2013 2:22 PM, Greg KH wrote: > On Thu, Oct 17, 2013 at 01:29:11PM -0700, H Hartley Sweeten wrote: >> Convert ep93xx to use the OHCI platform driver and remove the >> ohci-ep93xx bus glue driver. >> >> Enable CONFIG_OHCI_HCD_PLATFORM in the ep93xx_defconfig so that USB >> is still enabled by default on the EP93xx platform. >> >> Signed-off-by: H Hartley Sweeten [snip] > This patch doesn't apply to my usb-next branch of usb.git at all, what > happened? Did Alan's other recent changes interfere with this? > > Can you refresh this against my tree and resend it so that I can apply > it? I've applied the 1/2 patch already. Greg, I just refreshed this patch against usb-next and sent it as v5. Thanks, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH][Resend] Staging: comedi: replace printk() calls with dev_dbg() in pcmmio.c
On Wednesday, January 01, 2014 6:55 PM, Chase Southwood wrote: > > This is a patch for pcmmio.c that changes several printk() calls to dev_dbg() > or dev_err() to > fix checkpatch.pl warnings. Patched from 3.13-rc6. > > Signed-off-by: Chase Southwood > --- > drivers/staging/comedi/drivers/pcmmio.c | 23 +-- > 1 file changed, 13 insertions(+), 10 deletions(-) Hello Chase, This was already fixed, and merged into linux-next, by commit 4bb82d647dad7be06341ffdb9f07a56a387e213f Author: H Hartley Sweeten Date: Tue Nov 26 10:21:23 2013 -0700 staging: comedi: pcmmio: remove DAMMIT_ITS_BROKEN debug Please base any comedi patches on linux-next. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: Humusoft MF634 and MF624 DAQ cards driver
On Monday, December 30, 2013 6:37 PM, Rostislav Lisovy wrote: > > create mode 100644 drivers/staging/comedi/drivers/mf6x4.c Hello Rostislav, As pointed out by Dan Carpenter, you need to add a change log and Signed-off-by lines to this patch. Overall this looks pretty good. Comments below. > diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig > index bfa27e7..89e25b4 100644 > --- a/drivers/staging/comedi/Kconfig > +++ b/drivers/staging/comedi/Kconfig > @@ -884,6 +884,12 @@ config COMEDI_GSC_HPDI > To compile this driver as a module, choose M here: the module will be > called gsc_hpdi. > > +config COMEDI_MF6X4 > + tristate "Humusoft MF634 and MF624 DAQ Card support" > + ---help--- > + This driver supports both Humusoft MF634 and MF624 Data acquisition > + cards. The legacy Humusoft MF614 card is not supported. > + > config COMEDI_ICP_MULTI > tristate "Inova ICP_MULTI support" > ---help--- > diff --git a/drivers/staging/comedi/drivers/Makefile > b/drivers/staging/comedi/drivers/Makefile > index 94cbd26..9e979a9 100644 > --- a/drivers/staging/comedi/drivers/Makefile > +++ b/drivers/staging/comedi/drivers/Makefile > @@ -110,6 +110,7 @@ obj-$(CONFIG_COMEDI_NI_PCIMIO)+= ni_pcimio.o > obj-$(CONFIG_COMEDI_RTD520) += rtd520.o > obj-$(CONFIG_COMEDI_S626)+= s626.o > obj-$(CONFIG_COMEDI_SSV_DNP) += ssv_dnp.o > +obj-$(CONFIG_COMEDI_MF6X4) += mf6x4.o > > # Comedi PCMCIA drivers > obj-$(CONFIG_COMEDI_CB_DAS16_CS) += cb_das16_cs.o > diff --git a/drivers/staging/comedi/drivers/mf6x4.c > b/drivers/staging/comedi/drivers/mf6x4.c > new file mode 100644 > index 000..46c7ce5 > --- /dev/null > +++ b/drivers/staging/comedi/drivers/mf6x4.c > @@ -0,0 +1,368 @@ > +/* > + * comedi/drivers/mf6x4.c > + * Driver for Humusoft MF634 and MF624 Data acquisition cards > + * > + * COMEDI - Linux Control and Measurement Device Interface > + * Copyright (C) 2000 David A. Schleef > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +/* > + * Driver: mf6x4 > + * Description: Humusoft MF634 and MF624 Data acquisition card driver > + * Devices: Humusoft MF634, Humusoft MF624 > + * Author: Rostislav Lisovy > + * Status: works > + * Updated: > + * Configuration Options: none > + */ > + > +#include > +#include > +#include "../comedidev.h" > + > +/* PCI Vendor ID, Device ID */ > +#define PCI_VENDOR_ID_HUMUSOFT 0x186c > +#define PCI_DEVICE_ID_MF624 0x0624 > +#define PCI_DEVICE_ID_MF634 0x0634 Please put the PCI_VENDOR_ID_* in comedidev.h with the other PCI Vendor IDs. Also, remove the PCI_DEVICE_ID_* defines and just open code the values in the pci_device_id table. The mf6x4_boardid enums provide adequate documentation. > + > +/* Registers present in BAR0 memory region */ > +#define MF624_GPIOC_reg 0x54 > + > +#define MF6X4_GPIOC_EOLC /* End Of Last Conversion */(1 << 17) > +#define MF6X4_GPIOC_LDAC /* Load DACs */ (1 << 23) > +#define MF6X4_GPIOC_DACEN(1 << 26) > + > +/* BAR1 registers */ > +#define MF6X4_DIN_reg0x10 > +#define MF6X4_DIN_mask 0xff > +#define MF6X4_DOUT_reg 0x10 > +#define MF6X4_DOUT_mask 0xff > + > +#define MF6X4_ADSTART_reg0x20 > +#define MF6X4_ADDATA_reg 0x00 > +#define MF6X4_ADDATA_mask0x3fff > +#define MF6X4_ADCTRL_reg 0x00 > +#define MF6X4_ADCTRL_mask0xff > + > +#define MF6X4_DA0_reg0x20 > +#define MF6X4_DA1_reg0x22 > +#define MF6X4_DA2_reg0x24 > +#define MF6X4_DA3_reg0x26 > +#define MF6X4_DA4_reg0x28 > +#define MF6X4_DA5_reg0x2a > +#define MF6X4_DA6_reg0x2c > +#define MF6X4_DA7_reg0x2e > +#define MF6X4_DA_mask0x3fff > +#define MF6X4_DAC_CHANN_CNT 8 > + > +/* BAR2 reg
RE: [PATCH] staging: comedi: amplc_pci230: fix DACOUT write
On Monday, September 08, 2014 12:15 PM, Greg Kroah-Hartman wrote:
> On Tue, Aug 19, 2014 at 12:32:36PM +0100, Ian Abbott wrote:
>> Commit 4f9c63fe5333b27ab23ed399830c7977f6970744 ("staging: comedi:
>> amplc_pci230: refactor iobase addresses") removed some parentheses
>> (presumably to keep the line withing 80 chars) in
>> `pci230_ao_write_nofifo()` when writing to the DACOUT1 or DACOUT2
>> registers, but it removed the wrong parentheses. Fix it.
>>
>> Signed-off-by: Ian Abbott
>> ---
>> This bug is in linux-next master and staging-next.
>> ---
>> drivers/staging/comedi/drivers/amplc_pci230.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/amplc_pci230.c
>> b/drivers/staging/comedi/drivers/amplc_pci230.c
>> index 0fd212f..dd69e47 100644
>> --- a/drivers/staging/comedi/drivers/amplc_pci230.c
>> +++ b/drivers/staging/comedi/drivers/amplc_pci230.c
>> @@ -628,7 +628,7 @@ static inline void pci230_ao_write_nofifo(struct
>> comedi_device *dev,
>>
>> /* Write mangled datum to appropriate DACOUT register. */
>> outw(pci230_ao_mangle_datum(dev, datum),
>> - devpriv->daqio + ((chan) == 0) ? PCI230_DACOUT1 : PCI230_DACOUT2);
>> + devpriv->daqio + (chan == 0 ? PCI230_DACOUT1 : PCI230_DACOUT2));
>> }
>>
>> static inline void pci230_ao_write_fifo(struct comedi_device *dev,
>> --
>> 2.0.4
>
> This doesn't apply to my tree anymore, is it still needed?
Looks like you already applied a similar patch from Dan Carpenter:
commit 94254d1baec765b22cc5df3a9a16a8cc9a79d406
Author: Dan Carpenter
Date: Tue Aug 26 10:55:51 2014 +0300
staging: comedi: amplc_pci230: fix a precedence bug
Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/48] staging: comedi: avoid using comedi_board()
On Tuesday, September 09, 2014 3:26 AM, Ian Abbott wrote: > > The `comedi_board` inline function takes a single parameter of type > `struct comedi_device *` and merely returns the value of the `board_ptr` > member therein. This is somewhat superfluous as the member can be > accessed directly. > > Replace all uses of `comedi_board(dev)` with `dev->board_ptr`. > > Note, if there are any conflicts when applying this series, they can > just be skipped for now and corrected later. > > A subsequent patch will remove the `comedi_board` function, once there > is nothing using it. I would argue that the comedi_board() inline is a bit cleaner. But, since all the goofy macros and private boardinfo access functions in the drivers have been removed I guess this can go now. We will need to make that sure any new drivers don't introduce new goofy macros or access functions. The 'board(dev)->foo' ones really made the drivers hard to read... Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: ni_at_a2150: range check board index
On Monday, September 01, 2014 6:14 AM, Ian Abbott wrote: > The "ni_at_a2150" driver determines the board type by calling > `a2150_probe()`. This reads a register and converts it to a board index > in the range 0 to 3. However, the board table array it indexes into > (`a2150_boards[]`) only has 2 entries. Return an error from the > Comedi driver "attach" handler `a2150_attach()` if the probed board > index is beyond the end of the array. > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/37] staging: comedi: rename command flags for consistency
On Wednesday, September 03, 2014 5:45 AM, Ian Abbott wrote: > > The various macros in "comedi.h" used to construct values for the > `flags` member of `struct comedi_cmd` are named inconsistently. Some of > the macros with the `TRIG_` prefix are from a long defunct Comedi > trigger interface that was replaced with the current Comedi asynchronous > command interface many years ago, co-opting most of the `TRIG_` macros > in the process. Some of them are still used with various other members > of `struct comedi_cmd` to select trigger sources. Others are used with > the `flags` member. There are other macros for use with the command > `flags` member using the prefix `CMDF_`, and some of the old `TRIG_` > macros have been renamed already, keeping the old names around as > synonyms for backwards compatibility. > > For consistency, rename the `TRIG_` prefixed macros used with the > command `flags` member to use the prefix `CMDF_` and redefine the > renamed macros as synonyms for backwards compatibilty. > > There are still some left-over `TRIG_` macros unused by the Comedi > kernel code, but the user-space Comedilib library may still be trying to > use them, so hang onto them for now. Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: hwdrv_apci1500: use dev->class_dev in calls to dev_warn()
On Wednesday, September 03, 2014 4:59 PM, Chase Southwood wrote:
> git-grep reveals that hwdrv_apci1500.c is the only file in comedi that uses
> dev->hw_dev in calls to dev_{err,warn}(). The rest of the drivers pass
> dev->class_dev to these macros instead. Switch the dev_warn() calls in
> this driver to use dev->class_dev as well, for consistency.
>
> Signed-off-by: Chase Southwood
> Cc: Ian Abbott
> Cc: H Hartley Sweeten
Hmm.. I thought I caught all of these.
Thanks
Reviewed-by: H Hartley Sweeten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/28] staging: comedi: more clean-up and remove legacy attach
On Monday, September 01, 2014 4:04 AM, Ian Abbott wrote: > Continue to clean up the amplc_pci230 driver code and remove the legacy > attach mechanism, since it isn't very useful for this driver (see PATCH > 05/28). Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 15/18 v2] staging: comedi: amplc_pci224: put board indices in PCI driver_data
On Monday, August 04, 2014 4:14 AM, Ian Abbott wrote: > The `driver_data` member value from the matched entry of the PCI module > device table `amplc_pci224_pci_table[]` is passed through to our comedi > "auto_attach" handler, `pci224_auto_attach()`. Use that to index > directly into our static board data array `pci224_boards[]` instead of > calling `pci224_find_pci_board()` to search for the entry matching the > PCI device ID. That function can be removed. The `devid` and `model` > members of `struct pci224_board` are no longer needed either and can be > removed. > > Signed-off-by: Ian Abbott > --- > v2: Use C99 syntax for amplc_pci224_pci_table[] initializer. Thanks to > Hartley for finding that! Reviewed-by: H Hartley Sweeten Thanks, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: introduce some sample size manipulation functions
On Thursday, October 23, 2014 5:48 AM, Ian Abbott wrote:
> Introduce a few static inline helper functions:
>
> `comedi_bytes_per_sample(s)` is the same as the existing
> `bytes_per_sample(s)` and determines the size of a comedi sample in
> bytes. (`bytes_per_sample(s)` will be removed.)
>
> `comedi_sample_shift(s)` determines the log2 of the comedi sample size,
> so it can be used in bit-shift operations to multiply or divide by the
> sample size.
>
> `comedi_bytes_to_samples(s, nbytes)` converts a number of bytes to a
> number of samples (rounding down).
>
> `comedi_samples_to_bytes(s, nsamples)` converts a number of samples to a
> number of bytes.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/comedidev.h | 63
> +++---
> 1 file changed, 59 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedidev.h
> b/drivers/staging/comedi/comedidev.h
> index 1b2bbd5..69cf6fe 100644
> --- a/drivers/staging/comedi/comedidev.h
> +++ b/drivers/staging/comedi/comedidev.h
> @@ -391,12 +391,67 @@ static inline unsigned int comedi_offset_munge(struct
> comedi_subdevice *s,
> return val ^ s->maxdata ^ (s->maxdata >> 1);
> }
>
> -static inline unsigned int bytes_per_sample(const struct comedi_subdevice
> *subd)
> +/**
> + * comedi_bytes_per_sample - determine subdevice sample size
> + * @s: comedi_subdevice struct
> + *
> + * The sample size will be 4 (sizeof int) or 2 (sizeof short) depending on
> + * whether the SDF_LSAMPL subdevice flag is set or not.
> + *
> + * Returns the subdevice sample size.
> + */
> +static inline unsigned int comedi_bytes_per_sample(struct comedi_subdevice
> *s)
> {
> - if (subd->subdev_flags & SDF_LSAMPL)
> - return sizeof(unsigned int);
> + return s->subdev_flags & SDF_LSAMPL ? sizeof(int) : sizeof(short);
The samples are really 'unsigned' types but I guess it doesn't matter here.
> +}
Reviewed-by: H Hartley Sweeten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/2] staging: comedi: per-file read/write subdevice choice
On Tuesday, November 04, 2014 11:09 AM, Ian Abbott wrote: > This series of patches adds a couple of ioctl codes to the Comedi core > to allow the current "read" and "write" subdevice to be changed after > opening the comedi device. The current read and write subdevice > information is stored in file private data allocated for the lifetime of > the file object, so the notion of current "read" and "write" subdevice > is local to the file object and does not alter anything in the main > control structure for the comedi device. An extra level of indirection > is now required to access the main control structure. > > I've tested the multiple "read" subdevice case using a modified version > of the "comedi_test" module (modifying it to clone the existing AI > subdevice), and a modified version of the "comedi_test" application > (part of the "comedilib" installation) modified to use the new ioctls. > There isn't any support in comedilib itself yet. > > 1) staging: comedi: prepare support for per-file read and write >subdevices > 2) staging: comedi: add ioctls to set per-file read and write subdevice > > drivers/staging/comedi/comedi.h | 2 + > drivers/staging/comedi/comedi_compat32.c | 2 + > drivers/staging/comedi/comedi_fops.c | 217 > +++ > 3 files changed, 196 insertions(+), 25 deletions(-) Ian, If I understand this correctly, the user can now open the "comediX" device which will give then access to the default dev->read_subdev and dev->write_subdev. They can then use the new ioctls to change the read/write subdevice and use them without having the open the "comediX_subdY" devices. If this is correct it seems like a good idea. Reviewed-by: H Hartley Sweeten Also, does this mean we can get rid of comedi_alloc_subdevice_minor() and the "comediX_sudbY" stuff? Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/5] staging: comedi: split bus support into separate modules
On Wednesday, November 05, 2014 3:51 PM, Greg Kroah-Hartman wrote: > On Fri, Oct 31, 2014 at 11:00:54PM +, Ian Abbott wrote: >> On 31/10/14 22:53, Ian Abbott wrote: >>> I like the idea of the core comedi module being separated from all the >>> various buses. And other buses can be added easily without pulling extra >>> dependencies into the core (e.g. for platform or spi devices). >> >> And if this get's accepted, I'd like to complete the separation by moving >> the PCI, USB and PCMCIA stuf out of the "comedidev.h" header into their own >> headers, which obviously means small edits to most of the low-level drivers >> to #include different headers. (But then, "comedi_pci.h" could #include >> for example.) > > Hartley, any objection to me applying this series? No objections... Just questions... ;-) Reviewed-by: H Hartley Sweeten Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"
On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
> If a Comedi asynchronous command has been set up for data transfer in
> the "write" direction on the current "read" subdevice (for those
> subdevices that support both directions), don't allow the "read" file
> operation as that would mess with the data in the comedi data buffer
> that is read by the low-level comedi hardware driver.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/comedi_fops.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index fef68da..6805ec9 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2210,6 +2210,10 @@ static ssize_t comedi_read(struct file *file, char
> __user *buf, size_t nbytes,
> retval = -EACCES;
> goto out;
> }
> + if (async->cmd.flags & CMDF_WRITE) {
> + retval = -EINVAL;
> + goto out;
> + }
>
> add_wait_queue(&async->wait_head, &wait);
> while (nbytes > 0 && !retval) {
> @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char
> __user *buf, size_t nbytes,
> retval = -EACCES;
> break;
> }
> + if (async->cmd.flags & CMDF_WRITE) {
> + retval = -EINVAL;
> + break;
> + }
Is this second test really needed in the while() loop?
For that matter, are the s->busy tests needed in the while() loop?
> continue;
> }
> m = copy_to_user(buf, async->prealloc_buf +
Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 5/7] staging: comedi: don't allow write() on async command set up for "read"
On Thursday, October 30, 2014 5:43 AM, Ian Abbott wrote:
> If a Comedi asynchronous command has been set up for data transfer in
> the "read" direction on the current "write" subdevice (for those
> subdevices that support both directions), don't allow the "write" file
> operation as that would mess with the data in the comedi data buffer
> that is written by the low-level comedi hardware driver.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/comedi_fops.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index 6805ec9..6328965 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2075,6 +2075,10 @@ static ssize_t comedi_write(struct file *file, const
> char __user *buf,
> retval = -EACCES;
> goto out;
> }
> + if (!(async->cmd.flags & CMDF_WRITE)) {
> + retval = -EINVAL;
> + goto out;
> + }
>
> add_wait_queue(&async->wait_head, &wait);
> on_wait_queue = true;
> @@ -2146,6 +2150,10 @@ static ssize_t comedi_write(struct file *file, const
> char __user *buf,
> retval = -EACCES;
> break;
> }
> + if (!(async->cmd.flags & CMDF_WRITE)) {
> + retval = -EINVAL;
> + break;
> + }
Same question as with PATCH 4/7.
Is this test needed in the while () loop. Also, are the s->busy tests needed
here?
> continue;
> }
Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 4/7] staging: comedi: don't allow read() on async command set up for "write"
On Thursday, October 30, 2014 1:28 PM, Ian Abbott wrote:
> On 30/10/14 18:05, Hartley Sweeten wrote:
>> On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote:
> [snip]
>>> add_wait_queue(&async->wait_head, &wait);
>>> while (nbytes > 0 && !retval) {
>>> @@ -2249,6 +2253,10 @@ static ssize_t comedi_read(struct file *file, char
>>> __user *buf, size_t nbytes,
>>> retval = -EACCES;
>>> break;
>>> }
>>> + if (async->cmd.flags & CMDF_WRITE) {
>>> + retval = -EINVAL;
>>> + break;
>>> + }
>>
>> Is this second test really needed in the while() loop?
>>
>> For that matter, are the s->busy tests needed in the while() loop?
>
> To answer your second question, some other thread using the same file
> object might have cancelled the asynchronous command, causing the
> current thread to see that the command is no longer active when it wakes up.
>
> To answer your first question, that other thread might have managed to
> set up another asynchronous command in before we wake up, and it might
> have been set up as a "write" command (if the subdevice supports
> commands in both directions). This doesn't detect the case when the
> other thread has managed to set up another "read" command, but since the
> current read() call hasn't read any data yet, we can just pretend we
> didn't know about the original command and read data from the new
> command instead. (After all, the calling thread can't prove the read()
> started before the first command was cancelled, so we can just pretend
> it didn't.)
But when the command is first started by do_cmd_ioctl() we have this sequence:
if (s->busy)
return -EBUSY;
...
s->busy = file;
ret = s->do_cmd(dev, s);
>From then on the s->busy pointer can only be cleared in do_become_nonbusy()
(by way of a (*cancel)). So another command cannot be started until the current
command is completed.
The user could do a (*do_cmdtest) while a command is running but that does not
effect the read/write of the async buffer.
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/7] staging: comedi: enforce data transfer direction
On Thursday, October 30, 2014 5:42 AM, Ian Abbott wrote: > Most comedi subdevices that support asynchronous data acquisition > commands only support data transfer in a single direction, as indicated > by the `SDF_CMD_READ` and `SDF_CMD_WRITE` subdevice flags. A few allow > data transfer in either direction, but only a single direction at a > time, as indicated by the `CMDF_WRITE` command flag in the `flags` > member of the `struct comedi_cmd` from user-space used to set up the > asynchronous command. The `CMDF_WRITE` flag isn't commonly set for > "write" commands if that is the only direction the subdevice supports. > > Since it would be useful for comedi to have a clear indication of which > data transfer direction is being used, patch 1 forces the `CMDF_WRITE` > command flag to the correct state if only one direction is supported. > The flag can then be checked by comedi in the subdevice's local copy of > the current command (in `s->async->cmd`) to determine the data transfer > direction unambiguously. > > Patch 2 stops the "me4000" driver clobbering the command flags. > > Patch 3 removes some now redundant modification of the `CMDF_WRITE` flag > in "ni_mio_common.c" (used by the "ni_pcimio", "ni_atmio" and > "ni_mio_cs" drivers). > > Patches 4 and 5 disallow the read() and write() file operations if the > subdevice has been set up with a command in the "write" or "read" data > transfer direction respectively. > > Patch 6 changes the readable/writeable checks for the poll() file > operation. It already considers the file readable/writeable if > read()/write() would fail with an error, so change it to check for > "wrong direction" as one of the possible failures. > > Patch 7 makes the direction test in the handling of the `COMEDI_BUFINFO` > ioctl (used to update buffer positions for mmapped buffers) less > ambiguous. The current test used the `SDF_CMD_READ` and `SDF_CMD_WRITE` > subdevice flags to check the direction, which is ambiguous if both flags > are set. Update it to use the `CMDF_WRITE` command flag instead. Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/2] staging: comedi: das16: fix some timer sync issues
On Wednesday, October 29, 2014 10:35 AM, Ian Abbott wrote: > "das16" uses a kernel timer but never removes it from the queue > synchronously at the moment. Patch 1 makes sure this is done before it > is destroyed. Patch 2 uses the comedi device's main spin-lock to ensure > some state shared with the timer routine is updated in an SMP-safe > manner. > > 1) staging: comedi: das16: deschedule timer routine on detach > 2) staging: comedi: das16: use spin-lock when setting timer > > drivers/staging/comedi/drivers/das16.c | 9 - > 1 file changed, 8 insertions(+), 1 deletion(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: icp_multi: correct insn_bits returned data for DO
On Friday, October 31, 2014 7:32 AM, Ian Abbott wrote: > For some unfathomable reason, the Comedi `insn_bits` handler for the > digital output subdevice (`icp_multi_insn_bits_do()`) writes the digital > output register and reads back the unrelated digital input register. > Read back the current state of the outputs (held in `s->state`) instead. > > Signed-off-by: Ian Abbott > --- > drivers/staging/comedi/drivers/icp_multi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/icp_multi.c > b/drivers/staging/comedi/drivers/icp_multi.c > index f4e1c1c..db744c8 100644 > --- a/drivers/staging/comedi/drivers/icp_multi.c > +++ b/drivers/staging/comedi/drivers/icp_multi.c > @@ -53,7 +53,7 @@ Configuration options: not applicable, uses PCI auto config > #define ICP_MULTI_AI 2 /* R: Analogue input data */ > #define ICP_MULTI_DAC_CSR4 /* R/W: DAC command/status register */ > #define ICP_MULTI_AO 6 /* R/W: Analogue output data */ > -#define ICP_MULTI_DI 8 /* R/W: Digital inouts */ > +#define ICP_MULTI_DI 8 /* R/W: Digital inputs */ > #define ICP_MULTI_DO 0x0A/* R/W: Digital outputs */ > #define ICP_MULTI_INT_EN 0x0C/* R/W: Interrupt enable register */ > #define ICP_MULTI_INT_STAT 0x0E/* R/W: Interrupt status register */ > @@ -319,7 +319,7 @@ static int icp_multi_insn_bits_do(struct comedi_device > *dev, > if (comedi_dio_update_state(s, data)) > writew(s->state, dev->mmio + ICP_MULTI_DO); > > - data[1] = readw(dev->mmio + ICP_MULTI_DI); > + data[1] = s->state; > > return insn->n; > } Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/5] staging: comedi: split bus support into separate modules
On Friday, October 31, 2014 10:48 AM, Ian Abbott wrote: > The Comedi core module doesn't need support for PCI, USB or PCMCIA. > Only the low-level Comedi drivers need it. Split the support for these > bus types out of the core "comedi" module and into new modules, > "comedi_pci", "comedi_usb", and "comedi_pcmcia". > > 1) staging: comedi: comedidev.h: remove dummy PCI support functions > 2) staging: comedi: comedidev.h: remove some #ifdefs > 3) staging: comedi: split out PCMCIA support into new module > 4) staging: comedi: split out USB support into new module > 5) staging: comedi: split out PCI support into new module Ian, Is this really necessary? The pci, usb, and pcmcia support is already conditionally compiled in. The support does get added to the main comedi module instead of as separate modules but that shouldn't be a problem. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/5] staging: comedi: split bus support into separate modules
On Friday, October 31, 2014 3:09 PM, Ian Abbott wrote: > On 31/10/14 18:19, Hartley Sweeten wrote: >> On Friday, October 31, 2014 10:48 AM, Ian Abbott wrote: >>> The Comedi core module doesn't need support for PCI, USB or PCMCIA. >>> Only the low-level Comedi drivers need it. Split the support for these >>> bus types out of the core "comedi" module and into new modules, >>> "comedi_pci", "comedi_usb", and "comedi_pcmcia". >>> >>> 1) staging: comedi: comedidev.h: remove dummy PCI support functions >>> 2) staging: comedi: comedidev.h: remove some #ifdefs >>> 3) staging: comedi: split out PCMCIA support into new module >>> 4) staging: comedi: split out USB support into new module >>> 5) staging: comedi: split out PCI support into new module >> >> Ian, >> >> Is this really necessary? >> >> The pci, usb, and pcmcia support is already conditionally compiled in. >> The support does get added to the main comedi module instead of >> as separate modules but that shouldn't be a problem. > > Well a lot of potentially unused module space could get pulled in if > using a stock distro kernel. For example, the USB dependencies amount to > over 200k. Not so bad for the others, especially PCI where the code is > built in anyway. But its only pulled in if the user selected the usb, pci, and/or pcmcia comedi drivers. And if they selected any of those options they will need the that code anyway. Splitting the bus specific code out of the comdi core module doesn't lower the total module space used it just spreads it from one to possibly four modules. And it increases the module dependancies for all the usb, pci, and pcmcia comedi drivers. To be clear, I'm not against this series I just want to make sure it's worth applying. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] staging: comedi: usbduxsigma: updated contact details and status
On Friday, October 10, 2014 12:33 PM, Bernd Porr wrote: > > I've updated my contact details of the driver. I've also tested it > thoroughly and it works perfectly. I've changed the status to stable. > > Signed-off-by: Bernd Porr Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 2/3] staging: comedi: usbdux: updated contact details / comments
On Friday, October 10, 2014 12:34 PM, Bernd Porr wrote: > > I've updated my contact details and removed obsolete comments. > > Signed-off-by: Bernd Porr Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/3] staging: comedi: usbduxfast: updated address details
On Friday, October 10, 2014 12:35 PM, Bernd Porr wrote: > > Updated the range of years, e-mail and added driver desription as > usually done in comedi. > > Signed-off-by: Bernd Porr Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: (regression) channel list must be set for COMEDI_CMD ioctl
On Thursday, October 09, 2014 2:03 AM, Ian Abbott wrote:
> On 09/10/14 00:13, Hartley Sweeten wrote:
>> On Wednesday, October 08, 2014 8:09 AM, Ian Abbott wrote:
>>> `do_cmd_ioctl()`, the handler for the `COMEDI_CMD` ioctl can incorrectly
>>> call the Comedi subdevice's `do_cmd()` handler with a NULL channel list
>>> pointer. This is a regression as the `do_cmd()` handler has never been
>>> expected to deal with that, leading to a kernel OOPS when it tries to
>>> dereference it.
>>>
>>> A NULL channel list pointer is allowed for the `COMEDI_CMDTEST` ioctl,
>>> handled by `do_cmdtest_ioctl()` and the subdevice's `do_cmdtest()`
>>> handler, but not for the `COMEDI_CMD` ioctl and its handlers.
>>>
>>> Both `do_cmd_ioctl()` and `do_cmdtest_ioctl()` call
>>> `__comedi_get_user_chanlist()` to copy the channel list from user memory
>>> into dynamically allocated kernel memory and check it for consistency.
>>> That function currently returns 0 if the `user_chanlist` parameter
>>> (pointing to the channel list in user memory) is NULL. That's fine for
>>> `do_cmdtest_ioctl()`, but `do_cmd_ioctl()` incorrectly assumes the
>>> kernel copy of the channel list has been set-up correctly.
>>>
>>> Fix it by not allowing the `user_chanlist` parameter to be NULL in
>>> `__comedi_get_user_chanlist()`, and only calling it from
>>> `do_cmdtest_ioctl()` if the parameter is non-NULL.
>>>
>>> Thanks to Bernd Porr for reporting the bug via an initial patch sent
>>> privately.
>>>
>>> Fixes: c6cd0eefb27b ("staging: comedi: comedi_fops: introduce
>>> __comedi_get_user_chanlist()")
>>> Reported-by: Bernd Porr
>>> Signed-off-by: Ian Abbott
>>> Cc: Bernd Porr
>>> Cc: # 3.15.y 3.16.y 3.17.y
>>> ---
>>> drivers/staging/comedi/comedi_fops.c | 15 +++
>>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/staging/comedi/comedi_fops.c
>>> b/drivers/staging/comedi/comedi_fops.c
>>> index 495969f..a9b7fe5 100644
>>> --- a/drivers/staging/comedi/comedi_fops.c
>>> +++ b/drivers/staging/comedi/comedi_fops.c
>>> @@ -1462,10 +1462,6 @@ static int __comedi_get_user_chanlist(struct
>>> comedi_device *dev,
>>> unsigned int *chanlist;
>>> int ret;
>>>
>>> - /* user_chanlist could be NULL for do_cmdtest ioctls */
>>> - if (!user_chanlist)
>>> - return 0;
>>> -
>>
>> I think you need a check here to avoid a NULL pointer getting
>> passed to the memdup_user().
>>
>> If (!user_chanlist || cmd->chanlist_len == 0)
>> return -EINVAL;
>>
>>> chanlist = memdup_user(user_chanlist,
>>>cmd->chanlist_len * sizeof(unsigned int));
>>> if (IS_ERR(chanlist))
>
> It's fine to pass a NULL pointer to memdup_user(). It won't succeed
> (returning ERR_PTR(-EFAULT)), but it's fine.
Ah. OK.
Reviewed-by: H Hartley Sweeten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 3/3] staging: comedi: usbduxfast: updated address details
On Tuesday, October 14, 2014 9:51 AM, Hartley Sweeten wrote: > On Friday, October 10, 2014 12:35 PM, Bernd Porr wrote: >> >> Updated the range of years, e-mail and added driver desription as >> usually done in comedi. >> >> Signed-off-by: Bernd Porr > > Reviewed-by: H Hartley Sweeten Bernd, Oops, this patch has trailing whitespace. Could you fix that please? Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/3] staging: comedi: cb_pcimdas: support PCIe-DAS1602/16
On Monday, October 13, 2014 5:41 AM, Ian Abbott wrote: > Fix up the block comment style and add support for the PCIe-DAS1602/16. > This is the PCI-Express version of the PCIM-DAS1602/16 and is software > compatible with it, apart from a different PCI device ID. > > 1) staging: comedi: cb_pcimdas: use preferred block comment style > 2) staging: comedi: cb_pcimdas: update driver comment > 3) staging: comedi: cb_pcimdas: add support for PCIe-DAS1602/16 > > drivers/staging/comedi/Kconfig | 4 +- > drivers/staging/comedi/drivers/cb_pcimdas.c | 82 > - > 2 files changed, 46 insertions(+), 40 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: drivers: comedi_bond.c: Changed from using strncat to strlcat
On Sunday, October 12, 2014 5:23 AM, Rickard Strandqvist wrote:
>
> Changed from using strncat to strlcat to simplify the code
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/comedi/drivers/comedi_bond.c |6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/comedi_bond.c
> b/drivers/staging/comedi/drivers/comedi_bond.c
> index 8450c99..5d19861 100644
> --- a/drivers/staging/comedi/drivers/comedi_bond.c
> +++ b/drivers/staging/comedi/drivers/comedi_bond.c
> @@ -262,12 +262,10 @@ static int do_dev_config(struct comedi_device *dev,
> struct comedi_devconfig *it)
> {
> /* Append dev:subdev to devpriv->name */
> char buf[20];
> - int left =
> - MAX_BOARD_NAME - strlen(devpriv->name) - 1;
> snprintf(buf, sizeof(buf), "%u:%u ",
>bdev->minor, bdev->subdev);
> - buf[sizeof(buf) - 1] = 0;
>- strncat(devpriv->name, buf, left);
> + strlcat(devpriv->name, buf,
> + sizeof(devpriv->name));
> }
>
> }
Reviewed-by: H Hartley Sweeten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] drivers/staging/comedi/Kconfig: Let COMEDI_II_PCI20KC depend on HAS_IOMEM
On Thursday, October 02, 2014 7:41 AM, Chen Gang wrote:
> COMEDI_II_PCI20KC needs HAS_IOMEM, so depend on it. The related error (
> with allmodconfig under um):
>
> CC [M] drivers/staging/comedi/drivers/ii_pci20kc.o
> drivers/staging/comedi/drivers/ii_pci20kc.c: In function ‘ii20k_attach’:
> drivers/staging/comedi/drivers/ii_pci20kc.c:442:2: error: implicit
> declaration of function ‘ioremap’ [-Werror=implicit-function-declaration]
> dev->mmio = ioremap(membase, II20K_SIZE);
> ^
> drivers/staging/comedi/drivers/ii_pci20kc.c:442:12: warning: assignment
> makes pointer from integer without a cast [enabled by default]
> dev->mmio = ioremap(membase, II20K_SIZE);
> ^
> drivers/staging/comedi/drivers/ii_pci20kc.c: In function ‘ii20k_detach’:
> drivers/staging/comedi/drivers/ii_pci20kc.c:512:3: error: implicit
> declaration of function ‘iounmap’ [-Werror=implicit-function-declaration]
> iounmap(dev->mmio);
> ^
>
> Signed-off-by: Chen Gang
> ---
> drivers/staging/comedi/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/staging/comedi/Kconfig b/drivers/staging/comedi/Kconfig
> index a8bc2b5..b709736 100644
> --- a/drivers/staging/comedi/Kconfig
> +++ b/drivers/staging/comedi/Kconfig
> @@ -426,6 +426,7 @@ config COMEDI_AIO_IIRO_16
>
> config COMEDI_II_PCI20KC
> tristate "Intelligent Instruments PCI-20001C carrier support"
> + depends on HAS_IOMEM
> ---help---
> Enable support for Intelligent Instruments PCI-20001C carrier
> PCI-20001, PCI-20006 and PCI-20341
This seems appropriate.
Reviewed-by: H Hartley Sweeten
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH] drivers/staging/comedi/Kconfig: Let COMEDI_II_PCI20KC depend on HAS_IOMEM
On Thursday, October 02, 2014 10:26 AM, Rostislav Lisovy wrote: > > Since "egrep -irn "ioremap" drivers/staging/comedi/drivers | wc -l" > shows "39" does it mean that this should be added to all the other > drivers as well? The others are actually pci_ioremap_bar() and those drivers depend on PCI. I don't _think_ these drivers require the depends on HAS_IOMEM. The ii_pci20kc driver is goofy in that it's actually an ISA board that uses memory mapped I/O. Regards, Hartley
RE: [PATCH 0/6] staging: comedi: usbduxsigma: fix some problems in command handling
On Thursday, July 23, 2015 8:47 AM, Ian Abbott wrote: > > Fix some minor problems in the testing of asynchronous commands for the AI > and AO subdevices and remove some redundant code. > > The main problem is that the testing of a new command can affect the > operation of an already running command, which it isn't supposed to do. (In > practice, applications don't tend to test new commands while a command is > running on the same subdevice, so the bug can be classed as minor.) This is > corrected by the patches 1 and 2, for the AI and AO subdevices, > respectively. > > 1) staging: comedi: usbduxsigma: don't clobber ai_timer in command test > 2) staging: comedi: usbduxsigma: don't clobber ao_timer in command test > 3) staging: comedi: usbduxsigma: remove AI scan_begin_src == TRIG_FOLLOW > 4) staging: comedi: usbduxsigma: round down AI scan_begin_arg at step 4. > 5) staging: comedi: usbduxsigma: remove unused "convert" timing for AO > 6) staging: comedi: usbduxsigma: round down AO scan_begin_arg at step 4. > > drivers/staging/comedi/drivers/usbduxsigma.c | 139 > +++-------- > 1 file changed, 54 insertions(+), 85 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: improve comedi_check_chanlist() documentation
On Wednesday, August 05, 2015 10:13 AM, Ian Abbott wrote:
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/range.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/range.c b/drivers/staging/comedi/range.c
> index 6a393b2..ce3a58a 100644
> --- a/drivers/staging/comedi/range.c
> +++ b/drivers/staging/comedi/range.c
> @@ -102,7 +102,18 @@ int do_rangeinfo_ioctl(struct comedi_device *dev,
> * @s: comedi_subdevice struct
> * @n: number of elements in the chanlist
> * @chanlist: the chanlist to validate
> -*/
> + *
> + * Each element consists of a channel number, a range index, an analog
> + * reference type and some flags, all packed into an unsigned int.
> + *
> + * This checks that the channel number and range index are supported by
> + * the comedi subdevice. It does not check whether the analog reference
> + * type and the flags are supported. Drivers that care should check those
> + * themselves.
> + *
> + * Return: %0 if all @chanlist elements are valid (success),
> + * %-EINVAL if one or more elements are invalid.
> + */
> int comedi_check_chanlist(struct comedi_subdevice *s, int n,
> unsigned int *chanlist)
> {
Thanks,
Reviewed-by: H Hartley Sweeten
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH V2] Watchdog: Fix parent of watchdog_devices
On Tuesday, August 18, 2015 9:34 AM, Pratyush Anand wrote: > > /sys/class/watchdog/watchdogn/device/modalias can help to identify the > driver/module for a given watchdog node. However, many wdt devices does not > set > its parent and so, we do not see an entry for device in sysfs for such > devices. > > This patch fixes parent of watchdog_device so that > /sys/class/watchdog/watchdogn/device is populated. > > Exceptions: booke, diag288, mpc8xxx, octeon, softdog and w83627hf -- They do > not > have any parent. Not sure, how we can we identify driver for these devices. > > Signed-off-by: Pratyush Anand > --- [snip] drivers/watchdog/ep93xx_wdt.c | 1 + [snip] diff --git a/drivers/watchdog/ep93xx_wdt.c b/drivers/watchdog/ep93xx_wdt.c index 7a2cc7191c58..0a4d7cc05d54 100644 --- a/drivers/watchdog/ep93xx_wdt.c +++ b/drivers/watchdog/ep93xx_wdt.c @@ -132,6 +132,7 @@ static int ep93xx_wdt_probe(struct platform_device *pdev) val = readl(mmio_base + EP93XX_WATCHDOG); ep93xx_wdt_wdd.bootstatus = (val & 0x01) ? WDIOF_CARDRESET : 0; ep93xx_wdt_wdd.timeout = timeout; + ep93xx_wdt_wdd.parent = &pdev->dev; watchdog_set_nowayout(&ep93xx_wdt_wdd, nowayout); For ep93xx_wdt.c, use whichever tag you prefer: Reviewed-by: H Hartley Sweeten Acked-by: H Hartley Sweeten Thanks, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 03/20] staging: comedi: drivers: rename PLX PCI 9080 register offsets
On Friday, May 20, 2016 6:49 AM, Ian Abbott wrote: > Rename the macros in "plx9080.h" that define the offsets of registers, > following the pattern `PLX_REG_`, where `` is the register > name from the PLX PCI 9080 Data Book. > > Add defines for the "Mailbox" registers, and add parameterized macros > for the mailbox registers and the DMA control registers. Make use of > the parameterized versions of the macros where it seems appropriate. > > The registers for supporting the I2O (Intelligent Input/Output) feature > are largely left undefined, just defining enough to allow the I2O > feature to be disabled. > > Signed-off-by: Ian Abbott Ian, Just an comment on your renaming. You also renamed the registers in the daqbook2000 driver in the _REG_ format. Personally I prefer the format to be __REG. That way a grep for _ will return both the register and bit uses. Currently the _REG_ form is not as common in comedi. Looks like it's only used in amcc_35933.h and will be in daqboard2000.c. The __REG form is used a lot. Not a big deal, just a comment. Hartley
RE: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values
On Friday, May 20, 2016 6:49 AM, Ian Abbott wrote:
> Rename the macros for the PLX PCI 9080 LAS0RR and LAS1RR registers in
> "plx9080.h", using the prefix `PLX_LASRR_`. Make use of the `BIT(x)`
> and `GENMASK(h,l)` macros to define the values.
>
> Define a macro `PLX_LASRR_PREFETCH` for the "prefetchable memory" bit in
> this register, and define a macro `PLX_LASRR_MLOC_MASK` to mask the PCI
> memory location control bits.
>
> Signed-off-by: Ian Abbott
> ---
[snip]
> diff --git a/drivers/staging/comedi/drivers/plx9080.h
> b/drivers/staging/comedi/drivers/plx9080.h
> index 92d2480..8788117 100644
> --- a/drivers/staging/comedi/drivers/plx9080.h
> +++ b/drivers/staging/comedi/drivers/plx9080.h
> @@ -54,14 +54,16 @@ struct plx_dma_desc {
> /* Local Address Space 1 Range Register */
> #define PLX_REG_LAS1RR 0x00f0
>
> -#define LRNG_IO 0x0001/* Map to: 1=I/O, 0=Mem */
> -#define LRNG_ANY320x/* Locate anywhere in 32 bit */
> -#define LRNG_LT1MB0x0002/* Locate in 1st meg */
> -#define LRNG_ANY640x0004/* Locate anywhere in 64 bit */
> -/* bits that specify range for memory io */
> -#define LRNG_MEM_MASK 0xfff0
> -/* bits that specify range for normal io */
> -#define LRNG_IO_MASK 0xfffc
> +#define PLX_LASRR_IO BIT(0) /* Map to: 1=I/O, 0=Mem */
> +#define PLX_LASRR_ANY32 (BIT(1) * 0)/* Locate anywhere in
> 32 bit */
> +#define PLX_LASRR_LT1MB (BIT(1) * 1)/* Locate in 1st meg */
> +#define PLX_LASRR_ANY64 (BIT(1) * 2)/* Locate anywhere in
> 64 bit */
The (BIT(n) * x) looks ugly.
These bit define the memory space encoding. I would prefer something
like this:
#define PLX_LASSR_MLOC(x) (((x) & 0x3) << 1)
#define PLX_LASSR_MLOC_ANY32PLX_LASSR_MLOC(0)
#define PLX_LASSR_MLOC_LT1MBPLX_LASSR_MLOC(1)
#define PLX_LASSR_MLOC_ANY64PLX_LASSR_MLOC(2)
> +#define PLX_LASRR_MLOC_MASK GENMASK(2, 1) /* Memory location bits */
I guess the GENMASK() macro is common but it's currently
not used by any of the comedi code.
Using the macro above, the 'mask' would be:
#define PLX_LASSR_MLOC_MASK PLX_LASSR_MLOC(3)
> +#define PLX_LASRR_PREFETCH BIT(3) /* Memory is prefetchable */
> +/* bits that specify range for memory space decode bits */
> +#define PLX_LASRR_MEM_MASK GENMASK(31, 4)
> +/* bits that specify range for i/o space decode bits */
> +#define PLX_LASRR_IO_MASKGENMASK(31, 2)
I suppose the GENMASK() use makes sense for these.
Regards,
Hartley
RE: [PATCH 03/20] staging: comedi: drivers: rename PLX PCI 9080 register offsets
On Friday, May 20, 2016 9:31 AM, Ian Abbott wrote: > On 20/05/16 17:21, Hartley Sweeten wrote: >> Just an comment on your renaming. >> >> You also renamed the registers in the daqbook2000 driver in the >> _REG_ format. Personally I prefer the format to be >> __REG. That way a grep for _ will >> return both the register and bit uses. > > It makes it easier to distinguish the register offsets from the register > values, imho. I guess. It's just not as common in comedi right now and it does make grep'ing for all the register/bit uses a bit more cumbersome. Regards, Hartley
RE: [PATCH 04/20] staging: comedi: drivers: re-do macros for PLX PCI 9080 LASxRR values
On Friday, May 20, 2016 10:18 AM, Ian Abbott wrote: > On 20/05/16 17:37, Hartley Sweeten wrote: >> On Friday, May 20, 2016 6:49 AM, Ian Abbott wrote: >>> Rename the macros for the PLX PCI 9080 LAS0RR and LAS1RR registers in >>> "plx9080.h", using the prefix `PLX_LASRR_`. Make use of the `BIT(x)` >>> and `GENMASK(h,l)` macros to define the values. >>> >>> Define a macro `PLX_LASRR_PREFETCH` for the "prefetchable memory" bit in >>> this register, and define a macro `PLX_LASRR_MLOC_MASK` to mask the PCI >>> memory location control bits. [snip] >>> +#define PLX_LASRR_IO BIT(0) /* Map to: 1=I/O, 0=Mem >>> */ >>> +#define PLX_LASRR_ANY32(BIT(1) * 0)/* Locate anywhere in >>> 32 bit */ >>> +#define PLX_LASRR_LT1MB(BIT(1) * 1)/* Locate in 1st meg */ >>> +#define PLX_LASRR_ANY64(BIT(1) * 2)/* Locate anywhere in >>> 64 bit */ >> >> The (BIT(n) * x) looks ugly. > > You won't like the remaining patches then! You are correct... ;-) > FWIW, all the constants end up with the same type (unsigned long) this way. That's probably good but it sure makes the defines look ugly, and a bit hard to understand imoh. You also don't know what the 'max' value for the bit-field is without further digging. I applied your whole series to see what the final header looks like. To me it actually looks worse than the original. The original had a number of whitespace issues that made it hard to follow and the defines were lacking namespace. Personally I also don't can for all the enums since the symbols are not actually used as enums just as raw values. But the 'bit' usage of the registers was fairly clear. With your series applied the whtespace and namespace issues are addressed. You also converted all the enums to defines which is great. But the 'bit' usage now is a bit muddled. I really don't care for the (BIT(n) * (x)) stuff. There are also the various, unused and unnecessary, _SHIFT defines. Those just add additional cruft. I'm also not sure if all the bits require a comment. They seem to clutter the header. Datasheets for the PLX-9080 are easy to find. Maybe just have a comment for each register and remove all the bit comments. > I have been looking for a solution to the problem where random people > change something like this: > > #define MY_COOLREG_VAL_FOO(0 << 5) > #define MY_COOLREG_VAL_BAR(1 << 5) > #define MY_COOLREG_VAL_BAZ(2 << 5) > > to: > > #define MY_COOLREG_VAL_FOO(0 << 5) > #define MY_COOLREG_VAL_BARBIT(5) > #define MY_COOLREG_VAL_BAZ(2 << 5) > > and this seemed like one way to do it. Like I stated previously, I prefer something like this for the multi-bit fields of a register. >> #define PLX_LASSR_MLOC(x)(((x) & 0x3) << 1) >> #define PLX_LASSR_MLOC_ANY32 PLX_LASSR_MLOC(0) >> #define PLX_LASSR_MLOC_LT1MB PLX_LASSR_MLOC(1) >> #define PLX_LASSR_MLOC_ANY64 PLX_LASSR_MLOC(2) >> #define PLX_LASSR_MLOC_MASK PLX_LASSR_MLOC(3) > > It is handy when matching it up with the data sheet though. I have a > field that occupies bits 2 and 1. It also doesn't expose a fairly > useless PLX_LASRR_MLOC() macro to the user of the header file. The (BIT(n) * (x)) just looks odd. The GENMASK() for a multi-bit field also makes it more difficult to figure out what the maximum value for the field is when there are more than just a few bits and the lower bit is not 0. Anyway.. Technically it looks like your series doesn't break anything I just don't feel that it adds much clarity. I'm still looking it over... Maybe I'll change my mind... ;-) Regards, Hartley
RE: [PATCH 05/14] staging: comedi: daqboard2000: rename serial EEPROM register macros
On Tuesday, May 17, 2016 2:53 AM, Ian Abbott wrote:
> Rename the macros defining values for the Serial EEPROM Control Register
> to avoid CamelCase.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/drivers/daqboard2000.c | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/daqboard2000.c
> b/drivers/staging/comedi/drivers/daqboard2000.c
> index fde0924..b3b68e8 100644
> --- a/drivers/staging/comedi/drivers/daqboard2000.c
> +++ b/drivers/staging/comedi/drivers/daqboard2000.c
> @@ -116,12 +116,12 @@
> #define DAQBOARD2000_SUBSYSTEM_IDS4 0x0004 /* Daqboard/2000 - 4 Dacs */
Ian,
This board uses a PLX-9080 chip for the PCI interface.
If would be better to include and use the register/bit defines
to remove the magic numbers.
The only PLX register used is PLX_CONTROL_REG (0x6c).
> /* Initialization bits for the Serial EEPROM Control Register */
> -#define DAQBOARD2000_SECRProgPinHi 0x8001767e
> -#define DAQBOARD2000_SECRProgPinLo 0x8000767e
> -#define DAQBOARD2000_SECRLocalBusHi 0xc000767e
> -#define DAQBOARD2000_SECRLocalBusLo 0x8000767e
> -#define DAQBOARD2000_SECRReloadHi 0xa000767e
> -#define DAQBOARD2000_SECRReloadLo 0x8000767e
> +#define DAQBOARD2000_SECR_PROG_PIN_HI0x8001767e
> +#define DAQBOARD2000_SECR_PROG_PIN_LO0x8000767e
> +#define DAQBOARD2000_SECR_LOCAL_BUS_HI 0xc000767e
> +#define DAQBOARD2000_SECR_LOCAL_BUS_LO 0x8000767e
> +#define DAQBOARD2000_SECR_RELOAD_HI 0xa000767e
> +#define DAQBOARD2000_SECR_RELOAD_LO 0x8000767e
These "Initialization bits" are just various combinations of the
PLX_CONTROL_REG bit defines (CTL_*) to toggle the various
EEPROM bits.
> /* SECR status bits */
> #define DAQBOARD2000_EEPROM_PRESENT 0x1000
> @@ -438,9 +438,9 @@ static void daqboard2000_resetLocalBus(struct
> comedi_device *dev)
> {
> struct daqboard2000_private *devpriv = dev->private;
>
> - writel(DAQBOARD2000_SECRLocalBusHi, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_LOCAL_BUS_HI, devpriv->plx + 0x6c);
> mdelay(10);
> - writel(DAQBOARD2000_SECRLocalBusLo, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_LOCAL_BUS_LO, devpriv->plx + 0x6c);
> mdelay(10);
> }
>
> @@ -448,11 +448,11 @@ static void daqboard2000_reloadPLX(struct comedi_device
> *dev)
> {
> struct daqboard2000_private *devpriv = dev->private;
>
> - writel(DAQBOARD2000_SECRReloadLo, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_RELOAD_LO, devpriv->plx + 0x6c);
> mdelay(10);
> - writel(DAQBOARD2000_SECRReloadHi, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_RELOAD_HI, devpriv->plx + 0x6c);
> mdelay(10);
> - writel(DAQBOARD2000_SECRReloadLo, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_RELOAD_LO, devpriv->plx + 0x6c);
> mdelay(10);
> }
>
> @@ -460,9 +460,9 @@ static void daqboard2000_pulseProgPin(struct
> comedi_device *dev)
> {
> struct daqboard2000_private *devpriv = dev->private;
>
> - writel(DAQBOARD2000_SECRProgPinHi, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_PROG_PIN_HI, devpriv->plx + 0x6c);
> mdelay(10);
> - writel(DAQBOARD2000_SECRProgPinLo, devpriv->plx + 0x6c);
> + writel(DAQBOARD2000_SECR_PROG_PIN_LO, devpriv->plx + 0x6c);
> mdelay(10); /* Not in the original code, but I like symmetry... */
> }
Regards,
Hartley
RE: [PATCH 01/14] staging: comedi: daqboard2000: remove commented out code
On Tuesday, May 17, 2016 2:53 AM, Ian Abbott wrote:
> Remove some commented out code. Some of it uses constructs that don't
> exist in the driver, and probably come from the source code for the MS
> Windows driver.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/drivers/daqboard2000.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/daqboard2000.c
> b/drivers/staging/comedi/drivers/daqboard2000.c
> index 57ab668..8c9a024 100644
> --- a/drivers/staging/comedi/drivers/daqboard2000.c
> +++ b/drivers/staging/comedi/drivers/daqboard2000.c
> @@ -278,9 +278,7 @@ struct daqboard2000_private {
>
> static void writeAcqScanListEntry(struct comedi_device *dev, u16 entry)
> {
> - /* udelay(4); */
> writew(entry & 0x00ff, dev->mmio + acqScanListFIFO);
> - /* udelay(4); */
> writew((entry >> 8) & 0x00ff, dev->mmio + acqScanListFIFO);
> }
>
> @@ -315,10 +313,6 @@ static void setup_sampling(struct comedi_device *dev,
> int chan, int gain)
> word3 = 0;
> break;
> }
> -/*
> - dev->eeprom.correctionDACSE[i][j][k].offset = 0x800;
> - dev->eeprom.correctionDACSE[i][j][k].gain = 0xc00;
> -*/
> /* These should be read from EEPROM */
> word2 |= 0x0800;
> word3 |= 0xc000;
It might be a good idea to add a comment about the magic 0x0800 and 0xc000
values:
> word2 |= 0x0800;/* offset */
> word3 |= 0xc000;/* gain */
Wish I could find a register map for this board. I sent an email to Measurement
Computing
to see if one is available.
Regards,
Hartley
RE: [PATCH 06/14] staging: comedi: daqboard2000: rename register offset macros
On Tuesday, May 17, 2016 2:53 AM, Ian Abbott wrote:
> Rename the macros defining register offsets to avoid CamelCase, and to
> use namespace associated with the driver.
>
> Signed-off-by: Ian Abbott
> ---
> Other CamelCase issues in this patch will be dealt with by later
> patches in the series.
> ---
> drivers/staging/comedi/drivers/daqboard2000.c | 112
> ++
> 1 file changed, 61 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/daqboard2000.c
> b/drivers/staging/comedi/drivers/daqboard2000.c
> index b3b68e8..92ff8f4 100644
> --- a/drivers/staging/comedi/drivers/daqboard2000.c
> +++ b/drivers/staging/comedi/drivers/daqboard2000.c
> @@ -151,35 +151,35 @@ static const struct comedi_lrange range_daqboard2000_ai
> = {
> /*
> * Register Memory Map
> */
> -#define acqControl 0x00/* u16 */
> -#define acqScanListFIFO 0x02/* u16 */
> -#define acqPacerClockDivLow 0x04/* u32 */
> -#define acqScanCounter 0x08/* u16 */
> -#define acqPacerClockDivHigh 0x0a/* u16 */
> -#define acqTriggerCount 0x0c/* u16 */
> -#define acqResultsFIFO 0x10/* u16 */
> -#define acqResultsShadow 0x14/* u16 */
> -#define acqAdcResult 0x18/* u16 */
> -#define dacScanCounter 0x1c/* u16 */
> -#define dacControl 0x20/* u16 */
> -#define dacFIFO 0x24/* s16 */
> -#define dacPacerClockDiv 0x2a/* u16 */
> -#define refDacs 0x2c/* u16 */
> -#define dioControl 0x30/* u16 */
> -#define dioP3hsioData0x32/* s16 */
> -#define dioP3Control 0x34/* u16 */
> -#define calEepromControl 0x36/* u16 */
> -#define dacSetting(x)(0x38 + (x) * 2) /* s16 */
> -#define dioP2ExpansionIO8Bit 0x40/* s16 */
> -#define ctrTmrControl0x80/* u16 */
> -#define ctrInput(x) (0x88 + (x) * 2) /* s16 */
> -#define timerDivisor(x) (0xa0 + (x) * 2) /* u16 */
> -#define dmaControl 0xb0/* u16 */
> -#define trigControl 0xb2/* u16 */
> -#define calEeprom0xb8/* u16 */
> -#define acqDigitalMark 0xba/* u16 */
> -#define trigDacs 0xbc/* u16 */
> -#define dioP2ExpansionIO16Bit(x) (0xc0 + (x) * 2) /* s16 */
> +#define DAQBOARD2000_REG_ACQ_CONTROL 0x00 /* u16 */
> +#define DAQBOARD2000_REG_ACQ_SCAN_LIST_FIFO 0x02 /* u16 */
> +#define DAQBOARD2000_REG_ACQ_PACER_CLOCK_DIV_LOW 0x04 /* u32 */
> +#define DAQBOARD2000_REG_ACQ_SCAN_COUNTER0x08 /* u16 */
> +#define DAQBOARD2000_REG_ACQ_PACER_CLOCK_DIV_HIGH0x0a /* u16 */
> +#define DAQBOARD2000_REG_ACQ_TRIGGER_COUNT 0x0c /* u16 */
> +#define DAQBOARD2000_REG_ACQ_RESULTS_FIFO0x10 /* u16 */
> +#define DAQBOARD2000_REG_ACQ_RESULTS_SHADOW 0x14 /* u16 */
> +#define DAQBOARD2000_REG_ACQ_ADC_RESULT 0x18 /* u16 */
> +#define DAQBOARD2000_REG_DAC_SCAN_COUNTER0x1c /* u16 */
> +#define DAQBOARD2000_REG_DAC_CONTROL 0x20 /* u16 */
> +#define DAQBOARD2000_REG_DAC_FIFO0x24 /* s16 */
> +#define DAQBOARD2000_REG_DAC_PACER_CLOCK_DIV 0x2a /* u16 */
> +#define DAQBOARD2000_REG_REF_DACS0x2c /* u16 */
> +#define DAQBOARD2000_REG_DIO_CONTROL 0x30 /* u16 */
> +#define DAQBOARD2000_REG_P3_HSIO_DATA0x32 /* s16 */
> +#define DAQBOARD2000_REG_P3_CONTROL 0x34 /* u16 */
> +#define DAQBOARD2000_REG_CAL_EEPROM_CONTROL 0x36 /* u16 */
> +#define DAQBOARD2000_REG_DAC_SETTING(x) (0x38 + (x) * 2) /* s16
> */
> +#define DAQBOARD2000_REG_DIO_P2_EXP_IO_8_BIT 0x40 /* s16 */
> +#define DAQBOARD2000_REG_COUNTER_TIMER_CONTROL 0x80 /* u16 */
> +#define DAQBOARD2000_REG_COUNTER_INPUT(x)(0x88 + (x) * 2) /* s16 */
> +#define DAQBOARD2000_REG_TIMER_DIV(x)(0xa0 + (x) * 2) /* u16
> */
> +#define DAQBOARD2000_REG_DMA_CONTROL 0xb0 /* u16 */
> +#define DAQBOARD2000_REG_TRIG_CONTROL0xb2 /* u16 */
> +#define DAQBOARD2000_REG_CAL_EEPROM 0xb8 /* u16 */
> +#define DAQBOARD2000_REG_ACQ_DIGITAL_MARK0xba /* u16 */
> +#define DAQBOARD2000_REG_TRIG_DACS 0xbc /* u16 */
> +#define DAQBOARD2000_REG_DIO_P2_EXP_IO_16_BIT(x) (0xc0 + (x) * 2) /* s16 */
Any chance we can shorten the
RE: [PATCH 14/14] staging: comedi: daqboard2000: prefer usleep_range()
On Tuesday, May 17, 2016 2:53 AM, Ian Abbott wrote: > The checkpatch.pl warns about two `udelay(x)` calls, one of 100 > microseconds, and one of 10 microseconds. The 100 microseconds one is > used when waiting for FPGA to become ready to accept firmware, and is > not that critical, so replace it with a call to `usleep_range(100, > 1000)`. The 10 microseconds one is called as each 16-bit word of > firmware data is written. A longer sleep would slow down firmware > loading, so leave it alone. The firmware blob in comedi-nonfree-firmware/daqboard2000 is 41236 bytes or 20618 words. With the 10 microsecond delay for each word to total delay time is only 0.0206 seconds. I don't think a small usleep_range() would slow down the firmware loading by much. How about usleep_range(10, 20)? Regards, Hartley
RE: [PATCH 00/14] staging: comedi: daqboard2000: checkpatch clean-ups
On Tuesday, May 17, 2016 2:53 AM, Ian Abbott wrote: > This series of patches to the daqboard2000 driver is mostly to fix the > checkpatch.pl warnings. There is one warning remaining about one of the > `udelay` calls with a parameter of 10 microseconds, but I decided to > leave it alone, as converting it to `usleep_range` could increase > firmware loading time. > > Patches 03 and 06 have checkpatch warnings themselves about CamelCase > issues, but they are not "new" issues, and are resolved by the later > patches in the series. > > 01) staging: comedi: daqboard2000: remove commented out code > 02) staging: comedi: daqboard2000: use usual block comment style > 03) staging: comedi: daqboard2000: CHECK: spaces preferred around that > '*' > 04) staging: comedi: daqboard2000: add blank line after struct > declaration > 05) staging: comedi: daqboard2000: rename serial EEPROM register macros > 06) staging: comedi: daqboard2000: rename register offset macros > 07) staging: comedi: daqboard2000: rename acquisition control register > macros > 08) staging: comedi: daqboard2000: rename acq status register macros > 09) staging: comedi: daqboard2000: redo DAC control register macros > 10) staging: comedi: daqboard2000: redo DAC status macros and fix busy > 11) staging: comedi: daqboard2000: rename trigger control register > macros > 12) staging: comedi: daqboard2000: rename reference DACs register macros > 13) staging: comedi: daqboard2000: rename CamelCase functions > 14) staging: comedi: daqboard2000: prefer usleep_range() > > drivers/staging/comedi/drivers/daqboard2000.c | 376 > +- > 1 file changed, 188 insertions(+), 188 deletions(-) Ian, I had a couple comments on patches 1, 5, 6, and 14 but they are really just nitpicks. If you prefer to leave this series as-is: Reviewed-by: H Hartley Sweeten Thanks for going through this one, it's annoyed me... Hartley
RE: [PATCH v2 06/14] staging: comedi: daqboard2000: rename register offset macros
On Wednesday, May 18, 2016 5:37 AM, Ian Abbott wrote: > Rename the macros defining register offsets to avoid CamelCase, and to > use namespace associated with the driver. > > Signed-off-by: Ian Abbott > Reviewed-by: H Hartley Sweeten > --- > Other CamelCase issues in this patch will be dealt with by later > patches in the series. > > v2: Shortened prefix from `DAQBOARD2000_` to `DB2K_`. > --- > drivers/staging/comedi/drivers/daqboard2000.c | 112 > ++ > 1 file changed, 61 insertions(+), 51 deletions(-) [snip] +#define DB2K_REG_DIO_P2_EXP_IO_16_BIT(x) (0xc0 + (x) * 2) /* s16 */ You slipped an extra space in here: WARNING: please, no space before tabs #184: FILE: drivers/staging/comedi/drivers/daqboard2000.c:184: +#define DB2K_REG_DIO_P2_EXP_IO_16_BIT(x) ^I(0xc0 + (x) * 2) /* s16 */$ Regards, Hartley
RE: [PATCH v2 12/14] staging: comedi: daqboard2000: rename reference DACs register macros
On Wednesday, May 18, 2016 5:37 AM, Ian Abbott wrote: > Rename the macros that define values for the reference DACs register to > avoid CamelCase, and to make it clearer which register they are > associated with. Add a macro `DAQBOARD2000_REF_DACS_SET` for the value > `0x80` that triggers setting one of the references. [snip] +#define DB2K_REF_DACS_SET 0x0080 Minor issue... Typo in the commit. Not sure if it's worth a v3. Regards, Hartley
RE: [PATCH v3 06/14] staging: comedi: daqboard2000: rename register offset macros
On Thursday, May 19, 2016 2:56 AM, Ian Abbott wrote: > Rename the macros defining register offsets to avoid CamelCase, and to > use namespace associated with the driver. > > Signed-off-by: Ian Abbott > Reviewed-by: H Hartley Sweeten > --- > Other CamelCase issues in this patch will be dealt with by later > patches in the series. > > v2: Shortened prefix from `DAQBOARD2000_` to `DB2K_`. > v3: Removed a space before tab that slipped in in v2. Ian, Your [PATCH v2 07/14: ...] does not apply after this one. It's probably better if Greg drops the previous patches and you post a full series. Regards, Hartley
RE: [PATCH 1/2] comedi: dmm32at: Fix coding style - use BIT macro
On Wednesday, November 18, 2015 9:42 AM, Ian Abbott wrote: > On 16/11/15 17:18, Ranjith Thangavel wrote: [snip] >> -#define DMM32AT_AI_CFG_SCINT_20US (0 << 4) >> -#define DMM32AT_AI_CFG_SCINT_15US (1 << 4) >> -#define DMM32AT_AI_CFG_SCINT_10US (2 << 4) >> -#define DMM32AT_AI_CFG_SCINT_5US(3 << 4) >> -#define DMM32AT_AI_CFG_RANGE(1 << 3) /* 0=5V 1=10V */ >> -#define DMM32AT_AI_CFG_ADBU (1 << 2) /* 0=bipolar 1=unipolar */ >> +#define DMM32AT_AI_CFG_SCINT_20US 0 >> +#define DMM32AT_AI_CFG_SCINT_15US BIT(4) >> +#define DMM32AT_AI_CFG_SCINT_10US (BIT(5) & ~BIT(4)) > > The `(BIT(5) & ~BIT(4))` is a bit ugly. You can just use `BIT(5)` to > fit in with the style of your other changes. > > (Personally though, I don't think BIT() is appropriate for shifted, > multi-bit values.) It would be more appropriate as a macro: #define DMM32AT_AI_CFG_SCINT(x) (((x) & 0x3) << 4) #define DMM32AT_AI_CFG_SCINT_20US DMM32AT_AI_CFG_SCINT (0) #define DMM32AT_AI_CFG_SCINT_15US DMM32AT_AI_CFG_SCINT (1) #define DMM32AT_AI_CFG_SCINT_10US DMM32AT_AI_CFG_SCINT (2) #define DMM32AT_AI_CFG_SCINT_5USDMM32AT_AI_CFG_SCINT (3) Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/8] staging: comedi: rearrange comedi_write() code
On Wednesday, November 18, 2015 10:55 AM, Ian Abbott wrote:
> Rearrange the code in `comedi_write()` to reduce the amount of
> indentation. The code never reiterates the `while` loop once `count`
> has become non-zero, so we can check that in the `while` condition to
> save an indentation level. (Note that `nbytes` has been checked to be
> non-zero before entering the loop, so we can remove that check.) Move
> the code that makes the subdevice "become non-busy" outside the `while`
> loop, using a new flag variable `become_nonbusy` to decide whether it
> needs to be done. This simplifies the wait queue handling so there is a
> single place where the task is removed from the wait queue, and we can
> remove the `on_wait_queue` flag variable.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/comedi_fops.c | 71
> +++-
> 1 file changed, 30 insertions(+), 41 deletions(-)
Ian,
Minor nit-pick...
[snip]
> -out:
> - if (on_wait_queue)
> - remove_wait_queue(&async->wait_head, &wait);
> + remove_wait_queue(&async->wait_head, &wait);
> set_current_state(TASK_RUNNING);
> + if (become_nonbusy && count == 0) {
It looks like 'count' will always be 0 here.
Regards
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/2] staging: comedi: s526: add counter register macros
On Thursday, November 19, 2015 7:49 AM, Ian Abbott wrote: > Add macros to describe the counter mode and counter control/status > registers. In patch 1, the macros for the counter mode register replace > the use of bitfield structure and union types. > > 1) staging: comedi: s526: replace counter mode bitfield struct > 2) staging: comedi: s526: add macros for counter control reg values > > drivers/staging/comedi/drivers/s526.c | 197 > ++ > 1 file changed, 130 insertions(+), 67 deletions(-) Thanks! This one has bugged me. Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/8] staging: comedi: some comedi_write() changes
On Wednesday, November 18, 2015 10:55 AM, Ian Abbott wrote: > Tidy up the "write" file operation handler, `comedi_write()` a bit and > improve the error handling. > > 1) staging: comedi: rearrange comedi_write() code > 2) staging: comedi: do extra checks for becoming non-busy for "write" > 3) staging: comedi: make some variables unsigned in comedi_write() > 4) staging: comedi: avoid bad truncation of a size_t in comedi_write() > 5) staging: comedi: allow buffer wraparound in comedi_write() > 6) staging: comedi: return error on "write" if no command set up > 7) staging: comedi: simplify returned errors for comedi_write() > 8) staging: comedi: check for more errors for zero-length write > > drivers/staging/comedi/comedi_fops.c | 124 > --- > 1 file changed, 56 insertions(+), 68 deletions(-) Ian, Other than the minor nit-pick in patch 1 about the 'count == 0' when becoming non-busy (the same situation is in comedi_read), this looks good to me. It also makes the 'write' look more like the 'read'. Thanks, Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/16] staging: comedi: comedi_test: enhancements
On Tuesday, October 27, 2015 9:59 AM, Ian Abbott wrote: > The "comedi_test" module is a driver for a dummy COMEDI device. It has > an analog input subdevice and an analog output subdevice. The analog > input subdevice supports COMEDI asynchronous acquisition commands using > waveform generators to generate the input data for each channel. A > kernel timer is used to driver the acquisition. > > This series of patches cleans up the driver, enhances the existing > asynchronous command support on the analog input subdevice, and adds > asynchronous command support on the analog output subdevice. > > 01) staging: comedi: comedi_test: reformat multi-line comments > 02) staging: comedi: comedi_test: saturate fake waveform values > 03) staging: comedi: comedi_test: remove nano_per_micro > 04) staging: comedi: comedi_test: limit maximum convert_arg > 05) staging: comedi: comedi_test: support scan_begin_src == TRIG_FOLLOW > 06) staging: comedi: comedi_test: move modulo operations for waveform > 07) staging: comedi: comedi_test: use unsigned int for waveform timing > 08) staging: comedi: comedi_test: simplify time since last AI scan > 09) staging: comedi: comedi_test: rename members for AI commands > 10) staging: comedi: comedi_test: rename waveform members > 11) staging: comedi: comedi_test: make timer rate similar to scan rate > 12) staging: comedi: comedi_test: use unsigned short for loopback values > 13) staging: comedi: comedi_test: allow read-back of AO channels > 14) staging: comedi: comedi_test: handle partial scans in timer routine > 15) staging: comedi: comedi_test: rename waveform_ai_interrupt() > 16) staging: comedi: comedi_test: implement commands on AO subdevice > > drivers/staging/comedi/drivers/comedi_test.c | 565 > ++++++++++++--- > 1 file changed, 416 insertions(+), 149 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: fix scan_end_arg == chanlist_len assumption
On Wednesday, November 12, 2014 9:01 AM, Ian Abbott wrote: > Some comedi drivers allow the `scan_end_arg` value of an asynchronous > command to be a multiple (> 1) of the `chanlist_len` although most > require them to be the same value. > > `comedi_bytes_per_scan()` is incorrectly using `chanlist_len` as the > length of the scan. Change it to use `scan_end_arg`. > > `comedi_nsamples_left()` is incorrectly using `cur_chan` as the current > sample position in the scan (it is actually the current position in the > channel list). Change it to use the actual sample position in the scan. > (Unfortunately we only have the current scan position in bytes currently, > so convert that to a sample position.) > > Signed-off-by: Ian Abbott > --- > drivers/staging/comedi/drivers.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: fix memory leak / bad pointer freeing for chanlist
On Monday, October 20, 2014 7:11 AM, Ian Abbott wrote:
> As a follow-up to commit 6cab7a37f5c04 ("staging: comedi: (regression)
> channel list must be set for COMEDI_CMD ioctl"), Hartley Sweeten pointed
> out another couple of bugs stemming from commit 6cab7a37f5c04 ("staging:
> comedi: comedi_fops: introduce __comedi_get_user_chanlist()").
>
> Firstly, `do_cmdtest_ioctl()` never frees the kernel copy of the user
> chanlist allocated by `__comedi_get_user_chanlist()`, so that memory is
> leaked. Fix it by freeing the allocated kernel memory pointed to by
> `cmd.chanlist` before that pointer is overwritten with its original
> pointer to user memory before `cmd` is copied back to user-space.
>
> Secondly, if `__comedi_get_user_chanlist()` returns an error,
> `cmd->chanlist` is left unchanged and in fact will be a pointer to user
> memory. This causes `do_cmd_ioctl()` to `goto cleanup` and call
> `do_become_nonbusy()` which would attempt to free the memory pointed to
> by the user-space pointer. Fix it by setting `cmd->chanlist` to NULL at
> the start of `__comedi_get_user_chanlist()`.
>
> Fixes: c6cd0eefb27b ("staging: comedi: comedi_fops: introduce
> __comedi_get_user_chanlist()")
> Reported-by: H Hartley Sweeten
> Cc: # 3.15.y 3.16.y 3.17.y: 6cab7a37f5c04
> Cc: # 3.15.y 3.16.y 3.17.y
> ---
> Greg, this patch applies to your "staging-linus" branch.
> ---
> drivers/staging/comedi/comedi_fops.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index a9b7fe5..a1788e8 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1462,6 +1462,7 @@ static int __comedi_get_user_chanlist(struct
> comedi_device *dev,
> unsigned int *chanlist;
> int ret;
>
> + cmd->chanlist = NULL;
> chanlist = memdup_user(user_chanlist,
> cmd->chanlist_len * sizeof(unsigned int));
> if (IS_ERR(chanlist))
I don't think this covers all the problems in your second issue above.
It does fix the attempt to free the memory pointed to by the user-space pointer
if the memdub_user() fails.
But in do_cmd_ioctl() we still have an issue if the s->do_cmdtest() fails or
the CMDF_BOGUS flag is set. There we restore the users cmd.chanlist
pointer to allow copy_to_user() to return the cmd/ But the kernel allocated
chanlist is not freed. The goto cleanup will again try to free the user-space
pointer.
The remaining goto cleanup jumps will correctly free the kernel allocated
pointer stored in cmd.chanlist.
> @@ -1615,6 +1616,8 @@ static int do_cmdtest_ioctl(struct comedi_device *dev,
>
> ret = s->do_cmdtest(dev, s, &cmd);
>
> + kfree(cmd.chanlist);/* free kernel copy of user chanlist */
> +
> /* restore chanlist pointer before copying back */
> cmd.chanlist = (unsigned int __force *)user_chanlist;
>
This one does fix the issue in do_cmdtest_ioctl().
Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: (regression) channel list must be set for COMEDI_CMD ioctl
On Wednesday, October 08, 2014 8:09 AM, Ian Abbott wrote:
> `do_cmd_ioctl()`, the handler for the `COMEDI_CMD` ioctl can incorrectly
> call the Comedi subdevice's `do_cmd()` handler with a NULL channel list
> pointer. This is a regression as the `do_cmd()` handler has never been
> expected to deal with that, leading to a kernel OOPS when it tries to
> dereference it.
>
> A NULL channel list pointer is allowed for the `COMEDI_CMDTEST` ioctl,
> handled by `do_cmdtest_ioctl()` and the subdevice's `do_cmdtest()`
> handler, but not for the `COMEDI_CMD` ioctl and its handlers.
>
> Both `do_cmd_ioctl()` and `do_cmdtest_ioctl()` call
> `__comedi_get_user_chanlist()` to copy the channel list from user memory
> into dynamically allocated kernel memory and check it for consistency.
> That function currently returns 0 if the `user_chanlist` parameter
> (pointing to the channel list in user memory) is NULL. That's fine for
> `do_cmdtest_ioctl()`, but `do_cmd_ioctl()` incorrectly assumes the
> kernel copy of the channel list has been set-up correctly.
>
> Fix it by not allowing the `user_chanlist` parameter to be NULL in
> `__comedi_get_user_chanlist()`, and only calling it from
> `do_cmdtest_ioctl()` if the parameter is non-NULL.
>
> Thanks to Bernd Porr for reporting the bug via an initial patch sent
> privately.
>
> Fixes: c6cd0eefb27b ("staging: comedi: comedi_fops: introduce
> __comedi_get_user_chanlist()")
> Reported-by: Bernd Porr
> Signed-off-by: Ian Abbott
> Cc: Bernd Porr
> Cc: # 3.15.y 3.16.y 3.17.y
> ---
> drivers/staging/comedi/comedi_fops.c | 15 +++
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index 495969f..a9b7fe5 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -1462,10 +1462,6 @@ static int __comedi_get_user_chanlist(struct
> comedi_device *dev,
> unsigned int *chanlist;
> int ret;
>
> - /* user_chanlist could be NULL for do_cmdtest ioctls */
> - if (!user_chanlist)
> - return 0;
> -
I think you need a check here to avoid a NULL pointer getting
passed to the memdup_user().
If (!user_chanlist || cmd->chanlist_len == 0)
return -EINVAL;
> chanlist = memdup_user(user_chanlist,
> cmd->chanlist_len * sizeof(unsigned int));
> if (IS_ERR(chanlist))
> @@ -1609,10 +1605,13 @@ static int do_cmdtest_ioctl(struct comedi_device *dev,
>
> s = &dev->subdevices[cmd.subdev];
>
> - /* load channel/gain list */
> - ret = __comedi_get_user_chanlist(dev, s, user_chanlist, &cmd);
> - if (ret)
> - return ret;
> + /* user_chanlist can be NULL for COMEDI_CMDTEST ioctl */
> + if (user_chanlist) {
> + /* load channel/gain list */
> + ret = __comedi_get_user_chanlist(dev, s, user_chanlist, &cmd);
> + if (ret)
> + return ret;
> + }
>
> ret = s->do_cmdtest(dev, s, &cmd);
The reset looks ok.
Side-note on mem_dupuser(). That function does a kmalloc to copy the
user data to the kernel. Shouldn't we be freeing it when we are done with
it?
do_become_nonbusy() does a kfree(async->cmd.chanlist) which will free
the memory for do_cmd_ioctl().
But, do_cmdtest_ioctl() actually copies the user data into a local variable.
I think we are missing a kfree() there.
Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: comedi_isadma.h: make self-reliant
O n Friday, May 22, 2015 10:45 AM, Ian Abbott wrote: > The Comedi "comedi_isadma.h" header is included by the source for the > "comedi_isadma" helper module and other modules that use it. It does > not compile cleanly when it is the first header file included. It uses > the `dma_addr_t` type, so include to declare it. (Also, > that indirectly takes care of the use of `NULL`.) It uses `struct > comedi_device *` in various function prototypes, so add an incomplete > declaration of `struct comedi_device`. > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: comedi_8254.h: make self-reliant
On Friday, May 22, 2015 10:33 AM, Ian Abbott wrote: > The Comedi "comedi_8254.h" header file is included by various Comedi > drivers with timer/counters based on the 8254 chip. The drivers do not > compile cleanly if this header file is included first. It uses pointers > to the `struct comedi_device`, `struct comedi_subdevice`, and `struct > comedi_insn` structures in various function prototypes, so declare those > as incomplete types. It use the `bool` type, so include > . It also uses the `__iomem` tag, but that seems to be > taken care of indirectly by including . > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/4] staging: comedi: 8255: fix cleanup and split module
On Friday, May 22, 2015 8:22 AM, Ian Abbott wrote: > These patches are for the Comedi "8255" driver module (including the > creation of a new "comedi_8255" module split off from it). > > Patch 1 fixes a problem cleaning up on failure in the Comedi "8255" driver. > > Patch 2 is just a minor change to header file inclusion. > > Patch 3 is a documentation fix. > > Patch 4 splits off the "8255 subdevice helper" functionality of the > driver module (which is used by other Comedi driver modules), into a > separate module called "comedi_8255", leaving the existing "8255" module > as the Comedi driver for "standalone" 8255 devices. > > 1) staging: comedi: 8255: fix I/O region leak on failure > 2) staging: comedi: 8255.h: don't include "../comedidev.h" > 3) staging: comedi: 8255: document callback parameters better > 4) staging: comedi: comedi_8255: new module split from 8255 > > drivers/staging/comedi/Kconfig | 18 +- > drivers/staging/comedi/drivers/8255.c| 232 ++ > drivers/staging/comedi/drivers/8255.h| 19 +- > drivers/staging/comedi/drivers/Makefile | 3 +- > drivers/staging/comedi/drivers/comedi_8255.c | 285 > +++ > 5 files changed, 322 insertions(+), 235 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: addi_watchdog.h: don't include "../comedidev.h"
On Friday, May 22, 2015 9:32 AM, Ian Abbott wrote: > The Comedi "addi_watchdog.h" header doesn't use anything form > "comedidev.h" apart from `struct comedi_subdevice`, which it only uses > to construct a corresponding pointer type within the parameter list of a > function prototype. Just declare the structure type incompletely and > don't bother including the header file. > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/6] staging: comedi: amplc_dio200*: minor source cleanups
On Friday, May 22, 2015 10:16 AM, Ian Abbott wrote: > A few coding style cleanups for the Comedi amplc_dio200* modules, and to > make the "amplc_dio200.h" header file self-reliant. > > 1) staging: comedi: amplc_dio200.h: reformat copyright comment > 2) staging: comedi: amplc_dio200.h: make self-reliant > 3) staging: comedi: amplc_dio200.c: reformat copyright comment > 4) staging: comedi: amplc_dio200_common.c: reformat copyright comment > 5) staging: comedi: amplc_dio200_common.c: fix up brace style > 6) staging: comedi: amplc_dio200_pci.c: reformat copyright comment > > drivers/staging/comedi/drivers/amplc_dio200.c | 37 --- > drivers/staging/comedi/drivers/amplc_dio200.h | 44 + > .../staging/comedi/drivers/amplc_dio200_common.c | 55 > -- > drivers/staging/comedi/drivers/amplc_dio200_pci.c | 35 +++--- > 4 files changed, 91 insertions(+), 80 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: Question about the function,ni_stc_dma_channel_select_bitfield
On Wednesday, May 06, 2015 7:06 AM, nick wrote: > On 2015-05-06 05:10 AM, Ian Abbott wrote: >> On 06/05/15 01:22, nick wrote: >>> Greetings All, >>> I am wondering if in the function,ni_stc_dma_channel_select_bitfield the >>> line: >>> return 1 << channel; >>> is guaranteed to be below the threshold that guarantees us to not overflow >>> on >>> a unsigned 32 integer due to bit wise shifting to the left. >>> Thanks Nick >>> >> >> if (channel < 4) >> return 1 << channel; >> >> So, yes. >> > This should be commented in my option as this is not common knowledge > unless you known the hardware specs really well. If I should send in > a patch adding a comment here please let me known. A comment is not necessary. The code explicitly states that channel (an unsigned var) needs to be < 4 for the bit shift to happen. Hartley
RE: [PATCH 0/6] staging: comedi: gsc_hpdi: some clean-ups
On Tuesday, May 05, 2015 10:47 AM, Ian Abbott wrote: > Simplify (eliminate) the board type matching code, since only a single > board type is supported. Reformat the comments. Fix the checkpatch > issues. Use a better module description string. > > 1) staging: comedi: gsc_hpdi: tidy up comments > 2) staging: comedi: gsc_hpdi: remove multiple board type support > 3) staging: comedi: gsc_hpdi: usleep_range is preferred over udelay > 4) staging: comedi: gsc_hpdi: prefer using the BIT() macro > 5) staging: comedi: gsc_hpdi: use PCI_DEVICE_SUB() > 6) staging: comedi: gsc_hpdi: use a better MODULE_DESCRIPTION() > > drivers/staging/comedi/drivers/gsc_hpdi.c | 184 +- > 1 file changed, 80 insertions(+), 104 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: rtc: ep93xx: Use readl/writel for io
On Tuesday, May 19, 2015 1:40 PM, Alexandre Belloni wrote: > On 21/03/2012 at 10:55:31 -0700, hartleys wrote : >> Drivers should not be using the __raw_* io accessors. >> >> Signed-off-by: H Hartley Sweeten >> Cc: Alessandro Zummo > > > Rebased and applied, thanks! Man. I forgot all about that patch... ;-) Thanks, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/8] staging: comedi: mite: some clean-ups
On Friday, May 01, 2015 10:04 AM, Ian Abbott wrote: > 1) staging: comedi: mite: move #include > 2) staging: comedi: mite.h: remove PCIMIO_COMPAT > 3) staging: comedi: mite.c: remove commented out USE_KMALLOC > 4) staging: comedi: mite.h: remove "../comedi_pci.h" and make >self-reliant > 5) staging: comedi: mite.h: reformat some comments > 6) staging: comedi: mite.c: reformat comments > 7) staging: comedi: mite.h: whitespace changes in function declarations > 8) staging: comedi: mite: use a better MODULE_DESCRIPTION() > > TODO: document the exported functions in "mite.c". > > drivers/staging/comedi/drivers/mite.c | 110 > +- > drivers/staging/comedi/drivers/mite.h | 88 ++- > 2 files changed, 101 insertions(+), 97 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: Change error return code for if statement in the function,cb_pcimdas_ai_rinsn
On Tuesday, February 24, 2015 9:13 PM, Nicholas Krause wrote: > This changes us using the incorrect error,-ETIMEOUT when checking if > the channel we are allocating to on the device structure pointer passed > to this function is greater then the maximum available channels for this > device to the correct error for a channel being out of range,-ECHRNG. > > Signed-off-by: Nicholas Krause > --- > drivers/staging/comedi/drivers/cb_pcimdas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/comedi/drivers/cb_pcimdas.c > b/drivers/staging/comedi/drivers/cb_pcimdas.c > index 70dd2c9..d91a6f3 100644 > --- a/drivers/staging/comedi/drivers/cb_pcimdas.c > +++ b/drivers/staging/comedi/drivers/cb_pcimdas.c > @@ -121,7 +121,7 @@ static int cb_pcimdas_ai_rinsn(struct comedi_device *dev, > maxchans = s->n_chan; > > if (chan > (maxchans - 1)) > - return -ETIMEDOUT; /* *** Wrong error code. Fixme. */ > + return -ECHRNG; > > /* configure for sw initiated read */ > d = inb(devpriv->BADR3 + 5); Hmm... This isn't quite right... The 16 single-ended / 8 differential analog input channels on this board is set with a switch on the PCB. The state of the switch should be read when the driver is attached and the subdevice initialized with the correct number of channels. The core will then validate the "chan" number before calling the (*insn_read) operation. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/56] staging: comedi: introduce comedi_pci.h header
On Tuesday, March 10, 2015 9:25 AM, Joe Perches wrote: > On Tue, 2015-03-10 at 16:10 +, Ian Abbott wrote: >> "comedidev.h" includes PCI-specific stuff that gets included by all >> comedi drivers including non-PCI ones. Separate it out into its own >> header "comedi_pci.h". Make the new header include and >> "comedidev.h" so that comedi PCI drivers do not need to include them >> explicitly. > > Isn't the kernel progressing to avoid indirect includes? It appears so, and in general I agree. But, I also agree with what Ian has done here. > Perhaps it'd be nicer to move generic comedi .h files into > a more common location and change the include path so that > the include statements don't have to be relative? > > Maybe add an include/comedi/ directory? Ian already submitted similar patches for the comedi USB and PCMCIA drivers. Those patches have been applied. The comedi PCI/PCMCIA/USB (i.e. "bus") drivers now just need to include the "bus" specific comedi driver header to get the bus specific interface functions. These functions include the: module_comedi_"bus"_driver helper macro for the module_init/module_exit boilerplate comedi_"bus"_driver_register() comedi_"bus"_driver_unregister() helper functions to register and unregister the "bus" and comedi drivers comedi_"bus"_auto_config() comedi_"bus"_auto_unconfig() helper functions for the "bus" driver to probe/remove the comedi driver comedi_"bus"_enable() comedi_"bus"_disable() helper functions to request and enable/disable and release the I/O comedi_to_"bus"_dev() helper function to get the "bus" device from the comedi device These comedi_"bus" headers keep the drivers clean and easy to maintain. The drivers are still responsible for including any headers that are not specifically needed for the "bus". When comedi gets moved out of staging we will need to decide where the headers go. The only one I know for sure is comedi.h. That one is the UAPI header (include/uapi/comedi.h). The rest in drivers/staging/comedi will need to either go into include/linux/*.h or include/linux/comedi/*.h. There are still a couple headers in the comedi/drivers directory. Those do not need to be exposed so I think the relative include is fine. Just my two cents Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/56] staging: comedi: introduce comedi_pci.h header
On Tuesday, March 10, 2015 9:10 AM, Ian Abbott wrote: > "comedidev.h" includes PCI-specific stuff that gets included by all > comedi drivers including non-PCI ones. Separate it out into its own > header "comedi_pci.h". Make the new header include and > "comedidev.h" so that comedi PCI drivers do not need to include them > explicitly. Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] net: ep93xx_eth: Delete unnecessary checks before the function call "kfree"
On Wednesday, February 04, 2015 8:01 AM, Markus Elfring wrote:
> The kfree() function tests whether its argument is NULL and then
> returns immediately. Thus the test around the call is not needed.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/net/ethernet/cirrus/ep93xx_eth.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/cirrus/ep93xx_eth.c
> b/drivers/net/ethernet/cirrus/ep93xx_eth.c
> index 3a12c09..de9f7c9 100644
> --- a/drivers/net/ethernet/cirrus/ep93xx_eth.c
> +++ b/drivers/net/ethernet/cirrus/ep93xx_eth.c
> @@ -475,8 +475,7 @@ static void ep93xx_free_buffers(struct ep93xx_priv *ep)
> if (d)
> dma_unmap_single(dev, d, PKT_BUF_SIZE, DMA_FROM_DEVICE);
>
> - if (ep->rx_buf[i] != NULL)
> - kfree(ep->rx_buf[i]);
> + kfree(ep->rx_buf[i]);
> }
>
> for (i = 0; i < TX_QUEUE_ENTRIES; i++) {
> @@ -486,8 +485,7 @@ static void ep93xx_free_buffers(struct ep93xx_priv *ep)
> if (d)
> dma_unmap_single(dev, d, PKT_BUF_SIZE, DMA_TO_DEVICE);
>
> - if (ep->tx_buf[i] != NULL)
> - kfree(ep->tx_buf[i]);
> + kfree(ep->tx_buf[i]);
> }
>
> dma_free_coherent(dev, sizeof(struct ep93xx_descs), ep->descs,
Signed-off-by: H Hartley Sweeten
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH RFC] staging: comedi: dt282x: condition with no effect - if identical to else
On Tuesday, February 03, 2015 8:13 AM, Ian Abbott wrote:
> On 03/02/15 12:38, Nicholas Mc Guire wrote:
>> The if and the else branch code are identical - so the condition has no
>> effect on the effective code - this patch removes the condition and the
>> duplicated code.
>>
>> Signed-off-by: Nicholas Mc Guire
>> ---
>>
>> The if and else branch are identical code thus the condition has no effect
>>
>> if (cmd->scan_begin_src == TRIG_FOLLOW) {
>> /* internal trigger */
>> err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>> } else {
>> /* external trigger */
>> /* should be level/edge, hi/lo specification here */
>> err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
>> }
>>
> I think what that comment means is that it should allow scan_begin_arg
> to have various combinations of the CR_EDGE and CR_INVERT bits set.
> I.e. it ought to allow whatever combination of CR_EDGE and CR_INVERT
> better describes the nature of the external trigger signal, in addition
> to allowing the lazy default value 0.
>
> I don't know what the nature of the external trigger signal is, as I
> haven't seen the manual. I think Hartley might have seen one.
According to the manual, the external trigger is not "programmable". It's
a Schmitt trigger input, enables on TTL logic low, with a 22K pullup.
Since the 'scan_begin_arg' is not actually used for the analog input async
command, I think removing the comments completely is fine. Just change
the check to:
err |= cfc_check_trigger_arg_is(&cmd->scan_begin_arg, 0);
Regards,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 00/10] staging: comedi: introduce comedi_pcmcia.h header
On Friday, January 30, 2015 2:57 AM, Ian Abbott wrote: > "comedidev.h" includes PCMCIA-specific stuff that gets included by all > comedi drivers including non-PCMCIA ones. Separate it out into its own > header "comedi_pcmcia.h". Make the new header include > , and "comedidev.h" so that comedi PCMCIA > drivers do not need to include them explicitly. > > 01) staging: comedi: add comedi_pcmcia.h > 02) staging: comedi: comedi_pcmcia.c: include new "comedi_pcmcia.h" > header > 03) staging: comedi: cb_das16_cs: include new "comedi_pcmcia.h" header > 04) staging: comedi: das08_cs: include new "comedi_pcmcia.h" header > 05) staging: comedi: ni_daq_700: include new "comedi_pcmcia.h" header > 06) staging: comedi: ni_daq_dio24: include new "comedi_pcmcia.h" header > 07) staging: comedi: ni_labpc_cs: include new "comedi_pcmcia.h" header > 08) staging: comedi: ni_mio_cs: include new "comedi_pcmcia.h" header > 09) staging: comedi: quatech_daqp_cs: include new "comedi_pcmcia.h" > header > 10) staging: comedi: comedi_pcmcia.h: move PCMCIA stuff out of > comedidev.h > > drivers/staging/comedi/comedi_pcmcia.c | 5 +-- > drivers/staging/comedi/comedi_pcmcia.h | 55 > > drivers/staging/comedi/comedidev.h | 33 -- > drivers/staging/comedi/drivers/cb_das16_cs.c | 5 +-- > drivers/staging/comedi/drivers/das08_cs.c| 5 +-- > drivers/staging/comedi/drivers/ni_daq_700.c | 5 +-- > drivers/staging/comedi/drivers/ni_daq_dio24.c| 6 +-- > drivers/staging/comedi/drivers/ni_labpc_cs.c | 6 +-- > drivers/staging/comedi/drivers/ni_mio_cs.c | 6 +-- > drivers/staging/comedi/drivers/quatech_daqp_cs.c | 7 +-- > 10 files changed, 63 insertions(+), 70 deletions(-) Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging/comedi/dt282x: avoid integer overflow warning
On Monday, March 14, 2016 3:48 PM, Arnd Bergmann wrote: > gcc-6 warns about passing negative signed integer into swab16() > in the dt282x driver: > The warning makes sense, though the code is correct as far as I > can tell. > > This disambiguates the operation by making the constant expressions > we pass here explicitly 'unsigned', which helps to avoid the warning. > > Signed-off-by: Arnd Bergmann > --- > drivers/staging/comedi/drivers/dt282x.c | 62 > - > 1 file changed, 31 insertions(+), 31 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/dt282x.c > b/drivers/staging/comedi/drivers/dt282x.c > index 40bf00984fa5..d4d45c759c62 100644 > --- a/drivers/staging/comedi/drivers/dt282x.c > +++ b/drivers/staging/comedi/drivers/dt282x.c > @@ -69,48 +69,48 @@ > * Register map > */ > #define DT2821_ADCSR_REG 0x00 > -#define DT2821_ADCSR_ADERR (1 << 15) > -#define DT2821_ADCSR_ADCLK (1 << 9) > -#define DT2821_ADCSR_MUXBUSY (1 << 8) > -#define DT2821_ADCSR_ADDONE (1 << 7) > -#define DT2821_ADCSR_IADDONE (1 << 6) > +#define DT2821_ADCSR_ADERR (1u << 15) Changing all of these to use the BIT() macro should also avoid the warning. Hartley
RE: [PATCH] staging/comedi/dt282x: avoid integer overflow warning
On Tuesday, March 15, 2016 2:50 PM, Arnd Bergmann wrote: > On Tuesday 15 March 2016 21:35:40 Hartley Sweeten wrote: >> On Monday, March 14, 2016 3:48 PM, Arnd Bergmann wrote: >>> gcc-6 warns about passing negative signed integer into swab16() >>> in the dt282x driver: >> >> >> >>> The warning makes sense, though the code is correct as far as I >>> can tell. >>> >>> This disambiguates the operation by making the constant expressions >>> we pass here explicitly 'unsigned', which helps to avoid the warning. >>> >>> Signed-off-by: Arnd Bergmann >>> --- >>> drivers/staging/comedi/drivers/dt282x.c | 62 >>> - >>> 1 file changed, 31 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/staging/comedi/drivers/dt282x.c >>> b/drivers/staging/comedi/drivers/dt282x.c >>> index 40bf00984fa5..d4d45c759c62 100644 >>> --- a/drivers/staging/comedi/drivers/dt282x.c >>> +++ b/drivers/staging/comedi/drivers/dt282x.c >>> @@ -69,48 +69,48 @@ >>> * Register map >>> */ >>> #define DT2821_ADCSR_REG 0x00 >> -#define DT2821_ADCSR_ADERR (1 << 15) >>> -#define DT2821_ADCSR_ADCLK (1 << 9) >>> -#define DT2821_ADCSR_MUXBUSY (1 << 8) >>> -#define DT2821_ADCSR_ADDONE(1 << 7) >>> -#define DT2821_ADCSR_IADDONE (1 << 6) >>> +#define DT2821_ADCSR_ADERR (1u << 15) >> >> Changing all of these to use the BIT() macro should also avoid the warning. > > Yes, but it won't work for the ones that have more than one bit: > > #define DT2821_SUPCSR_DS_AD_TRIG (3 << 10) Use a helper macro for those bits: #define DT2821_SUPCSR_DS(x) (((x) & 0x3) << 10) #define DT2821_SUPCSR_DS_PIODT2821_SUPCSR_DS(0) #define DT2821_SUPCSR_DS_AD_CLK DT2821_SUPCSR_DS(1) #define DT2821_SUPCSR_DS_DA_CLK DT2821_SUPCSR_DS(2) #define DT2821_SUPCSR_DS_AD_TRIGDT2821_SUPCSR_DS(3) > I considered using BIT() but decided against it for consistency. Your change may fix the gcc-6 issue but it doesn't fix the 28 checkpatch.pl issues: CHECK: Prefer using the BIT macro Regards, Hartley
RE: [PATCH] staging: comedi: das1800: remove unused variable
On Tuesday, April 05, 2016 7:23 AM, Sudip Mukherjee wrote: > The variable unipolar was never used. > > Signed-off-by: Sudip Mukherjee > --- > > There may be a chance that reading from DAS1800_CONTROL_C is necessary > before reading from DAS1800_STATUS. If that is true then please discard > this patch. Actually the driver has a bug here. The analog input samples should be munged if the inputs are configured for bipolar mode. I have a series almost ready that cleans up this driver and fixes the bug. Thanks, Hartley
RE: [PATCH] staging: comedi: das1800: remove unused variable
On Wednesday, April 06, 2016 2:41 AM, Ian Abbott wrote: > On 06/04/16 02:21, Hartley Sweeten wrote: >> On Tuesday, April 05, 2016 7:23 AM, Sudip Mukherjee wrote: >>> The variable unipolar was never used. >>> >>> Signed-off-by: Sudip Mukherjee >>> --- >>> >>> There may be a chance that reading from DAS1800_CONTROL_C is necessary >>> before reading from DAS1800_STATUS. If that is true then please discard >>> this patch. >> >> Actually the driver has a bug here. >> >> The analog input samples should be munged if the inputs are configured for >> bipolar mode. >> >> I have a series almost ready that cleans up this driver and fixes the bug. > > Hi Hartley, can the bug fix be placed at the top of your patch series? I'll split it out as a separate patch and post it shortly. Thanks, Hartley
RE: [PATCH] staging: comedi: das1800: remove unused variable
On Wednesday, April 06, 2016 3:41 AM, Ian Abbott wrote:
> On 06/04/16 10:41, Ian Abbott wrote:
>> On 06/04/16 02:21, Hartley Sweeten wrote:
>>> On Tuesday, April 05, 2016 7:23 AM, Sudip Mukherjee wrote:
>>>> The variable unipolar was never used.
>>>>
>>>> Signed-off-by: Sudip Mukherjee
>>>> ---
>>>>
>>>> There may be a chance that reading from DAS1800_CONTROL_C is necessary
>>>> before reading from DAS1800_STATUS. If that is true then please discard
>>>> this patch.
>>>
>>> Actually the driver has a bug here.
>>>
>>> The analog input samples should be munged if the inputs are
>>> configured for bipolar mode.
>>>
>>> I have a series almost ready that cleans up this driver and fixes the
>>> bug.
>>
>> Hi Hartley, can the bug fix be placed at the top of your patch series?
>> Thanks.
>>
Just posted the patch. It's a bit more involved than your fix in the comedi.org
code but I think it's more complete.
> The bug has been there forever (even in the "out-of-tree" version from
> comedi.org, where I have just fixed it). There have been patches applied
> to reformat and remove the incorrect bits of code, including
> a142785d7c9d ("Staging: comedi: das1800: fixed multiple brace coding
> style issues and pointer declaration style errors") and 82d28561b7e0
> ("staging: comedi: Remove if condition."). The latter patch,
> 82d28561b7e0, mainly served to hide the bug further!
Looks like both of those patches might have originated from one of the
outreachy programs that Greg deals with.
If you think this needs to be fixed in any of the stable trees, I can split it
to
match your comedi.org fix (to fix the stable trees) and a second patch for
the additional cleanup. This is an old legacy ISA board and, as you stated,
the bug has been there forever so I'm not sure if the backport is worth it.
Regards,
Hartley
RE: [PATCH 6/6] staging: comedi: don't use mutex when polling file
On Friday, October 09, 2015 4:27 AM, Ian Abbott wrote:
> The main mutex in a comedi device can get held for quite a while when
> processing comedi instructions, so for performance reasons, the "read"
> and "write" file operations do not use it; they use use the
> `attach_lock` rwsemaphore to protect against the comedi device becoming
> detached at an inopportune moment. Do the same for the "poll" file
> operation.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/comedi_fops.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/comedi/comedi_fops.c
> b/drivers/staging/comedi/comedi_fops.c
> index 07bb197..88e9334 100644
> --- a/drivers/staging/comedi/comedi_fops.c
> +++ b/drivers/staging/comedi/comedi_fops.c
> @@ -2264,7 +2264,7 @@ static unsigned int comedi_poll(struct file *file,
> poll_table *wait)
> struct comedi_device *dev = cfp->dev;
> struct comedi_subdevice *s, *s_read;
>
> - mutex_lock(&dev->mutex);
> + down_read(&dev->attach_lock);
>
> if (!dev->attached) {
> dev_dbg(dev->class_dev, "no driver attached\n");
> @@ -2294,7 +2294,7 @@ static unsigned int comedi_poll(struct file *file,
> poll_table *wait)
> }
>
> done:
> - mutex_unlock(&dev->mutex);
> + up_read(&dev->attach_lock);
> return mask;
> }
Ian,
No issues with this patch, just a comment:
checkpatch.pl reports some issue about the spinlock_t and mutex definitions
in comedidev.h:
CHECK: spinlock_t definition without comment
#177: FILE: drivers/staging/comedi/comedidev.h:177:
+ spinlock_t spin_lock;
CHECK: spinlock_t definition without comment
#540: FILE: drivers/staging/comedi/comedidev.h:540:
+ spinlock_t spinlock;
CHECK: struct mutex definition without comment
#541: FILE: drivers/staging/comedi/comedidev.h:541:
+ struct mutex mutex;
I know these are documented in the docbook comment for the structs but would
you mind adding some comments to the definitions to quiet checkpatch.pl?
Thanks,
Hartley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 0/6] staging: comedi: fix some minor issues with file poll op
On Friday, October 09, 2015 4:27 AM, Ian Abbott wrote: > A few changes for the "poll" file operation to avoid poll-waiting on the > same subdevice for both read and write (patch 1), avoid allocating write > buffer space unnecessarily and possibly inappropriately (patch 4), > consider whether any active commands belong to the current file object > (patch 5), and avoid using the main mutex (for performance reasons) > (patch 6). > > 1) staging: comedi: don't poll_wait on same subdevice twice > 2) staging: comedi: rename comedi_buf_write_n_available > 3) staging: comedi: add new comedi_buf_write_n_available() > 4) staging: comedi: don't allocate buffer space when polling for write > 5) staging: comedi: check command started by file being polled > 6) staging: comedi: don't use mutex when polling file > > drivers/staging/comedi/comedi_buf.c | 19 +-- > drivers/staging/comedi/comedi_fops.c | 17 + > drivers/staging/comedi/comedi_internal.h | 1 + > 3 files changed, 23 insertions(+), 14 deletions(-) Thanks! Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: ii_pci20kc: Fix coding style - use BIT macro
On Monday, November 09, 2015 9:26 AM, Ranjith wote: > BIT macro is used for defining BIT location instead of > shifting operator - coding style issue > > Signed-off-by: Ranjith This has already been fixed in by: commit c98f4011ebd41ab9ff15e1c52acc446e1ee7e191 staging: comedi: ii_pci20kc: prefer using the BIT macro Please based your patches on linux-next or Greg Kroah-Hartman's staging-next. Thanks, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: comedi_parport: Fix coding style - use BIT macro
On Monday, November 09, 2015 8:17 AM, Ranjith wrote: > BIT macro is used for defining BIT location instead of > shifting operator - coding style issue > > Signed-off-by: Ranjith > --- > drivers/staging/comedi/drivers/comedi_parport.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/comedi_parport.c > b/drivers/staging/comedi/drivers/comedi_parport.c > index 15a4093..1bf8ddc 100644 > --- a/drivers/staging/comedi/drivers/comedi_parport.c > +++ b/drivers/staging/comedi/drivers/comedi_parport.c > @@ -75,8 +75,8 @@ > #define PARPORT_DATA_REG 0x00 > #define PARPORT_STATUS_REG 0x01 > #define PARPORT_CTRL_REG 0x02 > -#define PARPORT_CTRL_IRQ_ENA (1 << 4) > -#define PARPORT_CTRL_BIDIR_ENA (1 << 5) > +#define PARPORT_CTRL_IRQ_ENA BIT(4) > +#define PARPORT_CTRL_BIDIR_ENA BIT(5) > > static int parport_data_reg_insn_bits(struct comedi_device *dev, > struct comedi_subdevice *s, Hello, No problems with the patch but your sign-off needs to include your full legal name. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: pcmmio: Fix coding style - use BIT macro
On Monday, November 09, 2015 8:09 AM, Ranjith wrote: > BIT macro is used for defining BIT location instead of > shifting operator - coding style issue > > Signed-off-by: Ranjith > --- > drivers/staging/comedi/drivers/pcmmio.c | 44 > +++ > 1 file changed, 22 insertions(+), 22 deletions(-) > Hello, Again, no problems with the patch but your sign-off needs to include your full legal name. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: me_daq: Fix coding style - use BIT macro
On Monday, November 09, 2015 10:14 AM, Ranjith wrote: > BIT macro is used for defining BIT location instead of > shifting operator - coding style issue > > Signed-off-by: Ranjith This has already been fixed. Please base your patches on linux-next or Greg Kroah-Hartman's staging-next tree. Also, your sign-off should include your full legal name. Regards, Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: comedi_pci.c: Fix kernel-doc Return tags
On Wednesday, September 30, 2015 8:44 AM, Ian Abbott wrote: > Fix the 'Return' tags in the kernel-doc comments as they currently say > 'Returns', which is not recognized by kernel-doc. > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: comedi_pcmcia.c: improve function documentation
On Wednesday, September 30, 2015 9:11 AM, Ian Abbott wrote: > Expand the descriptions of the functions and document the return values. > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] staging: comedi: comedi_usb.c: improve function documentation
On Wednesday, September 30, 2015 9:37 AM, Ian Abbott wrote: > Expand the descriptions of the functions and document the return values. > > Signed-off-by: Ian Abbott Reviewed-by: H Hartley Sweeten -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] comedi: dmm32at: Fix coding style - use BIT macro
On Monday, November 16, 2015 10:09 AM, Ranjith Thangavel wrote: > BIT macro is used for defining BIT location instead of > shifting operator, usleep_range is preferred over > udelay - coding style issue > > Signed-off-by: Ranjith Thangavel > --- > drivers/staging/comedi/drivers/dmm32at.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/comedi/drivers/dmm32at.c > b/drivers/staging/comedi/drivers/dmm32at.c > index d312fda..9f8c9eb 100644 > --- a/drivers/staging/comedi/drivers/dmm32at.c > +++ b/drivers/staging/comedi/drivers/dmm32at.c > @@ -508,7 +508,7 @@ static int dmm32at_reset(struct comedi_device *dev) > outb(DMM32AT_CTRL_RESETA, dev->iobase + DMM32AT_CTRL_REG); > > /* allow a millisecond to reset */ > - udelay(1000); > + usleep_range(1000, 1050); > > /* zero scan and fifo control */ > outb(0x0, dev->iobase + DMM32AT_FIFO_CTRL_REG); > @@ -524,7 +524,7 @@ static int dmm32at_reset(struct comedi_device *dev) > outb(DMM32AT_RANGE_U10, dev->iobase + DMM32AT_AI_CFG_REG); > > /* should take 10 us to settle, here's a hundred */ > - udelay(100); > + usleep_range(100, 150); > > /* read back the values */ > ailo = inb(dev->iobase + DMM32AT_AI_LO_CHAN_REG); Nothing in this patch uses the BIT macro. Please make sure your commit messages actually match the patch. You also posted a different patch on Saturday with the exact same commit message. Hartley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/2] watchdog: ep93xx_wdt: cleanup and let the core handle the heartbeat
On Monday, January 30, 2017 11:55 AM, Guenter Roeck wrote: > On Mon, Jan 30, 2017 at 09:55:47AM -0700, H Hartley Sweeten wrote: >> Cleanup this driver and remove the 200ms heartbeat timer. The core now >> has the ability to handle the heartbeat. >> >> Signed-off-by: H Hartley Sweeten >> Cc: Wim Van Sebroeck >> Cc: Guenter Roeck Hi Guenter, I wasn't sure this patch was going to get delivered correctly. I got an "Undeliverable" bounce due to possible spoofing. I am trying to figure out why right now. Anyway... >> -#define WDT_VERSION "0.4" >> - >> -/* default timeout (secs) */ >> -#define WDT_TIMEOUT 30 >> - > > Personally I like those constants, even if used only once (I know, it is a > good > candidate for bikeshedding). Two reasons: 1) It is already there, and 2) It > helps seeing the default without having to dig into the code. I assume the WDT_VERSION can go away... As far as the WDT_TIMEOUT, I have no problem leaving it. I was just trying to remove some cruft. >> static bool nowayout = WATCHDOG_NOWAYOUT; >> module_param(nowayout, bool, 0); >> MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started"); >> >> -static unsigned int timeout = WDT_TIMEOUT; >> -module_param(timeout, uint, 0); >> -MODULE_PARM_DESC(timeout, >> -"Watchdog timeout in seconds. (1<=timeout<=3600, default=" >> -__MODULE_STRING(WDT_TIMEOUT) ")"); >> - > > Are you sure you want to take away the means to set the timeout with > a module parameter ? You could easily retain the module parameter > and call watchdog_init_timeout(wdd, timeout, dev). The parameter should > then be initialized with 0, though, to have the watchdog core take the > timeout from devicetree if provided. Again, I have no problem leaving this. I personally don't use it but someone else might. I'm not sure if the ep93xx will ever get converted to devicetree but I might figure it one eventually. >> +watchdog_set_drvdata(wdd, priv); >> >> -setup_timer(&timer, ep93xx_wdt_timer_ping, 1); >> +ret = watchdog_register_device(wdd); > > Looks like a perfect candidate for devm_watchdog_register_device(). I just saw your patches that do this. I will update this patch to use it. Thanks, Hartley
RE: [PATCH 2/2] watchdog: ts72xx_wdt: convert driver to watchdog core
On Monday, January 30, 2017 12:02 PM, Guenter Roeck wrote:
> On Mon, Jan 30, 2017 at 09:55:48AM -0700, H Hartley Sweeten wrote:
>> Cleanup this driver and convert it to use the watchdog framework API.
>>
>> Signed-off-by: H Hartley Sweeten
>> Cc: Mika Westerberg
>> Cc: Wim Van Sebroeck
>> Cc: Guenter Roeck
>> ---
>> drivers/watchdog/ts72xx_wdt.c | 447
>> +-
>> 1 file changed, 93 insertions(+), 354 deletions(-)
>> -#define TS72XX_WDT_DEFAULT_TIMEOUT 8
>> -
>> -static int timeout = TS72XX_WDT_DEFAULT_TIMEOUT;
>> -module_param(timeout, int, 0);
>> -MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds. "
>> - "(1 <= timeout <= 8, default="
>> - __MODULE_STRING(TS72XX_WDT_DEFAULT_TIMEOUT)
>> - ")");
>
> Same question as with patch #1 - are you sure you want to take this away ?
Again, not a problem leaving it in.
> You might just drop the limits instead (also see below).
>> +wdd->min_timeout = 1;
>> +wdd->max_timeout = 8;
>
> With such a low maximum timeout, it might make sense to use the core
> to be able to support larger timeouts.
Agree. I'll update and test this for the next version.
>> +wdd->timeout = 8;
>> +wdd->parent = &pdev->dev;
>>
>> -/* make sure that the watchdog is disabled */
>> -ts72xx_wdt_stop(wdt);
>
> Are you sure this is no longer needed ? If there is a means to detect if the
> watchdog is running, it might make sense to set the WDOG_HW_RUNNING flag
> instead
> if it is running and let the core handle the ping until the watchdog device
> is opened.
A patch to make sure this watchdog is disabled early during the kernel
uncompress
will soon be applied. Arnd Bergmann has it in his todo folder:
[PATCH] ARM: ep93xx: Disable TS-72xx watchdog before uncompressing
The bootloader currently enables the watchdog for 8 seconds and it needs to be
disabled just in case the uncompress takes longer.
I don't have a problem leaving it in here it's just not necessary.
>> +watchdog_set_nowayout(wdd, nowayout);
>>
>> -error = misc_register(&ts72xx_wdt_miscdev);
>> -if (error) {
>> -dev_err(&pdev->dev, "failed to register miscdev\n");
>> -return error;
>> -}
>> +watchdog_set_drvdata(wdd, priv);
>> +
>> +ret = watchdog_register_device(wdd);
>
> devm_watchdog_register_device() ?
I'll update this.
Thanks,
Hartley
RE: [PATCH] ARM: ep93xx: Disable TS-72xx watchdog before uncompressing
On Sunday, December 18, 2016 7:08 PM, Florian Fainelli wrote: > Le 12/11/16 à 11:16, Florian Fainelli a écrit : >> The TS-72xx/73xx boards have a CPLD watchdog which is configured to >> reset the board after 8 seconds, if the kernel is large enough that this >> takes about this time to decompress the kernel, we will encounter a >> spurious reboot. >> >> Do not pull ts72xx.h, but instead locally define what we need to disable >> the watchdog. >> >> Signed-off-by: Florian Fainelli > > Hartley, Ryan, do you guys maintain a git tree with EP93xx patches, or > should I submit them through RMK's patch system once you are okay with them? Ryan has an old tree on github but it has not been updated since Oct 14, 2013. I'm not sure if he is doing any active development at this time. This patch will have to go through RMK's patch system or some other tree. Thanks, Hartley
RE: [PATCH v5 2/4] ARM: Define KERNEL_START and KERNEL_END
On Tuesday, January 03, 2017 6:14 PM, Florian Fainelli wrote:
>
> In preparation for adding CONFIG_DEBUG_VIRTUAL support, define a set of
> common constants: KERNEL_START and KERNEL_END which abstract
> CONFIG_XIP_KERNEL vs. !CONFIG_XIP_KERNEL. Update the code where
> relevant.
>
> Acked-by: Russell King
> Signed-off-by: Florian Fainelli
> ---
> arch/arm/include/asm/memory.h | 7 +++
> arch/arm/mm/init.c| 7 ++-
> arch/arm/mm/mmu.c | 6 +-
> 3 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 76cbd9c674df..bee7511c5098 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -111,6 +111,13 @@
>
> #endif /* !CONFIG_MMU */
>
> +#ifdef CONFIG_XIP_KERNEL
> +#define KERNEL_START _sdata
> +#else
> +#define KERNEL_START _stext
> +#endif
> +#define KERNEL_END _end
> +
> /*
> * We fix the TCM memories max 32 KiB ITCM resp DTCM at these
> * locations
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 370581aeb871..c87d0d5b65f2 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -230,11 +230,8 @@ phys_addr_t __init arm_memblock_steal(phys_addr_t size,
> phys_addr_t align)
> void __init arm_memblock_init(const struct machine_desc *mdesc)
> {
> /* Register the kernel text, kernel data and initrd with memblock. */
> -#ifdef CONFIG_XIP_KERNEL
> - memblock_reserve(__pa(_sdata), _end - _sdata);
> -#else
> - memblock_reserve(__pa(_stext), _end - _stext);
> -#endif
> + memblock_reserve(__pa(KERNEL_START), _end - KERNEL_START);
Shouldn't the '_end' above be 'KERNEL_END'?
Hartley
RE: [PATCH v3 1/2] ARM: ep93xx: Register ts73xx-fpga manager driver for TS-7300
On Tuesday, December 13, 2016 7:36 PM, Florian Fainelli wrote: > > Register the TS-7300 FPGA manager device drivers which allows us to load > bitstreams into the on-board Altera Cyclone II FPGA. > > Signed-off-by: Florian Fainelli > --- > arch/arm/mach-ep93xx/ts72xx.c | 26 ++ > 1 file changed, 26 insertions(+) For the ep93xx core change: Acked-by: H Hartley Sweeten
RE: [PATCH] drivers: staging: comedi: fix function prototypes
On December 14, 2016 6:42 AM, Piotr Gregor wrote: > Add names of parameters to function prototypes in comedi PCI. > Checkpatch reports now no errors. > > Signed-off-by: Piotr Gregor > --- > drivers/staging/comedi/comedi_pci.h | 18 ++ > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/comedi/comedi_pci.h > b/drivers/staging/comedi/comedi_pci.h > index 4005cc9..7dfd892 100644 > --- a/drivers/staging/comedi/comedi_pci.h > +++ b/drivers/staging/comedi/comedi_pci.h > @@ -34,18 +34,20 @@ > #define PCI_VENDOR_ID_RTD0x1435 > #define PCI_VENDOR_ID_HUMUSOFT 0x186c > > -struct pci_dev *comedi_to_pci_dev(struct comedi_device *); > +struct pci_dev *comedi_to_pci_dev(struct comedi_device *dev); For the function prototypes I prefer no names for the "pointer" parameters. The "struct foo *" declaration is just as clear as "struct foo *bar". Thanks, Hartley
RE: [PATCH 2/2] FPGA: Add TS-7300 FPGA manager
On Monday, December 12, 2016 9:02 AM, Alan Tull wrote: > On Sun, 11 Dec 2016, Florian Fainelli wrote: >> Add support for loading bitstreams on the Altera Cyclone II FPGA >> populated on the TS-7300 board. This is done through the configuration >> and data registers offered through a memory interface between the EP93xx >> SoC and the FPGA. >> >> Signed-off-by: Florian Fainelli > > Hi Florain, > > Thanks for submitting! > > How specific is this to the tx7300 board? > > I'm unclear about the programming method here. Are these registers > exposed by the EP93xx? Is it possible that another cpu could access > these two registers to configure the cyclone ii? Is this passive > serial? Alan, >From the schematic, it appears that the Cyclone II FPGA is configured for Fast AS programming. The glue chip (MAXII CLPD) on the board appears to implement some kind of state machine to handle the FPGA programming using two registers in the glue chip. Hartley
RE: [PATCH v3 2/2] FPGA: Add TS-7300 FPGA manager
On Wednesday, December 14, 2016 11:55 AM, Florian Fainelli wrote: > My understanding is that, yes, this triggers the final write. You are > right that ts73xx_fpga_write() can be called multiple times. It sounds > like what my write_complete function does right now is just return that > we successfully completed the bistream write, but this snippet that you > are quoting should actually be moved into write_complete. Florian, I'm in the process of getting a TS-7300 board so I can help test this. Hopefully I will have it by next week. Hartley
RE: [PATCH] drivers: staging: comedi: fix function prototypes
On Thursday, December 15, 2016 4:47 AM, Ian Abbott wrote: > On 14/12/16 16:14, Hartley Sweeten wrote: >> On December 14, 2016 6:42 AM, Piotr Gregor wrote: >>> -struct pci_dev *comedi_to_pci_dev(struct comedi_device *); >>> +struct pci_dev *comedi_to_pci_dev(struct comedi_device *dev); >> >> For the function prototypes I prefer no names for the "pointer" parameters. >> >> The "struct foo *" declaration is just as clear as "struct foo *bar". > > Maybe, but checkpatch.pl doesn't agree (not since commit > ca0d8929e75ab1f860f61208d46955f280a1b05e anyway)! Hmm.. Missed seeing that one go in. I still think it's silly to name struct pointers in function arguments. Especially since that normally leads to stuff like 'struct foo *foo' where the parameter name is the same as the struct name. Void pointers and generic types are a different matter. Naming those makes sense for clarity. Oh well... Just my 2 cents... Hartley
RE: [PATCH] drivers: staging: comedi: fix function prototypes
On Thursday, December 15, 2016 4:45 AM, Ian Abbott wrote: > On 14/12/16 13:42, Piotr Gregor wrote: >> Add names of parameters to function prototypes in comedi PCI. >> Checkpatch reports now no errors. >> >> Signed-off-by: Piotr Gregor > > Looks good, thanks! > > Reviewed-by: Ian Abbott Reviewed-by: H Hartley Sweeten
RE: [PATCH 02/35] ARM: ep93xx: Convert remaining uses of pr_warning to pr_warn
On Friday, February 17, 2017 12:11 AM, Joe Perches wrote:
> To enable eventual removal of pr_warning.
>
> This makes pr_warn use consistent for arch/arm
>
> Prior to this patch, there were 2 uses of pr_warning and
> 405 uses of pr_warn in arch/arm
>
> Signed-off-by: Joe Perches
> ---
> arch/arm/mach-ep93xx/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-ep93xx/core.c b/arch/arm/mach-ep93xx/core.c
> index c393b1b0310d..d6e907b6cc31 100644
> --- a/arch/arm/mach-ep93xx/core.c
> +++ b/arch/arm/mach-ep93xx/core.c
> @@ -346,9 +346,9 @@ void __init ep93xx_register_i2c(struct
> i2c_gpio_platform_data *data,
>* CMOS driver.
>*/
> if (data->sda_is_open_drain && data->sda_pin != EP93XX_GPIO_LINE_EEDAT)
> - pr_warning("sda != EEDAT, open drain has no effect\n");
> + pr_warn("sda != EEDAT, open drain has no effect\n");
> if (data->scl_is_open_drain && data->scl_pin != EP93XX_GPIO_LINE_EECLK)
> - pr_warning("scl != EECLK, open drain has no effect\n");
> + pr_warn("scl != EECLK, open drain has no effect\n");
>
> __raw_writel((data->sda_is_open_drain << 1) |
>(data->scl_is_open_drain << 0),
For the ep93xx part:
Acked-by: H Hartley Sweeten
RE: [PATCH RESEND] ARM: ep93xx: Disable TS-72xx watchdog before uncompressing
On Thursday, February 02, 2017 1:12 PM, Florian Fainelli wrote: > The TS-72xx/73xx boards have a CPLD watchdog which is configured to > reset the board after 8 seconds, if the kernel is large enough that this > takes about this time to decompress the kernel, we will encounter a > spurious reboot. > > Do not pull ts72xx.h, but instead locally define what we need to disable > the watchdog. > > Signed-off-by: Florian Fainelli > --- > Arnd, this is a recent, so you can queue this up, thanks! Reviewed-by: H Hartley Sweeten or Tested-by/Acked-by as appropriate... Hartley
RE: [PATCH 5/5] Staging: comedi: Prefer using the BIT macro issue in dmm32at.c
On Tuesday, June 07, 2016 4:51 AM, Ravishankar Karkala Mallikarjunayya wrote: > Subject: [PATCH 5/5] Staging: comedi: Prefer using the BIT macro issue in > dmm32at.c Please don't add random patches to a series. You started this series with 1/4, 2/4, 3/4 and 4/4 then added a 5/5. Also, please fix the subject lines to include the driver name. This makes thing clearer when multiple patches do similar things. The subject lines should look something like this: staging: comedi: : > This patch Replace all occurences of (1< to get rid of checkpatch.pl "CHECK" output "Prefer using the BIT macro" There is no need to state that this is a patch. That is already known. Regards, Hartley
RE: [PATCH 4/5] rtc: m48t86: move m48t86.h to platform_data
On Sunday, June 26, 2016 3:03 PM, Alexandre Belloni wrote:
> m48t86.h belongs to include/linux/platform_data/
>
> Signed-off-by: Alexandre Belloni
> ---
> Cc: Hartley Sweeten
> Cc: Ryan Mallon
> Cc: Alexander Clouter
> Cc: Jason Cooper
> Cc: Andrew Lunn
> Cc: Sebastian Hesselbarth
> Cc: Gregory Clement
> Cc: linux-arm-ker...@lists.infradead.org
>
> arch/arm/mach-ep93xx/ts72xx.c | 2 +-
> arch/arm/mach-orion5x/ts78xx-setup.c | 2 +-
> drivers/rtc/rtc-m48t86.c | 2 +-
> include/linux/{m48t86.h => platform_data/rtc-m48t86.h} | 0
> 4 files changed, 3 insertions(+), 3 deletions(-)
> rename include/linux/{m48t86.h => platform_data/rtc-m48t86.h} (100%)
For ep93xx:
Acked-by: H Hartley Sweeten
RE: [PATCH] staging: comedi: drivers: remove bogus ni_mio_c_common.c
On Wednesday, March 23, 2016 3:55 AM, Ian Abbott wrote:
> The zero-length file "ni_mio_c_common.c" was inadvertantly created by
> commit e563637b5fef ("staging: comedi: Use ARRAY_SIZE for sizes of
> arrays"). Remove it.
>
> Signed-off-by: Ian Abbott
> ---
> drivers/staging/comedi/drivers/ni_mio_c_common.c | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> delete mode 100644 drivers/staging/comedi/drivers/ni_mio_c_common.c
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_c_common.c
> b/drivers/staging/comedi/drivers/ni_mio_c_common.c
> deleted file mode 100644
> index e69de29..000
Weird. Good catch.
Reviewed-by: H Hartley Sweeten
RE: [PATCH v2] staging/comedi/dt282x: avoid integer overflow warning
On Wednesday, March 16, 2016 1:51 PM, Arnd Bergmann wrote: > > gcc-6 warns about passing negative signed integer into swab16() > in the dt282x driver: > > drivers/staging/comedi/drivers/dt282x.c: In function 'dt282x_load_changain': > include/uapi/linux/swab.h:14:33: warning: integer overflow in expression > [-Woverflow] > (((__u16)(x) & (__u16)0xff00U) >> 8))) > ~~~^ > include/uapi/linux/swab.h:107:2: note: in expansion of macro > '___constant_swab16' > ___constant_swab16(x) : \ > ^~ > include/uapi/linux/byteorder/big_endian.h:34:43: note: in expansion of macro > '__swab16' > #define __cpu_to_le16(x) ((__force __le16)__swab16((x))) >^~~~ > include/linux/byteorder/generic.h:89:21: note: in expansion of macro > '__cpu_to_le16' > #define cpu_to_le16 __cpu_to_le16 > ^ > arch/arm/include/asm/io.h:250:6: note: in expansion of macro 'cpu_to_le16' > cpu_to_le16(v),__io(p)); }) > ^~~ > drivers/staging/comedi/drivers/dt282x.c:566:2: note: in expansion of macro > 'outw' > outw(DT2821_CHANCSR_LLE | DT2821_CHANCSR_NUMB(n), > ^~~~ Arnd, Is this a gcc-6 specific issue? Seems line this warning should be showing up in a lot of drivers. > The warning makes sense, though the code is correct as far as I > can tell. > > This disambiguates the operation by making the constant expressions > we pass here explicitly 'unsigned', which helps to avoid the warning. > > As pointed out by Hartley Sweeten, scripts/checkpatch.pl notices that > the shifts here are rather unreadable, though the suggested BIT() > macro wouldn't work either. I'm changing it to a hexadecimal notation, > which hopefully improves readability. I'm leaving the DT2821_CHANCSR_PRESLA > alone because it seems wrong. BIT() should work for the ones pointed out by checpatch.pl. I would argue that the hexadecimal notation is still rather unreadable. These ones make my head hurt... -#define DT2821_ADCSR_GS(x) (((x) & 0x3) << 4) +#define DT2821_ADCSR_GS(x)(0x0030u & ((x) << 4)) -#define DT2821_DACSR_YSEL(x) ((x) << 9) +#define DT2821_DACSR_YSEL(x) (0x7e00u & (x) << 9) -#define DT2821_SUPCSR_DS_PIO (0 << 10) -#define DT2821_SUPCSR_DS_AD_CLK(1 << 10) -#define DT2821_SUPCSR_DS_DA_CLK(2 << 10) -#define DT2821_SUPCSR_DS_AD_TRIG (3 << 10) +#define DT2821_SUPCSR_DS_PIO (0x0c00u & (0u << 10)) +#define DT2821_SUPCSR_DS_AD_CLK (0x0c00u & (1u << 10)) +#define DT2821_SUPCSR_DS_DA_CLK (0x0c00u & (2u << 10)) +#define DT2821_SUPCSR_DS_AD_TRIG (0x0c00u & (3u << 10)) Also, most of the comedi drivers use the BIT() macro. Are you planning on changing all of them to use hexadecimal notation? Regards, Hartley
