Re: [PATCH v4 2/4] mfd: lubbock_cplds: add lubbock IO board

[Lee Jones]

On Tue, 17 Feb 2015, Nicolas Pitre wrote:
> On Tue, 17 Feb 2015, Lee Jones wrote:
> 
> > Arnd, Greg,
> > 
> >   Perhaps you have some ideas WRT programmables (PLDs/CPLDs/FPGAs)?
> 
> FWIW...
> 
> The Lubbock is an ancient development board (circa 2003) using a CPLD to 
> multiplex a couple things on the board.  I really doubt anyone would 
> reprogram this CPLD at this point. So I'd treat it just like another 
> interrupt controller + random peripherals that will never change.  And 
> yes, maybe a more appropriate name is needed.

Understood.  However, I'm tempted to occupy some higher ground here
and say that there will always be (good?) excuses to shoehorn drivers
into subsystems which they don't truly belong.  Rather than make
exceptions I'd rather see an implementation which would also serve
subsequent attempts to upstream these types of devices.

> And I happen to have one such beast on my desk wasting significant 
> realestate and picking up dust.  I don't think I booted it even once in 
> the last 3 years.  I'm seriously considering a trip to the recycling 
> facility to dispose of it unless someone wants it and is ready to pay 
> for the shipping.

Shameless.  I think you were looking for Craig's List. ;)

> > On Mon, 16 Feb 2015, Robert Jarzmik wrote:
> > > Lee Jones wrote:
> > > > What's all this?  Please configure your mail client correctly.
> > > >
> > > > For advice, see:
> > > >
> > > >   Documentation/email-clients.txt
> > > While at day work, I have only access to web mail ...
> > 
> > Probably best to hold off your reply until you get home then.  It's
> > not just a formatting issue, you also exposed many raw email addresses
> > to the internet, which are easily harvested up by web crawlers.
> > 
> > See this:
> >   https://lkml.org/lkml/2015/2/16/247
> > 
> > > >>  2) after v2, we _both_ agreed that the accurate name is "cplds"
> > > >>     which exactly what is in this patch
> > > >>     (see device registering with lubbock_cplds).
> > > >
> > > > There is no mention of this decision in the changelogs.  If it's not
> > > > in the change logs, it didn't happen. ;)
> > > Ah right.
> > > 
> > > > I'm still concerned with the fact that the driver file is named using
> > > > and is populated by lots of instances of a "board" name.  I'm sure you
> > > > would share my thoughts is someone submitted a driver called
> > > > beaglebone.c or raspberrypi.c to MFD.
> > > I understand your concern. Arnd, a thought about it ? The only viable
> > > alternative would be to move it to arch/arm/plat-pxa AFAIS.
> > 
> > I don't think this is correct either.  CPLD handling would probably be
> > slightly less out of place in drivers/misc, but perhaps a new
> > subsystem for PLDs/CPLDs/FPGAs would be more appropriate
> > drivers/programmables or similar maybe.
> > 
> > > >> > Besides, this is MFD, where we support single pieces of silicon which
> > > >> > happen to support multiple devices.  I definitely don't want to support
> > > >> > boards here.
> > > >> > You might want to re-think the naming and your (sales) pitch.
> > > >> I might need help. As for the (sales), no comment.
> > > >
> > > > By pitch, I mean to convince me that this belongs in MFD.
> > > I've been trying.
> > 
> > I'm pretty convinced that it doesn't belong in MFD now, but it doesn't
> > mean I'm going to leave you on the curb.  I'd like to help you get it
> > into a better home.
> > 
> > [...]
> > 
> > > > Understanding was lost when I learned that the whole file was centred
> > > > around the premise of board support.  I understand now that this is a
> > > > driver for a CPLD, which I'm not sure is even MFD.  Also, if it is
> > > > really a CPLD driver, shouldn't be named after the manufacturer/model
> > > > number of the chip, rather than the PCB which it's connected to?
> > > >
> > > > I'm also concerned that this driver has no functional CPLD code
> > > > contained.  All you're doing is defining an IRQ domain.  Why then
> > > > isn't this really an drivers/irqchip (Suggested-by: Josh Cartwright)
> > > > driver?
> > > Is not only a irqchip because, as explained at the bottom of the commit message,
> > > quoting myself :
> > >   This patch moves all that handling to a mfd driver. It's only purpose
> > >   for the time being is the interrupt handling, but in the future it
> > >   should encompass all the motherboard CPLDs handling :
> > >    - leds
> > >    - switches
> > >    - hexleds
> > 
> > I had a conversation about this on IRC yesterday and some good
> > points/questions were posed.  This is a difficult area, because you
> > can program these things to do whatever you like.  Depending on the
> > 'intention' (and it is only an intention -- someone else can come
> > along and reprogram these devices on a whim), the CPLD code could live
> > anywhere.  If you wanted to put watchdog functionality in there, then
> > there is an argument for it to live in drivers/watchdog, etc etc.  So
> > just because the plan is to support a few (i.e. more than one) simple
> > devices, it doesn't necessarily mean that the handling should be done
> > in MFD.
> > 
> > Yesterday I was asked "Are you wanting to restrict drivers in
> > drivers/mfd to those that make use of MFD_CORE functionality?".  My
> > answer to that was "No, however; I only want devices which
> > _intrinsically_ operate in multiple subsystems", which these
> > programmables no not do.
> > 
> > FYI, you're not on your own here.  There is at least one of these
> > devices in the kernel already and upon a short inspection there
> > appears to be a number of Out-of-Tree (OoT) drivers out there which
> > will require a home in Mainline sooner or later.
> > 


-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/