On Fri, Nov 03, 2023 at 03:28:22PM +0100, Alexandre Belloni wrote:
> Hello Carlos,
>
> On 03/11/2023 09:51:05-0300, Carlos Menin wrote:
> > +struct pcf85053a {
> > + struct rtc_device *rtc;
> > + struct regmap *regmap;
> > + struct regmap *regmap_nvmem;
> > +};
> > +
> > +struct pcf85053a_config {
> > + struct regmap_config regmap;
> > + struct regmap_config regmap_nvmem;
> > +};
> > +
> > +static int pcf85053a_read_offset(struct device *dev, long *offset)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + long val;
> > + u32 reg_offset, reg_oscillator;
> > + int ret;
> > +
> > + ret = regmap_read(pcf85053a->regmap, REG_OFFSET, ®_offset);
> > + if (ret)
> > + return -EIO;
>
> Why do you change the error returned by regmap?
>
I am not sure why I did that, should just return ret. Thanks for noting.
> > +
> > + ret = regmap_read(pcf85053a->regmap, REG_OSCILLATOR, ®_oscillator);
> > + if (ret)
> > + return -EIO;
> > +
> > + val = sign_extend32(reg_offset, 7);
> > +
> > + if (reg_oscillator & REG_OSC_OFFM)
> > + *offset = val * OFFSET_STEP1;
> > + else
> > + *offset = val * OFFSET_STEP0;
> > +
> > + return 0;
> > +}
> > +
> > +static int pcf85053a_set_offset(struct device *dev, long offset)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + s8 mode0, mode1, reg_offset;
> > + unsigned int ret, error0, error1;
> > +
> > + if (offset > OFFSET_STEP0 * 127)
> > + return -ERANGE;
> > + if (offset < OFFSET_STEP0 * -128)
> > + return -ERANGE;
> > +
> > + ret = regmap_set_bits(pcf85053a->regmap, REG_ACCESS, REG_ACCESS_XCLK);
> > + if (ret)
> > + return -EIO;
> > +
> > + mode0 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP0);
> > + mode1 = DIV_ROUND_CLOSEST(offset, OFFSET_STEP1);
> > +
> > + error0 = abs(offset - (mode0 * OFFSET_STEP0));
> > + error1 = abs(offset - (mode1 * OFFSET_STEP1));
> > + if (error0 < error1) {
> > + reg_offset = mode0;
> > + ret = regmap_clear_bits(pcf85053a->regmap, REG_OSCILLATOR,
> > + REG_OSC_OFFM);
> > + } else {
> > + reg_offset = mode1;
> > + ret = regmap_set_bits(pcf85053a->regmap, REG_OSCILLATOR,
> > + REG_OSC_OFFM);
> > + }
> > + if (ret)
> > + return -EIO;
> > +
> > + ret = regmap_write(pcf85053a->regmap, REG_OFFSET, reg_offset);
> > +
> > + return ret;
> > +}
> > +
> > +static int pcf85053a_rtc_check_reliability(struct device *dev, u8 status_reg)
> > +{
> > + int ret = 0;
> > +
> > + if (status_reg & REG_STATUS_CIF) {
> > + dev_warn(dev, "tamper detected,"
> > + " date/time is not reliable\n");
> You should not split strings. Also, I don't think most of the messages
> are actually useful as the end user doesn't have any specific action
> after seeing it. You should probably drop them.
>
About the usefullness of the message, do this apply to this CIF related
message or also to the other two flags?
> I don't think CIF means the time is not correct anymore.
>
You are right, it only clears the SRAM. I will remove this flag from
this check.
> > + ret = -EINVAL;
> > + }
> > +
> > + if (status_reg & REG_STATUS_OF) {
> > + dev_warn(dev, "oscillator fail detected,"
> > + " date/time is not reliable.\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + if (status_reg & REG_STATUS_RTCF) {
> > + dev_warn(dev, "power loss detected,"
> > + " date/time is not reliable.\n");
> > + ret = -EINVAL;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int pcf85053a_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > + u8 buf[REG_STATUS + 1];
> > + int ret, len = sizeof(buf);
> > +
> > + ret = regmap_bulk_read(pcf85053a->regmap, REG_SECS, buf, len);
> > + if (ret) {
> > + dev_err(dev, "%s: error %d\n", __func__, ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf85053a_rtc_check_reliability(dev, buf[REG_STATUS]);
> > + if (ret)
> > + return ret;
> > +
> > + tm->tm_year = buf[REG_YEARS];
> > + /* adjust for 1900 base of rtc_time */
> > + tm->tm_year += 100;
> > +
> > + tm->tm_wday = (buf[REG_WEEKDAYS] - 1) & 7; /* 1 - 7 */
> > + tm->tm_sec = buf[REG_SECS];
> > + tm->tm_min = buf[REG_MINUTES];
> > + tm->tm_hour = buf[REG_HOURS];
> > + tm->tm_mday = buf[REG_DAYS];
> > + tm->tm_mon = buf[REG_MONTHS] - 1; /* 1 - 12 */
>
> Those comments are not useful.
>
I Will improve them to explain why I am offsetting the register value.
> > +
> > + return 0;
> > +}
> > +
>
> > +static ssize_t attr_flag_clear(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count,
> > + u8 reg, u8 flag)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
> > + int ret;
> > +
> > + (void)attr;
> > + (void)buf;
> > + (void)count;
> > +
> > + ret = regmap_clear_bits(pcf85053a->regmap, reg, flag);
> > + if (ret)
> > + return -EIO;
> > +
> > + return count;
> > +}
> > +
> > +static ssize_t attr_flag_read(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf,
> > + u8 reg, u8 flag)
> > +{
> > + struct pcf85053a *pcf85053a = dev_get_drvdata(dev->parent);
> > + unsigned int status, val;
> > + int ret;
> > +
> > + (void)attr;
> > + ret = regmap_read(pcf85053a->regmap, reg, &status);
> > + if (ret)
> > + return -EIO;
> > +
> > + val = (status & flag) != 0;
> > +
> > + return sprintf(buf, "%u\n", val);
> > +}
>
>
>
> > +
> > +/* flags that can be read or written to be cleared */
> > +#define PCF85053A_ATTR_FLAG_RWC(name, reg, flag) \
> > + static ssize_t name ## _store( \
> > + struct device *dev, \
> > + struct device_attribute *attr, \
> > + const char *buf, \
> > + size_t count) \
> > + { \
> > + return attr_flag_clear(dev, attr, buf, count, \
> > + REG_ ## reg, REG_ ## reg ## _ ## flag); \
> > + } \
> > + static ssize_t name ## _show( \
> > + struct device *dev, \
> > + struct device_attribute *attr, \
> > + char *buf) \
> > + { \
> > + return attr_flag_read(dev, attr, buf, \
> > + REG_ ## reg, REG_ ## reg ## _ ## flag); \
> > + } \
> > + static DEVICE_ATTR_RW(name)
> > +
> > +PCF85053A_ATTR_FLAG_RWC(rtc_fail, STATUS, RTCF);
> > +PCF85053A_ATTR_FLAG_RWC(oscillator_fail, STATUS, OF);
> > +PCF85053A_ATTR_FLAG_RWC(rtc_clear, STATUS, CIF);
> > +
> > +static struct attribute *pcf85053a_attrs_flags[] = {
> > + &dev_attr_rtc_fail.attr,
> > + &dev_attr_oscillator_fail.attr,
> > + &dev_attr_rtc_clear.attr,
> > + 0,
> > +};
>
> Don't add undocumented sysfs files. Also, You must not allow userspace
> to clear those flags without setting the time properly.
>
Should the flags be cleared only by setting the time, or is there
another acceptable method? What would be the correct way to let
userspace read those flags?
> > +static void pcf85053a_set_drive_control(struct device *dev, u8 *reg_ctrl)
> > +{
> > + int ret;
> > + const char *val;
> > + u8 regval;
> > +
> > + ret = of_property_read_string(dev->of_node, "nxp,quartz-drive-control",
> > + &val);
>
> This property should rather be "nxp,quartz-drive".
>
> > + if (ret) {
> > + dev_warn(dev, "failed to read nxp,quartz-drive-control property,"
> > + " assuming 'normal' drive");
> > + val = "normal";
> > + }
> > +
> > + if (!strcmp(val, "normal")) {
> > + regval = 0;
> > + } else if (!strcmp(val, "low")) {
> > + regval = 1;
> > + } else if (!strcmp(val, "high")) {
> > + regval = 2;
> > + } else {
> > + dev_warn(dev, "invalid nxp,quartz-drive-control value: %s,"
> > + " assuming 'normal' drive", val);
> > + regval = 0;
> > + }
> > +
> > + *reg_ctrl |= (regval << 2);
>
> 2 needs a define, what about using FIELD_PREP?
>
> > +}
> > +
> > +static void pcf85053a_set_low_jitter(struct device *dev, u8 *reg_ctrl)
> > +{
> > + bool val;
> > + u8 regval;
> > +
> > + val = of_property_read_bool(dev->of_node, "nxp,low-jitter-mode");
>
> Bool properties don't work well with RTC because with this, there is now
> way to enable the normal mode.
>
Wouldn't the absence of the property enable normal mode? Or I am missing
something?
> > +
> > + regval = val ? 1 : 0;
> > + *reg_ctrl |= (regval << 4);
> 4 also needs a define
>
> > +}
> > +
> > +static void pcf85053a_set_clk_inverted(struct device *dev, u8 *reg_ctrl)
> > +{
> > + bool val;
> > + u8 regval;
> > +
> > + val = of_property_read_bool(dev->of_node, "nxp,clk-inverted");
> > +
> > + regval = val ? 1 : 0;
> > + *reg_ctrl |= (regval << 7);
>
> Ditto
> > +}
> > +
> > +static int pcf85053a_probe(struct i2c_client *client)
> > +{
> > + int ret;
> > + struct pcf85053a *pcf85053a;
> > + const struct pcf85053a_config *config = &pcf85053a_config;
> > + u8 reg_ctrl;
> > +
> > + pcf85053a = devm_kzalloc(&client->dev, sizeof(*pcf85053a), GFP_KERNEL);
> > + if (!pcf85053a) {
> > + dev_err(&client->dev, "failed to allocate device: no memory");
> > + return -ENOMEM;
> > + }
> > +
> > + pcf85053a->regmap = devm_regmap_init_i2c(client, &config->regmap);
> > + if (IS_ERR(pcf85053a->regmap)) {
> > + dev_err(&client->dev, "failed to allocate regmap: %ld\n",
> > + PTR_ERR(pcf85053a->regmap));
> > + return PTR_ERR(pcf85053a->regmap);
> > + }
> > +
> > + i2c_set_clientdata(client, pcf85053a);
> > +
> > + pcf85053a->rtc = devm_rtc_allocate_device(&client->dev);
> > + if (IS_ERR(pcf85053a->rtc)) {
> > + dev_err(&client->dev, "failed to allocate rtc: %ld\n",
> > + PTR_ERR(pcf85053a->rtc));
> > + return PTR_ERR(pcf85053a->rtc);
> > + }
> > +
> > + pcf85053a->rtc->ops = &pcf85053a_rtc_ops;
> > + pcf85053a->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > + pcf85053a->rtc->range_max = RTC_TIMESTAMP_END_2099;
> > +
> > + reg_ctrl = REG_CTRL_DM | REG_CTRL_HF | REG_CTRL_CIE;
>
> CIE enables an interrupt but you never use interrupts.
>
> > + pcf85053a_set_load_capacitance(&client->dev, ®_ctrl);
> > + pcf85053a_set_drive_control(&client->dev, ®_ctrl);
> > + pcf85053a_set_low_jitter(&client->dev, ®_ctrl);
> > + pcf85053a_set_clk_inverted(&client->dev, ®_ctrl);
> > +
> > + ret = regmap_write(pcf85053a->regmap, REG_CTRL, reg_ctrl);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to configure rtc: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = rtc_add_group(pcf85053a->rtc, &pcf85053a_attr_group);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to add sysfs entry: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = devm_rtc_register_device(pcf85053a->rtc);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to register rtc: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ret = pcf85053a_add_nvmem(client, pcf85053a);
> > + if (ret) {
> > + dev_err(&client->dev, "failed to register nvmem: %d\n", ret);
> > + return ret;
>
> probe must not fail after devm_rtc_register_device
>
> > + }
> > +
> > + ret = pcf85053a_hwmon_register(&client->dev, client->name);
> > + if (ret)
> > + dev_err(&client->dev, "failed to register hwmon: %d\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static const __maybe_unused struct of_device_id dev_ids[] = {
> > + { .compatible = "nxp,pcf85053a", .data = &pcf85053a_config },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, dev_ids);
> > +
> > +static struct i2c_driver pcf85053a_driver = {
> > + .driver = {
> > + .name = "pcf85053a",
> > + .of_match_table = of_match_ptr(dev_ids),
> > + },
> > + .probe_new = &pcf85053a_probe,
> > +};
> > +
> > +module_i2c_driver(pcf85053a_driver);
> > +
> > +MODULE_AUTHOR("Carlos Menin <menin@xxxxxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("PCF85053A I2C RTC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Thanks for reviewing! I will implement changes to improve all noted
points in the next version of the patch.
Carlos
