[PATCH 0/5] Few miscellaneous regulator improvements

[PATCH 0/5] Few miscellaneous regulator improvements

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

Minor regulator improvemnts / fixes.

This is a collection of minor improvemnts done while developing drivers
for yet another ROHM PMIC. Please note that the new generic function
added in Patch 2 does currently have only one in-tree driver using it.
(call added in patch 3). I intended to post this with the set of patches
bringing support to a new PMIC - but it appears this new PMIC requires
one more HW-iteration - meaning it will be delayed probably by months.
It'd be easier for me to add this upstream now in order to avoid
rebasing/conflicts with other changes introduced meanwhile. Please let
me know if you don't think that's Ok.

Patch 1:
IRQ-helpers do not really need to map IRQ to specific notification in
cases when the sole purpose of an IRQ is to notify this specific error.
Allow omitting the IRQ mapping callback.

Patch 2:
The new PMIC I am writing drivers for does once again allow (base) voltage
to be changed when regulator is disabled. Create a generic function for this
as it seems ROHM keeps designing such outputs.

Patch 3:
Use this generic function with the bd71837/47/50

Patch 4 & 5:
kerneldoc updates.

--

Matti Vaittinen (5):
  regulator: irq_helpers: Allow omitting map_event for simple IRQs
  regulator: rohm-regulator: add helper for restricted voltage setting
  regulator: bd718x7: Use rohm generic restricted voltage setting
  regulator: Add units to limit documentation
  regulator: Update protection IRQ helper docs

 drivers/regulator/bd718x7-regulator.c | 29 ++------------
 drivers/regulator/irq_helpers.c       | 52 +++++++++++++++++++++++-
 drivers/regulator/rohm-regulator.c    | 16 ++++++++
 include/linux/mfd/rohm-generic.h      |  7 ++++
 include/linux/regulator/driver.h      | 58 ++++++++++++++++++++++-----
 5 files changed, 126 insertions(+), 36 deletions(-)

-- 
2.31.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/5] regulator: irq_helpers: Allow omitting map_event for simple IRQs

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5909 bytes --]

If the device has dedicated IRQ (or irq controller already splits the IRQ
appropriately) for single error indication on a single regulator then the
map-event callback has not much to do.

Simplify the usage for such devices by using a common helper function to
return the regulator and the reason when map_event is not populated.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

---
 drivers/regulator/irq_helpers.c  | 52 ++++++++++++++++++++++++++++++--
 include/linux/regulator/driver.h | 36 ++++++++++++++++++++++
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/irq_helpers.c b/drivers/regulator/irq_helpers.c
index 522764435575..e06127006c1b 100644
--- a/drivers/regulator/irq_helpers.c
+++ b/drivers/regulator/irq_helpers.c
@@ -153,6 +153,42 @@ static void regulator_notifier_isr_work(struct work_struct *work)
 				 msecs_to_jiffies(tmo));
 }
 
