Date:   Sat, 25 Feb 2023 17:22:55 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Krzysztof Kozlowski <krzk@kernel.org>
Cc:     marius.cristea@microchip.com, lars@metafoo.de, robh+dt@kernel.org,
        linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 2/2] iio: adc: adding support for pac193x
Message-ID: <20230225172255.7b79374a@jic23-huawei>
In-Reply-To: <20230225171954.06d396f3@jic23-huawei>
References: <20230220123232.413029-1-marius.cristea@microchip.com>
        <20230220123232.413029-3-marius.cristea@microchip.com>
        <8ad45f24-dc78-dfc4-23d2-ad5a5d0e42d4@kernel.org>
        <20230225171954.06d396f3@jic23-huawei>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 25 Feb 2023 17:19:54 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 21 Feb 2023 14:46:08 +0100
> Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> > On 20/02/2023 13:32, marius.cristea@microchip.com wrote:  
> > > From: Marius Cristea <marius.cristea@microchip.com>
> > > 
> > > This is the iio driver for Microchip
> > > PAC193X series of Power Monitor with Accumulator chip family.
> > > 
> > > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> > > ---
> > >  MAINTAINERS               |    7 +
> > >  drivers/iio/adc/Kconfig   |   12 +
> > >  drivers/iio/adc/Makefile  |    1 +
> > >  drivers/iio/adc/pac193x.c | 2072 +++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 2092 insertions(+)
> > >  create mode 100644 drivers/iio/adc/pac193x.c
> > >     
> > 
> > 
> > Thank you for your patch. There is something to discuss/improve.
> >   
> > > +
> > > +#define PAC193X_NEG_PWR_CH1_BIDI(x)	((x) ? BIT(7) : 0)
> > > +#define PAC193X_NEG_PWR_CH2_BIDI(x)	((x) ? BIT(6) : 0)
> > > +#define PAC193X_NEG_PWR_CH3_BIDI(x)	((x) ? BIT(5) : 0)
> > > +#define PAC193X_NEG_PWR_CH4_BIDI(x)	((x) ? BIT(4) : 0)
> > > +#define PAC193X_NEG_PWR_CH1_BIDV(x)	((x) ? BIT(3) : 0)
> > > +#define PAC193X_NEG_PWR_CH2_BIDV(x)	((x) ? BIT(2) : 0)
> > > +#define PAC193X_NEG_PWR_CH3_BIDV(x)	((x) ? BIT(1) : 0)
> > > +#define PAC193X_NEG_PWR_CH4_BIDV(x)	((x) ? BIT(0) : 0)
> > > +
> > > +/*
> > > + * Universal Unique Identifier (UUID),
> > > + * 033771E0-1705-47B4-9535-D1BBE14D9A09, is
> > > + * reserved to Microchip for the PAC193X and must not be changed
> > > + */
> > > +#define PAC193X_DSM_UUID		"033771E0-1705-47B4-9535-D1BBE14D9A09"
> > > +
> > > +enum pac193x_ids {
> > > +	pac1934,
> > > +	pac1933,
> > > +	pac1932,
> > > +	pac1931    
> > 
> > Enums are usually uppercase.  
> 
> I'm not sure there is anything in coding standard around that and a grep finds
> a mixture of the two when it comes to ones used for IDs.  Mind you uppercase
> is fine :)
I take it back. Is indeed in coding style doc.  Glad checkpatch doesn't check for
this though as we'd get 1000s of 'fixes' if it did and in most cases it doesn't
hurt readability.  I'll try and be more consistent on this in review going forwards!

Thanks!

Jonathan

> 
> 
> > 
> > 
> > 
> > Best regards,
> > Krzysztof
> >   
>