Re: [PATCH V3] drivers: rtc: Add support for Qualcomm PMIC8xxx RTC

[Stephen Boyd <sboyd@codeaurora.org>]

On 5/25/2011 10:15 PM, Anirudh Ghayal wrote:
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> new file mode 100644
> index 0000000..4694c4e
> --- /dev/null
> +++ b/drivers/rtc/rtc-pm8xxx.c
> +
> +/**
> + * struct pm8xxx_rtc	rtc driver internal structure

This is missing a dash '-'.

> +static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
> +		int base, int count)
> +{
> +	int i, rc;
> +	struct device *parent = rtc_dd->rtc_dev->parent;
> +
> +	for (i = 0; i < count; i++) {
> +		rc = pm8xxx_readb(parent, base + i, &rtc_val[i]);
> +		if (rc < 0) {
> +			dev_err(rtc_dd->rtc_dev, "PMIC read failed\n");

I still think these printks are useless when you already have better
printks at the call sites on the error path.

> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
> +		int base, int count)
> +{
> +	int i, rc;
> +	struct device *parent = rtc_dd->rtc_dev->parent;
> +
> +	for (i = 0; i < count; i++) {
> +		rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
> +		if (rc < 0) {
> +			dev_err(rtc_dd->rtc_dev, "PMIC write failed\n");
> +			return rc;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Steps to write the RTC registers.
> + * 1. Disable alarm if enabled.
> + * 2. Write 0x00 to LSB.
> + * 3. Write Byte[1], Byte[2], Byte[3] then Byte[0].
> + * 4. Enable alarm if disabled in step 1.
> + */
> +static int
> +pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	int rc, i;
> +	unsigned long secs, irq_flags;
> +	u8 value[NUM_8_BIT_RTC_REGS], reg = 0, alarm_enabled = 0, ctrl_reg;
> +	struct pm8xxx_rtc *rtc_dd = dev_get_drvdata(dev);
> +
> +	rtc_tm_to_time(tm, &secs);
> +
> +	for (i = 0; i < NUM_8_BIT_RTC_REGS; i++) {
> +		value[i] = secs & 0xFF;
> +		secs >>= 8;
> +	}
> +
> +	dev_dbg(dev, "Seconds value to be written to RTC = %lu\n", secs);
> +
> +	spin_lock_irqsave(&rtc_dd->ctrl_reg_lock, irq_flags);
> +	ctrl_reg = rtc_dd->ctrl_reg;
> +
> +	if (ctrl_reg & PM8xxx_RTC_ALARM_ENABLE) {
> +		alarm_enabled = 1;
> +		ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
> +		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
> +				1);
> +		if (rc < 0) {
> +			dev_err(dev, "Write to RTC control register "
> +								"failed\n");
> +			goto rtc_rw_fail;
> +		}
> +		rtc_dd->ctrl_reg = ctrl_reg;
> +	} else
> +		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
> +
> +	/* Write 0 to Byte[0] */
> +	reg = 0;
> +	rc = pm8xxx_write_wrapper(rtc_dd, &reg, rtc_dd->rtc_write_base, 1);
> +	if (rc < 0) {
> +		dev_err(dev, "Write to RTC write data register failed\n");
> +		goto rtc_rw_fail;
> +	}
> +
> +	/* Write Byte[1], Byte[2], Byte[3] */
> +	rc = pm8xxx_write_wrapper(rtc_dd, value + 1,
> +					rtc_dd->rtc_write_base + 1, 3);
> +	if (rc < 0) {
> +		dev_err(dev, "Write to RTC write data register failed\n");
> +		goto rtc_rw_fail;
> +	}
> +
> +	/* Write Byte[0] */
> +	rc = pm8xxx_write_wrapper(rtc_dd, value, rtc_dd->rtc_write_base, 1);
> +	if (rc < 0) {
> +		dev_err(dev, "Write to RTC write data register failed\n");
> +		goto rtc_rw_fail;
> +	}
> +
> +	if (alarm_enabled) {
> +		ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
> +		rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
> +									1);
> +		if (rc < 0) {
> +			dev_err(dev, "Write to RTC control register "
> +								"failed\n");
> +			goto rtc_rw_fail;
> +		}
> +		rtc_dd->ctrl_reg = ctrl_reg;
> +	}
> +
> +rtc_rw_fail:
> +	if (alarm_enabled)
> +		spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
> +

This could be simplified by putting the label inside the if and then
putting the spin_unlock at the end of the if. You would save a branch.
Not sure it really matters though since the compiler should be doing it
anyway.

Is it really that bad to take the lock all the time? It would simplify
this function a bit if you did.

If you fix the kernel-doc error you can add a

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.