* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Pavel Machek @ 2019-02-13 11:02 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Lee Jones, Bartosz Golaszewski, Rob Herring, Mark Rutland,
Linus Walleij, Dmitry Torokhov, Jacek Anaszewski,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <CAMRc=MeTMq_GWzQB6LBvts1csMftj=On5sB22jsJZzQzONxbNg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 948 bytes --]
Hi!
> > As you can see, it will return the IRQ Domain for the chip.
> >
> > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > remove all the nastiness in max77650_setup_irqs() and have the Input
> > device use the standard (e.g. platform_get_irq()) APIs.
> >
> > How does that Sound?
>
> This does sound better! Why didn't you lead with that in the first
>place?
TBH, this can be considered rude and I'm not surprised Lee reacted the
way he did.
> It's a pity it's not documented, I had to look at the code to find out
> irq resources would get translated in mfd_add_devices() if a domain is
> present.
I guess documentation patch would be welcome.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 09/11] leds: max77650: add LEDs support
From: Bartosz Golaszewski @ 2019-02-13 10:58 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212121236.GD25613@amd>
wt., 12 lut 2019 o 13:12 Pavel Machek <pavel@ucw.cz> napisał(a):
>
>
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> > index a72f97fca57b..6e7a8f51eccc 100644
> > --- a/drivers/leds/Kconfig
> > +++ b/drivers/leds/Kconfig
> > @@ -608,6 +608,12 @@ config LEDS_TLC591XX
> > This option enables support for Texas Instruments TLC59108
> > and TLC59116 LED controllers.
> >
> > +config LEDS_MAX77650
> > + tristate "LED support for Maxim MAX77650 PMIC"
> > + depends on MFD_MAX77650
> > + help
> > + LEDs driver for MAX77650 family of PMICs from Maxim Integrated."
> > +
>
> List the chips? Remove extra "?
> Pavel
They are already listed in the core mfd Kconfig, but thanks for
spotting that stray ".
Bart
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-13 10:46 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <20190213103946.GG1863@dell>
śr., 13 lut 2019 o 11:39 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
>
> > śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > >
> > > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > >
> > > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > > > >
> > > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > > > >
> > > > > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > > > >
> > > > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > > > >
> > > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > > > > >
> > > > > > > > > > > > Need I go on? :)
> > > > > > > > > > > >
> > > > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > > > >
> > > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > > > >
> > > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > > > > >
> > > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > > > >
> > > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > > > >
> > > > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > > > >
> > > > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > > > >
> > > > > > > Exampe:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata {
> > > > > > > int gpi_irq;
> > > > > > > };
> > > > > > >
> > > > > > > In MFD driver:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > > > >
> > > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > > > >
> > > > > > > gpio_cell.platform_data = gpio_data;
> > > > > > >
> > > > > > > In GPIO driver:
> > > > > > >
> > > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > > > >
> > > > > > > int irq = gpio_data->gpi_irq;
> > > > > >
> > > > > > Definitely not. What you're trying to do is a hack.
> > > > > >
> > > > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > > > in the client to pull them out. Setting them via Regmap, then pulling
> > > > > > them out again in the *same driver*, only to store them in platform
> > > > > > data to be passed to a child device is bonkers.
> > > > > >
> > > > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > > > handle them via the Regmap APIs, *not* both.
> > > > >
> > > > > Right, a plan has been formed.
> > > > >
> > > > > Hopefully this works and you can avoid all this dancing around.
> > > > >
> > > > > Firstly, you need to make a small change to:
> > > > >
> > > > > drivers/base/regmap/regmap-irq.c
> > > > >
> > > > > Add the following function:
> > > > >
> > > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> > > >
> > > > We already do have such function and a lot of mfd drivers actually use it.
> > >
> > > Even better.
> > >
> > > > > As you can see, it will return the IRQ Domain for the chip.
> > > > >
> > > > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > > > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > > > device use the standard (e.g. platform_get_irq()) APIs.
> > > > >
> > > > > How does that Sound?
> > > >
> > > > This does sound better! Why didn't you lead with that in the first place?
> > >
> > > I'm not even going to dignify that stupid question with a response.
> >
> > It's not a stupid question and you're being unnecessarily rude. As an
> > expert in the subsystem you maintain you could have answered simply
> > with a "this is wrong, retrieve the irq domain from the regmap
> > irq_chip and pass it over to mfd_add_devices, the mfd core will create
> > appropriate mappings".
>
> Could be culture clash, but I found the question offensive which is
> why I chose not to answer it. The reason is actually explained below:
>
It wasn't meant to be offensive. I guess when dealing with non-native
English speakers over e-mail it's best to assume good faith.
> "It's only the craziness in this patch which forced me to look into how
> Regmap handles IRQs and come up with a subsequent solution which fits
> your use-case."
>
> Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain
> could be fetched and reused here didn't enter my thought process until
> I delved into the inner workings of Regmap.
>
> Yes, I know MFD pretty well, but I only tend to deep-dive into other
> subsystems, particularly ones as complicated as Regmap, when it's
> necessary to do so. Now I know a little more about it, I can provide
> the feedback you suggest going forward.
>
> > > > It's a pity it's not documented, I had to look at the code to find out
> > > > irq resources would get translated in mfd_add_devices() if a domain is
> > > > present.
> > >
> > > Where is it likely to be documented? MFD is pretty simple and seldom
> > > needs explanation. A 3 second look at the API you're trying to use
> > > (instead of blind copy & paste) would have told you that it's possible
> > > to take an IRQ domain as an argument.
> > >
> > > It's only the craziness in this patch which forced me to look into how
> > > Regmap handles IRQs and come up with a subsequent solution which fits
> > > your use-case.
> > >
> > > > In that case - I really don't see a reason to create a superfluous
> > > > structure to only hold the main regmap - we can very well get it from
> > > > the parent device in sub-drivers as I do now.
> > >
> > > The whole point of this solution is that you don't need to pass
> > > anything non-standard (i.e. not provided by the current APIs) to the
> > > child device.
> >
> > I don't understand what you're saying here.
>
> I'm saying that the structure you speak of is no longer required.
>
Thanks for clarifying.
Bartosz
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-13 10:39 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <CAMpxmJXqEwNtyT1ahmRd05QOjhdi0rT_-5S9Lk8O-PmV8BuA6w@mail.gmail.com>
On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > >
> > > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > > >
> > > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > > >
> > > > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > > >
> > > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > > >
> > > > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > > > >
> > > > > > > > > > > Need I go on? :)
> > > > > > > > > > >
> > > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > > >
> > > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > > >
> > > > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > > > >
> > > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > > >
> > > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > > >
> > > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > > >
> > > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > > >
> > > > > > Exampe:
> > > > > >
> > > > > > struct max77650_gpio_pdata {
> > > > > > int gpi_irq;
> > > > > > };
> > > > > >
> > > > > > In MFD driver:
> > > > > >
> > > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > > >
> > > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > > >
> > > > > > gpio_cell.platform_data = gpio_data;
> > > > > >
> > > > > > In GPIO driver:
> > > > > >
> > > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > > >
> > > > > > int irq = gpio_data->gpi_irq;
> > > > >
> > > > > Definitely not. What you're trying to do is a hack.
> > > > >
> > > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > > in the client to pull them out. Setting them via Regmap, then pulling
> > > > > them out again in the *same driver*, only to store them in platform
> > > > > data to be passed to a child device is bonkers.
> > > > >
> > > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > > handle them via the Regmap APIs, *not* both.
> > > >
> > > > Right, a plan has been formed.
> > > >
> > > > Hopefully this works and you can avoid all this dancing around.
> > > >
> > > > Firstly, you need to make a small change to:
> > > >
> > > > drivers/base/regmap/regmap-irq.c
> > > >
> > > > Add the following function:
> > > >
> > > > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> > >
> > > We already do have such function and a lot of mfd drivers actually use it.
> >
> > Even better.
> >
> > > > As you can see, it will return the IRQ Domain for the chip.
> > > >
> > > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > > device use the standard (e.g. platform_get_irq()) APIs.
> > > >
> > > > How does that Sound?
> > >
> > > This does sound better! Why didn't you lead with that in the first place?
> >
> > I'm not even going to dignify that stupid question with a response.
>
> It's not a stupid question and you're being unnecessarily rude. As an
> expert in the subsystem you maintain you could have answered simply
> with a "this is wrong, retrieve the irq domain from the regmap
> irq_chip and pass it over to mfd_add_devices, the mfd core will create
> appropriate mappings".
Could be culture clash, but I found the question offensive which is
why I chose not to answer it. The reason is actually explained below:
"It's only the craziness in this patch which forced me to look into how
Regmap handles IRQs and come up with a subsequent solution which fits
your use-case."
Thus the fact that a) Regmap used IRQ domains and b) the IRQ domain
could be fetched and reused here didn't enter my thought process until
I delved into the inner workings of Regmap.
Yes, I know MFD pretty well, but I only tend to deep-dive into other
subsystems, particularly ones as complicated as Regmap, when it's
necessary to do so. Now I know a little more about it, I can provide
the feedback you suggest going forward.
> > > It's a pity it's not documented, I had to look at the code to find out
> > > irq resources would get translated in mfd_add_devices() if a domain is
> > > present.
> >
> > Where is it likely to be documented? MFD is pretty simple and seldom
> > needs explanation. A 3 second look at the API you're trying to use
> > (instead of blind copy & paste) would have told you that it's possible
> > to take an IRQ domain as an argument.
> >
> > It's only the craziness in this patch which forced me to look into how
> > Regmap handles IRQs and come up with a subsequent solution which fits
> > your use-case.
> >
> > > In that case - I really don't see a reason to create a superfluous
> > > structure to only hold the main regmap - we can very well get it from
> > > the parent device in sub-drivers as I do now.
> >
> > The whole point of this solution is that you don't need to pass
> > anything non-standard (i.e. not provided by the current APIs) to the
> > child device.
>
> I don't understand what you're saying here.
I'm saying that the structure you speak of is no longer required.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-13 10:15 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <20190213095330.GF1863@dell>
śr., 13 lut 2019 o 10:53 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
>
> > śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Lee Jones wrote:
> > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > > >
> > > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > > >
> > > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > > >
> > > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > > >
> > > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > > >
> > > > > > > > > > Need I go on? :)
> > > > > > > > > >
> > > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > > set some alarm bells ringing?
> > > > > > > > > >
> > > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > > >
> > > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > > >
> > > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > > >
> > > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > > are supposed to be passed (like everyone else does).
> > > > > > >
> > > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > > >
> > > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > > >
> > > > > Exampe:
> > > > >
> > > > > struct max77650_gpio_pdata {
> > > > > int gpi_irq;
> > > > > };
> > > > >
> > > > > In MFD driver:
> > > > >
> > > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > > >
> > > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > > >
> > > > > gpio_cell.platform_data = gpio_data;
> > > > >
> > > > > In GPIO driver:
> > > > >
> > > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > > >
> > > > > int irq = gpio_data->gpi_irq;
> > > >
> > > > Definitely not. What you're trying to do is a hack.
> > > >
> > > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > > in the client to pull them out. Setting them via Regmap, then pulling
> > > > them out again in the *same driver*, only to store them in platform
> > > > data to be passed to a child device is bonkers.
> > > >
> > > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > > handle them via the Regmap APIs, *not* both.
> > >
> > > Right, a plan has been formed.
> > >
> > > Hopefully this works and you can avoid all this dancing around.
> > >
> > > Firstly, you need to make a small change to:
> > >
> > > drivers/base/regmap/regmap-irq.c
> > >
> > > Add the following function:
> > >
> > > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
> >
> > We already do have such function and a lot of mfd drivers actually use it.
>
> Even better.
>
> > > As you can see, it will return the IRQ Domain for the chip.
> > >
> > > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > > remove all the nastiness in max77650_setup_irqs() and have the Input
> > > device use the standard (e.g. platform_get_irq()) APIs.
> > >
> > > How does that Sound?
> >
> > This does sound better! Why didn't you lead with that in the first place?
>
> I'm not even going to dignify that stupid question with a response.
>
It's not a stupid question and you're being unnecessarily rude. As an
expert in the subsystem you maintain you could have answered simply
with a "this is wrong, retrieve the irq domain from the regmap
irq_chip and pass it over to mfd_add_devices, the mfd core will create
appropriate mappings".
> > It's a pity it's not documented, I had to look at the code to find out
> > irq resources would get translated in mfd_add_devices() if a domain is
> > present.
>
> Where is it likely to be documented? MFD is pretty simple and seldom
> needs explanation. A 3 second look at the API you're trying to use
> (instead of blind copy & paste) would have told you that it's possible
> to take an IRQ domain as an argument.
>
> It's only the craziness in this patch which forced me to look into how
> Regmap handles IRQs and come up with a subsequent solution which fits
> your use-case.
>
> > In that case - I really don't see a reason to create a superfluous
> > structure to only hold the main regmap - we can very well get it from
> > the parent device in sub-drivers as I do now.
>
> The whole point of this solution is that you don't need to pass
> anything non-standard (i.e. not provided by the current APIs) to the
> child device.
>
I don't understand what you're saying here.
Bartosz
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-13 9:53 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <CAMRc=MeTMq_GWzQB6LBvts1csMftj=On5sB22jsJZzQzONxbNg@mail.gmail.com>
On Wed, 13 Feb 2019, Bartosz Golaszewski wrote:
> śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Lee Jones wrote:
> >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > > >
> > > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > > >
> > > > > > > > > * The declaration of a superfluous struct
> > > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > > >
> > > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > > >
> > > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > > >
> > > > > > > > > Need I go on? :)
> > > > > > > > >
> > > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > > set some alarm bells ringing?
> > > > > > > > >
> > > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > > >
> > > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > > >
> > > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > > >
> > > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > > are supposed to be passed (like everyone else does).
> > > > > >
> > > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > > >
> > > > > How do you plan on deriving the VIRQs to place into the struct?
> > > >
> > > > Exampe:
> > > >
> > > > struct max77650_gpio_pdata {
> > > > int gpi_irq;
> > > > };
> > > >
> > > > In MFD driver:
> > > >
> > > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > > >
> > > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > > >
> > > > gpio_cell.platform_data = gpio_data;
> > > >
> > > > In GPIO driver:
> > > >
> > > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > > >
> > > > int irq = gpio_data->gpi_irq;
> > >
> > > Definitely not. What you're trying to do is a hack.
> > >
> > > If you're using Regmap to handle your IRQs, then you should use Regmap
> > > in the client to pull them out. Setting them via Regmap, then pulling
> > > them out again in the *same driver*, only to store them in platform
> > > data to be passed to a child device is bonkers.
> > >
> > > *Either* use the MFD provided platform-data helpers *or* pass and
> > > handle them via the Regmap APIs, *not* both.
> >
> > Right, a plan has been formed.
> >
> > Hopefully this works and you can avoid all this dancing around.
> >
> > Firstly, you need to make a small change to:
> >
> > drivers/base/regmap/regmap-irq.c
> >
> > Add the following function:
> >
> > struct irq_domain *regmap_irq_get_domain(struct regmap *map)
>
> We already do have such function and a lot of mfd drivers actually use it.
Even better.
> > As you can see, it will return the IRQ Domain for the chip.
> >
> > You can then pass this IRQ domain to mfd_add_devices() and it will do
> > the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> > remove all the nastiness in max77650_setup_irqs() and have the Input
> > device use the standard (e.g. platform_get_irq()) APIs.
> >
> > How does that Sound?
>
> This does sound better! Why didn't you lead with that in the first place?
I'm not even going to dignify that stupid question with a response.
> It's a pity it's not documented, I had to look at the code to find out
> irq resources would get translated in mfd_add_devices() if a domain is
> present.
Where is it likely to be documented? MFD is pretty simple and seldom
needs explanation. A 3 second look at the API you're trying to use
(instead of blind copy & paste) would have told you that it's possible
to take an IRQ domain as an argument.
It's only the craziness in this patch which forced me to look into how
Regmap handles IRQs and come up with a subsequent solution which fits
your use-case.
> In that case - I really don't see a reason to create a superfluous
> structure to only hold the main regmap - we can very well get it from
> the parent device in sub-drivers as I do now.
The whole point of this solution is that you don't need to pass
anything non-standard (i.e. not provided by the current APIs) to the
child device.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-13 9:35 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, Mark Brown
In-Reply-To: <20190213092553.GE1863@dell>
śr., 13 lut 2019 o 10:25 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Lee Jones wrote:
>
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > > >
> > > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > > >
> > > > > > > > * The declaration of a superfluous struct
> > > > > > > > * 100 lines of additional/avoidable code
> > > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > > >
> > > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > > >
> > > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > > >
> > > > > > > > Need I go on? :)
> > > > > > > >
> > > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > > set some alarm bells ringing?
> > > > > > > >
> > > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > > >
> > > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > > >
> > > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > > the regmap_irq_chip_data to child drivers?
> > > > > >
> > > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > > are supposed to be passed (like everyone else does).
> > > > >
> > > > > I'm not sure what you mean by "like everyone else does" - different
> > > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > > containing virtual irq numbers passed to sub-drivers fine?
> > > >
> > > > How do you plan on deriving the VIRQs to place into the struct?
> > >
> > > Exampe:
> > >
> > > struct max77650_gpio_pdata {
> > > int gpi_irq;
> > > };
> > >
> > > In MFD driver:
> > >
> > > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> > >
> > > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> > >
> > > gpio_cell.platform_data = gpio_data;
> > >
> > > In GPIO driver:
> > >
> > > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> > >
> > > int irq = gpio_data->gpi_irq;
> >
> > Definitely not. What you're trying to do is a hack.
> >
> > If you're using Regmap to handle your IRQs, then you should use Regmap
> > in the client to pull them out. Setting them via Regmap, then pulling
> > them out again in the *same driver*, only to store them in platform
> > data to be passed to a child device is bonkers.
> >
> > *Either* use the MFD provided platform-data helpers *or* pass and
> > handle them via the Regmap APIs, *not* both.
>
> Right, a plan has been formed.
>
> Hopefully this works and you can avoid all this dancing around.
>
> Firstly, you need to make a small change to:
>
> drivers/base/regmap/regmap-irq.c
>
> Add the following function:
>
> struct irq_domain *regmap_irq_get_domain(struct regmap *map)
>
We already do have such function and a lot of mfd drivers actually use it.
> As you can see, it will return the IRQ Domain for the chip.
>
> You can then pass this IRQ domain to mfd_add_devices() and it will do
> the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
> remove all the nastiness in max77650_setup_irqs() and have the Input
> device use the standard (e.g. platform_get_irq()) APIs.
>
> How does that Sound?
>
This does sound better! Why didn't you lead with that in the first place?
It's a pity it's not documented, I had to look at the code to find out
irq resources would get translated in mfd_add_devices() if a domain is
present.
In that case - I really don't see a reason to create a superfluous
structure to only hold the main regmap - we can very well get it from
the parent device in sub-drivers as I do now.
Thanks,
Bartosz
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-13 9:25 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list, broonie
In-Reply-To: <20190212132016.GA4781@dell>
On Tue, 12 Feb 2019, Lee Jones wrote:
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > > >
> > > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > > >
> > > > > > > * The declaration of a superfluous struct
> > > > > > > * 100 lines of additional/avoidable code
> > > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > > >
> > > > > > While the above may be right, this one is negligible and you know it. :)
> > > > >
> > > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > > >
> > > > > > > Need I go on? :)
> > > > > > >
> > > > > > > Surely the fact that you are using both sides of an API
> > > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > > set some alarm bells ringing?
> > > > > > >
> > > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > > >
> > > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > > >
> > > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > > the regmap_irq_chip_data to child drivers?
> > > > >
> > > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > > are supposed to be passed (like everyone else does).
> > > >
> > > > I'm not sure what you mean by "like everyone else does" - different
> > > > mfd drivers seem to be doing different things. Is a simple struct
> > > > containing virtual irq numbers passed to sub-drivers fine?
> > >
> > > How do you plan on deriving the VIRQs to place into the struct?
> >
> > Exampe:
> >
> > struct max77650_gpio_pdata {
> > int gpi_irq;
> > };
> >
> > In MFD driver:
> >
> > struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
> >
> > gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
> >
> > gpio_cell.platform_data = gpio_data;
> >
> > In GPIO driver:
> >
> > struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
> >
> > int irq = gpio_data->gpi_irq;
>
> Definitely not. What you're trying to do is a hack.
>
> If you're using Regmap to handle your IRQs, then you should use Regmap
> in the client to pull them out. Setting them via Regmap, then pulling
> them out again in the *same driver*, only to store them in platform
> data to be passed to a child device is bonkers.
>
> *Either* use the MFD provided platform-data helpers *or* pass and
> handle them via the Regmap APIs, *not* both.
Right, a plan has been formed.
Hopefully this works and you can avoid all this dancing around.
Firstly, you need to make a small change to:
drivers/base/regmap/regmap-irq.c
Add the following function:
struct irq_domain *regmap_irq_get_domain(struct regmap *map)
As you can see, it will return the IRQ Domain for the chip.
You can then pass this IRQ domain to mfd_add_devices() and it will do
the HWIRQ => VIRQ mapping for you on the fly. Meaning that you can
remove all the nastiness in max77650_setup_irqs() and have the Input
device use the standard (e.g. platform_get_irq()) APIs.
How does that Sound?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH 12/13] input: max77650: add onkey support
From: Dmitry Torokhov @ 2019-02-13 7:30 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Mark Brown, Greg Kroah-Hartman, lkml, open list:GPIO SUBSYSTEM,
DTML, linux-input@vger.kernel.org, Linux LED Subsystem, Linux PM,
Bartosz Golaszewski
In-Reply-To: <20190212203408.GA1863@dell>
Hi Lee,
On Tue, Feb 12, 2019 at 12:34 PM Lee Jones <lee.jones@linaro.org> wrote:
>
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > Add support for the push- and slide-button events for max77650.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > ---
> > > drivers/input/misc/Kconfig | 9 ++
> > > drivers/input/misc/Makefile | 1 +
> > > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > > 3 files changed, 145 insertions(+)
> > > create mode 100644 drivers/input/misc/max77650-onkey.c
>
> [...]
>
> Moving things around a bit:
>
> > > +static int max77650_onkey_probe(struct platform_device *pdev)
> > > +{
>
> > > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > > + if (irq_f <= 0)
> > > + return -EINVAL;
> > > +
> > > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > > + if (irq_r <= 0)
> > > + return -EINVAL;
> >
> > Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> > passed it as resources of platform device. Then you'd simply call
> > platform_get_irq() here and did not have to reach into parent device for
> > "irq_dara".
>
> These device IRQs were defined and registered with the Regmap *set*
> (actually init()) APIs and thus should be pulled out using the
> appropriate reverse Regmap *get* APIs here in the device.
>
> Registering them with Regmap *and* pulling them back out again in the
> same (MFD in this case) driver, only to register them as platform data
> is certainly not how I see the API being designed/used.
>
> > > + struct regmap_irq_chip_data *irq_data;
> > > + struct device *dev, *parent;
>
> > > + dev = &pdev->dev;
> > > + parent = dev->parent;
> > > + i2c = to_i2c_client(parent);
> > > + irq_data = i2c_get_clientdata(i2c);
> > > +
> > > + map = dev_get_regmap(parent, NULL);
> > > + if (!map)
> > > + return -ENODEV;
>
> However, this hoop jumping is a bit crazy and for the most part
> superfluous. Instead, create a struct of attributes you wish to share
> with the child devices (containing both regmap (which you should call
> regmap and not map by the way) and irq_data) and pass it as either
> platform data (preferred) or as device data.
>
> If you choose the latter, you do not need to convert from 'device' to
> 'i2c' to do the look-up. Since this function (probe()) is provided
> with a platform_device, you can just use platform_get_drvdata() to
> achieve the same as above.
>
> If you go the preferred (platform data) route, then you should use
> dev_get_platdata() instead.
By doing what you are suggesting (introducing platform data) you
introducing strong dependency between MFD and input piece for no
different reason. With the current implementation the parent can be
reworked completely without involving onkey driver.
I find such clean separation desirable. We are trying to move away
form platform data where it makes sense.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 12/13] input: max77650: add onkey support
From: Lee Jones @ 2019-02-12 20:34 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Jacek Anaszewski, Pavel Machek, Sebastian Reichel, Liam Girdwood,
Mark Brown, Greg Kroah-Hartman, linux-kernel, linux-gpio,
devicetree, linux-input, linux-leds, linux-pm,
Bartosz Golaszewski
In-Reply-To: <20190119090318.GB187380@dtor-ws>
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Add support for the push- and slide-button events for max77650.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > ---
> > drivers/input/misc/Kconfig | 9 ++
> > drivers/input/misc/Makefile | 1 +
> > drivers/input/misc/max77650-onkey.c | 135 ++++++++++++++++++++++++++++
> > 3 files changed, 145 insertions(+)
> > create mode 100644 drivers/input/misc/max77650-onkey.c
[...]
Moving things around a bit:
> > +static int max77650_onkey_probe(struct platform_device *pdev)
> > +{
> > + irq_f = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_F);
> > + if (irq_f <= 0)
> > + return -EINVAL;
> > +
> > + irq_r = regmap_irq_get_virq(irq_data, MAX77650_INT_nEN_R);
> > + if (irq_r <= 0)
> > + return -EINVAL;
>
> Ugh, it would be better if you handled IRQ mapping in the MFD piece and
> passed it as resources of platform device. Then you'd simply call
> platform_get_irq() here and did not have to reach into parent device for
> "irq_dara".
These device IRQs were defined and registered with the Regmap *set*
(actually init()) APIs and thus should be pulled out using the
appropriate reverse Regmap *get* APIs here in the device.
Registering them with Regmap *and* pulling them back out again in the
same (MFD in this case) driver, only to register them as platform data
is certainly not how I see the API being designed/used.
> > + struct regmap_irq_chip_data *irq_data;
> > + struct device *dev, *parent;
> > + dev = &pdev->dev;
> > + parent = dev->parent;
> > + i2c = to_i2c_client(parent);
> > + irq_data = i2c_get_clientdata(i2c);
> > +
> > + map = dev_get_regmap(parent, NULL);
> > + if (!map)
> > + return -ENODEV;
However, this hoop jumping is a bit crazy and for the most part
superfluous. Instead, create a struct of attributes you wish to share
with the child devices (containing both regmap (which you should call
regmap and not map by the way) and irq_data) and pass it as either
platform data (preferred) or as device data.
If you choose the latter, you do not need to convert from 'device' to
'i2c' to do the look-up. Since this function (probe()) is provided
with a platform_device, you can just use platform_get_drvdata() to
achieve the same as above.
If you go the preferred (platform data) route, then you should use
dev_get_platdata() instead.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v1] Input: st-keyscan - fix potential zalloc NULL dereference
From: gabriel.fernandez @ 2019-02-12 15:30 UTC (permalink / raw)
To: Dmitry Torokhov, Gabriel Fernandez
Cc: Nate Drude, linux-kernel, linux-stm32, giuseppe.condorelli,
linux-input, Dan Carpenter, linux-clk, linux-arm-kernel
From: Gabriel Fernandez <gabriel.fernandez@st.com>
This patch fixes the following static checker warning:
drivers/input/keyboard/st-keyscan.c:156 keyscan_probe()
error: potential zalloc NULL dereference: 'keypad_data->input_dev'
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
---
drivers/input/keyboard/st-keyscan.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/st-keyscan.c b/drivers/input/keyboard/st-keyscan.c
index babcfb165e4f..3b85631fde91 100644
--- a/drivers/input/keyboard/st-keyscan.c
+++ b/drivers/input/keyboard/st-keyscan.c
@@ -153,6 +153,8 @@ static int keyscan_probe(struct platform_device *pdev)
input_dev->id.bustype = BUS_HOST;
+ keypad_data->input_dev = input_dev;
+
error = keypad_matrix_key_parse_dt(keypad_data);
if (error)
return error;
@@ -168,8 +170,6 @@ static int keyscan_probe(struct platform_device *pdev)
input_set_drvdata(input_dev, keypad_data);
- keypad_data->input_dev = input_dev;
-
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
keypad_data->base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(keypad_data->base))
--
2.17.0
^ permalink raw reply related
* Re: [PATCH v3 07/11] power: supply: max77650: add support for battery charger
From: Pavel Machek @ 2019-02-12 13:54 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=Mf4Mx2ojbpxvF+Qej3oMHdB8hBwtZ85A5+pTj_ZhBij3A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2721 bytes --]
On Tue 2019-02-12 14:31:52, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 13:07 Pavel Machek <pavel@ucw.cz> napisał(a):
> >
> > Hi!
> >
> > > +#define MAX77650_CHARGER_ENABLED BIT(0)
> > > +#define MAX77650_CHARGER_DISABLED 0x00
> > > +#define MAX77650_CHARGER_CHG_EN_MASK BIT(0)
> > > +
> > > +#define MAX77650_CHARGER_CHG_DTLS_MASK GENMASK(7, 4)
> > > +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> > > + (((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> > > +
> > > +#define MAX77650_CHARGER_CHG_OFF 0x00
> > > +#define MAX77650_CHARGER_CHG_PREQ 0x01
> > > +#define MAX77650_CHARGER_CHG_ON_CURR 0x02
> > > +#define MAX77650_CHARGER_CHG_ON_JCURR 0x03
> > > +#define MAX77650_CHARGER_CHG_ON_VOLT 0x04
> > > +#define MAX77650_CHARGER_CHG_ON_JVOLT 0x05
> > > +#define MAX77650_CHARGER_CHG_ON_TOPOFF 0x06
> > > +#define MAX77650_CHARGER_CHG_ON_JTOPOFF 0x07
> > > +#define MAX77650_CHARGER_CHG_DONE 0x08
> > > +#define MAX77650_CHARGER_CHG_JDONE 0x09
> > > +#define MAX77650_CHARGER_CHG_SUSP_PF 0x0a
> > > +#define MAX77650_CHARGER_CHG_SUSP_FCF 0x0b
> > > +#define MAX77650_CHARGER_CHG_SUSP_BTF 0x0c
> >
> > These are really bad define names. We are in charger driver, so
> > MAX77650_CHARGER_ really should be shortened/ommited. OTOH
> > more space should be given to "CHG_SUSP_BTF" as it is impossible to decipher.
> >
>
> I disagree about the prefix bit. I prefer to use common prefixes for
> all symbols in a driver. I would rather add comments to every
> definition here.
So what about shortened? (And what about UVL/OVL below?)
And... what you prefer is not quite important. It is important that
other people can read your code. And what "PF" is is
important... unlike MAX77650_CHARGER_ which is long but unimportant.
> > > +#define MAX77650_CHARGER_CHGIN_DTLS_MASK GENMASK(3, 2)
> > > +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> > > + (((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> > > +
> > > +#define MAX77650_CHARGER_CHGIN_UVL 0x00
> > > +#define MAX77650_CHARGER_CHGIN_OVL 0x01
> > > +#define MAX77650_CHARGER_CHGIN_OKAY 0x11
> >
> > UVL -> UNDERVOLTAGE, OVL -> OVERVOLTAGE?
> >
> >
> > --
> > (english) http://www.livejournal.com/~pavelmachek
> > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 10/11] input: max77650: add onkey support
From: Pavel Machek @ 2019-02-12 13:47 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=Md57e93vCV+uJ4WSma2AefdN89iWXCyKiY7yLsWFGUihg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]
On Tue 2019-02-12 14:29:49, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 13:17 Pavel Machek <pavel@ucw.cz> napisał(a):
> >
> > Hi!
> >
> > > + error = device_property_read_string(dev,
> > > + "maxim,onkey-mode", &mode_prop);
> > > + if (error)
> > > + mode_prop = "push";
> > > +
> > > + if (strcmp(mode_prop, "push") == 0)
> > > + mode = MAX77650_ONKEY_MODE_PUSH;
> > > + else if (strcmp(mode_prop, "slide") == 0)
> > > + mode = MAX77650_ONKEY_MODE_SLIDE;
> > > + else
> > > + return -EINVAL;
> > ...
> > > + onkey->input->name = "max77650_onkey";
> > > + onkey->input->phys = "max77650_onkey/input0";
> > > + onkey->input->id.bustype = BUS_I2C;
> > > + input_set_capability(onkey->input, EV_KEY, onkey->code);
> >
> > Is EV_KEY correct for mode==slide?
> >
>
> Others are not really documented. Does EV_SW stand for switch event?
> In that case it would probably be better.
I believe so.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Pavel Machek @ 2019-02-12 13:47 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <CAMRc=MdCXfPh6g4u-v4GkQJxNDA+v1dqRChcEHdauczrVywkDg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]
On Tue 2019-02-12 14:28:10, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 12:54 Pavel Machek <pavel@ucw.cz> napisał(a):
> >
> > Hi!
> >
> > > diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > > new file mode 100644
> > > index 000000000000..f3e00d41e299
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > > @@ -0,0 +1,27 @@
> > > +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> > > +
> > > +This module is part of the MAX77650 MFD device. For more details
> > > +see Documentation/devicetree/bindings/mfd/max77650.txt.
> > > +
> > > +The charger is represented as a sub-node of the PMIC node on the device tree.
> > > +
> > > +Required properties:
> > > +--------------------
> > > +- compatible: Must be "maxim,max77650-charger"
> > > +
> > > +Optional properties:
> > > +--------------------
> > > +- maxim,vchgin-min: Minimum CHGIN regulation voltage (in microvolts). Must be
> > > + one of: 4000000, 4100000, 4200000, 4300000, 4400000,
> > > + 4500000, 4600000, 4700000.
> > > +- maxim,ichgin-lim: CHGIN input current limit (in microamps). Must be one of:
> > > + 95000, 190000, 285000, 380000, 475000.
> >
> > We already have richtek,min-input-voltage-regulation and
> > ti,minimum-sys-voltage and ti,in-dpm-voltage.
> >
> > This is not too consistent with with the rest.. and perhaps common
> > name should be used for those?
> >
> > See also ti,current-limit. That seems to be direct match.
> >
>
> Well I can't really use a TI-specific property name on a maxim part, can I?
Well, I'm saying that you should introduce common names for common
stuff. "current-limit" sounds like reasonable name.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 07/11] power: supply: max77650: add support for battery charger
From: Bartosz Golaszewski @ 2019-02-12 13:31 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212120742.GA25613@amd>
wt., 12 lut 2019 o 13:07 Pavel Machek <pavel@ucw.cz> napisał(a):
>
> Hi!
>
> > +#define MAX77650_CHARGER_ENABLED BIT(0)
> > +#define MAX77650_CHARGER_DISABLED 0x00
> > +#define MAX77650_CHARGER_CHG_EN_MASK BIT(0)
> > +
> > +#define MAX77650_CHARGER_CHG_DTLS_MASK GENMASK(7, 4)
> > +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> > + (((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> > +
> > +#define MAX77650_CHARGER_CHG_OFF 0x00
> > +#define MAX77650_CHARGER_CHG_PREQ 0x01
> > +#define MAX77650_CHARGER_CHG_ON_CURR 0x02
> > +#define MAX77650_CHARGER_CHG_ON_JCURR 0x03
> > +#define MAX77650_CHARGER_CHG_ON_VOLT 0x04
> > +#define MAX77650_CHARGER_CHG_ON_JVOLT 0x05
> > +#define MAX77650_CHARGER_CHG_ON_TOPOFF 0x06
> > +#define MAX77650_CHARGER_CHG_ON_JTOPOFF 0x07
> > +#define MAX77650_CHARGER_CHG_DONE 0x08
> > +#define MAX77650_CHARGER_CHG_JDONE 0x09
> > +#define MAX77650_CHARGER_CHG_SUSP_PF 0x0a
> > +#define MAX77650_CHARGER_CHG_SUSP_FCF 0x0b
> > +#define MAX77650_CHARGER_CHG_SUSP_BTF 0x0c
>
> These are really bad define names. We are in charger driver, so
> MAX77650_CHARGER_ really should be shortened/ommited. OTOH
> more space should be given to "CHG_SUSP_BTF" as it is impossible to decipher.
>
I disagree about the prefix bit. I prefer to use common prefixes for
all symbols in a driver. I would rather add comments to every
definition here.
Bart
> > +#define MAX77650_CHARGER_CHGIN_DTLS_MASK GENMASK(3, 2)
> > +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> > + (((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> > +
> > +#define MAX77650_CHARGER_CHGIN_UVL 0x00
> > +#define MAX77650_CHARGER_CHGIN_OVL 0x01
> > +#define MAX77650_CHARGER_CHGIN_OKAY 0x11
>
> UVL -> UNDERVOLTAGE, OVL -> OVERVOLTAGE?
>
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v3 10/11] input: max77650: add onkey support
From: Bartosz Golaszewski @ 2019-02-12 13:29 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212121745.GE25613@amd>
wt., 12 lut 2019 o 13:17 Pavel Machek <pavel@ucw.cz> napisał(a):
>
> Hi!
>
> > + error = device_property_read_string(dev,
> > + "maxim,onkey-mode", &mode_prop);
> > + if (error)
> > + mode_prop = "push";
> > +
> > + if (strcmp(mode_prop, "push") == 0)
> > + mode = MAX77650_ONKEY_MODE_PUSH;
> > + else if (strcmp(mode_prop, "slide") == 0)
> > + mode = MAX77650_ONKEY_MODE_SLIDE;
> > + else
> > + return -EINVAL;
> ...
> > + onkey->input->name = "max77650_onkey";
> > + onkey->input->phys = "max77650_onkey/input0";
> > + onkey->input->id.bustype = BUS_I2C;
> > + input_set_capability(onkey->input, EV_KEY, onkey->code);
>
> Is EV_KEY correct for mode==slide?
>
Others are not really documented. Does EV_SW stand for switch event?
In that case it would probably be better.
Bart
^ permalink raw reply
* Re: [PATCH v3 02/11] dt-bindings: power: supply: add DT bindings for max77650
From: Bartosz Golaszewski @ 2019-02-12 13:28 UTC (permalink / raw)
To: Pavel Machek
Cc: Rob Herring, Mark Rutland, Linus Walleij, Dmitry Torokhov,
Jacek Anaszewski, Lee Jones, Sebastian Reichel, Liam Girdwood,
Greg Kroah-Hartman, Linux Kernel Mailing List,
open list:GPIO SUBSYSTEM, devicetree, Linux Input,
Linux LED Subsystem, Linux PM list, Bartosz Golaszewski
In-Reply-To: <20190212115420.GA24296@amd>
wt., 12 lut 2019 o 12:54 Pavel Machek <pavel@ucw.cz> napisał(a):
>
> Hi!
>
> > diff --git a/Documentation/devicetree/bindings/power/supply/max77650-charger.txt b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > new file mode 100644
> > index 000000000000..f3e00d41e299
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/max77650-charger.txt
> > @@ -0,0 +1,27 @@
> > +Battery charger driver for MAX77650 PMIC from Maxim Integrated.
> > +
> > +This module is part of the MAX77650 MFD device. For more details
> > +see Documentation/devicetree/bindings/mfd/max77650.txt.
> > +
> > +The charger is represented as a sub-node of the PMIC node on the device tree.
> > +
> > +Required properties:
> > +--------------------
> > +- compatible: Must be "maxim,max77650-charger"
> > +
> > +Optional properties:
> > +--------------------
> > +- maxim,vchgin-min: Minimum CHGIN regulation voltage (in microvolts). Must be
> > + one of: 4000000, 4100000, 4200000, 4300000, 4400000,
> > + 4500000, 4600000, 4700000.
> > +- maxim,ichgin-lim: CHGIN input current limit (in microamps). Must be one of:
> > + 95000, 190000, 285000, 380000, 475000.
>
> We already have richtek,min-input-voltage-regulation and
> ti,minimum-sys-voltage and ti,in-dpm-voltage.
>
> This is not too consistent with with the rest.. and perhaps common
> name should be used for those?
>
> See also ti,current-limit. That seems to be direct match.
>
Well I can't really use a TI-specific property name on a maxim part, can I?
Bart
PS This version was superseded by v4.
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Lee Jones @ 2019-02-12 13:20 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list
In-Reply-To: <CAMpxmJWXOb1HW4QiEn8gMp3Nh05ahE0CMLXkac+hEKRjtGxTHQ@mail.gmail.com>
On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
> >
> > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> >
> > > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > >
> > > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > > >
> > > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > > >
> > > > > > * The declaration of a superfluous struct
> > > > > > * 100 lines of additional/avoidable code
> > > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > > >
> > > > > While the above may be right, this one is negligible and you know it. :)
> > > >
> > > > You have nested for() loops. You *are* wasting lots of cycles.
> > > >
> > > > > > Need I go on? :)
> > > > > >
> > > > > > Surely the fact that you are using both sides of an API
> > > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > > set some alarm bells ringing?
> > > > > >
> > > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > > >
> > > > > > And for what? To avoid passing IRQ data to a child driver?
> > > > >
> > > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > > the regmap_irq_chip_data to child drivers?
> > > >
> > > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > > are supposed to be passed (like everyone else does).
> > >
> > > I'm not sure what you mean by "like everyone else does" - different
> > > mfd drivers seem to be doing different things. Is a simple struct
> > > containing virtual irq numbers passed to sub-drivers fine?
> >
> > How do you plan on deriving the VIRQs to place into the struct?
>
> Exampe:
>
> struct max77650_gpio_pdata {
> int gpi_irq;
> };
>
> In MFD driver:
>
> struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
>
> gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
>
> gpio_cell.platform_data = gpio_data;
>
> In GPIO driver:
>
> struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
>
> int irq = gpio_data->gpi_irq;
Definitely not. What you're trying to do is a hack.
If you're using Regmap to handle your IRQs, then you should use Regmap
in the client to pull them out. Setting them via Regmap, then pulling
them out again in the *same driver*, only to store them in platform
data to be passed to a child device is bonkers.
*Either* use the MFD provided platform-data helpers *or* pass and
handle them via the Regmap APIs, *not* both.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 05/10] mfd: max77650: new core mfd driver
From: Bartosz Golaszewski @ 2019-02-12 12:29 UTC (permalink / raw)
To: Lee Jones
Cc: Bartosz Golaszewski, Rob Herring, Mark Rutland, Linus Walleij,
Dmitry Torokhov, Jacek Anaszewski, Pavel Machek,
Sebastian Reichel, Liam Girdwood, Greg Kroah-Hartman,
Linux Kernel Mailing List, open list:GPIO SUBSYSTEM, devicetree,
Linux Input, Linux LED Subsystem, Linux PM list
In-Reply-To: <20190212111403.GC20638@dell>
wt., 12 lut 2019 o 12:14 Lee Jones <lee.jones@linaro.org> napisał(a):
>
> On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
>
> > wt., 12 lut 2019 o 11:18 Lee Jones <lee.jones@linaro.org> napisał(a):
> > >
> > > On Tue, 12 Feb 2019, Bartosz Golaszewski wrote:
> > >
> > > > wt., 12 lut 2019 o 10:55 Lee Jones <lee.jones@linaro.org> napisał(a):
> > > > >
> > > > > * The declaration of a superfluous struct
> > > > > * 100 lines of additional/avoidable code
> > > > > * Hacky hoop jumping trying to fudge VIRQs into resources
> > > > > * Resources were designed for HWIRQs (unless a domain is present)
> > > > > * Loads of additional/avoidable CPU cycles setting all this up
> > > >
> > > > While the above may be right, this one is negligible and you know it. :)
> > >
> > > You have nested for() loops. You *are* wasting lots of cycles.
> > >
> > > > > Need I go on? :)
> > > > >
> > > > > Surely the fact that you are using both sides of an API
> > > > > (devm_regmap_init_i2c and regmap_irq_get_*) in the same driver, must
> > > > > set some alarm bells ringing?
> > > > >
> > > > > This whole HWIRQ setting, VIRQ getting, resource hacking is a mess.
> > > > >
> > > > > And for what? To avoid passing IRQ data to a child driver?
> > > >
> > > > What do you propose? Should I go back to the approach in v1 and pass
> > > > the regmap_irq_chip_data to child drivers?
> > >
> > > I'm saying you should remove all of this hackery and pass IRQs as they
> > > are supposed to be passed (like everyone else does).
> >
> > I'm not sure what you mean by "like everyone else does" - different
> > mfd drivers seem to be doing different things. Is a simple struct
> > containing virtual irq numbers passed to sub-drivers fine?
>
> How do you plan on deriving the VIRQs to place into the struct?
Exampe:
struct max77650_gpio_pdata {
int gpi_irq;
};
In MFD driver:
struct max77650_gpio_pdata *gpio_data = devm_kmalloc(dev, sizeof(*gpio_data));
gpio_data->gpi_irq = regmap_irq_get_virq(irqchip_data, GPI_NUM);
gpio_cell.platform_data = gpio_data;
In GPIO driver:
struct max77650_gpio_pdata *gpio_data = pdev->dev.platform_data;
int irq = gpio_data->gpi_irq;
Bart
^ permalink raw reply
* Re: [PATCH v3 10/11] input: max77650: add onkey support
From: Pavel Machek @ 2019-02-12 12:17 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-11-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 729 bytes --]
Hi!
> + error = device_property_read_string(dev,
> + "maxim,onkey-mode", &mode_prop);
> + if (error)
> + mode_prop = "push";
> +
> + if (strcmp(mode_prop, "push") == 0)
> + mode = MAX77650_ONKEY_MODE_PUSH;
> + else if (strcmp(mode_prop, "slide") == 0)
> + mode = MAX77650_ONKEY_MODE_SLIDE;
> + else
> + return -EINVAL;
...
> + onkey->input->name = "max77650_onkey";
> + onkey->input->phys = "max77650_onkey/input0";
> + onkey->input->id.bustype = BUS_I2C;
> + input_set_capability(onkey->input, EV_KEY, onkey->code);
Is EV_KEY correct for mode==slide?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 09/11] leds: max77650: add LEDs support
From: Pavel Machek @ 2019-02-12 12:12 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-10-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 705 bytes --]
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index a72f97fca57b..6e7a8f51eccc 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -608,6 +608,12 @@ config LEDS_TLC591XX
> This option enables support for Texas Instruments TLC59108
> and TLC59116 LED controllers.
>
> +config LEDS_MAX77650
> + tristate "LED support for Maxim MAX77650 PMIC"
> + depends on MFD_MAX77650
> + help
> + LEDs driver for MAX77650 family of PMICs from Maxim Integrated."
> +
List the chips? Remove extra "?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 09/11] leds: max77650: add LEDs support
From: Pavel Machek @ 2019-02-12 12:10 UTC (permalink / raw)
To: Dan Murphy
Cc: Jacek Anaszewski, Bartosz Golaszewski, Rob Herring, Mark Rutland,
Linus Walleij, Dmitry Torokhov, 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: <d86adc21-627f-0350-72e0-f6f10b023413@ti.com>
[-- Attachment #1: Type: text/plain, Size: 748 bytes --]
Hi!
> >> +static struct max77650_led *max77650_to_led(struct led_classdev *cdev)
> >> +{
> >> + return container_of(cdev, struct max77650_led, cdev);
> >> +}
> >> +
> >> +static int max77650_led_brightness_set(struct led_classdev *cdev,
> >> + enum led_brightness brightness)
> >> +{
> >> + struct max77650_led *led = max77650_to_led(cdev);
> >> + int val, mask;
> >> +
>
> The register value and bits are only 8 bit why an int?
int is word size CPU prefers. It is ok here. If you force u8, it may
actually result in slower/bigger code.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 07/11] power: supply: max77650: add support for battery charger
From: Pavel Machek @ 2019-02-12 12:08 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-8-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 869 bytes --]
On Fri 2019-02-01 10:47:32, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Add basic support for the battery charger for max77650 PMIC.
>
> 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.
> +
If available, pointer to a datasheet would be welcome 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 07/11] power: supply: max77650: add support for battery charger
From: Pavel Machek @ 2019-02-12 12:07 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-8-brgl@bgdev.pl>
[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]
Hi!
> +#define MAX77650_CHARGER_ENABLED BIT(0)
> +#define MAX77650_CHARGER_DISABLED 0x00
> +#define MAX77650_CHARGER_CHG_EN_MASK BIT(0)
> +
> +#define MAX77650_CHARGER_CHG_DTLS_MASK GENMASK(7, 4)
> +#define MAX77650_CHARGER_CHG_DTLS_BITS(_reg) \
> + (((_reg) & MAX77650_CHARGER_CHG_DTLS_MASK) >> 4)
> +
> +#define MAX77650_CHARGER_CHG_OFF 0x00
> +#define MAX77650_CHARGER_CHG_PREQ 0x01
> +#define MAX77650_CHARGER_CHG_ON_CURR 0x02
> +#define MAX77650_CHARGER_CHG_ON_JCURR 0x03
> +#define MAX77650_CHARGER_CHG_ON_VOLT 0x04
> +#define MAX77650_CHARGER_CHG_ON_JVOLT 0x05
> +#define MAX77650_CHARGER_CHG_ON_TOPOFF 0x06
> +#define MAX77650_CHARGER_CHG_ON_JTOPOFF 0x07
> +#define MAX77650_CHARGER_CHG_DONE 0x08
> +#define MAX77650_CHARGER_CHG_JDONE 0x09
> +#define MAX77650_CHARGER_CHG_SUSP_PF 0x0a
> +#define MAX77650_CHARGER_CHG_SUSP_FCF 0x0b
> +#define MAX77650_CHARGER_CHG_SUSP_BTF 0x0c
These are really bad define names. We are in charger driver, so
MAX77650_CHARGER_ really should be shortened/ommited. OTOH
more space should be given to "CHG_SUSP_BTF" as it is impossible to decipher.
> +#define MAX77650_CHARGER_CHGIN_DTLS_MASK GENMASK(3, 2)
> +#define MAX77650_CHARGER_CHGIN_DTLS_BITS(_reg) \
> + (((_reg) & MAX77650_CHARGER_CHGIN_DTLS_MASK) >> 2)
> +
> +#define MAX77650_CHARGER_CHGIN_UVL 0x00
> +#define MAX77650_CHARGER_CHGIN_OVL 0x01
> +#define MAX77650_CHARGER_CHGIN_OKAY 0x11
UVL -> UNDERVOLTAGE, OVL -> OVERVOLTAGE?
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* 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
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