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

[Parthiban Nallathambi]

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@xxxxxxx