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
