| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c13ed6b7-fd44-45ce-9429-30ff1f1f15a2@wanadoo.fr>
Date: Fri, 3 Jan 2025 15:25:56 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: pavithra.u@...log.com, Antoniu Miclaus <antoniu.miclaus@...log.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Jean Delvare <jdelvare@...e.com>,
Guenter Roeck <linux@...ck-us.net>
Cc: linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rtc:max31335: Add driver support for max31331
Le 03/01/2025 à 08:04, PavithraUdayakumar-adi via B4 Relay a écrit :
> From: PavithraUdayakumar-adi <pavithra.u@...log.com>
>
> Add driver support for max31331 RTC chip.
>
> Signed-off-by: PavithraUdayakumar-adi <pavithra.u@...log.com>
> ---
> drivers/rtc/rtc-max31335.c | 182 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 131 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/rtc/rtc-max31335.c b/drivers/rtc/rtc-max31335.c
> index 3fbcf5f6b92ffd4581e9c4dbc87ec848867522dc..f2c094686b5a89aee8041f3f563bb2cf9fc6275b 100644
> --- a/drivers/rtc/rtc-max31335.c
> +++ b/drivers/rtc/rtc-max31335.c
> @@ -34,7 +34,7 @@
> #define MAX31335_RTC_CONFIG2 0x06
> #define MAX31335_TIMESTAMP_CONFIG 0x07
> #define MAX31335_TIMER_CONFIG 0x08
> -#define MAX31335_SECONDS_1_128 0x09
> +#define MAX31335_SECONDS_1_128 0x09
No need to remove 1 tab here.
Things now look un'aligned.
> #define MAX31335_SECONDS 0x0A
> #define MAX31335_MINUTES 0x0B
> #define MAX31335_HOURS 0x0C
> @@ -45,7 +45,7 @@
> #define MAX31335_ALM1_SEC 0x11
> #define MAX31335_ALM1_MIN 0x12
> #define MAX31335_ALM1_HRS 0x13
> -#define MAX31335_ALM1_DAY_DATE 0x14
> +#define MAX31335_ALM1_DAY_DATE 0x14
No need to remove 1 tab here.
Things now look un'aligned.
> #define MAX31335_ALM1_MON 0x15
> #define MAX31335_ALM1_YEAR 0x16
> #define MAX31335_ALM2_MIN 0x17
...
> 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*/
Missing space before */
> + if (chip->temp_reg && (reg == chip->temp_reg || reg == chip->temp_reg + 1))
> return true;
>
> return false;
...
> @@ -444,28 +511,31 @@ static int max31335_clkout_set_rate(struct clk_hw *hw, unsigned long rate,
> struct max31335_data *max31335 = clk_hw_to_max31335(hw);
> unsigned int freq_mask;
> int index;
> + int ret;
>
> index = find_closest(rate, max31335_clkout_freq,
> 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);
> + ret = regmap_update_bits(max31335->regmap, max31335->chip->clkout_reg,
> + freq_mask, index);
> +
> + return ret;
You could keep a direct return here, instead of using a new 'ret'.
> }
>
> 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);
...
> @@ -576,9 +646,10 @@ 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 regmap_clear_bits(max31335->regmap, MAX31335_RTC_CONFIG2,
> - MAX31335_RTC_CONFIG2_ENCLKO);
> + if (!device_property_present(dev, "#clock-cells")) {
> + regmap_clear_bits(max31335->regmap, max31335->chip->clkout_reg,
> + MAX31335_RTC_CONFIG2_ENCLKO);
> + }
No need to add new { }.
Is it safe to change the behavior here?
If it is a fix, waybe it should be done in another patch before this one.
>
> max31335->clkout.init = &max31335_clk_init;
>
> @@ -599,12 +670,14 @@ static int max31335_clkout_register(struct device *dev)
> return 0;
> }
>
> +/* 6.1 probe() function still uses the second struct i2c_device_id argument */
Is this comment really needed?
Is this patch expected to be backported in 6.1?
> static int max31335_probe(struct i2c_client *client)
> {
> struct max31335_data *max31335;
> #if IS_REACHABLE(HWMON)
> struct device *hwmon;
> #endif
...
> @@ -648,19 +727,17 @@ static int max31335_probe(struct i2c_client *client)
> max31335_nvmem_cfg.priv = max31335;
> ret = devm_rtc_nvmem_register(max31335->rtc, &max31335_nvmem_cfg);
> if (ret)
> - return dev_err_probe(&client->dev, ret,
> - "cannot register rtc nvmem\n");
> + return dev_err_probe(&client->dev, ret, "cannot register rtc nvmem\n");
Unneeded clean-up.
>
> #if IS_REACHABLE(HWMON)
> - hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name,
> - max31335,
> - &max31335_chip_info,
> - NULL);
> - if (IS_ERR(hwmon))
> - return dev_err_probe(&client->dev, PTR_ERR(hwmon),
> - "cannot register hwmon device\n");
> + if (max31335->chip->temp_reg) {
> + hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name, max31335,
> + &max31335_chip_info, NULL);
> + if (IS_ERR(hwmon))
> + return dev_err_probe(&client->dev, PTR_ERR(hwmon),
> + "cannot register hwmon device\n");
> + }
> #endif
> -
> ret = max31335_trickle_charger_setup(&client->dev, max31335);
> if (ret)
> return ret;
> @@ -669,15 +746,17 @@ static int max31335_probe(struct i2c_client *client)
> }
>
> static const struct i2c_device_id max31335_id[] = {
> - { "max31335" },
> - { }
> + { "max31331", (kernel_ulong_t)&chip[ID_MAX31331] },
> + { "max31335", (kernel_ulong_t)&chip[ID_MAX31335] },
> + {}
No need to remove 1 space here.
> };
>
> MODULE_DEVICE_TABLE(i2c, max31335_id);
>
> static const struct of_device_id max31335_of_match[] = {
> - { .compatible = "adi,max31335" },
> - { }
> + { .compatible = "adi,max31331", .data = &chip[ID_MAX31331] },
> + { .compatible = "adi,max31335", .data = &chip[ID_MAX31335] },
> + {}
No need to remove 1 space here.
> };
>
> MODULE_DEVICE_TABLE(of, max31335_of_match);
...
CJ
Powered by blists - more mailing lists