Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp2321235ybb; Fri, 27 Mar 2020 03:22:54 -0700 (PDT) X-Google-Smtp-Source: ADFU+vuEc21xiTpVJANHUVseA47efRqkkNpIAtR+CpgkfwBHDdzDDz8oCGcG+aLG6NJxrKiDvIxd X-Received: by 2002:a4a:970f:: with SMTP id u15mr8414643ooi.90.1585304574727; Fri, 27 Mar 2020 03:22:54 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1585304574; cv=none; d=google.com; s=arc-20160816; b=FkRO4DHyumTRz5iKQvS9bZRJY3p31VIA72yfIYJbP+pGsQSH891TEUK9GPbVxceaWv cW1uJpXZ1iI8Jw872yOQgIoV8Epsk2nI6oGyia6UPf+UqE5MhwzHkouo3Uwt0sAHN2UD lYZo2UeTZ7gbSaP2IfsuMqDK4gBINeyObHSbGZ5ckJuhw0aa9HrRKYJUwEi7OyRj6eX5 eT/o31SkVpC+fGQtXwN2jocYRMM3LCCEqVhsQfoX8F/iobe6GcaUn4FtjlFVuTtM1eO5 vrltV1C70yML9gVpf/ldp/XW7RVxrjq4jSo6vD6uNEnvrD4/qW982gRLGbpH1k5nvyAf Lb6Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=mxz8+ilE2NjB6kYVNyYap8z20EWNc89Zq7z80dHig9g=; b=KSzf9gdh3Frn+Ep9jCoF/R9cYnrCMYUduDF/Ka9MgIM3Mzz7cGpCwLeY6z8xWI2XyA pxREn6Bg+i7TTGv7+n5yeySRS9DB76afdFizbWjkYftZTYwG0eEpXwkTSbD0vWESOcpi 2WTResSas0ZngvRiT5xoJYPwxeR83esUwDbOyjbe3hpt7nRuSVP6O2IWQ5N9h6vkd30W 8onLaoT1Us/TVw95waD+MjWIGs207oM89VvdA0nk4q9pyM2qdM1WrRX66QbO2x8HI7Vc Ib0DdmEg4Skb0T4tGU72VB8k6tJqHJyvbcjU5cj4y7D40Uj2XJqOG5bD5eLcy6hXgaN3 iq3g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=lwFfgP7q; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id c133si2412437oif.272.2020.03.27.03.22.41; Fri, 27 Mar 2020 03:22:54 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=lwFfgP7q; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727185AbgC0KVd (ORCPT + 99 others); Fri, 27 Mar 2020 06:21:33 -0400 Received: from mail-wm1-f66.google.com ([209.85.128.66]:53968 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726275AbgC0KVd (ORCPT ); Fri, 27 Mar 2020 06:21:33 -0400 Received: by mail-wm1-f66.google.com with SMTP id b12so10833313wmj.3 for ; Fri, 27 Mar 2020 03:21:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:content-transfer-encoding:in-reply-to; bh=mxz8+ilE2NjB6kYVNyYap8z20EWNc89Zq7z80dHig9g=; b=lwFfgP7q0u5LzYu5IHEQz+moTPm67eiVbNP8jvxisTOsI5Ma9EXK0h13/GmZJTgMnl +Onr6bFvSPfXY+9YSNs3mbaMYQKyYOx/Nun1R1AIDyUetdtqqChqnXG7/1EIZeZJwG+h AoYjZMrLCixc4B2+nEQHEZhYmkVUgGuEFxsGPd1u8CQiLy2L6AQ8e4C5sS5mdW24AHni L423xnfJL3YVYLmXGSdDIk0Rk/t8gmiLBSeHvfIrZebXL+l1ni5cFPFsMcSjyofPyV6m rZrGDRGPqN0mT60S1tgbFGUZi4IjDSYnD6UJGS6q6bGnrTlVviriUNh9zl3/vfCBifys 8g/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=mxz8+ilE2NjB6kYVNyYap8z20EWNc89Zq7z80dHig9g=; b=KPGjxXBgZ6QsWAm0tKQQBMGOTqP+5uNA0ZRtPkn7rJIFVi1J6eZDMl0uMOXBJA+nQp EdMdcA7l54LaRChLPSt5vOeYSJaTz/msGYom+eL+KeDcLYavTUfINxWK/lgNimAFjNAl +EMZT7aUW1GYyaTxswaqYCpztczjwXXLaQhLxoL8IqmendXxH1pIZeU0rPvPTkYGg7iy +b7o8RTDLIfaG8EaIWJAKCAiKA7O4bEjcJHDfruh9fyThtltU4bA0cWQrEOF2zXIlwvp RgI5Gp64LpASLBE6EVjbfaDvEldeljebFf5ZtztX3xmgism5ZhsaK8M0AWpKC39uARRO CI2g== X-Gm-Message-State: ANhLgQ14O212+csTivz4FJSsqxpy3Q+XmYNpSrqKf6ZhwFyKfTLtKFf6 NQCpDdnl90PaCGqBjsypH0idkw== X-Received: by 2002:a1c:b4c6:: with SMTP id d189mr4433247wmf.132.1585304491231; Fri, 27 Mar 2020 03:21:31 -0700 (PDT) Received: from dell ([95.149.164.95]) by smtp.gmail.com with ESMTPSA id n9sm7646671wru.50.2020.03.27.03.21.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Mar 2020 03:21:30 -0700 (PDT) Date: Fri, 27 Mar 2020 10:22:21 +0000 From: Lee Jones To: saravanan sekar Cc: robh+dt@kernel.org, jic23@kernel.org, knaack.h@gmx.de, lars@metafoo.de, pmeerw@pmeerw.net, sre@kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH v4 2/5] mfd: mp2629: Add support for mps battery charger Message-ID: <20200327102221.GA3383@dell> References: <20200322224626.13160-1-sravanhome@gmail.com> <20200322224626.13160-3-sravanhome@gmail.com> <20200327075541.GF603801@dell> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > --- > > > 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. > > > + 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(). [...] > > > + */ > > > + > > > +#ifndef __MP2629_H__ > > > +#define __MP2629_H__ > > > + > > > +#include > > > + > > > +struct device; > > > +struct regmap; > > Why not just add the includes? > Some more shared enum added in ADC driver Sorry? > > > +struct mp2629_info { > > > + struct device *dev; > > > + struct regmap *regmap; > > > +}; > > > + > > > +#endif -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog