RE: [PATCH v2 2/2] drivers: pinctrl: renesas: Add RZ/G2L POEG driver support

[Biju Das <biju.das.jz@xxxxxxxxxxxxxx>]

Hi Geert,

Thanks for the feedback.

> Subject: Re: [PATCH v2 2/2] drivers: pinctrl: renesas: Add RZ/G2L POEG
> driver support
> 
> Hi Biju,
> 
> On Fri, Jul 29, 2022 at 2:25 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > The output pins of the RZ/G2L general PWM timer (GPT) can be disabled
> > by using the port output enabling function for the GPT (POEG).
> >
> > This patch series add basic support using s/w control through sysfs to
> > enable/disable output from GPT.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > ---
> > v1->v2:
> >  * Renamed the file poeg-rzg2l->rzg2l-poeg
> >  * Removed the macro POEGG as there is only single register and
> >    updated rzg2l_poeg_write() and rzg2l_poeg_read()
> >  * Updated error handling in probe()
> 
> Thanks for the update!
> 
> > --- /dev/null
> > +++ b/drivers/pinctrl/renesas/poeg/Kconfig
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +config POEG_RZG2L
> > +       tristate "RZ/G2L poeg support"
> > +       depends on ARCH_RZG2L
> > +       depends on PWM_RZG2L_GPT
> 
> These are not hard dependencies, right?
> And PWM_RZG2L_GPT (sort of) implies ARCH_RZG2L.
> 
> So I think the above two lines should be replaced by
> 
>      depends on PWM_RZG2L_GPT || COMPILE_TEST

Agreed.

> 
> > +       depends on HAS_IOMEM
> > +       help
> > +         This driver exposes the General Port Output Enable for PWM
> found
> > +         in RZ/G2L.
> > +
> > +         To compile this driver as a module, choose M here: the
> module
> > +         will be called poeg-rzg2l.
> 
> > --- /dev/null
> > +++ b/drivers/pinctrl/renesas/poeg/rzg2l-poeg.c
> > @@ -0,0 +1,147 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L Port Output Enable for GPT (POEG) driver
> > + *
> > + * Copyright (C) 2022 Renesas Electronics Corporation  */
> > +
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/reset.h>
> > +
> > +#define POEGG_SSF      BIT(3)
> > +
> > +struct rzg2l_poeg_chip {
> > +       struct reset_control *rstc;
> > +       void __iomem *mmio;
> > +};
> > +
> > +static void rzg2l_poeg_write(struct rzg2l_poeg_chip *chip, u32 data)
> > +{
> > +       iowrite32(data, chip->mmio);
> > +}
> > +
> > +static u32 rzg2l_poeg_read(struct rzg2l_poeg_chip *chip) {
> > +       return ioread32(chip->mmio);
> > +}
> > +
> > +static ssize_t output_disable_store(struct device *dev,
> > +                                   struct device_attribute *attr,
> > +                                   const char *buf, size_t count) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct rzg2l_poeg_chip *chip = platform_get_drvdata(pdev);
> 
> chip = dev_get_drvdata(dev)

OK.

> 
> > +       unsigned int val;
> > +       u32 reg_val;
> > +       int ret;
> > +
> > +       ret = kstrtouint(buf, 0, &val);
> > +       if (ret)
> > +               return ret;
> > +
> > +       reg_val = rzg2l_poeg_read(chip);
> > +       if (val)
> > +               reg_val |= POEGG_SSF;
> > +       else
> > +               reg_val &= ~POEGG_SSF;
> > +
> > +       rzg2l_poeg_write(chip, reg_val);
> > +
> > +       return ret ? : count;
> > +}
> > +
> > +static ssize_t output_disable_show(struct device *dev,
> > +                                  struct device_attribute *attr, char
> > +*buf) {
> > +       struct platform_device *pdev = to_platform_device(dev);
> > +       struct rzg2l_poeg_chip *chip = platform_get_drvdata(pdev);
> 
> chip = dev_get_drvdata(dev)
OK.

> 
> > +       u32 reg;
> > +
> > +       reg = rzg2l_poeg_read(chip);
> > +
> > +       return sprintf(buf, "%u\n", (reg & POEGG_SSF) ? 1 : 0);
> 
> sysfs_emit().
> 
> > +}
> > +static DEVICE_ATTR_RW(output_disable);
> 
> Probably you want to document these properties under Documentation/,

OK, will document this as it is one of the option supported by hardware.

 or
> do you intend to have this as an interim solution?

User space can decide and disable the output, if needed using the above property.

> TBH, I still don't know if this is the proper way to handle POEG, but I
> see no better alternative...

Going forward will add more POEG features like

* Input level detection of the GTETRGA to GTETRGD pins.
* Output-disable request from the GPT.

Already BSP has some implementation for handling some of this features[1]

[1] https://github.com/renesas-rz/rz_linux-cip/blob/rz-5.10-cip3/drivers/pwm/gpt-rzg2l.c#L517

Cheers,
Biju