Re: [PATCH] macintosh: Switch i2c drivers back to use .probe()

[Dmitry Torokhov]

On Tue, May 23, 2023 at 09:50:53PM +0200, Uwe Kleine-König wrote:
> After commit b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new()
> call-back type"), all drivers being converted to .probe_new() and then
> 03c835f498b5 ("i2c: Switch .probe() to not take an id parameter") convert
> back to (the new) .probe() to be able to eventually drop .probe_new() from
> struct i2c_driver.
> 
> Signed-off-by: Uwe Kleine-König 

Reviewed-by: Dmitry Torokhov 

Thanks.

-- 
Dmitry

Re: [PATCH v4 08/18] gpiolib: remove gpio_set_debounce()

[Dmitry Torokhov]

On Wed, Feb 08, 2023 at 07:33:33PM +0200, Andy Shevchenko wrote:
> From: Arnd Bergmann 
> 
> gpio_set_debounce() only has a single user, which is trivially
> converted to gpiod_set_debounce().
> 
> Signed-off-by: Arnd Bergmann 
> Reviewed-by: Linus Walleij 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Andy Shevchenko 

Acked-by: Dmitry Torokhov 

Thanks.

-- 
Dmitry

Re: [PATCH v3 04/12] gpiolib: remove gpio_set_debounce

[Dmitry Torokhov]

On Tue, Feb 07, 2023 at 04:29:44PM +0200, Andy Shevchenko wrote:
> @@ -1010,14 +1009,21 @@ static int ads7846_setup_pendown(struct spi_device 
> *spi,
>   }
>  
>   ts->gpio_pendown = pdata->gpio_pendown;
> -
> - if (pdata->gpio_pendown_debounce)
> - gpio_set_debounce(pdata->gpio_pendown,
> -   pdata->gpio_pendown_debounce);

Can we please change only this to:

gpiod_set_debounce(gpio_to_desc(pdata->gpio_pendown),
   pdata->gpio_pendown_debounce);

and not change anything else (i.e. drop the changes below)?

