Re: [PATCH 4/7] soc: rockchip: add mfpwm driver

["Heiko Stübner" <heiko@sntech.de>]

Hi,

not a full review, just me making a first pass.

> +unsigned long mfpwm_clk_get_rate(struct rockchip_mfpwm *mfpwm)
> +{
> +	if (!mfpwm || !mfpwm->chosen_clk)
> +		return 0;
> +
> +	return clk_get_rate(mfpwm->chosen_clk);
> +}
> +EXPORT_SYMBOL_NS_GPL(mfpwm_clk_get_rate, "ROCKCHIP_MFPWM");

aren't you just re-implemeting a clk-mux with the whole chosen-clk
mechanism? See drivers/clk/clk-mux.c, so in theory you should be
able to just do a clk_register_mux(...) similar to for example
sound/soc/samsung/i2s.c .


> +
> +__attribute__((nonnull))
> +static int mfpwm_do_acquire(struct rockchip_mfpwm_func *pwmf)
> +{
> +	struct rockchip_mfpwm *mfpwm = pwmf->parent;
> +	unsigned int cnt;
> +
> +	if (mfpwm->active_func && pwmf->id != mfpwm->active_func->id)
> +		return -EBUSY;
> +
> +	if (!mfpwm->active_func)
> +		mfpwm->active_func = pwmf;
> +
> +	if (!check_add_overflow(mfpwm->acquire_cnt, 1, &cnt)) {
> +		mfpwm->acquire_cnt = cnt;
> +	} else {
> +		WARN(1, "prevented acquire counter overflow in %s\n", __func__);

dev_warn, as you have the mfpwm pointing to a pdev?

> +		return -EOVERFLOW;
> +	}
> +
> +	dev_dbg(&mfpwm->pdev->dev, "%d acquired mfpwm, acquires now at %u\n",
> +		pwmf->id, mfpwm->acquire_cnt);
> +
> +	return clk_enable(mfpwm->pclk);
> +}

> +/**
> + * mfpwm_get_clk_src - read the currently selected clock source
> + * @mfpwm: pointer to the driver's private &struct rockchip_mfpwm instance
> + *
> + * Read the device register to extract the currently selected clock source,
> + * and return it.
> + *
> + * Returns:
> + * * the numeric clock source ID on success, 0 <= id <= 2
> + * * negative errno on error
> + */
> +static int mfpwm_get_clk_src(struct rockchip_mfpwm *mfpwm)
> +{
> +	u32 val;
> +
> +	clk_enable(mfpwm->pclk);
> +	val = mfpwm_reg_read(mfpwm->base, PWMV4_REG_CLK_CTRL);
> +	clk_disable(mfpwm->pclk);
> +
> +	return (val & PWMV4_CLK_SRC_MASK) >> PWMV4_CLK_SRC_SHIFT;
> +}
> +
> +static int mfpwm_choose_clk(struct rockchip_mfpwm *mfpwm)
> +{
> +	int ret;
> +
> +	ret = mfpwm_get_clk_src(mfpwm);
> +	if (ret < 0) {
> +		dev_err(&mfpwm->pdev->dev, "couldn't get current clock source: %pe\n",
> +			ERR_PTR(ret));
> +		return ret;
> +	}
> +	if (ret == PWMV4_CLK_SRC_CRYSTAL) {
> +		if (mfpwm->osc_clk) {
> +			mfpwm->chosen_clk = mfpwm->osc_clk;
> +		} else {
> +			dev_warn(&mfpwm->pdev->dev, "initial state wanted 'osc' as clock source, but it's unavailable. Defaulting to 'pwm'.\n");
> +			mfpwm->chosen_clk = mfpwm->pwm_clk;
> +		}
> +	} else {
> +		mfpwm->chosen_clk = mfpwm->pwm_clk;
> +	}
> +
> +	return clk_rate_exclusive_get(mfpwm->chosen_clk);
> +}
>
> +/**
> + * mfpwm_switch_clk_src - switch between PWM clock sources
> + * @mfpwm: pointer to &struct rockchip_mfpwm driver data
> + * @clk_src: one of either %PWMV4_CLK_SRC_CRYSTAL or %PWMV4_CLK_SRC_PLL
> + *
> + * Switch between clock sources, ``_exclusive_put``ing the old rate,
> + * ``clk_rate_exclusive_get``ing the new one, writing the registers and
> + * swapping out the &struct_rockchip_mfpwm->chosen_clk.
> + *
> + * Returns:
> + * * %0        - Success
> + * * %-EINVAL  - A wrong @clk_src was given or it is unavailable
> + * * %-EBUSY   - Device is currently in use, try again later
> + */
> +__attribute__((nonnull))
> +static int mfpwm_switch_clk_src(struct rockchip_mfpwm *mfpwm,
> +					  unsigned int clk_src)
> +{
> +	struct clk *prev;
> +	int ret = 0;
> +
> +	scoped_cond_guard(spinlock_try, return -EBUSY, &mfpwm->state_lock) {
> +		/* Don't fiddle with any of this stuff if the PWM is on */
> +		if (mfpwm->active_func)
> +			return -EBUSY;
> +
> +		prev = mfpwm->chosen_clk;
> +		ret = mfpwm_get_clk_src(mfpwm);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == clk_src)
> +			return 0;
> +
> +		switch (clk_src) {
> +		case PWMV4_CLK_SRC_PLL:
> +			mfpwm->chosen_clk = mfpwm->pwm_clk;
> +			break;
> +		case PWMV4_CLK_SRC_CRYSTAL:
> +			if (!mfpwm->osc_clk)
> +				return -EINVAL;
> +			mfpwm->chosen_clk = mfpwm->osc_clk;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		clk_enable(mfpwm->pclk);
> +
> +		mfpwm_reg_write(mfpwm->base, PWMV4_REG_CLK_CTRL,
> +				PWMV4_CLK_SRC(clk_src));
> +		clk_rate_exclusive_get(mfpwm->chosen_clk);
> +		if (prev)
> +			clk_rate_exclusive_put(prev);
> +
> +		clk_disable(mfpwm->pclk);
> +	}
> +
> +	return ret;
> +}

ok, the relevant part might be the 
	/* Don't fiddle with any of this stuff if the PWM is on */
thing, which will require special set_rate operation, but in general I
think, if it ticks like a clock, it probably should be a real clock ;-) .


> +static ssize_t chosen_clock_show(struct device *dev,
> +				 struct device_attribute *attr, char *buf)
> +{
> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> +	unsigned long clk_src = 0;
> +
> +	/*
> +	 * Why the weird indirection here? I have the suspicion that if we
> +	 * emitted to sysfs with the lock still held, then a nefarious program
> +	 * could hog the lock by somehow forcing a full buffer condition and
> +	 * then refusing to read from it. Don't know whether that's feasible
> +	 * to achieve in reality, but I don't want to find out the hard way
> +	 * either.
> +	 */
> +	scoped_guard(spinlock, &mfpwm->state_lock) {
> +		if (mfpwm->chosen_clk == mfpwm->pwm_clk)
> +			clk_src = PWMV4_CLK_SRC_PLL;
> +		else if (mfpwm->osc_clk && mfpwm->chosen_clk == mfpwm->osc_clk)
> +			clk_src = PWMV4_CLK_SRC_CRYSTAL;
> +		else
> +			return -ENODEV;
> +	}
> +
> +	if (clk_src == PWMV4_CLK_SRC_PLL)
> +		return sysfs_emit(buf, "pll\n");
> +	else if (clk_src == PWMV4_CLK_SRC_CRYSTAL)
> +		return sysfs_emit(buf, "crystal\n");
> +
> +	return -ENODEV;
> +}

which brings me to my main point of contention. Why does userspace
need to select a clock source for the driver via sysfs.

Neither the commit message nor the code does seem to explain that,
or I'm just blind - which is also a real possibility.

In general I really think, userspace should not need to care about if
a PLL or directly the oscillator is used a clock input.
I assume which is needed results from some runtime factor, so the
driver should be able to select the correct one?

A mux-clock could ust use clk_mux_determine_rate_flags() to select
the best parent depending on a requested rate instead.


> +static ssize_t chosen_clock_store(struct device *dev,
> +				  struct device_attribute *attr,
> +				  const char *buf, size_t count)
> +{
> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (sysfs_streq(buf, "pll")) {
> +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_PLL);
> +		if (ret)
> +			return ret;
> +		return count;
> +	} else if (sysfs_streq(buf, "crystal")) {
> +		ret = mfpwm_switch_clk_src(mfpwm, PWMV4_CLK_SRC_CRYSTAL);
> +		if (ret)
> +			return ret;
> +		return count;
> +	} else {
> +		return -EINVAL;
> +	}
> +}
> +
> +static DEVICE_ATTR_RW(chosen_clock);
> +
> +static ssize_t available_clocks_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct rockchip_mfpwm *mfpwm = dev_get_drvdata(dev);
> +	ssize_t size = 0;
> +
> +	size += sysfs_emit_at(buf, size, "pll\n");
> +	if (mfpwm->osc_clk)
> +		size += sysfs_emit_at(buf, size, "crystal\n");
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RO(available_clocks);
> +
> +static struct attribute *mfpwm_attrs[] = {
> +	&dev_attr_available_clocks.attr,
> +	&dev_attr_chosen_clock.attr,
> +	NULL,
> +};

Not understanding the need for the sysfs stuff was my main point this
evening :-)

Heiko