Re: staging: mt7621-pci: factor out 'mt7621_pcie_enable_port' function
Hi Sergio, On 24/5/19 3:35 pm, Sergio Paracuellos wrote: On Fri, May 24, 2019 at 2:35 AM Greg Ungerer wrote: On 23/5/19 3:26 pm, Sergio Paracuellos wrote: On Thu, May 23, 2019 at 4:11 AM Greg Ungerer wrote: On 22/5/19 4:27 pm, Sergio Paracuellos wrote: [snip] There are some big changes between 4.20 and 5.x. One is the use of PERST_N instead of using gpio. This PERT_N stuff is used now on enable ports assuming the link of PCI is properly detected after enabling the phy. And it seems it is not according to your dmesg traces. The previous 4.20 code used gpio before this was done. This code is the one I am referring: /* Use GPIO control instead of PERST_N */ *(unsigned int *)(0xbe000620) |= BIT(19) | BIT(8) | BIT(7); // set DATA mdelay(1000); I have been looking closely at those, wondering why the old code drove that PERST line as a GPIO instead of using the built-in behavior. (I have ignored bits 7 and 8 here since they are control of UART 3) Yes, this was also at first one of my big concerns so I tried to change into to use builtin behaviour (which is much more cleaner) and when the code was tested it worked. It seems it is not valid for every board. I assume reset lines on your device tree are properly set up which is other of the big changes here: use reset lines instead of that hardcoding stuff. Also, the mt7621_reset_port routine is also using msleep(100) but maybe you can try a bigger value and change it into a mdelay, to see if that changes anything. I see the reset line configuration in the pcie section of mt7621.dtsi, is there any others absolutely required here? I couldn't see the gbpc1.dts devicetree do anything else with pcie - othe than enable it. My device tree for the EX15 is similar in that regard. I tried a couple of things with interesting results. 1. I made sure that the PERST_N line is set for PCIe operation (not GPIO). I forced it with: *(unsigned int *)(0xbe60) &= ~(0x3 << 10); I checked bits 10 and 11 at kernel PCI init and they were 00 anyway. So PERST_N was definitely in PCIe reset mode. No change in behavior, 2. I forced a GPIO style reset of that PERST line (using GPIO19) and got the following result on kernel boot: mt7621-pci 1e14.pcie: Port 454043648 N_FTS = 0 mt7621-pci-phy 1e149000.pcie-phy: Xtal is 40MHz mt7621-pci 1e14.pcie: Port 454043648 N_FTS = 1 mt7621-pci 1e14.pcie: pcie1 no card, disable it (RST & CLK) mt7621-pci 1e14.pcie: Initiating port 1 failed mt7621-pci 1e14.pcie: Port 454043648 N_FTS = 2 mt7621-pci-phy 1e14a000.pcie-phy: Xtal is 40MHz mt7621-pci 1e14.pcie: pcie2 no card, disable it (RST & CLK) mt7621-pci 1e14.pcie: Initiating port 2 failed mt7621-pci 1e14.pcie: de-assert port 0 PERST_N This line seems to be the problem. When ports are init, (and with your changes seems the are init properly), the ports with pcie link are stored into a list to be enabled afterwards. This code is located into 'mt7621_pcie_enable_ports' which call simple 'mt7621_pcie_enable_port' to enable each port on the list. In this process it uses the PERS_N built-in register deasserting and asserting it. If enabling fails (and this is ypour case now) the port is removed from the list and it is not properly set up. You should try to comment this code: /* assert port PERST_N */ val = pcie_read(pcie, RALINK_PCI_PCICFG_ADDR); val |= PCIE_PORT_PERST(slot); pcie_write(pcie, val, RALINK_PCI_PCICFG_ADDR); /* de-assert port PERST_N */ val = pcie_read(pcie, RALINK_PCI_PCICFG_ADDR); val &= ~PCIE_PORT_PERST(slot); pcie_write(pcie, val, RALINK_PCI_PCICFG_ADDR); /* 100ms timeout value should be enough for Gen1 training */ err = readl_poll_timeout(port->base + RALINK_PCI_STATUS, val, !!(val & PCIE_PORT_LINKUP), 20, 100 * USEC_PER_MSEC); if (err) return -ETIMEDOUT; because on enabling, it seems it is getting ETIMEOUT and hence the message ' mt7621-pci 1e14.pcie: de-assert port 0 PERST_N'. Commenting this code should end up into a properly configured pci? No, unfortunately it doesn't. It does show PCIE0 enabled now though: That is a surprise :( mt7621-pci 1e14.pcie: Port 454043648 N_FTS = 0 mt7621-pci-phy 1e149000.pcie-phy: Xtal is 40MHz mt7621-pci 1e14.pcie: Port 454043648 N_FTS = 1 mt7621-pci 1e14.pcie: pcie1 no card, disable it (RST & CLK) mt7621-pci 1e14.pcie: Initiating port 1 failed mt7621-pci 1e14.pcie: Port 454043648 N_FTS = 2 mt7621-pci-phy 1e14a000.pcie-phy: Xtal is 40MHz mt7621-pci 1e14.pcie: pcie2 no card, disable it (RST & CLK) mt7621-pci 1e14.pcie: Initiating port 2 failed mt7621-pci 1e14.pcie: PCIE0 enabled mt7621-pci 1e14.pcie: PCI coherence region base: 0x6000, mask/settings: 0xf002 mt7621-pci 1e14.pcie: PCI host bridge to bus :00 pci_bus :00: root bus resource [io 0x] pci_bus :00: root bus resource [mem 0x6000-0x6fff] pci_bus
Re: [PATCH net] staging: Remove set but not used variable ‘status’
On 2019/5/25 20:59, Sven Van Asbroeck wrote:
> On Sat, May 25, 2019 at 12:20 AM Mao Wenan wrote:
>>
>> The variable 'status' is not used any more, remve it.
>
>> /* do the transfers for this message */
>> list_for_each_entry(transfer, >transfers, transfer_list) {
>> if (transfer->tx_buf == NULL && transfer->rx_buf == NULL &&
>> transfer->len) {
>> -status = -EINVAL;
>> break;
>> }
>
> This looks like an error condition that's not reported to the spi core.
>
> Instead of removing the status variable (which also removes the error value!),
> maybe this should be reported to the spi core instead ?
>
> Other spi drivers appear to do the following on the error path:
> m->status = status;
> return status;
I have reviewed the code again, and it is good idea to store m->status in error
path, like below?
@@ -374,7 +374,7 @@ kp_spi_transfer_one_message(struct spi_master *master,
struct spi_message *m)
list_for_each_entry(transfer, >transfers, transfer_list) {
if (transfer->tx_buf == NULL && transfer->rx_buf == NULL &&
transfer->len) {
status = -EINVAL;
-break;
+goto error;
}
/* transfer */
@@ -412,7 +412,7 @@ kp_spi_transfer_one_message(struct spi_master *master,
struct spi_message *m)
if (count != transfer->len) {
status = -EIO;
-break;
+goto error;
}
}
...
/* done work */
spi_finalize_current_message(master);
return 0;
+
+ error:
+m->status = status;
+return status;
}
>
>>
>> @@ -370,7 +368,6 @@ kp_spi_transfer_one_message(struct spi_master *master,
>> struct spi_message *m)
>>
>> if (count != transfer->len) {
>> -status = -EIO;
>> break;
>
> Same issue here.
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] emxx_udc : Fix #if 0 coding style warning
This patch fixes the checkpathc.pl warning: WARNING: Consider removing the code enclosed by this #if 0 and its #endif Signed-off-by: Yacov Simhony --- drivers/staging/emxx_udc/emxx_udc.h | 6 ++ 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/staging/emxx_udc/emxx_udc.h b/drivers/staging/emxx_udc/emxx_udc.h index b8c3dee..985a1fb 100644 --- a/drivers/staging/emxx_udc/emxx_udc.h +++ b/drivers/staging/emxx_udc/emxx_udc.h @@ -10,10 +10,8 @@ /*---*/ /*- Default undef */ -#if 0 -#define DEBUG -#define UDC_DEBUG_DUMP -#endif +//#define DEBUG +//#define UDC_DEBUG_DUMP /*- Default define */ #defineUSE_DMA 1 -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] dt-bindings: iio: adc: add adi,ad7780.yaml binding
On Fri, 24 May 2019 21:28:02 -0300
Renato Lui Geh wrote:
> Hi Jonathan, Alex,
>
> Thanks for the review. Some comments inline.
>
> Thanks,
> Renato
>
> On 05/20, Ardelean, Alexandru wrote:
> >On Sun, 2019-05-19 at 12:32 +0100, Jonathan Cameron wrote:
> >> [External]
> >>
> >>
> >> On Sat, 18 May 2019 19:41:12 -0300
> >> Renato Lui Geh wrote:
> >>
> >> > This patch adds a YAML binding for the Analog Devices AD7780/1 and
> >> > AD7170/1 analog-to-digital converters.
> >> >
> >> > Signed-off-by: Renato Lui Geh
> >>
> >> One comment inline. I'll also be needing an ack from Analog on this,
> >> preferably Michael's.
> >>
> >> Thanks,
> >>
> >> Jonathan
> >> > ---
> >> > .../bindings/iio/adc/adi,ad7780.txt | 48 ---
> >> > .../bindings/iio/adc/adi,ad7780.yaml | 85 +++
> >
> >You should also update the MAINTAINERS file.
> >Maybe in a following patch.
> >It looks like there is not entry in there, so maybe you need to add a new
> >one.
> >
> >Something like:
> >
> >
> >ANALOG DEVICES INC AD7780 DRIVER
> >M: Michael Hennerich
> >M: Renato Lui Geh
> >L: linux-...@vger.kernel.org
> >W: http://ez.analog.com/community/linux-device-drivers
> >S: Supported
> >F: drivers/iio/adc/ad7780.c
> >F: Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> >
> >This should be after this block
> >ANALOG DEVICES INC AD7768-1 DRIVER
> >
> >Note that I added you as a co-maintainer.
> >If you want, you do not need to add that line.
> >
> >> > 2 files changed, 85 insertions(+), 48 deletions(-)
> >> > delete mode 100644
> >> > Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> >> > create mode 100644
> >> > Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> >> > b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> >> > deleted file mode 100644
> >> > index 440e52555349..
> >> > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> >> > +++ /dev/null
> >> > @@ -1,48 +0,0 @@
> >> > -* Analog Devices AD7170/AD7171/AD7780/AD7781
> >> > -
> >> > -Data sheets:
> >> > -
> >> > -- AD7170:
> >> > - *
> >> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> >> > -- AD7171:
> >> > - *
> >> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
> >> > -- AD7780:
> >> > - *
> >> > https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
> >> > -- AD7781:
> >> > - *
> >> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
> >> > -
> >> > -Required properties:
> >> > -
> >> > -- compatible: should be one of
> >> > - * "adi,ad7170"
> >> > - * "adi,ad7171"
> >> > - * "adi,ad7780"
> >> > - * "adi,ad7781"
> >> > -- reg: spi chip select number for the device
> >> > -- vref-supply: the regulator supply for the ADC reference voltage
> >> > -
> >> > -Optional properties:
> >> > -
> >> > -- powerdown-gpios: must be the device tree identifier of the PDRST
> >> > pin. If
> >> > - specified, it will be asserted during driver probe.
> >> > As the
> >> > - line is active high, it should be marked
> >> > GPIO_ACTIVE_HIGH.
> >> > -- adi,gain-gpios: must be the device tree identifier of the GAIN
> >> > pin. Only for
> >> > - the ad778x chips. If specified, it will be asserted
> >> > during
> >> > - driver probe. As the line is active low, it should be
> >> > marked
> >> > - GPIO_ACTIVE_LOW.
> >> > -- adi,filter-gpios: must be the device tree identifier of the FILTER
> >> > pin. Only
> >> > - for the ad778x chips. If specified, it will be
> >> > asserted
> >> > - during driver probe. As the line is active low, it
> >> > should be
> >> > - marked GPIO_ACTIVE_LOW.
> >> > -
> >> > -Example:
> >> > -
> >> > -adc@0 {
> >> > - compatible = "adi,ad7780";
> >> > - reg = <0>;
> >> > - vref-supply = <_supply>
> >> > -
> >> > - powerdown-gpios = < 12 GPIO_ACTIVE_HIGH>;
> >> > - adi,gain-gpios = < 5 GPIO_ACTIVE_LOW>;
> >> > - adi,filter-gpios = < 15 GPIO_ACTIVE_LOW>;
> >> > -};
> >> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> >> > b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> >> > new file mode 100644
> >> > index ..931bc4f8ec04
> >> > --- /dev/null
> >> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> >> > @@ -0,0 +1,85 @@
> >> > +# SPDX-License-Identifier: GPL-2.0
> >> > +%YAML 1.2
> >> > +---
> >> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7780.yaml#
> >> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> > +
> >> > +title: Analog Devices AD7170/AD7171/AD7780/AD7781 analog to digital
> >> > converters
> >> > +
> >> > +maintainers:
> >> > + -
Re: [PATCH v2 1/2] dt-bindings: iio: adc: add adi,ad7780.yaml binding
On Fri, 24 May 2019 22:26:30 -0300
Renato Lui Geh wrote:
> This patch adds a YAML binding for the Analog Devices AD7780/1 and
> AD7170/1 analog-to-digital converters.
>
> Signed-off-by: Renato Lui Geh
Looks good to me, but I'm still finding my feet with these so will
leave it for a few days for others to have time to comment.
Michael, looking for a quick reply from you to say if you are happy
being explicitly listed as maintainer for this one, or if you'd
rather land it on someone else. Same applies for patch 2.
Renato, if I seem to have forgotten this in a week or so, feel
free to give me a poke. I've been known to loose patches entirely!
Thanks,
Jonathan
> ---
> Changes in v2:
> - vref-supply to avdd-supply
> - remove avdd-supply from required list
> - include adc block in an spi block
>
> .../bindings/iio/adc/adi,ad7780.txt | 48 --
> .../bindings/iio/adc/adi,ad7780.yaml | 87 +++
> 2 files changed, 87 insertions(+), 48 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> deleted file mode 100644
> index 440e52555349..
> --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.txt
> +++ /dev/null
> @@ -1,48 +0,0 @@
> -* Analog Devices AD7170/AD7171/AD7780/AD7781
> -
> -Data sheets:
> -
> -- AD7170:
> - *
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> -- AD7171:
> - *
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
> -- AD7780:
> - *
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
> -- AD7781:
> - *
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
> -
> -Required properties:
> -
> -- compatible: should be one of
> - * "adi,ad7170"
> - * "adi,ad7171"
> - * "adi,ad7780"
> - * "adi,ad7781"
> -- reg: spi chip select number for the device
> -- vref-supply: the regulator supply for the ADC reference voltage
> -
> -Optional properties:
> -
> -- powerdown-gpios: must be the device tree identifier of the PDRST pin. If
> - specified, it will be asserted during driver probe. As the
> - line is active high, it should be marked GPIO_ACTIVE_HIGH.
> -- adi,gain-gpios: must be the device tree identifier of the GAIN pin. Only
> for
> - the ad778x chips. If specified, it will be asserted during
> - driver probe. As the line is active low, it should be marked
> - GPIO_ACTIVE_LOW.
> -- adi,filter-gpios: must be the device tree identifier of the FILTER pin.
> Only
> - for the ad778x chips. If specified, it will be asserted
> - during driver probe. As the line is active low, it should be
> - marked GPIO_ACTIVE_LOW.
> -
> -Example:
> -
> -adc@0 {
> - compatible = "adi,ad7780";
> - reg = <0>;
> - vref-supply = <_supply>
> -
> - powerdown-gpios = < 12 GPIO_ACTIVE_HIGH>;
> - adi,gain-gpios = < 5 GPIO_ACTIVE_LOW>;
> - adi,filter-gpios = < 15 GPIO_ACTIVE_LOW>;
> -};
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> new file mode 100644
> index ..d1109416963c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7780.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7170/AD7171/AD7780/AD7781 analog to digital
> converters
> +
> +maintainers:
> + - Michael Hennerich
> +
> +description: |
> + The ad7780 is a sigma-delta analog to digital converter. This driver
> provides
> + reading voltage values and status bits from both the ad778x and ad717x
> series.
> + Its interface also allows writing on the FILTER and GAIN GPIO pins on the
> + ad778x.
> +
> + Specifications on the converters can be found at:
> +AD7170:
> +
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7170.pdf
> +AD7171:
> +
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7171.pdf
> +AD7780:
> +
> https://www.analog.com/media/en/technical-documentation/data-sheets/ad7780.pdf
> +AD7781:
> +
> https://www.analog.com/media/en/technical-documentation/data-sheets/AD7781.pdf
> +
> +properties:
> + compatible:
> +enum:
> + - adi,ad7170
> + - adi,ad7171
> + - adi,ad7780
> + - adi,ad7781
> +
> + reg:
> +maxItems: 1
> +
> + avdd-supply:
> +description:
> + The regulator supply for the ADC
Re: [PATCH] staging: iio: adis16240: add of_match_table entry
On Fri, 24 May 2019 13:54:49 +
"Ardelean, Alexandru" wrote:
> On Fri, 2019-05-24 at 10:50 -0300, Marcelo Schmitt wrote:
> > [External]
> >
> >
> > Hi Alexandru,
> >
> > On 05/24, Alexandru Ardelean wrote:
> > > On Fri, May 24, 2019 at 6:30 AM Rodrigo Ribeiro
> > > wrote:
> > > >
> > > > This patch adds of_match_table entry in device driver in order to
> > > > enable spi fallback probing.
> > > >
> > > > Signed-off-by: Rodrigo Ribeiro
> > > > Reviewed-by: Marcelo Schmitt
> > > > ---
> > > > drivers/staging/iio/accel/adis16240.c | 1 +
> > > > 1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/staging/iio/accel/adis16240.c
> > > > b/drivers/staging/iio/accel/adis16240.c
> > > > index 8c6d23604eca..b80c8529784b 100644
> > > > --- a/drivers/staging/iio/accel/adis16240.c
> > > > +++ b/drivers/staging/iio/accel/adis16240.c
> > > > @@ -444,6 +444,7 @@ MODULE_DEVICE_TABLE(of, adis16240_of_match);
> > > > static struct spi_driver adis16240_driver = {
> > > > .driver = {
> > > > .name = "adis16240",
> > > > + .of_match_table = adis16240_of_match,
> > >
> > > This patch is missing the actual table.
> >
> > Struct with compatible devices table was included separately in a
> > previous patch at commit d9e533b6c0a26c7ef8116b7f3477c164c07bb6fb.
> > Yeah, I also thought it was missing the match table the first time I was
> > this patch. It's really confusing when we have two patches, one
> > depending on another, that are not part of the same patch set. We're
> > trying to avoid things like this the most but that slipped out from our
> > internal review. We're sorry about that.
>
> No worries.
>
> It happens to me too.
>
> Thanks
> Alex
Oops. I should have caught that one in review as well.
Oh well, these things happen.
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it. I rebased the tree to pick up on
all the other stuff in staging/staging-next after the mere window.
Thanks,
Jonathan
>
> >
> > >
> > > > },
> > > > .probe = adis16240_probe,
> > > > .remove = adis16240_remove,
> > > > --
> > > > 2.20.1
> > > >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: kpc2000: simplify nested conditionals that just return a boolean.
On 2019-05-24, at 16:59:45 +, Matt Sickler wrote:
> From: devel On Behalf Of
> Greg KH
> > On Fri, May 24, 2019 at 01:19:26PM +0100, Jeremy Sowden wrote:
> > > kp2000_check_uio_irq contained a pair of nested ifs which each
> > > evaluated a bitwise operation. If both operations yielded true,
> > > the function returned 1; otherwise it returned 0.
> > >
> > > Replaced the whole thing with one return statement that evaluates
> > > the combination of both bitwise operations.
> >
> > Does this really do the same thing?
> >
> > > Signed-off-by: Jeremy Sowden
> > > ---
> > > This applies to staging-testing.
> > >
> > > I was in two minds whether to send this patch or something less terse:
> > >
> > > + return (interrupt_active & irq_check_mask) &&
> > > +(interrupt_mask_inv & irq_check_mask);
> > >
> > > drivers/staging/kpc2000/kpc2000/cell_probe.c | 10 --
> > > 1 file changed, 4 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > index f731a97c6cac..d10f695b6673 100644
> > > --- a/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > +++ b/drivers/staging/kpc2000/kpc2000/cell_probe.c
> > > @@ -240,12 +240,10 @@ int kp2000_check_uio_irq(struct kp2000_device
> > > *pcard, u32 irq_num)
> > > u64 interrupt_mask_inv = ~readq(pcard-> sysinfo_regs_base +
> > > REG_INTERRUPT_MASK);
> > > u64 irq_check_mask = (1 << irq_num);
> > >
> > > - if (interrupt_active & irq_check_mask) { // if it's active
> > > (interrupt pending)
> > > - if (interrupt_mask_inv & irq_check_mask) {// and if
> > > it's not masked off
> > > - return 1;
> > > - }
> > > - }
> > > - return 0;
> > > + /*
> > > + * Is the IRQ active (interrupt pending) and not masked off?
> > > + */
> > > + return (interrupt_active & interrupt_mask_inv & irq_check_mask) !=
> > > 0;
> >
> > I have no idea what these bits really are, but try the above with
> > the following values:
>
> interrupt_active indicates which IRQs are active/pending
> 0 = not pending
> 1 = pending
>
> irq_check_mask has only a single bit set which is which IRQ we're
> testing for Each one is "associated" with one of the UIO "cores" (see
> core_probe.c for how that association is discovered).
>
> interrupt_mask_inv is a bitwise inversion of the HW register. The HW
> register tells the HW which interrupts to ignore. In the HW register:
> 0 = pass this IRQ up to the host.
> 1 = mask the IRQ
> In interrupt_mask_inv:
> 0 = ignore this IRQ
> 1 = care about this IRQ
>
> This code is checking 3 things:
> - Is there an interrupt for this UIO core
> - Is that interrupt signaled
> - Is the interrupt not masked
>
> Just in case it's not obvious yet: the mask/pending bits allow the HW
> to catch an interrupt but not signal the host until the interrupt is
> unmasked. This allows interrupts that happen while the IRQ is masked
> to still be handled once the IRQ is un-masked.
>
> >interrupt_active = BIT(0);
> >interrupt_mask_inv = BIT(1);
> >irq_check_mask = (BIT(1) | BIT(0));
> >
> > So in your new code you have:
> >BIT(0) & BIT(1) & (BIT(1) | BIT(0))
> > which reduces to:
> >0 & (BIT(1) | BIT(0))
> > which then reduces to:
> >0
> >
> > The original if statements will return 1.
> > Your new one will return 0.
> >
> > You can't AND interrupt_active with interrupt_mask_inv like you did
> > here, right?
> >
> > Or am I reading this all wrong?
As Matt explained above, irq_check_mask only ever has one bit set:
u64 irq_check_mask = (1 << irq_num);
So:
interrupt_mask_inv & irq_check_mask
yields irq_check_mask or 0, and:
interrupt_active & irq_check_mask
yields irq_check_mask or 0, and:
interrupt_mask_inv & interrupt_active & irq_check_mask
yields irq_check_mask or 0.
> > And what's wrong with the original code here? What is complaining about
> > it (other than the crazy comment style...)?
>
> I would agree with Greg, if there's nothing complaining about the way
> the original code was written it should probably be left that way.
> This new way seems overly tricky, even if it was proven to do the same
> thing.
This patch was originally part of a larger series of white-space and
formatting changes to cell_probe.c that I put to one side. It started
out as a change to the formatting of the comments, IIRC. I was reminded
of it while looking over the recent series by Simon Sandström (which
have fixed most of the issues that my series would have addressed).
Conditional blocks that contain nothing but statements returing boolean
constants are a (minor) pet peeve of mine, so I sent the patch by
itself. Clearly it's not a peeve shared by many people. :)
Happy to drop the patch.
J.
signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
[PATCH] drivers: staging: speakup: serialio: fix warning linux/serial.h is included more than once
fix below warning reported by includecheck ./drivers/staging/speakup/serialio.h: linux/serial.h is included more than once. Signed-off-by: Hariprasad Kelam --- drivers/staging/speakup/serialio.h | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/staging/speakup/serialio.h b/drivers/staging/speakup/serialio.h index aa691e4..6f8f86f 100644 --- a/drivers/staging/speakup/serialio.h +++ b/drivers/staging/speakup/serialio.h @@ -4,9 +4,6 @@ #include /* for rs_table, serial constants */ #include /* for more serial constants */ -#ifndef __sparc__ -#include -#endif #include #include "spk_priv.h" -- 2.7.4 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
