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
