On Thu, 2021-05-20 at 14:22 +0200, Michael Walle wrote:

Am 2021-05-20 14:00, schrieb Matti Vaittinen:

quoted

On Thu, 2021-05-20 at 13:42 +0200, Michael Walle wrote:

quoted

Am 2021-05-20 13:28, schrieb Matti Vaittinen:

quoted

The set_config and init_valid_mask GPIO operations are usually
very
IC
specific. Allow IC drivers to provide these custom operations
at
gpio-regmap registration.

Signed-off-by: Matti Vaittinen <
matti.vaittinen@fi.rohmeurope.com>
---
 drivers/gpio/gpio-regmap.c  | 49
+++++++++++++++++++++++++++++++++++++
 include/linux/gpio/regmap.h | 13 ++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-

regmap.c
index 134cedf151a7..315285cacd3f 100644

--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c

@@ -27,6 +27,10 @@ struct gpio_regmap {
 	int (*reg_mask_xlate)(struct gpio_regmap *gpio,

unsigned int
base,
 			      unsigned int offset, unsigned int
*reg,
 			      unsigned int *mask);
+	int (*set_config)(struct regmap *regmap, void *drvdata,
+			  unsigned int offset, unsigned long
config);
+	int (*init_valid_mask)(struct regmap *regmap, void
*drvdata,
+				unsigned long *valid_mask,
unsigned int
ngpios);

Maybe we should also make the first argument a "struct
gpio_regmap"
and provide a new gpio_regmap_get_regmap(struct gpio_regmap).
Thus
having a similar api as for the reg_mask_xlate(). Andy?

I don't really see the reason of making this any more complicated
for
IC drivers. If we don't open the struct gpio_regmap to IC drivers -
then they never need the struct gpio_regmap pointer itself but each
IC
driver would need to do some unnecessary function call
(gpio_regmap_get_regmap() in this case). I'd say that would be
unnecessary bloat.

If there is ever the need of additional parameters, you'll have to
modify that parameter list. Otherwise you'll just have to add a new
function. Thus might be more future proof.

I do hope the "void *drvdata" allows enough flexibility so that there
is no need to add new parameters. And if there is, then I don't see how
the struct gpio_regmap pointer would have saved us - unless we open the
contents of struct gpio_regmap to IC drivers. (Which might make sense
because that already contains plenty of register details which may need
to be duplicated to drvdata for some IC-specific callbacks. Here we
again have analogy to regulator_desc - which I have often used also in
IC-specific custom callbacks. But as long as we hope to keep the struct
gpio_regmap private I would not add it in arguments).

But I won't object to it.

quoted

quoted

quoted

@@ -39,6 +43,43 @@ static unsigned int

gpio_regmap_addr(unsigned
int
addr)
 	return addr;
 }

+static int regmap_gpio_init_valid_mask(struct gpio_chip *gc,
+					unsigned long
*valid_mask,
+					unsigned int ngpios)
+{
+	struct gpio_regmap *gpio;
+	void *drvdata;
+
+	gpio = gpiochip_get_data(gc);
+
+	if (!gpio->init_valid_mask) {
+		WARN_ON(!gpio->init_valid_mask);
+		return -EINVAL;
+	}

Why not the following?

if (!gpio->init_valid_mask)
     return 0;

It just feels like an error if regmap_gpio_init_valid_mask() is
ever
called by core without having the gpio->init_valid_mask set.
Probably
this would mean that the someone has errorneously modified the
gpio-

quoted

init_valid_mask set after gpio_regmap registration - whih smells
like

a problem. Thus the WARN() sounds like a correct course of action
to
me. (I may be wrong though - see below)

quoted

Thus copying the behavior of gpiolib.

I must admit I didn't check how this is implemented in gpiolib. But
the
gpio_chip's init_valid_mask should not be set if regmap_gpio_config
does not have valid init_valid_mask pointer at registration. Thus
it
smells like an error to me if the GPIO core calls the
regmap_gpio_init_valid_mask() and regmap_gpio has not set the
init_valid_mask pointer. But as I said, I haven't looked in gpiolib
for
this so I may be wrong.

Oh, I missed that you only set it when it is set in the
gpio_regmap_config. Thus, I'd say drop it entirely? It is only within
this module where things might go wrong.

I have no strong opinion on this. I can drop it if it's not needed.

quoted

quoted

quoted

+
+	drvdata = gpio_regmap_get_drvdata(gpio);
+
+	return gpio->init_valid_mask(gpio->regmap, drvdata,
valid_mask,
ngpios);
+}
+
+static int gpio_regmap_set_config(struct gpio_chip *gc,
unsigned
int
offset,
+				  unsigned long config)
+{
+	struct gpio_regmap *gpio;
+	void *drvdata;
+
+	gpio = gpiochip_get_data(gc);
+
+	if (!gpio->set_config) {
+		WARN_ON(!gpio->set_config);
+		return -EINVAL;
+	}

same here, return -ENOTSUPP.

As above -
if (!gpio->set_config) {
	the gpio-core should never call gpio_regmap_set_config() if the
}

Maybe I should add a comment to clarify the WARN() ?

quoted

quoted

+
+	drvdata = gpio_regmap_get_drvdata(gpio);
+
+	return gpio->set_config(gpio->regmap, drvdata, offset,
config);
+}
+
 static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
 				    unsigned int base, unsigned
int
offset,
 				    unsigned int *reg, unsigned
int
*mask)

@@ -235,6 +276,8 @@ struct gpio_regmap

*gpio_regmap_register(const
struct gpio_regmap_config *config
 	gpio->reg_clr_base = config->reg_clr_base;
 	gpio->reg_dir_in_base = config->reg_dir_in_base;
 	gpio->reg_dir_out_base = config->reg_dir_out_base;
+	gpio->set_config = config->set_config;
+	gpio->init_valid_mask = config->init_valid_mask;

 	/* if not set, assume there is only one register */
 	if (!gpio->ngpio_per_reg)

@@ -253,6 +296,10 @@ struct gpio_regmap

*gpio_regmap_register(const
struct gpio_regmap_config *config
 	chip->ngpio = config->ngpio;
 	chip->names = config->names;
 	chip->label = config->label ?: dev_name(config-

quoted

parent);

+	if (gpio->set_config)
+		chip->set_config = gpio_regmap_set_config;
+	if (gpio->init_valid_mask)
+		chip->init_valid_mask =
regmap_gpio_init_valid_mask;

 #if defined(CONFIG_OF_GPIO)
 	/* gpiolib will use of_node of the parent if chip-

quoted

of_node is

NULL */

@@ -280,6 +327,8 @@ struct gpio_regmap

*gpio_regmap_register(const
struct gpio_regmap_config *config
 		chip->direction_output =
gpio_regmap_direction_output;
 	}

+	gpio_regmap_set_drvdata(gpio, config->drvdata);

I'm wondering if we need the gpio_regmap_set_drvdata() anymore or
if
we can just drop it entirely.

I wouldn't drop it. I think there _may_ be cases where the drvdata
is
set only after the registration. (Just my gut-feeling, I may be
wrong
though)

Ok, but as you already mentioned on IRC, it could be a bit of a trap
because it might already be used after gpiochip_add_data() and thus
be NULL if not provided by gpio_regmap_config().

Hmm.. I think you are right. Setting the drvdata after registration is
a call for a race. After that reasoning I agree with you that this
should be dropped.

Best Regards
	Matti Vaittinen