Messages in this thread | | | From | Linus Walleij <> | Date | Wed, 31 Jan 2024 21:55:52 +0100 | Subject | Re: [PATCH v4 10/18] pinctrl: eyeq5: add platform driver |
| |
Hi Theo,
thanks for your patch!
On Wed, Jan 31, 2024 at 5:27 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:
> Add the Mobileye EyeQ5 pin controller driver. It might grow to add later > support of other platforms from Mobileye. It belongs to a syscon region > called OLB. > > Existing pins and their function live statically in the driver code > rather than in the devicetree, see compatible match data. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
The driver looks very nice and is using all standard features, I'm pretty sure we can merge this soon.
> +static void eq5p_update_bits(const struct eq5p_pinctrl *pctrl, > + enum eq5p_bank bank, enum eq5p_regs reg, > + u32 mask, u32 val) > +{ > + void __iomem *ptr = pctrl->base + eq5p_regs[bank][reg]; > + > + writel((readl(ptr) & ~mask) | (val & mask), ptr); > +}
This is in practice a reimplementation of regmap MMIO.
Can't you just use regmap MMIO to access the banks then...?
Maybe it doesn't add much here. I'm not sure.
> +static bool eq5p_readl_bit(const struct eq5p_pinctrl *pctrl,
eq5p_test_bit() maybe? that describes better what the function does.
> + enum eq5p_bank bank, enum eq5p_regs reg, int bit) > +{ > + u32 val = readl(pctrl->base + eq5p_regs[bank][reg]); > + > + return (val & BIT(bit)) != 0; > +}
Maybe add a check for bit > 31?
> +static int eq5p_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, > + unsigned int selector, > + const unsigned int **pins, > + unsigned int *num_pins) > +{ > + *pins = &pctldev->desc->pins[selector].number; > + *num_pins = 1; > + return 0; > +}
One pin per group, also known as the "qualcomm trick".
(It's fine.)
> + mask = 0b11 << offset;
That's pretty nonstandard but it's quite readable so let's keep it!
Yours, Linus Walleij
| |