On Tue, 10 Jul 2007 12:23:06 +0530
"Trilok Soni" <[EMAIL PROTECTED]> wrote:
> Add Texas Instruments TWL92330/Menelaus Power Management chip driver.
> This includes voltage regulators, Dual slot memory card tranceivers and
> real-time clock(RTC).
>
> The support for RTC is integrated with this driver only; it is not separate
> module. Passes 'rtctest' on OMAP H4 EVM, other than lack of
> "periodic" (1/N second) IRQs. System wakeup alarms (from suspend-to-RAM)
> work too.
>
> The battery keeps the RTC active over power off, so once you set clock
> (rdate/ntpdate/etc, then "hwclock -w") then RTC_HCTOSYS at boot time
> will behave as expected.
>
> ...
>
> +
> +struct menelaus_chip {
> + struct mutex lock;
> + struct i2c_client *client;
> + struct work_struct work;
> +#ifdef CONFIG_RTC_DRV_TWL92330
> + struct rtc_device *rtc;
> + u8 rtc_control;
> + unsigned uie:1;
> +#endif
> + unsigned vcore_hw_mode:1;
uie and vcore_hw_mode will share the same memory word. Consequently, on
preemptible or SMP kernels, a simple
p->uie = 1;
can race with a simple
p->vcore_hw_mode = 1;
in a different thread of control, potentially corrupting the other value.
The same race can occur even on uniprocessor, non-preempt kernels if one of
these fields gets assigned to from interrupt/softirq/etc contexts.
Does your code have suitable locking to prevent such races? If not, I'd
suggest that these two fields be converted to int or char or whatever.
> + u8 mask1, mask2;
> + void (*handlers[16])(struct menelaus_chip *);
What determined the value of this hard-wired 16?
> + void (*mmc_callback)(void *data, u8 mask);
> + void *mmc_callback_data;
> +};
> +
>
> ...
>
> +/* Adds a handler for an interrupt. Does not run in interrupt context */
> +static int menelaus_add_irq_work(int irq,
> + void (*handler)(struct menelaus_chip *))
> +{
> + int ret = 0;
Unneeded initialisation.
> + mutex_lock(&the_menelaus->lock);
> + the_menelaus->handlers[irq] = handler;
> + ret = menelaus_enable_irq(irq);
> + mutex_unlock(&the_menelaus->lock);
> +
> + return ret;
> +}
> +
> +/* Removes handler for an interrupt */
> +static int menelaus_remove_irq_work(int irq)
> +{
> + int ret = 0;
Ditto
> + mutex_lock(&the_menelaus->lock);
> + ret = menelaus_disable_irq(irq);
> + the_menelaus->handlers[irq] = NULL;
> + mutex_unlock(&the_menelaus->lock);
> +
> + return ret;
> +}
> +
> +/*
> + * Gets scheduled when a card detect interrupt happens. Note that in some
> cases
> + * this line is wired to card cover switch rather than the card detect switch
> + * in each slot. In this case the cards are not seen by menelaus.
> + * FIXME: Add handling for D1 too
> + */
> +static void menelaus_mmc_cd_work(struct menelaus_chip *menelaus_hw)
> +{
> + int reg;
> + unsigned char card_mask = 0;
> +
> + reg = menelaus_read_reg(MENELAUS_MCT_PIN_ST);
> + if (reg < 0)
> + return;
> +
> + if (!(reg & 0x1))
> + card_mask |= (1 << 0);
> +
> + if (!(reg & 0x2))
> + card_mask |= (1 << 1);
The above looks like a slow way of doing
card_mask = (reg & 3) ^ 3;
But perhaps what you have is a little clearer.
> + if (menelaus_hw->mmc_callback)
> + menelaus_hw->mmc_callback(menelaus_hw->mmc_callback_data,
> + card_mask);
> +}
> +
> +/*
> + * Toggles the MMC slots between open-drain and push-pull mode.
> + */
> +int menelaus_set_mmc_opendrain(int slot, int enable)
> +{
> + int ret, val;
> +
> + if (slot != 1 && slot != 2)
> + return -EINVAL;
Can this happen?
> + mutex_lock(&the_menelaus->lock);
> + ret = menelaus_read_reg(MENELAUS_MCT_CTRL1);
> + if (ret < 0) {
> + mutex_unlock(&the_menelaus->lock);
> + return ret;
> + }
> + val = ret;
> + if (slot == 1) {
> + if (enable)
> + val |= 1 << 2;
> + else
> + val &= ~(1 << 2);
> + } else {
> + if (enable)
> + val |= 1 << 3;
> + else
> + val &= ~(1 << 3);
> + }
> + ret = menelaus_write_reg(MENELAUS_MCT_CTRL1, val);
> + mutex_unlock(&the_menelaus->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(menelaus_set_mmc_opendrain);
Nothing uses this export. We should remove the export until some code
which requires it is merged.
> +int menelaus_set_slot_sel(int enable)
> +{
> + int ret;
> +
> + mutex_lock(&the_menelaus->lock);
> + ret = menelaus_read_reg(MENELAUS_GPIO_CTRL);
> + if (ret < 0)
> + goto out;
> + ret |= 0x02;
> + if (enable)
> + ret |= 1 << 5;
> + else
> + ret &= ~(1 << 5);
> + ret = menelaus_write_reg(MENELAUS_GPIO_CTRL, ret);
> +out:
> + mutex_unlock(&the_menelaus->lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(menelaus_set_slot_sel);
Ditto.
> +EXPORT_SYMBOL(menelaus_set_mmc_slot);
Ditto on all ten of these new exports.
> +
> +int menelaus_register_mmc_callback(void (*callback)(void *data, u8
> card_mask),
> + void *data)
> +{
> + int ret = 0;
Unneeded intialisation.
Probably gcc just fixes this up, but it is poor practice: if someone later
comes along and modifies this code and forgets an initialisation path, your
needless initialisation here will suppress the "might be used
uninitialised" warning which they wanted to see.
> + the_menelaus->mmc_callback_data = data;
> + the_menelaus->mmc_callback = callback;
> + ret = menelaus_add_irq_work(MENELAUS_MMC_S1CD_IRQ,
> + menelaus_mmc_cd_work);
> + if (ret < 0)
> + return ret;
> + ret = menelaus_add_irq_work(MENELAUS_MMC_S2CD_IRQ,
> + menelaus_mmc_cd_work);
> + if (ret < 0)
> + return ret;
> + ret = menelaus_add_irq_work(MENELAUS_MMC_S1D1_IRQ,
> + menelaus_mmc_cd_work);
> + if (ret < 0)
> + return ret;
> + ret = menelaus_add_irq_work(MENELAUS_MMC_S2D1_IRQ,
> + menelaus_mmc_cd_work);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(menelaus_register_mmc_callback);
> +
>
> ...
>
> +EXPORT_SYMBOL(menelaus_unregister_mmc_callback);
If we decide to remove all these exports for now, we really should make the
symbols static as well. Until they are really used externally.
> +struct menelaus_vtg {
> + const char *name;
> + u8 vtg_reg;
> + u8 vtg_shift;
> + u8 vtg_bits;
> + u8 mode_reg;
> +};
> +
> +struct menelaus_vtg_value {
> + u16 vtg;
> + u16 val;
> +};
Please try to document each field of each struct with inlined comments.
> +static int menelaus_set_voltage(const struct menelaus_vtg *vtg, int mV,
> + int vtg_val, int mode)
> +{
> + int val, ret;
> + struct i2c_client *c = the_menelaus->client;
> +
> + mutex_lock(&the_menelaus->lock);
> + if (vtg == 0)
> + goto set_voltage;
Please use NULL for zero-valued pointers. The code as you have it here
will generate a sparse warning (Documentation/SubmitChecklist, point 9,
guys) so I end up getting sent a dopey fixup patch a few weeks later.
> + ret = menelaus_read_reg(vtg->vtg_reg);
> + if (ret < 0)
> + goto out;
> + val = ret & ~(((1 << vtg->vtg_bits) - 1) << vtg->vtg_shift);
> + val |= vtg_val << vtg->vtg_shift;
> +
> + dev_dbg(&c->dev, "Setting voltage '%s'"
> + "to %d mV (reg 0x%02x, val 0x%02x)\n",
> + vtg->name, mV, vtg->vtg_reg, val);
> +
> + ret = menelaus_write_reg(vtg->vtg_reg, val);
> + if (ret < 0)
> + goto out;
> +set_voltage:
> + ret = menelaus_write_reg(vtg->mode_reg, mode);
> +out:
> + mutex_unlock(&the_menelaus->lock);
> + if (ret == 0) {
> + /* Wait for voltage to stabilize */
> + msleep(1);
> + }
> + return ret;
> +}
>
> ...
>
> +
> +int menelaus_set_vcore_sw(unsigned int mV)
Please check whether all the newly-added global symbols in fact need to be
global.
> +{
> + int val, ret;
> + struct i2c_client *c = the_menelaus->client;
> +
> + val = menelaus_get_vtg_value(mV, vcore_values,
> + ARRAY_SIZE(vcore_values));
> + if (val < 0)
> + return -EINVAL;
> +
> + dev_dbg(&c->dev, "Setting VCORE to %d mV (val 0x%02x)\n", mV, val);
> +
> + /* Set SW mode and the voltage in one go. */
> + mutex_lock(&the_menelaus->lock);
> + ret = menelaus_write_reg(MENELAUS_VCORE_CTRL1, val);
> + if (ret == 0)
> + the_menelaus->vcore_hw_mode = 0;
> + mutex_unlock(&the_menelaus->lock);
> + msleep(1);
It is not possible for the reader to determine why this msleep() is here
just by reading the code. Hence an explanatory comment is needed.
> + return ret;
> +}
> +
> +int menelaus_set_vcore_hw(unsigned int roof_mV, unsigned int floor_mV)
> +{
> + int fval, rval, val, ret;
> + struct i2c_client *c = the_menelaus->client;
> +
> + rval = menelaus_get_vtg_value(roof_mV, vcore_values,
> + ARRAY_SIZE(vcore_values));
> + if (rval < 0)
> + return -EINVAL;
> + fval = menelaus_get_vtg_value(floor_mV, vcore_values,
> + ARRAY_SIZE(vcore_values));
> + if (fval < 0)
> + return -EINVAL;
> +
> + dev_dbg(&c->dev, "Setting VCORE FLOOR to %d mV and ROOF to %d mV\n",
> + floor_mV, roof_mV);
> +
> + mutex_lock(&the_menelaus->lock);
> + ret = menelaus_write_reg(MENELAUS_VCORE_CTRL3, fval);
> + if (ret < 0)
> + goto out;
> + ret = menelaus_write_reg(MENELAUS_VCORE_CTRL4, rval);
> + if (ret < 0)
> + goto out;
> + if (!the_menelaus->vcore_hw_mode) {
> + val = menelaus_read_reg(MENELAUS_VCORE_CTRL1);
> + /* HW mode, turn OFF byte comparator */
> + val |= ((1 << 7) | (1 << 5));
> + ret = menelaus_write_reg(MENELAUS_VCORE_CTRL1, val);
> + the_menelaus->vcore_hw_mode = 1;
> + }
> + msleep(1);
Ditto, for all msleep()s, please.
> +out:
> + mutex_unlock(&the_menelaus->lock);
> + return ret;
> +}
> +
>
> ...
>
> +/*-----------------------------------------------------------------------*/
> +
> +/* Handles Menelaus interrupts. Does not run in interrupt context */
> +static void menelaus_work(struct work_struct *_menelaus)
> +{
> + struct menelaus_chip *menelaus =
> + container_of(_menelaus, struct menelaus_chip, work);
> + void (*handler)(struct menelaus_chip *menelaus);
> +
> + while (1) {
> + unsigned isr;
> +
> + isr = (menelaus_read_reg(MENELAUS_INT_STATUS2)
> + & ~menelaus->mask2) << 8;
> + isr |= menelaus_read_reg(MENELAUS_INT_STATUS1)
> + & ~menelaus->mask1;
> + if (!isr)
> + break;
> +
> + while (isr) {
> + int irq = fls(isr) - 1;
> + isr &= ~(1 << irq);
> +
> + mutex_lock(&menelaus->lock);
> + menelaus_disable_irq(irq);
> + menelaus_ack_irq(irq);
> + handler = menelaus->handlers[irq];
> + if (handler)
> + handler(menelaus);
> + menelaus_enable_irq(irq);
> + mutex_unlock(&menelaus->lock);
> + }
> + }
> + enable_irq(menelaus->client->irq);
This can do an enable_irq() of an already-enabled IRQ. I don't know if
that will cause runtime problems, but it doesn't seem desirable.
> +}
> +
> +/*
> + * We cannot use I2C in interrupt context, so we just schedule work.
> + */
> +static irqreturn_t menelaus_irq(int irq, void *_menelaus)
> +{
> + struct menelaus_chip *menelaus = _menelaus;
> +
> + disable_irq_nosync(irq);
I guess the reader of this code will wonder why the nosync variant was
used. A comment would explain that.
In fact the reader might wonder why the heck we use a workqueue at all,
rather than just doing the work in interrupt context as drivers normally
do.
Perhaps that was all explained somewhere and I missed it. If not, I'd
suggest that this design decision should be described in a code comment
somewhere.
> + (void)schedule_work(&menelaus->work);
Please remove the (void).
> + return IRQ_HANDLED;
> +}
>
> ...
>
> +#ifdef CONFIG_RTC_INTF_DEV
> +
> +static void menelaus_rtc_update_work(struct menelaus_chip *m)
> +{
> + /* report 1/sec update */
> + local_irq_disable();
> + rtc_update_irq(m->rtc, 1, RTC_IRQF | RTC_UF);
> + local_irq_enable();
> +}
ow. Is that local_irq_disable() assuming that this code will only ever run
on a uniprocessor machine?
If so, that's pretty poor, IMO. It would be better to design the code to
be SMP-capable from day one.
> +static int menelaus_ioctl(struct device *dev, unsigned cmd, unsigned long
> arg)
> +{
> + int status;
> +
> + if (the_menelaus->client->irq <= 0)
> + return -ENOIOCTLCMD;
> +
> + switch (cmd) {
> + /* alarm IRQ */
> + case RTC_AIE_ON:
> + if (the_menelaus->rtc_control & RTC_CTRL_AL_EN)
> + return 0;
> + the_menelaus->rtc_control |= RTC_CTRL_AL_EN;
> + break;
> + case RTC_AIE_OFF:
> + if (!(the_menelaus->rtc_control & RTC_CTRL_AL_EN))
> + return 0;
> + the_menelaus->rtc_control &= ~RTC_CTRL_AL_EN;
> + break;
> + /* 1/second "update" IRQ */
> + case RTC_UIE_ON:
> + if (the_menelaus->uie)
> + return 0;
> + status = menelaus_remove_irq_work(MENELAUS_RTCTMR_IRQ);
> + status = menelaus_add_irq_work(MENELAUS_RTCTMR_IRQ,
> + menelaus_rtc_update_work);
> + if (status == 0)
> + the_menelaus->uie = 1;
> + return status;
> + case RTC_UIE_OFF:
> + if (!the_menelaus->uie)
> + return 0;
> + status = menelaus_remove_irq_work(MENELAUS_RTCTMR_IRQ);
> + if (status == 0)
> + the_menelaus->uie = 0;
> + return status;
> + default:
> + return -ENOIOCTLCMD;
> + }
> + return menelaus_write_reg(MENELAUS_RTC_CTRL, the_menelaus->rtc_control);
> +}
> +
> +#else
> +#define menelaus_ioctl NULL
> +#endif
> +
> +/* REVISIT no compensation register support ... */
> +
> +static const struct rtc_class_ops menelaus_rtc_ops = {
> + .ioctl = menelaus_ioctl,
> + .read_time = menelaus_read_time,
> + .set_time = menelaus_set_time,
> + .read_alarm = menelaus_read_alarm,
> + .set_alarm = menelaus_set_alarm,
> +};
> +
>
> ...
>
> +static inline void menelaus_rtc_init(struct menelaus_chip *m)
> +{
The `inline' isn't needed here. If it has one callsite the compiler will
inline it for you. If we later gain a second callsite, your inline becomes
wrong: the function is far too large to be worth inlining at both callsites.
> + int alarm = (m->client->irq > 0);
> +
> + /* assume 32KDETEN pin is pulled high */
> + if (!(menelaus_read_reg(MENELAUS_OSC_CTRL) & 0x80)) {
> + dev_dbg(&m->client->dev, "no 32k oscillator\n");
> + return;
> + }
> +
> + /* support RTC alarm; it can issue wakeups */
> + if (alarm) {
> + if (menelaus_add_irq_work(MENELAUS_RTCALM_IRQ,
> + menelaus_rtc_alarm_work) < 0) {
> + dev_err(&m->client->dev, "can't handle RTC alarm\n");
> + return;
> + }
> + device_init_wakeup(&m->client->dev, 1);
> + }
> +
> + /* be sure RTC is enabled; allow 1/sec irqs; leave 12hr mode alone */
> + m->rtc_control = menelaus_read_reg(MENELAUS_RTC_CTRL);
> + if (!(m->rtc_control & RTC_CTRL_RTC_EN)
> + || (m->rtc_control & RTC_CTRL_AL_EN)
> + || (m->rtc_control & RTC_CTRL_EVERY_MASK)) {
> + if (!(m->rtc_control & RTC_CTRL_RTC_EN)) {
> + dev_warn(&m->client->dev, "rtc clock needs setting\n");
> + m->rtc_control |= RTC_CTRL_RTC_EN;
> + }
> + m->rtc_control &= ~RTC_CTRL_EVERY_MASK;
> + m->rtc_control &= ~RTC_CTRL_AL_EN;
> + menelaus_write_reg(MENELAUS_RTC_CTRL, m->rtc_control);
> + }
> +
> + m->rtc = rtc_device_register(DRIVER_NAME,
> + &m->client->dev,
> + &menelaus_rtc_ops, THIS_MODULE);
> + if (IS_ERR(m->rtc)) {
> + if (alarm) {
> + menelaus_remove_irq_work(MENELAUS_RTCALM_IRQ);
> + device_init_wakeup(&m->client->dev, 0);
> + }
> + dev_err(&m->client->dev, "can't register RTC: %d\n",
> + (int) PTR_ERR(m->rtc));
> + the_menelaus->rtc = NULL;
> + }
> +}
> +
> +#else
> +
> +static inline void menelaus_rtc_init(struct menelaus_chip *m)
> +{
> + /* nothing */
> +}
The inline here is OK. Strictly it isn't needed either, but what you have
here is a very typical kernel pattern.
> +#endif
> +
> +/*-----------------------------------------------------------------------*/
> +
> +static struct i2c_driver menelaus_i2c_driver;
> +
> +static int menelaus_probe(struct i2c_client *client)
> +{
> + struct menelaus_chip *menelaus;
> + int rev = 0, val;
unneeded initialisation.
> + int err = 0;
> + struct menelaus_platform_data *menelaus_pdata =
> + client->dev.platform_data;
> +
> + if (the_menelaus) {
> + dev_dbg(&client->dev, "only one %s for now\n",
> + DRIVER_NAME);
> + return -ENODEV;
I doubt if this can happen?
> + }
> +
> + menelaus = kzalloc(sizeof *menelaus, GFP_KERNEL);
> + if (!menelaus)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, menelaus);
> +
> + the_menelaus = menelaus;
> + menelaus->client = client;
> +
> + /* If a true probe check the device */
> + rev = menelaus_read_reg(MENELAUS_REV);
> + if (rev < 0) {
> + pr_err("device not found");
> + err = -ENODEV;
> + goto fail1;
> + }
> +
> + /* Ack and disable all Menelaus interrupts */
> + menelaus_write_reg(MENELAUS_INT_ACK1, 0xff);
> + menelaus_write_reg(MENELAUS_INT_ACK2, 0xff);
> + menelaus_write_reg(MENELAUS_INT_MASK1, 0xff);
> + menelaus_write_reg(MENELAUS_INT_MASK2, 0xff);
> + menelaus->mask1 = 0xff;
> + menelaus->mask2 = 0xff;
> +
> + /* Set output buffer strengths */
> + menelaus_write_reg(MENELAUS_MCT_CTRL1, 0x73);
> +
> + if (client->irq > 0) {
> + err = request_irq(client->irq, menelaus_irq, IRQF_DISABLED,
> + DRIVER_NAME, menelaus);
> + if (err) {
> + dev_dbg(&client->dev, "can't get IRQ %d, err %d",
> + client->irq, err);
> + goto fail1;
> + }
> + }
> +
> + mutex_init(&menelaus->lock);
> + INIT_WORK(&menelaus->work, menelaus_work);
> +
> + pr_info("Menelaus rev %d.%d\n", rev >> 4, rev & 0x0f);
> +
> + val = menelaus_read_reg(MENELAUS_VCORE_CTRL1);
> + if (val < 0)
> + goto fail2;
> + if (val & (1 << 7))
> + menelaus->vcore_hw_mode = 1;
> + else
> + menelaus->vcore_hw_mode = 0;
> +
> + if (menelaus_pdata != NULL && menelaus_pdata->late_init != NULL) {
> + err = menelaus_pdata->late_init(&client->dev);
> + if (err < 0)
> + goto fail2;
> + }
> +
> + menelaus_rtc_init(menelaus);
> +
> + return 0;
> +fail2:
> + free_irq(client->irq, menelaus);
> + flush_scheduled_work();
> +fail1:
> + kfree(menelaus);
> + return err;
> +}
> +
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
