Re: [PATCH v19 2/6] pwm: driver for qualcomm ipq6018 pwm block

[George Moussalem]

On 11/28/25 15:32, Konrad Dybcio wrote:
> On 11/28/25 11:29 AM, George Moussalem via B4 Relay wrote:
>> From: Devi Priya <quic_devipriy@xxxxxxxxxxx>
>>
>> Driver for the PWM block in Qualcomm IPQ6018 line of SoCs. Based on
>> driver from downstream Codeaurora kernel tree. Removed support for older
>> (V1) variants because I have no access to that hardware.
>>
>> Tested on IPQ5018 and IPQ6010 based hardware.
>>
>> Co-developed-by: Baruch Siach <baruch.siach@xxxxxxxxx>
>> Signed-off-by: Baruch Siach <baruch.siach@xxxxxxxxx>
>> Signed-off-by: Devi Priya <quic_devipriy@xxxxxxxxxxx>
>> Reviewed-by: Bjorn Andersson <andersson@xxxxxxxxxx>
>> Signed-off-by: George Moussalem <george.moussalem@xxxxxxxxxxx>
>> ---
> 
> [...]
> 
>> +/* The frequency range supported is 1 Hz to clock rate */
>> +#define IPQ_PWM_MAX_PERIOD_NS	((u64)NSEC_PER_SEC)
>> +
>> +/*
>> + * The max value specified for each field is based on the number of bits
>> + * in the pwm control register for that field
>> + */
>> +#define IPQ_PWM_MAX_DIV		0xFFFF
>> +
>> +/*
>> + * Two 32-bit registers for each PWM: REG0, and REG1.
>> + * Base offset for PWM #i is at 8 * #i.
>> + */
>> +#define IPQ_PWM_REG0			0
>> +#define IPQ_PWM_REG0_PWM_DIV		GENMASK(15, 0)
>> +#define IPQ_PWM_REG0_HI_DURATION	GENMASK(31, 16)
>> +
>> +#define IPQ_PWM_REG1			4
>> +#define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
> 
> Sorry for coming in so late, you may consider this as material for a
> follow-up patch (I *really* don't want to hold off your v19..)
> 
> I see that on ipq9574 the registers are named:
> 
> base = 0x1941010 = tcsr + 0xa010
> 
> 0x0	CFG0_R0
> 0x4	CFG1_R0
> 0x8	CFG0_R1
> 0xc	CFG1_R1
> 0x10	CFG0_R2
> 0x14	CFG1_R2
> 0x18	CFG0_R3
> 0x1c	CFG1_R3
> 
> CFG0 and CFG1 are what you called REG0/REG1 and Rn is confusingly the
> index of the controller/output
> 
> The other bits in CFG1 (29:16) are RESERVED so there's nothing you
> missed in there

thanks for your review and validation.

> 
>> +
>> +/*
>> + * Enable bit is set to enable output toggling in pwm device.
>> + * Update bit is set to trigger the change and is unset automatically
>> + * to reflect the changed divider and high duration values in register.
>> + */
>> +#define IPQ_PWM_REG1_UPDATE		BIT(30)
>> +#define IPQ_PWM_REG1_ENABLE		BIT(31)
>> +
>> +struct ipq_pwm_chip {
>> +	struct clk *clk;
>> +	void __iomem *mem;
>> +};
>> +
>> +static struct ipq_pwm_chip *ipq_pwm_from_chip(struct pwm_chip *chip)
>> +{
>> +	return pwmchip_get_drvdata(chip);
>> +}
>> +
>> +static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned int reg)
>> +{
>> +	struct ipq_pwm_chip *ipq_chip = ipq_pwm_from_chip(pwm->chip);
>> +	unsigned int off = 8 * pwm->hwpwm + reg;
> 
> This magic 8 could be #defined as IPQ6018_PWM_CONTROLLER_STRIDE or so

good suggestion, will add it should another iteration be required.

> 
> Konrad

BR,
George