Re: [RFC PATCHv2 2/4] drivers/otp: add support for Picoxcell PC3X3 OTP

[Mike Frysinger]

On Thu, Mar 24, 2011 at 16:59, Jamie Iles wrote:
> On Thu, Mar 24, 2011 at 02:50:35PM -0400, Mike Frysinger wrote:
>> On Thu, Mar 24, 2011 at 11:21, Jamie Iles wrote:
>> > +static void otp_charge_pump_enable(struct pc3x3_otp *otp, int enable)
>> > +{
>> > +#define OTP_MRA_CHARGE_PUMP_ENABLE_MASK Â Â Â Â Â Â(1 << 12)
>> > +#define OTP_MRA_CHARGE_PUMP_MONITOR_MASK Â Â(1 << 15)
>> > +#define OTP_MRA_READ_REFERENCE_LEVEL9_MASK Â(1 << 9)
>> > +#define OTP_MRA_READ_REFERENCE_LEVEL5_MASK Â(1 << 5)
>> > +#define OTP_STATUS_VPP_APPLIED Â Â Â Â Â Â (1 << 4)
>> > + Â Â Â u32 mra = enable ?
>> > + Â Â Â Â Â Â Â (OTP_MRA_CHARGE_PUMP_ENABLE_MASK |
>> > + Â Â Â Â Â Â Â ÂOTP_MRA_CHARGE_PUMP_MONITOR_MASK |
>> > + Â Â Â Â Â Â Â ÂOTP_MRA_READ_REFERENCE_LEVEL9_MASK |
>> > + Â Â Â Â Â Â Â ÂOTP_MRA_READ_REFERENCE_LEVEL5_MASK) : 0;
>> > +
>> > + Â Â Â otp_write_MRA(otp, mra);
>> > +
>> > + Â Â Â /* Now wait for VPP to reach the correct level. */
>> > + Â Â Â if (enable && !test_mode) {
>> > + Â Â Â Â Â Â Â while (!(otp_read_reg(otp, OTP_MACRO_STATUS_REG_OFFSET) &
>> > + Â Â Â Â Â Â Â Â Â Â Â ÂOTP_STATUS_VPP_APPLIED))
>> > + Â Â Â Â Â Â Â Â Â Â Â cpu_relax();
>> > + Â Â Â }
>> > +
>> > + Â Â Â udelay(1);
>> > +}
>>
>> is that udelay() really necessary ? Âcould it be refined to a smaller ndelay() ?
>
> It's what is specifed in the IP vendors datasheets so perhaps it could
> be less but I'd like to err on the side of caution.

if that's what the datasheet says, then np, it's fine.  presumably
you're not charging it for fun which means you cant background the
wait.

>> > + Â Â Â if (test_mode) {
>> > + Â Â Â Â Â Â Â u64 *p = devm_kzalloc(&pdev->dev, SZ_16K + SZ_1K, GFP_KERNEL);
>> > ...
>> > + Â Â Â } else {
>> > + Â Â Â Â Â Â Â pc3x3_dev->iomem = devm_ioremap(&pdev->dev, mem->start,
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â resource_size(mem));
>> > ...
>> > +out_unregister:
>> > + Â Â Â otp_device_unregister(otp);
>> > +out_clk_disable:
>> > + Â Â Â clk_disable(pc3x3_dev->clk);
>> > + Â Â Â clk_put(pc3x3_dev->clk);
>> > +out:
>> > + Â Â Â return err;
>>
>> hmm, but you dont iounmap or free any of the memory from earlier when
>> you error out ...
>
> No, that's what the devm_* stuff does for us.

hmm, wasnt aware of this devm* stuff.  sounds like fun and i could use
it elsewhere.

>> > +static struct platform_driver otp_driver = {
>> > +    .remove     = __devexit_p(otp_remove),
>> > +    .driver     = {
>> > +        .name  = "picoxcell-otp-pc3x3",
>> > +        .pm   = &otp_pm_ops,
>> > + Â Â Â },
>> > +};
>> >
>> > +static int __init pc3x3_otp_init(void)
>> > +{
>> > + Â Â Â return platform_driver_probe(&otp_driver, otp_probe);
>> > +}
>>
>> why call probe yourself ? Âwhy not platform_driver_register() ?
>
> I made this comment in another driver myself and another reviewer
> pointed out that if the device isn't some kind of hotplug device then it
> probably isn't needed to let the bus do the matching and probing but I'm
> happy to do what you've suggested, it feels a bit more natural anyway!

that's why the pseudo "platform" bus exists in the first place.  i
could somewhat understand if the probe function actually did probing
to see if the device in question existed before going further, but
this probe function simply assumes that if it gets called, the device
exists.  as such, it makes a lot more sense to force people to "opt
in" via their platform resources.  otherwise, the whole "build one
kernel but deploy to multiple boards" pretty much goes out the window.
-mike
--
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/