Re: [PATCH v2 5/5] rtc: rtc-rc5t619: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

Hi,

On Thu, 28 Nov 2019 11:57:51 +0100
Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> wrote:

> Hello,
> 
> checkpatch.pl --strict complains about multiple blank lines and alignment.
> 
I have not used the strict flag there. But I think I can make
--strict happy.

> On 31/10/2019 22:38:35+0100, Andreas Kemnade wrote:
> > +static int rc5t619_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct rn5t618 *rn5t618 = dev_get_drvdata(pdev->dev.parent);
> > +	struct rc5t619_rtc *rtc;
> > +	uint8_t alarm_flag;
> > +	unsigned int ctrl2;
> > +	int err;
> > +
> > +	rtc = devm_kzalloc(dev, sizeof(*rtc), GFP_KERNEL);
> > +	if (IS_ERR(rtc)) {
> > +		err = PTR_ERR(rtc);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	rtc->rn5t618 = rn5t618;
> > +
> > +	dev_set_drvdata(dev, rtc);
> > +	rtc->irq = -1;
> > +
> > +	if (rn5t618->irq_data)
> > +		rtc->irq = regmap_irq_get_virq(rn5t618->irq_data,
> > +				RN5T618_IRQ_RTC);
> > +
> > +	if (rtc->irq  < 0) {
> > +		dev_err(dev, "no irq specified, wakeup is disabled\n");  
> 
> I don't think it is worth having an error message here, especially since
> you have a second one later.
> 
agreed.

> > +		rtc->irq = -1;
> > +	}
> > +
> > +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &ctrl2);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	/* get interrupt flag */
> > +	err = rc5t619_rtc_alarm_is_enabled(dev, &alarm_flag);
> > +	if (err)
> > +		return err;
> > +
> > +	/* disable rtc periodic function */
> > +	err = rc5t619_rtc_periodic_disable(&pdev->dev);
> > +	if (err)
> > +		return err;
> > +
> > +	/* disable interrupt */
> > +	err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> > +	if (err)
> > +		return err;  
> 
> Is it really useful to disable the alarm to reenable them later?
> 
Well, yes, seems to be nonsense.
Am I right that I do not need to prevent alarm irqs between
alloc() and register()?

> > +
> > +	if (ctrl2 & CTRL2_PON) {
> > +		alarm_flag = 0;
> > +		err = rc5t619_rtc_alarm_flag_clr(&pdev->dev);
> > +		if (err)
> > +			return err;
> > +	}
> > +
> > +	rtc->rtc = devm_rtc_allocate_device(&pdev->dev);
> > +  
> 
> Please remove this blank line.
> 
Ok.

> > +	if (IS_ERR(rtc->rtc)) {
> > +		err = PTR_ERR(rtc->rtc);
> > +		dev_err(dev, "RTC device register: err %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	rtc->rtc->ops = &rc5t619_rtc_ops;
> > +	rtc->rtc->range_min = RTC_TIMESTAMP_BEGIN_1900;
> > +	rtc->rtc->range_max = RTC_TIMESTAMP_END_2099;
> > +
> > +	/* set interrupt and enable it */
> > +	if (rtc->irq != -1) {
> > +		device_init_wakeup(&pdev->dev, 1);
> > +
> > +		err = devm_request_threaded_irq(&pdev->dev,
> > rtc->irq, NULL,
> > +						rc5t619_rtc_irq,
> > +						IRQF_ONESHOT,
> > +						"rtc-rc5t619",
> > +						&pdev->dev);
> > +		if (err < 0) {
> > +			dev_err(&pdev->dev, "request IRQ:%d
> > fail\n", rtc->irq);
> > +			rtc->irq = -1;
> > +
> > +			err = rc5t619_rtc_alarm_enable(&pdev->dev,
> > 0);
> > +			if (err)
> > +				return err;
> > +
> > +		} else {
> > +			/* enable wake */  
> 
> I think you should move device_init_wakeup() here, unless your parse
> the wakeup-source property.
> 
yes, makes sense.

> > +			enable_irq_wake(rtc->irq);
> > +			/* enable alarm_d */
> > +			err = rc5t619_rtc_alarm_enable(&pdev->dev,
> > alarm_flag);
> > +			if (err) {
> > +				dev_err(&pdev->dev, "failed rtc
> > setup\n");
> > +				return -EBUSY;
> > +			}
> > +		}
> > +	} else {
> > +		/* system don't want to using alarm interrupt, so
> > close it */
> > +		err = rc5t619_rtc_alarm_enable(&pdev->dev, 0);
> > +		if (err) {
> > +			dev_err(&pdev->dev, "disable rtc alarm
> > error\n");  
> 
> I don't think this message is necessary.
> 
yes, agreed, that would be just another symptom of an i2c problem.
> > +			return err;
> > +		}
> > +
> > +		dev_err(&pdev->dev, "ricoh61x interrupt is
> > disabled\n");  
> 
> Maybe dev_warn as the driver just continues on.
> 
Ok.

Regards,
Andreas

Attachment: pgpgnnR6xno0w.pgp
Description: OpenPGP digital signature