Hi, Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx] > Sent: 2019年3月18日 16:08 > To: Anson Huang <anson.huang@xxxxxxx> > Cc: thierry.reding@xxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; > festevam@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; otavio@xxxxxxxxxxxxxxxx; > stefan@xxxxxxxx; Leonard Crestez <leonard.crestez@xxxxxxx>; > schnitzeltony@xxxxxxxxx; jan.tuerk@xxxxxxxxxxx; Robin Gong > <yibin.gong@xxxxxxx>; linux-pwm@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [PATCH V4 2/5] pwm: Add i.MX TPM PWM driver support > > On Mon, Mar 18, 2019 at 07:41:02AM +0000, Anson Huang wrote: > > Hi,Uwe > > > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > > > > > As this interrupts the output, please only do it if necessary. > > > > OK, will do it ONLY when it is enabled previously. > > I think you only need to do that when the value actually changes. OK, I will save the period/div count and ONLY do it when the value actually changes. > > > > > + /* set duty counter */ > > > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD) & > > > > +PWM_IMX_TPM_MOD_MOD_MASK; > > > > > > I recommend storing this value in driver data. > > > > NOT quite understand, as we did NOT use it in other places except the > > get_state, just reading the register once should be OK there. > > I had the impression it is used more than once. Will look again in the next > iteration. But also note that shadowing the value might already be beneficial > for a single call site as driver data might occupy more RAM than necessary > anyhow and reading from RAM is faster than from the hardware's register. > Probably this is not a fast path, so not worth the optimisation?! OK, will save it in driver data and avoid accessing register again. > > > > I wonder why MSA and MSB are two bits instead of making this a field > > > of width 2 with 2b10 meaning PWM mode. But maybe it's just me not > > > understanding the independent semantic of these two bits? > > > > I think making them a field makes more sense, but anyway we just > > follow the RM. > > If it makes the driver easier to understand (which I think it does) feel free to > derivate from the RM. Just add a comment to the definition, then it's fine. OK, I will change the register definition and a comment for it. > > > > Reading the reference manual I'd say in PWM mode the semantic of > > > ELSA and ELSB is: > > > > > > On counter reload set the output to ELSB > > > On counter match set the output to ELSA > > > > > > Noting that in a comment would ease understanding the code here. > > > > I added below comment for PWM modes: > > > > 137 /* > > 138 * set polarity (for edge-aligned PWM modes) > > 139 * > > 140 * CPWMS MSB:MSA ELSB:ELSA Mode Configuration > > 141 * 0 10 10 PWM High-true pulse > > 142 * 0 10 00 PWM Reserved > > 143 * 0 10 01 PWM Low-true pulse > > 144 * 0 10 11 PWM Reserved > > 145 * > > 146 * High-true pulse: clear output on counter match, set output on > > 147 * counter reload, set output when counter first enabled or paused. > > 148 * > > 149 * Low-true pulse: set output on counter match, clear output on > > 150 * counter reload, clear output when counter first enabled or > paused. > > 151 */ > > I stumbled over "high-true" and "low-true" in the RM, too. In my bubble this > is an uncommon wording. I'd write instead: > > /* > * set polarity > * > * ELSB:ELSA = 2b10 yields normal polarity behaviour, ELSB:ELSA > * = 2b01 yields inversed polarity. The other values are > * reserved. > */ > > And don't write about CPWM, MSA and MSB which are always used with > fixed values anyhow in the driver. > OK. > > > > + /* disable channel */ > > > > + writel(PWM_IMX_TPM_CnSC_CHF, > > > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > > > > > Clearing CHF doens't disable the channel as I read the manual. > > > > This write clears CHF as well as writing other bits 0, to disable the > > output. Maybe I can explicitly clear MSA/MSB/ELSA/ELSB to avoid > confusion. > > Ah, I misinterpreted the value written. > > > > > +static int pwm_imx_tpm_apply(struct pwm_chip *chip, struct > pwm_device *pwm, > > > > + struct pwm_state *state) > > > > +{ > > > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > > > + struct pwm_state curstate; > > > > + u32 duty_cycle = state->duty_cycle; > > > > + int ret; > > > > + > > > > + pwm_imx_tpm_get_state(chip, pwm, &curstate); > > > > + > > > > + mutex_lock(&tpm->lock); > > > > > > What should this lock protect? Does it hurt if the state changes > > > between pwm_imx_tpm_get_state releasing the lock and > > > pwm_imx_tpm_apply taking it? > > > > The idea is to protect the share resourced by multiple channels, but I > > think I can make the mutex_lock includes get_state and remove the lock in > get_state function. > A > You might need it in .get_state to return a consistent state to the caller. In > this case just introduce an unlocked variant of .get_state to share code > between the two functions. > > And BTW the question was honest. I'm not entirely sure that you need to > hold the lock. Agreed, if the different channel configuration ONLY access its own register, NOT any shared registers, then I think this lock is unnecessary. > > > > > + */ > > > > + if (tpm->user_count != 1) > > > > + return -EBUSY; > > > > > > Ideally if say period = 37 is requested but currently we have period > > > = > > > 36 and configuring 37 would result in 36 anyhow, don't return EBUSY. > > > > I think here the protection is just for making sure that is there are > > multiple users, period can NOT be changed, since all channels will be > impacted. > > I think you misunderstood what I intended to say here. > > Consider that in the case that there is only a single PWM in use configuring > for a period of 37 ns actually yields 36 ns because the hardware cannot > provide 37 ns and 36 ns is the best match. > > Then if a second user comes and requests 37 ns, the result here is, that the > second user gets the -EBUSY. This is ridiculous however because the request > is denied even though the period is already configured for the length that > would be configured if the second user were the only one. Ah, if I understand correctly, I think I can change it to, if second user try to set a period different from first user, then return -EBUSY, otherwise, return success. > > > > > + tpm->chip.dev = &pdev->dev; > > > > + tpm->chip.ops = &imx_tpm_pwm_ops; > > > > + tpm->chip.base = -1; > > > > + tpm->chip.npwm = PWM_IMX_TPM_MAX_CHANNEL_NUM; > > > > > > This is wrong, as some only have 2 channels? > > > > I saw we can get channel number from register, will read register to > > determine the channel number, but for the channel config and status > > saved in struct, I will still use the MAX channel number to define the array. > > Yeah, that is sensible. I will resend V5 patch set, since there are some mis-understanding for previous comments. Thanks, Anson. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.p > engutronix.de%2F&data=02%7C01%7Canson.huang%40nxp.com%7C31 > d10d3f4a7c46eea9d708d6ab78d965%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636884932859808816&sdata=LD2D%2BwEhNKFllK2FaI > Wvjttmre0YPV%2BXwv7sdvkZSpo%3D&reserved=0 |
