Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Sat, 25 Nov 2023 15:19:59 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Crt Mori <cmo@melexis.com>
Cc:     Kim Seer Paller <kimseer.paller@analog.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Michael Hennerich <Michael.Hennerich@analog.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] iio: frequency: admfm2000: New driver
Message-ID: <20231125151959.00261229@jic23-huawei>
In-Reply-To: <CAKv63uvod-zdYSy93sJPBefBrqUUGhKDRLrC3vxvdeTZ4eM-nA@mail.gmail.com>
References: <20231123094357.222371-1-kimseer.paller@analog.com>
        <20231123094357.222371-2-kimseer.paller@analog.com>
        <CAKv63uvod-zdYSy93sJPBefBrqUUGhKDRLrC3vxvdeTZ4eM-nA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 23 Nov 2023 11:19:51 +0100
Crt Mori <cmo@melexis.com> wrote:

> Hi,
> Just minor remark inline.
> 
> Best regards,
> Crt

Hi Crt,

Please crop replies / reviews to only relevant context. If there are lots of
comments it maybe fine to leave whole driver but that's not the case ehre.

Should only see something like...

Thanks,

Jonathan

> 
> On Thu, 23 Nov 2023 at 10:44, Kim Seer Paller <kimseer.paller@analog.com> wrote:
> >
> > Dual microwave down converter module with input RF and LO frequency
> > ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> > 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> > for each down conversion path.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > ---
...


> > +static int admfm2000_setup(struct admfm2000_state *st,
> > +                          struct iio_dev *indio_dev)
> > +{

...

> > +       if (st->dsa_gpios[1]->ndescs != ADMF20000_DSA_GPIOS) {
> > +               dev_err_probe(dev, -ENODEV, "%d GPIOs needed to operate\n",
> > +                             ADMF20000_DSA_GPIOS);  
> 
> no return -ENODEV here?
> 
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int admfm2000_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct iio_dev *indio_dev;
> > +       struct admfm2000_state *st;
> > +       int ret;  
> 
> Order these in reverse christmass tree like you did above.
> 
> 
> > +
> > +       indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +       if (!indio_dev)
> > +               return -ENOMEM;
> > +
> > +       st = iio_priv(indio_dev);
> > +
> > +       indio_dev->name = "admfm2000";
> > +       indio_dev->num_channels = ARRAY_SIZE(admfm2000_channels);
> > +       indio_dev->channels = admfm2000_channels;
> > +       indio_dev->info = &admfm2000_info;
> > +       indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +       st->gain[0] = ADMF20000_DEFAULT_GAIN;
> > +       st->gain[1] = ADMF20000_DEFAULT_GAIN;
> > +
> > +       mutex_init(&st->lock);
> > +
> > +       ret = admfm2000_setup(st, indio_dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = admfm2000_channel_config(st, indio_dev);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return devm_iio_device_register(dev, indio_dev);
> > +}

Thanks,

Jonathan