Message-ID: <CANhJrGMEN0QRLoBzntVnaYgfFDyre=Yfw-dNdmi226p6pnpgHw@mail.gmail.com>
Date: Mon, 13 Oct 2025 15:28:20 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Lee Jones <lee@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Pavel Machek <pavel@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Sebastian Reichel <sre@...nel.org>, Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>, 
	Andreas Kemnade <andreas@...nade.info>, linux-leds@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [RFC PATCH 06/13] mfd: bd71828: Support ROHM BD72720

pe 10.10.2025 klo 17.45 Lee Jones (lee@...nel.org) kirjoitti:
>
> On Fri, 10 Oct 2025, Matti Vaittinen wrote:
>
> > Hi deee Ho Lee,
> >
> > And Thanks for the review!
> >
> > On 09/10/2025 19:18, Lee Jones wrote:
> > > On Tue, 07 Oct 2025, Matti Vaittinen wrote:
> > >
> > > > The ROHM BD72720 is a power management IC which continues the BD71828
> > > > family of PMICs. Similarly to the BD71815 and BD71828, the BD72720
> > > > integrates regulators, charger, RTC, clock gate and GPIOs.
> > > >
> > > > The main difference to the earlier PMICs is that the BD72720 has two
> > > > different I2C slave addresses. In addition to the registers behind the
> > > > 'main I2C address', most of the charger (and to some extent LED) control
> > > > is done via registers behind a 'secondary I2C slave address', 0x4c.
> > > >
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>

// snip

> > > > +
> > > > +static struct regmap *bd72720_secondary_regmap;
> > >
> > > Dynamically allocate this and add it to .platform_data once it's
> > > populated.
> > >
> >
> > This can be done but I suppose it's unnecessary churn. This driver does not
> > (at the moment) support more than one instance of the PMIC anyways. (The
> > button data is not alloacted).
> >
> > This is not really a problem as typically there is only 1 of these PMICs to
> > be controlled.
>
> I'd take a few lines of extra code over a globally defined variable any
> day of the week.

Even though that'll require us to drop the const from the
bd72720_mfd_cells MFD cell array? Which, in turn, will probably
require us to drop the const from the MFD cell pointer in probe as
well. Additionally, this will require us to skim through the MFD cell
array in probe, so we locate the power cell, adding one more spot for
errors. I think this is quite a cost just a princible of dropping a
global, which is accessed from one function only. I'd definitely agree
if it was driver data which gets used in a variety of functions, but
here we really just need a memory location for a pointer so MFD can
copy it when kicking the 'sub drivers'. Do you think you can still
reconsider?

>
> > // snip
> >
> > > > +/*
> > > > + * The BD72720 is an odd beast in that it contains two separate sets of
> > > > + * registers, both starting from address 0x0. The twist is that these "pages"
> > > > + * are behind different I2C slave addresses. Most of the registers are behind
> > > > + * a slave address 0x4b, which will be used as the "main" address for this
> > > > + * device.
> > > > + * Most of the charger related registers are located behind slave address 0x4c.
> > > > + * It is tempting to push the dealing with the charger registers and the extra
> > > > + * 0x4c device in power-supply driver - but perhaps it's better for the sake of
> > > > + * the cleaner re-use to deal with setting up all of the regmaps here.
> > > > + * Furthermore, the LED stuff may need access to both of these devices.
> > > > + */
> > > > +#define BD72720_SECONDARY_I2C_SLAVE 0x4c
> > > > +static const struct regmap_range bd72720_volatile_ranges_4b[] = {
> > > > + {
> > > > +         /* RESETSRC1 and 2 are write '1' to clear */
> > > > +         .range_min = BD72720_REG_RESETSRC_1,
> > > > +         .range_max = BD72720_REG_RESETSRC_2,
> > >
> > > regmap_reg_range()?
> >
> > Ah, thanks. Out of the curiosity - do you know why this macro is written on
> > lowercase?
>
> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>
> Signed-off-by: Mark Brown <broonie@...aro.org>
>
> =:-)

Yeah. I just thought that maybe you knew :)

>
> > // snip
> > > > +static int bd72720_set_type_config(unsigned int **buf, unsigned int type,
> > > > +                            const struct regmap_irq *irq_data,
> > > > +                            int idx, void *irq_drv_data)
> > > > +{
> > > > + const struct regmap_irq_type *t = &irq_data->type;
> > > > +
> > > > + /*
> > > > +  * The regmap IRQ ecpects IRQ_TYPE_EDGE_BOTH to be written to register
> > > > +  * as logical OR of the type_falling_val and type_rising_val. This is
> > > > +  * not how the BD72720 implements this configuration, hence we need
> > > > +  * to handle this specific case separately.
> > > > +  */
> > > > + if (type == IRQ_TYPE_EDGE_BOTH) {
> > > > +         buf[0][idx] &= ~t->type_reg_mask;
> > > > +         buf[0][idx] |= BD72720_GPIO_IRQ_TYPE_BOTH;
> > > > +
> > > > +         return 0;
> > > > + }
> > > > +
> > > > + return regmap_irq_set_type_config_simple(buf, type, irq_data, idx,
> > > > +                                          irq_drv_data);
> > >
> > > Use 100-chars to avoid these pointless wraps please.
> >
> > gnarl. I think we have discussed this before :)
> > I would love to keep the lines short - closer to 80 chars - because that way
> > I can fit 3 terminals on my screen. All the years spent staring at the
> > monitor are taking their toll, and my vision isn't as good as it used to be.
> > Frightening thing being that it seems I will only need to increase the font
> > in the future :/
> >
> > Well, sure the lines can be split if you feel strongly about it - but I have
> > a real reason (other than the usual - "they have always been like that") to
> > try keep them short...
>
> Welcome to the year 2000 when 32" monitors are super affordable.

I know. But work rooms where I can fit larger table aren't. Not even
in Finland which should have plenty of space. And my table is really
packed.

Yours,
    -- Matti

-- 

Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Discuss - Estimate - Plan - Report and finally accomplish this:
void do_work(int time) __attribute__ ((const));

Powered by blists - more mailing lists