Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 8 Apr 2023 11:40:55 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Naresh Solanki <naresh.solanki@9elements.com>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Patrick Rudolph <patrick.rudolph@9elements.com>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v3 1/2] iio: max597x: Add support for max597x
Message-ID: <20230408114055.474a597a@jic23-huawei>
In-Reply-To: <c8f709d3-1a8e-c98c-6917-839a0ca4d4ff@9elements.com>
References: <20230328094416.3851801-1-Naresh.Solanki@9elements.com>
        <20230402180123.0fc1cfde@jic23-huawei>
        <c8f709d3-1a8e-c98c-6917-839a0ca4d4ff@9elements.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 4 Apr 2023 15:37:21 +0530
Naresh Solanki <naresh.solanki@9elements.com> wrote:

> Hi Jonathan,
> 
> On 02-04-2023 10:31 pm, Jonathan Cameron wrote:
> > On Tue, 28 Mar 2023 11:44:14 +0200
> > Naresh Solanki <naresh.solanki@9elements.com> wrote:
> >   
> >> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >>
> >> max5970 & max5978 has 10bit ADC for voltage & current
> >> monitoring.
> >> Use iio framework to expose the same in sysfs.
> >>
> >> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> >> ...  
> > 
> > --- not ...
> > 
> > As I mentioned in my reply to v2 thread (which crossed with this v3)
> > I'd like this series to be cc'd to the list and maintainers for Hwmon
> > with a cover letter explaining the reasoning for it being an IIO driver
> > + the restrictions that potentially brings.  
> Sure.

A minor request from me for future replies to make things a little
more efficient. Note I'm not always great at this myself - this
is a case of do as I say rather than as I do!

Don't bother agreeing with stuff - reviewers assume you agree
unless you comment!

Crop out any sections of the email reply that have no new content.

e.g.

...

> >> +static int max597x_iio_probe(struct platform_device *pdev)
> >> +{
> >> +	struct max597x_data *max597x = dev_get_drvdata(pdev->dev.parent);
> >> +	struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
> >> +	struct iio_dev *indio_dev;
> >> +	struct max597x_iio *priv;
> >> +	int ret, i;
> >> +
> >> +	if (!regmap)
> >> +		return -EPROBE_DEFER;
> >> +
> >> +	if (!max597x || !max597x->num_switches)
> >> +		return -EPROBE_DEFER;
> >> +
> >> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*priv));
> >> +	if (!indio_dev)
> >> +		return dev_err_probe(&pdev->dev, -ENOMEM,
> >> +				     "failed to allocate iio device\n");
> >> +
> >> +	indio_dev->info = &max597x_adc_iio_info;
> >> +	indio_dev->modes = INDIO_DIRECT_MODE;
> >> +
> >> +	switch (max597x->num_switches) {  
> > 
> > Having a value 'num_switches' that maps to a set of enums called _TYPE_ is unusual.
> > Perhaps rename it to type.  
> Will add a local variable type to track the same within with driver.

Not totally sure what you mean. I'll look for it in newer versions.

Thanks,

Jonathan