Linux Input Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff LaBundy <jeff@labundy.com>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: "lee.jones@linaro.org" <lee.jones@linaro.org>,
	"dmitry.torokhov@gmail.com" <dmitry.torokhov@gmail.com>,
	"thierry.reding@gmail.com" <thierry.reding@gmail.com>,
	"jic23@kernel.org" <jic23@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-input@vger.kernel.org" <linux-input@vger.kernel.org>,
	"linux-pwm@vger.kernel.org" <linux-pwm@vger.kernel.org>,
	"knaack.h@gmx.de" <knaack.h@gmx.de>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"pmeerw@pmeerw.net" <pmeerw@pmeerw.net>,
	"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"mark.rutland@arm.com" <mark.rutland@arm.com>
Subject: Re: [PATCH v3 4/7] pwm: Add support for Azoteq IQS620A PWM generator
Date: Thu, 16 Jan 2020 03:34:04 +0000
Message-ID: <20200116033355.GA8974@labundy.com> (raw)
In-Reply-To: <20200115073336.2bhlu22toua3vnuo@pengutronix.de>

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

  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-06  0:47 [PATCH v3 0/7] Add support for Azoteq IQS620A/621/622/624/625 Jeff LaBundy
2020-01-06  0:48 ` [PATCH v3 1/7] dt-bindings: Add bindings " Jeff LaBundy
2020-01-06  0:48 ` [PATCH v3 2/7] mfd: Add support " Jeff LaBundy
2020-01-06  0:48 ` [PATCH v3 4/7] pwm: Add support for Azoteq IQS620A PWM generator Jeff LaBundy
2020-01-14  8:08   ` Uwe Kleine-König
2020-01-15  3:29     ` Jeff LaBundy
2020-01-15  7:33       ` Uwe Kleine-König
2020-01-16  3:34         ` Jeff LaBundy [this message]
2020-01-06  0:48 ` [PATCH v3 3/7] input: keyboard: Add support for Azoteq IQS620A/621/622/624/625 Jeff LaBundy
2020-01-06  0:48 ` [PATCH v3 5/7] iio: temperature: Add support for Azoteq IQS620AT temperature sensor Jeff LaBundy
2020-01-06  0:48 ` [PATCH v3 6/7] iio: light: Add support for Azoteq IQS621/622 ambient light sensors Jeff LaBundy
2020-01-06  0:48 ` [PATCH v3 7/7] iio: position: Add support for Azoteq IQS624/625 angle sensors Jeff LaBundy

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200116033355.GA8974@labundy.com \
    --to=jeff@labundy.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=jic23@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pmeerw@pmeerw.net \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git