RE: [PATCH v3 0/6] iio: Add output buffer support
Hi Jonathan and others, With output/dac buffer support the semantics of the scan_element type may change. Today the Format is [be|le]:[s|u]bits/storagebitsXrepeat[>>shift]. While shift (if specified) is the shift that needs to be applied prior to masking out unused bits. So far so good and it sounds universal. However, we use the right shift (operator) for that, which makes sense for capture devices. For output devices the more logical operator would be the left shift. I'm not proposing a new Format here. I just want to get some agreement that for an output device le:s12/16>>4 is understood as a left shift of 4, since the unused bits are then on the LSB. Thoughts? Best Regards, Michael Analog Devices GmbH Michael Hennerich Otl-Aicher Strasse 60-64 D-80807 Muenchen; Germany Sitz der Gesellschaft München, Registergericht München HRB 40368, Geschäftsführer: Stefan Steyerl, Michael Paul Sondel, Yoon Ah Oh
RE: [PATCH] MAINTAINERS: replace my with email with replacements
> -Original Message- > From: Alexandru Ardelean > Sent: Mittwoch, 10. Februar 2021 12:01 > To: linux-kernel@vger.kernel.org > Cc: gre...@linuxfoundation.org; Hennerich, Michael > ; Sa, Nuno ; > Ardelean, Alexandru > Subject: [PATCH] MAINTAINERS: replace my with email with replacements > > This email will become inactive in a few weeks. > This change removes it from the MAINTAINERS file and adds the people that > will be responsible for the parts moving forward. > > Signed-off-by: Alexandru Ardelean Acked-by: Michael Hennerich > --- > MAINTAINERS | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index e697044d34d6..97a91fd7948d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1016,7 +1016,7 @@ F: > Documentation/devicetree/bindings/mux/adi,adgs1408.txt > F: drivers/mux/adgs1408.c > > ANALOG DEVICES INC ADIN DRIVER > -M: Alexandru Ardelean > +M: Michael Hennerich > L: net...@vger.kernel.org > S: Supported > W: http://ez.analog.com/community/linux-device-drivers > @@ -1024,7 +1024,7 @@ F: > Documentation/devicetree/bindings/net/adi,adin.yaml > F: drivers/net/phy/adin.c > > ANALOG DEVICES INC ADIS DRIVER LIBRARY > -M: Alexandru Ardelean > +M: Nuno Sa > L: linux-...@vger.kernel.org > S: Supported > F: drivers/iio/imu/adis.c > -- > 2.17.1
RE: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel modes
> -Original Message- > From: Andy Shevchenko > Sent: Mittwoch, 2. September 2020 10:52 > To: Hennerich, Michael > Cc: AceLan Kao ; Ardelean, Alexandru > ; William Sung > ; Lars-Peter Clausen ; > Jonathan Cameron ; Hartmut Knaack ; > Peter Meerwald-Stadler ; linux-iio i...@vger.kernel.org>; Linux Kernel Mailing List > ; > Campion Kang > Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel > modes > > [External] > > On Wed, Sep 2, 2020 at 11:09 AM Hennerich, Michael > wrote: > > > From: Andy Shevchenko > > > Sent: Montag, 31. August 2020 14:46 > > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao > > > > > > wrote: > > ... > > > > P.S. Jonathan, it seems this driver has artificial ACPI HID. We > > > probably have to remove it. However, ADS is indeed reserved for Analog > Devices in PNP registry. > > > Can we have AD's official answer on this? > > > Cc'ing additional AD people. > > > > Agreed, this ID was chosen under the PNP ID Vendor Space for Analog Devices > Inc. > > Days back, I did a quick kernel grep, and there are many drivers which > > use the 3-letter PNP ID as acpi_device_id. So, I thought this being not an > > issue. > > No, no, the use of PNP ID is not an issue. The point is if the ID is > artificial or > official. > > > I'm against removing it since I know people shipping this in their ACPI > > tables > already. > > I see. Can we consider this email as the official answer from AD that this ID > is > being allocated for this certain component? To my knowledge only PNP/ACPI vendor IDs need to be registered with the UEFI ACPI working group. AD part numbers are unique. The ID chosen uses the part number prefixed with the PNP Vendor ID. AD->ADS Please consider this as allocated. > > > Regarding ACPI DSD customization, one way to do this is to move this into > > the > BIOS. > > This way the particular piece of HW can be customized rather than > > manage HW connections in software. > > Assuming the confirmation on the above, indeed, one may use ACPI HID with > DSD properties in the firmware. Main purpose of PRP0001 is the > *development* stage of the product at which a vendor should take care about > allocation of proper ACPI IDs for the components in use. Yes, I know that > this is > not always feasible b/c some HW component vendors don't care about ACPI at > all. > > -- > With Best Regards, > Andy Shevchenko Best Regards, Michael Analog Devices GmbH Michael Hennerich Otl-Aicher Strasse 60-64 D-80807 Muenchen; Germany mail: michael.henner...@analog.com http://www.analog.com Sitz der Gesellschaft München, Registergericht München HRB 40368, Geschäftsführer: Stefan Steyerl, Thomas Edward Cribben, Michael Paul Sondel
RE: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel modes
> -Original Message- > From: Andy Shevchenko > Sent: Montag, 31. August 2020 14:46 > To: AceLan Kao ; Ardelean, Alexandru > > Cc: William Sung ; Lars-Peter Clausen > ; Hennerich, Michael ; > Jonathan Cameron ; Hartmut Knaack ; > Peter Meerwald-Stadler ; linux-iio i...@vger.kernel.org>; Linux Kernel Mailing List > ; > Campion Kang > Subject: Re: [PATCH] iio: dac: ad5593r: Dynamically set AD5593R channel > modes > > On Mon, Aug 31, 2020 at 2:28 PM AceLan Kao > wrote: > > This patch is mainly for Advantech's UNO-420[1] which is a x86-based > platform. > > This platform is more like a development platform for customers to > > customize their products, so, specify the channel modes in ACPI table > > is not generic enough, that's why William submit this patch. > > > > Are there other ways to specify or pass values to the module without > > using module parameters? > > It's good if we can leverage sysfs, but I don't know if there is one > > for this scenario. > > Can we provide DT bindings for that and use then in ACPI? ACPI has a > possibility > to reuse DT properties and compatible strings [1]. As far as I can see the > driver > uses fwnode API, so it supports ACPI case already [2]. So, what prevents you > to > utilize 'adi,mode' property? > > Also, we accept examples of ASL excerpt in meta-acpi project [3]. It has > already > plenty of examples [4] how to use PRP0001 for DIY / development boards. > > So, take all together I think this patch is simple redundant. > > [1]: > https://urldefense.com/v3/__https://www.kernel.org/doc/html/latest/firmwar > e-guide/acpi/enumeration.html*device-tree-namespace-link-device- > id__;Iw!!A3Ni8CS0y2Y!oe0bVwE- > D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efojRqroyg$ > [2]: https://urldefense.com/v3/__https://elixir.bootlin.com/linux/v5.9- > rc3/source/Documentation/devicetree/bindings/iio/dac/ad5592r.txt__;!!A3Ni8 > CS0y2Y!oe0bVwE- > D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efr_TTE4CQ > $ > [3]: https://urldefense.com/v3/__https://github.com/westeri/meta- > acpi__;!!A3Ni8CS0y2Y!oe0bVwE- > D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efpZnQjgBg > $ > [4]: https://urldefense.com/v3/__https://github.com/westeri/meta- > acpi/tree/master/recipes-bsp/acpi-tables/samples__;!!A3Ni8CS0y2Y!oe0bVwE- > D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYmQ3SvTvEn8OyjaL1efqpzDeR7A > $ > > P.S. Jonathan, it seems this driver has artificial ACPI HID. We probably have > to > remove it. However, ADS is indeed reserved for Analog Devices in PNP registry. > Can we have AD's official answer on this? > Cc'ing additional AD people. Agreed, this ID was chosen under the PNP ID Vendor Space for Analog Devices Inc. Days back, I did a quick kernel grep, and there are many drivers which use the 3-letter PNP ID as acpi_device_id. So, I thought this being not an issue. I'm against removing it since I know people shipping this in their ACPI tables already. Regarding ACPI DSD customization, one way to do this is to move this into the BIOS. This way the particular piece of HW can be customized rather than manage HW connections in software. Sorry for the misformatted email reply. > > > 1. > > > https://urldefense.com/v3/__https://www.advantech.com/products/9a0cc56 > > 1-8fc2-4e22-969c-9df90a3952b5/uno-420/mod_2d6a546b-39e3-4bc4- > bbf4-ac89 > > e6b7667c__;!!A3Ni8CS0y2Y!oe0bVwE- > D8Y6QiWYU06pwxclGSgFpFQ10Rdozy5pCKZYm > > Q3SvTvEn8OyjaL1efp-eZqbcQ$ > > > -- > With Best Regards, > Andy Shevchenko Best Regards, Michael Analog Devices GmbH Michael Hennerich Otl-Aicher Strasse 60-64 D-80807 Muenchen; Germany mail: michael.henner...@analog.com http://www.analog.com Sitz der Gesellschaft München, Registergericht München HRB 40368, Geschäftsführer: Stefan Steyerl, Thomas Edward Cribben, Michael Paul Sondel
RE: [PATCH] ieee802154/adf7242: check status of adf7242_read_reg
> -Original Message-
> From: t...@redhat.com
> Sent: Sonntag, 2. August 2020 16:24
> To: Hennerich, Michael ;
> alex.ar...@gmail.com; ste...@datenfreihafen.org; da...@davemloft.net;
> k...@kernel.org; mar...@holtmann.org
> Cc: linux-w...@vger.kernel.org; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Tom Rix
> Subject: [PATCH] ieee802154/adf7242: check status of adf7242_read_reg
>
>
> From: Tom Rix
>
> Clang static analysis reports this error
>
> adf7242.c:887:6: warning: Assigned value is garbage or undefined
> len = len_u8;
> ^ ~~
>
> len_u8 is set in
>adf7242_read_reg(lp, 0, &len_u8);
>
> When this call fails, len_u8 is not set.
>
> So check the return code.
>
> Fixes: 7302b9d90117 ("ieee802154/adf7242: Driver for ADF7242 MAC
> IEEE802154")
>
> Signed-off-by: Tom Rix
Acked-by: Michael Hennerich
> ---
> drivers/net/ieee802154/adf7242.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/adf7242.c
> b/drivers/net/ieee802154/adf7242.c
> index c11f32f644db..7db9cbd0f5de 100644
> --- a/drivers/net/ieee802154/adf7242.c
> +++ b/drivers/net/ieee802154/adf7242.c
> @@ -882,7 +882,9 @@ static int adf7242_rx(struct adf7242_local *lp)
> int ret;
> u8 lqi, len_u8, *data;
>
> - adf7242_read_reg(lp, 0, &len_u8);
> + ret = adf7242_read_reg(lp, 0, &len_u8);
> + if (ret)
> + return ret;
>
> len = len_u8;
>
> --
> 2.18.1
RE: [PATCH v2 1/2] dt-bindings: iio: adc: add adi,ad7780.yaml binding
> -Original Message-
> From: Jonathan Cameron [mailto:ji...@kernel.org]
> Sent: Sonntag, 26. Mai 2019 18:39
> To: Renato Lui Geh
> Cc: l...@metafoo.de; Hennerich, Michael ;
> knaac...@gmx.de; pme...@pmeerw.net;
> gre...@linuxfoundation.org; Popa, Stefan Serban
> ; Ardelean, Alexandru
> ; robh...@kernel.org; mark.rutl...@arm.com;
> linux-...@vger.kernel.org;
> de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org;
> kernel-...@googlegroups.com; devicet...@vger.kernel.org
> Subject: 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.
Hi Jonathan,
Listing me as an maintainer is ok.
Acked-by: Michael Hennerich
>
> 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 = <&vdd_supply>
> > -
> > - powerdown-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;
> > - adi,gain-gpios = <&gpio 5 GPIO_ACTIVE_LOW>;
> > - adi,filter-gpios = <&gpio 15 GPIO_ACTIVE_LOW>;
> > -};
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7780.yam
RE: [PATCH] gpio: fix gpio-adp5588 build errors
> -Original Message-
> From: Randy Dunlap [mailto:rdun...@infradead.org]
> Sent: Freitag, 24. Mai 2019 00:01
> To: LKML ; linux-g...@vger.kernel.org
> Cc: kbuild test robot ; Hennerich, Michael
> ; Linus Walleij
> ; Bartosz Golaszewski
> Subject: [PATCH] gpio: fix gpio-adp5588 build errors
>
> From: Randy Dunlap
>
> The gpio-adp5588 driver uses interfaces that are provided by
> GPIOLIB_IRQCHIP, so select that symbol in its Kconfig entry.
>
> Fixes these build errors:
>
> ../drivers/gpio/gpio-adp5588.c: In function ‘adp5588_irq_handler’:
> ../drivers/gpio/gpio-adp5588.c:266:26: error: ‘struct gpio_chip’ has no
> member named ‘irq’
> dev->gpio_chip.irq.domain, gpio));
> ^
> ../drivers/gpio/gpio-adp5588.c: In function ‘adp5588_irq_setup’:
> ../drivers/gpio/gpio-adp5588.c:298:2: error: implicit declaration of function
> ‘gpiochip_irqchip_add_nested’ [-Werror=implicit-
> function-declaration]
> ret = gpiochip_irqchip_add_nested(&dev->gpio_chip,
> ^
> ../drivers/gpio/gpio-adp5588.c:307:2: error: implicit declaration of function
> ‘gpiochip_set_nested_irqchip’ [-Werror=implicit-
> function-declaration]
> gpiochip_set_nested_irqchip(&dev->gpio_chip,
> ^
>
> Fixes: 459773ae8dbb ("gpio: adp5588-gpio: support interrupt controller")
>
> Reported-by: kbuild test robot
> Signed-off-by: Randy Dunlap
Acked-by: Michael Hennerich
> Cc: Michael Hennerich
> Cc: Linus Walleij
> Cc: Bartosz Golaszewski
> Cc: linux-g...@vger.kernel.org
> ---
> drivers/gpio/Kconfig |1 +
> 1 file changed, 1 insertion(+)
>
> --- lnx-52-rc1.orig/drivers/gpio/Kconfig
> +++ lnx-52-rc1/drivers/gpio/Kconfig
> @@ -822,6 +822,7 @@ config GPIO_ADP5588
> config GPIO_ADP5588_IRQ
> bool "Interrupt controller support for ADP5588"
> depends on GPIO_ADP5588=y
> + select GPIOLIB_IRQCHIP
> help
> Say yes here to enable the adp5588 to be used as an interrupt
> controller. It requires the driver to be built in the kernel.
>
RE: [PATCH] MAINTAINERS: normalize Michael Hennerich's email address
> -Original Message- > From: Lukas Bulwahn [mailto:lukas.bulw...@gmail.com] > Sent: Samstag, 13. April 2019 11:26 > To: Hennerich, Michael > Cc: linux-kernel@vger.kernel.org; Lukas Bulwahn > Subject: [PATCH] MAINTAINERS: normalize Michael Hennerich's email address > > MAINTAINERS contains a lower-case and upper-case variant of > Michael Hennerich' s email address. > > Only keep the lower-case variant in MAINTAINERS. > > Signed-off-by: Lukas Bulwahn Acked-by: Michael Hennerich > --- > MAINTAINERS | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/MAINTAINERS b/MAINTAINERS > index afa019b5c461..b486b9e5d73c 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -949,7 +949,7 @@ F: drivers/dma/dma-axi-dmac.c > > ANALOG DEVICES INC IIO DRIVERS > M: Lars-Peter Clausen > -M: Michael Hennerich > +M: Michael Hennerich > W: http://wiki.analog.com/ > W: http://ez.analog.com/community/linux-device-drivers > S: Supported > -- > 2.17.1
RE: [PATCH] staging: iio: ad5933: move out of staging
> -Original Message- > From: Marcelo Schmitt [mailto:marcelo.schmi...@gmail.com] > Sent: Montag, 11. Februar 2019 16:43 > To: l...@metafoo.de; Hennerich, Michael ; > ji...@kernel.org; knaac...@gmx.de; > pme...@pmeerw.net > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > kernel-...@googlegroups.com > Subject: [PATCH] staging: iio: ad5933: move out of staging > > Move ad5933 impedance-analyzer driver from staging to mainline. > > The ad5933 is a high precision impedance converter system > solution that combines an on-board frequency generator with an > analog-to-digital converter (ADC). This driver was designed to be > compatible with both ad5933 and ad5934 chips. > > Signed-off-by: Marcelo Schmitt Acked-by: Michael Hennerich > --- > drivers/iio/impedance-analyzer/Kconfig| 18 + > drivers/iio/impedance-analyzer/Makefile | 5 + > drivers/iio/impedance-analyzer/ad5933.c | 810 ++ > .../staging/iio/impedance-analyzer/Kconfig| 18 - > .../staging/iio/impedance-analyzer/Makefile | 5 - > .../staging/iio/impedance-analyzer/ad5933.c | 809 - > 6 files changed, 833 insertions(+), 832 deletions(-) > create mode 100644 drivers/iio/impedance-analyzer/Kconfig > create mode 100644 drivers/iio/impedance-analyzer/Makefile > create mode 100644 drivers/iio/impedance-analyzer/ad5933.c > delete mode 100644 drivers/staging/iio/impedance-analyzer/Kconfig > delete mode 100644 drivers/staging/iio/impedance-analyzer/Makefile > delete mode 100644 drivers/staging/iio/impedance-analyzer/ad5933.c > > diff --git a/drivers/iio/impedance-analyzer/Kconfig > b/drivers/iio/impedance-analyzer/Kconfig > new file mode 100644 > index ..dd97b6bb3fd0 > --- /dev/null > +++ b/drivers/iio/impedance-analyzer/Kconfig > @@ -0,0 +1,18 @@ > +# > +# Impedance Converter, Network Analyzer drivers > +# > +menu "Network Analyzer, Impedance Converters" > + > +config AD5933 > + tristate "Analog Devices AD5933, AD5934 driver" > + depends on I2C > + select IIO_BUFFER > + select IIO_KFIFO_BUF > + help > + Say yes here to build support for Analog Devices Impedance > Converter, > + Network Analyzer, AD5933/4, provides direct access via sysfs. > + > + To compile this driver as a module, choose M here: the > + module will be called ad5933. > + > +endmenu > diff --git a/drivers/iio/impedance-analyzer/Makefile > b/drivers/iio/impedance-analyzer/Makefile > new file mode 100644 > index ..7604d786583e > --- /dev/null > +++ b/drivers/iio/impedance-analyzer/Makefile > @@ -0,0 +1,5 @@ > +# > +# Makefile for Impedance Converter, Network Analyzer drivers > +# > + > +obj-$(CONFIG_AD5933) += ad5933.o > diff --git a/drivers/iio/impedance-analyzer/ad5933.c > b/drivers/iio/impedance-analyzer/ad5933.c > new file mode 100644 > index ..839bc30682e4 > --- /dev/null > +++ b/drivers/iio/impedance-analyzer/ad5933.c > @@ -0,0 +1,810 @@ > +/* > + * AD5933 AD5934 Impedance Converter, Network Analyzer > + * > + * Copyright 2011 Analog Devices Inc. > + * > + * Licensed under the GPL-2. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#include > + > +/* AD5933/AD5934 Registers */ > +#define AD5933_REG_CONTROL_HB 0x80/* R/W, 1 byte */ > +#define AD5933_REG_CONTROL_LB 0x81/* R/W, 1 byte */ > +#define AD5933_REG_FREQ_START 0x82/* R/W, 3 bytes */ > +#define AD5933_REG_FREQ_INC0x85/* R/W, 3 bytes */ > +#define AD5933_REG_INC_NUM 0x88/* R/W, 2 bytes, 9 bit */ > +#define AD5933_REG_SETTLING_CYCLES 0x8A/* R/W, 2 bytes */ > +#define AD5933_REG_STATUS 0x8F/* R, 1 byte */ > +#define AD5933_REG_TEMP_DATA 0x92/* R, 2 bytes*/ > +#define AD5933_REG_REAL_DATA 0x94/* R, 2 bytes*/ > +#define AD5933_REG_IMAG_DATA 0x96/* R, 2 bytes*/ > + > +/* AD5933_REG_CONTROL_HB Bits */ > +#define AD5933_CTRL_INIT_START_FREQ(0x1 << 4) > +#define AD5933_CTRL_START_SWEEP(0x2 << 4) > +#define AD5933_CTRL_INC_FREQ (0x3 << 4) > +#define AD5933_CTRL_REPEAT_FREQ(0x4 << 4) > +#define AD5933_CTRL_MEASURE_TEMP (0x9 << 4) > +#define AD5933_CTRL_POWER_DOWN (0xA << 4) > +#define AD5933_CTRL_STANDBY(0xB << 4) > + > +#define AD5933_CTRL_RAN
RE: [PATCH 1/2] drivers/gpio/gpio-adp5588.c: add device tree support
> -Original Message-
> From: Nikolaus Voss [mailto:n...@vosn.de]
> Sent: Mittwoch, 25. Juli 2018 10:44
> To: Hennerich, Michael ; Linus Walleij
>
> Cc: linux-g...@vger.kernel.org; linux-kernel@vger.kernel.org; Nikolaus Voss
>
> Subject: [PATCH 1/2] drivers/gpio/gpio-adp5588.c: add device tree support
>
> Make platform data optional and add DT id table.
> Switch to dynamically mapped GPIOs and IRQs if not provided
> via platform data.
Long overdue maintenance patch...
Looks good to me.
Thanks for this series!
>
> Signed-off-by: Nikolaus Voss
Acked-by: Michael Hennerich
> ---
> drivers/gpio/gpio-adp5588.c | 151
> 1 file changed, 68 insertions(+), 83 deletions(-)
>
> diff --git a/drivers/gpio/gpio-adp5588.c b/drivers/gpio/gpio-adp5588.c
> index da9781a2ef4a..0a8cfccba818 100644
> --- a/drivers/gpio/gpio-adp5588.c
> +++ b/drivers/gpio/gpio-adp5588.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> @@ -33,8 +34,6 @@ struct adp5588_gpio {
> struct mutex lock; /* protect cached dir, dat_out */
> /* protect serialized access to the interrupt controller bus */
> struct mutex irq_lock;
> - unsigned gpio_start;
> - unsigned irq_base;
> uint8_t dat_out[3];
> uint8_t dir[3];
> uint8_t int_lvl[3];
> @@ -148,16 +147,11 @@ static int adp5588_gpio_direction_output(struct
> gpio_chip *chip,
> }
>
> #ifdef CONFIG_GPIO_ADP5588_IRQ
> -static int adp5588_gpio_to_irq(struct gpio_chip *chip, unsigned off)
> -{
> - struct adp5588_gpio *dev = gpiochip_get_data(chip);
> -
> - return dev->irq_base + off;
> -}
>
> static void adp5588_irq_bus_lock(struct irq_data *d)
> {
> - struct adp5588_gpio *dev = irq_data_get_irq_chip_data(d);
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct adp5588_gpio *dev = gpiochip_get_data(gc);
>
> mutex_lock(&dev->irq_lock);
> }
> @@ -172,7 +166,8 @@ static void adp5588_irq_bus_lock(struct irq_data *d)
>
> static void adp5588_irq_bus_sync_unlock(struct irq_data *d)
> {
> - struct adp5588_gpio *dev = irq_data_get_irq_chip_data(d);
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct adp5588_gpio *dev = gpiochip_get_data(gc);
> int i;
>
> for (i = 0; i <= ADP5588_BANK(ADP5588_MAXGPIO); i++) {
> @@ -203,24 +198,25 @@ static void adp5588_irq_bus_sync_unlock(struct irq_data
> *d)
>
> static void adp5588_irq_mask(struct irq_data *d)
> {
> - struct adp5588_gpio *dev = irq_data_get_irq_chip_data(d);
> - unsigned gpio = d->irq - dev->irq_base;
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct adp5588_gpio *dev = gpiochip_get_data(gc);
>
> - dev->irq_mask[ADP5588_BANK(gpio)] &= ~ADP5588_BIT(gpio);
> + dev->irq_mask[ADP5588_BANK(d->hwirq)] &= ~ADP5588_BIT(d->hwirq);
> }
>
> static void adp5588_irq_unmask(struct irq_data *d)
> {
> - struct adp5588_gpio *dev = irq_data_get_irq_chip_data(d);
> - unsigned gpio = d->irq - dev->irq_base;
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct adp5588_gpio *dev = gpiochip_get_data(gc);
>
> - dev->irq_mask[ADP5588_BANK(gpio)] |= ADP5588_BIT(gpio);
> + dev->irq_mask[ADP5588_BANK(d->hwirq)] |= ADP5588_BIT(d->hwirq);
> }
>
> static int adp5588_irq_set_type(struct irq_data *d, unsigned int type)
> {
> - struct adp5588_gpio *dev = irq_data_get_irq_chip_data(d);
> - uint16_t gpio = d->irq - dev->irq_base;
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> + struct adp5588_gpio *dev = gpiochip_get_data(gc);
> + uint16_t gpio = d->hwirq;
> unsigned bank, bit;
>
> if ((type & IRQ_TYPE_EDGE_BOTH)) {
> @@ -281,10 +277,11 @@ static irqreturn_t adp5588_irq_handler(int irq, void
> *devid)
>
> while (pending) {
> if (pending & (1 << bit)) {
> - handle_nested_irq(dev->irq_base +
> - (bank << 3) + bit);
> + handle_nested_irq(
> + irq_find_mapping(
> + dev->gpio_chip.irq.domain,
> + (bank << 3) + bit));
> pending &= ~
RE: [PATCH 2/2] drivers/gpio/gpio-adp5588.c: switch to events system
> -Original Message-
> From: Nikolaus Voss [mailto:n...@vosn.de]
> Sent: Dienstag, 5. Februar 2019 14:31
> To: Hennerich, Michael ; Linus Walleij
>
> Cc: linux-g...@vger.kernel.org; linux-kernel@vger.kernel.org; Nikolaus Voss
>
> Subject: [PATCH 2/2] drivers/gpio/gpio-adp5588.c: switch to events system
>
> Interupts were generated using GPIN interrupts of
> ADP5588. These interrupts have two important limitations:
> 1. Interrupts can only be generated for either rising or
>falling edges but not both.
> 2. Interrupts are reasserted as long as the interrupt condition
>persists (i.e. high or low level on that GPIN). This generates
>lots of interrupts unless the event is very short.
>
> To overcome this, ADP5588 provides an event system which queues
> up to 10 events in a buffer. GPIN events are queued whenever the
> GPIN is asserted or deasserted. This makes it possible to support
> generating GPIN interrupts for both edges and to generate only one
> interrupt per state change.
> Thus it is possible to chain the gpio-keys driver for some GPIOs.
Looks like a viable and much better solution. This already works pretty well
for the input driver.
I can't test at the moment, but I assume you already did.
Well done and thanks for this patch!
>
> Signed-off-by: Nikolaus Voss
Acked-by: Michael Hennerich
> ---
> drivers/gpio/gpio-adp5588.c | 85 ++---
> 1 file changed, 32 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpio/gpio-adp5588.c b/drivers/gpio/gpio-adp5588.c
> index 0a8cfccba818..6583a3787035 100644
> --- a/drivers/gpio/gpio-adp5588.c
> +++ b/drivers/gpio/gpio-adp5588.c
> @@ -36,12 +36,11 @@ struct adp5588_gpio {
> struct mutex irq_lock;
> uint8_t dat_out[3];
> uint8_t dir[3];
> - uint8_t int_lvl[3];
> + uint8_t int_lvl_low[3];
> + uint8_t int_lvl_high[3];
> uint8_t int_en[3];
> uint8_t irq_mask[3];
> - uint8_t irq_stat[3];
> uint8_t int_input_en[3];
> - uint8_t int_lvl_cached[3];
> };
>
> static int adp5588_gpio_read(struct i2c_client *client, u8 reg)
> @@ -180,15 +179,9 @@ static void adp5588_irq_bus_sync_unlock(struct irq_data
> *d)
> mutex_unlock(&dev->lock);
> }
>
> - if (dev->int_lvl_cached[i] != dev->int_lvl[i]) {
> - dev->int_lvl_cached[i] = dev->int_lvl[i];
> - adp5588_gpio_write(dev->client, GPIO_INT_LVL1 + i,
> - dev->int_lvl[i]);
> - }
> -
> if (dev->int_en[i] ^ dev->irq_mask[i]) {
> dev->int_en[i] = dev->irq_mask[i];
> - adp5588_gpio_write(dev->client, GPIO_INT_EN1 + i,
> + adp5588_gpio_write(dev->client, GPI_EM1 + i,
>dev->int_en[i]);
> }
> }
> @@ -219,21 +212,17 @@ static int adp5588_irq_set_type(struct irq_data *d,
> unsigned int type)
> uint16_t gpio = d->hwirq;
> unsigned bank, bit;
>
> - if ((type & IRQ_TYPE_EDGE_BOTH)) {
> - dev_err(&dev->client->dev, "irq %d: unsupported type %d\n",
> - d->irq, type);
> - return -EINVAL;
> - }
> -
> bank = ADP5588_BANK(gpio);
> bit = ADP5588_BIT(gpio);
>
> - if (type & IRQ_TYPE_LEVEL_HIGH)
> - dev->int_lvl[bank] |= bit;
> - else if (type & IRQ_TYPE_LEVEL_LOW)
> - dev->int_lvl[bank] &= ~bit;
> - else
> - return -EINVAL;
> + dev->int_lvl_low[bank] &= ~bit;
> + dev->int_lvl_high[bank] &= ~bit;
> +
> + if (type & IRQ_TYPE_EDGE_BOTH || type & IRQ_TYPE_LEVEL_HIGH)
> + dev->int_lvl_high[bank] |= bit;
> +
> + if (type & IRQ_TYPE_EDGE_BOTH || type & IRQ_TYPE_LEVEL_LOW)
> + dev->int_lvl_low[bank] |= bit;
>
> dev->int_input_en[bank] |= bit;
>
> @@ -249,41 +238,32 @@ static struct irq_chip adp5588_irq_chip = {
> .irq_set_type = adp5588_irq_set_type,
> };
>
> -static int adp5588_gpio_read_intstat(struct i2c_client *client, u8 *buf)
> -{
> - int ret = i2c_smbus_read_i2c_block_data(client, GPIO_INT_STAT1, 3,
> buf);
> -
> - if (ret < 0)
> - dev_err(&client->dev, "Read INT_STAT Error\n");
> -
> - return ret;
> -}
> -
> static irqreturn_t adp5588_irq_hand
RE: [PATCH 02/18] mfd: adp5520: Make it explicitly non-modular
> -Original Message-
> From: Paul Gortmaker [mailto:paul.gortma...@windriver.com]
> Sent: Montag, 17. Dezember 2018 21:31
> To: Lee Jones
> Cc: linux-kernel@vger.kernel.org; Paul Gortmaker
> ; Hennerich, Michael
>
> Subject: [PATCH 02/18] mfd: adp5520: Make it explicitly non-modular
>
> The Makefile/Kconfig currently controlling compilation of this code is:
>
> drivers/mfd/Makefile:obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> drivers/mfd/Kconfig:config PMIC_ADP5520
> drivers/mfd/Kconfig:bool "Analog Devices ADP5520/01 MFD PMIC Core Support"
>
> ...meaning that it currently is not being built as a module by anyone.
>
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
>
> We explicitly disallow a driver unbind, since that doesn't have a
> sensible use case anyway, and it allows us to drop the ".remove"
> code for non-modular drivers.
>
> Since module_i2c_driver() uses the same init level priority as
> builtin_i2c_driver() the init ordering remains unchanged with
> this commit.
>
> Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
>
> We also delete the MODULE_LICENSE tag etc. since all that information
> was (or is now) contained at the top of the file in the comments.
>
> Cc: Michael Hennerich
> Cc: Lee Jones
> Signed-off-by: Paul Gortmaker
> Acked-by: Linus Walleij
Acked-by: Michael Hennerich
> ---
> drivers/mfd/adp5520.c | 30 +++---
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mfd/adp5520.c b/drivers/mfd/adp5520.c
> index be0497b96720..2cdd39cb8a18 100644
> --- a/drivers/mfd/adp5520.c
> +++ b/drivers/mfd/adp5520.c
> @@ -7,6 +7,8 @@
> *
> * Copyright 2009 Analog Devices Inc.
> *
> + * Author: Michael Hennerich
> + *
> * Derived from da903x:
> * Copyright (C) 2008 Compulab, Ltd.
> * Mike Rapoport
> @@ -18,7 +20,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -304,18 +306,6 @@ static int adp5520_probe(struct i2c_client *client,
> return ret;
> }
>
> -static int adp5520_remove(struct i2c_client *client)
> -{
> - struct adp5520_chip *chip = dev_get_drvdata(&client->dev);
> -
> - if (chip->irq)
> - free_irq(chip->irq, chip);
> -
> - adp5520_remove_subdevs(chip);
> - adp5520_write(chip->dev, ADP5520_MODE_STATUS, 0);
> - return 0;
> -}
> -
> #ifdef CONFIG_PM_SLEEP
> static int adp5520_suspend(struct device *dev)
> {
> @@ -346,20 +336,14 @@ static const struct i2c_device_id adp5520_id[] = {
> { "pmic-adp5501", ID_ADP5501 },
> { }
> };
> -MODULE_DEVICE_TABLE(i2c, adp5520_id);
>
> static struct i2c_driver adp5520_driver = {
> .driver = {
> - .name = "adp5520",
> - .pm = &adp5520_pm,
> + .name = "adp5520",
> + .pm = &adp5520_pm,
> + .suppress_bind_attrs= true,
> },
> .probe = adp5520_probe,
> - .remove = adp5520_remove,
> .id_table = adp5520_id,
> };
> -
> -module_i2c_driver(adp5520_driver);
> -
> -MODULE_AUTHOR("Michael Hennerich ");
> -MODULE_DESCRIPTION("ADP5520(01) PMIC-MFD Driver");
> -MODULE_LICENSE("GPL");
> +builtin_i2c_driver(adp5520_driver);
> --
> 2.7.4
RE: [PATCH 02/18] mfd: adp5520: Make it explicitly non-modular
> -Original Message-
> From: Paul Gortmaker [mailto:paul.gortma...@windriver.com]
> Sent: Freitag, 7. Dezember 2018 21:11
> To: Lee Jones
> Cc: linux-kernel@vger.kernel.org; Paul Gortmaker
> ; Hennerich, Michael
>
> Subject: [PATCH 02/18] mfd: adp5520: Make it explicitly non-modular
>
> The Makefile/Kconfig currently controlling compilation of this code is:
>
> drivers/mfd/Makefile:obj-$(CONFIG_PMIC_ADP5520) += adp5520.o
> drivers/mfd/Kconfig:config PMIC_ADP5520
> drivers/mfd/Kconfig:bool "Analog Devices ADP5520/01 MFD PMIC Core Support"
>
> ...meaning that it currently is not being built as a module by anyone.
>
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
>
> We explicitly disallow a driver unbind, since that doesn't have a
> sensible use case anyway, and it allows us to drop the ".remove"
> code for non-modular drivers.
>
> Since module_i2c_driver() uses the same init level priority as
> builtin_i2c_driver() the init ordering remains unchanged with
> this commit.
>
> Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
>
> We also delete the MODULE_LICENSE tag etc. since all that information
> was (or is now) contained at the top of the file in the comments.
>
> Cc: Michael Hennerich
> Cc: Lee Jones
> Acked-by: Linus Walleij
> Signed-off-by: Paul Gortmaker
Signed-off-by: Michael Hennerich
> ---
> drivers/mfd/adp5520.c | 30 +++---
> 1 file changed, 7 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/mfd/adp5520.c b/drivers/mfd/adp5520.c
> index be0497b96720..2cdd39cb8a18 100644
> --- a/drivers/mfd/adp5520.c
> +++ b/drivers/mfd/adp5520.c
> @@ -7,6 +7,8 @@
> *
> * Copyright 2009 Analog Devices Inc.
> *
> + * Author: Michael Hennerich
> + *
> * Derived from da903x:
> * Copyright (C) 2008 Compulab, Ltd.
> * Mike Rapoport
> @@ -18,7 +20,7 @@
> */
>
> #include
> -#include
> +#include
> #include
> #include
> #include
> @@ -304,18 +306,6 @@ static int adp5520_probe(struct i2c_client *client,
> return ret;
> }
>
> -static int adp5520_remove(struct i2c_client *client)
> -{
> - struct adp5520_chip *chip = dev_get_drvdata(&client->dev);
> -
> - if (chip->irq)
> - free_irq(chip->irq, chip);
> -
> - adp5520_remove_subdevs(chip);
> - adp5520_write(chip->dev, ADP5520_MODE_STATUS, 0);
> - return 0;
> -}
> -
> #ifdef CONFIG_PM_SLEEP
> static int adp5520_suspend(struct device *dev)
> {
> @@ -346,20 +336,14 @@ static const struct i2c_device_id adp5520_id[] = {
> { "pmic-adp5501", ID_ADP5501 },
> { }
> };
> -MODULE_DEVICE_TABLE(i2c, adp5520_id);
>
> static struct i2c_driver adp5520_driver = {
> .driver = {
> - .name = "adp5520",
> - .pm = &adp5520_pm,
> + .name = "adp5520",
> + .pm = &adp5520_pm,
> + .suppress_bind_attrs= true,
> },
> .probe = adp5520_probe,
> - .remove = adp5520_remove,
> .id_table = adp5520_id,
> };
> -
> -module_i2c_driver(adp5520_driver);
> -
> -MODULE_AUTHOR("Michael Hennerich ");
> -MODULE_DESCRIPTION("ADP5520(01) PMIC-MFD Driver");
> -MODULE_LICENSE("GPL");
> +builtin_i2c_driver(adp5520_driver);
> --
> 2.7.4
>
AW: [PATCH 1/1] mfd: adp5520: Use module_i2c_driver()
Von: Sachin Kamat [sachin.ka...@linaro.org]
Gesendet: Mittwoch, 27. März 2013 09:59
An: device-drivers-de...@blackfin.uclinux.org; LKML
Cc: Hennerich, Michael; sa...@linux.intel.com; sachin.ka...@linaro.org
Betreff: Re: [PATCH 1/1] mfd: adp5520: Use module_i2c_driver()
On 15 March 2013 17:10, Sachin Kamat wrote:
> module_i2c_driver() removes some boilerplate and makes the code
> simple.
>
> Signed-off-by: Sachin Kamat
Acked-by: Michael Hennerich
> ---
> drivers/mfd/adp5520.c | 12 +---
> 1 files changed, 1 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mfd/adp5520.c b/drivers/mfd/adp5520.c
> index 210dd03..5ab021f 100644
> --- a/drivers/mfd/adp5520.c
> +++ b/drivers/mfd/adp5520.c
> @@ -360,17 +360,7 @@ static struct i2c_driver adp5520_driver = {
> .id_table = adp5520_id,
> };
>
> -static int __init adp5520_init(void)
> -{
> - return i2c_add_driver(&adp5520_driver);
> -}
> -module_init(adp5520_init);
> -
> -static void __exit adp5520_exit(void)
> -{
> - i2c_del_driver(&adp5520_driver);
> -}
> -module_exit(adp5520_exit);
> +module_i2c_driver(adp5520_driver);
>
> MODULE_AUTHOR("Michael Hennerich ");
> MODULE_DESCRIPTION("ADP5520(01) PMIC-MFD Driver");
> --
> 1.7.4.1
>
ping...
--
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: [BUG][RFC][GENERIC IRQ] linux-2.6.24 (delayed) disable IRQ feature not functional for handle_simple_irq
>From: Thomas Gleixner, Dienstag, 19. Februar 2008 11:49
>Subject: Re: [BUG][RFC][GENERIC IRQ] linux-2.6.24 (delayed) disable IRQ
>feature not functional for handle_simple_irq
>
>On Tue, 19 Feb 2008, Hennerich, Michael wrote:
>> Thomas,
>>
>> I have reasonable doubt that the delayed disable feature on
>> linux-2.6.24 for handle_simple_irq is broken.
>>
>> In 2.6.22 there was something like this:
>>
>> if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
>> if (desc->chip->mask)
>> desc->chip->mask(irq);
>> ...
>>
>> However in 2.6.24 the "DISABLED" IRQ in case it happens is never
going to
>be masked.
>>
>> if (unlikely(!action || (desc->status & IRQ_DISABLED)))
>> goto out_unlock;
>>
>>
>> I see a disabled IRQ being invoked in an endless loop.
>
>Simple irq's are special, see the comment above handle_simple_irq:
>
>Simple interrupts are either sent from a demultiplexing interrupt
>handler or come from hardware, where no interrupt hardware control
>is necessary.
>
>Note: The caller is expected to handle the ack, clear, mask and
>unmask issues if necessary.
>
>Simple irqs are used for low level demultiplex handlers, where the full
>control of the irqchip is in the low level handler.
>
>See also the commit log of 971e5b35fb02c5088d49e6c024aab73582a35b71
>
>In commit 76d2160147f43f982dfe881404cfde9fd0a9da21 lazy irq
disabling
>was implemented, and the simple irq handler had a masking set to
it.
>
>Remy Bohmer discovered that some devices in the ARM architecture
>would trigger the mask, but never unmask it. His patch to do the
>unmasking was questioned by Russell King about masking simple irqs
>to begin with. Looking further, it was discovered that the problems
>Remy was seeing was due to improper use of the simple handler by
>devices, and he later submitted patches to fix those. But the issue
>that was uncovered was that the simple handler should never mask.
>
>This patch reverts the masking in the simple handler.
>
>Thanks,
>
> tglx
Ok - I see no problem that the simple handler doesn't mask the IRQ by
default like the level irq handler does.
However if I use disable_irq(irq) I would expect that the irq is somehow
disabled afterwards.
Our internal system IRQs doesn't need to be acked on the System
Interrupt Controller Level and we have simply just mask and unmask.
So I think I have two alternatives.
1) Bypass the delayed disable feature by implementing chip->disable and
chip->enable (simply map to mask and unmask)
2) Use level irq
Best regards,
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[BUG][RFC][GENERIC IRQ] linux-2.6.24 (delayed) disable IRQ feature not functional for handle_simple_irq
Thomas,
I have reasonable doubt that the delayed disable feature on linux-2.6.24 for
handle_simple_irq is broken.
In 2.6.22 there was something like this:
if (unlikely(!action || (desc->status & IRQ_DISABLED))) {
if (desc->chip->mask)
desc->chip->mask(irq);
...
However in 2.6.24 the "DISABLED" IRQ in case it happens is never going to be
masked.
if (unlikely(!action || (desc->status & IRQ_DISABLED)))
goto out_unlock;
I see a disabled IRQ being invoked in an endless loop.
-Michael
--
* Analog Devices GmbH [EMAIL PROTECTED]
** * Systems Engineering
** ** Wilhelm-Wagenfeld-Strasse 6
** * D-80807 Munich
* Germany
Registergericht München HRB 40368, Geschäftsführer: Thomas Wessel, Vincent
Roche, Joseph E. McDonough
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [BUG][RFC] [GENERIC IRQ] irq_chip_set_defaults shutdown / disable
>From: Thomas Gleixner Montag, 18. Februar 2008 21:38
>
>On Mon, 18 Feb 2008, Hennerich, Michael wrote:
>> >The patch below fixes the shutdown case and keeps the delayed
disable
>> >logic intact.
>>
>> >How did you notice ? I guess you got spurious interrupts after
calling
>> >free_irq(), right ?
>>
>> Exactly
>
>Can you please confirm, whether my version of the fix works for you as
>well.
>
>Thanks,
>
> tglx
Thomas,
Works - no problems.
There was another typo
>+ chip_disable : default_shutdown;
Should be better:
+ chip->disable : default_shutdown;
Best regards,
Michael
Signed-off-by: Michael Hennerich <[EMAIL PROTECTED]>
Index: kernel/irq/chip.c
===
--- kernel/irq/chip.c (revision 4270)
+++ kernel/irq/chip.c (working copy)
@@ -245,7 +245,20 @@
return 0;
}
+
/*
+ * default shutdown function
+ */
+static void default_shutdown(unsigned int irq)
+{
+ struct irq_desc *desc = irq_desc + irq;
+
+ desc->chip->mask(irq);
+ desc->status |= IRQ_MASKED;
+}
+
+
+/*
* Fixup enable/disable function pointers
*/
void irq_chip_set_defaults(struct irq_chip *chip)
@@ -257,7 +270,8 @@
if (!chip->startup)
chip->startup = default_startup;
if (!chip->shutdown)
- chip->shutdown = chip->disable;
+ chip->shutdown = chip->disable != default_disable ?
+ chip->disable : default_shutdown;
if (!chip->name)
chip->name = chip->typename;
if (!chip->end)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [BUG][RFC] [GENERIC IRQ] irq_chip_set_defaults shutdown / disable
>From: Thomas Gleixner Montag, 18. Februar 2008 18:49
>
>On Mon, 18 Feb 2008, Hennerich, Michael wrote:
>> RESENT: Add Maintainers
>>
>> free_irq() does not disable/mask the irq, in case disable or shutdown
in
>struct irq_chip is left uninitilazied.
>>
>> /**
>> * struct irq_chip - hardware interrupt chip descriptor
>> *
>> * @name:name for /proc/interrupts
>> * @startup: start up the interrupt (defaults to ->enable if NULL)
>> * @shutdown:shut down the interrupt (defaults to ->disable
if NULL)
>> * @enable: enable the interrupt (defaults to chip->unmask
if
>NULL)
>> * @disable: disable the interrupt (defaults to chip->mask if NULL)
>>
>>
>> According to linux/irq.h struct irq_chip information,
>> chip->disable should default to chip->mask if NULL.
>> However irq_chip_set_defaults(struct irq_chip *chip) will set it to
>default_disable an empty function.
>>
>> In earlier kernel versions such as 2.6.19 default_disable called
chip-
>>mask.
>> In 2.6.22 and 2.6.24 default_disable is an empty function.
>>
>> Looking through various architectures, it's still pretty common that
>disable and shutdown is NULL.
>
>Yeah, you are right. This was broken by commit
>76d2160147f43f982dfe881404cfde9fd0a9da21
>
>> Do I miss something here?
>
>Just a small detail. The commit optimized the irq_disable/enable logic
>by defaulting to delayed disable. So we want to keep that logic intact
>in case that we still have a device which is handling this
>interrupt. For the free_irq() case your patch is correct.
>
>The patch below fixes the shutdown case and keeps the delayed disable
>logic intact.
>
>How did you notice ? I guess you got spurious interrupts after calling
>free_irq(), right ?
Exactly
-Michael
>
>Thanks for analyzing and reporting,
>
> tglx
>
>->
>Subject: genirq: do not leave interupts enabled on free_irq
>From: Thomas Gleixner <[EMAIL PROTECTED]>
>Date: Mon, 18 Feb 2008 18:25:17 +0100
>
>The default_disable() function was changed in commit:
>
> 76d2160147f43f982dfe881404cfde9fd0a9da21
> genirq: do not mask interrupts by default
>
>It removed the mask function in favour of the default delayed
>interrupt disable handling. Unfortunately this also broke the shutdown
>in free_irq() when the last handler is removed from the interrupt for
>those architectures which rely on the default implementations. Now we
>can end up with a enabled interrupt line after the last handler was
>removed.
>
>Fix this by adding a default_shutdown function, which is only
>installed, when the irqchip implementation does provide neither a
>shutdown nor a disable function.
>
>Pointed-out-by: Michael Hennerich <[EMAIL PROTECTED]>
>Signed-off-by: Thomas Gleixner <[EMAIL PROTECTED]>
>CC: [EMAIL PROTECTED]
>
>---
> kernel/irq/chip.c | 14 +-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
>Index: linux-2.6/kernel/irq/chip.c
>===
>--- linux-2.6.orig/kernel/irq/chip.c
>+++ linux-2.6/kernel/irq/chip.c
>@@ -246,6 +246,17 @@ static unsigned int default_startup(unsi
> }
>
> /*
>+ * default shutdown function
>+ */
>+static void default_shutdown(unsigned int irq)
>+{
>+ struct irq_desc *desc = irq_desc + irq;
>+
>+ desc->chip->mask(irq);
>+ desc->status &= ~IRQ_MASKED;
>+}
>+
>+/*
> * Fixup enable/disable function pointers
> */
> void irq_chip_set_defaults(struct irq_chip *chip)
>@@ -257,7 +268,8 @@ void irq_chip_set_defaults(struct irq_ch
> if (!chip->startup)
> chip->startup = default_startup;
> if (!chip->shutdown)
>- chip->shutdown = chip->disable;
>+ chip->shutdown = chip->disable != default_disable ?
>+ chip_disable : default_shutdown;
> if (!chip->name)
> chip->name = chip->typename;
> if (!chip->end)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[BUG][RFC] [GENERIC IRQ] irq_chip_set_defaults shutdown / disable
RESENT: Add Maintainers
free_irq() does not disable/mask the irq, in case disable or shutdown in struct
irq_chip is left uninitilazied.
/**
* struct irq_chip - hardware interrupt chip descriptor
*
* @name: name for /proc/interrupts
* @startup:start up the interrupt (defaults to ->enable if NULL)
* @shutdown: shut down the interrupt (defaults to ->disable if NULL)
* @enable: enable the interrupt (defaults to chip->unmask if NULL)
* @disable:disable the interrupt (defaults to chip->mask if NULL)
According to linux/irq.h struct irq_chip information,
chip->disable should default to chip->mask if NULL.
However irq_chip_set_defaults(struct irq_chip *chip) will set it to
default_disable an empty function.
In earlier kernel versions such as 2.6.19 default_disable called chip->mask.
In 2.6.22 and 2.6.24 default_disable is an empty function.
Looking through various architectures, it's still pretty common that disable
and shutdown is NULL.
Do I miss something here?
Signed-off-by: Michael Hennerich <[EMAIL PROTECTED]>
Index: irq/chip.c
===
--- irq/chip.c (revision 4276)
+++ irq/chip.c (working copy)
@@ -233,6 +233,10 @@
*/
static void default_disable(unsigned int irq) {
+ struct irq_desc *desc = irq_desc + irq;
+
+ desc->chip->mask(irq);
+ desc->status |= IRQ_MASKED;
}
/*
Best regards,
Michael
--
* Analog Devices GmbH [EMAIL PROTECTED]
** * Systems Engineering
** ** Wilhelm-Wagenfeld-Strasse 6
** * D-80807 Munich
* Germany
Registergericht München HRB 40368, Geschäftsführer: Thomas Wessel, Vincent
Roche, Joseph E. McDonough
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
linux/kernel/irq/chip.c IRQ disable, shutdown bug
free_irq() does not disable/mask the irq, in case disable or shutdown in struct
irq_chip is left uninitilazied.
/**
* struct irq_chip - hardware interrupt chip descriptor
*
* @name: name for /proc/interrupts
* @startup:start up the interrupt (defaults to ->enable if NULL)
* @shutdown: shut down the interrupt (defaults to ->disable if NULL)
* @enable: enable the interrupt (defaults to chip->unmask if NULL)
* @disable:disable the interrupt (defaults to chip->mask if NULL)
According to linux/irq.h struct irq_chip information,
chip->disable should default to chip->mask if NULL.
However irq_chip_set_defaults(struct irq_chip *chip) will set it to
default_disable a empty function.
Looking through various architectures, it's pretty common that disable and
shutdown is NULL.
So this bug affects many architectures.
This patch fixes the issue.
Best regards,
Michael
Index: irq/chip.c
===
--- irq/chip.c (revision 4276)
+++ irq/chip.c (working copy)
@@ -233,6 +233,10 @@
*/
static void default_disable(unsigned int irq)
{
+ struct irq_desc *desc = irq_desc + irq;
+
+ desc->chip->mask(irq);
+ desc->status |= IRQ_MASKED;
}
/*
--
* Analog Devices GmbH [EMAIL PROTECTED]
** * Systems Engineering
** ** Wilhelm-Wagenfeld-Strasse 6
** * D-80807 Munich
* Germany
Registergericht München HRB 40368, Geschäftsführer: Thomas Wessel, Vincent
Roche, Joseph E. McDonough
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
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] Input/Touchscreen Driver: add support AD7877 touchscreen driver
Hi Dmitry,
>From: Dmitry Torokhov [mailto:[EMAIL PROTECTED]
>Sent: Donnerstag, 11. Oktober 2007 15:28
>To: Bryan Wu; Michael Hennerich
>Cc: [EMAIL PROTECTED]; linux-
>[EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-
>[EMAIL PROTECTED]; [EMAIL PROTECTED]
>Subject: Re: [PATCH 2/3] Input/Touchscreen Driver: add support AD7877
>touchscreen driver
>
>Hi Bryan, Michael,
>
>On 10/11/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
>> +
>> +static int gpio3 = 0;
>
>No need to initialize.
Sure - It's ZERO by default.
>
>> +
>> +static int ad7877_read(struct device *dev, u16 reg)
>> +{
>> + struct spi_device *spi = to_spi_device(dev);
>> + struct ser_req *req = kzalloc(sizeof *req,
GFP_KERNEL);
>
>How many reads can happen at once? Maybe allocate 1 ser_req per
>touchcsreen when creating it?
ad7877_read_adc, ad7877_read and ad7877_write are just used by the sysfs
hooks. Touchscreen samples are read by the kthread using a different
message struct. So far each sysfs invocation got its own storage for the
spi message, which then is handed over to the SPI bus driver.
The SPI bus driver serializes transfers in a kthread.
Two different processes could access the drivers sysfs hooks.
Using one ser_req per touch screen could require additional locking?
Things at is, looks pretty safe to me.
>
>> +
>> + if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
>> + /* compute touch pressure resistance using equation
#2 */
>> + Rt = z2;
>> + Rt -= z1;
>> + Rt *= x;
>> + Rt *= ts->x_plate_ohms;
>> + Rt /= z1;
>> + Rt = (Rt + 2047) >> 12;
>> + } else
>> + Rt = 0;
>> +
>> + if (Rt) {
>> + input_report_abs(input_dev, ABS_X, x);
>> + input_report_abs(input_dev, ABS_Y, y);
>> + sync = 1;
>> + }
>> +
>> + if (sync) {
>> + input_report_abs(input_dev, ABS_PRESSURE, Rt);
>> + input_sync(input_dev);
>> + }
>
>Confused about the logic - you set sync only if Rt != 0 so why don't
>fold second "if" into the first one?
Sure - I guess this was just a leftover form the original driver, this
driver was derived from.
>
>> +/* Must be called with ts->lock held */
>> +static void ad7877_disable(struct ad7877 *ts)
>> +{
>> + if (ts->disabled)
>> + return;
>> +
>> + ts->disabled = 1;
>> +
>> + if (!ts->pending) {
>> + ts->irq_disabled = 1;
>> + disable_irq(ts->spi->irq);
>> + } else {
>> + /* the kthread will run at least once more, and
>> +* leave everything in a clean state, IRQ disabled
>> +*/
>> + while (ts->pending) {
>> + spin_unlock_irq(&ts->lock);
>> + msleep(1);
>> + spin_lock_irq(&ts->lock);
>> + }
>> + }
>> +
>> + /* we know the chip's in lowpower mode since we always
>> +* leave it that way after every request
>> +*/
>> +
>> +}
>
>This looks scary. Can you try moving locking inside ad7877_enable and
>ad7877_disable?
This is also something imported from the ads7846.c driver.
Since it worked pretty well I never touched this function.
However I think the spin_locks here are not necessary.
>
>> +
>> +static int __devinit ad7877_probe(struct spi_device *spi)
>> +{
>> + struct ad7877 *ts;
>> + struct input_dev*input_dev;
>> + struct ad7877_platform_data *pdata =
spi->dev.platform_data;
>> + struct spi_message *m;
>> + int err;
>> + u16 verify;
>> +
>> +
>> + if (!spi->irq) {
>> + dev_dbg(&spi->dev, "no IRQ?\n");
>> + return -ENODEV;
>> + }
>> +
>> +
>> + if (!pdata) {
>> + dev_dbg(&spi->dev, "no platform data?\n");
>> + return -ENODEV;
>> + }
>> +
>> +
>> + /* don't exceed max specified SPI CLK frequency */
>> + if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) {
>> + dev_dbg(&spi->dev, "SPI CLK %d
Hz?\n",spi->max_speed_hz);
>> + return -EINVAL;
>> + }
>> +
>> + ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
>> + input_dev = input_allocate_device();
>> + if (!ts || !input_dev) {
>> + err = -ENOMEM;
>> + goto err_free_mem;
>> + }
>> +
>> +
>> + dev_set_drvdata(&spi->dev, ts);
>> + spi->dev.power.power_state = PMSG_ON;
>> +
>> + ts->spi = spi;
>> + ts->input = input_dev;
>> + ts->intr_flag = 0;
>> + init_timer(&ts->timer);
>> + ts->timer.data = (unsigned long) ts;
>> + ts->timer.function = ad7877_timer;
>> +
>> + spin_lock_init(&ts->lock);
>> +
>> + ts->model = pdata->model ? : 7877;
>> + ts->vref
RE: [PATCH try #3] Blackfin BF54x Input Keypad controller driver
Hi Dmitry, >From: Dmitry Torokhov [mailto:[EMAIL PROTECTED] >Sent: Donnerstag, 11. Oktober 2007 21:58 >To: Hennerich, Michael >Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; Linux >Kernel; [EMAIL PROTECTED] >Subject: Re: [PATCH try #3] Blackfin BF54x Input Keypad controller driver > >Hi Michael, > >On 10/11/07, Hennerich, Michael <[EMAIL PROTECTED]> wrote: >> >From: Dmitry Torokhov [mailto:[EMAIL PROTECTED] >> >Sent: Donnerstag, 11. Oktober 2007 19:27 >> >To: [EMAIL PROTECTED] >> >Cc: Michael Hennerich; [EMAIL PROTECTED]; Linux >> Kernel; >> >[EMAIL PROTECTED] >> >Subject: Re: [PATCH try #3] Blackfin BF54x Input Keypad controller >> driver >> > >> >On Thursday 11 October 2007, Bryan Wu wrote: >> >> From: Michael Hennerich <[EMAIL PROTECTED]> >> >> Date: Fri, 12 Oct 2007 00:20:19 +0800 >> >> Subject: [PATCH] Blackfin BF54x Input Keypad controller driver >> >> >> >> [try #2] Changelog: >> >> - Coding style issue fixes >> >> - using a temp variable for bf54x_kpad->input >> >> - Other updates according to Dmitry's review >> >> >> >> [try #3] Changelog: >> >> - Coding style cleanups >> >> - Use local copy of keymap >> >> - Remove empty PM functions >> >> - Use input_set_drvdata() since input->private is going away >> > >> > >> >This looks very good, thank youvery much. I have only 1 more suggestion >> >(and I can make the changes locally, you don't need to resubmit) - >> >since it is highly unlikely that we will have a keycode > 65535 do you >> >think we could change keymap to unsigned short. It would save you a >> >couple of bytes. I am also going to change "input->cdev.dev = >> &pdev->dev;" >> >to "input_dev->dev.parent = &pdev->dev;". Please let me know if you are >> >OK with it. >> >> Hi Dmitry, >> >> Thanks for your excellent feedback and support. >> >> We do a pretty similar thing like the omap keypad driver. >> Our keycodes do not exceed KEY_MAX but we encode the ROW and COL into >> the keycode matrix, in order simplify things in a "normal use case"? . >> What we store is u32, 16-bit for the keycode and the upper for private >> use. My understanding is that we then have to set input->keycodesize >> also being u32. >> >> Otherwise the generic input layer might get screwed up?! >> >> Am I wrong here, or do I miss something? >> Either this is a valid use case or I was inspired by a bad example. >> > >Thank you very much for alerting me of omap case, I missed it. > >Drivers that use complex values in their keymap tables should not use >input->keycode, keycodesize and keycodemax. These fields are only used >by input core if the driver relies on default implementation of >getkeycode() and setkeycode() for manipulations with its keymap. But >this will not work in your case (nor with omap) because your keymap >table does not contain "pure" input keycodes. > >Well, I guess the easiest is just not set these fields (and remove the >per-device map allocationand copying) and say that the driver does not >support altering keymaps. After all, like you said, it is an embedded >platform and it is unlikely that users will want to alter keymaps. > >However, if you think that this is a nice feature, then you either >need to rethink how you handle your keymap of implement custom >getkeycode and setkeycode methods in your driver. The coice is yours, >I will gladly accept either version of drver. Well it's a nice feature - I changed following: - Change keymapsize to u16 - Copy privately used LUT behind keycodemax in keycode table. This is more cache friendly when doing the tablewalk, and goes totally unnoticed be the input device layer. - Change Input_dev->cdev.dev to input_dev->dev.parent Bryan is going to send you an updated patch soon. Thanks and best regards, Michael > >-- >Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH try #3] Blackfin BF54x Input Keypad controller driver
>From: Dmitry Torokhov [mailto:[EMAIL PROTECTED]
>Sent: Donnerstag, 11. Oktober 2007 19:27
>To: [EMAIL PROTECTED]
>Cc: Michael Hennerich; [EMAIL PROTECTED]; Linux
Kernel;
>[EMAIL PROTECTED]
>Subject: Re: [PATCH try #3] Blackfin BF54x Input Keypad controller
driver
>
>On Thursday 11 October 2007, Bryan Wu wrote:
>> From: Michael Hennerich <[EMAIL PROTECTED]>
>> Date: Fri, 12 Oct 2007 00:20:19 +0800
>> Subject: [PATCH] Blackfin BF54x Input Keypad controller driver
>>
>> [try #2] Changelog:
>> - Coding style issue fixes
>> - using a temp variable for bf54x_kpad->input
>> - Other updates according to Dmitry's review
>>
>> [try #3] Changelog:
>> - Coding style cleanups
>> - Use local copy of keymap
>> - Remove empty PM functions
>> - Use input_set_drvdata() since input->private is going away
>
>
>This looks very good, thank youvery much. I have only 1 more suggestion
>(and I can make the changes locally, you don't need to resubmit) -
>since it is highly unlikely that we will have a keycode > 65535 do you
>think we could change keymap to unsigned short. It would save you a
>couple of bytes. I am also going to change "input->cdev.dev =
&pdev->dev;"
>to "input_dev->dev.parent = &pdev->dev;". Please let me know if you are
>OK with it.
Hi Dmitry,
Thanks for your excellent feedback and support.
We do a pretty similar thing like the omap keypad driver.
Our keycodes do not exceed KEY_MAX but we encode the ROW and COL into
the keycode matrix, in order simplify things in a "normal use case"? .
What we store is u32, 16-bit for the keycode and the upper for private
use. My understanding is that we then have to set input->keycodesize
also being u32.
Otherwise the generic input layer might get screwed up?!
Am I wrong here, or do I miss something?
Either this is a valid use case or I was inspired by a bad example.
Best regards,
Michael
>
>Thanks!
>
>
>>
>> Cc: Dmitry Torokhov <[EMAIL PROTECTED]>
>> Signed-off-by: Michael Hennerich <[EMAIL PROTECTED]>
>> Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
>> ---
>> arch/blackfin/mach-bf548/boards/ezkit.c |2 +-
>> drivers/input/keyboard/Kconfig |9 +
>> drivers/input/keyboard/Makefile |2 +-
>> drivers/input/keyboard/bf54x-keys.c | 378
>++
>> include/asm-blackfin/mach-bf548/bf54x_keys.h | 17 ++
>> 5 files changed, 406 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/input/keyboard/bf54x-keys.c
>> create mode 100644 include/asm-blackfin/mach-bf548/bf54x_keys.h
>>
>> diff --git a/arch/blackfin/mach-bf548/boards/ezkit.c
>b/arch/blackfin/mach-bf548/boards/ezkit.c
>> index 2c47db4..6734b3d 100644
>> --- a/arch/blackfin/mach-bf548/boards/ezkit.c
>> +++ b/arch/blackfin/mach-bf548/boards/ezkit.c
>> @@ -88,7 +88,7 @@ static struct platform_device bf54x_lq043_device =
{
>> #endif
>>
>> #if defined(CONFIG_KEYBOARD_BFIN) ||
>defined(CONFIG_KEYBOARD_BFIN_MODULE)
>> -static int bf548_keymap[] = {
>> +static unsigned int bf548_keymap[] = {
>> KEYVAL(0, 0, KEY_ENTER),
>> KEYVAL(0, 1, KEY_HELP),
>> KEYVAL(0, 2, KEY_0),
>> diff --git a/drivers/input/keyboard/Kconfig
>b/drivers/input/keyboard/Kconfig
>> index c97d5eb..2136a9e 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -253,4 +253,13 @@ config KEYBOARD_GPIO
>>To compile this driver as a module, choose M here: the
>>module will be called gpio-keys.
>>
>> +config KEYBOARD_BFIN
>> +tristate "Blackfin BF54x keypad support"
>> +depends on (BF54x)
>> +help
>> + Say Y here if you want to use the BF54x keypad.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called bf54x-keypad.
>> +
>> endif
>> diff --git a/drivers/input/keyboard/Makefile
>b/drivers/input/keyboard/Makefile
>> index 28d211b..3123277 100644
>> --- a/drivers/input/keyboard/Makefile
>> +++ b/drivers/input/keyboard/Makefile
>> @@ -21,4 +21,4 @@ obj-$(CONFIG_KEYBOARD_OMAP)+=
omap-keypad.o
>> obj-$(CONFIG_KEYBOARD_PXA27x) += pxa27x_keyboard.o
>> obj-$(CONFIG_KEYBOARD_AAED2000) += aaed2000_kbd.o
>> obj-$(CONFIG_KEYBOARD_GPIO) += gpio_keys.o
>> -
>> +obj-$(CONFIG_KEYBOARD_BFIN) += bf54x-keys.o
>> diff --git a/drivers/input/keyboard/bf54x-keys.c
>b/drivers/input/keyboard/bf54x-keys.c
>> new file mode 100644
>> index 000..2833c60
>> --- /dev/null
>> +++ b/drivers/input/keyboard/bf54x-keys.c
>> @@ -0,0 +1,378 @@
>> +/*
>> + * File: drivers/input/keyboard/bf54x-keys.c
>> + * Based on:
>> + * Author: Michael Hennerich <[EMAIL PROTECTED]>
>> + *
>> + * Created:
>> + * Description: keypad driver for Analog Devices Blackfin BF54x
>Processors
>> + *
>> + *
>> + * Modified:
>> + * Copyright 2007 Analog Devices Inc.
>> + *
>> + * Bugs: Enter bugs at http://blackfin.uclinux.org/
>> + *
>> + * This program is free software; you can redistribute it and/or
modify
RE: [PATCH try #2] Blackfin BF54x Input Keypad controller driver
Hi Dmitry,
Thanks for your feedback.
See my comment below.
Bryan is going to send you an updated patch shortly.
Best regards,
Michael
>Hi Bryan,
>
>On 10/4/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
>> From: Michael Hennerich <[EMAIL PROTECTED]>
>> Blackfin BF54x Input Keypad controller driver:
>>
>> [try #2] Changelog:
>> - Coding style issue fixes
>> - using a temp variable for bf54x_kpad->input
>> - Other updates according to Dmitry's review
>>
>
>I would like to ask you for a couple of additional changes:
>
>> +
>> +static u16 per_rows[] = {
>
>Change to "const" perhaps?
>
>> + P_KEY_ROW7,
>> + P_KEY_ROW6,
>> + P_KEY_ROW5,
>> + P_KEY_ROW4,
>> + P_KEY_ROW3,
>> + P_KEY_ROW2,
>> + P_KEY_ROW1,
>> + P_KEY_ROW0,
>> + 0
>> +};
>> +
>> +static u16 per_cols[] = {
>
>Same here.
>
>> + P_KEY_COL7,
>> + P_KEY_COL6,
>> + P_KEY_COL5,
>> + P_KEY_COL4,
>> + P_KEY_COL3,
>> + P_KEY_COL2,
>> + P_KEY_COL1,
>> + P_KEY_COL0,
>> + 0
>> +};
>> +
>> +
>> + if (bfin_kpad_get_keypressed(bf54x_kpad)) {
>> + disable_irq(bf54x_kpad->irq);
>> + bf54x_kpad->lastkey = key;
>> + bf54x_kpad->timer.expires = jiffies
>> + +
bf54x_kpad->keyup_test_jiffies;
>> + add_timer(&bf54x_kpad->timer);
>
>mod_timer()?
>
>> +
>> + input->keycode = pdata->keymap;
>
>Please consider allocating memory for keycode so that platfrom data
>can be kept constant.
Typical use cases for this driver module would be in a single user
embedded system. - More likely going through reboot than through a
module unload/load cycle with different user setting. But you are right
cleaner is to copy the key codes.
>
>> + input->keycodesize = sizeof(unsigned short);
>> + input->keycodemax = pdata->keymapsize;
>> +
>> + /* setup input device */
>> + set_bit(EV_KEY, input->evbit);
>> +
>> + if (pdata->repeat)
>> + set_bit(EV_REP, input->evbit);
>> +
>
>__set_bit() should suffice here and above.
>
>> + for (i = 0; i < pdata->keymapsize; i++)
>> + __set_bit(pdata->keymap[i] & KEY_MAX, input->keybit);
>> +
>> + __clear_bit(KEY_RESERVED, input->keybit);
>> +
>> + error = input_register_device(input);
>> +
>> + if (error) {
>> + printk(KERN_ERR DRV_NAME
>> + ": Unable to register input device (%d)\n",
>error);
>> + goto out4;
>> + }
>> +
>> + /* Init Keypad Key Up/Release test timer */
>> +
>> + init_timer(&bf54x_kpad->timer);
>> + bf54x_kpad->timer.function = bfin_kpad_timer;
>> + bf54x_kpad->timer.data = (unsigned long) pdev;
>> +
>
>setup_timer()?
>
>> +
>> + bfin_write_KPAD_PRESCALE(bfin_kpad_get_prescale(TIME_SCALE));
>> +
>> + bfin_write_KPAD_CTLpdata->cols - 1) << 13) & KPAD_COLEN)
|
>> + (((pdata->rows - 1) << 10) &
KPAD_ROWEN)
>|
>> + (2 & KPAD_IRQMODE));
>> +
>> +
>> + bfin_write_KPAD_CTL(bfin_read_KPAD_CTL() | KPAD_EN);
>> +
>> + printk(KERN_ERR DRV_NAME
>> + ": Blackfin BF54x Keypad registered IRQ %d\n",
>bf54x_kpad->irq);
>> +
>> + return 0;
>> +
>> +
>> +out4:
>> + input_free_device(input);
>> +out3:
>> + free_irq(bf54x_kpad->irq, pdev);
>> +out2:
>> + peripheral_free_list(&per_cols[MAX_RC - pdata->cols]);
>> +out1:
>> + peripheral_free_list(&per_rows[MAX_RC - pdata->rows]);
>> +out:
>> + kfree(bf54x_kpad);
>> + platform_set_drvdata(pdev, NULL);
>> +
>> + return error;
>> +}
>> +
>> +static int __devexit bfin_kpad_remove(struct platform_device *pdev)
>> +{
>> + struct bfin_kpad_platform_data *pdata =
pdev->dev.platform_data;
>> + struct bf54x_kpad *bf54x_kpad = platform_get_drvdata(pdev);
>> +
>> +
>> + del_timer_sync(&bf54x_kpad->timer);
>> + free_irq(bf54x_kpad->irq, pdev);
>> +
>> + peripheral_free_list(&per_rows[MAX_RC - pdata->rows]);
>> + peripheral_free_list(&per_cols[MAX_RC - pdata->cols]);
>> +
>> + input_unregister_device(bf54x_kpad->input);
>> +
>> + kfree(bf54x_kpad);
>> + platform_set_drvdata(pdev, NULL);
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int bfin_kpad_suspend(struct platform_device *pdev,
pm_message_t
>state)
>> +{
>
>Do you need these empty functions? At least stop timer here.
Currently not - I'm going to remove them.
>
>Thanks!
>
>Oh, one more thing - do not access input->private directly - it is
>going away - use input_set_drvdata() and input_get_drvdata().
>
>--
>Dmitry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
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] [INPUT] Blackfin BF54x Input Keypad controller driver
Hi Dmitry,
>-Original Message-
>From: Dmitry Torokhov [mailto:[EMAIL PROTECTED]
>Sent: Mittwoch, 26. September 2007 15:38
>To: [EMAIL PROTECTED]; Michael Hennerich
>Cc: [EMAIL PROTECTED]; Linux Kernel; uclinux-dist-
>[EMAIL PROTECTED]
>Subject: Re: [PATCH 1/2] [INPUT] Blackfin BF54x Input Keypad controller
>driver
>
>Hi Bryan, Michael,
>
>On 9/25/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
>> From: Michael Hennerich <[EMAIL PROTECTED]>
>> Date: Sun, 5 Aug 2007 18:45:26 +0800
>> Subject: [PATCH 1/2] [INPUT] Blackfin BF54x Input Keypad controller
>driver
>>
>
>Thank you for the patch. Couple of comments:
>
>> +
>> +static void bfin_kpad_timer(unsigned long data)
>> +{
>> + struct platform_device *pdev = (struct platform_device *)
data;
>> + struct bf54x_kpad *bf54x_kpad = platform_get_drvdata(pdev);
>> +
>> + if (bfin_kpad_get_keypressed(bf54x_kpad)) {
>> + /* Try again later */
>> + mod_timer(&bf54x_kpad->timer, jiffies
>> + + bf54x_kpad->keyup_test_jiffies);
>> + return;
>> + }
>> +
>> + input_report_key(bf54x_kpad->input, bf54x_kpad->lastkey, 0);
>> + input_sync(bf54x_kpad->input);
>> +
>
>So it is not possible to press another key without releaseing the first
>one?
No -
The main purpose of this driver is to serve a dedicated number (<=8x8)
of unique keys in an embedded application - it's not intended to replace
a full AT keyboard.
There are options to detect multiple keys pressed by hardware. But the
default I choose only generates an interrupt when a single key is
pressed.
Two or more keys don't generate interrupts.
In case you press a single key without releasing it and press another
key.
The second key pressed will be ignored.
If someone wants to have Multiple Key Resolution - he should drop me an
email.
It must be noted that only certain key-press combinations can be exactly
resolved.
1)Keys pressed in single row and single column
2)Keys pressed in single row and multiple columns
3)Keys pressed in multiple rows and single column
>
>> + /* Clear IRQ Status */
>> +
>> + bfin_kpad_clear_irq();
>> + enable_irq(bf54x_kpad->irq);
>> +}
>> +
>> +static irqreturn_t bfin_kpad_isr(int irq, void *dev_id)
>> +{
>> + struct platform_device *pdev = dev_id;
>> + struct bfin_kpad_platform_data *pdata =
pdev->dev.platform_data;
>> + struct bf54x_kpad *bf54x_kpad = platform_get_drvdata(pdev);
>> + int key;
>> + u16 rowcol = bfin_read_KPAD_ROWCOL();
>> +
>> + key = bfin_kpad_find_key(pdata, rowcol);
>> +
>> + input_report_key(bf54x_kpad->input, key, 1);
>> + input_sync(bf54x_kpad->input);
>> +
>> + if (bfin_kpad_get_keypressed(bf54x_kpad)) {
>> + disable_irq(bf54x_kpad->irq);
>> + bf54x_kpad->lastkey = key;
>> + bf54x_kpad->timer.expires = jiffies
>> + +
bf54x_kpad->keyup_test_jiffies;
>> + add_timer(&bf54x_kpad->timer);
>> +
>> + return IRQ_HANDLED;
>> + }
>> +
>> + input_report_key(bf54x_kpad->input, key, 0);
>> + input_sync(bf54x_kpad->input);
>> +
>> + bfin_kpad_clear_irq();
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int __devinit bfin_kpad_probe(struct platform_device *pdev)
>> +{
>> + struct bf54x_kpad *bf54x_kpad;
>> + struct bfin_kpad_platform_data *pdata =
pdev->dev.platform_data;
>> + int i, error;
>> +
>> +
>> + if (!pdata->rows || !pdata->cols || !pdata->keymap) {
>> + printk(KERN_ERR DRV_NAME"No rows, cols or keymap from
>pdata\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!pdata->keymapsize || pdata->keymapsize > (pdata->rows *
>pdata->cols)) {
>> + printk(KERN_ERR DRV_NAME"Invalid keymapsize\n");
>> + return -EINVAL;
>> + }
>> +
>> + bf54x_kpad = kzalloc(sizeof(struct bf54x_kpad), GFP_KERNEL);
>> +
>> + if (!bf54x_kpad) {
>> + return -ENOMEM;
>> + }
>
>Loose extra braces please.
>
>> +
>> + platform_set_drvdata(pdev, bf54x_kpad);
>> +
>> +
>> + if (!pdata->debounce_time || !pdata->debounce_time > MAX_MULT
||
>> + !pdata->coldrive_time || !pdata->coldrive_time >
>MAX_MULT) {
>> + printk(KERN_ERR DRV_NAME
>> + "Invalid Debounce/Columdrive Time from
pdata\n");
>> + bfin_write_KPAD_MSEL(0xFF0);/* Default MSEL */
>> + } else {
>> + bfin_write_KPAD_MSEL(((pdata->debounce_time /
TIME_SCALE)
>> +& DBON_SCALE) | (((pdata->coldrive_time /
>TIME_SCALE) << 8)
>> + & COLDRV_SCALE));
>> +
>> + }
>> +
>> + if (!pdata->keyup_test_interval) {
>> + bf54x_kpad->keyup_test_jiffies =
msecs_to_jiffies(50);
>> + } else {
>> + bf54x_kpad->keyup_test_jiffies =
>> +
RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API
>-Original Message- >From: David Brownell [mailto:[EMAIL PROTECTED] > >On Friday 17 August 2007, Hennerich, Michael wrote: >> What Mike wants to point out is that a external IRQ is first a GPIO and >> needs to be configured like an INPUT GPIO and then a specific bit needs >> to be set unmask it as IRQ. >> >> So why not use the GPIO infrastructure to setup this pin as GPIO? > >My comments about the advantages of using that infrastructure >for *early* binding captured the key points ... it's "failfast". > >For IRQs you're probably on decently firm ground, since it's >extremely rare that people not handle request_irq() errors. > >Remember, I just pointed out that the "late fail" strategy >is unusual. That doesn't mean it's wrong ... just it'll be >a bit of surprise, some cognitive dissonance to developers >picking up a Blackfin project, potentially more error prone. > Dave, Thanks - we really appreciate your feedback. Please believe me - since a great while we have similar internal discussion how we should handle these things. Things need to be DAU proof. We rather prefer having some verbal runtime messages, than having a system that doesn't do what expected and being silent. (The bootloader doesn't know what kernel modules are being loaded requiring specific HW setup) We also don't fear the memory overhead (compared to the support overhead), the runtime overhead is almost neglectable since these functions are only called once, best case twice (module remove). I see your points - I would prefer having a fix function board suiting all our customers' needs - or something like an x86 system where everything is fixed or dedicated and abstracted by IO/Memory and IRQ. -Michael >- Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API
>-Original Message- >From: David Brownell [mailto:[EMAIL PROTECTED] > >On Friday 17 August 2007, Mike Frysinger wrote: >> On 8/17/07, David Brownell <[EMAIL PROTECTED]> wrote: >> > ... >> > Just for the record, this is an unusual way to use these calls. >> > >> > Other platforms completely decouple these issues from the >> > IRQ infrastructure ... doing the pinmux and gpio claiming >> > separately from the request_irq()/free_irq() paths, mostly >> > as part of board setup. Doing all of that "early": >> > >> > - keeps those error returns from causing hard-to-track-down >> >runtime bugs; >> > >> > - works always, even on platforms where a given IRQ may >> >appear on any of several pins/balls; >> > >> > - makes it easier to cross-check against board schematics, >> >by keeping most board-specific setup in one source file; >> > >> > - shrinks the kernel's runtime footprint; >> > >> > - allows the label to be more descriptive ... describeing >> >exactly *which* IRQ, so that using the labels for better >> >diagnostics actually gives better diagnostics. >> > >> > Again, not "wrong"; but probably sub-optimal. You might >> > want to move towards earlier binding now, while Linux is >> > still young on Blackfin and you don't have legacy code to >> > worry about. >> >> in the Blackfin port, if you want to use a pin as an IRQ rather than >> GPIO, you use the normal request_irq/free_irq API ... those functions >> will call back into the proper GPIO/PORTMUX code so that the pin is >> setup properly. this is done so that code isnt duplicated across >> files and so that we can easily detect if someone does something >> incorrect like try to take the same pin and use it as >> irq/gpio/whatever at the same time ... >> >> are you saying that other ports dont unify the backend code paths at all >? > >Some platforms try to "unify" the pin setup in the boot loader. >Most of them cope with bogus bootloaders by doing it in the board >setup code. > >I don't know of any who try to do it "late" as you summarized. > >See above why "late" unification is not necessarily as good as >"early" unification. > >And then there's the OMAP1 example, where for example you might >know that you want MPUIO-0 but that's insufficient to tell whether >you must mux ball F18 or R13 ... so it's impossible to do the kind >of "late" unification done here, and pinmux *MUST* be separate from >IRQ setup. Dave, We are not talking about PIN routing. One physical PIN/BALL can only have one dedicated function the same time. It's more about telling about possible conflicts, on a development board level. What Mike wants to point out is that a external IRQ is first a GPIO and needs to be configured like an INPUT GPIO and then a specific bit needs to be set unmask it as IRQ. So why not use the GPIO infrastructure to setup this pin as GPIO? -Michael > >- Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support
Hi Dave,
>-Original Message-
>From: David Brownell [mailto:[EMAIL PROTECTED]
>Sent: Freitag, 17. August 2007 20:12
>To: Bryan Wu
>Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org;
>[EMAIL PROTECTED]; Michael Hennerich
>Subject: Re: [PATCH 01/12] Blackfin arch: add peripheral resource
>allocation support
>
>On Tuesday 07 August 2007, Bryan Wu wrote:
>> From: Michael Hennerich <[EMAIL PROTECTED]>
>
>The patch description here is IMO misleading, and is clearly
>weak-to-nonexistent ... what this patch does is
>
> * Start tracking the label strings provided by gpio_request()
> * Provide a new portmux mechanisms
> * Start using those in the serial support code
>
Right - our patch descriptions needs to be worked on.
>When I read "resource allocation" I think of "struct resource"
>from , allocate_resource(), and so on. So while
>it's true there are other kinds of driver resource, it's rather
>unnatural for me to think about pin mux and gpio issues in any
>terms other than chip and board setup.
Let me explain a bit. On some Blackfin derivatives almost all PINs can
be GPIOs besides up to 4 alternative functions. For a well experienced
systems engineer being the same time the same guy who does the Hardware
and the Software this is not an issue.
We provide all kind of drivers utilizing almost any peripheral on
Blackfin.
While potentially causing conflicting usage, for someone without
detailed hardware knowledge. The platform device board file is a good
thing to track conflicting memory or IO space resources as well as IRQs.
We also utilize platform device files for exactly these purposes.
The dynamic resource allocation for pinmux and gpio seems to us the best
way to handle things. The "resource allocation" mechanism will spill an
error and dump in case conflicting usage is detected. It'll also tell
you who is causing the conflicting usage.
>
>
>> +static int cmp_label(unsigned short ident, const char *label)
>> +{
>> +if (label && str_ident)
>> +return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
>> + label, strlen(label));
>> +else
>> +return -EINVAL;
>> +}
>
>GRPIO labels are purely for diagnostics. There's no reason to
>compare one to another. You seem to be using these for purposes
>in addition to GPIOs though ... probably worth commenting on that
>unusual scheme.
You are right - diagnostics:
Telling who claimed my resource.
In addition getting a signature, allowing double allocation.
Some drives provide the option for a simple callback function exported
though the platform device file, in order to toggle a GPIO powering up
some external device. Without some additional global external flag it's
pretty had to maintain whether this gpio was allocated before.
In this case I prefer to allow double allocation, for the same purpose.
>
>
>> +int peripheral_request(unsigned short per, const char *label)
>> +{
>> +...
>> +
>> +if (unlikely(reserved_peri_map[gpio_bank(ident)] &
gpio_bit(ident)))
>{
>> +
>> +/*
>> + * Pin functions like AMC address strobes my
>> + * be requested and used by several drivers
>> + */
>> +
>> +if (!(per & P_MAYSHARE)) {
>
>Goofy indentation. And as a rule, drivers have been kept out of
>the business of configuring pin usage. It's simpler that way;
>they don't need to try coping with configuration errors like two
>drivers wanting conflicting usage ... or as you say above, needing
>some explicit sharing mechanism ...
We define some PINs or better single PIN functions to be may shared.
This is only for PINs where the sharing of the function is in nature.
Think about an address strobe or a bus (Busy/Wait) signal, used by
several
drivers/devices sharing the same bus.
>
>
>> +
>> +/*
>> + * Allow that the identical pin function can
>> + * be requested from the same driver twice
>> + */
>
>... or as you say here, needing to structure themselves so they
>don't configure the same usage more than once ...
Same as explained above - this is only for these spots where the
request/free scheme doesn't work.
>
>
>That said, how you handle pinmux on Blackfin is your business.
>
>But you should know that this approach seems idiosyncratic and
>more complex than needed: when pin config is done early and as
>part of board setup, drivers don't need to care about it or to
>handle any pinmux errors. And heck, products can sometimes be
>shipped with the bootloader having done all pinmux setup, so
>Linux won't need to worry about it at all. That can help ship
>multiple board revisions using the same kernel.
This works for fixed function boards. But not for development boards
where we provide lego like add on cards, and allow people to connect
their homebrewn hardware.
Most people/customers I cope with, use the boot loader to only boot the
Linux kernel. The hardware setup we default the processor in the boot
loader might not fit their applications needs.
RE: [PATCH 01/12] Blackfin arch: add peripheral resource allocation support
Bryan,
This patch doesn't seem to be up to date.
It doesn't include the changes made based on feedback from Joe Perches.
Please see our SVN:
Modified: trunk/arch/blackfin/kernel/bfin_gpio.c (3489 => 3490)
-Michael
>-Original Message-
>From: Bryan Wu [mailto:[EMAIL PROTECTED]
>Sent: Mittwoch, 8. August 2007 05:35
>To: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org;
>[EMAIL PROTECTED]
>Cc: [EMAIL PROTECTED]; Michael Hennerich; Bryan Wu
>Subject: [PATCH 01/12] Blackfin arch: add peripheral resource
allocation
>support
>
>From: Michael Hennerich <[EMAIL PROTECTED]>
>
>Signed-off-by: Michael Hennerich <[EMAIL PROTECTED]>
>Signed-off-by: Bryan Wu <[EMAIL PROTECTED]>
>---
> arch/blackfin/kernel/bfin_gpio.c | 272
>++---
> include/asm-blackfin/mach-bf533/bfin_serial_5xx.h | 11 +-
> include/asm-blackfin/mach-bf537/bfin_serial_5xx.h | 23 +-
> include/asm-blackfin/mach-bf537/portmux.h |2 +-
> include/asm-blackfin/mach-bf561/bfin_serial_5xx.h | 11 +-
> 5 files changed, 274 insertions(+), 45 deletions(-)
>
>diff --git a/arch/blackfin/kernel/bfin_gpio.c
>b/arch/blackfin/kernel/bfin_gpio.c
>index bafcfa5..9f30948 100644
>--- a/arch/blackfin/kernel/bfin_gpio.c
>+++ b/arch/blackfin/kernel/bfin_gpio.c
>@@ -84,6 +84,7 @@
> #include
> #include
> #include
>+#include
> #include
>
> #ifdef BF533_FAMILY
>@@ -115,7 +116,11 @@ static struct gpio_port_t
>*gpio_bankb[gpio_bank(MAX_BLACKFIN_GPIOS)] = {
> };
> #endif
>
>-static unsigned short reserved_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
>+static unsigned short
reserved_gpio_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
>+static unsigned short reserved_peri_map[gpio_bank(MAX_BLACKFIN_GPIOS +
>16)];
>+char *str_ident = NULL;
>+
>+#define RESOURCE_LABEL_SIZE 16
>
> #ifdef CONFIG_PM
> static unsigned short wakeup_map[gpio_bank(MAX_BLACKFIN_GPIOS)];
>@@ -143,13 +148,39 @@ inline int check_gpio(unsigned short gpio)
> return 0;
> }
>
>+static void set_label(unsigned short ident, const char *label)
>+{
>+
>+ if (label && str_ident) {
>+ strncpy(str_ident + ident * RESOURCE_LABEL_SIZE, label,
>+ RESOURCE_LABEL_SIZE);
>+ str_ident[ident * RESOURCE_LABEL_SIZE +
>+ RESOURCE_LABEL_SIZE - 1] = 0;
>+ }
>+}
>+
>+static char *get_label(unsigned short ident)
>+{
>+ if (!str_ident)
>+ return "UNKNOWN";
>+
>+ return (str_ident[ident * RESOURCE_LABEL_SIZE] ?
>+ (str_ident + ident * RESOURCE_LABEL_SIZE) : "UNKNOWN");
>+}
>+
>+static int cmp_label(unsigned short ident, const char *label)
>+{
>+ if (label && str_ident)
>+ return strncmp(str_ident + ident * RESOURCE_LABEL_SIZE,
>+ label, strlen(label));
>+ else
>+ return -EINVAL;
>+}
>+
> #ifdef BF537_FAMILY
> static void port_setup(unsigned short gpio, unsigned short usage)
> {
> if (usage == GPIO_USAGE) {
>- if (*port_fer[gpio_bank(gpio)] & gpio_bit(gpio))
>- printk(KERN_WARNING "bfin-gpio: Possible
Conflict with
>Peripheral "
>- "usage and GPIO %d detected!\n", gpio);
> *port_fer[gpio_bank(gpio)] &= ~gpio_bit(gpio);
> } else
> *port_fer[gpio_bank(gpio)] |= gpio_bit(gpio);
>@@ -159,6 +190,56 @@ static void port_setup(unsigned short gpio,
unsigned
>short usage)
> # define port_setup(...) do { } while (0)
> #endif
>
>+#ifdef BF537_FAMILY
>+
>+#define PMUX_LUT_RES 0
>+#define PMUX_LUT_OFFSET 1
>+#define PMUX_LUT_ENTRIES 41
>+#define PMUX_LUT_SIZE 2
>+
>+static unsigned short port_mux_lut[PMUX_LUT_ENTRIES][PMUX_LUT_SIZE] =
{
>+ {P_PPI0_D13, 11}, {P_PPI0_D14, 11}, {P_PPI0_D15, 11},
>+ {P_SPORT1_TFS, 11}, {P_SPORT1_TSCLK, 11}, {P_SPORT1_DTPRI, 11},
>+ {P_PPI0_D10, 10}, {P_PPI0_D11, 10}, {P_PPI0_D12, 10},
>+ {P_SPORT1_RSCLK, 10}, {P_SPORT1_RFS, 10}, {P_SPORT1_DRPRI, 10},
>+ {P_PPI0_D8, 9}, {P_PPI0_D9, 9}, {P_SPORT1_DRSEC, 9},
>+ {P_SPORT1_DTSEC, 9}, {P_TMR2, 8}, {P_PPI0_FS3, 8}, {P_TMR3, 7},
>+ {P_SPI0_SSEL4, 7}, {P_TMR4, 6}, {P_SPI0_SSEL5, 6}, {P_TMR5, 5},
>+ {P_SPI0_SSEL6, 5}, {P_UART1_RX, 4}, {P_UART1_TX, 4}, {P_TMR6,
4},
>+ {P_TMR7, 4}, {P_UART0_RX, 3}, {P_UART0_TX, 3}, {P_DMAR0, 3},
>+ {P_DMAR1, 3}, {P_SPORT0_DTSEC, 1}, {P_SPORT0_DRSEC, 1},
>+ {P_CAN0_RX, 1}, {P_CAN0_TX, 1}, {P_SPI0_SSEL7, 1},
>+ {P_SPORT0_TFS, 0}, {P_SPORT0_DTPRI, 0}, {P_SPI0_SSEL2, 0},
>+ {P_SPI0_SSEL3, 0}
>+};
>+
>+static void portmux_setup(unsigned short per, unsigned short function)
>+{
>+ u16 y, muxreg, offset;
>+
>+ for (y = 0; y < PMUX_LUT_ENTRIES; y++) {
>+ if (port_mux_lut[y][PMUX_LUT_RES] == per) {
>+
>+ /* SET PORTMUX REG */
>+
>+ offset = port_mux_lut[y][PMUX_LUT_OFFSET];
>+ muxreg = bfin_read_PORT_MUX();
>+
>+ if (offset != 1) {
>+
RE: [GIT PULL try#2] Blackfin update
Hi Joe,
>Hi Joe, please reply to all of this email, because some developers need
>to consider your review.
>
>Hi Michael, how do you think of Joe's idea as below.
>
>Thanks,
>Best Regards,
>- Bryan Wu
>
>On Wed, 2007-07-25 at 09:46 -0700, Joe Perches wrote:
>> On Wed, 2007-07-25 at 23:26 +0800, Bryan Wu wrote:
>> > diff --git a/arch/blackfin/kernel/bfin_gpio.c
>> > b/arch/blackfin/kernel/bfin_gpio.c
>> > index bafcfa5..979cf79 100644
>> > --- a/arch/blackfin/kernel/bfin_gpio.c
>> > +++ b/arch/blackfin/kernel/bfin_gpio.c
>> > @@ -143,22 +148,100 @@ inline int check_gpio(unsigned short gpio)
>>
>> []
>>
>> > +#ifdef BF537_FAMILY
>> > +
>> > +#define PMUX_LUT_RES 0
>> > +#define PMUX_LUT_OFFSET 1
>> > +#define PMUX_LUT_ENTRIES 41
>> > +#define PMUX_LUT_SIZE 2
>> > +
>> > +static unsigned short
port_mux_lut[PMUX_LUT_ENTRIES][PMUX_LUT_SIZE] =
>{
>> > + {P_PPI0_D13, 11}, {P_PPI0_D14, 11}, {P_PPI0_D15, 11},
>> > + {P_SPORT1_TFS, 11}, {P_SPORT1_TSCLK, 11}, {P_SPORT1_DTPRI, 11},
>> > + {P_PPI0_D10, 10}, {P_PPI0_D11, 10}, {P_PPI0_D12, 10},
>> > + {P_SPORT1_RSCLK, 10}, {P_SPORT1_RFS, 10}, {P_SPORT1_DRPRI, 10},
>> > + {P_PPI0_D8, 9}, {P_PPI0_D9, 9}, {P_SPORT1_DRSEC, 9},
>> > + {P_SPORT1_DTSEC, 9}, {P_TMR2, 8}, {P_PPI0_FS3, 8}, {P_TMR3, 7},
>> > + {P_SPI0_SSEL4, 7}, {P_TMR4, 6}, {P_SPI0_SSEL5, 6}, {P_TMR5, 5},
>> > + {P_SPI0_SSEL6, 5}, {P_UART1_RX, 4}, {P_UART1_TX, 4}, {P_TMR6,
4},
>> > + {P_TMR7, 4}, {P_UART0_RX, 3}, {P_UART0_TX, 3}, {P_DMAR0, 3},
>> > + {P_DMAR1, 3}, {P_SPORT0_DTSEC, 1}, {P_SPORT0_DRSEC, 1},
>> > + {P_CAN0_RX, 1}, {P_CAN0_TX, 1}, {P_SPI0_SSEL7, 1},
>> > + {P_SPORT0_TFS, 0}, {P_SPORT0_DTPRI, 0}, {P_SPI0_SSEL2, 0},
>> > + {P_SPI0_SSEL3, 0}
>> > +};
>>
>> Wouldn't this be better as a struct?
>>
>> struct PMUX_LUT {
>> unsigned short res;
>> unsigned short offset;
>> }
>>
>> struct PMUX_LUT port_mux_lut = {
>> {.res = P_PPI0_D13, .offset = 11},
>> {.res = P_PPi0_D14, .offset = 11},
>> etc?
>> };
>>
Sure - makes much more sense
>> > +
>> > +static void portmux_setup(unsigned short per, unsigned short
>> > function)
>> > +{
>> > + u16 y, muxreg, offset;
>>
>> Shouldn't y be int?
Not necessarily.
Y is the loop counter which counts up to PMUX_LUT_ENTRIES.
I preferably use 16bit variables for loop counters in case I know they
would never overflow.
Reason why I'm doing this is because, Blackfin features zero overhead
hardware loops. Unfortunately the loop counter register is only 16-bit.
Gcc will turn this into slightly better code.
>>
>> > @@ -179,22 +262,15 @@ static void default_gpio(unsigned short gpio)
>> >
>> > static int __init bfin_gpio_init(void)
>> > {
>> > - int i;
>> > -
>> > - printk(KERN_INFO "Blackfin GPIO Controller\n");
>> >
>> > - for (i = 0; i < MAX_BLACKFIN_GPIOS; i += GPIO_BANKSIZE)
>> > - reserved_map[gpio_bank(i)] = 0;
>> > + str_ident = kzalloc(RESOURCE_LABEL_SIZE * 256, GFP_KERNEL);
>>
>> 256?
Just a number on top of my head (one page of memory), that fits current
and future needs. I guess we should better turn this into a define.
>>
>> perhaps better to use kcalloc(RESOURCE_LABEL_SIZE, 256, GFP_KERNEL)
>> and reference str_ident as an array of char* instead of the
>> multiplies in set_label, get_label and cmp_label?
>>
>> > + if (!str_ident)
>> > + return -ENOMEM;
>>
Sure - much nicer to read.
>> cheers, Joe
Thanks for the feedback!
Regards, Michael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 12/32] Blackfin arch: Fix bug using usb keyboard crashes kernel
I'm also not an expert...
But without conswitchp preset (potential fix):
During initcalls: con_init is called, and returns because of
!display_desc.
static int __init con_init(void)
{
const char *display_desc = NULL;
struct vc_data *vc;
unsigned int currcons = 0, i;
acquire_console_sem();
if (conswitchp)
display_desc = conswitchp->con_startup();
if (!display_desc) {
fg_console = 0;
release_console_sem();
return 0; // RETURNS HERE
}
--snip--
}
At this point there is no memory allocated for vc_cons[].d
A bit later vty_init calls kbd_init.
int __init vty_init(void)
{
--snip--
kbd_init();
--snip--
}
>From now on events are passed to kbd_event which will then call
kbd_keycode.
I don't see where vc_cons[].d in between there is initialized.
>-Original Message-
>From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Pekka
>Enberg
>Sent: Montag, 21. Mai 2007 14:51
>To: Hennerich, Michael
>Cc: Bryan Wu; [EMAIL PROTECTED]; [EMAIL PROTECTED];
>linux-kernel@vger.kernel.org
>Subject: Re: [PATCH 12/32] Blackfin arch: Fix bug using usb keyboard
>crashes kernel
>
>On 5/21/07, Hennerich, Michael <[EMAIL PROTECTED]> wrote:
>> With CONFIG_VT (drivers/char/vt.c) enabled and a USB HID keyboard
>connected,
>> we were seeing bad pointer dereferences in drivers/char/keyboard.c
>>
>> In function kbd_keycode vc_cons[fg_console].d was un-initialized.
>
>On 5/21/07, Pekka Enberg <[EMAIL PROTECTED]> wrote:
>> Makes sense. Please consider adding this to the changelog. Thanks.
>
>I am not an expert on this, but I don't see how vc_cons[fg_console].d
>would be uninitialized. It is always set in
>drivers/char/vt.c:con_init() and drivers/char/vt.c:vc_allocate(). The
>conswitchp change affects vc->vc_sw but I don't see that being used in
>drivers/char/keyboard.c:kbd_keycode() except indirectly via
>set_console et al.
>
>Perhaps I am missing something here?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 12/32] Blackfin arch: Fix bug using usb keyboard crashes kernel
I was fixing this issue some time ago.
With CONFIG_VT (drivers/char/vt.c) enabled and a USB HID keyboard connected, we
were seeing bad pointer dereferences in drivers/char/keyboard.c
In function kbd_keycode vc_cons[fg_console].d was un-initialized .
static void kbd_keycode(unsigned int keycode, int down, int hw_raw)
{
--snip--
struct vc_data *vc = vc_cons[fg_console].d;
--snip--
tty = vc->vc_tty;
--snip--
}
The workaround, almost any arch does is to initialize conswitchp with the dummy
console.
conswitchp = &dummy_con;
The dummy console gets automatically selected if there is no other suitable
console (VGA).
The bit we were missing is simply this fix.
Best regards,
Michael
--
* Analog Devices GmbH [EMAIL PROTECTED]
** * Systems Engineering
** ** Wilhelm-Wagenfeld-Strasse 6
** * D-80807 Munich
* Germany
Registergericht München HRB 40368, Geschäftsführer: Thomas Wessel, William A.
Martin, Margaret Seif
>-Original Message-
>From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Pekka
>Enberg
>Sent: Montag, 21. Mai 2007 13:40
>To: Bryan Wu
>Cc: [EMAIL PROTECTED]; [EMAIL PROTECTED]; linux-
>[EMAIL PROTECTED]; Michael Hennerich
>Subject: Re: [PATCH 12/32] Blackfin arch: Fix bug using usb keyboard
>crashes kernel
>
>Hi Bryan,
>
>On 5/21/07, Bryan Wu <[EMAIL PROTECTED]> wrote:
>> +#ifdef CONFIG_DUMMY_CONSOLE
>> + conswitchp = &dummy_con;
>> +#endif
>> cclk = get_cclk();
>> sclk = get_sclk();
>
>This patch has no changelog. While it is probably apparent to you why
>this fixes a crash when using an USB keyboard, it would be nice for
>the rest of us to know which crash it fixes and why.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] fix unaligned exception in /drivers/net/wireless/orinoco.c
This short patch prevents an unaligned exception to occur. (GCC 4.1)
tmp is defined as char pointer while it is later accessed as short.
Best regards,
Michael
Index: linux-2.6.19.2/drivers/net/wireless/orinoco.c
===
--- linux-2.6.x/drivers/net/wireless/orinoco.c (Revision 2649)
+++ linux-2.6.x/drivers/net/wireless/orinoco.c (Arbeitskopie)
@@ -2053,7 +2053,7 @@
int err;
struct comp_id nic_id, sta_id;
unsigned int firmver;
- char tmp[SYMBOL_MAX_VER_LEN+1];
+ char tmp[SYMBOL_MAX_VER_LEN+1]__attribute__((aligned(2)));
/* Get the hardware version */
err = HERMES_READ_RECORD(hw, USER_BAP, HERMES_RID_NICID,
&nic_id);
Data access misaligned address violation
- Attempted misaligned data memory or data cache access.
CURRENT PROCESS:
COMM=insmod PID=38
TEXT = 0x03694000-0x03697d08 DATA = 0x031904f8-0x0319bc18
BSS = 0x0319bc18-0x0316 USER-STACK = 0x0317fe60
return address: 0x000a326a; contents of [PC-16...PC+8]:
RETE: RETN: 03183d48 RETX: 000a326a RETS: 0313a614
IPEND: 8030 SYSCFG: 0036
SEQSTAT: 0024SP: 03183c6c
R0: R1: 03183d9fR2: R3: c000
R4: 0036R5: fc0eR6: 0012R7:
P0: 20310036P1: 03183d9fP2: 0011P3: 03678800
P4: 03183de2P5: 03678b84FP: 03678b84
A0.w: A0.x: A1.w: A1.x:
LB0: 000a326a LT0: 000a3268 LC0: 0011
LB1: 000a291c LT1: 000a2912 LC1:
B0: 004c L0: M0: 001f I0: 0308eb04
B1: 0067 L1: M1: 0365ff88 I1: 03183d74
B2: 0014458c L2: M2: I2: fffef6f2
B3: L3: M3: I3:
USP: 0317e0e8 ASTAT: 02002060
DCPLB_FAULT_ADDR=03183da0
ICPLB_FAULT_ADDR=000a326a
Hardware Trace:
0 Target : <0xc4a8> { _trap_c + 0x0 }
Source : <0x00010018> { _exception_to_level5 + 0xb4 }
1 Target : <0xff64> { _exception_to_level5 + 0x0 }
Source : <0xff62> { _ex_trap_c + 0x4e }
2 Target : <0xff14> { _ex_trap_c + 0x0 }
Source : <0x000100b8> { _trap + 0x28 }
3 Target : <0x00010090> { _trap + 0x0 }
Source : <0x000a3268> { _insw + 0x10 }
4 Target : <0x000a3258> { _insw + 0x0 }
Source : <0x0313a612> { :hermes:_hermes_read_ltv + 0xfe }
5 Target : <0x0313a5da> { :hermes:_hermes_read_ltv + 0xc6 }
Source : <0x0313a5b8> { :hermes:_hermes_read_ltv + 0xa4 }
6 Target : <0x0313a5a0> { :hermes:_hermes_read_ltv + 0x8c }
Source : <0x0313a59a> { :hermes:_hermes_read_ltv + 0x86 }
7 Target : <0x0313a570> { :hermes:_hermes_read_ltv + 0x5c }
Source : <0x0313a0e4> { :hermes:_hermes_bap_seek + 0x2c }
8 Target : <0x0313a0dc> { :hermes:_hermes_bap_seek + 0x24 }
Source : <0x0313a252> { :hermes:_hermes_bap_seek + 0x19a }
9 Target : <0x0313a250> { :hermes:_hermes_bap_seek + 0x198 }
Source : <0x0313a1f4> { :hermes:_hermes_bap_seek + 0x13c }
10 Target : <0x0313a1f2> { :hermes:_hermes_bap_seek + 0x13a }
Source : <0x0313a1ea> { :hermes:_hermes_bap_seek + 0x132 }
11 Target : <0x0313a1c4> { :hermes:_hermes_bap_seek + 0x10c }
Source : <0x0313a1f0> { :hermes:_hermes_bap_seek + 0x138 }
12 Target : <0x0313a1c4> { :hermes:_hermes_bap_seek + 0x10c }
Source : <0x0313a1f0> { :hermes:_hermes_bap_seek + 0x138 }
13 Target : <0x0313a1c4> { :hermes:_hermes_bap_seek + 0x10c }
Source : <0x0313a1f0> { :hermes:_hermes_bap_seek + 0x138 }
14 Target : <0x0313a1c4> { :hermes:_hermes_bap_seek + 0x10c }
Source : <0x0313a1f0> { :hermes:_hermes_bap_seek + 0x138 }
15 Target : <0x0313a1c4> { :hermes:_hermes_bap_seek + 0x10c }
Source : <0x0313a1f0> { :hermes:_hermes_bap_seek + 0x138 }
Stack from 03183c4c:
cb12 0001001c 0017e710 0017e710 0017e70c 03183d9c 000a2cce
001b6974
000a326a 8030 0024 03183d48 000a326a 000a326a
0313a614
02002060 000a291c 000a326a 000a2912 000a3268
0011
0014458c 0067
004c
0365ff88
001f
fffef6f2 03183d74 0308eb04 0317e0e8 03678b84 03678b84
03183de2
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
