Hi Uwe,

On Wed, Jan 15, 2020 at 08:33:36AM +0100, Uwe Kleine-König wrote:

Hello Jeff,

On Wed, Jan 15, 2020 at 03:29:52AM +0000, Jeff LaBundy wrote:

quoted

Thank you for your kind words and thorough review.

Great my feedback is welcome.

quoted

On Tue, Jan 14, 2020 at 09:08:28AM +0100, Uwe Kleine-König wrote:

quoted

On Mon, Jan 06, 2020 at 12:48:02AM +0000, Jeff LaBundy wrote:
I thought we dicussed having a comment here, saying something like:

	The device might reset when [...] and as a result looses it's
	configuration. So the registers must be rewritten when this
	happens to restore the expected operation.

Is it worth to issue a warning when this happens?

The detailed comments and an error message have always been in iqs62x_irq of the
parent MFD driver. The pwm is only one of up to three sub-devices that subscribe
to the chain and must update their locally owned registers after the MFD handles
the interrupt and restores the device's firmware. I opted to keep these comments
in the common MFD rather than repeating throughout the series.

That's fine then, a comment that the parent driver issues a message
would be great then.

Sure thing, will do.

quoted

quoted

quoted

+static int iqs620_pwm_notifier(struct notifier_block *notifier,
+			       unsigned long event_flags, void *context)
+{
+	struct iqs620_pwm_private *iqs620_pwm;
+	int ret;
+
+	iqs620_pwm = container_of(notifier, struct iqs620_pwm_private,
+				  notifier);
+
+	if (!completion_done(&iqs620_pwm->chip_ready) ||
+	    !(event_flags & BIT(IQS62X_EVENT_SYS_RESET)))
+		return NOTIFY_DONE;

Is here a (relevant?) race?  Consider the notifier triggers just when
pwmchip_add returned, (maybe even a consumer configured the device) and
before complete_all() is called. With my limited knowledge about
notifiers I'd say waiting for the completion here might be reasonable
and safe.

Great catch; this is theoretically possible. The problem with waiting, however,
is if the notifier is triggered right away during probe but probe returns early
due to an error (and completion never happens).

OK, the error path would need to complete .chip_ready then and the
notifier then check for this error. Indeed messy.
 

quoted

At this point, I think the best option is to simply cache the values written to
IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE and restore them from the
notifier, which is essentially what is done for the IIO drivers in this series.

Sounds good.
 

quoted

quoted

quoted

+	ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh,
+						 &iqs620_pwm->notifier);
+	if (ret)
+		dev_err(iqs620_pwm->chip.dev,
+			"Failed to unregister notifier: %d\n", ret);

	dev_err(iqs620_pwm->chip.dev,
		"Failed to unregister notifier: %pe\n", ERR_PTR(ret));

gives a nicer output. (Also applies to other error messages of course.)

I don't disagree, but this gives me some pause. If I made this change here, I'd
prefer to do so across the series for consistency. However, I am hesitant to do
so at this stage in the review since several patches are somewhat stable by now
(unless there was a compelling reason from a functional perspective).

Another reason is that there are many dev_err cases throughout this series, and
adopting this very recently introduced functionality would make the series even
harder to back port to the present lot of LTS kernels.

Unless this is a deal breaker, I'd like to pass on this for v4. However, please
let me know if you feel strongly about it. In the meantime, I'll get started on
the couple of other changes discussed here.

OK, being able to backport is a valid excuse. Consistency over the whole
series wouldn't be one of my reasons, your mileage obviously varies
(which is OK).

This can also be done later. Conversion to this is on my todo-list (not
at the top though), but if you beat me to it, I won't be angry :-)

Thank you for your understanding.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

I'll send out v4 in the next day or so after I finish some testing.

Kind regards,
Jeff LaBundy