Re: [PATCH v4 2/2] Input: bcm-keypad: Add Broadcom keypad controller

[Dmitry Torokhov]

Hi Scott,

On Mon, Mar 02, 2015 at 11:37:57AM -0800, Scott Branden wrote:
> Hi Dmitry,
> 
> I don't see the need for a check of the clock in bcm_kp_start or
> stop. See comment below.
> 
> On 15-02-28 02:15 PM, Dmitry Torokhov wrote:
> >On Sat, Feb 28, 2015 at 02:10:22PM -0800, Dmitry Torokhov wrote:
> >>Hi Scott,
> >>
> >>On Sat, Feb 28, 2015 at 08:35:57AM -0800, Scott Branden wrote:
> >>>+	/* Enable clock */
> >>>+
> >>>+	kp->clk = devm_clk_get(&pdev->dev, "peri_clk");
> >>>+	if (IS_ERR(kp->clk)) {
> >>>+		dev_info(&pdev->dev,
> >>>+			"No clock specified. Assuming it's enabled\n");
> >>
> >>I was doing the final pass before applying and I think that this is not
> >>quite correct, as it does not handle -EPROBE_DEFER properly which is
> >>quite common. I think we should do something like this:
> >>
> >>		error = PTR_ERR(kp->clk);
> >>		if (error != -ENOENT) {
> >>			if (error != -EPROBE_DEFER)
> >>				dev_err(.. "Failed to get clock" ...);
> >>			return error;
> >>		}
> >>		dev_dbg(... "No clock specified" ...);
> >>
> >>No need to resubmit if you are OK with this, I can make the change
> >>locally.
> >
> >Hmm, also bcm_kp_start() and bcm_kp_stop() should check if kp->clk is
> >valid before trying to enable/disable it.
> >
> I checked and other keyboard drivers do not check this.  I return an
> error in bcm_kp_start if the clk enable fails.  On stop, if the clk
> is not valid something is really, really wrong as well.

The other drivers simply abort probe() if they can't get clock, you
decided to allow probe() to finish and assume that clock is already
enabled, leaving kp->clk == ERR_PTR(-ENOENT) in your version. If you try
then calling clk_prepare_enable() with that pointer it is going to bomb,
that is why I said you need to check pointer validity in bcm_kp_start()
and bcm_kp_stop().

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/