| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190130214828.GK19959@piout.net>
Date: Wed, 30 Jan 2019 22:48:28 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Artem Panfilov <panfilov.artyom@...il.com>
Cc: Alessandro Zummo <a.zummo@...ertech.it>,
linux-kernel@...r.kernel.org, linux-rtc@...r.kernel.org
Subject: Re: [PATCH v2 1/2] rtc: add AB-RTCMC-32.768kHz-EOZ9 RTC support
Hi,
This seems pretty good.
On 17/01/2019 21:46:07+0300, Artem Panfilov wrote:
> +static int abeoz9_rtc_get_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct abeoz9_rtc_data *data = dev_get_drvdata(dev);
> + u8 regs[ABEOZ9_SEC_LEN];
> + int ret;
> +
I would check the validity here, there is no point in reading many
registers just to drop the result afterwards.
> + ret = regmap_bulk_read(data->regmap, ABEOZ9_REG_SEC,
> + regs,
> + sizeof(regs));
> + if (ret) {
> + dev_err(dev, "reading RTC time failed (%d)\n", ret);
> + return ret;
> + }
> +
> + tm->tm_sec = bcd2bin(regs[ABEOZ9_REG_SEC - ABEOZ9_REG_SEC] & 0x7F);
> + tm->tm_min = bcd2bin(regs[ABEOZ9_REG_MIN - ABEOZ9_REG_SEC] & 0x7F);
> +
> + if (regs[ABEOZ9_REG_HOURS - ABEOZ9_REG_SEC] & ABEOZ9_HOURS_PM) {
> + tm->tm_hour =
> + bcd2bin(regs[ABEOZ9_REG_HOURS - ABEOZ9_REG_SEC] & 0x1f);
> + if (regs[ABEOZ9_REG_HOURS - ABEOZ9_REG_SEC] & ABEOZ9_HOURS_PM)
> + tm->tm_hour += 12;
> + } else {
> + tm->tm_hour = bcd2bin(regs[ABEOZ9_REG_HOURS - ABEOZ9_REG_SEC]);
> + }
> +
> + tm->tm_mday = bcd2bin(regs[ABEOZ9_REG_DAYS - ABEOZ9_REG_SEC]);
> + tm->tm_wday = bcd2bin(regs[ABEOZ9_REG_WEEKDAYS - ABEOZ9_REG_SEC]);
> + tm->tm_mon = bcd2bin(regs[ABEOZ9_REG_MONTHS - ABEOZ9_REG_SEC]) - 1;
> + tm->tm_year = bcd2bin(regs[ABEOZ9_REG_YEARS - ABEOZ9_REG_SEC]) + 100;
> +
> + return abeoz9_check_validity(dev);
> +}
> +
> +static int abeoz9_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> + struct abeoz9_rtc_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> + u8 regs[ABEOZ9_SEC_LEN];
> + int ret;
> +
> + regs[ABEOZ9_REG_SEC - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_sec);
> + regs[ABEOZ9_REG_MIN - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_min);
> + regs[ABEOZ9_REG_HOURS - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_hour);
> + regs[ABEOZ9_REG_DAYS - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_mday);
> + regs[ABEOZ9_REG_WEEKDAYS - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_wday);
> + regs[ABEOZ9_REG_MONTHS - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_mon + 1);
> + regs[ABEOZ9_REG_YEARS - ABEOZ9_REG_SEC] = bin2bcd(tm->tm_year - 100);
> +
> + ret = regmap_bulk_write(data->regmap, ABEOZ9_REG_SEC,
> + regs,
> + sizeof(regs));
> +
ret should be checked.
> + return abeoz9_reset_validity(regmap);
> +}
> +
> +#if IS_REACHABLE(CONFIG_HWMON)
> +
> +static int abeoz9z3_temp_read(struct device *dev,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel, long *temp)
> +{
> + struct abeoz9_rtc_data *data = dev_get_drvdata(dev);
> + struct regmap *regmap = data->regmap;
> + int ret;
> + unsigned int regval = 37;
> +
Maybe this should also check V1F/V2F because the thermometer may be
disabled.
> + switch (attr) {
> + case hwmon_temp_input:
> + ret = regmap_read(regmap, ABEOZ9_REG_REG_TEMP, ®val);
> + if (ret < 0)
> + return ret;
> + *temp = 1000 * (regval + ABEOZ953_TEMP_MIN);
> + return 0;
> + case hwmon_temp_max:
> + *temp = 1000 * ABEOZ953_TEMP_MAX;
> + return 0;
> + case hwmon_temp_min:
> + *temp = 1000 * ABEOZ953_TEMP_MIN;
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +#endif /* CONFIG_RTC_DRV_ABEOZ9_HWMON */
This comment is not correct
> +static int abeoz9_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct abeoz9_rtc_data *data = NULL;
> + struct device *dev = &client->dev;
> + struct regmap *regmap;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> + I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_I2C_BLOCK)) {
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + regmap = devm_regmap_init_i2c(client, &abeoz9_rtc_regmap_config);
> + if (IS_ERR(regmap)) {
> + ret = PTR_ERR(regmap);
> + dev_err(dev, "regmap allocation failed: %d\n", ret);
> + goto err;
> + }
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + data->regmap = regmap;
> + dev_set_drvdata(dev, data);
> +
> + ret = abeoz9_rtc_setup(dev, client->dev.of_node);
> + if (ret)
> + goto err;
> +
> + data->rtc = devm_rtc_allocate_device(dev);
> + ret = PTR_ERR_OR_ZERO(data->rtc);
> + if (ret)
> + goto err;
> +
> + data->rtc->ops = &rtc_ops;
> + data->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> + data->rtc->range_max = RTC_TIMESTAMP_END_2099;
> +
> + ret = rtc_register_device(data->rtc);
> + if (ret)
> + goto err;
> +
> + abeoz9_hwmon_register(dev, data);
> + return 0;
I would add a blank line here.
> +err:
> + dev_err(dev, "unable to register RTC device (%d)\n", ret);
> + return ret;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id abeoz9_dt_match[] = {
> + { .compatible = "abracon,abeoz9" },
> + { },
> +};
> +#endif
> +
> +MODULE_DEVICE_TABLE(of, abeoz9_dt_match);
This should probably be included in the previous #ifdef
> +
> +static const struct i2c_device_id abeoz9_id[] = {
> + { "abeoz9", 0 },
> + { }
> +};
> +
> +static struct i2c_driver abeoz9_driver = {
> + .driver = {
> + .name = "rtc-ab-eoz9",
> + .of_match_table = of_match_ptr(abeoz9_dt_match),
> + },
> + .probe = abeoz9_probe,
> + .id_table = abeoz9_id,
> +};
> +
> +module_i2c_driver(abeoz9_driver);
> +
> +MODULE_AUTHOR("Artem Panfilov <panfilov.artyom@...il.com>");
> +MODULE_DESCRIPTION("Abracon AB-RTCMC-32.768kHz-EOZ9 RTC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.19.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists