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

["Sascha Hauer" <s.hauer@pengutronix.de>]

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@kernel.org/

> > +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.

> > +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?

> > +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.

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).

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 |