Message-ID: <YrYd+FkiFPz84twJ@mail.local>
Date:   Fri, 24 Jun 2022 22:26:32 +0200
From:   Alexandre Belloni <alexandre.belloni@...tlin.com>
To:     medadyoung@...il.com
Cc:     benjaminfair@...gle.com, yuenn@...gle.com, venture@...gle.com,
        tali.perry1@...il.com, tmaimon77@...il.com, avifishman70@...il.com,
        robh+dt@...nel.org, a.zummo@...ertech.it, KWLIU@...oton.com,
        YSCHU@...oton.com, JJLIU0@...oton.com, KFTING@...oton.com,
        ctcchien@...oton.com, openbmc@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-rtc@...r.kernel.org
Subject: Re: [PATCH v3 3/3] RTC: nuvoton: Add NCT3018Y real time clock driver

Hello,

Please run ./scripts/checkpatch.pl --strict on your patch, there are a
bunch of issues.

On 27/05/2022 16:46:47+0800, medadyoung@...il.com wrote:
> +static int nct3018y_set_alarm_mode(struct i2c_client *client, bool on)
> +{
> +	int err, flags;
> +
> +	dev_dbg(&client->dev, "%s:on:%d\n", __func__, on);
> +
> +	flags =  i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
> +	if (flags < 0) {
> +		dev_err(&client->dev,
> +			"Failed to read NCT3018Y_REG_CTRL\n");

You should cut down on the number of error messages, they are usually
not useful as the user doesn't have any specific action after getting
one of them apart from trying the action once again. Also, this will
make your code shorter. dev_dbg is fine.

> +/*
> + * In the routines that deal directly with the nct3018y hardware, we use
> + * rtc_time -- month 0-11, hour 0-23, yr = calendar year-epoch.
> + */
> +static int nct3018y_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned char buf[10];
> +	int err;
> +

You should still return an error if the time is invalid there but without
an error message.

> +	err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SC, sizeof(buf), buf);
> +	if (err < 0)
> +		return err;
> +
> +	tm->tm_sec = bcd2bin(buf[0] & 0x7F);
> +	tm->tm_min = bcd2bin(buf[2] & 0x7F);
> +	tm->tm_hour = bcd2bin(buf[4] & 0x3F);
> +	tm->tm_wday = buf[6] & 0x07;
> +	tm->tm_mday = bcd2bin(buf[7] & 0x3F);
> +	tm->tm_mon = bcd2bin(buf[8] & 0x1F) - 1;
> +	tm->tm_year = bcd2bin(buf[9]) + 100;
> +
> +	return 0;
> +}
> +
> +static int nct3018y_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned char buf[5];
> +	int err;
> +
> +	err = i2c_smbus_read_i2c_block_data(client, NCT3018Y_REG_SCA,
> +					    sizeof(buf), buf);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to read date\n");
> +		return -EIO;
> +	}
> +
> +	dev_dbg(&client->dev, "%s: raw data is sec=%02x, min=%02x hr=%02x\n",
> +		__func__, buf[0], buf[2], buf[4]);
> +
> +	tm->time.tm_sec = bcd2bin(buf[0] & 0x7F);
> +	tm->time.tm_min = bcd2bin(buf[2] & 0x7F);
> +	tm->time.tm_hour = bcd2bin(buf[4] & 0x3F);
> +
> +	err = nct3018y_get_alarm_mode(client, &tm->enabled, &tm->pending);
> +	if (err < 0)
> +		return err;
> +
> +	dev_dbg(&client->dev, "%s:s=%d m=%d, hr=%d, enabled=%d, pending=%d\n",
> +		__func__, tm->time.tm_sec, tm->time.tm_min,
> +		tm->time.tm_hour, tm->enabled, tm->pending);
> +
> +	return 0;
> +}
> +
> +static int nct3018y_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *tm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	unsigned char buf[3];
> +	int err;
> +
> +	dev_dbg(dev, "%s, sec=%d, min=%d hour=%d tm->enabled:%d\n",
> +		__func__, tm->time.tm_sec, tm->time.tm_min, tm->time.tm_hour,
> +		tm->enabled);
> +
> +	buf[0] = bin2bcd(tm->time.tm_sec);
> +	buf[1] = bin2bcd(tm->time.tm_min);
> +	buf[2] = bin2bcd(tm->time.tm_hour);

I don't think buf is useful in this function
> +
> +	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_SCA, buf[0]);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write NCT3018Y_REG_SCA\n");
> +		return err;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_MNA, buf[1]);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write NCT3018Y_REG_MNA\n");
> +		return err;
> +	}
> +
> +	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_HRA, buf[2]);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write NCT3018Y_REG_HRA\n");
> +		return err;
> +	}
> +
> +	return nct3018y_set_alarm_mode(client, tm->enabled);
> +}
> +


> +static struct clk *nct3018y_clkout_register_clk(struct nct3018y *nct3018y)
> +{
> +	struct i2c_client *client = nct3018y->client;
> +	struct device_node *node = client->dev.of_node;
> +	struct clk *clk;
> +	struct clk_init_data init;
> +	int flags, err;
> +
> +	/* disable the clkout output */
> +	flags = 0;
> +	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CLKO, flags);

