On Fri, 2019-01-18 at 08:59 +0100, Uwe Kleine-König wrote: > Hello, > > On Fri, Jan 18, 2019 at 11:24:41AM +0800, Ryder Lee wrote: > > This adds a property "mediatek,num-pwms" to avoid having an endless > > list of compatibles with no differences for the same driver. > > > > Thus, the driver should have backwards compatibility to older DTs. > > I still think Thierry should bless "num-pwms" without vendor prefix.
Okay. > > Signed-off-by: Ryder Lee <[email protected]> > > --- > > Changes since v1: add some checks for backwards compatibility. > > --- > > drivers/pwm/pwm-mediatek.c | 27 ++++++++++++++++++--------- > > 1 file changed, 18 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c > > index eb6674c..81b7e5e 100644 > > --- a/drivers/pwm/pwm-mediatek.c > > +++ b/drivers/pwm/pwm-mediatek.c > > @@ -55,7 +55,7 @@ enum { > > }; > > > > struct mtk_pwm_platform_data { > > Unrelated to this patch: This name is bad. This struct is not used as > platform_data and so should better be named mtk_pwm_of_data. While at > criticizing existing stuff: I'd prefer pwm_mediatek as common prefix to > match the filename. I think we can take care about that in another patch. > > - unsigned int num_pwms; > > + unsigned int num_pwms; /* it should not be used in the future SoCs */ > > I'd drop this comment in favour of a runtime warning. Sorry, I can't get you here. > > bool pwm45_fixup; > > bool has_clks; > > }; > > @@ -226,27 +226,36 @@ static void mtk_pwm_disable(struct pwm_chip *chip, > > struct pwm_device *pwm) > > > > static int mtk_pwm_probe(struct platform_device *pdev) > > { > > - const struct mtk_pwm_platform_data *data; > > + struct device_node *np = pdev->dev.of_node; > > struct mtk_pwm_chip *pc; > > struct resource *res; > > - unsigned int i; > > + unsigned int i, num_pwms; > > int ret; > > > > pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > > if (!pc) > > return -ENOMEM; > > > > - data = of_device_get_match_data(&pdev->dev); > > - if (data == NULL) > > - return -EINVAL; > > - pc->soc = data; > > + pc->soc = of_device_get_match_data(&pdev->dev); > > This might return NULL which ... The only way to call probe() is to match an entry in mtk_pwm_of_match[], so match cannot be NULL. > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > pc->regs = devm_ioremap_resource(&pdev->dev, res); > > if (IS_ERR(pc->regs)) > > return PTR_ERR(pc->regs); > > > > - for (i = 0; i < data->num_pwms + 2 && pc->soc->has_clks; i++) { > > + /* Check if we have set 'num-pwms' in DTs. */ > > + ret = of_property_read_u32(np, "mediatek,num-pwms", &num_pwms); > > + if (ret < 0) { > > + /* If no, fallback to use SoC data for backwards compatibility. > > */ > > + if (pc->soc->num_pwms) { > > ... here then results in a NULL pointer dereference. I think you want So we have no chance to get a NULL pointer, right? > if (pc->soc) > > here. > > > + num_pwms = pc->soc->num_pwms; > > + } else { > > + dev_err(&pdev->dev, "failed to get pwm number: %d\n", > > ret); > > + return ret; > > + } > > + } > > + > > + for (i = 0; i < num_pwms + 2 && pc->soc->has_clks; i++) { > > pc->clks[i] = devm_clk_get(&pdev->dev, mtk_pwm_clk_name[i]); > > if (IS_ERR(pc->clks[i])) { > > dev_err(&pdev->dev, "clock: %s fail: %ld\n", > > @@ -260,7 +269,7 @@ static int mtk_pwm_probe(struct platform_device *pdev) > > pc->chip.dev = &pdev->dev; > > pc->chip.ops = &mtk_pwm_ops; > > pc->chip.base = -1; > > - pc->chip.npwm = data->num_pwms; > > + pc->chip.npwm = num_pwms; > > > > ret = pwmchip_add(&pc->chip); > > if (ret < 0) { > > Best regards > Uwe >
