Re: [rtc-linux] [PATCH] rtc: rp5c01 - Add NVRAM support

From: Geert Uytterhoeven
Date: Sun Jun 13 2010 - 13:45:23 EST

Next message: Christian Kujau: "Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316"
Previous message: Jiri Slaby: "Re: 2.6.35-rc{12} regression: inactive console corrupted"
In reply to: Wan ZongShun: "Re: [rtc-linux] [PATCH] rtc: rp5c01 - Add NVRAM support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Jun 13, 2010 at 15:41, Wan ZongShun <mcuos.com@xxxxxxxxx> wrote:
> Minior comment below:
>
> 2010/6/13 Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>:
>> The Ricoh RP5C01 RTC contains 26 x 4 bits of NVRAM.
>> Provide access to it via a sysfs "nvram" attribute file.
>>
>> Signed-off-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>> --
>> Question: Is a spinlock in priv the right kind of locking?
>> Â Â Â Â ÂShould I use e.g. rtc-device.ops_lock instead?
>
> Firstly, this 'Question' text should not put here, please put all
> texts in front of '---'.

I explicitly put it there, as the question was meant for the reviewers only,
and I don't want it to be part of the final checkin comment.

> Secondly, You can not use rtc-device.ops_lock here, if so, it would
> arouse dead lock,
> before the ' rp5c01_read_time()' was called by RTC subsystem upper
> API, rtc-device.ops_lock
> has been required sucessfully, If continue to require the
> rtc-device.ops_lock in ' rp5c01_read_time()',
> the 'lock' will cannot be get, dead lock occurs.

Sorry, my question was not that correctly formulated...
I meant whether it's better to take rtc_device.ops_lock in the nvram
access functions
to synchronize with the RTC access functions (which already take that mutex),
instead of adding a spinlock to priv.

Thanks for your comments!

