| 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: <202311012223422e3387b3@mail.local>
Date: Wed, 1 Nov 2023 23:23:42 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: Alessandro Zummo <a.zummo@...ertech.it>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>, linux-rtc@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v4 2/2] rtc: max31335: add driver support
On 01/11/2023 11:48:14+0200, Antoniu Miclaus wrote:
> +static int max31335_get_hour(u8 hour_reg)
> +{
> + int hour;
> +
> + /* 24Hr mode */
> + if (!FIELD_GET(MAX31335_HOURS_F_24_12, hour_reg))
> + return bcd2bin(hour_reg & 0x3f);
> +
> + /* 12Hr mode */
> + hour = bcd2bin(hour_reg & 0x1f);
> + if (hour == 12)
> + hour = 0;
> +
Do you really need to support 12h mode?
> + if (FIELD_GET(MAX31335_HOURS_HR_20_AM_PM, hour_reg))
> + hour += 12;
> +
> + return hour;
> +}
> +
> +static int max31335_read_offset(struct device *dev, long *offset)
> +{
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> + u32 value;
> + int ret;
> +
> + ret = regmap_read(max31335->regmap, MAX31335_AGING_OFFSET, &value);
> + if (ret)
> + return ret;
> +
> + *offset = value;
This is super dubious, what is the unit of MAX31335_AGING_OFFSET ?
> +
> + return 0;
> +}
> +
> +static int max31335_set_offset(struct device *dev, long offset)
> +{
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> +
> + return regmap_write(max31335->regmap, MAX31335_AGING_OFFSET, offset);
> +}
> +
> +static int max31335_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> + struct mutex *lock = &max31335->rtc->ops_lock;
> + int ret, ctrl, status;
> + struct rtc_time time;
> + u8 regs[6];
> +
> + mutex_lock(lock);
Use rtc_lock(), I'm not quite sure why you would need locking here
though.
> +
> + ret = regmap_bulk_read(max31335->regmap, MAX31335_ALM1_SEC, regs,
> + sizeof(regs));
> + if (ret)
> + goto exit;
> +
> + alrm->time.tm_sec = bcd2bin(regs[0] & 0x7f);
> + alrm->time.tm_min = bcd2bin(regs[1] & 0x7f);
> + alrm->time.tm_hour = bcd2bin(regs[2] & 0x3f);
> + alrm->time.tm_mday = bcd2bin(regs[3] & 0x3f);
> + alrm->time.tm_mon = bcd2bin(regs[4] & 0x1f) - 1;
> + alrm->time.tm_year = bcd2bin(regs[5]) + 100;
> +
> + ret = max31335_read_time(dev, &time);
> + if (ret)
> + goto exit;
> +
> + if (time.tm_year >= 200)
> + alrm->time.tm_year += 100;
> +
> + ret = regmap_read(max31335->regmap, MAX31335_INT_EN1, &ctrl);
> + if (ret)
> + goto exit;
> +
> + ret = regmap_read(max31335->regmap, MAX31335_STATUS1, &status);
> + if (ret)
> + goto exit;
> +
> + alrm->enabled = FIELD_GET(MAX31335_INT_EN1_A1IE, ctrl);
> + alrm->pending = FIELD_GET(MAX31335_STATUS1_A1F, status);
> +
> +exit:
> + mutex_unlock(lock);
> +
> + return ret;
> +}
> +
> +static int max31335_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> + struct mutex *lock = &max31335->rtc->ops_lock;
> + unsigned int reg;
> + u8 regs[6];
> + int ret;
> +
> + regs[0] = bin2bcd(alrm->time.tm_sec);
> + regs[1] = bin2bcd(alrm->time.tm_min);
> + regs[2] = bin2bcd(alrm->time.tm_hour);
> + regs[3] = bin2bcd(alrm->time.tm_mday);
> + regs[4] = bin2bcd(alrm->time.tm_mon + 1);
> + regs[5] = bin2bcd(alrm->time.tm_year % 100);
> +
> + mutex_lock(lock);
I'm not sure why you need locking here either, maybe you should simply
disable the alarm first?
> +
> + ret = regmap_bulk_write(max31335->regmap, MAX31335_ALM1_SEC,
> + regs, sizeof(regs));
> + if (ret)
> + goto exit;
> +
> + reg = FIELD_PREP(MAX31335_INT_EN1_A1IE, alrm->enabled);
> + ret = regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> + MAX31335_INT_EN1_A1IE, reg);
> + if (ret)
> + goto exit;
> +
> + ret = regmap_update_bits(max31335->regmap, MAX31335_STATUS1,
> + MAX31335_STATUS1_A1F, 0);
> +
> +exit:
> + mutex_unlock(lock);
> +
> + return ret;
> +}
> +
> +static int max31335_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> +
> + return regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> + MAX31335_INT_EN1_A1IE, enabled);
> +}
> +
> +static int max31335_trickle_charger_setup(struct device *dev,
> + struct max31335_data *max31335)
> +{
> + u32 ohms, chargeable;
> + bool diode = false;
> + int i;
> +
> + if (device_property_read_u32(dev, "trickle-resistor-ohms", &ohms))
> + return 0;
> +
> + if (!device_property_read_u32(dev, "aux-voltage-chargeable",
> + &chargeable)) {
> + switch (chargeable) {
> + case 0:
> + diode = false;
> + break;
> + case 1:
> + diode = true;
> + break;
> + default:
> + dev_warn(dev,
> + "unsupported aux-voltage-chargeable value\n");
I don't think the string is necessary, checking the DT should be done at
compile time.
> + break;
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(max31335_trickle_resistors); i++)
> + if (ohms == max31335_trickle_resistors[i])
> + break;
> +
> + if (i >= ARRAY_SIZE(max31335_trickle_resistors)) {
> + dev_warn(dev, "invalid trickle resistor value\n");
Ditto.
> +
> + return 0;
> + }
> +
> + if (diode)
> + i = i + 4;
> + else
> + i = i + 1;
Do you actually need to configure the trickle charger when there is
nothing to charge?
> +
> + return regmap_write(max31335->regmap, MAX31335_TRICKLE_REG,
> + FIELD_PREP(MAX31335_TRICKLE_REG_TRICKLE, i) |
> + MAX31335_TRICKLE_REG_EN_TRICKLE);
> +}
> +
> +static int max31335_clkout_register(struct device *dev)
> +{
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> + int ret;
> +
> + if (!device_property_present(dev, "#clock-cells"))
> + return 0;
Is the clock output disabled by default?
> +
> +static int max31335_probe(struct i2c_client *client)
> +{
> + struct max31335_data *max31335;
> + struct device *hwmon;
> + int ret;
> +
> + max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), GFP_KERNEL);
> + if (!max31335)
> + return -ENOMEM;
> +
> + max31335->regmap = devm_regmap_init_i2c(client, ®map_config);
> + if (IS_ERR(max31335->regmap))
> + return PTR_ERR(max31335->regmap);
> +
> + i2c_set_clientdata(client, max31335);
> +
> + ret = regmap_write(max31335->regmap, MAX31335_RTC_RESET, 1);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(max31335->regmap, MAX31335_RTC_RESET, 0);
> + if (ret)
> + return ret;
What does this register do?
> +
> + max31335->rtc = devm_rtc_allocate_device(&client->dev);
> + if (IS_ERR(max31335->rtc))
> + return PTR_ERR(max31335->rtc);
> +
> + max31335->rtc->ops = &max31335_rtc_ops;
> + max31335->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> + max31335->rtc->range_max = RTC_TIMESTAMP_END_2199;
Please set alarm_offset_max too.
> +
> + ret = devm_rtc_register_device(max31335->rtc);
> + if (ret)
> + return ret;
> +
> + ret = max31335_clkout_register(&client->dev);
> + if (ret)
> + return ret;
> +
> + if (client->irq > 0) {
> + ret = devm_request_threaded_irq(&client->dev, client->irq,
> + NULL, max31335_handle_irq,
> + IRQF_ONESHOT,
> + "max31335", max31335);
> + if (ret) {
> + dev_warn(&client->dev,
> + "unable to request IRQ, alarm max31335 disabled\n");
> + client->irq = 0;
> + }
> + }
> +
> + if (!client->irq)
> + clear_bit(RTC_FEATURE_ALARM, max31335->rtc->features);
> +
> + max31335_nvmem_cfg.priv = max31335;
> + ret = devm_rtc_nvmem_register(max31335->rtc, &max31335_nvmem_cfg);
> + if (ret)
> + dev_err_probe(&client->dev, ret, "cannot register rtc nvmem\n");
> +
> + hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name,
> + max31335,
> + &max31335_chip_info,
> + NULL);
> + if (IS_ERR(hwmon))
> + dev_err_probe(&client->dev, PTR_ERR(hwmon),
> + "cannot register hwmon device\n");
> +
> + return max31335_trickle_charger_setup(&client->dev, max31335);
You must never fail probe after calling devm_rtc_register_device, else
you are open to a race condition with userspace.
> +}
> +
> +static const struct i2c_device_id max31335_id[] = {
> + { "max31335", 0 },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, max31335_id);
> +
> +static const struct of_device_id max31335_of_match[] = {
> + { .compatible = "adi,max31335" },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(of, max31335_of_match);
> +
> +static struct i2c_driver max31335_driver = {
> + .driver = {
> + .name = "rtc-max31335",
> + .of_match_table = max31335_of_match,
> + },
> + .probe = max31335_probe,
> + .id_table = max31335_id,
> +};
> +module_i2c_driver(max31335_driver);
> +
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@...log.com>");
> +MODULE_DESCRIPTION("MAX31335 RTC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.42.0
>
--
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists