Hi Thierry, On Tue, 18 Nov 2014 16:00:23 +0100 Thierry Reding <[email protected]> wrote:
> On Tue, Nov 18, 2014 at 01:40:53PM +0100, Boris Brezillon wrote: > > at91sam9x5 has an errata forbidding the use of slow clk as a clk source and > > sama5d3 SoCs has another errata forbidding the use of div1 prescaler. > > > > Take both of these erratas into account. > > > > Signed-off-by: Boris Brezillon <[email protected]> > > --- > > drivers/pwm/pwm-atmel-hlcdc.c | 28 +++++++++++++++++++++++----- > > 1 file changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c > > index eaf8b12..405f8a5 100644 > > --- a/drivers/pwm/pwm-atmel-hlcdc.c > > +++ b/drivers/pwm/pwm-atmel-hlcdc.c > > @@ -36,6 +36,8 @@ struct atmel_hlcdc_pwm { > > struct pwm_chip chip; > > struct atmel_hlcdc *hlcdc; > > struct clk *cur_clk; > > + bool slow_clk_errata; > > + bool div1_clk_errata; > > }; > > > > static inline struct atmel_hlcdc_pwm *to_atmel_hlcdc_pwm(struct pwm_chip > > *chip) > > @@ -56,20 +58,28 @@ static int atmel_hlcdc_pwm_config(struct pwm_chip *c, > > u32 pwmcfg; > > int pres; > > > > - clk_freq = clk_get_rate(new_clk); > > - clk_period_ns = (u64)NSEC_PER_SEC * 256; > > - do_div(clk_period_ns, clk_freq); > > + if (!chip->slow_clk_errata) { > > + clk_freq = clk_get_rate(new_clk); > > + clk_period_ns = (u64)NSEC_PER_SEC * 256; > > + do_div(clk_period_ns, clk_freq); > > + } > > > > - if (clk_period_ns > period_ns) { > > + /* Errata: cannot use slow clk on some IP revisions */ > > + if (chip->slow_clk_errata || clk_period_ns > period_ns) { > > new_clk = hlcdc->sys_clk; > > clk_freq = clk_get_rate(new_clk); > > clk_period_ns = (u64)NSEC_PER_SEC * 256; > > do_div(clk_period_ns, clk_freq); > > } > > > > - for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) > > + for (pres = 0; pres <= ATMEL_HLCDC_PWMPS_MAX; pres++) { > > + /* Errata: cannot divide by 1 on some IP revisions */ > > + if (!pres && chip->div1_clk_errata) > > + continue; > > + > > if ((clk_period_ns << pres) >= period_ns) > > break; > > + } > > > > if (pres > ATMEL_HLCDC_PWMPS_MAX) > > return -EINVAL; > > @@ -204,6 +214,14 @@ static int atmel_hlcdc_pwm_probe(struct > > platform_device *pdev) > > if (ret) > > return ret; > > > > + if (of_device_is_compatible(dev->parent->of_node, > > + "atmel,sama5d3-hlcdc")) > > + chip->div1_clk_errata = true; > > + > > + if (of_device_is_compatible(dev->parent->of_node, > > + "atmel,at91sam9x5-hlcdc")) > > + chip->slow_clk_errata = true; > > Generally I'd prefer this to be done as "SoC data", where the idea is to > not rely on these checks at probe time (because they don't scale very > well in the long term). Somewhat like the following: > > struct atmel_hlcdc_soc { > bool slow_clk_errata; > bool div1_clk_errata; > }; > > static const struct atmel_hlcdc_soc sama5d3_hlcdc = { > .slow_clk_errata = false, > .div1_clk_errata = true, > }; > > static const struct atmel_hlcdc_soc at91sam9x5_hlcdc = { > .slow_clk_errata = true, > .div1_clk_errata = false, > }; > > Then put those into a struct of_device_id table and do the matching > using of_match_device(). Then you can simply do something like: > > match = of_match_device(dev->parent, atmel_hlcdc_matches); > if (match) > chip->soc = match->data; I'm perfectly with using device_id data field to achieve that. I'll change that for my next version. > > And then check on the fields set therein. This works optimally if the > device isn't a subdevice because you have most of that code anyway. In > this particular case the lookup needs to happen on the parent, which > isn't so nice. > > I'm willing to let this go for now, but if this list is going to grow > I'll request that it be done differently so that .probe() doesn't need > to be cluttered. > > Also I wonder if it would be better to just add these new compatibles to > the PWM block binding, too, since it's obviously the IP blocks that have > these errata rather than the HLCDC block. So technically I think you'd > have to make the driver support atmel,at91sam9x5-hlcdc-pwm and > atmel,sama5d3-hlcdc-pwm anyway, and then you could just as well move to > the above matching and SoC data. Well, actually I did it because the datasheet describe the HLCDC IP not the HLCDC-PWM and HLCDC-DC IPs (I mean, IP revision is attached to the HLCDC block, not the HLCDC-PWM and HLCDC-DC sub devices). Anyway, I'm okay to move MFD compatible string to the sub-device nodes, but if I do so, I'm not sure it makes sense to have a specific compatible string for the parent device ("atmel-hlcdc" would do the job). Another point in favor of the "one compatible string for all sub-device revisions" is that it limits the number of mfd cells declared in the MFD driver. Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
