Hi Krzysztof, On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote: > On 16/09/2022 14:07, B. Niedermayr wrote: > > From: Benedikt Niedermayr <benedikt.niedermayr@xxxxxxxxxxx> > > > > The waitpin polarity can be configured via the WAITPIN<X>POLARITY bits > > in the GPMC_CONFIG register. This is currently not supported by the > > driver. This patch adds support for setting the required register bits > > with the "gpmc,wait-pin-polarity" dt-property. > > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@xxxxxxxxxxx> > > --- > > drivers/memory/omap-gpmc.c | 27 +++++++++++++++++++++++++ > > include/linux/platform_data/gpmc-omap.h | 6 ++++++ > > 2 files changed, 33 insertions(+) > > > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > > index ea495e93766b..2853fc28bccc 100644 > > --- a/drivers/memory/omap-gpmc.c > > +++ b/drivers/memory/omap-gpmc.c > > @@ -132,6 +132,7 @@ > > #define GPMC_CONFIG_DEV_SIZE 0x00000002 > > #define GPMC_CONFIG_DEV_TYPE 0x00000003 > > > > +#define GPMC_CONFIG_WAITPINPOLARITY(pin) (BIT(pin) << 8) > > #define GPMC_CONFIG1_WRAPBURST_SUPP (1 << 31) > > #define GPMC_CONFIG1_READMULTIPLE_SUPP (1 << 30) > > #define GPMC_CONFIG1_READTYPE_ASYNC (0 << 29) > > @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p) > > > > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1); > > > > + if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) { > > + config1 = gpmc_read_reg(GPMC_CONFIG); > > + > > + if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW) > > + config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > > + else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH) > > + config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > > + > > + gpmc_write_reg(GPMC_CONFIG, config1); > > What happens if wait pin is shared and you have different polarities in > both of devices? In this case the second one wins and will overwrite the polarity of the first one. But that would be the result of a misconfiguration in the DT. I'm not sure how to proceed here? Does it make sense to add a check for different waitpin polarities? > > > + } > > + > > return 0; > > } > > > > @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p) > > __func__); > > } > > > > + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; > > + > > if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { > > + if (!of_property_read_u32(np, "gpmc,wait-pin-polarity", > > + &p->wait_pin_polarity)) { > > + if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH && > > + p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW && > > + p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) { > > WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it. This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init value (right before the if clause). This helps in case no configuration from DT is done where the GPMC registers should stay untouched. > > > + pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n", > > dev_err, not pr_err Ok. I didn't want to introduce dev_* functions. Currently pr_* functions where used all over the place. > > > + __func__, p->wait_pin, p->wait_pin_polarity); > > Skip __func__ > Ok. > > + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; > > + } > > + } else { > > + pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__); > > Ditto. Ok. > > > + } > > + > > p->wait_on_read = of_property_read_bool(np, > > "gpmc,wait-on-read"); > > p->wait_on_write = of_property_read_bool(np, > > diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h > > index c9cc4e32435d..c46c28069c31 100644 > > --- a/include/linux/platform_data/gpmc-omap.h > > +++ b/include/linux/platform_data/gpmc-omap.h > > @@ -136,6 +136,11 @@ struct gpmc_device_timings { > > #define GPMC_MUX_AAD 1 /* Addr-Addr-Data multiplex */ > > #define GPMC_MUX_AD 2 /* Addr-Data multiplex */ > > > > +/* Wait pin polarity values */ > > +#define WAITPINPOLARITY_DEFAULT -1 > > Missing prefix. This is a global header. Ok. Makes sense. > > > +#define WAITPINPOLARITY_ACTIVE_LOW 0 > > +#define WAITPINPOLARITY_ACTIVE_HIGH 1 > > + > > struct gpmc_settings { > > bool burst_wrap; /* enables wrap bursting */ > > bool burst_read; /* enables read page/burst mode */ > > @@ -149,6 +154,7 @@ struct gpmc_settings { > > u32 device_width; /* device bus width (8 or 16 bit) */ > > u32 mux_add_data; /* multiplex address & data */ > > u32 wait_pin; /* wait-pin to be used */ > > + u32 wait_pin_polarity; /* wait-pin polarity */ > > Skip the comment. You just copied the name of variable. Such comments > are useless. > > We do not have KPIs in kernel to achieve some comment-ratio... > Ok, makes sense. > Best regards, > Krzysztof