Re: [PATCH] rtc: rv3028: add clkout support

[Parthiban Nallathambi <pn@denx.de>]

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@denx.de