From: "Nuno Sá" <noname.nuno@gmail.com>
To: pavithra.u@analog.com,
Antoniu Miclaus <antoniu.miclaus@analog.com>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Jean Delvare <jdelvare@suse.com>,
Guenter Roeck <linux@roeck-us.net>,
Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: linux-rtc@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH v3 2/2] rtc: max31335: Add driver support for max31331
Date: Thu, 16 Jan 2025 08:16:25 +0000 [thread overview]
Message-ID: <f30b5ceb77e44e7b74fd19b0c620d27a3c612e94.camel@gmail.com> (raw)
In-Reply-To: <20250109-add_support_max31331_fix_3-v1-2-a74fac29bf49@analog.com>
On Thu, 2025-01-09 at 15:59 +0530, PavithraUdayakumar-adi via B4 Relay wrote:
> From: PavithraUdayakumar-adi <pavithra.u@analog.com>
>
> MAX31331 is an ultra-low-power, I2C Real-Time Clock RTC.
>
> Signed-off-by: PavithraUdayakumar-adi <pavithra.u@analog.com>
> ---
> drivers/rtc/rtc-max31335.c | 163 +++++++++++++++++++++++++++++++++-----------
> -
> 1 file changed, 120 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/rtc/rtc-max31335.c b/drivers/rtc/rtc-max31335.c
> index
> 3fbcf5f6b92ffd4581e9c4dbc87ec848867522dc..9d8f220fba2b21fc6393d67bff47f99b0227
> c707 100644
> --- a/drivers/rtc/rtc-max31335.c
> +++ b/drivers/rtc/rtc-max31335.c
> @@ -184,31 +184,88 @@
> #define MAX31335_RAM_SIZE 32
> #define MAX31335_TIME_SIZE 0x07
>
> +/* MAX31331 Register Map */
> +#define MAX31331_RTC_CONFIG2 0x04
> +
> #define clk_hw_to_max31335(_hw) container_of(_hw, struct max31335_data,
> clkout)
>
> +/* Supported Maxim RTC */
> +enum max_rtc_ids {
> + ID_MAX31331,
> + ID_MAX31335,
> + MAX_RTC_ID_NR
> +};
> +
> +struct chip_desc {
> + u8 sec_reg;
> + u8 alarm1_sec_reg;
> +
> + u8 int_en_reg;
> + u8 int_status_reg;
> +
> + u8 ram_reg;
> + u8 ram_size;
> +
> + u8 temp_reg;
> +
> + u8 trickle_reg;
> +
> + u8 clkout_reg;
> +};
> +
> struct max31335_data {
> + enum max_rtc_ids id;
> struct regmap *regmap;
> struct rtc_device *rtc;
> struct clk_hw clkout;
> + struct clk *clkin;
> + const struct chip_desc *chip;
> + int irq;
> };
>
> static const int max31335_clkout_freq[] = { 1, 64, 1024, 32768 };
>
> +static const struct chip_desc chip[MAX_RTC_ID_NR] = {
> + [ID_MAX31331] = {
> + .int_en_reg = 0x01,
> + .int_status_reg = 0x00,
> + .sec_reg = 0x08,
> + .alarm1_sec_reg = 0x0F,
> + .ram_reg = 0x20,
> + .ram_size = 32,
> + .trickle_reg = 0x1B,
> + .clkout_reg = 0x04,
> + },
> + [ID_MAX31335] = {
> + .int_en_reg = 0x01,
> + .int_status_reg = 0x00,
> + .sec_reg = 0x0A,
> + .alarm1_sec_reg = 0x11,
> + .ram_reg = 0x40,
> + .ram_size = 32,
> + .temp_reg = 0x35,
> + .trickle_reg = 0x1D,
> + .clkout_reg = 0x06,
> + },
> +};
> +
> static const u16 max31335_trickle_resistors[] = {3000, 6000, 11000};
>
> static bool max31335_volatile_reg(struct device *dev, unsigned int reg)
> {
> + struct max31335_data *max31335 = dev_get_drvdata(dev);
> + const struct chip_desc *chip = max31335->chip;
> +
> /* time keeping registers */
> - if (reg >= MAX31335_SECONDS &&
> - reg < MAX31335_SECONDS + MAX31335_TIME_SIZE)
> + if (reg >= chip->sec_reg && reg < chip->sec_reg + MAX31335_TIME_SIZE)
> return true;
>
> /* interrupt status register */
> - if (reg == MAX31335_STATUS1)
> + if (reg == chip->int_status_reg)
> return true;
>
> - /* temperature registers */
> - if (reg == MAX31335_TEMP_DATA_MSB || reg == MAX31335_TEMP_DATA_LSB)
> + /* temperature registers if valid */
> + if (chip->temp_reg && (reg == chip->temp_reg || reg == chip->temp_reg
> + 1))
> return true;
>
> return false;
> @@ -227,7 +284,7 @@ static int max31335_read_time(struct device *dev, struct
> rtc_time *tm)
> u8 date[7];
> int ret;
>
> - ret = regmap_bulk_read(max31335->regmap, MAX31335_SECONDS, date,
> + ret = regmap_bulk_read(max31335->regmap, max31335->chip->sec_reg,
> date,
> sizeof(date));
> if (ret)
> return ret;
> @@ -262,7 +319,7 @@ static int max31335_set_time(struct device *dev, struct
> rtc_time *tm)
> if (tm->tm_year >= 200)
> date[5] |= FIELD_PREP(MAX31335_MONTH_CENTURY, 1);
>
> - return regmap_bulk_write(max31335->regmap, MAX31335_SECONDS, date,
> + return regmap_bulk_write(max31335->regmap, max31335->chip->sec_reg,
> date,
> sizeof(date));
> }
>
> @@ -273,7 +330,7 @@ static int max31335_read_alarm(struct device *dev, struct
> rtc_wkalrm *alrm)
> struct rtc_time time;
> u8 regs[6];
>
> - ret = regmap_bulk_read(max31335->regmap, MAX31335_ALM1_SEC, regs,
> + ret = regmap_bulk_read(max31335->regmap, max31335->chip-
> >alarm1_sec_reg, regs,
> sizeof(regs));
> if (ret)
> return ret;
> @@ -292,11 +349,11 @@ static int max31335_read_alarm(struct device *dev,
> struct rtc_wkalrm *alrm)
> if (time.tm_year >= 200)
> alrm->time.tm_year += 100;
>
> - ret = regmap_read(max31335->regmap, MAX31335_INT_EN1, &ctrl);
> + ret = regmap_read(max31335->regmap, max31335->chip->int_en_reg,
> &ctrl);
> if (ret)
> return ret;
>
> - ret = regmap_read(max31335->regmap, MAX31335_STATUS1, &status);
> + ret = regmap_read(max31335->regmap, max31335->chip->int_status_reg,
> &status);
> if (ret)
> return ret;
>
> @@ -320,18 +377,18 @@ static int max31335_set_alarm(struct device *dev, struct
> rtc_wkalrm *alrm)
> regs[4] = bin2bcd(alrm->time.tm_mon + 1);
> regs[5] = bin2bcd(alrm->time.tm_year % 100);
>
> - ret = regmap_bulk_write(max31335->regmap, MAX31335_ALM1_SEC,
> + ret = regmap_bulk_write(max31335->regmap, max31335->chip-
> >alarm1_sec_reg,
> regs, sizeof(regs));
> if (ret)
> return ret;
>
> reg = FIELD_PREP(MAX31335_INT_EN1_A1IE, alrm->enabled);
> - ret = regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> + ret = regmap_update_bits(max31335->regmap, max31335->chip-
> >int_en_reg,
> MAX31335_INT_EN1_A1IE, reg);
> if (ret)
> return ret;
>
> - ret = regmap_update_bits(max31335->regmap, MAX31335_STATUS1,
> + ret = regmap_update_bits(max31335->regmap, max31335->chip-
> >int_status_reg,
> MAX31335_STATUS1_A1F, 0);
>
> return 0;
> @@ -341,23 +398,33 @@ 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,
> + return regmap_update_bits(max31335->regmap, max31335->chip-
> >int_en_reg,
> MAX31335_INT_EN1_A1IE, enabled);
> }
>
> static irqreturn_t max31335_handle_irq(int irq, void *dev_id)
> {
> struct max31335_data *max31335 = dev_id;
> - bool status;
> - int ret;
> + struct mutex *lock = &max31335->rtc->ops_lock;
> + int ret, status;
> +
> + mutex_lock(lock);
>
> - ret = regmap_update_bits_check(max31335->regmap, MAX31335_STATUS1,
> - MAX31335_STATUS1_A1F, 0, &status);
> + ret = regmap_read(max31335->regmap, max31335->chip->int_status_reg,
> &status);
> if (ret)
> - return IRQ_HANDLED;
> + goto exit;
> +
> + if (FIELD_GET(MAX31335_STATUS1_A1F, status)) {
> + ret = regmap_update_bits(max31335->regmap, max31335->chip-
> >int_status_reg,
> + MAX31335_STATUS1_A1F, 0);
> + if (ret)
> + goto exit;
>
> - if (status)
> rtc_update_irq(max31335->rtc, 1, RTC_AF | RTC_IRQF);
> + }
> +
> +exit:
> + mutex_unlock(lock);
>
> return IRQ_HANDLED;
> }
> @@ -404,7 +471,7 @@ static int max31335_trickle_charger_setup(struct device
> *dev,
>
> i = i + trickle_cfg;
>
> - return regmap_write(max31335->regmap, MAX31335_TRICKLE_REG,
> + return regmap_write(max31335->regmap, max31335->chip->trickle_reg,
> FIELD_PREP(MAX31335_TRICKLE_REG_TRICKLE, i) |
> FIELD_PREP(MAX31335_TRICKLE_REG_EN_TRICKLE,
> chargeable));
> @@ -418,7 +485,7 @@ static unsigned long max31335_clkout_recalc_rate(struct
> clk_hw *hw,
> unsigned int reg;
> int ret;
>
> - ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, ®);
> + ret = regmap_read(max31335->regmap, max31335->chip->clkout_reg,
> ®);
> if (ret)
> return 0;
>
> @@ -449,23 +516,23 @@ static int max31335_clkout_set_rate(struct clk_hw *hw,
> unsigned long rate,
> ARRAY_SIZE(max31335_clkout_freq));
> freq_mask = __roundup_pow_of_two(ARRAY_SIZE(max31335_clkout_freq)) -
> 1;
>
> - return regmap_update_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> - freq_mask, index);
> + return regmap_update_bits(max31335->regmap, max31335->chip-
> >clkout_reg,
> + freq_mask, index);
> }
>
> static int max31335_clkout_enable(struct clk_hw *hw)
> {
> struct max31335_data *max31335 = clk_hw_to_max31335(hw);
>
> - return regmap_set_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> - MAX31335_RTC_CONFIG2_ENCLKO);
> + return regmap_set_bits(max31335->regmap, max31335->chip->clkout_reg,
> + MAX31335_RTC_CONFIG2_ENCLKO);
> }
>
> static void max31335_clkout_disable(struct clk_hw *hw)
> {
> struct max31335_data *max31335 = clk_hw_to_max31335(hw);
>
> - regmap_clear_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> + regmap_clear_bits(max31335->regmap, max31335->chip->clkout_reg,
> MAX31335_RTC_CONFIG2_ENCLKO);
> }
>
> @@ -475,7 +542,7 @@ static int max31335_clkout_is_enabled(struct clk_hw *hw)
> unsigned int reg;
> int ret;
>
> - ret = regmap_read(max31335->regmap, MAX31335_RTC_CONFIG2, ®);
> + ret = regmap_read(max31335->regmap, max31335->chip->clkout_reg,
> ®);
> if (ret)
> return ret;
>
> @@ -500,7 +567,7 @@ static int max31335_nvmem_reg_read(void *priv, unsigned
> int offset,
> void *val, size_t bytes)
> {
> struct max31335_data *max31335 = priv;
> - unsigned int reg = MAX31335_TS0_SEC_1_128 + offset;
> + unsigned int reg = max31335->chip->ram_reg + offset;
>
> return regmap_bulk_read(max31335->regmap, reg, val, bytes);
> }
> @@ -509,7 +576,7 @@ static int max31335_nvmem_reg_write(void *priv, unsigned
> int offset,
> void *val, size_t bytes)
> {
> struct max31335_data *max31335 = priv;
> - unsigned int reg = MAX31335_TS0_SEC_1_128 + offset;
> + unsigned int reg = max31335->chip->ram_reg + offset;
>
> return regmap_bulk_write(max31335->regmap, reg, val, bytes);
> }
> @@ -533,7 +600,7 @@ static int max31335_read_temp(struct device *dev, enum
> hwmon_sensor_types type,
> if (type != hwmon_temp || attr != hwmon_temp_input)
> return -EOPNOTSUPP;
>
> - ret = regmap_bulk_read(max31335->regmap, MAX31335_TEMP_DATA_MSB,
> + ret = regmap_bulk_read(max31335->regmap, max31335->chip->temp_reg,
> reg, 2);
> if (ret)
> return ret;
> @@ -577,8 +644,8 @@ static int max31335_clkout_register(struct device *dev)
> int ret;
>
> if (!device_property_present(dev, "#clock-cells"))
> - return regmap_clear_bits(max31335->regmap,
> MAX31335_RTC_CONFIG2,
> - MAX31335_RTC_CONFIG2_ENCLKO);
> + return regmap_clear_bits(max31335->regmap, max31335->chip-
> >clkout_reg,
> + MAX31335_RTC_CONFIG2_ENCLKO);
>
> max31335->clkout.init = &max31335_clk_init;
>
> @@ -605,6 +672,7 @@ static int max31335_probe(struct i2c_client *client)
> #if IS_REACHABLE(HWMON)
> struct device *hwmon;
> #endif
> + const struct chip_desc *match;
> int ret;
>
> max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), GFP_KERNEL);
> @@ -616,7 +684,11 @@ static int max31335_probe(struct i2c_client *client)
> return PTR_ERR(max31335->regmap);
>
> i2c_set_clientdata(client, max31335);
> -
> + match = i2c_get_match_data(client);
> + if (!match)
> + return -ENODEV;
> + max31335->chip = match;
> + max31335->id = max31335->chip - chip;
I kind of already expressed this internally... I can't agree with the above. Why
not making 'id' a member of 'struct chip_desc'? The above is very useless IMHO.
- Nuno Sá
next prev parent reply other threads:[~2025-01-16 8:16 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-09 10:29 [PATCH v3 0/2] Add support for MAX31331 RTC PavithraUdayakumar-adi via B4 Relay
2025-01-09 10:29 ` [PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support PavithraUdayakumar-adi via B4 Relay
2025-01-10 8:35 ` Krzysztof Kozlowski
2025-01-15 10:21 ` U, Pavithra
2025-01-15 16:07 ` Krzysztof Kozlowski
2025-01-09 10:29 ` [PATCH v3 2/2] rtc: max31335: Add driver support for max31331 PavithraUdayakumar-adi via B4 Relay
2025-01-16 8:16 ` Nuno Sá [this message]
2025-01-10 8:32 ` [PATCH v3 0/2] Add support for MAX31331 RTC Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f30b5ceb77e44e7b74fd19b0c620d27a3c612e94.camel@gmail.com \
--to=noname.nuno@gmail.com \
--cc=alexandre.belloni@bootlin.com \
--cc=antoniu.miclaus@analog.com \
--cc=christophe.jaillet@wanadoo.fr \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=krzk+dt@kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rtc@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=pavithra.u@analog.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®