Re: [PATCH] gpio: pca953x: Add Maxim MAX7313 PWM support

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

Hello,

On Mon, Nov 04, 2019 at 04:32:23PM +0100, Bartosz Golaszewski wrote:
> pon., 4 lis 2019 o 16:11 Miquel Raynal <miquel.raynal@xxxxxxxxxxx> napisał(a):
> > Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote on Tue, 15 Oct 2019
> > 17:55:33 +0300:
> >
> > > Or other way around. PWM registers GPIO (which actually I prefer since
> > > we have PCA9685 case where PWM provides GPIO functionality, though via
> > > different means)
> > >
> >
> > Can I have your input on this proposal?
> >
> > On one hand I agree that the GPIO driver is already quite big due to
> > its genericity and the amount of controllers it supports, on the other
> > hand:
> > 1/ Registering a PWM driver from the GPIO core seems strange. Maybe
> > registering a platform device could do the trick but I am not convinced
> > it is worth the trouble.
> > 2/ Putting the PWM logic in the drivers/pwm/ directory is not very
> > convenient as the object file will have to be embedded within the GPIO
> > one; this line in drivers/gpio/Makefile would be horrible:
> > ... += gpio-pca953x.o ../pwm/pwm-max7313.o (not even sure it works)
> > 3/ In any cases, the regmap's ->readable_reg(), ->writable_reg()
> > callbacks shall be tweaked to turn the PWM registers accessible, so we
> > would still have PWM related code in the PCA953x GPIO driver.
> >
> > In the end, I wonder if keeping everything in one file is not better?
> > Eventually I can create a separate file and fill it with just the PWM
> > helpers/hooks. Please advise on the better solution for you, I'll wait
> > your feedback before addressing Thierry Reding's other review and
> > resubmit.
> >
> 
> I'm not sure if this has been discussed, but is it possible to create
> an MFD driver for this chip and conditionally plug in the GPIO part
> from pca953x? I don't like the idea of having PWM functionality in a
> GPIO driver either.

I didn't check the manual or driver in depth, but I guess it doesn't
match the MFD abstraction well. (That is, the PWM and GPIO parts live in
different address areas of the chip and can be used independently of
each other.)

While it's not nice to have a driver that provides two different devices
(here: gpio controller and pwm controller) similar things are not
unseen. And for example the splitting of watchdog
(drivers/watchdog/stmp3xxx_rtc_wdt.c) and rtc
(drivers/rtc/rtc-stmp3xxx.c) of the device in the mx28 is more trouble
than worth.

So I'd vote for putting it in a single file that lives where the
bigger/more complex part fits to. So assuming that's the GPIO part (as
the driver supports several variants and not all of them have a PWM
function if I'm not mistaken) having it in drivers/gpio is fine for me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |