Re: [PATCH 2/2 v6] iio: light: Add a driver for Sharp GP2AP002x00F

From: Linus Walleij <linus.walleij@linaro.org>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio <linux-iio@vger.kernel.org>,
	"Hartmut Knaack" <knaack.h@gmx.de>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Peter Meerwald-Stadler" <pmeerw@pmeerw.net>,
	"Linux Input" <linux-input@vger.kernel.org>,
	"Stephan Gerhold" <stephan@gerhold.net>,
	"Minkyu Kang" <mk7.kang@samsung.com>,
	"Paweł Chmiel" <pawel.mikolaj.chmiel@gmail.com>,
	"Jonathan Bakker" <xc-racer2@live.ca>,
	"Oskar Andero" <oskar.andero@gmail.com>,
	"Dmitry Torokhov" <dmitry.torokhov@gmail.com>
Subject: Re: [PATCH 2/2 v6] iio: light: Add a driver for Sharp GP2AP002x00F
Date: Sat, 8 Feb 2020 12:47:01 +0100	[thread overview]
Message-ID: <CACRpkdZ3WWEmAaF6mSVTRTWXsPPQ=BVWNshBTS9wPn4Hc0CEDA@mail.gmail.com> (raw)
In-Reply-To: <20200202152742.073a4df6@archlinux>

On Sun, Feb 2, 2020 at 4:27 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 26 Jan 2020 16:05:48 +0100 Linus Walleij <linus.walleij@linaro.org> wrote:

> Hmm. There are a few minor things inline, but in the interests of
> saving everyone time I'll just fix them up.

Sweet! Thanks Jonathan.

> For the missing docs one I'll make something up based on what I think
> they are.  Please check!

OK

> Also it doesn't actually build..
>
> drivers/iio/light/gp2ap002.c:760:26: error: ‘gp2ap002_id’ undeclared here (not in a function); did you mean ‘gp2ap002_info’?
>   760 | MODULE_DEVICE_TABLE(i2c, gp2ap002_id);
>       |                          ^~~~~~~~~~~
> ./include/linux/module.h:230:15: note: in definition of macro ‘MODULE_DEVICE_TABLE’
>   230 | extern typeof(name) __mod_##type##__##name##_device_table  \
>       |               ^~~~
> ./include/linux/module.h:230:21: error: ‘__mod_i2c__gp2ap002_id_device_table’ aliased to undefined symbol ‘gp2ap002_id’
>   230 | extern typeof(name) __mod_##type##__##name##_device_table  \
>       |                     ^~~~~~
> drivers/iio/light/gp2ap002.c:760:1: note: in expansion of macro ‘MODULE_DEVICE_TABLE’
>   760 | MODULE_DEVICE_TABLE(i2c, gp2ap002_id);
>       | ^~~~~~~~~~~~~~~~~~~
>
> I'll fix that as well..

That's what I get for not testing allmodconfig.

I have some new testing infrastructure in place, that will hopefully
let me test this better. I use build servers because my poor desktops
and laptops literally take hours to churn through allyes allno and
allmod configs.

> > +/**
> > + * struct gp2ap002 - GP2AP002 state
> > + * @map: regmap pointer for the i2c regmap
> > + * @dev: pointer to parent device
> > + * @vdd: regulator controlling VDD
> > + * @vio: regulator controlling VIO
> > + * @alsout: IIO ADC channel to convert the ALSOUT signal
>
> Grumble.. Incomplete docs. Please send a follow up patch fixing that.

OK

> > +struct gp2ap002 {
> > +     struct regmap *map;
> > +     struct device *dev;
> > +     struct regulator *vdd;
> > +     struct regulator *vio;
> > +     struct iio_channel *alsout;
> > +     enum iio_event_direction dir;
> This one doesn't actually seem to be used so I dropped it rather than
> make up the docs ;)

OK

> > +     /* VIO should be between 1.65V and VDD */
> > +     ret = regulator_get_voltage(gp2ap002->vdd);
> > +     if (ret < 0) {
> > +             dev_err(dev, "failed to get VIO voltage\n");
>
> VDD. I'll fix that up if I don't find anything else.

Thanks!

I checked the result on your testing branch and it looks good to me.

Yours,
Linus Walleij