BTW, this introduces a glitch in the clock output if the clock is
actually used. Maybe you could just rely on the CCF core to disable this
clock when there are no users.

> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write oscillator status register\n");
> +		return ERR_PTR(err);
> +	}
> +
> +	init.name = "nct3018y-clkout";
> +	init.ops = &nct3018y_clkout_ops;
> +	init.flags = 0;
> +	init.parent_names = NULL;
> +	init.num_parents = 0;
> +	nct3018y->clkout_hw.init = &init;
> +
> +	/* optional override of the clockname */
> +	of_property_read_string(node, "clock-output-names", &init.name);
> +
> +	/* register the clock */
> +	clk = devm_clk_register(&client->dev, &nct3018y->clkout_hw);
> +
> +	if (!IS_ERR(clk))
> +		of_clk_add_provider(node, of_clk_src_simple_get, clk);
> +
> +	return clk;
> +}
> +#endif
> +
> +static const struct rtc_class_ops nct3018y_rtc_ops = {
> +	.read_time	= nct3018y_rtc_read_time,
> +	.set_time	= nct3018y_rtc_set_time,
> +	.read_alarm	= nct3018y_rtc_read_alarm,
> +	.set_alarm	= nct3018y_rtc_set_alarm,
> +	.alarm_irq_enable = nct3018y_irq_enable,
> +	.ioctl		= nct3018y_ioctl,
> +};
> +
> +static int nct3018y_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct nct3018y *nct3018y;
> +	int err, flags;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {

Don't you rather need I2C_FUNC_SMBUS_BYTE and I2C_FUNC_SMBUS_BLOCK_DATA?

> +		dev_err(&client->dev, "%s: ENODEV\n", __func__);
> +		return -ENODEV;
> +	}
> +
> +	nct3018y = devm_kzalloc(&client->dev, sizeof(struct nct3018y),
> +				GFP_KERNEL);
> +	if (!nct3018y)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, nct3018y);
> +	nct3018y->client = client;
> +	device_set_wakeup_capable(&client->dev, 1);
> +
> +	flags = i2c_smbus_read_byte_data(client, NCT3018Y_REG_CTRL);
> +	if (flags < 0) {
> +		dev_err(&client->dev, "%s: read error\n", __func__);
> +		return flags;
> +	} else if (flags & NCT3018Y_BIT_TWO)
> +		dev_dbg(&client->dev, "%s: NCT3018Y_BIT_TWO is set\n", __func__);
> +
> +
> +	flags = NCT3018Y_BIT_TWO;
> +	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_CTRL, flags);
> +	if (err < 0) {
> +		dev_err(&client->dev, "Unable to write NCT3018Y_REG_CTRL\n");
> +		return err;
> +	}
> +
> +	flags = 0;
> +	err = i2c_smbus_write_byte_data(client, NCT3018Y_REG_ST, flags);
> +	if (err < 0) {
> +		dev_err(&client->dev, "%s: write error\n", __func__);
> +		return err;
> +	}
> +
> +
> +	nct3018y->rtc = devm_rtc_allocate_device(&client->dev);
> +	if (IS_ERR(nct3018y->rtc))
> +		return PTR_ERR(nct3018y->rtc);
> +
> +	nct3018y->rtc->ops = &nct3018y_rtc_ops;
> +	nct3018y->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	nct3018y->rtc->range_max = RTC_TIMESTAMP_END_2099;
> +
> +	if (client->irq > 0) {
> +		err = devm_request_threaded_irq(&client->dev, client->irq,
> +				NULL, nct3018y_irq,
> +				IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +				"nct3018y", client);
> +		if (err) {
> +			dev_err(&client->dev, "unable to request IRQ %d\n", client->irq);
> +			return err;
> +		}
> +	}

You need to clear RTC_FEATURE_UPDATE_INTERRUPT and RTC_FEATURE_ALARM
from nct3018y->rtc->features when client->irq <= 0

> +
> +	return devm_rtc_register_device(nct3018y->rtc);
> +

> +#ifdef CONFIG_COMMON_CLK
> +	/* register clk in common clk framework */
> +	nct3018y_clkout_register_clk(nct3018y);
> +#endif
> +

This code is not reachable anymore

> +	return 0;
> +}
> +
> +static const struct i2c_device_id nct3018y_id[] = {
> +	{ "nct3018y", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, nct3018y_id);
> +
> +
> +static const struct of_device_id nct3018y_of_match[] = {
> +	{ .compatible = "nuvoton,nct3018y" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, nct3018y_of_match);
> +
> +static struct i2c_driver nct3018y_driver = {
> +	.driver		= {
> +		.name	= "rtc-nct3018y",
> +		.of_match_table = of_match_ptr(nct3018y_of_match),
> +	},
> +	.probe		= nct3018y_probe,
> +	.id_table	= nct3018y_id,
> +};
> +
> +module_i2c_driver(nct3018y_driver);
> +
> +MODULE_AUTHOR("Medad CChien <ctcchien@...oton.com>");
> +MODULE_DESCRIPTION("Nuvoton NCT3018Y RTC driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists