Re: [PATCH] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Uwe Kleine-König]

Hello,

On Mon, Dec 07, 2020 at 10:40:22PM -0600, Bjorn Andersson wrote:
> The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> with the primary purpose of controlling the backlight of the attached
> panel. Add an implementation that exposes this using the standard PWM
> framework, to allow e.g. pwm-backlight to expose this to the user.
> 
> Special thanks to Doug Anderson for suggestions related to the involved
> math.

Did you test this with CONFIG_PWM_DEBUG? (I think you didn't, because
otherwise there would be a .get_state callback.)

> @@ -162,6 +171,12 @@ struct ti_sn_bridge {
>  	struct gpio_chip		gchip;
>  	DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
>  #endif
> +#if defined(CONFIG_PWM)

Would it make sense to introduce a separate config symbol for this?
Something like CONFIG_PWM_SN65DSI87?

> +	struct pwm_chip			pchip;
> +	bool				pwm_enabled;
> +	unsigned int			pwm_refclk;
> +	atomic_t			pwm_pin_busy;

struct ti_sn_bridge has a kernel doc comment describing all members,
please add a description of the members you introduced here. Please also
point out that you use pwm_pin_busy to protect against concurrent use of
the pin as PWM and GPIO.

> +#endif
>  };
>  
>  static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -499,6 +514,14 @@ static void ti_sn_bridge_set_refclk_freq(struct ti_sn_bridge *pdata)
>  
>  	regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, REFCLK_FREQ_MASK,
>  			   REFCLK_FREQ(i));
> +
> +#if defined(CONFIG_PWM)
> +	/*
> +	 * The PWM refclk is based on the value written to SN_DPPLL_SRC_REG,
> +	 * regardless of its actual sourcing.
> +	 */
> +	pdata->pwm_refclk = ti_sn_bridge_refclk_lut[i];
> +#endif

I don't understand this code. 'i' seems to be something more special
than a counter variable, so I wonder if it should have a better name.
(This is however an issue separate from this patch, but it would be
great to first make the code a bit better understandable. Or is this
only me?)

>  }
>  
>  static void ti_sn_bridge_set_dsi_rate(struct ti_sn_bridge *pdata)
> @@ -981,6 +1004,161 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_PWM)
> +static int ti_sn_pwm_pin_request(struct ti_sn_bridge *pdata)
> +{
> +	return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0;
> +}
> +
> +static void ti_sn_pwm_pin_release(struct ti_sn_bridge *pdata)
> +{
> +	atomic_set(&pdata->pwm_pin_busy, 0);
> +}
> +
> +static struct ti_sn_bridge *
> +pwm_chip_to_ti_sn_bridge(struct pwm_chip *chip)

All your functions share the same function prefix (which is fine), but
this one doesn't.

> +{
> +	return container_of(chip, struct ti_sn_bridge, pchip);
> +}
> [...]
> +	if (state->enabled) {
> +		/*
> +		 * Per the datasheet the PWM frequency is given by:
> +		 *
> +		 * PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)
> +		 *
> +		 * In order to find the PWM_FREQ that best suits the requested
> +		 * state->period, the PWM_PRE_DIV is calculated with the
> +		 * maximum possible number of steps (BACKLIGHT_SCALE_MAX). The
> +		 * actual BACKLIGHT_SCALE is then adjusted down to match the
> +		 * requested period.
> +		 *
> +		 * The BACKLIGHT value is then calculated against the
> +		 * BACKLIGHT_SCALE, based on the requested duty_cycle and
> +		 * period.
> +		 */
> +		pwm_freq = NSEC_PER_SEC / state->period;

Here you should better have some range checking. Consider for example
state->period being > NSEC_PER_SEC. (Hint: This makes pwm_freq = 0 and
in the next line you divide by pwm_freq.)

> +		pre_div = DIV_ROUND_UP(pdata->pwm_refclk / pwm_freq - 1, BACKLIGHT_SCALE_MAX);
> +		scale = (pdata->pwm_refclk / pwm_freq - 1) / pre_div;

I'm still trying to wrap my head around this calculation, but dividing
by the result of a division is always loosing precision. This is really
involved and I'm willing to bet this can be done easier and with more
precision.

... some time later ...

You wrote "PWM_FREQ = REFCLK_FREQ / (PWM_PRE_DIV * BACKLIGHT_SCALE + 1)",
so (I think) that means you have:

	period = (PWM_PRE_DIV * BACKLIGHT_SCALE + 1) / refclk

right? I deduce from your formula how the duty_cycle is defined and I
think it's:

	duty_cycle = (PWM_PRE_DIV * BACKLIGHT + 1) / refclk

is this right? And now your idea to "best suite the requested period" is
to select a small divider such that you can still use a big value in
SCALE to define the period and so have a fine separation for the
duty_cycle, right?

I will stop doing maths here now until you confirm my steps up to now
are right.

> +		backlight = scale * state->duty_cycle / state->period;

This is an u64 division, you must use do_div for that. Also you're
losing precision here.

> +		ret = regmap_write(pdata->regmap, SN_PWM_PRE_DIV_REG, pre_div);
> +		if (ret) {
> +			dev_err(pdata->dev, "failed to update PWM_PRE_DIV\n");
> +			goto out;
> +		}
> +
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_SCALE_REG, scale);
> +		ti_sn_bridge_write_u16(pdata, SN_BACKLIGHT_REG, backlight);

