Re: [RFC PATCH 10/13] gpio: bd71828: Initial support for ROHM BD71828 PMIC GPIOs

[Vaittinen, Matti]

Thanks again Bart =)


On Mon, 2019-10-21 at 16:36 +0200, Bartosz Golaszewski wrote:
> pon., 21 paÅ 2019 o 09:00 Vaittinen, Matti
> <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> napisaÅ(a):
> > Hello Bartosz,
> > 
> > +
> > > > +       bdgpio->chip.dev = &pdev->dev;
> > > > +       bdgpio->gpio.parent = pdev->dev.parent;
> > > > +       bdgpio->gpio.label = "bd71828-gpio";
> > > > +       bdgpio->gpio.owner = THIS_MODULE;
> > > > +       bdgpio->gpio.get_direction = bd71828_get_direction;
> > > > +       bdgpio->gpio.set_config = bd71828_gpio_set_config;
> > > > +       bdgpio->gpio.can_sleep = true;
> > > > +       bdgpio->gpio.get = bd71828_gpio_get;
> > > > +       bdgpio->gpio.set = bd71828_gpio_set;
> > > 
> > > Not implementing direction_output() and direction_input() here
> > > will
> > > results in warnings from the GPIO framework: for instance you
> > > implement set() but not direction_output(). I'd say: just add
> > > those
> > > callbacks and return an error if they're called for invalid lines
> > > (for
> > > instance: direction_output() being called for line 3).
> > 
> > Ok. I will implement dummy functions.
> > 
> > But out of the curiosity - why the GPIO core emits the warnings if
> > these are not implemented? I think the core should not require "no-
> > operation" functions to be implemented for pins which don't support
> > both of the directions. GPIO core could only emit warning if it
> > needs
> > to set direction to something the HW does not support. That would
> > avoid
> > adding the dummy functions to all of the drivers, right?
> > 
> 
> I looked at the code again and it seems I was wrong. If we don't have
> direction_input() or direction_output() we check the actual direction
> with get_direction() before emitting any warnings and if there's no
> direction_output(), but line is in input mode then all's fine. In
> other words: false alarm, and you can keep it this way.

Thanks for clarifying this - it makes sense :) I wont change this then.

> > > Don't you need a MODULE_ALIAS() here since this is an MFD sub-
> > > module?
> > 
> > I must admit I don't know the details of how module loading is
> > done. I
> > used system where modules are load by scripts. (I guess the module
> > alias could be used to allow automatic module loading [by udev?])
> > 
> > Can you please educate me - If I add module aliases matching the
> > sub-
> > device name given in in MFD cell - should the sub module loading be
> > automatic when MFD driver gets probed? For some reason I didn't get
> > that working on my test bed. Or maybe I misunderstood something.
> > 
> 
> If the gpio module is a sub-node on the device tree than you may need
> to use a sub-compatible to get the module loaded by udev.

Just found out that the last update broke my test bed I2C completely.
I'll experiment with the MODULE_ALIAS when I get my board running...

I don't want to add own DT node for gpio (and all other sub devices).
That shouldn't be needed. It really should be enough to kick the MFD
part from DT using the compatible - subdevices should be load by MFD
without having own DT compatibles for them. But I'll see how this works
out - thanks!

Br,
	Matti Vaittinen