| Message ID | 20180223064138.18401-1-jh80.chung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: > 'clock-freq-min-max' property had already deprecated. > Remove the 'clock-freq-min-max' property that is kept to maintain > the compatibility. Removing a property without telling the user what to expect is a bad idea and ABI breakage.
Hi Andy, On 2018/2/23 21:27, Andy Shevchenko wrote: > On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> 'clock-freq-min-max' property had already deprecated. >> Remove the 'clock-freq-min-max' property that is kept to maintain >> the compatibility. > > Removing a property without telling the user what to expect is a bad > idea and ABI breakage. > What's the general process to remove a property? I guess we should do: 1) deprecate it in the first place and remove it from all upstream DT 2) wait some long enough days for expecting the stale of all old DTB containing that property 3) remove the functionality of the deprecated property from the driver but still leave some warning there 4) remove the left warning finally And for the ABI breakage, we should add something in Documentation/ABI /obsolete or Documentation/ABI/removed ?
On Fri, Feb 23, 2018 at 4:19 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > On 2018/2/23 21:27, Andy Shevchenko wrote: >> On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung <jh80.chung@samsung.com> >> wrote: >>> >>> 'clock-freq-min-max' property had already deprecated. >>> Remove the 'clock-freq-min-max' property that is kept to maintain >>> the compatibility. >> >> >> Removing a property without telling the user what to expect is a bad >> idea and ABI breakage. >> > > What's the general process to remove a property? > > I guess we should do: > 1) deprecate it in the first place and remove it from all upstream DT > 2) wait some long enough days for expecting the stale of all old DTB > containing that property > 3) remove the functionality of the deprecated property from the driver > but still leave some warning there > 4) remove the left warning finally I don't know. Perhaps Rob can shed a light here. But I would really OK with removal of some of such properties from some drivers where it's more burden to keep them. > And for the ABI breakage, we should add something in Documentation/ABI > /obsolete or Documentation/ABI/removed ?
On 02/24/2018 01:16 AM, Andy Shevchenko wrote: > On Fri, Feb 23, 2018 at 4:19 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: >> On 2018/2/23 21:27, Andy Shevchenko wrote: >>> On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung <jh80.chung@samsung.com> >>> wrote: >>>> >>>> 'clock-freq-min-max' property had already deprecated. >>>> Remove the 'clock-freq-min-max' property that is kept to maintain >>>> the compatibility. >>> >>> >>> Removing a property without telling the user what to expect is a bad >>> idea and ABI breakage. >>> >> >> What's the general process to remove a property? >> >> I guess we should do: >> 1) deprecate it in the first place and remove it from all upstream DT >> 2) wait some long enough days for expecting the stale of all old DTB >> containing that property >> 3) remove the functionality of the deprecated property from the driver >> but still leave some warning there >> 4) remove the left warning finally > > I don't know. Perhaps Rob can shed a light here. > But I would really OK with removal of some of such properties from > some drivers where it's more burden to keep them. This property had deprecated about 8months ago. I think that it was enough to keep this property for maintaining the compatibility. I didn't remove this property without any alternative. Best Regards, Jaehoon Chung > >> And for the ABI breakage, we should add something in Documentation/ABI >> /obsolete or Documentation/ABI/removed ? > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 06:16:39PM +0200, Andy Shevchenko wrote: > On Fri, Feb 23, 2018 at 4:19 PM, Shawn Lin <shawn.lin@rock-chips.com> wrote: > > On 2018/2/23 21:27, Andy Shevchenko wrote: > >> On Fri, Feb 23, 2018 at 8:41 AM, Jaehoon Chung <jh80.chung@samsung.com> > >> wrote: > >>> > >>> 'clock-freq-min-max' property had already deprecated. > >>> Remove the 'clock-freq-min-max' property that is kept to maintain > >>> the compatibility. > >> > >> > >> Removing a property without telling the user what to expect is a bad > >> idea and ABI breakage. > >> > > > > What's the general process to remove a property? > > > > I guess we should do: > > 1) deprecate it in the first place and remove it from all upstream DT Yes > > 2) wait some long enough days for expecting the stale of all old DTB > > containing that property Yes. How long that is depends on the platform. I think the minimum is 1 release cycle. Some stable platforms are years. If there are other DT changes with new features everyone should want/need, then that can be a decision point. Given this is a shared IP block it's harder to know, so you may need to err on the longer side. > > 3) remove the functionality of the deprecated property from the driver > > but still leave some warning there I'd say add a warning in step 1 and combine 3 and 4. > > 4) remove the left warning finally > > I don't know. Perhaps Rob can shed a light here. > But I would really OK with removal of some of such properties from > some drivers where it's more burden to keep them. > > > And for the ABI breakage, we should add something in Documentation/ABI > > /obsolete or Documentation/ABI/removed ? It is only an ABI break if someone notices. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 23, 2018 at 03:41:33PM +0900, Jaehoon Chung wrote: > 'clock-freq-min-max' property had already deprecated. > Remove the 'clock-freq-min-max' property that is kept to maintain > the compatibility. > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > --- > .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ---- > drivers/mmc/host/dw_mmc.c | 15 ++++----------- > 2 files changed, 4 insertions(+), 15 deletions(-) In no way an endorsement that this is the right time to remove, Reviewed-by: Rob Herring <robh@kernel.org> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23 February 2018 at 07:41, Jaehoon Chung <jh80.chung@samsung.com> wrote: > 'clock-freq-min-max' property had already deprecated. > Remove the 'clock-freq-min-max' property that is kept to maintain > the compatibility. > > Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> Thanks, applied for next! Kind regards Uffe > --- > .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ---- > drivers/mmc/host/dw_mmc.c | 15 ++++----------- > 2 files changed, 4 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > index ef3e5f14067a..75c9fdca4aaf 100644 > --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt > @@ -59,10 +59,6 @@ Optional properties: > is specified and the ciu clock is specified then we'll try to set the ciu > clock to this at probe time. > > -* clock-freq-min-max (DEPRECATED): Minimum and Maximum clock frequency for card output > - clock(cclk_out). If it's not specified, max is 200MHZ and min is 400KHz by default. > - (Use the "max-frequency" instead of "clock-freq-min-max".) > - > * num-slots (DEPRECATED): specifies the number of slots supported by the controller. > The number of physical slots actually used could be equal or less than the > value specified by num-slots. If this property is not specified, the value > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 0aa39975f33b..38e0e7c4ffd9 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -2784,7 +2784,6 @@ static int dw_mci_init_slot(struct dw_mci *host) > struct dw_mci_slot *slot; > const struct dw_mci_drv_data *drv_data = host->drv_data; > int ctrl_id, ret; > - u32 freq[2]; > > mmc = mmc_alloc_host(sizeof(struct dw_mci_slot), host->dev); > if (!mmc) > @@ -2798,16 +2797,6 @@ static int dw_mci_init_slot(struct dw_mci *host) > host->slot = slot; > > mmc->ops = &dw_mci_ops; > - if (device_property_read_u32_array(host->dev, "clock-freq-min-max", > - freq, 2)) { > - mmc->f_min = DW_MCI_FREQ_MIN; > - mmc->f_max = DW_MCI_FREQ_MAX; > - } else { > - dev_info(host->dev, > - "'clock-freq-min-max' property was deprecated.\n"); > - mmc->f_min = freq[0]; > - mmc->f_max = freq[1]; > - } > > /*if there are external regulators, get them*/ > ret = mmc_regulator_get_supply(mmc); > @@ -2846,6 +2835,10 @@ static int dw_mci_init_slot(struct dw_mci *host) > if (ret) > goto err_host_allocated; > > + mmc->f_min = DW_MCI_FREQ_MIN; > + if (!mmc->f_max) > + mmc->f_max = DW_MCI_FREQ_MAX; > + > /* Process SDIO IRQs through the sdio_irq_work. */ > if (mmc->caps & MMC_CAP_SDIO_IRQ) > mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD; > -- > 2.15.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/15/2018 07:22 PM, Ulf Hansson wrote: > On 23 February 2018 at 07:41, Jaehoon Chung <jh80.chung@samsung.com> wrote: >> 'clock-freq-min-max' property had already deprecated. >> Remove the 'clock-freq-min-max' property that is kept to maintain >> the compatibility. >> >> Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> > > Thanks, applied for next! Sorry. Thanks for applying this. :) Best Regards, Jaehoon Chung > > Kind regards > Uffe > >> --- >> .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ---- >> drivers/mmc/host/dw_mmc.c | 15 ++++----------- >> 2 files changed, 4 insertions(+), 15 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt >> index ef3e5f14067a..75c9fdca4aaf 100644 >> --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt >> +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt >> @@ -59,10 +59,6 @@ Optional properties: >> is specified and the ciu clock is specified then we'll try to set the ciu >> clock to this at probe time. >> >> -* clock-freq-min-max (DEPRECATED): Minimum and Maximum clock frequency for card output >> - clock(cclk_out). If it's not specified, max is 200MHZ and min is 400KHz by default. >> - (Use the "max-frequency" instead of "clock-freq-min-max".) >> - >> * num-slots (DEPRECATED): specifies the number of slots supported by the controller. >> The number of physical slots actually used could be equal or less than the >> value specified by num-slots. If this property is not specified, the value >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 0aa39975f33b..38e0e7c4ffd9 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -2784,7 +2784,6 @@ static int dw_mci_init_slot(struct dw_mci *host) >> struct dw_mci_slot *slot; >> const struct dw_mci_drv_data *drv_data = host->drv_data; >> int ctrl_id, ret; >> - u32 freq[2]; >> >> mmc = mmc_alloc_host(sizeof(struct dw_mci_slot), host->dev); >> if (!mmc) >> @@ -2798,16 +2797,6 @@ static int dw_mci_init_slot(struct dw_mci *host) >> host->slot = slot; >> >> mmc->ops = &dw_mci_ops; >> - if (device_property_read_u32_array(host->dev, "clock-freq-min-max", >> - freq, 2)) { >> - mmc->f_min = DW_MCI_FREQ_MIN; >> - mmc->f_max = DW_MCI_FREQ_MAX; >> - } else { >> - dev_info(host->dev, >> - "'clock-freq-min-max' property was deprecated.\n"); >> - mmc->f_min = freq[0]; >> - mmc->f_max = freq[1]; >> - } >> >> /*if there are external regulators, get them*/ >> ret = mmc_regulator_get_supply(mmc); >> @@ -2846,6 +2835,10 @@ static int dw_mci_init_slot(struct dw_mci *host) >> if (ret) >> goto err_host_allocated; >> >> + mmc->f_min = DW_MCI_FREQ_MIN; >> + if (!mmc->f_max) >> + mmc->f_max = DW_MCI_FREQ_MAX; >> + >> /* Process SDIO IRQs through the sdio_irq_work. */ >> if (mmc->caps & MMC_CAP_SDIO_IRQ) >> mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD; >> -- >> 2.15.1 >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt index ef3e5f14067a..75c9fdca4aaf 100644 --- a/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt +++ b/Documentation/devicetree/bindings/mmc/synopsys-dw-mshc.txt @@ -59,10 +59,6 @@ Optional properties: is specified and the ciu clock is specified then we'll try to set the ciu clock to this at probe time. -* clock-freq-min-max (DEPRECATED): Minimum and Maximum clock frequency for card output - clock(cclk_out). If it's not specified, max is 200MHZ and min is 400KHz by default. - (Use the "max-frequency" instead of "clock-freq-min-max".) - * num-slots (DEPRECATED): specifies the number of slots supported by the controller. The number of physical slots actually used could be equal or less than the value specified by num-slots. If this property is not specified, the value diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 0aa39975f33b..38e0e7c4ffd9 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -2784,7 +2784,6 @@ static int dw_mci_init_slot(struct dw_mci *host) struct dw_mci_slot *slot; const struct dw_mci_drv_data *drv_data = host->drv_data; int ctrl_id, ret; - u32 freq[2]; mmc = mmc_alloc_host(sizeof(struct dw_mci_slot), host->dev); if (!mmc) @@ -2798,16 +2797,6 @@ static int dw_mci_init_slot(struct dw_mci *host) host->slot = slot; mmc->ops = &dw_mci_ops; - if (device_property_read_u32_array(host->dev, "clock-freq-min-max", - freq, 2)) { - mmc->f_min = DW_MCI_FREQ_MIN; - mmc->f_max = DW_MCI_FREQ_MAX; - } else { - dev_info(host->dev, - "'clock-freq-min-max' property was deprecated.\n"); - mmc->f_min = freq[0]; - mmc->f_max = freq[1]; - } /*if there are external regulators, get them*/ ret = mmc_regulator_get_supply(mmc); @@ -2846,6 +2835,10 @@ static int dw_mci_init_slot(struct dw_mci *host) if (ret) goto err_host_allocated; + mmc->f_min = DW_MCI_FREQ_MIN; + if (!mmc->f_max) + mmc->f_max = DW_MCI_FREQ_MAX; + /* Process SDIO IRQs through the sdio_irq_work. */ if (mmc->caps & MMC_CAP_SDIO_IRQ) mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
'clock-freq-min-max' property had already deprecated. Remove the 'clock-freq-min-max' property that is kept to maintain the compatibility. Signed-off-by: Jaehoon Chung <jh80.chung@samsung.com> --- .../devicetree/bindings/mmc/synopsys-dw-mshc.txt | 4 ---- drivers/mmc/host/dw_mmc.c | 15 ++++----------- 2 files changed, 4 insertions(+), 15 deletions(-)