Date:	Fri, 13 Sep 2013 19:47:13 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Lee Jones <lee.jones@...aro.org>
Cc:	Samuel Ortiz <sameo@...ux.intel.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] mfd: add STw481x driver

On Mon, Sep 2, 2013 at 10:16 AM, Lee Jones <lee.jones@...aro.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists