Re: [PATCH v3 1/2] iio: max597x: Add support for max597x

[Jonathan Cameron]

On Tue, 4 Apr 2023 15:37:21 +0530
Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> wrote:
> >   
> >> From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> >> Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> >> ...  
> > 
> > --- 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