Re: [PATCH v3 4/7] pwm: Add support for Azoteq IQS620A PWM generator

[Jeff LaBundy <jeff@labundy.com>]

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:
> > Thank you for your kind words and thorough review.
> 
> Great my feedback is welcome.
> 
> > On Tue, Jan 14, 2020 at 09:08:28AM +0100, Uwe Kleine-König wrote:
> > > 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.

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