Message-ID: <ff321353fa27a156a0eab9cc9674dfad760f22a8.camel@redhat.com>
Subject: Re: [PATCH V5 0/6] Raspberry Pi Sense HAT driver
From:   Nicolas Saenz Julienne <nsaenzju@redhat.com>
To:     Charles Mirabile <cmirabil@redhat.com>,
        linux-kernel@vger.kernel.org
Cc:     Serge Schneider <serge@raspberrypi.org>,
        Stefan Wahren <stefan.wahren@i2se.com>,
        Mattias Brugger <mbrugger@suse.com>,
        linux-rpi-kernel@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, fedora-rpi@googlegroups.com,
        Mwesigwa Guma <mguma@redhat.com>,
        Joel Savitz <jsavitz@redhat.com>
Date:   Mon, 13 Dec 2021 12:29:52 +0100
In-Reply-To: <20211210221033.912430-1-cmirabil@redhat.com>
References: <20211210221033.912430-1-cmirabil@redhat.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.2 (3.42.2-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Charles, Thanks for iterating on this.

On Fri, 2021-12-10 at 17:10 -0500, Charles Mirabile wrote:
> Suggestions from v4 not addressed yet:
> 	- Use mfd_cells for the joystick and display platform devices
> 	  (as suggested by Matthias Brugger) or remove sensehat-core.c
> 	  entirely and use simple-mfd-i2c instead (as suggested by
> 	  Nicolás Sáenz Julienne).
> 	  	- while these suggestions might make great improvements
> 	  	  for a version two of the driver they would require a
> 	  	  large rewrite to move all of the complexity in core
> 	  	  elsewhere. While certainly possible and something
> 	  	  we want to iterate on and achieve eventually, we
> 	  	  think saving it for down the road makes sense at this
> 	  	  time.

Sorry for insisting, but I'm not convinced by this approach. The way you design
your driver affects its devicetree bindings, once we settle on those any
upgrade will have to be backwards compatible (i.e. an old DT would have to work
with the new implementation). This makes changing this further down the road
unlikely.

In terms of complexity, most of it is there due to not using the APIs and
facilities the mfd subsystem provides. See my comments in the driver.

Regards,

-- 
Nicolás Sáenz