>   } else {
> - dev_err(>dev, "no get_pendown_state nor gpio_pendown?\n");
> - return -EINVAL;
> + struct gpio_desc *desc;
> +
> + desc = devm_gpiod_get(>dev, "pendown", GPIOD_IN);
> + if (IS_ERR(desc)) {
> + dev_err(>dev, "no get_pendown_state nor 
> gpio_pendown?\n");
> + return PTR_ERR(desc);
> + }
> + gpiod_set_consumer_name(desc, "ads7846_pendown");
> +
> + ts->gpio_pendown = desc_to_gpio(desc);
>   }
> + if (pdata->gpio_pendown_debounce)
> + gpiod_set_debounce(gpio_to_desc(ts->gpio_pendown),
> +pdata->gpio_pendown_debounce);
>  
>   return 0;

Thanks.

-- 
Dmitry

[PATCH v2] soc: fsl: qe: request pins non-exclusively

[Dmitry Torokhov]

Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
qe_pin_request() to request and hold GPIO corresponding to a given pin.
Unfortunately this does not work, as fhci-hcd requests these GPIOs
first, befor calling qe_pin_request() (see
drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
To fix it change qe_pin_request() to request GPIOs non-exclusively, and
free them once the code determines GPIO controller and offset for each
GPIO/pin.

Also reaching deep into gpiolib implementation is not the best idea. We
should either export gpio_chip_hwgpio() or keep converting to the global
gpio numbers space until we fix the driver to implement proper pin
control.

Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
Signed-off-by: Dmitry Torokhov 
---

v2:

 - rebased on top of soc/driver. This will conflict with
   c9eb6e546a23 soc: fsl: qe: Switch to use fwnode instead of of_node
   found in next, the resolution is trivial: accept
   fwnode_device_is_compatible() line found in next.
 - fixed leak of gpiod when gc is not found
 - dropped Linus' reviewed-by as the patch changed

 drivers/soc/fsl/qe/gpio.c   | 51 +
 drivers/usb/host/fhci-hcd.c |  2 +-
 include/soc/fsl/qe/qe.h |  5 ++--
 3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index af9193e7e49b..1440922341d8 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -13,19 +13,12 @@
 #include 
 #include 
 #include 
-#include 
+#include  /* for of_mm_gpio_chip */
 #include 
 #include 
 #include 
 #include 
 #include 
-/*
- * FIXME: this is legacy code that is accessing gpiolib internals in order
- * to implement a custom pin controller. The proper solution is to create
- * a real combined pin control and GPIO driver in drivers/pinctrl. However
- * this hack is here for legacy code reasons.
- */
-#include "../../../gpio/gpiolib.h"
 
 struct qe_gpio_chip {
struct of_mm_gpio_chip mm_gc;
@@ -147,65 +140,70 @@ struct qe_pin {
 * something like qe_pio_controller. Someday.
 */
struct qe_gpio_chip *controller;
-   struct gpio_desc *gpiod;
int num;
 };
 
 /**
  * qe_pin_request - Request a QE pin
- * @np:device node to get a pin from
- * @index: index of a pin in the device tree
+ * @dev:   device to get the pin from
+ * @index: index of the pin in the device tree
  * Context:non-atomic
  *
  * This function return qe_pin so that you could use it with the rest of
  * the QE Pin Multiplexing API.
  */
-struct qe_pin *qe_pin_request(struct device_node *np, int index)
+struct qe_pin *qe_pin_request(struct device *dev, int index)
 {
struct qe_pin *qe_pin;
struct gpio_chip *gc;
struct gpio_desc *gpiod;
+   int gpio_num;
int err;
 
qe_pin = kzalloc(sizeof(*qe_pin), GFP_KERNEL);
if (!qe_pin) {
-   pr_debug("%s: can't allocate memory\n", __func__);
+   dev_dbg(dev, "%s: can't allocate memory\n", __func__);
return ERR_PTR(-ENOMEM);
}
 
-   gpiod = fwnode_gpiod_get_index(of_fwnode_handle(np), NULL, index, 
GPIOD_ASIS, "qe");
-   if (IS_ERR(gpiod)) {
-   err = PTR_ERR(gpiod);
-   goto err0;
-   }
-   if (!gpiod) {
-   err = -EINVAL;
+   /*
+* Request gpio as nonexclusive as it was likely reserved by the
+* caller, and we are not planning on controlling it, we only need
+* the descriptor to the to the gpio chip structure.
+*/
+   gpiod = gpiod_get_index(dev, NULL, index,
+   GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+   err = PTR_ERR_OR_ZERO(gpiod);
+   if (err)
goto err0;
-   }
+
gc = gpiod_to_chip(gpiod);
+   gpio_num = desc_to_gpio(gpiod);
+   /* We no longer need this descriptor */
+   gpiod_put(gpiod);
+
if (WARN_ON(!gc)) {
err = -ENODEV;
goto err0;
}
-   qe_pin->gpiod = gpiod;
+
qe_pin->controller = gpiochip_get_data(gc);
/*
 * FIXME: this gets the local offset on the gpio_chip so that the driver
 * can manipulate pin control settings through its custom API. The real
 * solution is to create a real pin control driver for this.
 */
-   qe_pin->num = gpio_chip_hwgpio(gpiod);
+   qe_pin->num = gpio_num - gc->base;
 
if (!of_device_is_compatible(gc->of_node, "fsl,mpc8323-qe-pario-bank")) 
{
-   pr_debug("%s: tried to get a non-qe pin\n", __func__);
-   gpiod_put(gpiod);
+   dev_dbg(dev, "%s: tried to get a non-qe pin\n", __func__);
err = -EINVAL;
goto err0;
}
return qe_pin;
 err

Re: [RESEND PATCH] soc: fsl: qe: request pins non-exclusively

[Dmitry Torokhov]

On Sun, Dec 04, 2022 at 01:10:19PM +0100, Arnd Bergmann wrote:
> On Sun, Dec 4, 2022, at 05:50, Dmitry Torokhov wrote:
> >
> > SoC team, the problematic patch has been in next for a while and it
> > would be great to get the fix in to make sure the driver is not broken
> > in 6.2. Thanks!
> 
> I have no problem taking thsi patch, but I get a merge conflict that
> I'm not sure how to resolve:
> 
> 
> @@@ -186,23 -182,27 +180,43 @@@ struct qe_pin *qe_pin_request(struct de
> if (WARN_ON(!gc)) {
> err = -ENODEV;
> goto err0;
> ++<<<<<<< HEAD
>  +  }
>  +  qe_pin->gpiod = gpiod;
>  +  qe_pin->controller = gpiochip_get_data(gc);
>  +  /*
>  +   * FIXME: this gets the local offset on the gpio_chip so that the 
> driver
>  +   * can manipulate pin control settings through its custom API. The 
> real
>  +   * solution is to create a real pin control driver for this.
>  +   */
>  +  qe_pin->num = gpio_chip_hwgpio(gpiod);
>  +
>  +  if (!of_device_is_compatible(gc->of_node, 
> "fsl,mpc8323-qe-pario-bank")) {
>  +  pr_debug("%s: tried to get a non-qe pin\n", __func__);
>  +  gpiod_put(gpiod);
> ++===
> +   } else if (!fwnode_device_is_compatible(gc->fwnode,
> +   "fsl,mpc8323-qe-pario-bank")) 
> {
> +   dev_dbg(dev, "%s: tried to get a non-qe pin\n", __func__);
> ++>>>>>>> soc: fsl: qe: request pins non-exclusively
> err = -EINVAL;
> -   goto err0;
> +   } else {
> +   qe_pin->controller = gpiochip_get_data(gc);
> +   /*
> +* FIXME: this gets the local offset on the gpio_chip so that
> +* the driver can manipulate pin control settings through its
> +* custom API. The real solution is to create a real pin 
> control
> +* driver for this.
> +*/
> +   qe_pin->num = desc_to_gpio(gpiod) - gc->base;
> }
> 
> Could you rebase the patch on top of the soc/driver branch in the
> soc tree and send the updated version?

I see, it conflicts with:

c9eb6e546a23 soc: fsl: qe: Switch to use fwnode instead of of_node

that is in next but not in soc/driver tree/branch. OK, I'll rebase and I
just noticed that I was leaking gpiod in case we could not locate gc
(unlikely but still...).

Thanks.

-- 
Dmitry

[RESEND PATCH] soc: fsl: qe: request pins non-exclusively

[Dmitry Torokhov]

Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
qe_pin_request() to request and hold GPIO corresponding to a given pin.
Unfortunately this does not work, as fhci-hcd requests these GPIOs
first, before calling qe_pin_request() (see
drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
To fix it change qe_pin_request() to request GPIOs non-exclusively, and
free them once the code determines GPIO controller and offset for each
GPIO/pin.

Also reaching deep into gpiolib implementation is not the best idea. We
should either export gpio_chip_hwgpio() or keep converting to the global
gpio numbers space until we fix the driver to implement proper pin
control.

Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
Reviewed-by: Linus Walleij 
Signed-off-by: Dmitry Torokhov 
---

SoC team, the problematic patch has been in next for a while and it
would be great to get the fix in to make sure the driver is not broken
in 6.2. Thanks!

 drivers/soc/fsl/qe/gpio.c   | 71 ++---
 drivers/usb/host/fhci-hcd.c |  2 +-
 include/soc/fsl/qe/qe.h |  5 +--
 3 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 0ee887f89deb..5bb71a2b5b7a 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -13,7 +13,7 @@
 #include 
 #include 
 #include 
-#include 
+#include  /* for of_mm_gpio_chip */
 #include 
 #include 
 #include 
@@ -21,13 +21,6 @@
 #include 
 
 #include 
-/*
- * FIXME: this is legacy code that is accessing gpiolib internals in order
- * to implement a custom pin controller. The proper solution is to create
- * a real combined pin control and GPIO driver in drivers/pinctrl. However
- * this hack is here for legacy code reasons.
- */
-#include "../../../gpio/gpiolib.h"
 
 struct qe_gpio_chip {
struct of_mm_gpio_chip mm_gc;
@@ -149,20 +142,19 @@ struct qe_pin {
 * something like qe_pio_controller. Someday.
 */
struct qe_gpio_chip *controller;
-   struct gpio_desc *gpiod;
int num;
 };
 
 /**
  * qe_pin_request - Request a QE pin
- * @np:device node to get a pin from
- * @index: index of a pin in the device tree
+ * @dev:   device to get the pin from
+ * @index: index of the pin in the device tree
  * Context:non-atomic
  *
  * This function return qe_pin so that you could use it with the rest of
  * the QE Pin Multiplexing API.
  */
-struct qe_pin *qe_pin_request(struct device_node *np, int index)
+struct qe_pin *qe_pin_request(struct device *dev, int index)
 {
struct qe_pin *qe_pin;
struct gpio_chip *gc;
@@ -171,40 +163,46 @@ struct qe_pin *qe_pin_request(struct device_node *np, int 
index)
 
qe_pin = kzalloc(sizeof(*qe_pin), GFP_KERNEL);
if (!qe_pin) {
-   pr_debug("%s: can't allocate memory\n", __func__);
+   dev_dbg(dev, "%s: can't allocate memory\n", __func__);
return ERR_PTR(-ENOMEM);
}
 
-   gpiod = fwnode_gpiod_get_index(of_fwnode_handle(np), NULL, index, 
GPIOD_ASIS, "qe");
-   if (IS_ERR(gpiod)) {
-   err = PTR_ERR(gpiod);
-   goto err0;
-   }
-   if (!gpiod) {
-   err = -EINVAL;
+   /*
+* Request gpio as nonexclusive as it was likely was reserved by
+* the caller, and we are not planning on controlling it, we only
+* need the descriptor to the to the gpio chip structure.
+*/
+   gpiod = gpiod_get_index(dev, NULL, index,
+   GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+   err = PTR_ERR_OR_ZERO(gpiod);
+   if (err)
goto err0;
-   }
+
gc = gpiod_to_chip(gpiod);
if (WARN_ON(!gc)) {
err = -ENODEV;
goto err0;
-   }
-   qe_pin->gpiod = gpiod;
-   qe_pin->controller = gpiochip_get_data(gc);
-   /*
-* FIXME: this gets the local offset on the gpio_chip so that the driver
-* can manipulate pin control settings through its custom API. The real
-* solution is to create a real pin control driver for this.
-*/
-   qe_pin->num = gpio_chip_hwgpio(gpiod);
-
-   if (!fwnode_device_is_compatible(gc->fwnode, 
"fsl,mpc8323-qe-pario-bank")) {
-   pr_debug("%s: tried to get a non-qe pin\n", __func__);
-   gpiod_put(gpiod);
+   } else if (!fwnode_device_is_compatible(gc->fwnode,
+   "fsl,mpc8323-qe-pario-bank")) {
+   dev_dbg(dev, "%s: tried to get a non-qe pin\n", __func__);
err = -EINVAL;
-   goto err0;
+   } else {
+   qe_pin->controller = gpiochip_get_data(gc);
+   /*
+* FIXME: this gets the local offset on the gpi

Re: [PATCH] macintosh/mac_hid.c: don't load by default

[Dmitry Torokhov]

On Tue, Nov 15, 2022 at 04:07:53AM +0100, Thomas Weißschuh wrote:
> On 2022-11-14 16:16-0800, Dmitry Torokhov wrote:
> > On Tue, Nov 15, 2022 at 12:54:41AM +0100, Thomas Weißschuh wrote:
> >> Cc Franz who wrote the driver originally.
> >> (I hope I got the correct one)
> >> 
> >> Hi Dmitry,
> >> 
> >> On 2022-11-14 10:33-0800, Dmitry Torokhov wrote:
> >>> On Sun, Nov 13, 2022 at 04:30:22AM +0100, Thomas Weißschuh wrote:
> >>>> There should be no need to automatically load this driver on *all*
> >>>> machines with a keyboard.
> >>>> 
> >>>> This driver is of very limited utility and has to be enabled by the user
> >>>> explicitly anyway.
> >>>> Furthermore its own header comment has deprecated it for 17 years.
> >>> 
> >>> I think if someone does not need a driver they can either not enable it
> >>> or blacklist it in /etc/modprobe.d/... There is no need to break
> >>> module loading in the kernel.
> >> 
> >> But nobody needs the driver as it is autoloaded in its current state.
> >> Without manual configuration after loading the driver does not provide any
> >> functionality.
> >> 
> >> Furthermore the autoloading should load the driver for a specific
> >> hardware/resource that it can provide additional functionality for.
> >> Right now the driver loads automatically for any system that has an input
> >> device with a key and then just does nothing.
> >> 
> >> It only wastes memory and confuses users why it is loaded.
> >> 
> >> If somebody really needs this (fringe) driver it should be on them to load 
> >> it
> >> it instead of everybody else having to disable it.
> > 
> > The driver is not enabled by default, so somebody has to enable it in
> > the first place. How did you end up with it?
> 
> My distro kernel configured it to be enabled as module.

Maybe you should talk to them? Which one is this? Not all distributions
seem to enable it (Debian for example does not).

> So people who want to use it can do so. It would be nice if the rest of us
> wouldn't have to care about it.
> 
> >> Furthermore the file has the following comment since the beginning of the 
> >> git
> >> history in 2005:
> >> 
> >> Copyright (C) 2000 Franz Sirl
> >> 
> >> This file will soon be removed in favor of an uinput userspace tool.
> > 
> > OK, that is a separate topic, if there are no users we can remove the
> > driver. Do we know if this tool ever came into existence?
> 
> One interpretation of it is attached as "mac_hid_userspace.c".
> 
> > What I do not want is to break the autoload for one single driver
> > because somebody enabled it without intending to use and now tries to
> > implement a one-off.
> 
> Is an autoloaded driver that then does not (ever) automatically provide any
> functionality not broken by definition?

No because it does not result in any regression in behavior.

> It was enabled by the distro. Which seems correct, because maybe somebody will
> use it.
> 
> Taken to an illogical extreme: If it is fine for modules to load automatically
> even if they are not useful, why not just always load all available modules?

Well, take for example a driver for a NIC. It is not really useful until
you configure it by assigning an address to the interface, etc. Should
we not automatically load drivers for NICs?

> 
> 
> Maybe we can take the removal of the autoload as a first step of deprecation
> and finally removal of the module.
> To quote you:
> 
> "I'd rather we did not promote from drivers/macintosh to other platforms,
>  but rather removed it. The same functionality can be done from
>  userspace." [0]

I think if you talk to your distro and see if they stop enabling it and
offer your userspace utility as a replacement if they indeed need the
functionality would be the best way of deprecating the driver.

Thanks.

-- 
Dmitry

Re: [PATCH] macintosh/mac_hid.c: don't load by default

[Dmitry Torokhov]

On Tue, Nov 15, 2022 at 12:54:41AM +0100, Thomas Weißschuh wrote:
> Cc Franz who wrote the driver originally.
> (I hope I got the correct one)
> 
> Hi Dmitry,
> 
> On 2022-11-14 10:33-0800, Dmitry Torokhov wrote:
> > On Sun, Nov 13, 2022 at 04:30:22AM +0100, Thomas Weißschuh wrote:
> >> There should be no need to automatically load this driver on *all*
> >> machines with a keyboard.
> >> 
> >> This driver is of very limited utility and has to be enabled by the user
> >> explicitly anyway.
> >> Furthermore its own header comment has deprecated it for 17 years.
> > 
> > I think if someone does not need a driver they can either not enable it
> > or blacklist it in /etc/modprobe.d/... There is no need to break
> > module loading in the kernel.
> 
> But nobody needs the driver as it is autoloaded in its current state.
> Without manual configuration after loading the driver does not provide any
> functionality.
> 
> Furthermore the autoloading should load the driver for a specific
> hardware/resource that it can provide additional functionality for.
> Right now the driver loads automatically for any system that has an input
> device with a key and then just does nothing.
> 
> It only wastes memory and confuses users why it is loaded.
> 
> If somebody really needs this (fringe) driver it should be on them to load it
> it instead of everybody else having to disable it.

The driver is not enabled by default, so somebody has to enable it in
the first place. How did you end up with it?

> 
> Furthermore the file has the following comment since the beginning of the git
> history in 2005:
> 
> Copyright (C) 2000 Franz Sirl
> 
> This file will soon be removed in favor of an uinput userspace tool.

OK, that is a separate topic, if there are no users we can remove the
driver. Do we know if this tool ever came into existence?

What I do not want is to break the autoload for one single driver
because somebody enabled it without intending to use and now tries to
implement a one-off.

> 
> >> Fixes: 99b089c3c38a ("Input: Mac button emulation - implement as an input 
> >> filter")
> >> Signed-off-by: Thomas Weißschuh 
> >> ---
> >>  drivers/macintosh/mac_hid.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >> 
> >> diff --git a/drivers/macintosh/mac_hid.c b/drivers/macintosh/mac_hid.c
> >> index d8c4d5664145..d01d28890db4 100644
> >> --- a/drivers/macintosh/mac_hid.c
> >> +++ b/drivers/macintosh/mac_hid.c
> >> @@ -149,8 +149,6 @@ static const struct input_device_id 
> >> mac_hid_emumouse_ids[] = {
> >>{ },
> >>  };
> >>  
> >> -MODULE_DEVICE_TABLE(input, mac_hid_emumouse_ids);
> >> -
> >>  static struct input_handler mac_hid_emumouse_handler = {
> >>.filter = mac_hid_emumouse_filter,
> >>.connect= mac_hid_emumouse_connect,
> >> 
> >> base-commit: fef7fd48922d11b22620e19f9c9101647bfe943d
> >> -- 
> >> 2.38.1

Thanks.

-- 
Dmitry

Re: [PATCH] macintosh/mac_hid.c: don't load by default

[Dmitry Torokhov]

Hi Thomas,

On Sun, Nov 13, 2022 at 04:30:22AM +0100, Thomas Weißschuh wrote:
> There should be no need to automatically load this driver on *all*
> machines with a keyboard.
> 
> This driver is of very limited utility and has to be enabled by the user
> explicitly anyway.
> Furthermore its own header comment has deprecated it for 17 years.

I think if someone does not need a driver they can either not enable it
or blacklist it in /etc/modprobe.d/... There is no need to break
module loading in the kernel.

> 
> Fixes: 99b089c3c38a ("Input: Mac button emulation - implement as an input 
> filter")
> Signed-off-by: Thomas Weißschuh 
> ---
>  drivers/macintosh/mac_hid.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/macintosh/mac_hid.c b/drivers/macintosh/mac_hid.c
> index d8c4d5664145..d01d28890db4 100644
> --- a/drivers/macintosh/mac_hid.c
> +++ b/drivers/macintosh/mac_hid.c
> @@ -149,8 +149,6 @@ static const struct input_device_id 
> mac_hid_emumouse_ids[] = {
>   { },
>  };
>  
> -MODULE_DEVICE_TABLE(input, mac_hid_emumouse_ids);
> -
>  static struct input_handler mac_hid_emumouse_handler = {
>   .filter = mac_hid_emumouse_filter,
>   .connect= mac_hid_emumouse_connect,
> 
> base-commit: fef7fd48922d11b22620e19f9c9101647bfe943d
> -- 
> 2.38.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH] soc: fsl: qe: request pins non-exclusively

[Dmitry Torokhov]

On November 8, 2022 2:50:07 AM PST, Linus Walleij  
wrote:
>On Tue, Nov 8, 2022 at 4:16 AM Dmitry Torokhov
> wrote:
>
>> Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
>> qe_pin_request() to request and hold GPIO corresponding to a given pin.
>> Unfortunately this does not work, as fhci-hcd requests these GPIOs
>> first, befor calling qe_pin_request() (see
>> drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
>> To fix it change qe_pin_request() to request GPIOs non-exclusively, and
>> free them once the code determines GPIO controller and offset for each
>> GPIO/pin.
>>
>> Also reaching deep into gpiolib implementation is not the best idea. We
>> should either export gpio_chip_hwgpio() or keep converting to the global
>> gpio numbers space until we fix the driver to implement proper pin
>> control.
>>
>> Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
>> Signed-off-by: Dmitry Torokhov 
>
>Wow! Thanks for fixing this!
>
>Reviewed-by: Linus Walleij 
>
>I just sent that patch into the SoC patch tracker (s...@kernel.org)
>with a not to apply it directly, I suggest you do the same (or ask me
>to sign it off and send it).

I am not really plugged into soc patch/workflow so if you could do that that 
would be wonderful. 

Thanks!


-- 
Dmitry

[PATCH] soc: fsl: qe: request pins non-exclusively

[Dmitry Torokhov]

Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
qe_pin_request() to request and hold GPIO corresponding to a given pin.
Unfortunately this does not work, as fhci-hcd requests these GPIOs
first, befor calling qe_pin_request() (see
drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
To fix it change qe_pin_request() to request GPIOs non-exclusively, and
free them once the code determines GPIO controller and offset for each
GPIO/pin.

Also reaching deep into gpiolib implementation is not the best idea. We
should either export gpio_chip_hwgpio() or keep converting to the global
gpio numbers space until we fix the driver to implement proper pin
control.

Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
Signed-off-by: Dmitry Torokhov 
---

Compiled only, not tested on hardware.

 drivers/soc/fsl/qe/gpio.c   | 71 ++---
 drivers/usb/host/fhci-hcd.c |  2 +-
 include/soc/fsl/qe/qe.h |  5 +--
 3 files changed, 38 insertions(+), 40 deletions(-)

diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 0ee887f89deb..5bb71a2b5b7a 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -13,7 +13,7 @@
 #include 
 #include 
 #include 
-#include 
+#include  /* for of_mm_gpio_chip */
 #include 
 #include 
 #include 
@@ -21,13 +21,6 @@
 #include 
 
 #include 
-/*
- * FIXME: this is legacy code that is accessing gpiolib internals in order
- * to implement a custom pin controller. The proper solution is to create
- * a real combined pin control and GPIO driver in drivers/pinctrl. However
- * this hack is here for legacy code reasons.
- */
-#include "../../../gpio/gpiolib.h"
 
 struct qe_gpio_chip {
struct of_mm_gpio_chip mm_gc;
@@ -149,20 +142,19 @@ struct qe_pin {
 * something like qe_pio_controller. Someday.
 */
struct qe_gpio_chip *controller;
-   struct gpio_desc *gpiod;
int num;
 };
 
 /**
  * qe_pin_request - Request a QE pin
- * @np:device node to get a pin from
- * @index: index of a pin in the device tree
+ * @dev:   device to get the pin from
+ * @index: index of the pin in the device tree
  * Context:non-atomic
  *
  * This function return qe_pin so that you could use it with the rest of
  * the QE Pin Multiplexing API.
  */
-struct qe_pin *qe_pin_request(struct device_node *np, int index)
+struct qe_pin *qe_pin_request(struct device *dev, int index)
 {
struct qe_pin *qe_pin;
struct gpio_chip *gc;
@@ -171,40 +163,46 @@ struct qe_pin *qe_pin_request(struct device_node *np, int 
index)
 
qe_pin = kzalloc(sizeof(*qe_pin), GFP_KERNEL);
if (!qe_pin) {
-   pr_debug("%s: can't allocate memory\n", __func__);
+   dev_dbg(dev, "%s: can't allocate memory\n", __func__);
return ERR_PTR(-ENOMEM);
}
 
-   gpiod = fwnode_gpiod_get_index(of_fwnode_handle(np), NULL, index, 
GPIOD_ASIS, "qe");
-   if (IS_ERR(gpiod)) {
-   err = PTR_ERR(gpiod);
-   goto err0;
-   }
-   if (!gpiod) {
-   err = -EINVAL;
+   /*
+* Request gpio as nonexclusive as it was likely was reserved by
+* the caller, and we are not planning on controlling it, we only
+* need the descriptor to the to the gpio chip structure.
+*/
+   gpiod = gpiod_get_index(dev, NULL, index,
+   GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+   err = PTR_ERR_OR_ZERO(gpiod);
+   if (err)
goto err0;
-   }
+
gc = gpiod_to_chip(gpiod);
if (WARN_ON(!gc)) {
err = -ENODEV;
goto err0;
-   }
-   qe_pin->gpiod = gpiod;
-   qe_pin->controller = gpiochip_get_data(gc);
-   /*
-* FIXME: this gets the local offset on the gpio_chip so that the driver
-* can manipulate pin control settings through its custom API. The real
-* solution is to create a real pin control driver for this.
-*/
-   qe_pin->num = gpio_chip_hwgpio(gpiod);
-
-   if (!fwnode_device_is_compatible(gc->fwnode, 
"fsl,mpc8323-qe-pario-bank")) {
-   pr_debug("%s: tried to get a non-qe pin\n", __func__);
-   gpiod_put(gpiod);
+   } else if (!fwnode_device_is_compatible(gc->fwnode,
+   "fsl,mpc8323-qe-pario-bank")) {
+   dev_dbg(dev, "%s: tried to get a non-qe pin\n", __func__);
err = -EINVAL;
-   goto err0;
+   } else {
+   qe_pin->controller = gpiochip_get_data(gc);
+   /*
+* FIXME: this gets the local offset on the gpio_chip so that
+* the driver can manipulate pin control settings through its
+* custom API. The real soluti

Re: [PATCH] powerpc/warp: switch to using gpiod API

[Dmitry Torokhov]

On Mon, Sep 26, 2022 at 11:03:25PM -0700, Dmitry Torokhov wrote:
> This switches PIKA Warp away from legacy gpio API and to newer gpiod
> API, so that we can eventually deprecate the former.
> 
> Because LEDs are normally driven by leds-gpio driver, but the
> platform code also wants to access the LEDs during thermal shutdown,
> and gpiod API does not allow locating GPIO without requesting it,
> the platform code is now responsible for locating GPIOs through device
> tree and requesting them. It then constructs platform data for
> leds-gpio platform device and registers it. This allows platform
> code to retain access to LED GPIO descriptors and use them when needed.
> 
> Signed-off-by: Dmitry Torokhov 

Gentle ping on this... Could I get a feedback if this is acceptable or
if you want me to rework this somehow?

Thanks!

> ---
> 
> Compiled only, no hardware to test this.
> 
>  arch/powerpc/boot/dts/warp.dts|   4 +-
>  arch/powerpc/platforms/44x/warp.c | 105 ++
>  2 files changed, 94 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/powerpc/boot/dts/warp.dts b/arch/powerpc/boot/dts/warp.dts
> index b4f32740870e..aa62d08e97c2 100644
> --- a/arch/powerpc/boot/dts/warp.dts
> +++ b/arch/powerpc/boot/dts/warp.dts
> @@ -258,14 +258,12 @@ GPIO1: gpio@ef600c00 {
>   };
>  
>   power-leds {
> - compatible = "gpio-leds";
> + compatible = "warp-power-leds";
>   green {
>   gpios = < 0 0>;
> - default-state = "keep";
>   };
>   red {
>   gpios = < 1 0>;
> - default-state = "keep";
>   };
>   };
>  
> diff --git a/arch/powerpc/platforms/44x/warp.c 
> b/arch/powerpc/platforms/44x/warp.c
> index f03432ef010b..cefa313c09f0 100644
> --- a/arch/powerpc/platforms/44x/warp.c
> +++ b/arch/powerpc/platforms/44x/warp.c
> @@ -5,15 +5,17 @@
>   * Copyright (c) 2008-2009 PIKA Technologies
>   *   Sean MacLennan 
>   */
> +#include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  
> @@ -92,8 +94,6 @@ static int __init warp_post_info(void)
>  
>  static LIST_HEAD(dtm_shutdown_list);
>  static void __iomem *dtm_fpga;
> -static unsigned green_led, red_led;
> -
>  
>  struct dtm_shutdown {
>   struct list_head list;
> @@ -101,7 +101,6 @@ struct dtm_shutdown {
>   void *arg;
>  };
>  
> -
>  int pika_dtm_register_shutdown(void (*func)(void *arg), void *arg)
>  {
>   struct dtm_shutdown *shutdown;
> @@ -132,6 +131,35 @@ int pika_dtm_unregister_shutdown(void (*func)(void 
> *arg), void *arg)
>   return -EINVAL;
>  }
>  
> +#define WARP_GREEN_LED   0
> +#define WARP_RED_LED 1
> +
> +static struct gpio_led warp_gpio_led_pins[] = {
> + [WARP_GREEN_LED] = {
> + .name   = "green",
> + .default_state  = LEDS_DEFSTATE_KEEP,
> + .gpiod  = NULL, /* to be filled by pika_setup_leds() */
> + },
> + [WARP_RED_LED] = {
> + .name   = "red",
> + .default_state  = LEDS_DEFSTATE_KEEP,
> + .gpiod  = NULL, /* to be filled by pika_setup_leds() */
> + },
> +};
> +
> +static struct gpio_led_platform_data warp_gpio_led_data = {
> + .leds   = warp_gpio_led_pins,
> + .num_leds   = ARRAY_SIZE(warp_gpio_led_pins),
> +};
> +
> +static struct platform_device warp_gpio_leds = {
> + .name   = "leds-gpio",
> + .id = -1,
> + .dev= {
> + .platform_data = _gpio_led_data,
> + },
> +};
> +
>  static irqreturn_t temp_isr(int irq, void *context)
>  {
>   struct dtm_shutdown *shutdown;
> @@ -139,7 +167,7 @@ static irqreturn_t temp_isr(int irq, void *context)
>  
>   local_irq_disable();
>  
> - gpio_set_value(green_led, 0);
> + gpiod_set_value(warp_gpio_led_pins[WARP_GREEN_LED].gpiod, 0);
>  
>   /* Run through the shutdown list. */
>   list_for_each_entry(shutdown, _shutdown_list, list)
> @@ -153,7 +181,7 @@ static irqreturn_t temp_isr(int irq, void *context)
>   out_be32(dtm_fpga + 0x14, reset);
>   }
>  
> - gpio_

[PATCH] powerpc/sgy_cts1000: convert to using gpiod API and facelift

[Dmitry Torokhov]

This patch converts the driver to newer gpiod API, and away from
OF-specific legacy gpio API that we want to stop using.

While at it, let's address a few more issues:

- switch to using dev_info()/pr_info() and friends
- cancel work when unbinding the driver

Note that the original code handled halt GPIO polarity incorrectly:
in halt callback, when line polarity is "low" it would set trigger to
"1" and drive halt line high, which is counter to the annotation.
gpiod API will drive such line low. However I do not see any DTSes
in mainline that have a DT node with "sgy,gpio-halt" compatible.

Signed-off-by: Dmitry Torokhov 
---

Cross-compiled with gcc-12.1.0, not tested on hardware.

 arch/powerpc/platforms/85xx/sgy_cts1000.c | 132 +-
 1 file changed, 53 insertions(+), 79 deletions(-)

diff --git a/arch/powerpc/platforms/85xx/sgy_cts1000.c 
b/arch/powerpc/platforms/85xx/sgy_cts1000.c
index e14d1b74d4e4..751395cbf022 100644
--- a/arch/powerpc/platforms/85xx/sgy_cts1000.c
+++ b/arch/powerpc/platforms/85xx/sgy_cts1000.c
@@ -7,10 +7,13 @@
  * Copyright 2012 by Servergy, Inc.
  */
 
+#define pr_fmt(fmt) "gpio-halt: " fmt
+
+#include 
 #include 
 #include 
+#include 
 #include 
-#include 
 #include 
 #include 
 #include 
@@ -18,7 +21,8 @@
 
 #include 
 
-static struct device_node *halt_node;
+static struct gpio_desc *halt_gpio;
+static int halt_irq;
 
 static const struct of_device_id child_match[] = {
{
@@ -36,23 +40,10 @@ static DECLARE_WORK(gpio_halt_wq, gpio_halt_wfn);
 
 static void __noreturn gpio_halt_cb(void)
 {
-   enum of_gpio_flags flags;
-   int trigger, gpio;
-
-   if (!halt_node)
-   panic("No reset GPIO information was provided in DT\n");
-
-   gpio = of_get_gpio_flags(halt_node, 0, );
-
-   if (!gpio_is_valid(gpio))
-   panic("Provided GPIO is invalid\n");
-
-   trigger = (flags == OF_GPIO_ACTIVE_LOW);
-
-   printk(KERN_INFO "gpio-halt: triggering GPIO.\n");
+   pr_info("triggering GPIO.\n");
 
/* Probably wont return */
-   gpio_set_value(gpio, trigger);
+   gpiod_set_value(halt_gpio, 1);
 
panic("Halt failed\n");
 }
@@ -61,95 +52,78 @@ static void __noreturn gpio_halt_cb(void)
  * to handle the shutdown/poweroff. */
 static irqreturn_t gpio_halt_irq(int irq, void *__data)
 {
-   printk(KERN_INFO "gpio-halt: shutdown due to power button IRQ.\n");
+   struct platform_device *pdev = __data;
+
+   dev_info(>dev, "scheduling shutdown due to power button IRQ\n");
schedule_work(_halt_wq);
 
 return IRQ_HANDLED;
 };
 
-static int gpio_halt_probe(struct platform_device *pdev)
+static int __gpio_halt_probe(struct platform_device *pdev,
+struct device_node *halt_node)
 {
-   enum of_gpio_flags flags;
-   struct device_node *node = pdev->dev.of_node;
-   struct device_node *child_node;
-   int gpio, err, irq;
-   int trigger;
-
-   if (!node)
-   return -ENODEV;
-
-   /* If there's no matching child, this isn't really an error */
-   child_node = of_find_matching_node(node, child_match);
-   if (!child_node)
-   return 0;
-
-   /* Technically we could just read the first one, but punish
-* DT writers for invalid form. */
-   if (of_gpio_count(child_node) != 1) {
-   err = -EINVAL;
-   goto err_put;
-   }
-
-   /* Get the gpio number relative to the dynamic base. */
-   gpio = of_get_gpio_flags(child_node, 0, );
-   if (!gpio_is_valid(gpio)) {
-   err = -EINVAL;
-   goto err_put;
-   }
+   int err;
 
-   err = gpio_request(gpio, "gpio-halt");
+   halt_gpio = fwnode_gpiod_get_index(of_fwnode_handle(halt_node),
+  NULL, 0, GPIOD_OUT_LOW, "gpio-halt");
+   err = PTR_ERR_OR_ZERO(halt_gpio);
if (err) {
-   printk(KERN_ERR "gpio-halt: error requesting GPIO %d.\n",
-  gpio);
-   goto err_put;
+   dev_err(>dev, "failed to request halt GPIO: %d\n", err);
+   return err;
}
 
-   trigger = (flags == OF_GPIO_ACTIVE_LOW);
-
-   gpio_direction_output(gpio, !trigger);
-
/* Now get the IRQ which tells us when the power button is hit */
-   irq = irq_of_parse_and_map(child_node, 0);
-   err = request_irq(irq, gpio_halt_irq, IRQF_TRIGGER_RISING |
- IRQF_TRIGGER_FALLING, "gpio-halt", child_node);
+   halt_irq = irq_of_parse_and_map(halt_node, 0);
+   err = request_irq(halt_irq, gpio_halt_irq,
+ IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+ "gpio-halt", pdev);
if (err) {
-   printk(KERN_ER

[PATCH] powerpc/warp: switch to using gpiod API

[Dmitry Torokhov]

This switches PIKA Warp away from legacy gpio API and to newer gpiod
API, so that we can eventually deprecate the former.

Because LEDs are normally driven by leds-gpio driver, but the
platform code also wants to access the LEDs during thermal shutdown,
and gpiod API does not allow locating GPIO without requesting it,
the platform code is now responsible for locating GPIOs through device
tree and requesting them. It then constructs platform data for
leds-gpio platform device and registers it. This allows platform
code to retain access to LED GPIO descriptors and use them when needed.

Signed-off-by: Dmitry Torokhov 
---

Compiled only, no hardware to test this.

 arch/powerpc/boot/dts/warp.dts|   4 +-
 arch/powerpc/platforms/44x/warp.c | 105 ++
 2 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/boot/dts/warp.dts b/arch/powerpc/boot/dts/warp.dts
index b4f32740870e..aa62d08e97c2 100644
--- a/arch/powerpc/boot/dts/warp.dts
+++ b/arch/powerpc/boot/dts/warp.dts
@@ -258,14 +258,12 @@ GPIO1: gpio@ef600c00 {
};
 
power-leds {
-   compatible = "gpio-leds";
+   compatible = "warp-power-leds";
green {
gpios = < 0 0>;
-   default-state = "keep";
};
red {
gpios = < 1 0>;
-   default-state = "keep";
};
};
 
diff --git a/arch/powerpc/platforms/44x/warp.c 
b/arch/powerpc/platforms/44x/warp.c
index f03432ef010b..cefa313c09f0 100644
--- a/arch/powerpc/platforms/44x/warp.c
+++ b/arch/powerpc/platforms/44x/warp.c
@@ -5,15 +5,17 @@
  * Copyright (c) 2008-2009 PIKA Technologies
  *   Sean MacLennan 
  */
+#include 
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
 #include 
-#include 
+#include 
 #include 
 #include 
 
@@ -92,8 +94,6 @@ static int __init warp_post_info(void)
 
 static LIST_HEAD(dtm_shutdown_list);
 static void __iomem *dtm_fpga;
-static unsigned green_led, red_led;
-
 
 struct dtm_shutdown {
struct list_head list;
@@ -101,7 +101,6 @@ struct dtm_shutdown {
void *arg;
 };
 
-
 int pika_dtm_register_shutdown(void (*func)(void *arg), void *arg)
 {
struct dtm_shutdown *shutdown;
@@ -132,6 +131,35 @@ int pika_dtm_unregister_shutdown(void (*func)(void *arg), 
void *arg)
return -EINVAL;
 }
 
+#define WARP_GREEN_LED 0
+#define WARP_RED_LED   1
+
+static struct gpio_led warp_gpio_led_pins[] = {
+   [WARP_GREEN_LED] = {
+   .name   = "green",
+   .default_state  = LEDS_DEFSTATE_KEEP,
+   .gpiod  = NULL, /* to be filled by pika_setup_leds() */
+   },
+   [WARP_RED_LED] = {
+   .name   = "red",
+   .default_state  = LEDS_DEFSTATE_KEEP,
+   .gpiod  = NULL, /* to be filled by pika_setup_leds() */
+   },
+};
+
+static struct gpio_led_platform_data warp_gpio_led_data = {
+   .leds   = warp_gpio_led_pins,
+   .num_leds   = ARRAY_SIZE(warp_gpio_led_pins),
+};
+
+static struct platform_device warp_gpio_leds = {
+   .name   = "leds-gpio",
+   .id = -1,
+   .dev= {
+   .platform_data = _gpio_led_data,
+   },
+};
+
 static irqreturn_t temp_isr(int irq, void *context)
 {
struct dtm_shutdown *shutdown;
@@ -139,7 +167,7 @@ static irqreturn_t temp_isr(int irq, void *context)
 
local_irq_disable();
 
-   gpio_set_value(green_led, 0);
+   gpiod_set_value(warp_gpio_led_pins[WARP_GREEN_LED].gpiod, 0);
 
/* Run through the shutdown list. */
list_for_each_entry(shutdown, _shutdown_list, list)
@@ -153,7 +181,7 @@ static irqreturn_t temp_isr(int irq, void *context)
out_be32(dtm_fpga + 0x14, reset);
}
 
-   gpio_set_value(red_led, value);
+   gpiod_set_value(warp_gpio_led_pins[WARP_RED_LED].gpiod, value);
value ^= 1;
mdelay(500);
}
@@ -162,25 +190,78 @@ static irqreturn_t temp_isr(int irq, void *context)
return IRQ_HANDLED;
 }
 
+/*
+ * Because green and red power LEDs are normally driven by leds-gpio driver,
+ * but in case of critical temperature shutdown we want to drive them
+ * ourselves, we acquire both and then create leds-gpio platform device
+ * ourselves, instead of doing it through device tree. This way we can still
+ * keep access to the gpios and use them when needed.
+ */
 static int pika_setup_leds(void)
 {
struct device_node *np, *child;
+   struct gpio_desc *gpio;
+   struct gpio_l

Re: Is PPC 44x PIKA Warp board still relevant?

[Dmitry Torokhov]

On Mon, Sep 26, 2022 at 08:41:53PM +1000, Michael Ellerman wrote:
> Christophe Leroy  writes:
> > Hi Dmitry
> >
> > Le 25/09/2022 à 07:06, Dmitry Torokhov a écrit :
> >> Hi Michael, Nick,
> >> 
> >> I was wondering if PIKA Warp board still relevant. The reason for my
> >> question is that I am interested in dropping legacy gpio APIs,
> >> especially OF-specific ones, in favor of newer gpiod APIs, and
> >> arch/powerpc/platforms/44x/warp.c is one of few users of it.
> >
> > As far as I can see, that board is still being sold, see
> >
> > https://www.voipon.co.uk/pika-warp-asterisk-appliance-p-932.html
> 
> On the other hand it looks like PIKA technologies went bankrupt earlier
> this year.
> 
> >> The code in question is supposed to turn off green led and flash red led
> >> in case of overheating, and is doing so by directly accessing GPIOs
> >> owned by led-gpio driver without requesting/allocating them. This is not
> >> really supported with gpiod API, and is not a good practice in general.
> >
> > As far as I can see, it was ported to led-gpio by
> >
> > ba703e1a7a0b powerpc/4xx: Have Warp take advantage of GPIO LEDs 
> > default-state = keep
> > 805e324b7fbd powerpc: Update Warp to use leds-gpio driver
> >
> >> Before I spend much time trying to implement a replacement without
> >> access to the hardware, I wonder if this board is in use at all, and if
> >> it is how important is the feature of flashing red led on critical
> >> temperature shutdown?
> >
> > Don't know who can tell it ?
> 
> I would be surprised if anyone is still running upstream kernels on it.
> 
> I can't find any sign of any activity on the mailing list related to it
> since it was initially merged.
> 
> > Maybe let's perform a more standard implementation is see if anybody 
> > screams ?
> 
> How much work is it to convert it?
> 
> Flashing a LED when the machine dies is nice, but not exactly critical,
> hopefully the machine *isn't* dying that often :)

OK, I think I figured out how to let platform code access the GPIOs
while still using leds-gpio driver for normal operation. I will send a
patch shortly.

Thanks.

-- 
Dmitry

Is PPC 44x PIKA Warp board still relevant?

[Dmitry Torokhov]

Hi Michael, Nick,

I was wondering if PIKA Warp board still relevant. The reason for my
question is that I am interested in dropping legacy gpio APIs,
especially OF-specific ones, in favor of newer gpiod APIs, and
arch/powerpc/platforms/44x/warp.c is one of few users of it.

The code in question is supposed to turn off green led and flash red led
in case of overheating, and is doing so by directly accessing GPIOs
owned by led-gpio driver without requesting/allocating them. This is not
really supported with gpiod API, and is not a good practice in general.
Before I spend much time trying to implement a replacement without
access to the hardware, I wonder if this board is in use at all, and if
it is how important is the feature of flashing red led on critical
temperature shutdown?

Thanks.

-- 
Dmitry

Re: [PATCH] input: i8042: Remove special PowerPC handling

[Dmitry Torokhov]

Hi Michael,

On Wed, May 20, 2020 at 04:07:00PM +1000, Michael Ellerman wrote:
> [ + Dmitry & linux-input ]
> 
> Nathan Chancellor  writes:
> > This causes a build error with CONFIG_WALNUT because kb_cs and kb_data
> > were removed in commit 917f0af9e5a9 ("powerpc: Remove arch/ppc and
> > include/asm-ppc").
> >
> > ld.lld: error: undefined symbol: kb_cs
> >> referenced by i8042-ppcio.h:28 (drivers/input/serio/i8042-ppcio.h:28)
> >> input/serio/i8042.o:(__i8042_command) in archive drivers/built-in.a
> >> referenced by i8042-ppcio.h:28 (drivers/input/serio/i8042-ppcio.h:28)
> >> input/serio/i8042.o:(__i8042_command) in archive drivers/built-in.a
> >> referenced by i8042-ppcio.h:28 (drivers/input/serio/i8042-ppcio.h:28)
> >> input/serio/i8042.o:(__i8042_command) in archive drivers/built-in.a
> >
> > ld.lld: error: undefined symbol: kb_data
> >> referenced by i8042.c:309 (drivers/input/serio/i8042.c:309)
> >> input/serio/i8042.o:(__i8042_command) in archive drivers/built-in.a
> >> referenced by i8042-ppcio.h:33 (drivers/input/serio/i8042-ppcio.h:33)
> >> input/serio/i8042.o:(__i8042_command) in archive drivers/built-in.a
> >> referenced by i8042.c:319 (drivers/input/serio/i8042.c:319)
> >> input/serio/i8042.o:(__i8042_command) in archive drivers/built-in.a
> >> referenced 15 more times
> >
> > Presumably since nobody has noticed this for the last 12 years, there is
> > not anyone actually trying to use this driver so we can just remove this
> > special walnut code and use the generic header so it builds for all
> > configurations.
> >
> > Fixes: 917f0af9e5a9 ("powerpc: Remove arch/ppc and include/asm-ppc")
> > Reported-by: kbuild test robot 
> > Signed-off-by: Nathan Chancellor 
> > ---
> >  drivers/input/serio/i8042-ppcio.h | 57 ---
> >  drivers/input/serio/i8042.h   |  2 --
> >  2 files changed, 59 deletions(-)
> >  delete mode 100644 drivers/input/serio/i8042-ppcio.h
> 
> This LGTM.
> 
> Acked-by: Michael Ellerman  (powerpc)
> 
> I assumed drivers/input/serio would be pretty quiet, but there's
> actually some commits to it in linux-next. So perhaps this should go via
> the input tree.
> 
> Dmitry do you want to take this, or should I take it via powerpc?
> 
> Original patch is here:
>   
> https://lore.kernel.org/lkml/20200518181043.3363953-1-natechancel...@gmail.com

I'm fine with you taking it through powerpc.

Acked-by: Dmitry Torokhov 

Also, while I have your attention ;), could you please ack or take
https://lore.kernel.org/lkml/20191002214854.GA114387@dtor-ws/ as I
believe this is the last user or input_polled_dev API and I would like
to drop it from the tree.

Thanks!

-- 
Dmitry

Re: [PATCH] macintosh/ams-input: switch to using input device polling mode

[Dmitry Torokhov]

On Wed, Oct 02, 2019 at 02:48:54PM -0700, Dmitry Torokhov wrote:
> Now that instances of input_dev support polling mode natively,
> we no longer need to create input_polled_dev instance.

Michael, could you please take this? Or I could push through my tree...

Thanks!

> 
> Signed-off-by: Dmitry Torokhov 
> ---
>  drivers/macintosh/Kconfig |  1 -
>  drivers/macintosh/ams/ams-input.c | 37 +++
>  drivers/macintosh/ams/ams.h   |  4 ++--
>  3 files changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
> index 574e122ae105..da6a943ad746 100644
> --- a/drivers/macintosh/Kconfig
> +++ b/drivers/macintosh/Kconfig
> @@ -247,7 +247,6 @@ config PMAC_RACKMETER
>  config SENSORS_AMS
>   tristate "Apple Motion Sensor driver"
>   depends on PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || 
> (ADB_PMU && !I2C) || I2C)
> - select INPUT_POLLDEV
>   help
> Support for the motion sensor included in PowerBooks. Includes
> implementations for PMU and I2C.
> diff --git a/drivers/macintosh/ams/ams-input.c 
> b/drivers/macintosh/ams/ams-input.c
> index 06a96b3f11de..0da493d449b2 100644
> --- a/drivers/macintosh/ams/ams-input.c
> +++ b/drivers/macintosh/ams/ams-input.c
> @@ -25,9 +25,8 @@ MODULE_PARM_DESC(invert, "Invert input data on X and Y 
> axis");
>  
>  static DEFINE_MUTEX(ams_input_mutex);
>  
> -static void ams_idev_poll(struct input_polled_dev *dev)
> +static void ams_idev_poll(struct input_dev *idev)
>  {
> - struct input_dev *idev = dev->input;
>   s8 x, y, z;
>  
>   mutex_lock(_info.lock);
> @@ -59,14 +58,10 @@ static int ams_input_enable(void)
>   ams_info.ycalib = y;
>   ams_info.zcalib = z;
>  
> - ams_info.idev = input_allocate_polled_device();
> - if (!ams_info.idev)
> + input = input_allocate_device();
> + if (!input)
>   return -ENOMEM;
>  
> - ams_info.idev->poll = ams_idev_poll;
> - ams_info.idev->poll_interval = 25;
> -
> - input = ams_info.idev->input;
>   input->name = "Apple Motion Sensor";
>   input->id.bustype = ams_info.bustype;
>   input->id.vendor = 0;
> @@ -75,28 +70,32 @@ static int ams_input_enable(void)
>   input_set_abs_params(input, ABS_X, -50, 50, 3, 0);
>   input_set_abs_params(input, ABS_Y, -50, 50, 3, 0);
>   input_set_abs_params(input, ABS_Z, -50, 50, 3, 0);
> + input_set_capability(input, EV_KEY, BTN_TOUCH);
>  
> - set_bit(EV_ABS, input->evbit);
> - set_bit(EV_KEY, input->evbit);
> - set_bit(BTN_TOUCH, input->keybit);
> + error = input_setup_polling(input, ams_idev_poll);
> + if (error)
> + goto err_free_input;
>  
> - error = input_register_polled_device(ams_info.idev);
> - if (error) {
> - input_free_polled_device(ams_info.idev);
> - ams_info.idev = NULL;
> - return error;
> - }
> + input_set_poll_interval(input, 25);
>  
> + error = input_register_device(input);
> + if (error)
> + goto err_free_input;
> +
> + ams_info.idev = input;
>   joystick = true;
>  
>   return 0;
> +
> +err_free_input:
> + input_free_device(input);
> + return error;
>  }
>  
>  static void ams_input_disable(void)
>  {
>   if (ams_info.idev) {
> - input_unregister_polled_device(ams_info.idev);
> - input_free_polled_device(ams_info.idev);
> + input_unregister_device(ams_info.idev);
>   ams_info.idev = NULL;
>   }
>  
> diff --git a/drivers/macintosh/ams/ams.h b/drivers/macintosh/ams/ams.h
> index fe8d596f9845..935bdd9cd9a6 100644
> --- a/drivers/macintosh/ams/ams.h
> +++ b/drivers/macintosh/ams/ams.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #include 
> -#include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -51,7 +51,7 @@ struct ams {
>  #endif
>  
>   /* Joystick emulation */
> - struct input_polled_dev *idev;
> + struct input_dev *idev;
>   __u16 bustype;
>  
>   /* calibrated null values */
> -- 
> 2.23.0.444.g18eeb5a265-goog
> 
> 
> -- 
> Dmitry

-- 
Dmitry

[PATCH] macintosh/ams-input: switch to using input device polling mode

[Dmitry Torokhov]

Now that instances of input_dev support polling mode natively,
we no longer need to create input_polled_dev instance.

Signed-off-by: Dmitry Torokhov 
---
 drivers/macintosh/Kconfig |  1 -
 drivers/macintosh/ams/ams-input.c | 37 +++
 drivers/macintosh/ams/ams.h   |  4 ++--
 3 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
index 574e122ae105..da6a943ad746 100644
--- a/drivers/macintosh/Kconfig
+++ b/drivers/macintosh/Kconfig
@@ -247,7 +247,6 @@ config PMAC_RACKMETER
 config SENSORS_AMS
tristate "Apple Motion Sensor driver"
depends on PPC_PMAC && !PPC64 && INPUT && ((ADB_PMU && I2C = y) || 
(ADB_PMU && !I2C) || I2C)
-   select INPUT_POLLDEV
help
  Support for the motion sensor included in PowerBooks. Includes
  implementations for PMU and I2C.
diff --git a/drivers/macintosh/ams/ams-input.c 
b/drivers/macintosh/ams/ams-input.c
index 06a96b3f11de..0da493d449b2 100644
--- a/drivers/macintosh/ams/ams-input.c
+++ b/drivers/macintosh/ams/ams-input.c
@@ -25,9 +25,8 @@ MODULE_PARM_DESC(invert, "Invert input data on X and Y axis");
 
 static DEFINE_MUTEX(ams_input_mutex);
 
-static void ams_idev_poll(struct input_polled_dev *dev)
+static void ams_idev_poll(struct input_dev *idev)
 {
-   struct input_dev *idev = dev->input;
s8 x, y, z;
 
mutex_lock(_info.lock);
@@ -59,14 +58,10 @@ static int ams_input_enable(void)
ams_info.ycalib = y;
ams_info.zcalib = z;
 
-   ams_info.idev = input_allocate_polled_device();
-   if (!ams_info.idev)
+   input = input_allocate_device();
+   if (!input)
return -ENOMEM;
 
-   ams_info.idev->poll = ams_idev_poll;
-   ams_info.idev->poll_interval = 25;
-
-   input = ams_info.idev->input;
input->name = "Apple Motion Sensor";
input->id.bustype = ams_info.bustype;
input->id.vendor = 0;
@@ -75,28 +70,32 @@ static int ams_input_enable(void)
input_set_abs_params(input, ABS_X, -50, 50, 3, 0);
input_set_abs_params(input, ABS_Y, -50, 50, 3, 0);
input_set_abs_params(input, ABS_Z, -50, 50, 3, 0);
+   input_set_capability(input, EV_KEY, BTN_TOUCH);
 
-   set_bit(EV_ABS, input->evbit);
-   set_bit(EV_KEY, input->evbit);
-   set_bit(BTN_TOUCH, input->keybit);
+   error = input_setup_polling(input, ams_idev_poll);
+   if (error)
+   goto err_free_input;
 
-   error = input_register_polled_device(ams_info.idev);
-   if (error) {
-   input_free_polled_device(ams_info.idev);
-   ams_info.idev = NULL;
-   return error;
-   }
+   input_set_poll_interval(input, 25);
 
+   error = input_register_device(input);
+   if (error)
+   goto err_free_input;
+
+   ams_info.idev = input;
joystick = true;
 
return 0;
+
+err_free_input:
+   input_free_device(input);
+   return error;
 }
 
 static void ams_input_disable(void)
 {
if (ams_info.idev) {
-   input_unregister_polled_device(ams_info.idev);
-   input_free_polled_device(ams_info.idev);
+   input_unregister_device(ams_info.idev);
ams_info.idev = NULL;
}
 
diff --git a/drivers/macintosh/ams/ams.h b/drivers/macintosh/ams/ams.h
index fe8d596f9845..935bdd9cd9a6 100644
--- a/drivers/macintosh/ams/ams.h
+++ b/drivers/macintosh/ams/ams.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include 
-#include 
+#include 
 #include 
 #include 
 #include 
@@ -51,7 +51,7 @@ struct ams {
 #endif
 
/* Joystick emulation */
-   struct input_polled_dev *idev;
+   struct input_dev *idev;
__u16 bustype;
 
/* calibrated null values */
-- 
2.23.0.444.g18eeb5a265-goog


-- 
Dmitry

Re: [RESEND PATCH] Input: joystick/analog - Use get_cycles() on PPC

[Dmitry Torokhov]

On Wed, Mar 14, 2018 at 10:17:52PM +1100, Michael Ellerman wrote:
> The analog joystick driver spits a warning at us:
> 
>   drivers/input/joystick/analog.c:176:2: warning: #warning Precise timer
>   not defined for this architecture.
> 
> PPC has get_cycles() so use that.
> 
> Signed-off-by: Michael Ellerman 

Applied, thank you.

> ---
>  drivers/input/joystick/analog.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> This is the third resend, I'll take it via the powerpc tree if no one else
> does.
> 
> cheers
> 
> diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
> index c868a878c84f..a942c4ccd2af 100644
> --- a/drivers/input/joystick/analog.c
> +++ b/drivers/input/joystick/analog.c
> @@ -163,7 +163,7 @@ static unsigned int get_time_pit(void)
>  #define GET_TIME(x)  do { x = (unsigned int)rdtsc(); } while (0)
>  #define DELTA(x,y)   ((y)-(x))
>  #define TIME_NAME"TSC"
> -#elif defined(__alpha__) || defined(CONFIG_MN10300) || defined(CONFIG_ARM) 
> || defined(CONFIG_ARM64) || defined(CONFIG_RISCV) || defined(CONFIG_TILE)
> +#elif defined(__alpha__) || defined(CONFIG_MN10300) || defined(CONFIG_ARM) 
> || defined(CONFIG_ARM64) || defined(CONFIG_PPC) || defined(CONFIG_RISCV) || 
> defined(CONFIG_TILE)
>  #define GET_TIME(x)  do { x = get_cycles(); } while (0)
>  #define DELTA(x,y)   ((y)-(x))
>  #define TIME_NAME"get_cycles"
> -- 
> 2.14.1
> 

-- 
Dmitry

Re: [PATCH] macintosh: move mac_hid driver to input/mouse.

[Dmitry Torokhov]

On Thu, Jun 8, 2017 at 4:07 PM, Peter Hutterer  wrote:
> On Thu, Jun 08, 2017 at 03:18:42PM +0200, Michal Suchánek wrote:
>> This is what evtest reports about my keyboard:
>>
>> Select the device event number [0-12]: 2
>> Input driver version is 1.0.1
>> Input device ID: bus 0x3 vendor 0x413c product 0x2107 version 0x111
>> Input device name: "DELL Dell USB Entry Keyboard"
>> Supported events:
>>   Event type 0 (EV_SYN)
>>   Event type 1 (EV_KEY)
>> Event code 1 (KEY_ESC)
>> Event code 2 (KEY_1)
>> Event code 3 (KEY_2)
>> Event code 4 (KEY_3)
>> ...
>> Event code 193 (KEY_F23)
>> Event code 194 (KEY_F24)
>> Event code 240 (KEY_UNKNOWN)
>> Event code 272 (BTN_LEFT)
>> Event code 273 (BTN_RIGHT)
>> Event code 274 (BTN_MIDDLE)
>>   Event type 4 (EV_MSC)
>> Event code 4 (MSC_SCAN)
>>   Event type 17 (EV_LED)
>> Event code 0 (LED_NUML) state 1
>> Event code 1 (LED_CAPSL) state 0
>> Event code 2 (LED_SCROLLL) state 0
>> Event code 3 (LED_COMPOSE) state 0
>> Event code 4 (LED_KANA) state 0
>> Key repeat handling:
>>   Repeat type 20 (EV_REP)
>> Repeat code 0 (REP_DELAY)
>>   Value250
>> Repeat code 1 (REP_PERIOD)
>>   Value 33
>> Properties:
>
> looks like it's not tagged as ID_INPUT_MOUSE by the default udev rules
> because for that we need x/y axes (either relative for real mice or absolute
> ones for the VMWare USB mouse). This keyboard only has buttons though. So it
> gets ID_INPUT_KEYBOARD for the keys, but no ID_INPUT_MOUSE.
>
> Google isn't overly forthcoming on "DELL Dell USB Entry Keyboard" but the
> few pictures I can find all point to a keyboard that doesn't have any
> physical mouse buttons at all. Does yours have buttons? Can you post a
> picture of it somewhere?
>

Michal is using udev/hwdb to replace some of the keys on his keyboard
to generate BTN_RIGHT/BTN_MIDDLE trying to achive the same end result
as with mac_hid. It is not the default keyboard behavior. Having
another custom udev rule to mark the device as ID_INPUT_MOUSE is a
fair requirement in this case I think.

-- 
Dmitry

Re: [PATCH] macintosh: move mac_hid driver to input/mouse.

[Dmitry Torokhov]

On Wed, Jun 07, 2017 at 06:53:51PM +0200, Michal Suchánek wrote:
> On Sun, 28 May 2017 10:55:40 -0700
> Dmitry Torokhov <dmitry.torok...@gmail.com> wrote:
> 
> > On Sun, May 28, 2017 at 11:47:58AM +0200, Michal Suchanek wrote:
> > > On Tue, 9 May 2017 17:43:27 -0700
> > > Dmitry Torokhov <dmitry.torok...@gmail.com> wrote:
> > >   
> > > > Hi Michal,
> > > > 
> > > > On Tue, May 09, 2017 at 09:14:18PM +0200, Michal Suchanek wrote:  
> > > > > There is nothing mac-specific about this driver. Non-mac
> > > > > hardware with suboptimal built-in pointer devices exists.
> > > > > 
> > > > > This makes it possible to use this emulation not only on x86
> > > > > and ppc notebooks but also on arm and mips.
> > > > 
> > > > I'd rather we did not promote from drivers/macintosh to other
> > > > platforms, but rather removed it. The same functionality can be
> > > > done from userspace.  
> > > 
> > > What is the status of this?  
> > 
> > The same as in above paragraph.
> > 
> > > 
> > > Do you reply to every patch to drivers/input that is not the the
> > > core infrastructure that you would rather drop the driver because
> > > it can be done is in userspace?
> > > 
> > > It sure can be done. Remove everything but the bus drivers and
> > > uinput from drivers/input and the rest can be done in userspace.
> > > 
> > > The question is who does it?
> > > 
> > > Are you saying that you will implement the userspace equivalent?  
> > 
> > No, I spend my time mostly with the kernel.
> > 
> > > 
> > > If not then please do your job as maintainer and accept trivial
> > > patches for perfectly working drivers we have now.  
> > 
> > I am doing my job as a maintainer right now. The driver might have
> > been beneficial 15 years ago, when we did not have better options,
> > but I would rather not continue expanding it's use.
> > 
> > The main problem with the driver is that the functionality it is not
> > easily discoverable by end users. And once you plumb it through
> > userspace to present users with options you might as well handle it
> > all in userspace.
> > 
> > > 
> > > If you want to move drivers/input into userspace I am not against it
> > > but I am not willing to do that for you either.  
> > 
> > Then we are at impasse.
> > 
> > >   
> > > > 
> > > > What hardware do you believe would benefit from this and why?  
> > > 
> > > Any touchpad hardware where you cannot press two buttons at once to
> > > emulate the third button due to hardware design. And any touchpad
> > > hardware on which some of the buttons are broken when it comes to
> > > it.
> > > 
> > > It is built into a notebook and works fine for moving the cursor but
> > > due to lack of usable buttons you still need a mouse to use the
> > > notebook.  
> > 
> > Have you tried simply redefining keymap of your keyboard to emit
> > BTN_RIGHT/BTN_MIDDLE? Both atkbd and HID keyboards support keymap
> > updates from userspace/udev/hwdb and if there is a driver that does
> > not support it I will take patches fixing that.
> 
> Indeed, they do support it. Such keymap update just does not work as
> mouse button regardless of sending the BTN_* event. At least not in X11.
> 
> So what is next?

Teach X11 to handle it properly.

Thanks.

-- 
Dmitry

Re: [PATCH] macintosh: move mac_hid driver to input/mouse.

[Dmitry Torokhov]

On Sun, May 28, 2017 at 11:47:58AM +0200, Michal Suchanek wrote:
> On Tue, 9 May 2017 17:43:27 -0700
> Dmitry Torokhov <dmitry.torok...@gmail.com> wrote:
> 
> > Hi Michal,
> > 
> > On Tue, May 09, 2017 at 09:14:18PM +0200, Michal Suchanek wrote:
> > > There is nothing mac-specific about this driver. Non-mac hardware
> > > with suboptimal built-in pointer devices exists.
> > > 
> > > This makes it possible to use this emulation not only on x86 and ppc
> > > notebooks but also on arm and mips.  
> > 
> > I'd rather we did not promote from drivers/macintosh to other
> > platforms, but rather removed it. The same functionality can be done
> > from userspace.
> 
> What is the status of this?

The same as in above paragraph.

> 
> Do you reply to every patch to drivers/input that is not the the core
> infrastructure that you would rather drop the driver because it can be
> done is in userspace?
> 
> It sure can be done. Remove everything but the bus drivers and uinput
> from drivers/input and the rest can be done in userspace.
> 
> The question is who does it?
> 
> Are you saying that you will implement the userspace equivalent?

No, I spend my time mostly with the kernel.

> 
> If not then please do your job as maintainer and accept trivial patches
> for perfectly working drivers we have now.

I am doing my job as a maintainer right now. The driver might have been
beneficial 15 years ago, when we did not have better options, but I
would rather not continue expanding it's use.

The main problem with the driver is that the functionality it is not
easily discoverable by end users. And once you plumb it through
userspace to present users with options you might as well handle it all
in userspace.

> 
> If you want to move drivers/input into userspace I am not against it
> but I am not willing to do that for you either.

Then we are at impasse.

> 
> > 
> > What hardware do you believe would benefit from this and why?
> 
> Any touchpad hardware where you cannot press two buttons at once to
> emulate the third button due to hardware design. And any touchpad
> hardware on which some of the buttons are broken when it comes to it.
> 
> It is built into a notebook and works fine for moving the cursor but
> due to lack of usable buttons you still need a mouse to use the
> notebook.

Have you tried simply redefining keymap of your keyboard to emit
BTN_RIGHT/BTN_MIDDLE? Both atkbd and HID keyboards support keymap
updates from userspace/udev/hwdb and if there is a driver that does not
support it I will take patches fixing that.

Thanks.

-- 
Dmitry

Re: [PATCH] macintosh: move mac_hid driver to input/mouse.

[Dmitry Torokhov]

Hi Michal,

On Tue, May 09, 2017 at 09:14:18PM +0200, Michal Suchanek wrote:
> There is nothing mac-specific about this driver. Non-mac hardware with
> suboptimal built-in pointer devices exists.
> 
> This makes it possible to use this emulation not only on x86 and ppc
> notebooks but also on arm and mips.

I'd rather we did not promote from drivers/macintosh to other platforms,
but rather removed it. The same functionality can be done from
userspace.

What hardware do you believe would benefit from this and why?

Thanks.

> 
> Signed-off-by: Michal Suchanek 
> ---
>  drivers/input/mouse/Kconfig  | 20 
>  drivers/input/mouse/Makefile |  1 +
>  drivers/{macintosh => input/mouse}/mac_hid.c |  0
>  drivers/macintosh/Kconfig| 17 -
>  drivers/macintosh/Makefile   |  1 -
>  5 files changed, 21 insertions(+), 18 deletions(-)
>  rename drivers/{macintosh => input/mouse}/mac_hid.c (100%)
> 
> diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
> index 89ebb8f39fee..5533fd3a113f 100644
> --- a/drivers/input/mouse/Kconfig
> +++ b/drivers/input/mouse/Kconfig
> @@ -12,6 +12,26 @@ menuconfig INPUT_MOUSE
>  
>  if INPUT_MOUSE
>  
> +config MAC_EMUMOUSEBTN
> + tristate "Support for mouse button 2+3 emulation"
> + depends on SYSCTL && INPUT
> + help
> +   This provides generic support for emulating the 2nd and 3rd mouse
> +   button with keypresses.  If you say Y here, the emulation is still
> +   disabled by default.  The emulation is controlled by these sysctl
> +   entries:
> +   /proc/sys/dev/mac_hid/mouse_button_emulation
> +   /proc/sys/dev/mac_hid/mouse_button2_keycode
> +   /proc/sys/dev/mac_hid/mouse_button3_keycode
> +
> +   If you have an Apple machine with a 1-button mouse, say Y here.
> +
> +   This emulation can be useful on notebooks with suboptimal touchpad
> +   hardware as well.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called mac_hid.
> +
>  config MOUSE_PS2
>   tristate "PS/2 mouse"
>   default y
> diff --git a/drivers/input/mouse/Makefile b/drivers/input/mouse/Makefile
> index 56bf0ad877c6..dfaad1dd8857 100644
> --- a/drivers/input/mouse/Makefile
> +++ b/drivers/input/mouse/Makefile
> @@ -4,6 +4,7 @@
>  
>  # Each configuration option enables a list of files.
>  
> +obj-$(CONFIG_MAC_EMUMOUSEBTN)+= mac_hid.o
>  obj-$(CONFIG_MOUSE_AMIGA)+= amimouse.o
>  obj-$(CONFIG_MOUSE_APPLETOUCH)   += appletouch.o
>  obj-$(CONFIG_MOUSE_ATARI)+= atarimouse.o
> diff --git a/drivers/macintosh/mac_hid.c b/drivers/input/mouse/mac_hid.c
> similarity index 100%
> rename from drivers/macintosh/mac_hid.c
> rename to drivers/input/mouse/mac_hid.c
> diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
> index 97a420c11eed..011df09c5167 100644
> --- a/drivers/macintosh/Kconfig
> +++ b/drivers/macintosh/Kconfig
> @@ -159,23 +159,6 @@ config INPUT_ADBHID
>  
> If unsure, say Y.
>  
> -config MAC_EMUMOUSEBTN
> - tristate "Support for mouse button 2+3 emulation"
> - depends on SYSCTL && INPUT
> - help
> -   This provides generic support for emulating the 2nd and 3rd mouse
> -   button with keypresses.  If you say Y here, the emulation is still
> -   disabled by default.  The emulation is controlled by these sysctl
> -   entries:
> -   /proc/sys/dev/mac_hid/mouse_button_emulation
> -   /proc/sys/dev/mac_hid/mouse_button2_keycode
> -   /proc/sys/dev/mac_hid/mouse_button3_keycode
> -
> -   If you have an Apple machine with a 1-button mouse, say Y here.
> -
> -   To compile this driver as a module, choose M here: the
> -   module will be called mac_hid.
> -
>  config THERM_WINDTUNNEL
>   tristate "Support for thermal management on Windtunnel G4s"
>   depends on I2C && I2C_POWERMAC && PPC_PMAC && !PPC_PMAC64
> diff --git a/drivers/macintosh/Makefile b/drivers/macintosh/Makefile
> index 516eb65bcacc..ab8b1e74d160 100644
> --- a/drivers/macintosh/Makefile
> +++ b/drivers/macintosh/Makefile
> @@ -7,7 +7,6 @@
>  obj-$(CONFIG_PPC_PMAC)   += macio_asic.o macio_sysfs.o
>  
>  obj-$(CONFIG_PMAC_MEDIABAY)  += mediabay.o
> -obj-$(CONFIG_MAC_EMUMOUSEBTN)+= mac_hid.o
>  obj-$(CONFIG_INPUT_ADBHID)   += adbhid.o
>  obj-$(CONFIG_ANSLCD) += ans-lcd.o
>  
> -- 
> 2.10.2
> 

-- 
Dmitry

Re: [PATCH 1/2] powerpc: make use of for_each_node_by_name() instead of open-coding it

[Dmitry Torokhov]

On Tue, Jan 31, 2017 at 05:54:37PM -0800, Dmitry Torokhov wrote:
> Instead of manually coding the loop with of_find_node_by_name(), let's
> switch to the standard macro for iterating over nodes with given name.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
> ---
>  arch/powerpc/platforms/83xx/mpc832x_mds.c | 2 +-
>  arch/powerpc/platforms/83xx/mpc832x_rdb.c | 2 +-
>  arch/powerpc/platforms/83xx/mpc836x_mds.c | 2 +-
>  arch/powerpc/platforms/cell/interrupt.c   | 3 +--
>  arch/powerpc/platforms/cell/setup.c   | 3 +--
>  arch/powerpc/platforms/cell/spider-pic.c  | 3 +--
>  arch/powerpc/platforms/powermac/feature.c | 2 +-
>  7 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/83xx/mpc832x_mds.c 
> b/arch/powerpc/platforms/83xx/mpc832x_mds.c
> index bb7b25acf26f..1e92f806ee60 100644
> --- a/arch/powerpc/platforms/83xx/mpc832x_mds.c
> +++ b/arch/powerpc/platforms/83xx/mpc832x_mds.c
> @@ -75,7 +75,7 @@ static void __init mpc832x_sys_setup_arch(void)
>   par_io_init(np);
>   of_node_put(np);
>  
> - for (np = NULL; (np = of_find_node_by_name(np, "ucc")) != NULL;)
> + for_each_node_by_name(np. "ucc")

Gah, kbuild robot even told me about this typo and I missed it. Please
let me know if you can fix up or if I should resend (or drop). I'd
rather it not get dropped as I want to convert of_find_node_by_name() to
stop dropping reference to the starting node, as half of the users do
not expect it.

Thanks.

-- 
Dmitry

[PATCH 2/2] powerpc: make use of for_each_node_by_type() instead of open-coding it

[Dmitry Torokhov]

Instead of manually coding the loop with of_find_node_by_type(), let's
switch to the standard macro for iterating over nodes with given type.

Also fixed a couple of refcount leaks in the aforementioned loops.

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 arch/powerpc/kernel/setup-common.c   | 9 +++--
 arch/powerpc/platforms/cell/spu_manage.c | 4 ++--
 arch/powerpc/platforms/powermac/pic.c| 7 ---
 arch/powerpc/platforms/powermac/smp.c| 4 ++--
 arch/powerpc/platforms/pseries/lparcfg.c | 4 ++--
 5 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c 
b/arch/powerpc/kernel/setup-common.c
index f516ac508ae3..4a36360c0a67 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -452,13 +452,13 @@ static void __init cpu_init_thread_core_maps(int tpc)
  */
 void __init smp_setup_cpu_maps(void)
 {
-   struct device_node *dn = NULL;
+   struct device_node *dn;
int cpu = 0;
int nthreads = 1;
 
DBG("smp_setup_cpu_maps()\n");
 
-   while ((dn = of_find_node_by_type(dn, "cpu")) && cpu < nr_cpu_ids) {
+   for_each_node_by_type(dn, "cpu") {
const __be32 *intserv;
__be32 cpu_be;
int j, len;
@@ -498,6 +498,11 @@ void __init smp_setup_cpu_maps(void)
set_cpu_possible(cpu, true);
cpu++;
}
+
+   if (cpu >= nr_cpu_ids) {
+   of_node_put(dn);
+   break;
+   }
}
 
/* If no SMT supported, nthreads is forced to 1 */
diff --git a/arch/powerpc/platforms/cell/spu_manage.c 
b/arch/powerpc/platforms/cell/spu_manage.c
index 672d310dcf14..4a28d647911c 100644
--- a/arch/powerpc/platforms/cell/spu_manage.c
+++ b/arch/powerpc/platforms/cell/spu_manage.c
@@ -292,12 +292,12 @@ static int __init of_enumerate_spus(int (*fn)(void *data))
unsigned int n = 0;
 
ret = -ENODEV;
-   for (node = of_find_node_by_type(NULL, "spe");
-   node; node = of_find_node_by_type(node, "spe")) {
+   for_each_node_by_type(node, "spe") {
ret = fn(node);
if (ret) {
printk(KERN_WARNING "%s: Error initializing %s\n",
__func__, node->name);
+   of_node_put(node);
break;
}
n++;
diff --git a/arch/powerpc/platforms/powermac/pic.c 
b/arch/powerpc/platforms/powermac/pic.c
index f5f9ad7c3398..057c0c4003ae 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -486,15 +486,16 @@ static int __init pmac_pic_probe_mpic(void)
struct device_node *np, *master = NULL, *slave = NULL;
 
/* We can have up to 2 MPICs cascaded */
-   for (np = NULL; (np = of_find_node_by_type(np, "open-pic"))
-!= NULL;) {
+   for_each_node_by_type(np, "open-pic") {
if (master == NULL &&
of_get_property(np, "interrupts", NULL) == NULL)
master = of_node_get(np);
else if (slave == NULL)
slave = of_node_get(np);
-   if (master && slave)
+   if (master && slave) {
+   of_node_put(np);
break;
+   }
}
 
/* Check for bogus setups */
diff --git a/arch/powerpc/platforms/powermac/smp.c 
b/arch/powerpc/platforms/powermac/smp.c
index c9eb7d6540ea..1109850d12bd 100644
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -772,8 +772,8 @@ static void __init smp_core99_probe(void)
if (ppc_md.progress) ppc_md.progress("smp_core99_probe", 0x345);
 
/* Count CPUs in the device-tree */
-   for (cpus = NULL; (cpus = of_find_node_by_type(cpus, "cpu")) != 
NULL;)
-   ++ncpus;
+   for_each_node_by_type(cpus, "cpu")
+   ++ncpus;
 
printk(KERN_INFO "PowerMac SMP probe found %d cpus\n", ncpus);
 
diff --git a/arch/powerpc/platforms/pseries/lparcfg.c 
b/arch/powerpc/platforms/pseries/lparcfg.c
index 779fc2a1c8f7..1c1668c6b8c7 100644
--- a/arch/powerpc/platforms/pseries/lparcfg.c
+++ b/arch/powerpc/platforms/pseries/lparcfg.c
@@ -370,10 +370,10 @@ static void parse_system_parameter_string(struct seq_file 
*m)
  */
 static int lparcfg_count_active_processors(void)
 {
-   struct device_node *cpus_dn = NULL;
+   struct device_node *cpus_dn;
int count = 0;
 
-   while ((cpus_dn = of_find_node_by_type(cpus_dn, "cpu"))) {
+   for_each_node_by_type(cpus_dn, "cpu") {
 #ifdef LPARCFG_DEBUG
printk(KERN_ERR "cpus_dn %p\n", cpus_dn);
 #endif
-- 
2.11.0.483.g087da7b7c-goog

[PATCH 1/2] powerpc: make use of for_each_node_by_name() instead of open-coding it

[Dmitry Torokhov]

Instead of manually coding the loop with of_find_node_by_name(), let's
switch to the standard macro for iterating over nodes with given name.

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---
 arch/powerpc/platforms/83xx/mpc832x_mds.c | 2 +-
 arch/powerpc/platforms/83xx/mpc832x_rdb.c | 2 +-
 arch/powerpc/platforms/83xx/mpc836x_mds.c | 2 +-
 arch/powerpc/platforms/cell/interrupt.c   | 3 +--
 arch/powerpc/platforms/cell/setup.c   | 3 +--
 arch/powerpc/platforms/cell/spider-pic.c  | 3 +--
 arch/powerpc/platforms/powermac/feature.c | 2 +-
 7 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/mpc832x_mds.c 
b/arch/powerpc/platforms/83xx/mpc832x_mds.c
index bb7b25acf26f..1e92f806ee60 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_mds.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_mds.c
@@ -75,7 +75,7 @@ static void __init mpc832x_sys_setup_arch(void)
par_io_init(np);
of_node_put(np);
 
-   for (np = NULL; (np = of_find_node_by_name(np, "ucc")) != NULL;)
+   for_each_node_by_name(np. "ucc")
par_io_of_config(np);
}
 
diff --git a/arch/powerpc/platforms/83xx/mpc832x_rdb.c 
b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
index d7c9b186954d..3dcbe06cf36c 100644
--- a/arch/powerpc/platforms/83xx/mpc832x_rdb.c
+++ b/arch/powerpc/platforms/83xx/mpc832x_rdb.c
@@ -204,7 +204,7 @@ static void __init mpc832x_rdb_setup_arch(void)
par_io_init(np);
of_node_put(np);
 
-   for (np = NULL; (np = of_find_node_by_name(np, "ucc")) != NULL;)
+   for_each_node_by_name(np, "ucc")
par_io_of_config(np);
}
 #endif /* CONFIG_QUICC_ENGINE */
diff --git a/arch/powerpc/platforms/83xx/mpc836x_mds.c 
b/arch/powerpc/platforms/83xx/mpc836x_mds.c
index 4fc3051c2b2e..fd44dd03e1f3 100644
--- a/arch/powerpc/platforms/83xx/mpc836x_mds.c
+++ b/arch/powerpc/platforms/83xx/mpc836x_mds.c
@@ -83,7 +83,7 @@ static void __init mpc836x_mds_setup_arch(void)
par_io_init(np);
of_node_put(np);
 
-   for (np = NULL; (np = of_find_node_by_name(np, "ucc")) != NULL;)
+   for_each_node_by_name(np, "ucc")
par_io_of_config(np);
 #ifdef CONFIG_QE_USB
/* Must fixup Par IO before QE GPIO chips are registered. */
diff --git a/arch/powerpc/platforms/cell/interrupt.c 
b/arch/powerpc/platforms/cell/interrupt.c
index a6bbbaba14a3..477a1fdd3ee1 100644
--- a/arch/powerpc/platforms/cell/interrupt.c
+++ b/arch/powerpc/platforms/cell/interrupt.c
@@ -315,8 +315,7 @@ static int __init setup_iic(void)
struct cbe_iic_regs __iomem *node_iic;
const u32 *np;
 
-   for (dn = NULL;
-(dn = of_find_node_by_name(dn,"interrupt-controller")) != NULL;) {
+   for_each_node_by_name(dn, "interrupt-controller") {
if (!of_device_is_compatible(dn,
 "IBM,CBEA-Internal-Interrupt-Controller"))
continue;
diff --git a/arch/powerpc/platforms/cell/setup.c 
b/arch/powerpc/platforms/cell/setup.c
index d3543e68efe8..7d31b8d14661 100644
--- a/arch/powerpc/platforms/cell/setup.c
+++ b/arch/powerpc/platforms/cell/setup.c
@@ -192,8 +192,7 @@ static void __init mpic_init_IRQ(void)
struct device_node *dn;
struct mpic *mpic;
 
-   for (dn = NULL;
-(dn = of_find_node_by_name(dn, "interrupt-controller"));) {
+   for_each_node_by_name(dn, "interrupt-controller") {
if (!of_device_is_compatible(dn, "CBEA,platform-open-pic"))
continue;
 
diff --git a/arch/powerpc/platforms/cell/spider-pic.c 
b/arch/powerpc/platforms/cell/spider-pic.c
index ff924af00e78..a0454b031e6f 100644
--- a/arch/powerpc/platforms/cell/spider-pic.c
+++ b/arch/powerpc/platforms/cell/spider-pic.c
@@ -343,8 +343,7 @@ void __init spider_init_IRQ(void)
 * device-tree is bogus anyway) so all we can do is pray or maybe test
 * the address and deduce the node-id
 */
-   for (dn = NULL;
-(dn = of_find_node_by_name(dn, "interrupt-controller"));) {
+   for_each_node_by_name(dn, "interrupt-controller") {
if (of_device_is_compatible(dn, "CBEA,platform-spider-pic")) {
if (of_address_to_resource(dn, 0, )) {
printk(KERN_WARNING "spider-pic: Failed\n");
diff --git a/arch/powerpc/platforms/powermac/feature.c 
b/arch/powerpc/platforms/powermac/feature.c
index 730e48111848..86ce02f15593 100644
--- a/arch/powerpc/platforms/powermac/feature.c
+++ b/arch/powerpc/platforms/powermac/feature.c
@@ -2641,7 +2641,7 @@ static void __init probe_one_macio(const char *name, 
const

[PATCH] powerpc/powermac: drop useless call to of_find_node_by_name

[Dmitry Torokhov]

We are not using result, so this simply results in a leaked refcount.

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---

Found by visual inspection, not tested...

 arch/powerpc/platforms/powermac/feature.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/platforms/powermac/feature.c 
b/arch/powerpc/platforms/powermac/feature.c
index 1e02328c3f2d..730e48111848 100644
--- a/arch/powerpc/platforms/powermac/feature.c
+++ b/arch/powerpc/platforms/powermac/feature.c
@@ -2853,7 +2853,6 @@ set_initial_features(void)
}
 
/* Enable ATA-100 before PCI probe. */
-   np = of_find_node_by_name(NULL, "ata-6");
for_each_node_by_name(np, "ata-6") {
if (np->parent
&& of_device_is_compatible(np->parent, "uni-north")
-- 
2.11.0.483.g087da7b7c-goog


-- 
Dmitry

[PATCH] powerpc/powermac: fix OF node refcount leak

[Dmitry Torokhov]

We need to call of_node_put() for device nodes obtained with
of_find_node_by_name().

Signed-off-by: Dmitry Torokhov <dmitry.torok...@gmail.com>
---

Found by visual inspection, not tested...

 arch/powerpc/platforms/powermac/pic.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/platforms/powermac/pic.c 
b/arch/powerpc/platforms/powermac/pic.c
index f5f9ad7c3398..d3a9ab0dbcb1 100644
--- a/arch/powerpc/platforms/powermac/pic.c
+++ b/arch/powerpc/platforms/powermac/pic.c
@@ -604,6 +604,7 @@ static int pmacpic_find_viaint(void)
if (np == NULL)
goto not_found;
viaint = irq_of_parse_and_map(np, 0);
+   of_node_put(np);
 
 not_found:
 #endif /* CONFIG_ADB_PMU */
-- 
2.11.0.483.g087da7b7c-goog


-- 
Dmitry

Re: [RFC] fs: add userspace critical mounts event support

[Dmitry Torokhov]

On Fri, Sep 23, 2016 at 6:37 PM, Herbert, Marc <marc.herb...@intel.com> wrote:
> On 03/09/2016 11:10, Dmitry Torokhov wrote:
>> I was thinking if we kernel could post
>> "conditions" (maybe simple stings) that it waits for, and userspace
>> could unlock these "conditions". One of them might be "firmware
>> available".
>
> On idea offered by Josh Triplett that seems to overlap with this one
> is to have something similar to the (deprecated) userhelper with
> *per-blob* requests and notifications except for one major difference:
> userspace would not anymore be in charge of *providing* the blob but
> would instead only *signal* when a given blob becomes available and is
> either found or found missing. Then the kernel loads the blob _by
> itself_; unlike the userhelper. No new “critical filesystem” concept
> and a *per-blob basis*, allowing any variation of blob locations
> across any number of initramfs and filesystems.
>

Really, I do not quite understand why people have issues with usermode
helper/uevents. It used to work reasonably well (if you were using
request_firmware_nowait()), as the kernel would post the request and
then, when userspace was ready[^Hier], uevents would be processed and
firmware would be loaded. We had a timeout of 60(?) seconds by
default, but that would be adjusted as systems needed.

Unfortunately it all broke when udev started insisting [1] on
servicing some uevents in strict sequence, which resulted in boot
stalls. Maybe the ultimate answer is to write a firmware loading
daemon that would also listen to netlink events and do properly what
udev refused to be doing? The distribution would know when it is ready
to service firmware requests (and thus when to start this daemon), and
we would have the freedom of having drivers both built-in and as
modules and bulding firmware into kernel, intiramfs or keep on a
"real" fs available at later time.

Thanks.

-- 
Dmitry

[1] https://lwn.net/Articles/518942/

Re: [RFC] fs: add userspace critical mounts event support

[Dmitry Torokhov]

On Sat, Sep 3, 2016 at 11:01 AM, Linus Torvalds
<torva...@linux-foundation.org> wrote:
> On Sat, Sep 3, 2016 at 10:49 AM, Dmitry Torokhov
> <dmitry.torok...@gmail.com> wrote:
>>
>> Unfortunately module loading and availability of firmware is very
>> loosely coupled.
>
> The whole "let's add a new magical proc entry to say that all
> filesystems are mounted" is all about the user space coupling them.

I will be first to say that the proposed *implementation* is nowhere
near what should be accepted. I was thinking if we kernel could post
"conditions" (maybe simple stings) that it waits for, and userspace
could unlock these "conditions". One of them might be "firmware
available".

>
> I'm just saying that if user space must know about when firmware is
> ready to be loaded anyway, just do it rigth. Not with some hacky "you
> can now do random things" flag. But by having user space actually say
> "put this module together with this firmware".
>
> If you just put the two pieces together, then the module "will just work".
>
> And quite frankly, that sounds like a much better maintenance practice
> anyway. If some module doesn't work without the firmware, then dammit,
> the two *should* go together. Putting them in two different places
> would be *INSANE*.

Quite often it does until it does not. Most of the touch controllers
work just fine until some event (abrupt cutting of power for example)
where nvram gets corrupted and they come up in bootloader mode. It is
just an example.

Thanks.

-- 
Dmitry

Re: [RFC] fs: add userspace critical mounts event support

[Dmitry Torokhov]

On Fri, Sep 02, 2016 at 09:41:18PM -0700, Linus Torvalds wrote:
> On Sep 2, 2016 9:20 PM, "Dmitry Torokhov" <dmitry.torok...@gmail.com> wrote:
> >
> > Like what? Some devices do need to have firmware loaded so we know
> > their capabilities, so we really can't push the firmware loading into
> > "open".
> 
> So you
> (a) document that

Document that device may come up half-broken? Not sure how that would
help end user.

> (b) make the driver only build as a module

Unfortunately module loading and availability of firmware is very
loosely coupled. Of course, if you only load modules from the same
partition that your firmware is on you can get away with it, but if some
of the modules are in initramfs and firmware is on final root fs then
it still does not work. And populating also initramfs with firmware that
might be used once in a 1000 boots is somewhat wasteful. That is not
talking about systems that do not wish to use modules for one reason or
another, or even more esoteric setups where non-essential for boot
firmware can be mounted later over nfs, etc, etc.

> (c) make sure the module and the firmware go together

I do not think it is always possible. Quite often it is though, at the
expense of increasing kernel/initramfs size.

> 
> End of problem.
> 
> Why make up random interfaces for crazy stuff?

Because we want a solution that works well for all cases, simple and
complex. This includes allowing drivers to be built into the kernel but
allow them waiting for additional data (config/firmware) that may become
available later in the game. We just need to be able to tell them when
it does not make sense to wait anymore as the data they want is not
coming, and do it more reliably then simply declaring 10 or 30 or 300
seconds time out.

Thanks.

-- 
Dmitry

Re: [RFC] fs: add userspace critical mounts event support

[Dmitry Torokhov]

On Fri, Sep 2, 2016 at 9:11 PM, Linus Torvalds
 wrote:
> On Fri, Sep 2, 2016 at 5:20 PM, Luis R. Rodriguez  wrote:
>>
>> Thoughts ?
>
> I really think this is a horrible hack.
>
> It's basically the kernel giving up, and relying on user space to give
> a single flag, and it's broken nasty crap.  Worse, it's broken nasty
> crap with a user interface, so we'll be stuck with it forever. Please
> no.

I agree that interface is bad, but I do believe we need something like this...

>
> What are the drivers that need this, and why can't those drivers just
> be fixed to not do braindead things?

Like what? Some devices do need to have firmware loaded so we know
their capabilities, so we really can't push the firmware loading into
"open". If your touch controller for some reason decided to crap over
it's nvram and comes in bootloader mode it is nice for it to slurp in
config data or firmware so use does not have to trigger update
manually. And while the controller is in bootloader mode we can't
create input device because we do not know what capabilities to
declare.

These devices we want to probe asynchronously and simply tell firmware
loader to wait for firmware to become available. The problem we do not
know when to give up, since we do not know where the firmware might
be. But userspace knows and can tel us.

Thanks.

-- 
Dmitry

Re: [PATCH v2 2/5] firmware: annotate thou shalt not request fw on init or probe

[Dmitry Torokhov]

On Thu, Aug 25, 2016 at 12:41 PM, Luis R. Rodriguez  wrote:
> On Thu, Aug 25, 2016 at 01:05:44PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 24, 2016 at 10:39 PM, Luis R. Rodriguez  
>> wrote:

>> > Can they use initramfs for this ?
>>
>> Apparently that's also uncool with the embedded folks.
>
> What's uncool with embedded folks? To use initramfs for firmware ?
> If so can you explain why ?

Because it is not needed? If you are embedded you have an option of
only compiling drivers and services that you need directly into the
kernel, then initramfs is just an extra piece that complicates your
boot process.

Thanks.

-- 
Dmitry

Re: [PATCH 09/14] Input: touchscreen: Broadcom iProc: DT spelling s/clock-name/clock-names/

[Dmitry Torokhov]

On Wed, Apr 20, 2016 at 05:32:14PM +0200, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven 

Applied, thank you.

> ---
>  .../devicetree/bindings/input/touchscreen/brcm,iproc-touchscreen.txt| 2 
> +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git 
> a/Documentation/devicetree/bindings/input/touchscreen/brcm,iproc-touchscreen.txt
>  
> b/Documentation/devicetree/bindings/input/touchscreen/brcm,iproc-touchscreen.txt
> index 25541f30e38701f2..ac5dff412e25266f 100644
> --- 
> a/Documentation/devicetree/bindings/input/touchscreen/brcm,iproc-touchscreen.txt
> +++ 
> b/Documentation/devicetree/bindings/input/touchscreen/brcm,iproc-touchscreen.txt
> @@ -7,7 +7,7 @@ Required properties:
>If this property is selected please make sure MFD_SYSCON config
>is enabled in the defconfig file.
>  - clocks:  The clock provided by the SOC to driver the tsc
> -- clock-name:  name for the clock
> +- clock-names:  name for the clock
>  - interrupts: The touchscreen controller's interrupt
>  - address-cells: Specify the number of u32 entries needed in child nodes.
>Should set to 1.
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3 00/29] bitops: add parity functions

[Dmitry Torokhov]

On Thu, Apr 14, 2016 at 10:36:41AM +0800, zengzhao...@163.com wrote:
>  drivers/input/joystick/grip_mp.c |  16 +--
>  drivers/input/joystick/sidewinder.c  |  24 +
>  drivers/input/mouse/elantech.c   |  10 +-
>  drivers/input/mouse/elantech.h   |   1 -
>  drivers/input/serio/ams_delta_serio.c|   8 +-
>  drivers/input/serio/pcips2.c |   2 +-
>  drivers/input/serio/saps2.c  |   2 +-

For input bits:

Acked-by: Dmitry Torokhov <dmitry.torok...@gmail.com>

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 4/8] xen: Use the correctly the Xen memory terminologies

[Dmitry Torokhov]

On Wed, Aug 05, 2015 at 11:08:55AM +0100, Stefano Stabellini wrote:
 On Tue, 4 Aug 2015, Julien Grall wrote:
  Based on include/xen/mm.h [1], Linux is mistakenly using MFN when GFN
  is meant, I suspect this is because the first support for Xen was for
  PV. This resulted in some misimplementation of helpers on ARM and
  confused developers about the expected behavior.
  
  For instance, with pfn_to_mfn, we expect to get an MFN based on the name.
  Although, if we look at the implementation on x86, it's returning a GFN.
  
  For clarity and avoid new confusion, replace any reference to mfn with
  gfn in any helpers used by PV drivers. The x86 code will still keep some
  reference of pfn_to_mfn but exclusively for PV (a BUG_ON has been added
  to ensure this). No changes as been made in the hypercall field, even
  though they may be invalid, in order to keep the same as the defintion
  in xen repo.
  
  Take also the opportunity to simplify simple construction such
  as pfn_to_mfn(page_to_pfn(page)) into page_to_gfn. More complex clean up
  will come in follow-up patches.
  
  [1] 
  http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=e758ed14f390342513405dd766e874934573e6cb
  
  Signed-off-by: Julien Grall julien.gr...@citrix.com
  Cc: Stefano Stabellini stefano.stabell...@eu.citrix.com
  Cc: Russell King li...@arm.linux.org.uk
  Cc: Konrad Rzeszutek Wilk konrad.w...@oracle.com
  Cc: Boris Ostrovsky boris.ostrov...@oracle.com
  Cc: David Vrabel david.vra...@citrix.com
  Cc: Thomas Gleixner t...@linutronix.de
  Cc: Ingo Molnar mi...@redhat.com
  Cc: H. Peter Anvin h...@zytor.com
  Cc: x...@kernel.org
  Cc: Roger Pau Monné roger@citrix.com
  Cc: Dmitry Torokhov dmitry.torok...@gmail.com
  Cc: Ian Campbell ian.campb...@citrix.com
  Cc: Wei Liu wei.l...@citrix.com
  Cc: Juergen Gross jgr...@suse.com
  Cc: James E.J. Bottomley jbottom...@odin.com
  Cc: Greg Kroah-Hartman gre...@linuxfoundation.org
  Cc: Jiri Slaby jsl...@suse.com
  Cc: Jean-Christophe Plagniol-Villard plagn...@jcrosoft.com
  Cc: Tomi Valkeinen tomi.valkei...@ti.com
  Cc: linux-in...@vger.kernel.org
  Cc: net...@vger.kernel.org
  Cc: linux-s...@vger.kernel.org
  Cc: linuxppc-dev@lists.ozlabs.org
  Cc: linux-fb...@vger.kernel.org
  Cc: linux-arm-ker...@lists.infradead.org
 
 Aside from the x86 bits:
 
 Reviewed-by: Stefano Stabellini stefano.stabell...@eu.citrix.com

Not really important, but just in case anyone waits for my ack on input
bits:

Acked-by: Dmitry Torokhov dmitry.torok...@gmail.com

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 00/27] Export I2C and OF module aliases in missing drivers

[Dmitry Torokhov]

On Thu, Jul 30, 2015 at 09:35:17AM -0700, Dmitry Torokhov wrote:
 Hi Javier,
 
 On Thu, Jul 30, 2015 at 06:18:25PM +0200, Javier Martinez Canillas wrote:
  Hello,
  
  Short version:
  
  This series add the missing MODULE_DEVICE_TABLE() for OF and I2C tables
  to export that information so modules have the correct aliases built-in
  and autoloading works correctly.
  
  Longer version:
  
  Currently it's mandatory for I2C drivers to have an I2C device ID table
  regardless if the device was registered using platform data or OF. This
  is because the I2C core needs an I2C device ID table for two reasons:
  
  1) Match the I2C client with a I2C device ID so a struct i2c_device_id
 is passed to the I2C driver probe() function.
  
  2) Export the module aliases from the I2C device ID table so userspace
 can auto-load the correct module. This is because i2c_device_uevent
 always reports a MODALIAS of the form i2c:client-name.
 
 Why are we not fixing this? We emit specially carved uevent for
 ACPI-based devices, why not the same for OF? Platform bus does this...

Ah, now I see the 27/27 patch. I think it is exactly what we need. And
probably for SPI bus as well.

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 00/27] Export I2C and OF module aliases in missing drivers

[Dmitry Torokhov]

Hi Javier,

On Thu, Jul 30, 2015 at 06:18:25PM +0200, Javier Martinez Canillas wrote:
 Hello,
 
 Short version:
 
 This series add the missing MODULE_DEVICE_TABLE() for OF and I2C tables
 to export that information so modules have the correct aliases built-in
 and autoloading works correctly.
 
 Longer version:
 
 Currently it's mandatory for I2C drivers to have an I2C device ID table
 regardless if the device was registered using platform data or OF. This
 is because the I2C core needs an I2C device ID table for two reasons:
 
 1) Match the I2C client with a I2C device ID so a struct i2c_device_id
is passed to the I2C driver probe() function.
 
 2) Export the module aliases from the I2C device ID table so userspace
can auto-load the correct module. This is because i2c_device_uevent
always reports a MODALIAS of the form i2c:client-name.

Why are we not fixing this? We emit specially carved uevent for
ACPI-based devices, why not the same for OF? Platform bus does this...

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 10/10] Kconfig: cleanup SERIO_I8042 dependencies

[Dmitry Torokhov]

On Sat, Dec 14, 2013 at 10:32:31AM -0800, H. Peter Anvin wrote:
 On 12/14/2013 08:59 AM, Mark Salter wrote:
  Remove messy dependencies from SERIO_I8042 by having it depend on one
  Kconfig symbol (ARCH_MIGHT_HAVE_PC_SERIO) and having architectures
  which need it select ARCH_MIGHT_HAVE_PC_SERIO in arch/*/Kconfig.
  New architectures are unlikely to need SERIO_I8042, so this avoids
  having an ever growing list of architectures to exclude.
  
  Signed-off-by: Mark Salter msal...@redhat.com
  CC: Dmitry Torokhov dmitry.torok...@gmail.com
  CC: Richard Henderson r...@twiddle.net
  CC: linux-al...@vger.kernel.org
  CC: Russell King li...@arm.linux.org.uk
  CC: linux-arm-ker...@lists.infradead.org
  CC: Tony Luck tony.l...@intel.com
  CC: Fenghua Yu fenghua...@intel.com
  CC: linux-i...@vger.kernel.org
  CC: Ralf Baechle r...@linux-mips.org
  CC: linux-m...@linux-mips.org
  CC: Benjamin Herrenschmidt b...@kernel.crashing.org
  CC: Paul Mackerras pau...@samba.org
  CC: linuxppc-dev@lists.ozlabs.org
  CC: Paul Mundt let...@linux-sh.org
  CC: linux...@vger.kernel.org
  CC: David S. Miller da...@davemloft.net
  CC: sparcli...@vger.kernel.org
  CC: Guan Xuetao g...@mprc.pku.edu.cn
  CC: Ingo Molnar mi...@redhat.com
  CC: Thomas Gleixner t...@linutronix.de
  CC: H. Peter Anvin h...@zytor.com
  CC: x...@kernel.org
 
 Acked-by: H. Peter Anvin h...@linux.intel.com

How are we going to merge this? In bulk through input tree or peacemeal
through all arches first?

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 10/10] Kconfig: cleanup SERIO_I8042 dependencies

[Dmitry Torokhov]

On Sun, Dec 15, 2013 at 08:27:25PM -0500, David Miller wrote:
 From: Mark Salter msal...@redhat.com
 Date: Sun, 15 Dec 2013 10:50:26 -0500
 
  On Sun, 2013-12-15 at 02:36 -0800, Dmitry Torokhov wrote:
  How are we going to merge this? In bulk through input tree or peacemeal
  through all arches first?
 
  They should all go together to eliminate the chance of bisect breakage.
  Either the input tree or maybe akpm tree.
 
 This sounds good to me.

OK, then I'll pick it up once I collect more acks from the arch
maintainers.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 5/5] kfifo: log based kfifo API

[Dmitry Torokhov]

Hi Yuanhan,

On Tue, Jan 08, 2013 at 10:57:53PM +0800, Yuanhan Liu wrote:
 The current kfifo API take the kfifo size as input, while it rounds
  _down_ the size to power of 2 at __kfifo_alloc. This may introduce
 potential issue.
 
 Take the code at drivers/hid/hid-logitech-dj.c as example:
 
   if (kfifo_alloc(djrcv_dev-notif_fifo,
DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report),
GFP_KERNEL)) {
 
 Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct dj_report)
 is 15.
 
 Which means it wants to allocate a kfifo buffer which can store 8
 dj_report entries at once. The expected kfifo buffer size would be
 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the
 size to rounddown_power_of_2(120) =  64, and then allocate a buf
 with 64 bytes, which I don't think this is the original author want.
 
 With the new log API, we can do like following:
 
   int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS *
   sizeof(struct dj_report));
 
   if (kfifo_alloc(djrcv_dev-notif_fifo, kfifo_size_order, GFP_KERNEL)) {
 
 This make sure we will allocate enough kfifo buffer for holding
 DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries.

Why don't you simply change __kfifo_alloc to round the allocation up
instead of down?

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [Pv-drivers] [PATCH 192/493] scsi: remove use of __devinit

[Dmitry Torokhov]

On Mon, Nov 19, 2012 at 01:22:21PM -0500, Bill Pemberton wrote:
 CONFIG_HOTPLUG is going away as an option so __devinit is no longer
 needed.
 

...

  drivers/scsi/vmw_pvscsi.c |  6 +-

For vmw_pvscsi:

Acked-by: Dmitry Torokhov d...@vmware.com

Thanks,
Dmitry


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 3/5] xhci: Add rmb() between reading event validity event data access.

[Dmitry Torokhov]

On Sunday, March 27, 2011 09:52:57 pm Matt Evans wrote:
 On weakly-ordered systems, the reading of an event's content must occur
 after reading the event's validity.
 
 Signed-off-by: Matt Evans m...@ozlabs.org
 ---
 Segher, thanks for the comment; explanation added.
 
  drivers/usb/host/xhci-ring.c |5 +
  1 files changed, 5 insertions(+), 0 deletions(-)
 
 diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
 index 45f3b77..d6aa880 100644
 --- a/drivers/usb/host/xhci-ring.c
 +++ b/drivers/usb/host/xhci-ring.c
 @@ -2152,6 +2152,11 @@ static void xhci_handle_event(struct xhci_hcd
 *xhci) }
   xhci_dbg(xhci, %s - OS owns TRB\n, __func__);
 
 + /*
 +  * Barrier between reading the TRB_CYCLE (valid) flag above and any
 +  * speculative reads of the event's flags/data below.
 +  */
 + rmb();
   /* FIXME: Handle more event types. */
   switch ((le32_to_cpu(event-event_cmd.flags)  TRB_TYPE_BITMASK)) {

Isn't it the same memory that is being read the first time around? How 
reordering could happen here?

Thanks.


-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 5/5] xhci: Remove recursive call to xhci_handle_event

[Dmitry Torokhov]

On Sunday, March 27, 2011 09:53:00 pm Matt Evans wrote:
 @@ -2282,7 +2284,7 @@ hw_died:
   /* FIXME this should be a delayed service routine
* that clears the EHB.
*/
 - xhci_handle_event(xhci);
 + while (xhci_handle_event(xhci)) {}
 

I must admit I dislike the style with empty loop bodies, do you think
we could have something like below instead?

Thanks!

-- 
Dmitry

From: Dmitry Torokhov d...@vmware.com
Subject: [PATCH] USB: xhci: avoid recursion in xhci_handle_event

Instead of having xhci_handle_event call itself if there are more
outstanding TRBs push the loop logic up one level, into xhci_irq().

xchI_handle_event() does not check for presence of event_ring
anymore since the ring is always allocated. Also, encountering
a TRB that does not belong to OS is a normal condition that
causes us to stop processing and exit ISR and so is not reported
in xhci-error_bitmask.

Also consolidate handling of the event handler busy flag in
xhci_irq().

Reported-by: Matt Evans m...@ozlabs.org
Signed-off-by: Dmitry Torokhov d...@vmware.com
---
 drivers/usb/host/xhci-ring.c |   77 +++---
 1 files changed, 27 insertions(+), 50 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 0e30281..8e6d8fa 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2131,26 +2131,12 @@ cleanup:
  * This function handles all OS-owned events on the event ring.  It may drop
  * xhci-lock between event processing (e.g. to pass up port status changes).
  */
-static void xhci_handle_event(struct xhci_hcd *xhci)
+static void xhci_handle_event(struct xhci_hcd *xhci, union xhci_trb *event)
 {
-   union xhci_trb *event;
-   int update_ptrs = 1;
+   bool update_ptrs = true;
int ret;
 
xhci_dbg(xhci, In %s\n, __func__);
-   if (!xhci-event_ring || !xhci-event_ring-dequeue) {
-   xhci-error_bitmask |= 1  1;
-   return;
-   }
-
-   event = xhci-event_ring-dequeue;
-   /* Does the HC or OS own the TRB? */
-   if ((event-event_cmd.flags  TRB_CYCLE) !=
-   xhci-event_ring-cycle_state) {
-   xhci-error_bitmask |= 1  2;
-   return;
-   }
-   xhci_dbg(xhci, %s - OS owns TRB\n, __func__);
 
/* FIXME: Handle more event types. */
switch ((event-event_cmd.flags  TRB_TYPE_BITMASK)) {
@@ -2163,7 +2149,7 @@ static void xhci_handle_event(struct xhci_hcd *xhci)
xhci_dbg(xhci, %s - calling handle_port_status\n, __func__);
handle_port_status(xhci, event);
xhci_dbg(xhci, %s - returned from handle_port_status\n, 
__func__);
-   update_ptrs = 0;
+   update_ptrs = false;
break;
case TRB_TYPE(TRB_TRANSFER):
xhci_dbg(xhci, %s - calling handle_tx_event\n, __func__);
@@ -2172,7 +2158,7 @@ static void xhci_handle_event(struct xhci_hcd *xhci)
if (ret  0)
xhci-error_bitmask |= 1  9;
else
-   update_ptrs = 0;
+   update_ptrs = false;
break;
default:
if ((event-event_cmd.flags  TRB_TYPE_BITMASK) = TRB_TYPE(48))
@@ -2180,21 +2166,9 @@ static void xhci_handle_event(struct xhci_hcd *xhci)
else
xhci-error_bitmask |= 1  3;
}
-   /* Any of the above functions may drop and re-acquire the lock, so check
-* to make sure a watchdog timer didn't mark the host as non-responsive.
-*/
-   if (xhci-xhc_state  XHCI_STATE_DYING) {
-   xhci_dbg(xhci, xHCI host dying, returning from 
-   event handler.\n);
-   return;
-   }
 
if (update_ptrs)
-   /* Update SW event ring dequeue pointer */
inc_deq(xhci, xhci-event_ring, true);
-
-   /* Are there more items on the event ring? */
-   xhci_handle_event(xhci);
 }
 
 /*
@@ -2258,34 +2232,37 @@ hw_died:
xhci_writel(xhci, irq_pending, xhci-ir_set-irq_pending);
}
 
-   if (xhci-xhc_state  XHCI_STATE_DYING) {
-   xhci_dbg(xhci, xHCI dying, ignoring interrupt. 
-   Shouldn't IRQs be disabled?\n);
-   /* Clear the event handler busy flag (RW1C);
-* the event ring should be empty.
-*/
-   temp_64 = xhci_read_64(xhci, xhci-ir_set-erst_dequeue);
-   xhci_write_64(xhci, temp_64 | ERST_EHB,
-   xhci-ir_set-erst_dequeue);
-   spin_unlock(xhci-lock);
-
-   return IRQ_HANDLED;
-   }
-
event_ring_deq = xhci-event_ring-dequeue;
+
/* FIXME this should be a delayed service routine
 * that clears the EHB.
 */
-   xhci_handle_event(xhci);
+   xhci_dbg(xhci, %s - Begin

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

Hi Ira,

On Tue, Feb 08, 2011 at 03:37:46PM -0800, Ira W. Snyder wrote:
 This driver allows userspace to access the data processing FPGAs on the
 OVRO CARMA board. It has two modes of operation:
 

Thank you for making the changes, some more comments below.

 +
 +#define inode_to_dev(inode) container_of(inode-i_cdev, struct fpga_device, 
 cdev)
 +

Does not seem to be used.

 +/*
 + * Data Buffer Allocation Helpers
 + */
 +
 +static int data_map_buffer(struct device *dev, struct data_buf *buf)
 +{
 + return videobuf_dma_map(dev, buf-vb);
 +}
 +
 +static void data_unmap_buffer(struct device *dev, struct data_buf *buf)
 +{
 + videobuf_dma_unmap(dev, buf-vb);
 +}

Why can't we all videobuf_dma_map() and videobuf_dma_unmap() directly?
What these helpers supposed to solve?

 +static int data_alloc_buffers(struct fpga_device *priv)
 +{
 + struct data_buf *buf;
 + int i, ret;
 +
 + for (i = 0; i  MAX_DATA_BUFS; i++) {
 +
 + /* allocate a buffer */
 + buf = data_alloc_buffer(priv-bufsize);
 + if (!buf)
 + continue;

break?

 +
 + /* map it for DMA */
 + ret = data_map_buffer(priv-dev, buf);
 + if (ret) {
 + data_free_buffer(buf);
 + continue;

and here as well?

 + }
 +
 + /* add it to the list of free buffers */
 + list_add_tail(buf-entry, priv-free);
 + priv-num_buffers++;
 + }
 +
 + /* Make sure we allocated the minimum required number of buffers */
 + if (priv-num_buffers  MIN_DATA_BUFS) {
 + dev_err(priv-dev, Unable to allocate enough data buffers\n);
 + data_free_buffers(priv);
 + return -ENOMEM;
 + }
 +
 + /* Warn if we are running in a degraded state, but do not fail */
 + if (priv-num_buffers  MAX_DATA_BUFS) {
 + dev_warn(priv-dev, Unable to allocate one second worth of 
 + buffers, using %d buffers instead\n, i);

The latest style is not break strings even if they exceed 80 column
limit to make sure they are easily greppable.

 +static void data_dma_cb(void *data)
 +{
 + struct fpga_device *priv = data;
 + struct data_buf *buf;
 + unsigned long flags;
 +
 + spin_lock_irqsave(priv-lock, flags);
 +
 + /* clear the FPGA status and re-enable interrupts */
 + data_enable_interrupts(priv);
 +
 + /* If the inflight list is empty, we've got a bug */
 + BUG_ON(list_empty(priv-inflight));
 +
 + /* Grab the first buffer from the inflight list */
 + buf = list_first_entry(priv-inflight, struct data_buf, entry);
 + list_del_init(buf-entry);
 +
 + /* Add it to the used list */
 + list_add_tail(buf-entry, priv-used);

or
list_move_tail(buf-entry, priv-used);

 +
 +static irqreturn_t data_irq(int irq, void *dev_id)
 +{
 + struct fpga_device *priv = dev_id;
 + struct data_buf *buf;
 + u32 status;
 + int i;
 +
 + /* detect spurious interrupts via FPGA status */
 + for (i = 0; i  4; i++) {
 + status = fpga_read_reg(priv, i, MMAP_REG_STATUS);
 + if (!(status  (CORL_DONE | CORL_ERR))) {
 + dev_err(priv-dev, spurious irq detected (FPGA)\n);
 + return IRQ_NONE;
 + }
 + }
 +
 + /* detect spurious interrupts via raw IRQ pin readback */
 + status = ioread32be(priv-regs + SYS_IRQ_INPUT_DATA);
 + if (status  IRQ_CORL_DONE) {
 + dev_err(priv-dev, spurious irq detected (IRQ)\n);
 + return IRQ_NONE;
 + }
 +
 + spin_lock(priv-lock);
 +
 + /* hide the interrupt by switching the IRQ driver to GPIO */
 + data_disable_interrupts(priv);
 +
 + /* Check that we actually have a free buffer */
 + if (list_empty(priv-free)) {
 + priv-num_dropped++;
 + data_enable_interrupts(priv);
 + goto out_unlock;
 + }
 +
 + buf = list_first_entry(priv-free, struct data_buf, entry);
 + list_del_init(buf-entry);
 +
 + /* Check the buffer size */
 + BUG_ON(buf-size != priv-bufsize);
 +
 + /* Submit a DMA transfer to get the correlation data */
 + if (data_submit_dma(priv, buf)) {
 + dev_err(priv-dev, Unable to setup DMA transfer\n);
 + list_add_tail(buf-entry, priv-free);
 + data_enable_interrupts(priv);
 + goto out_unlock;
 + }
 +
 + /* DMA setup succeeded, GO!!! */
 + list_add_tail(buf-entry, priv-inflight);
 + dma_async_memcpy_issue_pending(priv-chan);
 +
 +out_unlock:
 + spin_unlock(priv-lock);
 + return IRQ_HANDLED;
 +}

Hmm, I wonder if we could simplify this a bit by using list_move()...

bool submitted = false;
...

if (list_empty(priv-free)) {
priv-num_dropped++;
goto out;
}

buf = list_first_entry(priv-free, struct data_buf, 

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

On Wed, Feb 09, 2011 at 09:35:32AM -0800, Ira W. Snyder wrote:
 On Wed, Feb 09, 2011 at 12:33:25AM -0800, Dmitry Torokhov wrote:
   +
   + /* Warn if we are running in a degraded state, but do not fail */
   + if (priv-num_buffers  MAX_DATA_BUFS) {
   + dev_warn(priv-dev, Unable to allocate one second worth of 
   + buffers, using %d buffers instead\n, i);
  
  The latest style is not break strings even if they exceed 80 column
  limit to make sure they are easily greppable.
  
 
 I usually just follow checkpatch warnings. I'll combine the strings onto
 one line.

Does it still warn? I think they tried to fix it so it recognizes long
messages.

  
  OTOH, we can't have more than 1 in-flight buffer, can we? Should
  we even have a list?
  
 
 This looks like a good change. I didn't know about list_move_tail()
 before you mentioned it. Good catch.
 
 You are correct that it is not possible to have more than one in flight
 buffer at the same time. It was previously possible in an earlier
 version of the driver. That was before I was forced to come up with the
 ugly IRQ disabling scheme due to the hardware design.
 
 What would you replace the inflight list with? A struct data_buf
 *inflight; in the priv structure?
 

Yes. Then one would not worry of it is safe to always mark first element
of in-flight list as complete

  
  Have you considered enabling the device when someone opens the device
  node and closing it when last user drops off?
  
  
 
 Yes. Unfortunately it is not possible. Blame my userspace software
 engineers, who refused this model of operation.
 
 I must meet the requirement that the device must remain open while the
 DATA-FPGAs are reconfigured. This means that the FPGAs are completely
 powered down, and a new FPGA bitfile is loaded into them.
 
 After a reconfiguration, the data buffer size may change.
 
 The userspace coders were willing to use a sysfs file to enable/disable
 reading from the device, but they were not willing to open/close the
 device each time. It was the best compromise they would accept.
 
 I'm not happy about it either.

I see.

 
   +
   +/*
   + * FPGA Realtime Data Character Device
   + */
   +
   +static int data_open(struct inode *inode, struct file *filp)
   +{
   + /*
   +  * The miscdevice layer puts our struct miscdevice into the
   +  * filp-private_data field. We use this to find our private
   +  * data and then overwrite it with our own private structure.
   +  */
   + struct fpga_device *priv = container_of(filp-private_data,
   + struct fpga_device, miscdev);
   + struct fpga_reader *reader;
   + int ret;
   +
   + /* allocate private data */
   + reader = kzalloc(sizeof(*reader), GFP_KERNEL);
   + if (!reader)
   + return -ENOMEM;
   +
   + reader-priv = priv;
   + reader-buf = NULL;
   +
   + filp-private_data = reader;
   + ret = nonseekable_open(inode, filp);
   + if (ret) {
   + dev_err(priv-dev, nonseekable-open failed\n);
   + kfree(reader);
   + return ret;
   + }
   +
  
  I wonder if the device should allow only exclusive open... Unless you
  distribute copies of data between all readers.
  
 
 I wish to allow multiple different processes to mmap() the device. This
 requires open(), followed by mmap(). I only care about a single realtime
 data reader (which calls read()). Splitting the two use cases into two
 drivers seemed like a big waste of time and effort. I have no idea how
 to accomplish a single read()er while allowing multiple mmap()ers.

I see.

 
   + return 0;
   +}
   +
   +static int data_release(struct inode *inode, struct file *filp)
   +{
   + struct fpga_reader *reader = filp-private_data;
   +
   + /* free the per-reader structure */
   + data_free_buffer(reader-buf);
   + kfree(reader);
   + filp-private_data = NULL;
   + return 0;
   +}
   +
   +static ssize_t data_read(struct file *filp, char __user *ubuf, size_t 
   count,
   +  loff_t *f_pos)
   +{
   + struct fpga_reader *reader = filp-private_data;
   + struct fpga_device *priv = reader-priv;
   + struct list_head *used = priv-used;
   + struct data_buf *dbuf;
   + size_t avail;
   + void *data;
   + int ret;
   +
   + /* check if we already have a partial buffer */
   + if (reader-buf) {
   + dbuf = reader-buf;
   + goto have_buffer;
   + }
   +
   + spin_lock_irq(priv-lock);
   +
   + /* Block until there is at least one buffer on the used list */
   + while (list_empty(used)) {
  
  I believe we should stop and return error when device is disabled so the
  condition should be:
  
  while (!reader-buf  list_empty()  priv-enabled)
  
  
 
 The requirement is that the device stay open during reconfiguration.
 This provides for that. Readers just block for as long as the device is
 not producing data.

OK, you still need to make sure you do not touch free/used buffer while
device is disabled. Also, you need to kick readers

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

On Wed, Feb 09, 2011 at 03:35:45PM -0800, Ira W. Snyder wrote:
 On Wed, Feb 09, 2011 at 10:27:40AM -0800, Dmitry Torokhov wrote:
 
 [ snip stuff I've already fixed in the next version ]
 
   
   The requirement is that the device stay open during reconfiguration.
   This provides for that. Readers just block for as long as the device is
   not producing data.
  
  OK, you still need to make sure you do not touch free/used buffer while
  device is disabled. Also, you need to kick readers if you unbind the
  driver, so maybe a new flag priv-exists should be introduced and
  checked.
  
 
 I don't understand what you mean by kick readers if you unbind the
 driver. The kernel automatically increases the refcount on a module
 when a process is using the module. This shows up in the Used by
 column of lsmod's output.
 
 The kernel will not let you rmmod a module with a non-zero refcount. You
 cannot get into the situation where you have rmmod'ed the module and a
 reader is still blocking in read()/poll().

However you can still unbind the driver from the device by writing into
driver's sysfs 'unbind' attribute.

See drivers/base/bus.c::driver_unbind().

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

On Wed, Feb 09, 2011 at 04:10:55PM -0800, Ira W. Snyder wrote:
 On Wed, Feb 09, 2011 at 03:42:31PM -0800, Dmitry Torokhov wrote:
  On Wed, Feb 09, 2011 at 03:35:45PM -0800, Ira W. Snyder wrote:
   On Wed, Feb 09, 2011 at 10:27:40AM -0800, Dmitry Torokhov wrote:
   
   [ snip stuff I've already fixed in the next version ]
   
 
 The requirement is that the device stay open during reconfiguration.
 This provides for that. Readers just block for as long as the device 
 is
 not producing data.

OK, you still need to make sure you do not touch free/used buffer while
device is disabled. Also, you need to kick readers if you unbind the
driver, so maybe a new flag priv-exists should be introduced and
checked.

   
   I don't understand what you mean by kick readers if you unbind the
   driver. The kernel automatically increases the refcount on a module
   when a process is using the module. This shows up in the Used by
   column of lsmod's output.
   
   The kernel will not let you rmmod a module with a non-zero refcount. You
   cannot get into the situation where you have rmmod'ed the module and a
   reader is still blocking in read()/poll().
  
  However you can still unbind the driver from the device by writing into
  driver's sysfs 'unbind' attribute.
  
  See drivers/base/bus.c::driver_unbind().
  
 
 I was completely unaware of that feature. I hunch that many drivers
 are incapable of dealing with an unbind while they are still open.

Hmm, maybe older drivers... Anythig hotpluggable (USB, PCI, etc) should
be in a better shape because they expect to be yanked at any time.

 
 Matter of fact, I don't see how this can EVER be safe. The driver core
 automatically calls the data_of_remove() routine while there are still
 blocked readers. This kfree()s the private data structure, which
 contains the suggested priv-exists flag. What happens if the memory
 allocator re-allocates that memory to a different driver before the
 reader process is woken up to check the priv-exists flag?
 
 The only way to solve this is to count the number of open()s and
 close()s, and block the unbind until all users have close()d the device.
 

Yes, you can kick readers and wait, or you can refcount that private
structure and have readers grab a reference when they open your device
and drop it in their fops-release() method. Your remove() should also
drop reference instead of doing kfree() outright.

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

On Tue, Feb 08, 2011 at 09:20:46AM -0800, Ira W. Snyder wrote:
 On Mon, Feb 07, 2011 at 11:33:10PM -0800, Dmitry Torokhov wrote:
   +static void data_free_buffer(struct device *dev, struct data_buf *buf)
   +{
   + /* It is ok to free a NULL buffer */
   + if (!buf)
   + return;
   +
   + /* Make sure the buffer is not on any list */
   + list_del_init(buf-entry);
  
  And what happens if it is? Should it be WARN_ON(!list_empty()) instead?
  
 
 This was only defensive programming. Everywhere this function is called,
 the buffer has already been removed from the list.

I am concerned as sometimes defencive programming is the sign that we
arenot quite sure how the code works. I believe defensive programming
should be used when providing library-like code, not in local cases.

   +
   + list_for_each_entry_safe(buf, tmp, priv-free, entry) {
   + list_del_init(buf-entry);
   + spin_unlock_irq(priv-lock);
   + data_free_buffer(priv-dev, buf);
   + spin_lock_irq(priv-lock);
   + }
  
  This is messed up. If there is concurrent access to the free list then
  it is not safe to continue iterating list after releasing the lock, you
  need to do:
  
  spin_lock_irq(priv-lock);
  while (!list_empty(priv-free)) {
  buf = list_first_entry(priv-free, struct data_buf, entry);
  list_del_init(buf-entry);
  spin_unlock_irq(priv-lock);
  data_free_buffer(priv-dev, buf);
  spin_lock_irq(priv-lock);
  }
  
  BUT, the function is only called when you disable (or fail to enable) device
  which, at this point, should be quiesced, thus all this locking is not
  really needed.
  
 
 Correct.
 
 I thought it would be clearer to reviewers if I always used the lock to
 protect a data structure, even when it isn't technically needed.

No, locks should protect wehat needs to be protected. The rest just
muddles water.

   +
   + spin_lock_irq(priv-lock);
   + while (!list_empty(list)) {
   + spin_unlock_irq(priv-lock);
   +
   + ret = wait_event_interruptible(priv-wait, list_empty(list));
   + if (ret)
   + return -ERESTARTSYS;
   +
   + spin_lock_irq(priv-lock);
   + }
   + spin_unlock_irq(priv-lock);
  
  Locking is not needed - if you disable interrupyts what would put more
  stuff on the list?
  
 
 The locking is definitely needed.
 
 You've missed a critical piece of information. There are *two* devices
 we are interacting with here, and BOTH generate interrupts.
 

No, I did not miss this fact. The point is that when we get to this code
the device _putting_ items on wauiting list is stopped and we only need
to wait for the list to drain. Nobody puts more stuff on it. You can
check fir list_empty() condition without locking.

And if someone _is_ putting more stuff on the list - you are screwed
since list may become non-empty the moment you release the lock.

   +
   +static ssize_t data_num_buffers_show(struct device *dev,
   +  struct device_attribute *attr, char *buf)
   +{
   + struct fpga_device *priv = dev_get_drvdata(dev);
   + unsigned int num;
   +
   + spin_lock_irq(priv-lock);
   + num = priv-num_buffers;
   + spin_unlock_irq(priv-lock);
  
  This spin lock is pointless, priv-num_buffers might be already changed
  here, you can't guarantee that you show accurate data.
  
 
 Correct, I know this. I just wanted to protect the data structure at all
 points of use in the driver.

Protect from what? integer reads are guaranteed to be complete and you
are not concerned with missing updates as information is obsolete the
moment you release trhe lock.

 Would an atomic_t be better for this, or
 should I just remove the locking completely?

Just remove the locking.

   +
   + if (mutex_lock_interruptible(priv-mutex))
   + return -ERESTARTSYS;
  
  Why don't
  
  error = mutex_lock_interruptible(priv-mutex);
  if (error)
  return error;
  
  - do not clobber perfectly valid error codes.
  
 
 That's what the Linux Device Drivers 3rd Edition book does. See page
 112. I will change it to fix the return code.


LDD3 is quite old by now...

   +
   +static struct attribute *data_sysfs_attrs[] = {
   + dev_attr_num_buffers.attr,
   + dev_attr_buffer_size.attr,
   + dev_attr_num_inflight.attr,
   + dev_attr_num_free.attr,
   + dev_attr_num_used.attr,
   + dev_attr_num_dropped.attr,
   + dev_attr_enable.attr,
   + NULL,
   +};
  
  Are all of these really needed or most of them are for debug?
  
 
 Most are for debugging. They have proved useful a few times in
 production to track down bugs.
 

Consider switching over to debugfs then. Or, if you believe the device
is sufficiently bug-free just cut the code.

   +
   +static ssize_t data_read(struct file *filp, char __user *ubuf, size_t 
   count,
   +  loff_t *f_pos)
   +{
   + struct fpga_reader *reader = filp-private_data;
   + struct fpga_device *priv

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

On Tue, Feb 08, 2011 at 11:11:44AM -0800, Ira W. Snyder wrote:
 On Tue, Feb 08, 2011 at 09:50:29AM -0800, Dmitry Torokhov wrote:
  On Tue, Feb 08, 2011 at 09:20:46AM -0800, Ira W. Snyder wrote:
  
   Go back and re-think my loop. This is a
   common idiom straight of out LDD3 pages 153-154.
   
   You should note that it is only possible to exit the loop with the lock
   held AND !list_empty(used). The lock protects the used list, and
   therefore, there must be a buffer on the list.
  
  No, because you are woken up while not holding the lock so another
  reader is free to take it off the list.
  
 
 Correct. But then I go around the loop and check list_empty() again
 before exiting the loop. The list MUST NOT be empty before the loop will
 terminate.

Yes, you are right, I competely missed the fact that we'd loop around
and check the condition again. I'll go grab another coffee now.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] misc: add CARMA DATA-FPGA Access Driver

[Dmitry Torokhov]

Hi Ira,

On Mon, Feb 07, 2011 at 03:23:40PM -0800, Ira W. Snyder wrote:
 This driver allows userspace to access the data processing FPGAs on the
 OVRO CARMA board. It has two modes of operation:
 
 1) random access
 
 This allows users to poke any DATA-FPGA registers by using mmap to map
 the address region directly into their memory map.
 
 2) correlation dumping
 
 When correlating, the DATA-FPGA's have special requirements for getting
 the data out of their memory before the next correlation. This nominally
 happens at 64Hz (every 15.625ms). If the data is not dumped before the
 next correlation, data is lost.
 
 The data dumping driver handles buffering up to 1 second worth of
 correlation data from the FPGAs. This lowers the realtime scheduling
 requirements for the userspace process reading the device.

Kind of a fly-by review but it looks like the locking in the driver
needs work.

 
 Signed-off-by: Ira W. Snyder i...@ovro.caltech.edu
 ---
  drivers/misc/Kconfig|1 +
  drivers/misc/Makefile   |1 +
  drivers/misc/carma/Kconfig  |9 +
  drivers/misc/carma/Makefile |1 +
  drivers/misc/carma/carma-fpga.c | 1446 
 +++
  5 files changed, 1458 insertions(+), 0 deletions(-)
  create mode 100644 drivers/misc/carma/Kconfig
  create mode 100644 drivers/misc/carma/Makefile
  create mode 100644 drivers/misc/carma/carma-fpga.c
 
 diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
 index 4d073f1..f457f14 100644
 --- a/drivers/misc/Kconfig
 +++ b/drivers/misc/Kconfig
 @@ -457,5 +457,6 @@ source drivers/misc/eeprom/Kconfig
  source drivers/misc/cb710/Kconfig
  source drivers/misc/iwmc3200top/Kconfig
  source drivers/misc/ti-st/Kconfig
 +source drivers/misc/carma/Kconfig
  
  endif # MISC_DEVICES
 diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
 index 98009cc..2c1610e 100644
 --- a/drivers/misc/Makefile
 +++ b/drivers/misc/Makefile
 @@ -42,3 +42,4 @@ obj-$(CONFIG_ARM_CHARLCD)   += arm-charlcd.o
  obj-$(CONFIG_PCH_PHUB)   += pch_phub.o
  obj-y+= ti-st/
  obj-$(CONFIG_AB8500_PWM) += ab8500-pwm.o
 +obj-y+= carma/
 diff --git a/drivers/misc/carma/Kconfig b/drivers/misc/carma/Kconfig
 new file mode 100644
 index 000..4be183f
 --- /dev/null
 +++ b/drivers/misc/carma/Kconfig
 @@ -0,0 +1,9 @@
 +config CARMA_FPGA
 + tristate CARMA DATA-FPGA Access Driver
 + depends on FSL_SOC  PPC_83xx  MEDIA_SUPPORT  HAS_DMA  FSL_DMA
 + select VIDEOBUF_DMA_SG
 + default n
 + help
 +   Say Y here to include support for communicating with the data
 +   processing FPGAs on the OVRO CARMA board.
 +
 diff --git a/drivers/misc/carma/Makefile b/drivers/misc/carma/Makefile
 new file mode 100644
 index 000..0b69fa7
 --- /dev/null
 +++ b/drivers/misc/carma/Makefile
 @@ -0,0 +1 @@
 +obj-$(CONFIG_CARMA_FPGA) += carma-fpga.o
 diff --git a/drivers/misc/carma/carma-fpga.c b/drivers/misc/carma/carma-fpga.c
 new file mode 100644
 index 000..52620b3
 --- /dev/null
 +++ b/drivers/misc/carma/carma-fpga.c
 @@ -0,0 +1,1446 @@
 +/*
 + * CARMA DATA-FPGA Access Driver
 + *
 + * Copyright (c) 2009-2010 Ira W. Snyder i...@ovro.caltech.edu
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU General Public License as published by the
 + * Free Software Foundation; either version 2 of the License, or (at your
 + * option) any later version.
 + */
 +
 +/*
 + * FPGA Memory Dump Format
 + *
 + * FPGA #0 control registers (32 x 32-bit words)
 + * FPGA #1 control registers (32 x 32-bit words)
 + * FPGA #2 control registers (32 x 32-bit words)
 + * FPGA #3 control registers (32 x 32-bit words)
 + * SYSFPGA control registers (32 x 32-bit words)
 + * FPGA #0 correlation array (NUM_CORL0 correlation blocks)
 + * FPGA #1 correlation array (NUM_CORL1 correlation blocks)
 + * FPGA #2 correlation array (NUM_CORL2 correlation blocks)
 + * FPGA #3 correlation array (NUM_CORL3 correlation blocks)
 + *
 + * Each correlation array consists of:
 + *
 + * Correlation Data  (2 x NUM_LAGSn x 32-bit words)
 + * Pipeline Metadata (2 x NUM_METAn x 32-bit words)
 + * Quantization Counters (2 x NUM_QCNTn x 32-bit words)
 + *
 + * The NUM_CORLn, NUM_LAGSn, NUM_METAn, and NUM_QCNTn values come from
 + * the FPGA configuration registers. They do not change once the FPGA's
 + * have been programmed, they only change on re-programming.
 + */
 +
 +/*
 + * Basic Description:
 + *
 + * This driver is used to capture correlation spectra off of the four data
 + * processing FPGAs. The FPGAs are often reprogrammed at runtime, therefore
 + * this driver supports dynamic enable/disable of capture while the device
 + * remains open.
 + *
 + * The nominal capture rate is 64Hz (every 15.625ms). To facilitate this fast
 + * capture rate, all buffers are pre-allocated to avoid any potentially long
 + * running memory 

Re: [PATCH v3] powerpc: Add i8042 keyboard and mouse irq parsing

[Dmitry Torokhov]

On Tue, Jun 01, 2010 at 01:41:59PM +0100, Martyn Welch wrote:
 Benjamin Herrenschmidt wrote:
  O

  diff --git a/arch/powerpc/kernel/setup-common.c 
  b/arch/powerpc/kernel/setup-common.c
  index 48f0a00..3d169bb 100644
  --- a/arch/powerpc/kernel/setup-common.c
  +++ b/arch/powerpc/kernel/setup-common.c
  @@ -94,6 +94,10 @@ struct screen_info screen_info = {
 .orig_video_points = 16
   };
   
  +/* Variables required to store legacy IO irq routing */
  +int of_i8042_kbd_irq;
  +int of_i8042_aux_irq;
  
 
  Is there a reasonable ifdef to use for the above or we don't care ?
 

   #ifdef __DO_IRQ_CANON
   /* XXX should go elsewhere eventually */
   int ppc_do_canonicalize_irqs;
  @@ -567,6 +571,15 @@ int check_legacy_ioport(unsigned long base_port)
 np = of_find_compatible_node(NULL, NULL, pnpPNP,f03);
 if (np) {
 parent = of_get_parent(np);
  +
  +  of_i8042_kbd_irq = irq_of_parse_and_map(parent, 0);
  +  if (!of_i8042_kbd_irq)
  +  of_i8042_kbd_irq = 1;
  +
  +  of_i8042_aux_irq = irq_of_parse_and_map(parent, 1);
  +  if (!of_i8042_aux_irq)
  +  of_i8042_aux_irq = 12;
  +
 of_node_put(np);
 np = parent;
 break;
  diff --git a/drivers/input/serio/i8042-io.h 
  b/drivers/input/serio/i8042-io.h
  index 847f4aa..5d48bb6 100644
  --- a/drivers/input/serio/i8042-io.h
  +++ b/drivers/input/serio/i8042-io.h
  @@ -27,6 +27,11 @@
   #include asm/irq.h
   #elif defined(CONFIG_SH_CAYMAN)
   #include asm/irq.h
  +#elif defined(CONFIG_PPC)
  +extern int of_i8042_kbd_irq;
  +extern int of_i8042_aux_irq;
  +# define I8042_KBD_IRQ  of_i8042_kbd_irq
  +# define I8042_AUX_IRQ  of_i8042_aux_irq
   #else
   # define I8042_KBD_IRQ1
   # define I8042_AUX_IRQ12
  
 
  Now while that works, I do tend to dislike global variables like that.
 
  _Maybe_ a better approach would be to have those #define resolve to
  functions:
 
  #define I8042_KBD_IRQ   of_find_i8042_kbd_irq()
 
  Or something like that ?
 
  That means ending up having 2 functions which more/less reproduce the
  loop to find the driver in the device-tree but that's a minor
  inconvenience.
 
  Now, maybe the variables are less bloat here. What do you think ? Which
  way do you prefer ?
 

 
 Personally, I'm happy either way. If you'd prefer I implement these as
 functions, I can't quickly see why that wouldn't work. I thought using
 variables would be the least disruptive.

FYI: i8042 core expects I8042_{KBD|AUX}_IRQ to be integers, however it
is certainly fixable...

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] drivers: remove all i2c_set_clientdata(client, NULL)

[Dmitry Torokhov]

Hi Wolfram,

On Mon, May 31, 2010 at 02:55:48PM +0200, Wolfram Sang wrote:
 I2C-drivers can use the clientdata-pointer to point to private data. As I2C
 devices are not really unregistered, but merely detached from their driver, it
 used to be the drivers obligation to clear this pointer during remove() or a
 failed probe(). As a couple of drivers forgot to do this, it was agreed that 
 it
 was cleaner if the i2c-core does this clearance when appropriate, as there is
 no guarantee for the lifetime of the clientdata-pointer after remove() anyhow.
 This feature was added to the core with commit
 e4a7b9b04de15f6b63da5ccdd373ffa3057a3681 to fix the faulty drivers.
 
 As there is no need anymore to clear the clientdata-pointer, remove all 
 current
 occurrences in the drivers to simplify the code and prevent confusion.
 
 Signed-off-by: Wolfram Sang w.s...@pengutronix.de
 Cc: Jean Delvare kh...@linux-fr.org
 ---
 
 Some more notes:
 
 I waited for rc1 as I knew there were some drivers/patches coming along which
 needed to be processed, too.
 
 I'd suggest that this goes via the i2c-tree, so we get rid of all occurences 
 at
 once.
 

Frankly I'd prefer taking input stuff through my tree with the goal of
.36 merge window just to minimize potential merge issues. This is a
simple cleanup patch that has no dependencies, so there is little gain
from doing it all in one go.

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] drivers: remove all i2c_set_clientdata(client, NULL)

[Dmitry Torokhov]

On Mon, May 31, 2010 at 10:48:32PM +0100, Richard Purdie wrote:
 On Mon, 2010-05-31 at 12:09 -0700, Dmitry Torokhov wrote:
  On Mon, May 31, 2010 at 02:55:48PM +0200, Wolfram Sang wrote:
   I2C-drivers can use the clientdata-pointer to point to private data. As 
   I2C
   devices are not really unregistered, but merely detached from their 
   driver, it
   used to be the drivers obligation to clear this pointer during remove() 
   or a
   failed probe(). As a couple of drivers forgot to do this, it was agreed 
   that it
   was cleaner if the i2c-core does this clearance when appropriate, as 
   there is
   no guarantee for the lifetime of the clientdata-pointer after remove() 
   anyhow.
   This feature was added to the core with commit
   e4a7b9b04de15f6b63da5ccdd373ffa3057a3681 to fix the faulty drivers.
   
   As there is no need anymore to clear the clientdata-pointer, remove all 
   current
   occurrences in the drivers to simplify the code and prevent confusion.
   
   Signed-off-by: Wolfram Sang w.s...@pengutronix.de
   Cc: Jean Delvare kh...@linux-fr.org
   ---
   
   Some more notes:
   
   I waited for rc1 as I knew there were some drivers/patches coming along 
   which
   needed to be processed, too.
   
   I'd suggest that this goes via the i2c-tree, so we get rid of all 
   occurences at
   once.
   
  
  Frankly I'd prefer taking input stuff through my tree with the goal of
  .36 merge window just to minimize potential merge issues. This is a
  simple cleanup patch that has no dependencies, so there is little gain
  from doing it all in one go.
 
 How about asking Linus to take this one now, then its done and we can
 all move on rather than queuing up problems for the next merge window?
 

That should work.

Acked-by: Dmitry Torokhov d...@mail.ru

 Acked-by: Richard Purdie rpur...@linux.intel.com
 

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] device_attributes: add sysfs_attr_init() for dynamic attributes

[Dmitry Torokhov]

Hi Wolfram,

On Mon, Mar 22, 2010 at 07:21:17AM +0100, Wolfram Sang wrote:
 Made necessary by 6992f5334995af474c2b58d010d08bc597f0f2fe.
 
 Found by this semantic patch:
 
 @ init @
 type T;
 identifier A;
 @@
 
 T {
 ...
 struct device_attribute A;
 ...
 };
 
 @ main extends init @
 expression E;
 statement S;
 identifier err;
 T *name;
 @@
 
 ... when != sysfs_attr_init(name-A.attr);
 (
 +   sysfs_attr_init(name-A.attr);
 if (device_create_file(E, name-A))
 S
 |
 +   sysfs_attr_init(name-A.attr);
 err = device_create_file(E, name-A);
 )
 
 While reviewing, I put the initialization to apropriate places.
 

My standard question - are all of these need to be dynamically
allocated?

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] [V2] powerpc: Xilinx: PS2: driver updates based on review

[Dmitry Torokhov]

Hi John,

On Thu, Jul 10, 2008 at 12:34:43PM -0700, John Linn wrote:
 Review comments were incorporated to improve the driver.
 
 1. Some data was eliminated that was not needed.
 2. Renaming of variables for clarity.
 3. Removed unneeded type casting.
 4. Changed to use dev_err rather than other I/O.
 5. Merged together some functions.
 6. Added kerneldoc format to functions.
 


There were some changes made to the original version of the patch that I
applied so this one did not patch cleanly. I think I fixed it up right
but if you could give it a look over before I commit it that would be
great.

Thanks!

-- 
Dmitry

Input: xilinx_ps2 - various cleanups

From: John Linn [EMAIL PROTECTED]

Review comments were incorporated to improve the driver.

1. Some data was eliminated that was not needed.
2. Renaming of variables for clarity.
3. Removed unneeded type casting.
4. Changed to use dev_err rather than other I/O.
5. Merged together some functions.
6. Added kerneldoc format to functions.

Signed-off-by: Sadanand [EMAIL PROTECTED]
Signed-off-by: John Linn [EMAIL PROTECTED]
Acked-by: Peter Korsgaard [EMAIL PROTECTED]
Acked-by: Grant Likely [EMAIL PROTECTED]
Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---

 drivers/input/serio/xilinx_ps2.c |  211 +++---
 1 files changed, 108 insertions(+), 103 deletions(-)

diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
index 0ed044d..85515cf 100644
--- a/drivers/input/serio/xilinx_ps2.c
+++ b/drivers/input/serio/xilinx_ps2.c
@@ -58,23 +58,20 @@
 
 /* Mask for all the Receive Interrupts */
 #define XPS2_IPIXR_RX_ALL  (XPS2_IPIXR_RX_OVF | XPS2_IPIXR_RX_ERR |  \
-   XPS2_IPIXR_RX_FULL)
+XPS2_IPIXR_RX_FULL)
 
 /* Mask for all the Interrupts */
 #define XPS2_IPIXR_ALL (XPS2_IPIXR_TX_ALL | XPS2_IPIXR_RX_ALL |  \
-   XPS2_IPIXR_WDT_TOUT)
+XPS2_IPIXR_WDT_TOUT)
 
 /* Global Interrupt Enable mask */
 #define XPS2_GIER_GIE_MASK 0x8000
 
 struct xps2data {
int irq;
-   u32 phys_addr;
-   u32 remap_size;
spinlock_t lock;
-   u8 rxb; /* Rx buffer */
void __iomem *base_address; /* virt. address of control registers */
-   unsigned int dfl;
+   unsigned int flags;
struct serio serio; /* serio */
 };
 
@@ -82,8 +79,13 @@ struct xps2data {
 /* XPS PS/2 data transmission calls */
 //
 
-/*
- * xps2_recv() will attempt to receive a byte of data from the PS/2 port.
+/**
+ * xps2_recv() - attempts to receive a byte from the PS/2 port.
+ * @drvdata:   pointer to ps2 device private data structure
+ * @byte:  address where the read data will be copied
+ *
+ * If there is any data available in the PS/2 receiver, this functions reads
+ * the data, otherwise it returns error.
  */
 static int xps2_recv(struct xps2data *drvdata, u8 *byte)
 {
@@ -116,33 +118,27 @@ static irqreturn_t xps2_interrupt(int irq, void *dev_id)
 
/* Check which interrupt is active */
if (intr_sr  XPS2_IPIXR_RX_OVF)
-   printk(KERN_WARNING %s: receive overrun error\n,
-   drvdata-serio.name);
+   dev_warn(drvdata-serio.dev.parent, receive overrun error\n);
 
if (intr_sr  XPS2_IPIXR_RX_ERR)
-   drvdata-dfl |= SERIO_PARITY;
+   drvdata-flags |= SERIO_PARITY;
 
if (intr_sr  (XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_WDT_TOUT))
-   drvdata-dfl |= SERIO_TIMEOUT;
+   drvdata-flags |= SERIO_TIMEOUT;
 
if (intr_sr  XPS2_IPIXR_RX_FULL) {
-   status = xps2_recv(drvdata, drvdata-rxb);
+   status = xps2_recv(drvdata, c);
 
/* Error, if a byte is not received */
if (status) {
-   printk(KERN_ERR
-   %s: wrong rcvd byte count (%d)\n,
-   drvdata-serio.name, status);
+   dev_err(drvdata-serio.dev.parent,
+   wrong rcvd byte count (%d)\n, status);
} else {
-   c = drvdata-rxb;
-   serio_interrupt(drvdata-serio, c, drvdata-dfl);
-   drvdata-dfl = 0;
+   serio_interrupt(drvdata-serio, c, drvdata-flags);
+   drvdata-flags = 0;
}
}
 
-   if (intr_sr  XPS2_IPIXR_TX_ACK)
-   drvdata-dfl = 0;
-
return IRQ_HANDLED;
 }
 
@@ -150,8 +146,15 @@ static irqreturn_t xps2_interrupt(int irq, void *dev_id)
 /* serio callbacks */
 /***/
 
-/*
- * sxps2_write() sends a byte out through the PS/2 interface.
+/**
+ * sxps2_write() - sends a byte out through the PS/2 port.
+ * @pserio:pointer to the serio structure of the PS/2 port
+ * @c

Re: [PATCH] [V3] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[Dmitry Torokhov]

On Wed, Jul 9, 2008 at 12:14 PM, John Linn [EMAIL PROTECTED] wrote:
 These look like good comments from Peter.

 Dmitry, how would you like to do this, would you prefer us to do an
 incremental patch from the existing patch (V3)?  Or a new V4 for the
 patch?


Incremental please. I already committed V3 to the 'next' branch and I
don't want to rebuild it unless the patch is completely broken so
incremental cleanup is the way to go.

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] [V3] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[Dmitry Torokhov]

On Mon, Jul 07, 2008 at 12:27:09PM -0600, John Linn wrote:
 Thanks Dmitry, appreciate the help.
 
 Powerpc dependencies in the Kconfig sound right.  Sorry for not thinking
 that thru well across all architectures. 
 
 Do you want me to spin the patch again?
 

Please send me incremental patch that implements proper Kconfig
dependencies and I will fold it all together on my side.

Thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[Dmitry Torokhov]

Hi John,

On Thu, Jul 03, 2008 at 09:42:31AM -0700, John Linn wrote:
 +
 + /* Initialize the PS/2 interface */
 + mutex_lock(drvdata-cfg_mutex);
 + if (xps2_initialize(drvdata)) {
 + mutex_unlock(drvdata-cfg_mutex);
 + dev_err(dev, Could not initialize device\n);
 + retval = -ENODEV;
 + goto failed3;
 + }
 + mutex_unlock(drvdata-cfg_mutex);

The drvdata is allocated per-port and so both (there are 2 PS/2 ports,
right?) ports get their own copy of cfg_mutex. Since you are trying to
serialze access to resource shared by both ports it will not work.
The original driver-global mutex was appropriate (the only thing I
objected there was use of a counting semaphore instead of a mutex).

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[Dmitry Torokhov]

Hi Grant, John,

On Mon, Jun 30, 2008 at 11:16:28AM -0600, Grant Likely wrote:
 On Mon, Jun 30, 2008 at 07:24:48AM -0700, John Linn wrote:
  +config SERIO_XILINX_XPS_PS2
  +   tristate Xilinx XPS PS/2 Controller Support
  +   help
  + This driver supports XPS PS/2 IP from Xilinx EDK.
  +
 
 Consider moving this block to somewhere in the middle of the file to
 reduce the possibility of merge conflicts.
 

I can take care of that, no worries.

  + *
  + * (c) 2005 MontaVista Software, Inc.
  + * (c) 2008 Xilinx Inc.
  + *
  + * This program is free software; you can redistribute it and/or modify it
  + * under the terms of the GNU General Public License as published by the
  + * Free Software Foundation; either version 2 of the License, or (at your
  + * option) any later version.
  + *
  + * You should have received a copy of the GNU General Public License along
  + * with this program; if not, write to the Free Software Foundation, Inc.,
  + * 675 Mass Ave, Cambridge, MA 02139, USA.
 
 These two paragraphs are redundant.  Being in the Linux source tree
 implies that it is GPL licensed.  You can remove them.
 

I prefer having the statement in right in the code actually. While
being the in kernel implies that the code is GPLv2 compatible it could
be dual-licensed or GPLv2 only. This removes any doubt as to what
license is used on this particular piece of code.

  + */
  +
  +
  +#include linux/module.h
  +#include linux/serio.h
  +#include linux/interrupt.h
  +#include linux/errno.h
  +#include linux/init.h
  +#include linux/list.h
  +#include asm/io.h
  +
  +#ifdef CONFIG_OF   /* For open firmware */
  + #include linux/of_device.h
  + #include linux/of_platform.h
  +#endif /* CONFIG_OF */
 
 This is a given for mainline since arch/ppc will not exist in 2.6.27
 
  +
  +#include xilinx_ps2.h
 
 This header can simple be rolled into this .c file because the driver no
 longer has multiple .c files.
 
 
  +#define DRIVER_DESCRIPTION Xilinx XPS PS/2 driver
  +#define XPS2_NAME_DESC Xilinx XPS PS/2 Port #%d
  +#define XPS2_PHYS_DESC xilinxps2/serio%d
 
 These strings are only used in 1 place each, no need to use a #define
 
  +
  +
  +static DECLARE_MUTEX(cfg_sem);
 
 This mutex should be part of the driver private data structure
 
  +
  +/*/
  +/* Interrupt handler */
  +/*/
  +static irqreturn_t xps2_interrupt(int irq, void *dev_id)
  +{
  +   struct xps2data *drvdata = (struct xps2data *)dev_id;
  +   u32 intr_sr;
  +   u32 ier;
  +   u8 c;
  +   u8 retval;
  +
  +   /* Get the PS/2 interrupts and clear them */
  +   intr_sr = in_be32(drvdata-base_address + XPS2_IPISR_OFFSET);
  +   out_be32(drvdata-base_address + XPS2_IPISR_OFFSET, intr_sr);
  +
  +   /* Check which interrupt is active */
  +   if (intr_sr  XPS2_IPIXR_RX_OVF) {
  +   printk(KERN_ERR %s: receive overrun error\n,
  +   drvdata-serio.name);
  +   }
  +
  +   if (intr_sr  XPS2_IPIXR_RX_ERR) {
  +   drvdata-dfl |= SERIO_PARITY;
  +   }
  +
  +   if (intr_sr  (XPS2_IPIXR_TX_NOACK | XPS2_IPIXR_WDT_TOUT)) {
  +   drvdata-dfl |= SERIO_TIMEOUT;
  +   }
  +
  +   if (intr_sr  XPS2_IPIXR_RX_FULL) {
  +   retval = xps2_recv(drvdata, drvdata-rxb);
  +
  +   /* Error, if 1 byte is not received */
  +   if (retval != 1) {
  +   printk(KERN_ERR
  +   %s: wrong rcvd byte count (%d)\n,
  +   drvdata-serio.name, retval);

Don't you want to bail out here? Otherwise you will feed garbage to
serio_interrupt() I think.

  +   }
  +   c = drvdata-rxb;
  +   serio_interrupt(drvdata-serio, c, drvdata-dfl);
  +   drvdata-dfl = 0;
  +   }
  +
  +   if (intr_sr  XPS2_IPIXR_TX_ACK) {
  +
  +   /* Disable the TX interrupts after the transmission is
  +* complete */
  +   ier = in_be32(drvdata-base_address + XPS2_IPIER_OFFSET);
  +   ier = (~(XPS2_IPIXR_TX_ACK  XPS2_IPIXR_ALL ));
  +   out_be32(drvdata-base_address + XPS2_IPIER_OFFSET, ier);
  +   drvdata-dfl = 0;
  +   }
  +
  +   return IRQ_HANDLED;
  +}
  +
  +/***/
  +/* serio callbacks */
  +/***/
  +
  +/*
  + * sxps2_write() sends a byte out through the PS/2 interface.
  + *
  + * The sole purpose of drvdata-tx_end is to prevent the driver
  + * from locking up in the do {} while; loop when nothing is connected
  + * to the given PS/2 port. That's why we do not try to recover
  + * from the transmission failure.
  + * drvdata-tx_end needs not to be initialized to some far in the
  + * future value, as the very first attempt to xps2_send() a byte
  + * is always successful, and drvdata-tx_end will be set to a proper
  + * value at that moment - before the 1st use in the comparison.
  + */
 
 Good comment block.
 
 nitpick: can you reformat the comment blocks to be in kerneldoc format?
 That will allow 

Re: [PATCH] [V2] powerpc: Xilinx: PS2: Added new XPS PS2 driver

[Dmitry Torokhov]

On Mon, Jun 30, 2008 at 08:38:21AM -0700, John Linn wrote:
 + spin_lock_init(drvdata-lock);
 + dev_set_drvdata(dev, (void *)drvdata);

No need to cast to void *.

 +
 + if (!regs_res || !irq_res) {
 + dev_err(dev, IO resource(s) not found\n);
 + retval = -EFAULT;
 + goto failed1;
 + }
 +
 + drvdata-irq = irq_res-start;
 + remap_size = regs_res-end - regs_res-start + 1;
 + if (!request_mem_region(regs_res-start, remap_size, DRIVER_NAME)) {
 +
 + dev_err(dev, Couldn't lock memory region at 0x%08X\n,
 + (unsigned int)regs_res-start);
 + retval = -EBUSY;
 + goto failed1;
 + }
 +
 + /* Fill in configuration data and add them to the list */
 + drvdata-phys_addr = regs_res-start;
 + drvdata-remap_size = remap_size;
 + drvdata-base_address = ioremap(regs_res-start, remap_size);
 + if (drvdata-base_address == NULL) {
 +
 + dev_err(dev, Couldn't ioremap memory at 0x%08X\n,
 + (unsigned int)regs_res-start);
 + retval = -EFAULT;
 + goto failed2;
 + }
 +
 + /* Initialize the PS/2 interface */
 + down(cfg_sem);
 + if (xps2_initialize(drvdata)) {
 + up(cfg_sem);
 + dev_err(dev, Could not initialize device\n);
 + retval = -ENODEV;
 + goto failed3;
 + }
 + up(cfg_sem);

Do you need a counting semaphore here?

 +
 + dev_info(dev, Xilinx PS2 at 0x%08X mapped to 0x%08X, irq=%d\n,
 + drvdata-phys_addr, (u32)drvdata-base_address, drvdata-irq);
 +
 + drvdata-serio.id.type = SERIO_8042;
 + drvdata-serio.write = sxps2_write;
 + drvdata-serio.open = sxps2_open;
 + drvdata-serio.close = sxps2_close;
 + drvdata-serio.port_data = drvdata;
 + drvdata-serio.dev.parent = dev;
 + snprintf(drvdata-serio.name, sizeof(drvdata-serio.name),
 +  XPS2_NAME_DESC, id);
 + snprintf(drvdata-serio.phys, sizeof(drvdata-serio.phys),
 +  XPS2_PHYS_DESC, id);

I bet if you make a temp variable for drvdata-serio the code size wil
shrink a tiny bit.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] remove dead MAC_ADBKEYCODES

[Dmitry Torokhov]

On Nov 16, 2007 4:44 AM, Geert Uytterhoeven [EMAIL PROTECTED] wrote:

 Wrong mailing list ;-)

 On Thu, 15 Nov 2007, Stanislav Brabec wrote:
  It seems, that current kernel source code contains no traces of
  MAC_ADBKEYCODES and no reference to keyboard_sends_linux_keycodes any
  more.
 
  Attached patch removes them from configuration files.
 
  Signed-off-by: Stanislav Brabec [EMAIL PROTECTED]

 Acked-by: Geert Uytterhoeven [EMAIL PROTECTED]


Geert, are you going to push it through your tree?

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] fix appletouch geyser 1 breakage

[Dmitry Torokhov]

On Sunday 28 October 2007, Johannes Berg wrote:
 
  OK, then maybe instead of reverting the change outright we could try the
  patch below?
 
 That patch works,

Any chance Benjamin could also test it? The behaviour is different
from 2.6.24-rc1 since we call atp_geyser_init for all geysers now.
 
 minor comments: 
 
  Older models of fountains do not support change mode request and
 
 I think there's only one fountain model.

I was hoping that FOUNTAIN_TP_ONLY_PRODUCT_ID (0x30A) behaves similar
to Geyser in this regard. If you know that this assumption is incorrect
then we need to rename atp_is_older_fountain() to atp_is_fountain()
anf add this product ID to it.

 
  therefore shoudl be excluded from idle reset attempts.
 
 typo

OK

 
   /* MacBook Pro (Geyser 3  4) initialization constants */
 
 That comment is no longer correct, you should change it.
 

OK

  -#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1
 
  + * Reinitialise the device. This usually stops stream of empty packets
  + * coming form it.
 
 typo from
 

Yep, thanks.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] Prevent appletouch message log spam on 17 PB

[Dmitry Torokhov]

Hi Joseph,

On Sunday 28 October 2007 19:53, Joseph Jezak wrote:
 Perhaps this was a debugging message?  In any case, this printk
 causes lots of message log spam on my PB G4 1.67 (PowerBook5,9).
 
 This removes the printk.
 

I don't think we want do delete this message outright, we just need
to make sure we don't go through size detection when we re-initialize
the device. After all it is unlikely that touchpad will change its
size :)

BTW, could you tell me what product ID your touchpad has?

 Signed-off by: Joseph Jezak [EMAIL PROTECTED]
 ---
 
 diff --git a/drivers/input/mouse/appletouch.c 
 b/drivers/input/mouse/appletouch.c
 index f132702..6c47641 100644
 --- a/drivers/input/mouse/appletouch.c
 +++ b/drivers/input/mouse/appletouch.c
 @@ -433,7 +433,6 @@ static void atp_complete(struct urb* urb)
   for (i = (atp_is_geyser_2(dev)?15:16); i  ATP_XSENSORS; i++) {
   if (!dev-xy_cur[i]) continue;
  
 - printk(appletouch: 17\ model detected.\n);
   if(atp_is_geyser_2(dev))
   input_set_abs_params(dev-input, ABS_X, 0,
(20 - 1) *
 

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] fix appletouch geyser 1 breakage

[Dmitry Torokhov]

On Sunday 28 October 2007 11:08, Johannes Berg wrote:
 
  I was hoping that FOUNTAIN_TP_ONLY_PRODUCT_ID (0x30A) behaves similar
  to Geyser in this regard. If you know that this assumption is incorrect
  then we need to rename atp_is_older_fountain() to atp_is_fountain()
  anf add this product ID to it.
 
 Ah ok, I forgot about that one. If I were to venture a guess I'd say
 that they renamed it to geyser because of this idle behaviour
 technology upgrade, but I can't say, nor do I know anybody with such a
 version, sorry.

OK then let's play safe and don't touch fountains at all. How about the
patch below?

-- 
Dmitry

Input: appletouch - idle reset logic broke older Fountains

Fountains do not support change mode request and therefore
should be excluded from idle reset attempts.

Also:
 - do not re-submit URB when we decide that toucvhpad needs to be
   reinicialized
 - do not repeat size detection when reinitializing the touchpad
 - Add missing KERN_* prefixes to messages

Signed-off-by: Dmitry Torokhov [EMAIL PROTECTED]
---
 drivers/input/mouse/appletouch.c |  125 ---
 1 file changed, 77 insertions(+), 48 deletions(-)

Index: work/drivers/input/mouse/appletouch.c
===
--- work.orig/drivers/input/mouse/appletouch.c
+++ work/drivers/input/mouse/appletouch.c
@@ -129,12 +129,12 @@ MODULE_DEVICE_TABLE (usb, atp_table);
  */
 #define ATP_THRESHOLD   5
 
-/* MacBook Pro (Geyser 3  4) initialization constants */
-#define ATP_GEYSER3_MODE_READ_REQUEST_ID 1
-#define ATP_GEYSER3_MODE_WRITE_REQUEST_ID 9
-#define ATP_GEYSER3_MODE_REQUEST_VALUE 0x300
-#define ATP_GEYSER3_MODE_REQUEST_INDEX 0
-#define ATP_GEYSER3_MODE_VENDOR_VALUE 0x04
+/* Geyser initialization constants */
+#define ATP_GEYSER_MODE_READ_REQUEST_ID1
+#define ATP_GEYSER_MODE_WRITE_REQUEST_ID   9
+#define ATP_GEYSER_MODE_REQUEST_VALUE  0x300
+#define ATP_GEYSER_MODE_REQUEST_INDEX  0
+#define ATP_GEYSER_MODE_VENDOR_VALUE   0x04
 
 /* Structure to hold all of our device specific stuff */
 struct atp {
@@ -142,9 +142,11 @@ struct atp {
struct usb_device * udev;   /* usb device */
struct urb *urb;/* usb request block */
signed char *   data;   /* transferred data */
-   int open;   /* non-zero if opened */
-   struct input_dev*input; /* input dev */
-   int valid;  /* are the sensors valid ? */
+   struct input_dev *  input;  /* input dev */
+   unsigned char   open;   /* non-zero if opened */
+   unsigned char   valid;  /* are the sensors valid ? */
+   unsigned char   size_detect_done;
+   unsigned char   overflowwarn;   /* overflow warning printed? */
int x_old;  /* last reported x/y, */
int y_old;  /* used for smoothing */
/* current value of the sensors 
*/
@@ -153,7 +155,6 @@ struct atp {
signed char xy_old[ATP_XSENSORS + ATP_YSENSORS];
/* accumulated sensors */
int xy_acc[ATP_XSENSORS + ATP_YSENSORS];
-   int overflowwarn;   /* overflow warning printed? */
int datalen;/* size of an USB urb transfer 
*/
int idlecount;  /* number of empty packets */
struct work_struct  work;
@@ -170,7 +171,7 @@ struct atp {
 
 #define dprintk(format, a...)  \
do {\
-   if (debug) printk(format, ##a); \
+   if (debug) printk(KERN_DEBUG format, ##a);  
\
} while (0)
 
 MODULE_AUTHOR(Johannes Berg, Stelian Pop, Frank Arnold, Michael Hanselmann);
@@ -188,6 +189,15 @@ static int debug = 1;
 module_param(debug, int, 0644);
 MODULE_PARM_DESC(debug, Activate debugging output);
 
+static inline int atp_is_fountain(struct atp *dev)
+{
+   u16 productId = le16_to_cpu(dev-udev-descriptor.idProduct);
+
+   return productId == FOUNTAIN_ANSI_PRODUCT_ID ||
+  productId == FOUNTAIN_ISO_PRODUCT_ID ||
+  productId == FOUNTAIN_TP_ONLY_PRODUCT_ID;
+}
+
 /* Checks if the device a Geyser 2 (ANSI, ISO, JIS) */
 static inline int atp_is_geyser_2(struct atp *dev)
 {
@@ -211,52 +221,63 @@ static inline int atp_is_geyser_3(struct
 }
 
 /*
- * By default Geyser 3 device sends standard USB HID mouse
+ * By default newer Geyser devices send standard USB HID mouse
  * packets (Report ID 2). This code changes device mode, so it
  * sends raw sensor reports (Report ID 5).
  */
-static int

Re: [PATCH] fix appletouch geyser 1 breakage

[Dmitry Torokhov]

On 10/25/07, Benjamin Berg [EMAIL PROTECTED] wrote:
 On Thu, 2007-25-10 at 15:23 +0200, Johannes Berg wrote:
  On Wed, 2007-10-24 at 10:29 -0400, Dmitry Torokhov wrote:
 
   Do yo know who has powerbooks with older geyser models (0x214, 215,
   216)?
 
  Not sure, Benjamin? We're talking about the touchpad, just lsusb should
  be enough.

 lsusb says I have a 0x215
 (Bus 003 Device 002: ID 05ac:0215 Apple Computer, Inc.)

 Everything is working fine with a 2.6.24-rc1 kernel (ie. no bogus
 messages).

   It would be nice to know if they send the data continiously and
   whether the geyser 3 reset hack works on them.
 
  That'd be good, but for fountains it definitely doesn't work.

 Benjamin


Johannes, and what is product ID for your touchpad?

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] fix appletouch geyser 1 breakage

[Dmitry Torokhov]

Hi Johannes,

On 10/24/07, Johannes Berg [EMAIL PROTECTED] wrote:
 The patch 46249ea60fbb61a72ee6929b831b1f3e6865f024 was obviously done
 without testing on a Geyser 1,

My fault, sorry. However Anton's device has product ID of 90x30B which
is Geyser 1 as far as I understand... But yes, we should not expect
other geysers respond to Geyser 3-specific commands.

  and I'm a very annoyed that it was
 applied. It causes appletouch to continuously printk:

 drivers/input/mouse/appletouch.c: Could not do mode read request from device 
 (Geyser 3 mode)

 because the Geyser 1 doesn't respond to that. The patch description also
 states:

  if we see 10 empty packets the touchpad needs to be reset; good
  touchpads should not send empty packets anyway.

 which is *TOTALLY* bogus since Geyser 1 touchpads have no notion of
 empty packets, the simply continuously send measurements. One look at
 the specification would have confirmed that.


Is there a way to plug these Geysers? Waking up the kernel
continuously is not nice.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] fix appletouch geyser 1 breakage

[Dmitry Torokhov]

On 10/24/07, Johannes Berg [EMAIL PROTECTED] wrote:
 Hi,

  My fault, sorry.

 No, actually, I was wrong about Geyser 1, mine is a fountain.

  Is there a way to plug these Geysers? Waking up the kernel
  continuously is not nice.

 Not sure really, maybe checking for is_geyser instead of is_geyser_3?


Well, but what about fountains then? Regardless of the model, if there
is a way to stop empty meaurements, we should do it.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] fix appletouch geyser 1 breakage

[Dmitry Torokhov]

On 10/24/07, Johannes Berg [EMAIL PROTECTED] wrote:
 On Wed, 2007-10-24 at 09:34 -0400, Dmitry Torokhov wrote:

  Well, but what about fountains then? Regardless of the model, if there
  is a way to stop empty meaurements, we should do it.

 There is no way on fountains though. We could check the measurement
 ourselves and if no finger is detected decrease the polling frequency or
 something, but there's no hw support.


Do yo know who has powerbooks with older geyser models (0x214, 215,
216)? It would be nice to know if they send the data continiously and
whether the geyser 3 reset hack works on them.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: current -git adbhid.c build error

[Dmitry Torokhov]

On Wednesday 17 October 2007, Paul Mackerras wrote:
 
 It was a mis-merge between 555ddbb4, which made that change and *did*
 add the declaration of key, and 9a402b64, which deleted the line that
 the declaration of key was added to.

Oops, sorry.

 
 We need the patch below.  Dmitry, will you push it or will I?
 

Please do. Thanks!

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [patch 1/3] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible

[Dmitry Torokhov]

On Sunday 22 July 2007 08:51, Geert Uytterhoeven wrote:
 On Sun, 22 Jul 2007, Dmitry Torokhov wrote:
  On Saturday 21 July 2007 04:27, Geert Uytterhoeven wrote:
   On Fri, 20 Jul 2007, Dmitry Torokhov wrote:
I am OK with adding a new header file. I was just saying that placing
that declaration in linux/hid.h makes about the same sense as putting
it into linux/scsi.h
   
   At first I just wanted to move it. Then I thought about the angry
   comments I would get about not moving it to a header file ;-)
   
   linux/hid.h looked like the best candidate. linux/kbd_kern.h is
   another option.
   
  
  linux/kbd_kern.h sounds much better.
 
 And so it will be.

Applied to 'for-linus' branch of input tree, thank you.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [patch 1/3] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible

[Dmitry Torokhov]

Hi Geert,

On Saturday 21 July 2007 04:27, Geert Uytterhoeven wrote:
 On Fri, 20 Jul 2007, Dmitry Torokhov wrote:
  
  I am OK with adding a new header file. I was just saying that placing
  that declaration in linux/hid.h makes about the same sense as putting
  it into linux/scsi.h
 
 At first I just wanted to move it. Then I thought about the angry
 comments I would get about not moving it to a header file ;-)
 
 linux/hid.h looked like the best candidate. linux/kbd_kern.h is
 another option.
 

linux/kbd_kern.h sounds much better.

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [patch 1/3] m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible

[Dmitry Torokhov]

Hi Geert,

On 7/20/07, Geert Uytterhoeven [EMAIL PROTECTED] wrote:
 From: Geert Uytterhoeven [EMAIL PROTECTED]

 m68k/mac: Make mac_hid_mouse_emulate_buttons() declaration visible

 drivers/char/keyboard.c: In function 'kbd_keycode':
 drivers/char/keyboard.c:1142: error: implicit declaration of function 
 'mac_hid_mouse_emulate_buttons'

 The forward declaration of mac_hid_mouse_emulate_buttons() is not visible on
 m68k because it's hidden in the middle of a big #ifdef block.

 Move it to linux/hid.h, correct the type of the second parameter, and
 include linux/hid.h where needed.

linux/hid.h contains definitions needed for drivers speaking HID
protocol, I don't think we want to put quirks for legacy keyboard
driver there. I'd just move the #ifdef within drivers/char/keyboard.c
for now.

BTW, I don't think that mac button emulation will work well when x86
evdev-based driver gains popularity - it grabs the device and so no
event will flow through keyboard driver... We'd need a new solution...

-- 
Dmitry
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev