| Message ID | 1360736808-27209-2-git-send-email-gautam.vivek@samsung.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote: > With current FDT support driver tries to parse device node > twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't > happen ideally. > Making provision to store data in a global structure and thereby > passing its pointer when needed. > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> Nice patch. > --- > > This patch comes up as a fix for earlier problem of multiple FDT > decode. Actually no 'v1' present for this patch. > > drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- > 1 files changed, 11 insertions(+), 29 deletions(-) > > diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c > index 3ca4c5c..68f12fc 100644 > --- a/drivers/usb/host/ehci-exynos.c > +++ b/drivers/usb/host/ehci-exynos.c > @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; > */ > struct exynos_ehci { > struct exynos_usb_phy *usb; > - unsigned int *hcd; > + unsigned int hcd; I think this should be a pointer. Perhaps a (struct ehci_hccr *? > }; > > +static struct exynos_ehci exynos; > + > static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) > { > unsigned int node; > @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) > /* > * Get the base address for EHCI controller from the device node > */ > - exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg"); > - if (exynos->hcd == NULL) { > + exynos->hcd = fdtdec_get_addr(blob, node, "reg"); > + if (exynos->hcd < 0) { You need to check for FDT_ADDR_NONE here. > debug("Can't get the EHCI register address\n"); > return -ENXIO; > } > @@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) > */ > int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) > { > - struct exynos_ehci *exynos = NULL; > - > - exynos = (struct exynos_ehci *) > - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); > - if (!exynos) { > - debug("failed to allocate exynos ehci context\n"); > - return -ENOMEM; > - } > + struct exynos_ehci *ctx = &exynos; > > - exynos_usb_parse_dt(gd->fdt_blob, exynos); > + exynos_usb_parse_dt(gd->fdt_blob, ctx); > > - setup_usb_phy(exynos->usb); > + setup_usb_phy(ctx->usb); > > - *hccr = (struct ehci_hccr *)(exynos->hcd); > + *hccr = (struct ehci_hccr *)(ctx->hcd); > *hcor = (struct ehci_hcor *)((uint32_t) *hccr > + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); > > @@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) > (uint32_t)*hccr, (uint32_t)*hcor, > (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); > > - kfree(exynos); > - > return 0; > } > > @@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) > */ > int ehci_hcd_stop(int index) > { > - struct exynos_ehci *exynos = NULL; > - > - exynos = (struct exynos_ehci *) > - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); > - if (!exynos) { > - debug("failed to allocate exynos ehci context\n"); > - return -ENOMEM; > - } > - > - exynos_usb_parse_dt(gd->fdt_blob, exynos); > - > - reset_usb_phy(exynos->usb); > + struct exynos_ehci *ctx = &exynos; > > - kfree(exynos); > + reset_usb_phy(ctx->usb); > > return 0; > } > -- > 1.7.6.5 > Regards, Simon
Hi, On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote: >> With current FDT support driver tries to parse device node >> twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't >> happen ideally. >> Making provision to store data in a global structure and thereby >> passing its pointer when needed. >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > > Nice patch. > Really sorry for the delay in responding. >> --- >> >> This patch comes up as a fix for earlier problem of multiple FDT >> decode. Actually no 'v1' present for this patch. >> >> drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- >> 1 files changed, 11 insertions(+), 29 deletions(-) >> >> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c >> index 3ca4c5c..68f12fc 100644 >> --- a/drivers/usb/host/ehci-exynos.c >> +++ b/drivers/usb/host/ehci-exynos.c >> @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; >> */ >> struct exynos_ehci { >> struct exynos_usb_phy *usb; >> - unsigned int *hcd; >> + unsigned int hcd; > > I think this should be a pointer. Perhaps a (struct ehci_hccr *? > Isn't it better to maintain it as unsigned int ? Since later when fdtdec_get_addr() is used, which returns u32 or u64, we can easily check against FDT_ADDR_T_NONE. ehci_hcd_init() can later typecast it to (struct ehci_hccr *) when needed. >> }; >> >> +static struct exynos_ehci exynos; >> + >> static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) >> { >> unsigned int node; >> @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) >> /* >> * Get the base address for EHCI controller from the device node >> */ >> - exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg"); >> - if (exynos->hcd == NULL) { >> + exynos->hcd = fdtdec_get_addr(blob, node, "reg"); >> + if (exynos->hcd < 0) { > > You need to check for FDT_ADDR_NONE here. > Sure. >> debug("Can't get the EHCI register address\n"); >> return -ENXIO; >> } >> @@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) >> */ >> int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >> { >> - struct exynos_ehci *exynos = NULL; >> - >> - exynos = (struct exynos_ehci *) >> - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); >> - if (!exynos) { >> - debug("failed to allocate exynos ehci context\n"); >> - return -ENOMEM; >> - } >> + struct exynos_ehci *ctx = &exynos; >> >> - exynos_usb_parse_dt(gd->fdt_blob, exynos); >> + exynos_usb_parse_dt(gd->fdt_blob, ctx); >> >> - setup_usb_phy(exynos->usb); >> + setup_usb_phy(ctx->usb); >> >> - *hccr = (struct ehci_hccr *)(exynos->hcd); >> + *hccr = (struct ehci_hccr *)(ctx->hcd); >> *hcor = (struct ehci_hcor *)((uint32_t) *hccr >> + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >> >> @@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >> (uint32_t)*hccr, (uint32_t)*hcor, >> (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >> >> - kfree(exynos); >> - >> return 0; >> } >> >> @@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >> */ >> int ehci_hcd_stop(int index) >> { >> - struct exynos_ehci *exynos = NULL; >> - >> - exynos = (struct exynos_ehci *) >> - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); >> - if (!exynos) { >> - debug("failed to allocate exynos ehci context\n"); >> - return -ENOMEM; >> - } >> - >> - exynos_usb_parse_dt(gd->fdt_blob, exynos); >> - >> - reset_usb_phy(exynos->usb); >> + struct exynos_ehci *ctx = &exynos; >> >> - kfree(exynos); >> + reset_usb_phy(ctx->usb); >> >> return 0; >> } >> -- >> 1.7.6.5 >> >
On Tue, Mar 5, 2013 at 5:40 AM, Vivek Gautam <gautamvivek1987@gmail.com> wrote: > Hi, > > > On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi, >> >> On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote: >>> With current FDT support driver tries to parse device node >>> twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't >>> happen ideally. >>> Making provision to store data in a global structure and thereby >>> passing its pointer when needed. >>> >>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> >> >> Nice patch. >> > Really sorry for the delay in responding. No problem, we all have other things to do :-) > >>> --- >>> >>> This patch comes up as a fix for earlier problem of multiple FDT >>> decode. Actually no 'v1' present for this patch. >>> >>> drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- >>> 1 files changed, 11 insertions(+), 29 deletions(-) >>> >>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c >>> index 3ca4c5c..68f12fc 100644 >>> --- a/drivers/usb/host/ehci-exynos.c >>> +++ b/drivers/usb/host/ehci-exynos.c >>> @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; >>> */ >>> struct exynos_ehci { >>> struct exynos_usb_phy *usb; >>> - unsigned int *hcd; >>> + unsigned int hcd; >> >> I think this should be a pointer. Perhaps a (struct ehci_hccr *? >> > > Isn't it better to maintain it as unsigned int ? > Since later when fdtdec_get_addr() is used, which returns u32 or u64, > we can easily check against FDT_ADDR_T_NONE. > ehci_hcd_init() can later typecast it to (struct ehci_hccr *) when needed. How about having a local variable in your decode routine like: fdt_addr_t addr; addr = fdtdec_get_addr()... if (addr == FDT_ADDR_NONE) { debug(...) return -1; } exynos->hcd = (struct ... *)addr; I think this is better than holding a value that is the wrong type, and casting it throughout your driver. Best to get the casting/checking done at init time, > >>> }; >>> >>> +static struct exynos_ehci exynos; >>> + >>> static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) >>> { >>> unsigned int node; >>> @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) >>> /* >>> * Get the base address for EHCI controller from the device node >>> */ >>> - exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg"); >>> - if (exynos->hcd == NULL) { >>> + exynos->hcd = fdtdec_get_addr(blob, node, "reg"); >>> + if (exynos->hcd < 0) { >> >> You need to check for FDT_ADDR_NONE here. >> > Sure. > >>> debug("Can't get the EHCI register address\n"); >>> return -ENXIO; >>> } >>> @@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) >>> */ >>> int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >>> { >>> - struct exynos_ehci *exynos = NULL; >>> - >>> - exynos = (struct exynos_ehci *) >>> - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); >>> - if (!exynos) { >>> - debug("failed to allocate exynos ehci context\n"); >>> - return -ENOMEM; >>> - } >>> + struct exynos_ehci *ctx = &exynos; >>> >>> - exynos_usb_parse_dt(gd->fdt_blob, exynos); >>> + exynos_usb_parse_dt(gd->fdt_blob, ctx); >>> >>> - setup_usb_phy(exynos->usb); >>> + setup_usb_phy(ctx->usb); >>> >>> - *hccr = (struct ehci_hccr *)(exynos->hcd); >>> + *hccr = (struct ehci_hccr *)(ctx->hcd); >>> *hcor = (struct ehci_hcor *)((uint32_t) *hccr >>> + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >>> >>> @@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >>> (uint32_t)*hccr, (uint32_t)*hcor, >>> (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >>> >>> - kfree(exynos); >>> - >>> return 0; >>> } >>> >>> @@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >>> */ >>> int ehci_hcd_stop(int index) >>> { >>> - struct exynos_ehci *exynos = NULL; >>> - >>> - exynos = (struct exynos_ehci *) >>> - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); >>> - if (!exynos) { >>> - debug("failed to allocate exynos ehci context\n"); >>> - return -ENOMEM; >>> - } >>> - >>> - exynos_usb_parse_dt(gd->fdt_blob, exynos); >>> - >>> - reset_usb_phy(exynos->usb); >>> + struct exynos_ehci *ctx = &exynos; >>> >>> - kfree(exynos); >>> + reset_usb_phy(ctx->usb); >>> >>> return 0; >>> } >>> -- >>> 1.7.6.5 >>> >> > > -- > Thanks & Regards > Vivek Regards, Simon
Hi, On Wed, Mar 6, 2013 at 7:27 AM, Simon Glass <sjg@chromium.org> wrote: > On Tue, Mar 5, 2013 at 5:40 AM, Vivek Gautam <gautamvivek1987@gmail.com> wrote: >> Hi, >> >> >> On Thu, Feb 14, 2013 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote: >>> Hi, >>> >>> On Tue, Feb 12, 2013 at 10:26 PM, Vivek Gautam <gautam.vivek@samsung.com> wrote: >>>> With current FDT support driver tries to parse device node >>>> twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't >>>> happen ideally. >>>> Making provision to store data in a global structure and thereby >>>> passing its pointer when needed. >>>> >>>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> >>> >>> Nice patch. >>> >> Really sorry for the delay in responding. > > No problem, we all have other things to do :-) > >> >>>> --- >>>> >>>> This patch comes up as a fix for earlier problem of multiple FDT >>>> decode. Actually no 'v1' present for this patch. >>>> >>>> drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- >>>> 1 files changed, 11 insertions(+), 29 deletions(-) >>>> >>>> diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c >>>> index 3ca4c5c..68f12fc 100644 >>>> --- a/drivers/usb/host/ehci-exynos.c >>>> +++ b/drivers/usb/host/ehci-exynos.c >>>> @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; >>>> */ >>>> struct exynos_ehci { >>>> struct exynos_usb_phy *usb; >>>> - unsigned int *hcd; >>>> + unsigned int hcd; >>> >>> I think this should be a pointer. Perhaps a (struct ehci_hccr *? >>> >> >> Isn't it better to maintain it as unsigned int ? >> Since later when fdtdec_get_addr() is used, which returns u32 or u64, >> we can easily check against FDT_ADDR_T_NONE. >> ehci_hcd_init() can later typecast it to (struct ehci_hccr *) when needed. > > How about having a local variable in your decode routine like: > > fdt_addr_t addr; > Yeah, seem a nice idea. > addr = fdtdec_get_addr()... > if (addr == FDT_ADDR_NONE) { > debug(...) > return -1; > } > exynos->hcd = (struct ... *)addr; > > I think this is better than holding a value that is the wrong type, > and casting it throughout your driver. Best to get the > casting/checking done at init time, > True, will modify this and post it. >> >>>> }; >>>> >>>> +static struct exynos_ehci exynos; >>>> + >>>> static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) >>>> { >>>> unsigned int node; >>>> @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) >>>> /* >>>> * Get the base address for EHCI controller from the device node >>>> */ >>>> - exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg"); >>>> - if (exynos->hcd == NULL) { >>>> + exynos->hcd = fdtdec_get_addr(blob, node, "reg"); >>>> + if (exynos->hcd < 0) { >>> >>> You need to check for FDT_ADDR_NONE here. >>> >> Sure. >> >>>> debug("Can't get the EHCI register address\n"); >>>> return -ENXIO; >>>> } >>>> @@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) >>>> */ >>>> int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >>>> { >>>> - struct exynos_ehci *exynos = NULL; >>>> - >>>> - exynos = (struct exynos_ehci *) >>>> - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); >>>> - if (!exynos) { >>>> - debug("failed to allocate exynos ehci context\n"); >>>> - return -ENOMEM; >>>> - } >>>> + struct exynos_ehci *ctx = &exynos; >>>> >>>> - exynos_usb_parse_dt(gd->fdt_blob, exynos); >>>> + exynos_usb_parse_dt(gd->fdt_blob, ctx); >>>> >>>> - setup_usb_phy(exynos->usb); >>>> + setup_usb_phy(ctx->usb); >>>> >>>> - *hccr = (struct ehci_hccr *)(exynos->hcd); >>>> + *hccr = (struct ehci_hccr *)(ctx->hcd); >>>> *hcor = (struct ehci_hcor *)((uint32_t) *hccr >>>> + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >>>> >>>> @@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >>>> (uint32_t)*hccr, (uint32_t)*hcor, >>>> (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >>>> >>>> - kfree(exynos); >>>> - >>>> return 0; >>>> } >>>> >>>> @@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) >>>> */ >>>> int ehci_hcd_stop(int index) >>>> { >>>> - struct exynos_ehci *exynos = NULL; >>>> - >>>> - exynos = (struct exynos_ehci *) >>>> - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); >>>> - if (!exynos) { >>>> - debug("failed to allocate exynos ehci context\n"); >>>> - return -ENOMEM; >>>> - } >>>> - >>>> - exynos_usb_parse_dt(gd->fdt_blob, exynos); >>>> - >>>> - reset_usb_phy(exynos->usb); >>>> + struct exynos_ehci *ctx = &exynos; >>>> >>>> - kfree(exynos); >>>> + reset_usb_phy(ctx->usb); >>>> >>>> return 0; >>>> } >>>> -- >>>> 1.7.6.5 >>>> >>> >> >> --
diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c index 3ca4c5c..68f12fc 100644 --- a/drivers/usb/host/ehci-exynos.c +++ b/drivers/usb/host/ehci-exynos.c @@ -42,9 +42,11 @@ DECLARE_GLOBAL_DATA_PTR; */ struct exynos_ehci { struct exynos_usb_phy *usb; - unsigned int *hcd; + unsigned int hcd; }; +static struct exynos_ehci exynos; + static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) { unsigned int node; @@ -59,8 +61,8 @@ static int exynos_usb_parse_dt(const void *blob, struct exynos_ehci *exynos) /* * Get the base address for EHCI controller from the device node */ - exynos->hcd = (unsigned int *)fdtdec_get_addr(blob, node, "reg"); - if (exynos->hcd == NULL) { + exynos->hcd = fdtdec_get_addr(blob, node, "reg"); + if (exynos->hcd < 0) { debug("Can't get the EHCI register address\n"); return -ENXIO; } @@ -144,20 +146,13 @@ static void reset_usb_phy(struct exynos_usb_phy *usb) */ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) { - struct exynos_ehci *exynos = NULL; - - exynos = (struct exynos_ehci *) - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); - if (!exynos) { - debug("failed to allocate exynos ehci context\n"); - return -ENOMEM; - } + struct exynos_ehci *ctx = &exynos; - exynos_usb_parse_dt(gd->fdt_blob, exynos); + exynos_usb_parse_dt(gd->fdt_blob, ctx); - setup_usb_phy(exynos->usb); + setup_usb_phy(ctx->usb); - *hccr = (struct ehci_hccr *)(exynos->hcd); + *hccr = (struct ehci_hccr *)(ctx->hcd); *hcor = (struct ehci_hcor *)((uint32_t) *hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); @@ -165,8 +160,6 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) (uint32_t)*hccr, (uint32_t)*hcor, (uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); - kfree(exynos); - return 0; } @@ -176,20 +169,9 @@ int ehci_hcd_init(int index, struct ehci_hccr **hccr, struct ehci_hcor **hcor) */ int ehci_hcd_stop(int index) { - struct exynos_ehci *exynos = NULL; - - exynos = (struct exynos_ehci *) - kzalloc(sizeof(struct exynos_ehci), GFP_KERNEL); - if (!exynos) { - debug("failed to allocate exynos ehci context\n"); - return -ENOMEM; - } - - exynos_usb_parse_dt(gd->fdt_blob, exynos); - - reset_usb_phy(exynos->usb); + struct exynos_ehci *ctx = &exynos; - kfree(exynos); + reset_usb_phy(ctx->usb); return 0; }
With current FDT support driver tries to parse device node twice in ehci_hcd_init() and ehci_hcd_stop(), which shouldn't happen ideally. Making provision to store data in a global structure and thereby passing its pointer when needed. Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> --- This patch comes up as a fix for earlier problem of multiple FDT decode. Actually no 'v1' present for this patch. drivers/usb/host/ehci-exynos.c | 40 +++++++++++----------------------------- 1 files changed, 11 insertions(+), 29 deletions(-)