Messages in this thread | | | Date | Fri, 13 Sep 2013 19:47:13 +0200 | Subject | Re: [PATCH 1/4] mfd: add STw481x driver | From | Linus Walleij <> |
| |
On Mon, Sep 2, 2013 at 10:16 AM, Lee Jones <lee.jones@linaro.org> wrote: > [Me]
>> This adds a driver for the STw481x PMICs found in the Nomadik >> family of platforms. This one uses pure device tree probing. >> Print some of the OTP registers on boot and register a regulator >> MFD child. (...)
> Please see to the checkpatch.pl pointed out by Wang.
Fixed these.
>> +config MFD_STW481X >> + bool "Support for ST Microelectronics STw481x" >> + depends on I2C && ARCH_NOMADIK > > I believe this chip is commercially available, therefore it's surely > possible that they 'could' appear on !NOMADIK platforms? What are the > dependencies on the Nomadik platform?
It is basically a chipset and only appear together, but removing the ARCH_NOMADIK dependency gives better coverage on other platforms so I would prefer to remove it, but IIRC it blew up on the S390 (!) platform, but I'll try it again without.
>> + ret = regmap_write(stw481x->map, 0x1f, msb); > > Any chance in replacing the register param #defined?
Fixed this.
> I still don't know what this function is doing. I'm taking a guess > that we're fetching the power control registers as opposed to some > kind of pinctrl?
Its a set of set of registers which can be written once at manufacturing, then they cannot be changed. So it's something of a set of power ROM values.
> With the variables used in the function and in the map, coupled with > the !#defined register values is making it really hard to see exactly > what we're doing here.
I'm adding a piece of kerneldoc to the function to hash things out...
>> +static int stw481x_startup(struct stw481x *stw481x) >> +{ >> + u8 vcore_val[] = { 100, 105, 110, 115, 120, 122, 124, 126, 128, >> + 130, 132, 134, 136, 138, 140, 145 }; >> + u8 vpll_val[] = { 105, 120, 130, 180 }; >> + u8 vaux_val[] = { 15, 18, 25, 28 }; > > I had to look further down to see that these were voltages*100. Any > chance of a small comment here to allude to the fact?
OK fixed it.
>> +static int stw481x_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct stw481x *stw481x; >> + struct stw481x_platform_data *pdata = client->dev.platform_data; > > If this is a DT only won't this always be NULL, or does the I2C > subsystem populate it using some automagic routine?
No spot on, what kind of crap is this... just development cruft lying around in the corners ... :-/ deleted it.
>> + stw481x = devm_kzalloc(&client->dev, sizeof(*stw481x), GFP_KERNEL); >> + if (!stw481x) >> + return -ENOMEM; > > This is a question, rather than a statement; can *alloc ever return > anything other than a pointer or -ENOMEM? If so, you should probably > return stw481x here, if not, just ignore me.
It doesn't ever return a negative return code, it returns a valid pointer or NULL if the *alloc failed. This is to mimic the behaviour of POSIX malloc() I think, so we check if it's NULL then return the error code.
>> +fail: >> + devm_kfree(&client->dev, stw481x); > > Does this device carry on living after this probe function? I don't > think it does, so you can omit any freeing of memory if you're using > managed resources.
Good point, another copypaste bug.
>> + devm_kfree(&client->dev, stw481x); > > ... same here.
Fixed this too.
>> +/* >> + * This ID table is completely unused, as this is a pure >> + * device-tree probed driver, but it has to be here due to >> + * the structure of the I2C core. >> + */ > > That's not true. It will be used if anyone decides not to use the > of_device_id table. As this is an I2C device you can register it via > Device Tree by (in this case) putting 'dummy' as the node name, so > it's almost certainly not good to use that name here.
Sorry I don't get it. Whatever string I put in there can be abused instead of using the proper compatible string (which is the proper way to probe DT I2C devices), I've tried to fix this for DT-only I2C devices but a tiresome regression due to drivers relying on this i2c_device_id not being NULL and inability to remove it from the I2C core without refactoring the world ensued, see: http://marc.info/?l=linux-i2c&m=136847631009831&w=2
>> +static const struct i2c_device_id dummy_id[] = { >> + { "dummy", 0 }, >> + { } >> +};
I can rename it to "dummy-node-do-not-match-dt-node-to-i2c-device-id-damn-it" if you wish ;-)
>> +static const struct of_device_id stw481x_match[] = { >> + { .compatible = "st,stw4810", }, >> + { .compatible = "st,stw4811", }, >> + {}, > > Funny spacing here.
OK fixed this.
>> +static int __init stw481x_init(void) >> +{ >> + return i2c_add_driver(&stw481x_driver); >> +} >> +module_init(stw481x_init); >> + >> +static void __exit stw481x_exit(void) >> +{ >> + i2c_del_driver(&stw481x_driver); >> +} >> +module_exit(stw481x_exit); > > Better to remove all this boiler plate and replace with module_i2c_driver().
Good point. Fixed it.
Yours, Linus Walleij
| |