+static bool single_bit_set(int val, int bits_to_check)
+{
+	int bit;
+	const unsigned long bits = val;
+
+	bit = find_first_bit(&bits, bits_to_check);
+	if (bit == bits_to_check)
+		return false;
+
+	bit = find_next_bit(&bits, bits_to_check, bit + 1);
+
+	return (bit == bits_to_check);
+}
+
+static int map_event_simple(int irq, struct regulator_irq_data *rid,
+			    unsigned long *dev_mask)
+{
+	int err = rid->states[0].possible_errs;
+
+	*dev_mask = 1;
+	/*
+	 * This helper should only be used in a situation where the IRQ
+	 * can indicate only one type of problem for one specific rdev.
+	 * Something fishy is going on if we are having multiple rdevs or ERROR
+	 * flags here.
+	 */
+	if (WARN_ON(rid->num_states != 1 ||
+	    !single_bit_set(err, sizeof(err) * 8)))
+		return 0;
+
+	rid->states[0].errors = err;
+	rid->states[0].notifs = regulator_err2notif(err);
+
+	return 0;
+}
+
 static irqreturn_t regulator_notifier_isr(int irq, void *data)
 {
 	struct regulator_irq *h = data;
@@ -320,7 +356,10 @@ static void init_rdev_errors(struct regulator_irq *h)
  *			IRQF_ONESHOT when requesting the (threaded) irq.
  * @common_errs:	Errors which can be flagged by this IRQ for all rdevs.
  *			When IRQ is re-enabled these errors will be cleared
- *			from all associated regulators
+ *			from all associated regulators. Use this instead of the
+ *			per_rdev_errs if you have a simple device where the
+ *			IRQ can indicate only one type of error for one specific
+ *			regulator (and you omitted the map_event).
  * @per_rdev_errs:	Optional error flag array describing errors specific
  *			for only some of the regulators. These errors will be
  *			or'ed with common errors. If this is given the array
@@ -341,7 +380,7 @@ void *regulator_irq_helper(struct device *dev,
 	struct regulator_irq *h;
 	int ret;
 
-	if (!rdev_amount || !d || !d->map_event || !d->name)
+	if (!rdev_amount || !d || !d->name)
 		return ERR_PTR(-EINVAL);
 
 	h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
@@ -351,6 +390,15 @@ void *regulator_irq_helper(struct device *dev,
 	h->irq = irq;
 	h->desc = *d;
 
+	if (!h->desc.map_event) {
+		if (rdev_amount != 1 ||
+		    !single_bit_set(common_errs, sizeof(common_errs) * 8) ||
+		    per_rdev_errs)
+			return ERR_PTR(-EINVAL);
+
+		h->desc.map_event = map_event_simple;
+	}
+
 	ret = init_rdev_state(dev, h, rdev, common_errs, per_rdev_errs,
 			      rdev_amount);
 	if (ret)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index bd7a73db2e66..f9d1115627e5 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -529,6 +529,8 @@ struct regulator_irq_data {
  *		status reading from IC failed. If this is repeated for
  *		fatal_cnt times the core will call die() callback or power-off
  *		the system as a last resort to protect the HW.
+ *		Simple devices where a particular IRQ can only indicate one
+ *		type of error, for one regulator can omit this callback.
  * @renable:	Optional callback to check status (if HW supports that) before
  *		re-enabling IRQ. If implemented this should clear the error
  *		flags so that errors fetched by regulator_get_error_flags()
@@ -644,6 +646,40 @@ struct regulator_dev {
 	spinlock_t err_lock;
 };
 
+/*
+ * Convert error flags to corresponding notifications.
+ *
+ * Can be used by drivers which use the notification helpers to
+ * find out correct notification flags based on the error flags. Drivers
+ * can avoid storing both supported notification and error flags which
+ * may save few bytes.
+ */
+static inline int regulator_err2notif(int err)
+{
+	switch (err) {
+	case REGULATOR_ERROR_UNDER_VOLTAGE:
+		return REGULATOR_EVENT_UNDER_VOLTAGE;
+	case REGULATOR_ERROR_OVER_CURRENT:
+		return REGULATOR_EVENT_OVER_CURRENT;
+	case REGULATOR_ERROR_REGULATION_OUT:
+		return REGULATOR_EVENT_REGULATION_OUT;
+	case REGULATOR_ERROR_FAIL:
+		return REGULATOR_EVENT_FAIL;
+	case REGULATOR_ERROR_OVER_TEMP:
+		return REGULATOR_EVENT_OVER_TEMP;
+	case REGULATOR_ERROR_UNDER_VOLTAGE_WARN:
+		return REGULATOR_EVENT_UNDER_VOLTAGE_WARN;
+	case REGULATOR_ERROR_OVER_CURRENT_WARN:
+		return REGULATOR_EVENT_OVER_CURRENT_WARN;
+	case REGULATOR_ERROR_OVER_VOLTAGE_WARN:
+		return REGULATOR_EVENT_OVER_VOLTAGE_WARN;
+	case REGULATOR_ERROR_OVER_TEMP_WARN:
+		return REGULATOR_EVENT_OVER_TEMP_WARN;
+	}
+	return 0;
+}
+
+
 struct regulator_dev *
 regulator_register(const struct regulator_desc *regulator_desc,
 		   const struct regulator_config *config);
-- 
2.31.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/5] regulator: rohm-regulator: add helper for restricted voltage setting

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]

Few ROHM PMICs have regulators where voltage setting can be done only
when regulator is disabled. Add helper for those PMICs.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/regulator/rohm-regulator.c | 16 ++++++++++++++++
 include/linux/mfd/rohm-generic.h   |  7 +++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/regulator/rohm-regulator.c b/drivers/regulator/rohm-regulator.c
index 6e0d9c08ec1c..f97a9a51ee76 100644
--- a/drivers/regulator/rohm-regulator.c
+++ b/drivers/regulator/rohm-regulator.c
@@ -112,6 +112,22 @@ int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
 }
 EXPORT_SYMBOL(rohm_regulator_set_dvs_levels);
 
+/*
+ * Few ROHM PMIC ICs have constrains on voltage changing:
+ * BD71837 - only buck 1-4 voltages can be changed when they are enabled.
+ * Other bucks and all LDOs must be disabled when voltage is changed.
+ * BD96801 - LDO voltage levels can be changed when LDOs are disabled.
+ */
+int rohm_regulator_set_voltage_sel_restricted(struct regulator_dev *rdev,
+					      unsigned int sel)
+{
+	if (rdev->desc->ops->is_enabled(rdev))
+		return -EBUSY;
+
+	return regulator_set_voltage_sel_regmap(rdev, sel);
+}
+EXPORT_SYMBOL_GPL(rohm_regulator_set_voltage_sel_restricted);
+
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
 MODULE_DESCRIPTION("Generic helpers for ROHM PMIC regulator drivers");
diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
index 35b392a0d73a..35c5866f48b7 100644
--- a/include/linux/mfd/rohm-generic.h
+++ b/include/linux/mfd/rohm-generic.h
@@ -80,6 +80,8 @@ int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
 				  const struct regulator_desc *desc,
 				  struct regmap *regmap);
 
