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. > 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. > - 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. > 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 ... > > 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 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
