* Re: [PATCH v3 06/11] mfd: max77650: new core mfd driver
From: Pavel Machek @ 2019-02-12 12:01 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, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190201094736.32057-7-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 1660 bytes --]
On Fri 2019-02-01 10:47:31, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the core mfd driver for max77650 PMIC. We define five sub-devices
> for which the drivers will be added in subsequent patches.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/mfd/Kconfig | 11 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77650.h | 59 ++++++
> 4 files changed, 413 insertions(+)
> create mode 100644 drivers/mfd/max77650.c
> create mode 100644 include/linux/mfd/max77650.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f461460a2aeb..828fd193b4ee 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -734,6 +734,17 @@ config MFD_MAX77620
> provides common support for accessing the device; additional drivers
> must be enabled in order to use the functionality of the device.
>
> +config MFD_MAX77650
> + tristate "Maxim MAX77650/77651 PMIC Support"
> + depends on I2C
> + depends on OF || COMPILE_TEST
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + Say yes here to add support for Maxim Semiconductor MAX77650 and
"Say Y" would be more common and consistent with your ther patch.
> + MAX77651 Power Management ICs. This is the core multifunction
> + driver for interacting with the device.
Mention modular build and module name here?
--
(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 11: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, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190201094736.32057-3-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]
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.
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
* [PATCH v4 4/4] Input: goodix - Add GT5663 CTP support
From: Jagan Teki @ 2019-02-12 11:24 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, Jagan Teki
In-Reply-To: <20190212112439.2025-1-jagan@amarulasolutions.com>
GT5663 is capacitive touch controller with customized smart
wakeup gestures.
Add support for it by adding compatible and supported chip data.
The chip data on GT5663 is similar to GT1151, like
- config data register has 0x8050 address
- config data register max len is 240
- config data checksum has 16-bit
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index e92b90be1ac2..9a676dec79c6 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -218,6 +218,7 @@ static const struct goodix_chip_data *goodix_get_chip_data(u16 id)
{
switch (id) {
case 1151:
+ case 5663:
return >1x_chip_data;
case 911:
@@ -976,6 +977,7 @@ MODULE_DEVICE_TABLE(acpi, goodix_acpi_match);
#ifdef CONFIG_OF
static const struct of_device_id goodix_of_match[] = {
{ .compatible = "goodix,gt1151" },
+ { .compatible = "goodix,gt5663" },
{ .compatible = "goodix,gt911" },
{ .compatible = "goodix,gt9110" },
{ .compatible = "goodix,gt912" },
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v4 3/4] dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
From: Jagan Teki @ 2019-02-12 11:24 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, Jagan Teki
In-Reply-To: <20190212112439.2025-1-jagan@amarulasolutions.com>
GT5663 is capacitive touch controller with customized smart
wakeup gestures, it support chipdata which is similar to
existing GT1151 and require AVDD28 supply for some boards.
Document the compatible for the same.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index c4622c983e08..59c89276e6bb 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -3,6 +3,7 @@ Device tree bindings for Goodix GT9xx series touchscreen controller
Required properties:
- compatible : Should be "goodix,gt1151"
+ or "goodix,gt5663"
or "goodix,gt911"
or "goodix,gt9110"
or "goodix,gt912"
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v4 2/4] Input: goodix - Add AVDD28-supply regulator support
From: Jagan Teki @ 2019-02-12 11:24 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, Jagan Teki
In-Reply-To: <20190212112439.2025-1-jagan@amarulasolutions.com>
Goodix CTP controllers have AVDD28 pin connected to voltage
regulator which may not be turned on by default, like for GT5663.
Add support for such ctp used boards by adding voltage regulator
handling code to goodix ctp driver.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
drivers/input/touchscreen/goodix.c | 44 ++++++++++++++++++++++++++----
1 file changed, 39 insertions(+), 5 deletions(-)
diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index f2d9c2c41885..e92b90be1ac2 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -27,6 +27,7 @@
#include <linux/delay.h>
#include <linux/irq.h>
#include <linux/interrupt.h>
+#include <linux/regulator/consumer.h>
#include <linux/slab.h>
#include <linux/acpi.h>
#include <linux/of.h>
@@ -47,6 +48,7 @@ struct goodix_ts_data {
struct touchscreen_properties prop;
unsigned int max_touch_num;
unsigned int int_trigger_type;
+ struct regulator *avdd28;
struct gpio_desc *gpiod_int;
struct gpio_desc *gpiod_rst;
u16 id;
@@ -761,6 +763,13 @@ static void goodix_config_cb(const struct firmware *cfg, void *ctx)
complete_all(&ts->firmware_loading_complete);
}
+static void goodix_disable_regulator(void *arg)
+{
+ struct goodix_ts_data *ts = arg;
+
+ regulator_disable(ts->avdd28);
+}
+
static int goodix_ts_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
@@ -786,25 +795,46 @@ static int goodix_ts_probe(struct i2c_client *client,
if (error)
return error;
+ ts->avdd28 = devm_regulator_get(&client->dev, "AVDD28");
+ if (IS_ERR(ts->avdd28)) {
+ error = PTR_ERR(ts->avdd28);
+ if (error != -EPROBE_DEFER)
+ dev_err(&client->dev,
+ "Failed to get AVDD28 regulator: %d\n", error);
+ return error;
+ }
+
+ /* power the controller */
+ error = regulator_enable(ts->avdd28);
+ if (error) {
+ dev_err(&client->dev, "Controller fail to enable AVDD28\n");
+ return error;
+ }
+
+ error = devm_add_action_or_reset(&client->dev,
+ goodix_disable_regulator, ts);
+ if (error)
+ return error;
+
if (ts->gpiod_int && ts->gpiod_rst) {
/* reset the controller */
error = goodix_reset(ts);
if (error) {
dev_err(&client->dev, "Controller reset failed.\n");
- return error;
+ goto error;
}
}
error = goodix_i2c_test(client);
if (error) {
dev_err(&client->dev, "I2C communication failure: %d\n", error);
- return error;
+ goto error;
}
error = goodix_read_version(ts);
if (error) {
dev_err(&client->dev, "Read version failed.\n");
- return error;
+ goto error;
}
ts->chip = goodix_get_chip_data(ts->id);
@@ -823,17 +853,21 @@ static int goodix_ts_probe(struct i2c_client *client,
dev_err(&client->dev,
"Failed to invoke firmware loader: %d\n",
error);
- return error;
+ goto error;
}
return 0;
} else {
error = goodix_configure_dev(ts);
if (error)
- return error;
+ goto error;
}
return 0;
+
+error:
+ regulator_disable(ts->avdd28);
+ return error;
}
static int goodix_ts_remove(struct i2c_client *client)
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v4 1/4] dt-bindings: input: touchscreen: goodix: Document AVDD28-supply property
From: Jagan Teki @ 2019-02-12 11:24 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, Jagan Teki
In-Reply-To: <20190212112439.2025-1-jagan@amarulasolutions.com>
Most of the Goodix CTP controllers are supply with AVDD28 pin.
which need to supply for controllers like GT5663 on some boards
to trigger the power.
So, document the supply property so-that the require boards
that used on GT5663 can enable it via device tree.
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Documentation/devicetree/bindings/input/touchscreen/goodix.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
index f7e95c52f3c7..c4622c983e08 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
@@ -23,6 +23,7 @@ Optional properties:
- touchscreen-inverted-y : Y axis is inverted (boolean)
- touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
(swapping is done after inverting the axis)
+ - AVDD28-supply : Analog power supply regulator on AVDD28 pin
Example:
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply related
* [PATCH v4 0/4] input: touchscreen: Add goodix GT5553 CTP support
From: Jagan Teki @ 2019-02-12 11:24 UTC (permalink / raw)
To: Dmitry Torokhov, Bastien Nocera, Rob Herring
Cc: Henrik Rydberg, linux-input, linux-kernel, devicetree,
Mark Rutland, Jagan Teki
This patchset support goodix GT5553 CTP.
Changes for v4:
- devm_add_action_or_reset for disabling regulator
Changes for v3:
- add cover-letter
- s/ADVV28/AVDD28 on commit head
- fix few typo
Changes for v2:
- Rename vcc-supply with AVDD28-supply
- disable regulator in remove
- fix to setup regulator in probe code
- add chipdata
- drop example node in dt-bindings
Jagan Teki (4):
dt-bindings: input: touchscreen: goodix: Document AVDD28-supply
property
Input: goodix - Add AVDD28-supply regulator support
dt-bindings: input: touchscreen: goodix: Add GT5663 compatible
Input: goodix - Add GT5663 CTP support
.../bindings/input/touchscreen/goodix.txt | 2 +
drivers/input/touchscreen/goodix.c | 46 +++++++++++++++++--
2 files changed, 43 insertions(+), 5 deletions(-)
--
2.18.0.321.gffc6fa0e3
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-12 11:14 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: 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, Bartosz Golaszewski
In-Reply-To: <CAMRc=MfYeVbvY6O03xq4eLN9TVpAKWCAXPk0eP5LSBCU7VU9fA@mail.gmail.com>
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?
--
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-12 10:24 UTC (permalink / raw)
To: Lee Jones
Cc: 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, Bartosz Golaszewski
In-Reply-To: <20190212101835.GB20638@dell>
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?
Bart
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-12 10:18 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: 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, Bartosz Golaszewski
In-Reply-To: <CAMRc=MdyT7nMJ7dUTf6M3PvRdynaCurb_TftMyA4eCwzpR9fVA@mail.gmail.com>
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > > > > for which the drivers will be added in subsequent patches.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > > ---
> > > > > drivers/mfd/Kconfig | 11 ++
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++
> > > > > include/linux/mfd/max77650.h | 59 ++++++
> > > > > 4 files changed, 413 insertions(+)
> > > > > create mode 100644 drivers/mfd/max77650.c
> > > > > create mode 100644 include/linux/mfd/max77650.h
> >
> > [...]
> >
> > > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > > > > + {
> > > > > + .cell_num = MAX77650_CELL_CHARGER,
> > > > > + .irqs = max77650_charger_irqs,
> > > > > + .irq_names = max77650_charger_irq_names,
> > > > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs),
> > > > > + },
> > > > > + {
> > > > > + .cell_num = MAX77650_CELL_GPIO,
> > > > > + .irqs = max77650_gpio_irqs,
> > > > > + .irq_names = max77650_gpio_irq_names,
> > > > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs),
> > > > > + },
> > > > > + {
> > > > > + .cell_num = MAX77650_CELL_ONKEY,
> > > > > + .irqs = max77650_onkey_irqs,
> > > > > + .irq_names = max77650_onkey_irq_names,
> > > > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs),
> > > > > + },
> > > > > +};
> > > >
> > > > This is all a bit convoluted and nasty TBH.
> > > >
> > > > > +static const struct mfd_cell max77650_cells[] = {
> > > > > + [MAX77650_CELL_REGULATOR] = {
> > > > > + .name = "max77650-regulator",
> > > > > + .of_compatible = "maxim,max77650-regulator",
> > > > > + },
> > > > > + [MAX77650_CELL_CHARGER] = {
> > > > > + .name = "max77650-charger",
> > > > > + .of_compatible = "maxim,max77650-charger",
> > > > > + },
> > > > > + [MAX77650_CELL_GPIO] = {
> > > > > + .name = "max77650-gpio",
> > > > > + .of_compatible = "maxim,max77650-gpio",
> > > > > + },
> > > > > + [MAX77650_CELL_LED] = {
> > > > > + .name = "max77650-led",
> > > > > + .of_compatible = "maxim,max77650-led",
> > > > > + },
> > > > > + [MAX77650_CELL_ONKEY] = {
> > > > > + .name = "max77650-onkey",
> > > > > + .of_compatible = "maxim,max77650-onkey",
> > > > > + },
> > > > > +};
> > > >
> > > > Why are you numbering the cells? There is no need to do this.
> > > >
> > >
> > > Just for better readability. It makes sense to me coupled with
> > > MAX77650_NUM_CELLS.
> >
> > You have it the wrong way around. You define the cell data, then
> > provide the number of them using ARRAY_SIZE(). The enum containing
> > MAX77650_NUM_CELLS should not exist.
> >
> > > > > +static const struct regmap_irq max77650_irqs[] = {
> > > > > + [MAX77650_INT_GPI] = {
> > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > > + .mask = MAX77650_INT_GPI_MSK,
> > > > > + .type = {
> > > > > + .type_falling_val = MAX77650_INT_GPI_F_MSK,
> > > > > + .type_rising_val = MAX77650_INT_GPI_R_MSK,
> > > > > + .types_supported = IRQ_TYPE_EDGE_BOTH,
> > > > > + },
> > > > > + },
> > > > > + [MAX77650_INT_nEN_F] = {
> > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > > + .mask = MAX77650_INT_nEN_F_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_nEN_R] = {
> > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > > + .mask = MAX77650_INT_nEN_R_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_TJAL1_R] = {
> > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > > + .mask = MAX77650_INT_TJAL1_R_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_TJAL2_R] = {
> > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > > + .mask = MAX77650_INT_TJAL2_R_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_DOD_R] = {
> > > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > > + .mask = MAX77650_INT_DOD_R_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_THM] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_THM_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_CHG] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_CHG_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_CHGIN] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_CHGIN_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_TJ_REG] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_TJ_REG_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_CHGIN_CTRL] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_SYS_CTRL] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_SYS_CTRL_MSK,
> > > > > + },
> > > > > + [MAX77650_INT_SYS_CNFG] = {
> > > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > > + .mask = MAX77650_INT_SYS_CNFG_MSK,
> > > > > + },
> > > > > +};
> > > >
> > > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> > > > you can use REGMAP_IRQ_REG() like everyone else does.
> > > >
> > >
> > > I could even use it now - except for the first interrupt. I decided to
> > > not use it everywhere as it looks much better that way than having
> > > REGMAP_IRQ_REG() for all definitions and then the first one sticking
> > > out like that. It just looks better.
> >
> > The way it's done at the moment looks terrible.
> >
> > Please use the MACROs to simplify as much of the code as possible.
> >
> > > > > +static const struct regmap_irq_chip max77650_irq_chip = {
> > > > > + .name = "max77650-irq",
> > > > > + .irqs = max77650_irqs,
> > > > > + .num_irqs = ARRAY_SIZE(max77650_irqs),
> > > > > + .num_regs = 2,
> > > > > + .status_base = MAX77650_REG_INT_GLBL,
> > > > > + .mask_base = MAX77650_REG_INTM_GLBL,
> > > > > + .type_in_mask = true,
> > > > > + .type_invert = true,
> > > > > + .init_ack_masked = true,
> > > > > + .clear_on_unmask = true,
> > > > > +};
> > > > > +
> > > > > +static const struct regmap_config max77650_regmap_config = {
> > > > > + .name = "max77650",
> > > > > + .reg_bits = 8,
> > > > > + .val_bits = 8,
> > > > > +};
> > > > > +
> > > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > > > > +{
> > > > > + const struct max77650_irq_mapping *mapping;
> > > > > + struct regmap_irq_chip_data *irq_data;
> > > > > + struct i2c_client *i2c;
> > > > > + struct mfd_cell *cell;
> > > > > + struct resource *res;
> > > > > + struct regmap *map;
> > > > > + int i, j, irq, rv;
> > > > > +
> > > > > + i2c = to_i2c_client(dev);
> > > > > +
> > > > > + map = dev_get_regmap(dev, NULL);
> > > > > + if (!map)
> > > > > + return -ENODEV;
> > > > > +
> > > > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > > > > + IRQF_ONESHOT | IRQF_SHARED, -1,
> > > >
> > > > What is -1? Are you sure this isn't defined somewhere?
> > > >
> > >
> > > I don't see any define for negative irq_base argument. I can add that
> > > in a separate series and convert the users, but for now I'd stick with
> > > -1.
> >
> > IMO it should be defined. You can define it locally for now.
> >
> > > > > + &max77650_irq_chip, &irq_data);
> > > > > + if (rv)
> > > > > + return rv;
> > > > > +
> > > > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > > > > + mapping = &max77650_irq_mapping_table[i];
> > > > > + cell = &cells[mapping->cell_num];
> > > > > +
> > > > > + res = devm_kcalloc(dev, sizeof(*res),
> > > > > + mapping->num_irqs, GFP_KERNEL);
> > > > > + if (!res)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + cell->resources = res;
> > > > > + cell->num_resources = mapping->num_irqs;
> > > > > +
> > > > > + for (j = 0; j < mapping->num_irqs; j++) {
> > > > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > > > > + if (irq < 0)
> > > > > + return irq;
> > > > > +
> > > > > + res[j].start = res[j].end = irq;
> > > > > + res[j].flags = IORESOURCE_IRQ;
> > > > > + res[j].name = mapping->irq_names[j];
> > > > > + }
> > > > > + }
> > > >
> > > > This is the first time I've seen it done like this (and I hate it).
> > > >
> > > > Why are you storing the virqs in resources?
> > > >
> > > > I think this is highly irregular.
> > > >
> > >
> > > I initially just passed the regmap_irq_chip_data over i2c clientdata
> > > and sub-drivers would look up virq numbers from it but was advised by
> > > Dmitry Torokhov to use resources instead. After implementing it this
> > > way I too think it's more elegant in sub-drivers who can simply do
> > > platform_get_irq_byname(). Do you have a different idea?
> >
> > > What exactly don't you like about this?
> >
> > * 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).
> @Dmitry: what do you think?
--
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-12 10:12 UTC (permalink / raw)
To: Lee Jones
Cc: 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, Bartosz Golaszewski
In-Reply-To: <20190212095457.GA20638@dell>
wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > > > for which the drivers will be added in subsequent patches.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > ---
> > > > drivers/mfd/Kconfig | 11 ++
> > > > drivers/mfd/Makefile | 1 +
> > > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++
> > > > include/linux/mfd/max77650.h | 59 ++++++
> > > > 4 files changed, 413 insertions(+)
> > > > create mode 100644 drivers/mfd/max77650.c
> > > > create mode 100644 include/linux/mfd/max77650.h
>
> [...]
>
> > > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > > > + {
> > > > + .cell_num = MAX77650_CELL_CHARGER,
> > > > + .irqs = max77650_charger_irqs,
> > > > + .irq_names = max77650_charger_irq_names,
> > > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs),
> > > > + },
> > > > + {
> > > > + .cell_num = MAX77650_CELL_GPIO,
> > > > + .irqs = max77650_gpio_irqs,
> > > > + .irq_names = max77650_gpio_irq_names,
> > > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs),
> > > > + },
> > > > + {
> > > > + .cell_num = MAX77650_CELL_ONKEY,
> > > > + .irqs = max77650_onkey_irqs,
> > > > + .irq_names = max77650_onkey_irq_names,
> > > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs),
> > > > + },
> > > > +};
> > >
> > > This is all a bit convoluted and nasty TBH.
> > >
> > > > +static const struct mfd_cell max77650_cells[] = {
> > > > + [MAX77650_CELL_REGULATOR] = {
> > > > + .name = "max77650-regulator",
> > > > + .of_compatible = "maxim,max77650-regulator",
> > > > + },
> > > > + [MAX77650_CELL_CHARGER] = {
> > > > + .name = "max77650-charger",
> > > > + .of_compatible = "maxim,max77650-charger",
> > > > + },
> > > > + [MAX77650_CELL_GPIO] = {
> > > > + .name = "max77650-gpio",
> > > > + .of_compatible = "maxim,max77650-gpio",
> > > > + },
> > > > + [MAX77650_CELL_LED] = {
> > > > + .name = "max77650-led",
> > > > + .of_compatible = "maxim,max77650-led",
> > > > + },
> > > > + [MAX77650_CELL_ONKEY] = {
> > > > + .name = "max77650-onkey",
> > > > + .of_compatible = "maxim,max77650-onkey",
> > > > + },
> > > > +};
> > >
> > > Why are you numbering the cells? There is no need to do this.
> > >
> >
> > Just for better readability. It makes sense to me coupled with
> > MAX77650_NUM_CELLS.
>
> You have it the wrong way around. You define the cell data, then
> provide the number of them using ARRAY_SIZE(). The enum containing
> MAX77650_NUM_CELLS should not exist.
>
> > > > +static const struct regmap_irq max77650_irqs[] = {
> > > > + [MAX77650_INT_GPI] = {
> > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > + .mask = MAX77650_INT_GPI_MSK,
> > > > + .type = {
> > > > + .type_falling_val = MAX77650_INT_GPI_F_MSK,
> > > > + .type_rising_val = MAX77650_INT_GPI_R_MSK,
> > > > + .types_supported = IRQ_TYPE_EDGE_BOTH,
> > > > + },
> > > > + },
> > > > + [MAX77650_INT_nEN_F] = {
> > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > + .mask = MAX77650_INT_nEN_F_MSK,
> > > > + },
> > > > + [MAX77650_INT_nEN_R] = {
> > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > + .mask = MAX77650_INT_nEN_R_MSK,
> > > > + },
> > > > + [MAX77650_INT_TJAL1_R] = {
> > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > + .mask = MAX77650_INT_TJAL1_R_MSK,
> > > > + },
> > > > + [MAX77650_INT_TJAL2_R] = {
> > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > + .mask = MAX77650_INT_TJAL2_R_MSK,
> > > > + },
> > > > + [MAX77650_INT_DOD_R] = {
> > > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > > + .mask = MAX77650_INT_DOD_R_MSK,
> > > > + },
> > > > + [MAX77650_INT_THM] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_THM_MSK,
> > > > + },
> > > > + [MAX77650_INT_CHG] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_CHG_MSK,
> > > > + },
> > > > + [MAX77650_INT_CHGIN] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_CHGIN_MSK,
> > > > + },
> > > > + [MAX77650_INT_TJ_REG] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_TJ_REG_MSK,
> > > > + },
> > > > + [MAX77650_INT_CHGIN_CTRL] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK,
> > > > + },
> > > > + [MAX77650_INT_SYS_CTRL] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_SYS_CTRL_MSK,
> > > > + },
> > > > + [MAX77650_INT_SYS_CNFG] = {
> > > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > > + .mask = MAX77650_INT_SYS_CNFG_MSK,
> > > > + },
> > > > +};
> > >
> > > If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> > > you can use REGMAP_IRQ_REG() like everyone else does.
> > >
> >
> > I could even use it now - except for the first interrupt. I decided to
> > not use it everywhere as it looks much better that way than having
> > REGMAP_IRQ_REG() for all definitions and then the first one sticking
> > out like that. It just looks better.
>
> The way it's done at the moment looks terrible.
>
> Please use the MACROs to simplify as much of the code as possible.
>
> > > > +static const struct regmap_irq_chip max77650_irq_chip = {
> > > > + .name = "max77650-irq",
> > > > + .irqs = max77650_irqs,
> > > > + .num_irqs = ARRAY_SIZE(max77650_irqs),
> > > > + .num_regs = 2,
> > > > + .status_base = MAX77650_REG_INT_GLBL,
> > > > + .mask_base = MAX77650_REG_INTM_GLBL,
> > > > + .type_in_mask = true,
> > > > + .type_invert = true,
> > > > + .init_ack_masked = true,
> > > > + .clear_on_unmask = true,
> > > > +};
> > > > +
> > > > +static const struct regmap_config max77650_regmap_config = {
> > > > + .name = "max77650",
> > > > + .reg_bits = 8,
> > > > + .val_bits = 8,
> > > > +};
> > > > +
> > > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > > > +{
> > > > + const struct max77650_irq_mapping *mapping;
> > > > + struct regmap_irq_chip_data *irq_data;
> > > > + struct i2c_client *i2c;
> > > > + struct mfd_cell *cell;
> > > > + struct resource *res;
> > > > + struct regmap *map;
> > > > + int i, j, irq, rv;
> > > > +
> > > > + i2c = to_i2c_client(dev);
> > > > +
> > > > + map = dev_get_regmap(dev, NULL);
> > > > + if (!map)
> > > > + return -ENODEV;
> > > > +
> > > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > > > + IRQF_ONESHOT | IRQF_SHARED, -1,
> > >
> > > What is -1? Are you sure this isn't defined somewhere?
> > >
> >
> > I don't see any define for negative irq_base argument. I can add that
> > in a separate series and convert the users, but for now I'd stick with
> > -1.
>
> IMO it should be defined. You can define it locally for now.
>
> > > > + &max77650_irq_chip, &irq_data);
> > > > + if (rv)
> > > > + return rv;
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > > > + mapping = &max77650_irq_mapping_table[i];
> > > > + cell = &cells[mapping->cell_num];
> > > > +
> > > > + res = devm_kcalloc(dev, sizeof(*res),
> > > > + mapping->num_irqs, GFP_KERNEL);
> > > > + if (!res)
> > > > + return -ENOMEM;
> > > > +
> > > > + cell->resources = res;
> > > > + cell->num_resources = mapping->num_irqs;
> > > > +
> > > > + for (j = 0; j < mapping->num_irqs; j++) {
> > > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > > > + if (irq < 0)
> > > > + return irq;
> > > > +
> > > > + res[j].start = res[j].end = irq;
> > > > + res[j].flags = IORESOURCE_IRQ;
> > > > + res[j].name = mapping->irq_names[j];
> > > > + }
> > > > + }
> > >
> > > This is the first time I've seen it done like this (and I hate it).
> > >
> > > Why are you storing the virqs in resources?
> > >
> > > I think this is highly irregular.
> > >
> >
> > I initially just passed the regmap_irq_chip_data over i2c clientdata
> > and sub-drivers would look up virq numbers from it but was advised by
> > Dmitry Torokhov to use resources instead. After implementing it this
> > way I too think it's more elegant in sub-drivers who can simply do
> > platform_get_irq_byname(). Do you have a different idea?
>
> > What exactly don't you like about this?
>
> * 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. :)
>
> 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?
@Dmitry: what do you think?
Bart
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-12 9:54 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: 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, Bartosz Golaszewski
In-Reply-To: <CAMRc=MderQf20_8aG4-otBkCv60FSNSSqV3NVALPkFL-MqmJbg@mail.gmail.com>
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > > for which the drivers will be added in subsequent patches.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > > drivers/mfd/Kconfig | 11 ++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++
> > > include/linux/mfd/max77650.h | 59 ++++++
> > > 4 files changed, 413 insertions(+)
> > > create mode 100644 drivers/mfd/max77650.c
> > > create mode 100644 include/linux/mfd/max77650.h
[...]
> > > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > > + {
> > > + .cell_num = MAX77650_CELL_CHARGER,
> > > + .irqs = max77650_charger_irqs,
> > > + .irq_names = max77650_charger_irq_names,
> > > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs),
> > > + },
> > > + {
> > > + .cell_num = MAX77650_CELL_GPIO,
> > > + .irqs = max77650_gpio_irqs,
> > > + .irq_names = max77650_gpio_irq_names,
> > > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs),
> > > + },
> > > + {
> > > + .cell_num = MAX77650_CELL_ONKEY,
> > > + .irqs = max77650_onkey_irqs,
> > > + .irq_names = max77650_onkey_irq_names,
> > > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs),
> > > + },
> > > +};
> >
> > This is all a bit convoluted and nasty TBH.
> >
> > > +static const struct mfd_cell max77650_cells[] = {
> > > + [MAX77650_CELL_REGULATOR] = {
> > > + .name = "max77650-regulator",
> > > + .of_compatible = "maxim,max77650-regulator",
> > > + },
> > > + [MAX77650_CELL_CHARGER] = {
> > > + .name = "max77650-charger",
> > > + .of_compatible = "maxim,max77650-charger",
> > > + },
> > > + [MAX77650_CELL_GPIO] = {
> > > + .name = "max77650-gpio",
> > > + .of_compatible = "maxim,max77650-gpio",
> > > + },
> > > + [MAX77650_CELL_LED] = {
> > > + .name = "max77650-led",
> > > + .of_compatible = "maxim,max77650-led",
> > > + },
> > > + [MAX77650_CELL_ONKEY] = {
> > > + .name = "max77650-onkey",
> > > + .of_compatible = "maxim,max77650-onkey",
> > > + },
> > > +};
> >
> > Why are you numbering the cells? There is no need to do this.
> >
>
> Just for better readability. It makes sense to me coupled with
> MAX77650_NUM_CELLS.
You have it the wrong way around. You define the cell data, then
provide the number of them using ARRAY_SIZE(). The enum containing
MAX77650_NUM_CELLS should not exist.
> > > +static const struct regmap_irq max77650_irqs[] = {
> > > + [MAX77650_INT_GPI] = {
> > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > + .mask = MAX77650_INT_GPI_MSK,
> > > + .type = {
> > > + .type_falling_val = MAX77650_INT_GPI_F_MSK,
> > > + .type_rising_val = MAX77650_INT_GPI_R_MSK,
> > > + .types_supported = IRQ_TYPE_EDGE_BOTH,
> > > + },
> > > + },
> > > + [MAX77650_INT_nEN_F] = {
> > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > + .mask = MAX77650_INT_nEN_F_MSK,
> > > + },
> > > + [MAX77650_INT_nEN_R] = {
> > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > + .mask = MAX77650_INT_nEN_R_MSK,
> > > + },
> > > + [MAX77650_INT_TJAL1_R] = {
> > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > + .mask = MAX77650_INT_TJAL1_R_MSK,
> > > + },
> > > + [MAX77650_INT_TJAL2_R] = {
> > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > + .mask = MAX77650_INT_TJAL2_R_MSK,
> > > + },
> > > + [MAX77650_INT_DOD_R] = {
> > > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > > + .mask = MAX77650_INT_DOD_R_MSK,
> > > + },
> > > + [MAX77650_INT_THM] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_THM_MSK,
> > > + },
> > > + [MAX77650_INT_CHG] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_CHG_MSK,
> > > + },
> > > + [MAX77650_INT_CHGIN] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_CHGIN_MSK,
> > > + },
> > > + [MAX77650_INT_TJ_REG] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_TJ_REG_MSK,
> > > + },
> > > + [MAX77650_INT_CHGIN_CTRL] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_CHGIN_CTRL_MSK,
> > > + },
> > > + [MAX77650_INT_SYS_CTRL] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_SYS_CTRL_MSK,
> > > + },
> > > + [MAX77650_INT_SYS_CNFG] = {
> > > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > > + .mask = MAX77650_INT_SYS_CNFG_MSK,
> > > + },
> > > +};
> >
> > If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> > you can use REGMAP_IRQ_REG() like everyone else does.
> >
>
> I could even use it now - except for the first interrupt. I decided to
> not use it everywhere as it looks much better that way than having
> REGMAP_IRQ_REG() for all definitions and then the first one sticking
> out like that. It just looks better.
The way it's done at the moment looks terrible.
Please use the MACROs to simplify as much of the code as possible.
> > > +static const struct regmap_irq_chip max77650_irq_chip = {
> > > + .name = "max77650-irq",
> > > + .irqs = max77650_irqs,
> > > + .num_irqs = ARRAY_SIZE(max77650_irqs),
> > > + .num_regs = 2,
> > > + .status_base = MAX77650_REG_INT_GLBL,
> > > + .mask_base = MAX77650_REG_INTM_GLBL,
> > > + .type_in_mask = true,
> > > + .type_invert = true,
> > > + .init_ack_masked = true,
> > > + .clear_on_unmask = true,
> > > +};
> > > +
> > > +static const struct regmap_config max77650_regmap_config = {
> > > + .name = "max77650",
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > > +};
> > > +
> > > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > > +{
> > > + const struct max77650_irq_mapping *mapping;
> > > + struct regmap_irq_chip_data *irq_data;
> > > + struct i2c_client *i2c;
> > > + struct mfd_cell *cell;
> > > + struct resource *res;
> > > + struct regmap *map;
> > > + int i, j, irq, rv;
> > > +
> > > + i2c = to_i2c_client(dev);
> > > +
> > > + map = dev_get_regmap(dev, NULL);
> > > + if (!map)
> > > + return -ENODEV;
> > > +
> > > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > > + IRQF_ONESHOT | IRQF_SHARED, -1,
> >
> > What is -1? Are you sure this isn't defined somewhere?
> >
>
> I don't see any define for negative irq_base argument. I can add that
> in a separate series and convert the users, but for now I'd stick with
> -1.
IMO it should be defined. You can define it locally for now.
> > > + &max77650_irq_chip, &irq_data);
> > > + if (rv)
> > > + return rv;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > > + mapping = &max77650_irq_mapping_table[i];
> > > + cell = &cells[mapping->cell_num];
> > > +
> > > + res = devm_kcalloc(dev, sizeof(*res),
> > > + mapping->num_irqs, GFP_KERNEL);
> > > + if (!res)
> > > + return -ENOMEM;
> > > +
> > > + cell->resources = res;
> > > + cell->num_resources = mapping->num_irqs;
> > > +
> > > + for (j = 0; j < mapping->num_irqs; j++) {
> > > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > > + if (irq < 0)
> > > + return irq;
> > > +
> > > + res[j].start = res[j].end = irq;
> > > + res[j].flags = IORESOURCE_IRQ;
> > > + res[j].name = mapping->irq_names[j];
> > > + }
> > > + }
> >
> > This is the first time I've seen it done like this (and I hate it).
> >
> > Why are you storing the virqs in resources?
> >
> > I think this is highly irregular.
> >
>
> I initially just passed the regmap_irq_chip_data over i2c clientdata
> and sub-drivers would look up virq numbers from it but was advised by
> Dmitry Torokhov to use resources instead. After implementing it this
> way I too think it's more elegant in sub-drivers who can simply do
> platform_get_irq_byname(). Do you have a different idea?
> What exactly don't you like about this?
* 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
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?
--
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-12 8:52 UTC (permalink / raw)
To: Lee Jones
Cc: 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, Bartosz Golaszewski
In-Reply-To: <20190212083642.GT20638@dell>
wt., 12 lut 2019 o 09:36 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Add the core mfd driver for max77650 PMIC. We define five sub-devices
> > for which the drivers will be added in subsequent patches.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> > drivers/mfd/Kconfig | 11 ++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++
> > include/linux/mfd/max77650.h | 59 ++++++
> > 4 files changed, 413 insertions(+)
> > create mode 100644 drivers/mfd/max77650.c
> > create mode 100644 include/linux/mfd/max77650.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 76f9909cf396..a80c3fe80fbe 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -734,6 +734,17 @@ config MFD_MAX77620
> > provides common support for accessing the device; additional drivers
> > must be enabled in order to use the functionality of the device.
> >
> > +config MFD_MAX77650
> > + tristate "Maxim MAX77650/77651 PMIC Support"
> > + depends on I2C
> > + depends on OF || COMPILE_TEST
> > + select MFD_CORE
> > + select REGMAP_I2C
> > + help
> > + Say yes here to add support for Maxim Semiconductor MAX77650 and
> > + MAX77651 Power Management ICs. This is the core multifunction
> > + driver for interacting with the device.
> > +
> > config MFD_MAX77686
> > tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 12980a4ad460..3b912a4015d1 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150) += da9150-core.o
> >
> > obj-$(CONFIG_MFD_MAX14577) += max14577.o
> > obj-$(CONFIG_MFD_MAX77620) += max77620.o
> > +obj-$(CONFIG_MFD_MAX77650) += max77650.o
> > obj-$(CONFIG_MFD_MAX77686) += max77686.o
> > obj-$(CONFIG_MFD_MAX77693) += max77693.o
> > obj-$(CONFIG_MFD_MAX77843) += max77843.o
> > diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c
> > new file mode 100644
> > index 000000000000..7c6164f1fde4
> > --- /dev/null
> > +++ b/drivers/mfd/max77650.c
> > @@ -0,0 +1,342 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Copyright (C) 2018 BayLibre SAS
> > +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > +//
> > +// Core MFD driver for MAXIM 77650/77651 charger/power-supply.
> > +
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/max77650.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define MAX77650_INT_GPI_F_MSK BIT(0)
> > +#define MAX77650_INT_GPI_R_MSK BIT(1)
> > +#define MAX77650_INT_GPI_MSK \
> > + (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK)
> > +#define MAX77650_INT_nEN_F_MSK BIT(2)
> > +#define MAX77650_INT_nEN_R_MSK BIT(3)
> > +#define MAX77650_INT_TJAL1_R_MSK BIT(4)
> > +#define MAX77650_INT_TJAL2_R_MSK BIT(5)
> > +#define MAX77650_INT_DOD_R_MSK BIT(6)
> > +
> > +#define MAX77650_INT_THM_MSK BIT(0)
> > +#define MAX77650_INT_CHG_MSK BIT(1)
> > +#define MAX77650_INT_CHGIN_MSK BIT(2)
> > +#define MAX77650_INT_TJ_REG_MSK BIT(3)
> > +#define MAX77650_INT_CHGIN_CTRL_MSK BIT(4)
> > +#define MAX77650_INT_SYS_CTRL_MSK BIT(5)
> > +#define MAX77650_INT_SYS_CNFG_MSK BIT(6)
> > +
> > +#define MAX77650_INT_GLBL_OFFSET 0
> > +#define MAX77650_INT_CHG_OFFSET 1
> > +
> > +#define MAX77650_SBIA_LPM_MASK BIT(5)
> > +#define MAX77650_SBIA_LPM_DISABLED 0x00
> > +
> > +enum {
> > + MAX77650_INT_GPI = 0,
> > + MAX77650_INT_nEN_F,
> > + MAX77650_INT_nEN_R,
> > + MAX77650_INT_TJAL1_R,
> > + MAX77650_INT_TJAL2_R,
> > + MAX77650_INT_DOD_R,
> > + MAX77650_INT_THM,
> > + MAX77650_INT_CHG,
> > + MAX77650_INT_CHGIN,
> > + MAX77650_INT_TJ_REG,
> > + MAX77650_INT_CHGIN_CTRL,
> > + MAX77650_INT_SYS_CTRL,
> > + MAX77650_INT_SYS_CNFG,
> > +};
> > +
> > +enum {
> > + MAX77650_CELL_REGULATOR = 0,
> > + MAX77650_CELL_CHARGER,
> > + MAX77650_CELL_GPIO,
> > + MAX77650_CELL_LED,
> > + MAX77650_CELL_ONKEY,
> > + MAX77650_NUM_CELLS,
> > +};
> > +
> > +struct max77650_irq_mapping {
> > + int cell_num;
> > + const int *irqs;
> > + const char *const *irq_names;
> > + unsigned int num_irqs;
> > +};
> > +
> > +static const int max77650_charger_irqs[] = {
> > + MAX77650_INT_CHG,
> > + MAX77650_INT_CHGIN,
> > +};
> > +
> > +static const int max77650_gpio_irqs[] = {
> > + MAX77650_INT_GPI,
> > +};
> > +
> > +static const int max77650_onkey_irqs[] = {
> > + MAX77650_INT_nEN_F,
> > + MAX77650_INT_nEN_R,
> > +};
> > +
> > +static const char *const max77650_charger_irq_names[] = {
> > + "CHG",
> > + "CHGIN",
> > +};
> > +
> > +static const char *const max77650_gpio_irq_names[] = {
> > + "GPI",
> > +};
> > +
> > +static const char *const max77650_onkey_irq_names[] = {
> > + "nEN_F",
> > + "nEN_R",
> > +};
> > +
> > +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> > + {
> > + .cell_num = MAX77650_CELL_CHARGER,
> > + .irqs = max77650_charger_irqs,
> > + .irq_names = max77650_charger_irq_names,
> > + .num_irqs = ARRAY_SIZE(max77650_charger_irqs),
> > + },
> > + {
> > + .cell_num = MAX77650_CELL_GPIO,
> > + .irqs = max77650_gpio_irqs,
> > + .irq_names = max77650_gpio_irq_names,
> > + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs),
> > + },
> > + {
> > + .cell_num = MAX77650_CELL_ONKEY,
> > + .irqs = max77650_onkey_irqs,
> > + .irq_names = max77650_onkey_irq_names,
> > + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs),
> > + },
> > +};
>
> This is all a bit convoluted and nasty TBH.
>
> > +static const struct mfd_cell max77650_cells[] = {
> > + [MAX77650_CELL_REGULATOR] = {
> > + .name = "max77650-regulator",
> > + .of_compatible = "maxim,max77650-regulator",
> > + },
> > + [MAX77650_CELL_CHARGER] = {
> > + .name = "max77650-charger",
> > + .of_compatible = "maxim,max77650-charger",
> > + },
> > + [MAX77650_CELL_GPIO] = {
> > + .name = "max77650-gpio",
> > + .of_compatible = "maxim,max77650-gpio",
> > + },
> > + [MAX77650_CELL_LED] = {
> > + .name = "max77650-led",
> > + .of_compatible = "maxim,max77650-led",
> > + },
> > + [MAX77650_CELL_ONKEY] = {
> > + .name = "max77650-onkey",
> > + .of_compatible = "maxim,max77650-onkey",
> > + },
> > +};
>
> Why are you numbering the cells? There is no need to do this.
>
Just for better readability. It makes sense to me coupled with
MAX77650_NUM_CELLS.
> > +static const struct regmap_irq max77650_irqs[] = {
> > + [MAX77650_INT_GPI] = {
> > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > + .mask = MAX77650_INT_GPI_MSK,
> > + .type = {
> > + .type_falling_val = MAX77650_INT_GPI_F_MSK,
> > + .type_rising_val = MAX77650_INT_GPI_R_MSK,
> > + .types_supported = IRQ_TYPE_EDGE_BOTH,
> > + },
> > + },
> > + [MAX77650_INT_nEN_F] = {
> > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > + .mask = MAX77650_INT_nEN_F_MSK,
> > + },
> > + [MAX77650_INT_nEN_R] = {
> > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > + .mask = MAX77650_INT_nEN_R_MSK,
> > + },
> > + [MAX77650_INT_TJAL1_R] = {
> > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > + .mask = MAX77650_INT_TJAL1_R_MSK,
> > + },
> > + [MAX77650_INT_TJAL2_R] = {
> > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > + .mask = MAX77650_INT_TJAL2_R_MSK,
> > + },
> > + [MAX77650_INT_DOD_R] = {
> > + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> > + .mask = MAX77650_INT_DOD_R_MSK,
> > + },
> > + [MAX77650_INT_THM] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_THM_MSK,
> > + },
> > + [MAX77650_INT_CHG] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_CHG_MSK,
> > + },
> > + [MAX77650_INT_CHGIN] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_CHGIN_MSK,
> > + },
> > + [MAX77650_INT_TJ_REG] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_TJ_REG_MSK,
> > + },
> > + [MAX77650_INT_CHGIN_CTRL] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_CHGIN_CTRL_MSK,
> > + },
> > + [MAX77650_INT_SYS_CTRL] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_SYS_CTRL_MSK,
> > + },
> > + [MAX77650_INT_SYS_CNFG] = {
> > + .reg_offset = MAX77650_INT_CHG_OFFSET,
> > + .mask = MAX77650_INT_SYS_CNFG_MSK,
> > + },
> > +};
>
> If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
> you can use REGMAP_IRQ_REG() like everyone else does.
>
I could even use it now - except for the first interrupt. I decided to
not use it everywhere as it looks much better that way than having
REGMAP_IRQ_REG() for all definitions and then the first one sticking
out like that. It just looks better.
> > +static const struct regmap_irq_chip max77650_irq_chip = {
> > + .name = "max77650-irq",
> > + .irqs = max77650_irqs,
> > + .num_irqs = ARRAY_SIZE(max77650_irqs),
> > + .num_regs = 2,
> > + .status_base = MAX77650_REG_INT_GLBL,
> > + .mask_base = MAX77650_REG_INTM_GLBL,
> > + .type_in_mask = true,
> > + .type_invert = true,
> > + .init_ack_masked = true,
> > + .clear_on_unmask = true,
> > +};
> > +
> > +static const struct regmap_config max77650_regmap_config = {
> > + .name = "max77650",
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > +};
> > +
> > +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> > +{
> > + const struct max77650_irq_mapping *mapping;
> > + struct regmap_irq_chip_data *irq_data;
> > + struct i2c_client *i2c;
> > + struct mfd_cell *cell;
> > + struct resource *res;
> > + struct regmap *map;
> > + int i, j, irq, rv;
> > +
> > + i2c = to_i2c_client(dev);
> > +
> > + map = dev_get_regmap(dev, NULL);
> > + if (!map)
> > + return -ENODEV;
> > +
> > + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> > + IRQF_ONESHOT | IRQF_SHARED, -1,
>
> What is -1? Are you sure this isn't defined somewhere?
>
I don't see any define for negative irq_base argument. I can add that
in a separate series and convert the users, but for now I'd stick with
-1.
> > + &max77650_irq_chip, &irq_data);
> > + if (rv)
> > + return rv;
> > +
> > + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> > + mapping = &max77650_irq_mapping_table[i];
> > + cell = &cells[mapping->cell_num];
> > +
> > + res = devm_kcalloc(dev, sizeof(*res),
> > + mapping->num_irqs, GFP_KERNEL);
> > + if (!res)
> > + return -ENOMEM;
> > +
> > + cell->resources = res;
> > + cell->num_resources = mapping->num_irqs;
> > +
> > + for (j = 0; j < mapping->num_irqs; j++) {
> > + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> > + if (irq < 0)
> > + return irq;
> > +
> > + res[j].start = res[j].end = irq;
> > + res[j].flags = IORESOURCE_IRQ;
> > + res[j].name = mapping->irq_names[j];
> > + }
> > + }
>
> This is the first time I've seen it done like this (and I hate it).
>
> Why are you storing the virqs in resources?
>
> I think this is highly irregular.
>
I initially just passed the regmap_irq_chip_data over i2c clientdata
and sub-drivers would look up virq numbers from it but was advised by
Dmitry Torokhov to use resources instead. After implementing it this
way I too think it's more elegant in sub-drivers who can simply do
platform_get_irq_byname(). Do you have a different idea? What exactly
don't you like about this?
> > + return 0;
> > +}
> > +
> > +static int max77650_i2c_probe(struct i2c_client *i2c)
> > +{
> > + struct device *dev = &i2c->dev;
> > + struct mfd_cell *cells;
> > + struct regmap *map;
> > + unsigned int val;
> > + int rv;
> > +
> > + map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
> > + if (IS_ERR(map))
>
> What error messages does devm_regmap_init_i2c() report? Does it print
> out its own error messages internally? If not it would be better to
> provide a suitable error message here.
>
> > + return PTR_ERR(map);
> > +
> > + rv = regmap_read(map, MAX77650_REG_CID, &val);
> > + if (rv)
>
> Better to provide a suitable error message here.
>
> > + return rv;
> > +
> > + switch (MAX77650_CID_BITS(val)) {
> > + case MAX77650_CID_77650A:
> > + case MAX77650_CID_77650C:
> > + case MAX77650_CID_77651A:
> > + case MAX77650_CID_77651B:
> > + break;
> > + default:
>
> Better to provide a suitable error message here.
>
> > + return -ENODEV;
> > + }
> > +
> > + /*
> > + * This IC has a low-power mode which reduces the quiescent current
> > + * consumption to ~5.6uA but is only suitable for systems consuming
> > + * less than ~2mA. Since this is not likely the case even on
> > + * linux-based wearables - keep the chip in normal power mode.
> > + */
> > + rv = regmap_update_bits(map,
> > + MAX77650_REG_CNFG_GLBL,
> > + MAX77650_SBIA_LPM_MASK,
> > + MAX77650_SBIA_LPM_DISABLED);
> > + if (rv)
>
> Better to provide a suitable error message here.
>
> > + return rv;
> > +
> > + cells = devm_kmemdup(dev, max77650_cells,
> > + sizeof(max77650_cells), GFP_KERNEL);
> > + if (!cells)
> > + return -ENOMEM;
> > +
> > + rv = max77650_setup_irqs(dev, cells);
> > + if (rv)
> > + return rv;
> > +
> > + return devm_mfd_add_devices(dev, -1, cells,
>
> Use the correct defines instead of -1.
>
Will do that and add the error messages.
Bart
> `git grep mfd_add_devices`
>
> > + MAX77650_NUM_CELLS, NULL, 0, NULL);
> > +}
> > +
> > +static const struct of_device_id max77650_of_match[] = {
> > + { .compatible = "maxim,max77650" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, max77650_of_match);
> > +
> > +static struct i2c_driver max77650_i2c_driver = {
> > + .driver = {
> > + .name = "max77650",
> > + .of_match_table = of_match_ptr(max77650_of_match),
> > + },
> > + .probe_new = max77650_i2c_probe,
> > +};
> > +module_i2c_driver(max77650_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver");
> > +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
> > new file mode 100644
> > index 000000000000..c809e211a8cd
> > --- /dev/null
> > +++ b/include/linux/mfd/max77650.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2018 BayLibre SAS
> > + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > + *
> > + * Common definitions for MAXIM 77650/77651 charger/power-supply.
> > + */
> > +
> > +#ifndef MAX77650_H
> > +#define MAX77650_H
> > +
> > +#include <linux/bits.h>
> > +
> > +#define MAX77650_REG_INT_GLBL 0x00
> > +#define MAX77650_REG_INT_CHG 0x01
> > +#define MAX77650_REG_STAT_CHG_A 0x02
> > +#define MAX77650_REG_STAT_CHG_B 0x03
> > +#define MAX77650_REG_ERCFLAG 0x04
> > +#define MAX77650_REG_STAT_GLBL 0x05
> > +#define MAX77650_REG_INTM_GLBL 0x06
> > +#define MAX77650_REG_INTM_CHG 0x07
> > +#define MAX77650_REG_CNFG_GLBL 0x10
> > +#define MAX77650_REG_CID 0x11
> > +#define MAX77650_REG_CNFG_GPIO 0x12
> > +#define MAX77650_REG_CNFG_CHG_A 0x18
> > +#define MAX77650_REG_CNFG_CHG_B 0x19
> > +#define MAX77650_REG_CNFG_CHG_C 0x1a
> > +#define MAX77650_REG_CNFG_CHG_D 0x1b
> > +#define MAX77650_REG_CNFG_CHG_E 0x1c
> > +#define MAX77650_REG_CNFG_CHG_F 0x1d
> > +#define MAX77650_REG_CNFG_CHG_G 0x1e
> > +#define MAX77650_REG_CNFG_CHG_H 0x1f
> > +#define MAX77650_REG_CNFG_CHG_I 0x20
> > +#define MAX77650_REG_CNFG_SBB_TOP 0x28
> > +#define MAX77650_REG_CNFG_SBB0_A 0x29
> > +#define MAX77650_REG_CNFG_SBB0_B 0x2a
> > +#define MAX77650_REG_CNFG_SBB1_A 0x2b
> > +#define MAX77650_REG_CNFG_SBB1_B 0x2c
> > +#define MAX77650_REG_CNFG_SBB2_A 0x2d
> > +#define MAX77650_REG_CNFG_SBB2_B 0x2e
> > +#define MAX77650_REG_CNFG_LDO_A 0x38
> > +#define MAX77650_REG_CNFG_LDO_B 0x39
> > +#define MAX77650_REG_CNFG_LED0_A 0x40
> > +#define MAX77650_REG_CNFG_LED1_A 0x41
> > +#define MAX77650_REG_CNFG_LED2_A 0x42
> > +#define MAX77650_REG_CNFG_LED0_B 0x43
> > +#define MAX77650_REG_CNFG_LED1_B 0x44
> > +#define MAX77650_REG_CNFG_LED2_B 0x45
> > +#define MAX77650_REG_CNFG_LED_TOP 0x46
> > +
> > +#define MAX77650_CID_MASK GENMASK(3, 0)
> > +#define MAX77650_CID_BITS(_reg) (_reg & MAX77650_CID_MASK)
> > +
> > +#define MAX77650_CID_77650A 0x03
> > +#define MAX77650_CID_77650C 0x0a
> > +#define MAX77650_CID_77651A 0x06
> > +#define MAX77650_CID_77651B 0x08
> > +
> > +#endif /* MAX77650_H */
>
> --
> 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: Lee Jones @ 2019-02-12 8:36 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190205091237.6448-6-brgl@bgdev.pl>
On Tue, 05 Feb 2019, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add the core mfd driver for max77650 PMIC. We define five sub-devices
> for which the drivers will be added in subsequent patches.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/mfd/Kconfig | 11 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/max77650.c | 342 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/max77650.h | 59 ++++++
> 4 files changed, 413 insertions(+)
> create mode 100644 drivers/mfd/max77650.c
> create mode 100644 include/linux/mfd/max77650.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 76f9909cf396..a80c3fe80fbe 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -734,6 +734,17 @@ config MFD_MAX77620
> provides common support for accessing the device; additional drivers
> must be enabled in order to use the functionality of the device.
>
> +config MFD_MAX77650
> + tristate "Maxim MAX77650/77651 PMIC Support"
> + depends on I2C
> + depends on OF || COMPILE_TEST
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + Say yes here to add support for Maxim Semiconductor MAX77650 and
> + MAX77651 Power Management ICs. This is the core multifunction
> + driver for interacting with the device.
> +
> config MFD_MAX77686
> tristate "Maxim Semiconductor MAX77686/802 PMIC Support"
> depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 12980a4ad460..3b912a4015d1 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -151,6 +151,7 @@ obj-$(CONFIG_MFD_DA9150) += da9150-core.o
>
> obj-$(CONFIG_MFD_MAX14577) += max14577.o
> obj-$(CONFIG_MFD_MAX77620) += max77620.o
> +obj-$(CONFIG_MFD_MAX77650) += max77650.o
> obj-$(CONFIG_MFD_MAX77686) += max77686.o
> obj-$(CONFIG_MFD_MAX77693) += max77693.o
> obj-$(CONFIG_MFD_MAX77843) += max77843.o
> diff --git a/drivers/mfd/max77650.c b/drivers/mfd/max77650.c
> new file mode 100644
> index 000000000000..7c6164f1fde4
> --- /dev/null
> +++ b/drivers/mfd/max77650.c
> @@ -0,0 +1,342 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 BayLibre SAS
> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> +//
> +// Core MFD driver for MAXIM 77650/77651 charger/power-supply.
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/max77650.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#define MAX77650_INT_GPI_F_MSK BIT(0)
> +#define MAX77650_INT_GPI_R_MSK BIT(1)
> +#define MAX77650_INT_GPI_MSK \
> + (MAX77650_INT_GPI_F_MSK | MAX77650_INT_GPI_R_MSK)
> +#define MAX77650_INT_nEN_F_MSK BIT(2)
> +#define MAX77650_INT_nEN_R_MSK BIT(3)
> +#define MAX77650_INT_TJAL1_R_MSK BIT(4)
> +#define MAX77650_INT_TJAL2_R_MSK BIT(5)
> +#define MAX77650_INT_DOD_R_MSK BIT(6)
> +
> +#define MAX77650_INT_THM_MSK BIT(0)
> +#define MAX77650_INT_CHG_MSK BIT(1)
> +#define MAX77650_INT_CHGIN_MSK BIT(2)
> +#define MAX77650_INT_TJ_REG_MSK BIT(3)
> +#define MAX77650_INT_CHGIN_CTRL_MSK BIT(4)
> +#define MAX77650_INT_SYS_CTRL_MSK BIT(5)
> +#define MAX77650_INT_SYS_CNFG_MSK BIT(6)
> +
> +#define MAX77650_INT_GLBL_OFFSET 0
> +#define MAX77650_INT_CHG_OFFSET 1
> +
> +#define MAX77650_SBIA_LPM_MASK BIT(5)
> +#define MAX77650_SBIA_LPM_DISABLED 0x00
> +
> +enum {
> + MAX77650_INT_GPI = 0,
> + MAX77650_INT_nEN_F,
> + MAX77650_INT_nEN_R,
> + MAX77650_INT_TJAL1_R,
> + MAX77650_INT_TJAL2_R,
> + MAX77650_INT_DOD_R,
> + MAX77650_INT_THM,
> + MAX77650_INT_CHG,
> + MAX77650_INT_CHGIN,
> + MAX77650_INT_TJ_REG,
> + MAX77650_INT_CHGIN_CTRL,
> + MAX77650_INT_SYS_CTRL,
> + MAX77650_INT_SYS_CNFG,
> +};
> +
> +enum {
> + MAX77650_CELL_REGULATOR = 0,
> + MAX77650_CELL_CHARGER,
> + MAX77650_CELL_GPIO,
> + MAX77650_CELL_LED,
> + MAX77650_CELL_ONKEY,
> + MAX77650_NUM_CELLS,
> +};
> +
> +struct max77650_irq_mapping {
> + int cell_num;
> + const int *irqs;
> + const char *const *irq_names;
> + unsigned int num_irqs;
> +};
> +
> +static const int max77650_charger_irqs[] = {
> + MAX77650_INT_CHG,
> + MAX77650_INT_CHGIN,
> +};
> +
> +static const int max77650_gpio_irqs[] = {
> + MAX77650_INT_GPI,
> +};
> +
> +static const int max77650_onkey_irqs[] = {
> + MAX77650_INT_nEN_F,
> + MAX77650_INT_nEN_R,
> +};
> +
> +static const char *const max77650_charger_irq_names[] = {
> + "CHG",
> + "CHGIN",
> +};
> +
> +static const char *const max77650_gpio_irq_names[] = {
> + "GPI",
> +};
> +
> +static const char *const max77650_onkey_irq_names[] = {
> + "nEN_F",
> + "nEN_R",
> +};
> +
> +static const struct max77650_irq_mapping max77650_irq_mapping_table[] = {
> + {
> + .cell_num = MAX77650_CELL_CHARGER,
> + .irqs = max77650_charger_irqs,
> + .irq_names = max77650_charger_irq_names,
> + .num_irqs = ARRAY_SIZE(max77650_charger_irqs),
> + },
> + {
> + .cell_num = MAX77650_CELL_GPIO,
> + .irqs = max77650_gpio_irqs,
> + .irq_names = max77650_gpio_irq_names,
> + .num_irqs = ARRAY_SIZE(max77650_gpio_irqs),
> + },
> + {
> + .cell_num = MAX77650_CELL_ONKEY,
> + .irqs = max77650_onkey_irqs,
> + .irq_names = max77650_onkey_irq_names,
> + .num_irqs = ARRAY_SIZE(max77650_onkey_irqs),
> + },
> +};
This is all a bit convoluted and nasty TBH.
> +static const struct mfd_cell max77650_cells[] = {
> + [MAX77650_CELL_REGULATOR] = {
> + .name = "max77650-regulator",
> + .of_compatible = "maxim,max77650-regulator",
> + },
> + [MAX77650_CELL_CHARGER] = {
> + .name = "max77650-charger",
> + .of_compatible = "maxim,max77650-charger",
> + },
> + [MAX77650_CELL_GPIO] = {
> + .name = "max77650-gpio",
> + .of_compatible = "maxim,max77650-gpio",
> + },
> + [MAX77650_CELL_LED] = {
> + .name = "max77650-led",
> + .of_compatible = "maxim,max77650-led",
> + },
> + [MAX77650_CELL_ONKEY] = {
> + .name = "max77650-onkey",
> + .of_compatible = "maxim,max77650-onkey",
> + },
> +};
Why are you numbering the cells? There is no need to do this.
> +static const struct regmap_irq max77650_irqs[] = {
> + [MAX77650_INT_GPI] = {
> + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> + .mask = MAX77650_INT_GPI_MSK,
> + .type = {
> + .type_falling_val = MAX77650_INT_GPI_F_MSK,
> + .type_rising_val = MAX77650_INT_GPI_R_MSK,
> + .types_supported = IRQ_TYPE_EDGE_BOTH,
> + },
> + },
> + [MAX77650_INT_nEN_F] = {
> + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> + .mask = MAX77650_INT_nEN_F_MSK,
> + },
> + [MAX77650_INT_nEN_R] = {
> + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> + .mask = MAX77650_INT_nEN_R_MSK,
> + },
> + [MAX77650_INT_TJAL1_R] = {
> + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> + .mask = MAX77650_INT_TJAL1_R_MSK,
> + },
> + [MAX77650_INT_TJAL2_R] = {
> + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> + .mask = MAX77650_INT_TJAL2_R_MSK,
> + },
> + [MAX77650_INT_DOD_R] = {
> + .reg_offset = MAX77650_INT_GLBL_OFFSET,
> + .mask = MAX77650_INT_DOD_R_MSK,
> + },
> + [MAX77650_INT_THM] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_THM_MSK,
> + },
> + [MAX77650_INT_CHG] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_CHG_MSK,
> + },
> + [MAX77650_INT_CHGIN] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_CHGIN_MSK,
> + },
> + [MAX77650_INT_TJ_REG] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_TJ_REG_MSK,
> + },
> + [MAX77650_INT_CHGIN_CTRL] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_CHGIN_CTRL_MSK,
> + },
> + [MAX77650_INT_SYS_CTRL] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_SYS_CTRL_MSK,
> + },
> + [MAX77650_INT_SYS_CNFG] = {
> + .reg_offset = MAX77650_INT_CHG_OFFSET,
> + .mask = MAX77650_INT_SYS_CNFG_MSK,
> + },
> +};
If you get rid of all of the horrible hoop jumping in *_setup_irqs(),
you can use REGMAP_IRQ_REG() like everyone else does.
> +static const struct regmap_irq_chip max77650_irq_chip = {
> + .name = "max77650-irq",
> + .irqs = max77650_irqs,
> + .num_irqs = ARRAY_SIZE(max77650_irqs),
> + .num_regs = 2,
> + .status_base = MAX77650_REG_INT_GLBL,
> + .mask_base = MAX77650_REG_INTM_GLBL,
> + .type_in_mask = true,
> + .type_invert = true,
> + .init_ack_masked = true,
> + .clear_on_unmask = true,
> +};
> +
> +static const struct regmap_config max77650_regmap_config = {
> + .name = "max77650",
> + .reg_bits = 8,
> + .val_bits = 8,
> +};
> +
> +static int max77650_setup_irqs(struct device *dev, struct mfd_cell *cells)
> +{
> + const struct max77650_irq_mapping *mapping;
> + struct regmap_irq_chip_data *irq_data;
> + struct i2c_client *i2c;
> + struct mfd_cell *cell;
> + struct resource *res;
> + struct regmap *map;
> + int i, j, irq, rv;
> +
> + i2c = to_i2c_client(dev);
> +
> + map = dev_get_regmap(dev, NULL);
> + if (!map)
> + return -ENODEV;
> +
> + rv = devm_regmap_add_irq_chip(dev, map, i2c->irq,
> + IRQF_ONESHOT | IRQF_SHARED, -1,
What is -1? Are you sure this isn't defined somewhere?
> + &max77650_irq_chip, &irq_data);
> + if (rv)
> + return rv;
> +
> + for (i = 0; i < ARRAY_SIZE(max77650_irq_mapping_table); i++) {
> + mapping = &max77650_irq_mapping_table[i];
> + cell = &cells[mapping->cell_num];
> +
> + res = devm_kcalloc(dev, sizeof(*res),
> + mapping->num_irqs, GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
> +
> + cell->resources = res;
> + cell->num_resources = mapping->num_irqs;
> +
> + for (j = 0; j < mapping->num_irqs; j++) {
> + irq = regmap_irq_get_virq(irq_data, mapping->irqs[j]);
> + if (irq < 0)
> + return irq;
> +
> + res[j].start = res[j].end = irq;
> + res[j].flags = IORESOURCE_IRQ;
> + res[j].name = mapping->irq_names[j];
> + }
> + }
This is the first time I've seen it done like this (and I hate it).
Why are you storing the virqs in resources?
I think this is highly irregular.
> + return 0;
> +}
> +
> +static int max77650_i2c_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct mfd_cell *cells;
> + struct regmap *map;
> + unsigned int val;
> + int rv;
> +
> + map = devm_regmap_init_i2c(i2c, &max77650_regmap_config);
> + if (IS_ERR(map))
What error messages does devm_regmap_init_i2c() report? Does it print
out its own error messages internally? If not it would be better to
provide a suitable error message here.
> + return PTR_ERR(map);
> +
> + rv = regmap_read(map, MAX77650_REG_CID, &val);
> + if (rv)
Better to provide a suitable error message here.
> + return rv;
> +
> + switch (MAX77650_CID_BITS(val)) {
> + case MAX77650_CID_77650A:
> + case MAX77650_CID_77650C:
> + case MAX77650_CID_77651A:
> + case MAX77650_CID_77651B:
> + break;
> + default:
Better to provide a suitable error message here.
> + return -ENODEV;
> + }
> +
> + /*
> + * This IC has a low-power mode which reduces the quiescent current
> + * consumption to ~5.6uA but is only suitable for systems consuming
> + * less than ~2mA. Since this is not likely the case even on
> + * linux-based wearables - keep the chip in normal power mode.
> + */
> + rv = regmap_update_bits(map,
> + MAX77650_REG_CNFG_GLBL,
> + MAX77650_SBIA_LPM_MASK,
> + MAX77650_SBIA_LPM_DISABLED);
> + if (rv)
Better to provide a suitable error message here.
> + return rv;
> +
> + cells = devm_kmemdup(dev, max77650_cells,
> + sizeof(max77650_cells), GFP_KERNEL);
> + if (!cells)
> + return -ENOMEM;
> +
> + rv = max77650_setup_irqs(dev, cells);
> + if (rv)
> + return rv;
> +
> + return devm_mfd_add_devices(dev, -1, cells,
Use the correct defines instead of -1.
`git grep mfd_add_devices`
> + MAX77650_NUM_CELLS, NULL, 0, NULL);
> +}
> +
> +static const struct of_device_id max77650_of_match[] = {
> + { .compatible = "maxim,max77650" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, max77650_of_match);
> +
> +static struct i2c_driver max77650_i2c_driver = {
> + .driver = {
> + .name = "max77650",
> + .of_match_table = of_match_ptr(max77650_of_match),
> + },
> + .probe_new = max77650_i2c_probe,
> +};
> +module_i2c_driver(max77650_i2c_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 multi-function core driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/max77650.h b/include/linux/mfd/max77650.h
> new file mode 100644
> index 000000000000..c809e211a8cd
> --- /dev/null
> +++ b/include/linux/mfd/max77650.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 BayLibre SAS
> + * Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> + *
> + * Common definitions for MAXIM 77650/77651 charger/power-supply.
> + */
> +
> +#ifndef MAX77650_H
> +#define MAX77650_H
> +
> +#include <linux/bits.h>
> +
> +#define MAX77650_REG_INT_GLBL 0x00
> +#define MAX77650_REG_INT_CHG 0x01
> +#define MAX77650_REG_STAT_CHG_A 0x02
> +#define MAX77650_REG_STAT_CHG_B 0x03
> +#define MAX77650_REG_ERCFLAG 0x04
> +#define MAX77650_REG_STAT_GLBL 0x05
> +#define MAX77650_REG_INTM_GLBL 0x06
> +#define MAX77650_REG_INTM_CHG 0x07
> +#define MAX77650_REG_CNFG_GLBL 0x10
> +#define MAX77650_REG_CID 0x11
> +#define MAX77650_REG_CNFG_GPIO 0x12
> +#define MAX77650_REG_CNFG_CHG_A 0x18
> +#define MAX77650_REG_CNFG_CHG_B 0x19
> +#define MAX77650_REG_CNFG_CHG_C 0x1a
> +#define MAX77650_REG_CNFG_CHG_D 0x1b
> +#define MAX77650_REG_CNFG_CHG_E 0x1c
> +#define MAX77650_REG_CNFG_CHG_F 0x1d
> +#define MAX77650_REG_CNFG_CHG_G 0x1e
> +#define MAX77650_REG_CNFG_CHG_H 0x1f
> +#define MAX77650_REG_CNFG_CHG_I 0x20
> +#define MAX77650_REG_CNFG_SBB_TOP 0x28
> +#define MAX77650_REG_CNFG_SBB0_A 0x29
> +#define MAX77650_REG_CNFG_SBB0_B 0x2a
> +#define MAX77650_REG_CNFG_SBB1_A 0x2b
> +#define MAX77650_REG_CNFG_SBB1_B 0x2c
> +#define MAX77650_REG_CNFG_SBB2_A 0x2d
> +#define MAX77650_REG_CNFG_SBB2_B 0x2e
> +#define MAX77650_REG_CNFG_LDO_A 0x38
> +#define MAX77650_REG_CNFG_LDO_B 0x39
> +#define MAX77650_REG_CNFG_LED0_A 0x40
> +#define MAX77650_REG_CNFG_LED1_A 0x41
> +#define MAX77650_REG_CNFG_LED2_A 0x42
> +#define MAX77650_REG_CNFG_LED0_B 0x43
> +#define MAX77650_REG_CNFG_LED1_B 0x44
> +#define MAX77650_REG_CNFG_LED2_B 0x45
> +#define MAX77650_REG_CNFG_LED_TOP 0x46
> +
> +#define MAX77650_CID_MASK GENMASK(3, 0)
> +#define MAX77650_CID_BITS(_reg) (_reg & MAX77650_CID_MASK)
> +
> +#define MAX77650_CID_77650A 0x03
> +#define MAX77650_CID_77650C 0x0a
> +#define MAX77650_CID_77651A 0x06
> +#define MAX77650_CID_77651B 0x08
> +
> +#endif /* MAX77650_H */
--
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 06/10] power: supply: max77650: add support for battery charger
From: Lee Jones @ 2019-02-12 8:23 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, linux-kernel, linux-gpio, devicetree,
linux-input, linux-leds, linux-pm, Bartosz Golaszewski
In-Reply-To: <20190205091237.6448-7-brgl@bgdev.pl>
Sebastian,
Just waiting on your Ack for the set.
Could you review this please?
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add basic support for the battery charger for max77650 PMIC.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
> drivers/power/supply/Kconfig | 7 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/max77650-charger.c | 355 ++++++++++++++++++++++++
> 3 files changed, 363 insertions(+)
> create mode 100644 drivers/power/supply/max77650-charger.c
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index e901b9879e7e..0230c96fa94d 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -499,6 +499,13 @@ config CHARGER_DETECTOR_MAX14656
> Revision 1.2 and can be found e.g. in Kindle 4/5th generation
> readers and certain LG devices.
>
> +config CHARGER_MAX77650
> + tristate "Maxim MAX77650 battery charger driver"
> + depends on MFD_MAX77650
> + help
> + Say Y to enable support for the battery charger control of MAX77650
> + PMICs.
> +
> config CHARGER_MAX77693
> tristate "Maxim MAX77693 battery charger driver"
> depends on MFD_MAX77693
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index b731c2a9b695..b73eb8c5c1a9 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_CHARGER_MANAGER) += charger-manager.o
> obj-$(CONFIG_CHARGER_LTC3651) += ltc3651-charger.o
> obj-$(CONFIG_CHARGER_MAX14577) += max14577_charger.o
> obj-$(CONFIG_CHARGER_DETECTOR_MAX14656) += max14656_charger_detector.o
> +obj-$(CONFIG_CHARGER_MAX77650) += max77650-charger.o
> obj-$(CONFIG_CHARGER_MAX77693) += max77693_charger.o
> obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o
> obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o
> diff --git a/drivers/power/supply/max77650-charger.c b/drivers/power/supply/max77650-charger.c
> new file mode 100644
> index 000000000000..7055c9b5ee24
> --- /dev/null
> +++ b/drivers/power/supply/max77650-charger.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2018 BayLibre SAS
> +// Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> +//
> +// Battery charger driver for MAXIM 77650/77651 charger/power-supply.
> +
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/max77650.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +#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
> +
> +#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
> +
> +#define MAX77650_CHARGER_CHG_MASK BIT(1)
> +#define MAX77650_CHARGER_CHG_CHARGING(_reg) \
> + (((_reg) & MAX77650_CHARGER_CHG_MASK) > 1)
> +
> +#define MAX77650_CHARGER_VCHGIN_MIN_MASK 0xc0
> +#define MAX77650_CHARGER_VCHGIN_MIN_SHIFT(_val) ((_val) << 5)
> +
> +#define MAX77650_CHARGER_ICHGIN_LIM_MASK 0x1c
> +#define MAX77650_CHARGER_ICHGIN_LIM_SHIFT(_val) ((_val) << 2)
> +
> +struct max77650_charger_data {
> + struct regmap *map;
> + struct device *dev;
> +};
> +
> +static enum power_supply_property max77650_charger_properties[] = {
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_CHARGE_TYPE
> +};
> +
> +static const unsigned int max77650_charger_vchgin_min_table[] = {
> + 4000000, 4100000, 4200000, 4300000, 4400000, 4500000, 4600000, 4700000
> +};
> +
> +static const unsigned int max77650_charger_ichgin_lim_table[] = {
> + 95000, 190000, 285000, 380000, 475000
> +};
> +
> +static int max77650_charger_set_vchgin_min(struct max77650_charger_data *chg,
> + unsigned int val)
> +{
> + int i, rv;
> +
> + for (i = 0; i < ARRAY_SIZE(max77650_charger_vchgin_min_table); i++) {
> + if (val == max77650_charger_vchgin_min_table[i]) {
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_VCHGIN_MIN_MASK,
> + MAX77650_CHARGER_VCHGIN_MIN_SHIFT(i));
> + if (rv)
> + return rv;
> +
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int max77650_charger_set_ichgin_lim(struct max77650_charger_data *chg,
> + unsigned int val)
> +{
> + int i, rv;
> +
> + for (i = 0; i < ARRAY_SIZE(max77650_charger_ichgin_lim_table); i++) {
> + if (val == max77650_charger_ichgin_lim_table[i]) {
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_ICHGIN_LIM_MASK,
> + MAX77650_CHARGER_ICHGIN_LIM_SHIFT(i));
> + if (rv)
> + return rv;
> +
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static void max77650_charger_enable(struct max77650_charger_data *chg)
> +{
> + int rv;
> +
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_ENABLED);
> + if (rv)
> + dev_err(chg->dev, "unable to enable the charger: %d\n", rv);
> +}
> +
> +static void max77650_charger_disable(struct max77650_charger_data *chg)
> +{
> + int rv;
> +
> + rv = regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_DISABLED);
> + if (rv)
> + dev_err(chg->dev, "unable to disable the charger: %d\n", rv);
> +}
> +
> +static irqreturn_t max77650_charger_check_status(int irq, void *data)
> +{
> + struct max77650_charger_data *chg = data;
> + int rv, reg;
> +
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv) {
> + dev_err(chg->dev,
> + "unable to read the charger status: %d\n", rv);
> + return IRQ_HANDLED;
> + }
> +
> + switch (MAX77650_CHARGER_CHGIN_DTLS_BITS(reg)) {
> + case MAX77650_CHARGER_CHGIN_UVL:
> + dev_err(chg->dev, "undervoltage lockout detected, disabling charger\n");
> + max77650_charger_disable(chg);
> + break;
> + case MAX77650_CHARGER_CHGIN_OVL:
> + dev_err(chg->dev, "overvoltage lockout detected, disabling charger\n");
> + max77650_charger_disable(chg);
> + break;
> + case MAX77650_CHARGER_CHGIN_OKAY:
> + max77650_charger_enable(chg);
> + break;
> + default:
> + /* May be 0x10 - debouncing */
> + break;
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int max77650_charger_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct max77650_charger_data *chg = power_supply_get_drvdata(psy);
> + int rv, reg;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv)
> + return rv;
> +
> + if (MAX77650_CHARGER_CHG_CHARGING(reg)) {
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> + }
> +
> + switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
> + case MAX77650_CHARGER_CHG_OFF:
> + case MAX77650_CHARGER_CHG_SUSP_PF:
> + case MAX77650_CHARGER_CHG_SUSP_FCF:
> + case MAX77650_CHARGER_CHG_SUSP_BTF:
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + break;
> + case MAX77650_CHARGER_CHG_PREQ:
> + case MAX77650_CHARGER_CHG_ON_CURR:
> + case MAX77650_CHARGER_CHG_ON_JCURR:
> + case MAX77650_CHARGER_CHG_ON_VOLT:
> + case MAX77650_CHARGER_CHG_ON_JVOLT:
> + case MAX77650_CHARGER_CHG_ON_TOPOFF:
> + case MAX77650_CHARGER_CHG_ON_JTOPOFF:
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + break;
> + case MAX77650_CHARGER_CHG_DONE:
> + val->intval = POWER_SUPPLY_STATUS_FULL;
> + break;
> + default:
> + val->intval = POWER_SUPPLY_STATUS_UNKNOWN;
> + }
> + break;
> + case POWER_SUPPLY_PROP_ONLINE:
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv)
> + return rv;
> +
> + val->intval = MAX77650_CHARGER_CHG_CHARGING(reg);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
> + rv = regmap_read(chg->map, MAX77650_REG_STAT_CHG_B, ®);
> + if (rv)
> + return rv;
> +
> + if (!MAX77650_CHARGER_CHG_CHARGING(reg)) {
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
> + break;
> + }
> +
> + switch (MAX77650_CHARGER_CHG_DTLS_BITS(reg)) {
> + case MAX77650_CHARGER_CHG_PREQ:
> + case MAX77650_CHARGER_CHG_ON_CURR:
> + case MAX77650_CHARGER_CHG_ON_JCURR:
> + case MAX77650_CHARGER_CHG_ON_VOLT:
> + case MAX77650_CHARGER_CHG_ON_JVOLT:
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_FAST;
> + break;
> + case MAX77650_CHARGER_CHG_ON_TOPOFF:
> + case MAX77650_CHARGER_CHG_ON_JTOPOFF:
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> + break;
> + default:
> + val->intval = POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct power_supply_desc max77650_battery_desc = {
> + .name = "max77650",
> + .type = POWER_SUPPLY_TYPE_USB,
> + .get_property = max77650_charger_get_property,
> + .properties = max77650_charger_properties,
> + .num_properties = ARRAY_SIZE(max77650_charger_properties),
> +};
> +
> +static int max77650_charger_probe(struct platform_device *pdev)
> +{
> + struct power_supply_config pscfg = {};
> + struct max77650_charger_data *chg;
> + struct power_supply *battery;
> + struct device *dev, *parent;
> + int rv, chg_irq, chgin_irq;
> + unsigned int prop;
> +
> + dev = &pdev->dev;
> + parent = dev->parent;
> +
> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
> + if (!chg)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, chg);
> +
> + chg->map = dev_get_regmap(parent, NULL);
> + if (!chg->map)
> + return -ENODEV;
> +
> + chg->dev = dev;
> +
> + pscfg.of_node = dev->of_node;
> + pscfg.drv_data = chg;
> +
> + chg_irq = platform_get_irq_byname(pdev, "CHG");
> + if (chg_irq < 0)
> + return chg_irq;
> +
> + chgin_irq = platform_get_irq_byname(pdev, "CHGIN");
> + if (chgin_irq < 0)
> + return chgin_irq;
> +
> + rv = devm_request_any_context_irq(dev, chg_irq,
> + max77650_charger_check_status,
> + IRQF_ONESHOT, "chg", chg);
> + if (rv < 0)
> + return rv;
> +
> + rv = devm_request_any_context_irq(dev, chgin_irq,
> + max77650_charger_check_status,
> + IRQF_ONESHOT, "chgin", chg);
> + if (rv < 0)
> + return rv;
> +
> + battery = devm_power_supply_register(dev,
> + &max77650_battery_desc, &pscfg);
> + if (IS_ERR(battery))
> + return PTR_ERR(battery);
> +
> + rv = of_property_read_u32(dev->of_node, "maxim,vchgin-min", &prop);
> + if (rv == 0) {
> + rv = max77650_charger_set_vchgin_min(chg, prop);
> + if (rv)
> + return rv;
> + }
> +
> + rv = of_property_read_u32(dev->of_node, "maxim,ichgin-lim", &prop);
> + if (rv == 0) {
> + rv = max77650_charger_set_ichgin_lim(chg, prop);
> + if (rv)
> + return rv;
> + }
> +
> + return regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_ENABLED);
> +}
> +
> +static int max77650_charger_remove(struct platform_device *pdev)
> +{
> + struct max77650_charger_data *chg = platform_get_drvdata(pdev);
> +
> + return regmap_update_bits(chg->map,
> + MAX77650_REG_CNFG_CHG_B,
> + MAX77650_CHARGER_CHG_EN_MASK,
> + MAX77650_CHARGER_DISABLED);
> +}
> +
> +static struct platform_driver max77650_charger_driver = {
> + .driver = {
> + .name = "max77650-charger",
> + },
> + .probe = max77650_charger_probe,
> + .remove = max77650_charger_remove,
> +};
> +module_platform_driver(max77650_charger_driver);
> +
> +MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
> +MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
> +MODULE_LICENSE("GPL v2");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH] Input: synaptics_i2c - remove redundant spinlock
From: thesven73 @ 2019-02-12 1:34 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Sven Van Asbroeck, Tejun Heo
From: Sven Van Asbroeck <TheSven73@gmail.com>
Remove a leftover spinlock.
This was required back when mod_delayed_work() did not exist,
and had to be implemented with a cancel + schedule. See
commit e7c2f967445d ("workqueue: use mod_delayed_work() instead of
__cancel + queue")
schedule_delayed_work() and mod_delayed_work() can now be used
concurrently. So the spinlock is no longer needed.
Cc: Tejun Heo <tj@kernel.org>
Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
---
drivers/input/mouse/synaptics_i2c.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
index 8538318d332c..2b2230984ad5 100644
--- a/drivers/input/mouse/synaptics_i2c.c
+++ b/drivers/input/mouse/synaptics_i2c.c
@@ -219,7 +219,6 @@ struct synaptics_i2c {
struct i2c_client *client;
struct input_dev *input;
struct delayed_work dwork;
- spinlock_t lock;
int no_data_count;
int no_decel_param;
int reduce_report_param;
@@ -372,13 +371,7 @@ static bool synaptics_i2c_get_input(struct synaptics_i2c *touch)
static void synaptics_i2c_reschedule_work(struct synaptics_i2c *touch,
unsigned long delay)
{
- unsigned long flags;
-
- spin_lock_irqsave(&touch->lock, flags);
-
mod_delayed_work(system_wq, &touch->dwork, delay);
-
- spin_unlock_irqrestore(&touch->lock, flags);
}
static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
@@ -530,7 +523,6 @@ static struct synaptics_i2c *synaptics_i2c_touch_create(struct i2c_client *clien
touch->scan_rate_param = scan_rate;
set_scan_rate(touch, scan_rate);
INIT_DELAYED_WORK(&touch->dwork, synaptics_i2c_work_handler);
- spin_lock_init(&touch->lock);
return touch;
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2] Input: apanel - switch to using brightness_set_blocking()
From: Sven Van Asbroeck @ 2019-02-12 1:21 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Linux Kernel Mailing List
In-Reply-To: <20190211224729.GA199018@dtor-ws>
On Mon, Feb 11, 2019 at 5:47 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Now that LEDs core allows "blocking" flavor of "set brightness" method we
> can use it and get rid of private work item. As a bonus, we are no longer
> forgetting to cancel it when we unbind the driver.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> v2: get rid of led_bits member (Sven)
Looking good !
Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
^ permalink raw reply
* Re: [PATCH 1/3] Input: lpc32xx-key - add clocks property and fix DT binding example
From: Dmitry Torokhov @ 2019-02-11 22:52 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: linux-input, devicetree, Rob Herring, linux-arm-kernel,
Sylvain Lemieux
In-Reply-To: <bd0b4bf2-dc14-2f9d-1350-25996a46760d@mleia.com>
On Wed, Jan 30, 2019 at 11:22:40PM +0200, Vladimir Zapolskiy wrote:
> Hi Rob, Dmitry,
>
> On 01/26/2019 04:29 PM, Vladimir Zapolskiy wrote:
> > The keypad controller on NXP LPC32xx requires its clock gate to be open,
> > therefore add description of the requires 'clocks' property.
> >
> > In addition adjust the example by adding description of required 'clocks'
> > property and by fixing 'interrupts' property.
> >
> > Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> > ---
> > Documentation/devicetree/bindings/input/lpc32xx-key.txt | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/lpc32xx-key.txt b/Documentation/devicetree/bindings/input/lpc32xx-key.txt
> > index bcf62f856358..2b075a080d30 100644
> > --- a/Documentation/devicetree/bindings/input/lpc32xx-key.txt
> > +++ b/Documentation/devicetree/bindings/input/lpc32xx-key.txt
> > @@ -8,6 +8,7 @@ Required Properties:
> > - reg: Physical base address of the controller and length of memory mapped
> > region.
> > - interrupts: The interrupt number to the cpu.
> > +- clocks: phandle to clock controller plus clock-specifier pair
> > - nxp,debounce-delay-ms: Debounce delay in ms
> > - nxp,scan-delay-ms: Repeated scan period in ms
> > - linux,keymap: the key-code to be reported when the key is pressed
> > @@ -22,7 +23,9 @@ Example:
> > key@40050000 {
> > compatible = "nxp,lpc3220-key";
> > reg = <0x40050000 0x1000>;
> > - interrupts = <54 0>;
> > + clocks = <&clk LPC32XX_CLK_KEY>;
> > + interrupt-parent = <&sic1>;
> > + interrupts = <22 IRQ_TYPE_LEVEL_HIGH>;
> > keypad,num-rows = <1>;
> > keypad,num-columns = <1>;
> > nxp,debounce-delay-ms = <3>;
> >
>
> if you find time, please review/ack this change before 5.0-rc5 to give
> me a chance to push it through ARM tree, thank you in advance.
Sorry, I guess am a bit late, still, the binding is already used by the
driver so we should acknowledge it.
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-02-11 22:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Kees Cook
In-Reply-To: <20190211224816.GE149505@dtor-ws>
On 2/11/19 4:48 PM, Dmitry Torokhov wrote:
> On Mon, Feb 11, 2019 at 04:25:55PM -0600, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch
>> cases where we are expecting to fall through.
>>
>> This patch fixes the following warning:
>>
>> drivers/input/joystick/db9.c: In function ‘db9_saturn_read_packet’:
>> drivers/input/joystick/db9.c:256:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
>> if (tmp == 0xff) {
>> ^
>> drivers/input/joystick/db9.c:263:2: note: here
>> default:
>> ^~~~~~~
>>
>> Notice that, in this particular case, the code comment is modified
>> in accordance with what GCC is expecting to find.
>>
>> This patch is part of the ongoing efforts to enable
>> -Wimplicit-fallthrough.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>
> Applied, thank you.
>
Thanks, Dmitry.
--
Gustavo
^ permalink raw reply
* Re: [PATCH] Input: mark expected switch fall-through
From: Dmitry Torokhov @ 2019-02-11 22:48 UTC (permalink / raw)
To: Gustavo A. R. Silva; +Cc: linux-input, linux-kernel, Kees Cook
In-Reply-To: <20190211222555.GA5472@embeddedor>
On Mon, Feb 11, 2019 at 04:25:55PM -0600, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch
> cases where we are expecting to fall through.
>
> This patch fixes the following warning:
>
> drivers/input/joystick/db9.c: In function ‘db9_saturn_read_packet’:
> drivers/input/joystick/db9.c:256:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
> if (tmp == 0xff) {
> ^
> drivers/input/joystick/db9.c:263:2: note: here
> default:
> ^~~~~~~
>
> Notice that, in this particular case, the code comment is modified
> in accordance with what GCC is expecting to find.
>
> This patch is part of the ongoing efforts to enable
> -Wimplicit-fallthrough.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
Applied, thank you.
> ---
> drivers/input/joystick/db9.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c
> index 804b1b80a8be..5a52b65bef9a 100644
> --- a/drivers/input/joystick/db9.c
> +++ b/drivers/input/joystick/db9.c
> @@ -259,7 +259,7 @@ static unsigned char db9_saturn_read_packet(struct parport *port, unsigned char
> db9_saturn_write_sub(port, type, 3, powered, 0);
> return data[0] = 0xe3;
> }
> - /* else: fall through */
> + /* fall through */
> default:
> return data[0];
> }
> --
> 2.20.1
>
--
Dmitry
^ permalink raw reply
* [PATCH v2] Input: apanel - switch to using brightness_set_blocking()
From: Dmitry Torokhov @ 2019-02-11 22:47 UTC (permalink / raw)
To: linux-input; +Cc: Sven Van Asbroeck, linux-kernel
Now that LEDs core allows "blocking" flavor of "set brightness" method we
can use it and get rid of private work item. As a bonus, we are no longer
forgetting to cancel it when we unbind the driver.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
v2: get rid of led_bits member (Sven)
drivers/input/misc/apanel.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/drivers/input/misc/apanel.c b/drivers/input/misc/apanel.c
index 094bddf56755..c1e66f45d552 100644
--- a/drivers/input/misc/apanel.c
+++ b/drivers/input/misc/apanel.c
@@ -22,7 +22,6 @@
#include <linux/io.h>
#include <linux/input-polldev.h>
#include <linux/i2c.h>
-#include <linux/workqueue.h>
#include <linux/leds.h>
#define APANEL_NAME "Fujitsu Application Panel"
@@ -59,8 +58,6 @@ struct apanel {
struct i2c_client *client;
unsigned short keymap[MAX_PANEL_KEYS];
u16 nkeys;
- u16 led_bits;
- struct work_struct led_work;
struct led_classdev mail_led;
};
@@ -109,25 +106,13 @@ static void apanel_poll(struct input_polled_dev *ipdev)
report_key(idev, ap->keymap[i]);
}
-/* Track state changes of LED */
-static void led_update(struct work_struct *work)
-{
- struct apanel *ap = container_of(work, struct apanel, led_work);
-
- i2c_smbus_write_word_data(ap->client, 0x10, ap->led_bits);
-}
-
-static void mail_led_set(struct led_classdev *led,
+static int mail_led_set(struct led_classdev *led,
enum led_brightness value)
{
struct apanel *ap = container_of(led, struct apanel, mail_led);
+ u16 led_bits = value != LED_OFF ? 0x8000 : 0x0000;
- if (value != LED_OFF)
- ap->led_bits |= 0x8000;
- else
- ap->led_bits &= ~0x8000;
-
- schedule_work(&ap->led_work);
+ return i2c_smbus_write_word_data(ap->client, 0x10, led_bits);
}
static int apanel_remove(struct i2c_client *client)
@@ -179,7 +164,7 @@ static struct apanel apanel = {
},
.mail_led = {
.name = "mail:blue",
- .brightness_set = mail_led_set,
+ .brightness_set_blocking = mail_led_set,
},
};
@@ -235,7 +220,6 @@ static int apanel_probe(struct i2c_client *client,
if (err)
goto out3;
- INIT_WORK(&ap->led_work, led_update);
if (device_chip[APANEL_DEV_LED] != CHIP_NONE) {
err = led_classdev_register(&client->dev, &ap->mail_led);
if (err)
--
2.20.1.791.gb4d0f1c61a-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] Input: mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-02-11 22:25 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Gustavo A. R. Silva, Kees Cook
In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.
This patch fixes the following warning:
drivers/input/joystick/db9.c: In function ‘db9_saturn_read_packet’:
drivers/input/joystick/db9.c:256:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
if (tmp == 0xff) {
^
drivers/input/joystick/db9.c:263:2: note: here
default:
^~~~~~~
Notice that, in this particular case, the code comment is modified
in accordance with what GCC is expecting to find.
This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/input/joystick/db9.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c
index 804b1b80a8be..5a52b65bef9a 100644
--- a/drivers/input/joystick/db9.c
+++ b/drivers/input/joystick/db9.c
@@ -259,7 +259,7 @@ static unsigned char db9_saturn_read_packet(struct parport *port, unsigned char
db9_saturn_write_sub(port, type, 3, powered, 0);
return data[0] = 0xe3;
}
- /* else: fall through */
+ /* fall through */
default:
return data[0];
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] Input: qt2160 - remove redundant spinlock
From: Dmitry Torokhov @ 2019-02-11 22:16 UTC (permalink / raw)
To: thesven73; +Cc: linux-input, linux-kernel, Tejun Heo
In-Reply-To: <20190211023222.29168-1-TheSven73@gmail.com>
On Sun, Feb 10, 2019 at 09:32:22PM -0500, thesven73@gmail.com wrote:
> From: Sven Van Asbroeck <TheSven73@gmail.com>
>
> Remove a spinlock which prevents schedule_delayed_work() and
> mod_delayed_work() from executing concurrently.
>
> This was required back when mod_delayed_work() did not exist,
> and had to be implemented with a cancel + schedule. See
> commit e7c2f967445d ("workqueue: use mod_delayed_work() instead of
> __cancel + queue")
>
> schedule_delayed_work() and mod_delayed_work() can now be used
> concurrently. So the spinlock is no longer needed.
>
> Cc: Tejun Heo <tj@kernel.org>
> Signed-off-by: Sven Van Asbroeck <TheSven73@gmail.com>
Applied, thank you.
> ---
> drivers/input/keyboard/qt2160.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/drivers/input/keyboard/qt2160.c b/drivers/input/keyboard/qt2160.c
> index 43b86482dda0..23dc7c0c7d78 100644
> --- a/drivers/input/keyboard/qt2160.c
> +++ b/drivers/input/keyboard/qt2160.c
> @@ -69,7 +69,6 @@ struct qt2160_data {
> struct i2c_client *client;
> struct input_dev *input;
> struct delayed_work dwork;
> - spinlock_t lock; /* Protects canceling/rescheduling of dwork */
> unsigned short keycodes[ARRAY_SIZE(qt2160_key2code)];
> u16 key_matrix;
> #ifdef CONFIG_LEDS_CLASS
> @@ -221,22 +220,15 @@ static int qt2160_get_key_matrix(struct qt2160_data *qt2160)
> static irqreturn_t qt2160_irq(int irq, void *_qt2160)
> {
> struct qt2160_data *qt2160 = _qt2160;
> - unsigned long flags;
> -
> - spin_lock_irqsave(&qt2160->lock, flags);
>
> mod_delayed_work(system_wq, &qt2160->dwork, 0);
>
> - spin_unlock_irqrestore(&qt2160->lock, flags);
> -
> return IRQ_HANDLED;
> }
>
> static void qt2160_schedule_read(struct qt2160_data *qt2160)
> {
> - spin_lock_irq(&qt2160->lock);
> schedule_delayed_work(&qt2160->dwork, QT2160_CYCLE_INTERVAL);
> - spin_unlock_irq(&qt2160->lock);
> }
>
> static void qt2160_worker(struct work_struct *work)
> @@ -406,7 +398,6 @@ static int qt2160_probe(struct i2c_client *client,
> qt2160->client = client;
> qt2160->input = input;
> INIT_DELAYED_WORK(&qt2160->dwork, qt2160_worker);
> - spin_lock_init(&qt2160->lock);
>
> input->name = "AT42QT2160 Touch Sense Keyboard";
> input->id.bustype = BUS_I2C;
> --
> 2.17.1
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] dt-bindings: input: ti-tsc-adc: Add new compatible for AM654 SoCs
From: Dmitry Torokhov @ 2019-02-11 22:10 UTC (permalink / raw)
To: Vignesh R; +Cc: Rob Herring, linux-input, devicetree, linux-kernel, linux-omap
In-Reply-To: <3623af02-063d-9077-1e59-99d8cfe93ddd@ti.com>
Hi Vignesh,
On Mon, Feb 11, 2019 at 11:10:59AM +0530, Vignesh R wrote:
> Hi Dmitry,
>
> On 08/01/19 10:56 AM, Vignesh R wrote:
> > AM654 SoCs has ADC IP which is similar to AM335x, but without the
> > touchscreen part. Add new compatible to handle AM654 SoCs. Also, it
> > seems that existing compatible strings used in the kernel DTs were never
> > documented. So, document them now.
> >
> > Signed-off-by: Vignesh R <vigneshr@ti.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
>
> If there are no comments, could you queue this bindings patch? This will
> help me in getting ADC DT nodes to be merged into ARM SoC tree.
Sorry I did not realize you waited for me. Yes, please feel free to
merge through your tree.
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks,
--
Dmitry
^ permalink raw reply
* [PATCH] HID: wacom: Mark expected switch fall-through
From: Gustavo A. R. Silva @ 2019-02-11 22:04 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Gustavo A. R. Silva, Kees Cook
In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.
This patch fixes the following warning:
drivers/hid/wacom_wac.c: In function ‘wacom_setup_pen_input_capabilities’:
drivers/hid/wacom_wac.c:3506:3: warning: this statement may fall through [-Wimplicit-fallthrough=]
__clear_bit(ABS_MISC, input_dev->absbit);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/hid/wacom_wac.c:3508:2: note: here
case WACOM_MO:
^~~~
Warning level 3 was used: -Wimplicit-fallthrough=3
This patch is part of the ongoing efforts to enable
-Wimplicit-fallthrough.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/hid/wacom_wac.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 72477e872324..5ecda99bf431 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3504,6 +3504,7 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
switch (features->type) {
case GRAPHIRE_BT:
__clear_bit(ABS_MISC, input_dev->absbit);
+ /* fall through */
case WACOM_MO:
case WACOM_G4:
--
2.20.1
^ permalink raw reply related
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