+int rohm_regulator_set_voltage_sel_restricted(struct regulator_dev *rdev,
+					      unsigned int sel);
 #else
 static inline int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dvs,
 						struct device_node *np,
@@ -88,6 +90,11 @@ static inline int rohm_regulator_set_dvs_levels(const struct rohm_dvs_config *dv
 {
 	return 0;
 }
+static int rohm_regulator_set_voltage_sel_restricted(struct regulator_dev *rdev,
+						     unsigned int sel)
+{
+	return 0;
+}
 #endif
 
 #endif
-- 
2.31.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 3/5] regulator: bd718x7: Use rohm generic restricted voltage setting

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3292 bytes --]

Use common restricted voltage setting instead of implementing own.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/regulator/bd718x7-regulator.c | 29 ++++-----------------------
 1 file changed, 4 insertions(+), 25 deletions(-)

diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index d60fccedb250..00efb18a836c 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -125,27 +125,6 @@ static int bd71837_get_buck34_enable_hwctrl(struct regulator_dev *rdev)
 
 	return !!(BD718XX_BUCK_RUN_ON & val);
 }
-/*
- * On BD71837 (not on BD71847, BD71850, ...)
- * Bucks 1 to 4 support DVS. PWM mode is used when voltage is changed.
- * Bucks 5 to 8 and LDOs can use PFM and must be disabled when voltage
- * is changed. Hence we return -EBUSY for these if voltage is changed
- * when BUCK/LDO is enabled.
- *
- * On BD71847, BD71850, ... The LDO voltage can be changed when LDO is
- * enabled. But if voltage is increased the LDO power-good monitoring
- * must be disabled for the duration of changing + 1mS to ensure voltage
- * has reached the higher level before HW does next under voltage detection
- * cycle.
- */
-static int bd71837_set_voltage_sel_restricted(struct regulator_dev *rdev,
-						    unsigned int sel)
-{
-	if (rdev->desc->ops->is_enabled(rdev))
-		return -EBUSY;
-
-	return regulator_set_voltage_sel_regmap(rdev, sel);
-}
 
 static void voltage_change_done(struct regulator_dev *rdev, unsigned int sel,
 				unsigned int *mask)
