linux-kernel - Re: [PATCH] rtc: rv3028: add clkout support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <942df087-0f61-f573-77a0-7b27582e25c8@denx.de>
Date:   Fri, 18 Oct 2019 12:02:28 +0200
From:   Parthiban Nallathambi <pn@...x.de>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     pn@...x.de, a.zummo@...ertech.it, linux-rtc@...r.kernel.org,
        linux-kernel@...r.kernel.org, parthitce@...il.com
Subject: Re: [PATCH] rtc: rv3028: add clkout support

Hi,

Thanks for the comments. Will send v2 with changes.

On 10/11/19 11:17 AM, Alexandre Belloni wrote:
> Hi,
> 
> thanks for the patch, minor comments below.
> 
> On 27/09/2019 16:15:05+0200, Parthiban Nallathambi wrote:
>> +	ret = regmap_update_bits(rv3028->regmap, RV3028_STATUS,
>> +				 RV3028_STATUS_CLKF, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
> 
> This is already done earlier and this will not be enabled again unless
> CLKIE is set which should not happen. So I don't think it is necessary
> to do it once again here.

Agree, removed in v2.

> 
>> +	for (i = 0; i < ARRAY_SIZE(clkout_rates); i++) {
>> +		if (clkout_rates[i] == rate) {
>> +			ret = regmap_update_bits(rv3028->regmap,
>> +						 RV3028_CLKOUT,
>> +						 RV3028_CLKOUT_FD_MASK, i);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			return regmap_write(rv3028->regmap, RV3028_CLKOUT,
>> +				RV3028_CLKOUT_CLKSY | RV3028_CLKOUT_CLKOE);
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
> 
> 
>> +static int rv3028_clkout_register_clk(struct rv3028_data *rv3028,
>> +				      struct i2c_client *client)
>> +{
>> +	int ret;
>> +	struct clk *clk;
>> +	struct clk_init_data init;
>> +	struct device_node *node = client->dev.of_node;
>> +
>> +	/* disable the clkout output */
>> +	ret = regmap_write(rv3028->regmap, RV3028_CLKOUT, 0x0);
>> +	if (ret < 0)
>> +		return ret;
>> +
> 
> This is not what the user would expect and could introduce a glitch in
> the clock output every time the platform is booted. If there are no
> users of the clock, then you should probably let the core disable it
> once the boot has ended.

Thanks, removed in v2.

> 
> 

-- 
Thanks,
Parthiban N

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-22 Fax: (+49)-8142-66989-80 Email: pn@...x.de