| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Mar 2019 13:22:50 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Paul Cercueil <paul@...pouillou.net>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ralf Baechle <ralf@...ux-mips.org>,
Paul Burton <paul.burton@...s.com>,
James Hogan <jhogan@...nel.org>,
Jonathan Corbet <corbet@....net>,
Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>,
Mathieu Malaterre <malat@...ian.org>, od@...c.me,
linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-watchdog@...r.kernel.org,
linux-mips@...r.kernel.org, linux-doc@...r.kernel.org,
linux-clk@...r.kernel.org
Subject: Re: [PATCH v10 04/27] clocksource: Add a new timer-ingenic driver
On Sat, Mar 02, 2019 at 08:33:50PM -0300, Paul Cercueil wrote:
[...]
> diff --git a/drivers/clocksource/ingenic-timer.c b/drivers/clocksource/ingenic-timer.c
[...]
> +struct ingenic_tcu {
> + const struct ingenic_soc_info *soc_info;
> + struct regmap *map;
> + struct clk *clk, *timer_clk, *cs_clk;
> +
> + struct irq_domain *domain;
> + unsigned int nb_parent_irqs;
> + u32 parent_irqs[3];
> +
> + struct clk_hw_onecell_data *clocks;
> +
> + unsigned int timer_channel, cs_channel;
> + struct clock_event_device cevt;
> + struct clocksource cs;
> + char name[4];
> +
> + unsigned long pwm_channels_mask;
> +};
> +
> +static struct ingenic_tcu *ingenic_tcu;
Is there really a need for this? Seems like the only two reasons for why
you keep this around are as pointed out below.
> +void __iomem *ingenic_tcu_base;
> +EXPORT_SYMBOL_GPL(ingenic_tcu_base);
Same here.
> +static u64 notrace ingenic_tcu_timer_read(void)
> +{
> + unsigned int channel = ingenic_tcu->cs_channel;
> + u16 count;
> +
> + count = readw(ingenic_tcu_base + TCU_REG_TCNTc(channel));
Can't yo do this via the regmap?
> +
> + return count;
> +}
> +
> +static u64 notrace ingenic_tcu_timer_cs_read(struct clocksource *cs)
> +{
> + return ingenic_tcu_timer_read();
> +}
And here you've got access to the struct clocksource *, from which you
should be able to get at struct ingenic_tcu * using container_of.
[...]
> +static int __init ingenic_tcu_init(struct device_node *np)
> +{
> + const struct of_device_id *id = of_match_node(ingenic_tcu_of_match, np);
> + const struct ingenic_soc_info *soc_info = id->data;
> + struct ingenic_tcu *tcu;
> + struct resource res;
> + void __iomem *base;
> + long rate;
> + int ret;
> +
> + of_node_clear_flag(np, OF_POPULATED);
> +
> + tcu = kzalloc(sizeof(*tcu), GFP_KERNEL);
> + if (!tcu)
> + return -ENOMEM;
> +
> + /* Enable all TCU channels for PWM use by default except channel 0
> + * and channel 1.
> + */
> + tcu->pwm_channels_mask = GENMASK(soc_info->num_channels - 1, 2);
> + of_property_read_u32(np, "ingenic,pwm-channels-mask",
> + (u32 *)&tcu->pwm_channels_mask);
> +
> + /* Verify that we have at least two free channels */
> + if (hweight8(tcu->pwm_channels_mask) > soc_info->num_channels - 2) {
> + pr_crit("ingenic-tcu: Invalid PWM channel mask: 0x%02lx\n",
> + tcu->pwm_channels_mask);
> + return -EINVAL;
> + }
> +
> + tcu->soc_info = soc_info;
> + ingenic_tcu = tcu;
> +
> + tcu->timer_channel = find_first_zero_bit(&tcu->pwm_channels_mask,
> + soc_info->num_channels);
> + tcu->cs_channel = find_next_zero_bit(&tcu->pwm_channels_mask,
> + soc_info->num_channels,
> + tcu->timer_channel + 1);
> +
> + base = of_io_request_and_map(np, 0, NULL);
> + if (IS_ERR(base)) {
> + ret = PTR_ERR(base);
> + goto err_free_ingenic_tcu;
> + }
> +
> + of_address_to_resource(np, 0, &res);
> +
> + ingenic_tcu_base = base;
> +
> + tcu->map = regmap_init_mmio(NULL, base, &ingenic_tcu_regmap_config);
> + if (IS_ERR(tcu->map)) {
> + ret = PTR_ERR(tcu->map);
> + goto err_iounmap;
> + }
> +
> + tcu->clk = of_clk_get_by_name(np, "tcu");
> + if (IS_ERR(tcu->clk)) {
> + ret = PTR_ERR(tcu->clk);
> + pr_crit("ingenic-tcu: Unable to find TCU clock: %i\n", ret);
> + goto err_free_regmap;
> + }
> +
> + ret = clk_prepare_enable(tcu->clk);
> + if (ret) {
> + pr_crit("ingenic-tcu: Unable to enable TCU clock: %i\n", ret);
> + goto err_clk_put;
> + }
> +
> + ret = ingenic_tcu_intc_init(tcu, np);
> + if (ret)
> + goto err_clk_disable;
> +
> + ret = ingenic_tcu_clk_init(tcu, np);
> + if (ret)
> + goto err_tcu_intc_cleanup;
> +
> + ret = ingenic_tcu_clocksource_init(tcu);
> + if (ret)
> + goto err_tcu_clk_cleanup;
> +
> + ret = ingenic_tcu_timer_init(tcu);
> + if (ret)
> + goto err_tcu_clocksource_cleanup;
> +
> + // Register the sched_clock at the very end as there's no way to undo it
> + rate = clk_get_rate(tcu->cs_clk);
> + sched_clock_register(ingenic_tcu_timer_read, 16, rate);
Oh wow... so you managed to nicely encapsulate everything and now this
seems to be the only reason why you need to rely on global variables.
That's unfortunate. I suppose we could go and add a void *data parameter
to sched_clock_register() and pass that to the read() function. That way
you could make this completely independent of global variables, but
there are 73 callers of sched_clock_register() and they are spread all
over the place, so sounds a bit daunting to me.
> +
> + return 0;
> +
> +err_tcu_clocksource_cleanup:
> + ingenic_tcu_clocksource_cleanup(tcu);
> +err_tcu_clk_cleanup:
> + ingenic_tcu_clk_cleanup(tcu, np);
> +err_tcu_intc_cleanup:
> + ingenic_tcu_intc_cleanup(tcu);
> +err_clk_disable:
> + clk_disable_unprepare(tcu->clk);
> +err_clk_put:
> + clk_put(tcu->clk);
> +err_free_regmap:
> + regmap_exit(tcu->map);
> +err_iounmap:
> + iounmap(base);
> + release_mem_region(res.start, resource_size(&res));
> +err_free_ingenic_tcu:
> + kfree(tcu);
> + return ret;
> +}
> +
> +TIMER_OF_DECLARE(jz4740_tcu_intc, "ingenic,jz4740-tcu", ingenic_tcu_init);
> +TIMER_OF_DECLARE(jz4725b_tcu_intc, "ingenic,jz4725b-tcu", ingenic_tcu_init);
> +TIMER_OF_DECLARE(jz4770_tcu_intc, "ingenic,jz4770-tcu", ingenic_tcu_init);
> +
> +
> +static int __init ingenic_tcu_probe(struct platform_device *pdev)
> +{
> + platform_set_drvdata(pdev, ingenic_tcu);
Then there's also this. Oh well... nevermind then.
> +bool ingenic_tcu_pwm_can_use_chn(unsigned int channel)
> +{
> + return !!(ingenic_tcu->pwm_channels_mask & BIT(channel));
> +}
> +EXPORT_SYMBOL_GPL(ingenic_tcu_pwm_can_use_chn);
I wonder if we could make this slightly nicer, though. Judging by the
name, this one seems like it would only ever be used by the PWM. If the
PWM is a child of the TCU, could it not pass in a struct device * that
points to its parent into this? Or perhaps pass its own struct device *
and have this function look up the struct ingenic_tcu from that? Should
be something trivial as:
bool ingenic_tcu_pwm_can_use_channel(struct device *dev, unsigned int channel)
{
struct ingenic_tcu *tcu = dev_get_drvdata(dev->parent);
return !!(tcu->pwm_channels_mask & BIT(channel));
}
Thierry
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists