Re: [PATCH v4 4/6] mfd: max77759: add Maxim MAX77759 core mfd driver
From: André Draszik
Date: Tue Mar 25 2025 - 03:44:17 EST
Thanks Lee for your patience,
On Fri, 2025-03-21 at 10:54 +0000, Lee Jones wrote:
> On Fri, 14 Mar 2025, André Draszik wrote:
>
> > Hi Lee,
> >
> > Thanks for your review!
> >
> > On Fri, 2025-03-14 at 12:31 +0000, Lee Jones wrote:
> > > On Wed, 12 Mar 2025, André Draszik wrote:
[...]
> >
> > > This sort of goes without saying. Suggest you remove this comment.
> > >
> > > > +/* PMIC / TOP */
> > > > +#define MAX77759_PMIC_REG_PMIC_ID 0x00
> > > > +#define MAX77759_PMIC_REG_PMIC_ID_MAX77759 59
> > >
> > > Is this a register or a value?
> >
> > It's a value, which the suffix in the macro name was meant to
> > convey :-) 59 is the value (in decimal) for max77759 that
> > the ID register is expected to contain.
>
> You mean the REG suffix, that is in both the register address and value?
With suffix, I meant the last part '_MAX77759$' above.
>
> And why do you have MAX77759 in there twice? That's just odd.
Everything starts with max77759 in this driver, for namespace reasons. The
suffix could be different if support for a different ICs was added in the
future, but I'll use your suggested enum below instead.
>
> Suggest that expected values, like IDs are placed in an enum with the
> other supported platforms
>
> enum {
> MAX77759_CHIP_ID = 59,
> MAX77760_CHIP_ID = 60,
> MAX77761_CHIP_ID = 61,
> };
OK, will do, although this introduces a new prefix for a symbol name
in this driver in case support for a different ICs was added in the
future.
> >
> > >
> > > > +#define MAX77759_PMIC_REG_PMIC_REVISION 0x01
> > > > +#define MAX77759_PMIC_REG_OTP_REVISION 0x02
> > > > +
> > > > +#define MAX77759_PMIC_REG_INTSRC 0x22
> > > > +#define MAX77759_PMIC_REG_INTSRCMASK 0x23
> > > > +#define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3)
> > > > +#define MAX77759_PMIC_REG_INTSRC_TOPSYS BIT(1)
> > > > +#define MAX77759_PMIC_REG_INTSRC_CHGR BIT(0)
> > >
> > > These look like bit offsets rather than reg addresses?
> >
> > Of course, could you please clarify what you're hinting at
> > here?
>
> Register bits/masks {c,sh}ould be indented below the register names:
>
> #define MAX77759_PMIC_REG_INTSRCMASK 0x23
> #define MAX77759_PMIC_REG_INTSRC_MAXQ BIT(3)
Thanks for your clarification.
> > > > +#define MAX77759_PMIC_REG_TOPSYS_INT 0x24
> > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_MASK 0x26
> > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_TSHDN BIT(6)
> > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSOVLO BIT(5)
> > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_SYSUVLO BIT(4)
> > > > +#define MAX77759_PMIC_REG_TOPSYS_INT_FSHIP BIT(0)
> > > > +
> > > > +#define MAX77759_PMIC_REG_I2C_CNFG 0x40
> > > > +#define MAX77759_PMIC_REG_SWRESET 0x50
> > > > +#define MAX77759_PMIC_REG_CONTROL_FG 0x51
> > > > +#define MAX77759_PMIC_REG_LAST_REGISTER MAX77759_PMIC_REG_CONTROL_FG
> > >
> > > You could just use MAX77759_PMIC_REG_CONTROL_FG in-place?
> >
> > I think this makes it more obvious in the regmap definition
> > below - struct regmap_config::max_register vs
> > struct regmap_config::num_reg_defaults_raw
>
> The attributes are already suitably named.
>
> Creating a new define to make this doubly obvious is not required.
Ok.
>
> [...]
>
> > > Where are the other registers?
> >
> > The intention is to add them once we start working on charger.
>
> Add it when you do instead.
I've added them here now.
>
> > > > +enum max77759_i2c_subdev_id {
> > > > + MAX77759_I2C_SUBDEV_ID_MAXQ,
> > > > + MAX77759_I2C_SUBDEV_ID_CHARGER,
> > >
> > > Are these truly arbitrary or are you relying on the fact that the
> > > compiler usually starts from 0 and incs by 1?
> >
> > It's not arbitrary. A conforming compiler is required to start with
> > 0, this is part of the C standard, e.g.
> > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf 6.7.2.2.3
>
> I'm aware of the standard.
>
> However from a readability perspective they look arbitrary, so either
> add a comment to say that you're relying on this behaviour and that the
> presence and order of each entry is fixed and should not be amended or
> do future contributors a favour and hard code them from the beginning.
I've added a comment to clarify.
> [...]
>
> > > > + struct mutex maxq_lock;
> > > > + struct regmap *regmap_maxq;
> > > > + struct completion cmd_done;
> > > > +
> > > > + struct regmap *regmap_top;
> > >
> > > What is top?
> >
> > One of its functional blocks as per the data sheet. I think
> > it makes sense to use nomenclature from it.
>
> Maybe a comment should be added to say what top actually represents.
>
> Not all readers of this code will have the datasheet open.
I've added a kerneldoc to the struct, hopefully that's good enough :-)
>
> > > > + struct regmap *regmap_charger;
> > > > +};
> > > > +
> > > > +struct max77759_i2c_subdev {
> > > > + enum max77759_i2c_subdev_id id;
> > > > + const struct regmap_config *cfg;
> > > > + u16 i2c_address;
> > > > +};
> > > > +
> > > > +/* TOP registers */
> > > > +static const struct regmap_range max77759_top_registers[] = {
> > > > + regmap_reg_range(0x00, 0x02),
> > > > + regmap_reg_range(0x22, 0x24),
> > > > + regmap_reg_range(0x26, 0x26),
> > > > + regmap_reg_range(0x40, 0x40),
> > > > + regmap_reg_range(0x50, 0x51),
> > >
> > > What are these magic numbers? Can you define them?
> >
> > This is on purpose, as I think it makes it much easier to have
> > the ranges as numbers, otherwise you always have to look them up
> > in the header when trying to verify correctness or simply looking
> > at this. Additionally, things wouldn't be nicely aligned anymore,
> >
> > Without data sheet, whether these are numbers or macro names
> > makes no difference, and it's impossible to reason about them.
> >
> > With data sheet, it's easier this way to compare.
> >
> > But I'll change them.
>
> Or provide come helpful comments. Magic numbers are generally decried.
>
> This is made worse here, since it's not obvious what this function block does.
I've added some comments, hopefully better :-)
[...
> > > > + struct device *dev = regmap_get_device(max77759_mfd->regmap_maxq);
> > >
> > > > + static const unsigned int timeout_ms = 200;
> > >
> > > Why 200?
> >
> > This is what downstream uses, I can add a comment to say so.
>
> We're not generally interested in downstream here.
>
> Why do downstream use it? Is there a value in the datasheet?
No, there isn't. We know that 200ms must work reliably since downstream
uses that value, hence I re-used it.
I've dropped it to 20ms now, since in most cases in very simple tests it
takes less than 5ms, but I don't know if there are cases when e.g.
the uC is busy (e.g doing USB-C related handling) where it might take
longer. I guess we'll see.
>
> > > > +
> > > > + if (cmd->length > MAX77759_MAXQ_REG_AP_MESSAGESZ_MAX)
> > > > + return -EINVAL;
> > > > +
> > > > + /* rsp is allowed to be NULL. In that case we do need a temporary. */
> > >
> > > More details please.
> > >
> > > Why is NULL valid?
> >
> > The kernel doc explains it:
> >
> > * @rsp: Any response data associated with the command will be copied here;
> > * can be %NULL if the command has no response (other than ACK).
> >
> > Do you want me to duplicate this information?
>
> You already did, just in a less useful way.
>
> It's arguably more critical to document it here than tucked away in the header.
>
> > > Why does it need a placeholder? What are you using as the placeholder?
> >
> > Here, we still need a location to write the response to, as we
> > need to verify that the command was indeed completed correctly.
>
> Then please tell all of the future readers who may have the same queries.
The header / kdoc is for API users, so it makes sense to have it there. I've
updated the comment here as well.
> > > > [...]
> > > > + ret = devm_mutex_init(&client->dev, &max77759_mfd->maxq_lock);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + i2c_set_clientdata(client, max77759_mfd);
> > > > +
> > > > + for (int i = 0; i < ARRAY_SIZE(max77759_i2c_subdevs); ++i) {
> > >
> > > Any reason for the pre-increment?
> > >
> > > If not, it's more standards to post-inc.
> >
> > My reason is that historically compilers have created better code
> > with preinc, and I generally still prefer it, to make it obvious.
>
> Is this still true today?
>
> Historically, we used program computers with punched cards as well. =:-)
>
> > There are thousands of users doing preinc in for loops in the kernel
> > tree, so unless you really insist, I'd like to keep it that way for
> > that reason if you're OK with reason :-)
>
> Let's play top-trumps:
>
> % git grep "++i" | wc -l
> 7062
> % git grep "i++" | wc -l
> 75158
>
> There is more than an order of magnitude of difference between the
> styles and I bet quite a few of the aforementioned cases were authored
> because they _required_ pre-inc.
It's unlikely that
$ git grep 'for (i.*++i' | wc -l
5861
_require_ pre-inc. But sure, since that's the preference, I've changed it
>
> [...]
>
> > > > diff --git a/include/linux/mfd/max77759.h b/include/linux/mfd/max77759.h
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..b038b4e9b748287e23e3a7030496f09dc8bdc816
> > > > --- /dev/null
> > > > +++ b/include/linux/mfd/max77759.h
> > > > @@ -0,0 +1,98 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * Copyright 2020 Google Inc.
> > > > + * Copyright 2025 Linaro Ltd.
> > > > + *
> > > > + * Client interface for Maxim MAX77759 MFD driver
> > > > + */
> > > > +
> > > > +#ifndef __LINUX_MFD_MAX77759_H
> > > > +#define __LINUX_MFD_MAX77759_H
> > > > +
> > > > +/* MaxQ opcodes */
> > > > +#define MAX77759_MAXQ_OPCODE_MAXLENGTH 33
> > > > +
> > > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_READ 0x21
> > > > +#define MAX77759_MAXQ_OPCODE_GPIO_TRIGGER_WRITE 0x22
> > > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_READ 0x23
> > > > +#define MAX77759_MAXQ_OPCODE_GPIO_CONTROL_WRITE 0x24
> > > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_READ 0x81
> > > > +#define MAX77759_MAXQ_OPCODE_USER_SPACE_WRITE 0x82
> > > > +
> > > > +/*
> > > > + * register map (incomplete) - registers not useful for drivers are not
> > > > + * declared here
> > > > + */
> > > > +/* MaxQ */
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1 0x64
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_APCMDRESI BIT(7)
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_SYSMSGI BIT(6)
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO6I BIT(1)
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIO5I BIT(0)
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, en) (((en) & 1) << (offs))
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_GPIOxI_MASK(offs) \
> > > > + MAX77759_MAXQ_REG_UIC_INT1_GPIOxI(offs, ~0)
> > > > +
> > > > +#define MAX77759_MAXQ_REG_UIC_INT2 0x65
> > > > +#define MAX77759_MAXQ_REG_UIC_INT3 0x66
> > > > +#define MAX77759_MAXQ_REG_UIC_INT4 0x67
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS1 0x68
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS2 0x69
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS3 0x6a
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS4 0x6b
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS5 0x6c
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS6 0x6d
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS7 0x6f
> > > > +#define MAX77759_MAXQ_REG_UIC_UIC_STATUS8 0x6f
> > > > +#define MAX77759_MAXQ_REG_UIC_INT1_M 0x70
> > > > +#define MAX77759_MAXQ_REG_UIC_INT2_M 0x71
> > > > +#define MAX77759_MAXQ_REG_UIC_INT3_M 0x72
> > > > +#define MAX77759_MAXQ_REG_UIC_INT4_M 0x73
> > > > +
> > > > +/* charger */
> > > > +#define MAX77759_CHGR_REG_CHG_INT 0xb0
> > > > +#define MAX77759_CHGR_REG_CHG_INT2 0xb1
> > > > +#define MAX77759_CHGR_REG_CHG_INT_MASK 0xb2
> > > > +#define MAX77759_CHGR_REG_CHG_INT2_MASK 0xb3
> > > > +
> > > > +struct max77759_mfd;
> > >
> > > Place the definition in here instead.
> >
> > I would like to keep it private. There is no need for it to
> > become public, it's meant to be an opaque handle.
>
> No, it's not.
>
> It's device data that you already shared with all of the children.
>
> If you don't want them to have it, don't give it to them.
It's normal to have opaque handles for API users if those API users
don't need to know implementation details, like in this case.
Anyway, I've made it public.
Thanks Lee!
Cheers,
A.