Re: [PATCH v7 2/9] reboot: thermal: Export hardware protection shutdown
On Sat, 2021-04-17 at 06:57 +0200, Daniel Lezcano wrote: > On 14/04/2021 07:52, Matti Vaittinen wrote: > > Thermal core contains a logic for safety shutdown. System is > > attempted to > > be powered off if temperature exceeds safety limits. > > > > Currently this can be also utilized by regulator subsystem as a > > final > > protection measure if PMICs report dangerous over-voltage, over- > > current or > > over-temperature and if per regulator counter measures fail or do > > not > > exist. > > > > Move this logic to kernel/reboot.c and export the functionality for > > other > > subsystems to use. Also replace the mutex with a spinlock to allow > > using > > the function from any context. > > > > Also the EMIF bus code has implemented a safety shut-down. EMIF > > does not > > attempt orderly_poweroff at all. Thus the EMIF code is not > > converted to use > > this new function. > > > > Signed-off-by: Matti Vaittinen > > --- > > Changelog > > v7: > > - new patch > > > > Please note - this patch has received only a minimal amount of > > testing. > > (The new API call was tested to shut-down my system at driver probe > > but > > no odd corner-cases have been tested). > > > > Any testing for thermal shutdown is appreciated. > > You can test it easily by enabling the option > CONFIG_THERMAL_EMULATION > > Then in any thermal zone: > > Assuming the critical temp is below the one specified in the command: > > echo 10 > /sys/class/thermal/thermal_zone0/emul_temp > Thanks Daniel, I will see how that works when I create the next version :) Best Regards Matti Vaittinen
Re: [GIT PULL] Immutable branch between MFD, Clock, GPIO, Regulator and RTC due for the v5.13 merge window
Hello Lee, Mark, Stephen, Linus, Alexandre, On Wed, 2021-04-14 at 15:53 +0100, Lee Jones wrote: > On Wed, 14 Apr 2021, Lee Jones wrote: > > > Please note that this PR will break your build unless you have the > > required Regulator API update. > > > > fb8fee9efdcf0 regulator: Add regmap helper for ramp-delay setting > > e3baacf542756 regulator: helpers: Export helper voltage listing > > Looks like Mark has these: > > git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator > regulator-list-ramp-helpers > > > Pull at your peril! :) > > > > The following changes since commit > > a38fd8748464831584a19438cbb3082b5a2dab15: > > > > Linux 5.12-rc2 (2021-03-05 17:33:41 -0800) > > > > are available in the Git repository at: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git > > tags/ib-mfd-clk-gpio-regulator-rtc-v5.13 > > > > for you to fetch changes up to > > 5a8a64d9a38b9d3794f9f5e153fc0358b858cc24: > > > > MAINTAINERS: Add ROHM BD71815AGW (2021-04-14 10:21:43 +0100) > > > > > > Immutable branch between MFD, Clock, GPIO, Regulator and RTC due > > for the v5.13 merge window > > > > > > Matti Vaittinen (16): > > rtc: bd70528: Do not require parent data > > mfd: bd718x7: simplify by cleaning unnecessary device data > > dt_bindings: bd71828: Add clock output mode > > dt_bindings: regulator: Add ROHM BD71815 PMIC regulators > > dt_bindings: mfd: Add ROHM BD71815 PMIC > > mfd: Add ROHM BD71815 ID > > mfd: Sort ROHM chip ID list for better readability > > mfd: Support for ROHM BD71815 PMIC core > > gpio: Support ROHM BD71815 GPOs > > regulator: rohm-regulator: linear voltage support > > regulator: rohm-regulator: Support SNVS HW state. > > regulator: bd718x7, bd71828: Use ramp-delay helper > > regulator: Support ROHM BD71815 regulators > > clk: bd718x7: Add support for clk gate on ROHM BD71815 PMIC > > rtc: bd70528: Support RTC on ROHM BD71815 > > MAINTAINERS: Add ROHM BD71815AGW I think the original idea was that Lee could get the Tag from Mark and then get all the changes in via MFD tree. I can't say what would be the best way to get these in. I'm open to all suggestions :) Best Regards Matti Vaittinen
Re: [RFC PATCH v3 1/3] regmap-irq: Extend sub-irq to support non-fixed reg strides
Hi,
On Mon, 2021-04-12 at 09:05 +0300, Matti Vaittinen wrote:
> Hi All,
>
> On Wed, 2021-03-10 at 16:39 -0800, Guru Das Srinagesh wrote:
> > Qualcomm's MFD chips have a top level interrupt status register and
> > sub-irqs (peripherals). When a bit in the main status register
> > goes
> > high, it means that the peripheral corresponding to that bit has an
> > unserviced interrupt. If the bit is not set, this means that the
> > corresponding peripheral does not.
> >
> > Commit a2d21848d9211d ("regmap: regmap-irq: Add main status
> > register
> > support") introduced the sub-irq logic that is currently applied
> > only
> > when reading status registers, but not for any other functions like
> > acking
> > or masking. Extend the use of sub-irq to all other functions, with
> > two
> > caveats regarding the specification of offsets:
> >
> > - Each member of the sub_reg_offsets array should be of length 1
> > - The specified offsets should be the unequal strides for each sub-
> > irq
> > device.
> >
> > In QCOM's case, all the *_base registers are to be configured to
> > the
> > base addresses of the first sub-irq group, with offsets of each
> > subsequent group calculated as a difference from these addresses.
> >
> > Continuing from the example mentioned in the cover letter:
> >
> > /*
> > * Address of MISC_INT_MASK = 0x1011
> > * Address of TEMP_ALARM_INT_MASK = 0x2011
> > * Address of GPIO01_INT_MASK = 0x3011
> > *
> > * Calculate offsets as:
> > * offset_0 = 0x1011 - 0x1011 = 0 (to access MISC's
> > * registers)
> > * offset_1 = 0x2011 - 0x1011 = 0x1000
> > * offset_2 = 0x3011 - 0x1011 = 0x2000
> > */
> >
> > static unsigned int sub_unit0_offsets[] = {0};
> > static unsigned int sub_unit1_offsets[] = {0x1000};
> > static unsigned int sub_unit2_offsets[] = {0x2000};
> >
> > static struct regmap_irq_sub_irq_map chip_sub_irq_offsets[] = {
> > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > REGMAP_IRQ_MAIN_REG_OFFSET(sub_unit0_offsets),
> > };
> >
> > static struct regmap_irq_chip chip_irq_chip = {
> > 8<
> > .not_fixed_stride = true,
> > .mask_base= MISC_INT_MASK,
> > .type_base= MISC_INT_TYPE,
> > .ack_base = MISC_INT_ACK,
> > .sub_reg_offsets = chip_sub_irq_offsets,
> > 8<
> > };
> >
> > Signed-off-by: Guru Das Srinagesh
> > ---
> > drivers/base/regmap/regmap-irq.c | 81 ++--
> >
> > include/linux/regmap.h | 7
> > 2 files changed, 60 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/base/regmap/regmap-irq.c
> > b/drivers/base/regmap/regmap-irq.c
> > index 19db764..e1d8fc9e 100644
> > --- a/drivers/base/regmap/regmap-irq.c
> > +++ b/drivers/base/regmap/regmap-irq.c
> > @@ -45,6 +45,27 @@ struct regmap_irq_chip_data {
> > bool clear_status:1;
> > };
> >
>
> Sorry that I am late with the "review" but I only now noticed this
> change when I was following the references from PM8008 PMIC patch
> mail.
>
>
> >
> > +static int sub_irq_reg(struct regmap_irq_chip_data *data,
> > + unsigned int base_reg, int i)
>
> Do I read this correctly - this function should map the main status
> bit
> (given in i) to the (sub)IRQ register, right? How does this work for
> cases where one bit corresponds to more than one sub-register? Or do
> I
> misunderstand the purpose of this function? (This is the case with
> both
> the BD70528 and BD71828).
I did some quick test with BD71815 which I had at home. And it seems to
be I did indeed misunderstand this :) This is not for converting the
main-IRQ bits to sub-irq registers - this is for getting the sub-IRQ
register address based on the 'sub IRQ register index'.
So I do apologize the noise, it seems all is well and everything
(except my self confidence) keeps working as it did :)
Thanks for the improvement Guru Das!
Best Regards
Matti Vaittinen
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
On Thu, 2021-04-08 at 20:20 -0700, Kees Cook wrote: > On Wed, Apr 07, 2021 at 03:50:15PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 7, 2021 at 12:49 PM Vaittinen, Matti > > wrote: > > > On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen > > > > wrote: > > > > > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote: > > > > > > On Tuesday, April 6, 2021, Matti Vaittinen < > > > > > > matti.vaitti...@fi.rohmeurope.com> wrote: > > > > > > > + BUG(); > > > > > > > +} > > This, though, are you sure you want to use BUG()? Linus gets upset > about > such things: > https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on > I see. I am unsure of what would be the best action in the regulator case we are handling here. To give the context, we assume here a situation where power has gone out of regulation and the hardware is probably failing. First countermeasure to protect what is left of HW is to shut-down the failing regulator. BUG() was called here as a last resort if shutting the power via regulator interface was not implemented or working. Eg, we try to take what ever last measure we can to minimize the HW damage - and BUG() was used for this in the qcom driver where I stole the idea. Judging the comment related to BUG() in asm-generic/bug.h /* * Don't use BUG() or BUG_ON() unless there's really no way out; one * example might be detecting data structure corruption in the middle * of an operation that can't be backed out of. If the (sub)system * can somehow continue operating, perhaps with reduced functionality, * it's probably not BUG-worthy. * * If you're tempted to BUG(), think again: is completely giving up * really the *only* solution? There are usually better options, where * users don't need to reboot ASAP and can mostly shut down cleanly. */ https://elixir.bootlin.com/linux/v5.12-rc6/source/include/asm-generic/bug.h#L55 this really might be valid use-case. To me the real question is what happens after the BUG() - and if there is any generic handling or if it is platform/board specific? Does it actually have any chance to save the HW? Mark already pointed that we might need to figure a way to punt a "failing event" to the user-space to initiate better "safety shutdown". Such event does not currently exist so I think the main use-case here is to do logging and potentially prevent enabling any further actions in the failing HW. So - any better suggestions? Best Regards Matti Vaittinen
Re: [PATCH v6 3/8] regulator: IRQ based event/error notification helpers
On Thu, 2021-04-08 at 12:58 +0300, Andy Shevchenko wrote: > On Thu, Apr 8, 2021 at 11:21 AM Matti Vaittinen > wrote: > > Hello Andy, > > > > On Wed, 2021-04-07 at 16:21 +0300, Andy Shevchenko wrote: > > > On Wed, Apr 7, 2021 at 1:04 PM Matti Vaittinen > > > wrote: > > > > Provide helper function for IC's implementing regulator > > > > notifications > > > > when an IRQ fires. The helper also works for IRQs which can not > > > > be > > > > acked. > > > > Helper can be set to disable the IRQ at handler and then re- > > > > enabling it > > > > on delayed work later. The helper also adds > > > > regulator_get_error_flags() > > > > errors in cache for the duration of IRQ disabling. > > > > > > Thanks for an update, my comments below. After addressing them, > > > feel > > > free to add > > > Reviewed-by: Andy Shevchenko > > > > I (eventually) disagreed with couple of points here and haven't > > changed > > those. Please see list below. > > > > I still do think you did a really good job reviewing this - and you > > should get the recognition from that work. Thus I'd nevertheless > > would > > like to add your Reviewed-by to the next version. Please let me > > know if > > you think it's ok. (I have the v7 ready but I'll wait until the > > next > > Monday before sending it to see if this brings more discussion). > > Looks OK to me. > Just rename die_loudly() to rdev_die_loudly() I did that. Thanks. > and in any way of > conditionals with that, please mark it with __noreturn attribute, so > if (bla bla bla) > rdev_die_loudly(); > > will be an understandable trap. Valid point. Will do, thanks again. Best Regards Matti Vaittinen
Re: [PATCH v4 3/7] regulator: IRQ based event/error notification helpers
Hello Andy,
On Wed, 2021-04-07 at 12:10 +0300, Andy Shevchenko wrote:
> On Wed, Apr 7, 2021 at 8:02 AM Matti Vaittinen
> wrote:
> > On Wed, 2021-04-07 at 01:44 +0300, Andy Shevchenko wrote:
> > > On Tuesday, April 6, 2021, Matti Vaittinen <
> > > matti.vaitti...@fi.rohmeurope.com> wrote:
>
> ...
>
> > > > + pr_emerg(msg);
> > >
> > > Oh là là, besides build bot complaints, this has serious security
> > > implications. Never do like this.
> >
> > I'm not even trying to claim that was correct. And I did send a
> > fixup -
> > sorry for this. I don't intend to do this again.
> >
> > Now, when this is said - If you have a minute, please educate me.
> > Assuming we know all the callers and that all the callers use this
> > as
> >
> > die_loudly("foobarfoo\n");
> > - what is the exploit mechanism?
>
> Not a security guy, but my understanding is that this code may be
> used
> as a gadget in ROP technique of attacks.
Thanks Andy. It'd be interesting to learn more details as I am not a
security expert either :)
> In that case msg can be anything. On top of that, somebody may
> mistakenly (inadvertently) put the code that allows user controller
> input to go to this path.
Yes. This is a good reason to not to do this - but I was interested in
knowing if there is a potential risk even if:
> > all the callers use this
> > as
> >
> > die_loudly("foobarfoo\n");
> And last but not least, that some newbies might copy'n'paste bad
> examples where they will expose security breach.
Yes yes. As I said, I am not trying to say it is Ok. I was just
wondering what are the risks if users of the print function were known.
> With the modern world of Spectre, rowhammer, and other side channel
> attacks I may believe that one may exhaust the regulator for getting
> advantage on an attack vector.
>
> But again, not a security guy here.
Thanks anyways :)
>
> > > > + BUG();
> > > > +}
> > > > +
>
> ...
>
> > > > errors will be
> > > > + * or'ed with common erros. If this is
> > > > given
>
> errors ?
Thanks. I didn't first spot the typo even though you pointed it to me.
Luckily my evolution has occasional problems when communicating with
the mail server. I had enough time to hit the cancel before sending out
a message where I wondered how I should clarify this :]
> ...
>
> > > > + if (irq <= 0) {
> > > > + dev_err(dev, "No IRQ\n");
> > > > + return ERR_PTR(-EINVAL);
> > >
> > > Why shadowing error code? Negative IRQ is anything but “no IRQ”.
> >
> > This was a good point. The irq is passed here as parameter. From
> > this
> > function's perspective the negative irq is invalid parameter - we
> > don't
> > know how the caller has obtained it. Print could show the value
> > contained in irq though.
> > Now that you pointed this out I am unsure if this check is needed
> > here.
> > If we check it, then I still think we should report -EINVAL for
> > invalid
> > parameter. Other option is to just call the request_threaded_irq()
> > -
> > log the IRQ request failure and return what request_threaded_irq()
> > returns. Do you think that would make sense?
>
> Why is the parameter signed type then?
> Shouldn't the caller take care of it?
>
> Otherwise, what is the difference between passing negative IRQ to
> request_irq() call?
> As you said, you shouldn't make assumptions about what caller meant
> by this.
>
> So, I would simply drop the check (from easiness of the code
> perspective).
Yep. I was going to drop the check. Good point. Thanks.
I'll send v6 shortly to address the issues you spotted Andy. Thanks.
>
> ...
>
> > > > +void regulator_irq_helper_cancel(void **handle)
> > > > +{
> > > > + if (handle && *handle) {
> > >
> > > Can handle ever be NULL here ? (Yes, I understand that you export
> > > this)
> >
> > To tell the truth - I am not sure. I *guess* that if we allow this
> > to
> > be NULL, then one *could* implement a driver for IC where IRQs are
> > optional, in a way that when IRQs are supported the pointer to
> > handle
> > is valid, when IRQs aren't supported the pointer is NULL. (Why) do
> > you
> > think we should skip the check?
>
> Just my guts feeling. I don't remember that I ever saw checks like
> this for indirect pointers.
> Of course it doesn't mean there are no such checks present or may be
> present.
I think I'll keep the check unless there is some reason why it should
be omitted.
> > > Why not to use devm_add_action{_or_reset}()?
> >
> > I just followed the same approach that has been used in other
> > regulator
> > functions. (drivers/regulator/devres.c)
> > OTOH, the devm_add_action makes this little bit simpler so I'll
> > convert
> > to use it.
> >
> > Mark, do you have a reason of not using devm_add_action() in
> > devres.c?
> > Should devm_add_action() be used in some other functions there? And
> > should this be moved to devres.c?
>
> I think the reason for this is as simple as a historical one,
Re: [PATCH v5 00/19] Support ROHM BD71815 PMIC
On Fri, 2021-04-02 at 20:19 +0100, Mark Brown wrote: > On Tue, Mar 30, 2021 at 11:06:53AM +0000, Vaittinen, Matti wrote: > > > Do you think Lee could merge other but the regulator parts to MFD > > if > > Mark is busy? I'd like to be able to squeeze the amount of patches > > and > > recipients for future iterations. It might be easier to work > > directly > > on regulator tree if regulator part gets delayed to next cycle. (I > > do > > also plan further working with the GPIO part during 5.13-rc cycle > > to > > utilize the regmap_gpio. That could be done in the GPIO tree then). > > I > > think the other portions are in a pretty stable shape now. > > This wouldn't be a bad idea in general for these serieses, especially > the bigger ones or the ones that get a lot of review comments on some > patches. > > In any case, here's a pull request for the helpers that are added Thanks Mark. > Matti Vaittinen (2): > regulator: helpers: Export helper voltage listing > regulator: Add regmap helper for ramp-delay setting > If I understand this correctly, the idea is that Lee could pull these changes to his tree? So, I will drop these two patches from the series when I resend it. Helpers are needed for the regulator part of the series to apply. Lee, Mark, please let me know if I misunderstood. Best Regards Matti Vaittinen
Re: [PATCH v5 15/19] regulator: Support ROHM BD71815 regulators
On Sun, 2021-04-04 at 19:21 +0300, Matti Vaittinen wrote: > On Fri, 2021-04-02 at 18:42 +0100, Mark Brown wrote: > > On Mon, Mar 29, 2021 at 03:59:51PM +0300, Matti Vaittinen wrote: > > > > Acked-by: Mark Brown > > > > but... > > Do you want me to respin the series or do you think this can be > applied > as-is and fixed by a follow-up? Actually, this was a bit stupid question. I will resend the series as I have also some small fixes to the GPIO. Best Regards Matti Vaittinen
Re: [PATCH v5 15/19] regulator: Support ROHM BD71815 regulators
On Fri, 2021-04-02 at 18:42 +0100, Mark Brown wrote: > On Mon, Mar 29, 2021 at 03:59:51PM +0300, Matti Vaittinen wrote: > > Acked-by: Mark Brown > > but... Do you want me to respin the series or do you think this can be applied as-is and fixed by a follow-up? Best Regards Matti Vaittinen
Re: [RFC PATCH v3 7/7] regulator: bd9576: Fix the driver name in id table
On Fri, 2021-04-02 at 18:19 +0100, Mark Brown wrote: > On Thu, Mar 11, 2021 at 12:24:29PM +0200, Matti Vaittinen wrote: > > Driver name was changed in MFD cell: > > https://lore.kernel.org/lkml/560b9748094392493ebf7af11b6cc558776c4fd5.1613031055.git.matti.vaitti...@fi.rohmeurope.com/ > > Fix the ID table to match this. > > This looks unrelated to the rest of the series? Correct. I think I mentioned that somewhere - or at least I intended to do that. Probably in the cover-letter. I included this change to the series just to avoid conflicts. Do you want me to send it separately? Best Regards Matti Vaittinen
Re: [RFC PATCH v3 4/7] regulator: add property parsing and callbacks to set protection limits
Hi Mark, Thanks for the review(s)! On Fri, 2021-04-02 at 18:18 +0100, Mark Brown wrote: > On Thu, Mar 11, 2021 at 12:23:02PM +0200, Matti Vaittinen wrote: > > > + /* > > +* Existing logic does not warn if over_current_protection is > > given as > > +* a constraint but driver does not support that. I think we > > should > > +* warn about this type of issues as it is possible someone > > changes > > The "existing logic" bit here is for a changelog, not the code - as > soon > as the patch is applied the comment becomes inaccurate. This also > seems > like a separate patch. I don't think this patch changed the logic but kept it as it is now. Eg, for the existing over_current_protection property we still silently ignore case where property is given but driver does not support setting it. For me this sounds like fragile approach and I did handle the new properties (like detection) in a different way. Thus the comment should stay valid - and thus I didn't think this warrants a new patch. If you think we should change the logic, then we should definitely do that in separate patch. That allows revert if existing setups break everywhere. How would you like this to be? I can change the logic if you see it's worth the risk of breaking existing setups. Best Regards Matti Vaittinen
Re: [PATCH v5 09/19] gpio: support ROHM BD71815 GPOs
On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote:
> On Mon, Mar 29, 2021 at 3:58 PM Matti Vaittinen
> wrote:
> >
> > +struct bd71815_gpio {
> > + struct gpio_chip chip;
> > + struct device *dev;
>
> Wondering why you need this. Is it the same as chip.parent?
>
This is exactly the reason why I had the comments you objected in the
probe. dev is pointer to the platform device - which should be used for
prints and any potential devm stuff.
chip.parent is the MFD device which provides the regmap access and DT
node.
Best Regards
Matti Vaittinen
Re: [PATCH v5 00/19] Support ROHM BD71815 PMIC
On Mon, 2021-03-29 at 15:52 +0300, Matti Vaittinen wrote: > Patch series introducing support for ROHM BD71815 PMIC > > ROHM BD71815 is a power management IC used in some battery powered > systems. It contains regulators, GPO(s), charger + coulomb counter, > RTC > and a clock gate. Lee, Mark, (Linus, Bartosz, all) I think all other parts except the regulator have relevant acks. (Well, I changed GPIO in last version so Bart/Linus may want changes but those will follow to GPIO in near future anyways). Do you think Lee could merge other but the regulator parts to MFD if Mark is busy? I'd like to be able to squeeze the amount of patches and recipients for future iterations. It might be easier to work directly on regulator tree if regulator part gets delayed to next cycle. (I do also plan further working with the GPIO part during 5.13-rc cycle to utilize the regmap_gpio. That could be done in the GPIO tree then). I think the other portions are in a pretty stable shape now. Best Regards Matti Vaittinen
Re: [PATCH v5 09/19] gpio: support ROHM BD71815 GPOs
On Tue, 2021-03-30 at 13:54 +0300, Andy Shevchenko wrote: > On Tue, Mar 30, 2021 at 1:43 PM Matti Vaittinen > wrote: > > On Tue, 2021-03-30 at 13:11 +0300, Andy Shevchenko wrote: > > ... > > > Andy, how fatal do you think these issues are? I did put these > > comments > > on my 'things to clean-up' list. > > > > If you don't see them as fatal, then I rather not resend whole > > series > > of 19 patches just for these. I am anyway going to rework the ROHM > > PMIC > > GPIO drivers which I have authored during the next couple of months > > for > > regmap_gpio usage. This series has most of the acks except for the > > regulator part - so I was about to suggest to Lee that perhaps he > > could > > apply other but regulator stuff to MFD so I could squeeze the > > recipient > > list and amount of patches in series. > > I understand that. I'm not a maintainer, but my personal view is that > it can be fixed in follow ups. Thanks Andy. The series already had acks from Bartosz and Linus so I hope they are also Ok with fixing these when reworking for regmap_gpio (I intend to do that during 5.13-rc cycle). Best Regards Matti Vaittinen
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
On Thu, 2021-03-25 at 09:49 +0900, Chanwoo Choi wrote:
> On 3/24/21 6:51 PM, Vaittinen, Matti wrote:
> > Hello Hans, Chanwoo, Greg,
> >
> > On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 3/24/21 10:21 AM, Matti Vaittinen wrote:
> > > > Add error print for probe failure when resource managed work-
> > > > queue
> > > > initialization fails.
> > > >
> > > > Signed-off-by: Matti Vaittinen <
> > > > matti.vaitti...@fi.rohmeurope.com>
> > > > Suggested-by: Chanwoo Choi
> > > > ---
> > > > drivers/extcon/extcon-gpio.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/extcon/extcon-gpio.c
> > > > b/drivers/extcon/extcon-
> > > > gpio.c
> > > > index 4105df74f2b0..8ea2cda8f7f3 100644
> > > > --- a/drivers/extcon/extcon-gpio.c
> > > > +++ b/drivers/extcon/extcon-gpio.c
> > > > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct
> > > > platform_device *pdev)
> > > > return ret;
> > > >
> > > > ret = devm_delayed_work_autocancel(dev, >work,
> > > > gpio_extcon_work);
> > > > - if (ret)
> > > > + if (ret) {
> > > > + dev_err(dev, "Failed to initialize
> > > > delayed_work");
> > > > return ret;
> > > > + }
> > >
> > > The only ret which we can have here is -ENOMEM and as a rule we
> > > don't
> > > log
> > > errors for those, because the kernel memory-management code
> > > already
> > > complains
> > > loudly when this happens.
> >
> > I know. This is why I originally omitted the print. Besides, if the
> > memory is so low that devres adding fails - then we probably have
> > plenty of other complaints as well... But as Chanwoo maintains the
> > driver and wanted to have the print - I do not have objections to
> > that
> > either. Maybe someone some-day adds another error path to wq
> > initialization in which case seeing it failed could make sense.
> >
> > > So IMHO this patch should be dropped.
> > Fine for me - as well as keeping it. I have no strong opinion on
> > this.
>
> If it is the same handling way for -ENOMEM, don't need to add log ss
> Hans said.
> Thanks for Hans.
So be it :)
Greg, can you just apply the patch 2/2 and drop this one? (There should
be no dependency between these) or do you want me to resend 2/2 alone?
> > Br,
> > Matti
> >
>
>
Re: [PATCH 1/2] extcon: extcon-gpio: Log error if work-queue init fails
Hello Hans, Chanwoo, Greg,
On Wed, 2021-03-24 at 10:25 +0100, Hans de Goede wrote:
> Hi,
>
> On 3/24/21 10:21 AM, Matti Vaittinen wrote:
> > Add error print for probe failure when resource managed work-queue
> > initialization fails.
> >
> > Signed-off-by: Matti Vaittinen
> > Suggested-by: Chanwoo Choi
> > ---
> > drivers/extcon/extcon-gpio.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-
> > gpio.c
> > index 4105df74f2b0..8ea2cda8f7f3 100644
> > --- a/drivers/extcon/extcon-gpio.c
> > +++ b/drivers/extcon/extcon-gpio.c
> > @@ -114,8 +114,10 @@ static int gpio_extcon_probe(struct
> > platform_device *pdev)
> > return ret;
> >
> > ret = devm_delayed_work_autocancel(dev, >work,
> > gpio_extcon_work);
> > - if (ret)
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize delayed_work");
> > return ret;
> > + }
>
> The only ret which we can have here is -ENOMEM and as a rule we don't
> log
> errors for those, because the kernel memory-management code already
> complains
> loudly when this happens.
I know. This is why I originally omitted the print. Besides, if the
memory is so low that devres adding fails - then we probably have
plenty of other complaints as well... But as Chanwoo maintains the
driver and wanted to have the print - I do not have objections to that
either. Maybe someone some-day adds another error path to wq
initialization in which case seeing it failed could make sense.
> So IMHO this patch should be dropped.
Fine for me - as well as keeping it. I have no strong opinion on this.
Br,
Matti
Re: [RFC RESEND PATCH v2 0/8] Add managed version of delayed work init
Hi Greg, On Tue, 2021-03-23 at 13:43 +0100, Greg KH wrote: > On Mon, Mar 22, 2021 at 09:41:13AM +0200, Matti Vaittinen wrote: > > It's not rare that device drivers need delayed work. > > It's not rare that this work needs driver's data. > > I don't normally comment on "RFC" patch series as I can't take them > and > the submitter doesn't feel right with them being merged at this point > in > time. > > So if you think this is all correct now, please resubmit without that > so > we can review it properly :) Thanks for the guidance :) I'll drop the RFC and resubmit. Others - sorry for the noise. Best Regards Matti Vaittinen
Re: [PATCH v2 10/17] gpio: support ROHM BD71815 GPOs
On Tue, 2021-01-19 at 12:07 +0100, Bartosz Golaszewski wrote: > On Tue, Jan 19, 2021 at 8:23 AM Matti Vaittinen > wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC > > has two > > GPO pins but only one is properly documented in data-sheet. The > > driver > > exposes by default only the documented GPO. The second GPO is > > connected to > > E5 pin and is marked as GND in data-sheet. Control for this > > undocumented > > pin can be enabled using a special DT property. > > > > This driver is derived from work by Peter Yang < > > yang...@embest-tech.com> > > although not so much of original is left. > > > > Signed-off-by: Matti Vaittinen > > Hi Matti, > > looks great, just a couple nits. Hello Bartosz, I think fixed all the nits to v3. Can I translate this to an ack? (I will respin the series as I guess the regulator part may have fallen through the cracks so I'd like to add the relevant acks :] ) Best Regards Matti Vaittinen
Re: [PATCH v3 06/15] mfd: Add ROHM BD71815 ID
Hello Lee,
On Wed, 2021-03-10 at 10:36 +, Lee Jones wrote:
> On Mon, 08 Mar 2021, Matti Vaittinen wrote:
>
> > Add chip ID for ROHM BD71815 and PMIC so that drivers can identify
> > this IC.
> >
> > Signed-off-by: Matti Vaittinen
> > Acked-for-MFD-by: Lee Jones
> > ---
> > include/linux/mfd/rohm-generic.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/mfd/rohm-generic.h
> > b/include/linux/mfd/rohm-generic.h
> > index 66f673c35303..e5392bcbc098 100644
> > --- a/include/linux/mfd/rohm-generic.h
> > +++ b/include/linux/mfd/rohm-generic.h
> > @@ -14,6 +14,7 @@ enum rohm_chip_type {
> > ROHM_CHIP_TYPE_BD71828,
> > ROHM_CHIP_TYPE_BD9571,
> > ROHM_CHIP_TYPE_BD9574,
> > + ROHM_CHIP_TYPE_BD71815,
>
> Is there a technical reason why these can't be re-ordered?
No, I don't think so.
BTW. there will probably be a (trivial) conflict here as both this
series and the BD9576/BD9573 series add an ID here. Let me guess, you'd
like to see them sorted?
>
> > ROHM_CHIP_TYPE_AMOUNT
> > };
> >
Br,
Matti Vaittinen
Re: [PATCH] power: supply: max8997_charger: make EXTCON dependency unconditional
On Mon, 2021-03-08 at 18:06 +0200, Andy Shevchenko wrote: > On Mon, Mar 8, 2021 at 5:29 PM Arnd Bergmann wrote: > > > - depends on EXTCON || !EXTCON > > I stumbled over this. > What is the point of having this line at all? > What magic trick does it serve for? The logic was somewhat hairy. I used it once for BD70528 watchdog which provides lock functions for RTC - or stubs if WDG is not used. Problem which that solved was that when RTC was built-in and WDG was a module - these functions were missing. It might've been Guenter or A. Belloni who suggested it. I don't remember 100% sure but I think it might require EXTCON to be build as a module. I guess the discussion I had regarding BD70528 can be found from lore.kernel.org - but it is probably buried deep... Maybe someone will give better and more definite answer. Best Regards Matti Vaittinen
Re: [RFC PATCH 1/7] dt_bindings: Add protection limit properties
On Fri, 2021-03-05 at 13:30 -0600, Rob Herring wrote:
> On Thu, Feb 11, 2021 at 02:34:17PM +0200, Matti Vaittinen wrote:
> > Support specifying protection/error/warning limits for regulator
> > over current, over temperature and over/under voltage.
> >
> > Most of the PMICs support only "protection" feature but few
> > setups do also support error/warning level indications.
> >
> > On many ICs most of the protection limits can't actually be set.
> > But for example the ampere limit for over-current protection on
> > ROHM
> > BD9576 can be configured - or feature can be completely disabled.
> >
> > Provide limit setting for all protections/errors for the sake of
> > the completeness and do that using own properties for all so that
> > not all users would need to set all levels when only one or few are
> > supported.
> >
> > Signed-off-by: Matti Vaittinen
> > ---
> > .../bindings/regulator/regulator.yaml | 82
> > +++
> > 1 file changed, 82 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/regulator/regulator.yaml
> > b/Documentation/devicetree/bindings/regulator/regulator.yaml
> > index 6d0bc9cd4040..47fff75f0554 100644
> > --- a/Documentation/devicetree/bindings/regulator/regulator.yaml
> > +++ b/Documentation/devicetree/bindings/regulator/regulator.yaml
> > @@ -117,6 +117,88 @@ properties:
> > description: Enable over current protection.
> > type: boolean
> >
> > + regulator-over-current-protection-microamp:
>
> Kind of long and 'current' is implied by 'microamp'. Perhaps
> regulator-oc-protection-microamp and similar.
I like this idea but...
regulator-over-current-protection:
description: Enable over current protection.
type: boolean
is an existing property so it kind of makes sense to me to name the
limit setting in a similar way. But I will change it as you suggested
as I don't think this is a big thing.
Br,
Matti Vaittinen
>
> > --
> > Matti Vaittinen, Linux device drivers
> > ROHM Semiconductors, Finland SWDC
> > Kiviharjunlenkki 1E
> > 90220 OULU
> > FINLAND
> >
> > ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> > ~~~
> > Simon says - in Latin please.
> > ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
> > Thanks to Simon Glass for the translation =]
Re: [RFC PATCH 5/7] dt-bindings: regulator: bd9576 add FET ON-resistance for OCW
Morning Rob, On Wed, 2021-02-17 at 15:34 -0600, Rob Herring wrote: > On Thu, Feb 11, 2021 at 02:35:41PM +0200, Matti Vaittinen wrote: > > BD9576MUF provides over-current protection and detection. Current > > is > > measured as voltage loss over external FET. Allow specifying FET's > > on > > resistance so current monitoring limits can be converted to > > voltages. > > > > Signed-off-by: Matti Vaittinen > > --- > > .../devicetree/bindings/regulator/rohm,bd9576-regulator.yaml | 5 > > + > > 1 file changed, 5 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/regulator/rohm,bd9576- > > regulator.yaml > > b/Documentation/devicetree/bindings/regulator/rohm,bd9576- > > regulator.yaml > > index b6515a0cee62..ad3f57e9cd9b 100644 > > --- a/Documentation/devicetree/bindings/regulator/rohm,bd9576- > > regulator.yaml > > +++ b/Documentation/devicetree/bindings/regulator/rohm,bd9576- > > regulator.yaml > > @@ -27,6 +27,11 @@ patternProperties: > >Properties for single regulator. > > $ref: "regulator.yaml#" > > > > +properties: > > + rohm,ocw-fet-ron-milliohms: > > We have '-ohms' and '-micro-ohms' already. Neither range works for > you? Thanks for taking a look at this :) I expect values to be magnitude of few hundred milliohms - which means micro-ohms should be usable. I'll try to still check this as I am far from being HW-expert. I probably can convert this to micro-ohms for v2. Best Regards Matti Vaittinen -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init
On Mon, 2021-02-15 at 12:43 +0100, Hans de Goede wrote:
> Hi,
>
> On 2/15/21 12:31 PM, gre...@linuxfoundation.org wrote:
> > On Mon, Feb 15, 2021 at 11:37:26AM +0100, Hans de Goede wrote:
> > > Hi,
> > >
> > > On 2/15/21 8:22 AM, Vaittinen, Matti wrote:
> > > > On Sat, 2021-02-13 at 16:59 +0100, Hans de Goede wrote:
> > > > > Hi,
> > > > >
> > > > > On 2/13/21 4:27 PM, Guenter Roeck wrote:
> > > > > > On 2/13/21 7:03 AM, Hans de Goede wrote:
> > > > > > [ ... ]
> > > > > > > I think something like this should work:
> > > > > > >
> > > > > > > static int devm_delayed_work_autocancel(struct device
> > > > > > > *dev,
> > > > > > > struct delayed_work *w,
> > > > > > > void (*worker)(struct
> > > > > > > work_struct *work)) {
> > > > > > > INIT_DELAYED_WORK(w, worker);
> > > > > > > return devm_add_action(dev, (void (*action)(void
> > > > > > > *))cancel_delayed_work_sync, w);
> > > > > > > }
> > >
> > > include/linux/devm-cleanup-helpers.h
> > >
> > > And put everything (including kdoc texts) there.
> > >
> > > This way the functionality is 100% opt-in (by explicitly
> > > including
> > > the header if you want the helpers) which hopefully makes this a
> > > bit more acceptable to people who don't like this style of
> > > cleanups.
> > >
> > > I would be even happy to act as the upstream maintainer for such
> > > a
> > > include/linux/devm-cleanup-helpers.h file, I can maintain it as
> > > part
> > > of the platform-drivers-x86 tree (with its own MAINTAINERS
> > > entry).
> > >
> > > Greg, would this be an acceptable solution to you ?
> >
> > I don't know, sorry, let's revisit this after 5.12-rc1 is out, with
> > a
> > patch set that I can review again, and we can go from there as I
> > can't
> > do anything until then...
>
> Ok.
This is Ok for me too. I am in no hurry with this - I've already a few
things to work on.
So, I will rework this to be in a single header when v5.12-rc1 is out.
Best Regards
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)
Re: [RFC PATCH 3/7] regulator: IRQ based event/error notification helpers
On Thu, 2021-02-11 at 14:35 +0200, Matti Vaittinen wrote:
> Provide helper function for IC's implementing regulator notifications
> when an IRQ fires. The helper also works for IRQs which can not be
> acked.
> Helper can be set to disable the IRQ at handler and then re-enabling
> it
> on delayed work later. The helper also adds
> regulator_get_error_flags()
> errors in cache for the duration of IRQ disabling.
>
> Signed-off-by: Matti Vaittinen
> ---
>
> This patch has gone through only a very limited amount of testing.
> All
> reviews / suggestions / testing is highly appreciated.
>
// Snip
> +
> +static void dev_delayed_work_drop(struct device *dev, void *res)
> +{
> + cancel_delayed_work_sync(*(struct delayed_work **)res);
> +}
> +
> +int dev_delayed_work_autocancel(struct device *dev, struct
> delayed_work *w,
> + void (*worker)(struct work_struct
> *work))
> +{
> + struct delayed_work **ptr;
> +
> + ptr = devres_alloc(dev_delayed_work_drop, sizeof(*ptr),
> GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + INIT_DELAYED_WORK(w, worker);
> + *ptr = w;
> + devres_add(dev, ptr);
> +
> + return 0;
> +}
I sent this dev_delayed_work_autocancel() + few cleanup patches as own
series. Discussion that series created made me realize that we don't
want to force use of devm by hiding the WQ init here. We should
introduce also non devm variant + manual cancellation routine for those
who don't use devm to register rdevs.
And as I see that Greg was strongly opposing the devm based delayed
work cancellation - I guess that if we want to proceed with this one
we'd better first implement the 'non devm' variant which uses manual wq
cancellation + manual IRQ deregistering and use that cancellation to
build a devm one...
I'll try to cook v2 still this week.
Best Regards
Matti Vaittinen
Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init
On Sat, 2021-02-13 at 16:59 +0100, Hans de Goede wrote:
> Hi,
>
> On 2/13/21 4:27 PM, Guenter Roeck wrote:
> > On 2/13/21 7:03 AM, Hans de Goede wrote:
> > [ ... ]
> > > I think something like this should work:
> > >
> > > static int devm_delayed_work_autocancel(struct device *dev,
> > > struct delayed_work *w,
> > > void (*worker)(struct
> > > work_struct *work)) {
> > > INIT_DELAYED_WORK(w, worker);
> > > return devm_add_action(dev, (void (*action)(void
> > > *))cancel_delayed_work_sync, w);
> > > }
> > >
> > > I'm not sure about the cast, that may need something like this
> > > instead:
> > >
> > > typedef void (*devm_action_func)(void *);
> > >
> > > static int devm_delayed_work_autocancel(struct device *dev,
> > > struct delayed_work *w,
> > > void (*worker)(struct
> > > work_struct *work)) {
> > > INIT_DELAYED_WORK(w, worker);
> > > return devm_add_action(dev,
> > > (devm_action_func)cancel_delayed_work_sync, w);
> >
> > Unfortunately, you can not type cast function pointers in C. It is
> > against the C ABI.
> > I am sure it is done in a few places in the kernel anyway, but
> > those are wrong.
>
> I see, bummer.
I think using devm_add_action() is still a good idea.
>
> If we add a devm_clk_prepare_enable() helper that should probably be
> added
> to drivers/clk/clk-devres.c and not to drivers/base/devres.c .
>
> I also still wonder if we cannot find a better place for this new
> devm_delayed_work_autocancel() helper but nothing comes to mind.
I don't like the idea of including device.h from workqueue.h - and I
think this would be necessary if we added
devm_delayed_work_autocancel() as inline in workqueue.h, right?
I also see strong objection towards the devm managed clean-ups.
How about adding some devm-helpers.c in drivers/base - where we could
collect devm-based helpers - and which could be enabled by own CONFIG -
and left out by those who dislike it?
I know I wrote that the devm_delayed_work_autocancel() does probably
not warrant own file - but if you can foresee devm_work_autocancel()
and few other generally useful helpers - then we would have a place for
those. The devm stuff should in my opinion live under drivers/.
Best Regards
Matti Vaittinen
Re: [RFC PATCH 1/7] drivers: base: Add resource managed version of delayed work init
On Sat, 2021-02-13 at 13:16 +0100, Greg Kroah-Hartman wrote:
> On Sat, Feb 13, 2021 at 01:58:44PM +0200, Matti Vaittinen wrote:
> > A few drivers which need a delayed work-queue must cancel work at
> > exit.
> > Some of those implement remove solely for this purpose. Help
> > drivers
> > to avoid unnecessary remove and error-branch implementation by
> > adding
> > managed verision of delayed work initialization
> >
> > Signed-off-by: Matti Vaittinen
>
> That's not a good idea. As this would kick in when the device is
> removed from the system, not when it is unbound from the driver,
> right?
>
> > ---
> > drivers/base/devres.c | 33 +
> > include/linux/device.h | 5 +
> > 2 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index fb9d5289a620..2879595bb5a4 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -1231,3 +1231,36 @@ void devm_free_percpu(struct device *dev,
> > void __percpu *pdata)
> >(void *)pdata));
> > }
> > EXPORT_SYMBOL_GPL(devm_free_percpu);
> > +
> > +static void dev_delayed_work_drop(struct device *dev, void *res)
> > +{
> > + cancel_delayed_work_sync(*(struct delayed_work **)res);
> > +}
> > +
> > +/**
> > + * devm_delayed_work_autocancel - Resource-managed work allocation
> > + * @dev: Device which lifetime work is bound to
> > + * @pdata: work to be cancelled when device exits
> > + *
> > + * Initialize work which is automatically cancelled when device
> > exits.
>
> There is no such thing in the driver model as "when device exits".
> Please use the proper terminology as I do not understand what you
> think
> this is doing here...
>
> > + * A few drivers need delayed work which must be cancelled before
> > driver
> > + * is unload to avoid accessing removed resources.
> > + * devm_delayed_work_autocancel() can be used to omit the explicit
> > + * cancelleation when driver is unload.
> > + */
> > +int devm_delayed_work_autocancel(struct device *dev, struct
> > delayed_work *w,
> > +void (*worker)(struct work_struct
> > *work))
> > +{
> > + struct delayed_work **ptr;
> > +
> > + ptr = devres_alloc(dev_delayed_work_drop, sizeof(*ptr),
> > GFP_KERNEL);
> > + if (!ptr)
> > + return -ENOMEM;
> > +
> > + INIT_DELAYED_WORK(w, worker);
> > + *ptr = w;
> > + devres_add(dev, ptr);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_delayed_work_autocancel);
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 1779f90eeb4c..192456198de7 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -27,6 +27,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -249,6 +250,10 @@ void __iomem *devm_of_iomap(struct device
> > *dev,
> > struct device_node *node, int index,
> > resource_size_t *size);
> >
> > +/* delayed work which is cancelled when driver exits */
>
> Not when the "driver exits".
>
> There is two different lifespans here (well 3). Code and
> data*2. Don't
> confuse them as that will just cause lots of problems.
>
> The move toward more and more "devm" functions is not the way to go
> as
> they just more and more make things easier to get wrong.
>
> APIs should be impossible to get wrong, this one is going to be
> almost
> impossible to get right.
Thanks for prompt reply. I guess I must've misunderstood some of this
concept. Frankly to say, I don't understand how the devm based irq
management works and this does not. Maybe I'd better study this further
then.
Best Regards
Matti Vaittinen
Re: [RFC PATCH 3/7] regulator: IRQ based event/error notification helpers
On Thu, 2021-02-11 at 14:35 +0200, Matti Vaittinen wrote:
> Provide helper function for IC's implementing regulator notifications
> when an IRQ fires. The helper also works for IRQs which can not be
> acked.
> Helper can be set to disable the IRQ at handler and then re-enabling
> it
> on delayed work later. The helper also adds
> regulator_get_error_flags()
> errors in cache for the duration of IRQ disabling.
>
> Signed-off-by: Matti Vaittinen
> ---
>
> This patch has gone through only a very limited amount of testing.
> All
> reviews / suggestions / testing is highly appreciated.
>
/* SNIP */
> +
> +static void dev_delayed_work_drop(struct device *dev, void *res)
> +{
> + cancel_delayed_work_sync(*(struct delayed_work **)res);
> +}
> +
> +int dev_delayed_work_autocancel(struct device *dev, struct
> delayed_work *w,
> + void (*worker)(struct work_struct
> *work))
> +{
> + struct delayed_work **ptr;
> +
> + ptr = devres_alloc(dev_delayed_work_drop, sizeof(*ptr),
> GFP_KERNEL);
> + if (!ptr)
> + return -ENOMEM;
> +
> + INIT_DELAYED_WORK(w, worker);
> + *ptr = w;
> + devres_add(dev, ptr);
> +
> + return 0;
> +}
> +
I got mail from build-bot. Sparse warning. Bot suggested staticizing
the dev_delayed_work_autocancel(). I should've been more careful.
It how ever made me wonder if this would actually be worth exporting?
There seems to be few drivers which need delayed wq and which implement
.remove() just to call the cancel_delayed_work_sync().
I think this might help cleaning up those(?) Or am I completely off
here?
I just did:
git grep -A15 remove |grep -B10 -A10 cancel_delayed_work_sync
in drivers directory and spotted couple of candidates like
watchdog/retu_wdt.c (should also use devm_watchdog_register_device)
regulator/qcom_spmi-regulator.c
power/supply/sbs-charger.c
power/supply/sbs-battery.c
power/supply/ltc2941-battery-gauge.c
...
And no. I am not offering to go through _all_ drivers, but I guess I
could go through at least few of them :)
And sorry for noise if this has been suggested and rejected before - I
didn't spot something like this from mail lists. (Maybe I am missing
something?)
Best Regards
Matti Vaittinen
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)
Re: [PATCH v7 3/6] mfd: bd9576: Add IRQ support
Hello Again Lee & All, On Wed, 2021-02-10 at 09:01 +0200, Matti Vaittinen wrote: > On Tue, 2021-02-09 at 15:25 +, Lee Jones wrote: > > On Fri, 22 Jan 2021, Matti Vaittinen wrote: > > + /* BD9573 only supports fatal IRQs which we do not > > > handle */ > > > > Why not? > > Because 'fatal' in the context of this comment means that when this > condition occurs the PMIC will do emergency shut down for power > outputs After having a fresh look at this I see the wisdom in your comment. The HW provides IRQs and if they are listed in DT for BD9573 does not mean we should stop here. Thanks for pointing it out. Best Regards Matti Vaittinen
Re: [PATCH v7 0/6] Support ROHM BD9576MUF and BD9573MUF PMICs
Hello Lee, Mark All, On Fri, 2021-01-22 at 16:30 +0200, Matti Vaittinen wrote: > Initial support for ROHM BD9576MUF and BD9573MUF PMICs. > > These PMICs are primarily intended to be used to power the R-Car > family > processors. BD9576MUF includes some additional safety features the > BD9573MUF does not have. This initial version of drivers provides > temperature, over voltage and under voltage warnings is IRQ > information > is passed via DT. > > This patch series includes MFD and watchdog drivers. Regulator part > was > already applied but this series brings the over-/undervoltage and > temperature error notifications which consumer drivers can utilize. I had some discussion with Mark and Angelo about creating a helper for handling this kind of regulator notification IRQs. (For anyone interested: the discussion can be seen here: https://lore.kernel.org/lkml/6046836e22b8252983f08d5621c35ececb97820d.ca...@fi.rohmeurope.com/ ) I've now drafted RFCv1 for that support (not sent it yet). The RFC converts the BD9576 regulator driver to use the new helper and adds some new definitions to MFD headers. What would be the most convenient way of handling this? Should I merge this series in the RFC and make it just one big series? Or should I keep these as two separated series? If I keep these as separate series, should I then omit all the MFD patches from RFC series - and add potential MFD changes (like OVD/UVD configuration registers) in this series (which makes bd9576 regulators not compiling) - or should the MFD parts be included in both series - in which case we need to somehow stay on track what parts of MFD is reviewed. Simplest for me would be if we could get the oldest patches 1,2,4 and 5 from this series in-tree (only MFD is not acked) - but I guess it won't happen at this point of the development cycle - and bring in the IRQ (patch 3) and regulator notifications (patch 6) using the RFC series. How do you see it? Should I meld this in the RFC or keep two separate series - in which case, how should I handle the MFD changes brought by the RFC series? Best Regards Matti Vaittinen
Re: [PATCH v2 06/17] dt_bindings: regulator: Add ROHM BD71815 PMIC regulators
Hello Again Rob, And thanks for reviewing the bindings! On Mon, 2021-02-08 at 20:24 -0600, Rob Herring wrote: > On Tue, Jan 19, 2021 at 09:17:09AM +0200, Matti Vaittinen wrote: > > Add binding documentation for regulators on ROHM BD71815 PMIC. > > 5 bucks, 7 LDOs and a boost for LED. > > > > Signed-off-by: Matti Vaittinen > > --- snip > > + > > + rohm,dvs-run-voltage: > > Use standard unit suffix. > > > +description: > > + PMIC "RUN" state voltage in uV when PMIC HW states are > > used. See > > + comments below for bucks/LDOs which support this. 0 > > means > > + regulator should be disabled at RUN state. > > +$ref: "/schemas/types.yaml#/definitions/uint32" > > And then drop this. Quote from v1 review: > > > > > > + rohm,dvs-run-voltage: > > > > > > > > > > These should have a unit suffix. > > > > > > > > I know but these are existing properties. I'd like to re-use them > > > > as > > > > they have exported parser helpers - and I am unsure what kind of > > > > breakages changing them would cause. (The BD71837/BD71847 which > > > > introduced these properties are one of the PMICs which are pretty > > > > widely used.) > > > > > > Okay. Hopefully I remember next time I see this... > > > > Actually, I think I can add support for rohm,dvs-run-microvolt and > > fellows to these same helpers so new devices can use appropriately > > named properties. That would mean there is duplicate properties for > > same purpose - but maybe it allows us to eventually deprecate the old > > ones... Which of these options would you prefer? > > Just keep the existing ones. Seem you predicted this XD If you still think it's Ok to keep the existing ones, then I'll take this an ack, Ok? Best Regards Matti Vaittinen
Re: short-circuit and over-current IRQs
On Thu, 2021-01-28 at 12:10 +, Mark Brown wrote: > On Thu, Jan 28, 2021 at 09:23:08AM +0000, Vaittinen, Matti wrote: > > On Wed, 2021-01-27 at 16:32 +, Mark Brown wrote: > > > Note that the events the API currently has are expected to be for > > > the > > > actual error conditions, not for the warning ones - indicating > > > that > > > the > > > voltage is out of regulation for example. > > I am unsure how to interpret this. What is the criteria of issue > > being > > an error/warning. When I was talking about warning I meant that the > > issue which is detected is unexpected and abnormal (error?) - but > > might > > still be recoverable (warning?). I understand the regulator > > framework > > must not signal same events for different purposes - but I don't > > really > > know what the current events are used for - I am grateful for any > > guidance! > > What the majority of hardware interrupts on is situations where > things > have already gone out of spec and there are actual problems with the > output - for example with current limiting there's often an actual > limiter in there so the regulator simply won't supply any more > current > than is configured. With a warning everything is still working fine > but > getting close to not doing so. Sounds reasonable. Warning while things are still working - but are getting to the boundary. Error when things are already pretty wrong. Thanks. > > > Well, if these things are kicking in the hardware is in serious > > > trouble > > > anyway so it's unclear what the system would be likely to do in > > > software, and also unclear how safe it is to rely on software to > > > be > > > able > > > to take that action given that it let things get into such a bad > > > state > > > in the first place. > > Actually, bear with me but I am unsure why we have these > > notifications > > if we don't expect SW to be able to do anything? Wouldn't the panic > > print be all that is needed then? I think that setups which have > > dual > > You'll notice that there aren't any actual users of this stuff in > tree > at the minute - people don't generally put much effort into software > recovery as they're not expecting to be anywhere near limiting in > normal > operation. What I'd expect people to do where they do implement > handling is something like shutting down all other supplies on the > device, possibly also trying to shut down the system as a > whole. Things > more about preventing physical damage rather than being part of the > normal operation of the system. Again this makes sense. I will try to ask form HW colleagues what they thought to be the action SW take (I hope they have some scenario on mind - let's see). If they tell me that they expect SW to shut down system gracefully - then I keep errors, if they tell me they think SW will temporarily disable some HW blocks or do other "tricks" and later resume normal operation - then I will see if I can add some new 'warning' indicators. > For thermal issues systems generally try to apply software limits > well > before an individual component starts flagging things up with an > interrupt, the limits that devices have are generally super high and > often there'll be issues at a system level (eg, a case getting > unusably > hot) earlier and it can take a while for responses to have an impact. I think this is also case with the BD9576 - 140 C sounds pretty hot to me - and I expect this is really where things are already badly wrong. So I guess I can keep the 'error' here. > > > limits (one for initiating potential SW recovery - other for HW to > > forcing protection) actually make sense. So does implementing > > notifiers > > / error statuses for events where SW recovery is potentially > > helpful. > > But whether the existing event notifications / error flags are > > correct > > for these is something I can't decide :) Here I ask guidance for > > Mark & > > others who know what is the idea behind existing error- > > flags/events. > > It's not that we shouldn't implement support for warnings, it's that > they're not the common case for hardware and so won't line up with > behaviour for other users. Agreed. As I said, I understand we shouldn't send same events to different situations. If current errors are used to indicate things are really "wrong" to the point where safest thing is to shut down system - then we'd better add these "warnings" to indicate that there would potentially still be time to change something - before things are shut off. Thanks again! Best regards Matti Vaittinen
Re: short-circuit and over-current IRQs
On Wed, 2021-01-27 at 16:32 +, Mark Brown wrote: > On Wed, Jan 27, 2021 at 03:34:46PM +0100, AngeloGioacchino Del Regno > wrote: > > Il 27/01/21 13:56, Matti Vaittinen ha scritto: > > > I can only speak for BD9576MUF - which has two limits for > > > monitored > > > entity (temperature/voltage). One limit being 'warning' limit (or > > > 'detection' as data-sheet says), the other being 'protection' > > > limit. > > I would tend to agree with you here, Matti. Also from what I > > understand, > > the wanted outcome is software handling a possibly temporary issue > > with > > you charging caps, external IC initialization using (expectedly) > > much > > more power than needed before stabilizing, and eventually handling > > other > > "real" issues for which there is a solution that may not even > > include > > disabling the regulator itself, but some other handling on the > > connected > > device driver. > > Note that the events the API currently has are expected to be for the > actual error conditions, not for the warning ones - indicating that > the > voltage is out of regulation for example. I am unsure how to interpret this. What is the criteria of issue being an error/warning. When I was talking about warning I meant that the issue which is detected is unexpected and abnormal (error?) - but might still be recoverable (warning?). I understand the regulator framework must not signal same events for different purposes - but I don't really know what the current events are used for - I am grateful for any guidance! So, I would like to have an event which the driver can signal to consumers so that the consumer drivers can try to recover problem W/O shutting down the SoC. I try to explain what the BD9576 does and how I see it - I hope you can then tell me if I am misusing existing events/errors. Sorry for longish explanation... The patch I sent did just use existing flags as follows: REGULATOR_EVENT_UNDER_VOLTAGE: Sent when PMIC informs regulator output being under the target. Limit for this is configurable for each output - from roughly at 0.9*(set Vout) up to 0.99*(set Vout). The detection can also be disabled. I think that if this is enabled for system, then this should be regarded as "error condition" - but HW does not shut down. Furthermore, BD9576 does NOT have "protection" limit for output dropping. It does have a protection limit for regulator input dropping though - and PMIC will forcibly shut down if voltage input of a regulator drops below certain value. What action the system can take when output voltage drops depends on system and is not really up to the regulator driver to decide. But is this notification correct? (I thought so just judging the name). What I thought is that it would be nice to inform consumers about the detected under-voltage (as HW supports this). In addition to this notification I added a 'state flag' in driver so that if someone calls the regulator_get_error we do return REGULATOR_ERROR_UNDER_VOLTAGE. Are these correct flags? REGULATOR_EVENT_REGULATION_OUT: Current patch send this when PMIC informs regulator output being OVER the set voltage. PMIC supports similar limit configuration / feature disabling as with UVD. The over voltage limit can be set roughly from 1.01*(set Vout) to 1.1*(set Vout). PMIC has also protection limits for regulator outputs so that if output is roughly 1.2*(set Vout) the PMIC will shut down. Again, I think this is an error condition but what action system can/should take if output voltage is exceeding the limit is not known by me. I used same regulator_get_error logic and REGULATOR_ERROR_REGULATION_OUT is returned by regulator_get_error when condition is not cleared. Again - I would appreciate knowledge about if this is correct flag. REGULATOR_EVENT_OVER_TEMP: Sent when PMIC gives us "thermal warning" IRQ. The PMIC has thermal shut-down point 175 C when it will immediately cut the power from system to protect it from damaging. The thermal warning IRQ is firing at 30 C lower temperature and HW performs no other action here. AFAIR the temperature limit can not be adjusted. I think this is again an error condition - kind of last resort for the system to do something before PMIC shuts it down. I don't think this is intended to be any 'normal temperature regulation' feature. What system can do here (turn off some HW? Reduce clocks? maxx fans? Shut down graphichs block and send warning to display using TCON image overlay? Forcibly close the panorama windows to cut sunlight?) depends on system. Here I also added REGULATOR_ERROR_OVER_TEMP to be returned by regulator_get_error. Finally, for one of the outputs we have "over current warning" interrupt and over current protection. The limit for OCW is configurable (data-sheet speaks of milliVOLTS here so maybe shunt is used?) and it can also be disabled. Protection is again not configurable and will shut down the outputs immediately. I think it is again up to system designer to decide
Re: short-circuit and over-current IRQs
On Wed, 2021-01-27 at 15:34 +0100, AngeloGioacchino Del Regno wrote: > Il 27/01/21 13:56, Matti Vaittinen ha scritto: > > Hello Mark, > > > > Hey Matti, hey Mark! > > > Nice to hear from you. :) > > > > On Wed, 2021-01-27 at 12:27 +, Mark Brown wrote: > > > On Wed, Jan 27, 2021 at 12:01:55PM +, Vaittinen, Matti wrote: > > > I will see if I can cook-up something decent - but as I said, I > > would > > appreciate any/all testing if I get patch crafted :) > > I develop this stuff in my spare time: I can't make big promises, but > I can tell you that I will try to test your proposal on qcom-labibb > as > soon as I will be able to. Thanks a Lot! This is more than fair :) I'll CC you when (if) I have this drafted! Best Regards --Matti
short-circuit and over-current IRQs
Hi dee Ho peeps,
I just noticed the
390af53e04114f790d60b63802a4de9d815ade03 ("regulator: qcom-labibb:
Implement short-circuit and over-current IRQs")
in regulator tree. No worries - I haven't hit any problem with it :]
I've been working with ROHM BD9576MUF - which implements warning IRQs
for over/under-voltage and high temperature. (Short-circuit detection
does also generate IRQ but it also automatically shuts down the
regulators by HW). BD9576MUF does also keep the IRQ activated for as
long as the 'troubling condition' stays active in HW. This is why
BD9576MUF driver does also need to implement some logic to disable IRQ
for some time period just to keep CPU out of the IRQ loop.
https://lore.kernel.org/lkml/4d725ea6e9261f22d4c808b39013baf479f252dc.1611324968.git.matti.vaitti...@fi.rohmeurope.com/
My implementation lacks of the retry counter and regulator shut-down
(BD9576 has also OCP - "Over Current Protection/OVP - "Over Voltage
Protection"/TSD - "Thermal Shutdown" features so that HW will shut down
outputs if things get worse so those are not so crucial).
I've also had some hard time when trying to test this - I guess others
have faced similar problems too...
Anyways - I was wondering if this is common thing amongst many PMICs?
If yes - then, perhaps some generally useful regulator helper could be
added to help implementing the IRQ disabling + scheduling worker to
check status and re-enable IRQs? I think it *might* save some time in
the future - and help making same mistakes many times :]
I *might* be able to find some time to draft out some code for this
(not promising, I am currently having few patch series pending but I
could at least try) - but I am not able to do thorough testing with
BD9576... Any possibility to co-operate? Maybe we could try this also
onqcom-labibb? Or do you think this is overall just a dead idea?
Best Regards
Matti Vaittinen
Re: [PATCH v2 10/17] gpio: support ROHM BD71815 GPOs
Hi Bartosz, On Tue, 2021-01-19 at 12:07 +0100, Bartosz Golaszewski wrote: > On Tue, Jan 19, 2021 at 8:23 AM Matti Vaittinen > wrote: > > Support GPO(s) found from ROHM BD71815 power management IC. The IC > > has two > > GPO pins but only one is properly documented in data-sheet. The > > driver > > exposes by default only the documented GPO. The second GPO is > > connected to > > E5 pin and is marked as GND in data-sheet. Control for this > > undocumented > > pin can be enabled using a special DT property. > > > > This driver is derived from work by Peter Yang < > > yang...@embest-tech.com> > > although not so much of original is left. > > > > Signed-off-by: Matti Vaittinen > > Hi Matti, > > looks great, just a couple nits. > Thanks for the review! I'll store your finsings and fix them when I respin this. I think all of your points were valid. As I know this is largish series (and as I know I accidentally sent first 10 v2 patches to all recipients no matter what subsystem was impacted) I'll wait for a while before resending (at least a week). Besides I don't expect the dependencies to be merged before next kernel release so this is not urgent :) - but thanks! Br, Matti
Re: [PATCH] regulator: bd718x7, bd71828, Fix dvs voltage levels
On Fri, 2021-01-15 at 14:41 +, Lee Jones wrote:
> On Fri, 15 Jan 2021, Matti Vaittinen wrote:
>
> > The ROHM BD718x7 and BD71828 drivers support setting HW state
> > specific voltages from device-tree. This is used also by various
> > in-tree DTS files.
> >
> > These drivers do incorrectly try to compose bit-map using enum
> > values. By a chance this works for first two valid levels having
> > values 1 and 2 - but setting values for the rest of the levels
> > do indicate capbility of setting values for first levels as
> > well. Luckily the regulators which support settin values for
> > SUSPEND/LPSR do usually also support setting values for RUN
> > and IDLE too - thus this has not been such a fatal issue.
> >
> > Fix this by defining the old enum values as bits and using
> > new enum in parsing code. This allows keeping existing IC
> > specific drivers intact and only adding the defines and
> > slightly changing the rohm-regulator.c
> >
> > Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common
> > and bd718x7 specific parts")
> > Signed-off-by: Matti Vaittinen
> > ---
> >
> > One more attempt today. I did test the driver is not causing a
> > crash
> > when load but no further tests concluded as I don't have
> > BD71837/47/50
> > at home. This looks now trivial though so I decided to give it a
> > go.
> > Sorry for all the trouble this far - and also for the mistakes to
> > come.
>
> Why don't you wait until next week when you can run this on real h/w
> with some pretty debug to ensure it does the right thing?
I first thought I would. Then I didn't wait because - as I said - this
looks pretty trivial to me now - and because I thought it might get
merged quickly. If you see it risky, then please don't merge. I will
test this next week and can resend after that if necessary.
Sorry for the trouble again.
Best Regards
--Matti
Re: [PATCH] regulator: bd718x7, bd71828, Fix dvs voltage levels
On Fri, 2021-01-15 at 13:47 +, Lee Jones wrote:
> On Fri, 15 Jan 2021, Matti Vaittinen wrote:
>
> > The ROHM BD718x7 and BD71828 drivers support setting HW state
> > specific voltages from device-tree. This is used also by various
> > in-tree DTS files.
> >
> > These drivers do incorrectly try to compose bit-map using enum
> > values. By a chance this works for first two valid levels having
> > values 1 and 2 - but setting values for the rest of the levels
> > do indicate capbility of setting values for first levels as
> > well. Luckily the regulators which support settin values for
> > SUSPEND/LPSR do usually also support setting values for RUN
> > and IDLE too - thus this has not been such a fatal issue.
> >
> > Fix this by defining the old enum values as bits and using
> > new enum in parsing code. This allows keeping existing IC
> > specific drivers intact and only adding the defines and
> > slightly changing the rohm-regulator.c
> >
> > Fixes: 21b72156ede8b ("regulator: bd718x7: Split driver to common
> > and bd718x7 specific parts")
> > Signed-off-by: Matti Vaittinen
> > ---
> > drivers/regulator/rohm-regulator.c | 8
> > include/linux/mfd/rohm-generic.h | 22 --
> > 2 files changed, 20 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/regulator/rohm-regulator.c
> > b/drivers/regulator/rohm-regulator.c
> > index 399002383b28..96caae7dbef4 100644
> > --- a/drivers/regulator/rohm-regulator.c
> > +++ b/drivers/regulator/rohm-regulator.c
> > @@ -55,25 +55,25 @@ int rohm_regulator_set_dvs_levels(const struct
> > rohm_dvs_config *dvs,
> > for (i = 0; i < ROHM_DVS_LEVEL_MAX && !ret; i++) {
> > if (dvs->level_map & (1 << i)) {
> > switch (i + 1) {
> > - case ROHM_DVS_LEVEL_RUN:
> > + case _ROHM_DVS_LEVEL_RUN:
> > prop = "rohm,dvs-run-voltage";
> > reg = dvs->run_reg;
> > mask = dvs->run_mask;
> > omask = dvs->run_on_mask;
> > break;
> > - case ROHM_DVS_LEVEL_IDLE:
> > + case _ROHM_DVS_LEVEL_IDLE:
> > prop = "rohm,dvs-idle-voltage";
> > reg = dvs->idle_reg;
> > mask = dvs->idle_mask;
> > omask = dvs->idle_on_mask;
> > break;
> > - case ROHM_DVS_LEVEL_SUSPEND:
> > + case _ROHM_DVS_LEVEL_SUSPEND:
> > prop = "rohm,dvs-suspend-voltage";
> > reg = dvs->suspend_reg;
> > mask = dvs->suspend_mask;
> > omask = dvs->suspend_on_mask;
> > break;
> > - case ROHM_DVS_LEVEL_LPSR:
> > + case _ROHM_DVS_LEVEL_LPSR:
> > prop = "rohm,dvs-lpsr-voltage";
> > reg = dvs->lpsr_reg;
> > mask = dvs->lpsr_mask;
> > diff --git a/include/linux/mfd/rohm-generic.h
> > b/include/linux/mfd/rohm-generic.h
> > index 4283b5b33e04..a557988831d7 100644
> > --- a/include/linux/mfd/rohm-generic.h
> > +++ b/include/linux/mfd/rohm-generic.h
> > @@ -20,15 +20,25 @@ struct rohm_regmap_dev {
> > struct regmap *regmap;
> > };
> >
> > +/*
> > + * Do not use these in IC specific driver - the bit map should be
> > created using
> > + * defines without the underscore. These should be used only in
> > rohm-regulator.c
> > + */
> > enum {
> > - ROHM_DVS_LEVEL_UNKNOWN,
> > - ROHM_DVS_LEVEL_RUN,
> > - ROHM_DVS_LEVEL_IDLE,
> > - ROHM_DVS_LEVEL_SUSPEND,
> > - ROHM_DVS_LEVEL_LPSR,
> > - ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR,
> > + _ROHM_DVS_LEVEL_UNKNOWN,
> > + _ROHM_DVS_LEVEL_RUN,
> > + _ROHM_DVS_LEVEL_IDLE,
> > + _ROHM_DVS_LEVEL_SUSPEND,
> > + _ROHM_DVS_LEVEL_LPSR,
> > + ROHM_DVS_LEVEL_MAX = _ROHM_DVS_LEVEL_LPSR,
>
> I don't understand how this is still not broken.
>
> I think you either need to change the for() loop that consumes this
> to
> use "<=" or push the MAX enum to the last line (on its own).
>
> The latter would be my personal preference.
Bah. Occasionally getting it right is just hard. The for loop condition
is allright now as it does as it was originally intended and scans the
amount of valid values. Starting at 0, using condition i <
ROHM_DVS_LEVEL_MAX guarantees we process all valid values _knowing_
that value 0 is 'invalid'.
But now I made a new error in following if-condition. I didn't take
into account the fact that left-sifting the 1 by _ROHM_DVS_LEVEL_RUN
for first valid value (ROHM_DVS_LEVEL_RUN) makes the first valid
bitmask to be 2 not 1. So the if-condition
if (dvs->level_map & (1 << i))
following the for loop is now broken. It would probably work if it was
2 << i. Anyways -
Re: [PATCH 11/15] regulator: rohm-regulator: SNVS dvs and linear voltage support
Hello Lee,
Thanks for carefull review! I do appreciate this!
On Fri, 2021-01-15 at 08:26 +, Lee Jones wrote:
> On Fri, 08 Jan 2021, Matti Vaittinen wrote:
>
> > The helper for obtaining HW-state based DVS voltage levels
> > currently only
> > works for regulators using linear-ranges. Extend support to
> > regulators with
> > simple linear mappings and add also proper error path if pickable-
> > ranges
> > regulators call this.
> >
> > Signed-off-by: Matti Vaittinen
> > ---
> > @@ -27,7 +27,8 @@ enum {
> > ROHM_DVS_LEVEL_IDLE,
> > ROHM_DVS_LEVEL_SUSPEND,
> > ROHM_DVS_LEVEL_LPSR,
> > - ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_LPSR,
> > + ROHM_DVS_LEVEL_SNVS,
> > + ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_SNVS,
> > };
>
> Does this actually work?
>
> The code that consumes it looks like:
>
> for (i = 0; i < ROHM_DVS_LEVEL_MAX && !ret; i++)
>
> So it will loop through like:
>
> 0 (ROHM_DVS_LEVEL_IDLE)
> 1 (ROHM_DVS_LEVEL_SUSPEND)
> 2 (ROHM_DVS_LEVEL_LPSR)
> 3 (ROHM_DVS_LEVEL_SNVS)
>
> Then break, since 'i' will be (== 4) not (< 4).
>
> So the following will never be used:
>
> 4 (ROHM_DVS_LEVEL_MAX = ROHM_DVS_LEVEL_SNVS)
>
> Unless I'm missing something, I think MAX should be the last entry.
Good catch. I think you are correct. I will revise this for next
version (and add also fixes tag as it seems the current code is broken
for LPSR).
>
> > /**
> > @@ -66,6 +67,9 @@ struct rohm_dvs_config {
> > unsigned int lpsr_reg;
> > unsigned int lpsr_mask;
> > unsigned int lpsr_on_mask;
> > + unsigned int snvs_reg;
> > + unsigned int snvs_mask;
> > + unsigned int snvs_on_mask;
> > };
> >
> > #if IS_ENABLED(CONFIG_REGULATOR_ROHM)
Re: [PATCH RESEND v6 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
Hello Lee, Nice to see you are back :) On Thu, 2021-01-14 at 10:00 +, Lee Jones wrote: > On Tue, 29 Dec 2020, Vaittinen, Matti wrote: > > > Hello Again peeps, > > > > On Thu, 2020-12-17 at 12:04 +0200, Matti Vaittinen wrote: > > > On Wed, 2020-12-02 at 15:32 +0200, Matti Vaittinen wrote: > > > > Hello Lee, > > > > > > > > On Wed, 2020-12-02 at 12:57 +0000, Lee Jones wrote: > > > > > On Fri, 27 Nov 2020, Vaittinen, Matti wrote: > > > > > > > > > > > Hello Lee, > > > > > > > > > > > > On Fri, 2020-11-27 at 08:32 +, Lee Jones wrote: > > > > > > > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > > > > > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs > > > > > > > > which > > > > > > > > are > > > > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > > > > matti.vaitti...@fi.rohmeurope.com> > > > > > > > > --- > > > > > > > > drivers/mfd/Kconfig | 11 > > > > > > > > drivers/mfd/Makefile | 1 + > > > > > > > > drivers/mfd/rohm-bd9576.c| 108 > > > > > > > > +++ > > > > > > > > include/linux/mfd/rohm-bd957x.h | 59 > > > > > > > > + > > > > > > > > include/linux/mfd/rohm-generic.h | 2 + > > > > > > > > 5 files changed, 181 insertions(+) > > > > > > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > > > > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > > > > > > > > > > > > > Looks like a possible candidate for "simple-mfd-i2c". > > > > > > > > > > > > > > Could you look into that please? > > > > > > > > > > > > > I must admit I didn't know about "simple-mfd-i2c". Good > > > > > > thing > > > > > > to > > > > > > know > > > > > > when working with simple devices :) Is this a new thing? > > > > > > > > > > Yes, it's new. > > > > > > > > > > > I am unsure I understand the idea fully. Should users put > > > > > > all > > > > > > the > > > > > > different regamp configs in this file and just add the > > > > > > device > > > > > > IDs > > > > > > with > > > > > > pointer to correct config? (BD9576 and BD9573 need volatile > > > > > > ranges). > > > > > > Also, does this mean each sub-device should have own node > > > > > > and > > > > > > own > > > > > > compatible in DT to get correctly load and probed? I guess > > > > > > this > > > > > > would > > > > > > need a buy-in from Rob too then. > > > > > > > > > > You should describe the H/W in DT. > > > > > > > > Yes. And it is described. But I've occasionally received > > > > request > > > > from > > > > DT guys to add some properties directly to MFD node and not to > > > > add > > > > own > > > > sub-node. This is what is done for example with the BD71837/47 > > > > clocks > > > > - > > > > there is no own node for clk - the clk properties are placed > > > > directly > > > > in MFD node (as was requested by Stephen and Rob back then - I > > > > originally had own node for clk). I really have no clear view > > > > on > > > > when > > > > things warrant for own subnode and when they don't - but as far > > > > as > > > > I > > > > can see using simple-mfd-i2c forces one to always have a sub- > > > > node / > > > > device. Even just a empty node with nothing but the compatible > > > > even > > > > if > > > > device does not need stuff from DT? Anyways, I think this is > > > > nice > > > > addition for simple drivers. > >
Re: [PATCH 06/15] dt_bindings: regulator: Add ROHM BD71815 PMIC regulators
On Wed, 2021-01-13 at 07:53 -0600, Rob Herring wrote: > On Tue, Jan 12, 2021 at 08:10:14AM +0200, Matti Vaittinen wrote: > > On Mon, 2021-01-11 at 13:09 -0600, Rob Herring wrote: > > > On Fri, Jan 08, 2021 at 03:36:38PM +0200, Matti Vaittinen wrote: > > > > Add binding documentation for regulators on ROHM BD71815 PMIC. > > > > 5 bucks, 7 LDOs and a boost for LED. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaitti...@fi.rohmeurope.com> > > > > --- > > > > .../regulator/rohm,bd71815-regulator.yaml | 104 > > > > ++ > > > > 1 file changed, 104 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/regulator/rohm,bd71815- > > > > regulator.yaml > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/regulator/rohm,bd71815- > > > > regulator.yaml > > > > b/Documentation/devicetree/bindings/regulator/rohm,bd71815- > > > > regulator.yaml > > > > new file mode 100644 > > > > index ..2aa21603698c > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/regulator/rohm,bd71815- > > > > regulator.yaml > > > > @@ -0,0 +1,104 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > > +%YAML 1.2 > > > > +--- > > > > +$id: > > > > http://devicetree.org/schemas/regulator/rohm,bd71815-regulator.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: ROHM BD71815 Power Management Integrated Circuit > > > > regulators > > > > + > > > > +maintainers: > > > > + - Matti Vaittinen > > > > + > > > > +description: | > > > > + This module is part of the ROHM BD718215 MFD device. For > > > > more > > > > details > > > > + see Documentation/devicetree/bindings/mfd/rohm,bd71815- > > > > pmic.yaml. > > > > + > > > > + The regulator controller is represented as a sub-node of the > > > > PMIC node > > > > + on the device tree. > > > > + > > > > + The valid names for BD71815 regulator nodes are > > > > + buck1, buck2, buck3, buck4, buck5, > > > > + ldo1, ldo2, ldo3, ldo4, ldo5, > > > > + ldodvref, ldolpsr, wled > > > > > > No schema for the last 3? > > > > Thanks Rob. I'm unsure what I have been thinking of :( I'll fix > > this > > for next version. > > > > > > + > > > > +patternProperties: > > > > + "^(ldo|buck)[1-5]$": > > > > +type: object > > > > +description: > > > > + Properties for single LDO/BUCK regulator. > > > > +$ref: regulator.yaml# > > > > + > > > > +properties: > > > > + regulator-name: > > > > +pattern: "^(ldo|buck)[1-5]$" > > > > +description: > > > > + should be "ldo1", ..., "ldo5" and "buck1", ..., > > > > "buck5" > > > > + > > > > + rohm,vsel-gpios: > > > > +description: > > > > + GPIO used to control ldo4 state (when ldo4 is > > > > controlled > > > > by GPIO). > > > > + > > > > + rohm,dvs-run-voltage: > > > > > > These should have a unit suffix. > > > > I know but these are existing properties. I'd like to re-use them > > as > > they have exported parser helpers - and I am unsure what kind of > > breakages changing them would cause. (The BD71837/BD71847 which > > introduced these properties are one of the PMICs which are pretty > > widely used.) > > Okay. Hopefully I remember next time I see this... Actually, I think I can add support for rohm,dvs-run-microvolt and fellows to these same helpers so new devices can use appropriately named properties. That would mean there is duplicate properties for same purpose - but maybe it allows us to eventually deprecate the old ones... Which of these options would you prefer? > > Rob
Re: [PATCH 05/15] dt_bindings: mfd: Add ROHM BD71815 PMIC
On Mon, 2021-01-11 at 13:06 -0600, Rob Herring wrote: > On Fri, Jan 08, 2021 at 03:34:52PM +0200, Matti Vaittinen wrote: > > Document DT bindings for ROHM BD71815. > > > > BD71815 is a single-chip power management IC mainly for battery- > > powered > > portable devices. The IC integrates 5 bucks, 7 LDOs, a boost driver > > for > > LED, a battery charger with a Coulomb counter, a real-time clock, a > > 32kHz > > clock and two general-purpose outputs although only one is > > documented by > > the data-sheet. > > > > Signed-off-by: Matti Vaittinen > > --- > > .../bindings/mfd/rohm,bd71815-pmic.yaml | 198 > > ++ > > 1 file changed, 198 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/mfd/rohm,bd71815-pmic.yaml > > > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd71815- > > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd71815- > > pmic.yaml > > new file mode 100644 > > index ..2206b2008acd > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd71815-pmic.yaml > > @@ -0,0 +1,198 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mfd/rohm,bd71815-pmic.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ROHM BD71815 Power Management Integrated Circuit bindings > > + > > +maintainers: > > + - Matti Vaittinen > > + > > +description: | > > + BD71815AGW is a single-chip power management ICs for battery- > > powered > > + portable devices. It integrates 5 buck converters, 8 LDOs, a > > boost driver > > + for LED and a 500 mA single-cell linear charger. Also included > > is a Coulomb > > + counter, a real-time clock (RTC), and a 32.768 kHz clock gate > > and two GPOs. > > + > > +properties: > > + compatible: > > +const: rohm,bd71815 > > + > > + reg: > > +description: > > + I2C slave address. > > +maxItems: 1 > > + > > + interrupts: > > +maxItems: 1 > > + > > + gpio-controller: true > > Add a blank line here. > > > + "#gpio-cells": > > +const: 2 > > +description: | > > + The first cell is the pin number and the second cell is used > > to specify > > + flags. See ../gpio/gpio.txt for more information. > > + > > + clocks: > > +maxItems: 1 > > And here. > > > + "#clock-cells": > > +const: 0 > > And here. > > > + clock-output-names: > > +const: bd71815-32k-out > > + > > + rohm,clkout-mode: > > +description: clk32kout mode. Can be set to "open-drain" or > > "cmos". > > +$ref: "/schemas/types.yaml#/definitions/string" > > +enum: > > + - open-drain > > + - cmos > > + > > + rohm,charger-sense-resistor-ohms: > > +minimum: 1000 > > +maximum: 5000 > > +description: | > > + BD71827 and BD71828 have SAR ADC for measuring charging > > currents. > > + External sense resistor (RSENSE in data sheet) should be > > used. If some > > + other but 30MOhm resistor is used the resistance value > > should be given > > 'something other' > > Though this can be expressed as 'default: 3000' I guess I'll use both 'something other' and 'default: 3000' in next version. 'default: 3000' is nice for machine parser - but for human reader the 'something other' is likely to be superior. (No scientific test conducted to back off this statement though). Thanks Rob! Best Regards Matti Vaittinen
Re: [PATCH v2 1/3] regulator: mt6360: Add OF match table
On Mon, 2021-01-11 at 13:38 +0100, Matthias Brugger wrote: > Hi Matti, > > On 11/01/2021 11:32, Vaittinen, Matti wrote: > > Hello Matthias & All, > > > > On Mon, 2021-01-11 at 11:08 +0100, Matthias Brugger wrote: > > > On 11/01/2021 03:18, gene_chen(陳俊宇) wrote: > > > > [ Internal Use - External ] > > > > > > > > > > Please don't top-post in the future. > > > > > > > Hi Matthias, > > > > > > > > I discussed OF match table with Mark in previous mail in our > > > > PATCH > > > > v3, > > > > MFD should just instantiate the platform device. > > > > > > Did you ever test that? Which MFD driver should instantiate the > > > device? > > > I had a look at mt6360-core.c (the obvious one) but I don't see > > > any > > > reference to > > > the regulator [1]. What am I missing? > > > > > > > > Mark Brown 於 2020年8月20日 週四 下午7:45寫道： > > > > > > This device only exists in the context of a single parent > > > > > > device, there > > > > > > should be no need for a compatible string here - this is > > > > > > just a > > > > > > detail > > > > > > of how Linux does things. The MFD should just instntiate > > > > > > the > > > > > > platform > > > > > > device. > > > > > Trying to autoload module without of_id_table will cause run- > > > > > time > > > > > error: > > > > > ueventd: LoadWithAliases was unable to load > > > > > of:NregulatorT(null)Cmediatek,mt6360-regulator > > > > > > > > You shouldn't have this described in the device tree at all, > > > > like I > > > > say > > > > the MFD should just instantiate the platform device. > > > > > > > > > > Well from my understanding the regulator has a device-tree entry > > > [2], > > > so it > > > needs to match against a device-tree node. My understanding is, > > > that > > > you need a > > > the devicetree node to describe the regulators provided by the > > > device. TBH I'm a > > > bit puzzled about the comment from Mark here. How does another DT > > > node be able > > > to reference a regulator if this is not described in DT? I think > > > we > > > need a DT > > > node here and the matching in the regulator and MFD driver to get > > > the > > > regulator > > > loaded via udev. > > > > Others are better to answer - but as I spotted this from my inbox > > I'll > > give my 2 cents :) > > > > This can be done W/O regulators having own compatible. Please see > > following: > > > > drivers/mfd/rohm-bd718x7.c > > drivers/regulator/bd718x7-regulator.c > > Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml > > Documentation/devicetree/bindings/regulator/rohm,bd71847- > > regulator.yaml > > > > as example. > > Thanks for the links. Yes that's' a way to go, but... > > > The device matching can be done via platform_device_id table. > > I think the MODULE_ALIAS() is needed for module matching - but I > > don't > > remember without further code browsing. > > > > Anyways, the BD71837/47/50 DT entry without compatible for > > clk/regulators do probe and load the sub-devices. > > > > As a "tradeoff" subdevices must retrieve the DT node from the > > parent > > device. > > > > For my uneducated eyes the DT binding for regulators should be > > changed. > > Compatible should not be required and the example node should be > > moved > > to MFD binding document in the MFD node. But that's just my view on > > this - not willing to push this to any direction! > > > > Generally speaking, DT bindings are stable and can't be changed in a > none-compatible way. We could argue if there is any user of the > compatible out > there (probably there isn't any, as the code seems to not even be > working). I think this depends on the device where driver(s) are used. Quite a few devices allow updating the device-tree when SW is updated. But as I said, I don't want to try pushing this one direction or other - I leave it for those who have broader shoulders for pushing XD. I was merely trying to answer your question :) Best Regards Matti
Re: [PATCH v2 1/3] regulator: mt6360: Add OF match table
Hello Matthias & All, On Mon, 2021-01-11 at 11:08 +0100, Matthias Brugger wrote: > > On 11/01/2021 03:18, gene_chen(陳俊宇) wrote: > > [ Internal Use - External ] > > > > Please don't top-post in the future. > > > Hi Matthias, > > > > I discussed OF match table with Mark in previous mail in our PATCH > > v3, > > MFD should just instantiate the platform device. > > Did you ever test that? Which MFD driver should instantiate the > device? > I had a look at mt6360-core.c (the obvious one) but I don't see any > reference to > the regulator [1]. What am I missing? > > > > Mark Brown 於 2020年8月20日 週四 下午7:45寫道： > > > > This device only exists in the context of a single parent > > > > device, there > > > > should be no need for a compatible string here - this is just a > > > > detail > > > > of how Linux does things. The MFD should just instntiate the > > > > platform > > > > device. > > > Trying to autoload module without of_id_table will cause run-time > > > error: > > > ueventd: LoadWithAliases was unable to load > > > of:NregulatorT(null)Cmediatek,mt6360-regulator > > > > You shouldn't have this described in the device tree at all, like I > > say > > the MFD should just instantiate the platform device. > > > > Well from my understanding the regulator has a device-tree entry [2], > so it > needs to match against a device-tree node. My understanding is, that > you need a > the devicetree node to describe the regulators provided by the > device. TBH I'm a > bit puzzled about the comment from Mark here. How does another DT > node be able > to reference a regulator if this is not described in DT? I think we > need a DT > node here and the matching in the regulator and MFD driver to get the > regulator > loaded via udev. Others are better to answer - but as I spotted this from my inbox I'll give my 2 cents :) This can be done W/O regulators having own compatible. Please see following: drivers/mfd/rohm-bd718x7.c drivers/regulator/bd718x7-regulator.c Documentation/devicetree/bindings/mfd/rohm,bd71847-pmic.yaml Documentation/devicetree/bindings/regulator/rohm,bd71847-regulator.yaml as example. The device matching can be done via platform_device_id table. I think the MODULE_ALIAS() is needed for module matching - but I don't remember without further code browsing. Anyways, the BD71837/47/50 DT entry without compatible for clk/regulators do probe and load the sub-devices. As a "tradeoff" subdevices must retrieve the DT node from the parent device. For my uneducated eyes the DT binding for regulators should be changed. Compatible should not be required and the example node should be moved to MFD binding document in the MFD node. But that's just my view on this - not willing to push this to any direction! Best Regards Matti Vaittinen -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
Re: [PATCH resend] dt-bindings: mfd: mediatek: Add bindings for MT6360 PMIC
Hello Peeps,
On Thu, 2020-12-24 at 11:19 +0800, Gene Chen wrote:
> From: Gene Chen
>
> Add bindings for MT6360 PMIC
>
> Signed-off-by: Gene Chen
> ---
> Documentation/devicetree/bindings/mfd/mt6360.yaml | 69
> +++
> 1 file changed, 69 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/mt6360.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/mt6360.yaml
> b/Documentation/devicetree/bindings/mfd/mt6360.yaml
> new file mode 100644
> index 000..2781c31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mt6360.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/mt6360.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MT6360 PMIC from MediaTek Integrated
> +
> +maintainers:
> + - Gene Chen
> +
> +description: |
> + MT6360 is a PMIC device with the following sub modules.
> + It is interfaced to host controller using I2C interface.
> +
> + This document describes the binding for PMIC device and its sub
> module.
> +
> +properties:
> + compatible:
> +const: mediatek,mt6360
> +
> + reg:
> +description:
> + I2C device address.
> +maxItems: 1
> +
> + wakeup-source: true
> +
> + interrupts:
> +maxItems: 1
> +
> + interrupt-names:
> + enum:
> +- IRQB
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> +const: 1
> +description:
> + The first cell is the IRQ number.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - interrupt-controller
> + - "#interrupt-cells"
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +i2c {
> +#address-cells = <1>;
> +#size-cells = <0>;
> +
> +mt6360@34 {
> +compatible = "mediatek,mt6360";
> +reg = <0x34>;
> +wakeup-source;
> +interrupts-extended = < 0 IRQ_TYPE_LEVEL_LOW>;
> +interrupt-names = "IRQB";
> +interrupt-controller;
> +#interrupt-cells = <1>;
Out of the curiosity - is this the complete node? I would assume some
sub-devices like regulators here?
> +};
> +};
Re: [PATCH 09/15] gpio: support ROHM BD71815 GPOs
On Mon, 2021-01-11 at 08:15 +0200, Matti Vaittinen wrote:
> On Sat, 2021-01-09 at 01:45 +0100, Linus Walleij wrote:
> > On Fri, Jan 8, 2021 at 2:39 PM Matti Vaittinen
> > wrote:
> >
> > > Support GPO(s) found from ROHM BD71815 power management IC. The
> > > IC
> > > has two
> > > GPO pins but only one is properly documented in data-sheet. The
> > > driver
> > > exposes by default only the documented GPO. The second GPO is
> > > connected to
> > > E5 pin and is marked as GND in data-sheet. Control for this
> > > undocumented
> > > pin can be enabled using a special DT property.
> > >
> > > This driver is derived from work by Peter Yang <
> > > yang...@embest-tech.com>
> > > although not so much of original is left.
> > >
> > > Signed-off-by: Matti Vaittinen > > >
// Snip
> > > + g->chip.parent = pdev->dev.parent;
> > > + g->chip.of_node = pdev->dev.parent->of_node;
> > > + g->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > + g->dev = >dev;
> > > +
> > > + ret = devm_gpiochip_add_data(>dev, >chip, g);
> > > + if (ret < 0) {
> > > + dev_err(>dev, "could not register gpiochip,
> > > %d\n", ret);
> > > + return ret;
> > > + }
> >
> > It's a bit confusing how you use pdev->dev.parent for some stuff
> > and >dev for some.
> >
> > What about assinging
> >
> > struct device *dev = pdev->dev.parent;
> >
> > and use dev for all the calls, it looks like it'd work fine.
>
> I wouldn't bind the lifetime of devm functions to the parent device.
> I
> am not sure if it would work - what happens we bind lifetime of XX to
> parent device - and next call at probe fails (for example with
> DEFERRED?) I _assume_ the XX bound to parent is not released?
> (Please,
> do correct me if I am wrong!)
/*
* Bind devm lifetime to this platform device => use dev for devm.
* also the prints should originate from this device.
*/
dev = >dev;
/* The device-tree and regmap come from MFD => use parent for that */
parent = dev->parent;
Maybe adding dev and parent variables + comments would clear it up?
Br,
Matti Vaittinen
Re: [PATCH 09/15] gpio: support ROHM BD71815 GPOs
Hi Linus,
Thanks a lot for review!
On Sat, 2021-01-09 at 01:45 +0100, Linus Walleij wrote:
> On Fri, Jan 8, 2021 at 2:39 PM Matti Vaittinen
> wrote:
>
> > Support GPO(s) found from ROHM BD71815 power management IC. The IC
> > has two
> > GPO pins but only one is properly documented in data-sheet. The
> > driver
> > exposes by default only the documented GPO. The second GPO is
> > connected to
> > E5 pin and is marked as GND in data-sheet. Control for this
> > undocumented
> > pin can be enabled using a special DT property.
> >
> > This driver is derived from work by Peter Yang <
> > yang...@embest-tech.com>
> > although not so much of original is left.
> >
> > Signed-off-by: Matti Vaittinen
>
> Overall this looks good!
>
> > + depends on MFD_ROHM_BD71828
>
> I suppose this makes i possible to merge out-of-order with the
> core patches actually.
Actually not. MFD_ROHM_BD71828 is existing config as this BD71815 uses
same MFD driver with BD71828. So MFD headers should be in before
merging the depending sub-device drivers.
>
> > +#define DEBUG
>
> Why? Development artifact?
Ouch. Thanks for spotting it :) I'll get rid of that.
> > +#include
>
> You certainly do not need this.
>
> > +#include
> > +#include
>
> I guess registers come from these? Do you need both?
> Add a comment about what they provide.
Ok. Can do. (registers, I will recheck if I can get rid of including
the rohm-generic)
>
> > + g->chip.ngpio = 1;
> > + if (g->e5_pin_is_gpo)
> > + g->chip.ngpio = 2;
>
> Overwriting value, how not elegant.
>
> if (g->e5_pin_is_gpo)
> g->chip.ngpio = 2;
> else
> g->chip.ngpio = 1;
matter of taste I'd say :) As I would say about functions named like
_foo() ;) Not a poin I would fight over though - I can change this :]
> > + g->chip.parent = pdev->dev.parent;
> > + g->chip.of_node = pdev->dev.parent->of_node;
> > + g->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > + g->dev = >dev;
> > +
> > + ret = devm_gpiochip_add_data(>dev, >chip, g);
> > + if (ret < 0) {
> > + dev_err(>dev, "could not register gpiochip,
> > %d\n", ret);
> > + return ret;
> > + }
>
> It's a bit confusing how you use pdev->dev.parent for some stuff
> and >dev for some.
>
> What about assinging
>
> struct device *dev = pdev->dev.parent;
>
> and use dev for all the calls, it looks like it'd work fine.
I wouldn't bind the lifetime of devm functions to the parent device. I
am not sure if it would work - what happens we bind lifetime of XX to
parent device - and next call at probe fails (for example with
DEFERRED?) I _assume_ the XX bound to parent is not released? (Please,
do correct me if I am wrong!)
Br,
Matti Vaittinen
Re: [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828
On Thu, 2021-01-07 at 20:15 +0200, Matti Vaittinen wrote: > Thanks a lot for taking a careful look at this Guenter! > > On Thu, 2021-01-07 at 07:12 -0800, Guenter Roeck wrote: > > On Thu, Jan 07, 2021 at 08:37:03AM +0200, Matti Vaittinen wrote: > > > If config for BD70528 watchdog is enabled when BD71828 or BD71815 > > > are used the RTC module will issue call to BD70528 watchdog with > > > NULL data. Ignore this call and don't crash. > > > > > > Signed-off-by: Matti Vaittinen > > > > > > > I really think this should be handled in the calling code. > > Also, I am curious how this is supposed to work. > > > > The code is called with > > > > ret = bd70528_wdt_set(r->parent, new_state & > > BD70528_WDT_STATE_BIT, > > old_state); > > My brainfart. > The bd70528_wdt_set is not called as it is protected in RTC by > has_rtc_timers flag. > > I inserted this check in wrong function. The bd70528_wdt_lock() > is where we may hit the problem as it is not protected. Actually, after a fresh look - it seems the bd70528_wdt_lock() is also just fine. The RTC is not grabbing the lock on other PMICs but the BD70528. I'm not really sure what I have been thinking of. @_@ I should've been more careful. Thanks for spotting this in the review! Best Regards Matti
Re: [PATCH 1/2] watchdog: bd70528: don't crash if WDG is confiured with BD71828
Thanks a lot for taking a careful look at this Guenter! On Thu, 2021-01-07 at 07:12 -0800, Guenter Roeck wrote: > On Thu, Jan 07, 2021 at 08:37:03AM +0200, Matti Vaittinen wrote: > > If config for BD70528 watchdog is enabled when BD71828 or BD71815 > > are used the RTC module will issue call to BD70528 watchdog with > > NULL data. Ignore this call and don't crash. > > > > Signed-off-by: Matti Vaittinen > > I really think this should be handled in the calling code. > Also, I am curious how this is supposed to work. > > The code is called with > > ret = bd70528_wdt_set(r->parent, new_state & > BD70528_WDT_STATE_BIT, > old_state); My brainfart. The bd70528_wdt_set is not called as it is protected in RTC by has_rtc_timers flag. I inserted this check in wrong function. The bd70528_wdt_lock() is where we may hit the problem as it is not protected. > > from bd70528_set_rtc_based_timers(). That same function subsequently > calls bd70528_set_elapsed_tmr() with the same parameter, and that > parameter is dereferenced in bd70528_set_elapsed_tmr() without > checking. > > Conceptually, it should not be necessary to determine at compile-time > which of the chips is in the system. It should be posible to compile > a single kernel which supports all chips. I agree. The information whether WDT should be accessed should be judged by dt-compatible. MFD has this knowledge and passes it to RTC. So yes, RTC should omit the call if BD70528 is not used. Please ignore these patches, I'll do changes to RTC driver :) Best Regards Matti
Re: [PATCH] gpio: bd7xxxx: use helper variable for pdev->dev
Thanks for making this better :) On Wed, 2021-01-06 at 11:11 +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > Using a helper local variable to store the address of >dev adds > to readability and allows us to avoid unnecessary line breaks. > > Signed-off-by: Bartosz Golaszewski Reviewed-by: Matti Vaittinen
Re: [PATCH RESEND v6 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
Hello Again peeps, On Thu, 2020-12-17 at 12:04 +0200, Matti Vaittinen wrote: > On Wed, 2020-12-02 at 15:32 +0200, Matti Vaittinen wrote: > > Hello Lee, > > > > On Wed, 2020-12-02 at 12:57 +, Lee Jones wrote: > > > On Fri, 27 Nov 2020, Vaittinen, Matti wrote: > > > > > > > Hello Lee, > > > > > > > > On Fri, 2020-11-27 at 08:32 +, Lee Jones wrote: > > > > > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs > > > > > > which > > > > > > are > > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > > matti.vaitti...@fi.rohmeurope.com> > > > > > > --- > > > > > > drivers/mfd/Kconfig | 11 > > > > > > drivers/mfd/Makefile | 1 + > > > > > > drivers/mfd/rohm-bd9576.c| 108 > > > > > > +++ > > > > > > include/linux/mfd/rohm-bd957x.h | 59 + > > > > > > include/linux/mfd/rohm-generic.h | 2 + > > > > > > 5 files changed, 181 insertions(+) > > > > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > > > > > > > > > Looks like a possible candidate for "simple-mfd-i2c". > > > > > > > > > > Could you look into that please? > > > > > > > > > I must admit I didn't know about "simple-mfd-i2c". Good thing > > > > to > > > > know > > > > when working with simple devices :) Is this a new thing? > > > > > > Yes, it's new. > > > > > > > I am unsure I understand the idea fully. Should users put all > > > > the > > > > different regamp configs in this file and just add the device > > > > IDs > > > > with > > > > pointer to correct config? (BD9576 and BD9573 need volatile > > > > ranges). > > > > Also, does this mean each sub-device should have own node and > > > > own > > > > compatible in DT to get correctly load and probed? I guess this > > > > would > > > > need a buy-in from Rob too then. > > > > > > You should describe the H/W in DT. > > > > Yes. And it is described. But I've occasionally received request > > from > > DT guys to add some properties directly to MFD node and not to add > > own > > sub-node. This is what is done for example with the BD71837/47 > > clocks > > - > > there is no own node for clk - the clk properties are placed > > directly > > in MFD node (as was requested by Stephen and Rob back then - I > > originally had own node for clk). I really have no clear view on > > when > > things warrant for own subnode and when they don't - but as far as > > I > > can see using simple-mfd-i2c forces one to always have a sub-node / > > device. Even just a empty node with nothing but the compatible even > > if > > device does not need stuff from DT? Anyways, I think this is nice > > addition for simple drivers. > > > > > > By the way - for uneducated eyes like mine this does not look > > > > like > > > > it > > > > has much to do with MFD as a device - here MFD reminds me of a > > > > simple- > > > > bus on top of I2C. > > > > > > This is for MFD devices where the parent does little more than > > > create > > > a shared address space for child devices to operate on - like > > > yours. > > > > > > > Anyways, the BD9576 and BD9573 both have a few interrupts for > > > > OVD/UVD > > > > conditions and I am expecting that I will be asked to provide > > > > the > > > > regulator notifiers for those. Reason why I omitted the IRQs > > > > for > > > > now is > > > > that the HW is designed to keep the IRQ asserted for whole > > > > error > > > > duration so some delayed ack mechanism would be needed. I would > > > > like to > > > > keep the door open for adding IRQs to MFD core. > > > > > > You mean to add an IRQ Domain? > > > > Yes. I planned to use re
Re: [PATCH v4 12/12] mfd: bd9571mwv: Add support for BD9574MWF
On Tue, 2020-12-22 at 09:23 +, Yoshihiro Shimoda wrote:
> Hi Geert-san,
>
> Thank you for your review!
>
> > From: Geert Uytterhoeven, Sent: Tuesday, December 22, 2020 5:53 PM
> > On Mon, Dec 21, 2020 at 3:56 AM Yoshihiro Shimoda
> > wrote:
>
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> > > @@ -200,12 +277,14 @@ static int bd9571mwv_probe(struct
> > > i2c_client *client,
> > >
> > > static const struct of_device_id bd9571mwv_of_match_table[] = {
> > > { .compatible = "rohm,bd9571mwv", },
> > > + { .compatible = "rohm,bd9574mwf", },
> > > { /* sentinel */ }
> > > };
> > > MODULE_DEVICE_TABLE(of, bd9571mwv_of_match_table);
> > >
> > > static const struct i2c_device_id bd9571mwv_id_table[] = {
> > > - { "bd9571mwv", 0 },
> > > + { "bd9571mwv", ROHM_CHIP_TYPE_BD9571 },
> > > + { "bd9574mwf", ROHM_CHIP_TYPE_BD9574 },
> >
> > Why add the chip types? These are unused, and the driver uses
> > autodetection of the chip variant anyway.
>
> I just added the chip types in the future use. As you said,
> these are unused and we should not add a future use in general.
> So, I'll remove this change.
>
> Also, I think I should remove the following patch.
>
> [PATCH v4 08/12] gpio: bd9571mwv: Add BD9574MWF support
I think this depends on whether you wish to add support for the
> "RECOV_GPOUT", "FREQSEL" and "RTC_IN",
which you mention in GPIO commit message. If you plan on adding the
support, then you need to differentiate the ICs on GPIO driver, right?
Best Regards
Matti Vaittinen
Re: [PATCH v4 10/12] mfd: bd9571mwv: Use devm_regmap_add_irq_chip()
On Tue, 2020-12-22 at 09:25 +, Yoshihiro Shimoda wrote: > Hi Matti-san, > > > From: Vaittinen, Matti, Sent: Tuesday, December 22, 2020 6:15 PM > > > > On Tue, 2020-12-22 at 09:49 +0100, Geert Uytterhoeven wrote: > > > On Mon, Dec 21, 2020 at 3:57 AM Yoshihiro Shimoda > > > wrote: > > > > Use dev_regmap_add_irq_chip() to simplify the code. > > > > > > > > Signed-off-by: Yoshihiro Shimoda < > > > > yoshihiro.shimoda...@renesas.com> > > > > Acked-for-MFD-by: Lee Jones > > > > > > Reviewed-by: Geert Uytterhoeven > > Reviewed-by: Matti Vaittinen > > Thank you for your review! > > > I thought I did review this earlier... > > You're correct. I'm sorry, I completely overlooked your Reviewed-by > tag in previous. It happens :) But of you re-spin the series, please revise the tags for patches. I think I did review almost all of them (except the SPDX- patches as I am not competent on licenses...) > > Best regards, > Yoshihiro Shimoda >
Re: [PATCH v4 10/12] mfd: bd9571mwv: Use devm_regmap_add_irq_chip()
On Tue, 2020-12-22 at 09:49 +0100, Geert Uytterhoeven wrote: > On Mon, Dec 21, 2020 at 3:57 AM Yoshihiro Shimoda > wrote: > > Use dev_regmap_add_irq_chip() to simplify the code. > > > > Signed-off-by: Yoshihiro Shimoda > > Acked-for-MFD-by: Lee Jones > > Reviewed-by: Geert Uytterhoeven Reviewed-by: Matti Vaittinen I thought I did review this earlier... Br, Matti Vaittinen
Re: [PATCH v4 01/12] mfd: bd9571mwv: Use devm_mfd_add_devices()
On Tue, 2020-12-22 at 09:41 +0100, Geert Uytterhoeven wrote:
> On Mon, Dec 21, 2020 at 3:57 AM Yoshihiro Shimoda
> wrote:
> > To remove mfd devices when unload this driver, should use
> > devm_mfd_add_devices() instead.
> >
> > Fixes: d3ea21272094 ("mfd: Add ROHM BD9571MWV-M MFD PMIC driver")
> > Signed-off-by: Yoshihiro Shimoda
> > Acked-for-MFD-by: Lee Jones
>
> Reviewed-by: Geert Uytterhoeven
Reviewed-by: Matti Vaittinen
Re: [PATCH] lib/linear_ranges: fix repeated words & one typo
Thanks for corrections! On Sun, 2020-12-20 at 20:06 -0800, Randy Dunlap wrote: > Change "which which" to "for which" in 3 places. > Change "ranges" to possessive "range's" in 1 place. > > Signed-off-by: Randy Dunlap > Cc: Andrew Morton > Cc: Mark Brown > Cc: Matti Vaittinen Reviewed-by: Matti Vaittinen Best Regards Matti
Re: [PATCH v2] regulators: bd718x7: Add enable times
On Fri, 2020-12-18 at 19:38 +0100, Guido Günther wrote: > Use the typical startup times from the data sheet so boards get a > reasonable default. Not setting any enable time can lead to board > hangs > when e.g. clocks are enabled too soon afterwards. > > This fixes gpu power domain resume on the Librem 5. > > Signed-off-by: Guido Günther > > --- > v2 > - As per review comment by Matti Vaittinen > > https://lore.kernel.org/lkml/beba25e85db20649aa040fc0ae549895c9265f6f.ca...@fi.rohmeurope.com/ > Use defines instead of plain values > - As per review comment by Mark Brown > https://lore.kernel.org/lkml/20201216130637.gc4...@sirena.org.uk/ > Drop cover letter > --- > drivers/regulator/bd718x7-regulator.c | 27 > include/linux/mfd/rohm-bd718x7.h | 30 Thanks again Guido. I might have preferred having the defines in bd718x7-regulator.c as they are not likely to be used outside this file. That would have strictly limited the change to regulator subsystem. Having them in the header is still fine with me if it works for Mark & others. (I don't think these defines need much of changes in the future). Reviewed-by: Matti Vaittinen Best Regards Matti Vaittinen
Re: [PATCH 1/1] regulators: bd718x7: Add enable times
On Wed, 2020-12-16 at 13:41 +0100, Guido Günther wrote:
> Hi Matti,
> On Wed, Dec 16, 2020 at 12:29:20PM +, Vaittinen, Matti wrote:
> > Hello Guido,
> >
> > Thanks for looking at this!
> >
> > On Wed, 2020-12-16 at 12:05 +0100, Guido Günther wrote:
> > > Use the typical startup times from the data sheet so boards get a
> > > reasonable default. Not setting any enable time can lead to board
> > > hangs
> > > when e.g. clocks are enabled too soon afterwards.
> > >
> > > This fixes gpu power domain resume on the Librem 5.
> > >
> > > Signed-off-by: Guido Günther
> > > ---
> > > drivers/regulator/bd718x7-regulator.c | 27
> > > +++
> > > 1 file changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/regulator/bd718x7-regulator.c
> > > b/drivers/regulator/bd718x7-regulator.c
> > > index e6d5d98c3cea..d6d34aa4ee2e 100644
> > > --- a/drivers/regulator/bd718x7-regulator.c
> > > +++ b/drivers/regulator/bd718x7-regulator.c
> > > @@ -613,6 +613,7 @@ static struct bd718xx_regulator_data
> > > bd71847_regulators[] = {
> > > .vsel_mask = DVS_BUCK_RUN_MASK,
> > > .enable_reg = BD718XX_REG_BUCK1_CTRL,
> > > .enable_mask = BD718XX_BUCK_EN,
> > > + .enable_time = 144,
> >
> > Where are these values obtained from? I have a feeling they might
> > be
> > board / load specific. If this is the case - can the "regulator-
> > enable-
> > ramp-delay" from device-tree be used instead to avoid hard-coding
> > board
> > specific values in the driver? Although, sane defaults would
> > probably
> > not be a bad idea - if I read code correctly then the constrains
> > from
> > DT can be used to override these values.
>
> They're the 'typical values' from the data sheet and it's basically
> all
> about setting a default for "regulator-enable-ramp-delay" to avoid
> having every board do the same. If that's not the right thing todo
> let
> me know and i add these to each of our boards (which is where i
> basically started from but then figured that this would be busywork
> and every board would hit that problem).
>
> > I'd prefer well named defines over raw numeric values though.
>
> So s.th. like
>
> BD71837_BUCK1_STARTUP_TIME 144
>
> (using the terminology from the datasheet)? If that works I'll send a
> v2.
Just noticed I never replied. Sorry. This looks good to me!
Best Regards
--Matti
Re: [PATCH RESEND v6 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
Hi deee Ho Lee, On Wed, 2020-12-02 at 15:32 +0200, Matti Vaittinen wrote: > Hello Lee, > > On Wed, 2020-12-02 at 12:57 +, Lee Jones wrote: > > On Fri, 27 Nov 2020, Vaittinen, Matti wrote: > > > > > Hello Lee, > > > > > > On Fri, 2020-11-27 at 08:32 +, Lee Jones wrote: > > > > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which > > > > > are > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > matti.vaitti...@fi.rohmeurope.com> > > > > > --- > > > > > drivers/mfd/Kconfig | 11 > > > > > drivers/mfd/Makefile | 1 + > > > > > drivers/mfd/rohm-bd9576.c| 108 > > > > > +++ > > > > > include/linux/mfd/rohm-bd957x.h | 59 + > > > > > include/linux/mfd/rohm-generic.h | 2 + > > > > > 5 files changed, 181 insertions(+) > > > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > > > > > > > Looks like a possible candidate for "simple-mfd-i2c". > > > > > > > > Could you look into that please? > > > > > > > I must admit I didn't know about "simple-mfd-i2c". Good thing to > > > know > > > when working with simple devices :) Is this a new thing? > > > > Yes, it's new. > > > > > I am unsure I understand the idea fully. Should users put all the > > > different regamp configs in this file and just add the device IDs > > > with > > > pointer to correct config? (BD9576 and BD9573 need volatile > > > ranges). > > > Also, does this mean each sub-device should have own node and own > > > compatible in DT to get correctly load and probed? I guess this > > > would > > > need a buy-in from Rob too then. > > > > You should describe the H/W in DT. > > Yes. And it is described. But I've occasionally received request from > DT guys to add some properties directly to MFD node and not to add > own > sub-node. This is what is done for example with the BD71837/47 clocks > - > there is no own node for clk - the clk properties are placed directly > in MFD node (as was requested by Stephen and Rob back then - I > originally had own node for clk). I really have no clear view on when > things warrant for own subnode and when they don't - but as far as I > can see using simple-mfd-i2c forces one to always have a sub-node / > device. Even just a empty node with nothing but the compatible even > if > device does not need stuff from DT? Anyways, I think this is nice > addition for simple drivers. > > > > By the way - for uneducated eyes like mine this does not look > > > like > > > it > > > has much to do with MFD as a device - here MFD reminds me of a > > > simple- > > > bus on top of I2C. > > > > This is for MFD devices where the parent does little more than > > create > > a shared address space for child devices to operate on - like > > yours. > > > > > Anyways, the BD9576 and BD9573 both have a few interrupts for > > > OVD/UVD > > > conditions and I am expecting that I will be asked to provide the > > > regulator notifiers for those. Reason why I omitted the IRQs for > > > now is > > > that the HW is designed to keep the IRQ asserted for whole error > > > duration so some delayed ack mechanism would be needed. I would > > > like to > > > keep the door open for adding IRQs to MFD core. > > > > You mean to add an IRQ Domain? > > Yes. I planned to use regmap-irq and create irq chip in MFD when the > over / under voltage / temperature - notifications or watchdog IRQs > are > needed. I am sorry if I have missed your reply. The ROHM email had redirected almost all patch emails to spam + I am not sure if some mails are dropping :( (I am considering moving to gmail - but I'd rather keep all mails in one system and I can't transfer work mail traffic to gmail... I wonder how others are managing the mails - which mail system you are using?) I think this series is now pending the decision how to proceed with MFD part. If you still want me to start with "simple-mfd-i2c", then I would appreciate if you pointed me how you would like to see the regmap configs added. Although I am quite positive this (eventually) ends up being more than what simple-mfd-i2c is intended for (because at some point people want to add the use of the interrupts). Best Regards Matti Vaittinen
Re: [RFC PATCH v2 4/6] mfd: add BD71827 header
On Wed, 2020-12-16 at 08:53 +, Lee Jones wrote: > On Fri, 04 Dec 2020, Matti Vaittinen wrote: > > > Add BD71827 driver header. For a record - Header is originally > > based on work authored by Cong Pham although not much of original > > work is left now. > > > > Signed-off-by: Matti Vaittinen > > --- > > This patch was not in v1. This brings in some charger registers > > for the BD71827 charger which is in following patches. > > > > include/linux/mfd/rohm-bd71827.h | 295 > > +++ > > 1 file changed, 295 insertions(+) > > create mode 100644 include/linux/mfd/rohm-bd71827.h > > > > diff --git a/include/linux/mfd/rohm-bd71827.h > > b/include/linux/mfd/rohm-bd71827.h > > new file mode 100644 > > index ..0f5a343b10ae > > --- /dev/null > > +++ b/include/linux/mfd/rohm-bd71827.h > > @@ -0,0 +1,295 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright 2016 > > Out of date. ... Thanks for the review Lee! It was unexpected as I didn't add you to CC. Reason for skipping you is that this was very initial RFC patch set - intended to collect opinions of adding some fuel-gauge logic in power- supply. I will for sure clean up the findings (and possibly some other issues) when I proceed with these charger drivers - and at that point I will ask for a re-review & add you in CC. But I just wanted to point out that as the fate/format of this driver depends on how the RFC is seen by Sebastian & others so getting back to this may take some time... BTW - you have nice mail-filters as you caught these pathces, you pick every mail with MFD in subject(?) Best Regards Matti -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
Re: [PATCH 1/1] regulators: bd718x7: Add enable times
On Wed, 2020-12-16 at 12:48 +, Mark Brown wrote: > On Wed, Dec 16, 2020 at 01:41:33PM +0100, Guido Günther wrote: > > On Wed, Dec 16, 2020 at 12:29:20PM +, Vaittinen, Matti wrote: > > > > + .enable_time = 144, > > > Where are these values obtained from? I have a feeling they might > > > be > > > board / load specific. If this is the case - can the "regulator- > > > enable- > > > ramp-delay" from device-tree be used instead to avoid hard-coding > > > board > > > specific values in the driver? Although, sane defaults would > > > probably > > > not be a bad idea - if I read code correctly then the constrains > > > from > > > DT can be used to override these values. > > They're the 'typical values' from the data sheet and it's basically > > all > > about setting a default for "regulator-enable-ramp-delay" to avoid > > having every board do the same. If that's not the right thing todo > > let > > me know and i add these to each of our boards (which is where i > > basically started from but then figured that this would be busywork > > and every board would hit that problem). > > If it's a DCDC they're *probably* generic values rather than board > specific, they tend to be more predictable as they're regulating much > more actively than LDOs. Thank you for explanation Mark & Guido. Then I'd say the 'default' enable times do make sense. Thanks for improving the driver! Best Regards Matti Vaittinen
Re: [PATCH 1/1] regulators: bd718x7: Add enable times
Hello Guido,
Thanks for looking at this!
On Wed, 2020-12-16 at 12:05 +0100, Guido Günther wrote:
> Use the typical startup times from the data sheet so boards get a
> reasonable default. Not setting any enable time can lead to board
> hangs
> when e.g. clocks are enabled too soon afterwards.
>
> This fixes gpu power domain resume on the Librem 5.
>
> Signed-off-by: Guido Günther
> ---
> drivers/regulator/bd718x7-regulator.c | 27
> +++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/regulator/bd718x7-regulator.c
> b/drivers/regulator/bd718x7-regulator.c
> index e6d5d98c3cea..d6d34aa4ee2e 100644
> --- a/drivers/regulator/bd718x7-regulator.c
> +++ b/drivers/regulator/bd718x7-regulator.c
> @@ -613,6 +613,7 @@ static struct bd718xx_regulator_data
> bd71847_regulators[] = {
> .vsel_mask = DVS_BUCK_RUN_MASK,
> .enable_reg = BD718XX_REG_BUCK1_CTRL,
> .enable_mask = BD718XX_BUCK_EN,
> + .enable_time = 144,
Where are these values obtained from? I have a feeling they might be
board / load specific. If this is the case - can the "regulator-enable-
ramp-delay" from device-tree be used instead to avoid hard-coding board
specific values in the driver? Although, sane defaults would probably
not be a bad idea - if I read code correctly then the constrains from
DT can be used to override these values.
I'd prefer well named defines over raw numeric values though.
Best Regards
Matti Vaittinen
Re: [PATCH v3 12/12] mfd: bd9571mwv: Add support for BD9574MWF
On Wed, 2020-12-16 at 09:02 +, Lee Jones wrote: > On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen > > > > > > The new PMIC BD9574MWF inherits features from BD9571MWV. > > > Add the support of new PMIC to existing bd9571mwv driver. > > > > > > Signed-off-by: Khiem Nguyen > > > [shimoda: rebase and refactor] > > > Signed-off-by: Yoshihiro Shimoda < > > > yoshihiro.shimoda...@renesas.com> > > > > Reviewed-by: Matti Vaittinen > > > > > --- > > > drivers/mfd/bd9571mwv.c | 86 > > > ++- > > > include/linux/mfd/bd9571mwv.h | 18 +++-- > > > 2 files changed, 99 insertions(+), 5 deletions(-) > > ... and please snip long replies. > > Scrolling down past lots of un-reviewed code and/or past the end of > the last review comment is a waste of people's time. Thanks. > Hm. Right. I thought that leaving whole patch there when just adding ack or reviewed-by gives full information as to which exact patch version was reviewed. But I admit scrolling can be annoying - besides, the send/receive time in quote can probably be used to track the excat version which was reviewed. So point taken. Thanks. Best Regards Matti
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 2020-12-16 at 09:00 +, Lee Jones wrote: > On Wed, 16 Dec 2020, Vaittinen, Matti wrote: > > > On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen > > > > > > Since the driver supports BD9571MWV PMIC only, > > > this patch makes the functions and data structure become more > > > generic > > > so that it can support other PMIC variants as well. > > > > > > Signed-off-by: Khiem Nguyen > > > [shimoda: rebase and refactor] > > > Signed-off-by: Yoshihiro Shimoda < > > > yoshihiro.shimoda...@renesas.com> > > > > Reviewed-by: Matti Vaittinen > > Please place any *-by tags *after* the other comments. > > Fortunately, the first one below was still on my screen, else I would > have stopped reading here. > Good point. I'd better keep that on mind. Although missing stuff after a Reviewed-by is not that fatal ;) > > > --- > > > drivers/mfd/bd9571mwv.c | 95 +++ > > > > > > > > > include/linux/mfd/bd9571mwv.h | 18 ++-- > > > 2 files changed, 63 insertions(+), 50 deletions(-)
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 2020-12-16 at 10:25 +0200, Matti Vaittinen wrote:
> On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote:
> > From: Khiem Nguyen
> >
> > Since the driver supports BD9571MWV PMIC only,
> > this patch makes the functions and data structure become more
> > generic
> > so that it can support other PMIC variants as well.
> >
> > Signed-off-by: Khiem Nguyen
> > [shimoda: rebase and refactor]
> > Signed-off-by: Yoshihiro Shimoda
>
> Reviewed-by: Matti Vaittinen
>
> > ---
> > drivers/mfd/bd9571mwv.c | 95 +++
> >
> > include/linux/mfd/bd9571mwv.h | 18 ++--
> > 2 files changed, 63 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > index 49e968e..ccf1a60 100644
> > --- a/drivers/mfd/bd9571mwv.c
> > +++ b/drivers/mfd/bd9571mwv.c
> > @@ -3,6 +3,7 @@
> > * ROHM BD9571MWV-M MFD driver
> > *
> > * Copyright (C) 2017 Marek Vasut
> > + * Copyright (C) 2020 Renesas Electronics Corporation
> > *
> > * Based on the TPS65086 driver
> > */
> > @@ -14,6 +15,19 @@
> >
> > #include
> >
> > +/**
> > + * struct bd957x_data - internal data for the bd957x driver
> > + *
> > + * Internal data to distinguish bd957x variants
> > + */
> > +struct bd957x_data {
> > + char *part_name;
> > + const struct regmap_config *regmap_config;
> > + const struct regmap_irq_chip *irq_chip;
> > + const struct mfd_cell *cells;
> > + int num_cells;
> > +};
> > +
>
> I do like the way you placed the variant data in owns structs. Well
> thought.
>
> > static const struct mfd_cell bd9571mwv_cells[] = {
> > { .name = "bd9571mwv-regulator", },
> > { .name = "bd9571mwv-gpio", },
> > @@ -102,13 +116,21 @@ static struct regmap_irq_chip
> > bd9571mwv_irq_chip = {
> > .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> > };
> >
> > -static int bd9571mwv_identify(struct bd9571mwv *bd)
> > +static const struct bd957x_data bd9571mwv_data = {
> > + .part_name = BD9571MWV_PART_NAME,
> > + .regmap_config = _regmap_config,
> > + .irq_chip = _irq_chip,
> > + .cells = bd9571mwv_cells,
> > + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > +};
> > +
> > +static int bd9571mwv_identify(struct device *dev, struct regmap
> > *regmap,
> > + const char *part_name)
> > {
> > - struct device *dev = bd->dev;
> > unsigned int value;
> > int ret;
> >
> > - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, );
> > + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, );
> > if (ret) {
> > dev_err(dev, "Failed to read vendor code register
> > (ret=%i)\n",
> > ret);
> > @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct
> > bd9571mwv
> > *bd)
> > return -EINVAL;
> > }
> >
> > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, );
> > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, );
> > if (ret) {
> > dev_err(dev, "Failed to read product code register
> > (ret=%i)\n",
> > ret);
> > return ret;
> > }
> > -
> > - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> > - dev_err(dev, "Invalid product code ID %02x (expected
> > %02x)\n",
> > - value, BD9571MWV_PRODUCT_CODE_VAL);
> > - return -EINVAL;
> > - }
> > -
> > - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION,
> > );
> > + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, );
> > if (ret) {
> > dev_err(dev, "Failed to read revision register
> > (ret=%i)\n",
> > ret);
> > return ret;
> > }
> >
> > - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> > + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
> >
> > return 0;
> > }
> > @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct
> > bd9571mwv
> > *bd)
> > static int bd9571mwv_probe(struct i2c_client *client,
> > const struct i2c_device_id *ids)
> > {
> > - struct bd9571mwv *bd;
> > - int ret;
> > -
> > - bd = devm_kzalloc(>dev, sizeof(*bd), GFP_KERNEL);
> > - if (!bd)
> > - return -ENOMEM;
> > -
> > - i2c_set_clientdata(client, bd);
> > - bd->dev = >dev;
> > - bd->irq = client->irq;
> > + const struct bd957x_data *data;
> > + struct device *dev = >dev;
> > + struct regmap *regmap;
> > + struct regmap_irq_chip_data *irq_data;
> > + int ret, irq = client->irq;
> > +
> > + /* Read the PMIC product code */
> > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
>
> Having to use the i2c_smbus_read_byte_data for a device which is
> going
> to be used with regmap slightly bugs me. But as you want to do the
> runtime probing, then this access must be done prior regmap
> registration - so I can't think of a better way. :(
I just noticed that reading the product code is something one would
expect the bd9571mwv_identify() be
Re: [PATCH v3 12/12] mfd: bd9571mwv: Add support for BD9574MWF
On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote:
> From: Khiem Nguyen
>
> The new PMIC BD9574MWF inherits features from BD9571MWV.
> Add the support of new PMIC to existing bd9571mwv driver.
>
> Signed-off-by: Khiem Nguyen
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Matti Vaittinen
> ---
> drivers/mfd/bd9571mwv.c | 86
> ++-
> include/linux/mfd/bd9571mwv.h | 18 +++--
> 2 files changed, 99 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index ccf1a60..f660de6 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * ROHM BD9571MWV-M MFD driver
> + * ROHM BD9571MWV-M and BD9574MVF-M MFD driver
> *
> * Copyright (C) 2017 Marek Vasut
> * Copyright (C) 2020 Renesas Electronics Corporation
> @@ -11,6 +11,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -28,6 +29,7 @@ struct bd957x_data {
> int num_cells;
> };
>
> +/* For BD9571MWV */
> static const struct mfd_cell bd9571mwv_cells[] = {
> { .name = "bd9571mwv-regulator", },
> { .name = "bd9571mwv-gpio", },
> @@ -124,6 +126,81 @@ static const struct bd957x_data bd9571mwv_data =
> {
> .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> };
>
> +/* For BD9574MWF */
> +static const struct mfd_cell bd9574mwf_cells[] = {
> + { .name = "bd9574mwf-regulator", },
> + { .name = "bd9574mwf-gpio", },
> +};
> +
> +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> + regmap_reg_range(BD9571MWV_VENDOR_CODE,
> BD9571MWV_PRODUCT_REVISION),
> + regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> BD9571MWV_BKUP_MODE_CNT),
> + regmap_reg_range(BD9571MWV_DVFS_VINIT, BD9571MWV_DVFS_SETVMAX),
> + regmap_reg_range(BD9571MWV_DVFS_SETVID,
> BD9571MWV_DVFS_MONIVDAC),
> + regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN),
> + regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INTMASK),
> + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTMASK),
> +};
> +
> +static const struct regmap_access_table bd9574mwf_readable_table = {
> + .yes_ranges = bd9574mwf_readable_yes_ranges,
> + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_readable_yes_ranges),
> +};
> +
> +static const struct regmap_range bd9574mwf_writable_yes_ranges[] = {
> + regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> BD9571MWV_BKUP_MODE_CNT),
> + regmap_reg_range(BD9571MWV_DVFS_SETVID, BD9571MWV_DVFS_SETVID),
> + regmap_reg_range(BD9571MWV_GPIO_DIR, BD9571MWV_GPIO_OUT),
> + regmap_reg_range(BD9571MWV_GPIO_INT_SET,
> BD9571MWV_GPIO_INTMASK),
> + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTMASK),
> +};
> +
> +static const struct regmap_access_table bd9574mwf_writable_table = {
> + .yes_ranges = bd9574mwf_writable_yes_ranges,
> + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_writable_yes_ranges),
> +};
> +
> +static const struct regmap_range bd9574mwf_volatile_yes_ranges[] = {
> + regmap_reg_range(BD9571MWV_DVFS_MONIVDAC,
> BD9571MWV_DVFS_MONIVDAC),
> + regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN),
> + regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INT),
> + regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_INT_INTREQ),
> +};
> +
> +static const struct regmap_access_table bd9574mwf_volatile_table = {
> + .yes_ranges = bd9574mwf_volatile_yes_ranges,
> + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_volatile_yes_ranges),
> +};
> +
> +static const struct regmap_config bd9574mwf_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> + .rd_table = _readable_table,
> + .wr_table = _writable_table,
> + .volatile_table = _volatile_table,
> + .max_register = 0xff,
> +};
> +
> +static struct regmap_irq_chip bd9574mwf_irq_chip = {
> + .name = "bd9574mwf",
> + .status_base= BD9571MWV_INT_INTREQ,
> + .mask_base = BD9571MWV_INT_INTMASK,
> + .ack_base = BD9571MWV_INT_INTREQ,
> + .init_ack_masked = true,
> + .num_regs = 1,
> + .irqs = bd9571mwv_irqs,
> + .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> +};
> +
> +static const struct bd957x_data bd9574mwf_data = {
> + .part_name = BD9574MWF_PART_NAME,
> + .regmap_config = _regmap_config,
> + .irq_chip = _irq_chip,
> + .cells = bd9574mwf_cells,
> + .num_cells = ARRAY_SIZE(bd9574mwf_cells),
> +};
> +
> static int bd9571mwv_identify(struct device *dev, struct regmap
> *regmap,
> const char *part_name)
> {
> @@ -181,6 +258,9 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
> case BD9571MWV_PRODUCT_CODE_VAL:
> data = _data;
> break;
> + case BD9574MWF_PRODUCT_CODE_VAL:
> + data = _data;
>
Re: [PATCH v3 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote:
> From: Khiem Nguyen
>
> Since the driver supports BD9571MWV PMIC only,
> this patch makes the functions and data structure become more generic
> so that it can support other PMIC variants as well.
>
> Signed-off-by: Khiem Nguyen
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Matti Vaittinen
> ---
> drivers/mfd/bd9571mwv.c | 95 +++
>
> include/linux/mfd/bd9571mwv.h | 18 ++--
> 2 files changed, 63 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 49e968e..ccf1a60 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -3,6 +3,7 @@
> * ROHM BD9571MWV-M MFD driver
> *
> * Copyright (C) 2017 Marek Vasut
> + * Copyright (C) 2020 Renesas Electronics Corporation
> *
> * Based on the TPS65086 driver
> */
> @@ -14,6 +15,19 @@
>
> #include
>
> +/**
> + * struct bd957x_data - internal data for the bd957x driver
> + *
> + * Internal data to distinguish bd957x variants
> + */
> +struct bd957x_data {
> + char *part_name;
> + const struct regmap_config *regmap_config;
> + const struct regmap_irq_chip *irq_chip;
> + const struct mfd_cell *cells;
> + int num_cells;
> +};
> +
I do like the way you placed the variant data in owns structs. Well
thought.
> static const struct mfd_cell bd9571mwv_cells[] = {
> { .name = "bd9571mwv-regulator", },
> { .name = "bd9571mwv-gpio", },
> @@ -102,13 +116,21 @@ static struct regmap_irq_chip
> bd9571mwv_irq_chip = {
> .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> };
>
> -static int bd9571mwv_identify(struct bd9571mwv *bd)
> +static const struct bd957x_data bd9571mwv_data = {
> + .part_name = BD9571MWV_PART_NAME,
> + .regmap_config = _regmap_config,
> + .irq_chip = _irq_chip,
> + .cells = bd9571mwv_cells,
> + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> +};
> +
> +static int bd9571mwv_identify(struct device *dev, struct regmap
> *regmap,
> + const char *part_name)
> {
> - struct device *dev = bd->dev;
> unsigned int value;
> int ret;
>
> - ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, );
> + ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, );
> if (ret) {
> dev_err(dev, "Failed to read vendor code register
> (ret=%i)\n",
> ret);
> @@ -121,27 +143,20 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
> return -EINVAL;
> }
>
> - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, );
> + ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, );
> if (ret) {
> dev_err(dev, "Failed to read product code register
> (ret=%i)\n",
> ret);
> return ret;
> }
> -
> - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> - dev_err(dev, "Invalid product code ID %02x (expected
> %02x)\n",
> - value, BD9571MWV_PRODUCT_CODE_VAL);
> - return -EINVAL;
> - }
> -
> - ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION,
> );
> + ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, );
> if (ret) {
> dev_err(dev, "Failed to read revision register
> (ret=%i)\n",
> ret);
> return ret;
> }
>
> - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> + dev_info(dev, "Device: %s rev. %d\n", part_name, value & 0xff);
>
> return 0;
> }
> @@ -149,38 +164,48 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
> static int bd9571mwv_probe(struct i2c_client *client,
> const struct i2c_device_id *ids)
> {
> - struct bd9571mwv *bd;
> - int ret;
> -
> - bd = devm_kzalloc(>dev, sizeof(*bd), GFP_KERNEL);
> - if (!bd)
> - return -ENOMEM;
> -
> - i2c_set_clientdata(client, bd);
> - bd->dev = >dev;
> - bd->irq = client->irq;
> + const struct bd957x_data *data;
> + struct device *dev = >dev;
> + struct regmap *regmap;
> + struct regmap_irq_chip_data *irq_data;
> + int ret, irq = client->irq;
> +
> + /* Read the PMIC product code */
> + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
Having to use the i2c_smbus_read_byte_data for a device which is going
to be used with regmap slightly bugs me. But as you want to do the
runtime probing, then this access must be done prior regmap
registration - so I can't think of a better way. :(
> + if (ret < 0) {
> + dev_err(dev, "failed reading at 0x%02x\n",
> + BD9571MWV_PRODUCT_CODE);
> + return ret;
> + }
> + switch (ret) {
> + case BD9571MWV_PRODUCT_CODE_VAL:
> + data = _data;
> + break;
> + default:
> + dev_err(dev, "Unsupported
Re: [PATCH v3 10/12] mfd: bd9571mwv: Use devm_regmap_add_irq_chip()
On Wed, 2020-12-16 at 16:37 +0900, Yoshihiro Shimoda wrote:
> Use dev_regmap_add_irq_chip() to simplify the code.
>
> Signed-off-by: Yoshihiro Shimoda
Reviewed-by: Matti Vaittinen
> ---
> drivers/mfd/bd9571mwv.c | 27 ++-
> 1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index e68c3fa..49e968e 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -170,31 +170,17 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
> if (ret)
> return ret;
>
> - ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0,
> - _irq_chip, >irq_data);
> + ret = devm_regmap_add_irq_chip(bd->dev, bd->regmap, bd->irq,
> +IRQF_ONESHOT, 0,
> _irq_chip,
> +>irq_data);
> if (ret) {
> dev_err(bd->dev, "Failed to register IRQ chip\n");
> return ret;
> }
>
> - ret = devm_mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO,
> -bd9571mwv_cells,
> ARRAY_SIZE(bd9571mwv_cells),
> -NULL, 0, regmap_irq_get_domain(bd-
> >irq_data));
The funny diff formatting got me fooled. I spent a while wondering why
you do remove the devm_mfd_add_devices - untill I noticed that it was
just the diff playing tricks - it is added back in the remove. I should
practice my diff reading skills :) But the result looks good and clean
to me!
> - if (ret) {
> - regmap_del_irq_chip(bd->irq, bd->irq_data);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static int bd9571mwv_remove(struct i2c_client *client)
> -{
> - struct bd9571mwv *bd = i2c_get_clientdata(client);
> -
> - regmap_del_irq_chip(bd->irq, bd->irq_data);
> -
> - return 0;
> + return devm_mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO,
> + bd9571mwv_cells,
> ARRAY_SIZE(bd9571mwv_cells),
> + NULL, 0, regmap_irq_get_domain(bd-
> >irq_data));
> }
>
> static const struct of_device_id bd9571mwv_of_match_table[] = {
> @@ -215,7 +201,6 @@ static struct i2c_driver bd9571mwv_driver = {
> .of_match_table = bd9571mwv_of_match_table,
> },
> .probe = bd9571mwv_probe,
> - .remove = bd9571mwv_remove,
> .id_table = bd9571mwv_id_table,
> };
> module_i2c_driver(bd9571mwv_driver);
Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv
On Wed, 2020-12-16 at 06:29 +, Yoshihiro Shimoda wrote:
> Hi Matti-san,
>
> > From: Vaittinen, Matti, Sent: Wednesday, December 16, 2020 3:00 PM
> > On Wed, 2020-12-16 at 02:13 +, Yoshihiro Shimoda wrote:
> > > Hi Geert-san, Matti-san,
> > >
> > > > From: Geert Uytterhoeven, Sent: Wednesday, December 16, 2020
> > > > 1:13
> > > > AM
> > > > On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <
> > > > ge...@linux-m68k.org> wrote:
> > > > > On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> > > > > wrote:
> > > > > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > >
> > > > > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > > > > @@ -17,7 +17,7 @@
> > > > > > > #include
> > > > > > >
> > > > > > > struct bd9571mwv_reg {
> > > > > > > - struct bd9571mwv *bd;
> > > > > > > + struct regmap *regmap;
> > > > > >
> > > > > > As a 'nit':
> > > > > > I might consider adding the dev pointer here to avoid extra
> > > > > > argument
> > > > > > with all the bkup_mode functions below. (just pass this
> > > > > > struct
> > > > > > and
> > > > > > mode). But that's only my preference - feel free to ignore
> > > > > > this
> > > > > > comment
> > > > > > if patch is Ok to Mark, Marek & Others :)
> > > > >
> > > > > Struct regmap already contains a struct device pointer, but
> > > > > that's internal
> > > > > to regmap.
> > > > >
> > > > > Perhaps adding a regmap_device() helper to retrieve the
> > > > > device
> > > > > pointer
> > > > > might be worthwhile?
> > > >
> > > > -EEXISTS ;-)
> > > >
> > > > struct device *regmap_get_device(struct regmap *map)
> > >
> > > Thank you for finding this. I'll fix this patch.
> >
> > Just a small reminder that this device is probably the MFD device,
> > not
> > the device created for regulator driver. (Regmap is created for
> > MFD).
> > For prints this only means we're issuing prints as if MFD device
> > generated them, right? I'm not sure it is the best approach - but
> > I'll
> > leave this to Mark & others to judge :)
>
> Thank you for the comment. You're correct. regmap_get_device() is
> the MFD device. Also, original code had used the MFD device as
> "dev_err(bd->dev, ...)". So, printk behavior is the same as before :)
Right. I must admit didn't catch that. I actually think using the
>dev for prints issued by the regulator driver would be more
correct but I'm not complaining if using MFD device is Ok to Mark &
others :) I do appreciate your work with this, thanks!
>
> Best regards,
> Yoshihiro Shimoda
>
Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv
On Wed, 2020-12-16 at 02:13 +, Yoshihiro Shimoda wrote:
> Hi Geert-san, Matti-san,
>
> > From: Geert Uytterhoeven, Sent: Wednesday, December 16, 2020 1:13
> > AM
> > On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <
> > ge...@linux-m68k.org> wrote:
> > > On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> > > wrote:
> > > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
>
> > > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > > @@ -17,7 +17,7 @@
> > > > > #include
> > > > >
> > > > > struct bd9571mwv_reg {
> > > > > - struct bd9571mwv *bd;
> > > > > + struct regmap *regmap;
> > > >
> > > > As a 'nit':
> > > > I might consider adding the dev pointer here to avoid extra
> > > > argument
> > > > with all the bkup_mode functions below. (just pass this struct
> > > > and
> > > > mode). But that's only my preference - feel free to ignore this
> > > > comment
> > > > if patch is Ok to Mark, Marek & Others :)
> > >
> > > Struct regmap already contains a struct device pointer, but
> > > that's internal
> > > to regmap.
> > >
> > > Perhaps adding a regmap_device() helper to retrieve the device
> > > pointer
> > > might be worthwhile?
> >
> > -EEXISTS ;-)
> >
> > struct device *regmap_get_device(struct regmap *map)
>
> Thank you for finding this. I'll fix this patch.
Just a small reminder that this device is probably the MFD device, not
the device created for regulator driver. (Regmap is created for MFD).
For prints this only means we're issuing prints as if MFD device
generated them, right? I'm not sure it is the best approach - but I'll
leave this to Mark & others to judge :)
>
> Best regards,
> Yoshihiro Shimoda
>
Re: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support
Hello Shimoda-san,
On Mon, 2020-12-14 at 04:57 +, Yoshihiro Shimoda wrote:
> Hello Matti-san,
>
> > From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:34 PM
> >
> > Hello again Shimada-san,
> >
> > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > > Note that BD9574MWF doesn't support AVS and VID.
> >
> > I'd like to understand what is VID?
>
> It seems reading some voltages from registers.
> For example, BD9571 has "VD18_VID" register which
> is prohibit to write. But, BD9574 doesn't have this
> register. Also, the driver names "vid_ops",
> so I described "VID" here. Perhaps, we should revise
> the description to clear. (Please look "Updated description" in this
> email.)
Thank you for detailed explanation. So as far as I understood - VID is
a register which displays the current output voltage, right? If I am
not mistaken, this is different from register where (initial) voltage
is set?
>
> > > Signed-off-by: Yoshihiro Shimoda <
> > > yoshihiro.shimoda...@renesas.com>
> > > ---
> > > drivers/regulator/bd9571mwv-regulator.c | 10 --
> > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > > b/drivers/regulator/bd9571mwv-regulator.c
> > > index 02120b0..041339b 100644
> > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > @@ -1,6 +1,6 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > > /*
> > > - * ROHM BD9571MWV-M regulator driver
> > > + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> > > *
> > > * Copyright (C) 2017 Marek Vasut > > >
> > > *
> > > @@ -9,6 +9,7 @@
> > > * NOTE: VD09 is missing
> > > */
> > >
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> > > platform_device *pdev)
> > > struct regulator_dev *rdev;
> > > unsigned int val;
> > > int i;
> > > + enum rohm_chip_type chip = platform_get_device_id(pdev)-
> > > > driver_data;
> > >
> > > bdreg = devm_kzalloc(>dev, sizeof(*bdreg), GFP_KERNEL);
> > > if (!bdreg)
> > > @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> > > platform_device *pdev)
> > > config.regmap = bdreg->regmap;
> > >
> > > for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> > > + /* BD9574MWF supports DVFS only */
> > > + if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> > > != DVFS)
> > > + continue;
> >
> > Does this mean that reading VD09 voltage is not supported by
> > driver?
>
> Yes. Also, reading VD{18,25,33} voltage are not supported.
I think that would be excellent comment here. Maybe something like: "We
don't support voltage rails VD{09,18,25,33} by this driver on BD9574."
>
> > (I
> > assumed VD09 initial voltage can be set from eeprom(?) and read by
> > driver - I may be wrong though). Perhaps it is worth mentioning in
> > the
> > commit message as a driver restriction?
>
> Yes, these voltage can be set from eeprom and read by driver.
> So, I updated the description like below.
>
> -- Updated description --
> Add support for BD9574MWF which is similar chip with BD9571MWV.
> Note that since BD9574MWF doesn't have avs_ops and vid_ops
> related registers, this driver avoids to use these registers
> if BD9574MWF is used.
>
Thank you :) What I was after is that I would like to leave a note
about 'what could be improved' or about what is the 'software
limitation' here so that if anyone later needs the other voltage rails
he would have a hint about what could be done.
Do you think mentioning that "the VD09 voltage could be read from PMIC
but that is not supported by this commit" in commit message would be
Ok?
>
> > And just asking out of the curiosity - are the other voltage rails
> > listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4)
> > set-up
> > from DT as fixed-regulators? Any reason why they are not set-up
> > here?
>
> TBH, I don't know why. Perhaps, the driver cannot read DDR0, LDO[1-4]
> values?
I also think that all voltages can't be read. I was just thinking that
it might make sense to alw
Re: [PATCH v2 09/10] mfd: bd9571mwv: Make the driver more generic
Hi Yoshihiro-san,
On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> From: Khiem Nguyen
>
> Since the driver supports BD9571MWV PMIC only,
> this patch makes the functions and data structure become more generic
> so that it can support other PMIC variants as well.
>
> Signed-off-by: Khiem Nguyen
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/mfd/bd9571mwv.c | 71
> +++
> include/linux/mfd/bd9571mwv.h | 18 ++-
> 2 files changed, 62 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 80c6ef0..adb9e3d 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -3,6 +3,7 @@
> * ROHM BD9571MWV-M MFD driver
> *
> * Copyright (C) 2017 Marek Vasut
> + * Copyright (C) 2020 Renesas Electronics Corporation
> *
> * Based on the TPS65086 driver
> */
> @@ -14,6 +15,34 @@
>
> #include
>
> +/**
> + * struct bd957x_data - internal data for the bd957x driver
> + *
> + * Internal data to distinguish bd957x variants
> + */
> +struct bd957x_data {
> + char *part_name;
> + const struct regmap_config *regmap_config;
> + const struct regmap_irq_chip *irq_chip;
> + const struct mfd_cell *cells;
> + int num_cells;
> +};
> +
> +/**
> + * struct bd9571mwv - state holder for the bd9571mwv driver
> + *
> + * Device data may be used to access the BD9571MWV chip
> + */
> +struct bd9571mwv {
> + struct device *dev;
> + struct regmap *regmap;
> + const struct bd957x_data *data;
> +
> + /* IRQ Data */
> + int irq;
> + struct regmap_irq_chip_data *irq_data;
> +};
> +
I still don't see why you actually need this structure?
> static const struct mfd_cell bd9571mwv_cells[] = {
> { .name = "bd9571mwv-regulator", },
> { .name = "bd9571mwv-gpio", },
> @@ -102,6 +131,14 @@ static struct regmap_irq_chip bd9571mwv_irq_chip
> = {
> .num_irqs = ARRAY_SIZE(bd9571mwv_irqs),
> };
>
> +static const struct bd957x_data bd9571mwv_data = {
> + .part_name = BD9571MWV_PART_NAME,
> + .regmap_config = _regmap_config,
> + .irq_chip = _irq_chip,
> + .cells = bd9571mwv_cells,
> + .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> +};
> +
> static int bd9571mwv_identify(struct bd9571mwv *bd)
> {
> struct device *dev = bd->dev;
> @@ -127,13 +164,6 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
> ret);
> return ret;
> }
> -
> - if (value != BD9571MWV_PRODUCT_CODE_VAL) {
> - dev_err(dev, "Invalid product code ID %02x (expected
> %02x)\n",
> - value, BD9571MWV_PRODUCT_CODE_VAL);
> - return -EINVAL;
> - }
> -
> ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION,
> );
> if (ret) {
> dev_err(dev, "Failed to read revision register
> (ret=%i)\n",
> @@ -141,7 +171,8 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
> return ret;
> }
>
> - dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> + dev_info(dev, "Device: %s rev. %d\n", bd->data->part_name,
> + value & 0xff);
>
> return 0;
> }
> @@ -160,7 +191,23 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
> bd->dev = >dev;
> bd->irq = client->irq;
>
> - bd->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> + /* Read the PMIC product code */
> + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
> + if (ret < 0) {
> + dev_err(>dev, "failed reading at 0x%02x\n",
> + BD9571MWV_PRODUCT_CODE);
> + return ret;
> + }
> + switch (ret) {
> + case BD9571MWV_PRODUCT_CODE_VAL:
> + bd->data = _data;
> + break;
> + default:
> + dev_err(bd->dev, "Unsupported device 0x%x\n", ret);
> + return -ENOENT;
> + }
> +
> + bd->regmap = devm_regmap_init_i2c(client, bd->data-
> >regmap_config);
> if (IS_ERR(bd->regmap)) {
> dev_err(bd->dev, "Failed to initialize register
> map\n");
> return PTR_ERR(bd->regmap);
> @@ -171,14 +218,14 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
> return ret;
>
> ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0,
> - _irq_chip, >irq_data);
> + bd->data->irq_chip, >irq_data);
I think you already did the big task when you cleaned up the sub-
drivers from using the struct bd9571mwv. Thumbs up for that!
So, as I said in comment to previous version - I don't see this struct
bd9571mwv being really used anywhere else but as an argument to IC
identification function and argument for the remove. I think that by
switching regmap_add_irq_chip to devm_regmap_add_irq_chip you could get
rid of the remove, error cleanup path and
Re: [PATCH v2 07/10] gpio: bd9571mwv: Add BD9574MWF support
On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> Add support for BD9574MWF which is silimar chip with BD9571MWV.
> Note that BD9574MWF has an additional feature, but doesn't
> support it for now.
nit:
Perhaps mention which feature? And I think you mean the driver does not
support it yet?
>
> Signed-off-by: Yoshihiro Shimoda
FWIW:
Reviewed-By: Matti Vaittinen
> ---
> drivers/gpio/gpio-bd9571mwv.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-
> bd9571mwv.c
> index 0e5395f..df6102b 100644
> --- a/drivers/gpio/gpio-bd9571mwv.c
> +++ b/drivers/gpio/gpio-bd9571mwv.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * ROHM BD9571MWV-M GPIO driver
> + * ROHM BD9571MWV-M and BD9574MWF-M GPIO driver
> *
> * Copyright (C) 2017 Marek Vasut
> *
> @@ -10,6 +10,7 @@
> */
>
> #include
> +#include
> #include
> #include
>
> @@ -118,7 +119,8 @@ static int bd9571mwv_gpio_probe(struct
> platform_device *pdev)
> }
>
> static const struct platform_device_id bd9571mwv_gpio_id_table[] = {
> - { "bd9571mwv-gpio", },
> + { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> + { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
I guess these CHIP_TYPES are used by subsequent patches?
I guess this means the existing functionality in both chips is same,
right? (GPIO register addresses etc? - I don't have BD9571 data-sheet
so I can't check)
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(platform, bd9571mwv_gpio_id_table);
Re: [PATCH v2 06/10] gpio: bd9571mwv: rid of using struct bd9571mwv
On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> To simplify this driver, use dev_get_regmap() and
> rid of using struct bd9571mwv.
>
> Signed-off-by: Yoshihiro Shimoda
FWIW:
Reviewed-By: Matti Vaittinen
> ---
> drivers/gpio/gpio-bd9571mwv.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-
> bd9571mwv.c
> index abb622c..0e5395f 100644
> --- a/drivers/gpio/gpio-bd9571mwv.c
> +++ b/drivers/gpio/gpio-bd9571mwv.c
> @@ -16,8 +16,8 @@
> #include
>
> struct bd9571mwv_gpio {
> + struct regmap *regmap;
> struct gpio_chip chip;
> - struct bd9571mwv *bd;
> };
>
> static int bd9571mwv_gpio_get_direction(struct gpio_chip *chip,
> @@ -26,7 +26,7 @@ static int bd9571mwv_gpio_get_direction(struct
> gpio_chip *chip,
> struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
> int ret, val;
>
> - ret = regmap_read(gpio->bd->regmap, BD9571MWV_GPIO_DIR, );
> + ret = regmap_read(gpio->regmap, BD9571MWV_GPIO_DIR, );
> if (ret < 0)
> return ret;
> if (val & BIT(offset))
> @@ -40,8 +40,7 @@ static int bd9571mwv_gpio_direction_input(struct
> gpio_chip *chip,
> {
> struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>
> - regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_DIR,
> -BIT(offset), 0);
> + regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_DIR,
> BIT(offset), 0);
>
> return 0;
> }
> @@ -52,9 +51,9 @@ static int bd9571mwv_gpio_direction_output(struct
> gpio_chip *chip,
> struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>
> /* Set the initial value */
> - regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_OUT,
> + regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_OUT,
> BIT(offset), value ? BIT(offset) : 0);
> - regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_DIR,
> + regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_DIR,
> BIT(offset), BIT(offset));
>
> return 0;
> @@ -65,7 +64,7 @@ static int bd9571mwv_gpio_get(struct gpio_chip
> *chip, unsigned int offset)
> struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
> int ret, val;
>
> - ret = regmap_read(gpio->bd->regmap, BD9571MWV_GPIO_IN, );
> + ret = regmap_read(gpio->regmap, BD9571MWV_GPIO_IN, );
> if (ret < 0)
> return ret;
>
> @@ -77,7 +76,7 @@ static void bd9571mwv_gpio_set(struct gpio_chip
> *chip, unsigned int offset,
> {
> struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>
> - regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_OUT,
> + regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_OUT,
> BIT(offset), value ? BIT(offset) : 0);
> }
>
> @@ -105,9 +104,9 @@ static int bd9571mwv_gpio_probe(struct
> platform_device *pdev)
>
> platform_set_drvdata(pdev, gpio);
>
> - gpio->bd = dev_get_drvdata(pdev->dev.parent);
> + gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> gpio->chip = template_chip;
> - gpio->chip.parent = gpio->bd->dev;
> + gpio->chip.parent = pdev->dev.parent;
>
> ret = devm_gpiochip_add_data(>dev, >chip, gpio);
> if (ret < 0) {
Re: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support
Hello again Shimada-san,
On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> Add support for BD9574MWF which is silimar chip with BD9571MWV.
> Note that BD9574MWF doesn't support AVS and VID.
I'd like to understand what is VID?
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/regulator/bd9571mwv-regulator.c | 10 --
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/bd9571mwv-regulator.c
> b/drivers/regulator/bd9571mwv-regulator.c
> index 02120b0..041339b 100644
> --- a/drivers/regulator/bd9571mwv-regulator.c
> +++ b/drivers/regulator/bd9571mwv-regulator.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * ROHM BD9571MWV-M regulator driver
> + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> *
> * Copyright (C) 2017 Marek Vasut
> *
> @@ -9,6 +9,7 @@
> * NOTE: VD09 is missing
> */
>
> +#include
> #include
> #include
> #include
> @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
> struct regulator_dev *rdev;
> unsigned int val;
> int i;
> + enum rohm_chip_type chip = platform_get_device_id(pdev)-
> >driver_data;
>
> bdreg = devm_kzalloc(>dev, sizeof(*bdreg), GFP_KERNEL);
> if (!bdreg)
> @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
> config.regmap = bdreg->regmap;
>
> for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> + /* BD9574MWF supports DVFS only */
> + if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> != DVFS)
> + continue;
Does this mean that reading VD09 voltage is not supported by driver? (I
assumed VD09 initial voltage can be set from eeprom(?) and read by
driver - I may be wrong though). Perhaps it is worth mentioning in the
commit message as a driver restriction?
And just asking out of the curiosity - are the other voltage rails
listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4) set-up
from DT as fixed-regulators? Any reason why they are not set-up here?
> rdev = devm_regulator_register(>dev,
> [i],
> );
> if (IS_ERR(rdev)) {
> @@ -339,7 +344,8 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
> }
>
> static const struct platform_device_id
> bd9571mwv_regulator_id_table[] = {
> - { "bd9571mwv-regulator", },
> + { "bd9571mwv-regulator", ROHM_CHIP_TYPE_BD9571 },
> + { "bd9574mwf-regulator", ROHM_CHIP_TYPE_BD9574 },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(platform, bd9571mwv_regulator_id_table);
Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv
On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> To simplify this driver, use dev_get_regmap() and
> rid of using struct bd9571mwv.
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/regulator/bd9571mwv-regulator.c | 49 +
>
> 1 file changed, 26 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/regulator/bd9571mwv-regulator.c
> b/drivers/regulator/bd9571mwv-regulator.c
> index e690c2c..02120b0 100644
> --- a/drivers/regulator/bd9571mwv-regulator.c
> +++ b/drivers/regulator/bd9571mwv-regulator.c
> @@ -17,7 +17,7 @@
> #include
>
> struct bd9571mwv_reg {
> - struct bd9571mwv *bd;
> + struct regmap *regmap;
As a 'nit':
I might consider adding the dev pointer here to avoid extra argument
with all the bkup_mode functions below. (just pass this struct and
mode). But that's only my preference - feel free to ignore this comment
if patch is Ok to Mark, Marek & Others :)
Overall, looks good to me :)
Reviewed-By: Matti Vaittinen
>
> /* DDR Backup Power */
> u8 bkup_mode_cnt_keepon;/* from "rohm,ddr-backup-power" */
> @@ -137,26 +137,30 @@ static const struct regulator_desc regulators[]
> = {
> };
>
> #ifdef CONFIG_PM_SLEEP
> -static int bd9571mwv_bkup_mode_read(struct bd9571mwv *bd, unsigned
> int *mode)
> +static int bd9571mwv_bkup_mode_read(struct device * dev,
> + struct bd9571mwv_reg *bdreg,
> + unsigned int *mode)
> {
> int ret;
>
> - ret = regmap_read(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
> + ret = regmap_read(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT,
> mode);
> if (ret) {
> - dev_err(bd->dev, "failed to read backup mode (%d)\n",
> ret);
> + dev_err(dev, "failed to read backup mode (%d)\n", ret);
> return ret;
> }
>
> return 0;
> }
>
> -static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned
> int mode)
> +static int bd9571mwv_bkup_mode_write(struct device *dev,
> + struct bd9571mwv_reg *bdreg,
> + unsigned int mode)
> {
> int ret;
>
> - ret = regmap_write(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
> + ret = regmap_write(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT,
> mode);
> if (ret) {
> - dev_err(bd->dev, "failed to configure backup mode 0x%x
> (%d)\n",
> + dev_err(dev, "failed to configure backup mode 0x%x
> (%d)\n",
> mode, ret);
> return ret;
> }
> @@ -194,7 +198,7 @@ static ssize_t backup_mode_store(struct device
> *dev,
>* Configure DDR Backup Mode, to change the role of the
> accessory power
>* switch from a power switch to a wake-up switch, or vice
> versa
>*/
> - ret = bd9571mwv_bkup_mode_read(bdreg->bd, );
> + ret = bd9571mwv_bkup_mode_read(dev, bdreg, );
> if (ret)
> return ret;
>
> @@ -202,7 +206,7 @@ static ssize_t backup_mode_store(struct device
> *dev,
> if (bdreg->bkup_mode_enabled)
> mode |= bdreg->bkup_mode_cnt_keepon;
>
> - ret = bd9571mwv_bkup_mode_write(bdreg->bd, mode);
> + ret = bd9571mwv_bkup_mode_write(dev, bdreg, mode);
> if (ret)
> return ret;
>
> @@ -221,7 +225,7 @@ static int bd9571mwv_suspend(struct device *dev)
> return 0;
>
> /* Save DDR Backup Mode */
> - ret = bd9571mwv_bkup_mode_read(bdreg->bd, );
> + ret = bd9571mwv_bkup_mode_read(dev, bdreg, );
> if (ret)
> return ret;
>
> @@ -235,7 +239,7 @@ static int bd9571mwv_suspend(struct device *dev)
> mode |= bdreg->bkup_mode_cnt_keepon;
>
> if (mode != bdreg->bkup_mode_cnt_saved)
> - return bd9571mwv_bkup_mode_write(bdreg->bd, mode);
> + return bd9571mwv_bkup_mode_write(dev, bdreg, mode);
>
> return 0;
> }
> @@ -248,7 +252,7 @@ static int bd9571mwv_resume(struct device *dev)
> return 0;
>
> /* Restore DDR Backup Mode */
> - return bd9571mwv_bkup_mode_write(bdreg->bd, bdreg-
> >bkup_mode_cnt_saved);
> + return bd9571mwv_bkup_mode_write(dev, bdreg, bdreg-
> >bkup_mode_cnt_saved);
> }
>
> static const struct dev_pm_ops bd9571mwv_pm = {
> @@ -268,7 +272,6 @@ static int bd9571mwv_regulator_remove(struct
> platform_device *pdev)
>
> static int bd9571mwv_regulator_probe(struct platform_device *pdev)
> {
> - struct bd9571mwv *bd = dev_get_drvdata(pdev->dev.parent);
> struct regulator_config config = { };
> struct bd9571mwv_reg *bdreg;
> struct regulator_dev *rdev;
> @@ -279,40 +282,40 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
> if (!bdreg)
> return -ENOMEM;
>
> - bdreg->bd = bd;
> + bdreg->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>
> platform_set_drvdata(pdev, bdreg);
>
> config.dev = >dev;
>
Re: [PATCH v2 02/10] mfd: rohm-generic: Add BD9571 and BD9574
On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> Add chip IDs for BD9571MWV and BD9574MWF.
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> include/linux/mfd/rohm-generic.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/mfd/rohm-generic.h
> b/include/linux/mfd/rohm-generic.h
> index 4283b5b..affacf8 100644
> --- a/include/linux/mfd/rohm-generic.h
> +++ b/include/linux/mfd/rohm-generic.h
> @@ -12,6 +12,8 @@ enum rohm_chip_type {
> ROHM_CHIP_TYPE_BD71847,
> ROHM_CHIP_TYPE_BD70528,
> ROHM_CHIP_TYPE_BD71828,
> + ROHM_CHIP_TYPE_BD9571,
> + ROHM_CHIP_TYPE_BD9574,
> ROHM_CHIP_TYPE_AMOUNT
> };
>
Just a note to Lee & Others:
This will probably conflict with the BD9573/BD9576 patch series (which
introduces those chip IDs here). Conflict should be trivial though.
With that note:
Reviewed-By: Matti Vaittinen
--Matti
Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF
Hi Yoshihiro san,
On Thu, 2020-12-10 at 10:58 +, Yoshihiro Shimoda wrote:
> Hi Matti,
>
> > From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 5:28 PM
> >
> > On Tue, 2020-12-08 at 17:04 +0900, Yoshihiro Shimoda wrote:
> > > From: Khiem Nguyen
> > >
> > > The new PMIC BD9574MWF inherits features from BD9571MWV.
> > > Add the support of new PMIC to existing bd9571mwv driver.
> > >
> > > Signed-off-by: Khiem Nguyen
> > > [shimoda: rebase and refactor]
> > > Signed-off-by: Yoshihiro Shimoda <
> > > yoshihiro.shimoda...@renesas.com>
> > > ---
> > > drivers/mfd/bd9571mwv.c | 92
> > > +++
> > > include/linux/mfd/bd9571mwv.h | 80
> > > +
> > > 2 files changed, 172 insertions(+)
> > >
> > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > > index 57bdb6a..f8f0a87 100644
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> > > @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[]
> > > = {
> > > { .name = "bd9571mwv-gpio", },
> > > };
> > >
> > > +/* Regmap for BD9571MWV */
> > > static const struct regmap_range bd9571mwv_readable_yes_ranges[]
> > > = {
> > > regmap_reg_range(BD9571MWV_VENDOR_CODE,
> > > BD9571MWV_PRODUCT_REVISION),
> > > regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> > > BD9571MWV_BKUP_MODE_CNT),
> > > @@ -112,6 +113,95 @@ static const struct bd957x_data
> > > bd9571mwv_data =
> > > {
> > > .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > > };
> > >
> > > +static const struct mfd_cell bd9574mwf_cells[] = {
> > > + { .name = "bd9571mwv-gpio", },
> >
> > Another 'nit' suggestion which you can ignore if it does not make
> > sense
> > :)
> >
> > Are the GPIO blocks 100% identical? If not, then I would suggest
> > changing this to:
> > { .name = "bd9574mwf-gpio", },
>
> The GPIO blocks are not 100% identical. BD9574MWF seems to have
> an additional feature which GPIOx pin are used for other mode by
> using gpio mux regisiter.
>
> > and populating the platform_driver id_table for sub driver(s) using
> > something like:
> >
> > static const struct platform_device_id bd957x_gpio_id[] = {
> > { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> > { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
> > { },
> > };
> >
> > Then you can get the IC type using
> > platform_get_device_id(pdev)->driver_data.
>
> I got it. So, perhaps, I should add these types into
> include/linux/mfd/rohm-generic.h.
That would make sense to me. I like the idea of collecting the IDs in
same place - but on the other hand, the define in id-table is not
really visible outside the sub-driver - so you can probably also define
the type in sub-device driver if you wish. I don't have strong opinion
on that.
>
> > Next, I think the parent data from MFD is only used to get the
> > regmap
> > and dev in sub-devices, right? Maybe you could simplify this and
> > get
> > rid of the whole MFD parent data structure? I think you can use
> >
> > pdev->dev.parent to get the parent device and
> > dev_get_regmap(pdev->dev.parent, NULL);
> >
> > to get the regmap?
>
> IIUC, these comments are related to gpio-bd9571mwv.c.
> # Also, bd9571mwv-regulator.c?
> If so, I didn't try this yet, but perhaps, we can modify such things.
Correct. My suggestion was related to how regmap and dev pointers are
obtained in sub-devices. It is related to MFD because I think you could
remove the MFD driver data usage.
>
> > (After this I wonder if you need the
> > struct bd9571mwv at all?)
>
> I'm sorry, but I could not understand this.
I believe the struct bd9571mwv is defined only to collect all the MFD
driver data in one struct so that it can be passed to sub-drivers using
the MFD device private data. But as far as I can tell, the sub-devices
only use regmap and dev pointers from this data. If this is the case,
then I think there is no need to define this struct or populate the MFD
driver data (unless I am missing something).
(And as a further clen-up, one could probably switch from:
regmap_add_irq_chip
to
devm_regmap_add_irq_chip
and get rid of the remove function)
but as I said - these are only 'nit' comments and I am not insisting on
changing these. Especially since some of the comments are more related
to
Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF
On Thu, 2020-12-10 at 09:19 +0100, Geert Uytterhoeven wrote:
> Hi Matti, Shimoda-san,
>
> On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti
> wrote:
> > On Thu, 2020-12-10 at 04:44 +, Yoshihiro Shimoda wrote:
> > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020
> > > > 10:30
> > > > PM
> > >
> > > > > --- a/drivers/mfd/bd9571mwv.c
> > > > > +++ b/drivers/mfd/bd9571mwv.c
> > > > >
> > > > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct
> > > > > i2c_client
> > > > > *client,
> > > > > product_code = (unsigned int)ret;
> > > > > if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > > > > bd->data = _data;
> > > > > + else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > > > + bd->data = _data;
> > > > >
> > > > > if (!bd->data) {
> > > > > dev_err(bd->dev, "No found supported device
> > > > > %d\n",
> > > >
> > > > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > > > I think it would still be a good idea to document a
> > > > "rohm,bd9574mwf"
> > > > compatible value in the DT bindings, and let the driver match
> > > > on
> > > > that.
> > >
> > > In this driver point of view, we can use such the DT bindings,
> > > however, in the board point of view, it's difficult to describe
> > > which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> > > keep this runtime detection.
>
> To clarify: I meant to document and add the compatible value, but
> treat both compatible values the same in the driver, and still do
> runtime
> probing.
>
> > First of all - I don't want to insist changes here so my comment
> > can be
> > ignored. I would definitely like seeing the support for BD9574 in-
> > tree!
> >
> > But as a 'nit':
> > I don't know what are the difficulties you are referring to so it's
> > hard for me to comment this. Without better understanding of board
> > dts
> > files - I think BD9574MWF should really have own compatible as I
> > understood it is different from the BD9571MWV. Relying on product
> > code
> > probing sounds like something that may easily break when/if new
> > variants are produced. ( I've seen new HW variants using the same
> > ID information being produced in previous companies I've worked.
> > Sure
>
> Yes, this happens from time to time, unfortunately.
>
> > ROHM wouldn't do this but still... :] ). And producing boards where
> > DTS
> > does not allow describing the correct components sounds like asking
> > for
> > a nose-bleed to me... If probing of IC type fails AND there is
> > devices
> > with wrong PMIC information burned in DT - then fixing it can be a
> > nightmare. So I would really try make DTS files such that they can
> > be
>
> The issue we're facing is that older Ebisu-4D boards have BD9571,
> while
> newer boards have BD9574. The schematics say "BD9574MWF-M (tentative
> ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible
> (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574).
> Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device
> node
> compatible with "rohm,bd9571mwv". If we have runtime probing, we can
> keep on using that for both variants.
Thank you for the explanation :) This is a nice learning experience for
me!
> > changed to match the actual board. (Perhaps introduce the
> > compatible
> > for BD9574MWF - make this driver to match both of the PMICs - leave
> > the
> > runtime probing here for now - and in parallel work with the DTS
> > files
> > so that eventually the probing can be removed(?) That was my 10
> > cents
> > on this topic :] )
>
> Exactly. Thanks!
I am more than happy with this solution :)
--Matti
Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF
On Tue, 2020-12-08 at 17:04 +0900, Yoshihiro Shimoda wrote:
> From: Khiem Nguyen
>
> The new PMIC BD9574MWF inherits features from BD9571MWV.
> Add the support of new PMIC to existing bd9571mwv driver.
>
> Signed-off-by: Khiem Nguyen
> [shimoda: rebase and refactor]
> Signed-off-by: Yoshihiro Shimoda
> ---
> drivers/mfd/bd9571mwv.c | 92
> +++
> include/linux/mfd/bd9571mwv.h | 80
> +
> 2 files changed, 172 insertions(+)
>
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 57bdb6a..f8f0a87 100644
> --- a/drivers/mfd/bd9571mwv.c
> +++ b/drivers/mfd/bd9571mwv.c
> @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[] = {
> { .name = "bd9571mwv-gpio", },
> };
>
> +/* Regmap for BD9571MWV */
> static const struct regmap_range bd9571mwv_readable_yes_ranges[] = {
> regmap_reg_range(BD9571MWV_VENDOR_CODE,
> BD9571MWV_PRODUCT_REVISION),
> regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> BD9571MWV_BKUP_MODE_CNT),
> @@ -112,6 +113,95 @@ static const struct bd957x_data bd9571mwv_data =
> {
> .num_cells = ARRAY_SIZE(bd9571mwv_cells),
> };
>
> +static const struct mfd_cell bd9574mwf_cells[] = {
> + { .name = "bd9571mwv-gpio", },
Another 'nit' suggestion which you can ignore if it does not make sense
:)
Are the GPIO blocks 100% identical? If not, then I would suggest
changing this to:
{ .name = "bd9574mwf-gpio", },
and populating the platform_driver id_table for sub driver(s) using
something like:
static const struct platform_device_id bd957x_gpio_id[] = {
{ "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
{ "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
{ },
};
Then you can get the IC type using
platform_get_device_id(pdev)->driver_data.
Next, I think the parent data from MFD is only used to get the regmap
and dev in sub-devices, right? Maybe you could simplify this and get
rid of the whole MFD parent data structure? I think you can use
pdev->dev.parent to get the parent device and
dev_get_regmap(pdev->dev.parent, NULL);
to get the regmap?
(After this I wonder if you need the
struct bd9571mwv at all?)
> +};
> +
> +/* Regmap for BD9574MWF */
> +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> + regmap_reg_range(BD9574MWF_VENDOR_CODE,
> BD9574MWF_PRODUCT_REVISION),
> + regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> + regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
> + regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
> + regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTMASK),
> +};
> +
> +static const struct regmap_access_table bd9574mwf_readable_table = {
> + .yes_ranges = bd9574mwf_readable_yes_ranges,
> + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_readable_yes_ranges),
> +};
> +
> +static const struct regmap_range bd9574mwf_writable_yes_ranges[] = {
> + regmap_reg_range(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
> + regmap_reg_range(BD9574MWF_GPIO_INT_SET,
> BD9574MWF_GPIO_INTMASK),
> + regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTMASK),
> +};
> +
> +static const struct regmap_access_table bd9574mwf_writable_table = {
> + .yes_ranges = bd9574mwf_writable_yes_ranges,
> + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_writable_yes_ranges),
> +};
> +
> +static const struct regmap_range bd9574mwf_volatile_yes_ranges[] = {
> + regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> + regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INT),
> + regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTREQ),
> +};
Are you using the other interrupts/statuses or VDCORE MoniVDAC? Should
they be volatile too?
> +
> +static const struct regmap_access_table bd9574mwf_volatile_table = {
> + .yes_ranges = bd9574mwf_volatile_yes_ranges,
> + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_volatile_yes_ranges),
> +};
> +
> +static const struct regmap_config bd9574mwf_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> + .rd_table = _readable_table,
> + .wr_table = _writable_table,
> + .volatile_table = _volatile_table,
> + .max_register = 0xff,
> +};
> +
> +static const struct regmap_irq bd9574mwf_irqs[] = {
> + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD1, 0,
> +BD9574MWF_INT_INTREQ_MD1_INT),
> + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E1, 0,
> +BD9574MWF_INT_INTREQ_MD2_E1_INT),
> + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E2, 0,
> +BD9574MWF_INT_INTREQ_MD2_E2_INT),
> + REGMAP_IRQ_REG(BD9574MWF_IRQ_PROT_ERR, 0,
> +BD9574MWF_INT_INTREQ_PROT_ERR_INT),
> + REGMAP_IRQ_REG(BD9574MWF_IRQ_GP, 0,
> +BD9574MWF_INT_INTREQ_GP_INT),
> + REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_HOLD_OF, 0,
> +BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT),
> +
Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF
On Thu, 2020-12-10 at 04:44 +, Yoshihiro Shimoda wrote:
> Hi Geert-san,
>
> Thank you for your review!
>
> > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30
> > PM
>
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> > >
> > > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client
> > > *client,
> > > product_code = (unsigned int)ret;
> > > if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> > > bd->data = _data;
> > > + else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > > + bd->data = _data;
> > >
> > > if (!bd->data) {
> > > dev_err(bd->dev, "No found supported device
> > > %d\n",
> >
> > While BD9571MWV and BD9574MWF can be distinguished at runtime,
> > I think it would still be a good idea to document a
> > "rohm,bd9574mwf"
> > compatible value in the DT bindings, and let the driver match on
> > that.
>
> In this driver point of view, we can use such the DT bindings,
> however, in the board point of view, it's difficult to describe
> which chip is installed on r8a77990-ebisu.dts. So, I'd like to
> keep this runtime detection.
First of all - I don't want to insist changes here so my comment can be
ignored. I would definitely like seeing the support for BD9574 in-tree!
But as a 'nit':
I don't know what are the difficulties you are referring to so it's
hard for me to comment this. Without better understanding of board dts
files - I think BD9574MWF should really have own compatible as I
understood it is different from the BD9571MWV. Relying on product code
probing sounds like something that may easily break when/if new
variants are produced. ( I've seen new HW variants using the same
ID information being produced in previous companies I've worked. Sure
ROHM wouldn't do this but still... :] ). And producing boards where DTS
does not allow describing the correct components sounds like asking for
a nose-bleed to me... If probing of IC type fails AND there is devices
with wrong PMIC information burned in DT - then fixing it can be a
nightmare. So I would really try make DTS files such that they can be
changed to match the actual board. (Perhaps introduce the compatible
for BD9574MWF - make this driver to match both of the PMICs - leave the
runtime probing here for now - and in parallel work with the DTS files
so that eventually the probing can be removed(?) That was my 10 cents
on this topic :] )
Best Regards
Matti Vaittinen
Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF
Hi deee Ho Yoshihiro-san, Geert, All, On Wed, 2020-12-09 at 14:30 +0100, Geert Uytterhoeven wrote: > Hi Shimoda-san, > > CC Matti (BD9573/6 driver for R-Car platforms) Thank Geert! I wouldn't have noticed this :) > > (I don't have the BD9574 datasheet, so I have to base my review on > https://www.rohm.com/r-car-pmic) > > On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda > wrote: > > From: Khiem Nguyen > > > > The new PMIC BD9574MWF inherits features from BD9571MWV. > > Add the support of new PMIC to existing bd9571mwv driver. > > > > Signed-off-by: Khiem Nguyen > > [shimoda: rebase and refactor] > > Signed-off-by: Yoshihiro Shimoda > > Thanks for your patch! > Indeed! It's really nice to see other people working on upstreaming drivers for ROHM PMICs! Thanks for all the good work! Can you please add me to CC if you ever resend the series? I like to know what kind of support there is added in-tree for ROHM PMICs :) Best Regards Matti -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
Re: [RFC PATCH v2 1/6] power: supply: add cap2ocv batinfo helper
Hi deee Ho Linus,
Thanks (again) for taking a look at this! Highly appreciated :]
On Tue, 2020-12-08 at 09:54 +0100, Linus Walleij wrote:
> On Fri, Dec 4, 2020 at 1:41 PM Matti Vaittinen
> wrote:
>
> > The power-supply core supports concept of OCV (Open Circuit
> > Voltage) =>
> > SOC (State Of Charge) conversion tables. Usually these tables are
> > used
> > to estimate SOC based on OCV. Some systems use so called "Zero
> > Adjust"
> > where at the near end-of-battery condition the SOC from coulomb
> > counter
> > is used to retrieve the OCV - and OCV and VSYS difference is used
> > to
> > re-estimate the battery capacity.
> >
> > Add helper to do look-up the other-way around and also get the OCV
> > based on SOC
> >
> > Signed-off-by: Matti Vaittinen
>
> Overall a good idea!
>
> > +/**
> > + * power_supply_cap2ocv_simple() - find the battery OCV by
> > capacity
> > + * @table: Pointer to battery OCV/CAP lookup table
> > + * @table_len: OCV/CAP table length
> > + * @cap: Current cap value
> > + *
> > + * This helper function is used to look up battery OCV according
> > to
> > + * current capacity value from one OCV table, and the OCV table
> > must be ordered
> > + * descending.
> > + *
> > + * Return: the battery OCV.
> > + */
>
> Spell out the abbreviations in the kerneldoc and not just the commit.
> These will be read much more than the commit message so I would
> move all the excellent info in the commit message into the kerneldoc
> and
> diet the commit message instead.
Hm. I think you have a point here.
>
> > +int power_supply_cap2ocv_simple(struct
> > power_supply_battery_ocv_table *table,
> > + int table_len, int cap)
> > +{
> > + int i, ocv, tmp;
> > +
> > + for (i = 0; i < table_len; i++)
> > + if (cap > table[i].capacity)
> > + break;
> > +
> > + if (i > 0 && i < table_len) {
> > + tmp = (table[i - 1].ocv - table[i].ocv) *
> > + (cap - table[i].capacity);
> > +
> > + tmp /= table[i - 1].capacity - table[i].capacity;
> > + ocv = tmp + table[i].ocv;
>
> This is some linear interpolation right? It's not immediately evident
> so insert
> some comment about what is going on.
Yes. Code interpolates the OCV using two closest known values from
table. This is pretty much identical loop to the existing ocv2cap
calculation - it would have been better to include it in the diff. OTOH
- I did not expect seeing any proper careful reviewing - this RFC was
sent to collect opinion on whether this would be worth further work.
Anyways - If this function is added it should be changed to take more
accurate SOC - perhaps 0.1%(?) - I'm afraid rounding the current
capacity to nearest 1% will kill the accuracy and render this somewhat
useless.
This makes me wonder if the SOC/OCV table in DT should also support
providing values using 0.1% as unit? (I don't think this is a must but
it might be useful).
>
> > /**
> > * power_supply_ocv2cap_simple() - find the battery capacity
> > * @table: Pointer to battery OCV lookup table
> > @@ -847,6 +884,20 @@ power_supply_find_ocv2cap_table(struct
> > power_supply_battery_info *info,
>
> I suspect this kerneldoc could be improved in the process as well.
>
I agree. And also for few others. But that could be a separate patch no
matter if this RFC proceeds or not :)
> Yours,
> Linus Walleij
Re: [PATCH RESEND v6 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
On Wed, 2020-12-02 at 12:57 +, Lee Jones wrote: > On Fri, 27 Nov 2020, Vaittinen, Matti wrote: > > > Hello Lee, > > > > On Fri, 2020-11-27 at 08:32 +, Lee Jones wrote: > > > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which > > > > are > > > > mainly used to power the R-Car series processors. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaitti...@fi.rohmeurope.com> > > > > --- > > > > drivers/mfd/Kconfig | 11 > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/rohm-bd9576.c| 108 > > > > +++ > > > > include/linux/mfd/rohm-bd957x.h | 59 + > > > > include/linux/mfd/rohm-generic.h | 2 + > > > > 5 files changed, 181 insertions(+) > > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > > > > > Looks like a possible candidate for "simple-mfd-i2c". > > > > > > Could you look into that please? > > > > > I must admit I didn't know about "simple-mfd-i2c". Good thing to > > know > > when working with simple devices :) Is this a new thing? > > Yes, it's new. > > > I am unsure I understand the idea fully. Should users put all the > > different regamp configs in this file and just add the device IDs > > with > > pointer to correct config? (BD9576 and BD9573 need volatile > > ranges). > > Also, does this mean each sub-device should have own node and own > > compatible in DT to get correctly load and probed? I guess this > > would > > need a buy-in from Rob too then. > > You should describe the H/W in DT. After re-reading this - do you mean one should describe for example the register ranges in DT? I don't see code which parses the volatile ranges or other regmap configs here. I assume no. I guess you replied to the question whether each sub device would need own node and compatible. Best Regards Matti Vaittinen
Re: [PATCH RESEND v6 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
Hello Lee, On Wed, 2020-12-02 at 12:57 +, Lee Jones wrote: > On Fri, 27 Nov 2020, Vaittinen, Matti wrote: > > > Hello Lee, > > > > On Fri, 2020-11-27 at 08:32 +, Lee Jones wrote: > > > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which > > > > are > > > > mainly used to power the R-Car series processors. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaitti...@fi.rohmeurope.com> > > > > --- > > > > drivers/mfd/Kconfig | 11 > > > > drivers/mfd/Makefile | 1 + > > > > drivers/mfd/rohm-bd9576.c| 108 > > > > +++ > > > > include/linux/mfd/rohm-bd957x.h | 59 + > > > > include/linux/mfd/rohm-generic.h | 2 + > > > > 5 files changed, 181 insertions(+) > > > > create mode 100644 drivers/mfd/rohm-bd9576.c > > > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > > > > > Looks like a possible candidate for "simple-mfd-i2c". > > > > > > Could you look into that please? > > > > > I must admit I didn't know about "simple-mfd-i2c". Good thing to > > know > > when working with simple devices :) Is this a new thing? > > Yes, it's new. > > > I am unsure I understand the idea fully. Should users put all the > > different regamp configs in this file and just add the device IDs > > with > > pointer to correct config? (BD9576 and BD9573 need volatile > > ranges). > > Also, does this mean each sub-device should have own node and own > > compatible in DT to get correctly load and probed? I guess this > > would > > need a buy-in from Rob too then. > > You should describe the H/W in DT. Yes. And it is described. But I've occasionally received request from DT guys to add some properties directly to MFD node and not to add own sub-node. This is what is done for example with the BD71837/47 clocks - there is no own node for clk - the clk properties are placed directly in MFD node (as was requested by Stephen and Rob back then - I originally had own node for clk). I really have no clear view on when things warrant for own subnode and when they don't - but as far as I can see using simple-mfd-i2c forces one to always have a sub-node / device. Even just a empty node with nothing but the compatible even if device does not need stuff from DT? Anyways, I think this is nice addition for simple drivers. > > By the way - for uneducated eyes like mine this does not look like > > it > > has much to do with MFD as a device - here MFD reminds me of a > > simple- > > bus on top of I2C. > > This is for MFD devices where the parent does little more than create > a shared address space for child devices to operate on - like yours. > > > Anyways, the BD9576 and BD9573 both have a few interrupts for > > OVD/UVD > > conditions and I am expecting that I will be asked to provide the > > regulator notifiers for those. Reason why I omitted the IRQs for > > now is > > that the HW is designed to keep the IRQ asserted for whole error > > duration so some delayed ack mechanism would be needed. I would > > like to > > keep the door open for adding IRQs to MFD core. > > You mean to add an IRQ Domain? Yes. I planned to use regmap-irq and create irq chip in MFD when the over / under voltage / temperature - notifications or watchdog IRQs are needed. Best Regards, Matti Vaittinen
Re: [PATCH RESEND v6 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
Hello Lee, On Fri, 2020-11-27 at 08:32 +, Lee Jones wrote: > On Mon, 23 Nov 2020, Matti Vaittinen wrote: > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are > > mainly used to power the R-Car series processors. > > > > Signed-off-by: Matti Vaittinen > > --- > > drivers/mfd/Kconfig | 11 > > drivers/mfd/Makefile | 1 + > > drivers/mfd/rohm-bd9576.c| 108 > > +++ > > include/linux/mfd/rohm-bd957x.h | 59 + > > include/linux/mfd/rohm-generic.h | 2 + > > 5 files changed, 181 insertions(+) > > create mode 100644 drivers/mfd/rohm-bd9576.c > > create mode 100644 include/linux/mfd/rohm-bd957x.h > > Looks like a possible candidate for "simple-mfd-i2c". > > Could you look into that please? > I must admit I didn't know about "simple-mfd-i2c". Good thing to know when working with simple devices :) Is this a new thing? I am unsure I understand the idea fully. Should users put all the different regamp configs in this file and just add the device IDs with pointer to correct config? (BD9576 and BD9573 need volatile ranges). Also, does this mean each sub-device should have own node and own compatible in DT to get correctly load and probed? I guess this would need a buy-in from Rob too then. By the way - for uneducated eyes like mine this does not look like it has much to do with MFD as a device - here MFD reminds me of a simple- bus on top of I2C. Anyways, the BD9576 and BD9573 both have a few interrupts for OVD/UVD conditions and I am expecting that I will be asked to provide the regulator notifiers for those. Reason why I omitted the IRQs for now is that the HW is designed to keep the IRQ asserted for whole error duration so some delayed ack mechanism would be needed. I would like to keep the door open for adding IRQs to MFD core. Best Regards Matti
Re: [RFC PATCH 2/2] power: supply: add sw-gauge for SOC estimation and CC correction
On Thu, 2020-11-26 at 12:13 +0200, Matti Vaittinen wrote:
> Add generic 'sw gauge' helper for performing iterative SOC estimation
> and coulomb counter correction for devices with a (drifting) coulomb
> counter. This should allow few charger/fuel-gauge drivers to use
> generic
> loop instead of implementing their own.
>
> Charger/fuel-gauge drivers can register 'sw-gauge' which does
> periodically
> poll the driver and:
> - get battery state
> - adjust coulomb counter value (to fix drifting caused for example
> by ADC
>offset) if:
> - Battery is relaxed and OCV<=>SOC table is given
> - Battery is full charged
> - get battery age (cycles) from driver
> - get battery temperature
> - do battery capacity correction
> - by battery temperature
> - by battery age
> - by computed Vbat/OCV difference at low-battery condition if
>low-limit is set and OCV table given
> - by IC specific low-battery correction if provided
> - compute current State Of Charge (SOC)
> - do periodical calibration if IC supports that. (Many ICs do
> calibration
>of CC by shorting the ADC pins and getting the offset).
>
> The SW gauge provides the last computed SOC as
> POWER_SUPPLY_PROP_CAPACITY
> to power_supply_class when requested.
>
> Things that should/could be added but are missing from this commit:
> - Support starting calibration in HW when entering to suspend. This
>is useful for ICs supporting delayed calibration to mitigate CC
> error
>during suspend - and to make periodical wake-up less critical.
> - provide the user-space a consistent interface for getting/setting
> the
>battery-cycle information for ICs which can't store the battery
> aging
>information (like how many times battery has been charged to full)
>over reset. Should POWER_SUPPLY_PROP_CYCLE_COUNT be used?
> - periodical wake-up for performing SOC estimation computation (RTC
>integration)
>
> Signed-off-by: Matti Vaittinen
> ---
>
> Please do not spend hours with full formal review. The code here is
> provided to explain what I thought of working with and needs proper
> testing prior full review can be beneficial. I hope this still helps
> to explain what I have on mind. If this is seen worth the further
> work - then I will rework and test this + send a proper code for
> review. This one was only compile tested. I just hope to get opinion
> whether I should put further work on this or not :)
>
> drivers/power/supply/Kconfig| 8 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/power_supply_swgauge.c | 808
>
> include/linux/power/sw_gauge.h | 203 +
> include/linux/power_supply.h| 6 +
> 5 files changed, 1026 insertions(+)
> create mode 100644 drivers/power/supply/power_supply_swgauge.c
> create mode 100644 include/linux/power/sw_gauge.h
>
// Snip
> +
> +static int adjust_cc_full(struct sw_gauge *sw)
> +{
> + int ret, full_uah;
> +
> + if (sw->ops.get_uah_cc_full)
>
> + if (sw->ops.get_cycle) {+ ret = sw-
> >ops.get_uah_cc_full(sw, _uah);
I started converting the ROHM BD71827/BD71828/BD71815 drivers to this
format to get more concrete version... And I noticed I had completely
brainfarted here. Sorry! The idea is of course to set the new CC
value based on information that battery was fully charged. Eg, idea is
to allow driver to not implement get_uah_cc_full but
get_uah_cc_from_full() - to retrieve CC lost since charger informed
battery is full.
> + else
> + ret = sw->ops.get_uah_cc(sw, _uah);
This one is nonsense too. If no get_uah_cc_from_full() is implemented
we assume battery is full right now. What we do next is substract the
uAh's lost since full from designed (and age/temp corrected? TBD)
battery capacity and set this value to CC.
I was after something like:
full_uah = sw->designed_cap - uah_from_full;
As I said - this RFC is not for final review but this particular error
probably obfuscated the logic completely :| Sorry!
> + /*
> + * ROHM algorithm adjusts CC here based on designed capacity
> and not
> + * based on age/temperature corrected capacity.
> + *
> + * Let's try to use age-compensated capacity unlike original
> algorithm.
> + */
> + ret = age_correct_cap(sw, _uah);
> + if (ret) {
> + pr_err("Age correction of battery failed\n");
> + return ret;
> + }
> + return sw->ops.update_cc_uah(sw, full_uah);
> +}
> +/*
> + * Some charger ICs keep count of battery charge systems but can
> only store
> + * one or few cycles. They may need to clear the cycle counter and
> update
> + * counter in SW. This function fetches the counter from HW and
> allows HW to
> + * clear IC counter if needed.
> + *
> + * TODO: Add sysfs to export counter value to user-space and to
> allow
> + * user-space to set it after reset.
And while I was anyways writing this reply
Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
On Wed, 2020-11-11 at 06:41 -0800, Guenter Roeck wrote:
> On 11/11/20 6:01 AM, Vaittinen, Matti wrote:
> > On Thu, 2020-11-05 at 13:38 +0200, Matti Vaittinen wrote:
> > > Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which
> > > are
> > > mainly used to power the R-Car series processors. The watchdog is
> > > pinged using a GPIO and enabled using another GPIO. Additionally
> > > watchdog time-out can be configured to HW prior starting the
> > > watchdog.
> > > Watchdog timeout can be configured to detect only delayed ping or
> > > in
> > > a window mode where also too fast pings are detected.
> > >
> > > Signed-off-by: Matti Vaittinen > > >
> > > Reviewed-by: Guenter Roeck
> > > ---
> > >
> >
> > //snip
> >
> > > + ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout-
> > > ms",
> > > + _margin[0], 1, 2);
> > > + if (ret < 0 && ret != -EINVAL)
> > > + return ret;
> > > +
> > > + if (ret == 1)
> > > + hw_margin_max = hw_margin[0];
> > > +
> > > + if (ret == 2) {
> > > + hw_margin_max = hw_margin[1];
> > > + hw_margin_min = hw_margin[0];
> > > + }
> > > +
> > > + ret = bd957x_set_wdt_mode(priv, hw_margin_max, hw_margin_min);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->always_running = of_property_read_bool(np, "always-
> > > running");
> > > +
> > > + watchdog_set_drvdata(>wdd, priv);
> > > +
> > > + priv->wdd.info = _wdt_ident;
> > > + priv->wdd.ops = _wdt_ops;
> > > + priv->wdd.min_hw_heartbeat_ms = hw_margin_min;
> > > + priv->wdd.max_hw_heartbeat_ms = hw_margin_max;
> > > + priv->wdd.parent= dev;
> > > + priv->wdd.timeout = (hw_margin_max / 2) * 1000;
> >
> > Hmm. Just noticed this value does not make sense, right?
> > Maximum hw_margin is 4416 ms. If I read this correctly timeout
> > should
> > be in seconds - so result is around 2 000 000 seconds here. I
> > think it
> > is useless value...
> >
> > Perhaps
> > priv->wdd.timeout = (hw_margin_max / 2) / 1000;
> > if (!priv->wdd.timeout)
> > priv->wdd.timeout = 1;
> > would be more appropriate.
> >
>
> Yes. Good catch. Actually, since max_hw_heartbeat_ms is specified,
> it can and should be a reasonable constant (like the usual 30
> seconds).
> It does not and should not be bound by max_hw_heartbeat_ms.
Thanks for confirming this Guenter. I'd better admit I didn't
understand how the max_hw_heartbeat_ms works.
If I now read the code correctly, the "watchdog worker" takes care of
feeding for shorter periods than the "timeout" - and only stops
feeding max_hw_heartbeat_ms before timeout expires if userland has not
been feedin wdg. This is really cool approach for short(ish)
max_hw_heartbeat_ms configurations as user-space does not need to meet
"RT requirements". WDG framework is much more advanced that I knew :)
It's nice to learn!
--Matti
Re: [PATCH v5 3/4] wdt: Support wdt on ROHM BD9576MUF and BD9573MUF
On Thu, 2020-11-05 at 13:38 +0200, Matti Vaittinen wrote:
> Add Watchdog support for ROHM BD9576MUF and BD9573MUF PMICs which are
> mainly used to power the R-Car series processors. The watchdog is
> pinged using a GPIO and enabled using another GPIO. Additionally
> watchdog time-out can be configured to HW prior starting the
> watchdog.
> Watchdog timeout can be configured to detect only delayed ping or in
> a window mode where also too fast pings are detected.
>
> Signed-off-by: Matti Vaittinen
> Reviewed-by: Guenter Roeck
> ---
>
//snip
> + ret = of_property_read_variable_u32_array(np, "rohm,hw-timeout-
> ms",
> + _margin[0], 1, 2);
> + if (ret < 0 && ret != -EINVAL)
> + return ret;
> +
> + if (ret == 1)
> + hw_margin_max = hw_margin[0];
> +
> + if (ret == 2) {
> + hw_margin_max = hw_margin[1];
> + hw_margin_min = hw_margin[0];
> + }
> +
> + ret = bd957x_set_wdt_mode(priv, hw_margin_max, hw_margin_min);
> + if (ret)
> + return ret;
> +
> + priv->always_running = of_property_read_bool(np, "always-
> running");
> +
> + watchdog_set_drvdata(>wdd, priv);
> +
> + priv->wdd.info = _wdt_ident;
> + priv->wdd.ops = _wdt_ops;
> + priv->wdd.min_hw_heartbeat_ms = hw_margin_min;
> + priv->wdd.max_hw_heartbeat_ms = hw_margin_max;
> + priv->wdd.parent= dev;
> + priv->wdd.timeout = (hw_margin_max / 2) * 1000;
Hmm. Just noticed this value does not make sense, right?
Maximum hw_margin is 4416 ms. If I read this correctly timeout should
be in seconds - so result is around 2 000 000 seconds here. I think it
is useless value...
Perhaps
priv->wdd.timeout = (hw_margin_max / 2) / 1000;
if (!priv->wdd.timeout)
priv->wdd.timeout = 1;
would be more appropriate.
I need to do some testing when I get the HW at my hands - please don't
apply this patch just yet. I will respin this after some testing - or
if other patches are applied then I will just send this one alone.
Sorry for the hassle...
--Matti
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)
Re: [PATCH v4 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
On Thu, 2020-11-05 at 08:58 +, Lee Jones wrote: > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > On Thu, 2020-11-05 at 08:21 +, Lee Jones wrote: > > > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > > > > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > > > > > Morning Lee, > > > > > > > > > > Thanks for taking a look at this :) I see most of the > > > > > comments > > > > > being > > > > > valid. There's two I would like to clarify though... > > > > > > > > > > On Wed, 2020-11-04 at 15:51 +, Lee Jones wrote: > > > > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs > > > > > > > which > > > > > > > are > > > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > > > matti.vaitti...@fi.rohmeurope.com > > > > > > > --- > > > > > > > + unsigned int chip_type; > > > > > > > + > > > > > > > + chip_type = (unsigned int)(uintptr_t) > > > > > > > + of_device_get_match_data(>dev); > > > > > > > > > > > > Not overly keen on this casting. > > > > > > > > > > > > Why not just leave it as (uintptr_t)? > > > > > > > > > > I didn't do so because on x86_64 the address width is > > > > > probably 64 > > > > > bits > > > > > whereas the unsigned int is likely to be 32 bits. So the > > > > > assignment > > > > > will crop half of the value. It does not really matter as > > > > > values > > > > > are > > > > > small - but I would be surprized if no compilers/analyzers > > > > > emitted a > > > > > warning. > > > > > > > > > > I must admit I am not 100% sure though. I sure can change > > > > > this if > > > > > you > > > > > know it better? > > > > > > What if you used 'long', which I believe changed with the > > > architecture's bus width in Linux? > > > > I think this is exactly what uintptr_t was created for. To provide > > type > > which assures a pointer conversion to integer and back works. > > > > I guess I can change the > > > > unsigned int chip_type; > > > > to uintptr_t and get away with single cast if it looks better to > > you. > > For me the double cast does not look that bad when it allows use of > > native int size variable - but in this case it's really just a > > matter > > of taste. Both should work fine. > > I do see people casting to uintptr and placing the result into a > long. That should work because long is same size as address on architectures supported by Linux. But it still does not "feel correct" to me. Why bothering to use the uintptr_t in first place then? So ... I believe it should *work* on all currently supported architectures if I do: unsigned long chip_type; chip_type = (unsigned long)of_device_get_match_data(>dev); although fact it works does not make it *right* :] But casting to something (uintptr_t) and then assigning to something else (unsigned long) just trusting that it *works* does feel slightly fishy when we could use same type for variable and cast. But again - if unsigned long chip_type; ... chip_type = (unsigned long)of_device_get_match_data(>dev); is what you prefer - I can do that too. Especially in this case where I expect the highest number to definitely stay less than three digits. If we some day hit to architecture where addresses are not of same size as longs, then this is just one minor conversion to do... --Matti
Re: [PATCH v4 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
On Thu, 2020-11-05 at 08:23 +, Lee Jones wrote:
> On Wed, 04 Nov 2020, Lee Jones wrote:
>
> > On Wed, 28 Oct 2020, Matti Vaittinen wrote:
> >
> > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are
> > > mainly used to power the R-Car series processors.
> > >
> > > Signed-off-by: Matti Vaittinen > > >
> > > ---
> > > drivers/mfd/Kconfig | 11 +++
> > > drivers/mfd/Makefile | 1 +
> > > drivers/mfd/rohm-bd9576.c| 130
> > > +++
> > > include/linux/mfd/rohm-bd957x.h | 59 ++
> > > include/linux/mfd/rohm-generic.h | 2 +
> > > 5 files changed, 203 insertions(+)
> > > create mode 100644 drivers/mfd/rohm-bd9576.c
> > > create mode 100644 include/linux/mfd/rohm-bd957x.h
>
> [...]
>
> > > +static const struct regmap_range volatile_ranges[] = {
> > > + {
> > > + .range_min = BD957X_REG_SMRB_ASSERT,
> > > + .range_max = BD957X_REG_SMRB_ASSERT,
> > > + },
> > > + {
> >
> > The way you space your braces is not consistent.
> >
> > > + .range_min = BD957X_REG_PMIC_INTERNAL_STAT,
> > > + .range_max = BD957X_REG_PMIC_INTERNAL_STAT,
> > > + },
> > > + {
> > > + .range_min = BD957X_REG_INT_THERM_STAT,
> > > + .range_max = BD957X_REG_INT_THERM_STAT,
> > > + },
> > > + {
> > > + .range_min = BD957X_REG_INT_OVP_STAT,
> > > + .range_max = BD957X_REG_INT_SYS_STAT,
> > > + }, {
> > > + .range_min = BD957X_REG_INT_MAIN_STAT,
> > > + .range_max = BD957X_REG_INT_MAIN_STAT,
> > > + },
> > > +};
>
> Don't forget about this.
>
> I would prefer to have the braces on the same line (even if it means
> you have to change an extra line when editing), but I'm not 100% dead
> set on it. Consistency however, I am.
>
I won't forget. I intended to write that I was Ok with all the other
comments. Maybe I forgot though. Anyways, I'll fix the inconsistency -
thanks for pointing it out!
--Matti
Re: [PATCH v4 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
On Thu, 2020-11-05 at 08:21 +, Lee Jones wrote: > On Thu, 05 Nov 2020, Vaittinen, Matti wrote: > > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote: > > > Morning Lee, > > > > > > Thanks for taking a look at this :) I see most of the comments > > > being > > > valid. There's two I would like to clarify though... > > > > > > On Wed, 2020-11-04 at 15:51 +, Lee Jones wrote: > > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote: > > > > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which > > > > > are > > > > > mainly used to power the R-Car series processors. > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > matti.vaitti...@fi.rohmeurope.com > > > > > --- > > > > > + unsigned int chip_type; > > > > > + > > > > > + chip_type = (unsigned int)(uintptr_t) > > > > > + of_device_get_match_data(>dev); > > > > > > > > Not overly keen on this casting. > > > > > > > > Why not just leave it as (uintptr_t)? > > > > > > I didn't do so because on x86_64 the address width is probably 64 > > > bits > > > whereas the unsigned int is likely to be 32 bits. So the > > > assignment > > > will crop half of the value. It does not really matter as values > > > are > > > small - but I would be surprized if no compilers/analyzers > > > emitted a > > > warning. > > > > > > I must admit I am not 100% sure though. I sure can change this if > > > you > > > know it better? > > What if you used 'long', which I believe changed with the > architecture's bus width in Linux? I think this is exactly what uintptr_t was created for. To provide type which assures a pointer conversion to integer and back works. I guess I can change the unsigned int chip_type; to uintptr_t and get away with single cast if it looks better to you. For me the double cast does not look that bad when it allows use of native int size variable - but in this case it's really just a matter of taste. Both should work fine. I'll cook v5. --Matti
Re: [PATCH v4 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote:
> Morning Lee,
>
> Thanks for taking a look at this :) I see most of the comments being
> valid. There's two I would like to clarify though...
>
> On Wed, 2020-11-04 at 15:51 +, Lee Jones wrote:
> > On Wed, 28 Oct 2020, Matti Vaittinen wrote:
> >
> > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are
> > > mainly used to power the R-Car series processors.
> > >
> > > Signed-off-by: Matti Vaittinen > > >
> > > ---
> > > + unsigned int chip_type;
> > > +
> > > + chip_type = (unsigned int)(uintptr_t)
> > > + of_device_get_match_data(>dev);
> >
> > Not overly keen on this casting.
> >
> > Why not just leave it as (uintptr_t)?
>
> I didn't do so because on x86_64 the address width is probably 64
> bits
> whereas the unsigned int is likely to be 32 bits. So the assignment
> will crop half of the value. It does not really matter as values are
> small - but I would be surprized if no compilers/analyzers emitted a
> warning.
>
> I must admit I am not 100% sure though. I sure can change this if you
> know it better?
>
> > What happens when you don't cast to (uintptr_t) first?
>
> On some systems at least the gcc will warn:
> > warning: cast from pointer to integer of different size [-Wpointer-
> to-int-cast]
>
> I am pretty sure I did end up this double casting via trial and error
> :)
> +
> > > +static const struct of_device_id bd957x_of_match[] = {
> > > + {
> > > + .compatible = "rohm,bd9576",
> > > + .data = (void *)ROHM_CHIP_TYPE_BD9576,
> > > + },
> > > + {
> >
> > You could put the 2 lines above on a single line.
>
> Braces? I put braces on separate lines on purpose. Been doing this
> after we had this discussion:
>
> https://lore.kernel.org/lkml/20180705055226.GJ496@dell/
> https://lore.kernel.org/lkml/20180706070559.GW496@dell/
>
> ;)
>
> I can change it if you wishfeel it is important - not a point I feel
> like fighting over ;)
>
Ah. I guess you meant:
static const struct of_device_id bd957x_of_match[] = {
{ .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576, },
{ .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573, },
{},
};
Feeling "little bit" stupid... :rolleyes:
> > > + .compatible = "rohm,bd9573",
> > > + .data = (void *)ROHM_CHIP_TYPE_BD9573,
> > > + },
> > > + { },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, bd957x_of_match);
>
> Best Regards
> Matti
>
> --
> Matti Vaittinen, Linux device drivers
> ROHM Semiconductors, Finland
> SWDC
> Kiviharjunlenkki 1E
> 90220 OULU
> FINLAND
>
> ~~~ "I don't think so," said Rene Descartes. Just then he vanished
> ~~~
>
> Simon says - in Latin please.
> "non cogito me" dixit Rene Descarte, deinde evanescavit
>
> (Thanks for the translation Simon)
>
Re: [PATCH v4 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF
Morning Lee,
Thanks for taking a look at this :) I see most of the comments being
valid. There's two I would like to clarify though...
On Wed, 2020-11-04 at 15:51 +, Lee Jones wrote:
> On Wed, 28 Oct 2020, Matti Vaittinen wrote:
>
> > Add core support for ROHM BD9576MUF and BD9573MUF PMICs which are
> > mainly used to power the R-Car series processors.
> >
> > Signed-off-by: Matti Vaittinen
> > ---
> > + unsigned int chip_type;
> > +
> > + chip_type = (unsigned int)(uintptr_t)
> > + of_device_get_match_data(>dev);
>
> Not overly keen on this casting.
>
> Why not just leave it as (uintptr_t)?
I didn't do so because on x86_64 the address width is probably 64 bits
whereas the unsigned int is likely to be 32 bits. So the assignment
will crop half of the value. It does not really matter as values are
small - but I would be surprized if no compilers/analyzers emitted a
warning.
I must admit I am not 100% sure though. I sure can change this if you
know it better?
> What happens when you don't cast to (uintptr_t) first?
On some systems at least the gcc will warn:
> warning: cast from pointer to integer of different size [-Wpointer-
to-int-cast]
I am pretty sure I did end up this double casting via trial and error
:)
+
> > +static const struct of_device_id bd957x_of_match[] = {
> > + {
> > + .compatible = "rohm,bd9576",
> > + .data = (void *)ROHM_CHIP_TYPE_BD9576,
> > + },
> > + {
>
> You could put the 2 lines above on a single line.
Braces? I put braces on separate lines on purpose. Been doing this
after we had this discussion:
https://lore.kernel.org/lkml/20180705055226.GJ496@dell/
https://lore.kernel.org/lkml/20180706070559.GW496@dell/
;)
I can change it if you wishfeel it is important - not a point I feel
like fighting over ;)
>
> > + .compatible = "rohm,bd9573",
> > + .data = (void *)ROHM_CHIP_TYPE_BD9573,
> > + },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, bd957x_of_match);
Best Regards
Matti
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit
(Thanks for the translation Simon)
Re: [PATCH] gpio: bd70528: remove unneeded break
Hello, On Wed, 2020-10-21 at 07:39 -0700, Joe Perches wrote: > On Wed, 2020-10-21 at 07:25 +0000, Vaittinen, Matti wrote: > > Hello Joe & All, > > On Tue, 2020-10-20 at 11:36 -0700, Joe Perches wrote: > > > On Tue, 2020-10-20 at 11:48 +, Vaittinen, Matti wrote: > [] > > > > And for peeps who have not been following - following function > > > > triggers the checkpatch error above: > > > > > > Huh? what version of checkpatch are you using? > > > Send it to me please. > [] > > Please find my version of checkpatch and the patch to trigger the > > warning attached. > > Thanks. This test wasn't particularly useful > (and had some false positives) and was removed by > > commit ef3c005c0eb07a60949191bc6ee407d5f43cc502 > Author: Joe Perches > Date: Tue Aug 11 18:35:19 2020 -0700 > > checkpatch: remove missing switch/case break test > > This test doesn't work well and newer compilers are much better > at emitting this warning. > > Signed-off-by: Joe Perches > Signed-off-by: Andrew Morton > Cc: Cambda Zhu > Link: > http://lkml.kernel.org/r/7e25090c79f6a69d502ab8219863300790192fe2.ca...@perches.com > Signed-off-by: Linus Torvalds > Thanks for checking this Joe! And note to self - always check with newest kernel... ;) (Sorry for bothering) Br, Matti Vaittinen
Re: [PATCH] gpio: bd70528: remove unneeded break
On Tue, 2020-10-20 at 14:46 +0300, Andy Shevchenko wrote: > On Tue, Oct 20, 2020 at 2:26 PM Vaittinen, Matti > wrote: > > On Mon, 2020-10-19 at 12:33 -0700, t...@redhat.com wrote: > > > - break; > > My personal taste is also to omit these breaks but I am pretty sure > > I > > saw some tooling issuing a warning about falling through the > > switch- > > case back when I wrote this. Most probably checkpatch didn't like > > that > > back then. Anyways - if you have no warnings from any of the tools > > - > > this indeed looks better (to me) without the break :) > > JFYI: it's a clang which actually *is* complaining for an extra > break. > Oh. I just replied before seeing this. So actually, checkpatch complains about missing break and clang about existing break. I'm getting much more nagging at work than at home! Best Regards Matti
Re: [PATCH] gpio: bd70528: remove unneeded break
On Tue, 2020-10-20 at 13:07 +0300, Matti Vaittinen wrote:
> Thanks Tom,
>
> On Mon, 2020-10-19 at 12:33 -0700, t...@redhat.com wrote:
> > From: Tom Rix
> >
> > A break is not needed if it is preceded by a return
> >
> > Signed-off-by: Tom Rix
> > ---
> > drivers/gpio/gpio-bd70528.c | 3 ---
> > 1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio-
> > bd70528.c
> > index 45b3da8da336..931e5765fe92 100644
> > --- a/drivers/gpio/gpio-bd70528.c
> > +++ b/drivers/gpio/gpio-bd70528.c
> > @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct
> > gpio_chip *chip, unsigned int offset,
> > GPIO_OUT_REG(offset),
> > BD70528_GPIO_DRIVE_MASK,
> > BD70528_GPIO_OPEN_DRAIN);
> > - break;
> My personal taste is also to omit these breaks but I am pretty sure I
> saw some tooling issuing a warning about falling through the switch-
> case back when I wrote this. Most probably checkpatch didn't like
> that
> back then.
I did a test and removed the breaks. Then I copied the modified file to
drivers/gpio/dummy.c
Next I committed this dummy.c in git, ran git-format-patch -s and
finally ran the checkpatch on this... Following was produced:
[mvaittin@localhost linux]$ scripts/checkpatch.pl 0001-gpio-add-
dummy.patch
Traceback (most recent call last):
File "scripts/spdxcheck.py", line 6, in
from ply import lex, yacc
ImportError: No module named ply
WARNING: added, moved or deleted file(s), does MAINTAINERS need
updating?
#15:
new file mode 100644
WARNING: Possible switch case/default not preceded by break or
fallthrough comment
#91: FILE: drivers/gpio/dummy.c:72:
+ case PIN_CONFIG_DRIVE_PUSH_PULL:
WARNING: Possible switch case/default not preceded by break or
fallthrough comment
#96: FILE: drivers/gpio/dummy.c:77:
+ case PIN_CONFIG_INPUT_DEBOUNCE:
total: 0 errors, 3 warnings, 229 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-
inplace.
0001-gpio-add-dummy.patch has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
I guess that explains the odd "fallthrough" comments you mentioned in
another email. I guess the checkpatch should be fixed before you put
too much effort in clean-up...
And for peeps who have not been following - following function triggers
the checkpatch error above:
static int bd70528_gpio_set_config(struct gpio_chip *chip, unsigned int
offset,
unsigned long config)
{
struct bd70528_gpio *bdgpio = gpiochip_get_data(chip);
switch (pinconf_to_config_param(config)) {
case PIN_CONFIG_DRIVE_OPEN_DRAIN:
return regmap_update_bits(bdgpio->chip.regmap,
GPIO_OUT_REG(offset),
BD70528_GPIO_DRIVE_MASK,
BD70528_GPIO_OPEN_DRAIN);
case PIN_CONFIG_DRIVE_PUSH_PULL:
return regmap_update_bits(bdgpio->chip.regmap,
GPIO_OUT_REG(offset),
BD70528_GPIO_DRIVE_MASK,
BD70528_GPIO_PUSH_PULL);
case PIN_CONFIG_INPUT_DEBOUNCE:
return bd70528_set_debounce(bdgpio, offset,
pinconf_to_config_argument(
config));
default:
break;
}
return -ENOTSUPP;
}
Best Regards
Matti Vaittinen
Re: [PATCH] gpio: bd70528: remove unneeded break
Thanks Tom, On Mon, 2020-10-19 at 12:33 -0700, t...@redhat.com wrote: > From: Tom Rix > > A break is not needed if it is preceded by a return > > Signed-off-by: Tom Rix > --- > drivers/gpio/gpio-bd70528.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio- > bd70528.c > index 45b3da8da336..931e5765fe92 100644 > --- a/drivers/gpio/gpio-bd70528.c > +++ b/drivers/gpio/gpio-bd70528.c > @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct > gpio_chip *chip, unsigned int offset, > GPIO_OUT_REG(offset), > BD70528_GPIO_DRIVE_MASK, > BD70528_GPIO_OPEN_DRAIN); > - break; My personal taste is also to omit these breaks but I am pretty sure I saw some tooling issuing a warning about falling through the switch- case back when I wrote this. Most probably checkpatch didn't like that back then. Anyways - if you have no warnings from any of the tools - this indeed looks better (to me) without the break :) > case PIN_CONFIG_DRIVE_PUSH_PULL: > return regmap_update_bits(bdgpio->chip.regmap, > GPIO_OUT_REG(offset), > BD70528_GPIO_DRIVE_MASK, > BD70528_GPIO_PUSH_PULL); > - break; > case PIN_CONFIG_INPUT_DEBOUNCE: > return bd70528_set_debounce(bdgpio, offset, > pinconf_to_config_argument( > config)); > - break; > default: Actually - my personal taste would be to also get rid of the empty default here - but I guess it was also added to make some tool happy... > break; > } Acked-by: Matti Vaittinen
Re: [PATCH v1 1/6] dt_bindings: mfd: Add ROHM BD9576MUF and BD9573MUF PMICs
Hi dee Ho peeps! On Thu, 2020-09-24 at 09:12 +0300, Matti Vaittinen wrote: > On Wed, 2020-09-23 at 08:27 -0600, Rob Herring wrote: > > On Sat, Sep 19, 2020 at 5:46 AM Vaittinen, Matti > > wrote: > > > Thanks Rob for taking a look at this! > > > > > > On Fri, 2020-09-18 at 11:28 -0600, Rob Herring wrote: > > > > On Thu, Sep 17, 2020 at 11:01:52AM +0300, Matti Vaittinen > > > > wrote: > > > > > Add bindings for ROHM BD9576MUF and BD9573MUF PMICs. These > > > > > PMICs are primarily intended to be used to power the R-Car > > > > > series > > > > > processors. They provide 6 power outputs, safety features and > > > > > a > > > > > watchdog with two functional modes. > > > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > > matti.vaitti...@fi.rohmeurope.com> > > > > > --- > > > > > .../bindings/mfd/rohm,bd9576-pmic.yaml| 129 > > > > > ++ > > > > > 1 file changed, 129 insertions(+) > > > > > create mode 100644 > > > > > Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml // Snip > > > > > + > > > > > + hw_margin_ms: > > > > > > > > Needs a vendor prefix. > > > > > > > > s/_/-/ > > > > > > > > > +minimum: 4 > > > > > +maximum: 4416 > > > > > +description: Watchog timeout in milliseconds > > > > > > > > Maybe the words in the description should be in the property > > > > name > > > > as > > > > I don't see how 'h/w margin' relates to 'watchdog timeout'. > > > > > > The hw_margin_ms is an existing property. As I wrote to Guenter: > > > "hw_margin_ms" is an existing binding for specifying the maximum > > > TMO in > > > HW (if I understood it correctly). (It is used at least by the > > > generig > > > GPIO watchdog) I thought it's better to not invent a new vendor > > > specific binding when we have a generic one. > > > > > > https://elixir.bootlin.com/linux/v5.9-rc2/source/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt > > > > That one is odd and I haven't found an actual user of it. It would > > make more sense as a collection of properties devices could use > > rather > > than a virtual device. > > > > I think I'd do something like 'watchdog-ping-time-msec' that can be > > either ' ' or ''. > > Your suggestion looks good to me. If we introduce such then it would > make sense to add handling for this in GPIO watchdog too. > > What I do wonder is how "hw_margin_ms" is unused? I see it is a > required property for GPIO watchdog. The GPIO WDG probe seems to > actually error out if reading this property fails with any error. I > would assume the GPIO WDG is used somewhere? Hence I am a bit afraid > of > touching it. Breaking existing setups would not be nice. > > Guenter - how do you see this? Should we leave GPIO WDG as it is, > convert it to use this new binding Rob suggested - or support both > the > old and new (at least for some time) in the driver - and possibly > print > a warning when old is used? And one thing more - I don't think the 'watchdog-ping-time-msec' is best candidate as it sounds like the time when one should ping the WDG (SW feature). We already have the timeout-sec defined for that. This new property is to configure/advertice the HW time limit - Eg, time when WDG takes action if it has not been pinged (for max) or takes action if WDG is pinged too quickly (min). Thus I liked hw-margin better than ping-time. (For example with hw-margin <500ms> ... <4000ms> the ping-time 1000 ms would be just fine. Couple of things I would like to get opinions for ... 1. Corect location for this binding - and should it be vendor specific or generic? - I wonder if I should put this new property in rohm-bd9576.yaml? - Should it be vendor specific? - Or should I put it in watchdog.yaml and make it generic? I think it should be generic as many wdg chips implement the timeout configuration. 2. Should we extend WDG core to parse this property if it is placed in watchdog.yaml? 2a) And should we extend watchdog core to call the driver callback for setting timeout if it finds the tmo? 2b) Should we extend driver IF to allow callback for setting min tmo? 2c) Current tmo setting callback uses units of seconds. Should we support setting TMO in ms? I think it might make sense for few specific Linux setups. (I know people use Linux for things that are almost RT - no matter how clever that is. So some might benefit from sub-second scale wdg window). Thoughts? Best Regards Matti Vaittinen -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
Re: [PATCH v1 1/6] dt_bindings: mfd: Add ROHM BD9576MUF and BD9573MUF PMICs
On Wed, 2020-09-23 at 08:27 -0600, Rob Herring wrote: > On Sat, Sep 19, 2020 at 5:46 AM Vaittinen, Matti > wrote: > > Thanks Rob for taking a look at this! > > > > On Fri, 2020-09-18 at 11:28 -0600, Rob Herring wrote: > > > On Thu, Sep 17, 2020 at 11:01:52AM +0300, Matti Vaittinen wrote: > > > > Add bindings for ROHM BD9576MUF and BD9573MUF PMICs. These > > > > PMICs are primarily intended to be used to power the R-Car > > > > series > > > > processors. They provide 6 power outputs, safety features and a > > > > watchdog with two functional modes. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaitti...@fi.rohmeurope.com> > > > > --- > > > > .../bindings/mfd/rohm,bd9576-pmic.yaml| 129 > > > > ++ > > > > 1 file changed, 129 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd9576- > > > > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd9576- > > > > pmic.yaml > > > > new file mode 100644 > > > > index ..f17d4d621585 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd9576- > > > > pmic.yaml > > > > @@ -0,0 +1,129 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/mfd/rohm,bd9576-pmic.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: ROHM BD9576MUF and BD9573MUF Power Management > > > > Integrated > > > > Circuit bindings > > > > + > > > > +maintainers: > > > > + - Matti Vaittinen > > > > + > > > > +description: | > > > > + BD9576MUF and BD9573MUF are power management ICs primarily > > > > intended for > > > > + powering the R-Car series processors. > > > > + The IC provides 6 power outputs with configurable sequencing > > > > and > > > > safety > > > > + monitoring. A watchdog logic with slow ping/windowed modes > > > > is > > > > also included. > > > > + > > > > +properties: > > > > + compatible: > > > > +enum: > > > > + - rohm,bd9576 > > > > + - rohm,bd9573 > > > > + > > > > + reg: > > > > +description: > > > > + I2C slave address. > > > > +maxItems: 1 > > > > + > > > > + interrupts: > > > > +maxItems: 1 > > > > + > > > > + rohm,vout1-en-low: > > > > +description: > > > > + BD9576 and BD9573 VOUT1 regulator enable state can be > > > > individually > > > > + controlled by a GPIO. This is dictated by state of > > > > vout1-en > > > > pin during > > > > + the PMIC startup. If vout1-en is LOW during PMIC startup > > > > then the VOUT1 > > > > + enable sate is controlled via this pin. Set this > > > > property if > > > > vout1-en > > > > + is wired to be down at PMIC start-up. > > > > +type: boolean > > > > + > > > > + rohm,vout1-en-gpios: > > > > +description: > > > > + GPIO specifier to specify the GPIO connected to vout1-en > > > > for > > > > vout1 ON/OFF > > > > + state control. > > > > +maxItems: 1 > > > > + > > > > + rohm,ddr-sel-low: > > > > +description: > > > > + The BD9576 and BD9573 output voltage for DDR can be > > > > selected > > > > by setting > > > > + the ddr-sel pin low or high. Set this property if ddr- > > > > sel is > > > > grounded. > > > > +type: boolean > > > > + > > > > + rohm,watchdog-enable-gpios: > > > > +description: The GPIO line used to enable the watchdog. > > > > +maxItems: 1 > > > > + > > > > + rohm,watchdog-ping-gpios: > > > > +description: The GPIO line used to ping the watchdog. > > > > +maxItems: 1 > > > > + > > >
Re: [PATCH v1 1/6] dt_bindings: mfd: Add ROHM BD9576MUF and BD9573MUF PMICs
Thanks Rob for taking a look at this! On Fri, 2020-09-18 at 11:28 -0600, Rob Herring wrote: > On Thu, Sep 17, 2020 at 11:01:52AM +0300, Matti Vaittinen wrote: > > Add bindings for ROHM BD9576MUF and BD9573MUF PMICs. These > > PMICs are primarily intended to be used to power the R-Car series > > processors. They provide 6 power outputs, safety features and a > > watchdog with two functional modes. > > > > Signed-off-by: Matti Vaittinen > > --- > > .../bindings/mfd/rohm,bd9576-pmic.yaml| 129 > > ++ > > 1 file changed, 129 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml > > > > diff --git a/Documentation/devicetree/bindings/mfd/rohm,bd9576- > > pmic.yaml b/Documentation/devicetree/bindings/mfd/rohm,bd9576- > > pmic.yaml > > new file mode 100644 > > index ..f17d4d621585 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mfd/rohm,bd9576-pmic.yaml > > @@ -0,0 +1,129 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/mfd/rohm,bd9576-pmic.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: ROHM BD9576MUF and BD9573MUF Power Management Integrated > > Circuit bindings > > + > > +maintainers: > > + - Matti Vaittinen > > + > > +description: | > > + BD9576MUF and BD9573MUF are power management ICs primarily > > intended for > > + powering the R-Car series processors. > > + The IC provides 6 power outputs with configurable sequencing and > > safety > > + monitoring. A watchdog logic with slow ping/windowed modes is > > also included. > > + > > +properties: > > + compatible: > > +enum: > > + - rohm,bd9576 > > + - rohm,bd9573 > > + > > + reg: > > +description: > > + I2C slave address. > > +maxItems: 1 > > + > > + interrupts: > > +maxItems: 1 > > + > > + rohm,vout1-en-low: > > +description: > > + BD9576 and BD9573 VOUT1 regulator enable state can be > > individually > > + controlled by a GPIO. This is dictated by state of vout1-en > > pin during > > + the PMIC startup. If vout1-en is LOW during PMIC startup > > then the VOUT1 > > + enable sate is controlled via this pin. Set this property if > > vout1-en > > + is wired to be down at PMIC start-up. > > +type: boolean > > + > > + rohm,vout1-en-gpios: > > +description: > > + GPIO specifier to specify the GPIO connected to vout1-en for > > vout1 ON/OFF > > + state control. > > +maxItems: 1 > > + > > + rohm,ddr-sel-low: > > +description: > > + The BD9576 and BD9573 output voltage for DDR can be selected > > by setting > > + the ddr-sel pin low or high. Set this property if ddr-sel is > > grounded. > > +type: boolean > > + > > + rohm,watchdog-enable-gpios: > > +description: The GPIO line used to enable the watchdog. > > +maxItems: 1 > > + > > + rohm,watchdog-ping-gpios: > > +description: The GPIO line used to ping the watchdog. > > +maxItems: 1 > > + > > + hw_margin_ms: > > Needs a vendor prefix. > > s/_/-/ > > > +minimum: 4 > > +maximum: 4416 > > +description: Watchog timeout in milliseconds > > Maybe the words in the description should be in the property name as > I don't see how 'h/w margin' relates to 'watchdog timeout'. The hw_margin_ms is an existing property. As I wrote to Guenter: "hw_margin_ms" is an existing binding for specifying the maximum TMO in HW (if I understood it correctly). (It is used at least by the generig GPIO watchdog) I thought it's better to not invent a new vendor specific binding when we have a generic one. https://elixir.bootlin.com/linux/v5.9-rc2/source/Documentation/devicetree/bindings/watchdog/gpio-wdt.txt > > Is this a max and below is the min?: > > > + > > + rohm,hw-margin-min-ms: > > +minimum: 2 > > +maximum: 220 > > +description: > > + Watchdog on these ICs can be configured in a window mode > > where the ping > > + must come within certain time-window. Eg. too quick pinging > > will also > > + trigger timeout. Specify the minimum delay between pings if > > you wish to > > + use the window mode. Note, the maximum delay is internally > > configured as > > + a certain multiple of this value so maximum delay can be > > only up to 15 > > + times this value. For example for 73 ms short ping value the > > maximum > > + timeout will be close to 1 sec. Yes. I didn't find existing property for "minimum time to ping WDG" from existing properties. And I think it is not so common to have such "ping window" in watchdog HW that it would warrant generic minimum tmo. Hence vendor specific property for minimum tmo. Would you still prefer me to introduce a new vendor specific property for "max tmo"? Best Regards Matti Vaittinen
