Hi Andy,

Thank you for the patch.

On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:

Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti:

quoted

From: Haibo Chen <haibo.chen@nxp.com>

The ADP5585 is a 10/11 input/output port expander with a built in keypad
matrix decoder, programmable logic, reset generator, and PWM generator.
This driver supports the GPIO function using the platform device
registered by the core MFD driver.

The driver is derived from an initial implementation from NXP, available
in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add
adp5585-gpio support") in their BSP kernel tree. It has been extensively
rewritten.

Why is this not using gpio-regmap?

...

quoted

+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/mfd/adp5585.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>

+ types.h

...

quoted

+	bit = off * 2 + (off > 5 ? 4 : 0);

Right, but can you use >= 6 here which immediately follows to the next
question, i.e. why not use bank in this conditional?

The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a
set of registers with the same layout. Here the layout is different, the
registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd
rather not use ADP5585_BANK() either. I have decided to use > 5 instead
of >= 6 to match the R5 field name in the comment above:

        /*
         * The bias configuration fields are 2 bits wide and laid down in
         * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits
         * after R5.
         */

...

quoted

+	struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent);

(see below)

quoted

+	struct adp5585_gpio_dev *adp5585_gpio;
+	struct device *dev = &pdev->dev;

	struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent);

I prefer keeping the current ordering, with long lines first, I think
that's more readable.

quoted

+	struct gpio_chip *gc;
+	int ret;

...

quoted

+	platform_set_drvdata(pdev, adp5585_gpio);

Any use of driver data?

In v1, not v2. I'll drop it.

...

quoted

+	device_set_of_node_from_dev(dev, dev->parent);

Why not device_set_node()?

Because device_set_of_node_from_dev() is meant for this exact use case,
where the same node is used for multiple devices. It also puts any
previous dev->of_node, ensuring proper refcounting when devices are
unbound and rebound, without being deleted.

-- 
Regards,

Laurent Pinchart