Re: [PATCH v2 2/3] ECI: introducing ECI bus driver

[Peter Ujfalusi <peter.ujfalusi@nokia.com>]

Hi,

On 12/30/10 15:36, ext tapio.vihuri@nokia.com wrote:
> From: Tapio Vihuri <tapio.vihuri@nokia.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