| Message ID | 20210411073950.24772-6-dariobin@libero.it |
|---|---|
| State | Accepted |
| Commit | 9fd8a430f33f1a412a43c12a8b83e19ed91a16fd |
| Delegated to: | Lokesh Vutla |
| Headers | show |
| Series | Add support for pinmux status command on beaglebone | expand |
On 11.04.21 09:39, Dario Binacchi wrote: > Use dev_read_addr_size to get size of the controller's register area. > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> > > --- > > (no changes since v3) > > Changes in v3: > - Added Pratyush Yadav review tag. > > Changes in v2: > - Check dev_read_addr_size return value. > > drivers/pinctrl/pinctrl-single.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > index cec00e289c..d5656de8e8 100644 > --- a/drivers/pinctrl/pinctrl-single.c > +++ b/drivers/pinctrl/pinctrl-single.c > @@ -182,17 +182,19 @@ static int single_set_state(struct udevice *dev, > static int single_of_to_plat(struct udevice *dev) > { > fdt_addr_t addr; > - u32 of_reg[2]; > - int res; > + fdt_size_t size; > struct single_pdata *pdata = dev_get_plat(dev); > > pdata->width = > dev_read_u32_default(dev, "pinctrl-single,register-width", 0); > > - res = dev_read_u32_array(dev, "reg", of_reg, 2); > - if (res) > - return res; > - pdata->offset = of_reg[1] - pdata->width / 8; > + addr = dev_read_addr_size(dev, "reg", &size); Looks to me as if this has to be addr = dev_read_addr_size_index(dev, 0, &size); because dev_read_addr_size() assumes address and size width of fdt_addr_t, ie. 2 cells, rather than what is actually configured in the DT. I don't understand the use cases of dev_read_addr_size() vs. dev_read_addr_size_index(), if the former should also respect what the parent node configured size-wise. However, this patch breaks e.g. k3-am65 DTs, namely pinctrl@4301c000 which uses only one cell for address and size (according to bus@42040000). Jan > + if (addr == FDT_ADDR_T_NONE) { > + dev_err(dev, "failed to get base register size\n"); > + return -EINVAL; > + } > + > + pdata->offset = size - pdata->width / BITS_PER_BYTE; > > addr = dev_read_addr(dev); > if (addr == FDT_ADDR_T_NONE) { >
Hi Jan, > Il 01/05/2021 14:29 Jan Kiszka <jan.kiszka@web.de> ha scritto: > > > On 11.04.21 09:39, Dario Binacchi wrote: > > Use dev_read_addr_size to get size of the controller's register area. > > > > Signed-off-by: Dario Binacchi <dariobin@libero.it> > > Reviewed-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > > > (no changes since v3) > > > > Changes in v3: > > - Added Pratyush Yadav review tag. > > > > Changes in v2: > > - Check dev_read_addr_size return value. > > > > drivers/pinctrl/pinctrl-single.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c > > index cec00e289c..d5656de8e8 100644 > > --- a/drivers/pinctrl/pinctrl-single.c > > +++ b/drivers/pinctrl/pinctrl-single.c > > @@ -182,17 +182,19 @@ static int single_set_state(struct udevice *dev, > > static int single_of_to_plat(struct udevice *dev) > > { > > fdt_addr_t addr; > > - u32 of_reg[2]; > > - int res; > > + fdt_size_t size; > > struct single_pdata *pdata = dev_get_plat(dev); > > > > pdata->width = > > dev_read_u32_default(dev, "pinctrl-single,register-width", 0); > > > > - res = dev_read_u32_array(dev, "reg", of_reg, 2); > > - if (res) > > - return res; > > - pdata->offset = of_reg[1] - pdata->width / 8; > > + addr = dev_read_addr_size(dev, "reg", &size); > > Looks to me as if this has to be > > addr = dev_read_addr_size_index(dev, 0, &size); > > because dev_read_addr_size() assumes address and size width of > fdt_addr_t, ie. 2 cells, rather than what is actually configured in the DT. > > I don't understand the use cases of dev_read_addr_size() vs. > dev_read_addr_size_index(), if the former should also respect what the > parent node configured size-wise. However, this patch breaks e.g. > k3-am65 DTs, namely pinctrl@4301c000 which uses only one cell for > address and size (according to bus@42040000). If dev_read_addr_size() breaks k3-am65 DTs, the use of dev_read_addr_size_index() doesn't break am335x DTs. Sandbox and beaglebone tests are ok. IMHO you can submit the patch. Why not, add a test too. Thanks and regards, Dario > > Jan > > > + if (addr == FDT_ADDR_T_NONE) { > > + dev_err(dev, "failed to get base register size\n"); > > + return -EINVAL; > > + } > > + > > + pdata->offset = size - pdata->width / BITS_PER_BYTE; > > > > addr = dev_read_addr(dev); > > if (addr == FDT_ADDR_T_NONE) { > >
On 01.05.21 17:39, Dario Binacchi wrote: > Hi Jan, > >> Il 01/05/2021 14:29 Jan Kiszka <jan.kiszka@web.de> ha scritto: >> >> >> On 11.04.21 09:39, Dario Binacchi wrote: >>> Use dev_read_addr_size to get size of the controller's register area. >>> >>> Signed-off-by: Dario Binacchi <dariobin@libero.it> >>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com> >>> >>> --- >>> >>> (no changes since v3) >>> >>> Changes in v3: >>> - Added Pratyush Yadav review tag. >>> >>> Changes in v2: >>> - Check dev_read_addr_size return value. >>> >>> drivers/pinctrl/pinctrl-single.c | 14 ++++++++------ >>> 1 file changed, 8 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c >>> index cec00e289c..d5656de8e8 100644 >>> --- a/drivers/pinctrl/pinctrl-single.c >>> +++ b/drivers/pinctrl/pinctrl-single.c >>> @@ -182,17 +182,19 @@ static int single_set_state(struct udevice *dev, >>> static int single_of_to_plat(struct udevice *dev) >>> { >>> fdt_addr_t addr; >>> - u32 of_reg[2]; >>> - int res; >>> + fdt_size_t size; >>> struct single_pdata *pdata = dev_get_plat(dev); >>> >>> pdata->width = >>> dev_read_u32_default(dev, "pinctrl-single,register-width", 0); >>> >>> - res = dev_read_u32_array(dev, "reg", of_reg, 2); >>> - if (res) >>> - return res; >>> - pdata->offset = of_reg[1] - pdata->width / 8; >>> + addr = dev_read_addr_size(dev, "reg", &size); >> >> Looks to me as if this has to be >> >> addr = dev_read_addr_size_index(dev, 0, &size); >> >> because dev_read_addr_size() assumes address and size width of >> fdt_addr_t, ie. 2 cells, rather than what is actually configured in the DT. >> >> I don't understand the use cases of dev_read_addr_size() vs. >> dev_read_addr_size_index(), if the former should also respect what the >> parent node configured size-wise. However, this patch breaks e.g. >> k3-am65 DTs, namely pinctrl@4301c000 which uses only one cell for >> address and size (according to bus@42040000). > > If dev_read_addr_size() breaks k3-am65 DTs, the use of dev_read_addr_size_index() > doesn't break am335x DTs. Sandbox and beaglebone tests are ok. fdt_addr_t is 32 bits on the am335x, it's 64 on the am65xx while the address/size widths used in that branch is only 32. > IMHO you can submit the patch. Why not, add a test too. I will - I'm just not yet sure if I should patch the function above or rather ofnode_get_addr_size. Jan
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index cec00e289c..d5656de8e8 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -182,17 +182,19 @@ static int single_set_state(struct udevice *dev, static int single_of_to_plat(struct udevice *dev) { fdt_addr_t addr; - u32 of_reg[2]; - int res; + fdt_size_t size; struct single_pdata *pdata = dev_get_plat(dev); pdata->width = dev_read_u32_default(dev, "pinctrl-single,register-width", 0); - res = dev_read_u32_array(dev, "reg", of_reg, 2); - if (res) - return res; - pdata->offset = of_reg[1] - pdata->width / 8; + addr = dev_read_addr_size(dev, "reg", &size); + if (addr == FDT_ADDR_T_NONE) { + dev_err(dev, "failed to get base register size\n"); + return -EINVAL; + } + + pdata->offset = size - pdata->width / BITS_PER_BYTE; addr = dev_read_addr(dev); if (addr == FDT_ADDR_T_NONE) {