RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:18 PM, Linus Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > +++ b/drivers/mfd/pv88080-i2c.c
> > +
> > +static const struct of_device_id pv88080_of_match_table[] = {
> > +   { .compatible = "pvs,pv88080",.data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +   { },
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_of_match_table);
> 
> Actually you are doing something weird.
> 
> The only thing you put in your device tree is this I guess.
> 
> That means that the GPIO chip does *not* have a device tree entry, so you
> cannot reference the GPIOs there with  notation.
> 
> Please look around a bit to see how other OF-MFDs do it: they register and
> populate by struct mfd_cell using the .of_compatible member so that
> subdevices get their own DT nodes, which is necessary for nodes providing
> resources such as GPIOs, regulators and clocks, lest you cannot reference them
> in your device tree!
> 
> Therefore I think all your subdevices should instantiate from device tree with
> compatible matching as well, not as platform devices.
> 
> > +struct pv88080_pdata {
> > +   int (*init)(struct pv88080 *pv88080);
> > +   int irq_base;
> > +   int gpio_base;
> 
> NAK.
> 
> Get irq from the device tree, assign gpio base dynamically.
> 
> > +   struct regulator_init_data
> > + *regulators[PV88080_MAX_REGULATORS];
> 
> I suspect also this should come from the device tree.
> 
> Yours,
> Linus Walleij

Thank you for the comments.
I will send patch again.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:18 PM, Linus Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > +++ b/drivers/mfd/pv88080-i2c.c
> > +
> > +static const struct of_device_id pv88080_of_match_table[] = {
> > +   { .compatible = "pvs,pv88080",.data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +   { },
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_of_match_table);
> 
> Actually you are doing something weird.
> 
> The only thing you put in your device tree is this I guess.
> 
> That means that the GPIO chip does *not* have a device tree entry, so you
> cannot reference the GPIOs there with  notation.
> 
> Please look around a bit to see how other OF-MFDs do it: they register and
> populate by struct mfd_cell using the .of_compatible member so that
> subdevices get their own DT nodes, which is necessary for nodes providing
> resources such as GPIOs, regulators and clocks, lest you cannot reference them
> in your device tree!
> 
> Therefore I think all your subdevices should instantiate from device tree with
> compatible matching as well, not as platform devices.
> 
> > +struct pv88080_pdata {
> > +   int (*init)(struct pv88080 *pv88080);
> > +   int irq_base;
> > +   int gpio_base;
> 
> NAK.
> 
> Get irq from the device tree, assign gpio base dynamically.
> 
> > +   struct regulator_init_data
> > + *regulators[PV88080_MAX_REGULATORS];
> 
> I suspect also this should come from the device tree.
> 
> Yours,
> Linus Walleij

Thank you for the comments.
I will send patch again.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 5:11 AM, Linux Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > From: Eric Jeong 
> >
> > This patch adds supports for PV88080 MFD core device.
> >
> > It provides communication through the I2C interface.
> > It contains the following components:
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> (...)
> >  drivers/mfd/pv88080-i2c.c   |   99 
> 
> This looks like a pure I2C driver.
> 
> Why is it not in drivers/i2c/busses/i2c-pv88080.c?
> 
> Yours,
> Linus Walleij

Ok. I agree with you. So, I will send patch again after combining the relevant 
files into one.
Thank you.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 5:11 AM, Linux Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > From: Eric Jeong 
> >
> > This patch adds supports for PV88080 MFD core device.
> >
> > It provides communication through the I2C interface.
> > It contains the following components:
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> (...)
> >  drivers/mfd/pv88080-i2c.c   |   99 
> 
> This looks like a pure I2C driver.
> 
> Why is it not in drivers/i2c/busses/i2c-pv88080.c?
> 
> Yours,
> Linus Walleij

Ok. I agree with you. So, I will send patch again after combining the relevant 
files into one.
Thank you.

Regards
Eric

Re: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Linus Walleij]

On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
 wrote:

> +++ b/drivers/mfd/pv88080-i2c.c
> +
> +static const struct of_device_id pv88080_of_match_table[] = {
> +   { .compatible = "pvs,pv88080",.data = (void *)TYPE_PV88080_AA },
> +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, pv88080_of_match_table);

Actually you are doing something weird.

The only thing you put in your device tree is this I guess.

That means that the GPIO chip does *not* have a device tree
entry, so you cannot reference the GPIOs there with 
notation.

Please look around a bit to see how other OF-MFDs do it: they
register and populate by struct mfd_cell using the
.of_compatible member so that subdevices get
their own DT nodes, which is necessary for nodes providing
resources such as GPIOs, regulators and clocks, lest you
cannot reference them in your device tree!

Therefore I think all your subdevices should instantiate from
device tree with compatible matching as well, not as
platform devices.

> +struct pv88080_pdata {
> +   int (*init)(struct pv88080 *pv88080);
> +   int irq_base;
> +   int gpio_base;

NAK.

Get irq from the device tree, assign gpio base dynamically.

> +   struct regulator_init_data *regulators[PV88080_MAX_REGULATORS];

I suspect also this should come from the device tree.

Yours,
Linus Walleij

Re: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Linus Walleij]

On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
 wrote:

> +++ b/drivers/mfd/pv88080-i2c.c
> +
> +static const struct of_device_id pv88080_of_match_table[] = {
> +   { .compatible = "pvs,pv88080",.data = (void *)TYPE_PV88080_AA },
> +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> +   { },
> +};
> +MODULE_DEVICE_TABLE(of, pv88080_of_match_table);

Actually you are doing something weird.

The only thing you put in your device tree is this I guess.

That means that the GPIO chip does *not* have a device tree
entry, so you cannot reference the GPIOs there with 
notation.

Please look around a bit to see how other OF-MFDs do it: they
register and populate by struct mfd_cell using the
.of_compatible member so that subdevices get
their own DT nodes, which is necessary for nodes providing
resources such as GPIOs, regulators and clocks, lest you
cannot reference them in your device tree!

Therefore I think all your subdevices should instantiate from
device tree with compatible matching as well, not as
platform devices.

> +struct pv88080_pdata {
> +   int (*init)(struct pv88080 *pv88080);
> +   int irq_base;
> +   int gpio_base;

NAK.

Get irq from the device tree, assign gpio base dynamically.

> +   struct regulator_init_data *regulators[PV88080_MAX_REGULATORS];

I suspect also this should come from the device tree.

Yours,
Linus Walleij

Re: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Linus Walleij]

On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
 wrote:

> From: Eric Jeong 
>
> This patch adds supports for PV88080 MFD core device.
>
> It provides communication through the I2C interface.
> It contains the following components:
> - Regulators
> - Configurable GPIOs
>
> Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
>
> Signed-off-by: Eric Jeong 
(...)
>  drivers/mfd/pv88080-i2c.c   |   99 

This looks like a pure I2C driver.

Why is it not in drivers/i2c/busses/i2c-pv88080.c?

Yours,
Linus Walleij

Re: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Linus Walleij]

On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
 wrote:

> From: Eric Jeong 
>
> This patch adds supports for PV88080 MFD core device.
>
> It provides communication through the I2C interface.
> It contains the following components:
> - Regulators
> - Configurable GPIOs
>
> Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
>
> Signed-off-by: Eric Jeong 
(...)
>  drivers/mfd/pv88080-i2c.c   |   99 

This looks like a pure I2C driver.

Why is it not in drivers/i2c/busses/i2c-pv88080.c?

Yours,
Linus Walleij

Re: [PATCH V2 2/4] mfd: pv88080: MFD core support

[kbuild test robot]

Hi Eric,

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.9-rc2 next-20161027]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Eric-Jeong/Documentation-pv88080-Move-binding-document/20161027-091356
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `pv88080_i2c_probe':
>> drivers/mfd/pv88080-i2c.o:(.text+0x11652c): undefined reference to 
>> `__devm_regmap_init_i2c'
   drivers/mfd/pv88080-i2c.o:(.text+0x116548): undefined reference to 
`__devm_regmap_init_i2c'
   drivers/built-in.o: In function `pv88080_i2c_driver_init':
>> drivers/mfd/pv88080-i2c.o:(.init.text+0xa010): undefined reference to 
>> `i2c_register_driver'
   drivers/mfd/pv88080-i2c.o:(.init.text+0xa020): undefined reference to 
`i2c_register_driver'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH V2 2/4] mfd: pv88080: MFD core support

[kbuild test robot]

Hi Eric,

[auto build test ERROR on ljones-mfd/for-mfd-next]
[also build test ERROR on v4.9-rc2 next-20161027]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Eric-Jeong/Documentation-pv88080-Move-binding-document/20161027-091356
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: alpha-allmodconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `pv88080_i2c_probe':
>> drivers/mfd/pv88080-i2c.o:(.text+0x11652c): undefined reference to 
>> `__devm_regmap_init_i2c'
   drivers/mfd/pv88080-i2c.o:(.text+0x116548): undefined reference to 
`__devm_regmap_init_i2c'
   drivers/built-in.o: In function `pv88080_i2c_driver_init':
>> drivers/mfd/pv88080-i2c.o:(.init.text+0xa010): undefined reference to 
>> `i2c_register_driver'
   drivers/mfd/pv88080-i2c.o:(.init.text+0xa020): undefined reference to 
`i2c_register_driver'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip