Re: [PATCH v3] clk: Add Si5341/Si5340 driver

[Mike Looijmans <mike.looijmans@topic.nl>]

On 27-06-19 23:06, Stephen Boyd wrote:
> Quoting Mike Looijmans (2019-05-17 06:23:52)
>> Adds a driver for the Si5341 and Si5340 chips. The driver does not fully
>> support all features of these chips, but allows the chip to be used
>> without any support from the "clockbuilder pro" software.
>>
>> If the chip is preprogrammed, that is, you bought one with some defaults
>> burned in, or you programmed the NVM in some way, the driver will just
>> take over the current settings and only change them on demand. Otherwise
>> the input must be a fixed XTAL in its most basic configuration (no
>> predividers, no feedback, etc.).
>>
>> The driver supports dynamic changes of multisynth, output dividers and
>> enabling or powering down outputs and multisynths.
>>
>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl>
>> ---
> 
> Applied to clk-next + some fixes. I'm not super thrilled about the kHz
> thing but we don't have a solution for it right now so might as well
> come back to it later.

Thanks for the fixes. And I'm not exactly proud of that kHz part either.

While thinking about a solution, I've also had a use case for less than 1Hz 
frequency adjustment (a video clock to "follow" another one). These clock 
generators allow for ridiculous ranges and accuracy, you can request it to 
generate a 200000000.0005 Hz clock.


> diff --git a/drivers/clk/clk-si5341.c b/drivers/clk/clk-si5341.c
> index 1a310835b53c..72424eb7e5f8 100644
> --- a/drivers/clk/clk-si5341.c
> +++ b/drivers/clk/clk-si5341.c
> @@ -374,7 +374,7 @@ static unsigned long si5341_clk_recalc_rate(struct clk_hw *hw,
>   	 */
>   	shift = 0;
>   	res = m_num;
> -	while (res & 0xffff00000000) {
> +	while (res & 0xffff00000000ULL) {
>   		++shift;
>   		res >>= 1;
>   	}
> @@ -921,7 +921,7 @@ static int si5341_write_multiple(struct clk_si5341 *data,
>   	return 0;
>   }
>   
> -const struct si5341_reg_default si5341_preamble[] = {
> +static const struct si5341_reg_default si5341_preamble[] = {
>   	{ 0x0B25, 0x00 },
>   	{ 0x0502, 0x01 },
>   	{ 0x0505, 0x03 },
> @@ -994,7 +994,7 @@ static const struct regmap_range si5341_regmap_volatile_range[] = {
>   	regmap_reg_range(SI5341_SYNTH_N_UPD(4), SI5341_SYNTH_N_UPD(4)),
>   };
>   
> -const struct regmap_access_table si5341_regmap_volatile = {
> +static const struct regmap_access_table si5341_regmap_volatile = {
>   	.yes_ranges = si5341_regmap_volatile_range,
>   	.n_yes_ranges = ARRAY_SIZE(si5341_regmap_volatile_range),
>   };
> @@ -1016,7 +1016,6 @@ static const struct regmap_config si5341_regmap_config = {
>   	.reg_bits = 8,
>   	.val_bits = 8,
>   	.cache_type = REGCACHE_RBTREE,
> -	.max_register = 0,
>   	.ranges = si5341_regmap_ranges,
>   	.num_ranges = ARRAY_SIZE(si5341_regmap_ranges),
>   	.max_register = SI5341_REGISTER_MAX,
> @@ -1328,7 +1327,7 @@ MODULE_DEVICE_TABLE(i2c, si5341_id);
>   static const struct of_device_id clk_si5341_of_match[] = {
>   	{ .compatible = "silabs,si5340" },
>   	{ .compatible = "silabs,si5341" },
> -	{ },
> +	{ }
>   };
>   MODULE_DEVICE_TABLE(of, clk_si5341_of_match);
>   
>