Hi,Uwe Best Regards! Anson Huang > -----Original Message----- > From: Uwe Kleine-König [mailto:u.kleine-koenig@xxxxxxxxxxxxxx] > Sent: 2019年3月15日 17:36 > 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 Fri, Mar 15, 2019 at 12:46:51AM +0000, Anson Huang wrote: > > i.MX7ULP has TPM(Low Power Timer/Pulse Width Modulation Module) > > inside, add TPM PWM driver support. > > > > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx> > > --- > > Changes since V3: > > - use "PWM_IMX_" as macro definition prefix and "pwm_imx_" as > function prefix; > > - improve the limitation txt; > > - return error for configuring period/prescale fail; > > - disable clock when driver probe failed and remove; > > - improve module build dependency; > > - introduce user_count to determine whether configuing period is > allowed; > > - some logic improvement for setting duty/status etc.; > > --- > > drivers/pwm/Kconfig | 12 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-imx-tpm.c | 396 > > ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 409 insertions(+) > > create mode 100644 drivers/pwm/pwm-imx-tpm.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index > > a8f47df..6117fe6 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -201,6 +201,18 @@ config PWM_IMX > > To compile this driver as a module, choose M here: the module > > will be called pwm-imx. > > > > +config PWM_IMX_TPM > > + tristate "i.MX TPM PWM support" > > + depends on ARCH_MXC || COMPILE_TEST > > + depends on HAVE_CLK && HAS_IOMEM > > + > > I think this empty newline is unusual. Agreed. > > > + help > > + Generic PWM framework driver for i.MX7ULP TPM module, TPM's > full > > + name is Low Power Timer/Pulse Width Modulation Module. > > + > > + To compile this driver as a module, choose M here: the module > > + will be called pwm-imx-tpm. > > + > > config PWM_JZ4740 > > tristate "Ingenic JZ47xx PWM support" > > depends on MACH_INGENIC > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index > > 9c676a0..64e036c 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -18,6 +18,7 @@ obj-$(CONFIG_PWM_FSL_FTM) += pwm-fsl-ftm.o > > obj-$(CONFIG_PWM_HIBVT) += pwm-hibvt.o > > obj-$(CONFIG_PWM_IMG) += pwm-img.o > > obj-$(CONFIG_PWM_IMX) += pwm-imx.o > > +obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > diff --git a/drivers/pwm/pwm-imx-tpm.c b/drivers/pwm/pwm-imx-tpm.c > new > > file mode 100644 index 0000000..f108f75 > > --- /dev/null > > +++ b/drivers/pwm/pwm-imx-tpm.c > > @@ -0,0 +1,396 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright 2018-2019 NXP. > > + * > > + * Limitations: > > + * - The TPM counter and period counter are shared between > > + * multiple channels, so all channels should use same period > > + * settings. > > + */ > > + > > +#include <linux/bitops.h> > > +#include <linux/clk.h> > > +#include <linux/err.h> > > +#include <linux/io.h> > > +#include <linux/log2.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/slab.h> > > + > > +#define PWM_IMX_TPM_GLOBAL 0x8 > > +#define PWM_IMX_TPM_SC 0x10 > > +#define PWM_IMX_TPM_CNT 0x14 > > +#define PWM_IMX_TPM_MOD 0x18 > > +#define PWM_IMX_TPM_C0SC 0x20 > > +#define PWM_IMX_TPM_C0V 0x24 > > + > > +#define PWM_IMX_TPM_SC_CMOD GENMASK(4, 3) > > +#define PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK BIT(3) > > +#define PWM_IMX_TPM_SC_CPWMS BIT(5) > > + > > +#define PWM_IMX_TPM_CnSC_CHF BIT(7) > > +#define PWM_IMX_TPM_CnSC_MSnB BIT(5) > > +#define PWM_IMX_TPM_CnSC_MSnA BIT(4) > > +#define PWM_IMX_TPM_CnSC_ELSnB BIT(3) > > +#define PWM_IMX_TPM_CnSC_ELSnA BIT(2) > > + > > +#define PWM_IMX_TPM_SC_PS_MASK 0x7 > > +#define PWM_IMX_TPM_MOD_MOD_MASK 0xffff > > + > > +#define PWM_IMX_TPM_MAX_COUNT 0xffff > > + > > +#define PWM_IMX_TPM_MAX_CHANNEL_NUM 6 > > + > > +#define PWM_IMX_TPM_CnSC(n) (PWM_IMX_TPM_C0SC + n * 0x8) > > +#define PWM_IMX_TPM_CnV(n) (PWM_IMX_TPM_C0V + n * 0x8) > > parenthesis around n please. OK. > > > +struct imx_tpm_pwm_chip { > > + struct pwm_chip chip; > > + struct clk *clk; > > + void __iomem *base; > > + struct mutex lock; > > + u32 user_count; > > + u32 chn_config[PWM_IMX_TPM_MAX_CHANNEL_NUM]; > > + bool chn_status[PWM_IMX_TPM_MAX_CHANNEL_NUM]; > > +}; > > + > > +#define to_imx_tpm_pwm_chip(_chip) \ > > + container_of(_chip, struct imx_tpm_pwm_chip, chip) > > + > > +static int pwm_imx_tpm_config_counter(struct pwm_chip *chip, u32 > > +period) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 period_cnt, val, div, saved_cmod; > > + u64 tmp; > > + > > + tmp = clk_get_rate(tpm->clk); > > + tmp *= period; > > + val = DIV_ROUND_CLOSEST_ULL(tmp, NSEC_PER_SEC); > > + if (val < PWM_IMX_TPM_MAX_COUNT) > > <= ? Agreed. > > > + div = 0; > > + else > > + div = ilog2(roundup_pow_of_two(val / > > + (PWM_IMX_TPM_MAX_COUNT + 1))); > > + if (div > PWM_IMX_TPM_SC_PS_MASK) { > > #define PWM_IMX_TPM_SC_PS GENMASK(0, 2) > > if (!FIELD_FIT(PWM_IMX_TPM_SC_PS, div)) { > ... OK. > > > + dev_err(chip->dev, > > + "failed to find valid prescale value!\n"); > > + return -EINVAL; > > + } > > + > > + /* make sure counter is disabled for programming prescale */ > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + saved_cmod = val & PWM_IMX_TPM_SC_CMOD; > > saved_cmod = FIELD_GET(PWM_IMX_TPM_SC_CMOD, val) ? OK. > > > + 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. > > > + /* set TPM counter prescale */ > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + val &= ~PWM_IMX_TPM_SC_PS_MASK; > > + val |= div; > > val |= FIELD_PREP(PWM_IMX_TPM_SC_PS_MASK, div); OK. > > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + > > + /* > > + * set period counter: according to RM, the MOD register is > > + * updated immediately when CMOD[1:0] = 2b'00 (counter disabled). > > updated immediately after CMOD[1:0] = 2b'00 above > > > + */ > > + do_div(tmp, NSEC_PER_SEC); > > + period_cnt = DIV_ROUND_CLOSEST_ULL(tmp, 1 << div) > > The (partial) RHS is equivalent to > > (tmp + (1 << div) >> 1) >> div > > which might be cheaper for the CPU to calculate. OK > > > + & PWM_IMX_TPM_MOD_MOD_MASK; > > If it can happen, that this masking changes the result, this is an error that > needs handling. (And if not, drop it; maybe in favour of a > comment.) Will use below for error check: if (period_cnt > PWM_IMX_TPM_MOD_MOD) { dev_err(chip->dev, "failed to find valid period count!\n"); return -EINVAL; } > > > + writel(period_cnt, tpm->base + PWM_IMX_TPM_MOD); > > + > > + /* restore the clock mode */ > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + val |= saved_cmod; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + > > + return 0; > > +} > > + > > +static void pwm_imx_tpm_config(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + u32 period, > > + u32 duty_cycle, > > + enum pwm_polarity polarity) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 duty_cnt, val; > > + u64 tmp; > > + > > + /* 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. > > > + tmp *= duty_cycle; > > + duty_cnt = DIV_ROUND_CLOSEST_ULL(tmp, period); > > + writel(duty_cnt & PWM_IMX_TPM_MOD_MOD_MASK, > > + tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > Please align the 2nd line to the opening parenthesis. OK. > > > + > > + /* set polarity */ > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > + val &= ~(PWM_IMX_TPM_CnSC_ELSnB | > PWM_IMX_TPM_CnSC_ELSnA | > > + PWM_IMX_TPM_CnSC_MSnA); > > + val |= PWM_IMX_TPM_CnSC_MSnB; > > + val |= polarity ? PWM_IMX_TPM_CnSC_ELSnA : > PWM_IMX_TPM_CnSC_ELSnB; > > I'd recommend not hard coding here that PWM_POLARITY_NORMAL > evaluates to false. OK, I will add below compare for it: 157 val |= (polarity == PWM_POLARITY_NORMAL) ? 158 PWM_IMX_TPM_CnSC_ELSB : PWM_IMX_TPM_CnSC_ELSA; > > In the reference manual I have (Rev. F, 07/2017) MSnA is called MSA only. > Ditto for MSnB -> MSB, ELSnA -> ELSA, ELSnB -> ELSB. (Hmm, but it is not > entirely consistent. So Table 64-4 indeed has the 'n's.) I will remove the 'n's. > > 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. > > 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 */ > > > + /* > > + * polarity settings will enabled/disable output statue > > s/statue/status/ Fixed. > > > + * immediately, so here ONLY save the config and will be > > + * written into register when channel is enabled/disabled. > > s/will be written/write it/ Fixed. > > > + */ > > + tpm->chn_config[pwm->hwpwm] = val; > > +} > > A comment here about how and when the values written in > pwm_imx_tpm_config take effect would be good. OK. > > > +static void pwm_imx_tpm_enable(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + bool enable) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u32 val, i; > > + > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + if (enable) { > > + /* restore channel config */ > > + writel(tpm->chn_config[pwm->hwpwm], > > + tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > + > > + /* start TPM counter anyway */ > > + val |= PWM_IMX_TPM_SC_CMOD_INC_EVERY_CLK; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + } else { > > + /* > > + * When a channel is disabled, its polarity settings will be > > + * saved and its output will be disabled by clearing polarity > > + * setting, when channel is enabled, polarity settings will be > > + * restored and output will be enabled again. > > + */ > > + /* save channel config */ > > + tpm->chn_config[pwm->hwpwm] = readl(tpm->base + > > + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > Doesn't tpm->chn_config[pwm->hwpwm] already contain the right value? > Please align the 2nd line to the opening parenthesis. Ah, yes, no need to read it again. > > > + /* 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. > > > + for (i = 0; i < chip->npwm; i++) > > + if (i != pwm->hwpwm && tpm->chn_status[i]) > > If you set tpm->chn_status[i] = false before this loop, you don't have to care > for i != pwm->hwpwm. If you maintain an "enable count" you don't have to > loop at all. I think we can introduce a enable count for it. > > > + break; > > + if (i == chip->npwm) { > > + /* stop TPM counter since all channels are disabled > */ > > + val &= ~PWM_IMX_TPM_SC_CMOD; > > + writel(val, tpm->base + PWM_IMX_TPM_SC); > > + } > > + } > > + > > + /* update channel statue */ > > + tpm->chn_status[pwm->hwpwm] = enable; } > > + > > +static void pwm_imx_tpm_get_state(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + u64 tmp; > > + u32 val, rate; > > + > > + mutex_lock(&tpm->lock); > > + > > + /* get period */ > > + rate = clk_get_rate(tpm->clk); > > + tmp = readl(tpm->base + PWM_IMX_TPM_MOD); > > + val = readl(tpm->base + PWM_IMX_TPM_SC); > > + val &= PWM_IMX_TPM_SC_PS_MASK; > > + tmp *= (1 << val) * NSEC_PER_SEC; > > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > + > > + /* get duty cycle */ > > + tmp = readl(tpm->base + PWM_IMX_TPM_CnV(pwm->hwpwm)); > > + tmp *= (1 << val) * NSEC_PER_SEC; > > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate); > > + > > + /* get polarity */ > > + val = readl(tpm->base + PWM_IMX_TPM_CnSC(pwm->hwpwm)); > > + if (val & PWM_IMX_TPM_CnSC_ELSnA) > > + state->polarity = PWM_POLARITY_INVERSED; > > + else > > + state->polarity = PWM_POLARITY_NORMAL; > > + > > + /* get channel status */ > > + state->enabled = tpm->chn_status[pwm->hwpwm] ? true : false; > > + > > + mutex_unlock(&tpm->lock); > > +} > > + > > +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. > > > + > > + if (state->period != curstate.period) { > > + /* > > + * TPM counter is shared by multiple channels, so > > + * the prescale and period can NOT be modified when > > + * there are multiple channels used. > > s/the //; s/used/in use/ Fixed. > > > + */ > > + 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. > > > + ret = pwm_imx_tpm_config_counter(chip, state->period); > > + if (ret) > > + return ret; > > + } > > + > > + if (!state->enabled) > > + duty_cycle = 0; > > A comment above this block that explains why this is done would be great > (but see below). > > > + > > + if (state->duty_cycle != curstate.duty_cycle || > > + state->polarity != curstate.polarity) > > + pwm_imx_tpm_config(chip, pwm, > > + state->period, duty_cycle, state->polarity); > > + > > + if (state->enabled != curstate.enabled) > > + pwm_imx_tpm_enable(chip, pwm, state->enabled); > > This is a bit unintuitive I think. For example I had to think for a while why you > pass duty_cycle to pwm_imx_tpm_config() but check > state->duty_cycle in the if condition. I'd suggest: > > if (state->enabled == false) { > /* > * configure for duty_cycle == 0 here? Wait until this > * setting is active? > */ > if (curstate.enabled) > pwm_imx_tpm_enable(chip, pwm, false); > } else { > ... > > } OK. > > > > + > > + mutex_unlock(&tpm->lock); > > + > > + return 0; > > +} > > + > > +static int pwm_imx_tpm_request(struct pwm_chip *chip, struct > > +pwm_device *dev) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + > > + mutex_lock(&tpm->lock); > > + tpm->user_count++; > > + mutex_unlock(&tpm->lock); > > + > > + return 0; > > +} > > + > > +static void pwm_imx_tpm_free(struct pwm_chip *chip, struct > pwm_device > > +*dev) { > > + struct imx_tpm_pwm_chip *tpm = to_imx_tpm_pwm_chip(chip); > > + > > + mutex_lock(&tpm->lock); > > + tpm->user_count--; > > + mutex_unlock(&tpm->lock); > > +} > > + > > +static const struct pwm_ops imx_tpm_pwm_ops = { > > + .get_state = pwm_imx_tpm_get_state, > > + .request = pwm_imx_tpm_request, > > + .apply = pwm_imx_tpm_apply, > > + .free = pwm_imx_tpm_free, > > + .owner = THIS_MODULE, > > Can you please group "request" with "free"? The order as defined in struct > pwm_ops would be optimal. OK. > > > +}; > > + > > +static int pwm_imx_tpm_probe(struct platform_device *pdev) { > > + struct imx_tpm_pwm_chip *tpm; > > + struct resource *res; > > + int ret; > > + > > + tpm = devm_kzalloc(&pdev->dev, sizeof(*tpm), GFP_KERNEL); > > + if (!tpm) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, tpm); > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + tpm->base = devm_ioremap_resource(&pdev->dev, res); > > You can use devm_platform_ioremap_resource instead of the two previous > calls. OK, just notice that there is such API newly added. > > > + if (IS_ERR(tpm->base)) { > > + ret = PTR_ERR(tpm->base); > > + if (ret != -EPROBE_DEFER) > > + dev_err(&pdev->dev, "pwm ioremap failed %d\n", > ret); > > + return ret; > > + } > > + > > + tpm->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(tpm->clk)) { > > + ret = PTR_ERR(tpm->clk); > > + if (ret != -EPROBE_DEFER) > > + dev_err(&pdev->dev, "failed to get pwm clk %d\n", > ret); > > + return ret; > > + } > > + > > + ret = clk_prepare_enable(tpm->clk); > > + if (ret) { > > + dev_err(&pdev->dev, > > + "failed to prepare or enable clk %d\n", ret); > > + return ret; > > + } > > + > > + 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. > > > + mutex_init(&tpm->lock); > > + > > + ret = pwmchip_add(&tpm->chip); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to add pwm chip %d\n", ret); > > + clk_disable_unprepare(tpm->clk); > > + } > > + > > + return ret; > > +} > > + > > +static int pwm_imx_tpm_remove(struct platform_device *pdev) { > > + struct imx_tpm_pwm_chip *tpm = platform_get_drvdata(pdev); > > + > > + clk_disable_unprepare(tpm->clk); > > + > > + return pwmchip_remove(&tpm->chip); > > Wrong order. Before pwmchip_remove returns the PWM must stay > functional. Will make pwmchip_remove before clk disable. > > > +} > > + > > +static int __maybe_unused pwm_imx_tpm_suspend(struct device *dev) { > > + struct imx_tpm_pwm_chip *tpm = dev_get_drvdata(dev); > > + > > + clk_disable_unprepare(tpm->clk); > > You must not disable the clock if the PWM is in use. I will add the enable counter check for it. > > > + return 0; > > +} > > The time I want to/can spend on community review is over now for this week. > I didn't look at all details yet but I think it is still worth to send this mail out to > not make you bored :-) Also I think further thoughts by me will be eased if > you address my comments here first. Thanks for your time/patience, I will generate V5 after all your comments addressed. 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%7C23 > 698f5992914cbb461208d6a9299e8e%7C686ea1d3bc2b4c6fa92cd99c5c30163 > 5%7C0%7C0%7C636882393558954567&sdata=5tuFBU7w%2FAULwJGbn > BlOpxmd4JmQQO8wT2qsMgW5yXs%3D&reserved=0 |
