> 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?
Is is a feature supported by the part, so I think is nice to have.
>
> > + 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 ?
>
There is not additional information on the AGING_OFFSET register (no
other offset registers).
I treated it as a raw value that user can write/read. Should I drop the
offset implementation?
> > +
> > + 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?
These are the options for the trickle chager:
MAX31335_TRICKLE_REG_TRICKLE bits
0x0: 3KΩ in series with a Schottky diode
0x1: 3KΩ in series with a Schottky diode
0x2: 6KΩ in series with a Schottky diode
0x3: 11KΩ in series with a Schottky diode
0x4: 3KΩ in series with a diode+Schottky diode
0x5: 3KΩ in series with a diode+Schottky diode
0x6: 6KΩ in series with a diode+Schottky diode
0x7: 11KΩ in series with a diode+Schottky diode
>
> > +
> > + 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?
No, I will disable it.
>
> > +
> > +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?
>
RTC Software Reset Register:
0x0: Device is in normal mode.
0x1: Resets the digital block and the I2C programmable registers except for
Timestamp/RAM registers and the SWRST bit. Oscillator is disabled.
The bit doesn't clear itself.
> > +
> > + 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@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("MAX31335 RTC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.42.0
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://urldefense.com/v3/__https://bootlin.com__;!!A3Ni8CS0y2Y!9L5oCv
> HC0oT_FwMzkX2RA8fSIOBFPOxEQ_aycnNEcZqpwVS5HKLHl_s1Cji_iNKNHLFI
> Y37QpXsMZUPCyBmurhBxVvwaPVOU$
