Re: [PATCHv1 3/11] RTC: RTC module of DA9052 PMIC driver

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 6 Apr 2011 18:47:29 +0530
Ashish Jangam <Ashish.Jangam@kpitcummins.com> wrote:

> Hi Paul,
> 
> RTC Driver for Dialog Semiconductor DA9052 PMICs.
> 
> Changes made since last submission:
> . read and write operation moved to MFD
> 
> Linux Kernel Version: 2.6.37

The patch looks OK(ish) to me from a quick read.

> --- orig_linux-2.6.37/drivers/rtc/Kconfig	2011-01-05 05:50:19.000000000 +0500
> +++ linux-2.6.37/drivers/rtc/Kconfig	2011-03-31 21:07:39.000000000 +0500
> @@ -664,6 +664,13 @@
>  	help
>  	  If you say yes here you get support for the RTC subsystem of the
>  	  NUC910/NUC920 used in embedded systems.
> +
> +config RTC_DRV_DA9052
> +	tristate "Dialog DA9052 RTC"
> +	depends on PMIC_DA9052
> +	help
> +	  Say y here to support the RTC driver for
> +	  Dialog Semiconductor DA9052 PMIC.

But there's not much I can do with it because PMIC_DA9052 does not
exist in mainline or in linux-next.

What is a PMIC_DA9052, anyway?  What CPU architectures support it, etc?

Have you identified a maintainer who will be merging the main patch
which enables PMIC_DA9052?


Please feed all the patches through scritps/checkpatch.pl if you haven't
already done so, to clean up lots of trivial errors.

For example, "MFD: MFD module of DA9052 PMIC driver":

	total: 449 errors, 832 warnings, 2326 lines checked


A couple of minor comments:

> +static int da9052_rtc_enable_alarm(struct da9052 *da9052, unsigned char flag)
> +{
> +	int ret = 0;
> +	if (flag) {
> +		ret = da9052_set_bits(da9052, DA9052_ALARM_Y_REG,
> +					DA9052_ALARM_Y_ALARM_ON);
> +		if (ret != 0)
> +			dev_err(da9052->dev, "Failed to enable ALM: %d\n", ret);
> +	} else {
> +		ret = da9052_clear_bits(da9052, DA9052_ALARM_Y_REG,
> +					DA9052_ALARM_Y_ALARM_ON);
> +		if (ret != 0)
> +			dev_err(da9052->dev, "da9052_rtc_enable_alarm -> \
> +					da9052_clear_bits error %d\n", ret);
> +	}
> +	return ret;
> +}

"flag" is a poor identifier - it's largely meaningless.  Perhaps
"enable" would be a better choice in this case.  Making it have the
bool type wouild make sense also.

> +static irqreturn_t da9052_rtc_irq(int irq, void *data)
> +{
> +	struct da9052_rtc *rtc = (struct da9052_rtc *)data;

typecasting a void* like this is unneeded and is in fact undesirable,
as it will suppress possibly-useful warnings.


> +	int ret = 0;
> +
> +	ret = da9052_reg_read(rtc->da9052, DA9052_ALARM_MI_REG);
> +	if (ret < 0) {
> +		dev_err(rtc->da9052->dev, "da9052_rtc_notifier -> \
> +					da9052_reg_read error %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +	if (ret & DA9052_ALARMMI_ALARMTYPE)
> +		da9052_rtc_enable_alarm(rtc->da9052, 0);
> +
> +	return IRQ_HANDLED;
> +}