Hi Linus,

On Wed Jan 31, 2024 at 9:55 PM CET, Linus Walleij wrote:

Hi Theo,

thanks for your patch!

On Wed, Jan 31, 2024 at 5:27 PM Théo Lebrun [off-list ref] wrote:

quoted

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.

It is useful to get some feedback that tells me your state of mind as
one involved maintainer. Thanks.

quoted

+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.

Indeed, I went the minimalist route. You tell me if you'd prefer an MMIO
regmap.

I've not seen any helper to get a regmap based on a resource, targeting
by name. Is the expected procedure to acquire the resource then create
a regmap config then call devm_regmap_init_mmio()?

quoted

+static bool eq5p_readl_bit(const struct eq5p_pinctrl *pctrl,

eq5p_test_bit() maybe? that describes better what the
function does.

Good idea, thanks.

quoted

+                          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?

Will do. I like that sort of defensive programming. What behavior would
you expect?
 - WARN_ON(bit > 31) and return false?
 - Just return false?
 - Something else?

Actually looking at uses of eq5p_readl_bit() I'm thinking about a bug
that might be occuring wrt the second bank and that offset. I'll make
sure to fix it for next revision.

quoted

+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.)

:-)

Thanks!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com