How does the PWM behave in between these writes? Are the register values
shadowed until the third write happens (which would be the optimum), or
does this result in (maybe) emitting an output wave that doesn't
correspond to the requested setting (assuming the PWM is already enabled
of course)?

What happens if the value written to SN_BACKLIGHT_SCALE_REG is less than
the previous value in SN_BACKLIGHT_REG? ti_sn_bridge_write_u16 wraps two
regmap writes, is there a race, too?

> +	}
> +
> +	pwm_en_inv = FIELD_PREP(BIT(1), !!state->enabled) |
> +		     FIELD_PREP(BIT(0), state->polarity == PWM_POLARITY_INVERSED);

Please introduce symbolic names for BIT(1) and BIT(0) here.

How does the hardware behave with the enable bit unset? Does it emit the
inactive level according to the polarity bit?

> +	ret = regmap_write(pdata->regmap, SN_PWM_EN_INV_REG, pwm_en_inv);
> +	if (ret) {
> +		dev_err(pdata->dev, "failed to update PWM_EN/PWM_INV\n");
> +		goto out;
> +	}
> +
> +	pdata->pwm_enabled = !!state->enabled;
> +out:
> +
> +	if (!pdata->pwm_enabled)
> +		pm_runtime_put_sync(pdata->dev);
> +
> +	return ret;
> +}
> +
> [...]
> +static struct pwm_device *ti_sn_pwm_of_xlate(struct pwm_chip *pc,
> +					     const struct of_phandle_args *args)
> +{
> +	struct pwm_device *pwm;
> +
> +	if (args->args_count != 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	pwm = pwm_request_from_chip(pc, 0, NULL);
> +	if (IS_ERR(pwm))
> +		return pwm;
> +
> +	pwm->args.period = args->args[0];
> +
> +	return pwm;
> +}

This is done to optimise away the 0 needed in each phandle to implement
the "usual" pwm binding. IMHO this function should either move into the
pwm core, or you should stick to the usual binding.

Apropos binding: Is there already a binding document for the hardware?
You should expand it to describe your additions.

> @@ -1282,6 +1476,12 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	ret = ti_sn_setup_pwmchip(pdata);
> +	if (ret)  {
> +		pm_runtime_disable(pdata->dev);
> +		return ret;
> +	}

I'm not sure about the purpose of the containing hardware, but I wonder
if it would be saner to not break probing of the device if adding the
PWM functionality fails. Ideally the driver would provide an mfd driver
that allows its components to be probed independently.

>  	i2c_set_clientdata(client, pdata);
>  
>  	pdata->aux.name = "ti-sn65dsi86-aux";
> @@ -1320,6 +1520,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  
>  	drm_bridge_remove(&pdata->bridge);
>  
> +	ti_sn_remove_pwmchip(pdata);
> +
>  	return 0;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature