RE: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
Hi Jon, On Wed, Jun 13, 2012 at 21:09:52, Hunter, Jon wrote: > Sure, but reviewing the function it still looks odd from a readability > standpoint. At least it made me think "what is going on here ...". So a > comment is definitely needed. > > >> > >> 2. A bad setting in the configuration passed. Hopefully, people will > >> stick to the flags but we know that we expect the device type to be a 0 > >> or 2 and so should we check? > > > > Value of device type is something driver has to worry about, not its users, > > they have been provided with two flags, one for NAND & other for NOR. > > Yes, but the driver does not seem to worry about it. In other words, > there is no error checking. Ok so not a big deal a comment should suffice. Ok, comments will be added to make intentions clear Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
Hi Afzal, On 06/13/2012 12:50 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Tue, Jun 12, 2012 at 23:39:32, Hunter, Jon wrote: >> On 06/12/2012 07:58 AM, Mohammed, Afzal wrote: > >>> Thinking again over it, I am feeling above is sufficient, reason same as >>> said earlier, to keep code simple & currently this is sufficient to >>> handle GPMC bit patterns for IPs presently available. What you are >>> suggesting is to take care of the imaginary case when new GPMC IP happens >>> with new device type or size, I think that should be handled when such a >>> scenario happens. Probably, it is better here to add a comment to make >>> intention clear. >> >> That is one possibility but I think that more important reasons are ... >> >> 1. Readability, at first it appears that we are always configuring the >> CS for NAND. However, this is not the case. Maybe a comment here can >> help as you mentioned. > > As far as the users of GPMC is considered there is no confusion. Eg. For > device type, they have been provided with two macros, > > GPMC_CONFIG1_DEVICETYPE_NOR, GPMC_CONFIG1_DEVICETYPE_NAND > > So for NOR, user can feel satisfied by giving NOR flag, little does he know > that driver doesn't do anything with the flag, but he still gets what he want > NOR flag is defined as zero. Yes even if he doesn't specify any type, device > type will be set as NOR, but then that is the default - the only other option Sure, but reviewing the function it still looks odd from a readability standpoint. At least it made me think "what is going on here ...". So a comment is definitely needed. >> >> 2. A bad setting in the configuration passed. Hopefully, people will >> stick to the flags but we know that we expect the device type to be a 0 >> or 2 and so should we check? > > Value of device type is something driver has to worry about, not its users, > they have been provided with two flags, one for NAND & other for NOR. Yes, but the driver does not seem to worry about it. In other words, there is no error checking. Ok so not a big deal a comment should suffice. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
Hi Jon, On Tue, Jun 12, 2012 at 23:39:32, Hunter, Jon wrote: > On 06/12/2012 07:58 AM, Mohammed, Afzal wrote: > > Thinking again over it, I am feeling above is sufficient, reason same as > > said earlier, to keep code simple & currently this is sufficient to > > handle GPMC bit patterns for IPs presently available. What you are > > suggesting is to take care of the imaginary case when new GPMC IP happens > > with new device type or size, I think that should be handled when such a > > scenario happens. Probably, it is better here to add a comment to make > > intention clear. > > That is one possibility but I think that more important reasons are ... > > 1. Readability, at first it appears that we are always configuring the > CS for NAND. However, this is not the case. Maybe a comment here can > help as you mentioned. As far as the users of GPMC is considered there is no confusion. Eg. For device type, they have been provided with two macros, GPMC_CONFIG1_DEVICETYPE_NOR, GPMC_CONFIG1_DEVICETYPE_NAND So for NOR, user can feel satisfied by giving NOR flag, little does he know that driver doesn't do anything with the flag, but he still gets what he want NOR flag is defined as zero. Yes even if he doesn't specify any type, device type will be set as NOR, but then that is the default - the only other option > > 2. A bad setting in the configuration passed. Hopefully, people will > stick to the flags but we know that we expect the device type to be a 0 > or 2 and so should we check? Value of device type is something driver has to worry about, not its users, they have been provided with two flags, one for NAND & other for NOR. Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
Hi Jon, On Tue, Jun 12, 2012 at 23:36:17, Hunter, Jon wrote: > Well it is unclear what the code flow is for using this helper as this > change simply adds the helper. If someone other function is writing to > the CONFIG1 register before this function, then you may wish to > highlight the programming sequence in the changelog or at least describe > why this function performs a read-modify-write and not just a write. Logic followed here: CS configuration helper needs to worry only about bits that are relevant for CS configuration and don't alter any other bits/registers. Same is applicable for time setting helper. Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
On 06/12/2012 07:58 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Tue, Jun 12, 2012 at 14:10:08, Mohammed, Afzal wrote: > + l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND; + l |= conf & GPMC_CONFIG1_DEVICESIZE_16; >>> >>> I can see that it works to use the above definitions as masks because of >>> the possible values that can be programmed into these fields. However, >>> from a read-ability standpoint this is unclear and requires people to >>> review the documentation to understand what you are doing here. >>> Furthermore, if any new device-types or sizes were added in the future >>> this could lead to bugs. Hence, it would be better to define a mask for >>> these fields. >> >> I had thought about it initially, but then it was felt it will >> lead to a less simple code, that path was not taken, let me >> revisit this. > > Thinking again over it, I am feeling above is sufficient, reason same as > said earlier, to keep code simple & currently this is sufficient to > handle GPMC bit patterns for IPs presently available. What you are > suggesting is to take care of the imaginary case when new GPMC IP happens > with new device type or size, I think that should be handled when such a > scenario happens. Probably, it is better here to add a comment to make > intention clear. That is one possibility but I think that more important reasons are ... 1. Readability, at first it appears that we are always configuring the CS for NAND. However, this is not the case. Maybe a comment here can help as you mentioned. 2. A bad setting in the configuration passed. Hopefully, people will stick to the flags but we know that we expect the device type to be a 0 or 2 and so should we check? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
On 06/12/2012 03:40 AM, Mohammed, Afzal wrote:
> Hi Jon,
>
> On Tue, Jun 12, 2012 at 03:13:02, Hunter, Jon wrote:
>
>>> +static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
>>> +{
>>> + u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
>>
>> Why is it necessary to read the register first? I thought you wanted to
>> get away from relying on bootloader settings?
>
> This is not trying to depend on bootloader, it is to alter bits
> that are only meant for configuration. There are other bits in
> the same register configured as part of time setting.
Well it is unclear what the code flow is for using this helper as this
change simply adds the helper. If someone other function is writing to
the CONFIG1 register before this function, then you may wish to
highlight the programming sequence in the changelog or at least describe
why this function performs a read-modify-write and not just a write.
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
Hi Jon, On Tue, Jun 12, 2012 at 14:10:08, Mohammed, Afzal wrote: > > > + l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND; > > > + l |= conf & GPMC_CONFIG1_DEVICESIZE_16; > > > > I can see that it works to use the above definitions as masks because of > > the possible values that can be programmed into these fields. However, > > from a read-ability standpoint this is unclear and requires people to > > review the documentation to understand what you are doing here. > > Furthermore, if any new device-types or sizes were added in the future > > this could lead to bugs. Hence, it would be better to define a mask for > > these fields. > > I had thought about it initially, but then it was felt it will > lead to a less simple code, that path was not taken, let me > revisit this. Thinking again over it, I am feeling above is sufficient, reason same as said earlier, to keep code simple & currently this is sufficient to handle GPMC bit patterns for IPs presently available. What you are suggesting is to take care of the imaginary case when new GPMC IP happens with new device type or size, I think that should be handled when such a scenario happens. Probably, it is better here to add a comment to make intention clear. Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
Hi Jon,
On Tue, Jun 12, 2012 at 03:13:02, Hunter, Jon wrote:
> > +static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
> > +{
> > + u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
>
> Why is it necessary to read the register first? I thought you wanted to
> get away from relying on bootloader settings?
This is not trying to depend on bootloader, it is to alter bits
that are only meant for configuration. There are other bits in
the same register configured as part of time setting.
>
> > + l &= ~(GPMC_CONFIG1_MUXADDDATA |
> > + GPMC_CONFIG1_WRITETYPE_SYNC |
> > + GPMC_CONFIG1_WRITEMULTIPLE_SUPP |
> > + GPMC_CONFIG1_READTYPE_SYNC |
> > + GPMC_CONFIG1_READMULTIPLE_SUPP |
> > + GPMC_CONFIG1_WRAPBURST_SUPP |
> > + GPMC_CONFIG1_DEVICETYPE(~0) |
> > + GPMC_CONFIG1_DEVICESIZE(~0) |
> > + GPMC_CONFIG1_PAGE_LEN(~0));
> > +
> > + l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND;
> > + l |= conf & GPMC_CONFIG1_DEVICESIZE_16;
>
> I can see that it works to use the above definitions as masks because of
> the possible values that can be programmed into these fields. However,
> from a read-ability standpoint this is unclear and requires people to
> review the documentation to understand what you are doing here.
> Furthermore, if any new device-types or sizes were added in the future
> this could lead to bugs. Hence, it would be better to define a mask for
> these fields.
I had thought about it initially, but then it was felt it will
lead to a less simple code, that path was not taken, let me
revisit this.
Regards
Afzal
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 06/14] ARM: OMAP2+: gpmc: CS configuration helper
On 06/11/2012 09:26 AM, Afzal Mohammed wrote:
> Helper for configuring given CS based on flags.
>
> Signed-off-by: Afzal Mohammed
> ---
> arch/arm/mach-omap2/gpmc.c | 33
>
> arch/arm/plat-omap/include/plat/gpmc.h |5 +
> 2 files changed, 38 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 652b245..4e19010 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -927,6 +927,39 @@ static __devinit int gpmc_setup_cs_mem(struct
> gpmc_cs_data *cs,
> return 1;
> }
>
> +static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
> +{
> + u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
Why is it necessary to read the register first? I thought you wanted to
get away from relying on bootloader settings?
> + l &= ~(GPMC_CONFIG1_MUXADDDATA |
> + GPMC_CONFIG1_WRITETYPE_SYNC |
> + GPMC_CONFIG1_WRITEMULTIPLE_SUPP |
> + GPMC_CONFIG1_READTYPE_SYNC |
> + GPMC_CONFIG1_READMULTIPLE_SUPP |
> + GPMC_CONFIG1_WRAPBURST_SUPP |
> + GPMC_CONFIG1_DEVICETYPE(~0) |
> + GPMC_CONFIG1_DEVICESIZE(~0) |
> + GPMC_CONFIG1_PAGE_LEN(~0));
> +
> + l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND;
> + l |= conf & GPMC_CONFIG1_DEVICESIZE_16;
I can see that it works to use the above definitions as masks because of
the possible values that can be programmed into these fields. However,
from a read-ability standpoint this is unclear and requires people to
review the documentation to understand what you are doing here.
Furthermore, if any new device-types or sizes were added in the future
this could lead to bugs. Hence, it would be better to define a mask for
these fields.
Now you may say what happens if someone pass a bad device-type or
device-size that would equate to a reserved value being programmed. Well
if someone passes a bad value we don't know what they intended to
program and that raises the question should we be checking the conf
value of bad device types, size, and page lengths here?
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
