lkml.org 
[lkml]   [2012]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] mfd: add TI TPS80031 mfd core driver
    On Mon, Nov 05, 2012 at 03:14:17PM +0530, Laxman Dewangan wrote:

    Looks pretty good, a few smaller issues though.

    > +static bool is_volatile_reg_id0(struct device *dev, unsigned int reg)
    > +{
    > + return false;
    > +}

    This is the default for all registers, you should not need to provide
    a function like this.

    > + dev_info(&client->dev, "Jtag version 0x%02x and Eprom version 0x%02x\n",
    > + jtag_ver, ep_ver);

    EPROM is an acronym. For TI devices I think normally the ES revision
    is referred to as just that, otherwise JTAG is an acronym too.

    > + if (client->irq) {
    > + ret = tps80031_irq_init(tps80031, client->irq, pdata->irq_base);
    > + if (ret) {
    > + dev_err(&client->dev, "IRQ init failed: %d\n", ret);
    > + goto fail_client_reg;
    > + }
    > + }

    Since for current kernel versions we're using irqdomains we can set up
    all the interrupts without an irq. It can make life easier to just
    always set up the interrupt controller so drivers don't need to worry
    about it.

    > + ret = mfd_add_devices(tps80031->dev, -1,
    > + tps80031_cell, ARRAY_SIZE(tps80031_cell),
    > + NULL, tps80031->irq_base,
    > + regmap_irq_get_domain(tps80031->irq_data));

    If you're supplying a domain you shouldn't need to provide an irq_base,
    the domain fulfuls the same role better.

    > +static int __devexit tps80031_remove(struct i2c_client *client)
    > +{
    > + struct tps80031 *tps80031 = i2c_get_clientdata(client);
    > + int i;
    > +
    > + mfd_remove_devices(tps80031->dev);
    > +
    > + if (client->irq)
    > + free_irq(client->irq, tps80031);

    regmap_irq_exit()

    > +#ifdef CONFIG_PM_SLEEP
    > +static int tps80031_suspend(struct device *dev)
    > +{
    > + struct i2c_client *client = to_i2c_client(dev);
    > +
    > + if (client->irq)
    > + disable_irq(client->irq);

    The interrupt core should take care of this for you if it's important,
    and note that people often use things like RTC alarms to wake the
    system.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-11-05 12:02    [W:3.064 / U:0.968 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site