| 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
| ||
|
Message-ID: <4D218BEC.7010503@nokia.com>
Date: Mon, 03 Jan 2011 10:42:20 +0200
From: Peter Ujfalusi <peter.ujfalusi@...ia.com>
To: "ext tapio.vihuri@...ia.com" <tapio.vihuri@...ia.com>
CC: dmitry.torokhov@...il.com, randy.dunlap@...cle.com,
alsa-devel@...a-project.org, ilkka.koskinen@...ia.com,
linux-kernel@...r.kernel.org, samu.p.onkalo@...ia.com
Subject: Re: [alsa-devel] [PATCH v2 2/3] ECI: introducing ECI bus driver
Hi,
On 12/30/10 15:36, ext tapio.vihuri@...ia.com wrote:
> From: Tapio Vihuri <tapio.vihuri@...ia.com>
>
> ECI bus controller is kind of bridge between host CPU I2C and ECI accessory
> ECI communication.
It is kind of misleading to call this driver as a bus driver IMHO.
For what I gathered, this is a driver for a micro-controller, which
implements the ECI communication.
This could be a generic driver for the micro-controller, but two lines
makes it x86 specific...
...
> +#include <asm/intel_scu_ipc.h>
...
> +static int __init ecibus_probe(struct platform_device *pdev)
> +{
> + struct ecibus_data *ed;
> + struct ecibus_platform_data *pdata = pdev->dev.platform_data;
> + struct i2c_adapter *adapter;
> + int ret;
> +
> + if (!pdata) {
> + dev_err(&pdev->dev, "platform_data not available: %d\n",
> + __LINE__);
> + return -EINVAL;
> + }
> +
> + ed = kzalloc(sizeof(*ed), GFP_KERNEL);
> + if (!ed)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ed);
> + ed->dev = &pdev->dev;
> + ed->ecibus_rst_gpio = pdata->ecibus_rst_gpio;
> + ed->ecibus_sw_ctrl_gpio = pdata->ecibus_sw_ctrl_gpio;
> + ed->ecibus_int_gpio = pdata->ecibus_int_gpio;
> +
> + if (ed->ecibus_rst_gpio == 0 || ed->ecibus_sw_ctrl_gpio == 0 ||
> + ed->ecibus_int_gpio == 0) {
> + ret = -ENXIO;
> + goto gpio_err;
> + }
> +
> + the_ed = ed;
> +
> + adapter = i2c_get_adapter(1);
What happens if the mc is on another bus?
Shall you have platform data for this?
Or even better, use i2c device/driver probing for this driver?
> + if (adapter)
> + ed->client = i2c_new_device(adapter, &ecibus_i2c);
> + if (!ed->client) {
> + dev_err(ed->dev, "could not get i2c device %s at 0x%02x: %d\n",
> + ecibus_i2c.type, ecibus_i2c.addr, __LINE__);
> + kfree(ed);
> +
> + return -ENODEV;
> + }
> +
> + ret = ecibus_initialize_debugfs(ed);
> + if (ret)
> + dev_err(ed->dev, "could not create debugfs entries: %d\n",
> + __LINE__);
> +
> + ret = gpio_request(ed->ecibus_rst_gpio, "ECI_RSTn");
> + if (ret) {
> + dev_err(ed->dev, "could not request ECI_RSTn gpio %d: %d\n",
> + ed->ecibus_rst_gpio, __LINE__);
> + goto rst_gpio_err;
> + }
> +
> + gpio_direction_output(ed->ecibus_rst_gpio, 0);
> + gpio_set_value(ed->ecibus_rst_gpio, 1);
> +
> + ret = gpio_request(ed->ecibus_sw_ctrl_gpio, "ECI_SW_CTRL");
> + if (ret) {
> + dev_err(ed->dev, "could not request ECI_SW_CTRL gpio %d: %d\n",
> + ed->ecibus_sw_ctrl_gpio, __LINE__);
> + goto sw_ctrl_gpio_err;
> + }
> +
> + gpio_direction_input(ed->ecibus_sw_ctrl_gpio);
> +
> + ret = gpio_request(ed->ecibus_int_gpio, "ECI_INT");
> + if (ret) {
> + dev_err(ed->dev, "could not request ECI_INT gpio %d: %d\n",
> + ed->ecibus_int_gpio, __LINE__);
> + goto int_gpio_err;
> + }
> +
> + gpio_direction_input(ed->ecibus_int_gpio);
> +
> + ret = request_threaded_irq(gpio_to_irq(ed->ecibus_int_gpio), NULL,
> + ecibus_irq_handler, IRQF_TRIGGER_RISING, "ECI_INT", ed);
> + if (ret) {
> + dev_err(ed->dev, "could not request irq %d: %d\n",
> + gpio_to_irq(ed->ecibus_int_gpio), __LINE__);
> + goto int_irq_err;
> + }
> +
> + /* Register itself to the ECI accessory driver */
> + ed->eci_callback = eci_register(&ecibus_hw_ops);
> + if (IS_ERR(ed->eci_callback)) {
> + ret = PTR_ERR(ed->eci_callback);
> + goto eci_register_err;
> +
> + }
> +
> + /*
> + * Turn on vprog2
> + * Some decent power ctrl interface, please
> + */
> + intel_scu_ipc_iowrite8(AvP_MSIC_VPROG2, AvP_MSIC_VPROG2_2V5_ON);
Is this some sort of power enable for the controller itself?
If it is, why not use a callback from the platform to enable/disable the
power, so the driver will be platform independent, and you do this intel
specific thing in arch/ platform code.
If this is not related to the mc itself, then this shall not be part of
this driver.
...
> +static int __exit ecibus_remove(struct platform_device *pdev)
> +{
> + struct ecibus_data *ed = platform_get_drvdata(pdev);
> +
> + gpio_set_value(ed->ecibus_rst_gpio, 0);
> + gpio_free(ed->ecibus_rst_gpio);
> +
> + gpio_free(ed->ecibus_sw_ctrl_gpio);
> +
> + free_irq(gpio_to_irq(ed->ecibus_int_gpio), ed);
> + gpio_free(ed->ecibus_int_gpio);
> +
> + ecibus_uninitialize_debugfs();
> + i2c_unregister_device(ed->client);
> +
> + /*
> + * Turn off vprog2
> + * Some decent power ctrl interface, please
> + */
> + intel_scu_ipc_iowrite8(AvP_MSIC_VPROG2, AvP_MSIC_VPROG2_2V5_OFF);
Same applies here for the intel dependency.
--
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists