RE: [PATCH v4 1/3] watchdog: add rza_wdt driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thursday, March 02, 2017, Guenter Roeck worte:
> > > The above two lines are unnecessary.
> >
> > OK.
> >
> > #I'll assume you mean take out just the last sentence (2 lines), not
> > both sentences (all 3 lines).
> >
> The two empty lines.

Ooops! That makes more sense.


> > > > +	rate = clk_get_rate(priv->clk);
> > > > +	if (!rate)
> > > > +		return -ENOENT;
> > > > +
> > > > +	/* Assume slowest clock rate possible (CKS=7) */
> > > > +	rate /= 16384;
> > > > +
> > >
> > > The rate check should probably be here to avoid situations where
> > > rate < 16384.
> >
> > Do I need that if it's technically not possible to have a 'rate' less
> than 25MHz?
> >
> > These watchdogs HW are always feed directly from the peripheral clock
> > and there is no such thing as a 16kHz peripheral block an any Renesas
> SoC.
> >
> Following that line of argument, can clk_get_rate() ever return 0 ?

In the DT binding, it says that a clock source is required to be present.

If the user leaves out the "clocks =", then devm_clk_get will fail.

If the user puts in some crazy value for "clocks = ", then maybe you could get
0 (assuming there is a valid clock node they made by themselves somewhere that
runs at 0Hz).
But in that extreme case, I think they deserve to have it crash and burn because
who knows what they are doing.


> > > > +	priv->wdev.max_hw_heartbeat_ms = (1000 * U8_MAX)/rate;
> > >
> > > space before and after /
> >
> > OK.
> > #Funny because checkpatch.pl said it didn't like a space on one side
> > but  not the other, so I choose no spaces and it was happy. I'm way
> > below 80  characters for that line so it doesn't matter to me.
> >
> 
> That would be a bug in checkpatch. coding style, chapter 3.1, still
> applies.
> Or at least I hope so.

OK. Thank you for the clarification.


> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> 
> Also just
> 	return ret;

OK.


> > > > +static int rza_wdt_remove(struct platform_device *pdev) {
> > > > +	struct rza_wdt *priv = platform_get_drvdata(pdev);
> > > > +
> > > > +	watchdog_unregister_device(&priv->wdev);
> > > > +	iounmap(priv->base);
> > >
> > > iounmap is unnecessary (and wrong).
> >
> > Anything mapped with devm_ioremap_resource() automatically gets
> > unmapped when the drive gets unloaded?
> 
> That is the point of devm_ functions. It also means that you won't need a
> remove function if you also use devm_watchdog_register_device().

OK.
I see that only 1 driver is using devm_watchdog_register_device (wdat_wdt.c), so
maybe that is a new method.


Thank you,
Chris

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux