Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1585817

Re: [PATCHv2] rtc: cpcap: new rtc driver

Show all headers | View raw

Hi,

The patch has a few checkpatch issues. Some of those should really be
fixed. Can you do that?

Else, it is mostly fine, a few comments below.

On 21/02/2017 at 07:16:50 +0100, Sebastian Reichel wrote:
> +static int cpcap_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct cpcap_rtc *rtc;
> +	struct cpcap_time cpcap_tm;
> +	int ret = 0;
> +
> +	rtc = dev_get_drvdata(dev);
> +
> +	rtc2cpcap_time(&cpcap_tm, tm);
> +
> +	if (rtc->alarm_enabled)
> +		disable_irq(rtc->alarm_irq);
> +	if (rtc->update_enabled)
> +		disable_irq(rtc->update_irq);
> +
> +	if (rtc->vendor == CPCAP_VENDOR_ST) {
> +		/* The TOD1 and TOD2 registers MUST be written in this order
> +		 * for the change to properly set. */

Does this mean there is a race condition?

> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> +					  TOD1_MASK, cpcap_tm.tod1);
> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2,
> +					  TOD2_MASK, cpcap_tm.tod2);
> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY,
> +					  DAY_MASK, cpcap_tm.day);
> +	} else {
> +		/* Clearing the upper lower 8 bits of the TOD guarantees that
> +		 * the upper half of TOD (TOD2) will not increment for 0xFF RTC
> +		 * ticks (255 seconds).  During this time we can safely write
> +		 * to DAY, TOD2, then TOD1 (in that order) and expect RTC to be
> +		 * synchronized to the exact time requested upon the final write
> +		 * to TOD1. */
> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> +					  TOD1_MASK, 0);
> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_DAY,
> +					  DAY_MASK, cpcap_tm.day);
> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD2,
> +					  TOD2_MASK, cpcap_tm.tod2);
> +		ret |= regmap_update_bits(rtc->regmap, CPCAP_REG_TOD1,
> +					  TOD1_MASK, cpcap_tm.tod1);
> +	}
> +

> +	err = cpcap_get_vendor(dev, rtc->regmap, &rtc->vendor);
I think this means it depends on the mfd tree.

> +	if (err)
> +		return err;
> +
> +	rtc->alarm_irq= platform_get_irq(pdev, 0);
> +	err = devm_request_threaded_irq(dev, rtc->alarm_irq, NULL,
> +					cpcap_rtc_alarm_irq, IRQ_NONE,
> +					"rtc_alarm", rtc);
> +	if (err) {
> +		dev_err(dev, "Could not request alarm irq: %d\n", err);
> +		return err;
> +	}
> +	disable_irq(rtc->alarm_irq);
> +
> +	rtc->update_irq= platform_get_irq(pdev, 1);
> +	err = devm_request_threaded_irq(dev, rtc->update_irq, NULL,
> +					cpcap_rtc_update_irq, IRQ_NONE,
> +					"rtc_1hz", rtc);
I don't think this IRQ is actually useful. It doesn't really harm but
the tests should pass without it.

> +	if (err) {
> +		dev_err(dev, "Could not request update irq: %d\n", err);
> +		return err;
> +	}
> +	disable_irq(rtc->update_irq);
> +
> +	err = device_init_wakeup(dev, 1);

If you use device_init_wakeup, I think it needs to be called before
devm_rtc_device_register() to properly work.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH 0/1] Motorola CPCAP PMIC RTC Sebastian Reichel <sre@kernel.org> - 2017-02-20 08:40 +0100
  [PATCH 1/1] rtc: cpcap: new rtc driver Sebastian Reichel <sre@kernel.org> - 2017-02-20 08:40 +0100
    Re: [PATCH 1/1] rtc: cpcap: new rtc driver Tony Lindgren <tony@atomide.com> - 2017-02-20 17:40 +0100
      Re: [PATCH 1/1] rtc: cpcap: new rtc driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-20 17:40 +0100
        Re: [PATCH 1/1] rtc: cpcap: new rtc driver Tony Lindgren <tony@atomide.com> - 2017-02-20 18:30 +0100
          Re: [PATCH 1/1] rtc: cpcap: new rtc driver Sebastian Reichel <sre@kernel.org> - 2017-02-20 20:40 +0100
        Re: [PATCH 1/1] rtc: cpcap: new rtc driver Tony Lindgren <tony@atomide.com> - 2017-02-20 18:30 +0100
  [PATCHv2] rtc: cpcap: new rtc driver Sebastian Reichel <sre@kernel.org> - 2017-02-21 07:20 +0100
    Re: [PATCHv2] rtc: cpcap: new rtc driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-22 01:00 +0100
      Re: [PATCHv2] rtc: cpcap: new rtc driver Sebastian Reichel <sre@kernel.org> - 2017-02-22 03:00 +0100
        Re: [PATCHv2] rtc: cpcap: new rtc driver Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2017-02-22 09:20 +0100
  [PATCHv3 2/2] rtc: cpcap: new rtc driver Sebastian Reichel <sre@kernel.org> - 2017-02-23 02:30 +0100
    Re: [PATCHv3 2/2] rtc: cpcap: new rtc driver Rob Herring <robh@kernel.org> - 2017-02-28 01:00 +0100
  [PATCHv3 1/2] dt-bindings: Add vendor prefix for Motorola Sebastian Reichel <sre@kernel.org> - 2017-02-23 02:30 +0100
    Re: [PATCHv3 1/2] dt-bindings: Add vendor prefix for Motorola Rob Herring <robh@kernel.org> - 2017-02-28 01:10 +0100

csiph-web