| Message ID | 20161111203021.3384-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | New |
| Headers | show |
Hi Niklas, Thank you for the patch. On Friday 11 Nov 2016 21:30:17 Niklas Söderlund wrote: > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > On some SoC there are no simple mapping of pins to bias register bits > and a lookup table is needed. This logic is already implemented in some > SoC specific drivers that could benefit from a generic implementation. > > Add helpers to deal with the lookup which later can be used by the SoC > specific drivers. The logic used to lookup are different from the one it > aims to replace, this is intentional. This new method reduces the memory > consumption at the cost of increased CPU usage and fix a bug where a > WARN() would incorrectly be triggered if the register offset is 0. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/pinctrl/sh-pfc/core.c | 16 ++++++++++++++++ > drivers/pinctrl/sh-pfc/core.h | 4 ++++ > drivers/pinctrl/sh-pfc/sh_pfc.h | 6 ++++++ > 3 files changed, 26 insertions(+) > > diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c > index f3a8897..8e38df7 100644 > --- a/drivers/pinctrl/sh-pfc/core.c > +++ b/drivers/pinctrl/sh-pfc/core.c > @@ -389,6 +389,22 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned > mark, int pinmux_type) return 0; > } > > +int sh_pfc_pin_to_bias_info(const struct sh_pfc_bias_info *pullups, > + unsigned int num, unsigned int pin, > + struct sh_pfc_bias_info *info) How about returning a const struct sh_pfc_bias_info * instead ? This would avoid copying the data into the *info argument. You could then rename pullups to info (I don't really like the pullups name as the table can contain pulldown information as well). > +{ > + unsigned int i; > + > + for (i = 0; i < num; i++) { > + if (pullups[i].pin == pin) { > + *info = pullups[i]; > + return 0; > + } > + } > + > + return WARN_ON_ONCE(-EINVAL); > +} > + > static int sh_pfc_init_ranges(struct sh_pfc *pfc) > { > struct sh_pfc_pin_range *range; > diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h > index 0bbdea58..d902cd2 100644 > --- a/drivers/pinctrl/sh-pfc/core.h > +++ b/drivers/pinctrl/sh-pfc/core.h > @@ -33,4 +33,8 @@ void sh_pfc_write_reg(struct sh_pfc *pfc, u32 reg, > unsigned int width, int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned > int pin); > int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type); > > +int sh_pfc_pin_to_bias_info(const struct sh_pfc_bias_info *pullups, > + unsigned int num, unsigned int pin, > + struct sh_pfc_bias_info *info); > + > #endif /* __SH_PFC_CORE_H__ */ > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h > b/drivers/pinctrl/sh-pfc/sh_pfc.h index 2345421..fc82b6d 100644 > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > @@ -189,6 +189,12 @@ struct sh_pfc_window { > unsigned long size; > }; > > +struct sh_pfc_bias_info { > + unsigned int pin; You should make this a u16, otherwise the structure will be padded to 8 bytes anyway, removing the benefit of storing reg and bit in 16 bits. > + u16 reg : 11; > + u16 bit : 5; > +}; > + > struct sh_pfc_pin_range; > > struct sh_pfc {
Hi Laurent, Thanks you for the feedback. On 2016-11-12 03:46:11 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Friday 11 Nov 2016 21:30:17 Niklas Söderlund wrote: > > From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > On some SoC there are no simple mapping of pins to bias register bits > > and a lookup table is needed. This logic is already implemented in some > > SoC specific drivers that could benefit from a generic implementation. > > > > Add helpers to deal with the lookup which later can be used by the SoC > > specific drivers. The logic used to lookup are different from the one it > > aims to replace, this is intentional. This new method reduces the memory > > consumption at the cost of increased CPU usage and fix a bug where a > > WARN() would incorrectly be triggered if the register offset is 0. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/pinctrl/sh-pfc/core.c | 16 ++++++++++++++++ > > drivers/pinctrl/sh-pfc/core.h | 4 ++++ > > drivers/pinctrl/sh-pfc/sh_pfc.h | 6 ++++++ > > 3 files changed, 26 insertions(+) > > > > diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c > > index f3a8897..8e38df7 100644 > > --- a/drivers/pinctrl/sh-pfc/core.c > > +++ b/drivers/pinctrl/sh-pfc/core.c > > @@ -389,6 +389,22 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned > > mark, int pinmux_type) return 0; > > } > > > > +int sh_pfc_pin_to_bias_info(const struct sh_pfc_bias_info *pullups, > > + unsigned int num, unsigned int pin, > > + struct sh_pfc_bias_info *info) > > How about returning a const struct sh_pfc_bias_info * instead ? This would > avoid copying the data into the *info argument. You could then rename pullups > to info (I don't really like the pullups name as the table can contain > pulldown information as well). Good idea, I will update the patch in this way. > > > +{ > > + unsigned int i; > > + > > + for (i = 0; i < num; i++) { > > + if (pullups[i].pin == pin) { > > + *info = pullups[i]; > > + return 0; > > + } > > + } > > + > > + return WARN_ON_ONCE(-EINVAL); > > +} > > + > > static int sh_pfc_init_ranges(struct sh_pfc *pfc) > > { > > struct sh_pfc_pin_range *range; > > diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h > > index 0bbdea58..d902cd2 100644 > > --- a/drivers/pinctrl/sh-pfc/core.h > > +++ b/drivers/pinctrl/sh-pfc/core.h > > @@ -33,4 +33,8 @@ void sh_pfc_write_reg(struct sh_pfc *pfc, u32 reg, > > unsigned int width, int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned > > int pin); > > int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type); > > > > +int sh_pfc_pin_to_bias_info(const struct sh_pfc_bias_info *pullups, > > + unsigned int num, unsigned int pin, > > + struct sh_pfc_bias_info *info); > > + > > #endif /* __SH_PFC_CORE_H__ */ > > diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h > > b/drivers/pinctrl/sh-pfc/sh_pfc.h index 2345421..fc82b6d 100644 > > --- a/drivers/pinctrl/sh-pfc/sh_pfc.h > > +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h > > @@ -189,6 +189,12 @@ struct sh_pfc_window { > > unsigned long size; > > }; > > > > +struct sh_pfc_bias_info { > > + unsigned int pin; > > You should make this a u16, otherwise the structure will be padded to 8 bytes > anyway, removing the benefit of storing reg and bit in 16 bits. Thanks I did not know that, will update. > > > + u16 reg : 11; > > + u16 bit : 5; > > +}; > > + > > struct sh_pfc_pin_range; > > > > struct sh_pfc {
diff --git a/drivers/pinctrl/sh-pfc/core.c b/drivers/pinctrl/sh-pfc/core.c index f3a8897..8e38df7 100644 --- a/drivers/pinctrl/sh-pfc/core.c +++ b/drivers/pinctrl/sh-pfc/core.c @@ -389,6 +389,22 @@ int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type) return 0; } +int sh_pfc_pin_to_bias_info(const struct sh_pfc_bias_info *pullups, + unsigned int num, unsigned int pin, + struct sh_pfc_bias_info *info) +{ + unsigned int i; + + for (i = 0; i < num; i++) { + if (pullups[i].pin == pin) { + *info = pullups[i]; + return 0; + } + } + + return WARN_ON_ONCE(-EINVAL); +} + static int sh_pfc_init_ranges(struct sh_pfc *pfc) { struct sh_pfc_pin_range *range; diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h index 0bbdea58..d902cd2 100644 --- a/drivers/pinctrl/sh-pfc/core.h +++ b/drivers/pinctrl/sh-pfc/core.h @@ -33,4 +33,8 @@ void sh_pfc_write_reg(struct sh_pfc *pfc, u32 reg, unsigned int width, int sh_pfc_get_pin_index(struct sh_pfc *pfc, unsigned int pin); int sh_pfc_config_mux(struct sh_pfc *pfc, unsigned mark, int pinmux_type); +int sh_pfc_pin_to_bias_info(const struct sh_pfc_bias_info *pullups, + unsigned int num, unsigned int pin, + struct sh_pfc_bias_info *info); + #endif /* __SH_PFC_CORE_H__ */ diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h index 2345421..fc82b6d 100644 --- a/drivers/pinctrl/sh-pfc/sh_pfc.h +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h @@ -189,6 +189,12 @@ struct sh_pfc_window { unsigned long size; }; +struct sh_pfc_bias_info { + unsigned int pin; + u16 reg : 11; + u16 bit : 5; +}; + struct sh_pfc_pin_range; struct sh_pfc {