Hi Lee,
On 27/03/20 11:22 am, Lee Jones wrote:
Saravanan, Jonathan,
On Fri, 27 Mar 2020, saravanan sekar wrote:
On 27/03/20 8:55 am, Lee Jones wrote:
On Sun, 22 Mar 2020, Saravanan Sekar wrote:
mp2629 is a highly-integrated switching-mode battery charge management
device for single-cell Li-ion or Li-polymer battery.
Add MFD core enables chip access for ADC driver for battery readings,
and a power supply battery-charger driver
Signed-off-by: Saravanan Sekar <sravanhome@xxxxxxxxx>
---
drivers/mfd/Kconfig | 9 +++
drivers/mfd/Makefile | 2 +
drivers/mfd/mp2629.c | 116 +++++++++++++++++++++++++++++++++++++
include/linux/mfd/mp2629.h | 22 +++++++
4 files changed, 149 insertions(+)
create mode 100644 drivers/mfd/mp2629.c
create mode 100644 include/linux/mfd/mp2629.h
[...]
+static int mp2629_probe(struct i2c_client *client)
+{
+ struct mp2629_info *info;
Call this ddata instead of info.
Not sure the reason, I will do.
Because this is device data. Info is too loose of a definition.
Ok, noted
+ struct resource *resources;
+ int ret;
+ int i;
+
+ info = devm_kzalloc(&client->dev, sizeof(*info), GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ info->dev = &client->dev;
+ i2c_set_clientdata(client, info);
+
+ info->regmap = devm_regmap_init_i2c(client, &mp2629_regmap_config);
+ if (IS_ERR(info->regmap)) {
+ dev_err(info->dev, "Failed to allocate regmap!\n");
+ return PTR_ERR(info->regmap);
+ }
+
+ for (i = 0; i < MP2629_MFD_MAX; i++) {
+ mp2629mfd[i].platform_data = &info->regmap;
+ mp2629mfd[i].pdata_size = sizeof(info->regmap);
You don't need to store this in platform data as well.
You already have it in device data (ddata [currently 'info']).
"The IIO parts seems fine (minor comments inline) but I'm not keep on
directly accessing the internals of the mfd device info structure.
To my mind that should be opaque to the child drivers so as to provide
clear structure to any such accesses.
This mess in layering with the children directly using the parents
regmap is a little concerning. It means that the 3 drivers
really aren't very well separated and can't really be reviewed
independently (not a good thing)."
This is the review comments form Jonathan on V2, not to access parent data
structure directly.
Am I misunderstood his review comments? please suggest the better option to
follow as like in V2
or V2 + some improvements or V4 + improvements?
I will take this up with Jonathan separately if necessary.
For your FYI (and Jonathan if he's Cc'ed), it's very common for a
child of an MFD to acquire resources from their parent. That is the
point of a lot of MFDs, to obtain and register shared resources and
pass them onto their offspring. There are 10's of examples of this.
Things like Regmaps aren't platform data, they are device/driver data,
which is usually passed though platform_set_drvdata().
Thanks for clarification, I will go as like in V2 sharing mfd struct to
the childs.
[...]
+ */
+
+#ifndef __MP2629_H__
+#define __MP2629_H__
+
+#include <linux/types.h>
+
+struct device;
+struct regmap;
Why not just add the includes?
Some more shared enum added in ADC driver
Sorry?
I misunderstood your previous question that you are asking to remove
this mp2629.h file
"No user here. (Hint: Use forward declaration of struct device instead)"
- review comments on V1 from Andy Shevchenko.
So remove the includes
+struct mp2629_info {
+ struct device *dev;
+ struct regmap *regmap;
+};
+
+#endif
Thanks,
Saravanan
