| Message ID | 1805d1ddb5bbce8e86164e66421ddde481cce4f9.1668129763.git.william.gray@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Migrate i8255 GPIO drivers to regmap API | expand |
On Thu, Nov 10, 2022 at 08:55:50PM -0500, William Breathitt Gray wrote: > If you only have reg_dat_base set, then it is input-only; if you only > have reg_set_base set, then it is output-only. Thus, we can always set > gpio_chip get_direction to gpio_regmap_get_direction and return > GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective > register base addresses configuration. Seems legit to me. Have you checked if we have any gpio-regmap drivers that have something like this in their configuration already? In such cases we need to be sure they behave as expected.
On Sun, Nov 13, 2022 at 02:40:17PM +0200, Andy Shevchenko wrote: > On Thu, Nov 10, 2022 at 08:55:50PM -0500, William Breathitt Gray wrote: > > If you only have reg_dat_base set, then it is input-only; if you only > > have reg_set_base set, then it is output-only. Thus, we can always set > > gpio_chip get_direction to gpio_regmap_get_direction and return > > GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective > > register base addresses configuration. > > Seems legit to me. Have you checked if we have any gpio-regmap drivers that > have something like this in their configuration already? In such cases we need > to be sure they behave as expected. > > From the code perspective: > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like the change in this patch will cause problems for them, but I'll let Michael Walle and Robert Marko comment if they see issues here. William Breathitt Gray > > Signed-off-by: William Breathitt Gray <william.gray@linaro.org> > > --- > > drivers/gpio/gpio-regmap.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c > > index 6383136cbe59..f907c9c19fce 100644 > > --- a/drivers/gpio/gpio-regmap.c > > +++ b/drivers/gpio/gpio-regmap.c > > @@ -111,6 +111,11 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip, > > unsigned int base, val, reg, mask; > > int invert, ret; > > > > + if (gpio->reg_dat_base && !gpio->reg_set_base) > > + return GPIO_LINE_DIRECTION_IN; > > + if (gpio->reg_set_base && !gpio->reg_dat_base) > > + return GPIO_LINE_DIRECTION_OUT; > > + > > if (gpio->reg_dir_out_base) { > > base = gpio_regmap_addr(gpio->reg_dir_out_base); > > invert = 0; > > @@ -265,8 +270,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config > > else if (gpio->reg_set_base) > > chip->set = gpio_regmap_set; > > > > + chip->get_direction = gpio_regmap_get_direction; > > if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) { > > - chip->get_direction = gpio_regmap_get_direction; > > chip->direction_input = gpio_regmap_direction_input; > > chip->direction_output = gpio_regmap_direction_output; > > } > > -- > > 2.38.1 > > > > -- > With Best Regards, > Andy Shevchenko > >
On Sun, Nov 13, 2022 at 2:21 PM William Breathitt Gray <william.gray@linaro.org> wrote: > > On Sun, Nov 13, 2022 at 02:40:17PM +0200, Andy Shevchenko wrote: > > On Thu, Nov 10, 2022 at 08:55:50PM -0500, William Breathitt Gray wrote: > > > If you only have reg_dat_base set, then it is input-only; if you only > > > have reg_set_base set, then it is output-only. Thus, we can always set > > > gpio_chip get_direction to gpio_regmap_get_direction and return > > > GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective > > > register base addresses configuration. > > > > Seems legit to me. Have you checked if we have any gpio-regmap drivers that > > have something like this in their configuration already? In such cases we need > > to be sure they behave as expected. > > > > From the code perspective: > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and > SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types > TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like > the change in this patch will cause problems for them, but I'll let > Michael Walle and Robert Marko comment if they see issues here. Hi, sorry for the late reply. This should work fine for TN48M. Regards, Robert > > William Breathitt Gray > > > > Signed-off-by: William Breathitt Gray <william.gray@linaro.org> > > > --- > > > drivers/gpio/gpio-regmap.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c > > > index 6383136cbe59..f907c9c19fce 100644 > > > --- a/drivers/gpio/gpio-regmap.c > > > +++ b/drivers/gpio/gpio-regmap.c > > > @@ -111,6 +111,11 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip, > > > unsigned int base, val, reg, mask; > > > int invert, ret; > > > > > > + if (gpio->reg_dat_base && !gpio->reg_set_base) > > > + return GPIO_LINE_DIRECTION_IN; > > > + if (gpio->reg_set_base && !gpio->reg_dat_base) > > > + return GPIO_LINE_DIRECTION_OUT; > > > + > > > if (gpio->reg_dir_out_base) { > > > base = gpio_regmap_addr(gpio->reg_dir_out_base); > > > invert = 0; > > > @@ -265,8 +270,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config > > > else if (gpio->reg_set_base) > > > chip->set = gpio_regmap_set; > > > > > > + chip->get_direction = gpio_regmap_get_direction; > > > if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) { > > > - chip->get_direction = gpio_regmap_get_direction; > > > chip->direction_input = gpio_regmap_direction_input; > > > chip->direction_output = gpio_regmap_direction_output; > > > } > > > -- > > > 2.38.1 > > > > > > > -- > > With Best Regards, > > Andy Shevchenko > > > >
Am 2022-11-13 14:21, schrieb William Breathitt Gray: > On Sun, Nov 13, 2022 at 02:40:17PM +0200, Andy Shevchenko wrote: >> On Thu, Nov 10, 2022 at 08:55:50PM -0500, William Breathitt Gray >> wrote: >> > If you only have reg_dat_base set, then it is input-only; if you only >> > have reg_set_base set, then it is output-only. Thus, we can always set >> > gpio_chip get_direction to gpio_regmap_get_direction and return >> > GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective >> > register base addresses configuration. >> >> Seems legit to me. Have you checked if we have any gpio-regmap drivers >> that >> have something like this in their configuration already? In such cases >> we need >> to be sure they behave as expected. >> >> From the code perspective: >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and > SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types > TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look > like > the change in this patch will cause problems for them, but I'll let > Michael Walle and Robert Marko comment if they see issues here. For the sl28cpld driver this shouldn't be a problem. So for that Acked-by: Michael Walle <michael@walle.cc> But back when I wrote gpio-regmap the bgpio served as a blue print. There is the same handling. If you look at gpiolib-sysfs.c there is a comment about the direction property: * MAY BE OMITTED if kernel won't allow direction changes So from a gpiolib/sysfs POV I'm not sure about this change. Does get_direction == NULL means setting the direction isn't possible? OTHO there is a fat "MAY" :) Which brings me to the question of "why this change?". The commit message doesn't mention it. Just out of curiosity. -michael
On Wed, Nov 16, 2022 at 04:41:30PM +0100, Michael Walle wrote: > Am 2022-11-13 14:21, schrieb William Breathitt Gray: > > On Sun, Nov 13, 2022 at 02:40:17PM +0200, Andy Shevchenko wrote: > > > On Thu, Nov 10, 2022 at 08:55:50PM -0500, William Breathitt Gray > > > wrote: > > > > If you only have reg_dat_base set, then it is input-only; if you only > > > > have reg_set_base set, then it is output-only. Thus, we can always set > > > > gpio_chip get_direction to gpio_regmap_get_direction and return > > > > GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective > > > > register base addresses configuration. > > > > > > Seems legit to me. Have you checked if we have any gpio-regmap > > > drivers that > > > have something like this in their configuration already? In such > > > cases we need > > > to be sure they behave as expected. > > > > > > From the code perspective: > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > I see gpio-sl28cpld has two device types SL28CPLD_GPO (output-only) and > > SL28CPLD_GPI (input-only); gpio-tn48m similarly has two device types > > TN48M_GPO (output-only) and TN48M_GPI (input-only). It doesn't look like > > the change in this patch will cause problems for them, but I'll let > > Michael Walle and Robert Marko comment if they see issues here. > > For the sl28cpld driver this shouldn't be a problem. So for that > Acked-by: Michael Walle <michael@walle.cc> > > But back when I wrote gpio-regmap the bgpio served as a blue print. > There is the same handling. If you look at gpiolib-sysfs.c there > is a comment about the direction property: > > * MAY BE OMITTED if kernel won't allow direction changes > > So from a gpiolib/sysfs POV I'm not sure about this change. Does > get_direction == NULL means setting the direction isn't possible? > OTHO there is a fat "MAY" :) > > Which brings me to the question of "why this change?". The commit > message doesn't mention it. Just out of curiosity. Sysfs shouldn't be considered nowadays as anything but deprecated and not-to-use interface. Hence, I don't care what it tells there.
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 6383136cbe59..f907c9c19fce 100644 --- a/drivers/gpio/gpio-regmap.c +++ b/drivers/gpio/gpio-regmap.c @@ -111,6 +111,11 @@ static int gpio_regmap_get_direction(struct gpio_chip *chip, unsigned int base, val, reg, mask; int invert, ret; + if (gpio->reg_dat_base && !gpio->reg_set_base) + return GPIO_LINE_DIRECTION_IN; + if (gpio->reg_set_base && !gpio->reg_dat_base) + return GPIO_LINE_DIRECTION_OUT; + if (gpio->reg_dir_out_base) { base = gpio_regmap_addr(gpio->reg_dir_out_base); invert = 0; @@ -265,8 +270,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config else if (gpio->reg_set_base) chip->set = gpio_regmap_set; + chip->get_direction = gpio_regmap_get_direction; if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) { - chip->get_direction = gpio_regmap_get_direction; chip->direction_input = gpio_regmap_direction_input; chip->direction_output = gpio_regmap_direction_output; }
If you only have reg_dat_base set, then it is input-only; if you only have reg_set_base set, then it is output-only. Thus, we can always set gpio_chip get_direction to gpio_regmap_get_direction and return GPIO_LINE_DIRECTION_IN/GPIO_LINE_DIRECTION_OUT given the respective register base addresses configuration. Signed-off-by: William Breathitt Gray <william.gray@linaro.org> --- drivers/gpio/gpio-regmap.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)