Hi Afzal,
On 3/26/2012 3:04, Mohammed, Afzal wrote:
Hi Jon,
On Sat, Mar 24, 2012 at 04:51:13, Hunter, Jon wrote:
+struct gpmc_child {
+ char *name;
+ int id;
+ struct resource *res;
+ unsigned num_res;
+ struct resource gpmc_res[GPMC_CS_NUM];
Does this imply a gpmc child device can use more than one chip-select? I
am trying to understand the link between number of resources and
GPMC_CS_NUM.
Yes, relevant portion in commit message as follows,
A peripheral connected to GPMC can have multiple
address spaces using different chip select. Hence
GPMC driver has been provided capability to
distinguish this scenario, i.e. create platform
devices only once for each connected peripheral,
and not for each configured chip select. The
peripheral that made it necessary was tusb6010.
Ok, makes sense. I believe that most devices are using a single CS and
less common for devices to use more than one. Therefore, I was not sure
if it made sense to allocate the gpmc_res struct dynamically as I doubt
you will ever have a device using all 8 chip-selects ;-)
Also, I don't see where the gpmc_child->res and gpmc_child->num_res are
actually used. Are these needed?
[snip]
Do we need to free irqs here?
Irqs has been conveniently forgotten in this patch, in mainline, I could
not find any platforms using GPMC irq. This can be added later once
driver conversion is done, if required.
I just meant that if we allocate them during the probe maybe we should
remove when exiting.
[snip]
+ /* GPMC specific */
+ unsigned cs;
+ unsigned long mem_size;
+ unsigned long mem_start;
+ unsigned long mem_offset;
+ struct gpmc_config *config;
+ unsigned num_config;
+ struct gpmc_timings *timing;
+};
+
+struct gpmc_pdata {
+ /* GPMC_FCLK rate in picoseconds */
+ unsigned long fclk_rate;
fclk_period
+ struct gpmc_device_pdata *device_pdata;
+ unsigned num_device;
+};
Do you need both gpmc_pdata and gpmc_device_pdata? Would not a single
structure work?
Gpmc_device_data is dedicated to each CS, gpmc_pdata is required
at least to inform driver about clock rate.
Ok, understood! So the struct gpmc_device_pdata only has a single
chip-select entry and so looking at the code you will have multiple
instances of this structure of a gpmc device that uses more than one
chip-select. Any reason you did it this way and not have a single pdata
struct for each device defining all chip-selects it uses?
Generally, as the change involved moving a lot of code, seems more reviews
are on those than the actual changes than what I intended to get reviewed,
next patch series will be modified not to move existing code, hence some
of your suggested changes may not be present in it, probably those to be
done as another cleanup patch.
Yes I understand. However, it is a good opportunity to clean some of
this up even if it is existing code :-)
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