>> diff --git a/drivers/rtc/rtc-rp5c01.c b/drivers/rtc/rtc-rp5c01.c
>> index a95f733..36eb661 100644
>> --- a/drivers/rtc/rtc-rp5c01.c
>> +++ b/drivers/rtc/rtc-rp5c01.c
>> @@ -63,6 +63,8 @@ enum {
>> Âstruct rp5c01_priv {
>> Â Â Â Âu32 __iomem *regs;
>> Â Â Â Âstruct rtc_device *rtc;
>> + Â Â Â spinlock_t lock; Â Â Â Â/* against concurrent RTC/NVRAM access */
>> + Â Â Â struct bin_attribute nvram_attr;
>> Â};
>>
>> Âstatic inline unsigned int rp5c01_read(struct rp5c01_priv *priv,
>> @@ -92,6 +94,7 @@ static int rp5c01_read_time(struct device *dev, struct rtc_time *tm)
>> Â{
>> Â Â Â Âstruct rp5c01_priv *priv = dev_get_drvdata(dev);
>>
>> + Â Â Â spin_lock_irq(&priv->lock);
>> Â Â Â Ârp5c01_lock(priv);
>>
>> Â Â Â Âtm->tm_sec Â= rp5c01_read(priv, RP5C01_10_SECOND) * 10 +
>> @@ -111,6 +114,7 @@ static int rp5c01_read_time(struct device *dev, struct rtc_time *tm)
>> Â Â Â Â Â Â Â Âtm->tm_year += 100;
>>
>> Â Â Â Ârp5c01_unlock(priv);
>> + Â Â Â spin_unlock_irq(&priv->lock);
>>
>> Â Â Â Âreturn rtc_valid_tm(tm);
>> Â}
>> @@ -119,6 +123,7 @@ static int rp5c01_set_time(struct device *dev, struct rtc_time *tm)
>> Â{
>> Â Â Â Âstruct rp5c01_priv *priv = dev_get_drvdata(dev);
>>
>> + Â Â Â spin_lock_irq(&priv->lock);
>> Â Â Â Ârp5c01_lock(priv);
>>
>> Â Â Â Ârp5c01_write(priv, tm->tm_sec / 10, RP5C01_10_SECOND);
>> @@ -139,6 +144,7 @@ static int rp5c01_set_time(struct device *dev, struct rtc_time *tm)
>> Â Â Â Ârp5c01_write(priv, tm->tm_year % 10, RP5C01_1_YEAR);
>>
>> Â Â Â Ârp5c01_unlock(priv);
>> + Â Â Â spin_unlock_irq(&priv->lock);
>> Â Â Â Âreturn 0;
>> Â}
>>
>> @@ -147,6 +153,72 @@ static const struct rtc_class_ops rp5c01_rtc_ops = {
>> Â Â Â Â.set_time Â Â Â = rp5c01_set_time,
>> Â};
>>
>> +
>> +/*
>> + * The NVRAM is organized as 2 blocks of 13 nibbles of 4 bits.
>> + * We provide access to them like AmigaOS does: the high nibble of each 8-bit
>> + * byte is stored in BLOCK10, the low nibble in BLOCK11.
>> + */
>> +
>> +static ssize_t rp5c01_nvram_read(struct file *filp, struct kobject *kobj,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct bin_attribute *bin_attr,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âchar *buf, loff_t pos, size_t size)
>> +{
>> + Â Â Â struct device *dev = container_of(kobj, struct device, kobj);
>> + Â Â Â struct rp5c01_priv *priv = dev_get_drvdata(dev);
>> + Â Â Â ssize_t count;
>> +
>> + Â Â Â spin_lock_irq(&priv->lock);
>> +
>> + Â Â Â for (count = 0; size > 0 && pos < RP5C01_MODE; count++, size--) {
>> + Â Â Â Â Â Â Â u8 data;
>> +
>> + Â Â Â Â Â Â Â rp5c01_write(priv,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE_TIMER_EN | RP5C01_MODE_RAM_BLOCK10,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE);
>> + Â Â Â Â Â Â Â data = rp5c01_read(priv, pos) << 4;
>> + Â Â Â Â Â Â Â rp5c01_write(priv,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE_TIMER_EN | RP5C01_MODE_RAM_BLOCK11,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE);
>> + Â Â Â Â Â Â Â data |= rp5c01_read(priv, pos++);
>> + Â Â Â Â Â Â Â rp5c01_write(priv, RP5C01_MODE_TIMER_EN | RP5C01_MODE_MODE01,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE);
>> + Â Â Â Â Â Â Â *buf++ = data;
>> + Â Â Â }
>> +
>> + Â Â Â spin_unlock_irq(&priv->lock);
>> + Â Â Â return count;
>> +}
>> +
>> +static ssize_t rp5c01_nvram_write(struct file *filp, struct kobject *kobj,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct bin_attribute *bin_attr,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â char *buf, loff_t pos, size_t size)
>> +{
>> + Â Â Â struct device *dev = container_of(kobj, struct device, kobj);
>> + Â Â Â struct rp5c01_priv *priv = dev_get_drvdata(dev);
>> + Â Â Â ssize_t count;
>> +
>> + Â Â Â spin_lock_irq(&priv->lock);
>> +
>> + Â Â Â for (count = 0; size > 0 && pos < RP5C01_MODE; count++, size--) {
>> + Â Â Â Â Â Â Â u8 data = *buf++;
>> +
>> + Â Â Â Â Â Â Â rp5c01_write(priv,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE_TIMER_EN | RP5C01_MODE_RAM_BLOCK10,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE);
>> + Â Â Â Â Â Â Â rp5c01_write(priv, data >> 4, pos);
>> + Â Â Â Â Â Â Â rp5c01_write(priv,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE_TIMER_EN | RP5C01_MODE_RAM_BLOCK11,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE);
>> + Â Â Â Â Â Â Â rp5c01_write(priv, data & 0xf, pos++);
>> + Â Â Â Â Â Â Â rp5c01_write(priv, RP5C01_MODE_TIMER_EN | RP5C01_MODE_MODE01,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂRP5C01_MODE);
>> + Â Â Â }
>> +
>> + Â Â Â spin_unlock_irq(&priv->lock);
>> + Â Â Â return count;
>> +}
>> +
>> Âstatic int __init rp5c01_rtc_probe(struct platform_device *dev)
>> Â{
>> Â Â Â Âstruct resource *res;
>> @@ -168,6 +240,15 @@ static int __init rp5c01_rtc_probe(struct platform_device *dev)
>> Â Â Â Â Â Â Â Âgoto out_free_priv;
>> Â Â Â Â}
>>
>> + Â Â Â sysfs_bin_attr_init(&priv->nvram_attr);
>> + Â Â Â priv->nvram_attr.attr.name = "nvram";
>> + Â Â Â priv->nvram_attr.attr.mode = S_IRUGO | S_IWUSR;
>> + Â Â Â priv->nvram_attr.read = rp5c01_nvram_read;
>> + Â Â Â priv->nvram_attr.write = rp5c01_nvram_write;
>> + Â Â Â priv->nvram_attr.size = RP5C01_MODE;
>> +
>> + Â Â Â spin_lock_init(&priv->lock);
>> +
>> Â Â Â Ârtc = rtc_device_register("rtc-rp5c01", &dev->dev, &rp5c01_rtc_ops,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ÂTHIS_MODULE);
>> Â Â Â Âif (IS_ERR(rtc)) {
>> @@ -177,8 +258,15 @@ static int __init rp5c01_rtc_probe(struct platform_device *dev)
>>
>> Â Â Â Âpriv->rtc = rtc;
>> Â Â Â Âplatform_set_drvdata(dev, priv);
>> +
>> + Â Â Â error = sysfs_create_bin_file(&dev->dev.kobj, &priv->nvram_attr);
>> + Â Â Â if (error)
>> + Â Â Â Â Â Â Â goto out_unregister;
>> +
>> Â Â Â Âreturn 0;
>>
>> +out_unregister:
>> + Â Â Â rtc_device_unregister(rtc);
>> Âout_unmap:
>> Â Â Â Âiounmap(priv->regs);
>> Âout_free_priv:
>> @@ -190,6 +278,7 @@ static int __exit rp5c01_rtc_remove(struct platform_device *dev)
>> Â{
>> Â Â Â Âstruct rp5c01_priv *priv = platform_get_drvdata(dev);
>>
>> + Â Â Â sysfs_remove_bin_file(&dev->dev.kobj, &priv->nvram_attr);
>> Â Â Â Ârtc_device_unregister(priv->rtc);
>> Â Â Â Âiounmap(priv->regs);
>> Â Â Â Âkfree(priv);
>> --
>> 1.7.0.4

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Christian Kujau: "Re: [Bug #15589] 2.6.34-rc1: Badness at fs/proc/generic.c:316"
Previous message: Jiri Slaby: "Re: 2.6.35-rc{12} regression: inactive console corrupted"
In reply to: Wan ZongShun: "Re: [rtc-linux] [PATCH] rtc: rp5c01 - Add NVRAM support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]