@@ -642,22 +621,22 @@ BD718XX_OPS(bd71837_pickable_range_buck_ops,
 	    bd718x7_set_buck_ovp);
 
 BD718XX_OPS(bd71837_ldo_regulator_ops, regulator_list_voltage_linear_range,
-	    NULL, bd71837_set_voltage_sel_restricted,
+	    NULL, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
 	    NULL);
 
 BD718XX_OPS(bd71837_ldo_regulator_nolinear_ops, regulator_list_voltage_table,
-	    NULL, bd71837_set_voltage_sel_restricted,
+	    NULL, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, NULL, NULL, bd718x7_set_ldo_uvp,
 	    NULL);
 
 BD718XX_OPS(bd71837_buck_regulator_ops, regulator_list_voltage_linear_range,
-	    NULL, bd71837_set_voltage_sel_restricted,
+	    NULL, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
 	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp);
 
 BD718XX_OPS(bd71837_buck_regulator_nolinear_ops, regulator_list_voltage_table,
-	    regulator_map_voltage_ascend, bd71837_set_voltage_sel_restricted,
+	    regulator_map_voltage_ascend, rohm_regulator_set_voltage_sel_restricted,
 	    regulator_get_voltage_sel_regmap, regulator_set_voltage_time_sel,
 	    NULL, bd718x7_set_buck_uvp, bd718x7_set_buck_ovp);
 /*
-- 
2.31.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 4/5] regulator: Add units to limit documentation

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

The documentation for limits used at protection level setting
did not mention the units. Fix the units in documentation to
match values passed in from device-tree (uV, uA, Kelvin) to
avoid confusion.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 include/linux/regulator/driver.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index f9d1115627e5..cf87f435d7ca 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -101,11 +101,13 @@ enum regulator_detection_severity {
  *		is requested.
  * @set_over_voltage_protection: Support enabling of and setting limits for over
  *	voltage situation detection. Detection can be configured for same
- *	severities as over current protection.
+ *	severities as over current protection. Units of uV.
  * @set_under_voltage_protection: Support enabling of and setting limits for
- *	under situation detection.
+ *	under voltage situation detection. Detection can be configured for same
+ *	severities as over current protection. Units of uV.
  * @set_thermal_protection: Support enabling of and setting limits for over
- *	temperature situation detection.
+ *	temperature situation detection.Detection can be configured for same
+ *	severities as over current protection. Units of degree Kelvin.
  *
  * @set_active_discharge: Set active discharge enable/disable of regulators.
  *
-- 
2.31.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 5/5] regulator: Update protection IRQ helper docs

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2906 bytes --]

The documentation of IRQ notification helper had still references to
first RFC implementation which called BUG() while trying to protect the
hardware. Behaviour was improved as calling the BUG() was not a proper
solution. Current implementation attempts to call poweroff if handling
of potentially damaging error notification fails. Update the
documentation to reflect the actual behaviour.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 include/linux/regulator/driver.h | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index cf87f435d7ca..ddbca4971f26 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -501,7 +501,8 @@ struct regulator_irq_data {
  *		best to shut-down regulator(s) or reboot the SOC if error
  *		handling is repeatedly failing. If fatal_cnt is given the IRQ
  *		handling is aborted if it fails for fatal_cnt times and die()
- *		callback (if populated) or BUG() is called to try to prevent
+ *		callback (if populated) is called. If die() is not populated
+ *		poweroff for the system is attempted in order to prevent any
  *		further damage.
  * @reread_ms:	The time which is waited before attempting to re-read status
  *		at the worker if IC reading fails. Immediate re-read is done
@@ -518,11 +519,12 @@ struct regulator_irq_data {
  * @data:	Driver private data pointer which will be passed as such to
  *		the renable, map_event and die callbacks in regulator_irq_data.
  * @die:	Protection callback. If IC status reading or recovery actions
- *		fail fatal_cnt times this callback or BUG() is called. This
- *		callback should implement a final protection attempt like
- *		disabling the regulator. If protection succeeded this may
- *		return 0. If anything else is returned the core assumes final
- *		protection failed and calls BUG() as a last resort.
+ *		fail fatal_cnt times this callback is called or system is
+ *		powered off. This callback should implement a final protection
+ *		attempt like disabling the regulator. If protection succeeded
+ *		die() may return 0. If anything else is returned the core
+ *		assumes final protection failed and attempts to perform a
+ *		poweroff as a last resort.
  * @map_event:	Driver callback to map IRQ status into regulator devices with
  *		events / errors. NOTE: callback MUST initialize both the
  *		errors and notifs for all rdevs which it signals having
-- 
2.31.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] regulator: irq_helpers: Allow omitting map_event for simple IRQs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 964 bytes --]

On Thu, Nov 18, 2021 at 01:48:26PM +0200, Matti Vaittinen wrote:

> +static bool single_bit_set(int val, int bits_to_check)
> +{
> +	int bit;
> +	const unsigned long bits = val;
> +
> +	bit = find_first_bit(&bits, bits_to_check);
> +	if (bit == bits_to_check)
> +		return false;
> +
> +	bit = find_next_bit(&bits, bits_to_check, bit + 1);
> +
> +	return (bit == bits_to_check);
> +}

The namespacing here feels like it should be with the other _bit()
helpers rather than private to the regulator code, I can certainly see
other things wanting to use it.

> +	if (!h->desc.map_event) {
> +		if (rdev_amount != 1 ||
> +		    !single_bit_set(common_errs, sizeof(common_errs) * 8) ||
> +		    per_rdev_errs)
> +			return ERR_PTR(-EINVAL);
> +
> +		h->desc.map_event = map_event_simple;
> +	}

This isn't the usual pattern, normally we would have the driver assign
the helper operation.  We can always still do the check based on finding
the expected map_event set up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] regulator: irq_helpers: Allow omitting map_event for simple IRQs

[Vaittinen, Matti]

Hi Mark,

Thanks for the review :)

On 11/18/21 15:35, Mark Brown wrote:
> On Thu, Nov 18, 2021 at 01:48:26PM +0200, Matti Vaittinen wrote:
> 
>> +static bool single_bit_set(int val, int bits_to_check)
>> +{
>> +	int bit;
>> +	const unsigned long bits = val;
>> +
>> +	bit = find_first_bit(&bits, bits_to_check);
>> +	if (bit == bits_to_check)
>> +		return false;
>> +
>> +	bit = find_next_bit(&bits, bits_to_check, bit + 1);
>> +
>> +	return (bit == bits_to_check);
>> +}
> 
> The namespacing here feels like it should be with the other _bit()
> helpers rather than private to the regulator code, I can certainly see
> other things wanting to use it.

Hmm. Right, thanks. I can put that in the same place where other bit() 
operations are - let's see what the reception is :)

> 
>> +	if (!h->desc.map_event) {
>> +		if (rdev_amount != 1 ||
>> +		    !single_bit_set(common_errs, sizeof(common_errs) * 8) ||
>> +		    per_rdev_errs)
>> +			return ERR_PTR(-EINVAL);
>> +
>> +		h->desc.map_event = map_event_simple;
>> +	}
> 
> This isn't the usual pattern, normally we would have the driver assign
> the helper operation.  We can always still do the check based on finding
> the expected map_event set up.

So, do you suggest that we export the map_event_simple() as a helper 
which drivers can provide to irq_helpers? If yes, do you think we should 
leave out the sanity check regarding the conditions (only one common 
error and only one rdev)? Or should we compare the given map function to 
the adress of the map_event_simple() and perform checks if it matches? 
It looks a bit strange to me. Or did you have some other vision?

Best Regards
     Matti Vaittinen


-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

Re: [PATCH 1/5] regulator: irq_helpers: Allow omitting map_event for simple IRQs

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On Thu, Nov 18, 2021 at 03:14:02PM +0000, Vaittinen, Matti wrote:
> On 11/18/21 15:35, Mark Brown wrote:
> > On Thu, Nov 18, 2021 at 01:48:26PM +0200, Matti Vaittinen wrote:

> > This isn't the usual pattern, normally we would have the driver assign
> > the helper operation.  We can always still do the check based on finding
> > the expected map_event set up.

> So, do you suggest that we export the map_event_simple() as a helper 
> which drivers can provide to irq_helpers? If yes, do you think we should 

Yes.

> leave out the sanity check regarding the conditions (only one common 
> error and only one rdev)? Or should we compare the given map function to 
> the adress of the map_event_simple() and perform checks if it matches? 
> It looks a bit strange to me. Or did you have some other vision?

I don't really mind either way on the checks, they might help someone
but on the other hand having them based on a check that a particular
helper is used is a bit odd like you say so I wouldn't mind if they
went.  I don't really have any other idea for doing them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] regulator: irq_helpers: Allow omitting map_event for simple IRQs

[Vaittinen, Matti]

On 11/18/21 17:20, Mark Brown wrote:
> On Thu, Nov 18, 2021 at 03:14:02PM +0000, Vaittinen, Matti wrote:
>> On 11/18/21 15:35, Mark Brown wrote:
>>> On Thu, Nov 18, 2021 at 01:48:26PM +0200, Matti Vaittinen wrote: 
>> leave out the sanity check regarding the conditions (only one common
>> error and only one rdev)? Or should we compare the given map function to
>> the adress of the map_event_simple() and perform checks if it matches?
>> It looks a bit strange to me. Or did you have some other vision?
> 
> I don't really mind either way on the checks, they might help someone
> but on the other hand having them based on a check that a particular
> helper is used is a bit odd like you say so I wouldn't mind if they
> went.  I don't really have any other idea for doing them.

That's fine. I'll drop the checks. If the caller uses the 
map_event_simple() - then the caller should have checked it suits his 
needs. Besides, these restrictions/expectations can be documented at 
kerneldoc when map_event_simple() gets exported. I think that is sufficient.

I'll change these for next version.

Best Regards
     Matti

-- 
The Linux Kernel guy at ROHM Semiconductors

Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~ this year is the year of a signature writers block ~~

Re: (subset) [PATCH 0/5] Few miscellaneous regulator improvements

[Mark Brown]

On Thu, 18 Nov 2021 13:47:59 +0200, Matti Vaittinen wrote:
> Minor regulator improvemnts / fixes.
> 
> This is a collection of minor improvemnts done while developing drivers
> for yet another ROHM PMIC. Please note that the new generic function
> added in Patch 2 does currently have only one in-tree driver using it.
> (call added in patch 3). I intended to post this with the set of patches
> bringing support to a new PMIC - but it appears this new PMIC requires
> one more HW-iteration - meaning it will be delayed probably by months.
> It'd be easier for me to add this upstream now in order to avoid
> rebasing/conflicts with other changes introduced meanwhile. Please let
> me know if you don't think that's Ok.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-linus

Thanks!

[2/5] regulator: rohm-regulator: add helper for restricted voltage setting
      (no commit info)
[3/5] regulator: bd718x7: Use rohm generic restricted voltage setting
      (no commit info)
[4/5] regulator: Add units to limit documentation
      (no commit info)
[5/5] regulator: Update protection IRQ helper docs
      commit: 6966df483d7b5b218aeb0e13e7e334a8fc3c1744

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark