Re: [PATCH v9 2/2] clk: add TI CDCE6214 clock driver

[Jerome Brunet]

On Tue 29 Sep 2026 at 10:43, "Sascha Hauer" <s.hauer@xxxxxxxxxxxxxx> wrote:

> Hi Jerome,
>
> On 2026-09-25 11:54, Jerome Brunet wrote:
>> > +
>> > +#define R0	0
>> > +#define RO_I2C_A0			BIT(15)
>> > +#define RO_PDN_INPUT_SEL		BIT(14)
>> > +#define RO_GPIO4_DIR_SEL		BIT(13)
>> > +#define RO_GPIO1_DIR_SEL		BIT(12)
>> > +#define RO_ZDM_CLOCKSEL			BIT(10)
>> > +#define RO_ZDM_EN			BIT(8)
>> > +#define RO_SYNC				BIT(5)
>> > +#define RO_RECAL			BIT(4)
>> > +#define RO_RESETN_SOFT			BIT(3)
>> > +#define RO_SWRST			BIT(2)
>> > +#define RO_POWERDOWN			BIT(1)
>> > +#define RO_MODE				BIT(0)
>> > +
>> > +#define R1	1
>> 
>> I don't really get the point of those defines
>
> It was requested by Stephen [1]:
>
>> Maybe it would be better to have '#define R30 30' just so we can easily
>> jump to the fields like R30_PLL_NDIV. I see that the datasheet doesn't
>> give a name to these registers besides prefixing the decimal offset with
>> the letter 'R'.
>
> I don't have any personal preference.
>
> [1] https://lore.kernel.org/all/e5858fe5d4276a735c5354e955358f27@xxxxxxxxxx/
>

I've sync with him. keep it that way

>> > +static int cdce6214_clk_out_set_parent(struct clk_hw *hw, u8 index)
>> > +{
>> > +	struct cdce6214_clock *clock = hw_to_cdce6214_clk(hw);
>> > +	struct cdce6214 *priv = clock->priv;
>> > +
>> > +	switch (clock->index) {
>> > +	case CDCE6214_CLK_OUT1:
>> > +		regmap_update_bits(priv->regmap, R56, R56_CH1_MUX, FIELD_PREP(R56_CH1_MUX, index));
>> > +		break;
>> > +	case CDCE6214_CLK_OUT2:
>> > +		regmap_update_bits(priv->regmap, R62, R62_CH2_MUX, FIELD_PREP(R62_CH2_MUX, index));
>> > +		break;
>> > +	case CDCE6214_CLK_OUT3:
>> > +		regmap_update_bits(priv->regmap, R67, R67_CH3_MUX, FIELD_PREP(R67_CH3_MUX, index));
>> > +		break;
>> > +	case CDCE6214_CLK_OUT4:
>> > +		regmap_update_bits(priv->regmap, R72, R72_CH4_MUX, FIELD_PREP(R72_CH4_MUX, index));
>> > +		break;
>> 
>> Can you properly describe your clock rather than doing this sort of
>> matching ?
>
> You mean something like:
>
> struct cdce6214_clk_data {
>         unsigned int pd_reg;
>         u32 pd_mask;
>         unsigned int reg;
>         u32 mux_mask;
>         u32 div_mask;
> };
>
> static const struct cdce6214_clk_data cdce6214_clk_data[CDCE6214_NUM_CLOCKS] = {
>         [CDCE6214_CLK_OUT1] = {
>                 .pd_reg = R4,
>                 .pd_mask = R4_CH1_PD,
>                 .reg = R56,
>                 .mux_mask = R56_CH1_MUX,
>                 .div_mask = R56_CH1_DIV,
>         },
>         ...
> };
>
> If yes, I can do that.

The way you've done it above is duplication of code. Each clock appears
to have an offset and a field, those data should be associated to clock
then your function is just a single regmap_update_bits.

>
>> > +static int cdce6214_get_psx_div(unsigned long rate, unsigned long parent_rate)
>> > +{
>> > +	unsigned int div = DIV_ROUND_CLOSEST(parent_rate, max(rate, 1UL));
>> > +
>> > +	return clamp(div, 4, 6);
>> > +}
>> 
>> Can't you just use a regular divider operation ? split you clocks in
>> different entities ?
>
> I can switch to use the generic divider helpers (divider_recalc_rate,
> divider_determine_rate). Is that where you aiming at or do you want me
> to register the clocks as composite clocks?

No I don't mean composite, I mean use helper functions such has
divider_determine_rate(), divider_recalc_rate(), etc ...

>
>> > +static int cdce6214_probe(struct i2c_client *client)
>> > +{
>> > +	struct device *dev = &client->dev;
>> > +	struct cdce6214 *priv;
>> > +	struct pinctrl_dev *pctl;
>> > +	int ret;
>> > +
>> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> > +	if (!priv)
>> > +		return -ENOMEM;
>> > +
>> > +	priv->client = client;
>> > +	priv->dev = dev;
>> > +	i2c_set_clientdata(client, priv);
>> > +	dev_set_drvdata(dev, priv);
>> > +
>> > +	priv->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
>> > +	if (IS_ERR(priv->reset_gpio)) {
>> > +		return dev_err_probe(dev, PTR_ERR(priv->reset_gpio),
>> > +				     "failed to get reset gpio\n");
>> > +	}
>> > +
>> > +	priv->regmap = devm_regmap_init_i2c(client, &cdce6214_regmap_config);
>> > +	if (IS_ERR(priv->regmap))
>> > +		return dev_err_probe(dev, PTR_ERR(priv->regmap),
>> > +				     "failed to init regmap\n");
>> > +
>> > +	ret = cdce6214_configure(priv);
>> > +	if (ret)
>> > +		return ret;
>> > +
>> > +	ret = devm_pinctrl_register_and_init(dev, &cdce6214_pdesc, priv, &pctl);
>> > +	if (ret)
>> > +		return dev_err_probe(dev, ret, "pinctrl register failed");
>> > +
>> > +	ret = pinctrl_enable(pctl);
>> > +	if (ret)
>> > +		return dev_err_probe(dev, ret, "pinctrl enable failed");
>> 
>> It feels like it should an MFD driver. half the driver belong in pinctrl
>
> I'd rather not go that way as it means splitting a relatively straight
> forward driver into three subsystems with different maintainers. Also it
> means getting cross device dependencies right.

... and then the trouble is for us maintainers to sort things out
whenever the framework needs change.

>
> Taking "Multi Function Device" by its name it means it's a device with
> multiple functions. The CDCE6214 is not that, it's only purpose is to
> generate clocks, only that the pins need some configuration. Most other
> devices of this type just have device specific properties in the device
> tree for configuring their pins (which I was rightfully pushed away from
> in earlier reviews).

It may have a single purpose but it is still providing multiple functions
and we will not have this pinctrl driver under clock. Please submit it
to pinctrl where it will receive proper review and maintainance.

IMO MFD is proper way to do it.

>
> Sascha
>
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>

-- 
Jerome