Mon, May 22, 2023 at 06:31:13PM +0200, Esteban Blanc kirjoitti:
> TPS6594 PMIC is a MFD. This patch adds support for
> the RTC found inside TPS6594 family of PMIC.
>
> Alarm is also supported.
...
> + help
> + TI Power Management IC TPS6594 supports RTC functionality
> + along with alarm. This driver supports the RTC driver for
> + the TPS6594 RTC module.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tps6594-rtc
Grammar period at the end?
...
> +#define TPS6594_GET_TIME_ON TPS6594_BIT_GET_TIME
> +#define TPS6594_GET_TIME_OFF 0
Not used.
> +#define TPS6594_IT_ALARM_ON TPS6594_BIT_IT_ALARM
> +#define TPS6594_IT_ALARM_OFF 0
Used only once.
> +#define TPS6594_AUTO_COMP_ON TPS6594_BIT_IT_ALARM
No _OFF counterpart.
That said the _OFF can be dropped completely. And the rest I see no value to
have, just use those bit definitions directly?
...
> +static int tps6594_rtc_alarm_irq_enable(struct device *dev,
> + unsigned int enabled)
> +{
> + struct tps6594 *tps = dev_get_drvdata(dev->parent);
> + u8 val = 0;
Redundant assignment.
> + val = enabled ? TPS6594_IT_ALARM_ON : TPS6594_IT_ALARM_OFF;
> +
> + return regmap_update_bits(tps->regmap, TPS6594_REG_RTC_INTERRUPTS,
> + TPS6594_BIT_IT_ALARM, val);
> +}
...
> + // Read shadowed RTC registers.
> + ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_SECONDS, rtc_data,
> + NUM_TIME_REGS);
> + if (ret < 0)
> + return ret;
> +
> + tm->tm_sec = bcd2bin(rtc_data[0]);
> + tm->tm_min = bcd2bin(rtc_data[1]);
> + tm->tm_hour = bcd2bin(rtc_data[2]);
> + tm->tm_mday = bcd2bin(rtc_data[3]);
> + tm->tm_mon = bcd2bin(rtc_data[4]) - 1;
> + tm->tm_year = bcd2bin(rtc_data[5]) + 100;
> + tm->tm_wday = bcd2bin(rtc_data[6]);
> +
> + return ret;
return 0;
No?
...
> +static int tps6594_rtc_set_calibration(struct device *dev, int calibration)
> +{
> + unsigned char comp_data[NUM_COMP_REGS];
> + struct tps6594 *tps = dev_get_drvdata(dev->parent);
> + __le16 value;
> + int ret;
> +
> + /*
> + * TPS6594 uses two's complement 16 bit value for compensation of RTC
> + * crystal inaccuracies. One time every hour when seconds counter
> + * increments from 0 to 1 compensation value will be added to internal
> + * RTC counter value.
> + *
> + * Valid range for compensation value: [-32767 .. 32767].
This is defined naturally by the bits available, correct?
> + */
> + if (calibration < -32767 || calibration > 32767) {
So, this can be S16_MIN / S16_MAX range. The question here is what the
-32768 meaning is and why it can't be used.
> + dev_err(dev, "RTC calibration value out of range: %d\n",
> + calibration);
> + return -EINVAL;
-ERANGE
> + }
> + value = (__le16)calibration;
> +
> + comp_data[0] = value & 0xFF;
> + comp_data[1] = (value >> 8) & 0xFF;
Of course these three lines is not what expected.
value = cpu_to_le16();
> + // Update all the compensation registers in one shot.
> + ret = regmap_bulk_write(tps->regmap, TPS6594_REG_RTC_COMP_LSB,
> + comp_data, NUM_COMP_REGS);
&value, sizeof(value) ?
> + if (ret < 0)
> + return ret;
> +
> + // Enable automatic compensation.
> + return regmap_set_bits(tps->regmap, TPS6594_REG_RTC_CTRL_1,
> + TPS6594_BIT_AUTO_COMP);
> +}
...
> + ret = regmap_bulk_read(tps->regmap, TPS6594_REG_RTC_COMP_LSB, comp_data,
> + NUM_COMP_REGS);
> + if (ret < 0)
> + return ret;
> +
> + value = (__le16)comp_data[0] | ((__le16)comp_data[1] << 8);
> +
> + *calibration = value;
In the similar (complementary API) way as above.
...
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
Having
struct device *dev = &pdev->dev;
might make this and other lines shorter / neater.
> + tps6594_rtc_interrupt, IRQF_ONESHOT,
> + TPS6594_IRQ_NAME_ALARM, &pdev->dev);
> + if (ret < 0)
> + return dev_err_probe(&pdev->dev, ret,
> + "Failed to request_threaded_irq\n");
--
With Best Regards,
Andy Shevchenko
