* [PATCH v5 05/11] mfd: core: document mfd_add_devices()
From: Bartosz Golaszewski @ 2019-02-13 13:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190213134335.10838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add a kernel doc for mfd_add_devices().
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/mfd/mfd-core.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index 94e3f32ce935..0898a8db1747 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -269,6 +269,20 @@ static int mfd_add_device(struct device *parent, int id,
return ret;
}
+/**
+ * mfd_add_devices - register a set of child devices
+ *
+ * @parent: Parent device for all sub-nodes.
+ * @id: Platform device id. If >= 0, each sub-device will have its cell_id
+ * added to this number and use it as the platform device id.
+ * @cells: Array of mfd cells describing sub-devices.
+ * @n_devs: Number of sub-devices to register.
+ * @mem_base: Parent register range resource for sub-devices.
+ * @irq_base: Base of the range of virtual interrupt numbers allocated for
+ * this MFD device. Unused if @domain is specified.
+ * @domain: Interrupt domain used to create mappings for HW interrupt numbers
+ * specificed in sub-devices' IRQ resources.
+ */
int mfd_add_devices(struct device *parent, int id,
const struct mfd_cell *cells, int n_devs,
struct resource *mem_base,
--
2.20.1
^ permalink raw reply related
* [PATCH v5 04/11] dt-bindings: input: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-02-13 13:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190213134335.10838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the DT binding document for the onkey module of max77650.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../bindings/input/max77650-onkey.txt | 26 +++++++++++++++++++
1 file changed, 26 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
diff --git a/Documentation/devicetree/bindings/input/max77650-onkey.txt b/Documentation/devicetree/bindings/input/max77650-onkey.txt
new file mode 100644
index 000000000000..37c80898be4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/max77650-onkey.txt
@@ -0,0 +1,26 @@
+Onkey driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The onkey controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+Required properties:
+--------------------
+- compatible: Must be "maxim,max77650-onkey".
+
+Optional properties:
+- linux,code: The key-code to be reported when the key is pressed.
+ Defaults to KEY_POWER.
+- maxim,onkey-mode: Must be "push" or "slide" depending on the type of
+ button used by the system. Defaults to "push".
+
+Example:
+--------
+
+ onkey {
+ compatible = "maxim,max77650-onkey";
+ linux,code = <KEY_END>;
+ maxim,onkey-mode = "slide";
+ };
--
2.20.1
^ permalink raw reply related
* [PATCH v5 03/11] dt-bindings: leds: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-02-13 13:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190213134335.10838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the DT binding document for the LEDs module of max77650.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../bindings/leds/leds-max77650.txt | 57 +++++++++++++++++++
1 file changed, 57 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt
diff --git a/Documentation/devicetree/bindings/leds/leds-max77650.txt b/Documentation/devicetree/bindings/leds/leds-max77650.txt
new file mode 100644
index 000000000000..3a67115cc1da
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-max77650.txt
@@ -0,0 +1,57 @@
+LED driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The LED controller is represented as a sub-node of the PMIC node on
+the device tree.
+
+This device has three current sinks.
+
+Required properties:
+--------------------
+- compatible: Must be "maxim,max77650-led"
+- #address-cells: Must be <1>.
+- #size-cells: Must be <0>.
+
+Each LED is represented as a sub-node of the LED-controller node. Up to
+three sub-nodes can be defined.
+
+Required properties of the sub-node:
+------------------------------------
+
+- reg: Must be <0>, <1> or <2>.
+
+Optional properties of the sub-node:
+------------------------------------
+
+- label: See Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger: See Documentation/devicetree/bindings/leds/common.txt
+
+For more details, please refer to the generic GPIO DT binding document
+<devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+ leds {
+ compatible = "maxim,max77650-led";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ led@0 {
+ reg = <0>;
+ label = "blue:usr0";
+ };
+
+ led@1 {
+ reg = <1>;
+ label = "red:usr1";
+ linux,default-trigger = "heartbeat";
+ };
+
+ led@2 {
+ reg = <2>;
+ label = "green:usr2";
+ };
+ };
--
2.20.1
^ permalink raw reply related
* [PATCH v5 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-02-13 13:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190213134335.10838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add the DT binding document for the battery charger module of max77650.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../power/supply/max77650-charger.txt | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
new file mode 100644
index 000000000000..16e24173c80e
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
@@ -0,0 +1,27 @@
+Battery charger driver for MAX77650 PMIC from Maxim Integrated.
+
+This module is part of the MAX77650 MFD device. For more details
+see Documentation/devicetree/bindings/mfd/max77650.txt.
+
+The charger is represented as a sub-node of the PMIC node on the device tree.
+
+Required properties:
+--------------------
+- compatible: Must be "maxim,max77650-charger"
+
+Optional properties:
+--------------------
+- min-input-voltage: Minimum CHGIN regulation voltage (in microvolts). Must be
+ one of: 4000000, 4100000, 4200000, 4300000, 4400000,
+ 4500000, 4600000, 4700000.
+- current-limit: CHGIN input current limit (in microamps). Must be one of:
+ 95000, 190000, 285000, 380000, 475000.
+
+Example:
+--------
+
+ charger {
+ compatible = "maxim,max77650-charger";
+ maxim,vchgin-min = <4200000>;
+ maxim,ichgin-lim = <285000>;
+ };
--
2.20.1
^ permalink raw reply related
* [PATCH v5 01/11] dt-bindings: mfd: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-02-13 13:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
In-Reply-To: <20190213134335.10838-1-brgl@bgdev.pl>
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Add a DT binding document for max77650 ultra-low power PMIC. This
describes the core mfd device and the GPIO module.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
.../devicetree/bindings/mfd/max77650.txt | 47 +++++++++++++++++++
1 file changed, 47 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
diff --git a/Documentation/devicetree/bindings/mfd/max77650.txt b/Documentation/devicetree/bindings/mfd/max77650.txt
new file mode 100644
index 000000000000..0a40e9c6aab8
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/max77650.txt
@@ -0,0 +1,47 @@
+MAX77650 ultra low-power PMIC from Maxim Integrated.
+
+Required properties:
+-------------------
+- compatible: Must be "maxim,max77650"
+- reg: I2C device address.
+- interrupts: The interrupt on the parent the controller is
+ connected to.
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Must be <2>.
+
+- gpio-controller : Marks the device node as a gpio controller.
+- #gpio-cells : Must be <2>. The first cell is the pin number and
+ the second cell is used to specify the gpio active
+ state.
+
+Optional properties:
+--------------------
+gpio-line-names: Single string containing the name of the GPIO line.
+
+The GPIO-controller module is represented as part of the top-level PMIC
+node. The device exposes a single GPIO line.
+
+For device-tree bindings of other sub-modules (regulator, power supply,
+LEDs and onkey) refer to the binding documents under the respective
+sub-system directories.
+
+For more details on GPIO bindings, please refer to the generic GPIO DT
+binding document <devicetree/bindings/gpio/gpio.txt>.
+
+Example:
+--------
+
+ pmic: max77650@48 {
+ compatible = "maxim,max77650";
+ reg = <0x48>;
+
+ interrupt-controller;
+ interrupt-parent = <&gpio2>;
+ #interrupt-cells = <2>;
+ interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ gpio-line-names = "max77650-charger";
+ };
--
2.20.1
^ permalink raw reply related
* [PATCH v5 00/11] mfd: add support for max77650 PMIC
From: Bartosz Golaszewski @ 2019-02-13 13:43 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Lee Jones, Sebastian Reichel,
Liam Girdwood, Greg Kroah-Hartman
Cc: linux-kernel, linux-gpio, devicetree, linux-input, linux-leds,
linux-pm, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This series adds support for max77650 ultra low-power PMIC. It provides
the core mfd driver and a set of five sub-drivers for the regulator,
power supply, gpio, leds and input subsystems.
Patches 1-4 add the DT binding documents. Patch 5 documents mfd_add_devices().
Patches 6-10 add all drivers. Last patch adds a MAINTAINERS entry for this
device.
The regulator part has already been picked up into the regulator tree.
v1 -> v2:
=========
General:
- use C++ style comments for the SPDX license identifier and the
copyright header
- s/MODULE_LICENSE("GPL")/MODULE_LICENSE("GPL v2")/
- lookup the virtual interrupt numbers in the MFD driver, setup
resources for child devices and use platform_get_irq_byname()
in sub-drivers
- picked up review tags
- use devm_request_any_context_irq() for interrupt requests
LEDs:
- changed the max77650_leds_ prefix to max77650_led_
- drop the max77650_leds structure as the only field it held was the
regmap pointer, move said pointer to struct max77650_led
- change the driver name to "max77650-led"
- drop the last return value check and return the result of
regmap_write() directly
- change the labeling scheme to one consistent with other LED drivers
ONKEY:
- drop the key reporting helper and call the input functions directly
from interrupt handlers
- rename the rv local variable to error
- drop parent device asignment
Regulator:
- drop the unnecessary init_data lookup from the driver code
- drop unnecessary include
Charger:
- disable the charger on driver remove
- change the power supply type to POWER_SUPPLY_TYPE_USB
GPIO:
- drop interrupt support until we have correct implementation of hierarchical
irqs in gpiolib
v2 -> v3:
=========
General:
- dropped regulator patches as they're already in Mark Brown's branch
LED:
- fix the compatible string in the DT binding example
- use the max_brightness property
- use a common prefix ("MAX77650_LED") for all defines in the driver
MFD:
- add the MODULE_DEVICE_TABLE()
- add a sentinel to the of_device_id array
- constify the pointers to irq names
- use an enum instead of defines for interrupt indexes
v3 -> v4:
=========
GPIO:
- as discussed with Linus Walleij: the gpio-controller is now part of
the core mfd module (we don't spawn a sub-node anymore), the binding
document for GPIO has been dropped, the GPIO properties have been
defined in the binding document for the mfd core, the interrupt
functionality has been reintroduced with the irq directly passed from
the mfd part
- due to the above changes the Reviewed-by tag from Linus was dropped
v4 -> v5:
=========
General:
- add a patch documenting mfd_add_devices()
MFD:
- pass the regmap irq_chip irq domain to mfd over mfd_add_devices so that
the hw interrupts from resources can be correctly mapped to virtual irqs
- remove the enum listing cell indexes
- extend Kconfig help
- add a link to the programming manual
- use REGMAP_IRQ_REG() for regmap interrupts (except for GPI which has
is composed of two hw interrupts for rising and falling edge)
- add error messages in probe
- use PLATFORM_DEVID_NONE constant in devm_mfd_add_devices()
- set irq_base to 0 in regmap_add_irq_chip() as other users to, it's only
relevant if it's > 0
Charger:
- use non-maxim specific property names for minimum input voltage and current
limit
- code shrink by using the enable/disable charger helpers everywhere
- use more descriptive names for constants
Onkey:
- use EV_SW event type for slide mode
LED:
- remove stray " from Kconfig help
Bartosz Golaszewski (11):
dt-bindings: mfd: add DT bindings for max77650
dt-bindings: power: supply: add DT bindings for max77650
dt-bindings: leds: add DT bindings for max77650
dt-bindings: input: add DT bindings for max77650
mfd: core: document mfd_add_devices()
mfd: max77650: new core mfd driver
power: supply: max77650: add support for battery charger
gpio: max77650: add GPIO support
leds: max77650: add LEDs support
input: max77650: add onkey support
MAINTAINERS: add an entry for max77650 mfd driver
.../bindings/input/max77650-onkey.txt | 26 ++
.../bindings/leds/leds-max77650.txt | 57 +++
.../devicetree/bindings/mfd/max77650.txt | 47 +++
.../power/supply/max77650-charger.txt | 27 ++
MAINTAINERS | 14 +
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-max77650.c | 190 ++++++++++
drivers/input/misc/Kconfig | 9 +
drivers/input/misc/Makefile | 1 +
drivers/input/misc/max77650-onkey.c | 131 +++++++
drivers/leds/Kconfig | 6 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-max77650.c | 147 ++++++++
drivers/mfd/Kconfig | 14 +
drivers/mfd/Makefile | 1 +
drivers/mfd/max77650.c | 234 ++++++++++++
drivers/mfd/mfd-core.c | 14 +
drivers/power/supply/Kconfig | 7 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/max77650-charger.c | 353 ++++++++++++++++++
include/linux/mfd/max77650.h | 59 +++
22 files changed, 1347 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/max77650-onkey.txt
create mode 100644 Documentation/devicetree/bindings/leds/leds-max77650.txt
create mode 100644 Documentation/devicetree/bindings/mfd/max77650.txt
create mode 100644 Documentation/devicetree/bindings/power/supply/max77650-charger.txt
create mode 100644 drivers/gpio/gpio-max77650.c
create mode 100644 drivers/input/misc/max77650-onkey.c
create mode 100644 drivers/leds/leds-max77650.c
create mode 100644 drivers/mfd/max77650.c
create mode 100644 drivers/power/supply/max77650-charger.c
create mode 100644 include/linux/mfd/max77650.h
--
2.20.1
^ permalink raw reply
* [PATCH] Input: st1232 - include gpio/consumer.h header for gpiod_set_value_cansleep()
From: Martin Kepplinger @ 2019-02-13 11:19 UTC (permalink / raw)
To: dmitry.torokhov, kuninori.morimoto.gx, linux-input
Cc: linux-kernel, Martin Kepplinger
In-Reply-To: <201902131841.xHSZ8Cvm%fengguang.wu@intel.com>
gpiod_set_value_cansleep() needs it's declaration in the corresponding
header. This fixes build errors like
drivers/input/touchscreen/st1232.c: In function 'st1232_ts_power':
>> drivers/input/touchscreen/st1232.c:146:3: error: implicit declaration of
function 'gpiod_set_value_cansleep'; did you mean 'gpio_set_value_cansleep'?
[-Werror=implicit-function-declaration]
gpiod_set_value_cansleep(ts->reset_gpio, !poweron);
^~~~~~~~~~~~~~~~~~~~~~~~
gpio_set_value_cansleep
Signed-off-by: Martin Kepplinger <martink@posteo.de>
---
drivers/input/touchscreen/st1232.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/touchscreen/st1232.c b/drivers/input/touchscreen/st1232.c
index 1fbc0847416b..f4b7a4f1c366 100644
--- a/drivers/input/touchscreen/st1232.c
+++ b/drivers/input/touchscreen/st1232.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
#include <linux/pm_qos.h>
#include <linux/slab.h>
#include <linux/types.h>
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Pavel Machek @ 2019-02-13 11:02 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lee Jones, Bartosz Golaszewski, Rob Herring, Mark Rutland,
Linus Walleij, Dmitry Torokhov, Jacek Anaszewski,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <CAMRc=MeTMq_GWzQB6LBvts1csMftj=On5sB22jsJZzQzONxbNg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 948 bytes --]
Hi!
> > As you can see, it will return the IRQ Domain for the chip.
> >
> > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > remove all the nastiness in max77650_setup_irqs() and have the Input
> > device use the standard (e.g. platform_get_irq()) APIs.
> >
> > How does that Sound?
>
> This does sound better! Why didn't you lead with that in the first
>place?
TBH, this can be considered rude and I'm not surprised Lee reacted the
way he did.
> It's a pity it's not documented, I had to look at the code to find out
> irq resources would get translated in mfd_add_devices() if a domain is
> present.
I guess documentation patch would be welcome.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 09/11] leds: max77650: add LEDs support
From: Bartosz Golaszewski @ 2019-02-13 10:58 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212121236.GD25613@amd>
wt., 12 lut 2019 o 13:12 Pavel Machek <pavel@ucw.cz> napisał(a):
>
>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index a72f97fca57b..6e7a8f51eccc 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -608,6 +608,12 @@ config LEDS_TLC591XX
> > This option enables support for Texas Instruments TLC59108
> > and TLC59116 LED controllers.
> >
> > +config LEDS_MAX77650
> > + tristate "LED support for Maxim MAX77650 PMIC"
> > + depends on MFD_MAX77650
> > + help
> > + LEDs driver for MAX77650 family of PMICs from Maxim Integrated."
> > +
>
> List the chips? Remove extra "?
> Pavel
They are already listed in the core mfd Kconfig, but thanks for
spotting that stray ".
Bart
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-13 10:46 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <20190213103946.GG1863@dell>
śr., 13 lut 2019 o 11:39 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
>
> > śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > >
> > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > >
> > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > > > >
> > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > > > >
> > > > > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > > > >
> > > > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > > > >
> > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > > > > >
> > > > > > > > > > > > Need I go on? :)
> > > > > > > > > > > >
> > > > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > > > >
> > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > > > >
> > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > > > > >
> > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > > > >
> > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > > > >
> > > > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > > > >
> > > > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > > > >
> > > > > > > Exampe:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata {
> > > > > > > int gpi_irq;
> > > > > > > };
> > > > > > >
> > > > > > > In MFD driver:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > > > >
> > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > > > >
> > > > > > > gpio_cell.platform_data = gpio_data;
> > > > > > >
> > > > > > > In GPIO driver:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > > > >
> > > > > > > int irq = gpio_data->gpi_irq;
> > > > > >
> > > > > > Definitely not. What you're trying to do is a hack.
> > > > > >
> > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > > > in the client to pull them out. Setting them via Regmap, then pulling
> > > > > > them out again in the *same driver*, only to store them in platform
> > > > > > data to be passed to a child device is bonkers.
> > > > > >
> > > > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > > > handle them via the Regmap APIs, *not* both.
> > > > >
> > > > > Right, a plan has been formed.
> > > > >
> > > > > Hopefully this works and you can avoid all this dancing around.
> > > > >
> > > > > Firstly, you need to make a small change to:
> > > > >
> > > > > drivers/base/regmap/regmap-irq.c
> > > > >
> > > > > Add the following function:
> > > > >
> > > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> > > >
> > > > We already do have such function and a lot of mfd drivers actually use it.
> > >
> > > Even better.
> > >
> > > > > As you can see, it will return the IRQ Domain for the chip.
> > > > >
> > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > > > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > > > device use the standard (e.g. platform_get_irq()) APIs.
> > > > >
> > > > > How does that Sound?
> > > >
> > > > This does sound better! Why didn't you lead with that in the first place?
> > >
> > > I'm not even going to dignify that stupid question with a response.
> >
> > It's not a stupid question and you're being unnecessarily rude. As an
> > expert in the subsystem you maintain you could have answered simply
> > with a "this is wrong, retrieve the irq domain from the regmap
> > irq_chip and pass it over to mfd_add_devices, the mfd core will create
> > appropriate mappings".
>
> Could be culture clash, but I found the question offensive which is
> why I chose not to answer it. The reason is actually explained below:
>
It wasn't meant to be offensive. I guess when dealing with non-native
English speakers over e-mail it's best to assume good faith.
> "It's only the craziness in this patch which forced me to look into how
> Regmap handles IRQs and come up with a subsequent solution which fits
> your use-case."
>
> Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain
> could be fetched and reused here didn't enter my thought process until
> I delved into the inner workings of Regmap.
>
> Yes, I know MFD pretty well, but I only tend to deep-dive into other
> subsystems, particularly ones as complicated as Regmap, when it's
> necessary to do so. Now I know a little more about it, I can provide
> the feedback you suggest going forward.
>
> > > > It's a pity it's not documented, I had to look at the code to find out
> > > > irq resources would get translated in mfd_add_devices() if a domain is
> > > > present.
> > >
> > > Where is it likely to be documented? MFD is pretty simple and seldom
> > > needs explanation. A 3 second look at the API you're trying to use
> > > (instead of blind copy & paste) would have told you that it's possible
> > > to take an IRQ domain as an argument.
> > >
> > > It's only the craziness in this patch which forced me to look into how
> > > Regmap handles IRQs and come up with a subsequent solution which fits
> > > your use-case.
> > >
> > > > In that case - I really don't see a reason to create a superfluous
> > > > structure to only hold the main regmap - we can very well get it from
> > > > the parent device in sub-drivers as I do now.
> > >
> > > The whole point of this solution is that you don't need to pass
> > > anything non-standard (i.e. not provided by the current APIs) to the
> > > child device.
> >
> > I don't understand what you're saying here.
>
> I'm saying that the structure you speak of is no longer required.
>
Thanks for clarifying.
Bartosz
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-13 10:39 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <CAMpxmJXqEwNtyT1ahmRd05QOjhdi0rT_-5S9Lk8O-PmV8BuA6w@mail.gmail.com>
On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > >
> > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > > >
> > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > > >
> > > > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > > >
> > > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > > >
> > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > > > >
> > > > > > > > > > > Need I go on? :)
> > > > > > > > > > >
> > > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > > >
> > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > > >
> > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > > > >
> > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > > >
> > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > > >
> > > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > > >
> > > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > > >
> > > > > > Exampe:
> > > > > >
> > > > > > struct max77650_gpio_pdata {
> > > > > > int gpi_irq;
> > > > > > };
> > > > > >
> > > > > > In MFD driver:
> > > > > >
> > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > > >
> > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > > >
> > > > > > gpio_cell.platform_data = gpio_data;
> > > > > >
> > > > > > In GPIO driver:
> > > > > >
> > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > > >
> > > > > > int irq = gpio_data->gpi_irq;
> > > > >
> > > > > Definitely not. What you're trying to do is a hack.
> > > > >
> > > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > > in the client to pull them out. Setting them via Regmap, then pulling
> > > > > them out again in the *same driver*, only to store them in platform
> > > > > data to be passed to a child device is bonkers.
> > > > >
> > > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > > handle them via the Regmap APIs, *not* both.
> > > >
> > > > Right, a plan has been formed.
> > > >
> > > > Hopefully this works and you can avoid all this dancing around.
> > > >
> > > > Firstly, you need to make a small change to:
> > > >
> > > > drivers/base/regmap/regmap-irq.c
> > > >
> > > > Add the following function:
> > > >
> > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> > >
> > > We already do have such function and a lot of mfd drivers actually use it.
> >
> > Even better.
> >
> > > > As you can see, it will return the IRQ Domain for the chip.
> > > >
> > > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > > device use the standard (e.g. platform_get_irq()) APIs.
> > > >
> > > > How does that Sound?
> > >
> > > This does sound better! Why didn't you lead with that in the first place?
> >
> > I'm not even going to dignify that stupid question with a response.
>
> It's not a stupid question and you're being unnecessarily rude. As an
> expert in the subsystem you maintain you could have answered simply
> with a "this is wrong, retrieve the irq domain from the regmap
> irq_chip and pass it over to mfd_add_devices, the mfd core will create
> appropriate mappings".
Could be culture clash, but I found the question offensive which is
why I chose not to answer it. The reason is actually explained below:
"It's only the craziness in this patch which forced me to look into how
Regmap handles IRQs and come up with a subsequent solution which fits
your use-case."
Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain
could be fetched and reused here didn't enter my thought process until
I delved into the inner workings of Regmap.
Yes, I know MFD pretty well, but I only tend to deep-dive into other
subsystems, particularly ones as complicated as Regmap, when it's
necessary to do so. Now I know a little more about it, I can provide
the feedback you suggest going forward.
> > > It's a pity it's not documented, I had to look at the code to find out
> > > irq resources would get translated in mfd_add_devices() if a domain is
> > > present.
> >
> > Where is it likely to be documented? MFD is pretty simple and seldom
> > needs explanation. A 3 second look at the API you're trying to use
> > (instead of blind copy & paste) would have told you that it's possible
> > to take an IRQ domain as an argument.
> >
> > It's only the craziness in this patch which forced me to look into how
> > Regmap handles IRQs and come up with a subsequent solution which fits
> > your use-case.
> >
> > > In that case - I really don't see a reason to create a superfluous
> > > structure to only hold the main regmap - we can very well get it from
> > > the parent device in sub-drivers as I do now.
> >
> > The whole point of this solution is that you don't need to pass
> > anything non-standard (i.e. not provided by the current APIs) to the
> > child device.
>
> I don't understand what you're saying here.
I'm saying that the structure you speak of is no longer required.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-13 10:15 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <20190213095330.GF1863@dell>
śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
>
> > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > >
> > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > >
> > > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > >
> > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > >
> > > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > > >
> > > > > > > > > > Need I go on? :)
> > > > > > > > > >
> > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > >
> > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > >
> > > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > > >
> > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > >
> > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > >
> > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > >
> > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > >
> > > > > Exampe:
> > > > >
> > > > > struct max77650_gpio_pdata {
> > > > > int gpi_irq;
> > > > > };
> > > > >
> > > > > In MFD driver:
> > > > >
> > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > >
> > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > >
> > > > > gpio_cell.platform_data = gpio_data;
> > > > >
> > > > > In GPIO driver:
> > > > >
> > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > >
> > > > > int irq = gpio_data->gpi_irq;
> > > >
> > > > Definitely not. What you're trying to do is a hack.
> > > >
> > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > in the client to pull them out. Setting them via Regmap, then pulling
> > > > them out again in the *same driver*, only to store them in platform
> > > > data to be passed to a child device is bonkers.
> > > >
> > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > handle them via the Regmap APIs, *not* both.
> > >
> > > Right, a plan has been formed.
> > >
> > > Hopefully this works and you can avoid all this dancing around.
> > >
> > > Firstly, you need to make a small change to:
> > >
> > > drivers/base/regmap/regmap-irq.c
> > >
> > > Add the following function:
> > >
> > > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> >
> > We already do have such function and a lot of mfd drivers actually use it.
>
> Even better.
>
> > > As you can see, it will return the IRQ Domain for the chip.
> > >
> > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > device use the standard (e.g. platform_get_irq()) APIs.
> > >
> > > How does that Sound?
> >
> > This does sound better! Why didn't you lead with that in the first place?
>
> I'm not even going to dignify that stupid question with a response.
>
It's not a stupid question and you're being unnecessarily rude. As an
expert in the subsystem you maintain you could have answered simply
with a "this is wrong, retrieve the irq domain from the regmap
irq_chip and pass it over to mfd_add_devices, the mfd core will create
appropriate mappings".
> > It's a pity it's not documented, I had to look at the code to find out
> > irq resources would get translated in mfd_add_devices() if a domain is
> > present.
>
> Where is it likely to be documented? MFD is pretty simple and seldom
> needs explanation. A 3 second look at the API you're trying to use
> (instead of blind copy & paste) would have told you that it's possible
> to take an IRQ domain as an argument.
>
> It's only the craziness in this patch which forced me to look into how
> Regmap handles IRQs and come up with a subsequent solution which fits
> your use-case.
>
> > In that case - I really don't see a reason to create a superfluous
> > structure to only hold the main regmap - we can very well get it from
> > the parent device in sub-drivers as I do now.
>
> The whole point of this solution is that you don't need to pass
> anything non-standard (i.e. not provided by the current APIs) to the
> child device.
>
I don't understand what you're saying here.
Bartosz
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-13 9:53 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <CAMRc=MeTMq_GWzQB6LBvts1csMftj=On5sB22jsJZzQzONxbNg@mail.gmail.com>
On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Lee Jones wrote:
> >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > >
> > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > >
> > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > >
> > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > >
> > > > > > > > > Need I go on? :)
> > > > > > > > >
> > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > set some alarm bells ringing?
> > > > > > > > >
> > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > >
> > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > >
> > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > >
> > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > are supposed to be passed (like everyone else does).
> > > > > >
> > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > >
> > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > >
> > > > Exampe:
> > > >
> > > > struct max77650_gpio_pdata {
> > > > int gpi_irq;
> > > > };
> > > >
> > > > In MFD driver:
> > > >
> > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > >
> > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > >
> > > > gpio_cell.platform_data = gpio_data;
> > > >
> > > > In GPIO driver:
> > > >
> > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > >
> > > > int irq = gpio_data->gpi_irq;
> > >
> > > Definitely not. What you're trying to do is a hack.
> > >
> > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > in the client to pull them out. Setting them via Regmap, then pulling
> > > them out again in the *same driver*, only to store them in platform
> > > data to be passed to a child device is bonkers.
> > >
> > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > handle them via the Regmap APIs, *not* both.
> >
> > Right, a plan has been formed.
> >
> > Hopefully this works and you can avoid all this dancing around.
> >
> > Firstly, you need to make a small change to:
> >
> > drivers/base/regmap/regmap-irq.c
> >
> > Add the following function:
> >
> > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
>
> We already do have such function and a lot of mfd drivers actually use it.
Even better.
> > As you can see, it will return the IRQ Domain for the chip.
> >
> > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > remove all the nastiness in max77650_setup_irqs() and have the Input
> > device use the standard (e.g. platform_get_irq()) APIs.
> >
> > How does that Sound?
>
> This does sound better! Why didn't you lead with that in the first place?
I'm not even going to dignify that stupid question with a response.
> It's a pity it's not documented, I had to look at the code to find out
> irq resources would get translated in mfd_add_devices() if a domain is
> present.
Where is it likely to be documented? MFD is pretty simple and seldom
needs explanation. A 3 second look at the API you're trying to use
(instead of blind copy & paste) would have told you that it's possible
to take an IRQ domain as an argument.
It's only the craziness in this patch which forced me to look into how
Regmap handles IRQs and come up with a subsequent solution which fits
your use-case.
> In that case - I really don't see a reason to create a superfluous
> structure to only hold the main regmap - we can very well get it from
> the parent device in sub-drivers as I do now.
The whole point of this solution is that you don't need to pass
anything non-standard (i.e. not provided by the current APIs) to the
child device.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-13 9:35 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <20190213092553.GE1863@dell>
śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Lee Jones wrote:
>
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > * The declaration of a superfluous struct
> > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > >
> > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > >
> > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > >
> > > > > > > > Need I go on? :)
> > > > > > > >
> > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > set some alarm bells ringing?
> > > > > > > >
> > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > >
> > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > >
> > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > >
> > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > are supposed to be passed (like everyone else does).
> > > > >
> > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > >
> > > > How do you plan on deriving the VIRQs to place into the struct?
> > >
> > > Exampe:
> > >
> > > struct max77650_gpio_pdata {
> > > int gpi_irq;
> > > };
> > >
> > > In MFD driver:
> > >
> > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > >
> > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > >
> > > gpio_cell.platform_data = gpio_data;
> > >
> > > In GPIO driver:
> > >
> > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > >
> > > int irq = gpio_data->gpi_irq;
> >
> > Definitely not. What you're trying to do is a hack.
> >
> > If you're using Regmap to handle your IRQs, then you should use Regmap
> > in the client to pull them out. Setting them via Regmap, then pulling
> > them out again in the *same driver*, only to store them in platform
> > data to be passed to a child device is bonkers.
> >
> > *Either* use the MFD provided platform-data helpers *or* pass and
> > handle them via the Regmap APIs, *not* both.
>
> Right, a plan has been formed.
>
> Hopefully this works and you can avoid all this dancing around.
>
> Firstly, you need to make a small change to:
>
> drivers/base/regmap/regmap-irq.c
>
> Add the following function:
>
> struct irq_domain *regmap_irq_get_domain(struct regmap *map)
>
We already do have such function and a lot of mfd drivers actually use it.
> As you can see, it will return the IRQ Domain for the chip.
>
> You can then pass this IRQ domain to mfd_add_devices() and it will do
> the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> remove all the nastiness in max77650_setup_irqs() and have the Input
> device use the standard (e.g. platform_get_irq()) APIs.
>
> How does that Sound?
>
This does sound better! Why didn't you lead with that in the first place?
It's a pity it's not documented, I had to look at the code to find out
irq resources would get translated in mfd_add_devices() if a domain is
present.
In that case - I really don't see a reason to create a superfluous
structure to only hold the main regmap - we can very well get it from
the parent device in sub-drivers as I do now.
Thanks,
Bartosz
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-13 9:25 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, broonie
In-Reply-To: <20190212132016.GA4781@dell>
On Tue, 12 Feb 2019, Lee Jones wrote:
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > * The declaration of a superfluous struct
> > > > > > > * 100 lines of additional/avoidable code
> > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > >
> > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > >
> > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > >
> > > > > > > Need I go on? :)
> > > > > > >
> > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > set some alarm bells ringing?
> > > > > > >
> > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > >
> > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > >
> > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > the regmap_irq_chip_data to child drivers?
> > > > >
> > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > are supposed to be passed (like everyone else does).
> > > >
> > > > I'm not sure what you mean by "like everyone else does" - different
> > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > containing virtual irq numbers passed to sub-drivers fine?
> > >
> > > How do you plan on deriving the VIRQs to place into the struct?
> >
> > Exampe:
> >
> > struct max77650_gpio_pdata {
> > int gpi_irq;
> > };
> >
> > In MFD driver:
> >
> > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> >
> > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> >
> > gpio_cell.platform_data = gpio_data;
> >
> > In GPIO driver:
> >
> > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> >
> > int irq = gpio_data->gpi_irq;
>
> Definitely not. What you're trying to do is a hack.
>
> If you're using Regmap to handle your IRQs, then you should use Regmap
> in the client to pull them out. Setting them via Regmap, then pulling
> them out again in the *same driver*, only to store them in platform
> data to be passed to a child device is bonkers.
>
> *Either* use the MFD provided platform-data helpers *or* pass and
> handle them via the Regmap APIs, *not* both.
Right, a plan has been formed.
Hopefully this works and you can avoid all this dancing around.
Firstly, you need to make a small change to:
drivers/base/regmap/regmap-irq.c
Add the following function:
struct irq_domain *regmap_irq_get_domain(struct regmap *map)
As you can see, it will return the IRQ Domain for the chip.
You can then pass this IRQ domain to mfd_add_devices() and it will do
the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
remove all the nastiness in max77650_setup_irqs() and have the Input
device use the standard (e.g. platform_get_irq()) APIs.
How does that Sound?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 12/13] input: max77650: add onkey support
From: Dmitry Torokhov @ 2019-02-13 7:30 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Mark Brown, Greg Kroah-Hartman, lkml, open list:GPIO SUBSYSTEM,
DTML, linux-input@vger.kernel.org, Linux LED Subsystem, Linux PM,
Bartosz Golaszewski
In-Reply-To: <20190212203408.GA1863@dell>
Hi Lee,
On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Add support for the push- and slide-button events for max77650.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > > drivers/input/misc/Kconfig | 9 ++
> > > drivers/input/misc/Makefile | 1 +
> > > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > > 3 files changed, 145 insertions(+)
> > > create mode 100644 drivers/input/misc/max77650-onkey.c
>
> [...]
>
> Moving things around a bit:
>
> > > +static int max77650_onkey_probe(struct platform_device *pdev)
> > > +{
>
> > > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > > + if (irq_f <= 0)
> > > + return -EINVAL;
> > > +
> > > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > > + if (irq_r <= 0)
> > > + return -EINVAL;
> >
> > Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> > passed it as resources of platform device. Then you'd simply call
> > platform_get_irq() here and did not have to reach into parent device for
> > "irq_dara".
>
> These device IRQs were defined and registered with the Regmap *set*
> (actually init()) APIs and thus should be pulled out using the
> appropriate reverse Regmap *get* APIs here in the device.
>
> Registering them with Regmap *and* pulling them back out again in the
> same (MFD in this case) driver, only to register them as platform data
> is certainly not how I see the API being designed/used.
>
> > > + struct regmap_irq_chip_data *irq_data;
> > > + struct device *dev, *parent;
>
> > > + dev = &pdev->dev;
> > > + parent = dev->parent;
> > > + i2c = to_i2c_client(parent);
> > > + irq_data = i2c_get_clientdata(i2c);
> > > +
> > > + map = dev_get_regmap(parent, NULL);
> > > + if (!map)
> > > + return -ENODEV;
>
> However, this hoop jumping is a bit crazy and for the most part
> superfluous. Instead, create a struct of attributes you wish to share
> with the child devices (containing both regmap (which you should call
> regmap and not map by the way) and irq_data) and pass it as either
> platform data (preferred) or as device data.
>
> If you choose the latter, you do not need to convert from 'device' to
> 'i2c' to do the look-up. Since this function (probe()) is provided
> with a platform_device, you can just use platform_get_drvdata() to
> achieve the same as above.
>
> If you go the preferred (platform data) route, then you should use
> dev_get_platdata() instead.
By doing what you are suggesting (introducing platform data) you
introducing strong dependency between MFD and input piece for no
different reason. With the current implementation the parent can be
reworked completely without involving onkey driver.
I find such clean separation desirable. We are trying to move away
form platform data where it makes sense.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 12/13] input: max77650: add onkey support
From: Lee Jones @ 2019-02-12 20:34 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Mark Brown, Greg Kroah-Hartman, linux-kernel, linux-gpio,
devicetree, linux-input, linux-leds, linux-pm,
Bartosz Golaszewski
In-Reply-To: <20190119090318.GB187380@dtor-ws>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Add support for the push- and slide-button events for max77650.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> > drivers/input/misc/Kconfig | 9 ++
> > drivers/input/misc/Makefile | 1 +
> > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > 3 files changed, 145 insertions(+)
> > create mode 100644 drivers/input/misc/max77650-onkey.c
[...]
Moving things around a bit:
> > +static int max77650_onkey_probe(struct platform_device *pdev)
> > +{
> > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > + if (irq_f <= 0)
> > + return -EINVAL;
> > +
> > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > + if (irq_r <= 0)
> > + return -EINVAL;
>
> Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> passed it as resources of platform device. Then you'd simply call
> platform_get_irq() here and did not have to reach into parent device for
> "irq_dara".
These device IRQs were defined and registered with the Regmap *set*
(actually init()) APIs and thus should be pulled out using the
appropriate reverse Regmap *get* APIs here in the device.
Registering them with Regmap *and* pulling them back out again in the
same (MFD in this case) driver, only to register them as platform data
is certainly not how I see the API being designed/used.
> > + struct regmap_irq_chip_data *irq_data;
> > + struct device *dev, *parent;
> > + dev = &pdev->dev;
> > + parent = dev->parent;
> > + i2c = to_i2c_client(parent);
> > + irq_data = i2c_get_clientdata(i2c);
> > +
> > + map = dev_get_regmap(parent, NULL);
> > + if (!map)
> > + return -ENODEV;
However, this hoop jumping is a bit crazy and for the most part
superfluous. Instead, create a struct of attributes you wish to share
with the child devices (containing both regmap (which you should call
regmap and not map by the way) and irq_data) and pass it as either
platform data (preferred) or as device data.
If you choose the latter, you do not need to convert from 'device' to
'i2c' to do the look-up. Since this function (probe()) is provided
with a platform_device, you can just use platform_get_drvdata() to
achieve the same as above.
If you go the preferred (platform data) route, then you should use
dev_get_platdata() instead.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v1] Input: st-keyscan - fix potential zalloc NULL dereference
From: gabriel.fernandez @ 2019-02-12 15:30 UTC (permalink / raw)
To: Dmitry Torokhov, Gabriel Fernandez
Cc: Nate Drude, linux-kernel, linux-stm32, giuseppe.condorelli,
linux-input, Dan Carpenter, linux-clk, linux-arm-kernel
From: Gabriel Fernandez <gabriel.fernandez@st.com>
This patch fixes the following static checker warning:
drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
error: potential zalloc NULL dereference: 'keypad_data->input_dev'
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
---
drivers/input/keyboard/st-keyscan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
index babcfb165e4f..3b85631fde91 100644
--- a/drivers/input/keyboard/st-keyscan.c
+++ b/drivers/input/keyboard/st-keyscan.c
@@ -153,6 +153,8 @@ static int keyscan_probe(struct platform_device *pdev)
input_dev->id.bustype = BUS_HOST;
+ keypad_data->input_dev = input_dev;
+
error = keypad_matrix_key_parse_dt(keypad_data);
if (error)
return error;
@@ -168,8 +170,6 @@ static int keyscan_probe(struct platform_device *pdev)
input_set_drvdata(input_dev, keypad_data);
- keypad_data->input_dev = input_dev;
-
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(keypad_data->base))
--
2.17.0
^ permalink raw reply related
* Re: [PATCH v3 07/11] power: supply: max77650: add support for battery charger
From: Pavel Machek @ 2019-02-12 13:54 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=Mf4Mx2ojbpxvF+Qej3oMHdB8hBwtZ85A5+pTj_ZhBij3A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2721 bytes --]
On Tue 2019-02-12 14:31:52, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 13:07 Pavel Machek <pavel@ucw.cz> napisał(a):
> >
> > Hi!
> >
> > > +#define MAX77650_CHARGER_ENABLED BIT(0)
> > > +#define MAX77650_CHARGER_DISABLED 0x00
> > > +#define MAX77650_CHARGER_CHG_EN_MASK BIT(0)
> > > +
> > > +#define MAX77650_CHARGER_CHG_DTLS_MASK GENMASK(7, 4)
> > > +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> > > + (((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> > > +
> > > +#define MAX77650_CHARGER_CHG_OFF 0x00
> > > +#define MAX77650_CHARGER_CHG_PREQ 0x01
> > > +#define MAX77650_CHARGER_CHG_ON_CURR 0x02
> > > +#define MAX77650_CHARGER_CHG_ON_JCURR 0x03
> > > +#define MAX77650_CHARGER_CHG_ON_VOLT 0x04
> > > +#define MAX77650_CHARGER_CHG_ON_JVOLT 0x05
> > > +#define MAX77650_CHARGER_CHG_ON_TOPOFF 0x06
> > > +#define MAX77650_CHARGER_CHG_ON_JTOPOFF 0x07
> > > +#define MAX77650_CHARGER_CHG_DONE 0x08
> > > +#define MAX77650_CHARGER_CHG_JDONE 0x09
> > > +#define MAX77650_CHARGER_CHG_SUSP_PF 0x0a
> > > +#define MAX77650_CHARGER_CHG_SUSP_FCF 0x0b
> > > +#define MAX77650_CHARGER_CHG_SUSP_BTF 0x0c
> >
> > These are really bad define names. We are in charger driver, so
> > MAX77650_CHARGER_ really should be shortened/ommited. OTOH
> > more space should be given to "CHG_SUSP_BTF" as it is impossible to decipher.
> >
>
> I disagree about the prefix bit. I prefer to use common prefixes for
> all symbols in a driver. I would rather add comments to every
> definition here.
So what about shortened? (And what about UVL/OVL below?)
And... what you prefer is not quite important. It is important that
other people can read your code. And what "PF" is is
important... unlike MAX77650_CHARGER_ which is long but unimportant.
> > > +#define MAX77650_CHARGER_CHGIN_DTLS_MASK GENMASK(3, 2)
> > > +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> > > + (((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> > > +
> > > +#define MAX77650_CHARGER_CHGIN_UVL 0x00
> > > +#define MAX77650_CHARGER_CHGIN_OVL 0x01
> > > +#define MAX77650_CHARGER_CHGIN_OKAY 0x11
> >
> > UVL -> UNDERVOLTAGE, OVL -> OVERVOLTAGE?
> >
> >
> > --
> > (english) http://www.livejournal.com/~pavelmachek
> > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 10/11] input: max77650: add onkey support
From: Pavel Machek @ 2019-02-12 13:47 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=Md57e93vCV+uJ4WSma2AefdN89iWXCyKiY7yLsWFGUihg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]
On Tue 2019-02-12 14:29:49, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 13:17 Pavel Machek <pavel@ucw.cz> napisał(a):
> >
> > Hi!
> >
> > > + error = device_property_read_string(dev,
> > > + "maxim,onkey-mode", &mode_prop);
> > > + if (error)
> > > + mode_prop = "push";
> > > +
> > > + if (strcmp(mode_prop, "push") == 0)
> > > + mode = MAX77650_ONKEY_MODE_PUSH;
> > > + else if (strcmp(mode_prop, "slide") == 0)
> > > + mode = MAX77650_ONKEY_MODE_SLIDE;
> > > + else
> > > + return -EINVAL;
> > ...
> > > + onkey->input->name = "max77650_onkey";
> > > + onkey->input->phys = "max77650_onkey/input0";
> > > + onkey->input->id.bustype = BUS_I2C;
> > > + input_set_capability(onkey->input, EV_KEY, onkey->code);
> >
> > Is EV_KEY correct for mode==slide?
> >
>
> Others are not really documented. Does EV_SW stand for switch event?
> In that case it would probably be better.
I believe so.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Pavel Machek @ 2019-02-12 13:47 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=MdCXfPh6g4u-v4GkQJxNDA+v1dqRChcEHdauczrVywkDg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]
On Tue 2019-02-12 14:28:10, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 12:54 Pavel Machek <pavel@ucw.cz> napisał(a):
> >
> > Hi!
> >
> > > diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > > new file mode 100644
> > > index 000000000000..f3e00d41e299
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > > @@ -0,0 +1,27 @@
> > > +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> > > +
> > > +This module is part of the MAX77650 MFD device. For more details
> > > +see Documentation/devicetree/bindings/mfd/max77650.txt.
> > > +
> > > +The charger is represented as a sub-node of the PMIC node on the device tree.
> > > +
> > > +Required properties:
> > > +--------------------
> > > +- compatible: Must be "maxim,max77650-charger"
> > > +
> > > +Optional properties:
> > > +--------------------
> > > +- maxim,vchgin-min: Minimum CHGIN regulation voltage (in microvolts). Must be
> > > + one of: 4000000, 4100000, 4200000, 4300000, 4400000,
> > > + 4500000, 4600000, 4700000.
> > > +- maxim,ichgin-lim: CHGIN input current limit (in microamps). Must be one of:
> > > + 95000, 190000, 285000, 380000, 475000.
> >
> > We already have richtek,min-input-voltage-regulation and
> > ti,minimum-sys-voltage and ti,in-dpm-voltage.
> >
> > This is not too consistent with with the rest.. and perhaps common
> > name should be used for those?
> >
> > See also ti,current-limit. That seems to be direct match.
> >
>
> Well I can't really use a TI-specific property name on a maxim part, can I?
Well, I'm saying that you should introduce common names for common
stuff. "current-limit" sounds like reasonable name.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 07/11] power: supply: max77650: add support for battery charger
From: Bartosz Golaszewski @ 2019-02-12 13:31 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212120742.GA25613@amd>
wt., 12 lut 2019 o 13:07 Pavel Machek <pavel@ucw.cz> napisał(a):
>
> Hi!
>
> > +#define MAX77650_CHARGER_ENABLED BIT(0)
> > +#define MAX77650_CHARGER_DISABLED 0x00
> > +#define MAX77650_CHARGER_CHG_EN_MASK BIT(0)
> > +
> > +#define MAX77650_CHARGER_CHG_DTLS_MASK GENMASK(7, 4)
> > +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> > + (((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> > +
> > +#define MAX77650_CHARGER_CHG_OFF 0x00
> > +#define MAX77650_CHARGER_CHG_PREQ 0x01
> > +#define MAX77650_CHARGER_CHG_ON_CURR 0x02
> > +#define MAX77650_CHARGER_CHG_ON_JCURR 0x03
> > +#define MAX77650_CHARGER_CHG_ON_VOLT 0x04
> > +#define MAX77650_CHARGER_CHG_ON_JVOLT 0x05
> > +#define MAX77650_CHARGER_CHG_ON_TOPOFF 0x06
> > +#define MAX77650_CHARGER_CHG_ON_JTOPOFF 0x07
> > +#define MAX77650_CHARGER_CHG_DONE 0x08
> > +#define MAX77650_CHARGER_CHG_JDONE 0x09
> > +#define MAX77650_CHARGER_CHG_SUSP_PF 0x0a
> > +#define MAX77650_CHARGER_CHG_SUSP_FCF 0x0b
> > +#define MAX77650_CHARGER_CHG_SUSP_BTF 0x0c
>
> These are really bad define names. We are in charger driver, so
> MAX77650_CHARGER_ really should be shortened/ommited. OTOH
> more space should be given to "CHG_SUSP_BTF" as it is impossible to decipher.
>
I disagree about the prefix bit. I prefer to use common prefixes for
all symbols in a driver. I would rather add comments to every
definition here.
Bart
> > +#define MAX77650_CHARGER_CHGIN_DTLS_MASK GENMASK(3, 2)
> > +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> > + (((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> > +
> > +#define MAX77650_CHARGER_CHGIN_UVL 0x00
> > +#define MAX77650_CHARGER_CHGIN_OVL 0x01
> > +#define MAX77650_CHARGER_CHGIN_OKAY 0x11
>
> UVL -> UNDERVOLTAGE, OVL -> OVERVOLTAGE?
>
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v3 10/11] input: max77650: add onkey support
From: Bartosz Golaszewski @ 2019-02-12 13:29 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212121745.GE25613@amd>
wt., 12 lut 2019 o 13:17 Pavel Machek <pavel@ucw.cz> napisał(a):
>
> Hi!
>
> > + error = device_property_read_string(dev,
> > + "maxim,onkey-mode", &mode_prop);
> > + if (error)
> > + mode_prop = "push";
> > +
> > + if (strcmp(mode_prop, "push") == 0)
> > + mode = MAX77650_ONKEY_MODE_PUSH;
> > + else if (strcmp(mode_prop, "slide") == 0)
> > + mode = MAX77650_ONKEY_MODE_SLIDE;
> > + else
> > + return -EINVAL;
> ...
> > + onkey->input->name = "max77650_onkey";
> > + onkey->input->phys = "max77650_onkey/input0";
> > + onkey->input->id.bustype = BUS_I2C;
> > + input_set_capability(onkey->input, EV_KEY, onkey->code);
>
> Is EV_KEY correct for mode==slide?
>
Others are not really documented. Does EV_SW stand for switch event?
In that case it would probably be better.
Bart
^ permalink raw reply
* Re: [PATCH v3 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-02-12 13:28 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212115420.GA24296@amd>
wt., 12 lut 2019 o 12:54 Pavel Machek <pavel@ucw.cz> napisał(a):
>
> Hi!
>
> > diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > new file mode 100644
> > index 000000000000..f3e00d41e299
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > @@ -0,0 +1,27 @@
> > +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> > +
> > +This module is part of the MAX77650 MFD device. For more details
> > +see Documentation/devicetree/bindings/mfd/max77650.txt.
> > +
> > +The charger is represented as a sub-node of the PMIC node on the device tree.
> > +
> > +Required properties:
> > +--------------------
> > +- compatible: Must be "maxim,max77650-charger"
> > +
> > +Optional properties:
> > +--------------------
> > +- maxim,vchgin-min: Minimum CHGIN regulation voltage (in microvolts). Must be
> > + one of: 4000000, 4100000, 4200000, 4300000, 4400000,
> > + 4500000, 4600000, 4700000.
> > +- maxim,ichgin-lim: CHGIN input current limit (in microamps). Must be one of:
> > + 95000, 190000, 285000, 380000, 475000.
>
> We already have richtek,min-input-voltage-regulation and
> ti,minimum-sys-voltage and ti,in-dpm-voltage.
>
> This is not too consistent with with the rest.. and perhaps common
> name should be used for those?
>
> See also ti,current-limit. That seems to be direct match.
>
Well I can't really use a TI-specific property name on a maxim part, can I?
Bart
PS This version was superseded by v4.
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-12 13:20 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list
In-Reply-To: <CAMpxmJWXOb1HW4QiEn8gMp3Nh05ahE0CMLXkac+hEKRjtGxTHQ@mail.gmail.com>
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > * The declaration of a superfluous struct
> > > > > > * 100 lines of additional/avoidable code
> > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > >
> > > > > While the above may be right, this one is negligible and you know it. :)
> > > >
> > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > >
> > > > > > Need I go on? :)
> > > > > >
> > > > > > Surely the fact that you are using both sides of an API
> > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > set some alarm bells ringing?
> > > > > >
> > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > >
> > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > >
> > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > the regmap_irq_chip_data to child drivers?
> > > >
> > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > are supposed to be passed (like everyone else does).
> > >
> > > I'm not sure what you mean by "like everyone else does" - different
> > > mfd drivers seem to be doing different things. Is a simple struct
> > > containing virtual irq numbers passed to sub-drivers fine?
> >
> > How do you plan on deriving the VIRQs to place into the struct?
>
> Exampe:
>
> struct max77650_gpio_pdata {
> int gpi_irq;
> };
>
> In MFD driver:
>
> struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
>
> gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
>
> gpio_cell.platform_data = gpio_data;
>
> In GPIO driver:
>
> struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
>
> int irq = gpio_data->gpi_irq;
Definitely not. What you're trying to do is a hack.
If you're using Regmap to handle your IRQs, then you should use Regmap
in the client to pull them out. Setting them via Regmap, then pulling
them out again in the *same driver*, only to store them in platform
data to be passed to a child device is bonkers.
*Either* use the MFD provided platform-data helpers *or* pass and
handle them via the Regmap APIs, *not* both.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox