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: 11+ 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
2025-01-09 10:29 ` PavithraUdayakumar-adi via B4 Relay
2025-01-09 10:29 ` [PATCH v3 1/2] dt-bindings: rtc: max31335: Add max31331 support PavithraUdayakumar-adi
2025-01-09 10:29 ` 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
2025-01-09 10:29 ` 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.